Ability for demuxer clients to get to FFmpeg's AVStream object exposed as an ...

Ability for demuxer clients to get to FFmpeg's AVStream object exposed as an extended interface through DemuxerStream object.
DemuxerStream now derives from base::RefCountedThreadSafe.

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

[scherkus (not reviewing), 2009-03-23 20:54:41]

Still catching up on email -- will try to get this reviewed as soon as I can.

[scherkus (not reviewing), 2009-03-25 23:23:41]

nits nits nits :)

http://codereview.chromium.org/42521/diff/17/4005
File media/base/filters.h (right):

http://codereview.chromium.org/42521/diff/17/4005#newcode163
Line 163: // |Interface::interface_id()|, this method returns true and assigns
the
what are the requirements of interface_id()?

http://codereview.chromium.org/42521/diff/17/4005#newcode179
Line 179: virtual void* QueryInterface(const char* interface_id) { return NULL;
}
simply out of curiosity :)

why a string as opposed to an enum similar to our filter types?  are you
concerned with collisions? should we be worried about performance implications?

http://codereview.chromium.org/42521/diff/17/4007
File media/base/mock_media_filters.h (right):

http://codereview.chromium.org/42521/diff/17/4007#newcode268
Line 268: return mock_audio_stream_.get();
do you need .get() here?  It looks like mock_audio_stream_ is already a
scoped_refptr so maybe C++ knows how to do the conversion, if not don't worry
about it

http://codereview.chromium.org/42521/diff/17/4001
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/42521/diff/17/4001#newcode102
Line 102: if (0 == strcmp(id, interface_id())) {
should we be worried about NULL id here?

in the future I imagine we can generate this code with templates or a macro, but
it works for now :)

http://codereview.chromium.org/42521/diff/17/4002
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/42521/diff/17/4002#newcode63
Line 63: virtual AVStream* GetAVStream() {
Does this need to be virtual?

I imagine later on we can have something like an AVStreamProvider interface that
has this method and includes its own interface_id() and doesn't rely on having
it declared in ffmpeg_common.h, but this is completely functional for now

http://codereview.chromium.org/42521/diff/17/4002#newcode73
Line 73: 
remove this extra line

http://codereview.chromium.org/42521/diff/17/4009
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/42521/diff/17/4009#newcode322
Line 322: EXPECT_TRUE(video_stream->QueryInterface(&ffmpeg_demuxer_stream));
can you add EXPECT_TRUEs to check that the pointers are not NULL?

[ralphl, 2009-03-26 00:03:10]

Fixed everything Andrew commented on + fixed build break for linux and mac
caught by try servers.

http://codereview.chromium.org/42521/diff/17/4005
File media/base/filters.h (right):

http://codereview.chromium.org/42521/diff/17/4005#newcode163
Line 163: // |Interface::interface_id()|, this method returns true and assigns
the
On 2009/03/25 23:23:41, scherkus wrote:
> what are the requirements of interface_id()?

Done.

http://codereview.chromium.org/42521/diff/17/4005#newcode179
Line 179: virtual void* QueryInterface(const char* interface_id) { return NULL;
}
On 2009/03/25 23:23:41, scherkus wrote:
> simply out of curiosity :)
> 
> why a string as opposed to an enum similar to our filter types?  are you
> concerned with collisions? should we be worried about performance
implications?

I just wanted to avoid collisions, like we do in MedaiFormat.

http://codereview.chromium.org/42521/diff/17/4007
File media/base/mock_media_filters.h (right):

http://codereview.chromium.org/42521/diff/17/4007#newcode268
Line 268: return mock_audio_stream_.get();
On 2009/03/25 23:23:41, scherkus wrote:
> do you need .get() here?  It looks like mock_audio_stream_ is already a
> scoped_refptr so maybe C++ knows how to do the conversion, if not don't worry
> about it

Done.

http://codereview.chromium.org/42521/diff/17/4001
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/42521/diff/17/4001#newcode102
Line 102: if (0 == strcmp(id, interface_id())) {
On 2009/03/25 23:23:41, scherkus wrote:
> should we be worried about NULL id here?
> 
> in the future I imagine we can generate this code with templates or a macro,
but
> it works for now :)

Added DCHECK(id)

http://codereview.chromium.org/42521/diff/17/4002
File media/filters/ffmpeg_demuxer.h (right):

http://codereview.chromium.org/42521/diff/17/4002#newcode63
Line 63: virtual AVStream* GetAVStream() {
On 2009/03/25 23:23:41, scherkus wrote:
> Does this need to be virtual?
> 
> I imagine later on we can have something like an AVStreamProvider interface
that
> has this method and includes its own interface_id() and doesn't rely on having
> it declared in ffmpeg_common.h, but this is completely functional for now

Done.

http://codereview.chromium.org/42521/diff/17/4002#newcode73
Line 73: 
On 2009/03/25 23:23:41, scherkus wrote:
> remove this extra line

Done.

http://codereview.chromium.org/42521/diff/17/4009
File media/filters/ffmpeg_demuxer_unittest.cc (right):

http://codereview.chromium.org/42521/diff/17/4009#newcode322
Line 322: EXPECT_TRUE(video_stream->QueryInterface(&ffmpeg_demuxer_stream));
On 2009/03/25 23:23:41, scherkus wrote:
> can you add EXPECT_TRUEs to check that the pointers are not NULL?

Done.

[scherkus (not reviewing), 2009-03-26 00:06:26]

LGTM!