Use double microseconds for tracking back/front timestamp in AudioClock.

Use double microseconds for tracking back/front timestamp in AudioClock.

Back timestamp is computed by summing the new frames_written for every
call to WroteAudio. The number of microseconds per frame is often not
a whole number (e.g. 20.833 mu for sample rate of 48Khz). Prior to this
change, using TimeDelta to do the summing of frames_written meant we
truncated to the nearest microsecond with every call to WroteAudio. The
truncation error slowly accumulates in the back timestamp. After 2
hours of playback this error causes noticeable audio/video sync drift.

Having front_timestamp be a double is less critical. Front timestamp
is computed using back_timestamp at every call to WroteAudio, so fixing
back implicitly fixes front. Still, I've changed them both to double
for the sake of consistency and a slight improvement in accuracy.

BUG=564604
Committed: https://crrev.com/2ed08018de0593b905c18500c1784464dcbe5468
Cr-Commit-Position: refs/heads/master@{#376287}

[chcunningham, 2016-02-18 00:56:48]

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

[chcunningham, 2016-02-18 00:56:49]

This should fix the rest of the drift noticed in these 2-3 hour videos. Things
look good up to 2 hours with && without resampling. I'll let it play all the way
3 hours tomorrow when I have more time. 

Beyond 3 hours I think we still must have some small drift do to
truncating/rounding partial frames around AudioConverter (io_ratio) and the HW
latency being a number of msec that doesn't divide evenly into frames. Will
submit follow up cls for those.

[DaleCurtis, 2016-02-18 01:48:33]

Nice work! lgtm % nits.

https://codereview.chromium.org/1711473002/diff/20001/media/filters/audio_clo...
File media/filters/audio_clock.h (right):

https://codereview.chromium.org/1711473002/diff/20001/media/filters/audio_clo...
media/filters/audio_clock.h:136: // converting between frames-written/delayed
and time-passed (see WroteAudio).
WroteAudio().

https://codereview.chromium.org/1711473002/diff/20001/media/filters/audio_clo...
media/filters/audio_clock.h:137: // Particularly for back_timestamp,which
accumulates more time with each call
Use || around variable names and space after ","

https://codereview.chromium.org/1711473002/diff/20001/media/filters/audio_clo...
media/filters/audio_clock.h:138: // to WroteAudio, the loss of precision can
accumulate to create noticeable
WroteAudio()

https://codereview.chromium.org/1711473002/diff/20001/media/filters/audio_clo...
File media/filters/audio_clock_unittest.cc (right):

https://codereview.chromium.org/1711473002/diff/20001/media/filters/audio_clo...
media/filters/audio_clock_unittest.cc:76: SetupClock();
Instead of having every method call this, just call this during construction and
have the one test that wants something different reset the |clock_|.

https://codereview.chromium.org/1711473002/diff/20001/media/filters/audio_clo...
media/filters/audio_clock_unittest.cc:384:
static_cast<double>(base::Time::kMicrosecondsPerSecond) / 48000;
I'd just use 48000.0 to avoid the cast.

[chcunningham, 2016-02-18 20:43:37]

Thanks Dale!

https://codereview.chromium.org/1711473002/diff/20001/media/filters/audio_clo...
File media/filters/audio_clock.h (right):

https://codereview.chromium.org/1711473002/diff/20001/media/filters/audio_clo...
media/filters/audio_clock.h:136: // converting between frames-written/delayed
and time-passed (see WroteAudio).
On 2016/02/18 01:48:33, DaleCurtis wrote:
> WroteAudio().

Done.

https://codereview.chromium.org/1711473002/diff/20001/media/filters/audio_clo...
media/filters/audio_clock.h:137: // Particularly for back_timestamp,which
accumulates more time with each call
On 2016/02/18 01:48:33, DaleCurtis wrote:
> Use || around variable names and space after ","

Done.

https://codereview.chromium.org/1711473002/diff/20001/media/filters/audio_clo...
media/filters/audio_clock.h:138: // to WroteAudio, the loss of precision can
accumulate to create noticeable
On 2016/02/18 01:48:33, DaleCurtis wrote:
> WroteAudio()

Done.

https://codereview.chromium.org/1711473002/diff/20001/media/filters/audio_clo...
File media/filters/audio_clock_unittest.cc (right):

https://codereview.chromium.org/1711473002/diff/20001/media/filters/audio_clo...
media/filters/audio_clock_unittest.cc:76: SetupClock();
On 2016/02/18 01:48:33, DaleCurtis wrote:
> Instead of having every method call this, just call this during construction
and
> have the one test that wants something different reset the |clock_|.

Done.

https://codereview.chromium.org/1711473002/diff/20001/media/filters/audio_clo...
media/filters/audio_clock_unittest.cc:384:
static_cast<double>(base::Time::kMicrosecondsPerSecond) / 48000;
On 2016/02/18 01:48:33, DaleCurtis wrote:
> I'd just use 48000.0 to avoid the cast.

Done.

[chcunningham, 2016-02-18 20:44:01]

The CQ bit was checked by chcunningham@chromium.org

[chcunningham, 2016-02-18 20:44:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1711473002/#ps40001
(title: "Fixes for feedback && rebase.")

[commit-bot: I haz the power, 2016-02-18 20:49:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1711473002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1711473002/40001

[commit-bot: I haz the power, 2016-02-18 22:10:54]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-18 22:11:40]

Description was changed from

==========
Use double microseconds for tracking back/front timestamp in AudioClock.

Back timestamp is computed by summing the new frames_written for every
call to WroteAudio. The number of microseconds per frame is often not
a whole number (e.g. 20.833 mu for sample rate of 48Khz). Prior to this
change, using TimeDelta to do the summing of frames_written meant we
truncated to the nearest microsecond with every call to WroteAudio. The
truncation error slowly accumulates in the back timestamp. After 2
hours of playback this error causes noticeable audio/video sync drift.

Having front_timestamp be a double is less critical. Front timestamp
is computed using back_timestamp at every call to WroteAudio, so fixing
back implicitly fixes front. Still, I've changed them both to double
for the sake of consistency and a slight improvement in accuracy.

BUG=564604
==========

to

==========
Use double microseconds for tracking back/front timestamp in AudioClock.

Back timestamp is computed by summing the new frames_written for every
call to WroteAudio. The number of microseconds per frame is often not
a whole number (e.g. 20.833 mu for sample rate of 48Khz). Prior to this
change, using TimeDelta to do the summing of frames_written meant we
truncated to the nearest microsecond with every call to WroteAudio. The
truncation error slowly accumulates in the back timestamp. After 2
hours of playback this error causes noticeable audio/video sync drift.

Having front_timestamp be a double is less critical. Front timestamp
is computed using back_timestamp at every call to WroteAudio, so fixing
back implicitly fixes front. Still, I've changed them both to double
for the sake of consistency and a slight improvement in accuracy.

BUG=564604

Committed: https://crrev.com/2ed08018de0593b905c18500c1784464dcbe5468
Cr-Commit-Position: refs/heads/master@{#376287}
==========

[commit-bot: I haz the power, 2016-02-18 22:11:41]

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

[Kunihiko Sakamoto, 2016-02-19 06:54:23]

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

The reason for reverting is: Speculative revert: appears to break blink layout
test webaudio/periodicwave-normalization.html on Linux Debug.
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20(dbg)/....

[chcunningham, 2016-03-16 04:56:02]

On 2016/02/19 06:54:23, Kunihiko Sakamoto wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.chromium.org/1716753002/ by mailto:ksakamoto@chromium.org.
> 
> The reason for reverting is: Speculative revert: appears to break blink layout
> test webaudio/periodicwave-normalization.html on Linux Debug.
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20(dbg)/....

Just for posterity, this CL was not reverted (though I completely understand how
it was suspected). Landed without issue.