Move underflow threshold limits out of the video renderer.

Move underflow threshold limits out of the video renderer.

The threshold logic is incorrect when there's no audio track, which
the VideoRendererImpl has no knowledge of, so this logic needs to
live in the RendererImpl.

As a consequence of this, the underflow threshold is now based on
wall clock time instead of media time; which means non-realtime
playback will allow more or less media time to elapse before
underflow occurs.

Exposes a new command line flag --video-underflow-threshold-ms to
allow for experiments varying the threshold for YouTube.

BUG=423801, 470940
TEST=new unittests.

Committed: https://crrev.com/c11e7bb244652731cc67c023bd2cf39e52df3544
Cr-Commit-Position: refs/heads/master@{#323599}

[DaleCurtis, 2015-03-26 20:27:48]

Patchset #1 (id:1) has been deleted

[DaleCurtis, 2015-03-26 20:28:29]

dalecurtis@chromium.org changed reviewers:
+ strobe@chromium.org, xhwang@chromium.org

[DaleCurtis, 2015-03-26 20:29:01]

dalecurtis@chromium.org changed reviewers:
+ nasko@chromium.org

[DaleCurtis, 2015-03-26 20:29:02]

+nasko for stamp on new flag copy in RenderProcessHostImpl

[nasko, 2015-03-26 20:51:27]

content/browser/renderer_host/render_process_host_impl.cc LGTM

[xhwang, 2015-03-26 21:24:44]

Thanks! I like the idea to have a flag to override the threshold. But I am not
sure whether triggering underflow immdediately for video-only case is what we
want. If we don't treat video-only separately, we can keep our old code and the
logic would be much simpler (than the cancelable callback approach in
RendererImpl).

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl.cc:42:
video_underflow_threshold_(base::TimeDelta::FromSeconds(3)),
You can still use a file scope const instead of a hard coded value here.

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl.cc:53: if (base::StringToInt(threshold_ms_str,
&threshold_ms) && threshold_ms > 0) {
s/>/>=?

We should be able to set the threshold to 0 to underflow immediately.

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl.cc:354: } else if (!time_source_) {
hmm, why we need this?

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
File media/renderers/renderer_impl_unittest.cc (right):

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl_unittest.cc:475: // Underflow should occur
immediately with a single video track.
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...)

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl_unittest.cc:506: // underflows.
This is not accurate now given the threshold is 0.

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

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl.cc:42:
video_underflow_threshold_(base::TimeDelta::FromSeconds(3)),
On 2015/03/26 21:24:43, xhwang wrote:
> You can still use a file scope const instead of a hard coded value here.

This allows overriding for testing much more easily.

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl.cc:354: } else if (!time_source_) {
On 2015/03/26 21:24:43, xhwang wrote:
> hmm, why we need this?

Because I'm overriding the TimeSource for testing, without this the wall clock
time source would be used instead of the test source.

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
File media/renderers/renderer_impl_unittest.cc (right):

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl_unittest.cc:475: // Underflow should occur
immediately with a single video track.
On 2015/03/26 21:24:43, xhwang wrote:
> 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...)

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?

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl_unittest.cc:506: // underflows.
On 2015/03/26 21:24:43, xhwang wrote:
> This is not accurate now given the threshold is 0.

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.

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

More discussions...

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl.cc:42:
video_underflow_threshold_(base::TimeDelta::FromSeconds(3)),
On 2015/03/26 21:36:08, DaleCurtis wrote:
> On 2015/03/26 21:24:43, xhwang wrote:
> > You can still use a file scope const instead of a hard coded value here.
> 
> This allows overriding for testing much more easily.

You can declare this in the header file so it's available for the test as
well....

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl.cc:354: } else if (!time_source_) {
On 2015/03/26 21:36:08, DaleCurtis wrote:
> On 2015/03/26 21:24:43, xhwang wrote:
> > hmm, why we need this?
> 
> Because I'm overriding the TimeSource for testing, without this the wall clock
> time source would be used instead of the test source.

Thanks for the explanation. Maybe worth a comment.

BTW, can we also override the time source when we have audio? That could make
the logic more general/clear. Not sure whether there's any real value though ;)

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
File media/renderers/renderer_impl_unittest.cc (right):

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl_unittest.cc:475: // Underflow should occur
immediately with a single video track.
On 2015/03/26 21:36:08, DaleCurtis wrote:
> On 2015/03/26 21:24:43, xhwang wrote:
> > 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...)
> 
> 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?

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]

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
File media/renderers/renderer_impl_unittest.cc (right):

https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl_unittest.cc:475: // Underflow should occur
immediately with a single video track.
On 2015/03/28 00:32:40, xhwang wrote:
> On 2015/03/26 21:36:08, DaleCurtis wrote:
> > On 2015/03/26 21:24:43, xhwang wrote:
> > > 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...)
> > 
> > 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?
> 
> 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.

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.

[xhwang, 2015-03-31 18:09:11]

On 2015/03/30 16:17:16, DaleCurtis wrote:
>
https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
> File media/renderers/renderer_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/1034233002/diff/20001/media/renderers/rendere...
> media/renderers/renderer_impl_unittest.cc:475: // Underflow should occur
> immediately with a single video track.
> On 2015/03/28 00:32:40, xhwang wrote:
> > On 2015/03/26 21:36:08, DaleCurtis wrote:
> > > On 2015/03/26 21:24:43, xhwang wrote:
> > > > 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...)
> > > 
> > > 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?
> > 
> > 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.
> 
> 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 :)

Having both sgtm.

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

Yeah, there are a lot heuristics we can leverage upon. But I wouldn't over
complicate our system for these edge cases. As long as we provide some simple
and acceptable solution we should be fine.

[DaleCurtis, 2015-03-31 20:20:25]

Actually, thinking about this a bit more and trying out your linked patch set, I
think it makes sense to stop video rendering once we exceed the latest possible
paint time; which is the expected duration of the last frame.

The behavior in either case is a visible hang to the user, so I think it's worth
going with stopping the clock to preserve correct behavior during appends.

Allowing the video renderer to stop the clock via a new mechanism for the video
only case would add a lot of complexity to code that's going to be ripped out
soon. It's also worth noting that we underflow  immediately when we run out of
audio.

As such, I think the latest patch set I have uploaded makes sense. It waits
until the current frame has reached its expected duration and then fires
have_nothing.  WDYT?

[xhwang, 2015-04-01 16:32:58]

> Actually, thinking about this a bit more and trying out your linked patch set,
I
> think it makes sense to stop video rendering once we exceed the latest
possible
> paint time; which is the expected duration of the last frame.

It makes sense. Can you just double check what's happening in high fps (e.g.
60fps)? In that case, it's more likely to run out of decoded frames, and the
duration of a frame is actually shorter (so we have less headroom).

> The behavior in either case is a visible hang to the user, so I think it's
worth
> going with stopping the clock to preserve correct behavior during appends.

When we only miss one frame do we also see a "visible hang"? Human eyes usually
have some tolerance on this. My point is that dropping one frame (or 1-2 at
60fps) is a common case and we should make sure it's handled well.

> Allowing the video renderer to stop the clock via a new mechanism for the
video
> only case would add a lot of complexity to code that's going to be ripped out
> soon. 

Totally agreed. I am a big fan of simple code.

> It's also worth noting that we underflow  immediately when we run out of
> audio.

Audio is less affected by network/CPU, so this shouldn't happen as often as
running out of video. Also, humans are much more sensitive to any audio glitch.
So dropping one audio frame is not acceptable.

> As such, I think the latest patch set I have uploaded makes sense. It waits
> until the current frame has reached its expected duration and then fires
> have_nothing.  WDYT?

[DaleCurtis, 2015-04-02 18:11:07]

I retested some more with lower and random delays and the experience is still
the same with either approach in a video only playback: there's an observable
playback stall. As such, the approach which ensures time stops during this stall
seems most correct.

[xhwang, 2015-04-02 18:16:50]

On 2015/04/02 18:11:07, DaleCurtis wrote:
> I retested some more with lower and random delays and the experience is still
> the same with either approach in a video only playback: there's an observable
> playback stall. As such, the approach which ensures time stops during this
stall
> seems most correct.

Hmm, thanks for testing that out. Could you please upload all changes/hacks you
have for this retest so that I can try it as well? I am especially interested in
the case where you just drop 1-2 frames occasionally.

[DaleCurtis, 2015-04-02 18:31:39]

Sure, here's the CL I used: 

https://codereview.chromium.org/1054103002

Here's some soundless 60fps clip:

http://xorax.sea/fwong720p60.webm
http://xorax.sea/catzilla.webm

48kHz 60FPS AV sync clip:
http://xorax.sea/sync48_60.webm

[xhwang, 2015-04-02 21:38:49]

On 2015/04/02 18:31:39, DaleCurtis wrote:
> Sure, here's the CL I used: 
> 
> https://codereview.chromium.org/1054103002
> 
> Here's some soundless 60fps clip:
> 
> http://xorax.sea/fwong720p60.webm
> http://xorax.sea/catzilla.webm
> 
> 48kHz 60FPS AV sync clip:
> http://xorax.sea/sync48_60.webm

I tried it and am convinced!

The performance impact of underflow is smaller than I thought. So I don't really
see too much difference between your current CL, or waiting a bit longer. Given
that, having a simpler solution is preferred. LGTM!

Thank you for the patience :)

[DaleCurtis, 2015-04-02 21:46:49]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-04-02 21:46:49]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1034233002/#ps40001
(title: "Wait for frame duration.")

[commit-bot: I haz the power, 2015-04-02 21:51:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1034233002/40001

[commit-bot: I haz the power, 2015-04-03 05:07:16]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-04-03 20:30:51]

Patchset 2 (id:??) landed as
https://crrev.com/c11e7bb244652731cc67c023bd2cf39e52df3544
Cr-Commit-Position: refs/heads/master@{#323599}