Make PipelineImpl state change tasks consistent.

Make PipelineImpl state change tasks consistent.

Committed: https://crrev.com/12386d605430eaec8d7b5da7d7e3816eea3aa2b8
Cr-Commit-Position: refs/heads/master@{#408830}

[alokp, 2016-06-24 06:15:07]

Dan: I made these changes originally in
https://codereview.chromium.org/1999893004. But we decided to revert these
changes since that patch was already too big to review.

Please see if these changes look reasonable.

https://codereview.chromium.org/2091893003/diff/40001/media/base/pipeline_imp...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/2091893003/diff/40001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:264: DestroyRenderer();
DoStop() moved here.

https://codereview.chromium.org/2091893003/diff/40001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:300: // Queue asynchronous actions required to
start.
DoSeek() moved here.

https://codereview.chromium.org/2091893003/diff/40001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:698: shared_state_.renderer->StartPlayingFrom(
copied from PipelineImpl::RendererWrapper::StateTransitionTask; switch statement
kPlaying.

https://codereview.chromium.org/2091893003/diff/40001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:729: DestroyRenderer();
copied from PipelineImpl::RendererWrapper::StateTransitionTask; switch statement
kSuspended.

[alokp, 2016-06-24 17:54:15]

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

[sandersd (OOO until July 31), 2016-06-27 22:17:36]

lgtm

[alokp, 2016-06-27 23:21:50]

The CQ bit was checked by alokp@chromium.org

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

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 00:57:23]

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

[commit-bot: I haz the power, 2016-06-28 00:57:24]

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

[alokp, 2016-06-28 04:16:53]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2016-06-28 04:17:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 06:48:07]

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

[commit-bot: I haz the power, 2016-06-28 06:48:09]

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

[alokp, 2016-06-28 12:53:41]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2016-06-28 12:53:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 16:05:12]

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

[commit-bot: I haz the power, 2016-06-28 16:05:13]

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

[alokp, 2016-06-29 17:55:49]

The CQ bit was checked by alokp@chromium.org

[alokp, 2016-06-29 17:55:50]

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/2091893003/#ps60001
(title: "rebase only")

[commit-bot: I haz the power, 2016-06-29 17:57:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 20:11:36]

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

[commit-bot: I haz the power, 2016-06-29 20:11:38]

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

[alokp, 2016-07-28 18:18:37]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 18:19:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 19:42:12]

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

[commit-bot: I haz the power, 2016-07-28 19:42:13]

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

[alokp, 2016-07-28 19:56:47]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 19:57:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-28 22:26:36]

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

[commit-bot: I haz the power, 2016-07-28 22:26:37]

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

[alokp, 2016-07-28 23:07:49]

alokp@chromium.org changed reviewers:
+ servolk@chromium.org

[alokp, 2016-07-28 23:07:50]

Sergey: Can you check if the changes in OnEnabledAudioTracksChanged and
OnSelectedVideoTrackChanged are OK? Thanks!

[servolk, 2016-07-28 23:37:12]

On 2016/07/28 23:07:50, alokp wrote:
> Sergey: Can you check if the changes in OnEnabledAudioTracksChanged and
> OnSelectedVideoTrackChanged are OK? Thanks!

lgtm, although this made me wonder: shouldn't GetMediaTime return
demuxer->GetStartTime instead of 0 (base::TimeDelta) when playback hasn't
started yet?

[alokp, 2016-07-29 00:05:48]

On 2016/07/28 23:37:12, servolk wrote:
> On 2016/07/28 23:07:50, alokp wrote:
> > Sergey: Can you check if the changes in OnEnabledAudioTracksChanged and
> > OnSelectedVideoTrackChanged are OK? Thanks!
> 
> lgtm, although this made me wonder: shouldn't GetMediaTime return
> demuxer->GetStartTime instead of 0 (base::TimeDelta) when playback hasn't
> started yet?

Right. I wondered that too after looking at your implementation. But thats how
it has always been. I am not sure what it is supposed to return while seeking.
Dan?

Anyways if there is a bug in GetMediaTime, I will fix it separately.

[alokp, 2016-07-29 00:06:00]

The CQ bit was checked by alokp@chromium.org

[alokp, 2016-07-29 00:06:01]

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/2091893003/#ps100001
(title: "fix avtrack tests")

[commit-bot: I haz the power, 2016-07-29 00:06:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sandersd (OOO until July 31), 2016-07-29 00:29:34]

On 2016/07/28 23:37:12, servolk wrote:
> On 2016/07/28 23:07:50, alokp wrote:
> > Sergey: Can you check if the changes in OnEnabledAudioTracksChanged and
> > OnSelectedVideoTrackChanged are OK? Thanks!
> 
> lgtm, although this made me wonder: shouldn't GetMediaTime return
> demuxer->GetStartTime instead of 0 (base::TimeDelta) when playback hasn't
> started yet?

The renderer time and demuxer time are not the same; the timeline is shifted so
that the start is at time 0 (at least whenever the demuxer start time is
positive).

I believe that 0 is correct in this case.

[commit-bot: I haz the power, 2016-07-29 01:48:29]

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

[commit-bot: I haz the power, 2016-07-29 01:48:30]

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

[alokp, 2016-07-29 03:52:50]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2016-07-29 03:52:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

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

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

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

[alokp, 2016-07-29 18:30:52]

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

https://codereview.chromium.org/2091893003/diff/120001/media/base/pipeline_im...
media/base/pipeline_impl.cc:589: : demuxer_->GetStartTime();
I cannot call RendererWrapper::GetMediaTime here because this function can get
called even before Renderer has been initialized. And calling
Renderer::GetMediaTime before Renderer::Initialize is not legal.

Sergey advised me to use Demuxer::GetStartTime instead. Sergey: Can you check if
this is what you meant?

[servolk, 2016-07-29 18:37:36]

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

https://codereview.chromium.org/2091893003/diff/120001/media/base/pipeline_im...
media/base/pipeline_impl.cc:589: : demuxer_->GetStartTime();
On 2016/07/29 18:30:52, alokp wrote:
> I cannot call RendererWrapper::GetMediaTime here because this function can get
> called even before Renderer has been initialized. And calling
> Renderer::GetMediaTime before Renderer::Initialize is not legal.
> 
> Sergey advised me to use Demuxer::GetStartTime instead. Sergey: Can you check
if
> this is what you meant?

Yes, I believe this should be fine.
Indeed OnEnabledAudioTracksChanged and OnSelectedVideoTrackChanged can be
invoked before renderer is initialized, as part of HTMLMediaElement
initialization, since HTML ME selects the first audio track and the first video
track by default just before the playback is started. In those cases the
renderer might be not yet initialized and thus renderer->GetMediaTime would
return an incorrect time (kNoTimestamp or base::TimeDelta). Using the
demuxer_->GetStartTime here should be fine, since the demuxer should be able to
tell us the correct start time after media tracks are initialized (and HTML ME
wouldn't be able to select media tracks before they are populated by the
demuxer).

[alokp, 2016-07-29 18:43:04]

The CQ bit was checked by alokp@chromium.org

[alokp, 2016-07-29 18:43:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
servolk@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2091893003/#ps120001
(title: "restores avtrack currTime logic")

[commit-bot: I haz the power, 2016-07-29 18:44:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-29 21:14:53]

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

[commit-bot: I haz the power, 2016-07-29 21:14:55]

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

[alokp, 2016-07-29 21:30:04]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2016-07-29 21:30:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-30 01:16:28]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-07-30 01:18:13]

Description was changed from

==========
Make PipelineImpl state change tasks consistent.
==========

to

==========
Make PipelineImpl state change tasks consistent.

Committed: https://crrev.com/12386d605430eaec8d7b5da7d7e3816eea3aa2b8
Cr-Commit-Position: refs/heads/master@{#408830}
==========

[commit-bot: I haz the power, 2016-07-30 01:18:14]

Patchset 7 (id:??) landed as
https://crrev.com/12386d605430eaec8d7b5da7d7e3816eea3aa2b8
Cr-Commit-Position: refs/heads/master@{#408830}