Remove mock_ffmpeg and update media unittests.

Remove mock_ffmpeg and update media unittests.


BUG=92429
TEST=BitstreamConverterTest.*, ChunkDemuxerTest.*, FFmpegDemuxerTest.*, FFmpegGlueTest.*, FFmpegVideoDecoderTest.*, FFmpegH264BitstreamConverterTest.*, FFmpegVideoDecodeEngineTest.*

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

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

[acolwell GONE FROM CHROMIUM, 2011-08-10 23:59:04]

BOOM!

[scherkus (not reviewing), 2011-08-11 01:26:29]

initial pass -- actually pretty easy to review!

the biggest comment is how to best tackle the issue of loading FFmpeg
considering that internal FFmpeg state can now get dragged across multiple tests
leading to some potentially really nasty test failures

http://codereview.chromium.org/7587012/diff/1/media/base/media.h
File media/base/media.h (right):

http://codereview.chromium.org/7587012/diff/1/media/base/media.h#newcode29
media/base/media.h:29: bool InitializeMediaLibraryForTesting();
It looks like this gets called in many places.

Would it make sense to perhaps make a single call / inline the call inside of
media/base/run_all_unittests.cc itself?

Perhaps with a CHECK() and useful diagnostic error message

it's also worth noting we need to be careful that ffmpeg doesn't retain any
state between tests -- does re-loading the library take care of that?

There's a few options here we can consider:
  1) Extending base::TestSuite() to have custom Initialize/Shutdown() impls
  2) Extending testing::Test to have a testing::FFmpegTest that calls
InitializeMediaLibraryForTesting()
  3) Use ::testing::Environment:
http://code.google.com/p/googletest/wiki/AdvancedGuide#Global_Set-Up_and_Tear...
  4) ???

http://codereview.chromium.org/7587012/diff/1/media/base/test_data_util.h
File media/base/test_data_util.h (right):

http://codereview.chromium.org/7587012/diff/1/media/base/test_data_util.h#new...
media/base/test_data_util.h:16: bool ReadTestDataFile(const std::string& name,
docs

http://codereview.chromium.org/7587012/diff/1/media/base/test_data_util.h#new...
media/base/test_data_util.h:17: scoped_array<uint8>* buffer,
is the scoped_array out ptr intended to be a helpful reminder to free the
memory?

http://codereview.chromium.org/7587012/diff/1/media/base/test_data_util.h#new...
media/base/test_data_util.h:20: bool ReadTestDataFile(const std::string& name,
scoped_refptr<Buffer>* buffer);
I notice that everyone EXPECT_TRUEs these functions

would we ever have a case where we expect a file not to load?

can these be voids with CHECKs instead?  It seems that if a file can't load,
then most/all of the tests can't run

http://codereview.chromium.org/7587012/diff/1/media/filters/bitstream_convert...
File media/filters/bitstream_converter_unittest.cc (right):

http://codereview.chromium.org/7587012/diff/1/media/filters/bitstream_convert...
media/filters/bitstream_converter_unittest.cc:18: static int
DoFilter(AVBitStreamFilterContext *bsfc,
nit: instead of having everything be a static inside the test class, perhaps
just make it all static and do away with the BitstreamConverterTest:: mess and
prototype decls

http://codereview.chromium.org/7587012/diff/1/media/filters/bitstream_convert...
media/filters/bitstream_converter_unittest.cc:38:
InitializeMediaLibrary(file_path);
InitializeMediaLibraryForTest() ?

http://codereview.chromium.org/7587012/diff/1/media/filters/bitstream_convert...
media/filters/bitstream_converter_unittest.cc:41:
av_register_bitstream_filter(&stream_filter_);
does this prevent duplicate registration?

perhaps you want SetUpTestCase():
http://code.google.com/p/googletest/wiki/AdvancedGuide#Sharing_Resources_Betw...

http://codereview.chromium.org/7587012/diff/1/media/filters/bitstream_convert...
media/filters/bitstream_converter_unittest.cc:77: //static
// static

http://codereview.chromium.org/7587012/diff/1/media/filters/bitstream_convert...
media/filters/bitstream_converter_unittest.cc:78: int
BitstreamConverterTest::DoFilter(AVBitStreamFilterContext *bsfc,
pointers w/ types + docs on what this test filter does

http://codereview.chromium.org/7587012/diff/1/media/filters/bitstream_convert...
media/filters/bitstream_converter_unittest.cc:101: //static
// static

http://codereview.chromium.org/7587012/diff/1/media/filters/chunk_demuxer_uni...
File media/filters/chunk_demuxer_unittest.cc (left):

http://codereview.chromium.org/7587012/diff/1/media/filters/chunk_demuxer_uni...
media/filters/chunk_demuxer_unittest.cc:65: codecs_[VIDEO].codec_type =
AVMEDIA_TYPE_VIDEO;
my god this is amazing

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

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_demuxer_un...
media/filters/ffmpeg_demuxer_unittest.cc:68:
EXPECT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &file_path));
can we use the ReadTestData functions here?

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_demuxer_un...
media/filters/ffmpeg_demuxer_unittest.cc:91: void ValidateBuffer(const
tracked_objects::Location& location,
docs? what does this do exactly?

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

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_glue.h#new...
media/filters/ffmpeg_glue.h:41: FFmpegURLProtocol() {
want to clean these up to {}

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_glue_unitt...
File media/filters/ffmpeg_glue_unittest.cc (right):

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_glue_unitt...
media/filters/ffmpeg_glue_unittest.cc:40: virtual void SetUp() {
Shouldn't this test initialize the media library?

Also I wonder whether this bit of code get changed to static SetUpTestCase()
since it deals with a singleton and whatnot... if it works fine no real need to
change it

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_h264_bitst...
File media/filters/ffmpeg_h264_bitstream_converter_unittest.cc (left):

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_h264_bitst...
media/filters/ffmpeg_h264_bitstream_converter_unittest.cc:359:
TEST_F(FFmpegH264BitstreamConverterTest, Conversion_FailureOutOfMem) {
yup useless test :)

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_video_deco...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_video_deco...
media/filters/ffmpeg_video_decoder_unittest.cc:138: class FFmpegVideoDecoderTest
: public testing::Test {
heh this test should probably use the real FFmpegVideoDecodeEngine :P

but that's for another day

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_video_deco...
media/filters/ffmpeg_video_decoder_unittest.cc:143:
InitializeMediaLibrary(file_path);
ForTest() ?

http://codereview.chromium.org/7587012/diff/1/media/video/ffmpeg_video_decode...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/7587012/diff/1/media/video/ffmpeg_video_decode...
media/video/ffmpeg_video_decode_engine.cc:138: if (i >= 0) {
woah!??

what's all this doing

http://codereview.chromium.org/7587012/diff/1/media/video/ffmpeg_video_decode...
File media/video/ffmpeg_video_decode_engine_unittest.cc (right):

http://codereview.chromium.org/7587012/diff/1/media/video/ffmpeg_video_decode...
media/video/ffmpeg_video_decode_engine_unittest.cc:53:
FFmpegGlue::GetInstance();
is this to initialize the glue?

hrmm.... wonder if perhaps we should make this part of global set up

http://codereview.chromium.org/7587012/diff/1/media/video/ffmpeg_video_decode...
media/video/ffmpeg_video_decode_engine_unittest.cc:168: scoped_refptr<Buffer>
bufferA = new DataBuffer(1);
buffer_a

http://codereview.chromium.org/7587012/diff/1/media/video/ffmpeg_video_decode...
media/video/ffmpeg_video_decode_engine_unittest.cc:190: // Corrupt bytes by
flipping bits w/ xor.
:)

http://codereview.chromium.org/7587012/diff/1/media/video/ffmpeg_video_decode...
media/video/ffmpeg_video_decode_engine_unittest.cc:264:
EXPECT_TRUE(ReadTestDataFile("vp8-I-frame-320x120", &buffer));
wow very nice!

[acolwell GONE FROM CHROMIUM, 2011-08-11 23:54:40]

http://codereview.chromium.org/7587012/diff/1/media/base/media.h
File media/base/media.h (right):

http://codereview.chromium.org/7587012/diff/1/media/base/media.h#newcode29
media/base/media.h:29: bool InitializeMediaLibraryForTesting();
On 2011/08/11 01:26:29, scherkus wrote:
> It looks like this gets called in many places.
> 
> Would it make sense to perhaps make a single call / inline the call inside of
> media/base/run_all_unittests.cc itself?

Done.

> 
> Perhaps with a CHECK() and useful diagnostic error message
> 
> it's also worth noting we need to be careful that ffmpeg doesn't retain any
> state between tests -- does re-loading the library take care of that?

This is tricky because ffmpeg doesn't provide ways to unregister protocol
handlers and filters. Unloading and reloading might help a little bit. Aside
from registering the URL handler and the test filter, I don't believe FFmpeg
retains any other state between tests.

> 
> There's a few options here we can consider:
>   1) Extending base::TestSuite() to have custom Initialize/Shutdown() impls
>   2) Extending testing::Test to have a testing::FFmpegTest that calls
> InitializeMediaLibraryForTesting()
>   3) Use ::testing::Environment:
>
http://code.google.com/p/googletest/wiki/AdvancedGuide#Global_Set-Up_and_Tear...
>   4) ???

http://codereview.chromium.org/7587012/diff/1/media/base/test_data_util.h
File media/base/test_data_util.h (right):

http://codereview.chromium.org/7587012/diff/1/media/base/test_data_util.h#new...
media/base/test_data_util.h:16: bool ReadTestDataFile(const std::string& name,
On 2011/08/11 01:26:29, scherkus wrote:
> docs

Done.

http://codereview.chromium.org/7587012/diff/1/media/base/test_data_util.h#new...
media/base/test_data_util.h:17: scoped_array<uint8>* buffer,
On 2011/08/11 01:26:29, scherkus wrote:
> is the scoped_array out ptr intended to be a helpful reminder to free the
> memory?
yes

http://codereview.chromium.org/7587012/diff/1/media/base/test_data_util.h#new...
media/base/test_data_util.h:20: bool ReadTestDataFile(const std::string& name,
scoped_refptr<Buffer>* buffer);
On 2011/08/11 01:26:29, scherkus wrote:
> I notice that everyone EXPECT_TRUEs these functions
> 
> would we ever have a case where we expect a file not to load?
> 
> can these be voids with CHECKs instead?  It seems that if a file can't load,
> then most/all of the tests can't run

Done.

http://codereview.chromium.org/7587012/diff/1/media/filters/bitstream_convert...
File media/filters/bitstream_converter_unittest.cc (right):

http://codereview.chromium.org/7587012/diff/1/media/filters/bitstream_convert...
media/filters/bitstream_converter_unittest.cc:18: static int
DoFilter(AVBitStreamFilterContext *bsfc,
On 2011/08/11 01:26:29, scherkus wrote:
> nit: instead of having everything be a static inside the test class, perhaps
> just make it all static and do away with the BitstreamConverterTest:: mess and
> prototype decls

Done.

http://codereview.chromium.org/7587012/diff/1/media/filters/bitstream_convert...
media/filters/bitstream_converter_unittest.cc:38:
InitializeMediaLibrary(file_path);
On 2011/08/11 01:26:29, scherkus wrote:
> InitializeMediaLibraryForTest() ?

Done.

http://codereview.chromium.org/7587012/diff/1/media/filters/bitstream_convert...
media/filters/bitstream_converter_unittest.cc:41:
av_register_bitstream_filter(&stream_filter_);
On 2011/08/11 01:26:29, scherkus wrote:
> does this prevent duplicate registration?
> 
> perhaps you want SetUpTestCase():
>
http://code.google.com/p/googletest/wiki/AdvancedGuide#Sharing_Resources_Betw...

Done.

http://codereview.chromium.org/7587012/diff/1/media/filters/bitstream_convert...
media/filters/bitstream_converter_unittest.cc:77: //static
On 2011/08/11 01:26:29, scherkus wrote:
> // static

Done.

http://codereview.chromium.org/7587012/diff/1/media/filters/bitstream_convert...
media/filters/bitstream_converter_unittest.cc:78: int
BitstreamConverterTest::DoFilter(AVBitStreamFilterContext *bsfc,
On 2011/08/11 01:26:29, scherkus wrote:
> pointers w/ types + docs on what this test filter does

Done.

http://codereview.chromium.org/7587012/diff/1/media/filters/bitstream_convert...
media/filters/bitstream_converter_unittest.cc:101: //static
On 2011/08/11 01:26:29, scherkus wrote:
> // static

Done.

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

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_demuxer_un...
media/filters/ffmpeg_demuxer_unittest.cc:68:
EXPECT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &file_path));
On 2011/08/11 01:26:29, scherkus wrote:
> can we use the ReadTestData functions here?
No because we don't have an in-memory DataSource impl...yet.

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_demuxer_un...
media/filters/ffmpeg_demuxer_unittest.cc:91: void ValidateBuffer(const
tracked_objects::Location& location,
On 2011/08/11 01:26:29, scherkus wrote:
> docs? what does this do exactly?

Done. It just makes sure the buffer has the expected size and timestamp

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

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_glue.h#new...
media/filters/ffmpeg_glue.h:41: FFmpegURLProtocol() {
On 2011/08/11 01:26:29, scherkus wrote:
> want to clean these up to {}

Done.

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_glue_unitt...
File media/filters/ffmpeg_glue_unittest.cc (right):

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_glue_unitt...
media/filters/ffmpeg_glue_unittest.cc:40: virtual void SetUp() {
On 2011/08/11 01:26:29, scherkus wrote:
> Shouldn't this test initialize the media library?
> 
> Also I wonder whether this bit of code get changed to static SetUpTestCase()
> since it deals with a singleton and whatnot... if it works fine no real need
to
> change it

Done.

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_video_deco...
File media/filters/ffmpeg_video_decoder_unittest.cc (right):

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_video_deco...
media/filters/ffmpeg_video_decoder_unittest.cc:138: class FFmpegVideoDecoderTest
: public testing::Test {
On 2011/08/11 01:26:29, scherkus wrote:
> heh this test should probably use the real FFmpegVideoDecodeEngine :P
> 
> but that's for another day

yes... another day.

http://codereview.chromium.org/7587012/diff/1/media/filters/ffmpeg_video_deco...
media/filters/ffmpeg_video_decoder_unittest.cc:143:
InitializeMediaLibrary(file_path);
On 2011/08/11 01:26:29, scherkus wrote:
> ForTest() ?

Done.

http://codereview.chromium.org/7587012/diff/1/media/video/ffmpeg_video_decode...
File media/video/ffmpeg_video_decode_engine.cc (right):

http://codereview.chromium.org/7587012/diff/1/media/video/ffmpeg_video_decode...
media/video/ffmpeg_video_decode_engine.cc:138: if (i >= 0) {
On 2011/08/11 01:26:29, scherkus wrote:
> woah!??
> 
> what's all this doing
Oops. This is how I was extracting data for test files. I accidentally left it
in.

http://codereview.chromium.org/7587012/diff/1/media/video/ffmpeg_video_decode...
File media/video/ffmpeg_video_decode_engine_unittest.cc (right):

http://codereview.chromium.org/7587012/diff/1/media/video/ffmpeg_video_decode...
media/video/ffmpeg_video_decode_engine_unittest.cc:53:
FFmpegGlue::GetInstance();
On 2011/08/11 01:26:29, scherkus wrote:
> is this to initialize the glue?
> 
> hrmm.... wonder if perhaps we should make this part of global set up
Yes. Making this part of InitializeMediaLibraryXXX would require moving
FFmpegGlue to media/base unless you don't mind having media/base depend on code
in media/filter.

http://codereview.chromium.org/7587012/diff/1/media/video/ffmpeg_video_decode...
media/video/ffmpeg_video_decode_engine_unittest.cc:168: scoped_refptr<Buffer>
bufferA = new DataBuffer(1);
On 2011/08/11 01:26:29, scherkus wrote:
> buffer_a

Done.

[scherkus (not reviewing), 2011-08-12 21:32:21]

LGTM w/ nits

WOO!

http://codereview.chromium.org/7587012/diff/7001/media/base/media.h
File media/base/media.h (right):

http://codereview.chromium.org/7587012/diff/7001/media/base/media.h#newcode28
media/base/media.h:28: // Helper method for unit tests to avoid boiler plate
code everywhere.
want to mention that unlike the other method this one will crash if it fails
(that's why its for testing)

http://codereview.chromium.org/7587012/diff/7001/media/base/run_all_unittests.cc
File media/base/run_all_unittests.cc (right):

http://codereview.chromium.org/7587012/diff/7001/media/base/run_all_unittests...
media/base/run_all_unittests.cc:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
wow that's old :)

http://codereview.chromium.org/7587012/diff/7001/media/filters/bitstream_conv...
File media/filters/bitstream_converter_unittest.cc (right):

http://codereview.chromium.org/7587012/diff/7001/media/filters/bitstream_conv...
media/filters/bitstream_converter_unittest.cc:28: static int
DoFilter(AVBitStreamFilterContext *bsfc,
pointers w/ types

http://codereview.chromium.org/7587012/diff/7001/media/filters/bitstream_conv...
media/filters/bitstream_converter_unittest.cc:58: 0,
want to doc this one as well?

http://codereview.chromium.org/7587012/diff/7001/media/filters/bitstream_conv...
media/filters/bitstream_converter_unittest.cc:62: public:
remove?

[acolwell GONE FROM CHROMIUM, 2011-08-15 17:00:54]

http://codereview.chromium.org/7587012/diff/7001/media/base/media.h
File media/base/media.h (right):

http://codereview.chromium.org/7587012/diff/7001/media/base/media.h#newcode28
media/base/media.h:28: // Helper method for unit tests to avoid boiler plate
code everywhere.
On 2011/08/12 21:32:21, scherkus wrote:
> want to mention that unlike the other method this one will crash if it fails
> (that's why its for testing)

Done.

http://codereview.chromium.org/7587012/diff/7001/media/filters/bitstream_conv...
File media/filters/bitstream_converter_unittest.cc (right):

http://codereview.chromium.org/7587012/diff/7001/media/filters/bitstream_conv...
media/filters/bitstream_converter_unittest.cc:28: static int
DoFilter(AVBitStreamFilterContext *bsfc,
On 2011/08/12 21:32:21, scherkus wrote:
> pointers w/ types

Done.

http://codereview.chromium.org/7587012/diff/7001/media/filters/bitstream_conv...
media/filters/bitstream_converter_unittest.cc:58: 0,
On 2011/08/12 21:32:21, scherkus wrote:
> want to doc this one as well?

Done.

http://codereview.chromium.org/7587012/diff/7001/media/filters/bitstream_conv...
media/filters/bitstream_converter_unittest.cc:62: public:
On 2011/08/12 21:32:21, scherkus wrote:
> remove?

Done.

[acolwell GONE FROM CHROMIUM, 2011-08-22 23:40:41]

Uploaded patch that fixes FFmpegVideoDecodeEngineTest so it works with
multithreaded decoders.

[scherkus (not reviewing), 2011-08-23 14:42:08]

one q about making the corrupted I frame a test file but LGTM

http://codereview.chromium.org/7587012/diff/24001/media/video/ffmpeg_video_de...
File media/video/ffmpeg_video_decode_engine_unittest.cc (right):

http://codereview.chromium.org/7587012/diff/24001/media/video/ffmpeg_video_de...
media/video/ffmpeg_video_decode_engine_unittest.cc:52: // Create a corrupt
version of |i_frame_buffer_|.
couldn't this be a file?