Make painting a single frame a permanent API on VideoRendererSink.

Having to call VRS::Start(), wait for VRS::RC::Render(), and then queuing a VRS::Stop() across multiple threads just to paint a single frame is not desirable... so remove a bunch of TODOs and rename PaintFrameUsingOldRenderingPath() to PaintSingleFrame(). This allows a lot of simplification in VideoRendererImpl and avoids Chromecast from having to pick up this complexity. It also will likely resolve nullptr crashes due having two sink startup points. BUG=614099, 611714 TEST=existing unittests pass. CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Review-Url: https://codereview.chromium.org/1996763002 Cr-Commit-Position: refs/heads/master@{#395138} (cherry picked from commit e9c89e9) Review URL: https://codereview.chromium.org/2039443003 . Cr-Commit-Position: refs/branch-heads/2743@{#215} Cr-Branched-From: 2b3ae3b-refs/heads/master@{#394939}
crosswalk-project · Jun 3, 2016 · 6fd80e7 · 6fd80e7
1 parent 99c95d8
commit 6fd80e7
Show file tree

Hide file tree

Showing 17 changed files with 151 additions and 214 deletions.
diff --git a/cc/layers/video_frame_provider.h b/cc/layers/video_frame_provider.h
@@ -43,8 +43,6 @@ class CC_EXPORT VideoFrameProvider {
     virtual void StopRendering() = 0;
 
     // Notifies the client that GetCurrentFrame() will return new data.
-    // TODO(dalecurtis): Nuke this once VideoFrameProviderClientImpl is using a
-    // BeginFrameObserver based approach. http://crbug.com/336733
     virtual void DidReceiveFrame() = 0;
 
    protected:
@@ -75,9 +73,6 @@ class CC_EXPORT VideoFrameProvider {
   //
   // Clients should call this in response to UpdateCurrentFrame() returning true
   // or in response to a DidReceiveFrame() call.
-  //
-  // TODO(dalecurtis): Remove text about DidReceiveFrame() once the old path
-  // has been removed. http://crbug.com/439548
   virtual scoped_refptr<media::VideoFrame> GetCurrentFrame() = 0;
 
   // Called in response to DidReceiveFrame() or a return value of true from

diff --git a/chromecast/renderer/media/cma_renderer.cc b/chromecast/renderer/media/cma_renderer.cc
@@ -378,7 +378,7 @@ void CmaRenderer::OnStatisticsUpdated(
 
 void CmaRenderer::OnNaturalSizeChanged(const gfx::Size& size) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  video_renderer_sink_->PaintFrameUsingOldRenderingPath(
+  video_renderer_sink_->PaintSingleFrame(
       video_overlay_factory_->CreateFrame(size));
   client_->OnVideoNaturalSizeChange(size);
 }

diff --git a/media/base/null_video_sink.cc b/media/base/null_video_sink.cc
@@ -88,8 +88,7 @@ void NullVideoSink::CallRender() {
                                 delay);
 }
 
-void NullVideoSink::PaintFrameUsingOldRenderingPath(
-    const scoped_refptr<VideoFrame>& frame) {
+void NullVideoSink::PaintSingleFrame(const scoped_refptr<VideoFrame>& frame) {
   new_frame_cb_.Run(frame);
 }
 

diff --git a/media/base/null_video_sink.h b/media/base/null_video_sink.h
@@ -35,8 +35,7 @@ class MEDIA_EXPORT NullVideoSink : public VideoRendererSink {
   // VideoRendererSink implementation.
   void Start(RenderCallback* callback) override;
   void Stop() override;
-  void PaintFrameUsingOldRenderingPath(
-      const scoped_refptr<VideoFrame>& frame) override;
+  void PaintSingleFrame(const scoped_refptr<VideoFrame>& frame) override;
 
   void set_tick_clock_for_testing(base::TickClock* tick_clock) {
     tick_clock_ = tick_clock;

diff --git a/media/base/null_video_sink_unittest.cc b/media/base/null_video_sink_unittest.cc
@@ -74,7 +74,7 @@ TEST_F(NullVideoSinkTest, BasicFunctionality) {
 
   // The sink shouldn't have to be started to use the paint method.
   EXPECT_CALL(*this, FrameReceived(test_frame));
-  sink->PaintFrameUsingOldRenderingPath(test_frame);
+  sink->PaintSingleFrame(test_frame);
 
   {
     SCOPED_TRACE("Waiting for sink startup.");

diff --git a/media/base/video_renderer_sink.h b/media/base/video_renderer_sink.h
@@ -53,16 +53,12 @@ class MEDIA_EXPORT VideoRendererSink {
   // liberally if no new frames are expected.
   virtual void Stop() = 0;
 
-  // Instead of using a callback driven rendering path, allow clients to paint
-  // frames as they see fit without regard for the compositor.
-  // TODO(dalecurtis): This should be nuked once experiments show the new path
-  // is amazing and the old path is not! http://crbug.com/439548
-  virtual void PaintFrameUsingOldRenderingPath(
-      const scoped_refptr<VideoFrame>& frame) = 0;
-
-  // TODO(dalecurtis): We may need OnSizeChanged() and OnOpacityChanged()
-  // methods on this interface if background rendering is handled inside of
-  // the media layer instead of by VideoFrameCompositor.
+  // Instead of using a callback driven rendering path, allow clients to paint a
+  // single frame as they see fit without regard for the compositor; this is
+  // useful for painting poster images or hole frames without having to issue a
+  // Start() -> Render() -> Stop(). Clients are free to mix usage of Render()
+  // based painting and PaintSingleFrame().
+  virtual void PaintSingleFrame(const scoped_refptr<VideoFrame>& frame) = 0;
 
   virtual ~VideoRendererSink() {}
 };

diff --git a/media/blink/video_frame_compositor.cc b/media/blink/video_frame_compositor.cc
@@ -131,13 +131,12 @@ void VideoFrameCompositor::Stop() {
                             base::Unretained(this), false));
 }
 
-void VideoFrameCompositor::PaintFrameUsingOldRenderingPath(
+void VideoFrameCompositor::PaintSingleFrame(
     const scoped_refptr<VideoFrame>& frame) {
   if (!compositor_task_runner_->BelongsToCurrentThread()) {
     compositor_task_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(&VideoFrameCompositor::PaintFrameUsingOldRenderingPath,
-                   base::Unretained(this), frame));
+        FROM_HERE, base::Bind(&VideoFrameCompositor::PaintSingleFrame,
+                              base::Unretained(this), frame));
     return;
   }
 
@@ -172,7 +171,7 @@ base::TimeDelta VideoFrameCompositor::GetCurrentFrameTimestamp() const {
   // When the VFC is stopped, |callback_| is cleared; this synchronously
   // prevents CallRender() from invoking ProcessNewFrame(), and so
   // |current_frame_| won't change again until after Start(). (Assuming that
-  // PaintFrameUsingOldRenderingPath() is not also called while stopped.)
+  // PaintSingleFrame() is not also called while stopped.)
   if (!current_frame_)
     return base::TimeDelta();
   return current_frame_->timestamp();

diff --git a/media/blink/video_frame_compositor.h b/media/blink/video_frame_compositor.h
@@ -75,8 +75,7 @@ class MEDIA_BLINK_EXPORT VideoFrameCompositor
   // same thread (typically the media thread).
   void Start(RenderCallback* callback) override;
   void Stop() override;
-  void PaintFrameUsingOldRenderingPath(
-      const scoped_refptr<VideoFrame>& frame) override;
+  void PaintSingleFrame(const scoped_refptr<VideoFrame>& frame) override;
 
   // Returns |current_frame_| if |client_| is set.  If no |client_| is set,
   // |is_background_rendering_| is true, and |callback_| is set, it requests a
@@ -94,7 +93,7 @@ class MEDIA_BLINK_EXPORT VideoFrameCompositor
   // Returns the timestamp of the current (possibly stale) frame, or
   // base::TimeDelta() if there is no current frame. This method may be called
   // from the media thread as long as the VFC is stopped. (Assuming that
-  // PaintFrameUsingOldRenderingPath() is not also called while stopped.)
+  // PaintSingleFrame() is not also called while stopped.)
   base::TimeDelta GetCurrentFrameTimestamp() const;
 
   void set_tick_clock_for_testing(std::unique_ptr<base::TickClock> tick_clock) {

diff --git a/media/blink/video_frame_compositor_unittest.cc b/media/blink/video_frame_compositor_unittest.cc
@@ -102,12 +102,12 @@ TEST_F(VideoFrameCompositorTest, InitialValues) {
   EXPECT_FALSE(compositor()->GetCurrentFrame().get());
 }
 
-TEST_F(VideoFrameCompositorTest, PaintFrameUsingOldRenderingPath) {
+TEST_F(VideoFrameCompositorTest, PaintSingleFrame) {
   scoped_refptr<VideoFrame> expected = VideoFrame::CreateEOSFrame();
 
   // Should notify compositor synchronously.
   EXPECT_EQ(0, did_receive_frame_count());
-  compositor()->PaintFrameUsingOldRenderingPath(expected);
+  compositor()->PaintSingleFrame(expected);
   scoped_refptr<VideoFrame> actual = compositor()->GetCurrentFrame();
   EXPECT_EQ(expected, actual);
   EXPECT_EQ(1, did_receive_frame_count());

diff --git a/media/blink/webmediaplayer_impl.cc b/media/blink/webmediaplayer_impl.cc
@@ -913,7 +913,7 @@ void WebMediaPlayerImpl::OnPipelineSuspended() {
   if (isRemote()) {
     scoped_refptr<VideoFrame> frame = cast_impl_.GetCastingBanner();
     if (frame)
-      compositor_->PaintFrameUsingOldRenderingPath(frame);
+      compositor_->PaintSingleFrame(frame);
   }
 #endif
 
@@ -1199,7 +1199,7 @@ void WebMediaPlayerImpl::SuspendForRemote() {
   if (pipeline_controller_.IsPipelineSuspended()) {
     scoped_refptr<VideoFrame> frame = cast_impl_.GetCastingBanner();
     if (frame)
-      compositor_->PaintFrameUsingOldRenderingPath(frame);
+      compositor_->PaintSingleFrame(frame);
   }
 
   UpdatePlayState();

diff --git a/media/filters/video_renderer_algorithm.h b/media/filters/video_renderer_algorithm.h
@@ -136,6 +136,8 @@ class MEDIA_EXPORT VideoRendererAlgorithm {
 
   size_t frames_queued() const { return frame_queue_.size(); }
 
+  scoped_refptr<VideoFrame> first_frame() { return frame_queue_.front().frame; }
+
   // Returns the average of the duration of all frames in |frame_queue_|
   // as measured in wall clock (not media) time.
   base::TimeDelta average_frame_duration() const {

diff --git a/media/mojo/services/mojo_renderer_impl.cc b/media/mojo/services/mojo_renderer_impl.cc
@@ -190,7 +190,7 @@ void MojoRendererImpl::OnVideoNaturalSizeChange(mojo::SizePtr size) {
   DVLOG(2) << __FUNCTION__ << ": " << new_size.ToString();
   DCHECK(task_runner_->BelongsToCurrentThread());
 
-  video_renderer_sink_->PaintFrameUsingOldRenderingPath(
+  video_renderer_sink_->PaintSingleFrame(
       video_overlay_factory_->CreateFrame(new_size));
   client_->OnVideoNaturalSizeChange(new_size);
 }