Avoid repeated unnecessary task posting where possible.

Adds a delay before MaybeStopSinkAfterFirstPaint() is called to
allow autoplaying videos time to actually play. Avoids reposting
that task if multiple Render() calls come in succession.

Avoids calling AttemptRead() if we know it's going to fail. In
total this seems to reduce CPU% by 10% (120% -> 110%) and
energy_consumption_mwh is reduced by 1.6mwh for a 24fps 4K video.

Also cleans up ended handling, it was previously possible for
ended to fire during |render_first_frame_and_stop_| if time
moved forward too much. The delayed stop now exposes this in
the unit tests.

BUG=439548
TEST=tests pass

Review URL: https://codereview.chromium.org/1129603003

Cr-Commit-Position: refs/heads/master@{#328649}

3 files changed, 65 insertions, 38 deletions

diff --git a/media/renderers/video_renderer_impl.cc b/media/renderers/video_renderer_impl.cc
index 3da6ad4..e73b878 100644
--- a/media/renderers/video_renderer_impl.cc
+++ b/media/renderers/video_renderer_impl.cc
@@ -52,6 +52,7 @@ VideoRendererImpl::VideoRendererImpl(
       was_background_rendering_(false),
       time_progressing_(false),
       render_first_frame_and_stop_(false),
+      posted_maybe_stop_after_first_paint_(false),
       weak_factory_(this) {
 }
 
@@ -142,6 +143,7 @@ void VideoRendererImpl::Initialize(
   DCHECK(!wall_clock_time_cb.is_null());
   DCHECK_EQ(kUninitialized, state_);
   DCHECK(!render_first_frame_and_stop_);
+  DCHECK(!posted_maybe_stop_after_first_paint_);
   DCHECK(!was_background_rendering_);
   DCHECK(!time_progressing_);
 
@@ -208,20 +210,28 @@ scoped_refptr<VideoFrame> VideoRendererImpl::Render(
   UpdateStatsAndWait_Locked(base::TimeDelta());
   was_background_rendering_ = background_rendering;
 
-  // After painting the first frame, if playback hasn't started, we request that
-  // the sink be stopped.  OnTimeStateChanged() will clear this flag if time
-  // starts before we get here and MaybeStopSinkAfterFirstPaint() will ignore
-  // this request if time starts before the call executes.
-  if (render_first_frame_and_stop_) {
-    task_runner_->PostTask(
+  // After painting the first frame, if playback hasn't started, we post a
+  // delayed task to request that the sink be stopped.  The task is delayed to
+  // give videos with autoplay time to start.
+  //
+  // OnTimeStateChanged() will clear this flag if time starts before we get here
+  // and MaybeStopSinkAfterFirstPaint() will ignore this request if time starts
+  // before the call executes.
+  if (render_first_frame_and_stop_ && !posted_maybe_stop_after_first_paint_) {
+    posted_maybe_stop_after_first_paint_ = true;
+    task_runner_->PostDelayedTask(
         FROM_HERE, base::Bind(&VideoRendererImpl::MaybeStopSinkAfterFirstPaint,
-                              weak_factory_.GetWeakPtr()));
+                              weak_factory_.GetWeakPtr()),
+        base::TimeDelta::FromMilliseconds(250));
   }
 
-  // Always post this task, it will acquire new frames if necessary, reset the
-  // background rendering timer, and more.
-  task_runner_->PostTask(FROM_HERE, base::Bind(&VideoRendererImpl::AttemptRead,
-                                               weak_factory_.GetWeakPtr()));
+  // To avoid unnecessary work, only post this task if there is a chance of work
+  // to be done.  AttemptRead() may still ignore this call for other reasons.
+  if (!rendered_end_of_stream_ && !HaveReachedBufferingCap(effective_frames)) {
+    task_runner_->PostTask(FROM_HERE,
+                           base::Bind(&VideoRendererImpl::AttemptRead,
+                                      weak_factory_.GetWeakPtr()));
+  }
 
   return result;
 }
@@ -460,13 +470,9 @@ void VideoRendererImpl::FrameReady(VideoFrameStream::Status status,
       DCHECK(!received_end_of_stream_);
       received_end_of_stream_ = true;
 
-      // See if we can fire EOS immediately instead of waiting for Render() or
-      // to tick.  We also want to fire EOS if we have no frames and received
-      // EOS.
-      if (use_new_video_renderering_path_ &&
-          (time_progressing_ || !algorithm_->frames_queued())) {
+      // See if we can fire EOS immediately instead of waiting for Render().
+      if (use_new_video_renderering_path_)
         MaybeFireEndedCallback();
-      }
     } else {
       // Maintain the latest frame decoded so the correct frame is displayed
       // after prerolling has completed.
@@ -486,6 +492,7 @@ void VideoRendererImpl::FrameReady(VideoFrameStream::Status status,
           !rendered_end_of_stream_) {
         start_sink = true;
         render_first_frame_and_stop_ = true;
+        posted_maybe_stop_after_first_paint_ = false;
       }
     }
 
@@ -660,12 +667,17 @@ void VideoRendererImpl::MaybeStopSinkAfterFirstPaint() {
 
 bool VideoRendererImpl::HaveReachedBufferingCap() {
   DCHECK(task_runner_->BelongsToCurrentThread());
+  return HaveReachedBufferingCap(use_new_video_renderering_path_
+                                     ? algorithm_->EffectiveFramesQueued()
+                                     : 0);
+}
+
+bool VideoRendererImpl::HaveReachedBufferingCap(size_t effective_frames) {
   if (use_new_video_renderering_path_) {
     // When the display rate is less than the frame rate, the effective frames
     // queued may be much smaller than the actual number of frames queued.  Here
     // we ensure that frames_queued() doesn't get excessive.
-    return algorithm_->EffectiveFramesQueued() >=
-               static_cast<size_t>(limits::kMaxVideoFrames) ||
+    return effective_frames >= static_cast<size_t>(limits::kMaxVideoFrames) ||
            algorithm_->frames_queued() >=
                static_cast<size_t>(3 * limits::kMaxVideoFrames);
   }
@@ -694,10 +706,18 @@ size_t VideoRendererImpl::MaybeFireEndedCallback() {
   // to a single frame.
   const size_t effective_frames = algorithm_->EffectiveFramesQueued();
 
-  if ((!effective_frames ||
-       (algorithm_->frames_queued() == 1u &&
-        algorithm_->average_frame_duration() == base::TimeDelta())) &&
-      received_end_of_stream_ && !rendered_end_of_stream_) {
+  // Don't fire ended if we haven't received EOS or have already done so.
+  if (!received_end_of_stream_ || rendered_end_of_stream_)
+    return effective_frames;
+
+  // Don't fire ended if time isn't moving and we have frames.
+  if (!time_progressing_ && algorithm_->frames_queued())
+    return effective_frames;
+
+  // Fire ended if we have no more effective frames or only ever had one frame.
+  if (!effective_frames ||
+      (algorithm_->frames_queued() == 1u &&
+       algorithm_->average_frame_duration() == base::TimeDelta())) {
     rendered_end_of_stream_ = true;
     task_runner_->PostTask(FROM_HERE, ended_cb_);
   }
diff --git a/media/renderers/video_renderer_impl.h b/media/renderers/video_renderer_impl.h
index 8f765f6..d174c8a 100644
--- a/media/renderers/video_renderer_impl.h
+++ b/media/renderers/video_renderer_impl.h
@@ -135,8 +135,11 @@ class MEDIA_EXPORT VideoRendererImpl
   // false it Stop() on |sink_|.
   void MaybeStopSinkAfterFirstPaint();
 
-  // Returns true if there is no more room for additional buffered frames.
+  // Returns true if there is no more room for additional buffered frames.  The
+  // overloaded method is the same, but allows skipping an internal call to
+  // EffectiveFramesQueued() if that value is already known.
   bool HaveReachedBufferingCap();
+  bool HaveReachedBufferingCap(size_t effective_frames);
 
   // Starts or stops |sink_| respectively. Do not call while |lock_| is held.
   void StartSink();
@@ -266,8 +269,10 @@ class MEDIA_EXPORT VideoRendererImpl
   bool time_progressing_;
 
   // Indicates that Render() should only render the first frame and then request
-  // that the sink be stopped.
+  // that the sink be stopped.  |posted_maybe_stop_after_first_paint_| is used
+  // to avoid repeated task posts.
   bool render_first_frame_and_stop_;
+  bool posted_maybe_stop_after_first_paint_;
 
   // NOTE: Weak pointers must be invalidated before all other member variables.
   base::WeakPtrFactory<VideoRendererImpl> weak_factory_;
diff --git a/media/renderers/video_renderer_impl_unittest.cc b/media/renderers/video_renderer_impl_unittest.cc
index 7f5c8d9..b139c30 100644
--- a/media/renderers/video_renderer_impl_unittest.cc
+++ b/media/renderers/video_renderer_impl_unittest.cc
@@ -54,10 +54,8 @@ class VideoRendererImplTest : public testing::TestWithParam<bool> {
     ScopedVector<VideoDecoder> decoders;
     decoders.push_back(decoder_);
 
-    // Since the Underflow test needs a render interval shorter than the frame
-    // duration, use 120Hz (which makes each interval is < 10ms; ~9.9ms).
     null_video_sink_.reset(new NullVideoSink(
-        false, base::TimeDelta::FromSecondsD(1.0 / 120),
+        false, base::TimeDelta::FromSecondsD(1.0 / 60),
         base::Bind(&MockCB::FrameReceived, base::Unretained(&mock_cb_)),
         message_loop_.task_runner()));
 
@@ -525,7 +523,7 @@ TEST_P(VideoRendererImplTest, VideoDecoder_InitFailure) {
 
 TEST_P(VideoRendererImplTest, Underflow) {
   Initialize();
-  QueueFrames("0 10 20 30");
+  QueueFrames("0 30 60 90");
 
   {
     WaitableMessageLoopEvent event;
@@ -544,13 +542,15 @@ TEST_P(VideoRendererImplTest, Underflow) {
   {
     SCOPED_TRACE("Waiting for frame drops");
     WaitableMessageLoopEvent event;
-    EXPECT_CALL(mock_cb_, FrameReceived(HasTimestamp(10)))
+    EXPECT_CALL(mock_cb_, FrameReceived(HasTimestamp(30)))
         .Times(0);
-    EXPECT_CALL(mock_cb_, FrameReceived(HasTimestamp(20)))
+    EXPECT_CALL(mock_cb_, FrameReceived(HasTimestamp(60)))
         .Times(0);
-    EXPECT_CALL(mock_cb_, FrameReceived(HasTimestamp(30)))
+    EXPECT_CALL(mock_cb_, FrameReceived(HasTimestamp(90)))
         .WillOnce(RunClosure(event.GetClosure()));
-    AdvanceTimeInMs(31);
+    AdvanceWallclockTimeInMs(91);
+    AdvanceTimeInMs(91);
+
     event.RunAndWait();
     Mock::VerifyAndClearExpectations(&mock_cb_);
   }
@@ -562,7 +562,7 @@ TEST_P(VideoRendererImplTest, Underflow) {
     WaitableMessageLoopEvent event;
     EXPECT_CALL(mock_cb_, BufferingStateChange(BUFFERING_HAVE_NOTHING))
         .WillOnce(RunClosure(event.GetClosure()));
-    AdvanceWallclockTimeInMs(9);
+    AdvanceWallclockTimeInMs(29);
     event.RunAndWait();
     Mock::VerifyAndClearExpectations(&mock_cb_);
   }
@@ -635,11 +635,12 @@ TEST_P(VideoRendererImplTest, RenderingStopsAfterOneFrameWithEOS) {
 
     EXPECT_TRUE(IsReadPending());
     SatisfyPendingReadWithEndOfStream();
+    WaitForEnded();
 
+    renderer_->OnTimeStateChanged(false);
     event.RunAndWait();
   }
 
-  WaitForEnded();
   Destroy();
 }
 
@@ -651,7 +652,7 @@ TEST_P(VideoRendererImplTest, RenderingStartedThenStopped) {
     return;
 
   Initialize();
-  QueueFrames("0 10 20 30");
+  QueueFrames("0 30 60 90");
 
   // Start the sink and wait for the first callback.  Set statistics to a non
   // zero value, once we have some decoded frames they should be overwritten.
@@ -673,8 +674,9 @@ TEST_P(VideoRendererImplTest, RenderingStartedThenStopped) {
   // because this is a background render, we won't underflow by waiting until
   // a pending read is ready.
   null_video_sink_->set_background_render(true);
-  AdvanceTimeInMs(41);
-  EXPECT_CALL(mock_cb_, FrameReceived(HasTimestamp(30)));
+  AdvanceWallclockTimeInMs(91);
+  AdvanceTimeInMs(91);
+  EXPECT_CALL(mock_cb_, FrameReceived(HasTimestamp(90)));
   WaitForPendingRead();
   SatisfyPendingReadWithEndOfStream();