Refactor PipelineImpl to use CompositeFilter to manage Filter state transitions.

Refactor PipelineImpl to use CompositeFilter to manage Filter state transitions.

BUG=54110
TEST=media_unittests CompositeFilterTest.*

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

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

[acolwell GONE FROM CHROMIUM, 2010-12-10 20:07:41]

Fixed TestThreadTermination unit test.

[acolwell GONE FROM CHROMIUM, 2010-12-14 17:58:58]

- Added explicit MessageLoop usage to make sure code runs on the proper thread.
- Fixed handling errors that occur while stopping

[scherkus (not reviewing), 2010-12-15 16:44:02]

phew that was quite the review

sorry for the delay!

biggest things:
  - Does CompositeFilter really need to implement Filter?
  - Using less gmock in unit test (MockFilter might be better off saving the
callbacks + providing accessors/run methods
  - Being less strict in tests and letting valgrind do some of the work

awesome stuff!

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

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.cc:7: #include "media/base/composite_filter.h"
include corresponding .h first followed by blank line, then rest of includes

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.cc:12: {
style: { on previous line

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.cc:18: {
style: { on previous line

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.cc:283: CallFilter(filters_[i],
I get freaked out when I see multi-line for/while bodies without {} -- mind
adding them?

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.cc:435: void (CompositeFilter::*method)()) {
de-indent by 2

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.cc:491: DCHECK(host_);
I'd enforce a guarantee that host_ must be present and remove all these DCHECKs
and if statements

if CompositeFilter doesn't implement Filter, then there's no need for set_host()
and host_ can be set via constructor

WDYT?

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

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.h:14: class CompositeFilter : public Filter,
nit: all of this + { can fit on one line

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.h:15: private FilterHost
woah private inheritance!

style recommends public

in fact I've never used anything other than public so I can't really offer any
concrete reasons why other than style guide (but feel free to explain!)

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.h:41: virtual void set_message_loop(MessageLoop*
message_loop);
does CompositeFilter need to implement Filter, or just a subset of those
methods?

we might be able to get away with simply duplicating the signatures so we aren't
forced to implement methods that PipelineImpl will never use

would also avoid making CompositeFilter ref-counted

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.h:52: virtual ~CompositeFilter();
we could also make CompositeFilter non-ref-counted and make this public

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.h:164: unsigned int sequence_index_;
bit confused.. if this is an unsigned int I notice you're setting it to -1  as
well?

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
File media/base/composite_filter_unittest.cc (right):

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:33: SEEK
nit: add trailing ,

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:114: {}
move { to previous line

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:116:
CompositeFilterTest::~CompositeFilterTest()
one-liner w/ {} at end

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:205: InSequence seq;
warning: combination of InSequence + StrictMock can result in overly-strict and
fragile tests

we don't want to get into the situation where we're testing every line of code +
order of function execution, otherwise refactoring/changing code =
refactoring/changing this test

I've been trying to move towards NiceMock and asserting the end result when
possible (might not make as much sense here, but just throwing it out there)

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:224: EXPECT_NE((FilterCallback*)NULL,
filter_1_callback_);
does compiler complain when comparing to plain NULL?

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:229: delete filter_1_callback_;
seems like you have these three lines of code duplicated many times over

what about having:
RunFilter1Callback();
RunFilter2Callback();

---OR---

use less gmock in MockFilter and simply save the callback inside the MockFilter
and add HasCallback() / RunCallback() methods

that way we don't have this relationship between filter_1_ and
filter_1_callback_

it depends on how you want to tackle it.. I think you want to assert that Play()
was called but also save the callback pointer... you could trying using gmock
Actions

OR!! if it works you could also try something like this:
Play(Callback* cb) {
  play_callback_ = cb;
}
bool has_play_callback() { play_callback_ != NULL; }
void run_play_callback() { play_callback_->Run(); delete play_callback_;
play_callback_ = NULL; }


So you won't get the same gmock satisfaction of knowing that Play() was called,
but you could get a near-same-effect of calling
EXPECT_TRUE(filter_1_->has_play_callback()) etc... which I think reads a bit
better

OK I'll stop rambling!

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:354: virtual void CleanUp() {
are you using this to make sure the thread was stopped + deleted?

another bit of "things I wish I did differently" is testing for deletion tends
to complicate code + tests.  we run valgrind bots so it's usually good enough to
rely on them to catch leak errors

also I think the Thread dtor calls Stop() which calls CleanUp() so we can make
do without a lot of this (and possibly even the thread factory stuff inside
CompositeFilter)

I think it's enough to test that a Thread is simply created and set on a Filter
via set_message_loop()

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:412: EXPECT_CALL(*mock_callback,
OnCallbackDestroyed());
I think MockCallback is something I need to kill for similar valgrind reasons...
it's way too crazy to assert that a callback was deleted because that's why we
have valgrind :(

see if it's possible to do w/o MockCallback

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

http://codereview.chromium.org/5744002/diff/7001/media/base/mock_filters.cc#n...
media/base/mock_filters.cc:26: {}
pedantic style nit: I *think* when we have an empty method we have the { on the
previous line

we use the {} form when it's a one-liner (which you could do here for the dtor,
but not so much the ctor)

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

http://codereview.chromium.org/5744002/diff/7001/media/base/mock_filters.h#ne...
media/base/mock_filters.h:97: void set_requires_message_loop(bool
requires_message_loop);
looks like these are always called after creation.. perhaps make ctor arguments
instead?

[acolwell GONE FROM CHROMIUM, 2010-12-15 18:20:11]

Responses to some comments.

Code changes will be sent out later today

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

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.cc:7: #include "media/base/composite_filter.h"
On 2010/12/15 16:44:02, scherkus wrote:
> include corresponding .h first followed by blank line, then rest of includes

Done.

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.cc:12: {
On 2010/12/15 16:44:02, scherkus wrote:
> style: { on previous line

Done.

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.cc:18: {
On 2010/12/15 16:44:02, scherkus wrote:
> style: { on previous line

Done.

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.cc:283: CallFilter(filters_[i],
On 2010/12/15 16:44:02, scherkus wrote:
> I get freaked out when I see multi-line for/while bodies without {} -- mind
> adding them?

Done.

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.cc:435: void (CompositeFilter::*method)()) {
On 2010/12/15 16:44:02, scherkus wrote:
> de-indent by 2

Done.

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.cc:491: DCHECK(host_);
On 2010/12/15 16:44:02, scherkus wrote:
> I'd enforce a guarantee that host_ must be present and remove all these
DCHECKs
> and if statements

Once I move the FilterHost impl to an inner class I can remove all the checks
and replace it with one in the inner class constructor.

> 
> if CompositeFilter doesn't implement Filter, then there's no need for
set_host()
> and host_ can be set via constructor

I'm still standing by that this code should implement Filter. I think eventually
set_host() should be removed in favor of constructor parameters, but that is a
different CL.

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

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.h:14: class CompositeFilter : public Filter,
On 2010/12/15 16:44:02, scherkus wrote:
> nit: all of this + { can fit on one line

Done.

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.h:15: private FilterHost
FilterHost is private because I don't want other code to know that
CompositeFilter "is a" FilterHost. The only reason it implements this interface
is so that it can intercept calls from the Filters in the composite. 

I'll change this to an inner class so it matches the style guide. It just
requires a little more interaction between the CompositeFilter and the inner
class.

On 2010/12/15 16:44:02, scherkus wrote:
> woah private inheritance!
> 
> style recommends public
> 
> in fact I've never used anything other than public so I can't really offer any
> concrete reasons why other than style guide (but feel free to explain!)

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.h:41: virtual void set_message_loop(MessageLoop*
message_loop);
In my opinion it should implement Filter. The idea is that you can replace any
use of a Filter with one of these. This allows you to group Filters in a variety
of ways without other pieces of code having to know it is happening.

I think eventually the host & message_loop methods should be removed from this
interface and turned into constructor parameters.

On 2010/12/15 16:44:02, scherkus wrote:
> does CompositeFilter need to implement Filter, or just a subset of those
> methods?
> 
> we might be able to get away with simply duplicating the signatures so we
aren't
> forced to implement methods that PipelineImpl will never use
> 
> would also avoid making CompositeFilter ref-counted

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.h:52: virtual ~CompositeFilter();
On 2010/12/15 16:44:02, scherkus wrote:
> we could also make CompositeFilter non-ref-counted and make this public

Why? This is ref-counted like other filters.

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.h:164: unsigned int sequence_index_;
On 2010/12/15 16:44:02, scherkus wrote:
> bit confused.. if this is an unsigned int I notice you're setting it to -1  as
> well?

I'll fix this. The -1 was a hack to get around having to special case the empty
composite case.

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
File media/base/composite_filter_unittest.cc (right):

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:33: SEEK
On 2010/12/15 16:44:02, scherkus wrote:
> nit: add trailing ,

Done.

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:114: {}
On 2010/12/15 16:44:02, scherkus wrote:
> move { to previous line

Done.

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:116:
CompositeFilterTest::~CompositeFilterTest()
On 2010/12/15 16:44:02, scherkus wrote:
> one-liner w/ {} at end

Done.

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:205: InSequence seq;
On 2010/12/15 16:44:02, scherkus wrote:
> warning: combination of InSequence + StrictMock can result in overly-strict
and
> fragile tests
> 
> we don't want to get into the situation where we're testing every line of code
+
> order of function execution, otherwise refactoring/changing code =
> refactoring/changing this test
> 
> I've been trying to move towards NiceMock and asserting the end result when
> possible (might not make as much sense here, but just throwing it out there)

This is intentional. I'm trying to ensure a strict execution order on the
filters. Certain cases call the filters one after the other and in some cases
the filters are called all at once. If this behavior is changed the tests should
catch it and be updated appropriately.

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:224: EXPECT_NE((FilterCallback*)NULL,
filter_1_callback_);
On 2010/12/15 16:44:02, scherkus wrote:
> does compiler complain when comparing to plain NULL?
Yes. I get this error if I don't have the cast.

testing/gtest/include/gtest/gtest.h: In function ‘testing::AssertionResult
testing::internal::CmpHelperNE(const char*, const char*, const T1&, const T2&)
[with T1 = long int, T2 = media::FilterCallback*]’:

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:229: delete filter_1_callback_;
I'll look into reducing this duplicate code. I agree it should be more readable.

On 2010/12/15 16:44:02, scherkus wrote:
> seems like you have these three lines of code duplicated many times over
> 
> what about having:
> RunFilter1Callback();
> RunFilter2Callback();
> 
> ---OR---
> 
> use less gmock in MockFilter and simply save the callback inside the
MockFilter
> and add HasCallback() / RunCallback() methods
> 
> that way we don't have this relationship between filter_1_ and
> filter_1_callback_
> 
> it depends on how you want to tackle it.. I think you want to assert that
Play()
> was called but also save the callback pointer... you could trying using gmock
> Actions
> 
> OR!! if it works you could also try something like this:
> Play(Callback* cb) {
>   play_callback_ = cb;
> }
> bool has_play_callback() { play_callback_ != NULL; }
> void run_play_callback() { play_callback_->Run(); delete play_callback_;
> play_callback_ = NULL; }
> 
> 
> So you won't get the same gmock satisfaction of knowing that Play() was
called,
> but you could get a near-same-effect of calling
> EXPECT_TRUE(filter_1_->has_play_callback()) etc... which I think reads a bit
> better
> 
> OK I'll stop rambling!

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:354: virtual void CleanUp() {
On 2010/12/15 16:44:02, scherkus wrote:
> are you using this to make sure the thread was stopped + deleted?

Yes.

> 
> another bit of "things I wish I did differently" is testing for deletion tends
> to complicate code + tests.  we run valgrind bots so it's usually good enough
to
> rely on them to catch leak errors

This isn't that complicated and allows us to explicitly check for the desired
behavior.

> 
> also I think the Thread dtor calls Stop() which calls CleanUp() so we can make

Removed the destructor since it wasn't needed. I just added it because of the
comment on the base:Thread::~Thread().

> do without a lot of this (and possibly even the thread factory stuff inside
> CompositeFilter)

The thread factory stuff is there for reasons other than testing. It is the
first step towards allowing thread creation strategies to be used for the
filters. One could use this mechanism to allow thread pools to be used for
filters that have the same thread name etc. This will likely be useful in the
future when we address the "too many threads/resources" bugs.

> 
> I think it's enough to test that a Thread is simply created and set on a
Filter
> via set_message_loop()

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:412: EXPECT_CALL(*mock_callback,
OnCallbackDestroyed());
On 2010/12/15 16:44:02, scherkus wrote:
> I think MockCallback is something I need to kill for similar valgrind
reasons...
> it's way too crazy to assert that a callback was deleted because that's why we
> have valgrind :(

In most cases I agree, but sometimes it is handy to verify when an object has
given up its reference to a callback. For example if I call a method multiple
times, it could be handy to verify that the callback from the previous call is
destroyed in response to the arrival of the new callback.

> 
> see if it's possible to do w/o MockCallback

As a compromise, I'll modify MockCallback to conditionally call
OnCallbackDestroyed() based on a constructor parameter. 
 That way some of the expectations can be removed in cases where I don't
actually care about the callback being destroyed.

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

http://codereview.chromium.org/5744002/diff/7001/media/base/mock_filters.cc#n...
media/base/mock_filters.cc:26: {}
On 2010/12/15 16:44:02, scherkus wrote:
> pedantic style nit: I *think* when we have an empty method we have the { on
the
> previous line
> 
> we use the {} form when it's a one-liner (which you could do here for the
dtor,
> but not so much the ctor)

Done.

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

http://codereview.chromium.org/5744002/diff/7001/media/base/mock_filters.h#ne...
media/base/mock_filters.h:97: void set_requires_message_loop(bool
requires_message_loop);
On 2010/12/15 16:44:02, scherkus wrote:
> looks like these are always called after creation.. perhaps make ctor
arguments
> instead?

Done.

[scherkus (not reviewing), 2010-12-15 18:57:43]

Thanks for speedy reply!

Responded to a few comments

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

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.cc:491: DCHECK(host_);
On 2010/12/15 18:20:11, acolwell wrote:
> On 2010/12/15 16:44:02, scherkus wrote:
> > I'd enforce a guarantee that host_ must be present and remove all these
> DCHECKs
> > and if statements
> 
> Once I move the FilterHost impl to an inner class I can remove all the checks
> and replace it with one in the inner class constructor.
> 
> > 
> > if CompositeFilter doesn't implement Filter, then there's no need for
> set_host()
> > and host_ can be set via constructor
> 
> I'm still standing by that this code should implement Filter. I think
eventually
> set_host() should be removed in favor of constructor parameters, but that is a
> different CL.

Sounds good!

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

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.h:41: virtual void set_message_loop(MessageLoop*
message_loop);
On 2010/12/15 18:20:11, acolwell wrote:
> In my opinion it should implement Filter. The idea is that you can replace any
> use of a Filter with one of these. This allows you to group Filters in a
variety
> of ways without other pieces of code having to know it is happening.

Rietveld probably isn't the best for discussing, but I have a motivation to get
rid of the notion of generic filters one day because the abstraction hasn't
proved useful.

We more or less have the following structure duplicated everywhere:
Filter < VideoDecoder < VideoDecoderImpl < FFmpegVideoDecodeEngine

...and I'm tempted to see whether we can squash a few abstraction layers and
simply have stuff like a VideoDecoder instead of dealing with a Filter that is
secretly a VideoDecoder, which is secretly always a VideoDecoderImpl, etc...

We can leave it implementing Filter for now and then modify the Filter interface
to move some stuff into the ctor in follow up CL (see below), but let's try to
sync up on the overall plan for refactoring

> I think eventually the host & message_loop methods should be removed from this
> interface and turned into constructor parameters.

Agreed -- we can tackle that in separate CL!

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter....
media/base/composite_filter.h:52: virtual ~CompositeFilter();
On 2010/12/15 18:20:11, acolwell wrote:
> On 2010/12/15 16:44:02, scherkus wrote:
> > we could also make CompositeFilter non-ref-counted and make this public
> 
> Why? This is ref-counted like other filters.

We've been trying to move away from ref-counting since the lifetime of our
filters are known.  Not that it's a performance thing, but more for having clear
ownership + deterministic shutdown.

[acolwell GONE FROM CHROMIUM, 2010-12-15 21:08:26]

Changes related to Andrew's CR comments

[scherkus (not reviewing), 2010-12-16 18:43:39]

more comments

not everything needs to be addressed in this CL but we should figure out some
medium-term-ish plan of attack

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
File media/base/composite_filter_unittest.cc (right):

http://codereview.chromium.org/5744002/diff/7001/media/base/composite_filter_...
media/base/composite_filter_unittest.cc:412: EXPECT_CALL(*mock_callback,
OnCallbackDestroyed());
On 2010/12/15 18:20:11, acolwell wrote:
> On 2010/12/15 16:44:02, scherkus wrote:
> > I think MockCallback is something I need to kill for similar valgrind
> reasons...
> > it's way too crazy to assert that a callback was deleted because that's why
we
> > have valgrind :(
> 
> In most cases I agree, but sometimes it is handy to verify when an object has
> given up its reference to a callback. For example if I call a method multiple
> times, it could be handy to verify that the callback from the previous call is
> destroyed in response to the arrival of the new callback.

If you give it a new callback you can assert that the old callback is never
executed.  If it does get executed when it shouldn't, the test will likely crash
(that's a good thing).

If the implementation wants to hold onto old callbacks, that's fine and we
shouldn't care.  What we do care about is which callback is executed (or not!)
and that valgrind verifies old callbacks were released at some point in time.

I went down this path with FFmpegDemuxer tests and that resulted in no one else
in the team being able to modify code because the tests were too complicated to
fix.  An alarm goes off when I see stuff like this because I don't want to
replicate that experience :(

> > see if it's possible to do w/o MockCallback
> 
> As a compromise, I'll modify MockCallback to conditionally call
> OnCallbackDestroyed() based on a constructor parameter. 
>  That way some of the expectations can be removed in cases where I don't
> actually care about the callback being destroyed.

That's OK for now but we should simply remove OnCallbackDestroyed() and fix all
tests in follow up CL (I can tackle that if need be)

http://codereview.chromium.org/5744002/diff/24001/media/base/composite_filter...
media/base/composite_filter_unittest.cc:83: protected:
nit: , -> .

http://codereview.chromium.org/5744002/diff/24001/media/base/composite_filter...
media/base/composite_filter_unittest.cc:86: // The composite object being
tested.
nit: , -> .

http://codereview.chromium.org/5744002/diff/24001/media/base/composite_filter...
media/base/composite_filter_unittest.cc:119: void
CompositeFilterTest::SetupAndAdd2Filters() {
nit: space between ) and {

http://codereview.chromium.org/5744002/diff/24001/media/base/composite_filter...
media/base/composite_filter_unittest.cc:229: delete filter_1_callback_;
ok I debugged this a bit because that's strange...

turns out EXPECT_EQ, EXPECT_TRUE, and EXPECT_FALSE all work but it's only
EXPECT_NE that complains

you can either go around and static_cast<> everything but I think it'd be easier
to do EXPECT_TRUE/EXPECT_FALSE for pointer checks

caveat: I only tested this on linux so if it's win/mac that's complaining we
might have to static_cast<> it up :(

http://codereview.chromium.org/5744002/diff/24001/media/base/composite_filter...
media/base/composite_filter_unittest.cc:375: EXPECT_EQ(NULL,
g_thread_created_by_factory);
I still don't think we need (most of) this test since a good chunk seems to
verify Thread and not CompositeFilter

the real core of the test is to see if some thread was created and that it was
assigned to a filter, which can be accomplished with a much smaller test:

EXEPECT_FALSE(MessageLoop::current());  // make sure this thread has no message
loop
EXPECT_TRUE(filter->message_loop());  // make sure filter has some sort of
message loop

Now that I think about it I'm also wondering whether it's CompositeFilter's
responsibility to create + inject threads... does it make sense for it to be
accomplished at the AddFilter() level?  For example:
AddFilter(Filter*, MessageLoop*)
AddFilter(Filter*, Thread*)

Basically it's another red-flag of mine when I see unit tests that aren't
capable of running on a single thread (i.e., threads are getting
created/deleted), so just doing some sanity checking.

It might be required in a follow change if/when we get rid of
requires_message_loop() (I don't think Filters should be responsible for
determining whether they get a thread or not, but rather the application i.e.,
Pipeline)

http://codereview.chromium.org/5744002/diff/24001/media/base/mock_filters.h
File media/base/mock_filters.h (right):

http://codereview.chromium.org/5744002/diff/24001/media/base/mock_filters.h#n...
media/base/mock_filters.h:53: MockFilterCallback() :
callOnCallbackDestroyed_(true) {
callOnCallbackDestroyed_ -> call_on_callback_destroyed_

or maybe a less verbose name like "run_destroy_callback_" etc.....

[acolwell GONE FROM CHROMIUM, 2010-12-20 17:43:21]

New patch posted with suggested fixes

http://codereview.chromium.org/5744002/diff/24001/media/base/composite_filter...
File media/base/composite_filter_unittest.cc (right):

http://codereview.chromium.org/5744002/diff/24001/media/base/composite_filter...
media/base/composite_filter_unittest.cc:83: // Run the callback stored in
|filter_1_callback_|,
On 2010/12/16 18:43:39, scherkus wrote:
> nit: , -> .

Done.

http://codereview.chromium.org/5744002/diff/24001/media/base/composite_filter...
media/base/composite_filter_unittest.cc:86: // Run the callback stored in
|filter_2_callback_|,
On 2010/12/16 18:43:39, scherkus wrote:
> nit: , -> .

Done.

http://codereview.chromium.org/5744002/diff/24001/media/base/composite_filter...
media/base/composite_filter_unittest.cc:119: mock_filter_host_(new
StrictMock<MockFilterHost>()){
On 2010/12/16 18:43:39, scherkus wrote:
> nit: space between ) and {

Done.

http://codereview.chromium.org/5744002/diff/24001/media/base/composite_filter...
media/base/composite_filter_unittest.cc:229: EXPECT_NE((FilterCallback*)NULL,
filter_1_callback_);
On 2010/12/16 18:43:39, scherkus wrote:
> ok I debugged this a bit because that's strange...
> 
> turns out EXPECT_EQ, EXPECT_TRUE, and EXPECT_FALSE all work but it's only
> EXPECT_NE that complains
> 
> you can either go around and static_cast<> everything but I think it'd be
easier
> to do EXPECT_TRUE/EXPECT_FALSE for pointer checks
> 
> caveat: I only tested this on linux so if it's win/mac that's complaining we
> might have to static_cast<> it up :(

Done. I just changed the EXPECT_NE()s to EXPECT_TRUE()

http://codereview.chromium.org/5744002/diff/24001/media/base/composite_filter...
media/base/composite_filter_unittest.cc:375: TEST_F(CompositeFilterTest,
TestThreadTermination) {
Removed the test case.
Agree that thread management needs to be changed, but will do so in a separate
CL.

http://codereview.chromium.org/5744002/diff/24001/media/base/mock_filters.h
File media/base/mock_filters.h (right):

http://codereview.chromium.org/5744002/diff/24001/media/base/mock_filters.h#n...
media/base/mock_filters.h:53: MockFilterCallback() :
callOnCallbackDestroyed_(true) {
On 2010/12/16 18:43:39, scherkus wrote:
> callOnCallbackDestroyed_ -> call_on_callback_destroyed_
> 
> or maybe a less verbose name like "run_destroy_callback_" etc.....

Done.

[scherkus (not reviewing), 2010-12-20 20:36:21]

LGTM w/ nits

thanks again for doing this!

http://codereview.chromium.org/5744002/diff/38001/media/base/composite_filter...
File media/base/composite_filter_unittest.cc (right):

http://codereview.chromium.org/5744002/diff/38001/media/base/composite_filter...
media/base/composite_filter_unittest.cc:229: EXPECT_TRUE(filter_1_callback_ !=
NULL);
you can even do without the != NULL part if you want

http://codereview.chromium.org/5744002/diff/38001/media/base/mock_filters.h
File media/base/mock_filters.h (right):

http://codereview.chromium.org/5744002/diff/38001/media/base/mock_filters.h#n...
media/base/mock_filters.h:57: run_destroy_callback_(run_destroy_callback) {
nit: de-indent two spaces

[acolwell GONE FROM CHROMIUM, 2010-12-30 17:36:07]

Fixed null pointer exception caused by DisableAudioRenderer() calls during
pipeline initialization. This can happen if audio device initialization fails
for some reason.

Added a unit test to test for this scenario.

[scherkus (not reviewing), 2011-01-05 01:28:38]

super tiny nits but LGTM

thanks for adding the test!

http://codereview.chromium.org/5744002/diff/61001/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/5744002/diff/61001/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:727:
pipeline_filter_->SetPlaybackRate(playback_rate);
nit: looks like this file uses { } for one-line if statements, and your other
pipeline_filter_ if statement uses it as well, so I guess let's go with { }

http://codereview.chromium.org/5744002/diff/61001/media/base/pipeline_impl_un...
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/5744002/diff/61001/media/base/pipeline_impl_un...
media/base/pipeline_impl_unittest.cc:184: 
nit: remove blank line

[acolwell GONE FROM CHROMIUM, 2011-01-05 16:27:46]

fixed nits

http://codereview.chromium.org/5744002/diff/61001/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/5744002/diff/61001/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:727:
pipeline_filter_->SetPlaybackRate(playback_rate);
On 2011/01/05 01:28:38, scherkus wrote:
> nit: looks like this file uses { } for one-line if statements, and your other
> pipeline_filter_ if statement uses it as well, so I guess let's go with { }

Done.

http://codereview.chromium.org/5744002/diff/61001/media/base/pipeline_impl_un...
File media/base/pipeline_impl_unittest.cc (right):

http://codereview.chromium.org/5744002/diff/61001/media/base/pipeline_impl_un...
media/base/pipeline_impl_unittest.cc:184: 
On 2011/01/05 01:28:38, scherkus wrote:
> nit: remove blank line

Done.