Rewriting FFmpegAudioDecoder and eliminating DecoderBase.

Rewriting FFmpegAudioDecoder and eliminating DecoderBase.

BUG=93379
TEST=media_unittests, layout tests

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

[scherkus (not reviewing), 2011-04-29 20:03:44]

seeking preliminary thoughts w.r.t. DISCUSSION POINTS marked by XXX

also let me know what's easier for review... for example I can pull some of
these changes into standalone mini-patches

[acolwell GONE FROM CHROMIUM, 2011-05-13 19:35:37]

Looking pretty good. Here are some initial thoughts.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:20: return decoded_size != 0;
do we want this to return true if decoded_size < 0?

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:26: return result &&
!input->IsEndOfStream() &&
It's ok if result < 0? Perhaps a comment indicating what result represents would
be helpful.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:36: return result == 0 && decoded_size ==
0 && input->IsEndOfStream();
result -> read_size?

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:49: if (codec_context_) {
nit:braces not necesssary

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:54: void
FFmpegAudioDecoder::Play(FilterCallback* callback) {
remove

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:60: void
FFmpegAudioDecoder::Pause(FilterCallback* callback) {
remove

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:67: // XXX: move to proper thread.
Or you could just protect this with a lock?

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:76: // XXX: anything else?
Clear consume callback so we won't send any data downstream? Keep track of
stopped state so we don't call Read() on DemuxerStream?

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:81: void
FFmpegAudioDecoder::Seek(base::TimeDelta time, FilterCallback* callback) {
remove

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:91: // XXX: do we still care about
initializing on our own message_loop_?
I'm fine with keeping a message_loop for audio decoding.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:92: // XXX: do we want to try and use
MessageLoopProxy at this time?
separate CL IMO

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:107: message_loop_->PostTask(
What about just putting this in an  "if(MessageLoop::current() ==
message_loop_)" guard here and combine ProduceAudioSamples &
DoQueueOutputBuffer?

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:114: DemuxerStream* stream,
consider moving this to Initialize() and using an  "if(MessageLoop::current() ==
message_loop_)" guard.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:149: callback->Run();
Would this be a good time to convert Initialize() to use FilterStatusCB?

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:177: decoded_audio_.reset(new
int16_t[AVCODEC_MAX_AUDIO_FRAME_SIZE / 2]);
AVCODEC_MAX_AUDIO_FRAME_SIZE in bytes. We're allocating int16. We could just
have decoded_audio_ be uint8.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:210: int decoded_audio_size =
AVCODEC_MAX_AUDIO_FRAME_SIZE;
This should be based on the size allocated. Perhaps introduce
decoded_audio_size_ member variable so that decode_audio_'s allocated size can't
get out of sync with this code.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:216: // XXX: this is only for WMAPro audio
codec?! remove?!
I'm fine with removing this since I don't think we want to encourage WMAPro
usage.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:270: pending_reads_++;
Add stopped check to make sure we don't call the demuxer stream if we have
received a Stop().

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:281: NewRunnableMethod(this,
&FFmpegAudioDecoder::DoDecodeInputBuffer,
Put this in an message loop guard and copy DoDecodeInputBuffer code in here?

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
File media/filters/ffmpeg_audio_decoder.h (right):

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.h:24: virtual void Play(FilterCallback*
callback);
Remove because it isn't used. I envision AudioDecoder not deriving from Filter
in a not too distant future so I don't want to add the methods here.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.h:25: virtual void Pause(FilterCallback*
callback);
ditto

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.h:28: virtual void Seek(base::TimeDelta time,
FilterCallback* callback);
ditto

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.h:37: void DoInitialize(DemuxerStream*
stream, FilterCallback* callback,
Add comments for methods.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.h:52: DemuxerStream* demuxer_stream_;
scoped_refptr?

[scherkus (not reviewing), 2011-09-11 14:49:22]

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:20: return decoded_size != 0;
On 2011/05/13 19:35:37, acolwell wrote:
> do we want this to return true if decoded_size < 0?

Hmm... yeah these helper methods are a bit weird since they're called in order
but I agree I'll make them more robust to calling order (generally a good thing)

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:26: return result &&
!input->IsEndOfStream() &&
On 2011/05/13 19:35:37, acolwell wrote:
> It's ok if result < 0? Perhaps a comment indicating what result represents
would
> be helpful.

Done.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:49: if (codec_context_) {
On 2011/05/13 19:35:37, acolwell wrote:
> nit:braces not necesssary

Done.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:54: void
FFmpegAudioDecoder::Play(FilterCallback* callback) {
On 2011/05/13 19:35:37, acolwell wrote:
> remove

Done.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:60: void
FFmpegAudioDecoder::Pause(FilterCallback* callback) {
On 2011/05/13 19:35:37, acolwell wrote:
> remove

Done.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:67: // XXX: move to proper thread.
On 2011/05/13 19:35:37, acolwell wrote:
> Or you could just protect this with a lock?

Done.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:81: void
FFmpegAudioDecoder::Seek(base::TimeDelta time, FilterCallback* callback) {
On 2011/05/13 19:35:37, acolwell wrote:
> remove

Done.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:107: message_loop_->PostTask(
On 2011/05/13 19:35:37, acolwell wrote:
> What about just putting this in an  "if(MessageLoop::current() ==
> message_loop_)" guard here and combine ProduceAudioSamples &
> DoQueueOutputBuffer?

I prefer the extra methods for clarity and saner debugging

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:149: callback->Run();
On 2011/05/13 19:35:37, acolwell wrote:
> Would this be a good time to convert Initialize() to use FilterStatusCB?

Follow up CL

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:177: decoded_audio_.reset(new
int16_t[AVCODEC_MAX_AUDIO_FRAME_SIZE / 2]);
On 2011/05/13 19:35:37, acolwell wrote:
> AVCODEC_MAX_AUDIO_FRAME_SIZE in bytes. We're allocating int16. We could just
> have decoded_audio_ be uint8.

Done and we'll do a reinterpret_cast<> in decode call

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:210: int decoded_audio_size =
AVCODEC_MAX_AUDIO_FRAME_SIZE;
On 2011/05/13 19:35:37, acolwell wrote:
> This should be based on the size allocated. Perhaps introduce
> decoded_audio_size_ member variable so that decode_audio_'s allocated size
can't
> get out of sync with this code.

Done.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.cc:216: // XXX: this is only for WMAPro audio
codec?! remove?!
On 2011/05/13 19:35:37, acolwell wrote:
> I'm fine with removing this since I don't think we want to encourage WMAPro
> usage.

Done.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
File media/filters/ffmpeg_audio_decoder.h (right):

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.h:24: virtual void Play(FilterCallback*
callback);
On 2011/05/13 19:35:37, acolwell wrote:
> Remove because it isn't used. I envision AudioDecoder not deriving from Filter
> in a not too distant future so I don't want to add the methods here.

Done.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.h:25: virtual void Pause(FilterCallback*
callback);
On 2011/05/13 19:35:37, acolwell wrote:
> ditto

Done.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.h:28: virtual void Seek(base::TimeDelta time,
FilterCallback* callback);
On 2011/05/13 19:35:37, acolwell wrote:
> ditto

Done.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.h:37: void DoInitialize(DemuxerStream*
stream, FilterCallback* callback,
On 2011/05/13 19:35:37, acolwell wrote:
> Add comments for methods.

Done.

http://codereview.chromium.org/6901135/diff/1/media/filters/ffmpeg_audio_deco...
media/filters/ffmpeg_audio_decoder.h:52: DemuxerStream* demuxer_stream_;
On 2011/05/13 19:35:37, acolwell wrote:
> scoped_refptr?

Done.

[acolwell GONE FROM CHROMIUM, 2011-09-13 22:09:27]

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:82: void
FFmpegAudioDecoder::ProduceAudioSamples(scoped_refptr<Buffer> buffer) {
Remove buffer from method signature since it is never used & update
AudioRenderBase.

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:110: << " channels: " <<
codec_context_->channels
Indent

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:124: << codec_context_->codec_id;
Indent

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:150: void
FFmpegAudioDecoder::DoQueueOutput(scoped_refptr<Buffer> output) {
const&

Rename to DoProduceAudioSamples().

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:151: output_buffers_.push_back(output);
Change this to a simple counter member variable since ProduceAudioSamples()
won't pass a buffer.

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:152: ReadFromDemuxerStream();
Do we want to do this if we have reached EOS?

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:155: void
FFmpegAudioDecoder::DoDecodeInput(scoped_refptr<Buffer> input) {
const&

Rename to DoOnReadComplete() to match Flush() -> DoFlush() pattern.

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:172: packet.data =
const_cast<uint8*>(input->GetData());
Mind if we make this a little more explicit for the EOS case?

if (input->IsEndOfStream()) {
    packet.data = NULL;
    packet.size = 0;
  } else {
    packet.data = const_cast<uint8*>(input->GetData());
    packet.size = input->GetDataSize();
  }

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:188: ReadFromDemuxerStream();
Do we want to issue another read if this is IsEndOfStream() == true?

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:213: DCHECK_GT(output_buffers_.size(),
0u);
Replace w/ counter check and decrement.

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:247:
output->SetTimestamp(input->GetTimestamp());
Is this right given that there can be a codec delay where the first packet can
be passed in and not return any audio. When the second packet is passed in audio
comes out. Wouldn't this put the wrong timestamp on the data in this case?

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:249: return;
remove return.

Something still doesn't smell right in this function. If we hit the
IsResultError() if block in DoDecodeInput() won't this code start messing up the
timestamps?

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:263: double microseconds = size /
why do we convert to a double and then back to int64 here?

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
File media/filters/ffmpeg_audio_decoder.h (right):

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.h:25: virtual void Flush(FilterCallback*
callback);
OVERRIDE this and methods below.

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.h:30: virtual AudioDecoderConfig config();
const AudioDecoderConfig& config() const?

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
File media/filters/ffmpeg_audio_decoder_unittest.cc (right):

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder_unittest.cc:75: switch (vorbis_packet_index_)
{
Why not use an array or list?

http://codereview.chromium.org/6901135/diff/8019/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/6901135/diff/8019/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:10: #include "base/bind.h"
accidentally included?

[scherkus (not reviewing), 2011-09-16 18:10:13]

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:82: void
FFmpegAudioDecoder::ProduceAudioSamples(scoped_refptr<Buffer> buffer) {
On 2011/09/13 22:09:28, acolwell wrote:
> Remove buffer from method signature since it is never used & update
> AudioRenderBase.

Good suggestion! Deferring to later CL

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:110: << " channels: " <<
codec_context_->channels
On 2011/09/13 22:09:28, acolwell wrote:
> Indent

Done.

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:124: << codec_context_->codec_id;
On 2011/09/13 22:09:28, acolwell wrote:
> Indent

Done.

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:155: void
FFmpegAudioDecoder::DoDecodeInput(scoped_refptr<Buffer> input) {
On 2011/09/13 22:09:28, acolwell wrote:
> const&
> 
> Rename to DoOnReadComplete() to match Flush() -> DoFlush() pattern.

Done.

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:172: packet.data =
const_cast<uint8*>(input->GetData());
On 2011/09/13 22:09:28, acolwell wrote:
> Mind if we make this a little more explicit for the EOS case?
> 
> if (input->IsEndOfStream()) {
>     packet.data = NULL;
>     packet.size = 0;
>   } else {
>     packet.data = const_cast<uint8*>(input->GetData());
>     packet.size = input->GetDataSize();
>   }

Done.

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:188: ReadFromDemuxerStream();
On 2011/09/13 22:09:28, acolwell wrote:
> Do we want to issue another read if this is IsEndOfStream() == true?

Would that imply that the audio decoder in question is incorrectly reporting an
error given a flush packet of NULL/0 bytes?

I'll add a DCHECK

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:213: DCHECK_GT(output_buffers_.size(),
0u);
On 2011/09/13 22:09:28, acolwell wrote:
> Replace w/ counter check and decrement.

Future CL

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:247:
output->SetTimestamp(input->GetTimestamp());
On 2011/09/13 22:09:28, acolwell wrote:
> Is this right given that there can be a codec delay where the first packet can
> be passed in and not return any audio. When the second packet is passed in
audio
> comes out. Wouldn't this put the wrong timestamp on the data in this case?

Good catch! This behaviour is now codified in a unit test but yeah let's fix
that :)

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:263: double microseconds = size /
On 2011/09/13 22:09:28, acolwell wrote:
> why do we convert to a double and then back to int64 here? 

I looked at the original code review but I can't recall why. I think it was a
cheesy way to work around my fears of integer overflow given the numbers we are
working with

http://codereview.chromium.org/113619/

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
File media/filters/ffmpeg_audio_decoder.h (right):

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.h:25: virtual void Flush(FilterCallback*
callback);
On 2011/09/13 22:09:28, acolwell wrote:
> OVERRIDE this and methods below.

Done.

http://codereview.chromium.org/6901135/diff/8019/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.h:30: virtual AudioDecoderConfig config();
On 2011/09/13 22:09:28, acolwell wrote:
> const AudioDecoderConfig& config() const?

this is going away in follow up CL

http://codereview.chromium.org/6901135/diff/8019/webkit/glue/webmediaplayer_i...
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/6901135/diff/8019/webkit/glue/webmediaplayer_i...
webkit/glue/webmediaplayer_impl.cc:10: #include "base/bind.h"
On 2011/09/13 22:09:28, acolwell wrote:
> accidentally included?

nope -- decoder_base.h used to include it but now this is required

[Alpha Left Google, 2011-09-16 18:14:11]

Holy moly this is so cool!!! Just expression my appreciation. :)

[acolwell GONE FROM CHROMIUM, 2011-09-16 19:04:37]

LGTM. Just some const& nits for making the code consistent.

http://codereview.chromium.org/6901135/diff/17001/media/filters/ffmpeg_audio_...
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/6901135/diff/17001/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.cc:73: // scoped_refptr<>.
const scoped_refptr<>&

http://codereview.chromium.org/6901135/diff/17001/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.cc:93: scoped_refptr<DemuxerStream> stream,
const&

http://codereview.chromium.org/6901135/diff/17001/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.cc:154: void
FFmpegAudioDecoder::DoProduceAudioSamples(scoped_refptr<Buffer> output) {
const&

http://codereview.chromium.org/6901135/diff/17001/media/filters/ffmpeg_audio_...
File media/filters/ffmpeg_audio_decoder.h (right):

http://codereview.chromium.org/6901135/diff/17001/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.h:35: void
DoInitialize(scoped_refptr<DemuxerStream> stream,
const&

http://codereview.chromium.org/6901135/diff/17001/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.h:39: void
DoProduceAudioSamples(scoped_refptr<Buffer> output);
const&

http://codereview.chromium.org/6901135/diff/17001/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.h:66: uint8* decoded_audio_;  // Allocated
via av_malloc().
It would be nice if you explained, in a comment, why av_malloc() is needed now.

[scherkus (not reviewing), 2011-09-16 20:39:40]

http://codereview.chromium.org/6901135/diff/17001/media/filters/ffmpeg_audio_...
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/6901135/diff/17001/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.cc:154: void
FFmpegAudioDecoder::DoProduceAudioSamples(scoped_refptr<Buffer> output) {
On 2011/09/16 19:04:37, acolwell wrote:
> const&

Done.

http://codereview.chromium.org/6901135/diff/17001/media/filters/ffmpeg_audio_...
File media/filters/ffmpeg_audio_decoder.h (right):

http://codereview.chromium.org/6901135/diff/17001/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.h:35: void
DoInitialize(scoped_refptr<DemuxerStream> stream,
On 2011/09/16 19:04:37, acolwell wrote:
> const&

Done.

http://codereview.chromium.org/6901135/diff/17001/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.h:39: void
DoProduceAudioSamples(scoped_refptr<Buffer> output);
On 2011/09/16 19:04:37, acolwell wrote:
> const&

Done.

http://codereview.chromium.org/6901135/diff/17001/media/filters/ffmpeg_audio_...
media/filters/ffmpeg_audio_decoder.h:66: uint8* decoded_audio_;  // Allocated
via av_malloc().
On 2011/09/16 19:04:37, acolwell wrote:
> It would be nice if you explained, in a comment, why av_malloc() is needed
now.

Done.