Switch audio synchronization from sleep() based to select() based.

Switch audio synchronization from sleep() based to select() based.

Uses select() to efficiently wait for data from the renderer with a
timeout instead of using sleep() or always assuming the renderer
is ready.

Fixes glitching and / or CPU hogging when the renderer isn't ready.

BUG=179058, 180841, 260772, 269672
, 
TEST=Swap from 44kHz to 16kHz html5 playback, ensure glitch free.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230515

[DaleCurtis, 2013-08-16 22:15:55]

Guys, I'm not sure if we actually want to do this, but I suspect without this or
something similar we're always going to have a long tail of users with glitching
on OSX and Android.  We've already established we need some sort of
synchronization mechanism on Linux and Windows to prevent glitching.

Also, until we have renderer side device changes ( http://crbug.com/157216 ),
all low buffer size clients (WebRTC, WebAudio, HTML5) will see glitching on OSX
w/o sleeping when a device change occurs that requires back to back callbacks
for fulfillment.

This patch is my attempt at making the current sleep() based approach less hacky
and more efficient.  As a bonus, with this approach we can also remove the
shared memory magic.

crogers previously mentioned concerns that the socket round trip for this
approach is too high and will impede OSX performance.  In testing I found that,
at least with 10.8, the OS level traces remain stable even with select() delay. 
This would need testing on 10.6+ to ensure it does not bring down the system
like the old sleep() based approach did on OSX.

What I haven't implemented yet is the SyncSocket::Recieve() w/ timeout for NaCl
and Windows.  NaCl should be similar to the POSIX variant, and the Windows
version already uses an internal timeout based wrapper to support cancellation.

Thoughts?  Rants?  Insults?

[DaleCurtis, 2013-08-16 22:16:56]

Plus +dgreid, +davemoore too, since the Yield() / Sleep() approach recently came
under fire on ChromeOS.

[miu, 2013-08-17 02:39:13]

Dale,

I like where this is going: This may fix a "static in music" bug we've been
trying to diagnose for tab capture.  Using less CPU can only help us a lot!  :)

Drive-by initial comment below...

https://codereview.chromium.org/22886005/diff/1/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/22886005/diff/1/base/sync_socket_posix.cc#new...
base/sync_socket_posix.cc:120: struct timeval timeout_struct = { 0,
timeout.InMicroseconds() };
Since you're not populating the "seconds" part, consider adding:

  DCHECK_LT(timeout, base::TimeDelta::FromSeconds(1));

[henrika (OOO until Aug 14), 2013-08-19 07:18:40]

Dale,

your patch looks really nice but it is not clear to me how you have
verified that your patch improves performance. Have you found examples
where you hear glitches today where these glitches are now gone? Your test
says "TEST=Swap from 44kHz to 16kHz html5 playback, ensure glitch free." Do
you then modify the output hardware sample rate by manually switching
sample rate using the audio control panel? Has it been tested for all
platforms?


On Sat, Aug 17, 2013 at 4:39 AM, <miu@chromium.org> wrote:

> Dale,
>
> I like where this is going: This may fix a "static in music" bug we've been
> trying to diagnose for tab capture.  Using less CPU can only help us a
> lot!  :)
>
> Drive-by initial comment below...
>
>
>
> https://codereview.chromium.**org/22886005/diff/1/base/sync_**
>
socket_posix.cc<https://codereview.chromium.org/22886005/diff/1/base/sync_socket_posix.cc>
> File base/sync_socket_posix.cc (right):
>
> https://codereview.chromium.**org/22886005/diff/1/base/sync_**
>
socket_posix.cc#newcode120<https://codereview.chromium.org/22886005/diff/1/base/sync_socket_posix.cc#newcode120>
> base/sync_socket_posix.cc:120: struct timeval timeout_struct = { 0,
> timeout.InMicroseconds() };
> Since you're not populating the "seconds" part, consider adding:
>
>   DCHECK_LT(timeout, base::TimeDelta::FromSeconds(**1));
>
>
https://codereview.chromium.**org/22886005/<https://codereview.chromium.org/2...
>

[DaleCurtis, 2013-08-19 16:22:03]

Henrik, the most noticeable playback improvement is on OSX since we have no
blocking solution there currently.  Playing any audio at say 44kHz and then
using Audio MIDI Setup to switch your device to 16kHz results in glitching.

Because of the sample rate change an AudioOutputResampler is inserted on the
browser side; which ends up requiring multiple back-to-back callbacks to
fulfill.  On Windows and Linux we block to support these callbacks: Sleep(0ms)
on Windows, Sleep(2ms) on Linux/ChromeOS up to 20ms.

If this approach looks sound I'll try to get some CPU performance numbers from
ARM Chromebooks and older XP systems.

[DaleCurtis, 2013-08-19 16:23:27]

To be clearer: I've only tested this on OSX and Linux so far, I haven't built a
CrOS build or tried to write the Windows version yet.

[DaveMoore, 2013-08-19 22:49:43]

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

https://codereview.chromium.org/22886005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_sync_reader.cc:189:
maximum_wait_time_);
Does it mean anything (skipping) if something other than the right buffer_index
is read before we finally get the right one? Should there be an error or
histogram about it?

[DaleCurtis, 2013-08-19 23:08:22]

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

https://codereview.chromium.org/22886005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_sync_reader.cc:189:
maximum_wait_time_);
On 2013/08/19 22:49:43, DaveMoore wrote:
> Does it mean anything (skipping) if something other than the right
buffer_index
> is read before we finally get the right one? Should there be an error or
> histogram about it?

It means we timed out on one or more previous calls.  I'd expect this to loop no
more than once in most cases.  Timeouts are now recorded via
renderer_missed_callback_count_ since this change implies we're living in a
world where having to wait is unavoidable.  That variable maps to the
Media.AudioRendererMissedDeadline percentage in UMA.

Previously the miss count was incremented prior to data waiting; which meant the
miss may have been recoverable w/ some waiting.

[henrika (OOO until Aug 14), 2013-08-20 07:50:30]

https://codereview.chromium.org/22886005/diff/1/base/sync_socket.h
File base/sync_socket.h (right):

https://codereview.chromium.org/22886005/diff/1/base/sync_socket.h#newcode60
base/sync_socket.h:60: virtual size_t Receive(void* buffer, size_t length,
base::TimeDelta timeout);
Extend comment for new parameter.

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

https://codereview.chromium.org/22886005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_sync_reader.cc:31:
maximum_wait_time_(params.GetBufferDuration() / 2),
Just checking; what is the unit of GetBufferDuration()?

https://codereview.chromium.org/22886005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_sync_reader.cc:199: // Receive() timed
out or another unknown error occurred.
Could you add some more details about what can cause Receive to time out. Given
that is such an essential part of this method.

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

https://codereview.chromium.org/22886005/diff/1/media/audio/audio_device_thre...
media/audio/audio_device_thread.cc:176: buffer_index++;
Perhaps I am missing something here but isn't new code required to deal with the
new Send call on the render side?

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

https://codereview.chromium.org/22886005/diff/1/media/audio/audio_parameters....
media/audio/audio_parameters.h:72: base::TimeDelta GetBufferDuration() const;
OK, I see that you have added this method. Please add comment about the fact
that the unit is in microseconds.

[tommi (sloooow) - chröme, 2013-08-20 10:55:56]

Great that you're tackling this.
I did some work a while back in this direction but never actually got round
incorporating it in the code since we started looking into an alternate
mechanism of delivering the audio packets (ipc) but then that approach got
abandoned as well.  See other comments for details.

https://codereview.chromium.org/22886005/diff/1/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/22886005/diff/1/base/sync_socket_posix.cc#new...
base/sync_socket_posix.cc:113: // TODO(dalecurtis): Build fd_set during
construction?
would this make a measurable difference?

https://codereview.chromium.org/22886005/diff/1/base/sync_socket_posix.cc#new...
base/sync_socket_posix.cc:128: if (file_util::ReadFromFD(handle_, charbuffer,
length))
don't you first have to check if handle_ is in rfds after the select operation?

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

https://codereview.chromium.org/22886005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_sync_reader.cc:74: buffer_index_++;
nit: ++buffer_index_;

https://codereview.chromium.org/22886005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_sync_reader.cc:179: if (!socket_)
is this something exected or unexpected (i.e. [D]CHECK)?

https://codereview.chromium.org/22886005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_sync_reader.cc:187: bytes_received =
socket_->Receive(
Since we're using sockets, we should be able to check multiple in one go.
I did some work a long time ago that could possibly be used in this case.  Mind
taking a look?

https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/cross_...
There are tests etc there as well.

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

https://codereview.chromium.org/22886005/diff/1/media/audio/audio_device_thre...
media/audio/audio_device_thread.cc:176: buffer_index++;
nit: ++buffer_index;

[DaleCurtis, 2013-08-20 18:27:50]

https://codereview.chromium.org/22886005/diff/1/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/22886005/diff/1/base/sync_socket_posix.cc#new...
base/sync_socket_posix.cc:128: if (file_util::ReadFromFD(handle_, charbuffer,
length))
On 2013/08/20 10:55:57, tommi wrote:
> don't you first have to check if handle_ is in rfds after the select
operation?

Not if we only have one descriptor since select() returns the number of matched
descriptors.  I'll add a DCHECK() for it though.

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

https://codereview.chromium.org/22886005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_sync_reader.cc:187: bytes_received =
socket_->Receive(
On 2013/08/20 10:55:57, tommi wrote:
> Since we're using sockets, we should be able to check multiple in one go.
> I did some work a long time ago that could possibly be used in this case. 
Mind
> taking a look?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/cross_...
> There are tests etc there as well.

Not sure I follow which "multiple" items you want to check here?  We only have
one socket.  Can you elaborate on how and what waiting on multiple descriptors
would help with?

Yeah your code was my inspiration here; I wanted something that didn't need to
construct additional sockets though, so I came up with this back and forth model
w/ an index.

[tommi (sloooow) - chröme, 2013-08-21 12:33:25]

Is there another patchset on the way?

https://codereview.chromium.org/22886005/diff/1/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/22886005/diff/1/base/sync_socket_posix.cc#new...
base/sync_socket_posix.cc:128: if (file_util::ReadFromFD(handle_, charbuffer,
length))
On 2013/08/20 18:27:51, DaleCurtis wrote:
> On 2013/08/20 10:55:57, tommi wrote:
> > don't you first have to check if handle_ is in rfds after the select
> operation?
> 
> Not if we only have one descriptor since select() returns the number of
matched
> descriptors.  I'll add a DCHECK() for it though.

sgtm

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

https://codereview.chromium.org/22886005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_sync_reader.cc:187: bytes_received =
socket_->Receive(
On 2013/08/20 18:27:51, DaleCurtis wrote:
> On 2013/08/20 10:55:57, tommi wrote:
> > Since we're using sockets, we should be able to check multiple in one go.
> > I did some work a long time ago that could possibly be used in this case. 
> Mind
> > taking a look?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/cross_...
> > There are tests etc there as well.
> 
> Not sure I follow which "multiple" items you want to check here?  We only have
> one socket.  Can you elaborate on how and what waiting on multiple descriptors
> would help with?
> 
> Yeah your code was my inspiration here; I wanted something that didn't need to
> construct additional sockets though, so I came up with this back and forth
model
> w/ an index.

Sorry, for some reason I parsed this loop as cycling through sockets.
Great that you're aware of it and even better that you found it useful! ;-)

[DaleCurtis, 2013-08-21 17:36:09]

I was using the current patch set as a vehicle for discussion, so I haven't made
any updates to it.

I haven't heard any dissent yet, so barring any objections I'll clean this up
and deliver the Windows / NaCl implementations in the next few days.

Does anyone have any nagging doubts before I dive in?

[tommi (sloooow) - chröme, 2013-08-21 17:59:30]

On 2013/08/21 17:36:09, DaleCurtis wrote:
> I was using the current patch set as a vehicle for discussion, so I haven't
made
> any updates to it.
> 
> I haven't heard any dissent yet, so barring any objections I'll clean this up
> and deliver the Windows / NaCl implementations in the next few days.
> 
> Does anyone have any nagging doubts before I dive in?

no doubts here fwiw.

[henrika (OOO until Aug 14), 2013-08-21 20:17:45]

Go for it.

[miu, 2013-08-22 01:30:47]

On 2013/08/21 20:17:45, henrika wrote:
> Go for it.

+1

[scherkus (not reviewing), 2013-08-23 19:27:46]

On 2013/08/22 01:30:47, Yuri wrote:
> On 2013/08/21 20:17:45, henrika wrote:
> > Go for it.
> 
> +1

Would this be a good candidate for a server-side controlled experiment? We can
control who gets opted in w/o subjecting ourselves to code merges and affecting
the whole mac population.

[DaleCurtis, 2013-08-26 21:14:10]

In the event of failure, I suspect this will be sufficiently catastrophic to
roll back immediately from canary.  The last time sleep() was introduced on OSX
I don't think it made it past dev channel due to system hangs.

As such, I don't think a field trial would get us any more information than the
canary -> dev -> beta populations and might actually make it harder to figure
out if this is going awry -- due to a smaller population.

Additionally, with the existing UMA statistics we'll be able to note population
drop off and wait time improvements pretty quickly.

[kaustubh.j, 2013-08-30 07:34:54]

Have you verified this patch with multi-tab scenario ?; 
also could you please share performance details if you have.

[DaleCurtis, 2013-09-11 01:16:03]

xians, davemoore, scherkus to cc -- the rest, please review.  I've split out the
base/ changes into a separate CL.  https://codereview.chromium.org/23875019/

Those still on the review line, I'll probably also be asking for additional
testing assistance from your respective platforms (android, windows, cros,
mirroring :) prior to landing this.

PTAL!

https://codereview.chromium.org/22886005/diff/1/base/sync_socket.h
File base/sync_socket.h (right):

https://codereview.chromium.org/22886005/diff/1/base/sync_socket.h#newcode60
base/sync_socket.h:60: virtual size_t Receive(void* buffer, size_t length,
base::TimeDelta timeout);
On 2013/08/20 07:50:30, henrika wrote:
> Extend comment for new parameter.

Done.

https://codereview.chromium.org/22886005/diff/1/base/sync_socket_posix.cc
File base/sync_socket_posix.cc (right):

https://codereview.chromium.org/22886005/diff/1/base/sync_socket_posix.cc#new...
base/sync_socket_posix.cc:113: // TODO(dalecurtis): Build fd_set during
construction?
On 2013/08/20 10:55:57, tommi wrote:
> would this make a measurable difference?

Probably not. Comment removed.

https://codereview.chromium.org/22886005/diff/1/base/sync_socket_posix.cc#new...
base/sync_socket_posix.cc:120: struct timeval timeout_struct = { 0,
timeout.InMicroseconds() };
On 2013/08/17 02:39:13, Yuri wrote:
> Since you're not populating the "seconds" part, consider adding:
> 
>   DCHECK_LT(timeout, base::TimeDelta::FromSeconds(1));

Done.

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

https://codereview.chromium.org/22886005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_sync_reader.cc:31:
maximum_wait_time_(params.GetBufferDuration() / 2),
On 2013/08/20 07:50:30, henrika wrote:
> Just checking; what is the unit of GetBufferDuration()?

base::TimeDelta

https://codereview.chromium.org/22886005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_sync_reader.cc:74: buffer_index_++;
On 2013/08/20 10:55:57, tommi wrote:
> nit: ++buffer_index_;

Done.

https://codereview.chromium.org/22886005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_sync_reader.cc:179: if (!socket_)
On 2013/08/20 10:55:57, tommi wrote:
> is this something exected or unexpected (i.e. [D]CHECK)?

Set during Init() and never changed.  Checks removed.

https://codereview.chromium.org/22886005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_sync_reader.cc:199: // Receive() timed
out or another unknown error occurred.
On 2013/08/20 07:50:30, henrika wrote:
> Could you add some more details about what can cause Receive to time out.
Given
> that is such an essential part of this method.

Done.

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

https://codereview.chromium.org/22886005/diff/1/media/audio/audio_device_thre...
media/audio/audio_device_thread.cc:176: buffer_index++;
On 2013/08/20 07:50:30, henrika wrote:
> Perhaps I am missing something here but isn't new code required to deal with
the
> new Send call on the render side?

This is handled in the new WaitTilDataReady().

https://codereview.chromium.org/22886005/diff/1/media/audio/audio_device_thre...
media/audio/audio_device_thread.cc:176: buffer_index++;
On 2013/08/20 10:55:57, tommi wrote:
> nit: ++buffer_index;

Done.

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

https://codereview.chromium.org/22886005/diff/1/media/audio/audio_parameters....
media/audio/audio_parameters.h:72: base::TimeDelta GetBufferDuration() const;
On 2013/08/20 07:50:30, henrika wrote:
> OK, I see that you have added this method. Please add comment about the fact
> that the unit is in microseconds.

The resolution being limited to microseconds is a detail of TimeDelta, so I
don't think an extra comment is necessary.

https://codereview.chromium.org/22886005/diff/37001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/22886005/diff/37001/content/browser/renderer_...
content/browser/renderer_host/media/audio_sync_reader.cc:31:
maximum_wait_time_(params.GetBufferDuration() / 2),
We may want to fix this at 20ms to keep Windows, Linux, and ChromeOS consistent
with the existing value.

[henrika (OOO until Aug 14), 2013-09-13 10:25:44]

Thanks for improving and cleaning up in this rather complex area of the code
Dale. It is difficult to come with input like: "why not do like this instead"
since I know that you are doing changes that are crucial to our performance and
I trust your knowledge here.

Let me know if you need help to test on Android or Windows.

https://codereview.chromium.org/22886005/diff/37001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/22886005/diff/37001/content/browser/renderer_...
content/browser/renderer_host/media/audio_sync_reader.cc:63: // Zero out the
entire output buffer to avoid stuttering/repeating-buffers
Not sure about the details here but what happened with the pause mark. Glad to
see that it is not needed though.

https://codereview.chromium.org/22886005/diff/37001/content/browser/renderer_...
content/browser/renderer_host/media/audio_sync_reader.cc:81: // TODO(crogers):
In some cases with device and sample-rate changes
change to rtoy?

https://codereview.chromium.org/22886005/diff/37001/content/browser/renderer_...
content/browser/renderer_host/media/audio_sync_reader.cc:144: } while
(bytes_received > 0 && renderer_buffer_index != buffer_index_);
Can you comment on what this condition means? We are waiting for bytes to be
received and .....

https://codereview.chromium.org/22886005/diff/37001/content/browser/renderer_...
content/browser/renderer_host/media/audio_sync_reader.cc:148: if
(!bytes_received) {
what if renderer_buffer_index != buffer_index_? Any action then?

https://codereview.chromium.org/22886005/diff/37001/media/audio/audio_device_...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/22886005/diff/37001/media/audio/audio_device_...
media/audio/audio_device_thread.cc:181: // Let the other end know which buffer
we just filled.
Can you add a line about how the renderer and browser "communicates" using this
buffer index.

https://codereview.chromium.org/22886005/diff/37001/media/audio/audio_input_d...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/22886005/diff/37001/media/audio/audio_input_d...
media/audio/audio_input_device.cc:144: audio_callback_.get(), socket_handle,
"AudioInputDevice", false);
Do we want a TODO here to point out that we do things differently for input and
output?

[henrika (OOO until Aug 14), 2013-09-30 08:52:06]

I have tried this patch on Android and it does reduce the miss rate for WebRTC
clients. Looks good Dale.

LGTM

[tommi (sloooow) - chröme, 2013-10-12 09:14:09]

Lgtm. Sorry for the delay :-(

[DaleCurtis, 2013-10-22 23:13:48]

+bbudge for ppapi/ changes.

Okay guys, this is all ready for review.  The base/ changes have finally landed.

miu, dgreid: PTAL.

https://codereview.chromium.org/22886005/diff/37001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://codereview.chromium.org/22886005/diff/37001/content/browser/renderer_...
content/browser/renderer_host/media/audio_sync_reader.cc:63: // Zero out the
entire output buffer to avoid stuttering/repeating-buffers
On 2013/09/13 10:25:44, henrika wrote:
> Not sure about the details here but what happened with the pause mark. Glad to
> see that it is not needed though.

kPauseMark is now indicated by a negative pending data value.

https://codereview.chromium.org/22886005/diff/37001/content/browser/renderer_...
content/browser/renderer_host/media/audio_sync_reader.cc:81: // TODO(crogers):
In some cases with device and sample-rate changes
On 2013/09/13 10:25:44, henrika wrote:
> change to rtoy?

Done.

https://codereview.chromium.org/22886005/diff/37001/content/browser/renderer_...
content/browser/renderer_host/media/audio_sync_reader.cc:144: } while
(bytes_received > 0 && renderer_buffer_index != buffer_index_);
On 2013/09/13 10:25:44, henrika wrote:
> Can you comment on what this condition means? We are waiting for bytes to be
> received and .....

Done.

https://codereview.chromium.org/22886005/diff/37001/content/browser/renderer_...
content/browser/renderer_host/media/audio_sync_reader.cc:148: if
(!bytes_received) {
On 2013/09/13 10:25:44, henrika wrote:
> what if renderer_buffer_index != buffer_index_? Any action then?

Done.

https://codereview.chromium.org/22886005/diff/37001/media/audio/audio_device_...
File media/audio/audio_device_thread.cc (right):

https://codereview.chromium.org/22886005/diff/37001/media/audio/audio_device_...
media/audio/audio_device_thread.cc:181: // Let the other end know which buffer
we just filled.
On 2013/09/13 10:25:44, henrika wrote:
> Can you add a line about how the renderer and browser "communicates" using
this
> buffer index.

Done.

https://codereview.chromium.org/22886005/diff/37001/media/audio/audio_input_d...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/22886005/diff/37001/media/audio/audio_input_d...
media/audio/audio_input_device.cc:144: audio_callback_.get(), socket_handle,
"AudioInputDevice", false);
On 2013/09/13 10:25:44, henrika wrote:
> Do we want a TODO here to point out that we do things differently for input
and
> output?

I don't think that's necessary here.  That documentation belongs with the
Start() method.

[miu, 2013-10-23 00:03:39]

lgtm

[bbudge, 2013-10-23 00:31:07]

ppapi LGTM. Did you verify that NaCl audio works? I'm not sure how good our test
coverage is for that.

https://codereview.chromium.org/22886005/diff/103001/ppapi/proxy/audio_input_...
File ppapi/proxy/audio_input_resource.cc (right):

https://codereview.chromium.org/22886005/diff/103001/ppapi/proxy/audio_input_...
ppapi/proxy/audio_input_resource.cc:172: serialized_shared_memory_handle.size(),
socket_handle);
nit: separate line for third param.

[DaleCurtis, 2013-10-23 00:36:05]

Yes, I verified using NaClBox (Duke Nukem II!) which required me to fix this
bug: https://codereview.chromium.org/36103002/ :)

https://codereview.chromium.org/22886005/diff/103001/ppapi/proxy/audio_input_...
File ppapi/proxy/audio_input_resource.cc (right):

https://codereview.chromium.org/22886005/diff/103001/ppapi/proxy/audio_input_...
ppapi/proxy/audio_input_resource.cc:172: serialized_shared_memory_handle.size(),
socket_handle);
On 2013/10/23 00:31:08, bbudge1 wrote:
> nit: separate line for third param.

Done.

[dgreid, 2013-10-23 10:41:10]

code lgtm, haven't tested it and I'm away at a conference for the week. 
+chihchung,hychao FYI

[dgreid, 2013-10-23 13:00:50]

hychao was nice enough to test successfuly on link and spring.

[DaleCurtis, 2013-10-23 17:23:27]

Thanks everyone!

[commit-bot: I haz the power, 2013-10-23 17:25:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/22886005/353001

[commit-bot: I haz the power, 2013-10-23 21:33:48]

Change committed as 230515