Pass delay and timestamp to AudioSourceCallback::OnMoreData.

Pass delay and timestamp to AudioSourceCallback::OnMoreData.

Replace audio delay in bytes with audio delay in time and the
timestamp at which the delay was measured. This is needed to
accurately synchronize playback across Cast devices in a multizone
group.

BUG=internal b/29618700
TEST=base, cast, cast_media, content, and media unit tests

Committed: https://crrev.com/3de19856c9d181a606df679384c6e0e84c4b0f39
Cr-Commit-Position: refs/heads/master@{#421999}

[jameswest, 2016-06-29 03:27:13]

Description was changed from

==========
Add delay timestamp to AudioSourceCallback::OnMoreData.

The timestamp is needed to accurately determine the output delay
during multizone playback on Cast devices.

BUG=internal b/29618700
==========

to

==========
Add delay timestamp to AudioSourceCallback::OnMoreData.

The timestamp is needed to accurately determine the output delay
during multizone playback on Cast devices.

BUG=internal b/29618700
==========

[jameswest, 2016-06-29 03:27:13]

jameswest@google.com changed reviewers:
+ alokp@chromium.org

[alokp, 2016-06-30 23:37:39]

https://codereview.chromium.org/2101303004/diff/20001/media/audio/audio_io.h
File media/audio/audio_io.h (right):

https://codereview.chromium.org/2101303004/diff/20001/media/audio/audio_io.h#...
media/audio/audio_io.h:69: base::TimeDelta delay_timestamp,
please add comments

[DaleCurtis, 2016-06-30 23:46:29]

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

[DaleCurtis, 2016-06-30 23:46:30]

Can you elaborate on why we need two delay values? I don't understand the use
case.

[jameswest, 2016-07-01 00:08:26]

On 2016/06/30 23:46:30, DaleCurtis wrote:
> Can you elaborate on why we need two delay values? I don't understand the use
> case.

I added a comment that hopefully makes it more clear. For synchronized multizone
playback, we need to convert total_bytes_delay to microseconds and then add it
to the time at which it was measured in order to determine the time when the
next data added will be played.

[jameswest, 2016-07-01 00:08:45]

https://codereview.chromium.org/2101303004/diff/20001/media/audio/audio_io.h
File media/audio/audio_io.h (right):

https://codereview.chromium.org/2101303004/diff/20001/media/audio/audio_io.h#...
media/audio/audio_io.h:69: base::TimeDelta delay_timestamp,
On 2016/06/30 23:37:39, alokp wrote:
> please add comments

Done.

[DaleCurtis, 2016-07-01 00:11:15]

So you need both a timestamp and a delay value? Why can't the output stream
handle that itself? I.e. can we just convert bytes_delay to a TimeDelta?

If not, why is that information that should be delivered via the output stream
though? It sounds like you should be using a secondary channel to deliver this
information that doesn't need to pierce so many layers.

[alokp, 2016-07-01 00:22:47]

alokp@chromium.org changed reviewers:
+ kmackay@chromium.org

[alokp, 2016-07-01 00:22:48]

+kmackay@ who has more context about the usage.

James has now added comment about the timestamp. It is not another delay value.
It is the timestamp at which total_bytes_delay was computed.

[kmackay, 2016-07-01 00:32:32]

Some more context: if you just have the delay value and not the time when the
delay was calculated, then by the time you go to actually write data, the actual
delay may be less (in some cases significantly less, depending on thread
scheduling). By adding the timestamp for the delay, it can be as accurate as
your clock.

Another option was to provide the current |delay| param with a timestamp for
when the filled buffer is expected to begin playing out (which is equivalent to
this CL's timestamp + delay). We thought that adding a timestamp would be less
intrusive.

[DaleCurtis, 2016-07-01 00:39:34]

Okay that makes sense, we don't generally use a TimeDelta for timestamps though.
Instead we use TimeTicks for this, should we be passing that down instead of a
delta?

[DaleCurtis, 2016-07-01 00:41:54]

Also, can you support your claims that this is necessary? Is this a cast only
type issue?

We use callbacks that typically occur every 10ms, so practically that would
imply that the delay value can't be that inaccurate (i.e. it must be on the
order of a few ms).

[kmackay, 2016-07-01 00:44:15]

On 2016/07/01 00:39:34, DaleCurtis wrote:
> Okay that makes sense, we don't generally use a TimeDelta for timestamps
though.
> Instead we use TimeTicks for this, should we be passing that down instead of a
> delta?

TimeTicks would be fine, but we would like to use CLOCK_MONOTONIC_RAW (currently
there isn't a way to specify the clock for TimeTicks). Also, we get the
timestamp as a timespec from ALSA; I'm not sure if there is an obvious way to
convert that to TimeTicks.

[kmackay, 2016-07-01 00:45:35]

On 2016/07/01 00:41:54, DaleCurtis wrote:
> Also, can you support your claims that this is necessary? Is this a cast only
> type issue?
> 
> We use callbacks that typically occur every 10ms, so practically that would
> imply that the delay value can't be that inaccurate (i.e. it must be on the
> order of a few ms).

We rely on the delay information being accurate to within 1ms (and currently we
expect it to be accurate to within 100 microseconds) for Cast multiroom audio.

[jameswest, 2016-07-01 00:46:11]

We use CLOCK_MONOTONIC_RAW, which TimeTicks doesn't support as far as I can
tell. 10 ms isn't accurate enough for us. We require accuracy to within 200
microseconds, and we currently get accuracy within about 10 microseconds on
Chromecast Audio devices.

[kmackay, 2016-07-01 00:47:07]

On 2016/07/01 00:46:11, James West wrote:
> We use CLOCK_MONOTONIC_RAW, which TimeTicks doesn't support as far as I can
> tell. 10 ms isn't accurate enough for us. We require accuracy to within 200
> microseconds, and we currently get accuracy within about 10 microseconds on
> Chromecast Audio devices.

James's numbers are a bit different from mine, but the point still stands :-)

[DaleCurtis, 2016-07-01 02:07:20]

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

[DaleCurtis, 2016-07-01 02:07:21]

+miu who knows all things timeticks :)

[DaleCurtis, 2016-07-01 02:07:57]

Thanks for the explanation, lets see if we can find a way to get TimeTicks into
here before moving forward with the TimeDelta. Which may be fine, but I'd love
miu@'s opinion first.

[DaleCurtis, 2016-07-07 20:26:04]

ping miu for thoughts :)

[alokp, 2016-07-13 19:41:26]

On 2016/07/07 20:26:04, DaleCurtis wrote:
> ping miu for thoughts :)

TimeTicks currently uses CLOCK_MONOTONIC:
https://cs.chromium.org/chromium/src/base/time/time_posix.cc?rcl=1468408847&l...

miu: Would it make sense to add support for CLOCK_MONOTONIC_RAW?

[miu, 2016-07-14 20:53:09]

On 2016/07/01 00:46:11, James West wrote:
> We use CLOCK_MONOTONIC_RAW, which TimeTicks doesn't support as far as I can
> tell. 10 ms isn't accurate enough for us. We require accuracy to within 200
> microseconds, and we currently get accuracy within about 10 microseconds on
> Chromecast Audio devices.

There's no need to use CLOCK_MONOTONIC_RAW instead of CLOCK_MONOTONIC. Both are
1us-resolution clocks (on most Linux/CrOS systems). CLOCK_MONOTONIC does not
skip backward or forward. It it simply gradually adjusts in frequency so that it
converges on ticking at the same rate as the "rest of the world, according to
NTP." The adjustments are usually on the order of a few nanoseconds per second.
;-)

Looking at the rest of CL and comment thread now...

[miu, 2016-07-14 21:06:28]

On 2016/07/14 20:53:09, miu wrote:
> On 2016/07/01 00:46:11, James West wrote:
> > We use CLOCK_MONOTONIC_RAW, which TimeTicks doesn't support as far as I can
> 
> There's no need to use CLOCK_MONOTONIC_RAW instead of CLOCK_MONOTONIC. Both
are

Actually, to be fair, there's no reason why we couldn't switch base::TimeTicks
to use the RAW variant. That would make the TimeTicks behavior on POSIX
platforms equivalent to that on Windows (which uses the
QueryPerformanceCounter() that does not change in frequency). My point in the
prior comment was just to show it probably doesn't matter in terms of the
performance of your feature. ;-)

[miu, 2016-07-14 21:08:55]

https://codereview.chromium.org/2101303004/diff/40001/media/audio/audio_io.h
File media/audio/audio_io.h (right):

https://codereview.chromium.org/2101303004/diff/40001/media/audio/audio_io.h#...
media/audio/audio_io.h:70: base::TimeDelta delay_timestamp,
If "delay_timestamp is the time at which total_bytes_delay was measured," then
this doesn't make sense. base::TimeDelta is the time type for measuring
*duration*. TimeTicks would be appropriate for point-in-time values.

I was reading earlier comments, and am still a bit confused: Isn't audio
rendered synchronously in this call? Meaning, the OS audio library calls into
our "glue code" which calls OnMoreData(), and the entire call stack is executing
on a dedicated, realtime audio thread? If not, can you provide a link to code
which is asynchronous, or for some reason is "rendering" audio with some
unaccounted-for delay? In other words, when/why would this not be
base::TimeTicks::Now()?

Also, there are a number of problems with the existing interface:

1. |total_bytes_delay| is something we should get rid of: Client code works with
AudioBus data, which is only 32-bit float planar; and therefore it's irrelevant
to force client code to deal with concepts related to other audio formats. We
should not burden client code with having to do the math to compute delay times
from a byte count. This is a legacy interface issue...

2. The existing interface violates the C++ style guide in multiple ways: a) The
output argument should go last. b) uint32_t (for frames_skipped) is
inappropriate because it is not a size_t, nor used as a bit field or in
wire-formatting data for serialization.

Putting it all together, this would be a much better API:

  virtual int OnMoreData(
      base::TimeDelta delay_until_playout,
      int prior_frames_skipped,
      AudioBus* dest);

Then, the implementation of OnMoreData() knows it is rendering audio such that
the first sample in the AudioBus is targeting* a point-in-time of:

  base::TimeTicks playout_time = base::TimeTicks::Now() + delay_until_playout;

* targeting here means: Used as a reference time to detect when adjustments need
to be made (such as stretching/compressing audio in the time domain, or skipping
frames), and not as a precise required playout time.

[kmackay, 2016-07-14 21:12:50]

On 2016/07/14 21:06:28, miu wrote:
> On 2016/07/14 20:53:09, miu wrote:
> > On 2016/07/01 00:46:11, James West wrote:
> > > We use CLOCK_MONOTONIC_RAW, which TimeTicks doesn't support as far as I
can
> > 
> > There's no need to use CLOCK_MONOTONIC_RAW instead of CLOCK_MONOTONIC. Both
> are
> 
> Actually, to be fair, there's no reason why we couldn't switch base::TimeTicks
> to use the RAW variant. That would make the TimeTicks behavior on POSIX
> platforms equivalent to that on Windows (which uses the
> QueryPerformanceCounter() that does not change in frequency). My point in the
> prior comment was just to show it probably doesn't matter in terms of the
> performance of your feature. ;-)

We picked CLOCK_MONOTONIC_RAW specifically so that the frequency doesn't change.
As I understand it, the CLOCK_MONOTONIC frequency may be slewed at up to 500
ppm, which is pretty bad for our use case.

[kmackay, 2016-07-14 21:18:52]

On 2016/07/14 21:08:55, miu wrote:
> Putting it all together, this would be a much better API:
> 
>   virtual int OnMoreData(
>       base::TimeDelta delay_until_playout,
>       int prior_frames_skipped,
>       AudioBus* dest);
> 
> Then, the implementation of OnMoreData() knows it is rendering audio such that
> the first sample in the AudioBus is targeting* a point-in-time of:
> 
>   base::TimeTicks playout_time = base::TimeTicks::Now() + delay_until_playout;
> 
> * targeting here means: Used as a reference time to detect when adjustments
need
> to be made (such as stretching/compressing audio in the time domain, or
skipping
> frames), and not as a precise required playout time.

Now() + delay_until_playout isn't accurate enough for our use case, since
delay_until_playout will change between the time it is calculated and the time
when more audio data is pushed. For example, the thread may be switched out
between computing delay_until_timeout and the call to Now(), which could cause
an error of several milliseconds. What we really wanted was 'base::TimeTicks
next_playout_time' (this can be accurately computed by the audio driver).

[miu, 2016-07-19 02:04:17]

On 2016/07/14 21:12:50, kmackay wrote:
> We picked CLOCK_MONOTONIC_RAW specifically so that the frequency doesn't
change.
> As I understand it, the CLOCK_MONOTONIC frequency may be slewed at up to 500
> ppm, which is pretty bad for our use case.

That seems way high: 500 ppm is 1/2 a millisecond per second, after all. To put
things in perspective, one of the worst clocks in today's computers would be the
CMOS RTC clock, and it usually drifts less than 10 seconds per day, which is
10/86400*1000000 = ~116 ppm.

Regardless, let's take a step back: Are you really sure you need such a
high-precision clock? Because, frankly, it's simply not possible due to today's
OS and hardware support on all the "commodity platforms" Chrome runs on. I can
explain further, but here's one example:
https://cs.chromium.org/chromium/src/base/time/time_win.cc?rcl=0&l=442

For Cast Streaming, we accept that the system clock provides reference
timestamps that will jitter and skew over time. Furthermore, we accept that
sender and receiver are trying to "sync-up" their clocks, each of which skew and
jitter in different ways. Finally, synchronizing the clocks between the two is
being done by passing messages over a network with unpredictable packet
round-trip times. Thus, the timestamps from the system clock will always, at
best, be a "goal" of sorts and not a to-the-microsecond moment when an exact
audio sample should be playing out.

Therefore, I would suggest your system use the count of audio samples to
determine an "exact" passage of time. However, a receiver will have to modify
the audio signal by adding or removing audio samples at playback time, when the
playback rate of the media turns out to be too fast/slow with respect to the
reference timestamps. Example:
https://cs.chromium.org/chromium/src/media/base/audio_shifter.h?rcl=0&l=23

[miu, 2016-07-19 02:15:14]

On 2016/07/14 21:18:52, kmackay wrote:
> Now() + delay_until_playout isn't accurate enough for our use case, since
> delay_until_playout will change between the time it is calculated and the time
> when more audio data is pushed. For example, the thread may be switched out
> between computing delay_until_timeout and the call to Now(), which could cause
> an error of several milliseconds. What we really wanted was 'base::TimeTicks
> next_playout_time' (this can be accurately computed by the audio driver).

Good point. It's fine if we define the API to pass the "target_playout_time."
I'm guessing that's what "total_bytes_delay" was supposed to do, but it ignores
the length of time it will take before the audio starts rendering *and* is
delivered all the way back to the sound hardware buffers, *and* that the sound
buffers have been draining in the meantime.

So, what do you think of:

  virtual int OnMoreData(
      base::TimeTicks target_playout_time,
      int prior_frames_skipped,
      AudioBus* dest);

As I discussed in my prior comments, the code that renders the audio would only
skip/stretch/compress the audio signal if |target_playout_time| is drifting too
far off-target from what it expects the next sample's playout time to be.

BTW--As for thread context switching and time sampling: You'll have this problem
even if everything is running on a single thread. It's why we've had to write
"fancy" unit tests like these:
https://cs.chromium.org/chromium/src/base/time/time_unittest.cc?rcl=0&l=658

[kmackay, 2016-07-19 21:07:24]

On 2016/07/19 02:04:17, miu wrote:
> On 2016/07/14 21:12:50, kmackay wrote:
> > We picked CLOCK_MONOTONIC_RAW specifically so that the frequency doesn't
> change.
> > As I understand it, the CLOCK_MONOTONIC frequency may be slewed at up to 500
> > ppm, which is pretty bad for our use case.
> 
> That seems way high: 500 ppm is 1/2 a millisecond per second, after all.

ntpd is allowed to slew the clock at up to 500 ppm. As I understand it, this
slew affects CLOCK_MONOTONIC.

> 
> Regardless, let's take a step back: Are you really sure you need such a
> high-precision clock? Because, frankly, it's simply not possible due to
today's
> OS and hardware support on all the "commodity platforms" Chrome runs on. I can
> explain further, but here's one example:
> https://cs.chromium.org/chromium/src/base/time/time_win.cc?rcl=0&l=442
> 
> For Cast Streaming, we accept that the system clock provides reference
> timestamps that will jitter and skew over time. Furthermore, we accept that
> sender and receiver are trying to "sync-up" their clocks, each of which skew
and
> jitter in different ways. Finally, synchronizing the clocks between the two is
> being done by passing messages over a network with unpredictable packet
> round-trip times. Thus, the timestamps from the system clock will always, at
> best, be a "goal" of sorts and not a to-the-microsecond moment when an exact
> audio sample should be playing out.

Our current audio synchronization across wifi is within 200 microseconds (and
our audio timestamps are accurate to within 100 microseconds). We already have
this implemented on Cast Audio platforms. We'd like to start using public
Chromium interfaces, but we don't want to decrease our performance by doing so.

[kmackay, 2016-07-19 21:09:59]

On 2016/07/19 02:15:14, miu wrote:
> So, what do you think of:
> 
>   virtual int OnMoreData(
>       base::TimeTicks target_playout_time,
>       int prior_frames_skipped,
>       AudioBus* dest);
> 

SGTM

[alokp, 2016-07-20 04:53:01]

To summarize, we need to:

1. Switch base::TimeTicks to use CLOCK_MONOTONIC_RAW
2. Change OnMoreData interface to accept target_playout_time instead of
total_bytes_delay

miu@, dalecurtis@, kmackay@: Can you guys confirm if this is accurate?

[miu, 2016-07-20 19:21:10]

On 2016/07/20 04:53:01, alokp wrote:
> To summarize, we need to:
> 
> 1. Switch base::TimeTicks to use CLOCK_MONOTONIC_RAW

Yes, this sounds good. In fact, we should do this to make CrOS and Linux have
the same behavior as Win/Mac. Feel free to put up a code change and assign to me
for review (I am the owner of base/time/*).

> 2. Change OnMoreData interface to accept target_playout_time instead of
> total_bytes_delay

SGTM too. :)

[miu, 2016-07-20 19:27:51]

On 2016/07/19 21:07:24, kmackay wrote:
> Our current audio synchronization across wifi is within 200 microseconds (and
> our audio timestamps are accurate to within 100 microseconds). We already have
> this implemented on Cast Audio platforms. We'd like to start using public
> Chromium interfaces, but we don't want to decrease our performance by doing
so.

That's impressive! :) I'd love to chat about your synch methods some time.

BTW--Thanks for doing this work on the Chromium interfaces. The "legacy way"
we've been handling issues of time, delay, A/V sync, etc. have long been in bad
need of fixing. And, ideally these changes will improve performance for all
audio features (since your use case is setting a very high bar).

[jameswest, 2016-07-26 23:36:47]

I updated the interface to use target_playout_time. I created a separate CL for
changing TimeTicks to use CLOCK_MONOTONIC_RAW:
https://codereview.chromium.org/2172483002/.

[jameswest, 2016-07-26 23:39:01]

Description was changed from

==========
Add delay timestamp to AudioSourceCallback::OnMoreData.

The timestamp is needed to accurately determine the output delay
during multizone playback on Cast devices.

BUG=internal b/29618700
==========

to

==========
Pass target playout time to AudioSourceCallback::OnMoreData.

Replace audio delay in bytes with the timestamp at which the next
sample provided is expected to be played. This is needed to accurately
synchronize playback across Cast devices in a multizone group.

BUG=internal b/29618700
TEST=base_unittests, media_unittests
==========

[DaleCurtis, 2016-07-27 18:22:38]

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

[DaleCurtis, 2016-07-27 18:22:39]

=> chcunningham to do media/ review.

[DaleCurtis, 2016-07-27 18:25:55]

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
target_playout_time - base::TimeTicks::Now();
Hmm, if you don't pipe this through to the renderer I don't understand the point
of this change. Or are you really saying that you actually see noticeable
differences between the output device and this point? I can't imagine more than
a handle of microseconds have passed prior to this point.

[chcunningham1, 2016-07-27 21:34:43]

On 2016/07/27 18:25:55, DaleCurtis wrote:
>
https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
> File media/audio/audio_output_controller.cc (right):
> 
>
https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
> media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
> target_playout_time - base::TimeTicks::Now();
> Hmm, if you don't pipe this through to the renderer I don't understand the
point
> of this change. Or are you really saying that you actually see noticeable
> differences between the output device and this point? I can't imagine more
than
> a handle of microseconds have passed prior to this point.

+1 to this question

[jameswest, 2016-07-27 23:07:50]

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
target_playout_time - base::TimeTicks::Now();
We don't use this renderer for Cast multizone audio playback. My only goal with
this CL is to change the AudioOutputStream API so that our multizone code can
use CastAudioOutputStream instead of using the underlying media pipeline backend
directly. In order to do that, we need to pass the accurate timing data from the
media pipeline backend up to our multizone code through CastAudioOutputStream. I
was trying to change the API without changing any existing behavior in Chromium.

[DaleCurtis, 2016-07-28 01:21:47]

Ah, in that case this seems okay to me. For http://crbug.com/619533 and
https://codereview.chromium.org/2060833002 they'll have to plumb it the rest of
the way.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
target_playout_time - base::TimeTicks::Now();
On 2016/07/27 at 23:07:50, James West wrote:
> We don't use this renderer for Cast multizone audio playback. My only goal
with this CL is to change the AudioOutputStream API so that our multizone code
can use CastAudioOutputStream instead of using the underlying media pipeline
backend directly. In order to do that, we need to pass the accurate timing data
from the media pipeline backend up to our multizone code through
CastAudioOutputStream. I was trying to change the API without changing any
existing behavior in Chromium.

I thought this API was still used with WebAudio? Or is that unsupported on Cast
still?

[jameswest, 2016-07-28 02:09:39]

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
target_playout_time - base::TimeTicks::Now();
On 2016/07/28 01:21:47, DaleCurtis wrote:
> On 2016/07/27 at 23:07:50, James West wrote:
> > We don't use this renderer for Cast multizone audio playback. My only goal
> with this CL is to change the AudioOutputStream API so that our multizone code
> can use CastAudioOutputStream instead of using the underlying media pipeline
> backend directly. In order to do that, we need to pass the accurate timing
data
> from the media pipeline backend up to our multizone code through
> CastAudioOutputStream. I was trying to change the API without changing any
> existing behavior in Chromium.
> 
> I thought this API was still used with WebAudio? Or is that unsupported on
Cast
> still?

It's not used for multizone audio playback, but I can't say for certain that
this isn't used at all on any Cast device because some audio that isn't
synchronized across devices is output differently. If we do use it for
something, improving the accuracy isn't important.

I'm not opposed to passing target_playout_time into the renderer if you think
it's best to, but we don't need it for our use case.

[kmackay, 2016-07-28 02:17:19]

On 2016/07/28 02:09:39, James West wrote:
>
https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
> File media/audio/audio_output_controller.cc (right):
> 
>
https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
> media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
> target_playout_time - base::TimeTicks::Now();
> On 2016/07/28 01:21:47, DaleCurtis wrote:
> > On 2016/07/27 at 23:07:50, James West wrote:
> > > We don't use this renderer for Cast multizone audio playback. My only goal
> > with this CL is to change the AudioOutputStream API so that our multizone
code
> > can use CastAudioOutputStream instead of using the underlying media pipeline
> > backend directly. In order to do that, we need to pass the accurate timing
> data
> > from the media pipeline backend up to our multizone code through
> > CastAudioOutputStream. I was trying to change the API without changing any
> > existing behavior in Chromium.
> > 
> > I thought this API was still used with WebAudio? Or is that unsupported on
> Cast
> > still?
> 
> It's not used for multizone audio playback, but I can't say for certain that
> this isn't used at all on any Cast device because some audio that isn't
> synchronized across devices is output differently. If we do use it for
> something, improving the accuracy isn't important.
> 
> I'm not opposed to passing target_playout_time into the renderer if you think
> it's best to, but we don't need it for our use case.

We do support WebAudio, but not with multiroom (yet).

[chcunningham, 2016-07-29 01:21:10]

Comments incoming ;)

https://codereview.chromium.org/2101303004/diff/60001/content/browser/media/c...
File content/browser/media/capture/web_contents_audio_muter.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_audio_muter.cc:56:
callback->OnMoreData(base::TimeTicks(), 0, audio_bus_.get());
Should the be base::TimeTicks::Now()?

I'm not familiar with this class, but with it historically reporting a delay of
0, I think Now() is the right value to use for target_playout_time.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/alsa/alsa_o...
File media/audio/alsa/alsa_output_unittest.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/alsa/alsa_o...
media/audio/alsa/alsa_output_unittest.cc:598: EXPECT_CALL(mock_callback,
OnMoreData(_, 0, _))
Here they're setting the PcmDelay out argument to 0, so target_playout_time be
roughly equal to Now. Not sure if there's a good way to expect that things are
roughly equal though...

https://codereview.chromium.org/2101303004/diff/60001/media/audio/alsa/alsa_o...
media/audio/alsa/alsa_output_unittest.cc:624: EXPECT_CALL(mock_callback,
OnMoreData(_, 0, _))
This target playout time should also roughly equal Ticks::Now

https://codereview.chromium.org/2101303004/diff/60001/media/audio/alsa/alsa_o...
media/audio/alsa/alsa_output_unittest.cc:648: EXPECT_CALL(mock_callback,
OnMoreData(_, 0, _))
Its weird that this one was ever 0 - IIUC they aren't setting the PcmDelay
expectation for this method to SetARgumentPointee<1>(0) as they do in the
StartStop case. I think its fine to ignore the delay value here...

https://codereview.chromium.org/2101303004/diff/60001/media/audio/android/ope...
File media/audio/android/opensles_output.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/android/ope...
media/audio/android/opensles_output.cc:346: ? base::TimeTicks::Now() -
I don't follow your math here. I think what you want is to compute the delay as
we did before, then compute the target_playout_time = Now + delay. (same as in
also_output.cc)

https://codereview.chromium.org/2101303004/diff/60001/media/audio/android/ope...
media/audio/android/opensles_output.cc:348: : base::TimeTicks();
I don't think you want 0 ticks here. I think you want Now()

https://codereview.chromium.org/2101303004/diff/60001/media/audio/android/ope...
media/audio/android/opensles_output.cc:349: DCHECK_GE(target_playout_time,
base::TimeTicks());
Still use this to check delay >= 0 if you take my recommendation for lines 346

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_low_l...
File media/audio/audio_low_latency_input_output_unittest.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_low_l...
media/audio/audio_low_latency_input_output_unittest.cc:231: base::TimeDelta
output_delay = target_playout_time - base::TimeTicks::Now();
Guard against negative?

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
target_playout_time - base::TimeTicks::Now();
Should we be gaurding against negative values here. delay = max(math above, 0)? 

Miu, Dale and I had conversations about negative delay in the past. We never
really reached consensus, but Dale/my position is that it isn't reasonable. See
here https://codereview.chromium.org/2004283002/

I think that small values of negative delay may creep in without a guard for
output streams that don't have delay (and use an earlier call to Now() for their
target_playout_time) - see my comments in web_contents_audio_muter.cc

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_controller_unittest.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_controller_unittest.cc:204:
mock_stream_.callback()->OnMoreData(base::TimeTicks(), 0, dest.get());
Now?

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_controller_unittest.cc:218:
controller_->OnMoreData(base::TimeTicks(), 0, dest.get());
Now?

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_resampler.cc (left):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_resampler.cc:390: // TODO(dalecurtis): Stop passing
bytes around, it doesn't make sense since
Why is it ok for you to just drop frames_delayed? I think you need to factor
this into target_playout_time as additional delay.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/fake_audio_...
File media/audio/fake_audio_input_stream.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:102:
audio_source_->OnMoreData(base::TimeTicks(), 0, audio_bus_.get());
Now?

https://codereview.chromium.org/2101303004/diff/60001/media/audio/fake_audio_...
File media/audio/fake_audio_output_stream.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/fake_audio_...
media/audio/fake_audio_output_stream.cc:67:
callback_->OnMoreData(base::TimeTicks(), 0, audio_bus_.get());
Now?

https://codereview.chromium.org/2101303004/diff/60001/media/audio/mac/audio_a...
File media/audio/mac/audio_auhal_mac.cc (left):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/mac/audio_a...
media/audio/mac/audio_auhal_mac.cc:340: if ((output_time_stamp->mFlags &
kAudioTimeStampHostTimeValid) == 0)
This method had checks for validity of host time... these checks should probably
stay in your new usage.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/mac/audio_a...
media/audio/mac/audio_auhal_mac.cc:351: if (now_ns > output_time_ns)
I think you need to still account for this

https://codereview.chromium.org/2101303004/diff/60001/media/audio/pulse/pulse...
File media/audio/pulse/pulse_util.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/pulse/pulse...
media/audio/pulse/pulse_util.cc:1: 
space

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
File media/audio/simple_sources_unittest.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:35: source.OnMoreData(base::TimeTicks(),
0, audio_bus.get());
Now?

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:64:
EXPECT_EQ(source.OnMoreData(base::TimeTicks(), 0, audio_bus.get()),
Now?

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:67:
EXPECT_EQ(source.OnMoreData(base::TimeTicks(), 0, audio_bus.get()), 0);
Now?

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:70:
EXPECT_EQ(source.OnMoreData(base::TimeTicks(), 0, audio_bus.get()),
Now?

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:131: source.OnMoreData(base::TimeTicks(),
0, audio_bus.get()));
Now?

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:137: source.OnMoreData(base::TimeTicks(),
0, audio_bus.get());
Now?

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:167: source.OnMoreData(base::TimeTicks(),
0, audio_bus.get());
Now?

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:169: source.OnMoreData(base::TimeTicks(),
0, audio_bus.get());
Now?

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:187: EXPECT_EQ(0,
source.OnMoreData(base::TimeTicks(), 0, audio_bus.get()));
Now?

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:222: EXPECT_EQ(0,
source.OnMoreData(base::TimeTicks(), 0, audio_bus.get()));
Now?

[chcunningham, 2016-07-29 01:24:12]

https://codereview.chromium.org/2101303004/diff/60001/content/browser/media/c...
File content/browser/media/capture/web_contents_audio_muter.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_audio_muter.cc:56:
callback->OnMoreData(base::TimeTicks(), 0, audio_bus_.get());
On 2016/07/29 01:21:08, chcunningham wrote:
> Should the be base::TimeTicks::Now()?
> 
> I'm not familiar with this class, but with it historically reporting a delay
of
> 0, I think Now() is the right value to use for target_playout_time.

Sorry, I didn't say "why" - when you look at how you're computing the
target_playout_time in the various AudioOutputStreams, its generally
Ticks::Now() + delay. So in cases where you have 0 delay, its logical for the
target time to simply be Now()

[miu, 2016-07-30 21:17:02]

Catching up on code reviews. Sorry for the delay.

However, it's not clear whether we are attempting to land this change or
https://codereview.chromium.org/2060833002/. Which one?

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
target_playout_time - base::TimeTicks::Now();
On 2016/07/29 01:21:09, chcunningham wrote:
> Should we be gaurding against negative values here. delay = max(math above,
0)? 
> 
> I think that small values of negative delay may creep in without a guard for
> output streams that don't have delay (and use an earlier call to Now() for
their
> target_playout_time) - see my comments in web_contents_audio_muter.cc

If negative delay creeps in, for output streams where it does not make sense,
that would indicate that there are bugs in the code (e.g., erroneous time math,
rounding errors, etc.). Do we want a guard, which might mask an underlying
problem? Perhaps the guard should be in the form of a DCHECK instead?

[miu, 2016-07-30 22:45:00]

Bringing over the comment from https://codereview.chromium.org/2172483002...

On 2016/07/28 01:31:31, James West wrote:
> I think we have two options then:
> 
> 1) Continue to use TimeTicks with CLOCK_MONOTONIC in
> AudioSourceCallback::OnMoreData. On Cast devices the actual timestamp will
come
> from ALSA or the SoC as a raw integer representing CLOCK_MONOTONIC_RAW. We can
> pass that integer via TimeTicks using the serialization/deserialization
methods.
> It's hacky, but the hack will be isolated to internal Chromecast code.

This would be both a semantics and layering violation: TimeTicks is a value
type, different from Time and ThreadTicks, that exists explicitly to represent
values that are coming from the same clock (i.e., having the same origin point,
and behavioral semantics). This allows us to have the compiler ensure the
comparison and math between time values is semantically valid. FWIW, there have
been multiple horrendous bugs (i.e., near impossible to repro, but significant
timing performance issues) in the past caused by some who chose to circumvent
this design structure.

> 2) Create a new class, say AudioTimestamp, that is used in
> AudioSourceCallback::OnMoreData and explicitly supports both use cases.

So, while I really like the API change you're proposing for OnMoreData(), I'm
not convinced an "AudioTimestamp," nor your proposed use of CLOCK_MONOTONIC_RAW,
will do anything to help "accurately synchronize playback across Cast devices in
a multizone group." If I'm missing something, can you explain? Perhaps there is
a design doc somewhere explaining this?

BTW--I say this after re-reading the prior discussion in this code review and
also taking a look at the CastAudioOutputStream code. What's interesting is that
CastAudioOutputStream seems to treat the TimeTicks-based TimeDelta values
exactly the way I described in my earlier comments on this code review: Meaning,
that the delay value passed to OnMoreData() is w.r.t. to a reference clock, and
treated as a "approximate target" to guide audio rendering, but not an
exact-to-the-sample point-in-time. Furthermore, there are plenty of other >500
usec sources of error in the current implementation of CastAudioOutputStream.
For example:

1. The kernel may decide to pre-empt the thread at any time, such as during or
just after a call to TimeTicks::Now(), and the duration of that suspension would
be subject unpredictable externalities. This means that, from the code's POV,
the time value could occasionally be >1 millisecond old when it is being used
for OnMoreData().

2. The audio is not being rendered on a thread with realtime priority (e.g., see
media/audio/audio_device_thread.h).

3. The use of TaskRunner::PostDelayedTask() makes no guarantees about how late a
task will be run. It only guarantees a task won't be run until a certain amount
of time has passed. It's accuracy depends on the event loop implementation as
well as how many tasks are in front the CastAOS task in the task queue.

4. The use of media::AudioTimestampHelper isn't accounting for clock skew
between the audio hardware's clock and the system monotonic clock. Also, I'd
imagine the skew between "decode timestamps" and the other Chromecasts' clocks
is being accounted for somewhere downstream? Therefore, how will improving the
accuracy of the "TimeTicks" clock change anything?

[kmackay, 2016-07-31 06:16:38]

On 2016/07/30 22:45:00, miu wrote:
> Bringing over the comment from https://codereview.chromium.org/2172483002...
> 
> On 2016/07/28 01:31:31, James West wrote:
> > I think we have two options then:
> > 
> > 1) Continue to use TimeTicks with CLOCK_MONOTONIC in
> > AudioSourceCallback::OnMoreData. On Cast devices the actual timestamp will
> come
> > from ALSA or the SoC as a raw integer representing CLOCK_MONOTONIC_RAW. We
can
> > pass that integer via TimeTicks using the serialization/deserialization
> methods.
> > It's hacky, but the hack will be isolated to internal Chromecast code.
> 
> This would be both a semantics and layering violation: TimeTicks is a value
> type, different from Time and ThreadTicks, that exists explicitly to represent
> values that are coming from the same clock (i.e., having the same origin
point,
> and behavioral semantics). This allows us to have the compiler ensure the
> comparison and math between time values is semantically valid. FWIW, there
have
> been multiple horrendous bugs (i.e., near impossible to repro, but significant
> timing performance issues) in the past caused by some who chose to circumvent
> this design structure.
> 
> > 2) Create a new class, say AudioTimestamp, that is used in
> > AudioSourceCallback::OnMoreData and explicitly supports both use cases.
> 
> So, while I really like the API change you're proposing for OnMoreData(), I'm
> not convinced an "AudioTimestamp," nor your proposed use of
CLOCK_MONOTONIC_RAW,
> will do anything to help "accurately synchronize playback across Cast devices
in
> a multizone group." If I'm missing something, can you explain? Perhaps there
is
> a design doc somewhere explaining this?
> 
> BTW--I say this after re-reading the prior discussion in this code review and
> also taking a look at the CastAudioOutputStream code. What's interesting is
that
> CastAudioOutputStream seems to treat the TimeTicks-based TimeDelta values
> exactly the way I described in my earlier comments on this code review:
Meaning,
> that the delay value passed to OnMoreData() is w.r.t. to a reference clock,
and
> treated as a "approximate target" to guide audio rendering, but not an
> exact-to-the-sample point-in-time. Furthermore, there are plenty of other >500
> usec sources of error in the current implementation of CastAudioOutputStream.
> For example:
> 
> 1. The kernel may decide to pre-empt the thread at any time, such as during or
> just after a call to TimeTicks::Now(), and the duration of that suspension
would
> be subject unpredictable externalities. This means that, from the code's POV,
> the time value could occasionally be >1 millisecond old when it is being used
> for OnMoreData().
> 
> 2. The audio is not being rendered on a thread with realtime priority (e.g.,
see
> media/audio/audio_device_thread.h).
> 
> 3. The use of TaskRunner::PostDelayedTask() makes no guarantees about how late
a
> task will be run. It only guarantees a task won't be run until a certain
amount
> of time has passed. It's accuracy depends on the event loop implementation as
> well as how many tasks are in front the CastAOS task in the task queue.
> 
> 4. The use of media::AudioTimestampHelper isn't accounting for clock skew
> between the audio hardware's clock and the system monotonic clock. Also, I'd
> imagine the skew between "decode timestamps" and the other Chromecasts' clocks
> is being accounted for somewhere downstream? Therefore, how will improving the
> accuracy of the "TimeTicks" clock change anything?

CastAudioOutputStream is not used in general for casted audio content; we use
the CMA output pipeline. We only support multiroom audio via the CMA pipeline,
not through CastAudioOutputStream.

[kmackay, 2016-07-31 06:48:46]

On 2016/07/31 06:16:38, kmackay wrote:
> CastAudioOutputStream is not used in general for casted audio content; we use
> the CMA output pipeline. We only support multiroom audio via the CMA pipeline,
> not through CastAudioOutputStream.

And the reason for this API change is so that we can start using
AudioOutputStream internally for multiroom output. The code to do so has not yet
been written.

[jameswest, 2016-08-26 02:08:48]

https://codereview.chromium.org/2101303004/diff/60001/content/browser/media/c...
File content/browser/media/capture/web_contents_audio_muter.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/content/browser/media/c...
content/browser/media/capture/web_contents_audio_muter.cc:56:
callback->OnMoreData(base::TimeTicks(), 0, audio_bus_.get());
On 2016/07/29 01:24:12, chcunningham wrote:
> On 2016/07/29 01:21:08, chcunningham wrote:
> > Should the be base::TimeTicks::Now()?
> > 
> > I'm not familiar with this class, but with it historically reporting a delay
> of
> > 0, I think Now() is the right value to use for target_playout_time.
> 
> Sorry, I didn't say "why" - when you look at how you're computing the
> target_playout_time in the various AudioOutputStreams, its generally
> Ticks::Now() + delay. So in cases where you have 0 delay, its logical for the
> target time to simply be Now()
> 

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/alsa/alsa_o...
File media/audio/alsa/alsa_output_unittest.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/alsa/alsa_o...
media/audio/alsa/alsa_output_unittest.cc:598: EXPECT_CALL(mock_callback,
OnMoreData(_, 0, _))
On 2016/07/29 01:21:08, chcunningham wrote:
> Here they're setting the PcmDelay out argument to 0, so target_playout_time be
> roughly equal to Now. Not sure if there's a good way to expect that things are
> roughly equal though...

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/alsa/alsa_o...
media/audio/alsa/alsa_output_unittest.cc:624: EXPECT_CALL(mock_callback,
OnMoreData(_, 0, _))
On 2016/07/29 01:21:08, chcunningham wrote:
> This target playout time should also roughly equal Ticks::Now

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/alsa/alsa_o...
media/audio/alsa/alsa_output_unittest.cc:648: EXPECT_CALL(mock_callback,
OnMoreData(_, 0, _))
On 2016/07/29 01:21:08, chcunningham wrote:
> Its weird that this one was ever 0 - IIUC they aren't setting the PcmDelay
> expectation for this method to SetARgumentPointee<1>(0) as they do in the
> StartStop case. I think its fine to ignore the delay value here...

It doesn't call PcmDelay because PcmState returns SND_PCM_STATE_XRUN, which
results in the delay returned by GetCurrentDelay being 0.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/android/ope...
File media/audio/android/opensles_output.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/android/ope...
media/audio/android/opensles_output.cc:346: ? base::TimeTicks::Now() -
On 2016/07/29 01:21:09, chcunningham wrote:
> I don't follow your math here. I think what you want is to compute the delay
as
> we did before, then compute the target_playout_time = Now + delay. (same as in
> also_output.cc)
> 

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/android/ope...
media/audio/android/opensles_output.cc:348: : base::TimeTicks();
On 2016/07/29 01:21:09, chcunningham wrote:
> I don't think you want 0 ticks here. I think you want Now()

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/android/ope...
media/audio/android/opensles_output.cc:349: DCHECK_GE(target_playout_time,
base::TimeTicks());
On 2016/07/29 01:21:09, chcunningham wrote:
> Still use this to check delay >= 0 if you take my recommendation for lines 346

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_low_l...
File media/audio/audio_low_latency_input_output_unittest.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_low_l...
media/audio/audio_low_latency_input_output_unittest.cc:231: base::TimeDelta
output_delay = target_playout_time - base::TimeTicks::Now();
On 2016/07/29 01:21:09, chcunningham wrote:
> Guard against negative?

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
target_playout_time - base::TimeTicks::Now();
On 2016/07/30 21:17:02, miu_OOO_until_28_Aug wrote:
> On 2016/07/29 01:21:09, chcunningham wrote:
> > Should we be gaurding against negative values here. delay = max(math above,
> 0)? 
> > 
> > I think that small values of negative delay may creep in without a guard for
> > output streams that don't have delay (and use an earlier call to Now() for
> their
> > target_playout_time) - see my comments in web_contents_audio_muter.cc
> 
> If negative delay creeps in, for output streams where it does not make sense,
> that would indicate that there are bugs in the code (e.g., erroneous time
math,
> rounding errors, etc.). Do we want a guard, which might mask an underlying
> problem? Perhaps the guard should be in the form of a DCHECK instead?

There are some cases where the delay will be negative because we calculated a
delay of zero and passed TimeTicks::Now to OnMoreData and the clock has moved
forward by at least one tick. Alternatively, we could pass TimeTicks() with a
zero value and treat that as a sentinel value indicating no delay. If we use
TimeTicks::Now, we'll have to tolerate negative values and guard against them.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_controller_unittest.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_controller_unittest.cc:204:
mock_stream_.callback()->OnMoreData(base::TimeTicks(), 0, dest.get());
On 2016/07/29 01:21:09, chcunningham wrote:
> Now?

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_controller_unittest.cc:218:
controller_->OnMoreData(base::TimeTicks(), 0, dest.get());
On 2016/07/29 01:21:09, chcunningham wrote:
> Now?

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_resampler.cc (left):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_resampler.cc:390: // TODO(dalecurtis): Stop passing
bytes around, it doesn't make sense since
On 2016/07/29 01:21:09, chcunningham wrote:
> Why is it ok for you to just drop frames_delayed? I think you need to factor
> this into target_playout_time as additional delay. 

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/fake_audio_...
File media/audio/fake_audio_input_stream.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:102:
audio_source_->OnMoreData(base::TimeTicks(), 0, audio_bus_.get());
On 2016/07/29 01:21:09, chcunningham wrote:
> Now?

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/fake_audio_...
File media/audio/fake_audio_output_stream.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/fake_audio_...
media/audio/fake_audio_output_stream.cc:67:
callback_->OnMoreData(base::TimeTicks(), 0, audio_bus_.get());
On 2016/07/29 01:21:09, chcunningham wrote:
> Now?

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/mac/audio_a...
File media/audio/mac/audio_auhal_mac.cc (left):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/mac/audio_a...
media/audio/mac/audio_auhal_mac.cc:340: if ((output_time_stamp->mFlags &
kAudioTimeStampHostTimeValid) == 0)
On 2016/07/29 01:21:09, chcunningham wrote:
> This method had checks for validity of host time... these checks should
probably
> stay in your new usage.

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/mac/audio_a...
media/audio/mac/audio_auhal_mac.cc:351: if (now_ns > output_time_ns)
On 2016/07/29 01:21:09, chcunningham wrote:
> I think you need to still account for this

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/pulse/pulse...
File media/audio/pulse/pulse_util.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/pulse/pulse...
media/audio/pulse/pulse_util.cc:1: 
On 2016/07/29 01:21:09, chcunningham wrote:
> space

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
File media/audio/simple_sources_unittest.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:35: source.OnMoreData(base::TimeTicks(),
0, audio_bus.get());
On 2016/07/29 01:21:10, chcunningham wrote:
> Now?

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:64:
EXPECT_EQ(source.OnMoreData(base::TimeTicks(), 0, audio_bus.get()),
On 2016/07/29 01:21:09, chcunningham wrote:
> Now?

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:67:
EXPECT_EQ(source.OnMoreData(base::TimeTicks(), 0, audio_bus.get()), 0);
On 2016/07/29 01:21:09, chcunningham wrote:
> Now?

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:70:
EXPECT_EQ(source.OnMoreData(base::TimeTicks(), 0, audio_bus.get()),
On 2016/07/29 01:21:09, chcunningham wrote:
> Now?

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:131: source.OnMoreData(base::TimeTicks(),
0, audio_bus.get()));
On 2016/07/29 01:21:10, chcunningham wrote:
> Now?

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:137: source.OnMoreData(base::TimeTicks(),
0, audio_bus.get());
On 2016/07/29 01:21:09, chcunningham wrote:
> Now?

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:167: source.OnMoreData(base::TimeTicks(),
0, audio_bus.get());
On 2016/07/29 01:21:09, chcunningham wrote:
> Now?

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:169: source.OnMoreData(base::TimeTicks(),
0, audio_bus.get());
On 2016/07/29 01:21:10, chcunningham wrote:
> Now?

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:187: EXPECT_EQ(0,
source.OnMoreData(base::TimeTicks(), 0, audio_bus.get()));
On 2016/07/29 01:21:09, chcunningham wrote:
> Now?

Done.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/simple_sour...
media/audio/simple_sources_unittest.cc:222: EXPECT_EQ(0,
source.OnMoreData(base::TimeTicks(), 0, audio_bus.get()));
On 2016/07/29 01:21:09, chcunningham wrote:
> Now?

Done.

[chcunningham1, 2016-08-27 00:34:58]

chcunningham@google.com changed reviewers:
+ chcunningham@google.com

[chcunningham1, 2016-08-27 00:35:00]

This LGTM - Dale/Yuri should probably take a last pass though.

https://codereview.chromium.org/2101303004/diff/60001/media/audio/alsa/alsa_o...
File media/audio/alsa/alsa_output_unittest.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/alsa/alsa_o...
media/audio/alsa/alsa_output_unittest.cc:648: EXPECT_CALL(mock_callback,
OnMoreData(_, 0, _))
On 2016/08/26 02:08:47, James West wrote:
> On 2016/07/29 01:21:08, chcunningham wrote:
> > Its weird that this one was ever 0 - IIUC they aren't setting the PcmDelay
> > expectation for this method to SetARgumentPointee<1>(0) as they do in the
> > StartStop case. I think its fine to ignore the delay value here...
> 
> It doesn't call PcmDelay because PcmState returns SND_PCM_STATE_XRUN, which
> results in the delay returned by GetCurrentDelay being 0.

Acknowledged.

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
media/audio/audio_output_resampler.cc:392: base::TimeTicks
new_target_playout_time =
This new math looks good to me, but I'd like to have Dale double check. 

Dale, I think the original math was wrong to multiply the entire thing by
io_ratio_. We used to have:

new_bytes_delay = io_ratio_ * (current_total_bytes_delay_ + frames_delayed *
input_bytes_per_frame_);

I think this should have been:

new_bytes_delay = (io_ratio_ * current_total_bytes_delay_) + (frames_delayed *
input_bytes_per_frame_);

extra parens added for clarity ... my point being frames_delayed is already
given in terms of input frames (hence the scaling by input_bytes_per_frame). 

If I'm missing something subtle I may need to re-assess the math in this latest
ps.

[chcunningham, 2016-08-27 00:36:19]

Arg, I mixed my accounts up. Here's some extra nits.

lgtm

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/60001/media/audio/audio_outpu...
media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
target_playout_time - base::TimeTicks::Now();
On 2016/08/26 02:08:47, James West wrote:
> On 2016/07/30 21:17:02, miu_OOO_until_28_Aug wrote:
> > On 2016/07/29 01:21:09, chcunningham wrote:
> > > Should we be gaurding against negative values here. delay = max(math
above,
> > 0)? 
> > > 
> > > I think that small values of negative delay may creep in without a guard
for
> > > output streams that don't have delay (and use an earlier call to Now() for
> > their
> > > target_playout_time) - see my comments in web_contents_audio_muter.cc
> > 
> > If negative delay creeps in, for output streams where it does not make
sense,
> > that would indicate that there are bugs in the code (e.g., erroneous time
> math,
> > rounding errors, etc.). Do we want a guard, which might mask an underlying
> > problem? Perhaps the guard should be in the form of a DCHECK instead?
> 
> There are some cases where the delay will be negative because we calculated a
> delay of zero and passed TimeTicks::Now to OnMoreData and the clock has moved
> forward by at least one tick. Alternatively, we could pass TimeTicks() with a
> zero value and treat that as a sentinel value indicating no delay. If we use
> TimeTicks::Now, we'll have to tolerate negative values and guard against them.

I'm fine with either approach. I slightly favor the current approach.

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
Can you add a comment explaining why this guard is needed. Something like the
explanation you already gave in the CL discussion is great.

[miu, 2016-08-31 23:26:53]

miu@chromium.org changed reviewers:
+ hubbe@chromium.org

[miu, 2016-08-31 23:26:55]

hubbe: PTAL at changes to audio_shifter.* There was some question about the old
and new math there.

https://codereview.chromium.org/2101303004/diff/120001/media/audio/android/op...
File media/audio/android/opensles_output.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/android/op...
media/audio/android/opensles_output.cc:344: // Calculate the delay in frames.
nit: At first glance it feels wonky to get a time position, convert it to a
frame delay, and then convert that back to a time position in the code later.
However, this is absolutely correct because one is relative to the audio
system's clock, which is different from the system reference clock (the one that
creates TimeTicks values).

So, could you elaborate on this comment here? Perhaps something like:

"Given the media time position, compute the approximate number of frames
currently queued for playout. This will be added to the system reference time
below to compute the approximate playout time of the audio to be rendered in the
call to OnMoreData() below."

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
Looks like this API change should propagate through the IPC interface. If you're
not going to tackle that in this CL, can you put a TODO comment here and file a
bug at crbug.com for tracking purposes?

BTW--If we fix the IPC interface, there would be no reason to call
TimeTicks::Now() at all. ;-)

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
media/audio/audio_output_controller.cc:312: const int total_bytes_delay =
delay.InSeconds() * params_.GetBytesPerSecond();
s/InSeconds/InSecondsF/ to prevent serious loss of precision!

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
media/audio/audio_output_resampler.cc:392: base::TimeTicks
new_target_playout_time =
On 2016/08/27 00:35:00, chcunningham1 wrote:
> This new math looks good to me, but I'd like to have Dale double check. 

Actually, Hubbe originally wrote this code. I'll add him to this code review to
check.

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
File media/audio/audio_output_stream_sink.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
media/audio/audio_output_stream_sink.cc:96: uint32_t frames_delayed =
delay.InSeconds() * active_params_.sample_rate();
s/InSeconds/InSecondsF/ to prevent serious loss of precision!

https://codereview.chromium.org/2101303004/diff/120001/media/audio/mac/audio_...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:342: return 0;
The two "return 0" statements in this method won't compile since a
base::TimeTicks type is required. Based on the way the old code behaved, I think
they need to be base::TimeTicks::Now().

Side point: FWICT, it seems like the old code is purposely masking some
potentially huge problems here. That's bad for users, and would make bugs very
hard to resolve. For instance, one would hope the |output_time_stamp| always has
its |TimeValid| flag set. If we're certain it never should be, then this could
become a DCHECK().

Also, the "Prevent overflow leading to huge delay" below is a total hack that
could only mask bugs from the platform CoreAudio library, and should not be
necessary. FWIW, if there ever was a problem, it's very likely to have been
fixed over the years, as this would ruin audio in all apps on MacOS.

So, could you put a TODO comment here for someone to revisit this code? I'd
suggest looking at `git blame` history to see who wrote the code originally, and
send them an e-mail. Feel free to cc me, if they need further guidance. ;-)

https://codereview.chromium.org/2101303004/diff/120001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:356: return
base::TimeTicks::FromInternalValue(
I'd suggest adding a TimeTicks::FromMachAbsoluteTime(uint64_t
mach_absolute_ticks) method. We had to do a similar thing for Windows
(TimeTicks::FromQPCValue()) a couple years back for an analogous use case.

Note that the implementation of this new method can just re-use most of this
code (in base/time/time_mac.cc):
https://cs.chromium.org/chromium/src/base/time/time_mac.cc?rcl=0&l=44

Also, note that AudioConvertHostTimeToNanos() is just doing the conversion from
mach_absolute_time() tick units to nanoseconds. So, this and the prior ~12 lines
of code can just become:

  const base::TimeTicks playout_time =
      base::TimeTicks::FromMachAbsoluteTime(output_time_stamp->mHostTime);
  const base::TimeTicks now = base::TimeTicks::Now();

  if (now > playout_time)  // HACK
    return now;

  return playout_time;

...or, if you're feeling saucy and agree with my prior comment about the hack
being bogus, all those lines of code become just one:

  return base::TimeTicks::FromMachAbsoluteTime(output_time_stamp->mHostTime);

https://codereview.chromium.org/2101303004/diff/120001/media/audio/pulse/puls...
File media/audio/pulse/pulse_util.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/pulse/puls...
media/audio/pulse/pulse_util.cc:155: return
GetHardwareLatency(stream).InSeconds() * sample_rate * bytes_per_frame;
s/InSeconds/InSecondsF/ to prevent serious loss of precision!

[miu, 2016-08-31 23:29:35]

On 2016/08/31 23:26:55, miu wrote:
>
https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
> media/audio/audio_output_resampler.cc:392: base::TimeTicks
> new_target_playout_time =
> On 2016/08/27 00:35:00, chcunningham1 wrote:
> > This new math looks good to me, but I'd like to have Dale double check. 
> 
> Actually, Hubbe originally wrote this code. I'll add him to this code review
to
> check.

Whoops! I was thinking AudioShifter when I saw this (does a similar thing). So,
I guess Dale instead of Hubbe should take a look.

[miu, 2016-08-31 23:29:55]

miu@chromium.org changed reviewers:
- hubbe@chromium.org

[miu, 2016-08-31 23:29:55]

-hubbe@

[DaleCurtis, 2016-09-01 18:46:14]

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
media/audio/audio_output_resampler.cc:392: base::TimeTicks
new_target_playout_time =
On 2016/08/31 at 23:26:54, miu wrote:
> On 2016/08/27 00:35:00, chcunningham1 wrote:
> > This new math looks good to me, but I'd like to have Dale double check. 
> 
> Actually, Hubbe originally wrote this code. I'll add him to this code review
to check.

Looking at the code that does seem correct.

[jameswest, 2016-09-07 21:52:16]

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
On 2016/08/31 23:26:54, miu wrote:
> Looks like this API change should propagate through the IPC interface. If
you're
> not going to tackle that in this CL, can you put a TODO comment here and file
a
> bug at http://crbug.com for tracking purposes?
> 
> BTW--If we fix the IPC interface, there would be no reason to call
> TimeTicks::Now() at all. ;-)

When the number of bytes is passed to UpdatePendingBytes, where does that
ultimately go? I see it gets sent to a SyncSocket in AudioSyncReader, but I
can't figure out where that goes.

https://codereview.chromium.org/2101303004/diff/120001/media/audio/mac/audio_...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:342: return 0;
On 2016/08/31 23:26:54, miu wrote:
> The two "return 0" statements in this method won't compile since a
> base::TimeTicks type is required. Based on the way the old code behaved, I
think
> they need to be base::TimeTicks::Now().
> 
> Side point: FWICT, it seems like the old code is purposely masking some
> potentially huge problems here. That's bad for users, and would make bugs very
> hard to resolve. For instance, one would hope the |output_time_stamp| always
has
> its |TimeValid| flag set. If we're certain it never should be, then this could
> become a DCHECK().
> 
> Also, the "Prevent overflow leading to huge delay" below is a total hack that
> could only mask bugs from the platform CoreAudio library, and should not be
> necessary. FWIW, if there ever was a problem, it's very likely to have been
> fixed over the years, as this would ruin audio in all apps on MacOS.
> 
> So, could you put a TODO comment here for someone to revisit this code? I'd
> suggest looking at `git blame` history to see who wrote the code originally,
and
> send them an e-mail. Feel free to cc me, if they need further guidance. ;-)

I'll add the TODO, but it looks like it was written by crogers@google.com. I
can't find him in Teams, so I don't think he works here anymore.

[miu, 2016-09-07 22:19:37]

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
On 2016/09/07 21:52:16, James West wrote:
> On 2016/08/31 23:26:54, miu wrote:
> > Looks like this API change should propagate through the IPC interface.
>
> When the number of bytes is passed to UpdatePendingBytes, where does that
> ultimately go? I see it gets sent to a SyncSocket in AudioSyncReader, but I
> can't figure out where that goes.

The consumer is media::AudioDeviceThread:
https://cs.chromium.org/chromium/src/media/audio/audio_device_thread.cc?rcl=1...

That's a real-time thread, and when it calls callback_->Process(), the
implementation in media::AudioOutputDevice is invoked:
https://cs.chromium.org/chromium/src/media/audio/audio_output_device.cc?rcl=0...

From there, AudioOutputDevice calls AudioRendererSink::Render(), which should be
changed to identically match the OnMoreData() API:
https://cs.chromium.org/chromium/src/media/base/audio_renderer_sink.h?rcl=147...

It's up to you whether you want to do these things. On the one hand, they really
are needed now (by https://codereview.chromium.org/2060833002/), and I'm sure
Mikhail would really appreciate you doing the work since, otherwise, he'd have
to. On the other hand, changing the AudioRendererSink interface would require
tweaking the audio handling code in about 10-15 other classes, and so it's not
just a quick thing for you.

https://codereview.chromium.org/2101303004/diff/120001/media/audio/mac/audio_...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:342: return 0;
On 2016/09/07 21:52:16, James West wrote:
> On 2016/08/31 23:26:54, miu wrote:
> > The two "return 0" statements in this method won't compile since a
> > base::TimeTicks type is required. Based on the way the old code behaved, I
> think
> > they need to be base::TimeTicks::Now().
> > 
> > Side point: FWICT, it seems like the old code is purposely masking some
> > potentially huge problems here. That's bad for users, and would make bugs
very
> > hard to resolve. For instance, one would hope the |output_time_stamp| always
> has
> > its |TimeValid| flag set. If we're certain it never should be, then this
could
> > become a DCHECK().
> > 
> > Also, the "Prevent overflow leading to huge delay" below is a total hack
that
> > could only mask bugs from the platform CoreAudio library, and should not be
> > necessary. FWIW, if there ever was a problem, it's very likely to have been
> > fixed over the years, as this would ruin audio in all apps on MacOS.
> > 
> > So, could you put a TODO comment here for someone to revisit this code? I'd
> > suggest looking at `git blame` history to see who wrote the code originally,
> and
> > send them an e-mail. Feel free to cc me, if they need further guidance. ;-)
> 
> I'll add the TODO, but it looks like it was written by
mailto:crogers@google.com. I
> can't find him in Teams, so I don't think he works here anymore.

That's correct. He left several years ago. Hmm...Go ahead and assign the
TODO/crbug to me, and then it'll be on me to find someone to take ownership from
our wider "Chrome Media" group.

[Mikhail, 2016-09-08 07:44:11]

mikhail.pozdnyakov@intel.com changed reviewers:
+ mikhail.pozdnyakov@intel.com

[Mikhail, 2016-09-08 07:44:12]

https://codereview.chromium.org/2101303004/diff/120001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:515: base::TimeTicks
target_playout_time = base::TimeTicks::Now();
wouldn't it be more accurate to use the following 'audio_clock_->GetPosition()'
call (providing 2nd argument to it) to retrieve the current playout time than
just calling 'TimeTicks::Now()'? 

TimeTicks can be obtained from the returned QPC value (pls. look at
https://codereview.chromium.org/2060833002/diff/230001/media/audio/win/audio_...)

[miu, 2016-09-08 20:46:48]

https://codereview.chromium.org/2101303004/diff/120001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:515: base::TimeTicks
target_playout_time = base::TimeTicks::Now();
On 2016/09/08 07:44:12, Mikhail wrote:
> wouldn't it be more accurate to use the following
'audio_clock_->GetPosition()'
> call (providing 2nd argument to it) to retrieve the current playout time than
> just calling 'TimeTicks::Now()'? 
> 
> TimeTicks can be obtained from the returned QPC value (pls. look at
>
https://codereview.chromium.org/2060833002/diff/230001/media/audio/win/audio_...)

Good catch! Yes, that sounds like a great idea. WDYT, James?

However, note that the GetPosition() function return the time at which the
position was measured (i.e., it is not the target_playout_time). So, you'd need
to take that time and also add the delay based on the number of frames that were
queued.

Reference:
https://msdn.microsoft.com/en-us/library/windows/desktop/dd370889(v=vs.85).aspx

[miu, 2016-09-08 20:56:23]

https://codereview.chromium.org/2101303004/diff/120001/media/audio/mac/audio_...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:342: return 0;
Looking at this again, I think it hides a bug, but Dale pointed out to me (on
the side) that there's probably nothing we can do if this platform bug arises.

What this code is doing: If the output_timestamp for the next frames is bogus
(because it was in the past), we can only say the delay is zero (because delays
cannot be negative--that's meaningless).

The comments, as written, are misleading because they talk about "preventing
overflow" and "huge delay information" and that's not accurate.

Thus, could you just fix the comments to say something like: A platform bug has
been observed where sometimes the platform reports the next frames will be
output at a time in the past. Since delay-until-playout cannot be negative,
assume it is zero and return "now" as the target playout time.

[Mikhail, 2016-09-09 18:19:42]

If I understand the intention of this CL correctly you provide a system time
estimation of the moment when the next provided sample will be played.

Would an API similar to one from
https://codereview.chromium.org/2060833002/diff/230001/media/audio/audio_io.h
fit also here?

Once you have a pair of time values for the currently audible signal (similar to
AudioClock::GetPosition()
on Win) you can make accurate system time estimations for the slightly advanced
stream position as:

playout system time = recorded system time + (playout stream position in seconds
- recorded stream position in seconds);

(where "recorded" system time and stream position are obtained from
AudioClock::GetPosition() result)

[James West, 2016-09-13 07:40:49]

jameswest@chromium.org changed reviewers:
+ jameswest@chromium.org

[James West, 2016-09-13 07:40:51]

https://codereview.chromium.org/2101303004/diff/120001/media/audio/android/op...
File media/audio/android/opensles_output.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/android/op...
media/audio/android/opensles_output.cc:344: // Calculate the delay in frames.
On 2016/08/31 23:26:54, miu wrote:
> nit: At first glance it feels wonky to get a time position, convert it to a
> frame delay, and then convert that back to a time position in the code later.
> However, this is absolutely correct because one is relative to the audio
> system's clock, which is different from the system reference clock (the one
that
> creates TimeTicks values).
> 
> So, could you elaborate on this comment here? Perhaps something like:
> 
> "Given the media time position, compute the approximate number of frames
> currently queued for playout. This will be added to the system reference time
> below to compute the approximate playout time of the audio to be rendered in
the
> call to OnMoreData() below."

Done.

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
On 2016/09/07 22:19:36, miu wrote:
> On 2016/09/07 21:52:16, James West wrote:
> > On 2016/08/31 23:26:54, miu wrote:
> > > Looks like this API change should propagate through the IPC interface.
> >
> > When the number of bytes is passed to UpdatePendingBytes, where does that
> > ultimately go? I see it gets sent to a SyncSocket in AudioSyncReader, but I
> > can't figure out where that goes.
> 
> The consumer is media::AudioDeviceThread:
>
https://cs.chromium.org/chromium/src/media/audio/audio_device_thread.cc?rcl=1...
> 
> That's a real-time thread, and when it calls callback_->Process(), the
> implementation in media::AudioOutputDevice is invoked:
>
https://cs.chromium.org/chromium/src/media/audio/audio_output_device.cc?rcl=0...
> 
> From there, AudioOutputDevice calls AudioRendererSink::Render(), which should
be
> changed to identically match the OnMoreData() API:
>
https://cs.chromium.org/chromium/src/media/base/audio_renderer_sink.h?rcl=147...
> 
> It's up to you whether you want to do these things. On the one hand, they
really
> are needed now (by https://codereview.chromium.org/2060833002/), and I'm sure
> Mikhail would really appreciate you doing the work since, otherwise, he'd have
> to. On the other hand, changing the AudioRendererSink interface would require
> tweaking the audio handling code in about 10-15 other classes, and so it's not
> just a quick thing for you.

I'm happy to do this, but I think it should be a separate change to avoid
greatly expanding the size of this one. I added a TODO.

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
media/audio/audio_output_controller.cc:312: const int total_bytes_delay =
delay.InSeconds() * params_.GetBytesPerSecond();
On 2016/08/31 23:26:54, miu wrote:
> s/InSeconds/InSecondsF/ to prevent serious loss of precision!

Done.

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
media/audio/audio_output_resampler.cc:392: base::TimeTicks
new_target_playout_time =
On 2016/09/01 18:46:13, DaleCurtis wrote:
> On 2016/08/31 at 23:26:54, miu wrote:
> > On 2016/08/27 00:35:00, chcunningham1 wrote:
> > > This new math looks good to me, but I'd like to have Dale double check. 
> > 
> > Actually, Hubbe originally wrote this code. I'll add him to this code review
> to check.
> 
> Looking at the code that does seem correct. 

Acknowledged.

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
File media/audio/audio_output_stream_sink.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
media/audio/audio_output_stream_sink.cc:96: uint32_t frames_delayed =
delay.InSeconds() * active_params_.sample_rate();
On 2016/08/31 23:26:54, miu wrote:
> s/InSeconds/InSecondsF/ to prevent serious loss of precision!

Done.

https://codereview.chromium.org/2101303004/diff/120001/media/audio/mac/audio_...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:342: return 0;
On 2016/09/08 20:56:23, miu wrote:
> Looking at this again, I think it hides a bug, but Dale pointed out to me (on
> the side) that there's probably nothing we can do if this platform bug arises.
> 
> What this code is doing: If the output_timestamp for the next frames is bogus
> (because it was in the past), we can only say the delay is zero (because
delays
> cannot be negative--that's meaningless).
> 
> The comments, as written, are misleading because they talk about "preventing
> overflow" and "huge delay information" and that's not accurate.
> 
> Thus, could you just fix the comments to say something like: A platform bug
has
> been observed where sometimes the platform reports the next frames will be
> output at a time in the past. Since delay-until-playout cannot be negative,
> assume it is zero and return "now" as the target playout time.

Done.

https://codereview.chromium.org/2101303004/diff/120001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:356: return
base::TimeTicks::FromInternalValue(
On 2016/08/31 23:26:54, miu wrote:
> I'd suggest adding a TimeTicks::FromMachAbsoluteTime(uint64_t
> mach_absolute_ticks) method. We had to do a similar thing for Windows
> (TimeTicks::FromQPCValue()) a couple years back for an analogous use case.
> 
> Note that the implementation of this new method can just re-use most of this
> code (in base/time/time_mac.cc):
> https://cs.chromium.org/chromium/src/base/time/time_mac.cc?rcl=0&l=44
> 
> Also, note that AudioConvertHostTimeToNanos() is just doing the conversion
from
> mach_absolute_time() tick units to nanoseconds. So, this and the prior ~12
lines
> of code can just become:
> 
>   const base::TimeTicks playout_time =
>       base::TimeTicks::FromMachAbsoluteTime(output_time_stamp->mHostTime);
>   const base::TimeTicks now = base::TimeTicks::Now();
> 
>   if (now > playout_time)  // HACK
>     return now;
> 
>   return playout_time;
> 
> ...or, if you're feeling saucy and agree with my prior comment about the hack
> being bogus, all those lines of code become just one:
> 
>   return base::TimeTicks::FromMachAbsoluteTime(output_time_stamp->mHostTime);

Done.

https://codereview.chromium.org/2101303004/diff/120001/media/audio/pulse/puls...
File media/audio/pulse/pulse_util.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/pulse/puls...
media/audio/pulse/pulse_util.cc:155: return
GetHardwareLatency(stream).InSeconds() * sample_rate * bytes_per_frame;
On 2016/08/31 23:26:55, miu wrote:
> s/InSeconds/InSecondsF/ to prevent serious loss of precision!

Done.

https://codereview.chromium.org/2101303004/diff/120001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:515: base::TimeTicks
target_playout_time = base::TimeTicks::Now();
On 2016/09/08 20:46:48, miu wrote:
> On 2016/09/08 07:44:12, Mikhail wrote:
> > wouldn't it be more accurate to use the following
> 'audio_clock_->GetPosition()'
> > call (providing 2nd argument to it) to retrieve the current playout time
than
> > just calling 'TimeTicks::Now()'? 
> > 
> > TimeTicks can be obtained from the returned QPC value (pls. look at
> >
>
https://codereview.chromium.org/2060833002/diff/230001/media/audio/win/audio_...)
> 
> Good catch! Yes, that sounds like a great idea. WDYT, James?
> 
> However, note that the GetPosition() function return the time at which the
> position was measured (i.e., it is not the target_playout_time). So, you'd
need
> to take that time and also add the delay based on the number of frames that
were
> queued.
> 
> Reference:
>
https://msdn.microsoft.com/en-us/library/windows/desktop/dd370889(v=vs.85).aspx

I agree that's the correct way to do it. I implemented it and cleaned up the
logic a bit.

[James West, 2016-09-13 08:10:33]

On 2016/09/09 18:19:42, Mikhail wrote:
> If I understand the intention of this CL correctly you provide a system time
> estimation of the moment when the next provided sample will be played.
> 
> Would an API similar to one from
> https://codereview.chromium.org/2060833002/diff/230001/media/audio/audio_io.h
> fit also here?
> 
> Once you have a pair of time values for the currently audible signal (similar
to
> AudioClock::GetPosition()
> on Win) you can make accurate system time estimations for the slightly
advanced
> stream position as:
> 
> playout system time = recorded system time + (playout stream position in
seconds
> - recorded stream position in seconds);
> 
> (where "recorded" system time and stream position are obtained from
> AudioClock::GetPosition() result)

I'm not opposed to that. We could change the API to something like the
following:

int OnMoreData(base::TimeDelta stream_position,
               base::TimeTicks recorded_time,
               int prior_frames_skipped,
               AudioBus* dest);

[Mikhail, 2016-09-13 08:17:17]

On 2016/09/13 08:10:33, jameswest wrote:
> On 2016/09/09 18:19:42, Mikhail wrote:
> > If I understand the intention of this CL correctly you provide a system time
> > estimation of the moment when the next provided sample will be played.
> > 
> > Would an API similar to one from
> >
https://codereview.chromium.org/2060833002/diff/230001/media/audio/audio_io.h
> > fit also here?
> > 
> > Once you have a pair of time values for the currently audible signal
(similar
> to
> > AudioClock::GetPosition()
> > on Win) you can make accurate system time estimations for the slightly
> advanced
> > stream position as:
> > 
> > playout system time = recorded system time + (playout stream position in
> seconds
> > - recorded stream position in seconds);
> > 
> > (where "recorded" system time and stream position are obtained from
> > AudioClock::GetPosition() result)
> 
> I'm not opposed to that. We could change the API to something like the
> following:
> 
> int OnMoreData(base::TimeDelta stream_position,
>                base::TimeTicks recorded_time,
>                int prior_frames_skipped,
>                AudioBus* dest);
That would be awesome!

[jameswest, 2016-09-16 03:26:52]

On 2016/09/13 08:17:17, Mikhail wrote:
> On 2016/09/13 08:10:33, jameswest wrote:
> > On 2016/09/09 18:19:42, Mikhail wrote:
> > > If I understand the intention of this CL correctly you provide a system
time
> > > estimation of the moment when the next provided sample will be played.
> > > 
> > > Would an API similar to one from
> > >
> https://codereview.chromium.org/2060833002/diff/230001/media/audio/audio_io.h
> > > fit also here?
> > > 
> > > Once you have a pair of time values for the currently audible signal
> (similar
> > to
> > > AudioClock::GetPosition()
> > > on Win) you can make accurate system time estimations for the slightly
> > advanced
> > > stream position as:
> > > 
> > > playout system time = recorded system time + (playout stream position in
> > seconds
> > > - recorded stream position in seconds);
> > > 
> > > (where "recorded" system time and stream position are obtained from
> > > AudioClock::GetPosition() result)
> > 
> > I'm not opposed to that. We could change the API to something like the
> > following:
> > 
> > int OnMoreData(base::TimeDelta stream_position,
> >                base::TimeTicks recorded_time,
> >                int prior_frames_skipped,
> >                AudioBus* dest);
> That would be awesome!

Mikhail,

Is it possible for you to use the target playout time or a delay instead of
stream position and compute the stream position where you need it based on the
amount of audio that has been written to the output buffer but not necessarily
played out, i.e., stream_position = written_audio - delay? I've been looking
over all the code, and in order to use the API I mentioned above, we would have
to change all the implementations of AudioOutputStream and AudioSourceCallback,
other than the ones for Windows Core Audio and OpenSL, to keep track of how much
audio data has been written in order to compute the stream position from a delay
or the delay from a stream position.

ALSA, Pulse Audio, CRAS, Apple Core Audio, and others provide a delay or playout
time rather than a stream position, and all the AudioSourceCallback
implementations currently use a delay. Keeping state with the total amount of
audio data written across multiple function calls in a bunch of classes seems a
lot more complex and error-prone to me than using stateless playout time or
delay values provided with each function call, so I'd rather add that logic to a
few classes that actually need stream position than a couple dozen that don't
need it.

If you do need to add stream position to OnMoreData, I think it'll be better to
keep the target_playout time or delay in the OnMoreData API so that the
AudioSourceCallback implementations will have that data without having to track
the amount of audio data written out to the buffer.

[Mikhail, 2016-09-16 08:42:29]

On 2016/09/16 03:26:52, James West wrote:
> Mikhail,
> 
> Is it possible for you to use the target playout time or a delay instead of
> stream position and compute the stream position where you need it based on the
> amount of audio that has been written to the output buffer but not necessarily
> played out, i.e., stream_position = written_audio - delay? I've been looking
> over all the code, and in order to use the API I mentioned above, we would
have
> to change all the implementations of AudioOutputStream and
AudioSourceCallback,
> other than the ones for Windows Core Audio and OpenSL, to keep track of how
much
> audio data has been written in order to compute the stream position from a
delay
> or the delay from a stream position.
> 
> ALSA, Pulse Audio, CRAS, Apple Core Audio, and others provide a delay or
playout
> time rather than a stream position, and all the AudioSourceCallback
> implementations currently use a delay. Keeping state with the total amount of
> audio data written across multiple function calls in a bunch of classes seems
a
> lot more complex and error-prone to me than using stateless playout time or
> delay values provided with each function call, so I'd rather add that logic to
a
> few classes that actually need stream position than a couple dozen that don't
> need it.
> 
> If you do need to add stream position to OnMoreData, I think it'll be better
to
> keep the target_playout time or delay in the OnMoreData API so that the
> AudioSourceCallback implementations will have that data without having to
track
> the amount of audio data written out to the buffer.

James, thank you very much for evaluating this!
I think I could surviuve with 'stream_position = written_audio - delay'
(especially assuming that written_audio had been previously obtained as
'stream_position + delay'). I guess in this case both target playout time and
delay are needed, smth. like:
 virtual int OnMoreData(base::TimeTicks target_playout_time,
                        base::TimeTicks delay /*or 'latency'*/,
                        int prior_frames_skipped,
                        AudioBus* dest) = 0;
I could add the 'delay' argument after this CL lands so that you're not blocked
with it.
WDYT?

[miu, 2016-09-16 18:35:58]

Nice work! :) Just a few minor considerations left:

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/120001/media/audio/audio_outp...
media/audio/audio_output_controller.cc:310: const base::TimeDelta delay =
On 2016/09/13 07:40:50, jameswest wrote:
> On 2016/09/07 22:19:36, miu wrote:
> > On 2016/09/07 21:52:16, James West wrote:
> > > On 2016/08/31 23:26:54, miu wrote:
> > > > Looks like this API change should propagate through the IPC interface.
> > >
> > > When the number of bytes is passed to UpdatePendingBytes, where does that
> > > ultimately go? I see it gets sent to a SyncSocket in AudioSyncReader, but
I
> > > can't figure out where that goes.
> > 
> > The consumer is media::AudioDeviceThread:
> >
>
https://cs.chromium.org/chromium/src/media/audio/audio_device_thread.cc?rcl=1...
> > 
> > That's a real-time thread, and when it calls callback_->Process(), the
> > implementation in media::AudioOutputDevice is invoked:
> >
>
https://cs.chromium.org/chromium/src/media/audio/audio_output_device.cc?rcl=0...
> > 
> > From there, AudioOutputDevice calls AudioRendererSink::Render(), which
should
> be
> > changed to identically match the OnMoreData() API:
> >
>
https://cs.chromium.org/chromium/src/media/base/audio_renderer_sink.h?rcl=147...
> > 
> > It's up to you whether you want to do these things. On the one hand, they
> really
> > are needed now (by https://codereview.chromium.org/2060833002/), and I'm
sure
> > Mikhail would really appreciate you doing the work since, otherwise, he'd
have
> > to. On the other hand, changing the AudioRendererSink interface would
require
> > tweaking the audio handling code in about 10-15 other classes, and so it's
not
> > just a quick thing for you.
> 
> I'm happy to do this, but I think it should be a separate change to avoid
> greatly expanding the size of this one. I added a TODO.

SGTM.

https://codereview.chromium.org/2101303004/diff/140001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/2101303004/diff/140001/base/time/time.h#newco...
base/time/time.h:738: #if defined(OS_MACOSX)
To prevent accidental use of this function on IOS:

  #if defined(OS_MACOSX) && !defined(OS_IOS)

https://codereview.chromium.org/2101303004/diff/140001/base/time/time_mac.cc
File base/time/time_mac.cc (right):

https://codereview.chromium.org/2101303004/diff/140001/base/time/time_mac.cc#...
base/time/time_mac.cc:28: int64_t MachAbsoluteTimeToTicks(uint64_t
mach_absolute_time) {
Please wrap this function with:

  #if defined(OS_MACOSX) && !defined(OS_IOS)
  ...
  #endif  // defined(OS_MACOSX) && !defined(OS_IOS)

https://codereview.chromium.org/2101303004/diff/140001/media/audio/mac/audio_...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2101303004/diff/140001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:346: return base::TimeTicks::Now();
base::TimeTicks::Now() is being called twice in this method. Could you just call
it once and then use a var in two places:

  const base::TimeTicks now = base::TimeTicks::Now();
  if (...not valid...)
    return now;
  return std::max(...FromMachTime..., now);

https://codereview.chromium.org/2101303004/diff/140001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/2101303004/diff/140001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:524: num_written_frames_ +
packet_size_frames_ - played_out_frames;
Is is correct to include the |packet_size_frames_| in the new calculation?
Unless I'm missing something, it seems that would set |target_playout_delay| to
the *last* sample about to be rendered by OnMoreData(), rather than the *first*
one (which is what the interface intends).

[miu, 2016-09-16 18:38:03]

On 2016/09/16 08:42:29, Mikhail wrote:
> I think I could surviuve with 'stream_position = written_audio - delay'
> (especially assuming that written_audio had been previously obtained as
> 'stream_position + delay'). I guess in this case both target playout time and
> delay are needed, smth. like:
>  virtual int OnMoreData(base::TimeTicks target_playout_time,
>                         base::TimeTicks delay /*or 'latency'*/,
>                         int prior_frames_skipped,
>                         AudioBus* dest) = 0;
> I could add the 'delay' argument after this CL lands so that you're not
blocked
> with it.
> WDYT?

This plan SGTM. (But the delay argument would be either a base::TimeDelta or a
frame count, right?)

[chcunningham, 2016-09-16 20:52:44]

On 2016/09/16 18:38:03, miu wrote:
> On 2016/09/16 08:42:29, Mikhail wrote:
> > I think I could surviuve with 'stream_position = written_audio - delay'
> > (especially assuming that written_audio had been previously obtained as
> > 'stream_position + delay'). I guess in this case both target playout time
and
> > delay are needed, smth. like:
> >  virtual int OnMoreData(base::TimeTicks target_playout_time,
> >                         base::TimeTicks delay /*or 'latency'*/,
> >                         int prior_frames_skipped,
> >                         AudioBus* dest) = 0;
> > I could add the 'delay' argument after this CL lands so that you're not
> blocked
> > with it.
> > WDYT?
> 
> This plan SGTM. (But the delay argument would be either a base::TimeDelta or a
> frame count, right?)

Sorry, I'm confused now. Why is delay needed? Is delay not implicit in
target_playout_time?

[jameswest, 2016-09-16 21:10:31]

On 2016/09/16 20:52:44, chcunningham wrote:
> On 2016/09/16 18:38:03, miu wrote:
> > On 2016/09/16 08:42:29, Mikhail wrote:
> > > I think I could surviuve with 'stream_position = written_audio - delay'
> > > (especially assuming that written_audio had been previously obtained as
> > > 'stream_position + delay'). I guess in this case both target playout time
> and
> > > delay are needed, smth. like:
> > >  virtual int OnMoreData(base::TimeTicks target_playout_time,
> > >                         base::TimeTicks delay /*or 'latency'*/,
> > >                         int prior_frames_skipped,
> > >                         AudioBus* dest) = 0;
> > > I could add the 'delay' argument after this CL lands so that you're not
> > blocked
> > > with it.
> > > WDYT?
> > 
> > This plan SGTM. (But the delay argument would be either a base::TimeDelta or
a
> > frame count, right?)
> 
> Sorry, I'm confused now. Why is delay needed? Is delay not implicit in
> target_playout_time?

Mikhail needs to get the stream position and timestamp, where stream_position =
written_audio - delay and timestamp = target_playout_time - delay. The target
playout time alone isn't sufficient. We could alternatively use delay and
timestamp and compute target playout time as timestamp + delay where needed.

[jameswest, 2016-09-16 21:26:12]

On 2016/09/16 21:10:31, James West wrote:
> On 2016/09/16 20:52:44, chcunningham wrote:
> > On 2016/09/16 18:38:03, miu wrote:
> > > On 2016/09/16 08:42:29, Mikhail wrote:
> > > > I think I could surviuve with 'stream_position = written_audio - delay'
> > > > (especially assuming that written_audio had been previously obtained as
> > > > 'stream_position + delay'). I guess in this case both target playout
time
> > and
> > > > delay are needed, smth. like:
> > > >  virtual int OnMoreData(base::TimeTicks target_playout_time,
> > > >                         base::TimeTicks delay /*or 'latency'*/,
> > > >                         int prior_frames_skipped,
> > > >                         AudioBus* dest) = 0;
> > > > I could add the 'delay' argument after this CL lands so that you're not
> > > blocked
> > > > with it.
> > > > WDYT?
> > > 
> > > This plan SGTM. (But the delay argument would be either a base::TimeDelta
or
> a
> > > frame count, right?)
> > 
> > Sorry, I'm confused now. Why is delay needed? Is delay not implicit in
> > target_playout_time?
> 
> Mikhail needs to get the stream position and timestamp, where stream_position
=
> written_audio - delay and timestamp = target_playout_time - delay. The target
> playout time alone isn't sufficient. We could alternatively use delay and
> timestamp and compute target playout time as timestamp + delay where needed.

I think I prefer the API below because I see target playout time as more of a
derived value and the timestamp as more of a primitive value:

virtual int OnMoreData(base::TimeDelta delay,
                       base::TimeTicks timestamp,
                       int prior_frames_skipped,
                       AudioBus* dest) = 0;

In this case, target_playout_time = timestamp + delay. I'll go ahead and
implement this unless anyone is opposed.

[jameswest, 2016-09-16 21:59:43]

https://codereview.chromium.org/2101303004/diff/140001/media/audio/cras/cras_...
File media/audio/cras/cras_unified.cc (right):

https://codereview.chromium.org/2101303004/diff/140001/media/audio/cras/cras_...
media/audio/cras/cras_unified.cc:253: const timespec* output_ts,
Do you know if |output_ts| is target playout time? The documentation below says
it's "Playback time for the first sample written."

https://chromium.googlesource.com/chromiumos/third_party/adhd/+/stabilize-705...

If so, what do you think about adding TimeTicks::FromTimeSpec and using this
value directly?

https://codereview.chromium.org/2101303004/diff/140001/media/audio/mac/audio_...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2101303004/diff/140001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:263: current_target_playout_time_ + delay;
I realized I'm no longer adding hardware_latency_frames_ that was being added in
GetPlayoutLatency. I can't find any good documentation for this, but should we
add both frame_delay and hardware_latency_frames_ to
current_target_playout_time_ to get target_playout_time?

https://codereview.chromium.org/2101303004/diff/140001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:346: return base::TimeTicks::Now();
On 2016/09/16 18:35:58, miu wrote:
> base::TimeTicks::Now() is being called twice in this method. Could you just
call
> it once and then use a var in two places:
> 
>   const base::TimeTicks now = base::TimeTicks::Now();
>   if (...not valid...)
>     return now;
>   return std::max(...FromMachTime..., now);

I'm not opposed to this, but I'm curious what the rationale is when both
statements can't be executed during the same function call.

https://codereview.chromium.org/2101303004/diff/140001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/2101303004/diff/140001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:524: num_written_frames_ +
packet_size_frames_ - played_out_frames;
On 2016/09/16 18:35:58, miu wrote:
> Is is correct to include the |packet_size_frames_| in the new calculation?
> Unless I'm missing something, it seems that would set |target_playout_delay|
to
> the *last* sample about to be rendered by OnMoreData(), rather than the
*first*
> one (which is what the interface intends).

I believe you're correct. I'll change it.

[miu, 2016-09-16 22:56:10]

On 2016/09/16 21:26:12, James West wrote:
> On 2016/09/16 21:10:31, James West wrote:
> > On 2016/09/16 20:52:44, chcunningham wrote:
> > > On 2016/09/16 18:38:03, miu wrote:
> > > > On 2016/09/16 08:42:29, Mikhail wrote:
> > > > > I think I could surviuve with 'stream_position = written_audio -
delay'
> > > > > (especially assuming that written_audio had been previously obtained
as
> > > > > 'stream_position + delay'). I guess in this case both target playout
> time
> > > and
> > > > > delay are needed, smth. like:
> > > > >  virtual int OnMoreData(base::TimeTicks target_playout_time,
> > > > >                         base::TimeTicks delay /*or 'latency'*/,
> > > > >                         int prior_frames_skipped,
> > > > >                         AudioBus* dest) = 0;
> > > > > I could add the 'delay' argument after this CL lands so that you're
not
> > > > blocked
> > > > > with it.
> > > > > WDYT?
> > > > 
> > > > This plan SGTM. (But the delay argument would be either a
base::TimeDelta
> or
> > a
> > > > frame count, right?)
> > > 
> > > Sorry, I'm confused now. Why is delay needed? Is delay not implicit in
> > > target_playout_time?
> > 
> > Mikhail needs to get the stream position and timestamp, where
stream_position
> =
> > written_audio - delay and timestamp = target_playout_time - delay. The
target
> > playout time alone isn't sufficient. We could alternatively use delay and
> > timestamp and compute target playout time as timestamp + delay where needed.
> 
> I think I prefer the API below because I see target playout time as more of a
> derived value and the timestamp as more of a primitive value:
> 
> virtual int OnMoreData(base::TimeDelta delay,
>                        base::TimeTicks timestamp,
>                        int prior_frames_skipped,
>                        AudioBus* dest) = 0;
> 
> In this case, target_playout_time = timestamp + delay. I'll go ahead and
> implement this unless anyone is opposed.

SGTM. The more we all discuss this, and come to understand all the use cases,
this seems like a good approach. In a way, it very closely resembles the Windows
GetPosition() API; so maybe we've unfortunately re-invented a wheel here.  ;-)

nit: Instead of naming it |timestamp| can we be a little more specific to avoid
accidental misuse? Something like |request_time|? And, in the interface comments
clarify that |request_time + delay == target_playout_time of 1st frame in
|dest|)?

[miu, 2016-09-16 22:58:15]

On 2016/09/16 20:52:44, chcunningham wrote:
> Sorry, I'm confused now. Why is delay needed? Is delay not implicit in
> target_playout_time?

It is. The issue is that there is a use case (a newly-spec'ed WebAudio API) for
knowing the timestamp of the audio sample being played out of the speakers
exactly now (which would be |target_playout_time - delay|).

[miu, 2016-09-16 23:38:28]

https://codereview.chromium.org/2101303004/diff/140001/media/audio/cras/cras_...
File media/audio/cras/cras_unified.cc (right):

https://codereview.chromium.org/2101303004/diff/140001/media/audio/cras/cras_...
media/audio/cras/cras_unified.cc:253: const timespec* output_ts,
On 2016/09/16 21:59:43, James West wrote:
> Do you know if |output_ts| is target playout time? The documentation below
says
> it's "Playback time for the first sample written."
>
https://chromium.googlesource.com/chromiumos/third_party/adhd/+/stabilize-705...

That would be my interpretation (that output_ts is the target playout time).
However...

> If so, what do you think about adding TimeTicks::FromTimeSpec and using this
> value directly?

It could be relative to a different clock or time origin (i.e., not the same
clock/origin as clock_gettime(MONOTONIC)). You'd have to dig deeper to find out
whether that's the case. If it is, I'd still suggest adding a DCHECK to ensure
the assumption isn't broken by CrAS changes in the future, unless you can find
API comments that are specific about this.

https://codereview.chromium.org/2101303004/diff/140001/media/audio/mac/audio_...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2101303004/diff/140001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:263: current_target_playout_time_ + delay;
On 2016/09/16 21:59:43, James West wrote:
> I realized I'm no longer adding hardware_latency_frames_ that was being added
in
> GetPlayoutLatency. I can't find any good documentation for this, but should we
> add both frame_delay and hardware_latency_frames_ to
> current_target_playout_time_ to get target_playout_time?

I think the math you have is correct: First, note that the
|current_target_playout_time_| already accounts for the
hardware_latency_frames_. What it does not account for is the delay due to the
frames inside the FIFO buffer (the |frame_delay| argument). So, you need to add
the two to get the target playout time to pass to the OnMoreData() call.

Said another way, |current_target_playout_time_| would be the
|target_playout_time| you'd use if there was no additional delay from the FIFO
buffer. You could rename |current_target_playout_time_| to
|current_target_playout_time_minus_fifo_delay_| to clarify this.

https://codereview.chromium.org/2101303004/diff/140001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:346: return base::TimeTicks::Now();
On 2016/09/16 21:59:43, James West wrote:
> On 2016/09/16 18:35:58, miu wrote:
> > base::TimeTicks::Now() is being called twice in this method. Could you just
> call
> > it once and then use a var in two places:
> > 
> >   const base::TimeTicks now = base::TimeTicks::Now();
> >   if (...not valid...)
> >     return now;
> >   return std::max(...FromMachTime..., now);
> 
> I'm not opposed to this, but I'm curious what the rationale is when both
> statements can't be executed during the same function call.

Oh, I missed that. Yeah, no change necessary here. :)

[jameswest, 2016-09-17 00:06:50]

https://codereview.chromium.org/2101303004/diff/140001/media/audio/mac/audio_...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2101303004/diff/140001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:263: current_target_playout_time_ + delay;
On 2016/09/16 23:38:28, miu wrote:
> On 2016/09/16 21:59:43, James West wrote:
> > I realized I'm no longer adding hardware_latency_frames_ that was being
added
> in
> > GetPlayoutLatency. I can't find any good documentation for this, but should
we
> > add both frame_delay and hardware_latency_frames_ to
> > current_target_playout_time_ to get target_playout_time?
> 
> I think the math you have is correct: First, note that the
> |current_target_playout_time_| already accounts for the
> hardware_latency_frames_. What it does not account for is the delay due to the
> frames inside the FIFO buffer (the |frame_delay| argument). So, you need to
add
> the two to get the target playout time to pass to the OnMoreData() call.
> 
> Said another way, |current_target_playout_time_| would be the
> |target_playout_time| you'd use if there was no additional delay from the FIFO
> buffer. You could rename |current_target_playout_time_| to
> |current_target_playout_time_minus_fifo_delay_| to clarify this.

|hardware_latency_frames_| is no longer used with my change. It was being added
to the delay frames in GetPlayoutLatency, but I changed that function to just
return |output_time_stamp->mHostTime| converted to TimeTicks, which I think
changed the behavior.

With the old logic, it looks like the real target playout time was
|output_time_stamp->mHostTime| + |hardware_latency_frames_|. When I change
OnMoreData to take a delay, I believe the math equivalent to what is there now
will be as follows:

TimeTicks delay = TimeTicks(output_time_stamp->mHostTime) +
TimeDelta(hardware_latency_frames_) + TimeDelta(frame_delay) - TimeTicks::Now()

[miu, 2016-09-17 00:33:56]

https://codereview.chromium.org/2101303004/diff/140001/media/audio/mac/audio_...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2101303004/diff/140001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:263: current_target_playout_time_ + delay;
On 2016/09/17 00:06:50, James West wrote:
> On 2016/09/16 23:38:28, miu wrote:
> > On 2016/09/16 21:59:43, James West wrote:
> > > I realized I'm no longer adding hardware_latency_frames_ that was being
> added
> > in
> > > GetPlayoutLatency. I can't find any good documentation for this, but
should
> we
> > > add both frame_delay and hardware_latency_frames_ to
> > > current_target_playout_time_ to get target_playout_time?
> 
> With the old logic, it looks like the real target playout time was
> |output_time_stamp->mHostTime| + |hardware_latency_frames_|. When I change
> OnMoreData to take a delay, I believe the math equivalent to what is there now
> will be as follows:
> 
> TimeTicks delay = TimeTicks(output_time_stamp->mHostTime) +
> TimeDelta(hardware_latency_frames_) + TimeDelta(frame_delay) -
TimeTicks::Now()

OIC! I was confused (I thought you were referring to
|current_hardware_pending_bytes_|, from the old code). I think you're right.
Though, would it be better to account for this in GetTargetPlayoutTime() instead
of here?

[jameswest, 2016-09-19 23:32:36]

https://codereview.chromium.org/2101303004/diff/140001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/2101303004/diff/140001/base/time/time.h#newco...
base/time/time.h:738: #if defined(OS_MACOSX)
On 2016/09/16 18:35:58, miu wrote:
> To prevent accidental use of this function on IOS:
> 
>   #if defined(OS_MACOSX) && !defined(OS_IOS)

Done.

https://codereview.chromium.org/2101303004/diff/140001/base/time/time_mac.cc
File base/time/time_mac.cc (right):

https://codereview.chromium.org/2101303004/diff/140001/base/time/time_mac.cc#...
base/time/time_mac.cc:28: int64_t MachAbsoluteTimeToTicks(uint64_t
mach_absolute_time) {
On 2016/09/16 18:35:58, miu wrote:
> Please wrap this function with:
> 
>   #if defined(OS_MACOSX) && !defined(OS_IOS)
>   ...
>   #endif  // defined(OS_MACOSX) && !defined(OS_IOS)

Done.

https://codereview.chromium.org/2101303004/diff/140001/media/audio/cras/cras_...
File media/audio/cras/cras_unified.cc (right):

https://codereview.chromium.org/2101303004/diff/140001/media/audio/cras/cras_...
media/audio/cras/cras_unified.cc:253: const timespec* output_ts,
On 2016/09/16 23:38:28, miu wrote:
> On 2016/09/16 21:59:43, James West wrote:
> > Do you know if |output_ts| is target playout time? The documentation below
> says
> > it's "Playback time for the first sample written."
> >
>
https://chromium.googlesource.com/chromiumos/third_party/adhd/+/stabilize-705...
> 
> That would be my interpretation (that output_ts is the target playout time).
> However...
> 
> > If so, what do you think about adding TimeTicks::FromTimeSpec and using this
> > value directly?
> 
> It could be relative to a different clock or time origin (i.e., not the same
> clock/origin as clock_gettime(MONOTONIC)). You'd have to dig deeper to find
out
> whether that's the case. If it is, I'd still suggest adding a DCHECK to ensure
> the assumption isn't broken by CrAS changes in the future, unless you can find
> API comments that are specific about this.

I couldn't find anything with a quick search, so I left it as is. We need the
latency now anyway, so it would only help to improve the accuracy of the
timestamp, which probably isn't necessary.

https://codereview.chromium.org/2101303004/diff/140001/media/audio/mac/audio_...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2101303004/diff/140001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:263: current_target_playout_time_ + delay;
On 2016/09/17 00:33:56, miu wrote:
> On 2016/09/17 00:06:50, James West wrote:
> > On 2016/09/16 23:38:28, miu wrote:
> > > On 2016/09/16 21:59:43, James West wrote:
> > > > I realized I'm no longer adding hardware_latency_frames_ that was being
> > added
> > > in
> > > > GetPlayoutLatency. I can't find any good documentation for this, but
> should
> > we
> > > > add both frame_delay and hardware_latency_frames_ to
> > > > current_target_playout_time_ to get target_playout_time?
> > 
> > With the old logic, it looks like the real target playout time was
> > |output_time_stamp->mHostTime| + |hardware_latency_frames_|. When I change
> > OnMoreData to take a delay, I believe the math equivalent to what is there
now
> > will be as follows:
> > 
> > TimeTicks delay = TimeTicks(output_time_stamp->mHostTime) +
> > TimeDelta(hardware_latency_frames_) + TimeDelta(frame_delay) -
> TimeTicks::Now()
> 
> OIC! I was confused (I thought you were referring to
> |current_hardware_pending_bytes_|, from the old code). I think you're right.
> Though, would it be better to account for this in GetTargetPlayoutTime()
instead
> of here?

I changed GetHardwareLatency() to return TimeDelta instead of frames and added
it to the TimeTicks returned by GetPlayoutTime().

https://codereview.chromium.org/2101303004/diff/140001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:346: return base::TimeTicks::Now();
On 2016/09/16 23:38:28, miu wrote:
> On 2016/09/16 21:59:43, James West wrote:
> > On 2016/09/16 18:35:58, miu wrote:
> > > base::TimeTicks::Now() is being called twice in this method. Could you
just
> > call
> > > it once and then use a var in two places:
> > > 
> > >   const base::TimeTicks now = base::TimeTicks::Now();
> > >   if (...not valid...)
> > >     return now;
> > >   return std::max(...FromMachTime..., now);
> > 
> > I'm not opposed to this, but I'm curious what the rationale is when both
> > statements can't be executed during the same function call.
> 
> Oh, I missed that. Yeah, no change necessary here. :)

Acknowledged.

https://codereview.chromium.org/2101303004/diff/140001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/2101303004/diff/140001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:524: num_written_frames_ +
packet_size_frames_ - played_out_frames;
On 2016/09/16 21:59:43, James West wrote:
> On 2016/09/16 18:35:58, miu wrote:
> > Is is correct to include the |packet_size_frames_| in the new calculation?
> > Unless I'm missing something, it seems that would set |target_playout_delay|
> to
> > the *last* sample about to be rendered by OnMoreData(), rather than the
> *first*
> > one (which is what the interface intends).
> 
> I believe you're correct. I'll change it.

Done.

[James West, 2016-09-20 04:42:50]

The CQ bit was checked by jameswest@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 04:42:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 04:45:29]

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

[commit-bot: I haz the power, 2016-09-20 04:45:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[James West, 2016-09-20 05:55:07]

The CQ bit was checked by jameswest@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 05:55:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 06:11:08]

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

[commit-bot: I haz the power, 2016-09-20 06:11:09]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[jameswest, 2016-09-20 07:45:41]

The CQ bit was checked by jameswest@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 07:45:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James West, 2016-09-20 07:55:13]

The CQ bit was checked by jameswest@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 07:55:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 08:15:30]

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

[commit-bot: I haz the power, 2016-09-20 08:15:32]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[jameswest, 2016-09-20 08:52:01]

The CQ bit was checked by jameswest@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 08:52:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mikhail, 2016-09-20 09:39:02]

https://codereview.chromium.org/2101303004/diff/220001/media/audio/audio_io.h
File media/audio/audio_io.h (right):

https://codereview.chromium.org/2101303004/diff/220001/media/audio/audio_io.h...
media/audio/audio_io.h:72: virtual int OnMoreData(base::TimeDelta delay,
Thanks for providing this API!

[commit-bot: I haz the power, 2016-09-20 09:40:42]

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

[commit-bot: I haz the power, 2016-09-20 09:40:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[James West, 2016-09-20 19:56:43]

The CQ bit was checked by jameswest@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 19:57:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 21:34:30]

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

[commit-bot: I haz the power, 2016-09-20 21:34:33]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jameswest, 2016-09-20 22:23:11]

The CQ bit was checked by jameswest@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 22:23:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jameswest, 2016-09-20 22:24:06]

The CQ bit was checked by jameswest@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 22:24:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 00:18:37]

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

[commit-bot: I haz the power, 2016-09-21 00:18:40]

Dry run: This issue passed the CQ dry run.

[jameswest, 2016-09-21 00:49:43]

Description was changed from

==========
Pass target playout time to AudioSourceCallback::OnMoreData.

Replace audio delay in bytes with the timestamp at which the next
sample provided is expected to be played. This is needed to accurately
synchronize playback across Cast devices in a multizone group.

BUG=internal b/29618700
TEST=base_unittests, media_unittests
==========

to

==========
Pass delay and timestamp to AudioSourceCallback::OnMoreData.
    
Replace audio delay in bytes with audio delay in time and the
timestamp at which the delay was measured. This is needed to
accurately synchronize playback across Cast devices in a multizone
group.

BUG=internal b/29618700
TEST=base, cast, cast_media, content, and media unit tests
==========

[jameswest, 2016-09-21 00:54:57]

I fixed all the CQ errors, so it's ready for everyone to take a final look.

[chcunningham, 2016-09-23 20:53:30]

Some cases of integer division in computing delay where the truncation will
accumulate across the multiplication of frames - can you cast these to double?

Or, sidestep that conversion by changing the API to report delay in terms of
frames... a much bigger change. I prefer frames, but I also think this CL is in
great shape and the review has dragged on - your call.

https://codereview.chromium.org/2101303004/diff/240001/chromecast/media/audio...
File chromecast/media/audio/cast_audio_mixer.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/chromecast/media/audio...
chromecast/media/audio/cast_audio_mixer.cc:146: input_params_.sample_rate());
int division

https://codereview.chromium.org/2101303004/diff/240001/chromecast/media/audio...
chromecast/media/audio/cast_audio_mixer.cc:257: uint32_t frames_delayed =
delay.InSecondsF() * output_params_.sample_rate();
What do you think about delay in frames instead of TimeDelta? We could skip this
conversion and get a little more precision. Its late feedback for this review
though - I don't care enough to block you further if you feel this has gone on
long enough ;)

https://codereview.chromium.org/2101303004/diff/240001/media/audio/android/op...
File media/audio/android/opensles_output.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/android/op...
media/audio/android/opensles_output.cc:359: delay_frames *
base::Time::kMicrosecondsPerSecond /
int division

https://codereview.chromium.org/2101303004/diff/240001/media/audio/audio_outp...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/audio_outp...
media/audio/audio_output_resampler.cc:459: frames_delayed *
base::Time::kMicrosecondsPerSecond /
ditto about int division

https://codereview.chromium.org/2101303004/diff/240001/media/audio/mac/audio_...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:26: frames *
base::Time::kMicrosecondsPerSecond / sample_rate);
integer division

https://codereview.chromium.org/2101303004/diff/240001/media/audio/virtual_au...
File media/audio/virtual_audio_output_stream.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/virtual_au...
media/audio/virtual_audio_output_stream.cc:84: frames_delayed *
base::Time::kMicrosecondsPerSecond /
int division

https://codereview.chromium.org/2101303004/diff/240001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:522:
format_.Format.nSamplesPerSec * position / device_frequency;
int division

https://codereview.chromium.org/2101303004/diff/240001/media/audio/win/waveou...
File media/audio/win/waveout_output_win.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/win/waveou...
media/audio/win/waveout_output_win.cc:328: pending_bytes_ *
base::Time::kMicrosecondsPerSecond /
int division

[jameswest, 2016-09-23 22:56:19]

On 2016/09/23 20:53:30, chcunningham wrote:
> Some cases of integer division in computing delay where the truncation will
> accumulate across the multiplication of frames - can you cast these to double?

When converting frames to TimeDelta, I don't see how casting to double would
help because the integral number of frames is multiplied by 1000000 first, which
will always produce a whole number result, and the result of dividing that value
by sample rate is passed to FromMicroseconds, which requires an integer and
would truncate the value even if it were a double. Or do you want to round the
result instead of truncate?

[jameswest, 2016-09-23 23:00:28]

Is there a good place to put utility functions to convert frames to TimeDelta
and TimeDelta to frames? It'd be nice to have this logic in one place and with
some unit tests.

[chcunningham, 2016-09-28 19:39:14]

On 2016/09/23 22:56:19, James West wrote:
> On 2016/09/23 20:53:30, chcunningham wrote:
> > Some cases of integer division in computing delay where the truncation will
> > accumulate across the multiplication of frames - can you cast these to
double?
> 
> When converting frames to TimeDelta, I don't see how casting to double would
> help because the integral number of frames is multiplied by 1000000 first,
which
> will always produce a whole number result, and the result of dividing that
value
> by sample rate is passed to FromMicroseconds, which requires an integer and
> would truncate the value even if it were a double. Or do you want to round the
> result instead of truncate?

Ah, sorry I mixed up the order of operations. Agree, we don't seem to have an
issue as long the division comes after the multiplication. 

re: rounding, I know it won't matter for the use cases I'm familiar with
(<audio><video>MSE), but I'm not an expert on some of the cast and WebAudio code
paths. Some discussion about this here:
https://bugs.chromium.org/p/chromium/issues/detail?id=586540#c10

That link also has some discussion about using frames instead of TimeDelta

[chcunningham, 2016-09-28 19:40:53]

On 2016/09/23 23:00:28, James West wrote:
> Is there a good place to put utility functions to convert frames to TimeDelta
> and TimeDelta to frames? It'd be nice to have this logic in one place and with
> some unit tests.

We sort of have that here:
https://cs.chromium.org/chromium/src/media/base/audio_timestamp_helper.h?q=Ti...

Though thats probably more than you want. It might be worth it to make a static
function that just converts frames -> TimeDelta given a particular sample rate
(and use that in TimestampHelper as well).

[miu, 2016-09-28 23:04:41]

Patch Set 8 LGTM % nits:

https://codereview.chromium.org/2101303004/diff/240001/chromecast/media/audio...
File chromecast/media/audio/cast_audio_mixer.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/chromecast/media/audio...
chromecast/media/audio/cast_audio_mixer.cc:257: uint32_t frames_delayed =
delay.InSecondsF() * output_params_.sample_rate();
On 2016/09/23 20:53:29, chcunningham wrote:
> What do you think about delay in frames instead of TimeDelta? We could skip
this
> conversion and get a little more precision.

I'm torn between both, myself. But, overall, I suppose I'm slightly in favor of
the way we have it.

FWICT, here are the pros/cons of each:

* TimeDelta delay pros: Easier math in OnMoreData() impls, when computing target
playout time, and no need for them to look-up sample rate to map from # frames
to # microseconds. Also, resamplers do not need to recompute delay (unless
stretching a signal).

* TimeDelta delay cons: If client code needs to know an exact # frames, it may
occasionally be off-by-one unless we are more careful about the math. For
example, at 48kHz, one sample delay is equal to 20.8333... microseconds. As the
code is right now, if there was a one sample delay, our calls to OnMoreData()
would report the delay as 20 microseconds, not 21 microseconds. 21 is more
accurate, so maybe the caller of OnMoreData() should do rounding. OR, it would
also work for the impl of OnMoreData() to do rounding (20 usec is clearly one
frame of delay, not zero frames). OR, maybe we do NO rounding because such
accuracy is not necessary?

* frame delay pros: An exact delay measurement (because the timebase is the
sample rate).

* frame delay cons: Harder to compute target playout time. Also, depending on
client code to do the correct math downstream.

https://codereview.chromium.org/2101303004/diff/240001/media/audio/audio_outp...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/audio_outp...
media/audio/audio_output_controller.cc:324: const base::TimeTicks reference_time
= base::TimeTicks::Now() + delay;
Seems like this should be:

  reference_time = delay_timestamp + delay

https://codereview.chromium.org/2101303004/diff/240001/media/audio/cras/cras_...
File media/audio/cras/cras_unified.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/cras/cras_...
media/audio/cras/cras_unified.cc:292: const base::TimeDelta delay =
latency_ts.tv_sec > 0 && latency_ts.tv_nsec > 0
Is it possible for tv_sec to be greater than zero, but tv_nsec to be negative?

Might be simpler to just:

  const base::TimeDelta delay =
std::max(base::TimeDelta::FromTimeSpec(latency_ts), base::TimeDelta());

[miu, 2016-09-28 23:05:16]

On 2016/09/28 23:04:41, miu wrote:
> Patch Set 8 LGTM % nits:

Whoops! I mean, Patch Set 13 LGTM.

[jameswest, 2016-09-29 00:41:47]

The CQ bit was checked by jameswest@google.com

[jameswest, 2016-09-29 00:41:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org, chcunningham@google.com, miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2101303004/#ps280001
(title: "Changes based on comments")

[jameswest, 2016-09-29 00:41:49]

The CQ bit was unchecked by jameswest@google.com

[jameswest, 2016-09-29 00:42:02]

The CQ bit was checked by jameswest@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 00:42:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jameswest, 2016-09-29 00:52:24]

https://codereview.chromium.org/2101303004/diff/240001/chromecast/media/audio...
File chromecast/media/audio/cast_audio_mixer.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/chromecast/media/audio...
chromecast/media/audio/cast_audio_mixer.cc:146: input_params_.sample_rate());
On 2016/09/23 20:53:29, chcunningham wrote:
> int division

Moved to utility function.

https://codereview.chromium.org/2101303004/diff/240001/chromecast/media/audio...
chromecast/media/audio/cast_audio_mixer.cc:257: uint32_t frames_delayed =
delay.InSecondsF() * output_params_.sample_rate();
On 2016/09/28 23:04:41, miu wrote:
> On 2016/09/23 20:53:29, chcunningham wrote:
> > What do you think about delay in frames instead of TimeDelta? We could skip
> this
> > conversion and get a little more precision.
> 
> I'm torn between both, myself. But, overall, I suppose I'm slightly in favor
of
> the way we have it.
> 
> FWICT, here are the pros/cons of each:
> 
> * TimeDelta delay pros: Easier math in OnMoreData() impls, when computing
target
> playout time, and no need for them to look-up sample rate to map from # frames
> to # microseconds. Also, resamplers do not need to recompute delay (unless
> stretching a signal).
> 
> * TimeDelta delay cons: If client code needs to know an exact # frames, it may
> occasionally be off-by-one unless we are more careful about the math. For
> example, at 48kHz, one sample delay is equal to 20.8333... microseconds. As
the
> code is right now, if there was a one sample delay, our calls to OnMoreData()
> would report the delay as 20 microseconds, not 21 microseconds. 21 is more
> accurate, so maybe the caller of OnMoreData() should do rounding. OR, it would
> also work for the impl of OnMoreData() to do rounding (20 usec is clearly one
> frame of delay, not zero frames). OR, maybe we do NO rounding because such
> accuracy is not necessary?
> 
> * frame delay pros: An exact delay measurement (because the timebase is the
> sample rate).
> 
> * frame delay cons: Harder to compute target playout time. Also, depending on
> client code to do the correct math downstream.

I'm personally ambivalent. One thing I'll add is that frames are not always more
accurate because on some platforms, such as Chrome OS, the underlying audio
library gives us the delay in time rather than frames. I'll leave it as
TimeDelta for this change since that's the way I already have it. It shouldn't
be too difficult to change it later if we decide frames are better.

I didn't do any rounding because I don't think we should add an operation
without evidence that it actually helps, but I don't mind adding it if someone
thinks we should. We only have to change it in one place now if we need to
change it.

https://codereview.chromium.org/2101303004/diff/240001/media/audio/android/op...
File media/audio/android/opensles_output.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/android/op...
media/audio/android/opensles_output.cc:359: delay_frames *
base::Time::kMicrosecondsPerSecond /
On 2016/09/23 20:53:29, chcunningham wrote:
> int division

Moved to utility function.

https://codereview.chromium.org/2101303004/diff/240001/media/audio/audio_outp...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/audio_outp...
media/audio/audio_output_controller.cc:324: const base::TimeTicks reference_time
= base::TimeTicks::Now() + delay;
On 2016/09/28 23:04:41, miu wrote:
> Seems like this should be:
> 
>   reference_time = delay_timestamp + delay

Done.

https://codereview.chromium.org/2101303004/diff/240001/media/audio/audio_outp...
File media/audio/audio_output_resampler.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/audio_outp...
media/audio/audio_output_resampler.cc:459: frames_delayed *
base::Time::kMicrosecondsPerSecond /
On 2016/09/23 20:53:30, chcunningham wrote:
> ditto about int division

Moved to utility function.

https://codereview.chromium.org/2101303004/diff/240001/media/audio/cras/cras_...
File media/audio/cras/cras_unified.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/cras/cras_...
media/audio/cras/cras_unified.cc:292: const base::TimeDelta delay =
latency_ts.tv_sec > 0 && latency_ts.tv_nsec > 0
On 2016/09/28 23:04:41, miu wrote:
> Is it possible for tv_sec to be greater than zero, but tv_nsec to be negative?
> 
> Might be simpler to just:
> 
>   const base::TimeDelta delay =
> std::max(base::TimeDelta::FromTimeSpec(latency_ts), base::TimeDelta());

That shouldn't happen, and even if it does, using a TimeDelta of zero isn't
necessary more correct than computing it from the values as given. I changed it
as suggested.

https://codereview.chromium.org/2101303004/diff/240001/media/audio/mac/audio_...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/mac/audio_...
media/audio/mac/audio_auhal_mac.cc:26: frames *
base::Time::kMicrosecondsPerSecond / sample_rate);
On 2016/09/23 20:53:30, chcunningham wrote:
> integer division

Removed.

https://codereview.chromium.org/2101303004/diff/240001/media/audio/virtual_au...
File media/audio/virtual_audio_output_stream.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/virtual_au...
media/audio/virtual_audio_output_stream.cc:84: frames_delayed *
base::Time::kMicrosecondsPerSecond /
On 2016/09/23 20:53:30, chcunningham wrote:
> int division

Moved to utility function.

https://codereview.chromium.org/2101303004/diff/240001/media/audio/win/audio_...
File media/audio/win/audio_low_latency_output_win.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/win/audio_...
media/audio/win/audio_low_latency_output_win.cc:522:
format_.Format.nSamplesPerSec * position / device_frequency;
On 2016/09/23 20:53:30, chcunningham wrote:
> int division

As discussed, this integer division is safe.

https://codereview.chromium.org/2101303004/diff/240001/media/audio/win/waveou...
File media/audio/win/waveout_output_win.cc (right):

https://codereview.chromium.org/2101303004/diff/240001/media/audio/win/waveou...
media/audio/win/waveout_output_win.cc:328: pending_bytes_ *
base::Time::kMicrosecondsPerSecond /
On 2016/09/23 20:53:30, chcunningham wrote:
> int division

As discussed, this integer division is safe.

[commit-bot: I haz the power, 2016-09-29 02:01:16]

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

[commit-bot: I haz the power, 2016-09-29 02:01:20]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[James West, 2016-09-29 07:35:12]

The CQ bit was checked by jameswest@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 07:35:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 11:05:05]

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

[commit-bot: I haz the power, 2016-09-29 11:05:07]

Dry run: This issue passed the CQ dry run.

[chcunningham, 2016-09-29 21:30:16]

media LGTM

[jameswest, 2016-09-29 21:43:37]

The CQ bit was checked by jameswest@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 21:44:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 21:50:17]

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

[commit-bot: I haz the power, 2016-09-29 21:50:19]

Dry run: This issue passed the CQ dry run.

[kmackay, 2016-09-29 21:53:38]

chromecast changes lgtm (need alokp for owner's approval)

[alokp, 2016-09-30 00:12:21]

chromecast/ lgtm

https://codereview.chromium.org/2101303004/diff/300001/chromecast/media/audio...
File chromecast/media/audio/cast_audio_output_stream.h (right):

https://codereview.chromium.org/2101303004/diff/300001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.h:8: #include <memory>
is this due to restructuring of any header that this file includes?

[jameswest, 2016-09-30 00:30:16]

https://codereview.chromium.org/2101303004/diff/300001/chromecast/media/audio...
File chromecast/media/audio/cast_audio_output_stream.h (right):

https://codereview.chromium.org/2101303004/diff/300001/chromecast/media/audio...
chromecast/media/audio/cast_audio_output_stream.h:8: #include <memory>
On 2016/09/30 00:12:21, alokp wrote:
> is this due to restructuring of any header that this file includes?

This is just fixing a lint error for cast_audio_output_stream.cc. The #include
should be in the header because it also uses std::unique_ptr.

[jameswest, 2016-09-30 00:35:50]

The CQ bit was checked by jameswest@google.com

[jameswest, 2016-09-30 00:35:51]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org,
chcunningham@google.com
Link to the patchset: https://codereview.chromium.org/2101303004/#ps300001
(title: "Fix Mac CQ errors.")

[commit-bot: I haz the power, 2016-09-30 00:36:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 00:46:05]

Description was changed from

==========
Pass delay and timestamp to AudioSourceCallback::OnMoreData.
    
Replace audio delay in bytes with audio delay in time and the
timestamp at which the delay was measured. This is needed to
accurately synchronize playback across Cast devices in a multizone
group.

BUG=internal b/29618700
TEST=base, cast, cast_media, content, and media unit tests
==========

to

==========
Pass delay and timestamp to AudioSourceCallback::OnMoreData.
    
Replace audio delay in bytes with audio delay in time and the
timestamp at which the delay was measured. This is needed to
accurately synchronize playback across Cast devices in a multizone
group.

BUG=internal b/29618700
TEST=base, cast, cast_media, content, and media unit tests
==========

[commit-bot: I haz the power, 2016-09-30 00:46:07]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-09-30 00:50:16]

Description was changed from

==========
Pass delay and timestamp to AudioSourceCallback::OnMoreData.
    
Replace audio delay in bytes with audio delay in time and the
timestamp at which the delay was measured. This is needed to
accurately synchronize playback across Cast devices in a multizone
group.

BUG=internal b/29618700
TEST=base, cast, cast_media, content, and media unit tests
==========

to

==========
Pass delay and timestamp to AudioSourceCallback::OnMoreData.

Replace audio delay in bytes with audio delay in time and the
timestamp at which the delay was measured. This is needed to
accurately synchronize playback across Cast devices in a multizone
group.

BUG=internal b/29618700
TEST=base, cast, cast_media, content, and media unit tests

Committed: https://crrev.com/3de19856c9d181a606df679384c6e0e84c4b0f39
Cr-Commit-Position: refs/heads/master@{#421999}
==========

[commit-bot: I haz the power, 2016-09-30 00:50:19]

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