Update I420Buffer to new VideoFrameBuffer interface

webrtc/api/video/i420_buffer.cc

Index: webrtc/api/video/i420_buffer.cc
diff --git a/webrtc/api/video/i420_buffer.cc b/webrtc/api/video/i420_buffer.cc
index 51d83c9df58ba90508bb263434ebfca3c78d8e4e..c4afdba45bbf64ab10560bf540b2d9076914ceb1 100644
--- a/webrtc/api/video/i420_buffer.cc
+++ b/webrtc/api/video/i420_buffer.cc
@@ -76,8 +76,8 @@ rtc::scoped_refptr<I420Buffer> I420Buffer::Create(int width,
 }
 
 // static
-rtc::scoped_refptr<I420Buffer> I420Buffer::Copy(
-    const VideoFrameBuffer& source) {
+rtc::scoped_refptr<I420Buffer> I420Buffer::Copy(const PlanarYuvBuffer& source) {
+  RTC_CHECK(source.type() == Type::kI420);
   return Copy(source.width(), source.height(),
               source.DataY(), source.StrideY(),
               source.DataU(), source.StrideU(),
@@ -103,8 +103,9 @@ rtc::scoped_refptr<I420Buffer> I420Buffer::Copy(
 }
 
 // static
-rtc::scoped_refptr<I420Buffer> I420Buffer::Rotate(
-    const VideoFrameBuffer& src, VideoRotation rotation) {
+rtc::scoped_refptr<I420Buffer> I420Buffer::Rotate(const PlanarYuvBuffer& src,
+                                                  VideoRotation rotation) {
+  RTC_CHECK(src.type() == Type::kI420);

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

Hmm. To get better compile time type-checking, could we add some abstract
classes for I420 and I444 buffers, which would inherit PlanarYuvBuffer and
define the type() method only (and possibly define it using |final|).

Would be used for the input to functions like this which require I420, and the
GetI40, ToI420 and GetI444 methods should return a pointer of the right type,
and likewise should the I420BufferPool, etc.

Not sure if it can work out nicely, but what do you think?

[magjed_webrtc, 2017/05/29 12:12:06]

Yeah, this is something I thought about when I did the first CL for exactly the
reasons you mention. I think it might be inflexible though to have a class for
'type() == Type::kI420', something that is currently dynamic. For example, when
implementing WrappedPlanarYuvBuffer, we can now send the type as an argument and
only need one class. If we make separate types for I420 and I444 we would need
two classes WrappedI420 and WrappedI444 in order to return different types, and
possible need to duplicate some logic for that.

I'm still on the fence, but if we want to change it, we should do it asap. What
do you think?

[nisse-webrtc, 2017/05/29 12:37:17]

Maybe we have to prepare some experimental cl to find out if it's feasible?
I guess one could avoid most code duplication by making the base type be a
template argument, but it might still be a bit awkward.
I think proper type checking for functions which require input arguments to be
I420 is a pretty strong reasong for doing this change. And it's hard to judge
the drawbacks without writing the code.

And we also have the hard naming problem... I420Buffer is taken, for the
implementation class. Should it be I420BufferInterface? That's not very pretty.

[magjed_webrtc, 2017/05/29 13:57:02]

It's feasible, I uploaded a CL here: https://codereview.webrtc.org/2914463002/.
I had to go with the name I420BufferInterface. Maybe we could change I420Buffer
to I420BufferImpl and I420BufferInterface to I420Buffer in the future, but the
names are not a big issue for me.

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

I420BufferInterface is good enough, I think. And I'm not so fond of the *Impl
naming convention either.

   RTC_CHECK(src.DataY());
   RTC_CHECK(src.DataU());
   RTC_CHECK(src.DataV());
@@ -187,12 +188,12 @@ void I420Buffer::SetBlack(I420Buffer* buffer) {
                              0, 128, 128) == 0);
 }
 
-void I420Buffer::CropAndScaleFrom(
-    const VideoFrameBuffer& src,
-    int offset_x,
-    int offset_y,
-    int crop_width,
-    int crop_height) {
+void I420Buffer::CropAndScaleFrom(const PlanarYuvBuffer& src,
+                                  int offset_x,
+                                  int offset_y,
+                                  int crop_width,
+                                  int crop_height) {
+  RTC_CHECK(src.type() == Type::kI420);
   RTC_CHECK_LE(crop_width, src.width());
   RTC_CHECK_LE(crop_height, src.height());
   RTC_CHECK_LE(crop_width + offset_x, src.width());
@@ -224,8 +225,7 @@ void I420Buffer::CropAndScaleFrom(
   RTC_DCHECK_EQ(res, 0);
 }
 
-void I420Buffer::CropAndScaleFrom(
-    const VideoFrameBuffer& src) {
+void I420Buffer::CropAndScaleFrom(const PlanarYuvBuffer& src) {
   const int crop_width =
       std::min(src.width(), width() * src.height() / height());
   const int crop_height =
@@ -237,7 +237,7 @@ void I420Buffer::CropAndScaleFrom(
       crop_width, crop_height);
 }
 
-void I420Buffer::ScaleFrom(const VideoFrameBuffer& src) {
+void I420Buffer::ScaleFrom(const PlanarYuvBuffer& src) {
   CropAndScaleFrom(src, 0, 0, src.width(), src.height());
 }