Merge to M44: "Fix deferred video underflow if audio underflows first."

Looks like the new video rendering path caught a pre-existing bug
with how deferred video underflow is handled \o/

If audio underflowed first, the video renderer's underflow would
still be deferred.  If a Flush() comes in, the VideoRendererImpl
would think it's in the HAVE_NOTHING state and send no update to
its buffering state.

Once Flush() completed and playback was restarted the renderer
would start playback as soon as the audio renderer was ready since
the old buffering state for video was have enough.

The fix is to never defer video renderer underflow if audio has
already underflowed.  I've also added a DCHECK() to make it clear
that the audio renderer is expected to clear deferred underflow for
the video renderer during Flush().

BUG=485324
TEST=new unittest.
TBR=xhwang

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

Cr-Commit-Position: refs/heads/master@{#330463}
(cherry picked from commit 32315ab)

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

Cr-Commit-Position: refs/branch-heads/2403@{#45}
Cr-Branched-From: f54b809-refs/heads/master@{#330231}

media/renderers/renderer_impl.cc

@@ -405,6 +405,10 @@ void RendererImpl::OnAudioRendererFlushDone() {
   DCHECK_EQ(state_, STATE_FLUSHING);
   DCHECK(!flush_cb_.is_null());
 
+  // If we had a deferred video renderer underflow prior to the flush, it should
+  // have been cleared by the audio renderer changing to BUFFERING_HAVE_NOTHING.
+  DCHECK(deferred_underflow_cb_.IsCancelled());
+
   DCHECK_EQ(audio_buffering_state_, BUFFERING_HAVE_NOTHING);
   audio_ended_ = false;
   FlushVideoRenderer();
@@ -457,10 +461,12 @@ void RendererImpl::OnBufferingStateChanged(BufferingState* buffering_state,
 
   bool was_waiting_for_enough_data = WaitingForEnoughData();
 
-  // When audio is present, defer underflow callbacks for some time to avoid
-  // unnecessary glitches in audio; see http://crbug.com/144683#c53.
+  // When audio is present and has enough data, defer video underflow callbacks
+  // for some time to avoid unnecessary glitches in audio; see
+  // http://crbug.com/144683#c53.
   if (audio_renderer_ && !is_audio && state_ == STATE_PLAYING) {
     if (video_buffering_state_ == BUFFERING_HAVE_ENOUGH &&
+        audio_buffering_state_ == BUFFERING_HAVE_ENOUGH &&
         new_buffering_state == BUFFERING_HAVE_NOTHING &&
         deferred_underflow_cb_.IsCancelled()) {
       deferred_underflow_cb_.Reset(base::Bind(

media/renderers/renderer_impl_unittest.cc

@@ -564,4 +564,42 @@ TEST_F(RendererImplTest, VideoAndAudioUnderflow) {
   base::RunLoop().RunUntilIdle();
 }
 
+TEST_F(RendererImplTest, VideoUnderflowWithAudioFlush) {
+  InitializeWithAudioAndVideo();
+  Play();
+
+  // Set a massive threshold such that it shouldn't fire within this test.
+  renderer_impl_->set_video_underflow_threshold_for_testing(
+      base::TimeDelta::FromSeconds(100));
+
+  // Simulate the cases where audio underflows and then video underflows.
+  EXPECT_CALL(time_source_, StopTicking());
+  audio_buffering_state_cb_.Run(BUFFERING_HAVE_NOTHING);
+  video_buffering_state_cb_.Run(BUFFERING_HAVE_NOTHING);
+  Mock::VerifyAndClearExpectations(&time_source_);
+
+  // Flush the audio and video renderers, both think they're in an underflow
+  // state, but if the video renderer underflow was deferred, RendererImpl would
+  // think it still has enough data.
+  EXPECT_CALL(*audio_renderer_, Flush(_)).WillOnce(RunClosure<0>());
+  EXPECT_CALL(*video_renderer_, Flush(_)).WillOnce(RunClosure<0>());
+  EXPECT_CALL(callbacks_, OnFlushed());
+  renderer_impl_->Flush(
+      base::Bind(&CallbackHelper::OnFlushed, base::Unretained(&callbacks_)));
+  base::RunLoop().RunUntilIdle();
+
+  // Start playback after the flush, but never return BUFFERING_HAVE_ENOUGH from
+  // the video renderer (which simulates spool up time for the video renderer).
+  const base::TimeDelta kStartTime;
+  EXPECT_CALL(time_source_, SetMediaTime(kStartTime));
+  EXPECT_CALL(*audio_renderer_, StartPlaying())
+      .WillOnce(
+          SetBufferingState(&audio_buffering_state_cb_, BUFFERING_HAVE_ENOUGH));
+  EXPECT_CALL(*video_renderer_, StartPlayingFrom(kStartTime));
+  renderer_impl_->StartPlayingFrom(kStartTime);
+
+  // Nothing else should primed on the message loop.
+  base::RunLoop().RunUntilIdle();
+}
+
 }  // namespace media

media/renderers/video_renderer_impl.cc

@@ -701,7 +701,7 @@ bool VideoRendererImpl::HaveReachedBufferingCap() {
 
 void VideoRendererImpl::StartSink() {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  CHECK_GT(algorithm_->frames_queued(), 0u);
+  DCHECK_GT(algorithm_->frames_queued(), 0u);
   sink_->Start(this);
   sink_started_ = true;
   was_background_rendering_ = false;