Introduce media::TimeSource.

Introduce media::TimeSource.

TimeSource represents the canonical source of time and will eventually
replace the time-updating callback from AudioRenderer and VideoRenderer.

A wall-clock based implementation is also provided for media pipelines
that do not contain any audio.

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

[scherkus (not reviewing), 2014-07-11 23:02:38]

xhwang: I want you to take a look over this and feedback on how it related to
your media::Renderer work

there are a few minor things to fix (e.g., ARI doesn't interpolate, need to
implement underflow for VRI) but overall things are working quite well!

we can discuss more next week

[xhwang, 2014-07-12 06:36:44]

I love this CL. I wish I started working on media::Renderer based on something
like this :)

Just some general comments.

https://codereview.chromium.org/379343005/diff/1/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/379343005/diff/1/media/base/pipeline.cc#newco...
media/base/pipeline.cc:158: return time_source_->CurrentMediaTimestamp();
Other use of time_source_ in this file is not protected by the lock_. What's the
plan on that?

https://codereview.chromium.org/379343005/diff/1/media/base/time_source.h
File media/base/time_source.h (right):

https://codereview.chromium.org/379343005/diff/1/media/base/time_source.h#new...
media/base/time_source.h:21: // Signal the time source to start ticking. It is
expected that values from
s/start/stop

https://codereview.chromium.org/379343005/diff/1/media/base/time_source.h#new...
media/base/time_source.h:26: virtual void SetPlaybackRate(float playback_rate) =
0;
Can you add comments explaining how current media time increases based on start
timestamp and playback rate?

https://codereview.chromium.org/379343005/diff/1/media/base/time_source.h#new...
media/base/time_source.h:33: virtual base::TimeDelta CurrentMediaTimestamp() =
0;
const function?

Also, can we just have SetMediaTime() and CurrentMediaTime() since this is a
"time" source?

https://codereview.chromium.org/379343005/diff/1/media/base/wall_clock_time_s...
File media/base/wall_clock_time_source.cc (right):

https://codereview.chromium.org/379343005/diff/1/media/base/wall_clock_time_s...
media/base/wall_clock_time_source.cc:59: (now -
reference_wall_ticks_).InMicroseconds() * playback_rate_);
nit: This appears 3 times and deserve a helper function, e.g

TimeDelta ElapsedTime() {
  base::TimeDelta::FromMicroseconds(
    (base::TimeTicks::Now() - reference_wall_ticks_).InMicroseconds() *
playback_rate_);
}

Since TimeDelta supports multiplication and division with integers, can we
always store 1,000,000 * playback_rate_ so that we can do:
(now - reference_wall_ticks_) * playback_rate_ / 1000000 ?

https://codereview.chromium.org/379343005/diff/1/media/base/wall_clock_time_s...
File media/base/wall_clock_time_source.h (right):

https://codereview.chromium.org/379343005/diff/1/media/base/wall_clock_time_s...
media/base/wall_clock_time_source.h:29: base::TimeTicks reference_wall_ticks_;
Add comments for these variables. It's not obvious without looking at the impl.

https://codereview.chromium.org/379343005/diff/1/media/filters/audio_renderer...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/379343005/diff/1/media/filters/audio_renderer...
media/filters/audio_renderer_impl.cc:244: // TODO(scherkus): Take time since
last updating |audio_clock_| into account.
Yeah, this will improve time accuracy, also solve the "audio shorter than video"
issue.

[scherkus (not reviewing), 2014-07-17 23:52:57]

can you take a look?

I repurposed this CL to focus on delivering {WallClock}TimeSource + tests +
incorporating your feedback.

https://codereview.chromium.org/379343005/diff/1/media/base/time_source.h
File media/base/time_source.h (right):

https://codereview.chromium.org/379343005/diff/1/media/base/time_source.h#new...
media/base/time_source.h:21: // Signal the time source to start ticking. It is
expected that values from
On 2014/07/12 06:36:44, xhwang wrote:
> s/start/stop

Done.

https://codereview.chromium.org/379343005/diff/1/media/base/time_source.h#new...
media/base/time_source.h:26: virtual void SetPlaybackRate(float playback_rate) =
0;
On 2014/07/12 06:36:44, xhwang wrote:
> Can you add comments explaining how current media time increases based on
start
> timestamp and playback rate?

Done.

Let me know if what I wrote is what you had in mind.

https://codereview.chromium.org/379343005/diff/1/media/base/time_source.h#new...
media/base/time_source.h:33: virtual base::TimeDelta CurrentMediaTimestamp() =
0;
On 2014/07/12 06:36:44, xhwang wrote:
> const function?
> 
> Also, can we just have SetMediaTime() and CurrentMediaTime() since this is a
> "time" source?

Done for the "time" bit.

I don't think const makes as much sense for this interface as some of the
underlying bits aren't const (e.g., base::TimeTicks set of functions)

https://codereview.chromium.org/379343005/diff/1/media/base/wall_clock_time_s...
File media/base/wall_clock_time_source.cc (right):

https://codereview.chromium.org/379343005/diff/1/media/base/wall_clock_time_s...
media/base/wall_clock_time_source.cc:59: (now -
reference_wall_ticks_).InMicroseconds() * playback_rate_);
On 2014/07/12 06:36:44, xhwang wrote:
> nit: This appears 3 times and deserve a helper function, e.g
> 
> TimeDelta ElapsedTime() {
>   base::TimeDelta::FromMicroseconds(
>     (base::TimeTicks::Now() - reference_wall_ticks_).InMicroseconds() *
> playback_rate_);
> }
> 
> Since TimeDelta supports multiplication and division with integers, can we
> always store 1,000,000 * playback_rate_ so that we can do:
> (now - reference_wall_ticks_) * playback_rate_ / 1000000 ?

Done.

https://codereview.chromium.org/379343005/diff/1/media/base/wall_clock_time_s...
File media/base/wall_clock_time_source.h (right):

https://codereview.chromium.org/379343005/diff/1/media/base/wall_clock_time_s...
media/base/wall_clock_time_source.h:29: base::TimeTicks reference_wall_ticks_;
On 2014/07/12 06:36:44, xhwang wrote:
> Add comments for these variables. It's not obvious without looking at the
impl.

Done.

Let me know if this is what you had in mind.

[xhwang, 2014-07-18 04:50:17]

lgtm

[scherkus (not reviewing), 2014-07-18 15:16:47]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-07-18 15:18:53]

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

[commit-bot: I haz the power, 2014-07-18 16:36:35]

Change committed as 284122