Refactor WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() logic

content/renderer/media/webmediaplayer_ms_compositor.cc

Index: content/renderer/media/webmediaplayer_ms_compositor.cc
diff --git a/content/renderer/media/webmediaplayer_ms_compositor.cc b/content/renderer/media/webmediaplayer_ms_compositor.cc
index 99336fc6e68a23f9cc84f488f385648b208a11ab..f557155962c4493c2d56003abccff8a175adc446 100644
--- a/content/renderer/media/webmediaplayer_ms_compositor.cc
+++ b/content/renderer/media/webmediaplayer_ms_compositor.cc
@@ -14,6 +14,7 @@
 #include "cc/paint/skia_paint_canvas.h"
 #include "content/renderer/media/webmediaplayer_ms.h"
 #include "content/renderer/render_thread_impl.h"
+#include "media/base/bind_to_current_loop.h"
 #include "media/base/media_switches.h"
 #include "media/base/video_frame.h"
 #include "media/base/video_util.h"
@@ -37,62 +38,15 @@ namespace {
 scoped_refptr<media::VideoFrame> CopyFrame(
     const scoped_refptr<media::VideoFrame>& frame,
     media::SkCanvasVideoRenderer* video_renderer) {
-  scoped_refptr<media::VideoFrame> new_frame;
-  if (frame->HasTextures()) {
-    DCHECK(frame->format() == media::PIXEL_FORMAT_ARGB ||
-           frame->format() == media::PIXEL_FORMAT_XRGB ||
-           frame->format() == media::PIXEL_FORMAT_I420 ||
-           frame->format() == media::PIXEL_FORMAT_UYVY ||
-           frame->format() == media::PIXEL_FORMAT_NV12);
-    new_frame = media::VideoFrame::CreateFrame(
-        media::PIXEL_FORMAT_I420, frame->coded_size(), frame->visible_rect(),
-        frame->natural_size(), frame->timestamp());
-
-    ui::ContextProviderCommandBuffer* const provider =
-        RenderThreadImpl::current()->SharedMainThreadContextProvider().get();
-    if (!provider) {
-      // Return a black frame (yuv = {0, 0x80, 0x80}).
-      return media::VideoFrame::CreateColorFrame(
-          frame->visible_rect().size(), 0u, 0x80, 0x80, frame->timestamp());
-    }
-
-    SkBitmap bitmap;
-    bitmap.allocPixels(SkImageInfo::MakeN32Premul(
-        frame->visible_rect().width(), frame->visible_rect().height()));
-    cc::SkiaPaintCanvas paint_canvas(bitmap);
-
-    DCHECK(provider->ContextGL());
-    video_renderer->Copy(
-        frame.get(), &paint_canvas,
-        media::Context3D(provider->ContextGL(), provider->GrContext()));
-
-    SkPixmap pixmap;
-    const bool result = bitmap.peekPixels(&pixmap);
-    DCHECK(result) << "Error trying to access SkBitmap's pixels";
-
-    const uint32 source_pixel_format =
-        (kN32_SkColorType == kRGBA_8888_SkColorType) ? libyuv::FOURCC_ABGR
-                                                     : libyuv::FOURCC_ARGB;
-    libyuv::ConvertToI420(
-        static_cast<const uint8*>(pixmap.addr(0, 0)),
-        pixmap.getSafeSize64(),
-        new_frame->visible_data(media::VideoFrame::kYPlane),
-        new_frame->stride(media::VideoFrame::kYPlane),
-        new_frame->visible_data(media::VideoFrame::kUPlane),
-        new_frame->stride(media::VideoFrame::kUPlane),
-        new_frame->visible_data(media::VideoFrame::kVPlane),
-        new_frame->stride(media::VideoFrame::kVPlane),
-        0 /* crop_x */, 0 /* crop_y */,
-        pixmap.width(), pixmap.height(),
-        new_frame->visible_rect().width(), new_frame->visible_rect().height(),
-        libyuv::kRotate0, source_pixel_format);
-  } else {
+  // Frames originated from local video capture may have finite number of
+  // buffers, so copy only those.
+  if (!frame->HasTextures()) {

[qiangchen, 2017/05/04 16:25:59]

What about frame with texture? This way, you will simply not make a copy of it.

[emircan, 2017/05/04 17:58:14]

Yes, that is what I am intending to do. Look at the comment in l.341 before my
changes. It says we make copy because we have a limited number of video capture
buffers(3) and we don't want to hold onto one here. However, there is no texture
buffers originated from video capture. Local capture buffers are all mappable.
We download the texture buffers into CPU memory here and they are moved back to
textures before compositing, which is totally unnecessary.

[qiangchen, 2017/05/06 12:44:38]

But there is buffer limit for hardware decoder, I think the number is 4 if I
remember correctly. (You can test it by holding 4 frames in WMPMS, and see that
the stream is totally freezing) So at least, we need to copy the frame here and
release the old one. If you think the download upload is a performance concern,
what about copying it within GPU?

[emircan, 2017/05/08 17:14:02]

That would be 4 frames per decoder though. So, it would cause a problem if
somebody decides to play the same decoded stream in 4+ players and pause at
different times. It is unlikely but I understand that we can't allow that case.
Thanks for pointing out. I am reverting those changes.
I couldn't find any texture to texture copy example using SKCanvasVideoRenderer
that can do it within GPU. I am not sure if we can do that from renderer. 

     DCHECK(frame->IsMappable());
     DCHECK(frame->format() == media::PIXEL_FORMAT_YV12 ||
            frame->format() == media::PIXEL_FORMAT_YV12A ||
            frame->format() == media::PIXEL_FORMAT_I420);
     const gfx::Size& coded_size = frame->coded_size();
-    new_frame = media::VideoFrame::CreateFrame(
+    scoped_refptr<media::VideoFrame> new_frame = media::VideoFrame::CreateFrame(
         media::IsOpaque(frame->format()) ? media::PIXEL_FORMAT_I420
                                          : media::PIXEL_FORMAT_YV12A,
         coded_size, frame->visible_rect(), frame->natural_size(),
@@ -117,20 +71,22 @@ scoped_refptr<media::VideoFrame> CopyFrame(
                         new_frame->stride(media::VideoFrame::kAPlane),
                         coded_size.width(), coded_size.height());
     }
+    // Transfer metadata keys.
+    new_frame->metadata()->MergeMetadataFrom(frame->metadata());
+    return new_frame;
   }
-
-  // Transfer metadata keys.
-  new_frame->metadata()->MergeMetadataFrom(frame->metadata());
-  return new_frame;
+  return frame;
 }
 
 }  // anonymous namespace
 
 WebMediaPlayerMSCompositor::WebMediaPlayerMSCompositor(
-    const scoped_refptr<base::SingleThreadTaskRunner>& compositor_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> compositor_task_runner,
+    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
     const blink::WebMediaStream& web_stream,
     const base::WeakPtr<WebMediaPlayerMS>& player)
     : compositor_task_runner_(compositor_task_runner),
+      io_task_runner_(io_task_runner),
       player_(player),
       video_frame_provider_client_(nullptr),
       current_frame_used_by_compositor_(false),
@@ -139,7 +95,6 @@ WebMediaPlayerMSCompositor::WebMediaPlayerMSCompositor(
       dropped_frame_count_(0),
       stopped_(true) {
   main_message_loop_ = base::MessageLoop::current();
-  io_thread_checker_.DetachFromThread();
 
   blink::WebVector<blink::WebMediaStreamTrack> video_tracks;
   if (!web_stream.IsNull())
@@ -208,7 +163,7 @@ void WebMediaPlayerMSCompositor::SetVideoFrameProviderClient(
 
 void WebMediaPlayerMSCompositor::EnqueueFrame(
     scoped_refptr<media::VideoFrame> frame) {
-  DCHECK(io_thread_checker_.CalledOnValidThread());
+  DCHECK(io_task_runner_->BelongsToCurrentThread());
   base::AutoLock auto_lock(current_frame_lock_);
   ++total_frame_count_;
 
@@ -334,16 +289,13 @@ void WebMediaPlayerMSCompositor::StopRendering() {
 
 void WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopy() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  base::AutoLock auto_lock(current_frame_lock_);
-  if (!current_frame_.get() || !player_)
-    return;
-
-  // Copy the frame so that rendering can show the last received frame.
-  // The original frame must not be referenced when the player is paused since
-  // there might be a finite number of available buffers. E.g, video that
-  // originates from a video camera.
-  current_frame_ =
-      CopyFrame(current_frame_, player_->GetSkCanvasVideoRenderer());
+  // Bounce this call off of IO thread to since there might still be frames
+  // passed on IO thread.
+  io_task_runner_->PostTask(
+      FROM_HERE,
+      media::BindToCurrentLoop(base::Bind(
+          &WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopyInternal,
+          this)));
 }
 
 void WebMediaPlayerMSCompositor::StopUsingProvider() {
@@ -358,7 +310,7 @@ bool WebMediaPlayerMSCompositor::MapTimestampsToRenderTimeTicks(
     std::vector<base::TimeTicks>* wall_clock_times) {
   DCHECK(compositor_task_runner_->BelongsToCurrentThread() ||
          thread_checker_.CalledOnValidThread() ||
-         io_thread_checker_.CalledOnValidThread());
+         io_task_runner_->BelongsToCurrentThread());
   for (const base::TimeDelta& timestamp : timestamps) {
     DCHECK(timestamps_to_clock_times_.count(timestamp));
     wall_clock_times->push_back(timestamps_to_clock_times_[timestamp]);
@@ -449,6 +401,27 @@ void WebMediaPlayerMSCompositor::StopUsingProviderInternal() {
   video_frame_provider_client_ = nullptr;
 }
 
+void WebMediaPlayerMSCompositor::ReplaceCurrentFrameWithACopyInternal() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  scoped_refptr<media::VideoFrame> current_frame;
+  {
+    base::AutoLock auto_lock(current_frame_lock_);
+    if (!current_frame_.get() || !player_)

[DaleCurtis, 2017/05/04 00:45:37]

Can anyone else set current_frame_? Or is this called from multiple threads? Not
safe to check unlock and relock then.

[emircan, 2017/05/04 17:58:14]

I was thinking of releasing the lock so that basic getters in this class can
work: GetCurrentSize(), total_frame_count(), dropped_frame_count(),
HasCurrentFrame() . But I realized they are all running on main thread as well,
so this won't help at all. I am reverting it so that lock applies to the whole
scope.

+      return;
+
+    current_frame = current_frame_;
+  }
+  // Copy the frame so that rendering can show the last received frame.
+  // The original frame must not be referenced when the player is paused since
+  // there might be a finite number of available buffers. E.g, video that
+  // originates from a video camera.
+  current_frame = CopyFrame(current_frame, player_->GetSkCanvasVideoRenderer());
+  {
+    base::AutoLock auto_lock(current_frame_lock_);
+    current_frame_ = current_frame;
+  }
+}
+
 void WebMediaPlayerMSCompositor::SetAlgorithmEnabledForTesting(
     bool algorithm_enabled) {
   if (!algorithm_enabled) {