Move Pipeline permanent callbacks into Pipeline::Client interface.

Move Pipeline permanent callbacks into Pipeline::Client interface.

We have already reach the gmock limit of 10 arguments for
Pipeline::Start function. So we need to move the arguments into a
container to be able to add more callbacks.

BUG=571155
Committed: https://crrev.com/967c902454f4a36e0d0e44d686dc5b76c1467255
Cr-Commit-Position: refs/heads/master@{#392005}

[alokp, 2016-04-20 23:47:45]

alokp@chromium.org changed reviewers:
+ sandersd@chromium.org, xhwang@chromium.org

[alokp, 2016-04-20 23:47:45]

I still need to make everything compile and run. But could you guys please check
if the proposed Pipeline::Start API looks OK?

[sandersd (OOO until July 31), 2016-04-21 00:34:55]

On 2016/04/20 23:47:45, alokp wrote:
> I still need to make everything compile and run. But could you guys please
check
> if the proposed Pipeline::Start API looks OK?

I don't see that that's actually changed in this CL yet. What exactly do you
want reviewed?

FYI I'll be OOO from tomorrow until Tuesday, so this will mostly be on xhwang@.

[sandersd (OOO until July 31), 2016-04-21 00:36:30]

My mistake, I has assumed pipeline_impl.h would be updated as well.

Looks about how I would expect for specifically that method, and passing the
client in the constructor does seem like the best option.

https://codereview.chromium.org/1904793002/diff/30001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/1904793002/diff/30001/media/base/pipeline.h#n...
media/base/pipeline.h:47: const PipelineStatusCB& seek_cb) = 0;
Are we going to rename this init_cb while we are at it?

[xhwang, 2016-04-21 05:50:35]

The client interface looks good and it does simplify a lot of things. Thanks for
working on this!

I have a few comments on where to define the client interface and how to pass it
in.

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
media/base/pipeline.h:47: const PipelineStatusCB& seek_cb) = 0;
On 2016/04/21 00:36:30, sandersd wrote:
> Are we going to rename this init_cb while we are at it?

Or |start_cb|?

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
media/base/pipeline.h:135: Pipeline(PipelineClient* client) : client_(client) {}
What's the value of putting this in "protected"? Pipeline is pure virtual and
can't be instantiated anyways?

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
media/base/pipeline.h:140: PipelineClient* client_;
This would work. But I am not a huge fan of storing members on the pure
interface. Will it work if we pass the PipelineClient in Start() (part of the
interface) instead of in the constructor?

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
File media/base/pipeline_client.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
media/base/pipeline_client.h:15: class PipelineClient {
This can also be a inner class, e.g. Pipeline::Client, which IMHO is cleaner
since the interface and client interface will be in the same file. We have a
similar model for the mojo interface and mojo client interface.

[alokp, 2016-04-21 21:56:38]

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
media/base/pipeline.h:47: const PipelineStatusCB& seek_cb) = 0;
On 2016/04/21 05:50:35, xhwang wrote:
> On 2016/04/21 00:36:30, sandersd wrote:
> > Are we going to rename this init_cb while we are at it?
> 
> Or |start_cb|?

This particular callback is shared between Start, Seek, and Resume. Renaming it
would make it "wrong" for Seek. Nothing wrong with renaming the Start argument
though, but internally we might still want to call it seek_cb?

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
media/base/pipeline.h:135: Pipeline(PipelineClient* client) : client_(client) {}
On 2016/04/21 05:50:35, xhwang wrote:
> What's the value of putting this in "protected"? Pipeline is pure virtual and
> can't be instantiated anyways?

Done.

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
media/base/pipeline.h:140: PipelineClient* client_;
On 2016/04/21 05:50:35, xhwang wrote:
> This would work. But I am not a huge fan of storing members on the pure
> interface. Will it work if we pass the PipelineClient in Start() (part of the
> interface) instead of in the constructor?

Yeah this will work since client is only being used during playback. Done.
Although I do not like the interface name - PipelineClient - anymore. Clients
are usually passed in the constructor or Initialize calls.

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
File media/base/pipeline_client.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
media/base/pipeline_client.h:15: class PipelineClient {
On 2016/04/21 05:50:35, xhwang wrote:
> This can also be a inner class, e.g. Pipeline::Client, which IMHO is cleaner
> since the interface and client interface will be in the same file. We have a
> similar model for the mojo interface and mojo client interface.

Sure. Do you have a better name for Client? Now that we are passing it at Start,
I do think this is a good name anymore, may be Pipeline::PlaybackCallbacks?

[xhwang, 2016-04-21 22:26:03]

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

[xhwang, 2016-04-21 22:26:04]

Add dalecurtis@ since this CL isn't trivial and sandersd@ is OOO.

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
media/base/pipeline.h:47: const PipelineStatusCB& seek_cb) = 0;
On 2016/04/21 21:56:38, alokp wrote:
> On 2016/04/21 05:50:35, xhwang wrote:
> > On 2016/04/21 00:36:30, sandersd wrote:
> > > Are we going to rename this init_cb while we are at it?
> > 
> > Or |start_cb|?
> 
> This particular callback is shared between Start, Seek, and Resume. Renaming
it
> would make it "wrong" for Seek. Nothing wrong with renaming the Start argument
> though, but internally we might still want to call it seek_cb?

Right. We should separate the initial seek_cb (for Start) and the rest (for Seek
etc). Can we use a start_cb for the former, and have a OnSeeked() call on the
client for the latter?

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
media/base/pipeline.h:140: PipelineClient* client_;
On 2016/04/21 21:56:38, alokp wrote:
> On 2016/04/21 05:50:35, xhwang wrote:
> > This would work. But I am not a huge fan of storing members on the pure
> > interface. Will it work if we pass the PipelineClient in Start() (part of
the
> > interface) instead of in the constructor?
> 
> Yeah this will work since client is only being used during playback. Done.
> Although I do not like the interface name - PipelineClient - anymore. Clients
> are usually passed in the constructor or Initialize calls.

The Start() is really a combination of both Initialize() and StartPlayback().
Maybe the following will look nicer to you?

virtual void Initialize(Demuxer* demuxer,
                        scoped_ptr<Renderer> renderer,
                        Client* client) = 0;

virtual void Start(const PipelineStatusCB& start_cb) = 0;

[xhwang, 2016-04-21 22:32:06]

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
media/base/pipeline.h:140: PipelineClient* client_;
On 2016/04/21 22:26:04, xhwang wrote:
> On 2016/04/21 21:56:38, alokp wrote:
> > On 2016/04/21 05:50:35, xhwang wrote:
> > > This would work. But I am not a huge fan of storing members on the pure
> > > interface. Will it work if we pass the PipelineClient in Start() (part of
> the
> > > interface) instead of in the constructor?
> > 
> > Yeah this will work since client is only being used during playback. Done.
> > Although I do not like the interface name - PipelineClient - anymore.
Clients
> > are usually passed in the constructor or Initialize calls.
> 
> The Start() is really a combination of both Initialize() and StartPlayback().
> Maybe the following will look nicer to you?
> 
> virtual void Initialize(Demuxer* demuxer,
>                         scoped_ptr<Renderer> renderer,
>                         Client* client) = 0;
> 
> virtual void Start(const PipelineStatusCB& start_cb) = 0;

To be clear. I am not proposing this change. Just trying to show that  maybe
passing the Client in Start() isn't as bad as you thought.

[alokp, 2016-04-22 05:05:33]

Description was changed from

==========
Add media::PipelineClient.

BUG=571155
==========

to

==========
Move Pipeline permanent callbacks into Pipeline::Client interface.

We have already reach the gmock limit of 10 arguments for
Pipeline::Start function. So we need to move the arguments into a
container to be able to add more callbacks.

BUG=571155
==========

[alokp, 2016-04-22 05:06:06]

Description was changed from

==========
Move Pipeline permanent callbacks into Pipeline::Client interface.

We have already reach the gmock limit of 10 arguments for
Pipeline::Start function. So we need to move the arguments into a
container to be able to add more callbacks.

BUG=571155
==========

to

==========
Move Pipeline permanent callbacks into Pipeline::Client interface.

We have already reach the gmock limit of 10 arguments for
Pipeline::Start function. So we need to move the arguments into a
container to be able to add more callbacks.

BUG=571155
==========

[alokp, 2016-04-22 05:08:59]

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
media/base/pipeline.h:47: const PipelineStatusCB& seek_cb) = 0;
On 2016/04/21 22:26:04, xhwang wrote:
> On 2016/04/21 21:56:38, alokp wrote:
> > On 2016/04/21 05:50:35, xhwang wrote:
> > > On 2016/04/21 00:36:30, sandersd wrote:
> > > > Are we going to rename this init_cb while we are at it?
> > > 
> > > Or |start_cb|?
> > 
> > This particular callback is shared between Start, Seek, and Resume. Renaming
> it
> > would make it "wrong" for Seek. Nothing wrong with renaming the Start
argument
> > though, but internally we might still want to call it seek_cb?
> 
> Right. We should separate the initial seek_cb (for Start) and the rest (for
Seek
> etc). Can we use a start_cb for the former, and have a OnSeeked() call on the
> client for the latter?

I will need to look into this closely. I think this can be done in a separate
patch though. I would prefer to keep this focused on just moving the permanent
callbacks into Pipeline::Client class.

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
media/base/pipeline.h:140: PipelineClient* client_;
On 2016/04/21 22:32:05, xhwang wrote:
> On 2016/04/21 22:26:04, xhwang wrote:
> > On 2016/04/21 21:56:38, alokp wrote:
> > > On 2016/04/21 05:50:35, xhwang wrote:
> > > > This would work. But I am not a huge fan of storing members on the pure
> > > > interface. Will it work if we pass the PipelineClient in Start() (part
of
> > the
> > > > interface) instead of in the constructor?
> > > 
> > > Yeah this will work since client is only being used during playback. Done.
> > > Although I do not like the interface name - PipelineClient - anymore.
> Clients
> > > are usually passed in the constructor or Initialize calls.
> > 
> > The Start() is really a combination of both Initialize() and
StartPlayback().
> > Maybe the following will look nicer to you?
> > 
> > virtual void Initialize(Demuxer* demuxer,
> >                         scoped_ptr<Renderer> renderer,
> >                         Client* client) = 0;
> > 
> > virtual void Start(const PipelineStatusCB& start_cb) = 0;
> 
> To be clear. I am not proposing this change. Just trying to show that  maybe
> passing the Client in Start() isn't as bad as you thought.

This is a minor thing. I have kept it in Start for now.

[xhwang, 2016-04-22 16:05:05]

Is this CL ready for full review now or are you still updating?

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/30001/media/base/pipel...
media/base/pipeline.h:47: const PipelineStatusCB& seek_cb) = 0;
On 2016/04/22 05:08:59, alokp wrote:
> On 2016/04/21 22:26:04, xhwang wrote:
> > On 2016/04/21 21:56:38, alokp wrote:
> > > On 2016/04/21 05:50:35, xhwang wrote:
> > > > On 2016/04/21 00:36:30, sandersd wrote:
> > > > > Are we going to rename this init_cb while we are at it?
> > > > 
> > > > Or |start_cb|?
> > > 
> > > This particular callback is shared between Start, Seek, and Resume.
Renaming
> > it
> > > would make it "wrong" for Seek. Nothing wrong with renaming the Start
> argument
> > > though, but internally we might still want to call it seek_cb?
> > 
> > Right. We should separate the initial seek_cb (for Start) and the rest (for
> Seek
> > etc). Can we use a start_cb for the former, and have a OnSeeked() call on
the
> > client for the latter?
> 
> I will need to look into this closely. I think this can be done in a separate
> patch though. I would prefer to keep this focused on just moving the permanent
> callbacks into Pipeline::Client class.

sgtm. A TODO would be nice here.

[alokp, 2016-04-22 16:09:26]

On 2016/04/22 16:05:05, xhwang wrote:
> Is this CL ready for full review now or are you still updating?
> 

So far I have only updated unittests to use the new API. I still need to update
rest of the code using Pipeline. But before I go ahead and do that, I wanted to
get feedback on the new API.

Dale/Xiaohan: Does the API look reasonable now?

[xhwang, 2016-04-22 16:13:04]

The new API lg. Thanks!

[alokp, 2016-04-22 19:24:03]

ready for review - PTAL. Thanks!

[xhwang, 2016-04-25 22:49:45]

I didn't review everything. Please see the comments about threading. Thanks
again for working on this!

https://chromiumcodereview.appspot.com/1904793002/diff/170001/media/base/pipe...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/170001/media/base/pipe...
media/base/pipeline.h:78: const PipelineStatusCB& seek_cb) = 0;
Can you add a TODO about splitting |seek_cb|? You can put my name there if you
like :)

https://chromiumcodereview.appspot.com/1904793002/diff/170001/media/base/pipe...
File media/base/pipeline_impl.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/170001/media/base/pipe...
media/base/pipeline_impl.h:88: Client* client,
Document that |client| call be called on any thread. See below for more
threading discussion.

https://chromiumcodereview.appspot.com/1904793002/diff/170001/media/blink/web...
File media/blink/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/170001/media/blink/web...
media/blink/webmediaplayer_impl.cc:1279:
pipeline_controller_.Start(demuxer_.get(), this, is_streaming, is_static);
In the old code we use BIND_TO_RENDER_LOOP and BindToCurrentLoop a lot in WMPI
and PipelineController so that all callbacks will be fired on the main render
thread. The reason of this is that historically WMPI (as a blink WMP
implementation), lives exclusively on the main render thread. In theory,
PipelineImpl should the only class that knows about both threads.

Now with this CL, it appears we could call into Blink on the main thread (e.g.
encrypted_client_->didBlockPlaybackWaitingForKey()), which seems wrong.

Can we provide a Pipeline::Client implementation (e.g. ClientProxy) that makes
sure all calls are forwarded to the main render thread to avoid this issue?

[sandersd (OOO until July 31), 2016-04-26 23:16:36]

https://chromiumcodereview.appspot.com/1904793002/diff/170001/media/blink/web...
File media/blink/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/170001/media/blink/web...
media/blink/webmediaplayer_impl.cc:1279:
pipeline_controller_.Start(demuxer_.get(), this, is_streaming, is_static);
On 2016/04/25 22:49:45, xhwang wrote:
> In the old code we use BIND_TO_RENDER_LOOP and BindToCurrentLoop a lot in WMPI
> and PipelineController so that all callbacks will be fired on the main render
> thread. The reason of this is that historically WMPI (as a blink WMP
> implementation), lives exclusively on the main render thread. In theory,
> PipelineImpl should the only class that knows about both threads.
> 
> Now with this CL, it appears we could call into Blink on the main thread (e.g.
> encrypted_client_->didBlockPlaybackWaitingForKey()), which seems wrong.
> 
> Can we provide a Pipeline::Client implementation (e.g. ClientProxy) that makes
> sure all calls are forwarded to the main render thread to avoid this issue?

I would argue that Pipeline should take a loop along with the client interface,
and should be responsible for posting those calls. This isn't different, other
than whether there is an additional helper class involved.

https://chromiumcodereview.appspot.com/1904793002/diff/310001/media/base/pipe...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/310001/media/base/pipe...
media/base/pipeline.h:32: virtual ~Client() {}
It seems to me that this class should be pure-virtual and certainly
non-destructible. Are there enough client implementations that it makes a big
difference for the base class to provide implementations?

[xhwang, 2016-04-26 23:35:49]

https://chromiumcodereview.appspot.com/1904793002/diff/170001/media/blink/web...
File media/blink/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/170001/media/blink/web...
media/blink/webmediaplayer_impl.cc:1279:
pipeline_controller_.Start(demuxer_.get(), this, is_streaming, is_static);
On 2016/04/26 23:16:35, sandersd wrote:
> On 2016/04/25 22:49:45, xhwang wrote:
> > In the old code we use BIND_TO_RENDER_LOOP and BindToCurrentLoop a lot in
WMPI
> > and PipelineController so that all callbacks will be fired on the main
render
> > thread. The reason of this is that historically WMPI (as a blink WMP
> > implementation), lives exclusively on the main render thread. In theory,
> > PipelineImpl should the only class that knows about both threads.
> > 
> > Now with this CL, it appears we could call into Blink on the main thread
(e.g.
> > encrypted_client_->didBlockPlaybackWaitingForKey()), which seems wrong.
> > 
> > Can we provide a Pipeline::Client implementation (e.g. ClientProxy) that
makes
> > sure all calls are forwarded to the main render thread to avoid this issue?
> 
> I would argue that Pipeline should take a loop along with the client
interface,
> and should be responsible for posting those calls. This isn't different, other
> than whether there is an additional helper class involved.

That would also work for me.

[alokp, 2016-04-28 00:29:40]

https://chromiumcodereview.appspot.com/1904793002/diff/170001/media/blink/web...
File media/blink/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/170001/media/blink/web...
media/blink/webmediaplayer_impl.cc:1279:
pipeline_controller_.Start(demuxer_.get(), this, is_streaming, is_static);
On 2016/04/26 23:35:49, xhwang wrote:
> On 2016/04/26 23:16:35, sandersd wrote:
> > On 2016/04/25 22:49:45, xhwang wrote:
> > > In the old code we use BIND_TO_RENDER_LOOP and BindToCurrentLoop a lot in
> WMPI
> > > and PipelineController so that all callbacks will be fired on the main
> render
> > > thread. The reason of this is that historically WMPI (as a blink WMP
> > > implementation), lives exclusively on the main render thread. In theory,
> > > PipelineImpl should the only class that knows about both threads.
> > > 
> > > Now with this CL, it appears we could call into Blink on the main thread
> (e.g.
> > > encrypted_client_->didBlockPlaybackWaitingForKey()), which seems wrong.
> > > 
> > > Can we provide a Pipeline::Client implementation (e.g. ClientProxy) that
> makes
> > > sure all calls are forwarded to the main render thread to avoid this
issue?
> > 
> > I would argue that Pipeline should take a loop along with the client
> interface,
> > and should be responsible for posting those calls. This isn't different,
other
> > than whether there is an additional helper class involved.
> 
> That would also work for me.

Good catch - I missed the threading considerations. After much delibration I
decided to push all the threading logic to PipelineImpl because as Xiaohan
mentioned that is the only class that knows about the existence of multiple
threads.

Now the clients of PipelineImpl use it on the main thread and receive all
notifications/callbacks on the main thread. I also made Pipeline::Stop blocking
so that the clients do not need to worry about waiting for stop_cb.

There is still copious amount of logic and ugliness inside PipelineImpl but it
is at least isolated in one file. The client interface is much cleaner at least
from threading POV. I have a bunch of DCHECK to ensure that each function is
running on correct thread and a few TODOs to add to a some other functions.
PTAL.

[sandersd (OOO until July 31), 2016-05-02 18:37:47]

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline.h:32: virtual ~Client() {}
I still don't think we want to expose a destructor on this interface. It makes
no sense for Pipeline to call it.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:41: :
main_task_runner_(base::ThreadTaskRunnerHandle::Get()),
Perhaps this should be explicitly passed, since |client| is explicit and the two
are related?

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:99: FROM_HERE, base::Bind(&PipelineImpl::StopTask,
weak_this_, &waiter));
I'm not a fan of this pattern; it was bad in WMPI and it's worse hidden behind
an API. (Not that I have immediately implementable improvements to offer.)

At the very least, we should harden it against the |media_task_runner_| being
stopped, and document that it synchronously calls to the media thread (since
this could cause deadlocks).

If there is no bug already for replacing it, we should file one
(Hotlist-CodeHealth).

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:605: base::ResetAndReturn(&seek_cb_).Run(status_);
Are we sure it's safe to drop these from the Stop() path? It's an overall
improvement to the design, but I worry that other code may still be waiting for
these callbacks. (Maybe it doesn't matter anyway, since the resulting state is
kStopped, but I've not checked every caller.)

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
File media/base/pipeline_impl.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.h:227: // Stops media rendering and signals the
|waiter| when done.
Newline before comment.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.h:237: // Task runner used to execute pipeline tasks.
Newline before comment.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/blink/web...
File media/blink/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/blink/web...
media/blink/webmediaplayer_impl.cc:254: // Pipeline must be stopped before it is
destroyed.
Newline before comment.

[xhwang, 2016-05-02 20:58:35]

I didn't review everything. Have two kinda high-level comments for discussion.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline.h:58: virtual void OnWaitingForDecryptionKey() {}
How much does the default implementations save us here (compared to making this
interface pure virtual)? I am slightly worried that in a Client implementation
we could forget to override a method without any warning.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:41: :
main_task_runner_(base::ThreadTaskRunnerHandle::Get()),
On 2016/05/02 18:37:47, sandersd wrote:
> Perhaps this should be explicitly passed, since |client| is explicit and the
two
> are related?

+1

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:99: FROM_HERE, base::Bind(&PipelineImpl::StopTask,
weak_this_, &waiter));
On 2016/05/02 18:37:47, sandersd wrote:
> I'm not a fan of this pattern; it was bad in WMPI and it's worse hidden behind
> an API. (Not that I have immediately implementable improvements to offer.)
> 
> At the very least, we should harden it against the |media_task_runner_| being
> stopped, and document that it synchronously calls to the media thread (since
> this could cause deadlocks).
> 
> If there is no bug already for replacing it, we should file one
> (Hotlist-CodeHealth).

We already have a waiter in WMPI when Pipeline::Stop() is called:
https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed...

We don't like that pattern but it's kinda hard to remove (at least when I looked
at it last time). Therefore, it'd be nice if we could avoid adding another
waiter.

Is there anything particular that you want to solve by using a sync Stop with
waiter instead of the async Stop?

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:362: base::Unretained(client_)));
Could you explain why base::Unretained is safe here and below? Is it possible
that when this task is run, the whole media element and WMPI/Pipeline is already
destroyed?

[alokp, 2016-05-02 22:25:18]

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline.h:32: virtual ~Client() {}
On 2016/05/02 18:37:47, sandersd wrote:
> I still don't think we want to expose a destructor on this interface. It makes
> no sense for Pipeline to call it.

Done.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline.h:58: virtual void OnWaitingForDecryptionKey() {}
On 2016/05/02 20:58:35, xhwang wrote:
> How much does the default implementations save us here (compared to making
this
> interface pure virtual)? I am slightly worried that in a Client implementation
> we could forget to override a method without any warning.

Client is now abstract.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:41: :
main_task_runner_(base::ThreadTaskRunnerHandle::Get()),
On 2016/05/02 20:58:35, xhwang wrote:
> On 2016/05/02 18:37:47, sandersd wrote:
> > Perhaps this should be explicitly passed, since |client| is explicit and the
> two
> > are related?
> 
> +1

I disagree. IMO passing a task runner makes sense when instance of a class
should be used on a thread different from the one on which it is constructed. It
is implicit that non-thread-safe class must be created and used exclusively on
one thread.

That said, I do not feel strongly about this and am OK with adding it if you
guys do not agree with my argument above.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:99: FROM_HERE, base::Bind(&PipelineImpl::StopTask,
weak_this_, &waiter));
On 2016/05/02 20:58:35, xhwang wrote:
> On 2016/05/02 18:37:47, sandersd wrote:
> > I'm not a fan of this pattern; it was bad in WMPI and it's worse hidden
behind
> > an API. (Not that I have immediately implementable improvements to offer.)
> > 
> > At the very least, we should harden it against the |media_task_runner_|
being
> > stopped, and document that it synchronously calls to the media thread (since
> > this could cause deadlocks).
> > 
> > If there is no bug already for replacing it, we should file one
> > (Hotlist-CodeHealth).
> 
> We already have a waiter in WMPI when Pipeline::Stop() is called:
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed...
> 
> We don't like that pattern but it's kinda hard to remove (at least when I
looked
> at it last time). Therefore, it'd be nice if we could avoid adding another
> waiter.
> 
> Is there anything particular that you want to solve by using a sync Stop with
> waiter instead of the async Stop?

This is not a new waiter. The one in WMPI is simply moved here to make the
Pipeline API consistent from threading POV. I think it is better because earlier
stop_cb was the only callback called directly on the media thread. All the other
callbacks were called on the main thread. This is inconsistent from the client
POV.

I have added a TODO to get rid of this synchronous wait in a follow-up patch. I
think it should be easier now that all threading logic is localized in this
class.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:362: base::Unretained(client_)));
On 2016/05/02 20:58:35, xhwang wrote:
> Could you explain why base::Unretained is safe here and below? Is it possible
> that when this task is run, the whole media element and WMPI/Pipeline is
already
> destroyed?

It is not safe. The latest patch uses weak pointer.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:605: base::ResetAndReturn(&seek_cb_).Run(status_);
On 2016/05/02 18:37:47, sandersd wrote:
> Are we sure it's safe to drop these from the Stop() path? It's an overall
> improvement to the design, but I worry that other code may still be waiting
for
> these callbacks. (Maybe it doesn't matter anyway, since the resulting state is
> kStopped, but I've not checked every caller.)

It is not possible to call these since Stop is blocking. We should fix the
callers if they are waiting for these callbacks.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
File media/base/pipeline_impl.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.h:227: // Stops media rendering and signals the
|waiter| when done.
On 2016/05/02 18:37:47, sandersd wrote:
> Newline before comment.

Done.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.h:237: // Task runner used to execute pipeline tasks.
On 2016/05/02 18:37:47, sandersd wrote:
> Newline before comment.

Done.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/blink/web...
File media/blink/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/blink/web...
media/blink/webmediaplayer_impl.cc:254: // Pipeline must be stopped before it is
destroyed.
On 2016/05/02 18:37:47, sandersd wrote:
> Newline before comment.

Done.

[sandersd (OOO until July 31), 2016-05-02 22:45:52]

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:41: :
main_task_runner_(base::ThreadTaskRunnerHandle::Get()),
On 2016/05/02 22:25:17, alokp wrote:
> On 2016/05/02 20:58:35, xhwang wrote:
> > On 2016/05/02 18:37:47, sandersd wrote:
> > > Perhaps this should be explicitly passed, since |client| is explicit and
the
> > two
> > > are related?
> > 
> > +1
> 
> I disagree. IMO passing a task runner makes sense when instance of a class
> should be used on a thread different from the one on which it is constructed.
It
> is implicit that non-thread-safe class must be created and used exclusively on
> one thread.
> 
> That said, I do not feel strongly about this and am OK with adding it if you
> guys do not agree with my argument above.

This is reasonable, although I would feel better about it if the client was
passed to the constructor. As a general rule I prefer explicitness, but I'll let
xhwang@ make the call on how important that is.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:605: base::ResetAndReturn(&seek_cb_).Run(status_);
On 2016/05/02 22:25:17, alokp wrote:
> On 2016/05/02 18:37:47, sandersd wrote:
> > Are we sure it's safe to drop these from the Stop() path? It's an overall
> > improvement to the design, but I worry that other code may still be waiting
> for
> > these callbacks. (Maybe it doesn't matter anyway, since the resulting state
is
> > kStopped, but I've not checked every caller.)
> 
> It is not possible to call these since Stop is blocking. We should fix the
> callers if they are waiting for these callbacks.

That's fair, but we also can't commit this without doing that (or at least
scoping it) first.

https://chromiumcodereview.appspot.com/1904793002/diff/490001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/490001/media/base/pipe...
media/base/pipeline_impl.cc:100: media_task_runner_->PostTask(
At a minimum, DCHECK() the result of posting. (Better would be to signal the
event if the posted task is deleted, but that may take rather more code.)

[alokp, 2016-05-02 23:00:59]

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:41: :
main_task_runner_(base::ThreadTaskRunnerHandle::Get()),
On 2016/05/02 22:45:52, sandersd wrote:
> On 2016/05/02 22:25:17, alokp wrote:
> > On 2016/05/02 20:58:35, xhwang wrote:
> > > On 2016/05/02 18:37:47, sandersd wrote:
> > > > Perhaps this should be explicitly passed, since |client| is explicit and
> the
> > > two
> > > > are related?
> > > 
> > > +1
> > 
> > I disagree. IMO passing a task runner makes sense when instance of a class
> > should be used on a thread different from the one on which it is
constructed.
> It
> > is implicit that non-thread-safe class must be created and used exclusively
on
> > one thread.
> > 
> > That said, I do not feel strongly about this and am OK with adding it if you
> > guys do not agree with my argument above.
> 
> This is reasonable, although I would feel better about it if the client was
> passed to the constructor. As a general rule I prefer explicitness, but I'll
let
> xhwang@ make the call on how important that is.

I agree. In fact initial patches passed client in constructor but we decided
against it to keep Pipeline a pure abstract interface.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:605: base::ResetAndReturn(&seek_cb_).Run(status_);
On 2016/05/02 22:45:51, sandersd wrote:
> On 2016/05/02 22:25:17, alokp wrote:
> > On 2016/05/02 18:37:47, sandersd wrote:
> > > Are we sure it's safe to drop these from the Stop() path? It's an overall
> > > improvement to the design, but I worry that other code may still be
waiting
> > for
> > > these callbacks. (Maybe it doesn't matter anyway, since the resulting
state
> is
> > > kStopped, but I've not checked every caller.)
> > 
> > It is not possible to call these since Stop is blocking. We should fix the
> > callers if they are waiting for these callbacks.
> 
> That's fair, but we also can't commit this without doing that (or at least
> scoping it) first.

PipelineImpl::Seek/Suspend is only called from PipelineController, unittests for
which pass. Also I do not see PipelineController waiting for these
notifications.

https://chromiumcodereview.appspot.com/1904793002/diff/490001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/490001/media/base/pipe...
media/base/pipeline_impl.cc:100: media_task_runner_->PostTask(
On 2016/05/02 22:45:52, sandersd wrote:
> At a minimum, DCHECK() the result of posting. (Better would be to signal the
> event if the posted task is deleted, but that may take rather more code.)

Good idea. Added a CHECK.

[sandersd (OOO until July 31), 2016-05-02 23:20:47]

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:41: :
main_task_runner_(base::ThreadTaskRunnerHandle::Get()),
On 2016/05/02 23:00:59, alokp wrote:
> On 2016/05/02 22:45:52, sandersd wrote:
> > On 2016/05/02 22:25:17, alokp wrote:
> > > On 2016/05/02 20:58:35, xhwang wrote:
> > > > On 2016/05/02 18:37:47, sandersd wrote:
> > > > > Perhaps this should be explicitly passed, since |client| is explicit
and
> > the
> > > > two
> > > > > are related?
> > > > 
> > > > +1
> > > 
> > > I disagree. IMO passing a task runner makes sense when instance of a class
> > > should be used on a thread different from the one on which it is
> constructed.
> > It
> > > is implicit that non-thread-safe class must be created and used
exclusively
> on
> > > one thread.
> > > 
> > > That said, I do not feel strongly about this and am OK with adding it if
you
> > > guys do not agree with my argument above.
> > 
> > This is reasonable, although I would feel better about it if the client was
> > passed to the constructor. As a general rule I prefer explicitness, but I'll
> let
> > xhwang@ make the call on how important that is.
> 
> I agree. In fact initial patches passed client in constructor but we decided
> against it to keep Pipeline a pure abstract interface.

That's true, but the reason to do so was also removed (stashing the pointer in
the base class). We can still switch back without re-introducing that, if that
is better.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:605: base::ResetAndReturn(&seek_cb_).Run(status_);
On 2016/05/02 23:00:59, alokp wrote:
> On 2016/05/02 22:45:51, sandersd wrote:
> > On 2016/05/02 22:25:17, alokp wrote:
> > > On 2016/05/02 18:37:47, sandersd wrote:
> > > > Are we sure it's safe to drop these from the Stop() path? It's an
overall
> > > > improvement to the design, but I worry that other code may still be
> waiting
> > > for
> > > > these callbacks. (Maybe it doesn't matter anyway, since the resulting
> state
> > is
> > > > kStopped, but I've not checked every caller.)
> > > 
> > > It is not possible to call these since Stop is blocking. We should fix the
> > > callers if they are waiting for these callbacks.
> > 
> > That's fair, but we also can't commit this without doing that (or at least
> > scoping it) first.
> 
> PipelineImpl::Seek/Suspend is only called from PipelineController, unittests
for
> which pass. Also I do not see PipelineController waiting for these
> notifications.

PipelineController binds them all to OnPipelineStatus(), and does explicit error
handling there.

[alokp, 2016-05-03 05:22:18]

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:41: :
main_task_runner_(base::ThreadTaskRunnerHandle::Get()),
On 2016/05/02 23:20:47, sandersd wrote:
> On 2016/05/02 23:00:59, alokp wrote:
> > On 2016/05/02 22:45:52, sandersd wrote:
> > > On 2016/05/02 22:25:17, alokp wrote:
> > > > On 2016/05/02 20:58:35, xhwang wrote:
> > > > > On 2016/05/02 18:37:47, sandersd wrote:
> > > > > > Perhaps this should be explicitly passed, since |client| is explicit
> and
> > > the
> > > > > two
> > > > > > are related?
> > > > > 
> > > > > +1
> > > > 
> > > > I disagree. IMO passing a task runner makes sense when instance of a
class
> > > > should be used on a thread different from the one on which it is
> > constructed.
> > > It
> > > > is implicit that non-thread-safe class must be created and used
> exclusively
> > on
> > > > one thread.
> > > > 
> > > > That said, I do not feel strongly about this and am OK with adding it if
> you
> > > > guys do not agree with my argument above.
> > > 
> > > This is reasonable, although I would feel better about it if the client
was
> > > passed to the constructor. As a general rule I prefer explicitness, but
I'll
> > let
> > > xhwang@ make the call on how important that is.
> > 
> > I agree. In fact initial patches passed client in constructor but we decided
> > against it to keep Pipeline a pure abstract interface.
> 
> That's true, but the reason to do so was also removed (stashing the pointer in
> the base class). We can still switch back without re-introducing that, if that
> is better.

How do you propose doing that? It will be weird to talk about Client in the base
class without an API on the base class to pass it.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:605: base::ResetAndReturn(&seek_cb_).Run(status_);
On 2016/05/02 23:20:47, sandersd wrote:
> On 2016/05/02 23:00:59, alokp wrote:
> > On 2016/05/02 22:45:51, sandersd wrote:
> > > On 2016/05/02 22:25:17, alokp wrote:
> > > > On 2016/05/02 18:37:47, sandersd wrote:
> > > > > Are we sure it's safe to drop these from the Stop() path? It's an
> overall
> > > > > improvement to the design, but I worry that other code may still be
> > waiting
> > > > for
> > > > > these callbacks. (Maybe it doesn't matter anyway, since the resulting
> > state
> > > is
> > > > > kStopped, but I've not checked every caller.)
> > > > 
> > > > It is not possible to call these since Stop is blocking. We should fix
the
> > > > callers if they are waiting for these callbacks.
> > > 
> > > That's fair, but we also can't commit this without doing that (or at least
> > > scoping it) first.
> > 
> > PipelineImpl::Seek/Suspend is only called from PipelineController, unittests
> for
> > which pass. Also I do not see PipelineController waiting for these
> > notifications.
> 
> PipelineController binds them all to OnPipelineStatus(), and does explicit
error
> handling there.

Right. I am sorry I do not see a problem there. Can you elaborate?

PipelineController::OnPipelineStatus will not be called *only* if WMPI stops the
Pipeline before it has a chance to call the seek/resume callback. Is that a
problem?

BTW I find it confusing to control the pipeline from two places - WMPI and
PipelineController. Do we really need PipelineController?

[sandersd (OOO until July 31), 2016-05-03 19:28:55]

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:41: :
main_task_runner_(base::ThreadTaskRunnerHandle::Get()),
On 2016/05/03 05:22:18, alokp wrote:
> On 2016/05/02 23:20:47, sandersd wrote:
> > On 2016/05/02 23:00:59, alokp wrote:
> > > On 2016/05/02 22:45:52, sandersd wrote:
> > > > On 2016/05/02 22:25:17, alokp wrote:
> > > > > On 2016/05/02 20:58:35, xhwang wrote:
> > > > > > On 2016/05/02 18:37:47, sandersd wrote:
> > > > > > > Perhaps this should be explicitly passed, since |client| is
explicit
> > and
> > > > the
> > > > > > two
> > > > > > > are related?
> > > > > > 
> > > > > > +1
> > > > > 
> > > > > I disagree. IMO passing a task runner makes sense when instance of a
> class
> > > > > should be used on a thread different from the one on which it is
> > > constructed.
> > > > It
> > > > > is implicit that non-thread-safe class must be created and used
> > exclusively
> > > on
> > > > > one thread.
> > > > > 
> > > > > That said, I do not feel strongly about this and am OK with adding it
if
> > you
> > > > > guys do not agree with my argument above.
> > > > 
> > > > This is reasonable, although I would feel better about it if the client
> was
> > > > passed to the constructor. As a general rule I prefer explicitness, but
> I'll
> > > let
> > > > xhwang@ make the call on how important that is.
> > > 
> > > I agree. In fact initial patches passed client in constructor but we
decided
> > > against it to keep Pipeline a pure abstract interface.
> > 
> > That's true, but the reason to do so was also removed (stashing the pointer
in
> > the base class). We can still switch back without re-introducing that, if
that
> > is better.
> 
> How do you propose doing that? It will be weird to talk about Client in the
base
> class without an API on the base class to pass it.

A fair point, I'll accept that.

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:605: base::ResetAndReturn(&seek_cb_).Run(status_);
On 2016/05/03 05:22:18, alokp wrote:
> On 2016/05/02 23:20:47, sandersd wrote:
> > On 2016/05/02 23:00:59, alokp wrote:
> > > On 2016/05/02 22:45:51, sandersd wrote:
> > > > On 2016/05/02 22:25:17, alokp wrote:
> > > > > On 2016/05/02 18:37:47, sandersd wrote:
> > > > > > Are we sure it's safe to drop these from the Stop() path? It's an
> > overall
> > > > > > improvement to the design, but I worry that other code may still be
> > > waiting
> > > > > for
> > > > > > these callbacks. (Maybe it doesn't matter anyway, since the
resulting
> > > state
> > > > is
> > > > > > kStopped, but I've not checked every caller.)
> > > > > 
> > > > > It is not possible to call these since Stop is blocking. We should fix
> the
> > > > > callers if they are waiting for these callbacks.
> > > > 
> > > > That's fair, but we also can't commit this without doing that (or at
least
> > > > scoping it) first.
> > > 
> > > PipelineImpl::Seek/Suspend is only called from PipelineController,
unittests
> > for
> > > which pass. Also I do not see PipelineController waiting for these
> > > notifications.
> > 
> > PipelineController binds them all to OnPipelineStatus(), and does explicit
> error
> > handling there.
> 
> Right. I am sorry I do not see a problem there. Can you elaborate?
> 
> PipelineController::OnPipelineStatus will not be called *only* if WMPI stops
the
> Pipeline before it has a chance to call the seek/resume callback. Is that a
> problem?
> 
> BTW I find it confusing to control the pipeline from two places - WMPI and
> PipelineController. Do we really need PipelineController?

I'm not sure if it's a problem, it's probably not, but it's not trivial to
check.

The actual plan is to get rid of everywhere that WMPI controls the Pipeline, and
then likely merge PipelineController into Pipeline. I would say to think of it
as an in-progress refactor of the Pipeline API.

[alokp, 2016-05-03 20:15:23]

xhwang: Can you please review. Thanks!

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:41: :
main_task_runner_(base::ThreadTaskRunnerHandle::Get()),
On 2016/05/03 19:28:55, sandersd wrote:
> On 2016/05/03 05:22:18, alokp wrote:
> > On 2016/05/02 23:20:47, sandersd wrote:
> > > On 2016/05/02 23:00:59, alokp wrote:
> > > > On 2016/05/02 22:45:52, sandersd wrote:
> > > > > On 2016/05/02 22:25:17, alokp wrote:
> > > > > > On 2016/05/02 20:58:35, xhwang wrote:
> > > > > > > On 2016/05/02 18:37:47, sandersd wrote:
> > > > > > > > Perhaps this should be explicitly passed, since |client| is
> explicit
> > > and
> > > > > the
> > > > > > > two
> > > > > > > > are related?
> > > > > > > 
> > > > > > > +1
> > > > > > 
> > > > > > I disagree. IMO passing a task runner makes sense when instance of a
> > class
> > > > > > should be used on a thread different from the one on which it is
> > > > constructed.
> > > > > It
> > > > > > is implicit that non-thread-safe class must be created and used
> > > exclusively
> > > > on
> > > > > > one thread.
> > > > > > 
> > > > > > That said, I do not feel strongly about this and am OK with adding
it
> if
> > > you
> > > > > > guys do not agree with my argument above.
> > > > > 
> > > > > This is reasonable, although I would feel better about it if the
client
> > was
> > > > > passed to the constructor. As a general rule I prefer explicitness,
but
> > I'll
> > > > let
> > > > > xhwang@ make the call on how important that is.
> > > > 
> > > > I agree. In fact initial patches passed client in constructor but we
> decided
> > > > against it to keep Pipeline a pure abstract interface.
> > > 
> > > That's true, but the reason to do so was also removed (stashing the
pointer
> in
> > > the base class). We can still switch back without re-introducing that, if
> that
> > > is better.
> > 
> > How do you propose doing that? It will be weird to talk about Client in the
> base
> > class without an API on the base class to pass it.
> 
> A fair point, I'll accept that.

One way to add Client in the base class interface without passing it in the
constructor would be a static Create function. Would you prefer that?

https://chromiumcodereview.appspot.com/1904793002/diff/430001/media/base/pipe...
media/base/pipeline_impl.cc:605: base::ResetAndReturn(&seek_cb_).Run(status_);
On 2016/05/03 19:28:55, sandersd wrote:
> On 2016/05/03 05:22:18, alokp wrote:
> > On 2016/05/02 23:20:47, sandersd wrote:
> > > On 2016/05/02 23:00:59, alokp wrote:
> > > > On 2016/05/02 22:45:51, sandersd wrote:
> > > > > On 2016/05/02 22:25:17, alokp wrote:
> > > > > > On 2016/05/02 18:37:47, sandersd wrote:
> > > > > > > Are we sure it's safe to drop these from the Stop() path? It's an
> > > overall
> > > > > > > improvement to the design, but I worry that other code may still
be
> > > > waiting
> > > > > > for
> > > > > > > these callbacks. (Maybe it doesn't matter anyway, since the
> resulting
> > > > state
> > > > > is
> > > > > > > kStopped, but I've not checked every caller.)
> > > > > > 
> > > > > > It is not possible to call these since Stop is blocking. We should
fix
> > the
> > > > > > callers if they are waiting for these callbacks.
> > > > > 
> > > > > That's fair, but we also can't commit this without doing that (or at
> least
> > > > > scoping it) first.
> > > > 
> > > > PipelineImpl::Seek/Suspend is only called from PipelineController,
> unittests
> > > for
> > > > which pass. Also I do not see PipelineController waiting for these
> > > > notifications.
> > > 
> > > PipelineController binds them all to OnPipelineStatus(), and does explicit
> > error
> > > handling there.
> > 
> > Right. I am sorry I do not see a problem there. Can you elaborate?
> > 
> > PipelineController::OnPipelineStatus will not be called *only* if WMPI stops
> the
> > Pipeline before it has a chance to call the seek/resume callback. Is that a
> > problem?
> > 
> > BTW I find it confusing to control the pipeline from two places - WMPI and
> > PipelineController. Do we really need PipelineController?
> 
> I'm not sure if it's a problem, it's probably not, but it's not trivial to
> check.
> 
> The actual plan is to get rid of everywhere that WMPI controls the Pipeline,
and
> then likely merge PipelineController into Pipeline. I would say to think of it
> as an in-progress refactor of the Pipeline API.

SGTM

[alokp, 2016-05-03 20:15:24]

xhwang: Can you please review. Thanks!

[sandersd (OOO until July 31), 2016-05-03 21:51:43]

Overall looking pretty good.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.cc:554: client_weak_factory_->GetWeakPtr()));
It is not safe to vend weak pointers from the same factory on multiple threads.
(In fact the key reason that we invalidate in Stop() is to be able to vend and
deref on separate threads, but we also never call GetWeakPtr on the media
thread.)

[alokp, 2016-05-03 23:00:35]

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.cc:554: client_weak_factory_->GetWeakPtr()));
On 2016/05/03 21:51:43, sandersd wrote:
> It is not safe to vend weak pointers from the same factory on multiple
threads.
> (In fact the key reason that we invalidate in Stop() is to be able to vend and
> deref on separate threads, but we also never call GetWeakPtr on the media
> thread.)

This factory is for the Client, not PipelineImpl. Vending happens exclusively on
the media thread. Deref/Invalidation happens on the main thread.

[sandersd (OOO until July 31), 2016-05-03 23:03:39]

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.cc:554: client_weak_factory_->GetWeakPtr()));
On 2016/05/03 23:00:35, alokp wrote:
> On 2016/05/03 21:51:43, sandersd wrote:
> > It is not safe to vend weak pointers from the same factory on multiple
> threads.
> > (In fact the key reason that we invalidate in Stop() is to be able to vend
and
> > deref on separate threads, but we also never call GetWeakPtr on the media
> > thread.)
> 
> This factory is for the Client, not PipelineImpl. Vending happens exclusively
on
> the media thread. Deref/Invalidation happens on the main thread.

SetDuration() is not on the media thread.

[sandersd (OOO until July 31), 2016-05-03 23:05:25]

On 2016/05/03 23:03:39, sandersd wrote:
>
https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
> File media/base/pipeline_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
> media/base/pipeline_impl.cc:554: client_weak_factory_->GetWeakPtr()));
> On 2016/05/03 23:00:35, alokp wrote:
> > On 2016/05/03 21:51:43, sandersd wrote:
> > > It is not safe to vend weak pointers from the same factory on multiple
> > threads.
> > > (In fact the key reason that we invalidate in Stop() is to be able to vend
> and
> > > deref on separate threads, but we also never call GetWeakPtr on the media
> > > thread.)
> > 
> > This factory is for the Client, not PipelineImpl. Vending happens
exclusively
> on
> > the media thread. Deref/Invalidation happens on the main thread.
> 
> SetDuration() is not on the media thread.

Actually, you are right, SetDuration() is a DemuxerHost interface method.

Sorry!

[sandersd (OOO until July 31), 2016-05-03 23:05:37]

lgtm

[xhwang, 2016-05-04 00:13:51]

On 2016/05/03 20:15:24, alokp wrote:
> xhwang: Can you please review. Thanks!

Sorry for the delay. I'll finish the review tonight.

[xhwang, 2016-05-04 06:32:20]

xhwang@chromium.org changed reviewers:
+ wez@chromium.org

[xhwang, 2016-05-04 06:32:22]

Add wez@ about a question on WeakPtrFactory.

Otherwise mostly nits.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/mock...
File media/base/mock_filters.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/mock...
media/base/mock_filters.h:57: // take scoped_ptr* instead of scoped_ptr so that
they can be mock methods.
nit: s/scoped_ptr/unique_ptr

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline.h:33: // through another callback.
What do you mean by "reported already through another callback"?

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline.h:51: // Executed whenever a text track is added.
When should |done_cb| be fired?

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline.h:73: //
These are calls on the Client interface, not really callbacks. Also, since this
is the list of all methods, it's probably clearer to just say "methods on Client
interface may be called up until Stop() has completed".

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline.h:75: // TODO(xhwang): Rename |seek_cb| to |start_cb|.
Since Start/Seek/Resume all uses seek_cb, after a second thought, I think
seek_cb still makes some sense.

Could you please drop this TODO for me? Thanks! Sorry for being back and forth.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline.h:82: // The pipeline must be stopped before destroying it.
... "if the pipeline is started" according to the comment on IsRunning(). Either
way, we should be consistent.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline.h:87: // for Start/Seek/Suspend/Resume or PipelineClient
callbacks will *not*
s/PipelineClient/Client

Since Client methods are not callbacks, s/Client callbacks/Client methods/?

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.cc:81: client_weak_factory_.reset(new
base::WeakPtrFactory<Client>(client));
wez: This is a bit new to me and it seems working well. But the documentation
only recommended "primitive types" for this kind of use. Do you see any issues?
If not, shall we update the documentation? 

https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

Ulitmately I think we want to split this class into two different classes, one
lives on the main thread and one lives on the media thread, to avoid the
complexity here.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.cc:618: // log this when |error_cb_| is called. If the
pipeline is already stopped or
There's no |error_cb_| now. Please update this comment.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
File media/base/pipeline_impl.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.h:298: // Temporary callback used for Stop().
This is really for StopTask() now. 

Actually it seems we can just pass the |stop_cb| to DoStop(), in which case we
don't need to store this as a class member, which seems cleaner to me.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.h:313: std::unique_ptr<base::WeakPtrFactory<Client>>
client_weak_factory_;
Can you add comments about the threading expectation on this factory?

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/blink/web...
File media/blink/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/blink/web...
media/blink/webmediaplayer_impl.cc:255: // Pipeline must be stopped before it is
destroyed.
nit: Add a note that this is a blocking call.

[Wez, 2016-05-04 15:56:26]

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.cc:81: client_weak_factory_.reset(new
base::WeakPtrFactory<Client>(client));
On 2016/05/04 06:32:22, xhwang wrote:
> wez: This is a bit new to me and it seems working well. But the documentation
> only recommended "primitive types" for this kind of use. Do you see any
issues?
> If not, shall we update the documentation? 
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...
> 
> Ulitmately I think we want to split this class into two different classes, one
> lives on the main thread and one lives on the media thread, to avoid the
> complexity here.

Agreed re there being too much complexity due to having the object spanning
threads!

This is a perfectly valid use of WeakPtrFactory(). However, I'd strongly
recommend that you take a weak_client_ WeakPtr here, rather than calling
GetWeakPtr() at each Bind(), for both thread-safety and efficiency.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.cc:101: FROM_HERE, base::Bind(&PipelineImpl::StopTask,
weak_this_, stop_cb)));
What is your intent in CHECKing the result of PostTask() here?

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.cc:554: client_weak_factory_->GetWeakPtr()));
On 2016/05/03 23:03:39, sandersd wrote:
> On 2016/05/03 23:00:35, alokp wrote:
> > On 2016/05/03 21:51:43, sandersd wrote:
> > > It is not safe to vend weak pointers from the same factory on multiple
> > threads.
> > > (In fact the key reason that we invalidate in Stop() is to be able to vend
> and
> > > deref on separate threads, but we also never call GetWeakPtr on the media
> > > thread.)
> > 
> > This factory is for the Client, not PipelineImpl. Vending happens
exclusively
> on
> > the media thread. Deref/Invalidation happens on the main thread.
> 
> SetDuration() is not on the media thread.

Looks like SetDuration() should be, but isn't currently in all impls, so yes,
that is potentially dangerous. Even if it weren't, it's more obviously correct,
and slightly more efficient, to create a WeakPtr once and Bind() it into
multiple calls than to keep creating new ones.

[alokp, 2016-05-04 17:40:01]

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/mock...
File media/base/mock_filters.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/mock...
media/base/mock_filters.h:57: // take scoped_ptr* instead of scoped_ptr so that
they can be mock methods.
On 2016/05/04 06:32:22, xhwang wrote:
> nit: s/scoped_ptr/unique_ptr

Done.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline.h:33: // through another callback.
On 2016/05/04 06:32:22, xhwang wrote:
> What do you mean by "reported already through another callback"?

clarified.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline.h:51: // Executed whenever a text track is added.
On 2016/05/04 06:32:22, xhwang wrote:
> When should |done_cb| be fired?

Added comment to the best of my knowledge. The old code did not have comment
about this.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline.h:73: //
On 2016/05/04 06:32:22, xhwang wrote:
> These are calls on the Client interface, not really callbacks. Also, since
this
> is the list of all methods, it's probably clearer to just say "methods on
Client
> interface may be called up until Stop() has completed".

Done.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline.h:75: // TODO(xhwang): Rename |seek_cb| to |start_cb|.
On 2016/05/04 06:32:22, xhwang wrote:
> Since Start/Seek/Resume all uses seek_cb, after a second thought, I think
> seek_cb still makes some sense.
> 
> Could you please drop this TODO for me? Thanks! Sorry for being back and
forth.

Done.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline.h:82: // The pipeline must be stopped before destroying it.
On 2016/05/04 06:32:22, xhwang wrote:
> ... "if the pipeline is started" according to the comment on IsRunning().
Either
> way, we should be consistent.

Done.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline.h:87: // for Start/Seek/Suspend/Resume or PipelineClient
callbacks will *not*
On 2016/05/04 06:32:22, xhwang wrote:
> s/PipelineClient/Client
> 
> Since Client methods are not callbacks, s/Client callbacks/Client methods/?

Done.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.cc:81: client_weak_factory_.reset(new
base::WeakPtrFactory<Client>(client));
On 2016/05/04 15:56:26, Wez wrote:
> On 2016/05/04 06:32:22, xhwang wrote:
> > wez: This is a bit new to me and it seems working well. But the
documentation
> > only recommended "primitive types" for this kind of use. Do you see any
> issues?
> > If not, shall we update the documentation? 
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...
> > 
> > Ulitmately I think we want to split this class into two different classes,
one
> > lives on the main thread and one lives on the media thread, to avoid the
> > complexity here.
> 
> Agreed re there being too much complexity due to having the object spanning
> threads!
> 
> This is a perfectly valid use of WeakPtrFactory(). However, I'd strongly
> recommend that you take a weak_client_ WeakPtr here, rather than calling
> GetWeakPtr() at each Bind(), for both thread-safety and efficiency.

Agreed about thread-safety, so created a weak_client_. For my knowledge, can you
explain why calling GetWeakPtr is less efficient than the copy constructor for
WeakPtr? On the surface they look equivalent.

I already have a TODO in Stop for splitting the class along thread lines.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.cc:101: FROM_HERE, base::Bind(&PipelineImpl::StopTask,
weak_this_, stop_cb)));
On 2016/05/04 15:56:26, Wez wrote:
> What is your intent in CHECKing the result of PostTask() here?

Clarified in comment.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.cc:554: client_weak_factory_->GetWeakPtr()));
On 2016/05/04 15:56:26, Wez wrote:
> On 2016/05/03 23:03:39, sandersd wrote:
> > On 2016/05/03 23:00:35, alokp wrote:
> > > On 2016/05/03 21:51:43, sandersd wrote:
> > > > It is not safe to vend weak pointers from the same factory on multiple
> > > threads.
> > > > (In fact the key reason that we invalidate in Stop() is to be able to
vend
> > and
> > > > deref on separate threads, but we also never call GetWeakPtr on the
media
> > > > thread.)
> > > 
> > > This factory is for the Client, not PipelineImpl. Vending happens
> exclusively
> > on
> > > the media thread. Deref/Invalidation happens on the main thread.
> > 
> > SetDuration() is not on the media thread.
> 
> Looks like SetDuration() should be, but isn't currently in all impls, so yes,
> that is potentially dangerous. Even if it weren't, it's more obviously
correct,
> and slightly more efficient, to create a WeakPtr once and Bind() it into
> multiple calls than to keep creating new ones.

Done.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.cc:618: // log this when |error_cb_| is called. If the
pipeline is already stopped or
On 2016/05/04 06:32:22, xhwang wrote:
> There's no |error_cb_| now. Please update this comment.

Done.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
File media/base/pipeline_impl.h (right):

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.h:298: // Temporary callback used for Stop().
On 2016/05/04 06:32:22, xhwang wrote:
> This is really for StopTask() now. 
> 
> Actually it seems we can just pass the |stop_cb| to DoStop(), in which case we
> don't need to store this as a class member, which seems cleaner to me.

In StopTask, we skip calling DoStop if the pipeline is already in the kStopping
state. In that case we simply store the stop_cb as a member variable for DoStop
to call later. I think this is from the days when we used to post tasks for
DoStop and OnStopCompleted. We no longer need the kStopping state. I have
already combined OnStopCompleted into DoStop. We can further combine StopTask
and DoStop and get rid of kStopping state. I would prefer to make this change in
a follow-up patch - perhaps when we split this class along threads.

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/base/pipe...
media/base/pipeline_impl.h:313: std::unique_ptr<base::WeakPtrFactory<Client>>
client_weak_factory_;
On 2016/05/04 06:32:22, xhwang wrote:
> Can you add comments about the threading expectation on this factory?

Added comments and also created weak_client_ as suggested by wez@

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/blink/web...
File media/blink/webmediaplayer_impl.cc (right):

https://chromiumcodereview.appspot.com/1904793002/diff/510001/media/blink/web...
media/blink/webmediaplayer_impl.cc:255: // Pipeline must be stopped before it is
destroyed.
On 2016/05/04 06:32:22, xhwang wrote:
> nit: Add a note that this is a blocking call.

IMO this note would be redundant and confusing. If the function does not take
any callback arguments, it is implicit that nothing special needs to be done.
Also, I plan to split PipelineImpl along thread line and DeleteSoon the class on
media-thread. In that case, the blocking comment is not really accurate.

[Wez, 2016-05-04 20:45:21]

Re GetWeakPtr(), it is permissible for the implementation to reconstruct
the internal Flag when the number of WeakPtrs drops to zero. That's a quirk
that should probably be removed, though.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[alokp, 2016-05-05 13:17:39]

xhwang: friendly ping :)

[xhwang, 2016-05-05 16:25:07]

LGTM, thanks!

[alokp, 2016-05-05 16:29:27]

The CQ bit was checked by alokp@chromium.org

[alokp, 2016-05-05 16:29:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1904793002/#ps550001
(title: "rebase")

[commit-bot: I haz the power, 2016-05-05 16:29:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1904793002/550001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1904793002/550001

[commit-bot: I haz the power, 2016-05-05 23:03:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-05 23:03:37]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alokp, 2016-05-06 04:18:19]

The CQ bit was checked by alokp@chromium.org

[alokp, 2016-05-06 04:18:20]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/1904793002/#ps590001
(title: "post stop callback")

[commit-bot: I haz the power, 2016-05-06 04:18:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1904793002/590001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1904793002/590001

[commit-bot: I haz the power, 2016-05-06 05:21:48]

Description was changed from

==========
Move Pipeline permanent callbacks into Pipeline::Client interface.

We have already reach the gmock limit of 10 arguments for
Pipeline::Start function. So we need to move the arguments into a
container to be able to add more callbacks.

BUG=571155
==========

to

==========
Move Pipeline permanent callbacks into Pipeline::Client interface.

We have already reach the gmock limit of 10 arguments for
Pipeline::Start function. So we need to move the arguments into a
container to be able to add more callbacks.

BUG=571155
==========

[commit-bot: I haz the power, 2016-05-06 05:21:49]

Committed patchset #31 (id:590001)

[commit-bot: I haz the power, 2016-05-06 05:23:05]

Description was changed from

==========
Move Pipeline permanent callbacks into Pipeline::Client interface.

We have already reach the gmock limit of 10 arguments for
Pipeline::Start function. So we need to move the arguments into a
container to be able to add more callbacks.

BUG=571155
==========

to

==========
Move Pipeline permanent callbacks into Pipeline::Client interface.

We have already reach the gmock limit of 10 arguments for
Pipeline::Start function. So we need to move the arguments into a
container to be able to add more callbacks.

BUG=571155

Committed: https://crrev.com/967c902454f4a36e0d0e44d686dc5b76c1467255
Cr-Commit-Position: refs/heads/master@{#392005}
==========

[commit-bot: I haz the power, 2016-05-06 05:23:06]

Patchset 31 (id:??) landed as
https://crrev.com/967c902454f4a36e0d0e44d686dc5b76c1467255
Cr-Commit-Position: refs/heads/master@{#392005}