Add support for multiple pixel formats in VideoFrameBuffer

webrtc/api/video/video_frame_buffer.h

Index: webrtc/api/video/video_frame_buffer.h
diff --git a/webrtc/api/video/video_frame_buffer.h b/webrtc/api/video/video_frame_buffer.h
index c8c2e5d5d4087c5bcf6bb1ec4d24ea9df30da88e..e6998c647aa391f907a976d2778a1d6250e87d14 100644
--- a/webrtc/api/video/video_frame_buffer.h
+++ b/webrtc/api/video/video_frame_buffer.h
@@ -18,38 +18,96 @@
 
 namespace webrtc {
 
-// Interface of a simple frame buffer containing pixel data. This interface does
-// not contain any frame metadata such as rotation, timestamp, pixel_width, etc.
+class PlanarYuvBuffer;
+
+// Base class for frame buffers of different kinds of pixel format. The tag
+// in Format() indicates what pixel format the buffer contains, and each pixel
+// format is implemented as a subclass. To access the pixel data, call the
+// appropriate GetXXX() function, where XXX represents the pixel format
+// specified by Format(). There is also a function ToI420() that can always be
+// called that will convert to I420. This serves as a fallback for e.g. the

[nisse-webrtc, 2017/05/04 08:15:29]

Maybe add some guidelines on when it makes sense to add support for additional
formats at this level? As far as I understand, it's only useful to add formats
that can be handled efficiently by some codec.

Wording and punctuation could be improved a bit, e.g.,  ", e.g.,", not "for
e.g", and the sentence with the two "that".

[magjed_webrtc, 2017/05/04 12:25:13]

Done. It's a bit more tricky to describe when it's useful to add new formats.
For example, I444 is needed because our internal SW decoder can produce such
frames.

+// internal WebRTC software encoders that can only handle I420. A special enum
+// value 'kNative' is provided for external clients to implement their own frame
+// buffer representations, e.g. as textures. The external client is then
+// responsible for casting the native buffer into the correct subclass. Frame
+// metadata such as rotation and timestamp are stored in webrtc::VideoFrame, and
+// not here.
 class VideoFrameBuffer : public rtc::RefCountInterface {
  public:
+  enum class PixelFormat {

[nisse-webrtc, 2017/05/04 08:15:29]

Is "PixelFormat" the right name for this? Maybe just "Type" aka
"VideoFrameBuffer::Type" is better? The format doesn't really apply to
individual pixels, but to the higher-level organization of the buffer.

[magjed_webrtc, 2017/05/04 12:25:13]

Done, I also prefer just Type. I changed the Format() name to type(). Type()
does not work because of the naming conflict, but I think it's ok to use lower
case even if it's a virtual function (we also already do this for width/height).
The style guide says:
"Accessors and mutators (get and set functions) may be named like variables.
These often correspond to actual member variables, but this is not required. For
example, int count() and void set_count(int count)."

+    kNative,

[nisse-webrtc, 2017/05/04 08:15:29]

For kNative, if casting of the void* returned by native_handle() is replaced by
casting of the VideoFrameBuffer itself, that should eliminate a couple of
NativeHandle classes and make related code simpler, I hope? 

Are the NativeHandle and the corresponding NativeHandleBuffer always created
together in the current code? I seem to remember they where separated in the
Android jni code. 

[magjed_webrtc, 2017/05/04 12:25:13]

Yes, we will be able to simplify the code after this. The Android jni case won't
be a problem.

+    kI420,
+    kI444,
+  };
+
+  // This function specifies in what pixel format the data is stored in.
+  virtual PixelFormat Format() const;
+
   // The resolution of the frame in pixels. For formats where some planes are
   // subsampled, this is the highest-resolution plane.
   virtual int width() const = 0;
   virtual int height() const = 0;
 
+  // Returns a memory-backed frame buffer in I420 format. If the pixel data is
+  // in another format, a conversion will take place. All implementations must
+  // provide a fallback to I420 for compatibility with e.g the internal WebRTC
+  // software encoders.
+  virtual rtc::scoped_refptr<PlanarYuvBuffer> ToI420();
+
+  // These functions should only be called if Format() is of the correct type.

[nisse-webrtc, 2017/05/04 08:15:29]

With "should", what's the failure behavior? Are they expected to crash, or just
return nullptr? I think we should decide on one or the other, and clarify the
comment accordingly.

[magjed_webrtc, 2017/05/04 12:25:13]

Right now I'm enforcing it with RTC_NOTREACHED and then returning null. This
will crash in debug builds and return null in release builds. I updated the
comment to just say undefined behavior when called with incorrect format.

[nisse-webrtc, 2017/05/05 11:33:04]

And then likely crash in release builds too, since we do *not* want to clutter
up calling code with null checks and error handling.
 
Ok. Returning nullptr would also make sense to me, but we can relax this later
if we find some actual code to benefit from that.

+  virtual rtc::scoped_refptr<PlanarYuvBuffer> GetI420();
+  virtual rtc::scoped_refptr<PlanarYuvBuffer> GetI444();
+
+  // Deprecated - use ToI420() first instead.
   // Returns pointer to the pixel data for a given plane. The memory is owned by
   // the VideoFrameBuffer object and must not be freed by the caller.
-  virtual const uint8_t* DataY() const = 0;
-  virtual const uint8_t* DataU() const = 0;
-  virtual const uint8_t* DataV() const = 0;
-
+  virtual const uint8_t* DataY() const;
+  virtual const uint8_t* DataU() const;
+  virtual const uint8_t* DataV() const;
   // Returns the number of bytes between successive rows for a given plane.
-  virtual int StrideY() const = 0;
-  virtual int StrideU() const = 0;
-  virtual int StrideV() const = 0;
+  virtual int StrideY() const;
+  virtual int StrideU() const;
+  virtual int StrideV() const;
 
+  // Deprecated - use BufferType() to determine if the stored data is kNative,
+  // and then cast into the appropriate type.
   // Return the handle of the underlying video frame. This is used when the
   // frame is backed by a texture.
-  virtual void* native_handle() const = 0;
+  virtual void* native_handle() const;
 
-  // Returns a new memory-backed frame buffer converted from this buffer's
-  // native handle.
-  virtual rtc::scoped_refptr<VideoFrameBuffer> NativeToI420Buffer() = 0;
+  // Deprecated - use ToI420() instead.
+  virtual rtc::scoped_refptr<VideoFrameBuffer> NativeToI420Buffer();
 
  protected:
   ~VideoFrameBuffer() override {}
 };
 
+// This interface represents PixelFormat::kI420 and kI444.
+class PlanarYuvBuffer : public VideoFrameBuffer {
+ public:
+  int ChromaWidth() const;
+  int ChromaHeight() const;
+
+  // Returns pointer to the pixel data for a given plane. The memory is owned by
+  // the VideoFrameBuffer object and must not be freed by the caller.
+  const uint8_t* DataY() const override = 0;
+  const uint8_t* DataU() const override = 0;
+  const uint8_t* DataV() const override = 0;
+
+  // Returns the number of bytes between successive rows for a given plane.
+  int StrideY() const override = 0;
+  int StrideU() const override = 0;
+  int StrideV() const override = 0;
+
+  rtc::scoped_refptr<PlanarYuvBuffer> ToI420() override;
+
+  rtc::scoped_refptr<PlanarYuvBuffer> GetI420() override;
+  rtc::scoped_refptr<PlanarYuvBuffer> GetI444() override;
+
+ protected:
+  ~PlanarYuvBuffer() override {}
+};
+
 }  // namespace webrtc
 
 #endif  // WEBRTC_API_VIDEO_VIDEO_FRAME_BUFFER_H_