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..c7ab988e1a5760d8379e389f8bc4e0a765be1cbf 100644
--- a/content/renderer/media/media_stream_video_renderer_sink.cc
+++ b/content/renderer/media/media_stream_video_renderer_sink.cc
@@ -5,156 +5,297 @@
 #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 {
 
+// FrameDeliverer 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::FrameDeliverer

[Wez, 2016/11/16 01:47:10]

This naming is rather confusing; MediaStreamVideoSink already has a
FrameDeliverer sub-class.

[emircan, 2016/11/17 01:23:25]

Are you referring to MediaStreamVideoTrack::FrameDeliverer?[0] It is used for a 
similar purpose, so I wanted to keep the naming similar for these inner classes.
Do you have any suggestions?

[0]
https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_vide...

+    : public base::SupportsWeakPtr<FrameDeliverer> {

[Wez, 2016/11/16 01:47:10]

Please use WeakPtrFactory rather than SupportsWeakPtr. Thanks!

[emircan, 2016/11/17 01:23:25]

I had WeakPtrFactory, but switched to SupportsWeakPtr after comments on #54.
Reverting now.

[DaleCurtis, 2016/11/17 02:13:17]

Wez, I'm surprised by this advice. If you have a class with a public
GetWeakPtr() method, why go through the fuss of a factory and getter when it's
provided by SupportsWeakPtr?

[Wez, 2016/11/17 02:30:27]

Sorry, I meant to add the suggestion of instead having a getter on
FrameDeliverer, that returns Bind(&FrameDeliverer::OnVideoFrame,
weak_factory_.GetWeakPtr()) that the calling class would use to get a callback
that it can pass to the FrameReceiver, so you can avoid AsWeakPtr() entirely.

FWIW the two main arguments against SupportsWeakPtr are (1) it forces you to
expose AsWeakPtr(), risking later changes using WeakPtrs in ways you didn't
intend, and (2) it is a base-class, so doesn't invalidate WeakPtrs until *after*
the sub-class members have torn-down (only a problem if callbacks might be
invoked mid-destructor, though).

In this case the Deliverer and Receiver are implementation details, so (1) is
less of an issue.

[emircan, 2016/11/17 19:43:46]

Done.

+ public:
+  FrameDeliverer(const RepaintCB& repaint_cb,
+                 scoped_refptr<base::SingleThreadTaskRunner> media_task_runner,
+                 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) {
+    compositor_thread_checker_.DetachFromThread();

[Wez, 2016/11/16 01:47:10]

Is the compositor thread one of the ones passed in as a task-runner, by any
chance?

[emircan, 2016/11/17 01:23:25]

No. However, I can pass it in ctor and change thread checkers into
|compositor_task_runner_.BelongsToCurrentThread()| if that is what you are
suggesting.

+    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));
+    }
+  }
+
+  ~FrameDeliverer() {
+    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::"
+                         "FrameDeliverer::OnVideoFrame",
+                         TRACE_EVENT_SCOPE_THREAD, "timestamp",
+                         frame->timestamp().InMilliseconds());
+
+    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(&FrameDeliverer::FrameReady, AsWeakPtr()))));
+  }
+
+  void FrameReady(const scoped_refptr<media::VideoFrame>& frame) {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    DCHECK(frame);
+    TRACE_EVENT_INSTANT1(
+        "webrtc", "MediaStreamVideoRendererSink::FrameDeliverer::FrameReady",
+        TRACE_EVENT_SCOPE_THREAD, "timestamp",
+        frame->timestamp().InMilliseconds());
+
+    frame_size_ = frame->natural_size();
+    repaint_cb_.Run(frame);
+  }
+
+  void RenderEndOfStream() {
+    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)
+                              : 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_;

[Wez, 2016/11/16 01:47:10]

Do you need << state_ here?  I thought DCHECK_EQ() logged both values on
failure?

[emircan, 2016/11/17 01:23:25]

Done.

+    state_ = STARTED;
+  }
+
+  void Stop() {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    DCHECK(state_ == STARTED || state_ == PAUSED) << state_;
+    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;
+  }
+
+ 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_;
+
+  DISALLOW_COPY_AND_ASSIGN(FrameDeliverer);
+};
+
+// FrameReceiver is responsible for trampolining frames from IO thread to
+// 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::FrameReceiver
+    : public base::SupportsWeakPtr<FrameReceiver> {
+ public:
+  FrameReceiver(
+      const scoped_refptr<base::SingleThreadTaskRunner>& compositor_task_runner,
+      const base::WeakPtr<FrameDeliverer>& deliverer)
+      : compositor_task_runner_(compositor_task_runner), deliverer_(deliverer) {
+    io_thread_checker_.DetachFromThread();
+  }
+
+  ~FrameReceiver() { 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::FrameDeliverer::OnVideoFrame,
+                   deliverer_, frame, current_time));
+  }
+
+ private:
+  const scoped_refptr<base::SingleThreadTaskRunner> compositor_task_runner_;
+  const base::WeakPtr<FrameDeliverer> deliverer_;
+
+  // Used for DCHECKs to ensure method calls executed in the correct thread.
+  base::ThreadChecker io_thread_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(FrameReceiver);
+};
+
 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::FrameDeliverer(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(&FrameDeliverer::Start,
+                            base::Unretained(frame_deliverer_.get())));
+
+  frame_receiver_.reset(new MediaStreamVideoRendererSink::FrameReceiver(
+      compositor_task_runner_, frame_deliverer_->AsWeakPtr()));

[Wez, 2016/11/16 01:47:10]

Bear in mind that FrameReceiver's scope is Start->Stop, whereas the
FrameDeliverer's scope is the lifetime of the Sink - you may have an
OnVideoFrame task in-flight to the FD when you call Stop(), and that will still
be processed and result in a call to |repaint_cb_|.

If you have a rapid Start()->Stop()->Start() cycle then that means you may get a
|repaint_cb_| after the second Start(), but with frame content from between the
first Start()->Stop().

Is that a problem?

[emircan, 2016/11/17 01:23:25]

Yes, it looks like a problem, i.e. OnVideoFrame() is scheduled after
RenderEndOfStream(). Thanks for catching this. 

I couldn't think of another solution then calling InvalidateWeakPtrs in
FrameDeliverer::Stop(). Then, we would need to make sure that FrameReceiver has
a valid WeakPtr. To make things clear, I synched both classes' lifetimes. WDYT?

[Wez, 2016/11/18 02:26:56]

If having the two classes' lifetimes scoped to Start/Stop works for this impl
then yes, that seems a good solution to the problem (plus as a minor bonus it
avoids the FrameDeliverer sitting around idle for no good reason :)

   MediaStreamVideoSink::ConnectToTrack(
-      video_track_, media::BindToCurrentLoop(
-                        base::Bind(&MediaStreamVideoRendererSink::OnVideoFrame,
-                                   weak_factory_.GetWeakPtr())),
+      video_track_,
+      base::Bind(&FrameReceiver::OnVideoFrame, frame_receiver_->AsWeakPtr()),

[Wez, 2016/11/16 01:47:10]

nit: It is not obvious from the context that ConnectToTrack actually causes the
supplied callback to be called on the IO thread; may be worth mentioning here
with a comment, for clarity.

Also, since MediaStreamVideoSink will invoke the callback on the IO thread, and
the FrameReceiver is DeleteSoon()d to the IO thread, could you use RemoveSink()
in Stop(), and then you can use base::Unretained() for the receiver and avoid
exposing WeakPtr?

[emircan, 2016/11/17 01:23:25]

Added the comment.
Thanks for pointing this out. It makes sense now that it will be synchronized on
IO thread, i.e. DeliverFrameOnIO()->RemoveCallbackOnIO()->~FrameReceiver()
sequence. 
Note that I similarly removed WebMediaPlayerMSCompositor::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(&FrameDeliverer::RenderEndOfStream,
+                              base::Unretained(frame_deliverer_.get())));
   }
 }
 
 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(

[Wez, 2016/11/16 01:47:10]

Rather than getting at the IO task runner for this process this way, I'd suggest
either (1) wrapping the logic in a private io_task_runner() getter on this class
or (2) passing it in explicitly from the caller, to the constructor.

[emircan, 2016/11/17 01:23:25]

I tried to do (1). Note that this is currently how IO thread is acquired in
source.
https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_vide...

[Wez, 2016/11/18 02:26:56]

OK; if there is a //src/media/ convention for that approach then feel free to
follow it rather than defining a helper getter. :)

[emircan, 2016/11/18 21:14:18]

Done.

+        FROM_HERE, frame_receiver_.release());
+  }
+  compositor_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&FrameDeliverer::Stop,
+                            base::Unretained(frame_deliverer_.get())));
 }
 
 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(&FrameDeliverer::Resume,
+                            base::Unretained(frame_deliverer_.get())));
 }
 
 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(&FrameDeliverer::Pause,
+                            base::Unretained(frame_deliverer_.get())));
+}
+
+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(&FrameDeliverer::SetGpuMemoryBufferVideoForTesting,
+                            base::Unretained(frame_deliverer_.get()),
+                            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(&FrameDeliverer::RenderEndOfStream,
+                              base::Unretained(frame_deliverer_.get())));
   }
-
-  //  |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