Update I420Buffer to new VideoFrameBuffer interface

webrtc/api/video/i420_buffer.cc

 /*
  *  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.
  */
 #include "webrtc/api/video/i420_buffer.h"
@@ skipping 58 matching lines @@
 rtc::scoped_refptr<I420Buffer> I420Buffer::Create(int width,
                                                   int height,
                                                   int stride_y,
                                                   int stride_u,
                                                   int stride_v) {
   return new rtc::RefCountedObject<I420Buffer>(
       width, height, stride_y, stride_u, stride_v);
 }
 
 // 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(),
               source.DataV(), source.StrideV());
 }
 
 // static
 rtc::scoped_refptr<I420Buffer> I420Buffer::Copy(
       int width, int height,
       const uint8_t* data_y, int stride_y,
       const uint8_t* data_u, int stride_u,
       const uint8_t* data_v, int stride_v) {
   // Note: May use different strides than the input data.
   rtc::scoped_refptr<I420Buffer> buffer = Create(width, height);
   RTC_CHECK_EQ(0, libyuv::I420Copy(data_y, stride_y,
                                    data_u, stride_u,
                                    data_v, stride_v,
                                    buffer->MutableDataY(), buffer->StrideY(),
                                    buffer->MutableDataU(), buffer->StrideU(),
                                    buffer->MutableDataV(), buffer->StrideV(),
                                    width, height));
   return buffer;
 }
 
 // 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());
 
   int rotated_width = src.width();
   int rotated_height = src.height();
   if (rotation == webrtc::kVideoRotation_90 ||
       rotation == webrtc::kVideoRotation_270) {
     std::swap(rotated_width, rotated_height);
   }
@@ skipping 62 matching lines @@
 
 // static
 void I420Buffer::SetBlack(I420Buffer* buffer) {
   RTC_CHECK(libyuv::I420Rect(buffer->MutableDataY(), buffer->StrideY(),
                              buffer->MutableDataU(), buffer->StrideU(),
                              buffer->MutableDataV(), buffer->StrideV(),
                              0, 0, buffer->width(), buffer->height(),
                              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());
   RTC_CHECK_LE(crop_height + offset_y, src.height());
   RTC_CHECK_GE(offset_x, 0);
   RTC_CHECK_GE(offset_y, 0);
 
   // Make sure offset is even so that u/v plane becomes aligned.
   const int uv_offset_x = offset_x / 2;
   const int uv_offset_y = offset_y / 2;
@@ skipping 11 matching lines @@
                               v_plane, src.StrideV(),
                               crop_width, crop_height,
                               MutableDataY(), StrideY(),
                               MutableDataU(), StrideU(),
                               MutableDataV(), StrideV(),
                               width(), height(), libyuv::kFilterBox);
 
   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 =
       std::min(src.height(), height() * src.width() / width());
 
   CropAndScaleFrom(
       src,
       (src.width() - crop_width) / 2, (src.height() - crop_height) / 2,
       crop_width, crop_height);
 }
 
-void I420Buffer::ScaleFrom(const VideoFrameBuffer& src) {
+void I420Buffer::ScaleFrom(const PlanarYuvBuffer& src) {
   CropAndScaleFrom(src, 0, 0, src.width(), src.height());
 }
 
 }  // namespace webrtc