FFmpeg roll for M31.

FFmpeg roll for M31.

BUG=none
TEST=asan media_unittests, ffmpeg_regression_tests + manual.

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

[scherkus (not reviewing), 2013-08-27 00:23:44]

https://codereview.chromium.org/23172019/diff/1/media/filters/ffmpeg_audio_de...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/23172019/diff/1/media/filters/ffmpeg_audio_de...
media/filters/ffmpeg_audio_decoder.cc:486: av_frame_unref(av_frame_);
when are refs taken?

worth writing a ScopedAVFrameUnreffer?

[DaleCurtis, 2013-08-27 00:30:13]

https://codereview.chromium.org/23172019/diff/1/media/filters/ffmpeg_audio_de...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/23172019/diff/1/media/filters/ffmpeg_audio_de...
media/filters/ffmpeg_audio_decoder.cc:486: av_frame_unref(av_frame_);
On 2013/08/27 00:23:44, scherkus wrote:
> when are refs taken?
> 
> worth writing a ScopedAVFrameUnreffer?

API contract says we have to call this to destroy the frame; so presumably the
refs are taken internally by FFmpeg.  A scoper might be a little confusing since
we aren't freeing the frame, just unref'ing it.

I think the code would be clearer with a scoper though, plus we'll need it in
the video decoder too.  So if a scoped_ptr acting like a scoped_refptr isn't a
concern I'm happy to add it.

[DaleCurtis, 2013-08-27 00:30:51]

https://codereview.chromium.org/23172019/diff/1/media/filters/ffmpeg_audio_de...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/23172019/diff/1/media/filters/ffmpeg_audio_de...
media/filters/ffmpeg_audio_decoder.cc:486: av_frame_unref(av_frame_);
On 2013/08/27 00:30:13, DaleCurtis wrote:
> On 2013/08/27 00:23:44, scherkus wrote:
> > when are refs taken?
> > 
> > worth writing a ScopedAVFrameUnreffer?
> 
> API contract says we have to call this to destroy the frame; so presumably the
> refs are taken internally by FFmpeg.  A scoper might be a little confusing
since
> we aren't freeing the frame, just unref'ing it.
> 
> I think the code would be clearer with a scoper though, plus we'll need it in
> the video decoder too.  So if a scoped_ptr acting like a scoped_refptr isn't a
> concern I'm happy to add it.

(contract when refcounted_frames=1 that is).

[jrummell, 2013-08-27 01:05:23]

Some drive-by comments.

Is the cost of avcodec_alloc_frame() (ultimately the call to  av_malloc()) too
high to make av_frame_ a local variable to RunDecodeLoop()?

https://codereview.chromium.org/23172019/diff/1/media/filters/ffmpeg_audio_de...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/23172019/diff/1/media/filters/ffmpeg_audio_de...
media/filters/ffmpeg_audio_decoder.cc:509: av_frame_unref(av_frame_);
Should this be done outside the if block just in case ffmpeg allocated some
stuff, but then found out it had no data to process? This reinitializes the
av_frame_, so would it be possible to call avcodec_decode_audio4() again without
initializing it (like subsequent call to RunDecodeLoop())?

[DaleCurtis, 2013-08-27 01:16:58]

I suspect its just an unnecessary allocation.  I don't think there's any
noticeable perf impact, but avoiding extra allocations is always nice.

https://codereview.chromium.org/23172019/diff/1/media/filters/ffmpeg_audio_de...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/23172019/diff/1/media/filters/ffmpeg_audio_de...
media/filters/ffmpeg_audio_decoder.cc:509: av_frame_unref(av_frame_);
On 2013/08/27 01:05:24, jrummell wrote:
> Should this be done outside the if block just in case ffmpeg allocated some
> stuff, but then found out it had no data to process? This reinitializes the
> av_frame_, so would it be possible to call avcodec_decode_audio4() again
without
> initializing it (like subsequent call to RunDecodeLoop())?

I used ffmpeg and ffplay as reference here and they both only unref the frame if
frame_decoded == true.

[DaleCurtis, 2013-08-28 18:26:24]

Bots are green. Any more comments?

[scherkus (not reviewing), 2013-08-28 19:11:03]

lgtm

[commit-bot: I haz the power, 2013-08-28 21:01:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/23172019/8001

[jrummell, 2013-08-28 22:23:15]

I guess it is looking for a LGTM from me.

[commit-bot: I haz the power, 2013-08-29 09:00:10]

Change committed as 220228