Fixes a bug in the media pipeline that resulted in the video dimensions never...

Fixes a bug in the media pipeline that resulted in the video dimensions never getting set.  Includes a new unit test, along with new
mocks for a video decoder and video renderer.

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

[ralphl, 2009-02-12 22:17:54]

When working on more unit tests I found a bug in the pipeline.  It never set the
video height and width (you'll see the stupidity in pipeline_impl.cc).

New mocks were required to allow for a pipeline that has a video renderer.

[scherkus (not reviewing), 2009-02-13 03:13:24]

In general, try to keep your mocks configurable.  Other than that looking good
so far.

http://codereview.chromium.org/20326/diff/1011/1012
File media/base/mock_media_filters.h (right):

http://codereview.chromium.org/20326/diff/1011/1012#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.  Use of
this
it's almost getting to the point where I think this class should be broken into
its own subdirectory!

we'll worry about doing that later though :)

http://codereview.chromium.org/20326/diff/1011/1012#newcode123
Line 123: static FilterFactory* CreateFactory(bool include_video) {
Can you name has_video, or make has_video_ include_video_?

http://codereview.chromium.org/20326/diff/1011/1012#newcode124
Line 124: return new FilterFactoryImpl1<MockDemuxer, bool>(include_video);
thinking about it, it makes sense to have this demuxer be more configurable than
just assume audio/video.

for example, there are a lot of places where our previous code broke when
introduced with video-only media or media containing captioning tracks or other
data tracks

The most versatile approach would be to pre-configure the MockDemuxerStreams
from the outside and pass them into the CreateFactory/MockDemuxer using an
std::vector.  Since these are for testing you can make it easier on yourself by
implementing the copy, equality and assignment operators for MockDemuxerStream.

http://codereview.chromium.org/20326/diff/1011/1012#newcode143
Line 143: virtual media::DemuxerStream* GetStream(int stream_id) {
the contract of this method is to return NULL if the stream doesnt exist, so we
should return NULL when stream_id == 1 but has_video_ == false.

same with the default condition

http://codereview.chromium.org/20326/diff/1011/1012#newcode170
Line 170: explicit MockDemuxerStream(bool video) {
looks like it'd be easier to pass in a const char* here instead

http://codereview.chromium.org/20326/diff/1011/1012#newcode284
Line 284: class MockVideoFrame : public VideoFrame {
can you add a ctor and dtor?

also you might find it easier to have a ctor that accepts the timestamp and
duration

http://codereview.chromium.org/20326/diff/1011/1012#newcode314
Line 314: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/20326/diff/1011/1012#newcode319
Line 319: return new FilterFactoryImpl0<MockVideoDecoder>();
can you change this to pass in width/height to have it configurable?

http://codereview.chromium.org/20326/diff/1011/1012#newcode357
Line 357: base::TimeDelta duration = base::TimeDelta::FromMilliseconds(33);
can you make the duration a public constant?

you also might find it clearer to test if it was a multiple of 10 (so 4 reads
results in 40 as opposed to 132)

http://codereview.chromium.org/20326/diff/1011/1012#newcode361
Line 361: buffer->OnAssignment();
I think you need to call buffer->Release() here

http://codereview.chromium.org/20326/diff/1011/1012#newcode389
Line 389: host_->SetVideoSize(640u, 480u);
can you inspect the incoming media format instead of hardcoding these values?

http://codereview.chromium.org/20326/diff/1011/1013
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/20326/diff/1011/1013#newcode98
Line 98: EXPECT_NE(0u, width);
it'd be a better test if we can configure the decoder and then assert the exact
value

[ralphl, 2009-02-13 21:11:57]

Good feedback -- the mocks are now very configurable, and we can easily add more
configuration parameters in the future.  There's a struct named MockFilterConfig
which contains things like video height and width, default frame duration, etc.

We can add anything we want to this, and then every mock filter gets to examine
it when it starts up.

http://codereview.chromium.org/20326/diff/1011/1012
File media/base/mock_media_filters.h (right):

http://codereview.chromium.org/20326/diff/1011/1012#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.  Use of
this
On 2009/02/13 03:13:24, scherkus wrote:
> it's almost getting to the point where I think this class should be broken
into
> its own subdirectory!
> 
> we'll worry about doing that later though :)

I agree with both comments.  This also needs to be broken up into individual
filters and/or .h and .cc files at some point.

http://codereview.chromium.org/20326/diff/1011/1012#newcode124
Line 124: return new FilterFactoryImpl1<MockDemuxer, bool>(include_video);
On 2009/02/13 03:13:24, scherkus wrote:
> thinking about it, it makes sense to have this demuxer be more configurable
than
> just assume audio/video.
> 
> for example, there are a lot of places where our previous code broke when
> introduced with video-only media or media containing captioning tracks or
other
> data tracks
> 
> The most versatile approach would be to pre-configure the MockDemuxerStreams
> from the outside and pass them into the CreateFactory/MockDemuxer using an
> std::vector.  Since these are for testing you can make it easier on yourself
by
> implementing the copy, equality and assignment operators for
MockDemuxerStream.

Made a big struct that is passed to all of the filters.  See what you think.

http://codereview.chromium.org/20326/diff/1011/1012#newcode143
Line 143: virtual media::DemuxerStream* GetStream(int stream_id) {
On 2009/02/13 03:13:24, scherkus wrote:
> the contract of this method is to return NULL if the stream doesnt exist, so
we
> should return NULL when stream_id == 1 but has_video_ == false.
> 
> same with the default condition

Done.

http://codereview.chromium.org/20326/diff/1011/1012#newcode170
Line 170: explicit MockDemuxerStream(bool video) {
On 2009/02/13 03:13:24, scherkus wrote:
> looks like it'd be easier to pass in a const char* here instead

You are right.  Done.

http://codereview.chromium.org/20326/diff/1011/1012#newcode284
Line 284: class MockVideoFrame : public VideoFrame {
On 2009/02/13 03:13:24, scherkus wrote:
> can you add a ctor and dtor?
> 
> also you might find it easier to have a ctor that accepts the timestamp and
> duration

Done.

http://codereview.chromium.org/20326/diff/1011/1012#newcode314
Line 314: };
On 2009/02/13 03:13:24, scherkus wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/20326/diff/1011/1012#newcode319
Line 319: return new FilterFactoryImpl0<MockVideoDecoder>();
On 2009/02/13 03:13:24, scherkus wrote:
> can you change this to pass in width/height to have it configurable?

Done.

http://codereview.chromium.org/20326/diff/1011/1012#newcode357
Line 357: base::TimeDelta duration = base::TimeDelta::FromMilliseconds(33);
On 2009/02/13 03:13:24, scherkus wrote:
> can you make the duration a public constant?
> 
> you also might find it clearer to test if it was a multiple of 10 (so 4 reads
> results in 40 as opposed to 132)

Done.

http://codereview.chromium.org/20326/diff/1011/1012#newcode361
Line 361: buffer->OnAssignment();
On 2009/02/13 03:13:24, scherkus wrote:
> I think you need to call buffer->Release() here

Good catch.  Done.

http://codereview.chromium.org/20326/diff/1011/1012#newcode389
Line 389: host_->SetVideoSize(640u, 480u);
On 2009/02/13 03:13:24, scherkus wrote:
> can you inspect the incoming media format instead of hardcoding these values?

Done.

http://codereview.chromium.org/20326/diff/1011/1013
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/20326/diff/1011/1013#newcode98
Line 98: EXPECT_NE(0u, width);
On 2009/02/13 03:13:24, scherkus wrote:
> it'd be a better test if we can configure the decoder and then assert the
exact
> value

Done.

[scherkus (not reviewing), 2009-02-17 23:57:36]

really liking the configurable mock!

just some tiny comments

http://codereview.chromium.org/20326/diff/28/1030
File media/base/mock_media_filters.h (right):

http://codereview.chromium.org/20326/diff/28/1030#newcode47
Line 47: frame_duration(base::TimeDelta::FromMilliseconds(33)) {
can we change the default from 33 to 10 or 1?  it's a lot easier to do the math
and determine the number of frames that have gone through the pipeline

http://codereview.chromium.org/20326/diff/28/1030#newcode50
Line 50: void set_no_video() {
given we have public access to the variables, we could probably do away with
this

also bracket should go on next line

http://codereview.chromium.org/20326/diff/28/1030#newcode54
Line 54: bool has_video() const { return (video_width > 0); }
can you put this on the next line and drop the parentheses?

http://codereview.chromium.org/20326/diff/28/1030#newcode131
Line 131: const MockFilterConfig*>;
spacing here

http://codereview.chromium.org/20326/diff/28/1030#newcode194
Line 194: EXPECT_TRUE(false);
You can use ADD_FAILURE() to mark a non-fatal failure (mark the test as failing
but continue execution).  FAIL() aborts the test completely.

http://codereview.chromium.org/20326/diff/28/1030#newcode359
Line 359: // TODO(ralphl): mock the data for video surfaces too
period at end of comment

http://codereview.chromium.org/20326/diff/28/1030#newcode418
Line 418: config_ = config;
can you use an initializer list for config?

http://codereview.chromium.org/20326/diff/28/1031
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/20326/diff/28/1031#newcode32
Line 32: TEST(PipelineImplTest, Initialization) {
I should have busted you before on this, but the single letter variable names
are really throwing me off.

can you add a TODO comment reminding us to rename them?  don't do inside this
changelist or else it'll never get checked in :P

http://codereview.chromium.org/20326/diff/28/1031#newcode39
Line 39: h.TimedWait(base::TimeDelta::FromMilliseconds(300));
out of curiosity, why is this 300ms as opposed to 5 seconds?

http://codereview.chromium.org/20326/diff/28/1031#newcode67
Line 67: void AddAllMockFilters(FilterFactoryCollection* factories,
could you move this function to the top of the file and into an anonymous
namespace before the tests and include a quick comment about it?

helper code/functions usually go before your tests and are placed in anonymous
namespaces to avoid any sort of linker clash

take a look at src\chrome\browser\net\url_fetcher_unittest.cc for an example

http://codereview.chromium.org/20326/diff/28/1031#newcode101
Line 101: TEST(PipelineImplTest, MockVideoPipeline) {
don't worry about renaming the variables here -- we can do it in a later
changelist.

http://codereview.chromium.org/20326/diff/28/1033
File media/filters/file_data_source_unittest.cc (right):

http://codereview.chromium.org/20326/diff/28/1033#newcode35
Line 35: std::string TestFileURL() {
just catching this now -- could you place this function in an anonymous
namespace and add a quick comment?

[ralphl, 2009-02-18 01:17:33]

All changes have been made or comments as to why I don't think we should change
them.

http://codereview.chromium.org/20326/diff/28/1030
File media/base/mock_media_filters.h (right):

http://codereview.chromium.org/20326/diff/28/1030#newcode47
Line 47: frame_duration(base::TimeDelta::FromMilliseconds(33)) {
On 2009/02/17 23:57:37, scherkus wrote:
> can we change the default from 33 to 10 or 1?  it's a lot easier to do the
math
> and determine the number of frames that have gone through the pipeline

This is the actual frame rate of 720p movies according to Frank.  Perhaps we
should choose the real, exact value in Microseconds (33333).  I was trying to
really emulate an actual video stream.

http://codereview.chromium.org/20326/diff/28/1030#newcode50
Line 50: void set_no_video() {
On 2009/02/17 23:57:37, scherkus wrote:
> given we have public access to the variables, we could probably do away with
> this
> 
> also bracket should go on next line

I like it for the sake of clarity for the client:
MockFilterConfig config;
config.set_no_video();

seems to be more obvious than

config.video_width_ = 0;

http://codereview.chromium.org/20326/diff/28/1030#newcode131
Line 131: const MockFilterConfig*>;
On 2009/02/17 23:57:37, scherkus wrote:
> spacing here

Done.

http://codereview.chromium.org/20326/diff/28/1030#newcode194
Line 194: EXPECT_TRUE(false);
On 2009/02/17 23:57:37, scherkus wrote:
> You can use ADD_FAILURE() to mark a non-fatal failure (mark the test as
failing
> but continue execution).  FAIL() aborts the test completely.
> 
> 
> 

Done.

http://codereview.chromium.org/20326/diff/28/1030#newcode359
Line 359: // TODO(ralphl): mock the data for video surfaces too
On 2009/02/17 23:57:37, scherkus wrote:
> period at end of comment

Done.

http://codereview.chromium.org/20326/diff/28/1030#newcode418
Line 418: config_ = config;
On 2009/02/17 23:57:37, scherkus wrote:
> can you use an initializer list for config?

Done.

http://codereview.chromium.org/20326/diff/28/1031
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/20326/diff/28/1031#newcode32
Line 32: TEST(PipelineImplTest, Initialization) {
On 2009/02/17 23:57:37, scherkus wrote:
> I should have busted you before on this, but the single letter variable names
> are really throwing me off.
> 
> can you add a TODO comment reminding us to rename them?  don't do inside this
> changelist or else it'll never get checked in :P

Added the TODO()

http://codereview.chromium.org/20326/diff/28/1031#newcode39
Line 39: h.TimedWait(base::TimeDelta::FromMilliseconds(300));
On 2009/02/17 23:57:37, scherkus wrote:
> out of curiosity, why is this 300ms as opposed to 5 seconds?

I expect this test to hang, and I don't want it to take a long time.  1/3 second
should be plenty of time to hang.  The other tests should complete quickly, so I
wait for a while to make sure... They normally complete in < 10ms, but this one
will hang for as long as it runs.

The purpose of the test is to make sure we can tear down the pipeline while the
nested message loop is running and you call stop().

http://codereview.chromium.org/20326/diff/28/1031#newcode67
Line 67: void AddAllMockFilters(FilterFactoryCollection* factories,
On 2009/02/17 23:57:37, scherkus wrote:
> could you move this function to the top of the file and into an anonymous
> namespace before the tests and include a quick comment about it?
> 
> helper code/functions usually go before your tests and are placed in anonymous
> namespaces to avoid any sort of linker clash
> 
> take a look at src\chrome\browser\net\url_fetcher_unittest.cc for an example

Done.

http://codereview.chromium.org/20326/diff/28/1031#newcode101
Line 101: TEST(PipelineImplTest, MockVideoPipeline) {
On 2009/02/17 23:57:37, scherkus wrote:
> don't worry about renaming the variables here -- we can do it in a later
> changelist.

Done.

http://codereview.chromium.org/20326/diff/28/1033
File media/filters/file_data_source_unittest.cc (right):

http://codereview.chromium.org/20326/diff/28/1033#newcode35
Line 35: std::string TestFileURL() {
On 2009/02/17 23:57:37, scherkus wrote:
> just catching this now -- could you place this function in an anonymous
> namespace and add a quick comment?

Done.

[scherkus (not reviewing), 2009-02-19 01:25:18]

I know it almost seems silly at this point, but consider using human-friendly
frame duration and a boolean flag instead of dual-purposing width.

Other than that LGTM

http://codereview.chromium.org/20326/diff/28/1030
File media/base/mock_media_filters.h (right):

http://codereview.chromium.org/20326/diff/28/1030#newcode50
Line 50: void set_no_video() {
How about we add a boolean flag instead of dual-purposing width?

On 2009/02/18 01:17:33, ralphl wrote:
> On 2009/02/17 23:57:37, scherkus wrote:
> > given we have public access to the variables, we could probably do away with
> > this
> > 
> > also bracket should go on next line
> 
> I like it for the sake of clarity for the client:
> MockFilterConfig config;
> config.set_no_video();
> 
> seems to be more obvious than
> 
> config.video_width_ = 0;
>

http://codereview.chromium.org/20326/diff/28/1031
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/20326/diff/28/1031#newcode39
Line 39: h.TimedWait(base::TimeDelta::FromMilliseconds(300));
Got it!

On 2009/02/18 01:17:33, ralphl wrote:
> On 2009/02/17 23:57:37, scherkus wrote:
> > out of curiosity, why is this 300ms as opposed to 5 seconds?
> 
> I expect this test to hang, and I don't want it to take a long time.  1/3
second
> should be plenty of time to hang.  The other tests should complete quickly, so
I
> wait for a while to make sure... They normally complete in < 10ms, but this
one
> will hang for as long as it runs.
> 
> The purpose of the test is to make sure we can tear down the pipeline while
the
> nested message loop is running and you call stop().