Splits PipelineImpl into main and media thread components.

Splits PipelineImpl into main and media thread components.

The outer PipelineImpl runs on the main thread and acts as a proxy
between PipelineClient and the internal RendererWrapper that runs on
the media thread. This sets the stage to get rid of WaitableEvent
and Locks used in PipelineImpl.

This patch should not change anything functionality wise except for
timing of the notifications received from Demuxer and Renderer. In
response to the notifications we used to change member variables
inside a lock. Now we post a task to change those variables to the
main thread. The list of variables affected by this change are:
- buffered_time_ranges_
- did_loading_progress_
- duration_
- statistics_

BUG=616959
Committed: https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662
Cr-Commit-Position: refs/heads/master@{#399376}

[alokp, 2016-06-03 00:03:35]

Description was changed from

==========
PipelineImpl refactor.
==========

to

==========
Splits PipelineImpl into main and media thread components.

The outer PipelineImpl runs on the main thread and acts as a proxy
between PipelineClient and the internal RendererWrapper that runs on
the media thread.

BUG=616959
==========

[alokp, 2016-06-03 00:04:03]

Description was changed from

==========
Splits PipelineImpl into main and media thread components.

The outer PipelineImpl runs on the main thread and acts as a proxy
between PipelineClient and the internal RendererWrapper that runs on
the media thread.

BUG=616959
==========

to

==========
Splits PipelineImpl into main and media thread components.

The outer PipelineImpl runs on the main thread and acts as a proxy
between PipelineClient and the internal RendererWrapper that runs on
the media thread.

BUG=616959
==========

[alokp, 2016-06-03 20:58:57]

Description was changed from

==========
Splits PipelineImpl into main and media thread components.

The outer PipelineImpl runs on the main thread and acts as a proxy
between PipelineClient and the internal RendererWrapper that runs on
the media thread.

BUG=616959
==========

to

==========
Splits PipelineImpl into main and media thread components.

The outer PipelineImpl runs on the main thread and acts as a proxy
between PipelineClient and the internal RendererWrapper that runs on
the media thread.

This patch should not change anything functionality wise. It just sets
the stage to get rid of WaitableEvent and Locks used in PipelineImpl.

BUG=616959
==========

[alokp, 2016-06-03 21:00:03]

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

[xhwang, 2016-06-03 21:08:22]

Description was changed from

==========
Splits PipelineImpl into main and media thread components.

The outer PipelineImpl runs on the main thread and acts as a proxy
between PipelineClient and the internal RendererWrapper that runs on
the media thread.

This patch should not change anything functionality wise. It just sets
the stage to get rid of WaitableEvent and Locks used in PipelineImpl.

BUG=616959
==========

to

==========
Splits PipelineImpl into main and media thread components.

The outer PipelineImpl runs on the main thread and acts as a proxy
between PipelineClient and the internal RendererWrapper that runs on
the media thread.

This patch should not change anything functionality wise. It just sets
the stage to get rid of WaitableEvent and Locks used in PipelineImpl.

BUG=616959
==========

[xhwang, 2016-06-03 21:08:22]

xhwang@chromium.org changed required reviewers:
+ sandersd@chromium.org

[xhwang, 2016-06-03 21:09:11]

Assign Dan as the main reviewer as I am busy looking at other CLs. I'll take a
high level review early next week if that timing is okay.

[sandersd (OOO until July 31), 2016-06-03 22:39:13]

It's going to take a day or two to finish this review.

Until then, I also wanted to work out what the future refactoring plans are.
After this CL, we have:
  - PipelineController, exposing the desired interface to Pipeline (has some
state machine parts).
  - Pipeline (abstract interface), exposed for testing but also called directly
by WMPI in a few places.
  - PipelineImpl, a wrapper around RendererWrapper to hop between threads. (And
instantiated directly by some tests.)
  - RendererWrapper (private to PipelineImpl), a wrapper around a Renderer that
implements the Pipeline state machine.

The original refactoring plans called for the Pipeline interface to remain,
mostly so that the current tests keep working, but from the above list it looks
more like the internal tests should be against RendererWrapper and
PipelineController/PipelineImpl should be merged. What's your take?

[sandersd (OOO until July 31), 2016-06-03 22:59:35]

On 2016/06/03 22:39:13, sandersd wrote:
> It's going to take a day or two to finish this review.
> 
> Until then, I also wanted to work out what the future refactoring plans are.
> After this CL, we have:
>   - PipelineController, exposing the desired interface to Pipeline (has some
> state machine parts).
>   - Pipeline (abstract interface), exposed for testing but also called
directly
> by WMPI in a few places.
>   - PipelineImpl, a wrapper around RendererWrapper to hop between threads.
(And
> instantiated directly by some tests.)
>   - RendererWrapper (private to PipelineImpl), a wrapper around a Renderer
that
> implements the Pipeline state machine.
> 
> The original refactoring plans called for the Pipeline interface to remain,
> mostly so that the current tests keep working, but from the above list it
looks
> more like the internal tests should be against RendererWrapper and
> PipelineController/PipelineImpl should be merged. What's your take?

I should note that we can achieve both, if RendererWrapper becomes a public.
Then we can move PipelineImpl into PipelineController as a private class, if we
so desire. They don't actually have to be merged and the tests can stay
unmodified (hopefully temporarily) to achieve the desired interface.

[sandersd (OOO until July 31), 2016-06-03 23:11:45]

(Covers everything except the changes in pipeline_impl.*)

https://codereview.chromium.org/1999893004/diff/400001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (left):

https://codereview.chromium.org/1999893004/diff/400001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:244: if (data_source_)
I don't see any change in PipelineImpl related to these deletions, but I'm also
not sure how Pipeline gets blocked. What's the reasoning here?

https://codereview.chromium.org/1999893004/diff/400001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1999893004/diff/400001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:241: suppress_destruction_errors_ = true;
Probably group this with setWebLayer().

[alokp, 2016-06-03 23:57:49]

I have designed RendererWrapper as an internal class of Pipeline. I do not see
much value in making it public. Pipeline presents a much better interface to
test.

I do not know much about PipelineController so I would let you make the call
about the future of Pipeline and PipelineController. From the high level both
seem like convenience wrappers for Renderer, so I would think that merging them
would be a good idea.

https://codereview.chromium.org/1999893004/diff/400001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (left):

https://codereview.chromium.org/1999893004/diff/400001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:244: if (data_source_)
On 2016/06/03 23:11:45, sandersd wrote:
> I don't see any change in PipelineImpl related to these deletions, but I'm
also
> not sure how Pipeline gets blocked. What's the reasoning here?

I can move these changes to a separate patch if you like. None of this code is
required. Pipeline::Stop stops the Demuxer, which in turn stops the data source
in correct order.

I also discovered that both - Demuxer and DataSource - have duplicate functions
named Stop, Abort, and Shutdown that do the same thing. We might be able to
remove some duplication there.

Pipeline::Stop blocks until the renderer is destroyed and demuxer is stopped on
the media thread using a WaitableEvent. We might be able to get rid of this
WaitableEvent if we moved the ownership of Demuxer and DataSource to
RendererWrapper. That would allow us to just post a DeleteSoon task for
RendererWrapper on the media thread. See my comment in Pipeline::Stop.

https://codereview.chromium.org/1999893004/diff/400001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1999893004/diff/400001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:241: suppress_destruction_errors_ = true;
On 2016/06/03 23:11:45, sandersd wrote:
> Probably group this with setWebLayer().

Done.

[sandersd (OOO until July 31), 2016-06-08 00:01:53]

On 2016/06/03 23:57:49, alokp wrote:
> I have designed RendererWrapper as an internal class of Pipeline. I do not see
> much value in making it public. Pipeline presents a much better interface to
> test.
> 
> I do not know much about PipelineController so I would let you make the call
> about the future of Pipeline and PipelineController. From the high level both
> seem like convenience wrappers for Renderer, so I would think that merging
them
> would be a good idea.
> 
>
https://codereview.chromium.org/1999893004/diff/400001/media/blink/webmediapl...
> File media/blink/webmediaplayer_impl.cc (left):
> 
>
https://codereview.chromium.org/1999893004/diff/400001/media/blink/webmediapl...
> media/blink/webmediaplayer_impl.cc:244: if (data_source_)
> On 2016/06/03 23:11:45, sandersd wrote:
> > I don't see any change in PipelineImpl related to these deletions, but I'm
> also
> > not sure how Pipeline gets blocked. What's the reasoning here?
> 
> I can move these changes to a separate patch if you like. None of this code is
> required. Pipeline::Stop stops the Demuxer, which in turn stops the data
source
> in correct order.
> 
> I also discovered that both - Demuxer and DataSource - have duplicate
functions
> named Stop, Abort, and Shutdown that do the same thing. We might be able to
> remove some duplication there.
> 
> Pipeline::Stop blocks until the renderer is destroyed and demuxer is stopped
on
> the media thread using a WaitableEvent. We might be able to get rid of this
> WaitableEvent if we moved the ownership of Demuxer and DataSource to
> RendererWrapper. That would allow us to just post a DeleteSoon task for
> RendererWrapper on the media thread. See my comment in Pipeline::Stop.
> 
>
https://codereview.chromium.org/1999893004/diff/400001/media/blink/webmediapl...
> File media/blink/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/1999893004/diff/400001/media/blink/webmediapl...
> media/blink/webmediaplayer_impl.cc:241: suppress_destruction_errors_ = true;
> On 2016/06/03 23:11:45, sandersd wrote:
> > Probably group this with setWebLayer().
> 
> Done.

I spent a lot of time trying to munge this CL into a diffable state, but I have
not been entirely successful. The set of changes it too large to be able to
reliably review.

Here are some ideas to make the review more manageable (in importance order):
  - Provide a list of the changes, detailed enough that each method can be
reviewed for correctness in isolation.
  - Move some of the refactorings from this CL to a followup. (Possible
examples: don't rewrite the state machine, don't rename or re-order methods,
don't split out helpers, don't change function prologues unnecessarily.)
  - Split RendererWrapper into its own file.

Any ideas you have to make it easier to review would be great!

[alokp, 2016-06-08 19:59:49]

Dan: Sorry for the large patch. I have rearranged the code in pipeline_impl.cc
so that diffs line up better. I also added some comments to explain the changes.
Please see if this is any better.

https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
File media/base/pipeline_impl.cc (left):

https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
media/base/pipeline_impl.cc:44: running_(false),
no need for running_. We now use client_ != null as the condition for running.

https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
media/base/pipeline_impl.cc:48: status_(PIPELINE_OK),
state machine has been moved to RendererWrapper.

https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
media/base/pipeline_impl.cc:611: StateTransitionTask(PIPELINE_OK);
Instead of using StateTransitionTask, we use SerialRunner for all tasks. I find
it easier to follow since the steps are listed in one place.

This also allows us to eliminate two intermediate states - kInitDemuxer and
kInitRenderer.

https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
media/base/pipeline_impl.cc:38: static bool enabled =
base::CommandLine::ForCurrentProcess()->HasSwitch(
cached here instead of querying the command line for efficiency.

https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
media/base/pipeline_impl.cc:187: std::unique_ptr<TextRenderer> text_renderer;
We create TextRenderer on the main thread because:
- it is consistent with how media::Renderer is created
- PipelineClient::OnAddTextTrack needs to be called on main thread

TextRenderer is still initialized on the media thread.

https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
media/base/pipeline_impl.cc:211: if
(media_task_runner_->BelongsToCurrentThread()) {
no major change here - just swapped the condition because we no longer have the
main_task_runner_ member variable.

https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
media/base/pipeline_impl.cc:247: DVLOG(2) << __FUNCTION__;
These functions just post tasks to the media thread.

https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
media/base/pipeline_impl.cc:295: DCHECK(thread_checker_.CalledOnValidThread());
client_ is cleared as soon the pipeline stops. So we no longer need a separate
variable.

[sandersd (OOO until July 31), 2016-06-09 00:33:50]

On 2016/06/08 19:59:49, alokp wrote:
> Dan: Sorry for the large patch. I have rearranged the code in pipeline_impl.cc
> so that diffs line up better. I also added some comments to explain the
changes.
> Please see if this is any better.
> 
>
https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
> File media/base/pipeline_impl.cc (left):
> 
>
https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
> media/base/pipeline_impl.cc:44: running_(false),
> no need for running_. We now use client_ != null as the condition for running.
> 
>
https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
> media/base/pipeline_impl.cc:48: status_(PIPELINE_OK),
> state machine has been moved to RendererWrapper.
> 
>
https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
> media/base/pipeline_impl.cc:611: StateTransitionTask(PIPELINE_OK);
> Instead of using StateTransitionTask, we use SerialRunner for all tasks. I
find
> it easier to follow since the steps are listed in one place.
> 
> This also allows us to eliminate two intermediate states - kInitDemuxer and
> kInitRenderer.
> 
>
https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
> File media/base/pipeline_impl.cc (right):
> 
>
https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
> media/base/pipeline_impl.cc:38: static bool enabled =
> base::CommandLine::ForCurrentProcess()->HasSwitch(
> cached here instead of querying the command line for efficiency.
> 
>
https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
> media/base/pipeline_impl.cc:187: std::unique_ptr<TextRenderer> text_renderer;
> We create TextRenderer on the main thread because:
> - it is consistent with how media::Renderer is created
> - PipelineClient::OnAddTextTrack needs to be called on main thread
> 
> TextRenderer is still initialized on the media thread.
> 
>
https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
> media/base/pipeline_impl.cc:211: if
> (media_task_runner_->BelongsToCurrentThread()) {
> no major change here - just swapped the condition because we no longer have
the
> main_task_runner_ member variable.
> 
>
https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
> media/base/pipeline_impl.cc:247: DVLOG(2) << __FUNCTION__;
> These functions just post tasks to the media thread.
> 
>
https://codereview.chromium.org/1999893004/diff/460001/media/base/pipeline_im...
> media/base/pipeline_impl.cc:295:
DCHECK(thread_checker_.CalledOnValidThread());
> client_ is cleared as soon the pipeline stops. So we no longer need a separate
> variable.

Thanks, this helps!

It's still far too large of a change to a core media class to properly review
though. Why was it necessary to rip out the existing state machine and replace
it with OnFooDone() methods in this CL, for example?

I understand that these should be no-op changes, but it's hard to prove that
when there are so many changes, and sadly I don't trust our unit tests here.

[alokp, 2016-06-09 06:31:06]

OK. Reverted the state machine changes. I still think we should make those
changes but it does not need to be part of this change.

[sandersd (OOO until July 31), 2016-06-09 19:30:26]

There are a number of places where what used to be atomic is now posted as a
task; please list those in the CL description since they are likely reasons to
need to revert the CL if bugs are encountered.

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
File media/base/pipeline_impl.cc (left):

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:641: UMA_HISTOGRAM_COUNTS("Media.DroppedFrameCount",
This UMA report seems to have disappeared entirely?

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:37: bool TextTracksEnabled() {
Is this worth the indirection, given that it's called at most once per WMPI
instance?

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:142: base::WeakPtrFactory<RendererWrapper>
weak_factory_;
There seem to be weak pointers being created from this factory from different
threads, which is not safe. There should be a |weak_this_| so that the factory
never needs thread synchronization.

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:171: weak_factory_.InvalidateWeakPtrs();
Is there a specific reason you do not want to rely on the ordinary destruction
of the factory? We use that pattern throughout media/.

That said, I also see a lot of IsRunning() checks in the callbacks. Perhaps the
weak factory should be invalidated in Stop() instead?

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:174: media_task_runner_->DeleteSoon(FROM_HERE,
renderer_wrapper_.release());
Should we be taking any specific precautions to ensure that the media task loop
is running / will run again? (In particular for unit tests.)

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:220: media_task_runner_->PostTask(
If this task is posted, it's not obvious why the rest of the method is correct.
Why not just directly call it as before?

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:233: // destroyed on the media thread.
Perhaps this is a case where ref counting would be simpler ;-)

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:238: media_task_runner_->PostTask(
We should still verify that the task is successfully posted.

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:321: base::Passed(&renderer)));
Why swap the parameter order here?

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:339: base::Unretained(renderer_wrapper_.get()),
volume));
I see that some of these wrappers are passing the parameter, others are passing
the private member value. They are of course the same, but we should pick a
consistent style.

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:346: : renderer_wrapper_->GetMediaTime();
There is a window here between RendererWrapper::Suspend() saving the timestamp
and PipelineImpl finding out about it, during which time this call will go
through to RendererWrapper and various bad things will happen.

I expect the correct solution is to always call though to RendererWrapper and
put |suspend_timestamp_| under a lock. (Sharing |renderer_lock_| is fine too,
they are closely related.)

If you keep something like this, then nit: pick only one of |suspend_time_| and
|suspent_timestamp_|.

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:605: main_task_runner_->PostTask(
Is there a particular reason the order was changed here?

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:687: // Demuxer and Renderer during stopping.
?

(Also, we can't truly rely on that ordering, since they could queue a sequence
of tasks.)

https://codereview.chromium.org/1999893004/diff/480001/media/base/pipeline_im...
media/base/pipeline_impl.cc:1170: // |pending_callbacks_| running on the media
thread would result in crashes.
This comment should be moved to where base::Unretained() is used.

[alokp, 2016-06-10 00:06:02]

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
File media/base/pipeline_impl.cc (left):

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:641: UMA_HISTOGRAM_COUNTS("Media.DroppedFrameCount",
On 2016/06/09 19:30:26, sandersd wrote:
> This UMA report seems to have disappeared entirely?

Do you think it is important to check for state_ here? This pipeline can be
suspended/resumed multiple times, so it does not seem important to check for
current state.

The reason I am asking is because now PipelineImpl, which collects
PipelineStatistics does not know about the state_. To replicate the current
behaviour I will need to collect video_frames_decoded in RendererWrapper as
well.

I have added it in PipelineImpl::Stop for now. If you think checking for state_
is important, I will move the implementation to RendererWrapper.

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

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:37: bool TextTracksEnabled() {
On 2016/06/09 19:30:26, sandersd wrote:
> Is this worth the indirection, given that it's called at most once per WMPI
> instance?

May be not but it should not hurt. Your call.

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:142: base::WeakPtrFactory<RendererWrapper>
weak_factory_;
On 2016/06/09 19:30:26, sandersd wrote:
> There seem to be weak pointers being created from this factory from different
> threads, which is not safe. There should be a |weak_this_| so that the factory
> never needs thread synchronization.

Done.

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:171: weak_factory_.InvalidateWeakPtrs();
On 2016/06/09 19:30:25, sandersd wrote:
> Is there a specific reason you do not want to rely on the ordinary destruction
> of the factory? We use that pattern throughout media/.

Yes - we pass a weak_ptr for PipelineImpl to RendererWrapper. If the DeleteSoon
task gets run on the media thread before PipelineImpl::weak_factory_ is
destroyed, the weak_ptr will be invalidated on the wrong thread. Added comment.

> That said, I also see a lot of IsRunning() checks in the callbacks. Perhaps
the
> weak factory should be invalidated in Stop() instead?

I did that in one of the initial patches but then
PipelineImplTest.BufferedTimeRangesCanChangeAfterStop fails. I am not sure if
the test is valid. It calls PipelineImpl::DidLoadingProgress() and
PipelineImpl::GetBufferedTimeRanges() after the pipeline has stopped. If this is
not a valid usecase, I can move InvalidateWeakPtrs to Stop as you suggest.

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:174: media_task_runner_->DeleteSoon(FROM_HERE,
renderer_wrapper_.release());
On 2016/06/09 19:30:26, sandersd wrote:
> Should we be taking any specific precautions to ensure that the media task
loop
> is running / will run again? (In particular for unit tests.)

I am draining the message loop in PipelineIntegrationTestBase destructor. If not
asan bots turn red.

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:220: media_task_runner_->PostTask(
On 2016/06/09 19:30:26, sandersd wrote:
> If this task is posted, it's not obvious why the rest of the method is
correct.
> Why not just directly call it as before?

That could cause RendererWrapper::Stop to run before the tasks
(InitializeDemuxer, InitializeRenderer, etc) already posted by SerialRunner. I
would need to detect the stopped state and abort those tasks. Currently I DCHECK
because in production code, Stop always runs after the tasks posted by
SerialRunner. I am not sure if we should change our assumptions too much for
unittests.

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:238: media_task_runner_->PostTask(
On 2016/06/09 19:30:26, sandersd wrote:
> We should still verify that the task is successfully posted.

Done.

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:321: base::Passed(&renderer)));
On 2016/06/09 19:30:26, sandersd wrote:
> Why swap the parameter order here?

did not mean to. reverted.

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:339: base::Unretained(renderer_wrapper_.get()),
volume));
On 2016/06/09 19:30:26, sandersd wrote:
> I see that some of these wrappers are passing the parameter, others are
passing
> the private member value. They are of course the same, but we should pick a
> consistent style.

used member value everywhere.

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:346: : renderer_wrapper_->GetMediaTime();
On 2016/06/09 19:30:26, sandersd wrote:
> There is a window here between RendererWrapper::Suspend() saving the timestamp
> and PipelineImpl finding out about it, during which time this call will go
> through to RendererWrapper and various bad things will happen.
> 
> I expect the correct solution is to always call though to RendererWrapper and
> put |suspend_timestamp_| under a lock. (Sharing |renderer_lock_| is fine too,
> they are closely related.)
> 
> If you keep something like this, then nit: pick only one of |suspend_time_|
and
> |suspent_timestamp_|.

Good catch! Done.

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:605: main_task_runner_->PostTask(
On 2016/06/09 19:30:25, sandersd wrote:
> Is there a particular reason the order was changed here?

not really - it seemed more natural but not necessary for this patch. Reverted.

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:687: // Demuxer and Renderer during stopping.
On 2016/06/09 19:30:26, sandersd wrote:
> ?
> 
> (Also, we can't truly rely on that ordering, since they could queue a sequence
> of tasks.)

you are right - no need to post it.

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:1170: // |pending_callbacks_| running on the media
thread would result in crashes.
On 2016/06/09 19:30:26, sandersd wrote:
> This comment should be moved to where base::Unretained() is used.

Done.

[xhwang, 2016-06-10 18:00:24]

I don't feel I have any cycle to look at this CL any time soon so I'll just
defer to sandersd to do the review.

[sandersd (OOO until July 31), 2016-06-10 18:22:50]

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
File media/base/pipeline_impl.cc (left):

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:641: UMA_HISTOGRAM_COUNTS("Media.DroppedFrameCount",
On 2016/06/10 00:06:02, alokp wrote:
> On 2016/06/09 19:30:26, sandersd wrote:
> > This UMA report seems to have disappeared entirely?
> 
> Do you think it is important to check for state_ here? This pipeline can be
> suspended/resumed multiple times, so it does not seem important to check for
> current state.
> 
> The reason I am asking is because now PipelineImpl, which collects
> PipelineStatistics does not know about the state_. To replicate the current
> behaviour I will need to collect video_frames_decoded in RendererWrapper as
> well.
> 
> I have added it in PipelineImpl::Stop for now. If you think checking for
state_
> is important, I will move the implementation to RendererWrapper.

The goal is to only report a number when at least one frame has been
successfully decoded. We expect a relatively large number of failed playbacks
that should not be counted.

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

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:37: bool TextTracksEnabled() {
On 2016/06/10 00:06:02, alokp wrote:
> On 2016/06/09 19:30:26, sandersd wrote:
> > Is this worth the indirection, given that it's called at most once per WMPI
> > instance?
> 
> May be not but it should not hurt. Your call.

I think I would prefer to not extract this.

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:687: // Demuxer and Renderer during stopping.
On 2016/06/10 00:06:02, alokp wrote:
> On 2016/06/09 19:30:26, sandersd wrote:
> > ?
> > 
> > (Also, we can't truly rely on that ordering, since they could queue a
sequence
> > of tasks.)
> 
> you are right - no need to post it.

Actually I just couldn't parse the comment, but this looks better :-)

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

https://chromiumcodereview.appspot.com/1999893004/diff/500001/media/base/pipe...
media/base/pipeline_impl.cc:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
There are a number of places where what used to be atomic is now posted as a
task; please list those in the CL description since they are likely reasons to
need to revert the CL if bugs are encountered.

[alokp, 2016-06-10 21:25:37]

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
File media/base/pipeline_impl.cc (left):

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:641: UMA_HISTOGRAM_COUNTS("Media.DroppedFrameCount",
On 2016/06/10 18:22:50, sandersd wrote:
> On 2016/06/10 00:06:02, alokp wrote:
> > On 2016/06/09 19:30:26, sandersd wrote:
> > > This UMA report seems to have disappeared entirely?
> > 
> > Do you think it is important to check for state_ here? This pipeline can be
> > suspended/resumed multiple times, so it does not seem important to check for
> > current state.
> > 
> > The reason I am asking is because now PipelineImpl, which collects
> > PipelineStatistics does not know about the state_. To replicate the current
> > behaviour I will need to collect video_frames_decoded in RendererWrapper as
> > well.
> > 
> > I have added it in PipelineImpl::Stop for now. If you think checking for
> state_
> > is important, I will move the implementation to RendererWrapper.
> 
> The goal is to only report a number when at least one frame has been
> successfully decoded. We expect a relatively large number of failed playbacks
> that should not be counted.

Then we probably do not need to look at the current state because we still check
for "video_frames_decoded > 0" before reporting video_frames_dropped. Is the
current code OK then?

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

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:37: bool TextTracksEnabled() {
On 2016/06/10 18:22:49, sandersd wrote:
> On 2016/06/10 00:06:02, alokp wrote:
> > On 2016/06/09 19:30:26, sandersd wrote:
> > > Is this worth the indirection, given that it's called at most once per
WMPI
> > > instance?
> > 
> > May be not but it should not hurt. Your call.
> 
> I think I would prefer to not extract this.

Do you want to move the entire definition back to the callsite or just remove
the static bool?

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:687: // Demuxer and Renderer during stopping.
On 2016/06/10 18:22:50, sandersd wrote:
> On 2016/06/10 00:06:02, alokp wrote:
> > On 2016/06/09 19:30:26, sandersd wrote:
> > > ?
> > > 
> > > (Also, we can't truly rely on that ordering, since they could queue a
> sequence
> > > of tasks.)
> > 
> > you are right - no need to post it.
> 
> Actually I just couldn't parse the comment, but this looks better :-)

Acknowledged.

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

https://chromiumcodereview.appspot.com/1999893004/diff/500001/media/base/pipe...
media/base/pipeline_impl.cc:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2016/06/10 18:22:50, sandersd wrote:
> There are a number of places where what used to be atomic is now posted as a
> task; please list those in the CL description since they are likely reasons to
> need to revert the CL if bugs are encountered.

AFAICT the only place this changed is OnBufferedTimeRangesChanged, which is why
I had to drain the message loop in unittests before querying for buffered time
ranges. Do you see this kind of change anywhere else?

[sandersd (OOO until July 31), 2016-06-10 22:02:06]

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
File media/base/pipeline_impl.cc (left):

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:641: UMA_HISTOGRAM_COUNTS("Media.DroppedFrameCount",
On 2016/06/10 21:25:36, alokp wrote:
> On 2016/06/10 18:22:50, sandersd wrote:
> > On 2016/06/10 00:06:02, alokp wrote:
> > > On 2016/06/09 19:30:26, sandersd wrote:
> > > > This UMA report seems to have disappeared entirely?
> > > 
> > > Do you think it is important to check for state_ here? This pipeline can
be
> > > suspended/resumed multiple times, so it does not seem important to check
for
> > > current state.
> > > 
> > > The reason I am asking is because now PipelineImpl, which collects
> > > PipelineStatistics does not know about the state_. To replicate the
current
> > > behaviour I will need to collect video_frames_decoded in RendererWrapper
as
> > > well.
> > > 
> > > I have added it in PipelineImpl::Stop for now. If you think checking for
> > state_
> > > is important, I will move the implementation to RendererWrapper.
> > 
> > The goal is to only report a number when at least one frame has been
> > successfully decoded. We expect a relatively large number of failed
playbacks
> > that should not be counted.
> 
> Then we probably do not need to look at the current state because we still
check
> for "video_frames_decoded > 0" before reporting video_frames_dropped. Is the
> current code OK then?

I guess it is, I can't actually think of any special case the old code handled
differently.

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

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:37: bool TextTracksEnabled() {
On 2016/06/10 21:25:37, alokp wrote:
> On 2016/06/10 18:22:49, sandersd wrote:
> > On 2016/06/10 00:06:02, alokp wrote:
> > > On 2016/06/09 19:30:26, sandersd wrote:
> > > > Is this worth the indirection, given that it's called at most once per
> WMPI
> > > > instance?
> > > 
> > > May be not but it should not hurt. Your call.
> > 
> > I think I would prefer to not extract this.
> 
> Do you want to move the entire definition back to the callsite or just remove
> the static bool?

The entire definition.

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

https://chromiumcodereview.appspot.com/1999893004/diff/500001/media/base/pipe...
media/base/pipeline_impl.cc:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2016/06/10 21:25:37, alokp wrote:
> On 2016/06/10 18:22:50, sandersd wrote:
> > There are a number of places where what used to be atomic is now posted as a
> > task; please list those in the CL description since they are likely reasons
to
> > need to revert the CL if bugs are encountered.
> 
> AFAICT the only place this changed is OnBufferedTimeRangesChanged, which is
why
> I had to drain the message loop in unittests before querying for buffered time
> ranges. Do you see this kind of change anywhere else?

I believe the full list is:
  - buffered_time_ranges_
  - did_loading_progress_
  - duration_
  - statistics_

[alokp, 2016-06-10 22:34:39]

Description was changed from

==========
Splits PipelineImpl into main and media thread components.

The outer PipelineImpl runs on the main thread and acts as a proxy
between PipelineClient and the internal RendererWrapper that runs on
the media thread.

This patch should not change anything functionality wise. It just sets
the stage to get rid of WaitableEvent and Locks used in PipelineImpl.

BUG=616959
==========

to

==========
Splits PipelineImpl into main and media thread components.

The outer PipelineImpl runs on the main thread and acts as a proxy
between PipelineClient and the internal RendererWrapper that runs on
the media thread. This sets the stage to get rid of WaitableEvent
and Locks used in PipelineImpl.

This patch should not change anything functionality wise except for
timing of the notifications received from Demuxer and Renderer. In
response to the notifications we used to change member variables
inside a lock. Now we post a task to change those variables to the
main thread. The list of variables affected by this change are:
- buffered_time_ranges_
- did_loading_progress_
- duration_
- statistics_

BUG=616959
==========

[alokp, 2016-06-10 22:35:04]

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
File media/base/pipeline_impl.cc (left):

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:641: UMA_HISTOGRAM_COUNTS("Media.DroppedFrameCount",
On 2016/06/10 22:02:06, sandersd wrote:
> On 2016/06/10 21:25:36, alokp wrote:
> > On 2016/06/10 18:22:50, sandersd wrote:
> > > On 2016/06/10 00:06:02, alokp wrote:
> > > > On 2016/06/09 19:30:26, sandersd wrote:
> > > > > This UMA report seems to have disappeared entirely?
> > > > 
> > > > Do you think it is important to check for state_ here? This pipeline can
> be
> > > > suspended/resumed multiple times, so it does not seem important to check
> for
> > > > current state.
> > > > 
> > > > The reason I am asking is because now PipelineImpl, which collects
> > > > PipelineStatistics does not know about the state_. To replicate the
> current
> > > > behaviour I will need to collect video_frames_decoded in RendererWrapper
> as
> > > > well.
> > > > 
> > > > I have added it in PipelineImpl::Stop for now. If you think checking for
> > > state_
> > > > is important, I will move the implementation to RendererWrapper.
> > > 
> > > The goal is to only report a number when at least one frame has been
> > > successfully decoded. We expect a relatively large number of failed
> playbacks
> > > that should not be counted.
> > 
> > Then we probably do not need to look at the current state because we still
> check
> > for "video_frames_decoded > 0" before reporting video_frames_dropped. Is the
> > current code OK then?
> 
> I guess it is, I can't actually think of any special case the old code handled
> differently.

Acknowledged.

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

https://chromiumcodereview.appspot.com/1999893004/diff/480001/media/base/pipe...
media/base/pipeline_impl.cc:37: bool TextTracksEnabled() {
On 2016/06/10 22:02:06, sandersd wrote:
> On 2016/06/10 21:25:37, alokp wrote:
> > On 2016/06/10 18:22:49, sandersd wrote:
> > > On 2016/06/10 00:06:02, alokp wrote:
> > > > On 2016/06/09 19:30:26, sandersd wrote:
> > > > > Is this worth the indirection, given that it's called at most once per
> > WMPI
> > > > > instance?
> > > > 
> > > > May be not but it should not hurt. Your call.
> > > 
> > > I think I would prefer to not extract this.
> > 
> > Do you want to move the entire definition back to the callsite or just
remove
> > the static bool?
> 
> The entire definition.

Done.

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

https://chromiumcodereview.appspot.com/1999893004/diff/500001/media/base/pipe...
media/base/pipeline_impl.cc:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2016/06/10 22:02:06, sandersd wrote:
> On 2016/06/10 21:25:37, alokp wrote:
> > On 2016/06/10 18:22:50, sandersd wrote:
> > > There are a number of places where what used to be atomic is now posted as
a
> > > task; please list those in the CL description since they are likely
reasons
> to
> > > need to revert the CL if bugs are encountered.
> > 
> > AFAICT the only place this changed is OnBufferedTimeRangesChanged, which is
> why
> > I had to drain the message loop in unittests before querying for buffered
time
> > ranges. Do you see this kind of change anywhere else?
> 
> I believe the full list is:
>   - buffered_time_ranges_
>   - did_loading_progress_
>   - duration_
>   - statistics_

Ah yes. Thanks. Added to the patch description.

[sandersd (OOO until July 31), 2016-06-10 22:38:37]

lgtm

[alokp, 2016-06-10 22:44:24]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2016-06-10 22:44:56]

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

[commit-bot: I haz the power, 2016-06-10 23:40:41]

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

[commit-bot: I haz the power, 2016-06-10 23:40:42]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alokp, 2016-06-10 23:58:46]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2016-06-10 23:59:29]

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

[commit-bot: I haz the power, 2016-06-11 00:23:39]

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

[commit-bot: I haz the power, 2016-06-11 00:23:40]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[alokp, 2016-06-11 00:59:57]

The CQ bit was checked by alokp@chromium.org

[alokp, 2016-06-11 00:59:58]

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/1999893004/#ps560001
(title: "no-op state transition after error")

[commit-bot: I haz the power, 2016-06-11 01:00:30]

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

[commit-bot: I haz the power, 2016-06-11 02:27:51]

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

[commit-bot: I haz the power, 2016-06-11 02:27:53]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alokp, 2016-06-11 07:31:16]

The CQ bit was checked by alokp@chromium.org

[alokp, 2016-06-11 07:31:17]

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/1999893004/#ps580001
(title: "restores posting stop done_cb")

[commit-bot: I haz the power, 2016-06-11 07:31:25]

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

[commit-bot: I haz the power, 2016-06-11 09:20:55]

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

[commit-bot: I haz the power, 2016-06-11 09:20:57]

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-06-11 15:25:47]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2016-06-11 15:25:58]

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

[commit-bot: I haz the power, 2016-06-11 17:31:15]

Committed patchset #30 (id:580001)

[commit-bot: I haz the power, 2016-06-11 17:33:09]

Description was changed from

==========
Splits PipelineImpl into main and media thread components.

The outer PipelineImpl runs on the main thread and acts as a proxy
between PipelineClient and the internal RendererWrapper that runs on
the media thread. This sets the stage to get rid of WaitableEvent
and Locks used in PipelineImpl.

This patch should not change anything functionality wise except for
timing of the notifications received from Demuxer and Renderer. In
response to the notifications we used to change member variables
inside a lock. Now we post a task to change those variables to the
main thread. The list of variables affected by this change are:
- buffered_time_ranges_
- did_loading_progress_
- duration_
- statistics_

BUG=616959
==========

to

==========
Splits PipelineImpl into main and media thread components.

The outer PipelineImpl runs on the main thread and acts as a proxy
between PipelineClient and the internal RendererWrapper that runs on
the media thread. This sets the stage to get rid of WaitableEvent
and Locks used in PipelineImpl.

This patch should not change anything functionality wise except for
timing of the notifications received from Demuxer and Renderer. In
response to the notifications we used to change member variables
inside a lock. Now we post a task to change those variables to the
main thread. The list of variables affected by this change are:
- buffered_time_ranges_
- did_loading_progress_
- duration_
- statistics_

BUG=616959

Committed: https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662
Cr-Commit-Position: refs/heads/master@{#399376}
==========

[commit-bot: I haz the power, 2016-06-11 17:33:10]

Patchset 30 (id:??) landed as
https://crrev.com/1116967f7c15965538b42c2a402712279d4e5662
Cr-Commit-Position: refs/heads/master@{#399376}