Introduce AudioDecoderConfig to migrate away from GetAVStream().

Introduce AudioDecoderConfig to migrate away from GetAVStream().

Instead add DemuxerStream::audio_decoder_config() to break FFmpegAudioDecoder's dependency on the AVCodecContext object maintained by FFmpegDemuxer.


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

[scherkus (not reviewing), 2011-09-12 19:51:29]

OK mostly looking for early feedback since this patch depends on the following:
http://codereview.chromium.org/7796033/
http://codereview.chromium.org/6901135/

[Ami GONE FROM CHROMIUM, 2011-09-12 20:54:21]

I like it!

http://codereview.chromium.org/7867051/diff/3001/media/base/audio_decoder_con...
File media/base/audio_decoder_config.cc (right):

http://codereview.chromium.org/7867051/diff/3001/media/base/audio_decoder_con...
media/base/audio_decoder_config.cc:22: CHECK(extra_data_size_ == 0 ||
extra_data);
CHECK, really?

http://codereview.chromium.org/7867051/diff/3001/media/base/audio_decoder_con...
media/base/audio_decoder_config.cc:22: CHECK(extra_data_size_ == 0 ||
extra_data);
IWBN to assert the stronger:
(size!=0) == (data!=NULL)

http://codereview.chromium.org/7867051/diff/3001/media/base/audio_decoder_con...
File media/base/audio_decoder_config.h (right):

http://codereview.chromium.org/7867051/diff/3001/media/base/audio_decoder_con...
media/base/audio_decoder_config.h:32: const uint8* extra_data, size_t
extra_data_size);
Document (lack of) ownership semantics of |extra_data|

http://codereview.chromium.org/7867051/diff/3001/media/base/audio_decoder_con...
media/base/audio_decoder_config.h:46: int sample_rate_;
comment units?

http://codereview.chromium.org/7867051/diff/3001/media/base/audio_decoder_con...
media/base/audio_decoder_config.h:48: // Optional byte data required to
initialize audio decoders.
Examples?
Comment might be more useful on public accessors, not private impl.

http://codereview.chromium.org/7867051/diff/3001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/7867051/diff/3001/media/base/filters.h#newcode180
media/base/filters.h:180: // Returns an |AVStream*| if supported, or NULL.
Is there a chance to remove this at the same time?

http://codereview.chromium.org/7867051/diff/3001/media/base/filters.h#newcode183
media/base/filters.h:183: // Returns a weak reference to the audio decoder
configuration if present,
"weak reference"??

http://codereview.chromium.org/7867051/diff/3001/media/ffmpeg/ffmpeg_common.h
File media/ffmpeg/ffmpeg_common.h (right):

http://codereview.chromium.org/7867051/diff/3001/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:69: AudioDecoderConfig*
AVCodecContextToAudioDecoderConfig(
Returning a new'd struct isn't very google-c++-stylish.
Any reason not to use the style in the next method, or just return a copy?

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

http://codereview.chromium.org/7867051/diff/3001/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:113: AudioDecoderConfig* config =
stream->audio_decoder_config();
Hopefully this can be a const& or at least a const*.

http://codereview.chromium.org/7867051/diff/3001/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:116: // Sanity checking.
Shouldn't this be happening earlier (ideally during construction/initialization
of stream->audio_decoder_config())

http://codereview.chromium.org/7867051/diff/3001/media/filters/ffmpeg_demuxer.h
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/7867051/diff/3001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.h:86: virtual AudioDecoderConfig*
audio_decoder_config();
OVERRIDE everywhere?

[scherkus (not reviewing), 2011-09-19 21:19:45]

http://codereview.chromium.org/7867051/diff/3001/media/base/audio_decoder_con...
File media/base/audio_decoder_config.cc (right):

http://codereview.chromium.org/7867051/diff/3001/media/base/audio_decoder_con...
media/base/audio_decoder_config.cc:22: CHECK(extra_data_size_ == 0 ||
extra_data);
On 2011/09/12 20:54:21, Ami Fischman wrote:
> IWBN to assert the stronger:
> (size!=0) == (data!=NULL)

Done.

http://codereview.chromium.org/7867051/diff/3001/media/base/audio_decoder_con...
File media/base/audio_decoder_config.h (right):

http://codereview.chromium.org/7867051/diff/3001/media/base/audio_decoder_con...
media/base/audio_decoder_config.h:32: const uint8* extra_data, size_t
extra_data_size);
On 2011/09/12 20:54:21, Ami Fischman wrote:
> Document (lack of) ownership semantics of |extra_data|

Done.

http://codereview.chromium.org/7867051/diff/3001/media/base/audio_decoder_con...
media/base/audio_decoder_config.h:46: int sample_rate_;
On 2011/09/12 20:54:21, Ami Fischman wrote:
> comment units?

This is typically understood to be in Hz but we could rename the variable to be
samples_per_second_

WDYT?

http://codereview.chromium.org/7867051/diff/3001/media/base/audio_decoder_con...
media/base/audio_decoder_config.h:48: // Optional byte data required to
initialize audio decoders.
On 2011/09/12 20:54:21, Ami Fischman wrote:
> Examples?
> Comment might be more useful on public accessors, not private impl.

Done.

http://codereview.chromium.org/7867051/diff/3001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/7867051/diff/3001/media/base/filters.h#newcode180
media/base/filters.h:180: // Returns an |AVStream*| if supported, or NULL.
On 2011/09/12 20:54:21, Ami Fischman wrote:
> Is there a chance to remove this at the same time?

Not yet :\

http://codereview.chromium.org/7867051/diff/3001/media/ffmpeg/ffmpeg_common.h
File media/ffmpeg/ffmpeg_common.h (right):

http://codereview.chromium.org/7867051/diff/3001/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:69: AudioDecoderConfig*
AVCodecContextToAudioDecoderConfig(
On 2011/09/12 20:54:21, Ami Fischman wrote:
> Returning a new'd struct isn't very google-c++-stylish.
> Any reason not to use the style in the next method, or just return a copy?

Done.

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

http://codereview.chromium.org/7867051/diff/3001/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:113: AudioDecoderConfig* config =
stream->audio_decoder_config();
On 2011/09/12 20:54:21, Ami Fischman wrote:
> Hopefully this can be a const& or at least a const*.

Done.

http://codereview.chromium.org/7867051/diff/3001/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:116: // Sanity checking.
On 2011/09/12 20:54:21, Ami Fischman wrote:
> Shouldn't this be happening earlier (ideally during
construction/initialization
> of stream->audio_decoder_config())

I'm torn.. on one hand you could argue that said limits apply to this particular
use of ADC in conjunction with FFmpeg... but I think that's a little silly since
these values are crazy for any realistic application

Will move to ADC::IsValidConfig()

http://codereview.chromium.org/7867051/diff/3001/media/filters/ffmpeg_demuxer.h
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/7867051/diff/3001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.h:86: virtual AudioDecoderConfig*
audio_decoder_config();
On 2011/09/12 20:54:21, Ami Fischman wrote:
> OVERRIDE everywhere?

Done.

[scherkus (not reviewing), 2011-09-20 20:08:20]

pinging in case either of you want to perfcrastinate

[Ami GONE FROM CHROMIUM, 2011-09-20 20:34:49]

Almost there.

http://codereview.chromium.org/7867051/diff/3001/media/base/audio_decoder_con...
File media/base/audio_decoder_config.h (right):

http://codereview.chromium.org/7867051/diff/3001/media/base/audio_decoder_con...
media/base/audio_decoder_config.h:46: int sample_rate_;
On 2011/09/19 21:19:45, scherkus wrote:
> On 2011/09/12 20:54:21, Ami Fischman wrote:
> > comment units?
> 
> This is typically understood to be in Hz but we could rename the variable to
be
> samples_per_second_
> 
> WDYT?

I like that.

http://codereview.chromium.org/7867051/diff/9001/media/base/audio_decoder_con...
File media/base/audio_decoder_config.cc (right):

http://codereview.chromium.org/7867051/diff/9001/media/base/audio_decoder_con...
media/base/audio_decoder_config.cc:13: : codec_(kUnknownAudioCodec),
Could also impl. as Initialize() call?
I don't think I have a preference.

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

http://codereview.chromium.org/7867051/diff/9001/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:118: if (!config.IsValidConfig()) {
This is better, but my point was that it would be nicer if not every caller of
audio_decoder_config() needed to check the return value for validity.  Is there
no opportunity for erroring-out earlier, presumably during stream construction?

[scherkus (not reviewing), 2011-09-21 18:19:16]

changed sample_rate -> samples_per_second for both AudioDecoderConfig and
AudioDecoder interface (and corresponding impls + mocks)

http://codereview.chromium.org/7867051/diff/9001/media/base/audio_decoder_con...
File media/base/audio_decoder_config.cc (right):

http://codereview.chromium.org/7867051/diff/9001/media/base/audio_decoder_con...
media/base/audio_decoder_config.cc:13: : codec_(kUnknownAudioCodec),
On 2011/09/20 20:34:50, Ami Fischman wrote:
> Could also impl. as Initialize() call?
> I don't think I have a preference.

I'll leave as is -- I think it reads a bit easier as to what the uninitialized
values are

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

http://codereview.chromium.org/7867051/diff/9001/media/filters/ffmpeg_audio_d...
media/filters/ffmpeg_audio_decoder.cc:118: if (!config.IsValidConfig()) {
On 2011/09/20 20:34:50, Ami Fischman wrote:
> This is better, but my point was that it would be nicer if not every caller of
> audio_decoder_config() needed to check the return value for validity.  Is
there
> no opportunity for erroring-out earlier, presumably during stream
construction?

You're right in that the correct place for this check is before this object is
even created, however I'm hesitant to toss more code into PipelineImpl at this
time as it needs a lot of loving itself

TODOing it up for now

[Ami GONE FROM CHROMIUM, 2011-09-21 18:29:25]

lgtm

[commit-bot: I haz the power, 2011-09-21 22:44:08]

Change committed as 102183

[Ami GONE FROM CHROMIUM, 2011-09-29 18:16:35]

http://codereview.chromium.org/7867051/diff/3001/media/base/filters.h
File media/base/filters.h (right):

http://codereview.chromium.org/7867051/diff/3001/media/base/filters.h#newcode180
media/base/filters.h:180: // Returns an |AVStream*| if supported, or NULL.
On 2011/09/19 21:19:45, scherkus wrote:
> On 2011/09/12 20:54:21, Ami Fischman wrote:
> > Is there a chance to remove this at the same time?
> 
> Not yet :\

How about now that AdaptiveDemuxer is dead? :)

[scherkus (not reviewing), 2011-09-29 20:39:45]

On 2011/09/29 18:16:35, Ami Fischman wrote:
> http://codereview.chromium.org/7867051/diff/3001/media/base/filters.h
> File media/base/filters.h (right):
> 
>
http://codereview.chromium.org/7867051/diff/3001/media/base/filters.h#newcode180
> media/base/filters.h:180: // Returns an |AVStream*| if supported, or NULL.
> On 2011/09/19 21:19:45, scherkus wrote:
> > On 2011/09/12 20:54:21, Ami Fischman wrote:
> > > Is there a chance to remove this at the same time?
> > 
> > Not yet :\
> 
> How about now that AdaptiveDemuxer is dead? :)

Well I _am_ going on a plane soon...