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..a16822a2e7754f9d6009826d35ba0a9006b26588 100644
--- a/webrtc/api/video/video_frame_buffer.h
+++ b/webrtc/api/video/video_frame_buffer.h
@@ -18,38 +18,121 @@
 
 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 I420BufferInterface;
+class I444BufferInterface;
+
+// 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 ToXXX() function, where XXX represents the pixel format specified
+// by Format(). The function ToI420() can always be called, and serves as a
+// fallback for e.g. the internal WebRTC software encoders that can only handle
+// I420. Calling ToI420() requires a pixel format conversion for formats other
+// than 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 {
+    kNative,
+    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<I420BufferInterface> ToI420();
+
+  // This function should only be called if Format() == kI444.
+  virtual rtc::scoped_refptr<I444BufferInterface> ToI444();
+
+  // 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.
+class I420BufferInterface : public VideoFrameBuffer {
+ public:
+  PixelFormat Format() const override;
+
+  int ChromaWidth() const { return (width() + 1) / 2; }

[nisse-webrtc, 2017/05/02 09:52:37]

Implementing these separately in the two interfaces defeats the purpose of these
methods since one would need to always have separate code for I420 and I444.

I'd suggest having a YuvBufferInterface, with ChromaWidth and ChromaHeight
methods deriving the value from Format().

And have methods

  ToI420() which returns a YuvBufferInterface, promising that it really is I420.

  ToYuv() which returns an YuvBufferInterface, with identical Format(), which
can be either kI420 or kI444.

If we really want to have separate C++ interfaces for I420 and I444, those
interfaces should inherit YuvBufferInterface, and we could add an ToI444 method
too. But I don't think that's essential.

I haven't though about the transition issues, how hard do you think it is?

[magjed_webrtc, 2017/05/03 13:36:55]

I added a PlanarYuvBuffer interface that is common for I420 and I444, and
implemented ToI420 and ChromaWidth/ChromaHeight with switch-statements instead.

I did not add a ToYuv function though. I want to keep it as straight forward as
possible and have one function per pixel format (except kNative). Also, I
renamed the functions, because ToI420 is special and should have a different
name than the other functions that don't convert. We can always add a ToYuv
helper function outside the interface that does exactly what you want.

This CL is backwards compatible and will not break any external client. After
this CL, it should be possible to update external client as well as the rest of
the WebRTC code base, and then remove the deprecated functions without issues.

+  int ChromaHeight() const { return (height() + 1) / 2; }
+
+  // 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<I420BufferInterface> ToI420() override;
+
+ protected:
+  ~I420BufferInterface() override {}
+};
+
+// This interface represents PixelFormat::kI444.
+class I444BufferInterface : public VideoFrameBuffer {
+ public:
+  PixelFormat Format() const override;
+
+  int ChromaWidth() const { return width(); }
+  int ChromaHeight() const { return height(); }
+
+  // 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<I420BufferInterface> ToI420() override;
+  rtc::scoped_refptr<I444BufferInterface> ToI444() override;
+
+ protected:
+  ~I444BufferInterface() override {}
+};
+
 }  // namespace webrtc
 
 #endif  // WEBRTC_API_VIDEO_VIDEO_FRAME_BUFFER_H_