[Video] Pause videos with no audio when in the background.

[Video] Pause videos with no audio when in the background.

Pause video-only players when they go to background and continue playback when they go into foreground and hasn't been paused explicitly in the mean time.
Behind the experiment for optimizing background video playback.

BUG=671381
TEST=Manual

Review-Url: https://codereview.chromium.org/2567833002
Cr-Commit-Position: refs/heads/master@{#442110}
Committed: https://chromium.googlesource.com/chromium/src/+/eb9098d5864f531fdc7d72550c4928daed0e626b

[whywhat, 2016-12-10 01:20:18]

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

[whywhat, 2016-12-10 01:20:18]

PTaL

I think I should also prevent suspended video only media from being paused - it
should always keep playing when foregrounded (since no audio videos are often
used for animations).

How about adding hasAudio() to background_pause_suspended? Or is the relevant
logic somewhere else?

[whywhat, 2016-12-10 02:17:55]

Don't suspend video if paused for some time but has no audio

[whywhat, 2016-12-13 17:58:09]

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

[whywhat, 2016-12-13 17:58:09]

+Dale since Dan is OOO

[DaleCurtis, 2016-12-13 20:25:47]

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

[DaleCurtis, 2016-12-13 20:25:47]

Seems okay, needs update to the WMPI tests though.

[sandersd (OOO until July 31), 2016-12-15 01:09:47]

This is a bit of a hack (in that it changes the meaning of
IsBackgroundSuspendEnabled(), but incompletely), so I don't want to directly
copy it into my refactor CL (https://codereview.chromium.org/2490783002/).

Other than that the code is reasonable, if the result is actually a UX goal. As
a fix for the listed bug, it seems over complex, with more inter-dependencies
(between this algorithm, the looping algorithm, and the track toggling
algorithm) than I am comfortable with.

I will need to be convinced that this is a maintainable solution to the problem.

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

https://codereview.chromium.org/2567833002/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1960: if (paused_ ||
!pipeline_controller_.IsSuspended() || !hasAudio())
This doesn't look quite right to me. The purpose of the pause timer to to define
a threshold after which suspended media becomes paused, rather than resuming
automatically when foregrounded.

This seems to say that media without audio is always resumed automatically,
which is a rather broader statement than the CL description. If this change is
necessary to avoid breaking sites, it may mean that the pause timer was a bad
idea to begin with, or that a much more involved heuristic is required.

[whywhat, 2016-12-16 01:20:21]

On 2016/12/15 at 01:09:47, sandersd wrote:
> This is a bit of a hack (in that it changes the meaning of
IsBackgroundSuspendEnabled(), but incompletely), so I don't want to directly
copy it into my refactor CL (https://codereview.chromium.org/2490783002/).

I always found it a bit hacky that on desktop |is_backgrounded| was always true
instead of the logic within ComputePlayState not suspending background videos
depending on the flag state. This is the change I added (with the exception of
no-audio media which is the point of the CL).

> 
> Other than that the code is reasonable, if the result is actually a UX goal.
As a fix for the listed bug, it seems over complex, with more inter-dependencies
(between this algorithm, the looping algorithm, and the track toggling
algorithm) than I am comfortable with.

I agree that there might be a smaller fix for the actual issue I'm trying to fix
-- I'm happy to hear suggestions here or on the bug. 
Maybe we should specifically check if the video is looped and not restart it if
it doesn't have audio and is in the background, restarting it when it becomes
visible again?

> 
> I will need to be convinced that this is a maintainable solution to the
problem.
> 
>
https://codereview.chromium.org/2567833002/diff/40001/media/blink/webmediapla...
> File media/blink/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/2567833002/diff/40001/media/blink/webmediapla...
> media/blink/webmediaplayer_impl.cc:1960: if (paused_ ||
!pipeline_controller_.IsSuspended() || !hasAudio())
> This doesn't look quite right to me. The purpose of the pause timer to to
define a threshold after which suspended media becomes paused, rather than
resuming automatically when foregrounded.
> 
> This seems to say that media without audio is always resumed automatically,
which is a rather broader statement than the CL description. If this change is
necessary to avoid breaking sites, it may mean that the pause timer was a bad
idea to begin with, or that a much more involved heuristic is required.

[whywhat, 2016-12-16 01:20:24]

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

https://codereview.chromium.org/2567833002/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1960: if (paused_ ||
!pipeline_controller_.IsSuspended() || !hasAudio())
On 2016/12/15 at 01:09:47, sandersd wrote:
> This doesn't look quite right to me. The purpose of the pause timer to to
define a threshold after which suspended media becomes paused, rather than
resuming automatically when foregrounded.
> 
> This seems to say that media without audio is always resumed automatically,
which is a rather broader statement than the CL description. If this change is
necessary to avoid breaking sites, it may mean that the pause timer was a bad
idea to begin with, or that a much more involved heuristic is required.

Well, it might be - AFAIK it only affects Android (do we suspend videos that are
offscreen on desktop atm?) and if the video is an animation with no controls,
there's no way to resume it for the user once it's paused, no?

So autoresuming the silent videos seems natural to me and that's how it worked
with WMPA IIRC. That's probably how it should work on desktop as well.

[whywhat, 2016-12-16 20:09:37]

Dan, could you elaborate a bit more about the maintainability question?

To me, the change in ComputePlayState doesn't depend on looping or track
toggling at all so I don't fully understand your concern.

I think I understand the change of the meaning of the flag: on Android it will
mean that if we set the flag to stop suspending anything, we will still suspend
silent videos. I'll look into fixing it. On desktop I think the meaning of the
flag doesn't change when it's present.

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

https://codereview.chromium.org/2567833002/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1960: if (paused_ ||
!pipeline_controller_.IsSuspended() || !hasAudio())
On 2016/12/16 at 01:20:24, whywhat wrote:
> On 2016/12/15 at 01:09:47, sandersd wrote:
> > This doesn't look quite right to me. The purpose of the pause timer to to
define a threshold after which suspended media becomes paused, rather than
resuming automatically when foregrounded.
> > 
> > This seems to say that media without audio is always resumed automatically,
which is a rather broader statement than the CL description. If this change is
necessary to avoid breaking sites, it may mean that the pause timer was a bad
idea to begin with, or that a much more involved heuristic is required.
> 
> Well, it might be - AFAIK it only affects Android (do we suspend videos that
are offscreen on desktop atm?) and if the video is an animation with no
controls, there's no way to resume it for the user once it's paused, no?
> 
> So autoresuming the silent videos seems natural to me and that's how it worked
with WMPA IIRC. That's probably how it should work on desktop as well.

Dan, am I correct that the timer would only trigger for videos suspended in the
background? It seems like it doesn't trigger for the audible videos even on
Android since background video playback was added and it breaks pages when
triggered silent videos. Is there any other use for it? I'm hesitant to just
remove it assuming I don't know of all the use cases.

Maybe this should be a separate change as a bug fix for Android autoplay videos
(I'd even merge it to M56 as I suspect gfycat.com is broken there too).

[sandersd (OOO until July 31), 2016-12-16 21:09:40]

> Dan, could you elaborate a bit more about the maintainability question?
> 
> To me, the change in ComputePlayState doesn't depend on looping or track
> toggling at all so I don't fully understand your concern.

For example, this would require that track removal only be enabled when
suspend/resume is also enabled; the two features would become interdependent,
which is especially bad because suspend/resume was explicitly designed to be
invisible from the POV of HTMLMediaElement.

In addition to that, this design is the reverse of how suspend/resume was
designed. The intended implementation is that WMPI is in some state, and we
decide whether suspended would be a better use of resources. This new code
actually requires the suspend to get correct behavior.

Something closer to the intended design would be to enter an internal pause
state. This would still trigger suspend, but it would fundamentally be the pause
state that prevents looping, instead of the suspend.

> Dan, am I correct that the timer would only trigger for videos suspended in
the
> background? It seems like it doesn't trigger for the audible videos even on
> Android since background video playback was added and it breaks pages when
> triggered silent videos. Is there any other use for it? I'm hesitant to just
> remove it assuming I don't know of all the use cases.

The default behavior is for all playing media elements to resume playback when
foregrounded. The pause timer implements the behavior that videos are paused
when suspended, with a delay so that a rapid switch away from and back to Chrome
won't pause everything permanently.

The conditions should be set such that only unpaused but suspended videos are
affected. Whether they have audio is secondary, although audio-only media may
not be suspended and thus fails the condition.

Background playback should also override this behavior; if the user explicitly
changes the play state then their action should stick.

What we really want to know is whether there is a UI the user can use to unpause
the video. At least when there is audio there is always a play button in the
notification, so it's not an unreasonable distinction to make.

[whywhat, 2017-01-05 22:58:59]

Pause videos without audio

[whywhat, 2017-01-06 01:36:09]

Description was changed from

==========
[Video] Suspend videos with no audio on desktop when in the background.

Pass the actual backgrounded state to ComputePlayState and check the
suspend flag inside, only for audible content.

BUG=671381
TEST=Manual
==========

to

==========
[Video] Pause videos with no audio when in the background.

Pause video-only players when they go to background and continue playback when
they go into foreground and hasn't been paused explicitly in the mean time.
Behind the experiment for optimizing background video playback.

BUG=671381
TEST=Manual
==========

[whywhat, 2017-01-06 01:36:26]

Updated the patch and the CL description. PTAL

[sandersd (OOO until July 31), 2017-01-06 01:44:15]

lgtm % source code comment request.

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

https://codereview.chromium.org/2567833002/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1349: paused_when_hidden_ = true;
Probably needs a comment to explain that the order of these lines is important.

This is also relying on pause() to call UpdatePlayState(), which works, but
isn't obvious.

[whywhat, 2017-01-06 16:56:35]

Added comments

[whywhat, 2017-01-06 16:59:31]

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

https://codereview.chromium.org/2567833002/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:1349: paused_when_hidden_ = true;
On 2017/01/06 at 01:44:15, sandersd wrote:
> Probably needs a comment to explain that the order of these lines is
important.
> 
> This is also relying on pause() to call UpdatePlayState(), which works, but
isn't obvious.

Done. Alternatively I could perhaps take the most of pause() into its own method
(pause_internal() ?) and call that along with setting the flag, calling
UpdatePlayState() and client_->UpdatePlayState() -- sounds like some duplication
but no hidden dependencies. OnPause() call below would need something similar
too though.

[whywhat, 2017-01-06 16:59:38]

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

[commit-bot: I haz the power, 2017-01-06 17:00: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, 2017-01-06 17:03:10]

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

[commit-bot: I haz the power, 2017-01-06 17:03:10]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[sandersd (OOO until July 31), 2017-01-06 20:40:46]

Still lgtm. Comments are enough for now that I don't think any reorganization is
immediately necessary.

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

Rebase

[whywhat, 2017-01-06 22:23:48]

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

https://codereview.chromium.org/2567833002/diff/100001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:2074: return hasVideo() && !hasAudio();
Mounir - should we add autoplay_muted() for this maybe?

[whywhat, 2017-01-06 22:23:57]

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

[commit-bot: I haz the power, 2017-01-06 22:24: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-06 23:56:21]

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

[commit-bot: I haz the power, 2017-01-06 23:56:22]

Dry run: This issue passed the CQ dry run.

[whywhat, 2017-01-07 00:26:30]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2017-01-07 00:26:30]

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/2567833002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-07 00:26:53]

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-07 00:33:07]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483748790421680,
"parent_rev": "1e3963fb07388dd5175f9e2d7bcd7ee1e3c8f134", "commit_rev":
"eb9098d5864f531fdc7d72550c4928daed0e626b"}

[commit-bot: I haz the power, 2017-01-07 00:33:34]

Description was changed from

==========
[Video] Pause videos with no audio when in the background.

Pause video-only players when they go to background and continue playback when
they go into foreground and hasn't been paused explicitly in the mean time.
Behind the experiment for optimizing background video playback.

BUG=671381
TEST=Manual
==========

to

==========
[Video] Pause videos with no audio when in the background.

Pause video-only players when they go to background and continue playback when
they go into foreground and hasn't been paused explicitly in the mean time.
Behind the experiment for optimizing background video playback.

BUG=671381
TEST=Manual

Review-Url: https://codereview.chromium.org/2567833002
Cr-Commit-Position: refs/heads/master@{#442110}
Committed:
https://chromium.googlesource.com/chromium/src/+/eb9098d5864f531fdc7d72550c49...
==========

[commit-bot: I haz the power, 2017-01-07 00:33:35]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/eb9098d5864f531fdc7d72550c49...