Move audio decoder initialization to AudioRendererImpl.

Move audio decoder initialization to AudioRendererImpl.

Also add support to multiple audio decoder initializaiton.

BUG=145635
TEST=media_unittests


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

[xhwang, 2012-10-15 17:50:28]

Mostly copying acolwell's CL (10836167).

scherkus/acolwell: please review.
ddorwin: FYI

http://codereview.chromium.org/11148011/diff/2001/media/filters/audio_rendere...
File media/filters/audio_renderer_impl.cc (right):

http://codereview.chromium.org/11148011/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:196: // pipeline teardown waiting for this?
In VideoRendererBase we return directly here w/o return the pending init_cb_.
That's why I ask this question. Is that correct?

[acolwell GONE FROM CHROMIUM, 2012-10-15 21:00:06]

looks pretty good. Here are my initial comments.

http://codereview.chromium.org/11148011/diff/2001/media/base/filter_collection.h
File media/base/filter_collection.h (right):

http://codereview.chromium.org/11148011/diff/2001/media/base/filter_collectio...
media/base/filter_collection.h:53: AudioDecoderList* GetAudioDecoders();
Do we really want a list here? We don't have more than one audio decoder.

http://codereview.chromium.org/11148011/diff/2001/media/base/filter_collectio...
File media/base/filter_collection_unittest.cc (right):

http://codereview.chromium.org/11148011/diff/2001/media/base/filter_collectio...
media/base/filter_collection_unittest.cc:23: TEST_F(FilterCollectionTest,
SelectXXXMethods) {
Are these tests really worth having anymore? We keep changing which filter type
is being tested and the actual tests are pretty lame. Delete file?

http://codereview.chromium.org/11148011/diff/2001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/11148011/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:22: #include "media/base/demuxer_stream.h"
nit: why is this needed now?

http://codereview.chromium.org/11148011/diff/2001/media/filters/audio_rendere...
File media/filters/audio_renderer_impl.cc (right):

http://codereview.chromium.org/11148011/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:100: if (!callback.is_null()) {
nit: Is this actually ever called w/o a callback? If not, please remove this
conditional and add a DCHECK at the top.

http://codereview.chromium.org/11148011/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:197:
base::ResetAndReturn(&init_cb_).Run(PIPELINE_ERROR_ABORT);
No. You should not call init_cb_ here. init_cb_ should be cleared when Stop() is
called.

http://codereview.chromium.org/11148011/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:229:
init_cb_.Run(PIPELINE_ERROR_INITIALIZATION_FAILED);
nit: Change to base::ResetAndReturn(&init_cb_)

http://codereview.chromium.org/11148011/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:256: init_cb_.Run(PIPELINE_OK);
ditto

[xhwang, 2012-10-15 22:52:22]

Comments resolved w/ a new question. PTAL!

http://codereview.chromium.org/11148011/diff/2001/media/base/filter_collection.h
File media/base/filter_collection.h (right):

http://codereview.chromium.org/11148011/diff/2001/media/base/filter_collectio...
media/base/filter_collection.h:53: AudioDecoderList* GetAudioDecoders();
On 2012/10/15 21:00:07, acolwell wrote:
> Do we really want a list here? We don't have more than one audio decoder.

The reason I work on this CL now is to be prepared for adding another audio
decoder: DecryptingAudioDecoder.

http://codereview.chromium.org/11148011/diff/2001/media/base/filter_collectio...
File media/base/filter_collection_unittest.cc (right):

http://codereview.chromium.org/11148011/diff/2001/media/base/filter_collectio...
media/base/filter_collection_unittest.cc:23: TEST_F(FilterCollectionTest,
SelectXXXMethods) {
On 2012/10/15 21:00:07, acolwell wrote:
> Are these tests really worth having anymore? We keep changing which filter
type
> is being tested and the actual tests are pretty lame. Delete file?

I am totally fine with removing them. But I'd wait for scherkus's opinion before
doing that.

http://codereview.chromium.org/11148011/diff/2001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/11148011/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:22: #include "media/base/demuxer_stream.h"
On 2012/10/15 21:00:07, acolwell wrote:
> nit: why is this needed now?

Removed.

http://codereview.chromium.org/11148011/diff/2001/media/filters/audio_rendere...
File media/filters/audio_renderer_impl.cc (right):

http://codereview.chromium.org/11148011/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:100: if (!callback.is_null()) {
On 2012/10/15 21:00:07, acolwell wrote:
> nit: Is this actually ever called w/o a callback? If not, please remove this
> conditional and add a DCHECK at the top.

Done.

http://codereview.chromium.org/11148011/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:197:
base::ResetAndReturn(&init_cb_).Run(PIPELINE_ERROR_ABORT);
On 2012/10/15 21:00:07, acolwell wrote:
> No. You should not call init_cb_ here. init_cb_ should be cleared when Stop()
is
> called.

Done. Also added init_cb_.Reset() in Stop().

http://codereview.chromium.org/11148011/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:229:
init_cb_.Run(PIPELINE_ERROR_INITIALIZATION_FAILED);
On 2012/10/15 21:00:07, acolwell wrote:
> nit: Change to base::ResetAndReturn(&init_cb_)

Done.

http://codereview.chromium.org/11148011/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:256: init_cb_.Run(PIPELINE_OK);
On 2012/10/15 21:00:07, acolwell wrote:
> ditto

Done.

http://codereview.chromium.org/11148011/diff/5004/media/filters/audio_rendere...
File media/filters/audio_renderer_impl.cc (right):

http://codereview.chromium.org/11148011/diff/5004/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:106: callback.Run();
I found AudioDecoder doesn't have a Stop() method. So we don't need to stop the
audio decoder anywhere during teardown? What if the audio decoder has a pending
demuxer read? Are we relying on the fact that the demuxer is shut down before we
stopping the audio renderer? Or are we relying on the fact that AudioDecoder is
ref-counted, so the audio decoder will not be destroyed before all pending
callbacks are executed. When we add DecryptingAudioDecoder later, it will not
only has pending demuxer reads, but also has pending decrypt callbacks. I need
to make sure everything still work :)

[acolwell GONE FROM CHROMIUM, 2012-10-16 16:23:38]

LGTM

http://codereview.chromium.org/11148011/diff/5004/media/filters/audio_rendere...
File media/filters/audio_renderer_impl.cc (right):

http://codereview.chromium.org/11148011/diff/5004/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:106: callback.Run();
On 2012/10/15 22:52:23, xhwang wrote:
> I found AudioDecoder doesn't have a Stop() method. So we don't need to stop
the
> audio decoder anywhere during teardown? What if the audio decoder has a
pending
> demuxer read? Are we relying on the fact that the demuxer is shut down before
we
> stopping the audio renderer? Or are we relying on the fact that AudioDecoder
is
> ref-counted, so the audio decoder will not be destroyed before all pending
> callbacks are executed. When we add DecryptingAudioDecoder later, it will not
> only has pending demuxer reads, but also has pending decrypt callbacks. I need
> to make sure everything still work :)

I was a little surprised to see that the decode interface didn't have a Stop()
method myself. I believe this code is relying on the fact that the demuxer is
stopped before the renderer is and when that Stop() happens, the pending reads
are cancelled. By the time this Stop() call happens I believe there shouldn't be
any reads pending or if there are they will have kAborted status.

[scherkus (not reviewing), 2012-10-18 00:30:28]

lgtm w/ nits

Sadly I had coded this a while ago but hadn't landed:
http://codereview.chromium.org/10918022/

fischman@ and I got hung up on whether we wanted to support multiple audio
decoders in world where only FFmpegAudioDecoder exists but had forgot about
decryption.

http://codereview.chromium.org/11148011/diff/5004/media/filters/audio_rendere...
File media/filters/audio_renderer_impl.cc (right):

http://codereview.chromium.org/11148011/diff/5004/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:198: if (state_ == kStopped)
also check stopped_?

http://codereview.chromium.org/11148011/diff/5004/media/filters/audio_rendere...
File media/filters/audio_renderer_impl_unittest.cc (right):

http://codereview.chromium.org/11148011/diff/5004/media/filters/audio_rendere...
media/filters/audio_renderer_impl_unittest.cc:244:
.WillOnce(RunPipelineStatusCB1(PIPELINE_OK));
indent

[scherkus (not reviewing), 2012-10-18 00:49:53]

my CL had some additional cleanup which I salvaged + rebased ontop of this CL:
https://codereview.chromium.org/11183048/

[xhwang, 2012-10-18 01:35:02]

nits resolved

http://codereview.chromium.org/11148011/diff/5004/media/filters/audio_rendere...
File media/filters/audio_renderer_impl.cc (right):

http://codereview.chromium.org/11148011/diff/5004/media/filters/audio_rendere...
media/filters/audio_renderer_impl.cc:198: if (state_ == kStopped)
On 2012/10/18 00:30:28, scherkus wrote:
> also check stopped_?

Done.

http://codereview.chromium.org/11148011/diff/5004/media/filters/audio_rendere...
File media/filters/audio_renderer_impl_unittest.cc (right):

http://codereview.chromium.org/11148011/diff/5004/media/filters/audio_rendere...
media/filters/audio_renderer_impl_unittest.cc:244:
.WillOnce(RunPipelineStatusCB1(PIPELINE_OK));
On 2012/10/18 00:30:28, scherkus wrote:
> indent

Done.

[commit-bot: I haz the power, 2012-10-18 03:37:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/11148011/29003

[commit-bot: I haz the power, 2012-10-18 07:02:46]

Change committed as 162658