Implement renderer AudioDevice API for low-latency audio output...

Implement renderer AudioDevice API for low-latency audio output
BUG=none
TEST=none
(I tested locally for Mac OS X, but we'll need further testing for Windows and Linux as we add real-time threads, etc.)

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

[Chris Rogers, 2010-12-22 22:56:36]

This is an initial implementation of an AudioDevice abstraction which can be
used by code executing in the renderer process.  It is intended to offer
low-latency audio device access.  Parts of this code were taken from
pepper_devices.h/.cc (Thanks neb!).

* ability to run on a high-priority thread can later be added
* currently this API only supports audio output, but the API can be adapted to
offer isochronous input and output in the same callback.

For those who are interested, please also see companion WebKit patch which uses
this API to implement WebKit's audio output:
https://bugs.webkit.org/show_bug.cgi?id=51424

[neb, 2011-01-10 23:11:57]

I can't really help much with WebKit stuff, but I have a few questions. For one,
do you expect the PPAPI implementation to re-use this?

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
File chrome/renderer/audio_device.cc (right):

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:35: bool AudioDevice::Start() {
For PPAPI, there was a requirement that all the audio calls happen on one
thread, so we can get away with not making them thread safe. Checking stream_id_
non-atomically, using scoped_ptrs without mutexes and so on. What threading
model does this support?

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:151: // Interleave, scale, and clip to int16.
In PPAPI, we expect to provide different sample types in the future, such as
floats. Are you sure you want to do this conversion here?

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
File webkit/glue/webaudiodevice_impl.h (right):

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.h:11: #include "chrome/renderer/audio_device.h"
c before t

[scherkus (not reviewing), 2011-01-10 23:42:30]

overall looking good, mostly style nits and sanity checks

awesome!

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
File chrome/renderer/audio_device.cc (right):

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:21: audio_data_.reserve(channels);
(thinking aloud) I wonder if it makes sense to make audio_data_ a class that
takes care of this stuff... probably not

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:24: malloc(buffer_size * sizeof(float)));
malloc is rarely used -- I'd just go for new float[]

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:32: free(audio_data_[i]);
and delete[] here

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:42: if (!filter_.get()) {
sanity check: are we sure there's no race condition here?

which threads can Start() be executed on?

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:88: void
AudioDevice::OnRequestPacket(AudioBuffersState buffers_state) {
is there any plan to implement these?

perhaps have NOTIMPLEMENTED or a comment or a TODO etc

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:100: base::SharedMemoryHandle handle,
base::SyncSocket::Handle socket_handle,
drop 2nd arg to next line

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:107: DCHECK_NE(-1, handle.fd);
sanity check: is -1 the only possible bad value or should this be
DCHECK_GE(handle.fd, 0) ?

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:122: // TODO(crogers) : we could optionally set
the thread to high-priority
nit: no space between ) and :

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:130: void AudioDevice::OnVolume(double volume) {
ditto for this method

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:138: pending_data >= 0) {
this should be aligned at (

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:146: DCHECK_EQ(2, channels_);
instead of failing late perhaps this should get checked in Start() and return
false?

I haven't done the math+checks but if the client assumes there's 6 channels and
we're only supporting 2 it sounds ripe for a buffer overflow

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:149: callback_->render(audio_data_,
buffer_size_);
double-checking something just to be safe..

the header file says 2nd arg is number_of_frames but if I understand this
correctly this is buffer_size_ (in bytes?) which I guess is technically
number_of_frames * sizeof(float)

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:151: // Interleave, scale, and clip to int16.
Is it worth converting everything to float at some point?  If I recall I think
we do a few conversions between int/float before we hit the sound card

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:152: int16* output_buffer16 =
static_cast<int16*>(shared_memory_data());
is there anything in media/audio/audio_util.h we can consider using and/or
moving there?

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_device.h
File chrome/renderer/audio_device.h (right):

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:9: #include <vector>
nit: blank line between system + non-system headers

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:17: // Each instance of AudioDevice corresponds
to one host stream.
are there any threading restrictions as to how AudioDevices are used?

should AudioDevice only be accessed on the main render thread or can any thread
create/delete an AudioDevice, call Stop/Start, etc...

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:21: class RenderCallback {
what about typedeffing a callback type instead of introducing a type?

typedef Callback2<const std::vector<float*>&, size_t>::Type RenderCallback;

It's mostly a preference/style thing.

--UPDATE--

I think you're reflecting the name of the callback in WebKit -- in which case I
think this is OK (unless that's not what you're doing).

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:23: virtual void render(const
std::vector<float*>& audio_data,
considering this isn't a non-trivial function it should be capitalized: Render

--UPDATE--

I see that this is trying to duplicate the WebKit RenderCallback interface,
however this is straight up Chrome code so the capitalized Render() still sticks

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:26: virtual ~RenderCallback() { }
no inlined ctors/dtors in header files

http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:40: // AudioMessageFilter::Delegate
implementation
nit: period at end

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:49: // End of AudioMessageFilter::Delegate
implementation
nit: based on recent chromium-dev style conversations there's no need for this
comment

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:51: // DelegateSimpleThread::Delegate
implementation
nit: period at end

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:53: // End of DelegateSimpleThread::Delegate
implementation
ditto

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:76: base::SharedMemory* shared_memory() { return
shared_memory_.get(); }
nit: class order style has helper methods go before data memebers

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:79: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
File webkit/glue/webaudiodevice_impl.cc (right):

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.cc:16: : audio_device_(buffer_size,
nit: should be indented extra 2 spaces

nit: the audio_device_ line can fit on a single line

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.cc:38: if (client_callback_) {
is client_callback_ allowed to be NULL?

looks like it's set once (in ctor)

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.cc:41: for (unsigned i = 0; i <
audio_data.size(); ++i)
unsigned -> size_t

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
File webkit/glue/webaudiodevice_impl.h (right):

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.h:8: #include <vector>
nit: blank line after system includes

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.h:19: unsigned channels,
unsigned -> int (to match AudioDevice)

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.h:27: // End of WebKit::WebAudioDevice
implementation
nit: no need for this comment

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.h:29: void render(const std::vector<float*>&
audio_data, size_t number_of_frames);
should have an implementation comment

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.h:33: WebKit::WebAudioDevice::RenderCallback*
client_callback_;
sanity check: who's responsible for deleting the callback?

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.h:34: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webkitclient_im...
File webkit/glue/webkitclient_impl.h (right):

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webkitclient_im...
webkit/glue/webkitclient_impl.h:45: size_t buffer_size, unsigned channels,
double sample_rate,
unsigned -> int

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webkitclient_im...
webkit/glue/webkitclient_impl.h:78: 
nit: remove extra blank line

[Chris Rogers, 2011-01-11 23:01:02]

Guys, thanks for the review.  I think I've addressed your comments.

Neb, you asked the following:
"I can't really help much with WebKit stuff, but I have a few questions. For
one, do you expect the PPAPI implementation to re-use this?"

My original intention was to have an API which Pepper could use, but in going
through the current Pepper implementation in some detail and discussing with
Nicholas and Darin, it may not be possible.  The Pepper implementation needs to
expose the concepts of the shared memory and SyncSocket so they can be used in
other processes than the renderer.  The idea behind the AudioDevice API is to
hide these implementation details away.  So, it may be best to simply leave the
Pepper implementation as is.  That said, if you have some other
ideas/possibilities then we should explore them.

But, I do think the API will still be suitable for use with <audio>/<video> and
the upcoming RTP code (as well as the web audio API of course).

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
File chrome/renderer/audio_device.cc (right):

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:21: audio_data_.reserve(channels);
I think for the simple case here it might not be necessary, but if we start
doing a lot more with planar format like this in chromium then we could consider
something like what I've done in WebKit with the AudioChannel and AudioBus
classes:

https://svn.webkit.org/repository/webkit/trunk/Source/WebCore/platform/audio/...
https://svn.webkit.org/repository/webkit/trunk/Source/WebCore/platform/audio/...

On 2011/01/10 23:42:32, scherkus wrote:
> (thinking aloud) I wonder if it makes sense to make audio_data_ a class that
> takes care of this stuff... probably not

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:24: malloc(buffer_size * sizeof(float)));
On 2011/01/10 23:42:32, scherkus wrote:
> malloc is rarely used -- I'd just go for new float[]

Done.

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:32: free(audio_data_[i]);
On 2011/01/10 23:42:32, scherkus wrote:
> and delete[] here

Done.

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:35: bool AudioDevice::Start() {
Andrew also asked the same question.  The class is not thread-safe, so all calls
must be made on the same thread.  I've added a class comment in the header file.

On 2011/01/10 23:11:57, neb wrote:
> For PPAPI, there was a requirement that all the audio calls happen on one
> thread, so we can get away with not making them thread safe. Checking
stream_id_
> non-atomically, using scoped_ptrs without mutexes and so on. What threading
> model does this support?

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:42: if (!filter_.get()) {
Yes, you're right.  Unless the API contract is that calls for all instances can
only be made on the same thread (like main thread) then we need a lock which
I've added.

On 2011/01/10 23:42:32, scherkus wrote:
> sanity check: are we sure there's no race condition here?
> 
> which threads can Start() be executed on?

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:88: void
AudioDevice::OnRequestPacket(AudioBuffersState buffers_state) {
On 2011/01/10 23:42:32, scherkus wrote:
> is there any plan to implement these?
> 
> perhaps have NOTIMPLEMENTED or a comment or a TODO etc

Done.

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:100: base::SharedMemoryHandle handle,
base::SyncSocket::Handle socket_handle,
On 2011/01/10 23:42:32, scherkus wrote:
> drop 2nd arg to next line

Done.

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:107: DCHECK_NE(-1, handle.fd);
This was taken from Neb's code, but yes, your check might be better -- changing.

On 2011/01/10 23:42:32, scherkus wrote:
> sanity check: is -1 the only possible bad value or should this be
> DCHECK_GE(handle.fd, 0) ?

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:122: // TODO(crogers) : we could optionally set
the thread to high-priority
On 2011/01/10 23:42:32, scherkus wrote:
> nit: no space between ) and :

Done.

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:130: void AudioDevice::OnVolume(double volume) {
On 2011/01/10 23:42:32, scherkus wrote:
> ditto for this method

Done.

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:138: pending_data >= 0) {
On 2011/01/10 23:42:32, scherkus wrote:
> this should be aligned at (

Done.

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:146: DCHECK_EQ(2, channels_);
In looking over this code again, I don't think the check for stereo is necessary
- removing.

On 2011/01/10 23:42:32, scherkus wrote:
> instead of failing late perhaps this should get checked in Start() and return
> false?
> 
> I haven't done the math+checks but if the client assumes there's 6 channels
and
> we're only supporting 2 it sounds ripe for a buffer overflow

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:149: callback_->render(audio_data_,
buffer_size_);
Actually, buffer_size_ is in sample-frames, so this should be OK.  In the header
file, I've added a comment for the constructor and the member variable.

I don't like the field name "samples_per_packet" in
ViewHostMsg_Audio_CreateStream_Params, but it appears to actually mean
"number_of_sample_frames".


On 2011/01/10 23:42:32, scherkus wrote:
> double-checking something just to be safe..
> 
> the header file says 2nd arg is number_of_frames but if I understand this
> correctly this is buffer_size_ (in bytes?) which I guess is technically
> number_of_frames * sizeof(float)

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:151: // Interleave, scale, and clip to int16.
I would be in favor of switching most of our code to float.  One advantage is
that it's not lossy like int16 is.  Some audio hardware outputs at 24bits, so
we're currently losing information when converting to int16.  Floating-point is
easy to work with, and is fast nowadays.  The entire web audio engine operates
on floating-point data.

We still need to keep in mind that FFmpeg (I believe) generally decodes to
int16, and some of the OS-level audio APIs only support int16 output, but that's
OK.

We can talk about moving more of our code to deal with float as a possible goal.


On 2011/01/10 23:42:32, scherkus wrote:
> Is it worth converting everything to float at some point?  If I recall I think
> we do a few conversions between int/float before we hit the sound card

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:151: // Interleave, scale, and clip to int16.
Currently the API requires the client provide the audio data in floating-point
format (in the callback).  We could engineer this to be a bit more flexible
where the AudioDevice can be configured to either deal with int16 or float data.
 Then we'd have to deal with void* pointers in the callback, which get casted to
one or the other depending on type.  Or, we could extend the "RenderCallback"
class to have two or more different flavors of callback which get invoked
depending on how it's configured.  I'm very open to making this more flexible,
but it would be good to get a base-level implementation landed, then work on
extending the API in subsequent patches.

On 2011/01/10 23:11:57, neb wrote:
> In PPAPI, we expect to provide different sample types in the future, such as
> floats. Are you sure you want to do this conversion here?

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:152: int16* output_buffer16 =
static_cast<int16*>(shared_memory_data());
Sure, I made a more general function in audio_util.h called
InterleaveFloatToInt16()

On 2011/01/10 23:42:32, scherkus wrote:
> is there anything in media/audio/audio_util.h we can consider using and/or
> moving there?

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_device.h
File chrome/renderer/audio_device.h (right):

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:9: #include <vector>
On 2011/01/10 23:42:32, scherkus wrote:
> nit: blank line between system + non-system headers

Done.

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:17: // Each instance of AudioDevice corresponds
to one host stream.
I've added a comment that the class is *not* thread-safe.

On 2011/01/10 23:42:32, scherkus wrote:
> are there any threading restrictions as to how AudioDevices are used?
> 
> should AudioDevice only be accessed on the main render thread or can any
thread
> create/delete an AudioDevice, call Stop/Start, etc...

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:21: class RenderCallback {
Yes, it's pretty much mirroring what we're doing in the WebKit API.

On 2011/01/10 23:42:32, scherkus wrote:
> what about typedeffing a callback type instead of introducing a type?
> 
> typedef Callback2<const std::vector<float*>&, size_t>::Type RenderCallback;
> 
> It's mostly a preference/style thing.
> 
> --UPDATE--
> 
> I think you're reflecting the name of the callback in WebKit -- in which case
I
> think this is OK (unless that's not what you're doing).

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:23: virtual void render(const
std::vector<float*>& audio_data,
On 2011/01/10 23:42:32, scherkus wrote:
> considering this isn't a non-trivial function it should be capitalized: Render
> 
> --UPDATE--
> 
> I see that this is trying to duplicate the WebKit RenderCallback interface,
> however this is straight up Chrome code so the capitalized Render() still
sticks

Done.

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:26: virtual ~RenderCallback() { }
I agree with the style guideline in general, but I'm not sure that it's meant to
apply to abstract base classes like this.  The media directory has a few
abstract base classes where the virtual destructor is inline:

class AudioManager;
class AudioOutputStream;
class FFmpegURLProtocol;


On 2011/01/10 23:42:32, scherkus wrote:
> no inlined ctors/dtors in header files
> 
> http://dev.chromium.org/developers/coding-style#TOC-Inline-functions

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:40: // AudioMessageFilter::Delegate
implementation
On 2011/01/10 23:42:32, scherkus wrote:
> nit: period at end

Done.

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:49: // End of AudioMessageFilter::Delegate
implementation
On 2011/01/10 23:42:32, scherkus wrote:
> nit: based on recent chromium-dev style conversations there's no need for this
> comment

Done.

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:51: // DelegateSimpleThread::Delegate
implementation
On 2011/01/10 23:42:32, scherkus wrote:
> nit: period at end

Done.

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:53: // End of DelegateSimpleThread::Delegate
implementation
On 2011/01/10 23:42:32, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:76: base::SharedMemory* shared_memory() { return
shared_memory_.get(); }
On 2011/01/10 23:42:32, scherkus wrote:
> nit: class order style has helper methods go before data memebers

Done.

http://codereview.chromium.org/6002005/diff/12001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:79: };
On 2011/01/10 23:42:32, scherkus wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
File webkit/glue/webaudiodevice_impl.cc (right):

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.cc:16: : audio_device_(buffer_size,
On 2011/01/10 23:42:32, scherkus wrote:
> nit: should be indented extra 2 spaces
> 
> nit: the audio_device_ line can fit on a single line

Done.

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.cc:38: if (client_callback_) {
It's not allowed to be NULL.  This is just a safety check.  I just added a
DCHECK as well...

On 2011/01/10 23:42:32, scherkus wrote:
> is client_callback_ allowed to be NULL?
> 
> looks like it's set once (in ctor)

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.cc:41: for (unsigned i = 0; i <
audio_data.size(); ++i)
On 2011/01/10 23:42:32, scherkus wrote:
> unsigned -> size_t

Done.

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
File webkit/glue/webaudiodevice_impl.h (right):

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.h:8: #include <vector>
On 2011/01/10 23:42:32, scherkus wrote:
> nit: blank line after system includes

Done.

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.h:11: #include "chrome/renderer/audio_device.h"
On 2011/01/10 23:11:57, neb wrote:
> c before t

Done.

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.h:19: unsigned channels,
On 2011/01/10 23:42:32, scherkus wrote:
> unsigned -> int (to match AudioDevice)

Done.

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.h:27: // End of WebKit::WebAudioDevice
implementation
On 2011/01/10 23:42:32, scherkus wrote:
> nit: no need for this comment

Done.

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.h:29: void render(const std::vector<float*>&
audio_data, size_t number_of_frames);
On 2011/01/10 23:42:32, scherkus wrote:
> should have an implementation comment

Done.

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.h:33: WebKit::WebAudioDevice::RenderCallback*
client_callback_;
Added a comment about this being a weak reference.

On 2011/01/10 23:42:32, scherkus wrote:
> sanity check: who's responsible for deleting the callback?

http://codereview.chromium.org/6002005/diff/12001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.h:34: };
On 2011/01/10 23:42:32, scherkus wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[neb, 2011-01-12 00:55:32]

LGTM.

[scherkus (not reviewing), 2011-01-12 03:11:43]

few nits and also need to figure out how we can handle the single
AudioMessageFilter without resorting to statically declared Lock etc...

other than that this is looking really great!

http://codereview.chromium.org/6002005/diff/29001/chrome/renderer/audio_devic...
File chrome/renderer/audio_device.cc (right):

http://codereview.chromium.org/6002005/diff/29001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:12: Lock AudioDevice::message_filter_lock_;
hrmm.. global/static non-basic-types are no good I'm afraid :\

have you looked into how other MessageFilter classes are used?  perhaps
base/singleton.h might help out here

http://codereview.chromium.org/6002005/diff/29001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:25: float* channel_data =
static_cast<float*>(new float[buffer_size]);
don't think we need static_cast<> anymore :)

http://codereview.chromium.org/6002005/diff/29001/chrome/renderer/audio_device.h
File chrome/renderer/audio_device.h (right):

http://codereview.chromium.org/6002005/diff/29001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:30: virtual ~RenderCallback() { }
micronit: no space between {}

http://codereview.chromium.org/6002005/diff/29001/media/audio/audio_util.h
File media/audio/audio_util.h (right):

http://codereview.chromium.org/6002005/diff/29001/media/audio/audio_util.h#ne...
media/audio/audio_util.h:73: void InterleaveFloatToInt16(std::vector<float*>
source,
can source be const ref?
const std::vector<float*>& source

http://codereview.chromium.org/6002005/diff/29001/webkit/glue/webkitclient_im...
File webkit/glue/webkitclient_impl.h (right):

http://codereview.chromium.org/6002005/diff/29001/webkit/glue/webkitclient_im...
webkit/glue/webkitclient_impl.h:45: size_t buffer_size, unsigned channels,
double sample_rate,
nit: unsigned -> int

http://codereview.chromium.org/6002005/diff/29001/webkit/glue/webkitclient_im...
webkit/glue/webkitclient_impl.h:78: 
nit: remove blank line

[Chris Rogers, 2011-01-12 21:26:09]

Andrew, thanks for the latest comments.  I think I've addressed them all.  I
tried using a Singleton as you suggested to handle the shared
AudioMessageFilter...

http://codereview.chromium.org/6002005/diff/29001/chrome/renderer/audio_devic...
File chrome/renderer/audio_device.cc (right):

http://codereview.chromium.org/6002005/diff/29001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:25: float* channel_data =
static_cast<float*>(new float[buffer_size]);
On 2011/01/12 03:11:43, scherkus wrote:
> don't think we need static_cast<> anymore :)

Done.

http://codereview.chromium.org/6002005/diff/29001/chrome/renderer/audio_device.h
File chrome/renderer/audio_device.h (right):

http://codereview.chromium.org/6002005/diff/29001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.h:30: virtual ~RenderCallback() { }
On 2011/01/12 03:11:43, scherkus wrote:
> micronit: no space between {}

Done.

http://codereview.chromium.org/6002005/diff/29001/media/audio/audio_util.h
File media/audio/audio_util.h (right):

http://codereview.chromium.org/6002005/diff/29001/media/audio/audio_util.h#ne...
media/audio/audio_util.h:73: void InterleaveFloatToInt16(std::vector<float*>
source,
Yes, of course! --- FIXED

On 2011/01/12 03:11:43, scherkus wrote:
> can source be const ref?
> const std::vector<float*>& source

http://codereview.chromium.org/6002005/diff/29001/webkit/glue/webkitclient_im...
File webkit/glue/webkitclient_impl.h (right):

http://codereview.chromium.org/6002005/diff/29001/webkit/glue/webkitclient_im...
webkit/glue/webkitclient_impl.h:45: size_t buffer_size, unsigned channels,
double sample_rate,
This one I can't easily change since the base class has already been landed in
WebKit and we must override with the same signature.

On 2011/01/12 03:11:43, scherkus wrote:
> nit: unsigned -> int

http://codereview.chromium.org/6002005/diff/29001/webkit/glue/webkitclient_im...
webkit/glue/webkitclient_impl.h:78: 
On 2011/01/12 03:11:43, scherkus wrote:
> nit: remove blank line
> 

Done.

[scherkus (not reviewing), 2011-01-12 21:32:10]

LGTM w/ one nit about AudioDevice being a pointer and forward declaration

feel free to land though!  this looks really great

http://codereview.chromium.org/6002005/diff/47001/chrome/renderer/audio_devic...
File chrome/renderer/audio_device.cc (right):

http://codereview.chromium.org/6002005/diff/47001/chrome/renderer/audio_devic...
chrome/renderer/audio_device.cc:21: class AudioMessageFilterCreator {
nice!

http://codereview.chromium.org/6002005/diff/47001/webkit/glue/webaudiodevice_...
File webkit/glue/webaudiodevice_impl.h (right):

http://codereview.chromium.org/6002005/diff/47001/webkit/glue/webaudiodevice_...
webkit/glue/webaudiodevice_impl.h:11: #include "chrome/renderer/audio_device.h"
nit: if audio_device_ is now a pointer I think we can remove this header and
replace it with a forward declare

[Chris Rogers, 2011-01-13 01:55:18]

Thanks Andrew for the LGTM.  This last patch upload fixed a build problem with
the try-bots by moving the WebKit::WebKitClient implementation to
RendererWebKitClientImpl from webkit_glue::WebKitClientImpl.  This is to avoid
link errors with test_shell and DRT.

Ken, could you look over this change just to double-check?

[Chris Rogers, 2011-01-13 01:55:19]

Thanks Andrew for the LGTM.  This last patch upload fixed a build problem with
the try-bots by moving the WebKit::WebKitClient implementation to
RendererWebKitClientImpl from webkit_glue::WebKitClientImpl.  This is to avoid
link errors with test_shell and DRT.

Ken, could you look over this change just to double-check?

[Ken Russell (switch to Gerrit), 2011-01-13 02:37:57]

The movement of the creation of the WebAudioDevice to
renderer_webkitclient_impl.cc/h looks good to me.