Remove media::VideoRenderer::SetPlaybackRate().

Remove media::VideoRenderer::SetPlaybackRate().

Video renderers don't need the information as they are already making
audio/video synchronization decisions based on a media timeline that
already incorporates the current playback rate via the time callback.

In particular, VideoRendererImpl only used playback rate to estimate
a better duration to sleep until the current frame was ready ...
except that in most cases we'd sleep for kIdleTimeDelta and wait
until the media timeline had progressed past the current frame's
timestamp. In other words, there's absolutely no reason to even try
to estimate the sleep duration based on the playback rate.

BUG=370634
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282797

[scherkus (not reviewing), 2014-07-10 22:29:56]

here's another one for you that should make things easier :)

[xhwang, 2014-07-10 22:57:56]

Thanks for making my life easier and easier!

One general question, in case of high playback rate, is it still true that we
almost always wait for kIdelTimeDelta? In that case, will this CL make us over
sleep and actually drop more frames?

[scherkus (not reviewing), 2014-07-10 23:29:39]

On 2014/07/10 22:57:56, xhwang wrote:
> Thanks for making my life easier and easier!
> 
> One general question, in case of high playback rate, is it still true that we
> almost always wait for kIdelTimeDelta? In that case, will this CL make us over
> sleep and actually drop more frames?

This CL will make it more likely to wait for kIdleTimeDelta (and hence
potentially miss our deadline) since we won't be scaling the delta by
1/playback_rate

I tested before+after on 16x content and we do end up counting more dropped
frames.

The current experience is pretty terrible anyway (lots of jankiness and
stuttering...) so it's nothing too concerning. I will keep doing more tests
though.

[xhwang, 2014-07-10 23:33:26]

That makes sense. Definitely we don't expect to play 30x16 fps video when 16x is
selected. 10ms idle time give us roughly 100fps which should be pretty good.

lgtm

[xhwang, 2014-07-10 23:54:05]

Sorry, some extra after-thought question...

https://codereview.chromium.org/384943002/diff/20001/media/filters/video_rend...
File media/filters/video_renderer_impl.cc (right):

https://codereview.chromium.org/384943002/diff/20001/media/filters/video_rend...
media/filters/video_renderer_impl.cc:240: remaining_time =
std::min(remaining_time, kIdleTimeDelta);
Does it make a big difference if you always use kIdleTimeDelta? The calculation
of l.235 isn't correct now in theory. What's the value of keeping remaining_time
here? I can imagine it's useful in some cases, but this kinda makes things
messy...

[scherkus (not reviewing), 2014-07-11 00:01:03]

https://codereview.chromium.org/384943002/diff/20001/media/filters/video_rend...
File media/filters/video_renderer_impl.cc (right):

https://codereview.chromium.org/384943002/diff/20001/media/filters/video_rend...
media/filters/video_renderer_impl.cc:240: remaining_time =
std::min(remaining_time, kIdleTimeDelta);
On 2014/07/10 23:54:05, xhwang wrote:
> Does it make a big difference if you always use kIdleTimeDelta? The
calculation
> of l.235 isn't correct now in theory. What's the value of keeping
remaining_time
> here? I can imagine it's useful in some cases, but this kinda makes things
> messy...

I thought about it and we can certainly play around with this code! For example:
  1) Always use kIdleTimeDelta, like you suggested
  2) Tweak value of kIdleTimeDelta (maybe to 5ms?!)

Perhaps it's worth doing to simplify this code a bit more.

[scherkus (not reviewing), 2014-07-11 18:42:52]

xhwang: can you take another look?

I wrote a manual testing page to test out changes. By tweaking the target
display window a bit we can now just rely on kIdleTimeDelta and achieve
equivalent-if-not-better dropped frame counts when playing at higher playback
rates.

http://sleep.sea/demos/droppedframes/

[xhwang, 2014-07-11 20:49:49]

I like it! lgtm++

https://codereview.chromium.org/384943002/diff/40001/media/filters/video_rend...
File media/filters/video_renderer_impl.cc (right):

https://codereview.chromium.org/384943002/diff/40001/media/filters/video_rend...
media/filters/video_renderer_impl.cc:253: ready_frames_.front()->timestamp() -
last_timestamp_;
nit: ready_frames_.front()->timestamp() is used 4 times. how about assigning it
to a local var/ref?

[scherkus (not reviewing), 2014-07-11 20:54:02]

https://codereview.chromium.org/384943002/diff/40001/media/filters/video_rend...
File media/filters/video_renderer_impl.cc (right):

https://codereview.chromium.org/384943002/diff/40001/media/filters/video_rend...
media/filters/video_renderer_impl.cc:253: ready_frames_.front()->timestamp() -
last_timestamp_;
On 2014/07/11 20:49:49, xhwang wrote:
> nit: ready_frames_.front()->timestamp() is used 4 times. how about assigning
it
> to a local var/ref?

Done.

[scherkus (not reviewing), 2014-07-11 20:54:05]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-07-11 20:55:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scherkus@chromium.org/384943002/60001

[commit-bot: I haz the power, 2014-07-12 04:49:33]

Change committed as 282797

[falken, 2014-07-14 02:40:11]

On 2014/07/12 04:49:33, I haz the power (commit-bot) wrote:
> Change committed as 282797

I'm wondering whether this could have caused the following layout tests
failures:
media/track/track-cue-rendering-horizontal.html
media/track/track-cue-rendering-vertical.html

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

The image diffs show what appears to be a timestamp change (something like 0:18
to 0:19):
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux__db...

[falken, 2014-07-14 02:46:43]

A revert of this CL has been created in
https://codereview.chromium.org/390733002/ by falken@chromium.org.

The reason for reverting is: Sorry to revert this change. It made the following
layout tests flakily fail:
media/track/track-cue-rendering-horizontal.html
media/track/track-cue-rendering-vertical.html

Reverting locally healed the tests.

The image diffs show what appears to be a timestamp change (something like 0:18
to 0:19):
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Linux__db....