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..81ae529a6407861d071a1890efed2e5c4eb98832 100644
--- a/content/renderer/media/media_stream_video_renderer_sink.cc
+++ b/content/renderer/media/media_stream_video_renderer_sink.cc
@@ -5,156 +5,313 @@
 #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 {

[DaleCurtis, 2016/11/15 00:30:16]

Generally we describe where a class runs by it's variable name, not it's class
name (if it all). I think this naming is more confusing than anything, but up to
you. I'd just call this a FrameDeliverer or FrameSender if you want to keep it
inline with the receiver terminology on its pair-wise class.

[emircan, 2016/11/15 19:54:01]

Done.

+ public:
+  FrameDelivererOnCompositor(
+      const RepaintCB repaint_cb,

[DaleCurtis, 2016/11/15 00:30:15]

const&

[emircan, 2016/11/15 19:54:01]

Done.

+      const scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner,

[DaleCurtis, 2016/11/15 00:30:16]

New classes should pass scoped_refptr by value.

[emircan, 2016/11/15 19:54:01]

Done.

+      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) {

[DaleCurtis, 2016/11/15 00:30:16]

There's only one, so not really any reason to call this "for_compositor"

[emircan, 2016/11/15 19:54:01]

Done.

+    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() {

[DaleCurtis, 2016/11/15 00:30:16]

Add blank line.

[emircan, 2016/11/15 19:54:01]

Done.

+    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);
+    TRACE_EVENT_INSTANT1("webrtc",
+                         "MediaStreamVideoRendererSink::"
+                         "FrameDelivererOnCompositor::OnVideoFrame",
+                         TRACE_EVENT_SCOPE_THREAD, "timestamp",
+                         frame->timestamp().InMilliseconds());
+
+    if (state_ != STARTED)
+      return;
+    frame_size_ = frame->natural_size();

[DaleCurtis, 2016/11/15 00:30:16]

Isn't this going to get out of sync with what's actually rendering if you update
it here instead of after the copy completes?

[emircan, 2016/11/15 19:54:01]

Moving it to after copy.

+
+    if (!gpu_memory_buffer_pool_) {
+      repaint_cb_.Run(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) {

[DaleCurtis, 2016/11/15 00:30:16]

Sequeneces of Start/Stop/Start are going to result in this method calling
repaint_cb_ with something from a previous run. Is that okay?

[emircan, 2016/11/15 19:54:01]

I would say that is OK and consistent, considering that the frame would be shown
if |gpu_memory_buffer_pool_| isn't supported. Time spent in GMBVFP would
otherwise be spent during the copies in rendering SW backed frame. Considering
this, the second |state_| check looks inconsistent now and I am removing. WDYT?

[DaleCurtis, 2016/11/15 20:29:03]

Seems fine, but defer to qiangchen@.

+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    DCHECK(frame);
+    TRACE_EVENT_INSTANT1(
+        "webrtc",
+        "MediaStreamVideoRendererSink::FrameDelivererOnCompositor::FrameReady",
+        TRACE_EVENT_SCOPE_THREAD, "timestamp",
+        frame->timestamp().InMilliseconds());
+
+    if (state_ != STARTED)
+      return;
+    repaint_cb_.Run(frame);
+  }
+
+  void RenderSignalingFrame() {

[DaleCurtis, 2016/11/15 00:30:16]

RenderEndOfStream?

[emircan, 2016/11/15 19:54:01]

Done.

+    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 doesn'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)

[DaleCurtis, 2016/11/15 00:30:16]

No unnecessary parens.

[emircan, 2016/11/15 19:54:01]

Done.

+                                : 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());
+  }
+
+  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);

[DaleCurtis, 2016/11/15 00:30:16]

for multiple DCHECKs() like this add a  <<state_ to save yourself some debugging
later.

[emircan, 2016/11/15 19:54:01]

Done.

+    state_ = STOPPED;
+  }
+
+  void Resume() {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    if (state_ == PAUSED)

[DaleCurtis, 2016/11/15 00:30:15]

Instead of conditionals can these be DCHECKs() ?

[emircan, 2016/11/15 19:54:01]

I am not sure. Running "play(); play();" on JS would result in double Resume()
calls. I guess we wouldn't want to crash on second one, but just ignore.

[DaleCurtis, 2016/11/15 20:29:03]

Does calling play() play() not already have an existing state check?

[emircan, 2016/11/16 00:52:24]

It actually does, "load(); play();" causes the problem.

+      state_ = STARTED;
+  }
+
+  void Pause() {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    if (state_ == STARTED)

[DaleCurtis, 2016/11/15 00:30:16]

Ditto?

[emircan, 2016/11/15 19:54:01]

Same as above, "pause(); pause();" would crash if we add.

+      state_ = PAUSED;
+  }
+
+  base::WeakPtr<FrameDelivererOnCompositor> GetWeakPtr() {

[DaleCurtis, 2016/11/15 00:30:15]

This is a bit confusingly named since we also have a SupportsWeakPtr class you
can inherit for this function. Instead of keeping a factory and having the
method just extend SupportsWeakPtr if you really need to vend these externally
(typically this is frowned upon).

[emircan, 2016/11/15 19:54:01]

I switched to AsWeakPtr().

+    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 are executed on the correct thread.
+  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

[DaleCurtis, 2016/11/15 00:30:16]

Ditto on naming.

[emircan, 2016/11/15 19:54:01]

Done.

+// the 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 the IO thread.
+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) {

[DaleCurtis, 2016/11/15 00:30:16]

Ditto on SupportsWeakPtr.

[emircan, 2016/11/15 19:54:01]

Done.

+    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()));

[DaleCurtis, 2016/11/15 00:30:16]

You can't vend WeakPtr's for the compositor from the main thread. WeakPtr's must
be created and dereferenced on the same thread. Once created they can be passed
anywhere though. This and every other instance of GetWeakPtr() from the main
thread is incorrect.

[emircan, 2016/11/15 19:54:01]

I am confused by what you said. WeakPtrs must be dereferenced and invalidated on
the same thread[0]. In this case, both happens on compositor thread. PostTask
dereferences them via WeakPtr::get() and pins them to |compositor_task_runner_|.
~FrameDelivererOnCompositor invalidates them on |compositor_task_runner_|. There
are DCHECKs ensuring that and it passes those DCHECKs currently. AFAIK, there
isn't any restriction on where WeakPtr instances are created and WeakPtr
instances can be handed off to other threads. 

Let me know if I am missing something. Alternative would be making these classes
scoped_refptr's but that makes ownership and lifetime less clear. I tried hard
to avoid using scoped_refptr here. Currently, these classes are unique_ptr's
belonging to MediaStreamVideoRendererSink. WeakPtr's are used to pass references
between threads. 

[0] https://cs.chromium.org/chromium/src/base/memory/weak_ptr.h?rcl=0&l=52

[DaleCurtis, 2016/11/15 20:29:03]

If you create the weak ptr on a different thread than it's used you have a race
in WeakReferenceOwner::GetRef() (see weak_ptr.cc) - HasRefs() and GetRefs() may
race. I could have sworn the WeakPtr documentation used to have a note about not
creating them on threads other than their used on. +wez in case the times have
changed and I've missed something.

+
+  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