Roll FFmpeg for M37.

Roll FFmpeg for M37.

Syncs to head as of bebce653e5601ceafa004db0eb6b2c7d4d16f0c0.  The
roll requires us to stop using some deprecated features:
- Replaces av_get_frame_defaults() with av_frame_unref() per docs.
- Switches FFmpegVideoDecoder to use ref-counted frames.
- Removes error concealment settings and disables error resilience
for all platforms except ChromeOS (since it's required for mpeg4).

Avoiding these also allows us to disable some deprecated features
wholesale via #defines.

BUG=119020, 236611
TEST=all tests (and regression tests) pass under asan
NOTRY=true
R=jrummell@chromium.org, wolenetz@chromium.org

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

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

[DaleCurtis, 2014-05-15 23:40:10]

You guys are the last ones to really look at these features, so please take a
look!

[Alpha Left Google, 2014-05-16 00:26:36]

Drive by: media/cast LGTM.

[jrummell, 2014-05-16 19:08:52]

lgtm

[wolenetz, 2014-05-16 19:45:58]

Looking pretty good. I have a few comments and some questions w.r.t. ffmpeg API
usage.
Also, do ffmpeg_cdm_{audio,video}_decoder need similar changes?

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
File media/base/media_file_checker.cc (left):

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
media/base/media_file_checker.cc:87: avcodec_get_frame_defaults(frame.get());
nit: In CL description, s/get_buffer_defaults/get_frame_defaults

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
File media/base/media_file_checker.cc (right):

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
media/base/media_file_checker.cc:79: AVCodecContext* av_context = it->second;
I'm confused a bit. Codesearch failed to indicate to me where we ensure
av_context.refcounted_frames is set to 1 in this code path.
So I'm not sure av_frame_unref() is always good to use here.

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
media/base/media_file_checker.cc:97: result = avcodec_decode_video2(av_context,
frame.get(), &frame_decoded,
(Possibly for a different CL): Per
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...,
ILTM like we might not be protecting against overreads for damaged MPEG streams.
By design or am I missing something?

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
media/base/media_file_checker.cc:99: if (result >= 0 && frame_decoded)
(Possibly for a different CL): Per
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...,
some decoders may support multiple frames in a single AVPacket. For video, why
don't we use temp_packet, and update its size & data like we do for audio? Is
this possibly because the return value (unlike audio) is zero if no frame could
be decompressed, instead of number of bytes consumed regardless?

https://codereview.chromium.org/286953005/diff/1/media/cast/test/sender.cc
File media/cast/test/sender.cc (right):

https://codereview.chromium.org/286953005/diff/1/media/cast/test/sender.cc#ne...
media/cast/test/sender.cc:540: avcodec_get_frame_defaults(avframe);
Why not replace with av_frame_unref in this method after the AudioBuffer is
created, below?

https://codereview.chromium.org/286953005/diff/1/media/cast/test/sender.cc#ne...
media/cast/test/sender.cc:621: avcodec_get_frame_defaults(avframe);
nit: Doesn't av_frame_alloc() already do this?

https://codereview.chromium.org/286953005/diff/1/media/cast/test/sender.cc#ne...
media/cast/test/sender.cc:625: CHECK(avcodec_decode_video2(
ditto: Why don't we iterate through decoding video packets like we do for audio,
if decode retval >0 and < packet.size (and use a shallow temp_packet,
similarly)?

https://codereview.chromium.org/286953005/diff/1/media/cast/test/sender.cc#ne...
media/cast/test/sender.cc:626: av_video_context(), avframe, &got_picture,
packet.get()) >= 0)
ditto: protect against MPEG overreads?

https://codereview.chromium.org/286953005/diff/1/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/286953005/diff/1/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:57: static int GetVideoBufferImpl(struct
AVCodecContext* s,
Curious - why do we need this Impl vs just assigning get_buffer2 =
GetVideoBuffer, below (like ffmpeg_audio_decoder does)?

https://codereview.chromium.org/286953005/diff/1/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:301: int result =
avcodec_decode_video2(codec_context_.get(),
ditto: iterate through multiple packets in buffer?

https://codereview.chromium.org/286953005/diff/1/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:303: &frame_decoded,
ditto: protect against MPEG overreads?

https://codereview.chromium.org/286953005/diff/1/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.h (right):

https://codereview.chromium.org/286953005/diff/1/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.h:44: // the dimensions of |codec_context|.
See AVCodecContext.get_buffer
nit: s/get_buffer/get_buffer2/

https://codereview.chromium.org/286953005/diff/1/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.h:46: int GetVideoBuffer(struct
AVCodecContext* codec_context,
Curious - why not just static in .cc like FFmpegAudioDecoder::GetAudioBuffer()?

[DaleCurtis, 2014-05-16 19:54:26]

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
File media/base/media_file_checker.cc (right):

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
media/base/media_file_checker.cc:79: AVCodecContext* av_context = it->second;
On 2014/05/16 19:45:58, wolenetz wrote:
> I'm confused a bit. Codesearch failed to indicate to me where we ensure
> av_context.refcounted_frames is set to 1 in this code path.
> So I'm not sure av_frame_unref() is always good to use here.

Looks like this may need to wait until the roll after all.  You can see that in
the new version of FFmpeg avcodec_get_frame_defaults() just calls
av_buffer_unref() regardless of that setting:

http://git.chromium.org/gitweb/?p=chromium/third_party/ffmpeg.git;a=blob;f=li...

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
media/base/media_file_checker.cc:97: result = avcodec_decode_video2(av_context,
frame.get(), &frame_decoded,
On 2014/05/16 19:45:58, wolenetz wrote:
> (Possibly for a different CL): Per
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...,
> ILTM like we might not be protecting against overreads for damaged MPEG
streams.
> By design or am I missing something? 

DecoderBuffer automatically pads and zeros.

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
media/base/media_file_checker.cc:99: if (result >= 0 && frame_decoded)
On 2014/05/16 19:45:58, wolenetz wrote:
> (Possibly for a different CL): Per
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...,
> some decoders may support multiple frames in a single AVPacket. For video, why
> don't we use temp_packet, and update its size & data like we do for audio? Is
> this possibly because the return value (unlike audio) is zero if no frame
could
> be decompressed, instead of number of bytes consumed regardless?

Hmm, that comment has changed I think. When I talked to Ronald about this in the
past FFmpeg only outputs 1 frame per packet.

We could add a check to ensure this is true though.

https://codereview.chromium.org/286953005/diff/1/media/cast/test/sender.cc
File media/cast/test/sender.cc (right):

https://codereview.chromium.org/286953005/diff/1/media/cast/test/sender.cc#ne...
media/cast/test/sender.cc:540: avcodec_get_frame_defaults(avframe);
On 2014/05/16 19:45:58, wolenetz wrote:
> Why not replace with av_frame_unref in this method after the AudioBuffer is
> created, below?

Hmm, looks like I missed some in the time this CL was created (months ago) and
now. I'll change these.

https://codereview.chromium.org/286953005/diff/1/media/cast/test/sender.cc#ne...
media/cast/test/sender.cc:621: avcodec_get_frame_defaults(avframe);
On 2014/05/16 19:45:58, wolenetz wrote:
> nit: Doesn't av_frame_alloc() already do this?

Yeah

https://codereview.chromium.org/286953005/diff/1/media/cast/test/sender.cc#ne...
media/cast/test/sender.cc:625: CHECK(avcodec_decode_video2(
On 2014/05/16 19:45:58, wolenetz wrote:
> ditto: Why don't we iterate through decoding video packets like we do for
audio,
> if decode retval >0 and < packet.size (and use a shallow temp_packet,
> similarly)?

Ditto.

https://codereview.chromium.org/286953005/diff/1/media/cast/test/sender.cc#ne...
media/cast/test/sender.cc:626: av_video_context(), avframe, &got_picture,
packet.get()) >= 0)
On 2014/05/16 19:45:58, wolenetz wrote:
> ditto: protect against MPEG overreads?

Ditto.

https://codereview.chromium.org/286953005/diff/1/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.cc (right):

https://codereview.chromium.org/286953005/diff/1/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.cc:57: static int GetVideoBufferImpl(struct
AVCodecContext* s,
On 2014/05/16 19:45:58, wolenetz wrote:
> Curious - why do we need this Impl vs just assigning get_buffer2 =
> GetVideoBuffer, below (like ffmpeg_audio_decoder does)?

Because video uses a frame pool which is owned the the FFmpegVideoDecoder class,
so this method uses the opaque pointer to store that pointer.

[DaleCurtis, 2014-05-20 01:13:44]

Okay, this is ready to go.  The one thing incorrect is the value in DEPS, which
currently points to a ref on the merge-m37 branch.  After review I'll merge that
branch into master and update the ref before CQ'ing.

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
File media/base/media_file_checker.cc (left):

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
media/base/media_file_checker.cc:87: avcodec_get_frame_defaults(frame.get());
On 2014/05/16 19:45:58, wolenetz wrote:
> nit: In CL description, s/get_buffer_defaults/get_frame_defaults

Done.

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
File media/base/media_file_checker.cc (right):

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
media/base/media_file_checker.cc:99: if (result >= 0 && frame_decoded)
On 2014/05/16 19:54:26, DaleCurtis wrote:
> On 2014/05/16 19:45:58, wolenetz wrote:
> > (Possibly for a different CL): Per
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...,
> > some decoders may support multiple frames in a single AVPacket. For video,
why
> > don't we use temp_packet, and update its size & data like we do for audio?
Is
> > this possibly because the return value (unlike audio) is zero if no frame
> could
> > be decompressed, instead of number of bytes consumed regardless?
> 
> Hmm, that comment has changed I think. When I talked to Ronald about this in
the
> past FFmpeg only outputs 1 frame per packet.
> 
> We could add a check to ensure this is true though.

Done.

https://codereview.chromium.org/286953005/diff/1/media/cast/test/sender.cc
File media/cast/test/sender.cc (right):

https://codereview.chromium.org/286953005/diff/1/media/cast/test/sender.cc#ne...
media/cast/test/sender.cc:540: avcodec_get_frame_defaults(avframe);
On 2014/05/16 19:54:26, DaleCurtis wrote:
> On 2014/05/16 19:45:58, wolenetz wrote:
> > Why not replace with av_frame_unref in this method after the AudioBuffer is
> > created, below?
> 
> Hmm, looks like I missed some in the time this CL was created (months ago) and
> now. I'll change these.

Done.

https://codereview.chromium.org/286953005/diff/1/media/cast/test/sender.cc#ne...
media/cast/test/sender.cc:626: av_video_context(), avframe, &got_picture,
packet.get()) >= 0)
On 2014/05/16 19:54:26, DaleCurtis wrote:
> On 2014/05/16 19:45:58, wolenetz wrote:
> > ditto: protect against MPEG overreads?
> 
> Ditto.

Modulo, that ffmpeg does this automatically for us similar to how DecoderBuffer
does this.

https://codereview.chromium.org/286953005/diff/1/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.h (right):

https://codereview.chromium.org/286953005/diff/1/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.h:44: // the dimensions of |codec_context|.
See AVCodecContext.get_buffer
On 2014/05/16 19:45:58, wolenetz wrote:
> nit: s/get_buffer/get_buffer2/

Done.

[wolenetz, 2014-05-20 22:37:10]

PS4 is looking pretty good. I suspect my multiple comments like "unref before
first decode looks wrong" may not indicate a code problem, iff unref of a
freshly alloc'ed frame is a no-op other than get_frame_defaults() (which also
occurs during frame alloc). It looks wrong simply because I'm wary of potential
double-frees.

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
File media/base/media_file_checker.cc (right):

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
media/base/media_file_checker.cc:97: result = avcodec_decode_video2(av_context,
frame.get(), &frame_decoded,
On 2014/05/16 19:54:26, DaleCurtis wrote:
> On 2014/05/16 19:45:58, wolenetz wrote:
> > (Possibly for a different CL): Per
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...,
> > ILTM like we might not be protecting against overreads for damaged MPEG
> streams.
> > By design or am I missing something? 
> 
> DecoderBuffer automatically pads and zeros.

(Clarification from offline / CR reply comment in other file: ffmpeg packet
creation in this path pads (in ffmpeg code). In main media pipeline path (not in
this file), ffmpeg_demuxer.cc code using DecoderBuffer that similarly pads and
zeros is used).

https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
media/base/media_file_checker.cc:99: if (result >= 0 && frame_decoded)
On 2014/05/20 01:13:45, DaleCurtis wrote:
> On 2014/05/16 19:54:26, DaleCurtis wrote:
> > On 2014/05/16 19:45:58, wolenetz wrote:
> > > (Possibly for a different CL): Per
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...,
> > > some decoders may support multiple frames in a single AVPacket. For video,
> why
> > > don't we use temp_packet, and update its size & data like we do for audio?
> Is
> > > this possibly because the return value (unlike audio) is zero if no frame
> > could
> > > be decompressed, instead of number of bytes consumed regardless?
> > 
> > Hmm, that comment has changed I think. When I talked to Ronald about this in
> the
> > past FFmpeg only outputs 1 frame per packet.
> > 
> > We could add a check to ensure this is true though.
> 
> Done.

Clarification from offline: DCHECK is added to main media pipeline, but not this
checker.

https://codereview.chromium.org/286953005/diff/1/media/filters/ffmpeg_video_d...
File media/filters/ffmpeg_video_decoder.h (right):

https://codereview.chromium.org/286953005/diff/1/media/filters/ffmpeg_video_d...
media/filters/ffmpeg_video_decoder.h:46: int GetVideoBuffer(struct
AVCodecContext* codec_context,
On 2014/05/16 19:45:58, wolenetz wrote:
> Curious - why not just static in .cc like
FFmpegAudioDecoder::GetAudioBuffer()?

Never mind: this is needed for video since the object is managing a frame pool.

https://codereview.chromium.org/286953005/diff/160001/DEPS
File DEPS (right):

https://codereview.chromium.org/286953005/diff/160001/DEPS#newcode216
DEPS:216:
"/chromium/third_party/ffmpeg.git@729388d1521c3ac755ca93daadb4a7b44e047e7c",
In
http://git.chromium.org/gitweb/?p=chromium/third_party/ffmpeg.git;a=blob;f=li...
, if zeros_left is < 0 prior to the STORE_BLOCK, then ILTM like an out-of-range
dereference of |scantable| within STORE_BLOCK could occur. Why are we dropping
this check (even if just downstream)
(http://git.chromium.org/gitweb/?p=chromium/third_party/ffmpeg.git;a=blobdiff;...)
?

https://codereview.chromium.org/286953005/diff/160001/media/cast/test/sender.cc
File media/cast/test/sender.cc (right):

https://codereview.chromium.org/286953005/diff/160001/media/cast/test/sender....
media/cast/test/sender.cc:628: if (!got_picture)
Need to free avframe before returning early here?

https://codereview.chromium.org/286953005/diff/160001/media/cast/test/sender....
media/cast/test/sender.cc:650: base::Bind(&AVFreeFrame, avframe)));
Need to av_frame_unref(avframe) prior to AVFreeFrame(avframe)?

https://codereview.chromium.org/286953005/diff/160001/media/cdm/ppapi/externa...
File media/cdm/ppapi/external_clear_key/ffmpeg_cdm_audio_decoder.cc (right):

https://codereview.chromium.org/286953005/diff/160001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/ffmpeg_cdm_audio_decoder.cc:274:
av_frame_unref(av_frame_.get());
Unref prior to first decode looks wrong to me.

https://codereview.chromium.org/286953005/diff/160001/media/cdm/ppapi/externa...
File media/cdm/ppapi/external_clear_key/ffmpeg_cdm_video_decoder.cc (right):

https://codereview.chromium.org/286953005/diff/160001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/ffmpeg_cdm_video_decoder.cc:229:
av_frame_unref(av_frame_.get());
ditto: unref prior to first decode looks wrong to me.

https://codereview.chromium.org/286953005/diff/160001/media/ffmpeg/ffmpeg_com...
File media/ffmpeg/ffmpeg_common.h (right):

https://codereview.chromium.org/286953005/diff/160001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.h:25: #define FF_API_PIX_FMT_DESC 0
I would prefer these be exported by ffmpeg.gyp and used in media.gyp when
building target(s) containing ffmpeg_common.cc, so they don't become out of
sync. wdyt?

https://codereview.chromium.org/286953005/diff/160001/media/ffmpeg/ffmpeg_uni...
File media/ffmpeg/ffmpeg_unittest.cc (right):

https://codereview.chromium.org/286953005/diff/160001/media/ffmpeg/ffmpeg_uni...
media/ffmpeg/ffmpeg_unittest.cc:236: av_frame_unref(audio_buffer_.get());
ditto: unref prior to first decode looks wrong.

https://codereview.chromium.org/286953005/diff/160001/media/ffmpeg/ffmpeg_uni...
media/ffmpeg/ffmpeg_unittest.cc:290: av_frame_unref(video_buffer_.get());
ditto

https://codereview.chromium.org/286953005/diff/160001/media/filters/audio_fil...
File media/filters/audio_file_reader.cc (right):

https://codereview.chromium.org/286953005/diff/160001/media/filters/audio_fil...
media/filters/audio_file_reader.cc:135: av_frame_unref(av_frame.get());
ditto: unref prior to first decode looks wrong.

https://codereview.chromium.org/286953005/diff/160001/media/filters/ffmpeg_h2...
File media/filters/ffmpeg_h264_to_annex_b_bitstream_converter.cc (right):

https://codereview.chromium.org/286953005/diff/160001/media/filters/ffmpeg_h2...
media/filters/ffmpeg_h264_to_annex_b_bitstream_converter.cc:57:
av_packet_copy_props(&dest_packet, packet);
nit: it looks like FFmpegDemuxer::OnReadFrameDone() could use similar
av_packet_copy_props(), too.

[wolenetz, 2014-05-20 22:39:42]

On 2014/05/20 22:37:10, wolenetz wrote:
> PS4 is looking pretty good. I suspect my multiple comments like "unref before
> first decode looks wrong" may not indicate a code problem, iff unref of a
> freshly alloc'ed frame is a no-op other than get_frame_defaults() (which also
> occurs during frame alloc). It looks wrong simply because I'm wary of
potential
> double-frees.
> 
>
https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
> File media/base/media_file_checker.cc (right):
> 
>
https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
> media/base/media_file_checker.cc:97: result =
avcodec_decode_video2(av_context,
> frame.get(), &frame_decoded,
> On 2014/05/16 19:54:26, DaleCurtis wrote:
> > On 2014/05/16 19:45:58, wolenetz wrote:
> > > (Possibly for a different CL): Per
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...,
> > > ILTM like we might not be protecting against overreads for damaged MPEG
> > streams.
> > > By design or am I missing something? 
> > 
> > DecoderBuffer automatically pads and zeros.
> 
> (Clarification from offline / CR reply comment in other file: ffmpeg packet
> creation in this path pads (in ffmpeg code). In main media pipeline path (not
in
> this file), ffmpeg_demuxer.cc code using DecoderBuffer that similarly pads and
> zeros is used).
> 
>
https://codereview.chromium.org/286953005/diff/1/media/base/media_file_checke...
> media/base/media_file_checker.cc:99: if (result >= 0 && frame_decoded)
> On 2014/05/20 01:13:45, DaleCurtis wrote:
> > On 2014/05/16 19:54:26, DaleCurtis wrote:
> > > On 2014/05/16 19:45:58, wolenetz wrote:
> > > > (Possibly for a different CL): Per
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg...,
> > > > some decoders may support multiple frames in a single AVPacket. For
video,
> > why
> > > > don't we use temp_packet, and update its size & data like we do for
audio?
> > Is
> > > > this possibly because the return value (unlike audio) is zero if no
frame
> > > could
> > > > be decompressed, instead of number of bytes consumed regardless?
> > > 
> > > Hmm, that comment has changed I think. When I talked to Ronald about this
in
> > the
> > > past FFmpeg only outputs 1 frame per packet.
> > > 
> > > We could add a check to ensure this is true though.
> > 
> > Done.
> 
> Clarification from offline: DCHECK is added to main media pipeline, but not
this
> checker.
> 
>
https://codereview.chromium.org/286953005/diff/1/media/filters/ffmpeg_video_d...
> File media/filters/ffmpeg_video_decoder.h (right):
> 
>
https://codereview.chromium.org/286953005/diff/1/media/filters/ffmpeg_video_d...
> media/filters/ffmpeg_video_decoder.h:46: int GetVideoBuffer(struct
> AVCodecContext* codec_context,
> On 2014/05/16 19:45:58, wolenetz wrote:
> > Curious - why not just static in .cc like
> FFmpegAudioDecoder::GetAudioBuffer()?
> 
> Never mind: this is needed for video since the object is managing a frame
pool.
> 
> https://codereview.chromium.org/286953005/diff/160001/DEPS
> File DEPS (right):
> 
> https://codereview.chromium.org/286953005/diff/160001/DEPS#newcode216
> DEPS:216:
> "/chromium/third_party/ffmpeg.git@729388d1521c3ac755ca93daadb4a7b44e047e7c",
> In
>
http://git.chromium.org/gitweb/?p=chromium/third_party/ffmpeg.git;a=blob;f=li...
> , if zeros_left is < 0 prior to the STORE_BLOCK, then ILTM like an
out-of-range
> dereference of |scantable| within STORE_BLOCK could occur. Why are we dropping
> this check (even if just downstream)
>
(http://git.chromium.org/gitweb/?p=chromium/third_party/ffmpeg.git;a=blobdiff;...)
> ?
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/cast/test/sender.cc
> File media/cast/test/sender.cc (right):
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/cast/test/sender....
> media/cast/test/sender.cc:628: if (!got_picture)
> Need to free avframe before returning early here?
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/cast/test/sender....
> media/cast/test/sender.cc:650: base::Bind(&AVFreeFrame, avframe)));
> Need to av_frame_unref(avframe) prior to AVFreeFrame(avframe)?
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/cdm/ppapi/externa...
> File media/cdm/ppapi/external_clear_key/ffmpeg_cdm_audio_decoder.cc (right):
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/cdm/ppapi/externa...
> media/cdm/ppapi/external_clear_key/ffmpeg_cdm_audio_decoder.cc:274:
> av_frame_unref(av_frame_.get());
> Unref prior to first decode looks wrong to me.
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/cdm/ppapi/externa...
> File media/cdm/ppapi/external_clear_key/ffmpeg_cdm_video_decoder.cc (right):
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/cdm/ppapi/externa...
> media/cdm/ppapi/external_clear_key/ffmpeg_cdm_video_decoder.cc:229:
> av_frame_unref(av_frame_.get());
> ditto: unref prior to first decode looks wrong to me.
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/ffmpeg/ffmpeg_com...
> File media/ffmpeg/ffmpeg_common.h (right):
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/ffmpeg/ffmpeg_com...
> media/ffmpeg/ffmpeg_common.h:25: #define FF_API_PIX_FMT_DESC 0
> I would prefer these be exported by ffmpeg.gyp and used in media.gyp when
> building target(s) containing ffmpeg_common.cc, so they don't become out of
> sync. wdyt?
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/ffmpeg/ffmpeg_uni...
> File media/ffmpeg/ffmpeg_unittest.cc (right):
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/ffmpeg/ffmpeg_uni...
> media/ffmpeg/ffmpeg_unittest.cc:236: av_frame_unref(audio_buffer_.get());
> ditto: unref prior to first decode looks wrong.
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/ffmpeg/ffmpeg_uni...
> media/ffmpeg/ffmpeg_unittest.cc:290: av_frame_unref(video_buffer_.get());
> ditto
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/filters/audio_fil...
> File media/filters/audio_file_reader.cc (right):
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/filters/audio_fil...
> media/filters/audio_file_reader.cc:135: av_frame_unref(av_frame.get());
> ditto: unref prior to first decode looks wrong.
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/filters/ffmpeg_h2...
> File media/filters/ffmpeg_h264_to_annex_b_bitstream_converter.cc (right):
> 
>
https://codereview.chromium.org/286953005/diff/160001/media/filters/ffmpeg_h2...
> media/filters/ffmpeg_h264_to_annex_b_bitstream_converter.cc:57:
> av_packet_copy_props(&dest_packet, packet);
> nit: it looks like FFmpegDemuxer::OnReadFrameDone() could use similar
> av_packet_copy_props(), too.

Also: nit: fix CL title (it's not actually disabling ER yet, is it? "Roll FFmpeg
for M37" would be good to include, too, in title.

[DaleCurtis, 2014-05-20 23:00:44]

https://codereview.chromium.org/286953005/diff/160001/DEPS
File DEPS (right):

https://codereview.chromium.org/286953005/diff/160001/DEPS#newcode216
DEPS:216:
"/chromium/third_party/ffmpeg.git@729388d1521c3ac755ca93daadb4a7b44e047e7c",
On 2014/05/20 22:37:10, wolenetz wrote:
> In
>
http://git.chromium.org/gitweb/?p=chromium/third_party/ffmpeg.git;a=blob;f=li...
> , if zeros_left is < 0 prior to the STORE_BLOCK, then ILTM like an
out-of-range
> dereference of |scantable| within STORE_BLOCK could occur. Why are we dropping
> this check (even if just downstream)
>
(http://git.chromium.org/gitweb/?p=chromium/third_party/ffmpeg.git;a=blobdiff;...)
> ?

Because the discussion here indicates that this is not correct and the original
issue never repro'd in Chrome and doesn't repro in ToT FFmpeg:
http://ffmpeg.org/pipermail/ffmpeg-devel/2012-December/135409.html
http://crbug.com/161647

https://codereview.chromium.org/286953005/diff/160001/media/cast/test/sender.cc
File media/cast/test/sender.cc (right):

https://codereview.chromium.org/286953005/diff/160001/media/cast/test/sender....
media/cast/test/sender.cc:628: if (!got_picture)
On 2014/05/20 22:37:10, wolenetz wrote:
> Need to free avframe before returning early here?

Done.

https://codereview.chromium.org/286953005/diff/160001/media/cast/test/sender....
media/cast/test/sender.cc:650: base::Bind(&AVFreeFrame, avframe)));
On 2014/05/20 22:37:10, wolenetz wrote:
> Need to av_frame_unref(avframe) prior to AVFreeFrame(avframe)?

No, that's done by av_frame_free().

https://codereview.chromium.org/286953005/diff/160001/media/cdm/ppapi/externa...
File media/cdm/ppapi/external_clear_key/ffmpeg_cdm_audio_decoder.cc (right):

https://codereview.chromium.org/286953005/diff/160001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/ffmpeg_cdm_audio_decoder.cc:274:
av_frame_unref(av_frame_.get());
On 2014/05/20 22:37:10, wolenetz wrote:
> Unref prior to first decode looks wrong to me.

I agree it looks weird, but that's the API as written by FFmpeg.  I've verified
the method does the right thing.  av_frame_unref() does nothing when no
references are held:

http://git.chromium.org/gitweb/?p=chromium/third_party/ffmpeg.git;a=blob;f=li...

https://codereview.chromium.org/286953005/diff/160001/media/cdm/ppapi/externa...
File media/cdm/ppapi/external_clear_key/ffmpeg_cdm_video_decoder.cc (right):

https://codereview.chromium.org/286953005/diff/160001/media/cdm/ppapi/externa...
media/cdm/ppapi/external_clear_key/ffmpeg_cdm_video_decoder.cc:229:
av_frame_unref(av_frame_.get());
On 2014/05/20 22:37:10, wolenetz wrote:
> ditto: unref prior to first decode looks wrong to me.

Ditto.

https://codereview.chromium.org/286953005/diff/160001/media/ffmpeg/ffmpeg_com...
File media/ffmpeg/ffmpeg_common.h (right):

https://codereview.chromium.org/286953005/diff/160001/media/ffmpeg/ffmpeg_com...
media/ffmpeg/ffmpeg_common.h:25: #define FF_API_PIX_FMT_DESC 0
On 2014/05/20 22:37:10, wolenetz wrote:
> I would prefer these be exported by ffmpeg.gyp and used in media.gyp when
> building target(s) containing ffmpeg_common.cc, so they don't become out of
> sync. wdyt?

Hmm, I don't really like using direct_dependent_settings since it's not easily
discoverable.  Additionally that would apply these defines to all media source
files instead of just the ffmpeg headers.

https://codereview.chromium.org/286953005/diff/160001/media/filters/ffmpeg_h2...
File media/filters/ffmpeg_h264_to_annex_b_bitstream_converter.cc (right):

https://codereview.chromium.org/286953005/diff/160001/media/filters/ffmpeg_h2...
media/filters/ffmpeg_h264_to_annex_b_bitstream_converter.cc:57:
av_packet_copy_props(&dest_packet, packet);
On 2014/05/20 22:37:10, wolenetz wrote:
> nit: it looks like FFmpegDemuxer::OnReadFrameDone() could use similar
> av_packet_copy_props(), too.

Done.

[wolenetz, 2014-05-20 23:22:39]

PS7 lgtm

[DaleCurtis, 2014-05-20 23:24:16]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2014-05-20 23:25:44]

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

[DaleCurtis, 2014-05-21 17:57:45]

The CQ bit was unchecked by dalecurtis@chromium.org

[DaleCurtis, 2014-05-21 17:57:58]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2014-05-21 19:54:04]

Committed patchset #7 manually as r271945 (presubmit successful).

[DaleCurtis, 2014-05-21 20:40:19]

Committed patchset #8 manually as r271969 (presubmit successful).

[earthdok, 2014-05-22 14:25:31]

On 2014/05/21 20:40:19, DaleCurtis wrote:
> Committed patchset #8 manually as r271969 (presubmit successful).

This broke compile on the MSan bot:

http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Linux%20MSan/bui...

MSan uses the "linux-noasm" config which disables assembly optimizations. The
roll appears to have broken it somehow.

[DaleCurtis, 2014-05-22 17:28:49]

Hmm, weird, I was sure I generated the linux-noasm configs, I'll take a look
immediately.