Add separate base classes for I420 and I444 buffers

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 6d0c99107cdced3adbedc87d5da8958dc909af95..13005afc8a57b5370e3fce5bd88717e51e970f66 100644
--- a/webrtc/api/video/video_frame_buffer.h
+++ b/webrtc/api/video/video_frame_buffer.h
@@ -18,7 +18,8 @@
 
 namespace webrtc {
 
-class PlanarYuvBuffer;
+class I420BufferInterface;
+class I444BufferInterface;
 
 // Base class for frame buffers of different types of pixel format and storage.
 // The tag in type() indicates how the data is represented, and each type is
@@ -58,12 +59,12 @@ class VideoFrameBuffer : public rtc::RefCountInterface {
   // 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();
+  virtual rtc::scoped_refptr<I420BufferInterface> ToI420();
 
   // These functions should only be called if type() is of the correct type.
   // Calling with a different type will result in a crash.
-  rtc::scoped_refptr<PlanarYuvBuffer> GetI420();
-  rtc::scoped_refptr<PlanarYuvBuffer> GetI444();
+  rtc::scoped_refptr<I420BufferInterface> GetI420();

[nisse-webrtc, 2017/05/29 14:20:05]

I think it's nicer if we make these functions virtual, possibly with default
implementations using RTC_NOTREACHED. And then override appropriately in the
I420 and I444 subclasses, without any explicit casts.

Unrelated to this cl: Do these methods really need to return smart pointers?
Depends a bit on how they're used, but they will never do anything interesting
with ownership. 

And if we move to std::shared_ptr (with a refcount outside of the object itself)
it can no longer return a smart pointer, as far as I understand, which seems
like a tricky problem for methods like ToI420 which sometimes want to return a
reference to |this|, and sometimes a reference to a newly constructed object.

[magjed_webrtc, 2017/05/29 17:40:07]

What benefit do you see from making these functions virtual? IMO, it will
complicate the interface. Do you have any practical use case in mind that
requires virtual functions?

Regarding returning smart pointers. It's true that it would be possible to
return raw pointers, but I prefer to not expose raw pointers of ref counted
objects.

[nisse-webrtc, 2017/05/30 07:05:32]

I see two (weak) benefits: Consistency with the other methods whose
implementation depend on the type, i.e., ChromaWidth() and ChromaHeight(), which
are virtual. And we get rid of two explicit casts. But it's partly a question of
taste, where I prefer dispatch via the vtable over explicit run-time type
checks.
I see. Style-guide isn't crystal clear, "return raw pointers when the caller
does not take ownership, and return smart pointers by value otherwise"
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md 

In this case, the caller already has ownership, but returning a smart pointer
enables the caller to destroy and forget about the the original reference.

I think we can leave as is and revisit if/when we want to move over to
std::shared_ptr.

[kwiberg-webrtc, 2017/05/30 07:32:07]

Drive-by opinion: Don't be afraid of raw pointers to refcounted objects. A raw
pointer simply means that you point to something you don't own, and that you
shouldn't make assumptions about how its lifetime is managed.

(We have historically allowed conversion from T* to scoped_refptr<T>, but that's
a bad idea and no one should be doing it.)

+  rtc::scoped_refptr<I444BufferInterface> GetI444();
 
   // Deprecated - use ToI420() first instead.
   // Returns pointer to the pixel data for a given plane. The memory is owned by
@@ -92,8 +93,8 @@ class VideoFrameBuffer : public rtc::RefCountInterface {
 // This interface represents Type::kI420 and Type::kI444.
 class PlanarYuvBuffer : public VideoFrameBuffer {
  public:
-  int ChromaWidth() const;
-  int ChromaHeight() const;
+  virtual int ChromaWidth() const = 0;
+  virtual int ChromaHeight() const = 0;
 
   // 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.
@@ -106,12 +107,36 @@ class PlanarYuvBuffer : public VideoFrameBuffer {
   int StrideU() const override = 0;
   int StrideV() const override = 0;
 
-  rtc::scoped_refptr<PlanarYuvBuffer> ToI420() override;
-
  protected:
   ~PlanarYuvBuffer() override {}
 };
 
+class I420BufferInterface : public PlanarYuvBuffer {
+ public:
+  Type type() const final;
+
+  int ChromaWidth() const final;
+  int ChromaHeight() const final;
+
+  rtc::scoped_refptr<I420BufferInterface> ToI420() final;

[nisse-webrtc, 2017/05/29 14:20:05]

Here we could also put the GetI420() final {return this;}

+
+ protected:
+  ~I420BufferInterface() override {}
+};
+
+class I444BufferInterface : public PlanarYuvBuffer {
+ public:
+  Type type() const final;
+
+  int ChromaWidth() const final;
+  int ChromaHeight() const final;
+
+  rtc::scoped_refptr<I420BufferInterface> ToI420() final;
+
+ protected:
+  ~I444BufferInterface() override {}
+};
+
 }  // namespace webrtc
 
 #endif  // WEBRTC_API_VIDEO_VIDEO_FRAME_BUFFER_H_