Allow audio system to handle synchronized low-latency audio I/O

Allow audio system to handle synchronized low-latency audio I/O

As an important part of WebAudio/WebRTC integration, we need to be able to process and analyse
live audio.  This change adds the ability to our audio system for handling synchronized audio
input and output in the same callback (same thread) which is important for good performance
and low-latency.

As a part of this change, the audio IPC system now takes an optional |input_channels| argument
in the CreateStream() message and associated browser-side code in AudioRendererHost::OnCreateStream(), etc.
|input_channels| will be 0 during normal operation of audio output (and no input).
But when synchronized audio I/O is needed, then a non-zero value can be passed in here.
The |params| passed in represents both the input and output format, particularly the
frames_per_buffer() and sample_rate().

AudioRendererSink now has an new InitializeIO() method which will allow the use of
synchronized I/O with the |input_channels| argument.  AudioRendererSink::RenderCallback
now has a new RenderIO() which will be called instead of Render() in the case where a
non-zero value is passed in for |input_channels|.

BUG=none
TEST=none
(manual testing on early Mac OS X and Windows audio back-ends)

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

[Chris Rogers, 2012-08-10 22:31:29]

Hey guys, I have an early patch ready for high-level comments.  Some things to
note:

* The "audio_unified_mac.cc" will be moved into a separate CL, but is included
here in case anybody wants to play with things early on.  I do have this
working, so contact me if you're interested.

* The rest of the CL is mainly plumbing

* The intention is that AudioDevice will behave normally for all clients as
before (same calling sequences in both browser/renderer, etc.).  There will be
an AudioDevice::Initialization() option to request "I/O mode" which is opt-in. 
In this case the RenderIO() method will be called instead of Render()

[henrika (OOO until Aug 14), 2012-08-13 15:43:59]

Hi Chris,

just took a very quick look at this today. My plan was to try it out on my Mac
but I realized that I was not sure what to test actually. There is nothing I can
do based on this CL alone, right?

[Chris Rogers, 2012-08-13 17:27:25]

On 2012/08/13 15:43:59, henrika wrote:
> Hi Chris,
> 
> just took a very quick look at this today. My plan was to try it out on my Mac
> but I realized that I was not sure what to test actually. There is nothing I
can
> do based on this CL alone, right?

Correct, for now there are two additional WebKit patches which need to be
applied, but I'm working on getting those landed quickly.

[Ami GONE FROM CHROMIUM, 2012-08-14 06:24:23]

drive-by

I think you have a particular source & sink pair in mind, but I don't know what
they are :)  Can you describe in more detail your goal for this CL?  (either in
the CL description or in a linked crbug; if this is going to take multiple CLs,
probably a crbug is best)

[Chris Rogers, 2012-08-14 06:51:27]

On 2012/08/14 06:24:23, Ami Fischman wrote:
> drive-by
> 
> I think you have a particular source & sink pair in mind, but I don't know
what
> they are :)  Can you describe in more detail your goal for this CL?  (either
in
> the CL description or in a linked crbug; if this is going to take multiple
CLs,
> probably a crbug is best)

Hi Ami, thanks for having a look.  The idea is that, at the driver level and
low-level OS-level, the audio hardware often includes *both* input and output,
and that this allows for lower latency to handle both in the same thread (I/O
cycle).  This is an important part of the work we're doing for WebAudio/WebRTC
integration for live audio processing and analysis.

There has been ongoing work in WebKit also.  I can add more details, and we can
refine this CL, coordinating the work with Dale's current re-factorings of
OnMoreData(), etc.

[Ami GONE FROM CHROMIUM, 2012-08-14 06:56:16]

Is this really about handling both input and output in the same timeslice,
or is it about allowing a single callback to modify the output it sends
based on the input it received (in the same invocation)?  Or both /
something else?

[henrika (OOO until Aug 14), 2012-08-14 11:23:07]

Ami: my interpretation of this CL is that it allows us to process the
captured data before sending it to the output for rendering. We could use
the existing input and output streams but it would then use more threads
and other resources. In addition, the input and output sides might use
different rates or not share the same internal clock which could lead to
clock-drift issues and we would need some sort of compensation for that. In
WebRTC we can loop audio in full-duplex but we need a complex jitter buffer
(called NetEQ) in the middle to ensure OK audio for all scenarios.


On Tue, Aug 14, 2012 at 8:56 AM, Ami Fischman <fischman@chromium.org> wrote:

> Is this really about handling both input and output in the same timeslice,
> or is it about allowing a single callback to modify the output it sends
> based on the input it received (in the same invocation)?  Or both /
> something else?
>

[scherkus (not reviewing), 2012-08-15 20:06:02]

we should definitely get a proper meta bug setup to track the work!

crogers: let me know when you want a proper review -- so far the approach seems
reasonable

fischman: the basic idea is to generate output based on the input data in the
same timeslice/callback

opening up separate input/output streams and attempting to synchronize
processing between the two introduces latency due to buffering

I think it's worth calling out that this requires OS / driver model support. For
example I'm not sure what we're going to do for Win XP users that only have
waveout to work with (nothing?)

[henrika (OOO until Aug 14), 2012-08-17 14:00:04]

crogers: as we have discussed. I'll try to come ups with something similar for
Windows based on Core Audio (WASAPI). I don't think I can find a "perfect"
solution that gives me both in and out at the same time. Will do my best. I am
unable to give any comments on what we can do on XP at this stage.

Also, could you list the hardware requirements for this patch. Thanks.

[Chris Rogers, 2012-08-25 02:03:09]

* Added --enable-webaudio-input.  None of the new I/O stuff can run unless this
flag has been explicitly enabled

* Added InitializeIO() method so client may select optional I/O

* Pass through optional I/O info (input number of channels) to browser process
so it may allocate space for input buffers in shared memory

* Still need to rebase when Dale's shared-memory / OnMoreData() re-factoring
lands:
http://codereview.chromium.org/10832285/

[jtlinden, 2012-08-29 14:41:33]

On 2012/08/25 02:03:09, Chris Rogers wrote:
> * Added --enable-webaudio-input.  None of the new I/O stuff can run unless
this
> flag has been explicitly enabled
> 
> * Added InitializeIO() method so client may select optional I/O
> 
> * Pass through optional I/O info (input number of channels) to browser process
> so it may allocate space for input buffers in shared memory
> 
> * Still need to rebase when Dale's shared-memory / OnMoreData() re-factoring
> lands:
> http://codereview.chromium.org/10832285/

I created a launch bug here: http://codereview.chromium.org/10830268/

And there is the original bug to track the work here: 
http://code.google.com/p/chromium/issues/detail?id=121673

[Chris Rogers, 2012-09-06 01:36:45]

PTAL: now ready for formal review

[henrika (OOO until Aug 14), 2012-09-06 07:13:39]

Nice work Chris,

one initial comment regarding the kEnableWebAudioInput flag which is notw in
content. I think it prevents us from using it in media since media can't depend
on content. And we need it in the media namespace, right?

[Ami GONE FROM CHROMIUM, 2012-09-06 07:21:10]

(removing myself from reviewers as my previous comment was meant to be a
drive-by)

Chris: the CL description in rietveld needs updating :)

[tommi (sloooow) - chröme, 2012-09-07 09:58:18]

https://chromiumcodereview.appspot.com/10830268/diff/31001/content/public/com...
File content/public/common/content_switches.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/31001/content/public/com...
content/public/common/content_switches.cc:233: const char kEnableWebAudioInput[]
    = "enable-webaudio-input";
looks like =  should be aligned in this file.

https://chromiumcodereview.appspot.com/10830268/diff/31001/content/public/com...
File content/public/common/content_switches.h (right):

https://chromiumcodereview.appspot.com/10830268/diff/31001/content/public/com...
content/public/common/content_switches.h:90: CONTENT_EXPORT extern const char
kEnableWebAudioInput[];
Will we ever need to check this switch from media/?
If so, then we can't have it here since media is not allowed to depend on
content.  We could move it to base.

https://chromiumcodereview.appspot.com/10830268/diff/31001/content/renderer/m...
File content/renderer/media/renderer_webaudiodevice_impl.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/31001/content/renderer/m...
content/renderer/media/renderer_webaudiodevice_impl.cc:25: // TODO(crogers):
remove once we properly handle input device selection.
file a bug for tracking?

https://chromiumcodereview.appspot.com/10830268/diff/31001/content/renderer/m...
content/renderer/media/renderer_webaudiodevice_impl.cc:28:
audio_device_->InitializeIO(params, 2, this);
won't hardcoding this at 2 be problematic for non-stereo devices? e.g. a mono
mic?

https://chromiumcodereview.appspot.com/10830268/diff/31001/media/audio/audio_...
File media/audio/audio_input_device.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/31001/media/audio/audio_...
media/audio/audio_input_device.cc:309: :
AudioDeviceThread::Callback(audio_parameters, 0, memory, memory_length),
It is a bit odd that the AudioInputDevice class passes 0 for the number of input
devices.  I know the reason for it but we're making an architectural change that
makes things confusing.  Will AudioInputDevice eventually be going away?

https://chromiumcodereview.appspot.com/10830268/diff/31001/media/audio/audio_...
File media/audio/audio_output_device_unittest.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/31001/media/audio/audio_...
media/audio/audio_output_device_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
What about adding tests for the new code?

[Chris Rogers, 2012-09-07 20:34:01]

https://chromiumcodereview.appspot.com/10830268/diff/31001/content/public/com...
File content/public/common/content_switches.h (right):

https://chromiumcodereview.appspot.com/10830268/diff/31001/content/public/com...
content/public/common/content_switches.h:90: CONTENT_EXPORT extern const char
kEnableWebAudioInput[];
On 2012/09/07 09:58:19, tommi wrote:
> Will we ever need to check this switch from media/?
> If so, then we can't have it here since media is not allowed to depend on
> content.  We could move it to base.

Fixed: moved to "media_switches"

https://chromiumcodereview.appspot.com/10830268/diff/31001/content/renderer/m...
File content/renderer/media/renderer_webaudiodevice_impl.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/31001/content/renderer/m...
content/renderer/media/renderer_webaudiodevice_impl.cc:25: // TODO(crogers):
remove once we properly handle input device selection.
On 2012/09/07 09:58:19, tommi wrote:
> file a bug for tracking?

Done.

https://chromiumcodereview.appspot.com/10830268/diff/31001/content/renderer/m...
content/renderer/media/renderer_webaudiodevice_impl.cc:28:
audio_device_->InitializeIO(params, 2, this);
On 2012/09/07 09:58:19, tommi wrote:
> won't hardcoding this at 2 be problematic for non-stereo devices? e.g. a mono
> mic?

The Mac-backend will automatically handle this case, and I need to work with
Henrik to make sure we do something reasonable here as wel on Windowsl.  I
actually have a USB guitar cable (USB at one end and 1/4" guitar on the other :)
which is mono.  So I'm on this one :)

I added this bug# to the TODO:
https://code.google.com/p/chromium/issues/detail?id=147326

https://chromiumcodereview.appspot.com/10830268/diff/31001/media/audio/audio_...
File media/audio/audio_input_device.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/31001/media/audio/audio_...
media/audio/audio_input_device.cc:309: :
AudioDeviceThread::Callback(audio_parameters, 0, memory, memory_length),
On 2012/09/07 09:58:19, tommi wrote:
> It is a bit odd that the AudioInputDevice class passes 0 for the number of
input
> devices.  I know the reason for it but we're making an architectural change
that
> makes things confusing.  Will AudioInputDevice eventually be going away?

When I first created the AudioDevice class, I had anticipated that it could be
used for both input and/or output.  I think we have a great opportunity to
rework some of this code, and potentially remove AudioInputDevice.  Obviously we
need to look carefully here and discuss this.  But I'm optimistic that we can
actually simplify this all in the end, given some careful work.

https://chromiumcodereview.appspot.com/10830268/diff/31001/media/audio/audio_...
File media/audio/audio_output_device_unittest.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/31001/media/audio/audio_...
media/audio/audio_output_device_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/09/07 09:58:19, tommi wrote:
> What about adding tests for the new code?

Good idea - will do in next patch-set, for now I'm addressing the other
comments.

[scherkus (not reviewing), 2012-09-10 11:46:32]

mostly nits

There are two design decisions I'd like to see documented in the CL description:
  1) Why plumbing input_channels everywhere as opposed to being part of
AudioParameters?
  2) Why a separate RenderIO() callback as opposed to a single Render() callback
w/ a potentially-NULL input AudioBus*?

https://chromiumcodereview.appspot.com/10830268/diff/31014/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://chromiumcodereview.appspot.com/10830268/diff/31014/chrome/browser/DEP...
chrome/browser/DEPS:13: "+media/base",  # For media switches
we shouldn't need this -- see my comments in media_switches.cc

https://chromiumcodereview.appspot.com/10830268/diff/31014/content/browser/re...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/31014/content/browser/re...
content/browser/renderer_host/media/audio_sync_reader.cc:79: // for consumption
by renderer process.
nit: some of this comment can fit on previous line

https://chromiumcodereview.appspot.com/10830268/diff/31014/content/renderer/m...
File content/renderer/media/renderer_webaudiodevice_impl.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/31014/content/renderer/m...
content/renderer/media/renderer_webaudiodevice_impl.cc:12: #include <math.h>
where's the math.h usage?

https://chromiumcodereview.appspot.com/10830268/diff/31014/content/renderer/m...
content/renderer/media/renderer_webaudiodevice_impl.cc:71: // Wrap the inputs
pointers using WebVector.
s/inputs/input/?

https://chromiumcodereview.appspot.com/10830268/diff/31014/media/audio/audio_...
File media/audio/audio_output_ipc.h (right):

https://chromiumcodereview.appspot.com/10830268/diff/31014/media/audio/audio_...
media/audio/audio_output_ipc.h:72: //  Once the stream has been created, the
implementation must
remove extra space

https://chromiumcodereview.appspot.com/10830268/diff/31014/media/base/audio_r...
File media/base/audio_renderer_sink.h (right):

https://chromiumcodereview.appspot.com/10830268/diff/31014/media/base/audio_r...
media/base/audio_renderer_sink.h:31: virtual void RenderIO(AudioBus*
audio_input_data,
nit: most other code has these params as "source / dest" -- here they are
audio_input_data / audio_bus

which one should it be? :)

https://chromiumcodereview.appspot.com/10830268/diff/31014/media/base/media_s...
File media/base/media_switches.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/31014/media/base/media_s...
media/base/media_switches.cc:42: const char kEnableWebAudioInput[] =
"enable-webaudio-input";
general rule of thumb: switches should go in the *_switches.cc file where they
are used

since kEnableWebAudioInput affects higher-level components (gUM() / Web Audio)
and is only used in src/content/ this switch should go under content_switches.cc

[Chris Rogers, 2012-09-10 19:21:38]

Andrew, thanks for taking a look.  I think I've addressed your nits.

I made some comments about kEnableWebAudioInput needing to be in
media_switches.h 

I've updated the CL description with a bit more detail about the plumbing
changes, but can try to explain a little more of the reasoning here:

> There are two design decisions I'd like to see documented in the CL
description:
>   1) Why plumbing input_channels everywhere as opposed to being part of
> AudioParameters?

It *could* be possible to add an additional "input_channels_" field into
AudioParameters, but I felt this was piling on more stuff into that class than
really belongs.

>  2) Why a separate RenderIO() callback as opposed to a single Render()
callback
> w/ a potentially-NULL input AudioBus*?

As you suggest, an alternative *is* to have a single method called Render() with
the potentially-NULL input AudioBus*.  But since there are so many places which
implement the AudioRendererSink::RenderCallback interface using the simple
Render() method (like the resampler, fifo, and mixer code), I thought it would
be better to have a separate method.

One other approach would be to continue to have the resampler, fifo, and mixer
use the RenderCallback interface which only deals with output.  And then add a
new RenderIOCallback interface which all clients of AudioOutputDevice would then
use, and would only need to deal with RenderIO() (or just call it Render() with
|source| and |dest| arguments.

For now, I felt this was the simplest approach impacting the fewest other parts
of our code as possible, but I can see the potential to refine this over time.

https://chromiumcodereview.appspot.com/10830268/diff/31014/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://chromiumcodereview.appspot.com/10830268/diff/31014/chrome/browser/DEP...
chrome/browser/DEPS:13: "+media/base",  # For media switches
On 2012/09/10 11:46:32, scherkus wrote:
> we shouldn't need this -- see my comments in media_switches.cc

I think we'll need to have kEnableWebAudioInput in media_switches.h, because it
will need to be accessible to media code (in particular the AudioManagerMac and
AudioManagerWin).  Then I noticed that about_flags.cc required access to it...

https://chromiumcodereview.appspot.com/10830268/diff/31014/content/browser/re...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/31014/content/browser/re...
content/browser/renderer_host/media/audio_sync_reader.cc:79: // for consumption
by renderer process.
On 2012/09/10 11:46:32, scherkus wrote:
> nit: some of this comment can fit on previous line

Done.

https://chromiumcodereview.appspot.com/10830268/diff/31014/content/renderer/m...
File content/renderer/media/renderer_webaudiodevice_impl.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/31014/content/renderer/m...
content/renderer/media/renderer_webaudiodevice_impl.cc:12: #include <math.h>
On 2012/09/10 11:46:32, scherkus wrote:
> where's the math.h usage?

Sorry, that was only needed for testing -- removed...

https://chromiumcodereview.appspot.com/10830268/diff/31014/content/renderer/m...
content/renderer/media/renderer_webaudiodevice_impl.cc:71: // Wrap the inputs
pointers using WebVector.
On 2012/09/10 11:46:32, scherkus wrote:
> s/inputs/input/?

Done.

https://chromiumcodereview.appspot.com/10830268/diff/31014/media/audio/audio_...
File media/audio/audio_output_ipc.h (right):

https://chromiumcodereview.appspot.com/10830268/diff/31014/media/audio/audio_...
media/audio/audio_output_ipc.h:72: //  Once the stream has been created, the
implementation must
On 2012/09/10 11:46:32, scherkus wrote:
> remove extra space

Done.

https://chromiumcodereview.appspot.com/10830268/diff/31014/media/base/audio_r...
File media/base/audio_renderer_sink.h (right):

https://chromiumcodereview.appspot.com/10830268/diff/31014/media/base/audio_r...
media/base/audio_renderer_sink.h:31: virtual void RenderIO(AudioBus*
audio_input_data,
On 2012/09/10 11:46:32, scherkus wrote:
> nit: most other code has these params as "source / dest" -- here they are
> audio_input_data / audio_bus
> 
> which one should it be? :)

Fixed to be more consistent:
I'm going to go with "source" and "dest" because I think they're more
descriptive than names like "audio_bus".  I would also be fine with "source_bus"
and "dest_bus", but just wanted to keep things simpler.

https://chromiumcodereview.appspot.com/10830268/diff/31014/media/base/media_s...
File media/base/media_switches.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/31014/media/base/media_s...
media/base/media_switches.cc:42: const char kEnableWebAudioInput[] =
"enable-webaudio-input";
On 2012/09/10 11:46:32, scherkus wrote:
> general rule of thumb: switches should go in the *_switches.cc file where they
> are used
> 
> since kEnableWebAudioInput affects higher-level components (gUM() / Web Audio)
> and is only used in src/content/ this switch should go under
content_switches.cc

It will need to be used in media (in the AudioManagers).  I originally had the
switch in content_switches.h, but my code (not in this CL) in AudioManagerMac
triggered a dependency violation.

[Chris Rogers, 2012-09-11 03:32:54]

PTAL:
Tommi: added tests in audio_output_device_unittest

[tommi (sloooow) - chröme, 2012-09-11 09:17:01]

lgtm!

Thanks for adding the test.  I agree that it would be good if we can remove
AudioInputDevice eventually.  The two classes still have quite a bit of
duplicated code and tend to get out of sync.

[DaleCurtis, 2012-09-11 09:44:39]

https://chromiumcodereview.appspot.com/10830268/diff/42013/content/browser/re...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/42013/content/browser/re...
content/browser/renderer_host/media/audio_sync_reader.cc:113:
output_bus_->Zero();
This doesn't Zero everything when frames are padded; just the channel arrays,
which is probably okay, but FYI.

https://chromiumcodereview.appspot.com/10830268/diff/42013/media/audio/audio_...
File media/audio/audio_output_controller.h (right):

https://chromiumcodereview.appspot.com/10830268/diff/42013/media/audio/audio_...
media/audio/audio_output_controller.h:98: virtual int Read(AudioBus* source,
AudioBus* dest) = 0;
This is a bit confusing now; SyncReader and Read() don't really make sense as
names now that we're both reading and writing data to the shared memory.

https://chromiumcodereview.appspot.com/10830268/diff/42013/media/audio/audio_...
File media/audio/audio_output_device.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/42013/media/audio/audio_...
media/audio/audio_output_device.cc:68: int input_channels,
Does it make sense to bite the bullet and have an "AudioParameters input_params"
everywhere?

https://chromiumcodereview.appspot.com/10830268/diff/42013/media/base/audio_b...
File media/base/audio_bus.h (right):

https://chromiumcodereview.appspot.com/10830268/diff/42013/media/base/audio_b...
media/base/audio_bus.h:51: int* aligned_frames);
I'd prefer this to be just CalculateMemorySize(int,int) and paired with the
method above both to keep style with the existing Create(int, int),
WrapMemory(int, int) methods and because FromChannels is only half the equation.
The (int, int) method be ordered first.

Per the style guide, overloading is fine "only if a reader looking at a call
site can get a good idea of what is happening without having to first figure out
exactly which overload is being called." Which fits the bill here. 

The aligned_frames value also shouldn't be exposed as we're leaking internal
details about the AudioBus. I don't see where you're using this value so lets
not expose it.

[scherkus (not reviewing), 2012-09-11 12:13:00]

lgtm!

http://codereview.chromium.org/10830268/diff/31014/media/base/media_switches.cc
File media/base/media_switches.cc (right):

http://codereview.chromium.org/10830268/diff/31014/media/base/media_switches....
media/base/media_switches.cc:42: const char kEnableWebAudioInput[] =
"enable-webaudio-input";
On 2012/09/10 19:21:38, Chris Rogers wrote:
> On 2012/09/10 11:46:32, scherkus wrote:
> > general rule of thumb: switches should go in the *_switches.cc file where
they
> > are used
> > 
> > since kEnableWebAudioInput affects higher-level components (gUM() / Web
Audio)
> > and is only used in src/content/ this switch should go under
> content_switches.cc
> 
> It will need to be used in media (in the AudioManagers).  I originally had the
> switch in content_switches.h, but my code (not in this CL) in AudioManagerMac
> triggered a dependency violation.

Good to know -- thanks!

http://codereview.chromium.org/10830268/diff/42013/media/audio/audio_output_c...
File media/audio/audio_output_controller.h (right):

http://codereview.chromium.org/10830268/diff/42013/media/audio/audio_output_c...
media/audio/audio_output_controller.h:98: virtual int Read(AudioBus* source,
AudioBus* dest) = 0;
On 2012/09/11 09:44:39, DaleCurtis wrote:
> This is a bit confusing now; SyncReader and Read() don't really make sense as
> names now that we're both reading and writing data to the shared memory.

Do you have a suggestion for a name? Perhaps we can TODO-ify a follow-up CL to
rename some parts of this class.

http://codereview.chromium.org/10830268/diff/42013/media/base/audio_bus.h
File media/base/audio_bus.h (right):

http://codereview.chromium.org/10830268/diff/42013/media/base/audio_bus.h#new...
media/base/audio_bus.h:51: int* aligned_frames);
On 2012/09/11 09:44:39, DaleCurtis wrote:
> I'd prefer this to be just CalculateMemorySize(int,int) and paired with the
> method above both to keep style with the existing Create(int, int),
> WrapMemory(int, int) methods and because FromChannels is only half the
equation.
> The (int, int) method be ordered first.
> 
> Per the style guide, overloading is fine "only if a reader looking at a call
> site can get a good idea of what is happening without having to first figure
out
> exactly which overload is being called." Which fits the bill here. 
> 
> The aligned_frames value also shouldn't be exposed as we're leaking internal
> details about the AudioBus. I don't see where you're using this value so lets
> not expose it.

Perhaps we need an internal impl w/ |aligned_frames| to remain inside the .cc as
a static method and expose a version here that doesn't return |aligned_frames|

[DaleCurtis, 2012-09-11 12:22:23]

Since we're a time zone away, LGTM assuming the AudioBus changes.

http://codereview.chromium.org/10830268/diff/42013/media/audio/audio_output_c...
File media/audio/audio_output_controller.h (right):

http://codereview.chromium.org/10830268/diff/42013/media/audio/audio_output_c...
media/audio/audio_output_controller.h:98: virtual int Read(AudioBus* source,
AudioBus* dest) = 0;
On 2012/09/11 12:13:00, scherkus wrote:
> On 2012/09/11 09:44:39, DaleCurtis wrote:
> > This is a bit confusing now; SyncReader and Read() don't really make sense
as
> > names now that we're both reading and writing data to the shared memory.
> 
> Do you have a suggestion for a name? Perhaps we can TODO-ify a follow-up CL to
> rename some parts of this class.

TODO-ify is fine with me. No suggestions that I actually like: SyncReaderWriter,
AudioTransferAgent.

http://codereview.chromium.org/10830268/diff/42013/media/base/audio_bus.h
File media/base/audio_bus.h (right):

http://codereview.chromium.org/10830268/diff/42013/media/base/audio_bus.h#new...
media/base/audio_bus.h:51: int* aligned_frames);
On 2012/09/11 12:13:00, scherkus wrote:
> On 2012/09/11 09:44:39, DaleCurtis wrote:
> > I'd prefer this to be just CalculateMemorySize(int,int) and paired with the
> > method above both to keep style with the existing Create(int, int),
> > WrapMemory(int, int) methods and because FromChannels is only half the
> equation.
> > The (int, int) method be ordered first.
> > 
> > Per the style guide, overloading is fine "only if a reader looking at a call
> > site can get a good idea of what is happening without having to first figure
> out
> > exactly which overload is being called." Which fits the bill here. 
> > 
> > The aligned_frames value also shouldn't be exposed as we're leaking internal
> > details about the AudioBus. I don't see where you're using this value so
lets
> > not expose it.
> 
> Perhaps we need an internal impl w/ |aligned_frames| to remain inside the .cc
as
> a static method and expose a version here that doesn't return |aligned_frames|

Yes, that's what I was thinking.

[jamesr, 2012-09-11 19:24:02]

content/renderer lgtm

[henrika (OOO until Aug 14), 2012-09-11 21:44:48]

LGTM with some comments.

I assume that the next step is to land the required changes in the audio manager
and low-latency impl for Mac, right?

https://chromiumcodereview.appspot.com/10830268/diff/34021/content/browser/re...
File content/browser/renderer_host/media/audio_sync_reader.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/34021/content/browser/re...
content/browser/renderer_host/media/audio_sync_reader.cc:80: if (source &&
input_bus_.get()) {
See my previous comment about a new method. Not so clean to check for NULL here
I guess.

https://chromiumcodereview.appspot.com/10830268/diff/34021/content/common/med...
File content/common/media/audio_messages.h (right):

https://chromiumcodereview.appspot.com/10830268/diff/34021/content/common/med...
content/common/media/audio_messages.h:87: // Request that got sent to browser
for creating an audio output stream
Modify comment perhaps?

https://chromiumcodereview.appspot.com/10830268/diff/34021/media/audio/audio_...
File media/audio/audio_io.h (right):

https://chromiumcodereview.appspot.com/10830268/diff/34021/media/audio/audio_...
media/audio/audio_io.h:66: virtual int OnMoreIOData(AudioBus* source,
Comment?

https://chromiumcodereview.appspot.com/10830268/diff/34021/media/audio/audio_...
File media/audio/audio_output_controller.h (right):

https://chromiumcodereview.appspot.com/10830268/diff/34021/media/audio/audio_...
media/audio/audio_output_controller.h:100: virtual int Read(AudioBus* source,
AudioBus* dest) = 0;
You could also create two different Read methods, one with source only and one
with both source and dest. More clear perhaps?

https://chromiumcodereview.appspot.com/10830268/diff/34021/media/audio/audio_...
File media/audio/audio_output_device.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/34021/media/audio/audio_...
media/audio/audio_output_device.cc:278: int io_size = output_memory_size +
input_memory_size;
Do we need all the added space in this section?

https://chromiumcodereview.appspot.com/10830268/diff/34021/media/audio/audio_...
media/audio/audio_output_device.cc:314:
render_callback_->RenderIO(input_bus_.get(),
Why not num_frames = render_callback->?

https://chromiumcodereview.appspot.com/10830268/diff/34021/media/base/audio_b...
File media/base/audio_bus.h (right):

https://chromiumcodereview.appspot.com/10830268/diff/34021/media/base/audio_b...
media/base/audio_bus.h:48: // Calculates the required size for an AudioBus given
the number of channels
Why is this comment different from the comment above. Is it not the same method
just with two different signatures?

https://chromiumcodereview.appspot.com/10830268/diff/34021/media/base/audio_r...
File media/base/audio_renderer_sink.h (right):

https://chromiumcodereview.appspot.com/10830268/diff/34021/media/base/audio_r...
media/base/audio_renderer_sink.h:31: virtual void RenderIO(AudioBus* source,
Nit, are we OK with default implementations in interfaces?

https://chromiumcodereview.appspot.com/10830268/diff/34021/media/base/audio_r...
media/base/audio_renderer_sink.h:54: virtual void InitializeIO(const
AudioParameters& params,
It is not clear to me why we need inputy_channels. Is it not sufficient to have
two different Init methods?

https://chromiumcodereview.appspot.com/10830268/diff/34021/media/base/media_s...
File media/base/media_switches.cc (right):

https://chromiumcodereview.appspot.com/10830268/diff/34021/media/base/media_s...
media/base/media_switches.cc:44: // Enable live audio input with getUserMedia()
and the Web Audio API.
Nit, why add 'live"? Not sure what it adds really. Guess you mean "not reading
from a file". Hmm, sounds OK actually ;-) Anyhow, keeping the comment.

[piman, 2012-09-11 22:39:30]

I only looked at content/ but LGTM there.

[Nico, 2012-09-12 01:55:06]

chrome/ lgtm

[commit-bot: I haz the power, 2012-09-12 02:05:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/crogers@google.com/10830268/44121

[commit-bot: I haz the power, 2012-09-12 04:37:06]

Change committed as 156234