Checking in media::FFmpegGlue and some common FFmpeg code.

Checking in media::FFmpegGlue and some common FFmpeg code.

FFmpegGlue acts as an adapter between FFmpeg's URLProtocol and the media::DataSource interface, allowing us to use media::DataSource implementations for handling FFmpeg's IO.

[scherkus (not reviewing), 2009-02-26 01:47:13]

More potentially confusing code for you to look at, but your feedback has been
really helpful.  This time it's some code to glue FFmpeg's C-style IO to our C++
interfaces.  It includes a unit test, which is something I should write more of
:)

[Erik does not do reviews, 2009-02-26 18:50:24]

Thanks for writing the unit test.  I should have given you a harder time on the
previous reviews.

http://codereview.chromium.org/28165/diff/3053/33
File media/filters/ffmpeg_common.h (right):

http://codereview.chromium.org/28165/diff/3053/33#newcode8
Line 8: #pragma warning(disable: 4244)
Even for third party code, whenever you disable a warning, a comment would be
helpful.
Also, you need to wrap these pragma lines with #if defined(OS_WIN) since other
compilers obviously don't recognize this syntax.

http://codereview.chromium.org/28165/diff/3053/34
File media/filters/ffmpeg_glue.cc (right):

http://codereview.chromium.org/28165/diff/3053/34#newcode5
Line 5: #include <cerrno>
where is this used?

http://codereview.chromium.org/28165/diff/3053/34#newcode12
Line 12: namespace media {
Rather than putting the protocol methods in namespace media as statics, instead
put them in an anonymous namespace (no static needed).  It's a bit more clear.

http://codereview.chromium.org/28165/diff/3053/34#newcode14
Line 14: // FFmpeg protocol interface.
Although these are part of the interface, you're passing in function pointers,
so please follow Google naming conventions.

http://codereview.chromium.org/28165/diff/3053/34#newcode49
Line 49: : lock_(),
no need to wrap these initializers

http://codereview.chromium.org/28165/diff/3053/34#newcode59
Line 59: FFmpegGlue::~FFmpegGlue() {
Is there a way to ensure that FFmpeg is shut down at this point?  Even if you'd
prefer to actually do it somewhere else, a DCHECK seems useful.

http://codereview.chromium.org/28165/diff/3053/34#newcode64
Line 64: data_source->Release();
Rather than managing this manually, just make your map use
scoped_refptr<DataSource> for the second.  It'll clean up your code a lot.

http://codereview.chromium.org/28165/diff/3053/34#newcode95
Line 95: std::ostringstream str;
Almost all of our code uses string_util/SStringPrintf rather than STL for this
kind of construction.  Nothing wrong with this in particular, it just stands out
as being different  from most of the rest of our code (and slightly less
readable as a result).

http://codereview.chromium.org/28165/diff/3053/34#newcode105
Line 105: // TODO(scherkus): data source should be able to tell us if we're
streaming.
move this next to the is_streamed line.  it feels disconnected here.

http://codereview.chromium.org/28165/diff/3053/34#newcode116
Line 116: int result = data_source->Read(reinterpret_cast<char*>(buf), size);
Maybe DataSource should use uint8 instead of char*?

http://codereview.chromium.org/28165/diff/3053/35
File media/filters/ffmpeg_glue.h (right):

http://codereview.chromium.org/28165/diff/3053/35#newcode53
Line 53: // FFmpeg protocol interface.
These methods are called by your own code, so there's no reason they can't
follow Google naming conventions (capitalized).  Also, since they're only called
internally by the static functions that you passed to FFmpeg, they should be
made private.  They're not designed to be called outside of this class, right?

http://codereview.chromium.org/28165/diff/3053/36
File media/filters/ffmpeg_glue_unittest.cc (right):

http://codereview.chromium.org/28165/diff/3053/36#newcode5
Line 5: #include <cerrno>
where is this used?

http://codereview.chromium.org/28165/diff/3053/36#newcode41
Line 41: class TestDataSource : public DataSource {
this class doesn't seem specific to ffmpeg.  pull it out into a shared object
for other tests?

http://codereview.chromium.org/28165/diff/3053/36#newcode130
Line 130: // NOTE: this test must be the first test in order to properly test
creating
This feels quite fragile.  What if another unit test winds up needing to use
ffmpeg in the future?  I don't think there's a good way to guarantee test order.
 Maybe you can just get rid of the EXPECT_FALSE tests at the beginning.  Then
you're not dependent on order.

http://codereview.chromium.org/28165/diff/3053/36#newcode174
Line 174: // Remove the data sources and then releases our references.  They
should be
nit: Remove -> Removes

http://codereview.chromium.org/28165/diff/3053/36#newcode225
Line 225: const size_t kDataSize = arraysize(data)-1;
space around operator

[scherkus (not reviewing), 2009-02-26 23:39:27]

All fixed up!

Moved TestDataSource to its own file as MockDataSource (no changes other than
naming).

Cleaned up FFmpegGlue a bunch by adding a GetDataSource() method.

http://codereview.chromium.org/28165/diff/3053/33
File media/filters/ffmpeg_common.h (right):

http://codereview.chromium.org/28165/diff/3053/33#newcode8
Line 8: #pragma warning(disable: 4244)
On 2009/02/26 18:50:24, Erik Kay wrote:
> Even for third party code, whenever you disable a warning, a comment would be
> helpful.
> Also, you need to wrap these pragma lines with #if defined(OS_WIN) since other
> compilers obviously don't recognize this syntax.

Found out there are macros for these in base/compiler_specific.h

Also, I'll try to upstream changes to fix the warnings.

http://codereview.chromium.org/28165/diff/3053/34
File media/filters/ffmpeg_glue.cc (right):

http://codereview.chromium.org/28165/diff/3053/34#newcode5
Line 5: #include <cerrno>
On 2009/02/26 18:50:24, Erik Kay wrote:
> where is this used?

FFmpeg's error codes (AVERROR_IO) are actually macros based on <cerrno>.  I'll
move the <cerrno> include to ffmpeg_common.h

http://codereview.chromium.org/28165/diff/3053/34#newcode12
Line 12: namespace media {
On 2009/02/26 18:50:24, Erik Kay wrote:
> Rather than putting the protocol methods in namespace media as statics,
instead
> put them in an anonymous namespace (no static needed).  It's a bit more clear.

Done.

http://codereview.chromium.org/28165/diff/3053/34#newcode14
Line 14: // FFmpeg protocol interface.
On 2009/02/26 18:50:24, Erik Kay wrote:
> Although these are part of the interface, you're passing in function pointers,
> so please follow Google naming conventions.

Done.

http://codereview.chromium.org/28165/diff/3053/34#newcode64
Line 64: data_source->Release();
On 2009/02/26 18:50:24, Erik Kay wrote:
> Rather than managing this manually, just make your map use
> scoped_refptr<DataSource> for the second.  It'll clean up your code a lot.

Done.

http://codereview.chromium.org/28165/diff/3053/34#newcode95
Line 95: std::ostringstream str;
On 2009/02/26 18:50:24, Erik Kay wrote:
> Almost all of our code uses string_util/SStringPrintf rather than STL for this
> kind of construction.  Nothing wrong with this in particular, it just stands
out
> as being different  from most of the rest of our code (and slightly less
> readable as a result).

Done.

http://codereview.chromium.org/28165/diff/3053/34#newcode105
Line 105: // TODO(scherkus): data source should be able to tell us if we're
streaming.
On 2009/02/26 18:50:24, Erik Kay wrote:
> move this next to the is_streamed line.  it feels disconnected here.

Done.

http://codereview.chromium.org/28165/diff/3053/34#newcode116
Line 116: int result = data_source->Read(reinterpret_cast<char*>(buf), size);
On 2009/02/26 18:50:24, Erik Kay wrote:
> Maybe DataSource should use uint8 instead of char*?

Yep -- I've got this tracked as a work item :)

http://codereview.chromium.org/28165/diff/3053/35
File media/filters/ffmpeg_glue.h (right):

http://codereview.chromium.org/28165/diff/3053/35#newcode53
Line 53: // FFmpeg protocol interface.
On 2009/02/26 18:50:24, Erik Kay wrote:
> These methods are called by your own code, so there's no reason they can't
> follow Google naming conventions (capitalized).  Also, since they're only
called
> internally by the static functions that you passed to FFmpeg, they should be
> made private.  They're not designed to be called outside of this class, right?

Looking through this again, it turns out there is no reason for these methods to
even be part of this class, other than open().  However, open really just needs
to look up a DataSource* via the filename.

I've added a GetDataSource(const std::string& key) and moved these methods into
the static methods themselves.

http://codereview.chromium.org/28165/diff/3053/36
File media/filters/ffmpeg_glue_unittest.cc (right):

http://codereview.chromium.org/28165/diff/3053/36#newcode5
Line 5: #include <cerrno>
On 2009/02/26 18:50:24, Erik Kay wrote:
> where is this used?

moved to ffmpeg_common.h + comment

http://codereview.chromium.org/28165/diff/3053/36#newcode41
Line 41: class TestDataSource : public DataSource {
On 2009/02/26 18:50:24, Erik Kay wrote:
> this class doesn't seem specific to ffmpeg.  pull it out into a shared object
> for other tests?

Moved to its own header file, mock_data_source.h

http://codereview.chromium.org/28165/diff/3053/36#newcode130
Line 130: // NOTE: this test must be the first test in order to properly test
creating
On 2009/02/26 18:50:24, Erik Kay wrote:
> This feels quite fragile.  What if another unit test winds up needing to use
> ffmpeg in the future?  I don't think there's a good way to guarantee test
order.
>  Maybe you can just get rid of the EXPECT_FALSE tests at the beginning.  Then
> you're not dependent on order.

I was also wondering about this.  I'll remove the EXPECT_FALSE checks.

http://codereview.chromium.org/28165/diff/3053/36#newcode174
Line 174: // Remove the data sources and then releases our references.  They
should be
On 2009/02/26 18:50:24, Erik Kay wrote:
> nit: Remove -> Removes

Done.

http://codereview.chromium.org/28165/diff/3053/36#newcode225
Line 225: const size_t kDataSize = arraysize(data)-1;
On 2009/02/26 18:50:24, Erik Kay wrote:
> space around operator

Done.

[Erik does not do reviews, 2009-02-27 00:13:17]

overall looking good.  Feels much better.

http://codereview.chromium.org/28165/diff/3070/3076
File media/filters/ffmpeg_glue.cc (right):

http://codereview.chromium.org/28165/diff/3070/3076#newcode14
Line 14: media::DataSource* data_source =
for safety, you need to used a scoped_refptr here   otherwise it might get
released out from under you between here and the addref.  in fact, it also needs
to be returned as a scoped_refptr from GetDataSource (or you need to pass the
scoped_refptr to the method as an in argument).

http://codereview.chromium.org/28165/diff/3070/3076#newcode28
Line 28: media::DataSource* data_source =
ditto

http://codereview.chromium.org/28165/diff/3070/3076#newcode42
Line 42: media::DataSource* data_source =
ditto

http://codereview.chromium.org/28165/diff/3070/3076#newcode80
Line 80: media::DataSource* data_source =
ditto

http://codereview.chromium.org/28165/diff/3070/3076#newcode120
Line 120: data_source->Release();
isn't this a double release?  (erase should have called the destructor)

http://codereview.chromium.org/28165/diff/3070/3076#newcode145
Line 145: DataSource* FFmpegGlue::GetDataSource(const std::string& key) {
see earlier comment about returning a scoped_refptr

http://codereview.chromium.org/28165/diff/3070/3076#newcode158
Line 158: return StringPrintf("%s://0x%lx", kProtocol,
static_cast<void*>(data_source));
Just curious, does this really need to be a valid URL?  Does FFmpeg validate the
URL or use it for any purpose?  Or is it just a string?  It definitely feels
very hacky.

http://codereview.chromium.org/28165/diff/3070/3079
File media/filters/mock_data_source.h (right):

http://codereview.chromium.org/28165/diff/3070/3079#newcode104
Line 104: };
missing DISALLOW_COPY_AND_ASSIGN

[scherkus (not reviewing), 2009-02-27 02:18:20]

Updated + one more unit test for destructor code.

Also realized that C++ is smart and automatically calls turns my scoped_refptr
into raw pointers so I removed my get()s.

http://codereview.chromium.org/28165/diff/3070/3076
File media/filters/ffmpeg_glue.cc (right):

http://codereview.chromium.org/28165/diff/3070/3076#newcode14
Line 14: media::DataSource* data_source =
On 2009/02/27 00:13:18, Erik Kay wrote:
> for safety, you need to used a scoped_refptr here   otherwise it might get
> released out from under you between here and the addref.  in fact, it also
needs
> to be returned as a scoped_refptr from GetDataSource (or you need to pass the
> scoped_refptr to the method as an in argument).

Done.

http://codereview.chromium.org/28165/diff/3070/3076#newcode28
Line 28: media::DataSource* data_source =
On 2009/02/27 00:13:18, Erik Kay wrote:
> ditto
we're not re-getting the source here, just using it assuming it has been
properly AddRef()'d by OpenContext()

http://codereview.chromium.org/28165/diff/3070/3076#newcode42
Line 42: media::DataSource* data_source =
On 2009/02/27 00:13:18, Erik Kay wrote:
> ditto
we're not re-getting the source here, just using it assuming it has been
properly AddRef()'d by OpenContext()

http://codereview.chromium.org/28165/diff/3070/3076#newcode80
Line 80: media::DataSource* data_source =
On 2009/02/27 00:13:18, Erik Kay wrote:
> ditto
we're not re-getting the source here, just using it assuming it has been
properly AddRef()'d by OpenContext()

http://codereview.chromium.org/28165/diff/3070/3076#newcode120
Line 120: data_source->Release();
On 2009/02/27 00:13:18, Erik Kay wrote:
> isn't this a double release?  (erase should have called the destructor)
Great catch!  I added a test that relies on this destructor to release the
object so we get to run this code.  My unit test crashed at the end before
removing this line.

http://codereview.chromium.org/28165/diff/3070/3076#newcode145
Line 145: DataSource* FFmpegGlue::GetDataSource(const std::string& key) {
On 2009/02/27 00:13:18, Erik Kay wrote:
> see earlier comment about returning a scoped_refptr

Done.

http://codereview.chromium.org/28165/diff/3070/3076#newcode158
Line 158: return StringPrintf("%s://0x%lx", kProtocol,
static_cast<void*>(data_source));
On 2009/02/27 00:13:18, Erik Kay wrote:
> Just curious, does this really need to be a valid URL?  Does FFmpeg validate
the
> URL or use it for any purpose?  Or is it just a string?  It definitely feels
> very hacky.
FFmpeg looks for the protocol in the filename (i.e., everything before the "://"
part) and then checks its list of registered URLProtocols for a matching
protocol string (http in our case).  It then calls the URLProtocol's url_open
function pointer with the filename and flags.

I agree it feels a little hacky, but I suppose we could use any arbitrary
protocol, such as "media://" or "hacks://".

http://codereview.chromium.org/28165/diff/3070/3079
File media/filters/mock_data_source.h (right):

http://codereview.chromium.org/28165/diff/3070/3079#newcode104
Line 104: };
On 2009/02/27 00:13:18, Erik Kay wrote:
> missing DISALLOW_COPY_AND_ASSIGN

Done.

[scherkus (not reviewing), 2009-03-06 22:56:34]

I merged in some other changes by ralphl and myself.

Instead of a new MockDataSource class we now re-use the one declared in
media/base/mock_media_filters.h