Implemented a proper clock for audio/video synchronization.

Implemented a proper clock for audio/video synchronization.

More or less a change to pull out time management from PipelineImpl into a new class ClockImpl.  Biggest difference is ClockImpl will use the system clock + linear interpolation to provide a more "precise" representation of the current playback position.

BUG=16508
TEST=a/v sync should remain the same, currentTime should report more precise values

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=21882

[awong, 2009-07-28 18:34:18]

couple of comments. Look pretty cool though!

http://codereview.chromium.org/159517/diff/11/13
File media/base/clock_impl.cc (right):

http://codereview.chromium.org/159517/diff/11/13#newcode13
Line 13: playback_rate_(1.0f) {
I thought the clock started off as paused with rate = 0.0? (Comment in clock.h"

http://codereview.chromium.org/159517/diff/11/14
File media/base/clock_impl.h (right):

http://codereview.chromium.org/159517/diff/11/14#newcode18
Line 18: // Static function pointer that acts as a time source.
Static -> Type for static

http://codereview.chromium.org/159517/diff/11/14#newcode31
Line 31: virtual base::TimeDelta Now() const;
I wonder if there's a better name than "Now."  "Now" to me wants to imply an
absolute time frame.  Can we call it "Elapsed()" or something like that?

http://codereview.chromium.org/159517/diff/11/14#newcode36
Line 36: base::TimeDelta NowUsingProvidedTime(const base::Time& time) const;
Can we call this "NowViaProvidedTime"?  The "NowUsing" parsed wrong for me...

http://codereview.chromium.org/159517/diff/11/15
File media/base/clock_impl_unittest.cc (right):

http://codereview.chromium.org/159517/diff/11/15#newcode17
Line 17: std::ostream& operator<<(std::ostream& stream, const base::TimeDelta&
time) {
Add a TODO to move this into the testing package?

http://codereview.chromium.org/159517/diff/11/15#newcode71
Line 71: EXPECT_EQ(kExpected, clock.Now());
This makes me wonder about the API a little.  There's basically no way to get t0
is there?  Shoudl Play() return the "start" time?

http://codereview.chromium.org/159517/diff/11/15#newcode80
Line 80: .WillOnce(Return(base::Time::FromDoubleT(8)));
Use a number other than 8 here so a playback rate of 1.0 doesn't mask this.

http://codereview.chromium.org/159517/diff/11/18
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/159517/diff/11/18#newcode28
Line 28: // catch up) looks worse.
This comment is confusing to me.  You are sleeping for 0 milliseconds aren't
you?

[scherkus (not reviewing), 2009-07-28 19:30:25]

http://codereview.chromium.org/159517/diff/11/14
File media/base/clock_impl.h (right):

http://codereview.chromium.org/159517/diff/11/14#newcode18
Line 18: // Static function pointer that acts as a time source.
On 2009/07/28 18:34:20, awong wrote:
> Static -> Type for static

Done.

http://codereview.chromium.org/159517/diff/11/14#newcode31
Line 31: virtual base::TimeDelta Now() const;
On 2009/07/28 18:34:20, awong wrote:
> I wonder if there's a better name than "Now."  "Now" to me wants to imply an
> absolute time frame.  Can we call it "Elapsed()" or something like that?

Yeah I was going for symmetry with time.h APIs, but technically we're monitoring
media time.  I like Elapsed()

http://codereview.chromium.org/159517/diff/11/14#newcode36
Line 36: base::TimeDelta NowUsingProvidedTime(const base::Time& time) const;
On 2009/07/28 18:34:20, awong wrote:
> Can we call this "NowViaProvidedTime"?  The "NowUsing" parsed wrong for me...

Done.

http://codereview.chromium.org/159517/diff/11/15
File media/base/clock_impl_unittest.cc (right):

http://codereview.chromium.org/159517/diff/11/15#newcode17
Line 17: std::ostream& operator<<(std::ostream& stream, const base::TimeDelta&
time) {
On 2009/07/28 18:34:20, awong wrote:
> Add a TODO to move this into the testing package?

Done.

http://codereview.chromium.org/159517/diff/11/15#newcode71
Line 71: EXPECT_EQ(kExpected, clock.Now());
On 2009/07/28 18:34:20, awong wrote:
> This makes me wonder about the API a little.  There's basically no way to get
t0
> is there?  Shoudl Play() return the "start" time?

Hmm.. well assuming it was paused you would call Elapsed() right?

We could have Play() and Pause() return Elapsed() for convenience.

http://codereview.chromium.org/159517/diff/11/18
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/159517/diff/11/18#newcode28
Line 28: // catch up) looks worse.
On 2009/07/28 18:34:20, awong wrote:
> This comment is confusing to me.  You are sleeping for 0 milliseconds aren't
> you?

nope -- I changed it to drop the frame and skip sleeping altogether when we're
behind

so in a way, if sleep must be calculated to be >= 0 in order to actually sleep
and not drop the frame

I guess we don't need the constant any more.. I'll move the comment below to
where we perform the check

[awong, 2009-07-28 19:35:15]

http://codereview.chromium.org/159517/diff/11/15
File media/base/clock_impl_unittest.cc (right):

http://codereview.chromium.org/159517/diff/11/15#newcode71
Line 71: EXPECT_EQ(kExpected, clock.Now());
On 2009/07/28 19:30:25, scherkus wrote:
> On 2009/07/28 18:34:20, awong wrote:
> > This makes me wonder about the API a little.  There's basically no way to
get
> t0
> > is there?  Shoudl Play() return the "start" time?
> 
> Hmm.. well assuming it was paused you would call Elapsed() right?
> 
> We could have Play() and Pause() return Elapsed() for convenience.

Yeah, that's was my thought.

If it was paused, calling Elapsed() works fine.  If it's playing, calling
Elapsed() is a short race.

[scherkus (not reviewing), 2009-07-28 19:48:23]

added convenience + tests

[awong, 2009-07-28 19:50:12]

LGTM