Introduce TimeSource notifications to the VideoRenderer.

Introduce TimeSource notifications to the VideoRenderer.

The upcoming VideoRenderer needs a way to know when time starts and
stops moving; this extends the RendererImpl to let the VideoRenderer
know about time changes.

I considered introducing a TimeSourceDependent observer-like interface
for TimeSources, but it ended up being way more complicated. I also
considered a proxy TimeSource, but again it was way more complicated.
The approach here is quite simple and small in complexity.

BUG=439548
TEST=new unittest.

Committed: https://crrev.com/5cc4fb592bd377ff503003a781f5fa9f68a4e8cc
Cr-Commit-Position: refs/heads/master@{#324172}

[DaleCurtis, 2015-04-07 00:25:53]

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

[DaleCurtis, 2015-04-07 00:25:54]

The approach we discussed ultimately added a bunch of complexity that didn't
seem worthwhile; this approach allows us to remove the
StartRendering/StopRendering and similar gunk from WebMediaPlayerImpl in the
priming patch. I'll upload the new version shortly.

[xhwang, 2015-04-07 06:13:08]

https://codereview.chromium.org/1068593003/diff/1/media/renderers/renderer_im...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/1068593003/diff/1/media/renderers/renderer_im...
media/renderers/renderer_impl.cc:190: video_renderer_->OnTimeStopped();
This will definitely work. I just wonder whether we can have a slightly cleaner
interface. Here are some brainstorming ideas:
- VideoRenderer actually can detect time stop because wall_clock_time_cb starts
to return null time. This could replace OnTimeStopped()...
- Even with that, we still need something like OnTimeProgressing() to kick start
decoding again.
- Maybe we can just reintroduce SetPlaybackRate() to VideoRenderer again :( ?
Sounds bad though...
- Maybe we can extend/reuse VideoRenderer::StartPlayingFrom(time) such that when
|time| is null, we keep the original |start_timestamp_| and just kick start a
Read()?

[DaleCurtis, 2015-04-07 16:51:40]

https://codereview.chromium.org/1068593003/diff/1/media/renderers/renderer_im...
File media/renderers/renderer_impl.cc (right):

https://codereview.chromium.org/1068593003/diff/1/media/renderers/renderer_im...
media/renderers/renderer_impl.cc:190: video_renderer_->OnTimeStopped();
On 2015/04/07 06:13:08, xhwang wrote:
> This will definitely work. I just wonder whether we can have a slightly
cleaner
> interface. Here are some brainstorming ideas:
> - VideoRenderer actually can detect time stop because wall_clock_time_cb
starts
> to return null time. This could replace OnTimeStopped()...
> - Even with that, we still need something like OnTimeProgressing() to kick
start
> decoding again.
> - Maybe we can just reintroduce SetPlaybackRate() to VideoRenderer again :( ?
> Sounds bad though...
> - Maybe we can extend/reuse VideoRenderer::StartPlayingFrom(time) such that
when
> |time| is null, we keep the original |start_timestamp_| and just kick start a
> Read()?

Yeah, I considered all of these except the last one; I don't really like them
any more than your comments suggest. The detection one ends up with repeated
polling.

Adding SetPlaybackRate back can make things a little more accurate in the future
VideoRendererAlgorithm, because we can instead calculate the frame duration
based on the frame timestamps + playback rate instead of the wallclock
timestamps (which might have shifted between subsequent calls; this could be
fixed by instead changing WallClockTimeCB to work with a vector of timestamps if
it ends up being a problem).

I think the last idea you suggest won't work for preroll.

Another complicated idea I had in the vein of what we discussed, is to create a
TimeSourceOracle who is a TimeSource. All renderers would expose a
GetTimeSource() which vended a subset of today's TimeSource interface (say
TimeSourceFeeder). The oracle would decide amongst the TimeSourceFeeder objects
and also notify them as time starts or stops.

In the ideal case that would have saved a bunch of code in AudioRenderer,
Renderer, and VideoRenderer. In the actual case, the AudioClock is strongly tied
into the AudioRendererImpl and AudioRendererAlgorithm so it didn't end up saving
very much after all and seems overly complicated.

[DaleCurtis, 2015-04-07 19:55:17]

PTAL. Per our offline discussion I've switched to a single method.  I didn't
like the complications SetPlaybackRate() added to the future VideoRendererImpl
state machine (i.e. in addition to checking null time ticks Render() now needs
to watch for playback rate changes), so I just merged the methods per your
suggestion earlier.

[xhwang, 2015-04-07 23:57:50]

lgtm

https://codereview.chromium.org/1068593003/diff/20001/media/base/video_render...
File media/base/video_renderer.h (right):

https://codereview.chromium.org/1068593003/diff/20001/media/base/video_render...
media/base/video_renderer.h:87: // |time_progressing| will be false.
Add a comment about why we need this method in the first place. For example, we
need to notify the compositor so that it can stopping polling for new frames. If
you don't like to have impl details on the interface, maybe add it to VRI?

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

https://codereview.chromium.org/1068593003/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl.cc:169: video_renderer_->StartPlayingFrom(time);
The |time| is only used in one place in VideoRendererImpl, wondering whether we
can remove it since we are wall clock based?

[DaleCurtis, 2015-04-08 00:04:32]

https://codereview.chromium.org/1068593003/diff/20001/media/base/video_render...
File media/base/video_renderer.h (right):

https://codereview.chromium.org/1068593003/diff/20001/media/base/video_render...
media/base/video_renderer.h:87: // |time_progressing| will be false.
On 2015/04/07 23:57:49, xhwang wrote:
> Add a comment about why we need this method in the first place. For example,
we
> need to notify the compositor so that it can stopping polling for new frames.
If
> you don't like to have impl details on the interface, maybe add it to VRI?

I'll add it to the VRI once we have such details. I don't think it makes sense
to add them here.

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

https://codereview.chromium.org/1068593003/diff/20001/media/renderers/rendere...
media/renderers/renderer_impl.cc:169: video_renderer_->StartPlayingFrom(time);
On 2015/04/07 23:57:50, xhwang wrote:
> The |time| is only used in one place in VideoRendererImpl, wondering whether
we
> can remove it since we are wall clock based?

Seems like it's still needed to determine effective preroll buffers?

[DaleCurtis, 2015-04-08 00:04:58]

CQ'ing, please uncheck if those comments were hard objections.

[DaleCurtis, 2015-04-08 00:05:02]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2015-04-08 00:05:24]

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

[commit-bot: I haz the power, 2015-04-08 01:48:07]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-08 01:48:57]

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