[Video] Don't pause video-only elements if they're already paused...

[Video] Don't pause video-only elements if they're already paused...

...or if the pipeline is not running, resuming or seeking.

BUG=681443
TEST=manual

Review-Url: https://codereview.chromium.org/2644723004
Cr-Commit-Position: refs/heads/master@{#445250}
Committed: https://chromium.googlesource.com/chromium/src/+/56e1f3949a865e2cccf125d0f6858d47a0e423c8

[whywhat, 2017-01-19 23:50:23]

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

[whywhat, 2017-01-19 23:50:24]

PTaL

We discussed a proper fix would move the pausing logic to UpdatePlayState,
however, this seems like a simpler way.

[whywhat, 2017-01-20 03:38:21]

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

[commit-bot: I haz the power, 2017-01-20 03:38:34]

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, 2017-01-20 05:10:21]

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

[commit-bot: I haz the power, 2017-01-20 05:10:21]

Dry run: This issue passed the CQ dry run.

[whywhat, 2017-01-20 17:59:39]

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

[DaleCurtis, 2017-01-20 18:20:23]

Seems like this belongs in ShouldPauseVideoWhenHidden() ?

[sandersd (OOO until July 31), 2017-01-20 18:58:57]

> ...or if the pipeline is resuming or seeking.

Does pausing actually cause problems during those?

I don't recall how Blink orders pause() relative to a seek completing, but it
will certainly call pause() during a non-seeking-resume.

> Seems like this belongs in ShouldPauseVideoWhenHidden() ?

This can't go there because the alternative to pausing is track removal, which
we don't want to trigger.

It is getting complex enough that it may make sense to extract as well though,
something like:

  if (ShouldPauseVideoWhenHidden()) {
    PauseWhenHiddenIfNeeded();
  } else {
    DisableVideoTrackIfNeeded();
  }

We would then also want to add checks at each of the edges;
OnBufferingStateChange() actually covers both seek and resume, but
OnPipelineResumed() already has edge logic for disabled tracks, so logic there
makes sense as well.

[whywhat, 2017-01-20 21:11:54]

Added PauseVideoIfNeeded

[whywhat, 2017-01-20 21:15:46]

On 2017/01/20 at 18:58:57, sandersd wrote:
> > ...or if the pipeline is resuming or seeking.
> 
> Does pausing actually cause problems during those?

So the crash happens when GetMediaTime is called in the RendererImpl when
time_source_ is not set. It seems to be set in VideoRendererInitDone callback
which is after the renderer is fully initialized -- that seems to be about the
time when OnPipelineResumed or OnPipelineSeeked are called. 
pause() calls GetMediaTime() to record the exact paused time, perhaps it could
just fallback to some value if the renderer is not initialized yet?

> 
> I don't recall how Blink orders pause() relative to a seek completing, but it
will certainly call pause() during a non-seeking-resume.
> 
> > Seems like this belongs in ShouldPauseVideoWhenHidden() ?
> 
> This can't go there because the alternative to pausing is track removal, which
we don't want to trigger.
> 
> It is getting complex enough that it may make sense to extract as well though,
something like:
> 
>   if (ShouldPauseVideoWhenHidden()) {
>     PauseWhenHiddenIfNeeded();
>   } else {
>     DisableVideoTrackIfNeeded();
>   }
> 
> We would then also want to add checks at each of the edges;
OnBufferingStateChange() actually covers both seek and resume, but
OnPipelineResumed() already has edge logic for disabled tracks, so logic there
makes sense as well.

Done, except for OnBufferingStateChange. Should I copy/call OnPipelineResumed()
there?

[whywhat, 2017-01-20 21:17:03]

On 2017/01/20 at 21:15:46, whywhat wrote:
> On 2017/01/20 at 18:58:57, sandersd wrote:
> > > ...or if the pipeline is resuming or seeking.
> > 
> > Does pausing actually cause problems during those?
> 
> So the crash happens when GetMediaTime is called in the RendererImpl when
time_source_ is not set. It seems to be set in VideoRendererInitDone callback
which is after the renderer is fully initialized -- that seems to be about the
time when OnPipelineResumed or OnPipelineSeeked are called. 
> pause() calls GetMediaTime() to record the exact paused time, perhaps it could
just fallback to some value if the renderer is not initialized yet?
> 
> > 
> > I don't recall how Blink orders pause() relative to a seek completing, but
it will certainly call pause() during a non-seeking-resume.
> > 
> > > Seems like this belongs in ShouldPauseVideoWhenHidden() ?
> > 
> > This can't go there because the alternative to pausing is track removal,
which we don't want to trigger.
> > 
> > It is getting complex enough that it may make sense to extract as well
though, something like:
> > 
> >   if (ShouldPauseVideoWhenHidden()) {
> >     PauseWhenHiddenIfNeeded();
> >   } else {
> >     DisableVideoTrackIfNeeded();
> >   }
> > 
> > We would then also want to add checks at each of the edges;
OnBufferingStateChange() actually covers both seek and resume, but
OnPipelineResumed() already has edge logic for disabled tracks, so logic there
makes sense as well.
> 
> Done, except for OnBufferingStateChange. Should I copy/call
OnPipelineResumed() there?

Actually, I don't see how OnBufferingStateChange affects seeking_ or
is_pipeline_resuming_...

[sandersd (OOO until July 31), 2017-01-20 21:24:19]

lgtm

https://codereview.chromium.org/2644723004/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2644723004/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:2184: if (is_pipeline_resuming_ || seeking_
|| paused_ || paused_when_hidden_)
Require or DCHECK IsHidden?

Do we need to also check if the pipeline has been started yet?

[sandersd (OOO until July 31), 2017-01-20 21:26:32]

On 2017/01/20 21:17:03, whywhat wrote:
> On 2017/01/20 at 21:15:46, whywhat wrote:
> > On 2017/01/20 at 18:58:57, sandersd wrote:
> > > > ...or if the pipeline is resuming or seeking.
> > > 
> > > Does pausing actually cause problems during those?
> > 
> > So the crash happens when GetMediaTime is called in the RendererImpl when
> time_source_ is not set. It seems to be set in VideoRendererInitDone callback
> which is after the renderer is fully initialized -- that seems to be about the
> time when OnPipelineResumed or OnPipelineSeeked are called. 
> > pause() calls GetMediaTime() to record the exact paused time, perhaps it
could
> just fallback to some value if the renderer is not initialized yet?
> > 
> > > 
> > > I don't recall how Blink orders pause() relative to a seek completing, but
> it will certainly call pause() during a non-seeking-resume.
> > > 
> > > > Seems like this belongs in ShouldPauseVideoWhenHidden() ?
> > > 
> > > This can't go there because the alternative to pausing is track removal,
> which we don't want to trigger.
> > > 
> > > It is getting complex enough that it may make sense to extract as well
> though, something like:
> > > 
> > >   if (ShouldPauseVideoWhenHidden()) {
> > >     PauseWhenHiddenIfNeeded();
> > >   } else {
> > >     DisableVideoTrackIfNeeded();
> > >   }
> > > 
> > > We would then also want to add checks at each of the edges;
> OnBufferingStateChange() actually covers both seek and resume, but
> OnPipelineResumed() already has edge logic for disabled tracks, so logic there
> makes sense as well.
> > 
> > Done, except for OnBufferingStateChange. Should I copy/call
> OnPipelineResumed() there?
> 
> Actually, I don't see how OnBufferingStateChange affects seeking_ or
> is_pipeline_resuming_...

I would just consider adding a call from OnPipelineSeeked() for now.

[whywhat, 2017-01-20 22:19:45]

Added extra checks

[whywhat, 2017-01-20 22:43:53]

Improved comments, extracted method

[whywhat, 2017-01-20 22:51:06]

Description was changed from

==========
[Video] Don't pause video-only elements if they're already paused...

...or if the pipeline is resuming or seeking.

BUG=681443
TEST=manual
==========

to

==========
[Video] Don't pause video-only elements if they're already paused...

...or if the pipeline is not running, resuming or seeking.

BUG=681443
TEST=manual
==========

[sandersd (OOO until July 31), 2017-01-20 22:57:00]

lgtm.

One optional refactoring suggestion.

https://codereview.chromium.org/2644723004/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2644723004/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1413: if (ShouldPauseVideoWhenHidden()) {
Should just call UpdateBackgroundVideoOptimizationState().

if (IsHidden())
  UpdateBackgroundVideoOptimizationState();

is a reasonable structure if we are being pedantic, but if anything it would
make more sense to use IsFrameHidden() there instead of OnHidden(); I don't
think we want undo the optimization on frame close.

[whywhat, 2017-01-20 23:24:45]

https://codereview.chromium.org/2644723004/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2644723004/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1413: if (ShouldPauseVideoWhenHidden()) {
On 2017/01/20 at 22:57:00, sandersd wrote:
> Should just call UpdateBackgroundVideoOptimizationState().
> 
> if (IsHidden())
>   UpdateBackgroundVideoOptimizationState();
> 
> is a reasonable structure if we are being pedantic, but if anything it would
make more sense to use IsFrameHidden() there instead of OnHidden(); I don't
think we want undo the optimization on frame close.

Hm, here we already know we're hidden.
We also want to return early in the pause case since UpdatePlayState is already
called.
I guess UpdateBVO could return a bool and we'd return if it's true.

[whywhat, 2017-01-20 23:25:08]

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

[commit-bot: I haz the power, 2017-01-20 23:25:25]

Dry run: 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), 2017-01-20 23:29:42]

https://codereview.chromium.org/2644723004/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2644723004/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1413: if (ShouldPauseVideoWhenHidden()) {
On 2017/01/20 23:24:45, whywhat wrote:
> On 2017/01/20 at 22:57:00, sandersd wrote:
> > Should just call UpdateBackgroundVideoOptimizationState().
> > 
> > if (IsHidden())
> >   UpdateBackgroundVideoOptimizationState();
> > 
> > is a reasonable structure if we are being pedantic, but if anything it would
> make more sense to use IsFrameHidden() there instead of OnHidden(); I don't
> think we want undo the optimization on frame close.
> 
> Hm, here we already know we're hidden.
> We also want to return early in the pause case since UpdatePlayState is
already
> called.
> I guess UpdateBVO could return a bool and we'd return if it's true.

Ah I see, I missed the early return when comparing the two blocks of code. It's
not a big deal to run UpdatePlayState() twice, but this duplicate code isn't
going to make refactoring any harder.

I do want to point out that we don't actually know for sure here whether
IsHidden() is true. I don't think it's possible to get OnFrameHidden() after
OnFrameClosed(), but I don't think the API explicitly makes that guarantee.

[commit-bot: I haz the power, 2017-01-21 01:02:53]

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

[commit-bot: I haz the power, 2017-01-21 01:02:54]

Dry run: This issue passed the CQ dry run.

[whywhat, 2017-01-21 02:00:33]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2017-01-21 02:00:56]

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, 2017-01-21 02:06:38]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484964033241630,
"parent_rev": "2d74d5b6592b0cef8b085bfa07711473dfae707f", "commit_rev":
"56e1f3949a865e2cccf125d0f6858d47a0e423c8"}

[commit-bot: I haz the power, 2017-01-21 02:07:15]

Description was changed from

==========
[Video] Don't pause video-only elements if they're already paused...

...or if the pipeline is not running, resuming or seeking.

BUG=681443
TEST=manual
==========

to

==========
[Video] Don't pause video-only elements if they're already paused...

...or if the pipeline is not running, resuming or seeking.

BUG=681443
TEST=manual

Review-Url: https://codereview.chromium.org/2644723004
Cr-Commit-Position: refs/heads/master@{#445250}
Committed:
https://chromium.googlesource.com/chromium/src/+/56e1f3949a865e2cccf125d0f685...
==========

[commit-bot: I haz the power, 2017-01-21 02:07:16]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/56e1f3949a865e2cccf125d0f685...