Switch AudioRenderSink::Callback to use AudioBus.

Switch AudioRenderSink::Callback to use AudioBus.

As titled, switches everything over to using the AudioBus
class instead of const std::vector<float*>.  Allows removal
of lots of crufty allocations and memsets.

BUG=114700
TEST=unit tests, layout tests, try bots.  Nothing should change.


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

[tommi (sloooow) - chröme, 2012-08-04 09:07:37]

looks great.  lgtm for when AudioBus has landed.

[DaleCurtis, 2012-08-07 23:12:19]

Finishes the conversion to AudioBus!  Next step: Convert
AudioSourceCallback::OnMoreData()

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_device_th...
File media/audio/audio_device_thread.cc (right):

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_device_th...
media/audio/audio_device_thread.cc:200: audio_bus_ =
AudioBus::Create(audio_parameters_);
Coming back to my Q on the other CL, if we wrapped the shared memory here
instead of creating a new AudioBus, we can drop the memcpy that will replace
InterleaveFloatToInt() in AudioOutputDevice.

http://codereview.chromium.org/10823175/diff/3008/media/base/audio_renderer_s...
File media/base/audio_renderer_sink.h (right):

http://codereview.chromium.org/10823175/diff/3008/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:32: int number_of_frames,
Do we need/want number_of_frames still? Or should Render() always fill the
AudioBus object completely.

Currently, SincResampler complicates making this change by requesting different
amounts of frames relative to the creation size of the AudioBus. If we fixed
that though, every caller is specifying AudioBus->frames() to Render().

[scherkus (not reviewing), 2012-08-07 23:23:41]

I like :)

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_output_de...
File media/audio/audio_output_device_unittest.cc (right):

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_output_de...
media/audio/audio_output_device_unittest.cc:92: void ZeroAudioData(int
number_of_frames, AudioBus* audio_bus) {
if this was an ACTION_P2() you can get rid of all of the Invoke<> stuff in the
test

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

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_util.h#ne...
media/audio/audio_util.h:89: MEDIA_EXPORT void InterleaveFloatToInt(const
AudioBus& audio_bus,
any reason this can't be AudioBus*?

http://codereview.chromium.org/10823175/diff/3008/media/base/audio_renderer_s...
File media/base/audio_renderer_sink.h (right):

http://codereview.chromium.org/10823175/diff/3008/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:32: int number_of_frames,
On 2012/08/07 23:12:19, DaleCurtis wrote:
> Do we need/want number_of_frames still? Or should Render() always fill the
> AudioBus object completely.
> 
> Currently, SincResampler complicates making this change by requesting
different
> amounts of frames relative to the creation size of the AudioBus. If we fixed
> that though, every caller is specifying AudioBus->frames() to Render().

Doesn't seem too crazy -- what would the fix to SincResampler look like?
Creating/Wrapping AudioBuses?

[DaleCurtis, 2012-08-07 23:49:24]

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

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_util.h#ne...
media/audio/audio_util.h:89: MEDIA_EXPORT void InterleaveFloatToInt(const
AudioBus& audio_bus,
On 2012/08/07 23:23:41, scherkus wrote:
> any reason this can't be AudioBus*?

It could be, but this way enforces that the AudioBus is read only and is
style-guide kosher.

http://codereview.chromium.org/10823175/diff/3008/media/base/audio_renderer_s...
File media/base/audio_renderer_sink.h (right):

http://codereview.chromium.org/10823175/diff/3008/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:32: int number_of_frames,
On 2012/08/07 23:23:41, scherkus wrote:
> On 2012/08/07 23:12:19, DaleCurtis wrote:
> > Do we need/want number_of_frames still? Or should Render() always fill the
> > AudioBus object completely.
> > 
> > Currently, SincResampler complicates making this change by requesting
> different
> > amounts of frames relative to the creation size of the AudioBus. If we fixed
> > that though, every caller is specifying AudioBus->frames() to Render().
> 
> Doesn't seem too crazy -- what would the fix to SincResampler look like?
> Creating/Wrapping AudioBuses?

There are few ways I can think of:
- Creating/wrapping AudioBus objects as you mention. E.g., create one, use
WrapVector() on the first one with different frame sizes. Has the bonus of
allowing one less memcpy per cycle like the existing implementation.
 - Recreate an AudioBus every ProvideInput call (what WebKit's
MultiChannelResampler does).
- Have AudioBus offer a SetFrames()/MaxFrames() method which allows downward
capping of AudioBus->frames(). Alternatively, a Resize() method which just caps
frames_ on downsizing/same-sizing and reallocates on upsizing.
- Fix SincResampler to always request the same amount of data (difficult I
think, maybe crogers would know better).

Buffering inside MCR is out since it would need to sync with some sort of 
flush() API.

[DaleCurtis, 2012-08-08 00:17:58]

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_output_de...
File media/audio/audio_output_device_unittest.cc (right):

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_output_de...
media/audio/audio_output_device_unittest.cc:92: void ZeroAudioData(int
number_of_frames, AudioBus* audio_bus) {
On 2012/08/07 23:23:41, scherkus wrote:
> if this was an ACTION_P2() you can get rid of all of the Invoke<> stuff in the
> test

Actually, I'm not sure I understand what that ZeroAudioData is for. The comment
states it's being invoked with 0 to let valgrind errors through. So it seems to
do nothing? I confirmed it is indeed passing 0, so maybe just remove instead?

[scherkus (not reviewing), 2012-08-08 00:49:01]

lgtm w/ nits

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_device_th...
File media/audio/audio_device_thread.cc (right):

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_device_th...
media/audio/audio_device_thread.cc:200: audio_bus_ =
AudioBus::Create(audio_parameters_);
On 2012/08/07 23:12:19, DaleCurtis wrote:
> Coming back to my Q on the other CL, if we wrapped the shared memory here
> instead of creating a new AudioBus, we can drop the memcpy that will replace
> InterleaveFloatToInt() in AudioOutputDevice.

That seems like a worthwhile TODO / follow-up CL -- avoiding copies are a nice
win as long as it doesn't horribly complicate things

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_output_de...
File media/audio/audio_output_device_unittest.cc (right):

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_output_de...
media/audio/audio_output_device_unittest.cc:92: void ZeroAudioData(int
number_of_frames, AudioBus* audio_bus) {
On 2012/08/08 00:17:58, DaleCurtis wrote:
> On 2012/08/07 23:23:41, scherkus wrote:
> > if this was an ACTION_P2() you can get rid of all of the Invoke<> stuff in
the
> > test
> 
> Actually, I'm not sure I understand what that ZeroAudioData is for. The
comment
> states it's being invoked with 0 to let valgrind errors through. So it seems
to
> do nothing? I confirmed it is indeed passing 0, so maybe just remove instead?

Strange indeed! I'm not sure what's going on here either -- run git blame + ping
the author?

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

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_util.h#ne...
media/audio/audio_util.h:89: MEDIA_EXPORT void InterleaveFloatToInt(const
AudioBus& audio_bus,
On 2012/08/07 23:49:24, DaleCurtis wrote:
> On 2012/08/07 23:23:41, scherkus wrote:
> > any reason this can't be AudioBus*?
> 
> It could be, but this way enforces that the AudioBus is read only and is
> style-guide kosher.

But so does "const AudioBus*"!

http://codereview.chromium.org/10823175/diff/3008/media/base/audio_renderer_s...
File media/base/audio_renderer_sink.h (right):

http://codereview.chromium.org/10823175/diff/3008/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:32: int number_of_frames,
On 2012/08/07 23:49:24, DaleCurtis wrote:
> On 2012/08/07 23:23:41, scherkus wrote:
> > On 2012/08/07 23:12:19, DaleCurtis wrote:
> > > Do we need/want number_of_frames still? Or should Render() always fill the
> > > AudioBus object completely.
> > > 
> > > Currently, SincResampler complicates making this change by requesting
> > different
> > > amounts of frames relative to the creation size of the AudioBus. If we
fixed
> > > that though, every caller is specifying AudioBus->frames() to Render().
> > 
> > Doesn't seem too crazy -- what would the fix to SincResampler look like?
> > Creating/Wrapping AudioBuses?
> 
> There are few ways I can think of:
> - Creating/wrapping AudioBus objects as you mention. E.g., create one, use
> WrapVector() on the first one with different frame sizes. Has the bonus of
> allowing one less memcpy per cycle like the existing implementation.
>  - Recreate an AudioBus every ProvideInput call (what WebKit's
> MultiChannelResampler does).
> - Have AudioBus offer a SetFrames()/MaxFrames() method which allows downward
> capping of AudioBus->frames(). Alternatively, a Resize() method which just
caps
> frames_ on downsizing/same-sizing and reallocates on upsizing.
> - Fix SincResampler to always request the same amount of data (difficult I
> think, maybe crogers would know better).
> 
> Buffering inside MCR is out since it would need to sync with some sort of 
> flush() API.

The first option sounds OK -- but we can tackle that in a future CL.

[DaleCurtis, 2012-08-08 01:26:07]

PTAL. Removing number_of_frames was actually pretty easy.

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_device_th...
File media/audio/audio_device_thread.cc (right):

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_device_th...
media/audio/audio_device_thread.cc:200: audio_bus_ =
AudioBus::Create(audio_parameters_);
On 2012/08/08 00:49:01, scherkus wrote:
> On 2012/08/07 23:12:19, DaleCurtis wrote:
> > Coming back to my Q on the other CL, if we wrapped the shared memory here
> > instead of creating a new AudioBus, we can drop the memcpy that will replace
> > InterleaveFloatToInt() in AudioOutputDevice.
> 
> That seems like a worthwhile TODO / follow-up CL -- avoiding copies are a nice
> win as long as it doesn't horribly complicate things

Should be relatively simple. I'll add a TODO.

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_output_de...
File media/audio/audio_output_device_unittest.cc (right):

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_output_de...
media/audio/audio_output_device_unittest.cc:92: void ZeroAudioData(int
number_of_frames, AudioBus* audio_bus) {
On 2012/08/08 00:49:01, scherkus wrote:
> On 2012/08/08 00:17:58, DaleCurtis wrote:
> > On 2012/08/07 23:23:41, scherkus wrote:
> > > if this was an ACTION_P2() you can get rid of all of the Invoke<> stuff in
> the
> > > test
> > 
> > Actually, I'm not sure I understand what that ZeroAudioData is for. The
> comment
> > states it's being invoked with 0 to let valgrind errors through. So it seems
> to
> > do nothing? I confirmed it is indeed passing 0, so maybe just remove
instead?
> 
> Strange indeed! I'm not sure what's going on here either -- run git blame +
ping
> the author?

tommi just added this file I think (git blame confirms). tommi?

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

http://codereview.chromium.org/10823175/diff/3008/media/audio/audio_util.h#ne...
media/audio/audio_util.h:89: MEDIA_EXPORT void InterleaveFloatToInt(const
AudioBus& audio_bus,
On 2012/08/08 00:49:01, scherkus wrote:
> On 2012/08/07 23:49:24, DaleCurtis wrote:
> > On 2012/08/07 23:23:41, scherkus wrote:
> > > any reason this can't be AudioBus*?
> > 
> > It could be, but this way enforces that the AudioBus is read only and is
> > style-guide kosher.
> 
> But so does "const AudioBus*"!

I didn't realize that was acceptable, done.

http://codereview.chromium.org/10823175/diff/3008/media/base/audio_renderer_s...
File media/base/audio_renderer_sink.h (right):

http://codereview.chromium.org/10823175/diff/3008/media/base/audio_renderer_s...
media/base/audio_renderer_sink.h:32: int number_of_frames,
On 2012/08/08 00:49:01, scherkus wrote:
> On 2012/08/07 23:49:24, DaleCurtis wrote:
> > On 2012/08/07 23:23:41, scherkus wrote:
> > > On 2012/08/07 23:12:19, DaleCurtis wrote:
> > > > Do we need/want number_of_frames still? Or should Render() always fill
the
> > > > AudioBus object completely.
> > > > 
> > > > Currently, SincResampler complicates making this change by requesting
> > > different
> > > > amounts of frames relative to the creation size of the AudioBus. If we
> fixed
> > > > that though, every caller is specifying AudioBus->frames() to Render().
> > > 
> > > Doesn't seem too crazy -- what would the fix to SincResampler look like?
> > > Creating/Wrapping AudioBuses?
> > 
> > There are few ways I can think of:
> > - Creating/wrapping AudioBus objects as you mention. E.g., create one, use
> > WrapVector() on the first one with different frame sizes. Has the bonus of
> > allowing one less memcpy per cycle like the existing implementation.
> >  - Recreate an AudioBus every ProvideInput call (what WebKit's
> > MultiChannelResampler does).
> > - Have AudioBus offer a SetFrames()/MaxFrames() method which allows downward
> > capping of AudioBus->frames(). Alternatively, a Resize() method which just
> caps
> > frames_ on downsizing/same-sizing and reallocates on upsizing.
> > - Fix SincResampler to always request the same amount of data (difficult I
> > think, maybe crogers would know better).
> > 
> > Buffering inside MCR is out since it would need to sync with some sort of 
> > flush() API.
> 
> The first option sounds OK -- but we can tackle that in a future CL.

Actually it was pretty easy to do, so the next patch set has this change. Take a
look.

[scherkus (not reviewing), 2012-08-08 01:33:38]

lgtm++

[Chris Rogers, 2012-08-08 22:24:00]

LGTM

Hi Dale, thanks for doing this, I can tell it must have been pretty tedious :) 
It's really nice to see all the places where this simplifies the code - I think
we'll be seeing more benefits of AudioBus as time goes on!

After having looked carefully I could see no problems, but just to be on the
safe side, it might be good to have someone from WebRTC verify everything still
works.

[DaleCurtis, 2012-08-09 19:20:22]

I ran some of the WebRTC demos and everything seems to be working fine. hernika,
tommi: Any last comments?

[henrika (OOO until Aug 14), 2012-08-09 19:48:59]

LGTM. 

Please run the WebRTC tests in content_unittests as well and ensure that the
audio is OK.

Just an idea. Would it be a bad design to add some more helper methods to
AudioBus? I am thinking of the conversions we (=WebRTC) do today where we go
from float to interleaved 16-bit integer. And perhaps the reverse. Or do you
prefer to keep these separated?

[DaleCurtis, 2012-08-09 20:27:59]

On 2012/08/09 19:48:59, henrika wrote:
> LGTM. 
> 
> Please run the WebRTC tests in content_unittests as well and ensure that the
> audio is OK.
> 
> Just an idea. Would it be a bad design to add some more helper methods to
> AudioBus? I am thinking of the conversions we (=WebRTC) do today where we go
> from float to interleaved 16-bit integer. And perhaps the reverse. Or do you
> prefer to keep these separated?

All tests pass and Audio is fine AFAICT. CQ'ing.

I don't think that's unreasonable, audio_util is kind of a mess anyways :) Once
we get all this stuff settled, I'll look into creating optimized versions of
those methods using intrinsics.

[commit-bot: I haz the power, 2012-08-09 20:28:25]

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

[commit-bot: I haz the power, 2012-08-09 21:48:33]

Change committed as 150906