Make sure that DecodeAudioFileData() always returns an AudioBus of the exact length of the file

Make sure that DecodeAudioFileData() always returns an AudioBus of the exact length of the file

BUG=none
TEST=extensive manual testing, two Web Audio layout tests for decoding need to be re-baselined
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=161914

[DaleCurtis, 2012-10-12 22:42:02]

https://codereview.chromium.org/11137005/diff/5001/media/filters/audio_file_r...
File media/filters/audio_file_reader.cc (right):

https://codereview.chromium.org/11137005/diff/5001/media/filters/audio_file_r...
media/filters/audio_file_reader.cc:122: DCHECK_EQ(audio_bus->channels(),
channels());
Style violation: Remove DCHECK() since this is handled by the if statement. Feel
free to replace with a DVLOG(1) or DLOG(ERROR) if you want.

https://codereview.chromium.org/11137005/diff/5001/webkit/media/audio_decoder.cc
File webkit/media/audio_decoder.cc (right):

https://codereview.chromium.org/11137005/diff/5001/webkit/media/audio_decoder...
webkit/media/audio_decoder.cc:43: size_t number_of_frames =
static_cast<size_t>(reader.number_of_frames());
Unnecessary static_cast.

https://codereview.chromium.org/11137005/diff/5001/webkit/media/audio_decoder...
webkit/media/audio_decoder.cc:69: size_t actual_frames =
static_cast<size_t>(reader.Read(audio_bus.get()));
This only works for down sizing and not up sizing since Read() will stop when
audio_bus->frames() are filled. E.g. if the frame estimate returned is too
small, you'll still lose frames.

FYI, if someone tries to load 2GB or 4GB files via this API on 32bit platforms
we may not OOM, but will hit a CHECK() failure in AudioBus::WrapVector(). Should
probably run a test to see what happens with a 2GB/4GB+ clip.

https://codereview.chromium.org/11137005/diff/5001/webkit/media/audio_decoder...
webkit/media/audio_decoder.cc:74: DCHECK_LE(actual_frames, number_of_frames);
2 space indent.

[Chris Rogers, 2012-10-12 23:22:29]

Note: before landing this requires WebKit patch:
https://bugs.webkit.org/show_bug.cgi?id=99215

https://codereview.chromium.org/11137005/diff/5001/media/filters/audio_file_r...
File media/filters/audio_file_reader.cc (right):

https://codereview.chromium.org/11137005/diff/5001/media/filters/audio_file_r...
media/filters/audio_file_reader.cc:122: DCHECK_EQ(audio_bus->channels(),
channels());
This isn't part of this CL

On 2012/10/12 22:42:02, DaleCurtis wrote:
> Style violation: Remove DCHECK() since this is handled by the if statement.
Feel
> free to replace with a DVLOG(1) or DLOG(ERROR) if you want.

https://codereview.chromium.org/11137005/diff/5001/webkit/media/audio_decoder.cc
File webkit/media/audio_decoder.cc (right):

https://codereview.chromium.org/11137005/diff/5001/webkit/media/audio_decoder...
webkit/media/audio_decoder.cc:43: size_t number_of_frames =
static_cast<size_t>(reader.number_of_frames());
On 2012/10/12 22:42:02, DaleCurtis wrote:
> Unnecessary static_cast.

Done.

https://codereview.chromium.org/11137005/diff/5001/webkit/media/audio_decoder...
webkit/media/audio_decoder.cc:69: size_t actual_frames =
static_cast<size_t>(reader.Read(audio_bus.get()));
On 2012/10/12 22:42:02, DaleCurtis wrote:
> This only works for down sizing and not up sizing since Read() will stop when
> audio_bus->frames() are filled. E.g. if the frame estimate returned is too
> small, you'll still lose frames.

Added TODO that this case is still not handled.  The current CL is still
worthwhile, since it at least gets us the correct size when down-sizing, which
happens a lot.

> 
> FYI, if someone tries to load 2GB or 4GB files via this API on 32bit platforms
> we may not OOM, but will hit a CHECK() failure in AudioBus::WrapVector().
Should
> probably run a test to see what happens with a 2GB/4GB+ clip.

https://codereview.chromium.org/11137005/diff/5001/webkit/media/audio_decoder...
webkit/media/audio_decoder.cc:74: DCHECK_LE(actual_frames, number_of_frames);
On 2012/10/12 22:42:02, DaleCurtis wrote:
> 2 space indent.

Done.

[DaleCurtis, 2012-10-13 01:10:35]

https://codereview.chromium.org/11137005/diff/5001/media/filters/audio_file_r...
File media/filters/audio_file_reader.cc (right):

https://codereview.chromium.org/11137005/diff/5001/media/filters/audio_file_r...
media/filters/audio_file_reader.cc:122: DCHECK_EQ(audio_bus->channels(),
channels());
On 2012/10/12 23:22:29, Chris Rogers wrote:
> This isn't part of this CL
> 
> On 2012/10/12 22:42:02, DaleCurtis wrote:
> > Style violation: Remove DCHECK() since this is handled by the if statement.
> Feel
> > free to replace with a DVLOG(1) or DLOG(ERROR) if you want.
> 

I know, but it's a good idea to fix these things if they're small when seen.

https://codereview.chromium.org/11137005/diff/5001/webkit/media/audio_decoder.cc
File webkit/media/audio_decoder.cc (right):

https://codereview.chromium.org/11137005/diff/5001/webkit/media/audio_decoder...
webkit/media/audio_decoder.cc:43: size_t number_of_frames =
static_cast<size_t>(reader.number_of_frames());
On 2012/10/12 23:22:29, Chris Rogers wrote:
> On 2012/10/12 22:42:02, DaleCurtis wrote:
> > Unnecessary static_cast.
> 
> Done.

Doesn't look changed in the latest patch set?

https://codereview.chromium.org/11137005/diff/8002/media/filters/audio_file_r...
File media/filters/audio_file_reader.h (right):

https://codereview.chromium.org/11137005/diff/8002/media/filters/audio_file_r...
media/filters/audio_file_reader.h:42: // This corresponds to the actual file
length.
I'd remove this sentence since it's not correct as the file may be longer.

[DaleCurtis, 2012-10-13 01:11:06]

Oh, lgtm as well % nits.