Convert WebAudio file handlers to use AudioBus.

Convert WebAudio file handlers to use AudioBus.

Lets us remove AudioUtil::DeinterleaveAudioChannel().  Modifies
AudioBus to add a new FromInterleavedPartial() function which
allows for streaming deinterleave.  Also adds supporting method:
ZeroFramesPartial().

BUG=114700, 120319
TEST=unittests + WebAudio test page.


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

[DaleCurtis, 2012-08-24 00:15:02]

PTAL.

[Chris Rogers, 2012-08-24 20:49:28]

Looks great overall.

https://chromiumcodereview.appspot.com/10871051/diff/10003/media/base/audio_b...
File media/base/audio_bus.cc (right):

https://chromiumcodereview.appspot.com/10871051/diff/10003/media/base/audio_b...
media/base/audio_bus.cc:37: int frames, AudioBus* dest) {
how about some DCHECKS, including negative values for start_frame and frames.

https://chromiumcodereview.appspot.com/10871051/diff/10003/media/base/audio_b...
media/base/audio_bus.cc:165: DCHECK_LE(start_frame + frames, frames_);
What about checking for start_frame < 0 or frames < 0?

This is why I don't like using signed integers for things which should be
unsigned...

https://chromiumcodereview.appspot.com/10871051/diff/10003/media/filters/audi...
File media/filters/audio_file_reader.h (right):

https://chromiumcodereview.appspot.com/10871051/diff/10003/media/filters/audi...
media/filters/audio_file_reader.h:36: // |audio_data| must be of the same size
as channels().
Please update comment

[DaleCurtis, 2012-08-24 22:23:22]

https://chromiumcodereview.appspot.com/10871051/diff/10003/media/base/audio_b...
File media/base/audio_bus.cc (right):

https://chromiumcodereview.appspot.com/10871051/diff/10003/media/base/audio_b...
media/base/audio_bus.cc:37: int frames, AudioBus* dest) {
On 2012/08/24 20:49:28, Chris Rogers wrote:
> how about some DCHECKS, including negative values for start_frame and frames.
> 

Not here, the DCHECKS are in the class methods.  Eventually I'll add a benchmark
for this and having DCHECKs in here hurts performance unless built with
branding=Chrome (since local Release builds leave DCHECKS in, just disabled for
use with --enable-dcheck).

https://chromiumcodereview.appspot.com/10871051/diff/10003/media/base/audio_b...
media/base/audio_bus.cc:165: DCHECK_LE(start_frame + frames, frames_);
On 2012/08/24 20:49:28, Chris Rogers wrote:
> What about checking for start_frame < 0 or frames < 0?
> 
> This is why I don't like using signed integers for things which should be
> unsigned...

Yeah, it's a bit weird, but is Chrome style. I've add DCHECK(CheckOverflow()),
although maybe it should be a CHECK?

https://chromiumcodereview.appspot.com/10871051/diff/10003/media/filters/audi...
File media/filters/audio_file_reader.h (right):

https://chromiumcodereview.appspot.com/10871051/diff/10003/media/filters/audi...
media/filters/audio_file_reader.h:36: // |audio_data| must be of the same size
as channels().
On 2012/08/24 20:49:28, Chris Rogers wrote:
> Please update comment

Done.

[vrk (LEFT CHROMIUM), 2012-08-28 20:25:11]

LGTM with a smattering of minor nits!

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
File media/base/audio_bus.cc (right):

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
media/base/audio_bus.cc:106: return (start_frame >= 0 && frames >= 0 &&
total_frames > 0 &&
Remove the return bool and have this a void method like ValidateConfig? This
return statement is repeating the logic of the above DCHECKs, so
DCHECK(CheckOverflow) will never fire because one of the DCHECKs above would
have fired first.

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
media/base/audio_bus.cc:179: if (frames > 0) {
nit: reverse test and return early

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
File media/base/audio_bus_unittest.cc (right):

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
media/base/audio_bus_unittest.cc:219: // First fill the bus with dummy data.
nit: revise comment

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
media/base/audio_bus_unittest.cc:280: // Verify FromInterleavedPartial()
deinterleaves audio in correctly.
nit: drop the "in"?

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
media/base/audio_bus_unittest.cc:282: // Only deinterleave the middle four
frames.
nit: "middle four frames" was a little confusing. Maybe "middle 2 frames in each
channel"?

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/filters/audio...
File media/filters/audio_file_reader.cc (right):

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/filters/audio...
media/filters/audio_file_reader.cc:115: int channels = this->channels();
nit: any reason not to just use channels()?

[DaleCurtis, 2012-08-29 03:43:13]

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
File media/base/audio_bus.cc (right):

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
media/base/audio_bus.cc:106: return (start_frame >= 0 && frames >= 0 &&
total_frames > 0 &&
On 2012/08/28 20:25:12, Victoria Kirst wrote:
> Remove the return bool and have this a void method like ValidateConfig? This
> return statement is repeating the logic of the above DCHECKs, so
> DCHECK(CheckOverflow) will never fire because one of the DCHECKs above would
> have fired first.

I have it this way since I wasn't sure if those DCHECK(CheckOverflow()) should
be CHECKs instead.  Otherwise you're correct.  So, CHECK or DCHECK, wdyt?

[DaleCurtis, 2012-08-29 04:47:11]

All comments addressed.

crogers: Any comments?

+darin for webkit/glue/webkit_glue.gypi OWNERS stamp.

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
File media/base/audio_bus.cc (right):

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
media/base/audio_bus.cc:106: return (start_frame >= 0 && frames >= 0 &&
total_frames > 0 &&
On 2012/08/29 03:43:13, DaleCurtis wrote:
> On 2012/08/28 20:25:12, Victoria Kirst wrote:
> > Remove the return bool and have this a void method like ValidateConfig? This
> > return statement is repeating the logic of the above DCHECKs, so
> > DCHECK(CheckOverflow) will never fire because one of the DCHECKs above would
> > have fired first.
> 
> I have it this way since I wasn't sure if those DCHECK(CheckOverflow()) should
> be CHECKs instead.  Otherwise you're correct.  So, CHECK or DCHECK, wdyt?

Feels safer with CHECK, going that route.

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
media/base/audio_bus.cc:179: if (frames > 0) {
On 2012/08/28 20:25:12, Victoria Kirst wrote:
> nit: reverse test and return early

Done.

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
File media/base/audio_bus_unittest.cc (right):

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
media/base/audio_bus_unittest.cc:219: // First fill the bus with dummy data.
On 2012/08/28 20:25:12, Victoria Kirst wrote:
> nit: revise comment

Done.

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
media/base/audio_bus_unittest.cc:280: // Verify FromInterleavedPartial()
deinterleaves audio in correctly.
On 2012/08/28 20:25:12, Victoria Kirst wrote:
> nit: drop the "in"?

Done.

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/base/audio_bu...
media/base/audio_bus_unittest.cc:282: // Only deinterleave the middle four
frames.
On 2012/08/28 20:25:12, Victoria Kirst wrote:
> nit: "middle four frames" was a little confusing. Maybe "middle 2 frames in
each
> channel"?

Done.

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/filters/audio...
File media/filters/audio_file_reader.cc (right):

https://chromiumcodereview.appspot.com/10871051/diff/2012/media/filters/audio...
media/filters/audio_file_reader.cc:115: int channels = this->channels();
On 2012/08/28 20:25:12, Victoria Kirst wrote:
> nit: any reason not to just use channels()?

Done.

[Chris Rogers, 2012-08-29 05:16:19]

Thanks Dale, looking good!  I'll have one last look tomorrow morning...

https://chromiumcodereview.appspot.com/10871051/diff/20001/media/base/audio_b...
File media/base/audio_bus.cc (right):

https://chromiumcodereview.appspot.com/10871051/diff/20001/media/base/audio_b...
media/base/audio_bus.cc:99: int sum = start_frame + frames;
It looks like if sum overflows then it becomes negative.  It seems like the
for() loop in FromInterleavedInternal() can handle this.  But, it's still
anomalous and we can CHECK (or at least DCHECK) since it's not supposed to
happen.

[Chris Rogers, 2012-08-30 00:39:44]

LGTM - thanks Dale!

[commit-bot: I haz the power, 2012-08-30 00:44:32]

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

[commit-bot: I haz the power, 2012-08-30 00:44:39]

Presubmit check for 10871051-20001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit

Presubmit checks took 1.1s to calculate.

[DaleCurtis, 2012-08-30 00:46:15]

On 2012/08/30 00:44:39, I haz the power (commit-bot) wrote:
> Presubmit check for 10871051-20001 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     webkit
> 
> Presubmit checks took 1.1s to calculate.

Doh, I forgot about that change. @darin: Can you PTAL at the gypi change?

[darin (slow to review), 2012-08-30 04:53:53]

LGTM

[commit-bot: I haz the power, 2012-08-30 06:44:01]

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

[commit-bot: I haz the power, 2012-08-30 09:03:25]

Change committed as 154099