Move AudioRenderer out of Filter heirarchy.

Move AudioRenderer out of Filter heirarchy.

BUG=108341
TEST=none


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

[scherkus (not reviewing), 2012-07-10 03:58:22]

http://codereview.chromium.org/10753021/diff/2001/media/base/audio_renderer.h
File media/base/audio_renderer.h (right):

http://codereview.chromium.org/10753021/diff/2001/media/base/audio_renderer.h...
media/base/audio_renderer.h:73: virtual void Seek(base::TimeDelta time, const
PipelineStatusCB& callback) = 0;
think it's time to finally call this Preroll()? :)

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:64: struct Pipeline::PipelineInitState {
I think we can remove this entirely very soon

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:456: void Pipeline::DoPause(const base::Closure& done_cb)
{
these DoXXX() methods essentially replicate CompositeFilter's async serial
execution but via closure chaining.

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:459: if (pipeline_filter_) {
note that pipeline_filter_ ~= video_renderer_ (it's the only Filter class left!)

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:1271: // a bug.
FYI I think it's impossible for demuxer_ to be NULL as we always have a demuxer_
if we're going to be doing any seeking (i.e., initialization was successful)

time to remove the checks here?

http://codereview.chromium.org/10753021/diff/2001/media/filters/audio_rendere...
File media/filters/audio_renderer_impl_unittest.cc (left):

http://codereview.chromium.org/10753021/diff/2001/media/filters/audio_rendere...
media/filters/audio_renderer_impl_unittest.cc:71: EXPECT_CALL(host_, GetTime())
turns out ARI doesn't call GetTime() anymore!

[scherkus (not reviewing), 2012-07-10 04:02:51]

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:579: void Pipeline::OnFilterStateTransition() {
we might be to remove this trampoline since the DoXXX() methods incorporate
trampolining via RunOnPipelineThread()

hmm...

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:1165: audio_renderer_->SetHost(this);
I think SetHost() can go away after we replace ARH with individual CBs passed
into Initialize() (see TODO by ARH definition)

[ivan_cobs.com.sg, 2012-07-10 04:13:34]

pls remove ivan@cobs.com.sg & lynn@cobs.com.sg NOW

-----Original Message-----
From: scherkus@chromium.org [mailto:scherkus@chromium.org] 
Sent: Tuesday, 10 July, 2012 11:58 AM
To: acolwell@chromium.org
Cc: chromium-reviews@chromium.org; feature-media-reviews@chromium.org
Subject: [chromium-reviews] Move AudioRenderer out of Filter heirarchy.
(issue 10753021)

Reviewers: acolwell,


http://codereview.chromium.org/10753021/diff/2001/media/base/audio_renderer.
h
File media/base/audio_renderer.h (right):

http://codereview.chromium.org/10753021/diff/2001/media/base/audio_renderer.
h#newcode73
media/base/audio_renderer.h:73: virtual void Seek(base::TimeDelta time,
const PipelineStatusCB& callback) = 0; think it's time to finally call this
Preroll()? :)

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#new
code64
media/base/pipeline.cc:64: struct Pipeline::PipelineInitState { I think we
can remove this entirely very soon

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#new
code456
media/base/pipeline.cc:456: void Pipeline::DoPause(const base::Closure&
done_cb) {
these DoXXX() methods essentially replicate CompositeFilter's async serial
execution but via closure chaining.

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#new
code459
media/base/pipeline.cc:459: if (pipeline_filter_) { note that
pipeline_filter_ ~= video_renderer_ (it's the only Filter class left!)

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#new
code1271
media/base/pipeline.cc:1271: // a bug.
FYI I think it's impossible for demuxer_ to be NULL as we always have a
demuxer_ if we're going to be doing any seeking (i.e., initialization was
successful)

time to remove the checks here?

http://codereview.chromium.org/10753021/diff/2001/media/filters/audio_render
er_impl_unittest.cc
File media/filters/audio_renderer_impl_unittest.cc (left):

http://codereview.chromium.org/10753021/diff/2001/media/filters/audio_render
er_impl_unittest.cc#oldcode71
media/filters/audio_renderer_impl_unittest.cc:71: EXPECT_CALL(host_,
GetTime())
turns out ARI doesn't call GetTime() anymore!

Description:
Move AudioRenderer out of Filter heirarchy.

BUG=108341
TEST=none


Please review this at http://codereview.chromium.org/10753021/

SVN Base: svn://chrome-svn/chrome/trunk/src

Affected files:
   M media/base/audio_renderer.h
   M media/base/composite_filter.cc
   M media/base/filter_host.h
   M media/base/mock_filters.h
   M media/base/mock_filters.cc
   M media/base/pipeline.h
   M media/base/pipeline.cc
   M media/base/pipeline_unittest.cc
   M media/filters/audio_renderer_impl.h
   M media/filters/audio_renderer_impl.cc
   M media/filters/audio_renderer_impl_unittest.cc

[acolwell GONE FROM CHROMIUM, 2012-07-10 16:34:37]

http://codereview.chromium.org/10753021/diff/2001/media/base/audio_renderer.h
File media/base/audio_renderer.h (right):

http://codereview.chromium.org/10753021/diff/2001/media/base/audio_renderer.h...
media/base/audio_renderer.h:25: // Notifcation that audio rendering has reached
the end of the stream.
nit: s/Notifcation/Notification

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:68: scoped_refptr<CompositeFilter> composite;
If this is only holding video_renderer now can we just remove composite?

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:456: void Pipeline::DoPause(const base::Closure& done_cb)
{
On 2012/07/10 03:58:22, scherkus wrote:
> these DoXXX() methods essentially replicate CompositeFilter's async serial
> execution but via closure chaining.
Wow. This is not as clean as I had hoped. I think having a ClosureListHelper or
something might make this more straightforward and remove some of the duplicate
code. IWBN if you could make the code look something like this.

ClosureList closures;

if (audio_renderer_) {
   closures.Add(base::Bind(&AudioRenderer::Pause, audio_renderer_));

if (pipeline_filter_)
  closures.Add(base::Bind(&Filter::Pause,pipeline_filter_));

closures.Run(done_cb);

To me this seems more readable & execution order corresponds to reading the code
from top to bottom. The existing code is a little confusing because reading top
to bottom actually shows execution in the reverse order.

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:459: if (pipeline_filter_) {
On 2012/07/10 03:58:22, scherkus wrote:
> note that pipeline_filter_ ~= video_renderer_ (it's the only Filter class
left!)
Any reason we shouldn't rename pipeline_filter_ -> video_renderer_ right now to
reflect this fact?

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:579: void Pipeline::OnFilterStateTransition() {
On 2012/07/10 04:02:52, scherkus wrote:
> we might be to remove this trampoline since the DoXXX() methods incorporate
> trampolining via RunOnPipelineThread()
> 
> hmm...
This is an unconditional post though. RunOnPipelineThread() isn't. I'm pretty
sure this being unconditional is key because it makes sure any SetError() calls
that happen before the callback is run get executed first.

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:826: if (audio_renderer_)
This isn't consistent with the original code. It should be inside the
pipeline_filter_ conditional since that is only set once everything has been
initialized.

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:1165: audio_renderer_->SetHost(this);
On 2012/07/10 04:02:52, scherkus wrote:
> I think SetHost() can go away after we replace ARH with individual CBs passed
> into Initialize() (see TODO by ARH definition)
I think we should just add the callbacks now. I can't think of any good reason
to introduce the ARH class as an intermediate step.

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:1271: // a bug.
On 2012/07/10 03:58:22, scherkus wrote:
> FYI I think it's impossible for demuxer_ to be NULL as we always have a
demuxer_
> if we're going to be doing any seeking (i.e., initialization was successful)
> 
> time to remove the checks here?
Go for it. We'll see if it goes boom.

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.h
File media/base/pipeline.h (right):

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.h#newco...
media/base/pipeline.h:424: // Initiates an asynchronous Seek() call sequence
executing |done_cb| when
s/done_cb/seek_cb_ since there are no done_cb parameter here?

[Ami GONE FROM CHROMIUM, 2012-07-10 17:16:00]

drive-by

\o/

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:472: void Pipeline::DoFlush(const base::Closure& done_cb)
{
Before this CL CompositeFilter::Flush() was parallel, but this one isn't.

http://codereview.chromium.org/10753021/diff/2001/media/base/pipeline.cc#newc...
media/base/pipeline.cc:579: void Pipeline::OnFilterStateTransition() {
On 2012/07/10 16:34:37, acolwell wrote:
> On 2012/07/10 04:02:52, scherkus wrote:
> > we might be to remove this trampoline since the DoXXX() methods incorporate
> > trampolining via RunOnPipelineThread()
> > 
> > hmm...
> This is an unconditional post though. RunOnPipelineThread() isn't. I'm pretty
> sure this being unconditional is key because it makes sure any SetError()
calls
> that happen before the callback is run get executed first.

I agree w/ acolwell and suspect this means RunOnPipelineThread() can be dropped
entirely, because the done_cb passed to DoXXX() is invariably either
OnFilterStateTransition or OnTeardownStateTransition, each of which post
unconditionally.
In fact, instead of DoXXX() taking a mysterious black-box Closure, why not pass
a bool is_teardown and have it dispatch to OFST or OTST, as appropriate?

[scherkus (not reviewing), 2012-07-17 03:43:00]

https://chromiumcodereview.appspot.com/10753021/diff/2001/media/base/audio_re...
File media/base/audio_renderer.h (right):

https://chromiumcodereview.appspot.com/10753021/diff/2001/media/base/audio_re...
media/base/audio_renderer.h:25: // Notifcation that audio rendering has reached
the end of the stream.
On 2012/07/10 16:34:37, acolwell wrote:
> nit: s/Notifcation/Notification

Done.

https://chromiumcodereview.appspot.com/10753021/diff/2001/media/base/audio_re...
media/base/audio_renderer.h:73: virtual void Seek(base::TimeDelta time, const
PipelineStatusCB& callback) = 0;
On 2012/07/10 03:58:22, scherkus wrote:
> think it's time to finally call this Preroll()? :)

making it a TODO

https://chromiumcodereview.appspot.com/10753021/diff/2001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/10753021/diff/2001/media/base/pipeline...
media/base/pipeline.cc:68: scoped_refptr<CompositeFilter> composite;
On 2012/07/10 16:34:37, acolwell wrote:
> If this is only holding video_renderer now can we just remove composite?

Not yet because we set pipeline_filter_ to composite after init completes

https://chromiumcodereview.appspot.com/10753021/diff/2001/media/base/pipeline...
media/base/pipeline.cc:456: void Pipeline::DoPause(const base::Closure& done_cb)
{
On 2012/07/10 16:34:37, acolwell wrote:
> On 2012/07/10 03:58:22, scherkus wrote:
> > these DoXXX() methods essentially replicate CompositeFilter's async serial
> > execution but via closure chaining.
> Wow. This is not as clean as I had hoped. I think having a ClosureListHelper
or
> something might make this more straightforward and remove some of the
duplicate
> code. IWBN if you could make the code look something like this.
> 
> ClosureList closures;
> 
> if (audio_renderer_) {
>    closures.Add(base::Bind(&AudioRenderer::Pause, audio_renderer_));
> 
> if (pipeline_filter_)
>   closures.Add(base::Bind(&Filter::Pause,pipeline_filter_));
> 
> closures.Run(done_cb);
> 
> To me this seems more readable & execution order corresponds to reading the
code
> from top to bottom. The existing code is a little confusing because reading
top
> to bottom actually shows execution in the reverse order.

I played around with this for an hour or so and it looks like you can't have a
callback-to-a-callback.

Specifically, something like this (which is quite nice!) doesn't work:

typedef base::Callback<void(base::Closure)> ClosureCB;
scoped_ptr<std::queue<ClosureCB> > closures(new std::queue<ClosureCB>());
closures->push(base::Bind(&AudioRenderer::Pause, audio_renderer_));  // ***
Compiler can't resolve templates

// later on...

void RunSerially(scoped_ptr<std::queue<ClosureCB> > closures,
                 const base::Closure& done_cb) {
  ClosureCB cb = closures->front();
  closures->pop();

  if (!closures->empty()) {
    cb.Run(base::Bind(&RunSerially, base::Passed(&closures), done_cb));
    return;
  }

  cb.Run(done_cb);
}



I hacked around a bit with a class that would let you add closures to run but
it's a bit kludgy.

ClosureRunner* crunner = new ClosureRunner();
crunner->Add(base::Bind(&AudioRenderer::Pause, audio_renderer_,
crunner->GetAddCB()));  // Yuck.


I'm not an expert here so I'll chat with ajwong@.

https://chromiumcodereview.appspot.com/10753021/diff/2001/media/base/pipeline...
media/base/pipeline.cc:459: if (pipeline_filter_) {
On 2012/07/10 16:34:37, acolwell wrote:
> On 2012/07/10 03:58:22, scherkus wrote:
> > note that pipeline_filter_ ~= video_renderer_ (it's the only Filter class
> left!)
> Any reason we shouldn't rename pipeline_filter_ -> video_renderer_ right now
to
> reflect this fact?
> 

We have an actual video_renderer_ that we use for different purposes -- I think
I'd like to leave the name unchanged for now but add a TODO comment in the .h

https://chromiumcodereview.appspot.com/10753021/diff/2001/media/base/pipeline...
media/base/pipeline.cc:472: void Pipeline::DoFlush(const base::Closure& done_cb)
{
On 2012/07/10 17:16:00, Ami Fischman wrote:
> Before this CL CompositeFilter::Flush() was parallel, but this one isn't.

I forget why Flush() was parallel. Possibly as a work around for some race
condition, but I'm not sure anymore.

I need to think a bit more on how I want to write this callback chaining code.

https://chromiumcodereview.appspot.com/10753021/diff/2001/media/base/pipeline...
media/base/pipeline.cc:579: void Pipeline::OnFilterStateTransition() {
On 2012/07/10 17:16:00, Ami Fischman wrote:
> On 2012/07/10 16:34:37, acolwell wrote:
> > On 2012/07/10 04:02:52, scherkus wrote:
> > > we might be to remove this trampoline since the DoXXX() methods
incorporate
> > > trampolining via RunOnPipelineThread()
> > > 
> > > hmm...
> > This is an unconditional post though. RunOnPipelineThread() isn't. I'm
pretty
> > sure this being unconditional is key because it makes sure any SetError()
> calls
> > that happen before the callback is run get executed first.
> 
> I agree w/ acolwell and suspect this means RunOnPipelineThread() can be
dropped
> entirely, because the done_cb passed to DoXXX() is invariably either
> OnFilterStateTransition or OnTeardownStateTransition, each of which post
> unconditionally.

Done.

> In fact, instead of DoXXX() taking a mysterious black-box Closure, why not
pass
> a bool is_teardown and have it dispatch to OFST or OTST, as appropriate?

Hmm it's funny you mention the black box closure bit. I agree it's black-boxy
when reading DoXXX() code but I found it a bit clearer when reading the callsite
as you know exactly where you end up after all is said and done (i.e.,
FilterStateTransitionTask always binds to OFST).

https://chromiumcodereview.appspot.com/10753021/diff/2001/media/base/pipeline...
media/base/pipeline.cc:826: if (audio_renderer_)
On 2012/07/10 16:34:37, acolwell wrote:
> This isn't consistent with the original code. It should be inside the
> pipeline_filter_ conditional since that is only set once everything has been
> initialized.

Done.

https://chromiumcodereview.appspot.com/10753021/diff/2001/media/base/pipeline...
media/base/pipeline.cc:1165: audio_renderer_->SetHost(this);
On 2012/07/10 16:34:37, acolwell wrote:
> On 2012/07/10 04:02:52, scherkus wrote:
> > I think SetHost() can go away after we replace ARH with individual CBs
passed
> > into Initialize() (see TODO by ARH definition)
> I think we should just add the callbacks now. I can't think of any good reason
> to introduce the ARH class as an intermediate step.

Done.

https://chromiumcodereview.appspot.com/10753021/diff/2001/media/base/pipeline...
media/base/pipeline.cc:1271: // a bug.
On 2012/07/10 16:34:37, acolwell wrote:
> On 2012/07/10 03:58:22, scherkus wrote:
> > FYI I think it's impossible for demuxer_ to be NULL as we always have a
> demuxer_
> > if we're going to be doing any seeking (i.e., initialization was successful)
> > 
> > time to remove the checks here?
> Go for it. We'll see if it goes boom.

Committed separately as r146024.

https://chromiumcodereview.appspot.com/10753021/diff/2001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/10753021/diff/2001/media/base/pipeline...
media/base/pipeline.h:424: // Initiates an asynchronous Seek() call sequence
executing |done_cb| when
On 2012/07/10 16:34:37, acolwell wrote:
> s/done_cb/seek_cb_ since there are no done_cb parameter here?

Done.

[scherkus (not reviewing), 2012-07-18 22:42:54]

PTAL

acolwell pointed out my brainfart when I sent this out on monday that my typedef
was "base::Closure" instead of "const base::Closure&".

[acolwell GONE FROM CHROMIUM, 2012-07-19 00:58:18]

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/audio_r...
File media/base/audio_renderer.h (right):

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/audio_r...
media/base/audio_renderer.h:45: const base::Closure& ended_cb,
WDYT of moving this to Play() since it only makes sense to call this while
playing? Clear it when Pause()/Stop() is called. underflow_cb could be moved as
well for the same reasons.

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/callbac...
File media/base/callback_util.cc (right):

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/callbac...
media/base/callback_util.cc:12: namespace {
remove per our discussion.

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/callbac...
media/base/callback_util.cc:20: CountingCB(const base::Closure& done_cb)
explicit

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/pipelin...
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/pipelin...
media/base/pipeline.cc:1256: demuxer_->Seek(seek_timestamp, base::Bind(
It feels like you should be able to do the same thing as RunInSeries() here.
Create a queue of base::Callback<void((base::TimeDelta const PipelineStatusCB&)
and then create a function that has the trampoline code, calls ReportStatus() on
errors, and pops & runs the next callback on the queue. done_cb gets called when
the queue is empty.

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/pipelin...
File media/base/pipeline_unittest.cc (right):

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/pipelin...
media/base/pipeline_unittest.cc:203: WithArg<5>(RunClosure()))); //
|disabled_cb|.
nit: 2 spaces before comment

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/pipelin...
media/base/pipeline_unittest.cc:581: message_loop_.RunAllPending();
Why do you need to run the message_loop_ now?

[scherkus (not reviewing), 2012-07-19 21:29:02]

PTAL

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/audio_r...
File media/base/audio_renderer.h (right):

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/audio_r...
media/base/audio_renderer.h:45: const base::Closure& ended_cb,
On 2012/07/19 00:58:18, acolwell wrote:
> WDYT of moving this to Play() since it only makes sense to call this while
> playing? Clear it when Pause()/Stop() is called. underflow_cb could be moved
as
> well for the same reasons.

I tried it out but the only difference seemed to be adding even more DCHECKs
along with all the other state DCHECKs -- don't think it's particularly helpful
at the moment

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/callbac...
File media/base/callback_util.cc (right):

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/callbac...
media/base/callback_util.cc:12: namespace {
On 2012/07/19 00:58:18, acolwell wrote:
> remove per our discussion.

Done.

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/callbac...
media/base/callback_util.cc:20: CountingCB(const base::Closure& done_cb)
On 2012/07/19 00:58:18, acolwell wrote:
> explicit

Done.

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/pipelin...
File media/base/pipeline.cc (right):

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/pipelin...
media/base/pipeline.cc:1256: demuxer_->Seek(seek_timestamp, base::Bind(
On 2012/07/19 00:58:18, acolwell wrote:
> It feels like you should be able to do the same thing as RunInSeries() here.
> Create a queue of base::Callback<void((base::TimeDelta const
PipelineStatusCB&)
> and then create a function that has the trampoline code, calls ReportStatus()
on
> errors, and pops & runs the next callback on the queue. done_cb gets called
when
> the queue is empty.

Done and using the power of Bind() we don't need to keep carrying the
seek_timestamp param either!

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/pipelin...
File media/base/pipeline_unittest.cc (right):

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/pipelin...
media/base/pipeline_unittest.cc:203: WithArg<5>(RunClosure()))); //
|disabled_cb|.
On 2012/07/19 00:58:18, acolwell wrote:
> nit: 2 spaces before comment

Done.

https://chromiumcodereview.appspot.com/10753021/diff/21001/media/base/pipelin...
media/base/pipeline_unittest.cc:581: message_loop_.RunAllPending();
On 2012/07/19 00:58:18, acolwell wrote:
> Why do you need to run the message_loop_ now?

Turns out I don't need to!

I thought we might have to due to trampolining but the trampoline in this is
case is conditional

https://chromiumcodereview.appspot.com/10753021/diff/12030/media/base/callbac...
File media/base/callback_util.cc (right):

https://chromiumcodereview.appspot.com/10753021/diff/12030/media/base/callbac...
media/base/callback_util.cc:15: // Executes the given closure if and only if the
closure returned by
I tried the ref-counting approach for fun but something I overlooked yesterday
was that relying on ref-counting assumed that all closures would release their
ref via Run() but refs may also get released if cbs are reset / deallocated,
which would lead to mistakenly executing done_cb!

it's still refcounted (to avoid use-after-free) but we now require the closure
to be executed an exact # of times + DCHECK if we execute too many times

https://chromiumcodereview.appspot.com/10753021/diff/12030/media/base/callbac...
File media/base/callback_util.h (right):

https://chromiumcodereview.appspot.com/10753021/diff/12030/media/base/callbac...
media/base/callback_util.h:22: // All closures (including |done_cb|) are
executed on same thread as the
NOTE: removing the trampolining was a mistake as it caused closures to be called
on the previous closure's thread (composite filter would trampoline serial
callbacks), so now all closures + the done_cb are trampolined as needed.

Pipeline still passes in the force-trampolined version so order of tasks will
not have changed in the error condition.

https://chromiumcodereview.appspot.com/10753021/diff/12030/media/base/callbac...
media/base/callback_util.h:46: // All closures (including |done_cb|) are
executed on same thread as the
ditto for parallel

[acolwell GONE FROM CHROMIUM, 2012-07-19 23:09:14]

LGTM

https://chromiumcodereview.appspot.com/10753021/diff/12030/media/base/callbac...
File media/base/callback_util.cc (right):

https://chromiumcodereview.appspot.com/10753021/diff/12030/media/base/callbac...
media/base/callback_util.cc:21: explicit CountingCB(int count, const
base::Closure& done_cb)
nit: remove explicit now.

https://chromiumcodereview.appspot.com/10753021/diff/12030/media/base/callbac...
media/base/callback_util.cc:58: int completed_;
nit: Consider decrementing count_ so you don't need this.

[scherkus (not reviewing), 2012-07-19 23:46:47]

thanks!

https://chromiumcodereview.appspot.com/10753021/diff/12030/media/base/callbac...
File media/base/callback_util.cc (right):

https://chromiumcodereview.appspot.com/10753021/diff/12030/media/base/callbac...
media/base/callback_util.cc:21: explicit CountingCB(int count, const
base::Closure& done_cb)
On 2012/07/19 23:09:14, acolwell wrote:
> nit: remove explicit now.

Done.

https://chromiumcodereview.appspot.com/10753021/diff/12030/media/base/callbac...
media/base/callback_util.cc:58: int completed_;
On 2012/07/19 23:09:14, acolwell wrote:
> nit: Consider decrementing count_ so you don't need this.

Done.

[commit-bot: I haz the power, 2012-07-20 04:26:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/10753021/19038

[commit-bot: I haz the power, 2012-07-20 06:01:18]

Change committed as 147619