Checking in media::FFmpegDemuxer and tests.

Checking in media::FFmpegDemuxer and tests.

FFmpegDemuxer is a Demuxer implemenation using FFmpeg's libavformat.  It is written in a way to work with any format and assume that the downstream decoders can interpret FFmpeg's CodecID enumerations.

[scherkus (not reviewing), 2009-03-07 00:13:17]

FYI this changelist depends on my other one:
http://codereview.chromium.org/28165/show

[Erik does not do reviews, 2009-03-09 16:38:18]

http://codereview.chromium.org/39295/diff/1/4
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/39295/diff/1/4#newcode58
Line 58: AVCodecContext* codec = stream_->codec;
you created a local var for stream_->codec, but then don't use it below

http://codereview.chromium.org/39295/diff/1/4#newcode92
Line 92: // Since our containers use scoped_refptr everything should get
released.
"containers"?  If you mean InputStream and OutputStream, say it.

http://codereview.chromium.org/39295/diff/1/4#newcode121
Line 121: demuxer_->ScheduleDemux();
calling into an object from inside the AutoLock

http://codereview.chromium.org/39295/diff/1/4#newcode169
Line 169: std::string filename = FFmpegGlue::get()->AddDataSource(data_source);
Is this a real filename on disk?  Or is it just something meta?  If it's meta,
you should call it something different so that people doing porting work don't
think they need to fix this.

http://codereview.chromium.org/39295/diff/1/4#newcode177
Line 177: FFmpegGlue::get()->RemoveDataSource(data_source);
This pattern is a bit weird (AddDataSource, open, RemoveDataSource, read).  Is
it explained somewhere?  Maybe add a "see XXX for info" is warranted here.

http://codereview.chromium.org/39295/diff/1/5
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/39295/diff/1/5#newcode44
Line 44: FFmpegDemuxerStream(FFmpegDemuxer* demuxer, AVStream* stream);
This constructor should have a comment.  Does it take ownership of the
arguments?  Or is it just holding a reference?

http://codereview.chromium.org/39295/diff/1/6
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/39295/diff/1/6#newcode195
Line 195: EXPECT_FALSE(filter_host->IsInitialized());
It would be nice if these actually had real error codes so that you could verify
that they were failing for the reason you're trying to test.  As-is, bugs in
your test will mask bugs in your code.  If you can't add error codes, after each
failure, I'd undo the specific failure part and prove success.

[scherkus (not reviewing), 2009-03-09 18:31:29]

http://codereview.chromium.org/39295/diff/1/4
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/39295/diff/1/4#newcode58
Line 58: AVCodecContext* codec = stream_->codec;
On 2009/03/09 16:38:19, Erik Kay wrote:
> you created a local var for stream_->codec, but then don't use it below

Done.

http://codereview.chromium.org/39295/diff/1/4#newcode92
Line 92: // Since our containers use scoped_refptr everything should get
released.
On 2009/03/09 16:38:19, Erik Kay wrote:
> "containers"?  If you mean InputStream and OutputStream, say it.

Done.

http://codereview.chromium.org/39295/diff/1/4#newcode121
Line 121: demuxer_->ScheduleDemux();
On 2009/03/09 16:38:19, Erik Kay wrote:
> calling into an object from inside the AutoLock

Done.

http://codereview.chromium.org/39295/diff/1/4#newcode169
Line 169: std::string filename = FFmpegGlue::get()->AddDataSource(data_source);
On 2009/03/09 16:38:19, Erik Kay wrote:
> Is this a real filename on disk?  Or is it just something meta?  If it's meta,
> you should call it something different so that people doing porting work don't
> think they need to fix this.

Right it is a key and not a real filename.

http://codereview.chromium.org/39295/diff/1/4#newcode177
Line 177: FFmpegGlue::get()->RemoveDataSource(data_source);
On 2009/03/09 16:38:19, Erik Kay wrote:
> This pattern is a bit weird (AddDataSource, open, RemoveDataSource, read).  Is
> it explained somewhere?  Maybe add a "see XXX for info" is warranted here.

We are basically transferring ownership of a data source from the glue layer to
an FFmpeg context:
 1) Add (+1 ref)
 2) open (+1 ref)
 3) Remove (-1 ref)
 4) read, etc....

I'll add some comments.

http://codereview.chromium.org/39295/diff/1/5
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/39295/diff/1/5#newcode44
Line 44: FFmpegDemuxerStream(FFmpegDemuxer* demuxer, AVStream* stream);
On 2009/03/09 16:38:19, Erik Kay wrote:
> This constructor should have a comment.  Does it take ownership of the
> arguments?  Or is it just holding a reference?

Done.

http://codereview.chromium.org/39295/diff/1/6
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/39295/diff/1/6#newcode195
Line 195: EXPECT_FALSE(filter_host->IsInitialized());
On 2009/03/09 16:38:19, Erik Kay wrote:
> It would be nice if these actually had real error codes so that you could
verify
> that they were failing for the reason you're trying to test.  As-is, bugs in
> your test will mask bugs in your code.  If you can't add error codes, after
each
> failure, I'd undo the specific failure part and prove success.

We have some error codes lying around that I really should be using.  I've
updated the tests.

[Erik does not do reviews, 2009-03-09 20:03:02]

http://codereview.chromium.org/39295/diff/2026/2030
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/39295/diff/2026/2030#newcode46
Line 46: FFmpegDemuxerStream(FFmpegDemuxer* demuxer, const AVStream* stream);
pass stream by reference

http://codereview.chromium.org/39295/diff/2026/2031
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/39295/diff/2026/2031#newcode171
Line 171: EXPECT_EQ(PIPELINE_ERROR_INITIALIZATION_FAILED, pipeline.GetError());
While this is better, it's not exactly what I meant.  The issue is that each of
these tests has exactly the same error.  How do you know in the second test that
you're not just hitting the first error again because you didn't reset variables
properly?  The end result is that only your first test actually does anything.

[scherkus (not reviewing), 2009-03-10 07:00:02]

Fixes.

http://codereview.chromium.org/39295/diff/2026/2030
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/39295/diff/2026/2030#newcode46
Line 46: FFmpegDemuxerStream(FFmpegDemuxer* demuxer, const AVStream* stream);
On 2009/03/09 20:03:02, Erik Kay wrote:
> pass stream by reference

Done.

FFmpeg gives us an AVStream* and I have a fear of dereferencing and using const
T& for anything other than std::string for some reason :)

http://codereview.chromium.org/39295/diff/2026/2031
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/39295/diff/2026/2031#newcode171
Line 171: EXPECT_EQ(PIPELINE_ERROR_INITIALIZATION_FAILED, pipeline.GetError());
On 2009/03/09 20:03:02, Erik Kay wrote:
> While this is better, it's not exactly what I meant.  The issue is that each
of
> these tests has exactly the same error.  How do you know in the second test
that
> you're not just hitting the first error again because you didn't reset
variables
> properly?  The end result is that only your first test actually does anything.

Makes sense.  I added Demuxer-specific error codes to PipelineError and now
check for those.

[Erik does not do reviews, 2009-03-10 16:05:59]

LGTM