Remove AudioDecoder from the Filter heirarchy.

Remove AudioDecoder from the Filter heirarchy.

BUG=108339
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120718

[acolwell GONE FROM CHROMIUM, 2012-02-03 23:23:24]

On 2012/02/03 22:46:42, Ami Fischman wrote:

WDYT about having the AudioRendererBase own the AudioDecoder and remove it from
Pipeline's awareness completely?

[scherkus (not reviewing), 2012-02-03 23:25:20]

On 2012/02/03 23:23:24, acolwell wrote:
> On 2012/02/03 22:46:42, Ami Fischman wrote:
> 
> WDYT about having the AudioRendererBase own the AudioDecoder and remove it
from
> Pipeline's awareness completely?

yes in the longer term picture but baby steps imo!

[Ami GONE FROM CHROMIUM, 2012-02-03 23:26:18]

On Fri, Feb 3, 2012 at 3:25 PM, <scherkus@chromium.org> wrote:

> On 2012/02/03 23:23:24, acolwell wrote:
>
>> On 2012/02/03 22:46:42, Ami Fischman wrote:
>>
>
>  WDYT about having the AudioRendererBase own the AudioDecoder and remove it
>>
> from
>
>> Pipeline's awareness completely?
>>
>
> yes in the longer term picture but baby steps imo!
>

Agreed on both counts.

[scherkus (not reviewing), 2012-02-03 23:31:29]

LGTM w/ some qs

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/base/audio_dec...
File media/base/audio_decoder.h (right):

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/base/audio_dec...
media/base/audio_decoder.h:24: virtual void Initialize(DemuxerStream* stream,
I thought we agreed to do const scoped_refptr<T>& ?

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/base/pipeline....
media/base/pipeline.cc:686: has_audio_ = true;
I wonder if we can replace has_audio_/has_video_ w/ ptr checks for decoders and
renderers... (maybe TODO-ify?)

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/filters/audio_...
File media/filters/audio_renderer_base.h (right):

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/filters/audio_...
media/filters/audio_renderer_base.h:45: virtual void Initialize(AudioDecoder*
decoder,
const scoped_refptr<T>& ?

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/filters/ffmpeg...
File media/filters/ffmpeg_audio_decoder.cc (left):

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.cc:71: void FFmpegAudioDecoder::Flush(const
base::Closure& callback) {
\o/

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/filters/ffmpeg...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.cc:62: // XXX: should we require Stop() to be
called? this might end up getting
can you change this to TODO(scherkus)

I use XXX as comments in the code I'm supposed to address BEFORE committing but
sometimes they slip by D:

[Ami GONE FROM CHROMIUM, 2012-02-04 00:10:21]

acolwell@ pointed out that my change orphans FFmpegAudioDecoder::DoFlush(), and
that without it being called it's likely that decoded audio samples will have
incorrect timestamps.

I'm going to take a crack at moving ownership of AudioDecoder into
AudioRendererBase (from Pipeline), to avoid having to plumb
AudioDecoder-specific code into CompositeFilter::Flush (ewww).

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/base/audio_dec...
File media/base/audio_decoder.h (right):

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/base/audio_dec...
media/base/audio_decoder.h:24: virtual void Initialize(DemuxerStream* stream,
On 2012/02/03 23:31:29, scherkus wrote:
> I thought we agreed to do const scoped_refptr<T>& ?

Not sure who 'we' is...  ffmpeg_audio_decoder.cc had this:

  // TODO(scherkus): change Initialize() signature to pass |stream| as a
  // scoped_refptr<>.

Done.

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/base/pipeline....
media/base/pipeline.cc:686: has_audio_ = true;
On 2012/02/03 23:31:29, scherkus wrote:
> I wonder if we can replace has_audio_/has_video_ w/ ptr checks for decoders
and
> renderers... (maybe TODO-ify?)

TODO'd.

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/filters/audio_...
File media/filters/audio_renderer_base.h (right):

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/filters/audio_...
media/filters/audio_renderer_base.h:45: virtual void Initialize(AudioDecoder*
decoder,
On 2012/02/03 23:31:29, scherkus wrote:
> const scoped_refptr<T>& ?

Done.

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/filters/ffmpeg...
File media/filters/ffmpeg_audio_decoder.cc (left):

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.cc:71: void FFmpegAudioDecoder::Flush(const
base::Closure& callback) {
On 2012/02/03 23:31:29, scherkus wrote:
> \o/

Done.

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/filters/ffmpeg...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/9325044/diff/2001/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.cc:62: // XXX: should we require Stop() to be
called? this might end up getting
On 2012/02/03 23:31:29, scherkus wrote:
> can you change this to TODO(scherkus)
> 
> I use XXX as comments in the code I'm supposed to address BEFORE committing
but
> sometimes they slip by D:

Done.

[Ami GONE FROM CHROMIUM, 2012-02-04 00:57:59]

On 2012/02/04 00:10:21, Ami Fischman wrote:
> acolwell@ pointed out that my change orphans FFmpegAudioDecoder::DoFlush(),
and
> that without it being called it's likely that decoded audio samples will have
> incorrect timestamps.
> I'm going to take a crack at moving ownership of AudioDecoder into
> AudioRendererBase (from Pipeline), to avoid having to plumb
> AudioDecoder-specific code into CompositeFilter::Flush (ewww).

Done; PTAL.
Ideas for testing are welcome.  (manually tested w/ testmatrix & lots of
seeking, and things sound right).

[scherkus (not reviewing), 2012-02-06 21:04:04]

LGTM w/ nits

re: testing
With Flush() being handled I don't see any reason why the final audio output
would be different. As a sanity check I'd test the full range of audio-only
clips (WAV, MP3, AAC, Vorbis) and do some seeking around

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/audio_dec...
File media/base/audio_decoder.h (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/audio_dec...
media/base/audio_decoder.h:10: #include "base/time.h"
don't see this used

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/audio_dec...
media/base/audio_decoder.h:12: #include "media/base/demuxer_stream.h"
fwd declare some of these instead?

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/audio_dec...
media/base/audio_decoder.h:20: : public base::RefCountedThreadSafe<AudioDecoder>
{
AFAIK after this patch we'll be very close to replacing ref-counting with
scoped_ptr<> and Pass()

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/audio_dec...
media/base/audio_decoder.h:22: // Initialize a AudioDecoder with the given
DemuxerStream, executing the
s/a/an

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/filter_co...
File media/base/filter_collection.h (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/filter_co...
media/base/filter_collection.h:11: #include "media/base/audio_decoder.h"
forward declare?

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/filters.h
File media/base/filters.h (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/filters.h...
media/base/filters.h:202: virtual void Initialize(const
scoped_refptr<AudioDecoder> decoder,
const-ref

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/mock_filt...
File media/base/mock_filters.h (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/mock_filt...
media/base/mock_filters.h:208: MOCK_METHOD3(Initialize, void(const
scoped_refptr<DemuxerStream>,
const-ref

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/mock_filt...
media/base/mock_filters.h:265: MOCK_METHOD4(Initialize, void(const
scoped_refptr<AudioDecoder> decoder,
const-ref

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/pipeline....
media/base/pipeline.cc:58: scoped_refptr<AudioDecoder> audio_decoder_;
nit: drop trailing _ for structs

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/pipeline_...
File media/base/pipeline_unittest.cc (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/pipeline_...
media/base/pipeline_unittest.cc:143: void InitializeAudioDecoder(const
scoped_refptr<DemuxerStream> stream) {
const-ref

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/audio_...
File media/filters/audio_renderer_base.cc (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/audio_...
media/filters/audio_renderer_base.cc:13: #include "media/base/audio_decoder.h"
this is included in .h

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/audio_...
media/filters/audio_renderer_base.cc:92: void
AudioRendererBase::Initialize(const scoped_refptr<AudioDecoder> decoder,
const-ref

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/audio_...
File media/filters/audio_renderer_base.h (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/audio_...
media/filters/audio_renderer_base.h:45: virtual void Initialize(const
scoped_refptr<AudioDecoder> decoder,
const-ref

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/ffmpeg...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.cc:8: #include "media/base/audio_decoder.h"
this is in .h

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/ffmpeg...
File media/filters/ffmpeg_audio_decoder.h (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.h:25: virtual void Initialize(const
scoped_refptr<DemuxerStream> stream,
const-ref

[Ami GONE FROM CHROMIUM, 2012-02-06 21:38:48]

Thanks.
Test matrix + seeking is happy.

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/audio_dec...
File media/base/audio_decoder.h (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/audio_dec...
media/base/audio_decoder.h:10: #include "base/time.h"
On 2012/02/06 21:04:04, scherkus wrote:
> don't see this used

Done.

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/audio_dec...
media/base/audio_decoder.h:12: #include "media/base/demuxer_stream.h"
On 2012/02/06 21:04:04, scherkus wrote:
> fwd declare some of these instead?

Done.

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/audio_dec...
media/base/audio_decoder.h:20: : public base::RefCountedThreadSafe<AudioDecoder>
{
On 2012/02/06 21:04:04, scherkus wrote:
> AFAIK after this patch we'll be very close to replacing ref-counting with
> scoped_ptr<> and Pass()

Yep.

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/audio_dec...
media/base/audio_decoder.h:22: // Initialize a AudioDecoder with the given
DemuxerStream, executing the
On 2012/02/06 21:04:04, scherkus wrote:
> s/a/an

Done.

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/filter_co...
File media/base/filter_collection.h (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/filter_co...
media/base/filter_collection.h:11: #include "media/base/audio_decoder.h"
On 2012/02/06 21:04:04, scherkus wrote:
> forward declare?

Done.

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/filters.h
File media/base/filters.h (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/filters.h...
media/base/filters.h:202: virtual void Initialize(const
scoped_refptr<AudioDecoder> decoder,
On 2012/02/06 21:04:04, scherkus wrote:
> const-ref

Done.

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/mock_filt...
File media/base/mock_filters.h (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/mock_filt...
media/base/mock_filters.h:208: MOCK_METHOD3(Initialize, void(const
scoped_refptr<DemuxerStream>,
On 2012/02/06 21:04:04, scherkus wrote:
> const-ref

Done.

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/mock_filt...
media/base/mock_filters.h:265: MOCK_METHOD4(Initialize, void(const
scoped_refptr<AudioDecoder> decoder,
On 2012/02/06 21:04:04, scherkus wrote:
> const-ref

Done.

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/pipeline....
media/base/pipeline.cc:58: scoped_refptr<AudioDecoder> audio_decoder_;
On 2012/02/06 21:04:04, scherkus wrote:
> nit: drop trailing _ for structs

Done.

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/pipeline_...
File media/base/pipeline_unittest.cc (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/base/pipeline_...
media/base/pipeline_unittest.cc:143: void InitializeAudioDecoder(const
scoped_refptr<DemuxerStream> stream) {
On 2012/02/06 21:04:04, scherkus wrote:
> const-ref

Done.

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/audio_...
File media/filters/audio_renderer_base.cc (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/audio_...
media/filters/audio_renderer_base.cc:13: #include "media/base/audio_decoder.h"
On 2012/02/06 21:04:04, scherkus wrote:
> this is included in .h

Done.

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/audio_...
media/filters/audio_renderer_base.cc:92: void
AudioRendererBase::Initialize(const scoped_refptr<AudioDecoder> decoder,
On 2012/02/06 21:04:04, scherkus wrote:
> const-ref

Done.

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/ffmpeg...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.cc:8: #include "media/base/audio_decoder.h"
On 2012/02/06 21:04:04, scherkus wrote:
> this is in .h

Done.

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/ffmpeg...
File media/filters/ffmpeg_audio_decoder.h (right):

https://chromiumcodereview.appspot.com/9325044/diff/4003/media/filters/ffmpeg...
media/filters/ffmpeg_audio_decoder.h:25: virtual void Initialize(const
scoped_refptr<DemuxerStream> stream,
On 2012/02/06 21:04:04, scherkus wrote:
> const-ref

Done.

[commit-bot: I haz the power, 2012-02-06 21:40:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/9325044/11003

[commit-bot: I haz the power, 2012-02-07 01:10:14]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 120616, job name
was 9325044-11003 (retry) (previous was lost) (previous was lost) (previous was
lost).

[commit-bot: I haz the power, 2012-02-07 01:27:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/9325044/11003