Implement WebKitClientImpl::loadAudioResource() to decode in-memory audio fil...

Implement WebKitClientImpl::loadAudioResource() to decode in-memory audio file data for use by WebKit.
Most of the interesting low-level code is being added in the media directory.

BUG=NONE
TEST=NONE (tested locally with web audio API loading files of format .wav .aif .mp3 .m4a 16bit 24bit
           In the longer term, WebKit layout tests will comprehensively exercise this code)

Committed on behalf of Chris Rogers: http://src.chromium.org/viewvc/chrome?view=rev&revision=69458

[Chris Rogers, 2010-12-08 01:48:34]

The code has been tested on a pretty wide variety of audio files, including
.mp3, .m4a, .wav, .aif, both 16 and 24 bit (for testing .aif and 24bit I had to
locally enable a few more features in FFMPEG - not part of this CL).  This
testing was done by running some of my most demanding javascript examples with
the web audio API.

[scherkus (not reviewing), 2010-12-09 17:35:40]

there's a bunch of style nits but I wanted to send out the high level comments
first and then I'll do another pass for style

but so far looking good!  thanks for doing this!

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader.cc
File media/filters/audio_file_reader.cc (right):

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.cc:28: size_t
AudioFileReader::NumberOfChannels() const {
I think it'd be best to not do type conversion here but rather in the glue layer

this means more or less copying FFmpeg's types for these accessors:
channels -> int
sample_rate -> int
duration -> int64
length -> int64

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.cc:37: return 1e-6 * format_context_->duration;
what are we converting here to/from?  what are the units of time duration is
specified in?

in general when working with FFmpeg durations and time units you should use
av_rescale_q

const AVRational av_time_base = {1, AV_TIME_BASE};

take a look at ConvertTimestamp() in src/media/ffmpeg/ffmpeg_util.cc

general best practice is to use TimeDelta which is a self-documenting unit of
time, then in the glue layer simply call InSecondsF()

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.cc:152: size_t sample_byte_size =
codec_context_->sample_fmt == AV_SAMPLE_FMT_S32
use av_get_bits_per_sample_fmt() and divide by 8 -- or right shift by 3 :)

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.cc:163: for (unsigned channelIndex = 0;
channelIndex < number_of_channels;
I can't remember if FFmpeg offers any sort of utility function like this, but in
any case I'd recommend extracting this and putting it under
src/media/audio/audio_util.h/cc

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader.h
File media/filters/audio_file_reader.h (right):

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.h:30: int Open();
what's the result of this operation?

it looks like the callee simply treats it as a bool -- perhaps we're better off
returning true/false since we don't want to leak FFmpeg error codes to the
callee

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.h:40: size_t NumberOfChannels() const;
these look like simple accessors and should be named:
channels()
sample_rate()
duration()
length()

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.h:43: size_t Length() const;
what is length computing?

judging by the call perhaps this should be renamed frames or number_of_frames

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.h:56: explicit InMemoryDataReader(const char*
data, int64 size);
no need for explicit

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.h:72: class InMemoryAudioFileReader : public
AudioFileReader {
will we ever have different AudioFileReaders?

I'm thinking we can either:
 1) Merge this class back into AudioFileReader (and get rid of the
constructor/SetProtocol)
 2) Remove this class, leave AudioFileReader as is and change
DecodeAudioFileData to instantiate the InMemoryDataReader + pass it into
AudioFileReader

[fbarchard, 2010-12-09 21:11:02]

note that Chrome support 32 bit float by converting immediately to int32.

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader.cc
File media/filters/audio_file_reader.cc (right):

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.cc:37: return 1e-6 * format_context_->duration;
1e-6 is obscure.

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.cc:152: size_t sample_byte_size =
codec_context_->sample_fmt == AV_SAMPLE_FMT_S32
You missed 8 bit.

[Chris Rogers, 2010-12-10 23:34:00]

Thanks guys, I think I've addressed all of your comments.

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader.cc
File media/filters/audio_file_reader.cc (right):

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.cc:28: size_t
AudioFileReader::NumberOfChannels() const {
On 2010/12/09 17:35:40, scherkus wrote:
> I think it'd be best to not do type conversion here but rather in the glue
layer
> 
> this means more or less copying FFmpeg's types for these accessors:
> channels -> int
> sample_rate -> int
> duration -> int64
> length -> int64

Done.

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.cc:37: return 1e-6 * format_context_->duration;
Thanks for the tip - I've switched the code over to use ConvertTimestamp()

On 2010/12/09 17:35:40, scherkus wrote:
> what are we converting here to/from?  what are the units of time duration is
> specified in?
> 
> in general when working with FFmpeg durations and time units you should use
> av_rescale_q
> 
> const AVRational av_time_base = {1, AV_TIME_BASE};
> 
> take a look at ConvertTimestamp() in src/media/ffmpeg/ffmpeg_util.cc
> 
> general best practice is to use TimeDelta which is a self-documenting unit of
> time, then in the glue layer simply call InSecondsF()

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.cc:37: return 1e-6 * format_context_->duration;
This is addressed by switching the code over to use CovertTimestamp()
On 2010/12/09 21:11:02, fbarchard wrote:
> 1e-6 is obscure.

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.cc:152: size_t sample_byte_size =
codec_context_->sample_fmt == AV_SAMPLE_FMT_S32
On 2010/12/09 17:35:40, scherkus wrote:
> use av_get_bits_per_sample_fmt() and divide by 8 -- or right shift by 3 :)

Done.

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.cc:152: size_t sample_byte_size =
codec_context_->sample_fmt == AV_SAMPLE_FMT_S32
On 2010/12/09 17:35:40, scherkus wrote:
> use av_get_bits_per_sample_fmt() and divide by 8 -- or right shift by 3 :)

Done.

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.cc:163: for (unsigned channelIndex = 0;
channelIndex < number_of_channels;
I've created a utility function called DeinterleaveAudioChannel() and put it in
audio_util.h/cc

On 2010/12/09 17:35:40, scherkus wrote:
> I can't remember if FFmpeg offers any sort of utility function like this, but
in
> any case I'd recommend extracting this and putting it under
> src/media/audio/audio_util.h/cc

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader.h
File media/filters/audio_file_reader.h (right):

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.h:30: int Open();
You're right, returning bool is better - switched to that.

On 2010/12/09 17:35:40, scherkus wrote:
> what's the result of this operation?
> 
> it looks like the callee simply treats it as a bool -- perhaps we're better
off
> returning true/false since we don't want to leak FFmpeg error codes to the
> callee

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.h:40: size_t NumberOfChannels() const;
On 2010/12/09 17:35:40, scherkus wrote:
> these look like simple accessors and should be named:
> channels()
> sample_rate()
> duration()
> length()

Done.

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.h:43: size_t Length() const;
changed method name to number_of_frames()

On 2010/12/09 17:35:40, scherkus wrote:
> what is length computing?
> 
> judging by the call perhaps this should be renamed frames or number_of_frames

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.h:56: explicit InMemoryDataReader(const char*
data, int64 size);
On 2010/12/09 17:35:40, scherkus wrote:
> no need for explicit

Done.

http://codereview.chromium.org/5550006/diff/1/media/filters/audio_file_reader...
media/filters/audio_file_reader.h:72: class InMemoryAudioFileReader : public
AudioFileReader {
I've switched the code to use option (2).  Strangely enough, I used to have the
code using this approach...

On 2010/12/09 17:35:40, scherkus wrote:
> will we ever have different AudioFileReaders?
> 
> I'm thinking we can either:
>  1) Merge this class back into AudioFileReader (and get rid of the
> constructor/SetProtocol)
>  2) Remove this class, leave AudioFileReader as is and change
> DecodeAudioFileData to instantiate the InMemoryDataReader + pass it into
> AudioFileReader

[Chris Rogers, 2010-12-10 23:37:35]

Sorry, last patch forgot to add the two new modified files:
media/audio/audio_util.cc
media/audio/audio_util.h

[scherkus (not reviewing), 2010-12-11 02:31:12]

this is looking really good!

lots of pedantic style nits

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

http://codereview.chromium.org/5550006/diff/20001/media/audio/audio_util.h#ne...
media/audio/audio_util.h:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
copyright year

http://codereview.chromium.org/5550006/diff/20001/media/audio/audio_util.h#ne...
media/audio/audio_util.h:61: AVSampleFormat sample_fmt,
similar to the other functions you should remove the dependency on FFmpeg types
and instead use "int bytes_per_sample" where:
  1 == 8 bits
  2 == 16 bit signed (little endian)
  4 == 32 bit signed (little endian)

Refer to the comment at the top of the file where it says these functions must
support these audio formats

http://codereview.chromium.org/5550006/diff/20001/media/audio/audio_util.h#ne...
media/audio/audio_util.h:62: size_t number_of_channels,
for the sake of API consistency I'd recommend we rename this "int channels"

same goes with channel_index and number_of_frames... just go with int

unsigned/size_t tends to propagate throughout the code (like const!) and isn't
needed in most cases

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
File media/filters/audio_file_reader.cc (right):

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:20: : protocol_(protocol),
indent this by extra 2 spaces

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:62: DLOG(WARNING)
de-indent by 2 spaces

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:95: return false;
I'd flip this condition around and simply exit early to avoid nesting

if (!codec_context_)
  return false

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:102: if (codec_context_ && codec_) {
nit: the rest of this file seems to not include { } on one-line if statements

I don't prefer either way but consistency is all good in my book so let's remove
the { }!!!

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:109: av_close_input_file(format_context_);
de-indent by 2 spaces

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:116: unsigned number_of_channels =
channels();
we haven't used unsigned anywhere in the media code, but when it calls for it
(as it does in this case as you're comparing it to std::vector::size()), just
use size_t

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:127: int16* output_buffer =
crazy ffmpeg stuff: you must use av_malloc()!!

otherwise things may explode for reasons I won't get into 

would also highly recommend using ScopedPtrAVFree from
media/ffmpeg/ffmpeg_common.h

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:128:
static_cast<int16*>(malloc(AVCODEC_MAX_AUDIO_FRAME_SIZE));
indent by extra 2 spaces

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:135: unsigned current_frame = 0;
unsigned -> size_t

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:138: && (result =
av_read_frame(format_context_, &avpkt)) >= 0) {
nit: && should be at end of last line + following line should be wrapped at (

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:155:
av_get_bits_per_sample_fmt(codec_context_->sample_fmt) >> 3;
indent by extra 2 spaces

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:156: unsigned frames_read = out_size /
(number_of_channels * sample_byte_size);
unsigned -> size_t

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:160: frames_read =
static_cast<int>(number_of_frames) - current_frame;
is cast needed here?  it looks like frames_read, number_of_frames, and
current_frame are all of type size_t

also remove { }

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:165: for (unsigned channel_index = 0;
channel_index < number_of_channels;
unsigned -> size_t
OR
unsinged -> int, number_of_channels -> channels()

take your pick (I think the int version will mesh with the
DeinterleaveAudioChannel() method)

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:185: return true;  // success
no need for comment

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:189: : data_(data),
indent by 2 extra spaces

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
File media/filters/audio_file_reader.h (right):

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.h:24: // Optionally, NULL may be passed in and
SetProtocal() can be called
comment needs updating

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.h:26: explicit
AudioFileReader(FFmpegURLProtocol* protocol);
can you add comment on ownership?  is AudioFileReader responsible for owning the
protocol?  should it?

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.h:59: InMemoryDataReader(const char* data, int64
size);
can you add comment on ownership?  is InMemoryDataReader responsible for owning
the data pointer?

I just get a bit nervous seeing data pointers passed around that aren't
scoped_ptr, etc :)

http://codereview.chromium.org/5550006/diff/20001/webkit/glue/media/audio_dec...
File webkit/glue/media/audio_decoder.cc (right):

http://codereview.chromium.org/5550006/diff/20001/webkit/glue/media/audio_dec...
webkit/glue/media/audio_decoder.cc:23: const char* data, size_t data_size,
double sample_rate) {
wrap these args at (

http://codereview.chromium.org/5550006/diff/20001/webkit/glue/media/audio_dec...
webkit/glue/media/audio_decoder.cc:28: // Uses the FFMPEG library for audio file
reading.
nit: FFMPEG -> FFmpeg

(crazy, I know!)

http://codereview.chromium.org/5550006/diff/20001/webkit/glue/media/audio_dec...
webkit/glue/media/audio_decoder.cc:35: unsigned number_of_channels =
reader.channels();
nit: unsigned -> size_t

http://codereview.chromium.org/5550006/diff/20001/webkit/glue/media/audio_dec...
webkit/glue/media/audio_decoder.cc:40: // TODO(crogers) : do sample-rate
conversion with FFMPEG.
ditto

http://codereview.chromium.org/5550006/diff/20001/webkit/glue/media/audio_dec...
webkit/glue/media/audio_decoder.cc:48: << " data: " << data
all these extra << lines should be indented by extra 2 spaces

http://codereview.chromium.org/5550006/diff/20001/webkit/glue/media/audio_dec...
webkit/glue/media/audio_decoder.cc:66: for (unsigned i = 0; i <
number_of_channels; ++i) {
unsigned -> size_t (or use int, etc...)

[Chris Rogers, 2010-12-13 20:25:27]

Thanks Andrew, I think I got everything.

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

http://codereview.chromium.org/5550006/diff/20001/media/audio/audio_util.h#ne...
media/audio/audio_util.h:1: // Copyright (c) 2009 The Chromium Authors. All
rights reserved.
On 2010/12/11 02:31:12, scherkus wrote:
> copyright year

Done.

http://codereview.chromium.org/5550006/diff/20001/media/audio/audio_util.h#ne...
media/audio/audio_util.h:61: AVSampleFormat sample_fmt,
On 2010/12/11 02:31:12, scherkus wrote:
> similar to the other functions you should remove the dependency on FFmpeg
types
> and instead use "int bytes_per_sample" where:
>   1 == 8 bits
>   2 == 16 bit signed (little endian)
>   4 == 32 bit signed (little endian)
> 
> Refer to the comment at the top of the file where it says these functions must
> support these audio formats

Done.

http://codereview.chromium.org/5550006/diff/20001/media/audio/audio_util.h#ne...
media/audio/audio_util.h:62: size_t number_of_channels,
On 2010/12/11 02:31:12, scherkus wrote:
> for the sake of API consistency I'd recommend we rename this "int channels"
> 
> same goes with channel_index and number_of_frames... just go with int
> 
> unsigned/size_t tends to propagate throughout the code (like const!) and isn't
> needed in most cases

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
File media/filters/audio_file_reader.cc (right):

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:20: : protocol_(protocol),
On 2010/12/11 02:31:12, scherkus wrote:
> indent this by extra 2 spaces

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:62: DLOG(WARNING)
On 2010/12/11 02:31:12, scherkus wrote:
> de-indent by 2 spaces

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:95: return false;
On 2010/12/11 02:31:12, scherkus wrote:
> I'd flip this condition around and simply exit early to avoid nesting
> 
> if (!codec_context_)
>   return false

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:102: if (codec_context_ && codec_) {
On 2010/12/11 02:31:12, scherkus wrote:
> nit: the rest of this file seems to not include { } on one-line if statements
> 
> I don't prefer either way but consistency is all good in my book so let's
remove
> the { }!!!

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:109: av_close_input_file(format_context_);
On 2010/12/11 02:31:12, scherkus wrote:
> de-indent by 2 spaces

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:116: unsigned number_of_channels =
channels();
On 2010/12/11 02:31:12, scherkus wrote:
> we haven't used unsigned anywhere in the media code, but when it calls for it
> (as it does in this case as you're comparing it to std::vector::size()), just
> use size_t

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:127: int16* output_buffer =
On 2010/12/11 02:31:12, scherkus wrote:
> crazy ffmpeg stuff: you must use av_malloc()!!
> 
> otherwise things may explode for reasons I won't get into 
> 
> would also highly recommend using ScopedPtrAVFree from
> media/ffmpeg/ffmpeg_common.h

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:128:
static_cast<int16*>(malloc(AVCODEC_MAX_AUDIO_FRAME_SIZE));
On 2010/12/11 02:31:12, scherkus wrote:
> indent by extra 2 spaces

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:135: unsigned current_frame = 0;
On 2010/12/11 02:31:12, scherkus wrote:
> unsigned -> size_t

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:138: && (result =
av_read_frame(format_context_, &avpkt)) >= 0) {
On 2010/12/11 02:31:12, scherkus wrote:
> nit: && should be at end of last line + following line should be wrapped at (

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:155:
av_get_bits_per_sample_fmt(codec_context_->sample_fmt) >> 3;
On 2010/12/11 02:31:12, scherkus wrote:
> indent by extra 2 spaces

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:156: unsigned frames_read = out_size /
(number_of_channels * sample_byte_size);
On 2010/12/11 02:31:12, scherkus wrote:
> unsigned -> size_t

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:160: frames_read =
static_cast<int>(number_of_frames) - current_frame;
On 2010/12/11 02:31:12, scherkus wrote:
> is cast needed here?  it looks like frames_read, number_of_frames, and
> current_frame are all of type size_t
> 
> also remove { } 

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:165: for (unsigned channel_index = 0;
channel_index < number_of_channels;
On 2010/12/11 02:31:12, scherkus wrote:
> unsigned -> size_t
> OR
> unsinged -> int, number_of_channels -> channels()
> 
> take your pick (I think the int version will mesh with the
> DeinterleaveAudioChannel() method)

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:185: return true;  // success
On 2010/12/11 02:31:12, scherkus wrote:
> no need for comment

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.cc:189: : data_(data),
On 2010/12/11 02:31:12, scherkus wrote:
> indent by 2 extra spaces

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
File media/filters/audio_file_reader.h (right):

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.h:24: // Optionally, NULL may be passed in and
SetProtocal() can be called
On 2010/12/11 02:31:12, scherkus wrote:
> comment needs updating

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.h:26: explicit
AudioFileReader(FFmpegURLProtocol* protocol);
On 2010/12/11 02:31:12, scherkus wrote:
> can you add comment on ownership?  is AudioFileReader responsible for owning
the
> protocol?  should it?

Done.

http://codereview.chromium.org/5550006/diff/20001/media/filters/audio_file_re...
media/filters/audio_file_reader.h:59: InMemoryDataReader(const char* data, int64
size);
On 2010/12/11 02:31:12, scherkus wrote:
> can you add comment on ownership?  is InMemoryDataReader responsible for
owning
> the data pointer?
> 
> I just get a bit nervous seeing data pointers passed around that aren't
> scoped_ptr, etc :)

Done.

http://codereview.chromium.org/5550006/diff/20001/webkit/glue/media/audio_dec...
File webkit/glue/media/audio_decoder.cc (right):

http://codereview.chromium.org/5550006/diff/20001/webkit/glue/media/audio_dec...
webkit/glue/media/audio_decoder.cc:23: const char* data, size_t data_size,
double sample_rate) {
On 2010/12/11 02:31:12, scherkus wrote:
> wrap these args at (

Done.

http://codereview.chromium.org/5550006/diff/20001/webkit/glue/media/audio_dec...
webkit/glue/media/audio_decoder.cc:28: // Uses the FFMPEG library for audio file
reading.
On 2010/12/11 02:31:12, scherkus wrote:
> nit: FFMPEG -> FFmpeg
> 
> (crazy, I know!)

Done.

http://codereview.chromium.org/5550006/diff/20001/webkit/glue/media/audio_dec...
webkit/glue/media/audio_decoder.cc:35: unsigned number_of_channels =
reader.channels();
On 2010/12/11 02:31:12, scherkus wrote:
> nit: unsigned -> size_t

Done.

http://codereview.chromium.org/5550006/diff/20001/webkit/glue/media/audio_dec...
webkit/glue/media/audio_decoder.cc:40: // TODO(crogers) : do sample-rate
conversion with FFMPEG.
On 2010/12/11 02:31:12, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/5550006/diff/20001/webkit/glue/media/audio_dec...
webkit/glue/media/audio_decoder.cc:48: << " data: " << data
On 2010/12/11 02:31:12, scherkus wrote:
> all these extra << lines should be indented by extra 2 spaces

Done.

http://codereview.chromium.org/5550006/diff/20001/webkit/glue/media/audio_dec...
webkit/glue/media/audio_decoder.cc:66: for (unsigned i = 0; i <
number_of_channels; ++i) {
On 2010/12/11 02:31:12, scherkus wrote:
> unsigned -> size_t (or use int, etc...)

Done.

[scherkus (not reviewing), 2010-12-14 20:41:48]

One FINAL thing is to include a TEST= and BUG= in your CL description

But LGTM w/ one unsigned nit

Thanks for putting up with me :)

http://codereview.chromium.org/5550006/diff/31001/media/audio/audio_util.cc
File media/audio/audio_util.cc (right):

http://codereview.chromium.org/5550006/diff/31001/media/audio/audio_util.cc#n...
media/audio/audio_util.cc:167: switch (bytes_per_sample) {
looks awesome!

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

http://codereview.chromium.org/5550006/diff/31001/media/audio/audio_util.h#ne...
media/audio/audio_util.h:63: unsigned number_of_frames);
nit: unsigned -> size_t/int (whatever makes sense)

[Chris Rogers, 2010-12-16 20:01:26]

Thanks Andrew!  I fixed the last unsigned issue, added the BUG and TEST fields
and ran through the trybots.  Ken Russell will be helping me land this one.

[scherkus (not reviewing), 2010-12-16 20:11:52]

cool!

[Ken Russell (switch to Gerrit), 2010-12-16 20:45:12]

Committed per above.