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/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.

[perkj_chrome, 2016/11/17 21:54:01]

/s on the compositor thread

[emircan, 2016/11/17 23:03:21]

Done.

+class MediaStreamVideoRendererSink::FrameDeliverer {
+ 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),
+        weak_factory_for_compositor_(this) {
+    compositor_thread_checker_.DetachFromThread();
+    weak_this_ = weak_factory_for_compositor_.GetWeakPtr();
+    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, weak_this_))));
+  }
+
+  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_ = STARTED;
+  }
+
+  void Stop() {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    DCHECK(state_ == STARTED || state_ == PAUSED) << state_;
+    state_ = STOPPED;
+    weak_factory_for_compositor_.InvalidateWeakPtrs();

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

Is there any point InvalidateWeakPtrs()ing here, since you always DeleteSoon()
this object immediately after Stop()?

In fact could you simply move this logic into the FrameDeliverer destructor and
eliminate this Stop() method?

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

Sure, I moved it into the dtor.

+  }
+
+  void Resume() {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    if (state_ == PAUSED)
+      state_ = STARTED;
+  }
+
+  void Pause() {
+    DCHECK(compositor_thread_checker_.CalledOnValidThread());
+    if (state_ == STARTED)
+      state_ = PAUSED;
+  }
+
+  VideoCaptureDeliverFrameCB GetDeliverFrameCallback() {
+    return base::Bind(&FrameDeliverer::OnVideoFrame, weak_this_);
+  }
+
+ 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::WeakPtr<FrameDeliverer> weak_this_;

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

nit: FYI it is strictly safest to put |weak_ptr_| as the first member, so that,
if something happens to call into |this| mid-destructor (e.g. via a callback)
and touches |weak_ptr_|, it is still constructed, albeit null, at that point. 
Shouldn't be a problem for this class, though, just an FYI.

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

Thanks, it is a very helpful tip.

+  base::WeakPtrFactory<FrameDeliverer> weak_factory_for_compositor_;

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

This can just be |weak_factory_|, since this class is used exclusively on the
compositor thread - even if it were used on multiple threads, you couldn't have
separate WeakPtrFactorys for each without causing yourself much horror. :P

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

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(FrameDeliverer);
+};
+
+// FrameReceiver is responsible for trampolining frames from IO thread to
+// the compositor thread by posting a task on |deliver_frame_cb_|.
+//
+// It is created on main thread, but methods should be called and class should
+// be destructed on the IO thread.
+class MediaStreamVideoRendererSink::FrameReceiver {
+ public:
+  FrameReceiver(
+      const scoped_refptr<base::SingleThreadTaskRunner>& compositor_task_runner,
+      const VideoCaptureDeliverFrameCB& deliver_frame_cb)
+      : compositor_task_runner_(compositor_task_runner),
+        deliver_frame_cb_(deliver_frame_cb) {
+    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(deliver_frame_cb_, frame, current_time));
+  }
+
+ private:
+  const scoped_refptr<base::SingleThreadTaskRunner> compositor_task_runner_;
+  const VideoCaptureDeliverFrameCB deliver_frame_cb_;
+
+  // 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),
+      compositor_task_runner_(compositor_task_runner),
       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));
-  }
-}
-
-MediaStreamVideoRendererSink::~MediaStreamVideoRendererSink() {
-  if (gpu_memory_buffer_pool_) {
-    media_task_runner_->DeleteSoon(FROM_HERE,
-                                   gpu_memory_buffer_pool_.release());
-  }
-}
+      worker_task_runner_(worker_task_runner),
+      gpu_factories_(gpu_factories) {}
+
+MediaStreamVideoRendererSink::~MediaStreamVideoRendererSink() {}
 
 void MediaStreamVideoRendererSink::Start() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK_EQ(state_, STOPPED);
-
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+
+  frame_deliverer_.reset(new MediaStreamVideoRendererSink::FrameDeliverer(
+      repaint_cb_, media_task_runner_, worker_task_runner_, gpu_factories_));
+  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_->GetDeliverFrameCallback()));
   MediaStreamVideoSink::ConnectToTrack(
-      video_track_, media::BindToCurrentLoop(
-                        base::Bind(&MediaStreamVideoRendererSink::OnVideoFrame,
-                                   weak_factory_.GetWeakPtr())),
+      video_track_,
+      // This callback is run on IO thread. It is safe to use base::Unretained
+      // here because |frame_receiver_| will be destroyed after sink is
+      // disconnected from track.
+      base::Bind(&FrameReceiver::OnVideoFrame,
+                 base::Unretained(frame_receiver_.get())),
       // 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_) {
+    GetIOTaskRunner()->DeleteSoon(FROM_HERE, frame_receiver_.release());

[perkj_chrome, 2016/11/17 21:54:01]

Is Stop always called? Othertwise frame_receiver will be deleted on the wrong
thread.

[emircan, 2016/11/17 23:03:21]

Yes, see
https://cs.chromium.org/chromium/src/content/renderer/media/webmediaplayer_ms....

+  }
+  if (frame_deliverer_) {
+    compositor_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&FrameDeliverer::Stop,
+                              base::Unretained(frame_deliverer_.get())));
+    compositor_task_runner_->DeleteSoon(FROM_HERE, frame_deliverer_.release());
+  }
 }
 
 void MediaStreamVideoRendererSink::Resume() {
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  if (state_ == PAUSED)
-    state_ = STARTED;
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+  if (!frame_deliverer_)
+    return;
+
+  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());
+  if (!frame_deliverer_)
+    return;
+
+  compositor_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&FrameDeliverer::Pause,
+                            base::Unretained(frame_deliverer_.get())));
+}
+
+void MediaStreamVideoRendererSink::OnReadyStateChanged(
+    blink::WebMediaStreamSource::ReadyState state) {
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+  if (state == blink::WebMediaStreamSource::ReadyStateEnded &&
+      frame_deliverer_) {
+    compositor_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&FrameDeliverer::RenderEndOfStream,
+                              base::Unretained(frame_deliverer_.get())));
+  }
+}
+
+scoped_refptr<base::SingleThreadTaskRunner>
+MediaStreamVideoRendererSink::GetIOTaskRunner() {
+  return ChildProcess::current()->io_task_runner();
+}
+
+MediaStreamVideoRendererSink::State
+MediaStreamVideoRendererSink::GetStateForTesting() {
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+  if (!frame_deliverer_)
+    return STOPPED;
+  return frame_deliverer_->state_;
 }
 
 void MediaStreamVideoRendererSink::SetGpuMemoryBufferVideoForTesting(
     media::GpuMemoryBufferVideoFramePool* gpu_memory_buffer_pool) {
-  gpu_memory_buffer_pool_.reset(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());
+  CHECK(frame_deliverer_);
+  compositor_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&FrameDeliverer::SetGpuMemoryBufferVideoForTesting,
+                            base::Unretained(frame_deliverer_.get()),
+                            gpu_memory_buffer_pool));
 }
 
 }  // namespace content