Switching decoders to use the injected message loop.

Switching decoders to use the injected message loop.

TEST=test should continue to pass, movies stay in sync
BUG=none

[scherkus (not reviewing), 2009-06-24 01:19:59]

This isn't 100% polished but it's interesting nonetheless.

Basically the public API starts to look very repetitive (in a good way!) 
Everything gets posted to the message loop.

It's somewhat of a re-write... a lot the logic got simplified and shuffled
around.

I tested it locally and everything still works great.  Bonus feature: treated
the message queue as input_queue_

[awong, 2009-06-24 01:27:02]

skimmed it really fast.  Looking nice!

If this were java, we'd be able to do some cool tricks with dynamic proxies to
get rid of the repetitive code. :)

Here, it's harder, though a Proxy/Pimpl (I get these confused sometimes) might
work later.

However, the repetition, I think, is just annoying, not dangerous because the
logic is very unlikely ever to diverge, and fanout is small, so merging is low
priority.

http://codereview.chromium.org/146068/diff/1/3
File media/filters/decoder_base.h (right):

http://codereview.chromium.org/146068/diff/1/3#newcode62
Line 62: max_pending_reads_(1),  // Temporary.
If it's any less temporary than a week, explain why 1 makes sense?

[Alpha Left Google, 2009-06-24 18:17:42]

Looking good!

http://codereview.chromium.org/146068/diff/1/3
File media/filters/decoder_base.h (right):

http://codereview.chromium.org/146068/diff/1/3#newcode15
Line 15: #include "base/stl_util-inl.h"
nit: reorder includes

http://codereview.chromium.org/146068/diff/1/3#newcode201
Line 201: DCHECK(input_queue_.empty());
I agree with you. I don't see any other place using pushing to input_queue_, may
better off get rid of it completely..

[scherkus (not reviewing), 2009-06-26 17:33:06]

ping!

[awong, 2009-06-26 17:45:03]

couple of comments.

http://codereview.chromium.org/146068/diff/1007/1009
File media/filters/decoder_base.h (right):

http://codereview.chromium.org/146068/diff/1007/1009#newcode56
Line 56: scoped_refptr<Buffer> buffer_ref = buffer;
Why aren't we getting a scoped_refptr<Buffer> in the parameter in the first
place?  Seems like a violation of how to use scoped_refptrs...

http://codereview.chromium.org/146068/diff/1007/1009#newcode70
Line 70: DCHECK(stopped_);
Is there a case wehre we're constructed and destructed before starting?

http://codereview.chromium.org/146068/diff/1007/1009#newcode117
Line 117: // Delegate to the subclass first.
Hmm...interesting....this start to make me think that moving forward, we really
_can_ move to a FilterProxy method of doing the message-loop indirection.

Just a thought. No action required.

http://codereview.chromium.org/146068/diff/1007/1009#newcode142
Line 142: DCHECK(!thread_id_);
DCHECKs at top?

http://codereview.chromium.org/146068/diff/1007/1009#newcode151
Line 151: DCHECK(!media_format_.empty()) << "Subclass did not set
media_format_";
Strange relationship of data...add TODO to change API so subclass does not need
to mutate base class's data?

[scherkus (not reviewing), 2009-06-29 22:52:36]

The unit tests were super broken but now they're not :)

http://codereview.chromium.org/146068/diff/1007/1009
File media/filters/decoder_base.h (right):

http://codereview.chromium.org/146068/diff/1007/1009#newcode56
Line 56: scoped_refptr<Buffer> buffer_ref = buffer;
On 2009/06/26 17:45:03, awong wrote:
> Why aren't we getting a scoped_refptr<Buffer> in the parameter in the first
> place?  Seems like a violation of how to use scoped_refptrs...

Another relic from when we weren't using scoped_refptr<> :(

Added TODOs in filters.h

http://codereview.chromium.org/146068/diff/1007/1009#newcode70
Line 70: DCHECK(stopped_);
On 2009/06/26 17:45:03, awong wrote:
> Is there a case wehre we're constructed and destructed before starting?

yes, most likely in the event of unit tests

I'll switch this to a mini state machine

http://codereview.chromium.org/146068/diff/1007/1009#newcode142
Line 142: DCHECK(!thread_id_);
On 2009/06/26 17:45:03, awong wrote:
> DCHECKs at top?

Done.

http://codereview.chromium.org/146068/diff/1007/1009#newcode151
Line 151: DCHECK(!media_format_.empty()) << "Subclass did not set
media_format_";
On 2009/06/26 17:45:03, awong wrote:
> Strange relationship of data...add TODO to change API so subclass does not
need
> to mutate base class's data?

Done.

[awong, 2009-06-29 23:06:23]

LGTM

couple small comment comments.

http://codereview.chromium.org/146068/diff/2001/3002
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/146068/diff/2001/3002#newcode461
Line 461: // TODO(scherkus): beware!  this can get posted multiple times!  it
shouldn't!
explain what we're supposed to do to fix?

http://codereview.chromium.org/146068/diff/2001/3003
File media/filters/decoder_base.h (right):

http://codereview.chromium.org/146068/diff/2001/3003#newcode55
Line 55: // that holds onto a reference via scoped_refptr<>.
TODO to kill this

[scherkus (not reviewing), 2009-06-29 23:16:02]

also added a fix for gcc + TODO

http://codereview.chromium.org/146068/diff/2001/3002
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/146068/diff/2001/3002#newcode461
Line 461: // TODO(scherkus): beware!  this can get posted multiple times!  it
shouldn't!
On 2009/06/29 23:06:23, awong wrote:
> explain what we're supposed to do to fix?

Done.

http://codereview.chromium.org/146068/diff/2001/3003
File media/filters/decoder_base.h (right):

http://codereview.chromium.org/146068/diff/2001/3003#newcode55
Line 55: // that holds onto a reference via scoped_refptr<>.
On 2009/06/29 23:06:23, awong wrote:
> TODO to kill this

Done.