Switch to AVIO instead of a custom FFmpeg URLProtocol handler.

Switch to AVIO instead of a custom FFmpeg URLProtocol handler.

FFmpegGlue is a filthy den of sin:
- Singleton.
- Unchecked initialization.
- Mixed signed, unsigned usage.
- Requires custom FFmpeg patches.
- Hacks IO through http://0xDEADBEEF

Switching to AVIO will absolve FFmpegGlue of its sins and has the added bonus
of allowing us to tweak the buffer sizes used for read requests over the wire.

AVIO works through a special AVIOContext created through avio_alloc_context()
which is attached to the AVFormatContext used for demuxing. The AVIO context
is initialized with read and seek methods identical to the existing URLProtocol
structures.

During avformat_open_input() we tell FFmpeg to use our AVIO context by
passing NULL in for the filename parameter. FFmpeg will now redirect all reads
and seeks through our AVIO context.

The new FFmpegGlue also handles all destruction cases which can occur after
an OpenContext(), allowing us to unify the slightly disparate shutdown paths
used by FFmpegDemuxer and AudioFileReader.
 
BUG=118986, 146529
TEST=unit tests under tooling. layout tests. manual playback pass.

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

[DaleCurtis, 2012-09-27 02:17:56]

Yay! @Ronald, please sanity check my FFmpeg construct usage here. Luca informed
me about AVIO during VideoLan Dev Days.

[Ami GONE FROM CHROMIUM, 2012-09-27 03:24:45]

drive-by

I guess most of my comments are negated by the "ah crap" one.  Leaving them in
the record as incentive for if you want to do the real cleanup in another CL :)

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_demux...
File media/filters/ffmpeg_demuxer.cc (left):

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_demux...
media/filters/ffmpeg_demuxer.cc:289: format_context_ = NULL;
Yay!

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_demux...
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_demux...
media/filters/ffmpeg_demuxer.cc:40: bitstream_converter_enabled_(false) {
Can you revert this and the rest of the madness from r153204 now that
~FFmpegDemuxer() doesn't shoot pieces of FFmpegDemuxerStream out from under it?

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_demux...
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_demux...
media/filters/ffmpeg_demuxer.h:160: // FFmpegURLProtocol implementation.
What now?

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue.cc
File media/filters/ffmpeg_glue.cc (right):

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue....
media/filters/ffmpeg_glue.cc:174: avcodec_close(stream->codec);
Ah crap - is this once again freeing out stuff from under FFmpegDemuxerStream?? 
Can we avoid that?

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue.h
File media/filters/ffmpeg_glue.h (right):

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue....
media/filters/ffmpeg_glue.h:5: // FFmpegGlue is an adapter for FFmpeg's
URLProtocol interface that allows us to
overhaul file-level comment

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue....
media/filters/ffmpeg_glue.h:40: class MEDIA_EXPORT FFmpegURLProtocol {
wants renaming?

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue....
media/filters/ffmpeg_glue.h:66: virtual ~FFmpegGlue();
why virtual?

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue....
media/filters/ffmpeg_glue.h:68: int OpenContext();
why not return a bool for success?
(here and elsewhere in this file)

[DaleCurtis, 2012-09-27 03:46:11]

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_demux...
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_demux...
media/filters/ffmpeg_demuxer.h:160: // FFmpegURLProtocol implementation.
On 2012/09/27 03:24:45, Ami Fischman wrote:
> What now?

Figured the rename is best done in another CL / after initial review for diff
clarity.

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue.cc
File media/filters/ffmpeg_glue.cc (right):

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue....
media/filters/ffmpeg_glue.cc:174: avcodec_close(stream->codec);
On 2012/09/27 03:24:45, Ami Fischman wrote:
> Ah crap - is this once again freeing out stuff from under
FFmpegDemuxerStream?? 
> Can we avoid that?

I'll need to take a closer look at how FFmpegDemuxer, FFmpegDemuxerStream, and
the bit stream reader are interacting.  It may not be unreasonable to move the
stream closures into FFmpegDemuxerStream, but I believe we'll still need to
close format context in FFmpegDemuxer.

Another option would be ref counting (eww) FFmpegGlue and passing a ref to each
stream.

We can discuss the issue tomorrow and see what fits.

[rbultje1, 2012-09-27 16:20:24]

On 2012/09/27 02:17:56, DaleCurtis wrote:
> Yay! @Ronald, please sanity check my FFmpeg construct usage here.

I see no obvious errors. Nice cleanup!

[DaleCurtis, 2012-09-28 23:17:41]

https://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue.cc
File media/filters/ffmpeg_glue.cc (right):

https://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue...
media/filters/ffmpeg_glue.cc:174: avcodec_close(stream->codec);
On 2012/09/27 03:46:12, DaleCurtis wrote:
> On 2012/09/27 03:24:45, Ami Fischman wrote:
> > Ah crap - is this once again freeing out stuff from under
> FFmpegDemuxerStream?? 
> > Can we avoid that?
> 
> I'll need to take a closer look at how FFmpegDemuxer, FFmpegDemuxerStream, and
> the bit stream reader are interacting.  It may not be unreasonable to
move the
> stream closures into FFmpegDemuxerStream, but I believe we'll still need to
> close format context in FFmpegDemuxer.
> 
> Another option would be ref counting (eww) FFmpegGlue and passing a ref to
each
> stream.
> 
> We can discuss the issue tomorrow and see what fits.

Discussed offline. The problem is FFmpegDemuxer does not own FFmpegDemuxerStream
and thus during shutdown things can end up such that FFmpegDemuxer is destoryed
before FFmpegDemuxerStream.

Short term we'll keep the existing hacks in place, but longer term
FFmpegDemuxer/FFmpegDemuxerStream should stop being refcounted in favor of
explicit ownership.

fischman filed http://crbug.com/153087 to track this.

[scherkus (not reviewing), 2012-10-02 00:20:56]

do you remember why we call FFmpegGlue::InitializeFFmpeg() everywhere? is it
"just in case"?

http://codereview.chromium.org/10912080/diff/10001/media/filters/audio_file_r...
File media/filters/audio_file_reader.cc (right):

http://codereview.chromium.org/10912080/diff/10001/media/filters/audio_file_r...
media/filters/audio_file_reader.cc:43: DCHECK(glue_.get());
dcheck not useful

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_audio...
File media/filters/ffmpeg_audio_decoder.cc (right):

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_audio...
media/filters/ffmpeg_audio_decoder.cc:57: FFmpegGlue::InitializeFFmpeg();
do you know why we still need this?

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_demux...
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_demux...
media/filters/ffmpeg_demuxer.h:267: // Binding glue for interfacing with FFmpeg.
I'd ditch the comment -- you haven't commented it in other instances and the
name is pretty self-explanatory

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue.cc
File media/filters/ffmpeg_glue.cc (right):

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue....
media/filters/ffmpeg_glue.cc:136: open_called_ = true;
do we need to worry about calling OpenContext() multiple times?

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue....
media/filters/ffmpeg_glue.cc:137: // By passing NULL for the filename (second
parameter) we are telling FFmpeg
blank line before

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue_...
File media/filters/ffmpeg_glue_unittest.cc (right):

http://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue_...
media/filters/ffmpeg_glue_unittest.cc:91: // |glue_| should be destroyed before
|protocol_|.
nit: blank lines before // comments to avoid wall-of-text

[DaleCurtis, 2012-10-02 01:24:23]

Pretty sure it's just in case. Maybe at one time it was necessary because the
unit tests didn't call it. Tests pass fine w/o it now. Remove?

https://codereview.chromium.org/10912080/diff/10001/media/filters/audio_file_...
File media/filters/audio_file_reader.cc (right):

https://codereview.chromium.org/10912080/diff/10001/media/filters/audio_file_...
media/filters/audio_file_reader.cc:43: DCHECK(glue_.get());
On 2012/10/02 00:20:56, scherkus wrote:
> dcheck not useful

Done.

https://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_audi...
File media/filters/ffmpeg_audio_decoder.cc (right):

https://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_audi...
media/filters/ffmpeg_audio_decoder.cc:57: FFmpegGlue::InitializeFFmpeg();
On 2012/10/02 00:20:56, scherkus wrote:
> do you know why we still need this?

I think it's just a safety check. The unittests need to call it at least once,
but both the audio and video decoder unittests do so.  We can probably remove
it.

https://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_demu...
File media/filters/ffmpeg_demuxer.h (right):

https://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_demu...
media/filters/ffmpeg_demuxer.h:267: // Binding glue for interfacing with FFmpeg.
On 2012/10/02 00:20:56, scherkus wrote:
> I'd ditch the comment -- you haven't commented it in other instances and the
> name is pretty self-explanatory

Done.

https://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue.cc
File media/filters/ffmpeg_glue.cc (right):

https://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue...
media/filters/ffmpeg_glue.cc:136: open_called_ = true;
On 2012/10/02 00:20:56, scherkus wrote:
> do we need to worry about calling OpenContext() multiple times?

Good point. It shouldn't be done in any of our existing use cases. I'll add a
DCHECK().  If anyone needs to they can just claim a new FFmpegGlue.

https://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue...
media/filters/ffmpeg_glue.cc:137: // By passing NULL for the filename (second
parameter) we are telling FFmpeg
On 2012/10/02 00:20:56, scherkus wrote:
> blank line before

Done.

https://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue.h
File media/filters/ffmpeg_glue.h (right):

https://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue...
media/filters/ffmpeg_glue.h:5: // FFmpegGlue is an adapter for FFmpeg's
URLProtocol interface that allows us to
On 2012/09/27 03:24:45, Ami Fischman wrote:
> overhaul file-level comment

Done.

https://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue...
media/filters/ffmpeg_glue.h:66: virtual ~FFmpegGlue();
On 2012/09/27 03:24:45, Ami Fischman wrote:
> why virtual?

Done.

https://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue...
media/filters/ffmpeg_glue.h:68: int OpenContext();
On 2012/09/27 03:24:45, Ami Fischman wrote:
> why not return a bool for success?
> (here and elsewhere in this file)

Done.

https://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue...
File media/filters/ffmpeg_glue_unittest.cc (right):

https://codereview.chromium.org/10912080/diff/10001/media/filters/ffmpeg_glue...
media/filters/ffmpeg_glue_unittest.cc:91: // |glue_| should be destroyed before
|protocol_|.
On 2012/10/02 00:20:56, scherkus wrote:
> nit: blank lines before // comments to avoid wall-of-text

Done.

[scherkus (not reviewing), 2012-10-02 01:36:35]

lgtm w/ comment

https://codereview.chromium.org/10912080/diff/27001/media/filters/ffmpeg_glue.h
File media/filters/ffmpeg_glue.h (right):

https://codereview.chromium.org/10912080/diff/27001/media/filters/ffmpeg_glue...
media/filters/ffmpeg_glue.h:64: explicit FFmpegGlue(FFmpegURLProtocol*
protocol);
seems like you may want to mention that the lifetime of |protocol| must exceed
the lifetime of FFmpegGlue

[DaleCurtis, 2012-10-02 01:46:30]

https://codereview.chromium.org/10912080/diff/27001/media/filters/ffmpeg_glue.h
File media/filters/ffmpeg_glue.h (right):

https://codereview.chromium.org/10912080/diff/27001/media/filters/ffmpeg_glue...
media/filters/ffmpeg_glue.h:64: explicit FFmpegGlue(FFmpegURLProtocol*
protocol);
On 2012/10/02 01:36:35, scherkus wrote:
> seems like you may want to mention that the lifetime of |protocol| must exceed
> the lifetime of FFmpegGlue

Whoops, documented everywhere in the unit tests, but not here :)

[commit-bot: I haz the power, 2012-10-02 01:46:51]

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

[commit-bot: I haz the power, 2012-10-02 04:12:11]

Retried try job too often for step(s) sync_integration_tests, content_unittests,
browser_tests, chrome_frame_net_tests

[DaleCurtis, 2012-10-06 00:07:55]

On 2012/10/02 04:12:11, I haz the power (commit-bot) wrote:
> Retried try job too often for step(s) sync_integration_tests,
content_unittests,
> browser_tests, chrome_frame_net_tests

Argh!!! I setup the .sigs incorrectly on the last roll, the avio_* sigs should
be in avformat-54.sigs not avcodec-54.sigs. Which means windows can't find its
delay loaded dependency at run time :(

This will have to wait to land until after the MSVC work, which will thankfully
merge all the sigs into one so no one will ever have to deal with this again!

[DaleCurtis, 2012-10-22 20:43:48]

On 2012/10/06 00:07:55, DaleCurtis wrote:
> On 2012/10/02 04:12:11, I haz the power (commit-bot) wrote:
> > Retried try job too often for step(s) sync_integration_tests,
> content_unittests,
> > browser_tests, chrome_frame_net_tests
> 
> Argh!!! I setup the .sigs incorrectly on the last roll, the avio_* sigs should
> be in avformat-54.sigs not avcodec-54.sigs. Which means windows can't find its
> delay loaded dependency at run time :(
> 
> This will have to wait to land until after the MSVC work, which will
thankfully
> merge all the sigs into one so no one will ever have to deal with this again!

Will land this once I know the ffmpeg roll is sticking (e.g. after today's
canary crash rates come in)

[commit-bot: I haz the power, 2012-11-01 19:34:45]

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

[commit-bot: I haz the power, 2012-11-01 21:23:36]

Change committed as 165502