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

content/renderer/media/media_stream_video_renderer_sink.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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/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);
+
+    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
+    // 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 in the correct thread.

[perkj_chrome, 2016/11/09 15:27:13]

s/ calls are executed ... on the correct

[emircan, 2016/11/11 22:27:19]

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.
+//
+// It is created on main thread, but methods should be called and class should
+// be destructed on IO thread.
+class MediaStreamVideoRendererSink::FrameReceiverOnIO {

[qiangchen, 2016/11/09 23:23:48]

Is it possible to remove this class? I mean just implement OnVideoFrame in
MediaStreamVideoRendererSink.

[emircan, 2016/11/11 22:27:19]

I think that would make the ownership and threading less clear. 
We would provide a callback to MediaStreamVideoRendererSink::OnVideoFrame when
calling MediaStreamVideoSink::ConnectToTrack then. MediaStreamVideoRendererSink
would receive frames on IO thread. If we were to provide
MediaStreamVideoRendererSink as a weak_ptr, we would have to make sure that the
weak pointers are invalidated on IO thread before dtor is run. Since, dtor is
run on main thread, that would require synchronization. Pinning
MediaStreamVideoRendererSink to only one thread makes it much easier to
maintain. 

[qiangchen, 2016/11/15 00:04:53]

Acknowledged.

+ 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(
+        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_) {
+    RenderThread* const render_thread = RenderThread::Get();
+    if (render_thread) {
+      render_thread->GetIOTaskRunner()->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::OnVideoFrameForTesting(

[perkj_chrome, 2016/11/09 15:27:13]

please remove. Is it possible to create a fake video track instead where you
inject frames. 

[emircan, 2016/11/11 22:27:19]

This is how test have been injecting the frames before my CL as well, see
MediaStreamVideoRendererSinkTest::OnVideoFrame.
But I agree with you that this is not ideal. I am removing this and injecting
frames via MockMediaStreamVideoSource. 

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

thanks

+    const scoped_refptr<media::VideoFrame>& frame,
+    base::TimeTicks current_time) {
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+  compositor_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&FrameDelivererOnCompositor::OnVideoFrame,
+                 frame_deliverer_->GetWeakPtr(), frame, current_time));
 }
 
 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;
-  }
-
-  //  |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());
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+  if (state == blink::WebMediaStreamSource::ReadyStateEnded) {
+    compositor_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&FrameDelivererOnCompositor::RenderSignalingFrame,
+                              frame_deliverer_->GetWeakPtr()));
+  }
 }
 
 }  // namespace content