Add separate base classes for I420 and I444 buffers

webrtc/api/video/video_frame_buffer.h

 /*
  *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #ifndef WEBRTC_API_VIDEO_VIDEO_FRAME_BUFFER_H_
 #define WEBRTC_API_VIDEO_VIDEO_FRAME_BUFFER_H_
 
 #include <stdint.h>
 
 #include "webrtc/base/refcount.h"
 #include "webrtc/base/scoped_ref_ptr.h"
 
 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
 // implemented as a subclass. To access the pixel data, call the appropriate
 // GetXXX() function, where XXX represents the type. There is also a function
 // ToI420() that returns a frame buffer in I420 format, converting from the
 // underlying representation if necessary. I420 is the most widely accepted
 // format and serves as a fallback for video sinks that can only handle I420,
 // e.g. the internal WebRTC software encoders. A special enum value 'kNative' is
 // provided for external clients to implement their own frame buffer
@@ skipping 19 matching lines @@
 
   // 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();
+  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
   // the VideoFrameBuffer object and must not be freed by the caller.
   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;
   virtual int StrideU() const;
   virtual int StrideV() const;
 
   // Deprecated - use type() 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;
 
   // Deprecated - use ToI420() instead.
   virtual rtc::scoped_refptr<VideoFrameBuffer> NativeToI420Buffer();
 
  protected:
   ~VideoFrameBuffer() override {}
 };
 
 // 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.
   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;
-
  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_