Avoid double task trampoline for Pipeline state transitions.

Avoid double task trampoline for Pipeline state transitions.

Previously the initialization of ChunkDemuxer and Seeking would
result in a back to back trampoline of callback tasks, this can
lead to odd interleaving of error states when multiple threads
are involved.

In recent history we've stated it's always the callees job to
ensure callbacks are run on the right thread, this change further
clarifies the contract for Renderers and Demuxers that the callback
given to Initialize() must only be run after Initialize() has
returned.

BUG=none
TEST=none

Committed: https://crrev.com/21b128297b19076f5ef83596b9fddbe7c68c4a68
Cr-Commit-Position: refs/heads/master@{#312313}

[DaleCurtis, 2015-01-16 22:14:52]

dalecurtis@chromium.org changed reviewers:
+ scherkus@chromium.org, xhwang@chromium.org

[DaleCurtis, 2015-01-16 22:14:52]

WDYT? This appears to be the root cause of the interleaved errors seen when
attempting https://codereview.chromium.org/710693003/

I ran into the issue while trying to fix our completely broken preload=metadata
state and event delivery :| http://crbug.com/448580

A concern I have with this change is that it might impact our downstream clients
(cast, others?) which implement their own renderers.

[xhwang, 2015-01-16 23:09:07]

It's a classic dilemma between enforcing the "not call the callback
synchronously" rule and avoiding too many layer of posts. I am not sure where
the best middle ground is.

https://codereview.chromium.org/827013005/diff/20001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/827013005/diff/20001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:58: ACTION_TEMPLATE(TrampolineRunCallback,
How about PostCallback?

https://codereview.chromium.org/827013005/diff/20001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:62: ::std::tr1::get<k>(args));
worth adding to media/base/gmock_callback_support.h?

https://codereview.chromium.org/827013005/diff/20001/media/filters/renderer_i...
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/827013005/diff/20001/media/filters/renderer_i...
media/filters/renderer_impl.cc:83: init_cb_ = BindToCurrentLoop(init_cb);
This is the tricky part of our rule that callee must "ensure" the callback is
called asynchronously. I think InitializeAudioRenderer() makes sure init_cb_ is
always called asynchronously. We didn't use BindToCurrentLoop here because we
don't want to have too many posts which is unnecessary. But I agree that the
intention is not obvious...

Or are you worrying about calling the callback directly in any public method, to
avoid reentrancy back into the caller?

https://codereview.chromium.org/827013005/diff/20001/media/filters/renderer_i...
media/filters/renderer_impl.cc:247: task_runner_->PostTask(FROM_HERE,
base::Bind(done_cb, PIPELINE_OK));
See above: We post here; then audio_renderer_->Initialize() *should* also post
the done_cb.

https://codereview.chromium.org/827013005/diff/20001/media/mojo/services/mojo...
File media/mojo/services/mojo_renderer_impl.cc (right):

https://codereview.chromium.org/827013005/diff/20001/media/mojo/services/mojo...
media/mojo/services/mojo_renderer_impl.cc:87: weak_factory_.GetWeakPtr())));
ditto: we have three posts here:
1, BindToCurrentLoop on l.59.
2, remote_media_renderer_->Initialize() is a mojo call, which is async.
3, BindToCurrentLoop on l.86.

[DaleCurtis, 2015-01-16 23:15:16]

https://codereview.chromium.org/827013005/diff/20001/media/filters/renderer_i...
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/827013005/diff/20001/media/filters/renderer_i...
media/filters/renderer_impl.cc:83: init_cb_ = BindToCurrentLoop(init_cb);
On 2015/01/16 23:09:07, xhwang wrote:
> This is the tricky part of our rule that callee must "ensure" the callback is
> called asynchronously. I think InitializeAudioRenderer() makes sure init_cb_
is
> always called asynchronously. We didn't use BindToCurrentLoop here because we
> don't want to have too many posts which is unnecessary. But I agree that the
> intention is not obvious...
> 
> Or are you worrying about calling the callback directly in any public method,
to
> avoid reentrancy back into the caller?

I realize we post below and init cb still runs asynchronously, but it's not on a
clean stack like it was before. That's likely fine if the caller runs the
init_cb and does nothing after, but may result in some long stacks. I erred on
the side of forcing a clean stack even if it's unnecessary. I'd be okay with not
doing that since the stack still wouldn't be very deep.

[xhwang, 2015-01-16 23:24:04]

https://codereview.chromium.org/827013005/diff/20001/media/filters/renderer_i...
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/827013005/diff/20001/media/filters/renderer_i...
media/filters/renderer_impl.cc:83: init_cb_ = BindToCurrentLoop(init_cb);
On 2015/01/16 23:15:15, DaleCurtis wrote:
> On 2015/01/16 23:09:07, xhwang wrote:
> > This is the tricky part of our rule that callee must "ensure" the callback
is
> > called asynchronously. I think InitializeAudioRenderer() makes sure init_cb_
> is
> > always called asynchronously. We didn't use BindToCurrentLoop here because
we
> > don't want to have too many posts which is unnecessary. But I agree that the
> > intention is not obvious...
> > 
> > Or are you worrying about calling the callback directly in any public
method,
> to
> > avoid reentrancy back into the caller?
> 
> I realize we post below and init cb still runs asynchronously, but it's not on
a
> clean stack like it was before. That's likely fine if the caller runs the
> init_cb and does nothing after, but may result in some long stacks. I erred on
> the side of forcing a clean stack even if it's unnecessary. I'd be okay with
not
> doing that since the stack still wouldn't be very deep.

I am not too worried about long stack as long as we are not reentering the
caller or the callee in the middle of a previous call. I am trying to think of
some more general rule on this but apparently I need to think more on this :(

[DaleCurtis, 2015-01-16 23:33:49]

https://codereview.chromium.org/827013005/diff/20001/media/filters/renderer_i...
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/827013005/diff/20001/media/filters/renderer_i...
media/filters/renderer_impl.cc:83: init_cb_ = BindToCurrentLoop(init_cb);
On 2015/01/16 23:24:04, xhwang wrote:
> On 2015/01/16 23:15:15, DaleCurtis wrote:
> > On 2015/01/16 23:09:07, xhwang wrote:
> > > This is the tricky part of our rule that callee must "ensure" the callback
> is
> > > called asynchronously. I think InitializeAudioRenderer() makes sure
init_cb_
> > is
> > > always called asynchronously. We didn't use BindToCurrentLoop here because
> we
> > > don't want to have too many posts which is unnecessary. But I agree that
the
> > > intention is not obvious...
> > > 
> > > Or are you worrying about calling the callback directly in any public
> method,
> > to
> > > avoid reentrancy back into the caller?
> > 
> > I realize we post below and init cb still runs asynchronously, but it's not
on
> a
> > clean stack like it was before. That's likely fine if the caller runs the
> > init_cb and does nothing after, but may result in some long stacks. I erred
on
> > the side of forcing a clean stack even if it's unnecessary. I'd be okay with
> not
> > doing that since the stack still wouldn't be very deep.
> 
> I am not too worried about long stack as long as we are not reentering the
> caller or the callee in the middle of a previous call. I am trying to think of
> some more general rule on this but apparently I need to think more on this :(

I think just adding a note to the demuxer and renderer interfaces saying the
init_cb/status_cb "must be run asynchronously (i.e. not during the context of
the initialize()) call." sounds fine. Is there something else you're thinking
of?

[DaleCurtis, 2015-01-16 23:34:18]

Also, there might be some way to enforce this using a sequence checker for debug
builds... I'll take a look.

[xhwang, 2015-01-16 23:55:32]

On 2015/01/16 23:33:49, DaleCurtis wrote:
>
https://codereview.chromium.org/827013005/diff/20001/media/filters/renderer_i...
> File media/filters/renderer_impl.cc (right):
> 
>
https://codereview.chromium.org/827013005/diff/20001/media/filters/renderer_i...
> media/filters/renderer_impl.cc:83: init_cb_ = BindToCurrentLoop(init_cb);
> On 2015/01/16 23:24:04, xhwang wrote:
> > On 2015/01/16 23:15:15, DaleCurtis wrote:
> > > On 2015/01/16 23:09:07, xhwang wrote:
> > > > This is the tricky part of our rule that callee must "ensure" the
callback
> > is
> > > > called asynchronously. I think InitializeAudioRenderer() makes sure
> init_cb_
> > > is
> > > > always called asynchronously. We didn't use BindToCurrentLoop here
because
> > we
> > > > don't want to have too many posts which is unnecessary. But I agree that
> the
> > > > intention is not obvious...
> > > > 
> > > > Or are you worrying about calling the callback directly in any public
> > method,
> > > to
> > > > avoid reentrancy back into the caller?
> > > 
> > > I realize we post below and init cb still runs asynchronously, but it's
not
> on
> > a
> > > clean stack like it was before. That's likely fine if the caller runs the
> > > init_cb and does nothing after, but may result in some long stacks. I
erred
> on
> > > the side of forcing a clean stack even if it's unnecessary. I'd be okay
with
> > not
> > > doing that since the stack still wouldn't be very deep.
> > 
> > I am not too worried about long stack as long as we are not reentering the
> > caller or the callee in the middle of a previous call. I am trying to think
of
> > some more general rule on this but apparently I need to think more on this
:(
> 
> I think just adding a note to the demuxer and renderer interfaces saying the
> init_cb/status_cb "must be run asynchronously (i.e. not during the context of
> the initialize()) call." sounds fine. Is there something else you're thinking
> of?

I was just trying to think of some more generic rule about callback
calling/posting. "Callee should post the callback" doesn't really work
everywhere.

[DaleCurtis, 2015-01-20 21:37:44]

PTAL.  I went with the text "|cb| must always be run after this method returns."

https://codereview.chromium.org/827013005/diff/20001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/827013005/diff/20001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:58: ACTION_TEMPLATE(TrampolineRunCallback,
On 2015/01/16 23:09:07, xhwang wrote:
> How about PostCallback?

Done.

https://codereview.chromium.org/827013005/diff/20001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:62: ::std::tr1::get<k>(args));
On 2015/01/16 23:09:07, xhwang wrote:
> worth adding to media/base/gmock_callback_support.h?

If we see this pattern elsewhere, maybe, but these days I think most people
avoid gmock like the plague (for many reasons; search chromium-dev for
illumination :).

https://codereview.chromium.org/827013005/diff/20001/media/filters/renderer_i...
File media/filters/renderer_impl.cc (right):

https://codereview.chromium.org/827013005/diff/20001/media/filters/renderer_i...
media/filters/renderer_impl.cc:83: init_cb_ = BindToCurrentLoop(init_cb);
On 2015/01/16 23:33:49, DaleCurtis wrote:
> On 2015/01/16 23:24:04, xhwang wrote:
> > On 2015/01/16 23:15:15, DaleCurtis wrote:
> > > On 2015/01/16 23:09:07, xhwang wrote:
> > > > This is the tricky part of our rule that callee must "ensure" the
callback
> > is
> > > > called asynchronously. I think InitializeAudioRenderer() makes sure
> init_cb_
> > > is
> > > > always called asynchronously. We didn't use BindToCurrentLoop here
because
> > we
> > > > don't want to have too many posts which is unnecessary. But I agree that
> the
> > > > intention is not obvious...
> > > > 
> > > > Or are you worrying about calling the callback directly in any public
> > method,
> > > to
> > > > avoid reentrancy back into the caller?
> > > 
> > > I realize we post below and init cb still runs asynchronously, but it's
not
> on
> > a
> > > clean stack like it was before. That's likely fine if the caller runs the
> > > init_cb and does nothing after, but may result in some long stacks. I
erred
> on
> > > the side of forcing a clean stack even if it's unnecessary. I'd be okay
with
> > not
> > > doing that since the stack still wouldn't be very deep.
> > 
> > I am not too worried about long stack as long as we are not reentering the
> > caller or the callee in the middle of a previous call. I am trying to think
of
> > some more general rule on this but apparently I need to think more on this
:(
> 
> I think just adding a note to the demuxer and renderer interfaces saying the
> init_cb/status_cb "must be run asynchronously (i.e. not during the context of
> the initialize()) call." sounds fine. Is there something else you're thinking
> of?

Done.

https://codereview.chromium.org/827013005/diff/20001/media/mojo/services/mojo...
File media/mojo/services/mojo_renderer_impl.cc (right):

https://codereview.chromium.org/827013005/diff/20001/media/mojo/services/mojo...
media/mojo/services/mojo_renderer_impl.cc:87: weak_factory_.GetWeakPtr())));
On 2015/01/16 23:09:07, xhwang wrote:
> ditto: we have three posts here:
> 1, BindToCurrentLoop on l.59.
> 2, remote_media_renderer_->Initialize() is a mojo call, which is async.
> 3, BindToCurrentLoop on l.86.
> 

Done.

https://codereview.chromium.org/827013005/diff/20001/media/mojo/services/mojo...
media/mojo/services/mojo_renderer_impl.cc:200: 
Hmm looks like this code should run init_cb_ on error?

[xhwang, 2015-01-20 23:00:39]

I like the new text.

LGTM

https://codereview.chromium.org/827013005/diff/20001/media/base/pipeline_unit...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/827013005/diff/20001/media/base/pipeline_unit...
media/base/pipeline_unittest.cc:62: ::std::tr1::get<k>(args));
On 2015/01/20 21:37:43, DaleCurtis wrote:
> On 2015/01/16 23:09:07, xhwang wrote:
> > worth adding to media/base/gmock_callback_support.h?
> 
> If we see this pattern elsewhere, maybe, but these days I think most people
> avoid gmock like the plague (for many reasons; search chromium-dev for
> illumination :).

\o/ Acknowledged.

https://codereview.chromium.org/827013005/diff/20001/media/mojo/services/mojo...
File media/mojo/services/mojo_renderer_impl.cc (right):

https://codereview.chromium.org/827013005/diff/20001/media/mojo/services/mojo...
media/mojo/services/mojo_renderer_impl.cc:200: 
On 2015/01/20 21:37:44, DaleCurtis wrote:
> Hmm looks like this code should run init_cb_ on error?

Looks like we are relying on OnInitialized() to fire |init_cb|. So if we have
error happened during initialization, we should get an OnError() first, then get
an OnInitialized().

[DaleCurtis, 2015-01-21 01:33:54]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2015-01-21 01:35:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/827013005/40001

[commit-bot: I haz the power, 2015-01-21 11:09:55]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-21 11:11:21]

Patchset 3 (id:??) landed as
https://crrev.com/21b128297b19076f5ef83596b9fddbe7c68c4a68
Cr-Commit-Position: refs/heads/master@{#312313}