Remove DemuxerStream::GetAVStream() once and for all.

Remove DemuxerStream::GetAVStream() once and for all.

We now use AudioDecoderConfig and VideoDecoderConfig to pass decoder initialization information.


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

[scherkus (not reviewing), 2011-10-25 22:42:15]

acolwell / vrk: PTAL

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

http://codereview.chromium.org/8341033/diff/2001/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:77: void AVCodecContextToVideoDecoderConfig(
I wonder whether this should be AVStreamToVideoDecoderConfig()

frame_rate and aspect_ratio are retrieved from AVStream

http://codereview.chromium.org/8341033/diff/2001/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:100: base::TimeDelta GetFrameDuration(const
VideoDecoderConfig& config);
should this be a method on VideoDecoderConfig?

I'm not entirely sure since we make use of av_q2d() -- hence why this function
is here

http://codereview.chromium.org/8341033/diff/2001/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:107: gfx::Size GetNaturalSize(const
VideoDecoderConfig& config);
ditto here due to av_q2d()

[acolwell GONE FROM CHROMIUM, 2011-10-26 17:09:13]

LGTM

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

http://codereview.chromium.org/8341033/diff/2001/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:77: void AVCodecContextToVideoDecoderConfig(
On 2011/10/25 22:42:15, scherkus wrote:
> I wonder whether this should be AVStreamToVideoDecoderConfig()
> 
> frame_rate and aspect_ratio are retrieved from AVStream
If we are only likely to call this from contexts where we have an AVStream then
I'm fine with that.

http://codereview.chromium.org/8341033/diff/2001/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:100: base::TimeDelta GetFrameDuration(const
VideoDecoderConfig& config);
On 2011/10/25 22:42:15, scherkus wrote:
> should this be a method on VideoDecoderConfig?
Yes.

> I'm not entirely sure since we make use of av_q2d() -- hence why this function
> is here
av_q2d() impl is simply a.num / (double) a.den so lets just drop the call an use
this code directly.

http://codereview.chromium.org/8341033/diff/2001/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:107: gfx::Size GetNaturalSize(const
VideoDecoderConfig& config);
On 2011/10/25 22:42:15, scherkus wrote:
> ditto here due to av_q2d()
ditto

http://codereview.chromium.org/8341033/diff/2001/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/8341033/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:34: //   ^^^^^^^^^^^ IS THIS STILL NEEDED?
^^^^^^^^^^^^
For now yes since we still need an AVStream to construct the VideoDecoderConfig.
In a separate CL I'll probably be able to extract the frame rate & aspect info
from the WebM headers.

Perhaps the comment should be changed to indicate this can be removed when we
are able to create AudioDecoderConfig & VideoDecoderConfigs w/o needing an
AVStream.

[vrk (LEFT CHROMIUM), 2011-10-26 18:07:11]

nice! LGTM

and i agree with/have nothing to add to acolwell's suggestions/answers!

http://codereview.chromium.org/8341033/diff/2001/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

http://codereview.chromium.org/8341033/diff/2001/media/ffmpeg/ffmpeg_common.c...
media/ffmpeg/ffmpeg_common.cc:174: const AVCodecContext* codec_context,
nit: const ref

http://codereview.chromium.org/8341033/diff/2001/media/ffmpeg/ffmpeg_common.c...
media/ffmpeg/ffmpeg_common.cc:180: codec_context->coded_height);
nit: move to line above?

http://codereview.chromium.org/8341033/diff/2001/media/ffmpeg/ffmpeg_common.c...
media/ffmpeg/ffmpeg_common.cc:185: codec_context->height);
nit: move to line above?

[scherkus (not reviewing), 2011-10-26 19:08:00]

thanks for the review!

might want to do a quick sanity check over some of the new stuff

http://codereview.chromium.org/8341033/diff/2001/media/ffmpeg/ffmpeg_common.cc
File media/ffmpeg/ffmpeg_common.cc (right):

http://codereview.chromium.org/8341033/diff/2001/media/ffmpeg/ffmpeg_common.c...
media/ffmpeg/ffmpeg_common.cc:174: const AVCodecContext* codec_context,
On 2011/10/26 18:07:12, Victoria Kirst wrote:
> nit: const ref

we tend to keep FFmpeg as pointers since that's how the library returns them

http://codereview.chromium.org/8341033/diff/2001/media/ffmpeg/ffmpeg_common.c...
media/ffmpeg/ffmpeg_common.cc:180: codec_context->coded_height);
On 2011/10/26 18:07:12, Victoria Kirst wrote:
> nit: move to line above?

Done.

http://codereview.chromium.org/8341033/diff/2001/media/ffmpeg/ffmpeg_common.c...
media/ffmpeg/ffmpeg_common.cc:185: codec_context->height);
On 2011/10/26 18:07:12, Victoria Kirst wrote:
> nit: move to line above?

Done.

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

http://codereview.chromium.org/8341033/diff/2001/media/ffmpeg/ffmpeg_common.h...
media/ffmpeg/ffmpeg_common.h:77: void AVCodecContextToVideoDecoderConfig(
On 2011/10/26 17:09:13, acolwell wrote:
> On 2011/10/25 22:42:15, scherkus wrote:
> > I wonder whether this should be AVStreamToVideoDecoderConfig()
> > 
> > frame_rate and aspect_ratio are retrieved from AVStream
> If we are only likely to call this from contexts where we have an AVStream
then
> I'm fine with that.

Done.

http://codereview.chromium.org/8341033/diff/2001/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

http://codereview.chromium.org/8341033/diff/2001/media/filters/chunk_demuxer....
media/filters/chunk_demuxer.cc:34: //   ^^^^^^^^^^^ IS THIS STILL NEEDED?
^^^^^^^^^^^^
On 2011/10/26 17:09:13, acolwell wrote:
> For now yes since we still need an AVStream to construct the
VideoDecoderConfig.
> In a separate CL I'll probably be able to extract the frame rate & aspect info
> from the WebM headers.
> 
> Perhaps the comment should be changed to indicate this can be removed when we
> are able to create AudioDecoderConfig & VideoDecoderConfigs w/o needing an
> AVStream.

Done.

[commit-bot: I haz the power, 2011-10-26 22:22:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/8341033/3002

[commit-bot: I haz the power, 2011-10-26 22:46:31]

Try job failure for 8341033-3002 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-10-26 22:53:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/8341033/10001

[commit-bot: I haz the power, 2011-10-27 00:43:21]

Change committed as 107494