Return WrappedI444Buffer in VP9Impl

webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc

 /*
  *  Copyright (c) 2014 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.
  *
  */
@@ skipping 948 matching lines @@
     return WEBRTC_VIDEO_CODEC_NO_OUTPUT;
   }
 
   // This buffer contains all of |img|'s image data, a reference counted
   // Vp9FrameBuffer. (libvpx is done with the buffers after a few
   // vpx_codec_decode calls or vpx_codec_destroy).
   Vp9FrameBufferPool::Vp9FrameBuffer* img_buffer =
       static_cast<Vp9FrameBufferPool::Vp9FrameBuffer*>(img->fb_priv);
   // The buffer can be used directly by the VideoFrame (without copy) by
   // using a WrappedI420Buffer.
-  rtc::scoped_refptr<WrappedI420Buffer> img_wrapped_buffer(
-      new rtc::RefCountedObject<webrtc::WrappedI420Buffer>(
-          img->d_w, img->d_h, img->planes[VPX_PLANE_Y],
-          img->stride[VPX_PLANE_Y], img->planes[VPX_PLANE_U],
-          img->stride[VPX_PLANE_U], img->planes[VPX_PLANE_V],
-          img->stride[VPX_PLANE_V],
-          // WrappedI420Buffer's mechanism for allowing the release of its frame
-          // buffer is through a callback function. This is where we should
-          // release |img_buffer|.
-          rtc::KeepRefUntilDone(img_buffer)));
+  rtc::scoped_refptr<VideoFrameBuffer> img_wrapped_buffer;
+
+  if (img->fmt == VPX_IMG_FMT_I444) {
+    img_wrapped_buffer = new rtc::RefCountedObject<webrtc::WrappedI444Buffer>(

[nisse-webrtc, 2017/06/09 09:01:22]

It's a pity classes aren't first class in C++, but maybe we can use static
Create methods as a substitute? They're nice to have anyway, to omit the
RefCountedObject boilerplate. So maybe this could be written more clearly and
concisely as 

img_wrapped_buffer = 
    (type == VPX_IMG_FMR_I444 
        ? WrappedI444Buffer::Create 
        : WrappedI420Buffer::Create) (
    ... the identical construction args...);

[Yuwei, 2017/06/10 00:13:53]

Unfortunately function that returns scoped_refptr<I420BufferInterface> is not
thought to be compatible with function that returns
scoped_refptr<I444BufferInterface> when applying ternary operator...


If we don't care about the I4XXBufferInterface information then we could simply
implement the factory function like this:

scoped_refptr<VideoFrameBuffer> WrapYuvBuffer(width, height, ..., pixel_format);

rather than having WrapI420 and WrapI444 separately.

Given that other uses of WrappedI420Buffer only treat it as a VideoFrameBuffer
and VideoFrameBuffer has a Type enum, it should be fine to do so?

[magjed_webrtc, 2017/06/13 11:38:27]

Yeah, I would like to spare users this duplication. Can you add an extra
function alongside WrapI420 and WrapI444 like this:
rtc::scoped_refptr<PlanarYuvBuffer> WrapYuvBuffer(Type type, width, height, ...)
that will call either WrapI420 or WrapI444 depending on |type|?

[nisse-webrtc, 2017/06/13 12:29:09]

Ouch. One would need to wrap them in a lambda or std::function with the intended
return type, then. Which is no longer so nice and concise. 

[nisse-webrtc, 2017/06/13 12:29:09]

Sounds good to me too.

[Yuwei, 2017/06/19 21:33:26]

Added the WrapYuvBuffer function.

Functional tricks may just be too complex in this case. I think the switch in
WrapYuvBuffer is manageable enough.

[Yuwei, 2017/06/19 21:33:26]

Done.

+        img->d_w, img->d_h, img->planes[VPX_PLANE_Y],
+        img->stride[VPX_PLANE_Y], img->planes[VPX_PLANE_U],
+        img->stride[VPX_PLANE_U], img->planes[VPX_PLANE_V],
+        img->stride[VPX_PLANE_V],
+        // WrappedI444Buffer's mechanism for allowing the release of its frame
+        // buffer is through a callback function. This is where we should
+        // release |img_buffer|.
+        rtc::KeepRefUntilDone(img_buffer));
+  } else {
+    img_wrapped_buffer = new rtc::RefCountedObject<webrtc::WrappedI420Buffer>(
+        img->d_w, img->d_h, img->planes[VPX_PLANE_Y],
+        img->stride[VPX_PLANE_Y], img->planes[VPX_PLANE_U],
+        img->stride[VPX_PLANE_U], img->planes[VPX_PLANE_V],
+        img->stride[VPX_PLANE_V],
+        // WrappedI420Buffer's mechanism for allowing the release of its frame
+        // buffer is through a callback function. This is where we should
+        // release |img_buffer|.
+        rtc::KeepRefUntilDone(img_buffer));
+  }
 
   VideoFrame decoded_image(img_wrapped_buffer, timestamp,
                            0 /* render_time_ms */, webrtc::kVideoRotation_0);
   decoded_image.set_ntp_time_ms(ntp_time_ms);
 
   decode_complete_callback_->Decoded(decoded_image, rtc::Optional<int32_t>(),
                                      rtc::Optional<uint8_t>(qp));
   return WEBRTC_VIDEO_CODEC_OK;
 }
 
@@ skipping 19 matching lines @@
   frame_buffer_pool_.ClearPool();
   inited_ = false;
   return WEBRTC_VIDEO_CODEC_OK;
 }
 
 const char* VP9DecoderImpl::ImplementationName() const {
   return "libvpx";
 }
 
 }  // namespace webrtc