Move underflow threshold limits out of the video renderer.

media/renderers/renderer_impl_unittest.cc

Index: media/renderers/renderer_impl_unittest.cc
diff --git a/media/renderers/renderer_impl_unittest.cc b/media/renderers/renderer_impl_unittest.cc
index 30bad2fe27040a12da619aa263b819472278cadf..1dacbcc5ea85b58ca9787744e054bb3625508687 100644
--- a/media/renderers/renderer_impl_unittest.cc
+++ b/media/renderers/renderer_impl_unittest.cc
@@ -110,6 +110,8 @@ class RendererImplTest : public ::testing::Test {
     if (start_status == PIPELINE_OK && audio_stream_) {
       EXPECT_CALL(*audio_renderer_, GetTimeSource())
           .WillOnce(Return(&time_source_));
+    } else {
+      renderer_impl_->set_time_source_for_testing(&time_source_);
     }
 
     renderer_impl_->Initialize(
@@ -174,10 +176,10 @@ class RendererImplTest : public ::testing::Test {
 
     base::TimeDelta start_time(
         base::TimeDelta::FromMilliseconds(kStartPlayingTimeInMs));
+    EXPECT_CALL(time_source_, SetMediaTime(start_time));
+    EXPECT_CALL(time_source_, StartTicking());
 
     if (audio_stream_) {
-      EXPECT_CALL(time_source_, SetMediaTime(start_time));
-      EXPECT_CALL(time_source_, StartTicking());
       EXPECT_CALL(*audio_renderer_, StartPlaying())
           .WillOnce(SetBufferingState(&audio_buffering_state_cb_,
                                       BUFFERING_HAVE_ENOUGH));
@@ -194,9 +196,10 @@ class RendererImplTest : public ::testing::Test {
   }
 
   void Flush(bool underflowed) {
+    if (!underflowed)
+      EXPECT_CALL(time_source_, StopTicking());
+
     if (audio_stream_) {
-      if (!underflowed)
-        EXPECT_CALL(time_source_, StopTicking());
       EXPECT_CALL(*audio_renderer_, Flush(_))
           .WillOnce(DoAll(SetBufferingState(&audio_buffering_state_cb_,
                                             BUFFERING_HAVE_NOTHING),
@@ -367,7 +370,7 @@ TEST_F(RendererImplTest, VideoStreamEnded) {
   InitializeWithVideo();
   Play();
 
-  // Video ended won't affect |time_source_|.
+  EXPECT_CALL(time_source_, StopTicking());
   EXPECT_CALL(callbacks_, OnEnded());
 
   video_ended_cb_.Run();
@@ -446,4 +449,87 @@ TEST_F(RendererImplTest, ErrorDuringInitialize) {
   InitializeAndExpect(PIPELINE_ERROR_DECODE);
 }
 
+TEST_F(RendererImplTest, AudioUnderflow) {
+  InitializeWithAudio();
+  Play();
+
+  // Underflow should occur immediately with a single audio track.
+  EXPECT_CALL(time_source_, StopTicking());
+  audio_buffering_state_cb_.Run(BUFFERING_HAVE_NOTHING);
+}
+
+TEST_F(RendererImplTest, AudioUnderflowWithVideo) {
+  InitializeWithAudioAndVideo();
+  Play();
+
+  // Underflow should be immediate when both audio and video are present and
+  // audio underflows.
+  EXPECT_CALL(time_source_, StopTicking());
+  audio_buffering_state_cb_.Run(BUFFERING_HAVE_NOTHING);
+}
+
+TEST_F(RendererImplTest, VideoUnderflow) {
+  InitializeWithVideo();
+  Play();
+
+  // Underflow should occur immediately with a single video track.

[xhwang, 2015/03/26 21:24:43]

I am not sure about this... did you try to trigger one or two dropped frames
once a while to compare the effect with and without your CL? 

Previously if we drop one or two frames video won't underflow and user usually
won't notice. Now I wonder whether we'll see more glitches in that case.
(Imagine you have a smooth panning scene...)

[DaleCurtis, 2015/03/26 21:36:08]

Hmm, the main goal is to fix http://crbug.com/423801 which requires that media
time stop immediately if there's a video only stream.  I think at most we would
want to delay the HAVE_NOTHING state based on the expected duration of each
VideoFrame.  Although that means we need some logic back in VideoRendererImpl
too I think.

The user would definitely notice in the video only case if frame display exceeds
its duration, so I'm not clear on what you mean about not noticing. Are you
talking about the case where we get a new frame within the expected time?

[xhwang, 2015/03/28 00:32:40]

Ah, I forgot about the bug. Thanks for reminding me!

Totally agree that the delay to report HAVE_NOTHING should be way smaller (if
not zero) than 3s. Now the questions is whether we should choose zero (as this
CL does), or some other small value.

This is the case I am concerned about. So we rendered all frames in the queue
and the queue is empty. Then our video renderer thread wakes up and found
ready_frames_ is empty. Let's say this is at time T1. But there was really just
a glitch in decoding (e.g. the frame is a weird I frame which takes longer to
decode, or just the CPU is busy). The next frame (F1) arrived immediately after
T1 (say at T1'). Then F2, F3 arrived shortly as well.

If we declare underflow at T1 immediately, IIRC we'll go through the preroll
process again. So we'll queue 4 frames before we start playback.  Say the
average decoding time is D. Then F1 will be rendered at about T + 4D. Also, we
may have some other delay to underflow then resume. The good news is we are not
dropping any frames.

If we do not declare underflow at T1 immediately. F1 arrived at T1', and we'll
render it (or we may drop it depending on the value of T1' - T1). So we'll
likely to drop zero or one or even two frames, but there's no interruption in
the playback.

Now the question is, which one do we prefer? I think dropping one frame is
better than reprerolling in terms of user experience. If user watch carefully,
one or two dropped can be noticeable. But it should be better than waiting for
reprerolling.

Since small glitches are more frequent than large delays (I assume), we should
probably choose a small threshold to make sure we have good user experience in
that case. When the delay is large (e.g. due to network conditions), we should
definitely report underflow. In that case, a small delay in reporting underflow
shouldn't hurt.

Now the question is what's the optimal value of time-to-report-overflow. Maybe
4D (just wild guess). We probably want to experiment on that.

Another idea is while we waiting before we report overflow, we watch how long we
waited. If the next frame arrived before the threshold, we continue to play, but
we'll take the waiting time out when we calculate wall time, so that we don't
drop the frames coming late.

These are just my mind practice. I haven't tried any of these to see whether the
theory is correct. But I'd like to experiment more on these.

BTW, this was the hack that I used to test these conditions:
https://codereview.chromium.org/464113003/#ps40001

Let me know WDYT and we can discuss more on these.

[DaleCurtis, 2015/03/30 16:17:16]

Thanks for the detailed thoughts. I agree with what you wrote and think, why not
both? We have two problems that feel like they need separate solutions then:
- Time should stop ticking when we run out of frames.
- Underflow should be based on a threshold; the "time-to-report-underflow" I
think you meant above :)

Alternatively, I wonder if there's some combined signal from the renderers and
demuxer that the RendererImpl can use to determine when we've reached the end of
available encoded data. Though I think that might just resolve that bug and not
the real world situation it models.

+  EXPECT_CALL(time_source_, StopTicking());
+  video_buffering_state_cb_.Run(BUFFERING_HAVE_NOTHING);
+}
+
+TEST_F(RendererImplTest, VideoUnderflowWithAudio) {
+  InitializeWithAudioAndVideo();
+  Play();
+
+  // Set a zero threshold such that the underflow will be executed on the next
+  // run of the message loop.
+  renderer_impl_->set_video_underflow_threshold_for_testing(base::TimeDelta());
+
+  // Underflow should be delayed when both audio and video are present and video
+  // underflows.
+  video_buffering_state_cb_.Run(BUFFERING_HAVE_NOTHING);
+  Mock::VerifyAndClearExpectations(&time_source_);
+
+  EXPECT_CALL(time_source_, StopTicking());
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(RendererImplTest, VideoUnderflowWithAudioVideoRecovers) {
+  InitializeWithAudioAndVideo();
+  Play();
+
+  // Set a zero threshold such that the underflow will be executed on the next
+  // run of the message loop.
+  renderer_impl_->set_video_underflow_threshold_for_testing(base::TimeDelta());
+
+  // Underflow should be delayed when both audio and video are present and video
+  // underflows.

[xhwang, 2015/03/26 21:24:43]

This is not accurate now given the threshold is 0.

[DaleCurtis, 2015/03/26 21:36:08]

Well, it's accurate in the sense that it's delayed until the message loop runs,
which is what we want to avoid having the test depend on wall clock time.

+  video_buffering_state_cb_.Run(BUFFERING_HAVE_NOTHING);
+  Mock::VerifyAndClearExpectations(&time_source_);
+
+  // If video recovers, the underflow should never occur.
+  video_buffering_state_cb_.Run(BUFFERING_HAVE_ENOUGH);
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(RendererImplTest, VideoAndAudioUnderflow) {
+  InitializeWithAudioAndVideo();
+  Play();
+
+  // Set a zero threshold such that the underflow will be executed on the next
+  // run of the message loop.
+  renderer_impl_->set_video_underflow_threshold_for_testing(base::TimeDelta());
+
+  // Underflow should be delayed when both audio and video are present and video
+  // underflows.
+  video_buffering_state_cb_.Run(BUFFERING_HAVE_NOTHING);
+  Mock::VerifyAndClearExpectations(&time_source_);
+
+  EXPECT_CALL(time_source_, StopTicking());
+  audio_buffering_state_cb_.Run(BUFFERING_HAVE_NOTHING);
+
+  // Nothing else should primed on the message loop.
+  base::RunLoop().RunUntilIdle();
+}
+
 }  // namespace media