Move passing of WebRTC rendering frames from main thread to compositor thread

content/renderer/media/media_stream_video_renderer_sink.cc

Index: content/renderer/media/media_stream_video_renderer_sink.cc
diff --git a/content/renderer/media/media_stream_video_renderer_sink.cc b/content/renderer/media/media_stream_video_renderer_sink.cc
index 6ddf1eb1b8ba978cc25a1d896a22b718e84896e1..1fa3ef90c475cc2d0a304e298f3fe95d36b8e910 100644
--- a/content/renderer/media/media_stream_video_renderer_sink.cc
+++ b/content/renderer/media/media_stream_video_renderer_sink.cc
@@ -5,156 +5,306 @@
 #include "content/renderer/media/media_stream_video_renderer_sink.h"
 
 #include "base/feature_list.h"
+#include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
+#include "content/child/child_process.h"
 #include "content/public/common/content_features.h"
+#include "content/public/renderer/render_thread.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/video_frame.h"
 #include "media/base/video_frame_metadata.h"
 #include "media/base/video_util.h"
 #include "media/renderers/gpu_video_accelerator_factories.h"
+#include "media/video/gpu_memory_buffer_video_frame_pool.h"
 
 const int kMinFrameSize = 2;
 
 namespace content {
 
+// FrameDelivererOnCompositor is responsible for delivering frames received on
+// OnVideoFrame() to |repaint_cb_| on compositor thread.
+//
+// It is created on main thread, but methods should be called and class should
+// be destructed on compositor thread.
+class MediaStreamVideoRendererSink::FrameDelivererOnCompositor {
+ public:
+  FrameDelivererOnCompositor(
+      const RepaintCB repaint_cb,
+      const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner,
+      const scoped_refptr<base::TaskRunner>& worker_task_runner,
+      media::GpuVideoAcceleratorFactories* gpu_factories)
+      : repaint_cb_(repaint_cb),
+        state_(STOPPED),
+        frame_size_(kMinFrameSize, kMinFrameSize),
+        media_task_runner_(media_task_runner),
+        weak_factory_for_compositor_(this) {
+    compositor_thread_checker_.DetachFromThread();
+    if (gpu_factories &&
+        gpu_factories->ShouldUseGpuMemoryBuffersForVideoFrames() &&
+        base::FeatureList::IsEnabled(
+            features::kWebRtcUseGpuMemoryBufferVideoFrames)) {
+      gpu_memory_buffer_pool_.reset(new media::GpuMemoryBufferVideoFramePool(
+          media_task_runner, worker_task_runner, gpu_factories));
+    }
+  }
+  ~FrameDelivererOnCompositor() {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+
+    if (gpu_memory_buffer_pool_) {
+      media_task_runner_->DeleteSoon(FROM_HERE,
+                                     gpu_memory_buffer_pool_.release());
+    }
+  }
+
+  void OnVideoFrame(const scoped_refptr<media::VideoFrame>& frame,
+                    base::TimeTicks current_time) {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    DCHECK(frame);
+
+    if (state_ != STARTED)
+      return;
+
+    if (!gpu_memory_buffer_pool_) {
+      FrameReady(frame);
+      return;
+    }
+    //  |gpu_memory_buffer_pool_| deletion is going to be posted to
+    //  |media_task_runner_|. base::Unretained() usage is fine since
+    //  |gpu_memory_buffer_pool_| outlives the task.
+    media_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &media::GpuMemoryBufferVideoFramePool::MaybeCreateHardwareFrame,
+            base::Unretained(gpu_memory_buffer_pool_.get()), frame,
+            media::BindToCurrentLoop(base::Bind(
+                &FrameDelivererOnCompositor::FrameReady, GetWeakPtr()))));
+  }
+
+  void FrameReady(const scoped_refptr<media::VideoFrame>& frame) {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    DCHECK(frame);

[perkj_chrome, 2016/11/14 15:19:55]

should you check the state here too? This frame can now be rendered after Stop()
since OnVideoFrame is async.- To answer my own question-No, it looks like
RenderSingnalingFrame renderers a frame in state == Stopped. Maybe add a comment
about that.

[emircan, 2016/11/14 22:43:32]

Actually, we should check it. Thanks for catching this. It looks like I modified
this behavior in the earlier CL below. I refactored it so it works like before. 

https://codereview.chromium.org/1358883003/diff/330001/content/renderer/media...

+
+    frame_size_ = frame->natural_size();
+    TRACE_EVENT_INSTANT1(
+        "webrtc",
+        "MediaStreamVideoRendererSink::FrameDelivererOnCompositor::FrameReady",
+        TRACE_EVENT_SCOPE_THREAD, "timestamp",
+        frame->timestamp().InMilliseconds());
+    repaint_cb_.Run(frame);
+  }
+
+  void RenderSignalingFrame() {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    // This is necessary to make sure audio can play if the video tag src is
+    // a MediaStream video track that has been rejected or ended.
+    // It also ensure that the renderer don't hold a reference to a real video

[perkj_chrome, 2016/11/14 15:19:55]

/s doesn't

[emircan, 2016/11/14 22:43:32]

Done.

+    // frame if no more frames are provided. This is since there might be a
+    // finite number of available buffers. E.g, video that
+    // originates from a video camera.
+    scoped_refptr<media::VideoFrame> video_frame =
+        media::VideoFrame::CreateBlackFrame(
+            (state_ == STOPPED) ? gfx::Size(kMinFrameSize, kMinFrameSize)
+                                : frame_size_);
+    video_frame->metadata()->SetBoolean(
+        media::VideoFrameMetadata::END_OF_STREAM, true);
+    video_frame->metadata()->SetTimeTicks(
+        media::VideoFrameMetadata::REFERENCE_TIME, base::TimeTicks::Now());
+    FrameReady(video_frame);
+  }
+
+  void Start() {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    DCHECK_EQ(state_, STOPPED);
+    state_ = STARTED;
+  }
+
+  void Stop() {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    DCHECK(state_ == STARTED || state_ == PAUSED);
+    state_ = STOPPED;
+  }
+
+  void Resume() {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    if (state_ == PAUSED)
+      state_ = STARTED;
+  }
+
+  void Pause() {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    if (state_ == STARTED)
+      state_ = PAUSED;
+  }
+
+  base::WeakPtr<FrameDelivererOnCompositor> GetWeakPtr() {
+    return weak_factory_for_compositor_.GetWeakPtr();
+  }
+
+ private:
+  friend class MediaStreamVideoRendererSink;
+
+  void SetGpuMemoryBufferVideoForTesting(
+      media::GpuMemoryBufferVideoFramePool* gpu_memory_buffer_pool) {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    gpu_memory_buffer_pool_.reset(gpu_memory_buffer_pool);
+  }
+
+  const RepaintCB repaint_cb_;
+  State state_;
+  gfx::Size frame_size_;
+
+  // Pool of GpuMemoryBuffers and resources used to create hardware frames.
+  std::unique_ptr<media::GpuMemoryBufferVideoFramePool> gpu_memory_buffer_pool_;
+  const scoped_refptr<base::SingleThreadTaskRunner> media_task_runner_;
+
+  // Used for DCHECKs to ensure method calls executed on the correct thread.

[perkj_chrome, 2016/11/14 15:19:55]

/s calls are

[emircan, 2016/11/14 22:43:32]

Done.

+  base::ThreadChecker compositor_thread_checker_;
+
+  base::WeakPtrFactory<FrameDelivererOnCompositor> weak_factory_for_compositor_;
+
+  DISALLOW_COPY_AND_ASSIGN(FrameDelivererOnCompositor);
+};
+
+// FrameReceiverOnIO is responsible for trampolining frames from IO thread to
+// compositor thread by posting a task on |deliverer_|'s OnFrame() method.

[perkj_chrome, 2016/11/14 15:19:55]

/s to the compositor thread

[emircan, 2016/11/14 22:43:32]

Done.

+//
+// It is created on main thread, but methods should be called and class should
+// be destructed on IO thread.

[perkj_chrome, 2016/11/14 15:19:55]

/s the IO

[emircan, 2016/11/14 22:43:32]

Done.

+class MediaStreamVideoRendererSink::FrameReceiverOnIO {
+ public:
+  FrameReceiverOnIO(
+      const scoped_refptr<base::SingleThreadTaskRunner>& compositor_task_runner,
+      const base::WeakPtr<FrameDelivererOnCompositor>& deliverer)
+      : compositor_task_runner_(compositor_task_runner),
+        deliverer_(deliverer),
+        weak_factory_for_io_(this) {
+    io_thread_checker_.DetachFromThread();
+  }
+  ~FrameReceiverOnIO() { DCHECK(io_thread_checker_.CalledOnValidThread()); }
+
+  void OnVideoFrame(const scoped_refptr<media::VideoFrame>& frame,
+                    base::TimeTicks current_time) {
+    DCHECK(io_thread_checker_.CalledOnValidThread());
+    compositor_task_runner_->PostTask(

[perkj_chrome, 2016/11/14 15:19:55]

What is the most common case? with or without GPU buffers?, ie- can we avoid the
extra thread hop from IO->compositor->media_task_runner->compositor  here? and
first jump to the media_task_runner? 

It looks like media::GpuMemoryBufferVideoFramePool::MaybeCreateHardwareFrame do
yet another thread hop to worker_task_runner. That is a lot of thread hops....

[emircan, 2016/11/14 22:43:32]

Right now GpuMemoryBufferVideoFramePool is disabled by default, see
crbug.com/652068. But AFAIK future plans are that it would be the only way to
render, such that VideoResourceUpdater would expect only GMB backed frames.

Here, we still should check the state before posting the task to GMBVFP so we
don't have an unnecessary texture upload when paused. That would mean we would
move state interactions on IO thread, but I think we should avoid using IO
thread so that it is more responsive. I thought about this trade-off earlier and
decided to do the thread hop instead. WDYT? 

+        FROM_HERE, base::Bind(&MediaStreamVideoRendererSink::
+                                  FrameDelivererOnCompositor::OnVideoFrame,
+                              deliverer_, frame, current_time));
+  }
+
+  base::WeakPtr<FrameReceiverOnIO> GetWeakPtr() {
+    return weak_factory_for_io_.GetWeakPtr();
+  }
+
+ private:
+  const scoped_refptr<base::SingleThreadTaskRunner> compositor_task_runner_;
+  const base::WeakPtr<FrameDelivererOnCompositor> deliverer_;
+
+  // Used for DCHECKs to ensure method calls executed in the correct thread.
+  base::ThreadChecker io_thread_checker_;
+
+  base::WeakPtrFactory<FrameReceiverOnIO> weak_factory_for_io_;
+
+  DISALLOW_COPY_AND_ASSIGN(FrameReceiverOnIO);
+};
+
 MediaStreamVideoRendererSink::MediaStreamVideoRendererSink(
     const blink::WebMediaStreamTrack& video_track,
     const base::Closure& error_cb,
     const RepaintCB& repaint_cb,
+    const scoped_refptr<base::SingleThreadTaskRunner>& compositor_task_runner,
     const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner,
     const scoped_refptr<base::TaskRunner>& worker_task_runner,
     media::GpuVideoAcceleratorFactories* gpu_factories)
     : error_cb_(error_cb),
-      repaint_cb_(repaint_cb),
-      task_runner_(base::ThreadTaskRunnerHandle::Get()),
-      state_(STOPPED),
-      frame_size_(kMinFrameSize, kMinFrameSize),
       video_track_(video_track),
-      media_task_runner_(media_task_runner),
-      weak_factory_(this) {
-  if (gpu_factories &&
-      gpu_factories->ShouldUseGpuMemoryBuffersForVideoFrames() &&
-      base::FeatureList::IsEnabled(
-          features::kWebRtcUseGpuMemoryBufferVideoFrames)) {
-    gpu_memory_buffer_pool_.reset(new media::GpuMemoryBufferVideoFramePool(
-        media_task_runner, worker_task_runner, gpu_factories));
-  }
-}
+      frame_deliverer_(
+          new MediaStreamVideoRendererSink::FrameDelivererOnCompositor(
+              repaint_cb,
+              media_task_runner,
+              worker_task_runner,
+              gpu_factories)),
+      compositor_task_runner_(compositor_task_runner) {}
 
 MediaStreamVideoRendererSink::~MediaStreamVideoRendererSink() {
-  if (gpu_memory_buffer_pool_) {
-    media_task_runner_->DeleteSoon(FROM_HERE,
-                                   gpu_memory_buffer_pool_.release());
-  }
+  compositor_task_runner_->DeleteSoon(FROM_HERE, frame_deliverer_.release());
 }
 
 void MediaStreamVideoRendererSink::Start() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK_EQ(state_, STOPPED);
+  DCHECK(main_thread_checker_.CalledOnValidThread());
 
+  compositor_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&FrameDelivererOnCompositor::Start,
+                            frame_deliverer_->GetWeakPtr()));
+
+  frame_receiver_.reset(new MediaStreamVideoRendererSink::FrameReceiverOnIO(
+      compositor_task_runner_, frame_deliverer_->GetWeakPtr()));
   MediaStreamVideoSink::ConnectToTrack(
-      video_track_, media::BindToCurrentLoop(
-                        base::Bind(&MediaStreamVideoRendererSink::OnVideoFrame,
-                                   weak_factory_.GetWeakPtr())),
+      video_track_, base::Bind(&FrameReceiverOnIO::OnVideoFrame,
+                               frame_receiver_->GetWeakPtr()),
       // Local display video rendering is considered a secure link.
       true);
-  state_ = STARTED;
 
   if (video_track_.source().getReadyState() ==
           blink::WebMediaStreamSource::ReadyStateEnded ||
       !video_track_.isEnabled()) {
-    RenderSignalingFrame();
+    compositor_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&FrameDelivererOnCompositor::RenderSignalingFrame,
+                              frame_deliverer_->GetWeakPtr()));
   }
 }
 
 void MediaStreamVideoRendererSink::Stop() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(state_ == STARTED || state_ == PAUSED);
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+
   MediaStreamVideoSink::DisconnectFromTrack();
-  weak_factory_.InvalidateWeakPtrs();
-  state_ = STOPPED;
-  frame_size_.set_width(kMinFrameSize);
-  frame_size_.set_height(kMinFrameSize);
+  if (frame_receiver_) {
+    ChildProcess::current()->io_task_runner()->DeleteSoon(
+        FROM_HERE, frame_receiver_.release());
+  }
+  compositor_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&FrameDelivererOnCompositor::Stop,
+                            frame_deliverer_->GetWeakPtr()));
 }
 
 void MediaStreamVideoRendererSink::Resume() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  if (state_ == PAUSED)
-    state_ = STARTED;
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+  compositor_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&FrameDelivererOnCompositor::Resume,
+                            frame_deliverer_->GetWeakPtr()));
 }
 
 void MediaStreamVideoRendererSink::Pause() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  if (state_ == STARTED)
-    state_ = PAUSED;
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+  compositor_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&FrameDelivererOnCompositor::Pause,
+                            frame_deliverer_->GetWeakPtr()));
+}
+
+MediaStreamVideoRendererSink::State
+MediaStreamVideoRendererSink::GetStateForTesting() {
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+  return frame_deliverer_->state_;
 }
 
 void MediaStreamVideoRendererSink::SetGpuMemoryBufferVideoForTesting(
     media::GpuMemoryBufferVideoFramePool* gpu_memory_buffer_pool) {
-  gpu_memory_buffer_pool_.reset(gpu_memory_buffer_pool);
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+  compositor_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&FrameDelivererOnCompositor::SetGpuMemoryBufferVideoForTesting,
+                 frame_deliverer_->GetWeakPtr(), gpu_memory_buffer_pool));
 }
 
 void MediaStreamVideoRendererSink::OnReadyStateChanged(
     blink::WebMediaStreamSource::ReadyState state) {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  if (state == blink::WebMediaStreamSource::ReadyStateEnded)
-    RenderSignalingFrame();
-}
-
-void MediaStreamVideoRendererSink::OnVideoFrame(
-    const scoped_refptr<media::VideoFrame>& frame,
-    base::TimeTicks estimated_capture_time) {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(frame);
-  if (state_ != STARTED)
-    return;
-
-  if (!gpu_memory_buffer_pool_) {
-    FrameReady(frame);
-    return;
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+  if (state == blink::WebMediaStreamSource::ReadyStateEnded) {
+    compositor_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&FrameDelivererOnCompositor::RenderSignalingFrame,
+                              frame_deliverer_->GetWeakPtr()));
   }
-
-  //  |gpu_memory_buffer_pool_| deletion is going to be posted to
-  //  |media_task_runner_|. base::Unretained() usage is fine since
-  //  |gpu_memory_buffer_pool_| outlives the task.
-  media_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(
-          &media::GpuMemoryBufferVideoFramePool::MaybeCreateHardwareFrame,
-          base::Unretained(gpu_memory_buffer_pool_.get()), frame,
-          media::BindToCurrentLoop(
-              base::Bind(&MediaStreamVideoRendererSink::FrameReady,
-                         weak_factory_.GetWeakPtr()))));
-}
-
-void MediaStreamVideoRendererSink::FrameReady(
-    const scoped_refptr<media::VideoFrame>& frame) {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(frame);
-
-  frame_size_ = frame->natural_size();
-  TRACE_EVENT_INSTANT1("media_stream_video_renderer_sink", "FrameReady",
-                       TRACE_EVENT_SCOPE_THREAD, "timestamp",
-                       frame->timestamp().InMilliseconds());
-  repaint_cb_.Run(frame);
-}
-
-void MediaStreamVideoRendererSink::RenderSignalingFrame() {
-  // This is necessary to make sure audio can play if the video tag src is
-  // a MediaStream video track that has been rejected or ended.
-  // It also ensure that the renderer don't hold a reference to a real video
-  // frame if no more frames are provided. This is since there might be a
-  // finite number of available buffers. E.g, video that
-  // originates from a video camera.
-  scoped_refptr<media::VideoFrame> video_frame =
-      media::VideoFrame::CreateBlackFrame(frame_size_);
-  video_frame->metadata()->SetBoolean(media::VideoFrameMetadata::END_OF_STREAM,
-                                      true);
-  video_frame->metadata()->SetTimeTicks(
-      media::VideoFrameMetadata::REFERENCE_TIME, base::TimeTicks::Now());
-  OnVideoFrame(video_frame, base::TimeTicks());
 }
 
 }  // namespace content