Switch to using media::TimeSource inside media::RendererImpl.

Switch to using media::TimeSource inside media::RendererImpl.

This finally replaces the time-updating-callback inside of AudioRenderer
and VideoRenderer with a model where clients can query the time. As a
result, many other bits of cleanup are now possible:
  - Renderers no longer need to be aware of the media duration
  - TimeDeltaInterpolator can be replaced with WallClockTimeSource
  - We can separate time values used for syncing video vs. informational

BUG=370634
Committed: https://crrev.com/ece894570c5f32f7f404f4c8da9ff8b9f03b75cd
Cr-Commit-Position: refs/heads/master@{#294029}

[scherkus (not reviewing), 2014-09-03 03:30:57]

Patchset #1 (id:1) has been deleted

[scherkus (not reviewing), 2014-09-03 22:31:21]

Patchset #1 (id:20001) has been deleted

[scherkus (not reviewing), 2014-09-03 23:28:33]

Patchset #1 (id:40001) has been deleted

[scherkus (not reviewing), 2014-09-03 23:34:08]

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

[scherkus (not reviewing), 2014-09-03 23:34:09]

xhwang: while I've fixed all the tests 'n stuff, I'd like for you to at least do
some high level review. it'll be a few days until I can actually land this.

I'd start by looking at time_source.h and renderer_impl.{cc,h} to see how I
ripped out TimeDeltaInterpolator.

From there you'll notice that I did a bunch of other clean up in
{Audio,Video}RendererImpl as a result.

[xhwang, 2014-09-08 22:56:06]

Looking pretty good. I just have a few minor suggestions.

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

https://codereview.chromium.org/534073002/diff/60001/media/base/time_source.h...
media/base/time_source.h:50: virtual base::TimeDelta
ApproximateCurrentMediaTime() = 0;
As discussed offline, it's a bit confusing that the non-informational time (more
accurate?) is approximate. How about "RawCurrentMediaTime" or
"CurrentMediaTimeForSync"? Or maybe some comments would help.

https://codereview.chromium.org/534073002/diff/60001/media/filters/audio_rend...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/534073002/diff/60001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:159: return
audio_clock_->front_timestamp();
Add a comment that since this is updated pretty frequently, we don't need
interpolation? Is there any lower bound on the update frequency we can rely on?

https://codereview.chromium.org/534073002/diff/60001/media/filters/audio_rend...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/534073002/diff/60001/media/filters/audio_rend...
media/filters/audio_renderer_impl.h:252: base::TimeTicks last_render_ticks_;
As discussed offline, it would be nicer to add some comments about these
variables.

https://codereview.chromium.org/534073002/diff/60001/media/filters/renderer_i...
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/534073002/diff/60001/media/filters/renderer_i...
media/filters/renderer_impl.cc:415: time_source_->StopTicking();
How about making time_ticking() a method in TimeSource? Then you don't need a
separate variable tracking the status.

https://codereview.chromium.org/534073002/diff/60001/media/filters/renderer_i...
File media/filters/renderer_impl.h (left):

https://codereview.chromium.org/534073002/diff/60001/media/filters/renderer_i...
media/filters/renderer_impl.h:167: mutable base::Lock interpolator_lock_;
+1 !

[scherkus (not reviewing), 2014-09-09 18:12:01]

Patchset #2 (id:80001) has been deleted

[scherkus (not reviewing), 2014-09-09 18:17:18]

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

https://codereview.chromium.org/534073002/diff/60001/media/base/time_source.h...
media/base/time_source.h:50: virtual base::TimeDelta
ApproximateCurrentMediaTime() = 0;
On 2014/09/08 22:56:06, xhwang wrote:
> As discussed offline, it's a bit confusing that the non-informational time
(more
> accurate?) is approximate. How about "RawCurrentMediaTime" or
> "CurrentMediaTimeForSync"? Or maybe some comments would help.

Done.

https://codereview.chromium.org/534073002/diff/60001/media/filters/audio_rend...
File media/filters/audio_renderer_impl.cc (right):

https://codereview.chromium.org/534073002/diff/60001/media/filters/audio_rend...
media/filters/audio_renderer_impl.cc:159: return
audio_clock_->front_timestamp();
On 2014/09/08 22:56:06, xhwang wrote:
> Add a comment that since this is updated pretty frequently, we don't need
> interpolation? Is there any lower bound on the update frequency we can rely
on?

Done.

https://codereview.chromium.org/534073002/diff/60001/media/filters/audio_rend...
File media/filters/audio_renderer_impl.h (right):

https://codereview.chromium.org/534073002/diff/60001/media/filters/audio_rend...
media/filters/audio_renderer_impl.h:252: base::TimeTicks last_render_ticks_;
On 2014/09/08 22:56:06, xhwang wrote:
> As discussed offline, it would be nicer to add some comments about these
> variables.

Done.

https://codereview.chromium.org/534073002/diff/60001/media/filters/renderer_i...
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/534073002/diff/60001/media/filters/renderer_i...
media/filters/renderer_impl.cc:415: time_source_->StopTicking();
On 2014/09/08 22:56:06, xhwang wrote:
> How about making time_ticking() a method in TimeSource? Then you don't need a
> separate variable tracking the status.

I tried this out ... but decided to leave as is. We have 3 implementations of
TimeSource (Mock, WallClock, AudioRendererImpl) so having the bool tracking code
once inside of RendererImpl is nice. It also simplifies mocking and testing as
we don't need to replicate the behaviour of an IsTicking() method.

[xhwang, 2014-09-09 18:58:07]

lgtm

[scherkus (not reviewing), 2014-09-09 19:24:14]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-09-09 19:34:02]

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

[commit-bot: I haz the power, 2014-09-09 22:08:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-09 22:08:19]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[scherkus (not reviewing), 2014-09-09 22:08:52]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-09-09 22:11:21]

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

[commit-bot: I haz the power, 2014-09-09 23:14:25]

Committed patchset #3 (id:120001) as 4e437704f48acbe6cbd9a92bff4a12c23dff959a

[commit-bot: I haz the power, 2014-09-10 03:56:19]

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

[earthdok, 2014-09-10 13:46:54]

A revert of this CL (patchset #3 id:120001) has been created in
https://codereview.chromium.org/557333002/ by earthdok@chromium.org.

The reason for reverting is: ThreadSanitizer reports data races (see issue).

BUG=412764.