Forward the number of skipped frames by the OS in audio playout.

Forward the number of skipped frames by the OS in audio playout.

* Only for Mac.

Will be done in follow-up CLs:
* Windows.
* Hooking this up to the AEC.

BUG=560371
Committed: https://crrev.com/db03a8d1128783ff4803364ba616ae5fac8822f7
Cr-Commit-Position: refs/heads/master@{#364990}

[Henrik Grunell, 2015-12-01 10:26:58]

grunell@chromium.org changed reviewers:
+ tommi@chromium.org

[Henrik Grunell, 2015-12-01 10:26:58]

Tommi, could you take a look?

We could also just always pass the number of skipped frames, also for input.
That would remove the ugly conditionals in AudioOutputDevice that I used now due
to the todo comment about sharing thread for multiple in/outputs.

I waited with making two new interface functions pure virtual until first review
round(s).

Will be done in a follow-up CLs:
* Windows.
* Hooking this up to the AEC.

[Henrik Grunell, 2015-12-01 10:28:45]

Description was changed from

==========
Forward the number of skipped frames by the OS in audio playout.

BUG=560371
==========

to

==========
Forward the number of skipped frames by the OS in audio playout.

* Only for Mac.

Will be done in follow-up CLs:
* Windows.
* Hooking this up to the AEC.

BUG=560371
==========

[tommi (sloooow) - chröme, 2015-12-01 13:34:26]

https://codereview.chromium.org/1487983002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/1487983002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_sync_reader.cc:89: uint64_t data =
(static_cast<uint64_t>(bytes) << 32) | frames_skipped;
can we use a struct with two uint32_t's instead?

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.cc:182: uint64_t data = 0;
if you don't want to use a struct, you can also use an array of two uint32_t's.

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
File media/audio/audio_device_thread.h (right):

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.h:86: // TODO(grunell): |input| is ugly, but
there is plans to re-use a thread for
s/is plans/are plans

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.h:88: // meanwhile instead of using separating
classes.
using separate classes

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_output_co...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_output_co...
media/audio/audio_output_controller.cc:298:
base::saturated_cast<uint32>(total_bytes_delay +
this saturated cast looks like a hack. :-/

Can we at least pull it out of this expression since total_bytes_delay is
already the right type, and do:

uint32_t frame_bytes = base::saturated_cast<uint32>(frames *
params_.GetBytesPerFrame());
sync_reader_->UpdatePendingBytes(total_bytes_delay + frame_bytes);

Even so, frame_bytes should never overflow, so maybe we could do something more
drastic if it ever does (if we need to).

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_output_co...
media/audio/audio_output_controller.cc:309: void
AudioOutputController::OnSkippedData(uint32 frames_skipped) {
thread check

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_output_de...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_output_de...
media/audio/audio_output_device.cc:41: // Process().
nit: and modified in FramesSkipped

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_output_de...
media/audio/audio_output_device.cc:440: void
AudioOutputDevice::AudioThreadCallback::FramesSkipped(
thread check (must be on the same thread as Process is called on)

[DaleCurtis, 2015-12-01 18:12:46]

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

[DaleCurtis, 2015-12-01 18:12:46]

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.cc:182: uint64_t data = 0;
On 2015/12/01 13:34:25, tommi wrote:
> if you don't want to use a struct, you can also use an array of two
uint32_t's.

Long ago we were worried anything more than 4 bytes might lead to being
descheduled, crogers@ felt pretty strongly about this, I don't know if it's
actually an issue though.

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

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_io.h#newc...
media/audio/audio_io.h:66: // Informs that the consumer has skipped
|frames_skipped| frames. The source
Why a new method instead of rolling this into OnMoreData() ?

https://codereview.chromium.org/1487983002/diff/1/media/base/audio_renderer_s...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/1487983002/diff/1/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:44: // Signals that the consumer has skipped
frames in playout.
Why a new method instead of rolling this into the Render() call?

[tommi (sloooow) - chröme, 2015-12-01 19:03:31]

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.cc:182: uint64_t data = 0;
On 2015/12/01 18:12:46, DaleCurtis wrote:
> On 2015/12/01 13:34:25, tommi wrote:
> > if you don't want to use a struct, you can also use an array of two
> uint32_t's.
> 
> Long ago we were worried anything more than 4 bytes might lead to being
> descheduled, crogers@ felt pretty strongly about this, I don't know if it's
> actually an issue though.

Would it work to use the shared memory buffer instead?

[DaleCurtis, 2015-12-01 20:06:48]

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.cc:182: uint64_t data = 0;
On 2015/12/01 19:03:31, tommi wrote:
> On 2015/12/01 18:12:46, DaleCurtis wrote:
> > On 2015/12/01 13:34:25, tommi wrote:
> > > if you don't want to use a struct, you can also use an array of two
> > uint32_t's.
> > 
> > Long ago we were worried anything more than 4 bytes might lead to being
> > descheduled, crogers@ felt pretty strongly about this, I don't know if it's
> > actually an issue though.
> 
> Would it work to use the shared memory buffer instead?

That's what we do for input I believe.

[Henrik Grunell, 2015-12-02 13:47:05]

I've done some CR fixes, will continue tomorrow and reply to the comments.

Dale: The reason I separated the FramesSkipped() in the output controller is
because of the fifo on Mac.

[DaleCurtis, 2015-12-02 17:17:03]

It's the pass through components responsibility for making sure Render() data is
accurate; e.g., see all the manipulation to delay in AudioConverter to account
for resampling.

In this case, you may not need to go that far anyways, just tweak the frames
missing value on back-to-back calls within the AUHAL drivers call to OnMoreData
itself.

[Henrik Grunell, 2015-12-03 14:01:37]

On 2015/12/02 17:17:03, DaleCurtis wrote:
> It's the pass through components responsibility for making sure Render() data
is
> accurate; e.g., see all the manipulation to delay in AudioConverter to account
> for resampling.
> 
> In this case, you may not need to go that far anyways, just tweak the frames
> missing value on back-to-back calls within the AUHAL drivers call to
OnMoreData
> itself.

Argh, of course: the fifo pulls from AUHALStream::ProvideInput. Will change
that.

[Henrik Grunell, 2015-12-03 17:01:07]

Addressed all comments except the 4 byte limit for the socket. Will do that
tomorrow.

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.cc:182: uint64_t data = 0;
On 2015/12/01 20:06:48, DaleCurtis wrote:
> On 2015/12/01 19:03:31, tommi wrote:
> > On 2015/12/01 18:12:46, DaleCurtis wrote:
> > > On 2015/12/01 13:34:25, tommi wrote:
> > > > if you don't want to use a struct, you can also use an array of two
> > > uint32_t's.
> > > 
> > > Long ago we were worried anything more than 4 bytes might lead to being
> > > descheduled, crogers@ felt pretty strongly about this, I don't know if
it's
> > > actually an issue though.
> > 
> > Would it work to use the shared memory buffer instead?
> 
> That's what we do for input I believe.

Good to know. We can use the shared memory. Yes we do use it for input, with the
difference that in that case the meta data goes along with the data. Here it
goes in different directions. I'll upload a new patch with this tomorrow.

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
File media/audio/audio_device_thread.h (right):

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.h:86: // TODO(grunell): |input| is ugly, but
there is plans to re-use a thread for
On 2015/12/01 13:34:25, tommi wrote:
> s/is plans/are plans

Done.

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_device_th...
media/audio/audio_device_thread.h:88: // meanwhile instead of using separating
classes.
On 2015/12/01 13:34:25, tommi wrote:
> using separate classes

Done.

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

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_io.h#newc...
media/audio/audio_io.h:66: // Informs that the consumer has skipped
|frames_skipped| frames. The source
On 2015/12/01 18:12:46, DaleCurtis wrote:
> Why a new method instead of rolling this into OnMoreData() ?

(See previous comment exchange.) Put this in OnMoreData().

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_output_co...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_output_co...
media/audio/audio_output_controller.cc:298:
base::saturated_cast<uint32>(total_bytes_delay +
On 2015/12/01 13:34:25, tommi wrote:
> this saturated cast looks like a hack. :-/
> 
> Can we at least pull it out of this expression since total_bytes_delay is
> already the right type, and do:
> 
> uint32_t frame_bytes = base::saturated_cast<uint32>(frames *
> params_.GetBytesPerFrame());
> sync_reader_->UpdatePendingBytes(total_bytes_delay + frame_bytes);
> 
> Even so, frame_bytes should never overflow, so maybe we could do something
more
> drastic if it ever does (if we need to).

Agree, I don't know why we would fear it overflowing.

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_output_co...
media/audio/audio_output_controller.cc:309: void
AudioOutputController::OnSkippedData(uint32 frames_skipped) {
On 2015/12/01 13:34:25, tommi wrote:
> thread check

Done.

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_output_de...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_output_de...
media/audio/audio_output_device.cc:41: // Process().
On 2015/12/01 13:34:26, tommi wrote:
> nit: and modified in FramesSkipped

Done.

https://codereview.chromium.org/1487983002/diff/1/media/audio/audio_output_de...
media/audio/audio_output_device.cc:440: void
AudioOutputDevice::AudioThreadCallback::FramesSkipped(
On 2015/12/01 13:34:25, tommi wrote:
> thread check (must be on the same thread as Process is called on)

The contract with AudioDeviceThread is that all callbacks will run on the same
thread. (See ADT class comment.) I can still add a thread check  of course.
Better safe than sorry?

https://codereview.chromium.org/1487983002/diff/1/media/base/audio_renderer_s...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/1487983002/diff/1/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:44: // Signals that the consumer has skipped
frames in playout.
On 2015/12/01 18:12:46, DaleCurtis wrote:
> Why a new method instead of rolling this into the Render() call?

I had some reason that I don't recall or can figure out now. Changed.

[DaleCurtis, 2015-12-04 01:07:33]

https://codereview.chromium.org/1487983002/diff/20001/media/base/audio_render...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/1487983002/diff/20001/media/base/audio_render...
media/base/audio_renderer_sink.h:41: int audio_delay_milliseconds,
Should we change this to uint32_t while we're at it?

[DaleCurtis, 2015-12-04 01:07:57]

interfaces lgtm, defer to tommi@ for the main review.

[Henrik Grunell, 2015-12-04 09:48:37]

grunell@chromium.org changed reviewers:
+ lcwu@chromium.org, rkc@chromium.org

[Henrik Grunell, 2015-12-04 09:50:25]

lcwu@: chromecast/*
rkc@: components/audio/modem/*
(trivial changes)

Tommi: ptal.

https://codereview.chromium.org/1487983002/diff/20001/media/base/audio_render...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/1487983002/diff/20001/media/base/audio_render...
media/base/audio_renderer_sink.h:41: int audio_delay_milliseconds,
On 2015/12/04 01:07:33, DaleCurtis wrote:
> Should we change this to uint32_t while we're at it?

Good idea. Done.

[Henrik Grunell, 2015-12-04 13:30:45]

Now also writing skipped frames to shared memory instead.

[Henrik Grunell, 2015-12-08 07:49:24]

Tommi, rkc@, lcwu@: ping for review.

[tommi (sloooow) - chröme, 2015-12-08 08:34:51]

https://codereview.chromium.org/1487983002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/1487983002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_sync_reader.cc:89: // Increase the
number of skipped frames stored in shared memory. We don't
why do we increase the number of skipped frames?
I would have expected just to set how many have been skipped directly and not
accumulate.

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_devic...
File media/audio/audio_device_thread.h (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_devic...
media/audio/audio_device_thread.h:78: // |socket| every time
AudioDeviceThread::Process() completes. |input| should
what does |input| refer to?

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_input...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_input...
media/audio/audio_input_device.cc:145: true);
no change?

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_outpu...
File media/audio/audio_output_controller.h (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_outpu...
media/audio/audio_output_controller.h:87: // frames has been skipped by the
consumer. The source can handle this
Maybe I'm misunderstanding the context but it seems like 'producer' might be
more fitting than 'consumer' since the SyncReader is a consumer and whatever
higher up it feeds the data too.
'source' also feels backwards to me since it's the source that's providing this
data and it's up to the recipient of the data, i.e. the actual consumer, to
handle it.

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_outpu...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_outpu...
media/audio/audio_output_device.cc:428: buffer->params.frames_skipped = 0;
ah, so we reset the value here... is there a reason why we do it this way
instead of just overwriting an old value on the other side?
If frames_skipped is something else than 0 when we increment it, wouldn't that
be an error?  (and aren't we then about to overwrite a buffer?)

https://codereview.chromium.org/1487983002/diff/80001/media/audio/mac/audio_a...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/mac/audio_a...
media/audio/mac/audio_auhal_mac.cc:359: void
AUHALStream::UpdatePlayoutTimestampAndStats(
nit: I'd just keep the name as it was.  "AndStats" indicates we're updating
stats, but I don't see that we're doing that.

https://codereview.chromium.org/1487983002/diff/80001/media/base/audio_render...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/1487983002/diff/80001/media/base/audio_render...
media/base/audio_renderer_sink.h:39: // the consumer has skipped, if any.
Since it's the OS that is skipping these frames and to us, it's actually the
provider of the data, I find it confusing to use the term consumer

[Henrik Grunell, 2015-12-08 09:30:34]

https://codereview.chromium.org/1487983002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/1487983002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_sync_reader.cc:89: // Increase the
number of skipped frames stored in shared memory. We don't
On 2015/12/08 08:34:51, tommi wrote:
> why do we increase the number of skipped frames?
> I would have expected just to set how many have been skipped directly and not
> accumulate.

Afaict, there is a possibility that we write more than once before the render
side has read the data. That would happen if it takes a very long time for it to
execute, WaitUntilDataIsReady times out on this side, and this function is
called again before the other side is done putting the previously requested data
in the shared memory. In that case we'd overwrite and lose information about
those skipped frames.

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_devic...
File media/audio/audio_device_thread.h (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_devic...
media/audio/audio_device_thread.h:78: // |socket| every time
AudioDeviceThread::Process() completes. |input| should
On 2015/12/08 08:34:51, tommi wrote:
> what does |input| refer to?

Whether this object is used for input audio or not (i.e. output).

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_input...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_input...
media/audio/audio_input_device.cc:145: true);
On 2015/12/08 08:34:51, tommi wrote:
> no change?

Mandatory "git cl format media"...

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_outpu...
File media/audio/audio_output_controller.h (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_outpu...
media/audio/audio_output_controller.h:87: // frames has been skipped by the
consumer. The source can handle this
On 2015/12/08 08:34:51, tommi wrote:
> Maybe I'm misunderstanding the context but it seems like 'producer' might be
> more fitting than 'consumer' since the SyncReader is a consumer and whatever
> higher up it feeds the data too.
> 'source' also feels backwards to me since it's the source that's providing
this
> data and it's up to the recipient of the data, i.e. the actual consumer, to
> handle it.

OK. Well, it refers to the actual audio data, which flows towards the OS, who is
the end consumer for that data. This function and its parameters go in the
opposite direction as a requester for the data. I see that the comment can be
confusing. We should agree on the terminology, or I can clarify this in the
text.

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_outpu...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_outpu...
media/audio/audio_output_device.cc:428: buffer->params.frames_skipped = 0;
On 2015/12/08 08:34:51, tommi wrote:
> ah, so we reset the value here... is there a reason why we do it this way
> instead of just overwriting an old value on the other side?
> If frames_skipped is something else than 0 when we increment it, wouldn't that
> be an error?  (and aren't we then about to overwrite a buffer?)

Yes, as my reply to the other comment, we can overwrite a buffer, both the
skipped frames information, and the audio data. If we miss the deadline.

https://codereview.chromium.org/1487983002/diff/80001/media/audio/mac/audio_a...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/mac/audio_a...
media/audio/mac/audio_auhal_mac.cc:359: void
AUHALStream::UpdatePlayoutTimestampAndStats(
On 2015/12/08 08:34:51, tommi wrote:
> nit: I'd just keep the name as it was.  "AndStats" indicates we're updating
> stats, but I don't see that we're doing that.

Yeah, it's not entirely correctly named either. Changed back.

https://codereview.chromium.org/1487983002/diff/80001/media/base/audio_render...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/1487983002/diff/80001/media/base/audio_render...
media/base/audio_renderer_sink.h:39: // the consumer has skipped, if any.
On 2015/12/08 08:34:51, tommi wrote:
> Since it's the OS that is skipping these frames and to us, it's actually the
> provider of the data, I find it confusing to use the term consumer

Yep, it's confusing. Refers to consumer of the audio data. (See other comment.)

[Henrik Grunell, 2015-12-08 09:38:43]

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

[commit-bot: I haz the power, 2015-12-08 09:39:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1487983002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1487983002/100001

[commit-bot: I haz the power, 2015-12-08 09:59:10]

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

[commit-bot: I haz the power, 2015-12-08 09:59:10]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[tommi (sloooow) - chröme, 2015-12-08 10:08:10]

https://codereview.chromium.org/1487983002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/1487983002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_sync_reader.cc:89: // Increase the
number of skipped frames stored in shared memory. We don't
On 2015/12/08 09:30:34, Henrik Grunell wrote:
> On 2015/12/08 08:34:51, tommi wrote:
> > why do we increase the number of skipped frames?
> > I would have expected just to set how many have been skipped directly and
not
> > accumulate.
> 
> Afaict, there is a possibility that we write more than once before the render
> side has read the data. That would happen if it takes a very long time for it
to
> execute, WaitUntilDataIsReady times out on this side, and this function is
> called again before the other side is done putting the previously requested
data
> in the shared memory. In that case we'd overwrite and lose information about
> those skipped frames.

Do we still overwrite in that case? I thought we would rather drop buffers in
that case until we know for sure it's safe to write.  However, in this case
though, don't we also need to account for the buffers that we (and not the OS)
are skipping?

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_devic...
File media/audio/audio_device_thread.h (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_devic...
media/audio/audio_device_thread.h:78: // |socket| every time
AudioDeviceThread::Process() completes. |input| should
On 2015/12/08 09:30:34, Henrik Grunell wrote:
> On 2015/12/08 08:34:51, tommi wrote:
> > what does |input| refer to?
> 
> Whether this object is used for input audio or not (i.e. output).

It looks like this refers to a variable that can be either true or false.  Where
is that variable?
The TODO also refers to |input|, indicating that it is ugly and "so we do it
this way".   If there's no variable, then it's not clear to me what is ugly or
why we do what in what way :) (since there's no variable to choose between
different things)
This is a thread class, so looks like it should be fine to have it agnostic to
input vs output, right?

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_input...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_input...
media/audio/audio_input_device.cc:145: true);
On 2015/12/08 09:30:34, Henrik Grunell wrote:
> On 2015/12/08 08:34:51, tommi wrote:
> > no change?
> 
> Mandatory "git cl format media"...

git cl format isn't supposed to change code that has not been modified.  Could
it be you changed this, ran git cl format, reverted your change?

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_outpu...
File media/audio/audio_output_controller.h (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_outpu...
media/audio/audio_output_controller.h:87: // frames has been skipped by the
consumer. The source can handle this
On 2015/12/08 09:30:34, Henrik Grunell wrote:
> On 2015/12/08 08:34:51, tommi wrote:
> > Maybe I'm misunderstanding the context but it seems like 'producer' might be
> > more fitting than 'consumer' since the SyncReader is a consumer and whatever
> > higher up it feeds the data too.
> > 'source' also feels backwards to me since it's the source that's providing
> this
> > data and it's up to the recipient of the data, i.e. the actual consumer, to
> > handle it.
> 
> OK. Well, it refers to the actual audio data, which flows towards the OS, who
is
> the end consumer for that data. This function and its parameters go in the
> opposite direction as a requester for the data. I see that the comment can be
> confusing. We should agree on the terminology, or I can clarify this in the
> text.

ah, I was confused about which direction we were going.  what about using
'renderer'?

https://codereview.chromium.org/1487983002/diff/100001/media/base/audio_rende...
File media/base/audio_renderer_mixer_unittest.cc (right):

https://codereview.chromium.org/1487983002/diff/100001/media/base/audio_rende...
media/base/audio_renderer_mixer_unittest.cc:43: std::vector<int>
kTestInputLower(1, 44100);
fyi  (I know this is only a rebase, but since this is probably a related change
and we should catch this in reviews)
we should avoid using complex objects like this at the global level.  Even
though this is a unit test.  Global state that's shared between potential
parallel runs of tests, can lead to problems so we should avoid it.
Additionally, this is code that always runs even though we're not running these
tests.
kTestInputLower should be const int[], the kDefaultDeviceId should be const
char[] ... actually, I don't think we need kDefaultDeviceId or
kDefaultSecurityOrigin, etc.

[o1ka, 2015-12-08 10:57:44]

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

[o1ka, 2015-12-08 10:57:45]

https://codereview.chromium.org/1487983002/diff/100001/media/base/audio_rende...
File media/base/audio_renderer_mixer_unittest.cc (right):

https://codereview.chromium.org/1487983002/diff/100001/media/base/audio_rende...
media/base/audio_renderer_mixer_unittest.cc:43: std::vector<int>
kTestInputLower(1, 44100);
On 2015/12/08 10:08:09, tommi wrote:
> fyi  (I know this is only a rebase, but since this is probably a related
change
> and we should catch this in reviews)
> we should avoid using complex objects like this at the global level.  Even
> though this is a unit test.  Global state that's shared between potential
> parallel runs of tests, can lead to problems so we should avoid it.
> Additionally, this is code that always runs even though we're not running
these
> tests.
> kTestInputLower should be const int[], the kDefaultDeviceId should be const
> char[] ... actually, I don't think we need kDefaultDeviceId or
> kDefaultSecurityOrigin, etc.

This one I mine, I'll fix it
(https://code.google.com/p/chromium/issues/detail?id=567694&thanks=567694&ts=1...)

[Henrik Grunell, 2015-12-08 11:12:32]

Fixed messed up merge.

https://codereview.chromium.org/1487983002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/1487983002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_sync_reader.cc:89: // Increase the
number of skipped frames stored in shared memory. We don't
On 2015/12/08 10:08:09, tommi wrote:
> On 2015/12/08 09:30:34, Henrik Grunell wrote:
> > On 2015/12/08 08:34:51, tommi wrote:
> > > why do we increase the number of skipped frames?
> > > I would have expected just to set how many have been skipped directly and
> not
> > > accumulate.
> > 
> > Afaict, there is a possibility that we write more than once before the
render
> > side has read the data. That would happen if it takes a very long time for
it
> to
> > execute, WaitUntilDataIsReady times out on this side, and this function is
> > called again before the other side is done putting the previously requested
> data
> > in the shared memory. In that case we'd overwrite and lose information about
> > those skipped frames.
> 
> Do we still overwrite in that case? I thought we would rather drop buffers in
> that case until we know for sure it's safe to write.  However, in this case
> though, don't we also need to account for the buffers that we (and not the OS)
> are skipping?

If Wait...() times out, we'll give the OS an empty buffer instead. So we're not
really skipping any data, since we still requested that buffer. (Skipping means
we did not and will not ask for the data.) On the render side it will just write
data to the buffer, it doesn't know about what has been read or not.

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_devic...
File media/audio/audio_device_thread.h (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_devic...
media/audio/audio_device_thread.h:78: // |socket| every time
AudioDeviceThread::Process() completes. |input| should
On 2015/12/08 10:08:09, tommi wrote:
> On 2015/12/08 09:30:34, Henrik Grunell wrote:
> > On 2015/12/08 08:34:51, tommi wrote:
> > > what does |input| refer to?
> > 
> > Whether this object is used for input audio or not (i.e. output).
> 
> It looks like this refers to a variable that can be either true or false. 
Where
> is that variable?
> The TODO also refers to |input|, indicating that it is ugly and "so we do it
> this way".   If there's no variable, then it's not clear to me what is ugly or
> why we do what in what way :) (since there's no variable to choose between
> different things)
> This is a thread class, so looks like it should be fine to have it agnostic to
> input vs output, right?

Merge error. removed.

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_input...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_input...
media/audio/audio_input_device.cc:145: true);
On 2015/12/08 10:08:09, tommi wrote:
> On 2015/12/08 09:30:34, Henrik Grunell wrote:
> > On 2015/12/08 08:34:51, tommi wrote:
> > > no change?
> > 
> > Mandatory "git cl format media"...
> 
> git cl format isn't supposed to change code that has not been modified.  Could
> it be you changed this, ran git cl format, reverted your change?

Ah, yes that's what happened. Reverted.

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_outpu...
File media/audio/audio_output_controller.h (right):

https://codereview.chromium.org/1487983002/diff/80001/media/audio/audio_outpu...
media/audio/audio_output_controller.h:87: // frames has been skipped by the
consumer. The source can handle this
On 2015/12/08 10:08:09, tommi wrote:
> On 2015/12/08 09:30:34, Henrik Grunell wrote:
> > On 2015/12/08 08:34:51, tommi wrote:
> > > Maybe I'm misunderstanding the context but it seems like 'producer' might
be
> > > more fitting than 'consumer' since the SyncReader is a consumer and
whatever
> > > higher up it feeds the data too.
> > > 'source' also feels backwards to me since it's the source that's providing
> > this
> > > data and it's up to the recipient of the data, i.e. the actual consumer,
to
> > > handle it.
> > 
> > OK. Well, it refers to the actual audio data, which flows towards the OS,
who
> is
> > the end consumer for that data. This function and its parameters go in the
> > opposite direction as a requester for the data. I see that the comment can
be
> > confusing. We should agree on the terminology, or I can clarify this in the
> > text.
> 
> ah, I was confused about which direction we were going.  what about using
> 'renderer'?

Sounds good, but I also added "(typically the OS)" to not be confused with
render process...

https://codereview.chromium.org/1487983002/diff/100001/media/audio/audio_devi...
File media/audio/audio_device_thread.h (right):

https://codereview.chromium.org/1487983002/diff/100001/media/audio/audio_devi...
media/audio/audio_device_thread.h:83: // meanwhile instead of using separate
classes.
Ah, I see, I seem to have messed up resolving the merge conflicts. Removed this
comment.

https://codereview.chromium.org/1487983002/diff/100001/media/base/audio_rende...
File media/base/audio_renderer_mixer_unittest.cc (right):

https://codereview.chromium.org/1487983002/diff/100001/media/base/audio_rende...
media/base/audio_renderer_mixer_unittest.cc:43: std::vector<int>
kTestInputLower(1, 44100);
On 2015/12/08 10:57:45, o1ka wrote:
> On 2015/12/08 10:08:09, tommi wrote:
> > fyi  (I know this is only a rebase, but since this is probably a related
> change
> > and we should catch this in reviews)
> > we should avoid using complex objects like this at the global level.  Even
> > though this is a unit test.  Global state that's shared between potential
> > parallel runs of tests, can lead to problems so we should avoid it.
> > Additionally, this is code that always runs even though we're not running
> these
> > tests.
> > kTestInputLower should be const int[], the kDefaultDeviceId should be const
> > char[] ... actually, I don't think we need kDefaultDeviceId or
> > kDefaultSecurityOrigin, etc.
> 
> This one I mine, I'll fix it
>
(https://code.google.com/p/chromium/issues/detail?id=567694&thanks=567694&ts=1...)

Thanks for pointing out this should be avoided in tests too.

[tommi (sloooow) - chröme, 2015-12-08 11:43:02]

https://codereview.chromium.org/1487983002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/1487983002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_sync_reader.cc:89: // Increase the
number of skipped frames stored in shared memory. We don't
On 2015/12/08 11:12:32, Henrik Grunell wrote:
> On 2015/12/08 10:08:09, tommi wrote:
> > On 2015/12/08 09:30:34, Henrik Grunell wrote:
> > > On 2015/12/08 08:34:51, tommi wrote:
> > > > why do we increase the number of skipped frames?
> > > > I would have expected just to set how many have been skipped directly
and
> > not
> > > > accumulate.
> > > 
> > > Afaict, there is a possibility that we write more than once before the
> render
> > > side has read the data. That would happen if it takes a very long time for
> it
> > to
> > > execute, WaitUntilDataIsReady times out on this side, and this function is
> > > called again before the other side is done putting the previously
requested
> > data
> > > in the shared memory. In that case we'd overwrite and lose information
about
> > > those skipped frames.
> > 
> > Do we still overwrite in that case? I thought we would rather drop buffers
in
> > that case until we know for sure it's safe to write.  However, in this case
> > though, don't we also need to account for the buffers that we (and not the
OS)
> > are skipping?
> 
> If Wait...() times out, we'll give the OS an empty buffer instead. So we're
not
> really skipping any data, since we still requested that buffer. (Skipping
means
> we did not and will not ask for the data.) On the render side it will just
write
> data to the buffer, it doesn't know about what has been read or not.

I think I'm out of brain power... can you help me get back on track? Here's what
I got so far:

* We are only aware of frames_skipped for input (audio capture)
* This CL is about getting that information "upwards", i.e. from the OS layer
and to the upper layers that get audio from the microphone/capture device.

Yet, somehow we're now talking about giving the OS an empty buffer for _output_.
 Can you help me connect the dots?

[tommi (sloooow) - chröme, 2015-12-08 12:27:39]

https://codereview.chromium.org/1487983002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/1487983002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_sync_reader.cc:89: // Increase the
number of skipped frames stored in shared memory. We don't
On 2015/12/08 11:43:02, tommi wrote:
> On 2015/12/08 11:12:32, Henrik Grunell wrote:
> > On 2015/12/08 10:08:09, tommi wrote:
> > > On 2015/12/08 09:30:34, Henrik Grunell wrote:
> > > > On 2015/12/08 08:34:51, tommi wrote:
> > > > > why do we increase the number of skipped frames?
> > > > > I would have expected just to set how many have been skipped directly
> and
> > > not
> > > > > accumulate.
> > > > 
> > > > Afaict, there is a possibility that we write more than once before the
> > render
> > > > side has read the data. That would happen if it takes a very long time
for
> > it
> > > to
> > > > execute, WaitUntilDataIsReady times out on this side, and this function
is
> > > > called again before the other side is done putting the previously
> requested
> > > data
> > > > in the shared memory. In that case we'd overwrite and lose information
> about
> > > > those skipped frames.
> > > 
> > > Do we still overwrite in that case? I thought we would rather drop buffers
> in
> > > that case until we know for sure it's safe to write.  However, in this
case
> > > though, don't we also need to account for the buffers that we (and not the
> OS)
> > > are skipping?
> > 
> > If Wait...() times out, we'll give the OS an empty buffer instead. So we're
> not
> > really skipping any data, since we still requested that buffer. (Skipping
> means
> > we did not and will not ask for the data.) On the render side it will just
> write
> > data to the buffer, it doesn't know about what has been read or not.
> 
> I think I'm out of brain power... can you help me get back on track? Here's
what
> I got so far:
> 
> * We are only aware of frames_skipped for input (audio capture)
> * This CL is about getting that information "upwards", i.e. from the OS layer
> and to the upper layers that get audio from the microphone/capture device.
> 
> Yet, somehow we're now talking about giving the OS an empty buffer for
_output_.
>  Can you help me connect the dots?

Got some brain power back :)  I realize now that I was stuck thinking about
input, but it's actually output where we are aware of the skipped frames.  The
comments make sense now that I finally managed to wrap my head around this (yes,
I know I did this myself initially!)

[tommi (sloooow) - chröme, 2015-12-08 12:27:56]

so... lgtm :)

[Henrik Grunell, 2015-12-08 12:56:10]

https://codereview.chromium.org/1487983002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/1487983002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/audio_sync_reader.cc:89: // Increase the
number of skipped frames stored in shared memory. We don't
On 2015/12/08 12:27:39, tommi wrote:
> On 2015/12/08 11:43:02, tommi wrote:
> > On 2015/12/08 11:12:32, Henrik Grunell wrote:
> > > On 2015/12/08 10:08:09, tommi wrote:
> > > > On 2015/12/08 09:30:34, Henrik Grunell wrote:
> > > > > On 2015/12/08 08:34:51, tommi wrote:
> > > > > > why do we increase the number of skipped frames?
> > > > > > I would have expected just to set how many have been skipped
directly
> > and
> > > > not
> > > > > > accumulate.
> > > > > 
> > > > > Afaict, there is a possibility that we write more than once before the
> > > render
> > > > > side has read the data. That would happen if it takes a very long time
> for
> > > it
> > > > to
> > > > > execute, WaitUntilDataIsReady times out on this side, and this
function
> is
> > > > > called again before the other side is done putting the previously
> > requested
> > > > data
> > > > > in the shared memory. In that case we'd overwrite and lose information
> > about
> > > > > those skipped frames.
> > > > 
> > > > Do we still overwrite in that case? I thought we would rather drop
buffers
> > in
> > > > that case until we know for sure it's safe to write.  However, in this
> case
> > > > though, don't we also need to account for the buffers that we (and not
the
> > OS)
> > > > are skipping?
> > > 
> > > If Wait...() times out, we'll give the OS an empty buffer instead. So
we're
> > not
> > > really skipping any data, since we still requested that buffer. (Skipping
> > means
> > > we did not and will not ask for the data.) On the render side it will just
> > write
> > > data to the buffer, it doesn't know about what has been read or not.
> > 
> > I think I'm out of brain power... can you help me get back on track? Here's
> what
> > I got so far:
> > 
> > * We are only aware of frames_skipped for input (audio capture)
> > * This CL is about getting that information "upwards", i.e. from the OS
layer
> > and to the upper layers that get audio from the microphone/capture device.
> > 
> > Yet, somehow we're now talking about giving the OS an empty buffer for
> _output_.
> >  Can you help me connect the dots?
> 
> Got some brain power back :)  I realize now that I was stuck thinking about
> input, but it's actually output where we are aware of the skipped frames.  The
> comments make sense now that I finally managed to wrap my head around this
(yes,
> I know I did this myself initially!)

:) I'm glad you could.

[Henrik Grunell, 2015-12-08 12:56:19]

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

[commit-bot: I haz the power, 2015-12-08 12:56:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1487983002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1487983002/120001

[commit-bot: I haz the power, 2015-12-08 13:07:53]

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

[commit-bot: I haz the power, 2015-12-08 13:07:55]

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

[Henrik Grunell, 2015-12-09 08:15:34]

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

[commit-bot: I haz the power, 2015-12-09 08:16:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1487983002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1487983002/140001

[commit-bot: I haz the power, 2015-12-09 09:01:47]

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

[commit-bot: I haz the power, 2015-12-09 09:01:47]

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

[tommi (sloooow) - chröme, 2015-12-09 10:48:28]

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

https://codereview.chromium.org/1487983002/diff/140001/media/audio/win/audio_...
media/audio/win/audio_output_win_unittest.cc:481: EXPECT_CALL(source,
OnMoreData(NotNull(), 0, 0)).WillOnce(Invoke(ClearData));
There's a compilation error for this line.

"'int (__cdecl *)(media::AudioBus *,uint32)' : too many arguments for call"

MockAudioSourceCallback's OnData method, only has two arguments.

[Henrik Grunell, 2015-12-09 10:53:10]

https://codereview.chromium.org/1487983002/diff/140001/media/audio/mock_audio...
File media/audio/mock_audio_source_callback.h (right):

https://codereview.chromium.org/1487983002/diff/140001/media/audio/mock_audio...
media/audio/mock_audio_source_callback.h:21: uint32_t frames_skipped));
This has three arguments.

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

https://codereview.chromium.org/1487983002/diff/140001/media/audio/win/audio_...
media/audio/win/audio_output_win_unittest.cc:481: EXPECT_CALL(source,
OnMoreData(NotNull(), 0, 0)).WillOnce(Invoke(ClearData));
On 2015/12/09 10:48:28, tommi wrote:
> There's a compilation error for this line.
> 
> "'int (__cdecl *)(media::AudioBus *,uint32)' : too many arguments for call"
> 
> MockAudioSourceCallback's OnData method, only has two arguments.

But it has three arguments.

[Henrik Grunell, 2015-12-10 17:35:20]

On 2015/12/09 10:53:10, Henrik Grunell wrote:
>
https://codereview.chromium.org/1487983002/diff/140001/media/audio/mock_audio...
> File media/audio/mock_audio_source_callback.h (right):
> 
>
https://codereview.chromium.org/1487983002/diff/140001/media/audio/mock_audio...
> media/audio/mock_audio_source_callback.h:21: uint32_t frames_skipped));
> This has three arguments.
> 
>
https://codereview.chromium.org/1487983002/diff/140001/media/audio/win/audio_...
> File media/audio/win/audio_output_win_unittest.cc (right):
> 
>
https://codereview.chromium.org/1487983002/diff/140001/media/audio/win/audio_...
> media/audio/win/audio_output_win_unittest.cc:481: EXPECT_CALL(source,
> OnMoreData(NotNull(), 0, 0)).WillOnce(Invoke(ClearData));
> On 2015/12/09 10:48:28, tommi wrote:
> > There's a compilation error for this line.
> > 
> > "'int (__cdecl *)(media::AudioBus *,uint32)' : too many arguments for call"
> > 
> > MockAudioSourceCallback's OnData method, only has two arguments.
> 
> But it has three arguments.

But another one has only two. ClearData. Silly. Fixed now.

rkc@: please review your part (trivial).

lcwu@: please review your part (trivial).

[Henrik Grunell, 2015-12-10 17:35:30]

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

[commit-bot: I haz the power, 2015-12-10 17:37:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1487983002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1487983002/180001

[commit-bot: I haz the power, 2015-12-10 18:59:14]

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

[commit-bot: I haz the power, 2015-12-10 18:59:15]

Dry run: This issue passed the CQ dry run.

[Henrik Grunell, 2015-12-11 10:10:21]

grunell@chromium.org changed reviewers:
+ ckehoe@chromium.org, gunsch@chromium.org
- lcwu@chromium.org, rkc@chromium.org

[Henrik Grunell, 2015-12-11 10:11:57]

Changing reviewers for //chromecast and //components/audio_modem.

gunsch@: //chromecast
ckehoe@: //components/audio_modem

[gunsch, 2015-12-11 16:53:11]

lgtm, +cc kmackay as FYI

[Charlie, 2015-12-11 21:16:19]

audio_modem changes lgtm.

[Henrik Grunell, 2015-12-14 08:02:41]

The CQ bit was checked by grunell@chromium.org

[Henrik Grunell, 2015-12-14 08:02:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/1487983002/#ps180001
(title: "Fix in Windows unit test.")

[commit-bot: I haz the power, 2015-12-14 08:03:07]

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

[commit-bot: I haz the power, 2015-12-14 09:52:19]

Description was changed from

==========
Forward the number of skipped frames by the OS in audio playout.

* Only for Mac.

Will be done in follow-up CLs:
* Windows.
* Hooking this up to the AEC.

BUG=560371
==========

to

==========
Forward the number of skipped frames by the OS in audio playout.

* Only for Mac.

Will be done in follow-up CLs:
* Windows.
* Hooking this up to the AEC.

BUG=560371
==========

[commit-bot: I haz the power, 2015-12-14 09:52:20]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-12-14 09:53:25]

Description was changed from

==========
Forward the number of skipped frames by the OS in audio playout.

* Only for Mac.

Will be done in follow-up CLs:
* Windows.
* Hooking this up to the AEC.

BUG=560371
==========

to

==========
Forward the number of skipped frames by the OS in audio playout.

* Only for Mac.

Will be done in follow-up CLs:
* Windows.
* Hooking this up to the AEC.

BUG=560371

Committed: https://crrev.com/db03a8d1128783ff4803364ba616ae5fac8822f7
Cr-Commit-Position: refs/heads/master@{#364990}
==========

[commit-bot: I haz the power, 2015-12-14 09:53:26]

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

[David Tseng, 2015-12-16 23:12:43]

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

[David Tseng, 2015-12-16 23:12:44]

It looks like this cl is part of some larger chunk of work.

As a heads-up, after this change, text to speech now contains artifacts
(crackling mostly) making it virtually unusable. Most easily seen on Chrome OS
with ChromeVox or web speech synthesis i.e. in dev tools
speechSynthesis.speak(new SpeechSynthesisUtterance('hello'));


From the description itself, I can't tell what "only on Mac" means.

[Henrik Grunell, 2015-12-17 08:13:21]

On 2015/12/16 23:12:44, David Tseng wrote:
> It looks like this cl is part of some larger chunk of work.
> 
> As a heads-up, after this change, text to speech now contains artifacts
> (crackling mostly) making it virtually unusable. Most easily seen on Chrome OS
> with ChromeVox or web speech synthesis i.e. in dev tools
> speechSynthesis.speak(new SpeechSynthesisUtterance('hello'));
> 
> 
> From the description itself, I can't tell what "only on Mac" means.

Yes, there's issues with this CL. It will be reverted shortly.

[Henrik Grunell, 2015-12-17 08:20:45]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/1538463002/ by grunell@chromium.org.

The reason for reverting is: Causes crackling sound on all ChromeOS boards..

[Henrik Grunell, 2016-04-08 08:25:43]

Description was changed from

==========
Forward the number of skipped frames by the OS in audio playout.

* Only for Mac.

Will be done in follow-up CLs:
* Windows.
* Hooking this up to the AEC.

BUG=560371

Committed: https://crrev.com/db03a8d1128783ff4803364ba616ae5fac8822f7
Cr-Commit-Position: refs/heads/master@{#364990}
==========

to

==========
Forward the number of skipped frames by the OS in audio playout.

* Only for Mac.

Will be done in follow-up CLs:
* Windows.
* Hooking this up to the AEC.

BUG=560371

Committed: https://crrev.com/db03a8d1128783ff4803364ba616ae5fac8822f7
Cr-Commit-Position: refs/heads/master@{#364990}
==========