|
|
Created:
3 years, 10 months ago by whywhat Modified:
3 years, 9 months ago Reviewers:
DaleCurtis, mlamouri (slow - plz ping), Zhiqiang Zhang (Slow), *sandersd (OOO until July 31) CC:
apacible+watch_chromium.org, chfremer+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, erickung+watch_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org, Zhiqiang Zhang (Slow) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Video] MediaSession API event handlers can resume background video.
ShouldPauseVideoWhenHidden is now used for videos with audio too so that HTMLMediaElement::play() actually reaches WMPI because the video is paused.
WMPI::UpdatePlayState_ComputePlayState now only cares about the presence
of the session and allows playback of anything as long as it got play() command.
WMPI now tracks whether background video playback has been unlocked by a user gesture. RendererWebMediaPlayerDelegate emulates user gesture in OnPlay/OnPause to behave the same way as MediaSession.
BUG=689637
TEST=manual+updated tests
Review-Url: https://codereview.chromium.org/2681863005
Cr-Commit-Position: refs/heads/master@{#452700}
Committed: https://chromium.googlesource.com/chromium/src/+/65fad2795b571f868c559941187f785aaa0414a0
Patch Set 1 #Patch Set 2 : Fixed the unittests #Patch Set 3 : Fixed browser tests and removed some #if ANDROID #Patch Set 4 : Restore permanent 5s pause. #
Total comments: 14
Patch Set 5 : Less state in WMPI? #Patch Set 6 : Restore locking background playback in DidPause #Patch Set 7 : Added UserGesture check to DidPause #Patch Set 8 : has_session is true on desktop #
Total comments: 4
Patch Set 9 : Addressed Dale's comments #Patch Set 10 : Move UserGesture logic to HTMLMediaElement #Patch Set 11 : Remove the flag from RendererWMPDelegate #Patch Set 12 : Whitespace polishing #
Total comments: 6
Patch Set 13 : Tweaked the logic in HTMLME a bit #
Total comments: 4
Patch Set 14 : Moved the lock logic into WMPI #Patch Set 15 : Added tests #
Total comments: 4
Patch Set 16 : Updated the comment in ComputePlayState #Messages
Total messages: 117 (59 generated)
avayvod@chromium.org changed reviewers: + dalecurtis@chromium.org, sandersd@chromium.org
PTaL (+Dale if Dan is OOO)
Fixed the unittests
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
This doesn't seem to touch the speculative pause after 5 seconds code? If we're always pausing on background that should be removed.
On 2017/02/09 at 20:30:50, dalecurtis wrote: > This doesn't seem to touch the speculative pause after 5 seconds code? If we're always pausing on background that should be removed. We don't always pause on desktop though (if the experiment is off or the video is not a candidate). We don't suspend on desktop when hidden either I guess, unless enable-media-suspend flag is set. On Android, we can remove suspend pause timer after this change.
On 2017/02/09 at 21:54:34, avayvod wrote: > On 2017/02/09 at 20:30:50, dalecurtis wrote: > > This doesn't seem to touch the speculative pause after 5 seconds code? If we're always pausing on background that should be removed. > > We don't always pause on desktop though (if the experiment is off or the video is not a candidate). We don't suspend on desktop when hidden either I guess, unless enable-media-suspend flag is set. > > On Android, we can remove suspend pause timer after this change. What's the behavior when tapping the application switcher momentarily and going back to the page now?
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
On 2017/02/09 at 22:43:56, dalecurtis wrote: > On 2017/02/09 at 21:54:34, avayvod wrote: > > On 2017/02/09 at 20:30:50, dalecurtis wrote: > > > This doesn't seem to touch the speculative pause after 5 seconds code? If we're always pausing on background that should be removed. > > > > We don't always pause on desktop though (if the experiment is off or the video is not a candidate). We don't suspend on desktop when hidden either I guess, unless enable-media-suspend flag is set. > > > > On Android, we can remove suspend pause timer after this change. > > What's the behavior when tapping the application switcher momentarily and going back to the page now? We pause the video while in the tab switcher and resume it, AFAIK. Though, I don't think this change is related to this behaviour. It's mostly related to the Media Session API play action that can't resume videos in the background contrary to the notification.
On 2017/02/13 at 12:19:29, mlamouri wrote: > On 2017/02/09 at 22:43:56, dalecurtis wrote: > > On 2017/02/09 at 21:54:34, avayvod wrote: > > > On 2017/02/09 at 20:30:50, dalecurtis wrote: > > > > This doesn't seem to touch the speculative pause after 5 seconds code? If we're always pausing on background that should be removed. > > > > > > We don't always pause on desktop though (if the experiment is off or the video is not a candidate). We don't suspend on desktop when hidden either I guess, unless enable-media-suspend flag is set. > > > > > > On Android, we can remove suspend pause timer after this change. > > > > What's the behavior when tapping the application switcher momentarily and going back to the page now? > > We pause the video while in the tab switcher and resume it, AFAIK. We don't consider tabs background while in the tab switcher, so we won't pause the video. We resume the video upon foregrounding if it was paused because of going into background. > > Though, I don't think this change is related to this behaviour. It's mostly related to the Media Session API play action that can't resume videos in the background contrary to the notification. This change adjusts how audible video is paused when backgrounded with or without MediaSession attached (we pretended playing to the page, now we would explicitly tell the page the element is paused). Since it's paused, there's no need to pause 5s after background suspend. The question is whether we want to keep this pause timer behavior for desktop or never intended to. Dale, since I'd like to land and merge this fix in 57, I'm willing to try to remove the timer in a follow up review for 58 just to keep the diff small. WDYT?
Fixed browser tests and removed some #if ANDROID
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry some of the comments here aren't making sense. I'll patch this in and take a look.
Okay after testing, this breaks the auto-pause after 5 seconds; which isn't okay. It doesn't regress intent switching like I thought it might, but probably because it breaks auto-pause entirely :)
I.e., if you put a playing video in the background, it should not resume into the playing state after 5 seconds have passed.
On 2017/02/13 at 19:27:34, dalecurtis wrote: > I.e., if you put a playing video in the background, it should not resume into the playing state after 5 seconds have passed. What's the argument for the auto-pause behavior? I thought the reason was because we didn't pause immediately. Now we pause immediately. Do we want to: - auto resume when foregrounded? - auto resume when foregrounded within 5s? - never auto resume?
Restore permanent 5s pause.
Should never auto-resume after some timeout, currently at 5 seconds. This prevents user surprise when returning to a long-forgotten tab.
On 2017/02/13 at 21:09:39, dalecurtis wrote: > Should never auto-resume after some timeout, currently at 5 seconds. This prevents user surprise when returning to a long-forgotten tab. Yep, I remembered and so restored it (scheduling the timer doesn't exit early if paused_ && paused_when_hidden_ - then OnPause sets paused_when_hidden_ to false and we don't auto-resume).
https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:118: if (playing_videos_.count(player_id) && IsFrameHidden() && !IsFrameClosed()) Hmm, is this right? Can you add some comments about why you'd want to prevent this here? It looks like it would stop playlist functionality from working in the background properly. https://codereview.chromium.org/2681863005/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2681863005/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1966: bool is_background_playing_allowed = Needs a comment explaining use of the user gesture here.
https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:101: if (has_video && IsFrameHidden() && !IsFrameClosed()) Duplicate |has_video| condition. https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:119: background_video_allowed_ = false; This can leave us in a strange state where there is background video playing, but |background_video_allowed_| is false. This could result in surprising behavior in WMPI if that flag is used in ComputePlayState(). https://codereview.chromium.org/2681863005/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2681863005/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1966: bool is_background_playing_allowed = On 2017/02/13 22:44:54, DaleCurtis wrote: > Needs a comment explaining use of the user gesture here. Indeed; the entire design goal of this function is to be stateless, and this is explicitly stateful. This state should be saved by the caller so that we can query it without consequence at any time in the future.
https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:118: if (playing_videos_.count(player_id) && IsFrameHidden() && !IsFrameClosed()) On 2017/02/13 at 22:44:54, DaleCurtis wrote: > Hmm, is this right? Can you add some comments about why you'd want to prevent this here? It looks like it would stop playlist functionality from working in the background properly. This is consistent with the user pressing on the pause button in the notification (handled in OnMediaDelegatePause below) but covers the MediaSession path (where it's the page that calls pause()) so we don't allow the page to resume despite the user wishes. Perhaps we should just check for user gesture in play() for that (and perhaps we do already). But you're right, if the user presses on the next track, it's possible the page will do something like: v.pause(); // this will prevent play() from working. v.src = nextTrackSrc; v.play(); Which will be broken. If it doesn't call pause(), it will still work though. https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:119: background_video_allowed_ = false; On 2017/02/13 at 22:56:20, sandersd wrote: > This can leave us in a strange state where there is background video playing, but |background_video_allowed_| is false. This could result in surprising behavior in WMPI if that flag is used in ComputePlayState(). Hm, how? This is called when video is being paused and would prevent the video to resume in the background, no? https://codereview.chromium.org/2681863005/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2681863005/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1966: bool is_background_playing_allowed = On 2017/02/13 at 22:56:20, sandersd wrote: > On 2017/02/13 22:44:54, DaleCurtis wrote: > > Needs a comment explaining use of the user gesture here. > > Indeed; the entire design goal of this function is to be stateless, and this is explicitly stateful. This state should be saved by the caller so that we can query it without consequence at any time in the future. Something like play_called_with_user_gesture_ set in play() and then reset in any other method if there's no user gesture? And call UPS again. This kind of states are hard to add to WMPI or at least I still haven't wrapped my head around it :)
https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:119: background_video_allowed_ = false; On 2017/02/14 01:36:28, whywhat_slow_PST_till_Feb_6 wrote: > On 2017/02/13 at 22:56:20, sandersd wrote: > > This can leave us in a strange state where there is background video playing, > but |background_video_allowed_| is false. This could result in surprising > behavior in WMPI if that flag is used in ComputePlayState(). > > Hm, how? This is called when video is being paused and would prevent the video > to resume in the background, no? There can be more than one playing video on a page at a time.
Less state in WMPI?
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Video] MediaSession API event handlers can resume background video. MediaSession API handlers will generally call play() and have the user gesture token. HTMLMediaElement would not pass play() to WMPI as WMPI was lying about the playing state. Now WMPI actually pauses the video when backgrounded instead of suspending it and pretending it's paused to the delegate. WMPI would ignore the play() call since the delegate wouldn't allow any videos to be resumed. WMPI now allows resuming videos if there's a user gesture token. RenderWebMediaPlayerDelegate would not allow resuming the playback again so it has to account for play/pause commands coming from the page to track whether background video playback is unlocked. WMPI would then pause the video again when pipeline was resuming so ShouldPauseVideoWhenHidden now asks the delegate if background video playback is unlocked. BUG=689637 TEST=manual (have to update the unittests yet) ========== to ========== [Video] MediaSession API event handlers can resume background video. MediaSession API handlers will generally call play() and have the user gesture token. HTMLMediaElement would not pass play() to WMPI as WMPI was lying about the playing state. Now WMPI actually pauses the video when backgrounded instead of suspending it and pretending it's paused to the delegate. WMPI would ignore the play() call since the delegate wouldn't allow any videos to be resumed. WMPI now allows resuming videos if there's a user gesture token. RenderWebMediaPlayerDelegate would not allow resuming the playback again so it has to account for the play command coming from the page to track whether background video playback is unlocked. WMPI would then pause the video again when pipeline was resuming so ShouldPauseVideoWhenHidden now asks the delegate if background video playback is unlocked. BUG=689637 TEST=manual (have to update the unittests yet) ==========
Restore locking background playback in DidPause
On 2017/02/14 at 01:39:39, sandersd wrote: > https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... > File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): > > https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... > content/renderer/media/renderer_webmediaplayer_delegate.cc:119: background_video_allowed_ = false; > On 2017/02/14 01:36:28, whywhat_slow_PST_till_Feb_6 wrote: > > On 2017/02/13 at 22:56:20, sandersd wrote: > > > This can leave us in a strange state where there is background video playing, > > but |background_video_allowed_| is false. This could result in surprising > > behavior in WMPI if that flag is used in ComputePlayState(). > > > > Hm, how? This is called when video is being paused and would prevent the video > > to resume in the background, no? > > There can be more than one playing video on a page at a time. The same holds for the OnMediaDelegatePause behavior I think. We don't want to allow the page to resume anything from the background without a user gesture when user paused the session.
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
The CQ bit was unchecked by avayvod@chromium.org
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Added UserGesture check to DidPause
PTAL https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:101: if (has_video && IsFrameHidden() && !IsFrameClosed()) On 2017/02/13 at 22:56:20, sandersd wrote: > Duplicate |has_video| condition. Done. https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:118: if (playing_videos_.count(player_id) && IsFrameHidden() && !IsFrameClosed()) On 2017/02/14 at 01:36:28, whywhat_slow_PST_till_Feb_6 wrote: > On 2017/02/13 at 22:44:54, DaleCurtis wrote: > > Hmm, is this right? Can you add some comments about why you'd want to prevent this here? It looks like it would stop playlist functionality from working in the background properly. > > This is consistent with the user pressing on the pause button in the notification (handled in OnMediaDelegatePause below) but covers the MediaSession path (where it's the page that calls pause()) so we don't allow the page to resume despite the user wishes. Perhaps we should just check for user gesture in play() for that (and perhaps we do already). > > But you're right, if the user presses on the next track, it's possible the page will do something like: > > v.pause(); // this will prevent play() from working. > v.src = nextTrackSrc; > v.play(); > > Which will be broken. If it doesn't call pause(), it will still work though. Added a user gesture check to that so we won't block queuing unless it was the user who paused the playback. If the page will resume the next track in the same event handler it will still work as there will still be a user gesture. https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:119: background_video_allowed_ = false; On 2017/02/14 at 01:39:39, sandersd wrote: > On 2017/02/14 01:36:28, whywhat_slow_PST_till_Feb_6 wrote: > > On 2017/02/13 at 22:56:20, sandersd wrote: > > > This can leave us in a strange state where there is background video playing, > > but |background_video_allowed_| is false. This could result in surprising > > behavior in WMPI if that flag is used in ComputePlayState(). > > > > Hm, how? This is called when video is being paused and would prevent the video > > to resume in the background, no? > > There can be more than one playing video on a page at a time. This is already the case with OnMediaDelegatePause. https://codereview.chromium.org/2681863005/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2681863005/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1966: bool is_background_playing_allowed = On 2017/02/14 at 01:36:28, whywhat_slow_PST_till_Feb_6 wrote: > On 2017/02/13 at 22:56:20, sandersd wrote: > > On 2017/02/13 22:44:54, DaleCurtis wrote: > > > Needs a comment explaining use of the user gesture here. > > > > Indeed; the entire design goal of this function is to be stateless, and this is explicitly stateful. This state should be saved by the caller so that we can query it without consequence at any time in the future. > > Something like play_called_with_user_gesture_ set in play() and then reset in any other method if there's no user gesture? And call UPS again. This kind of states are hard to add to WMPI or at least I still haven't wrapped my head around it :) I simplified the whole background logic in ComputePlayState now (at the expense of blocking play() if IsHidden() and ShouldPauseVideoWhenHidden() are both true).
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... content/renderer/media/renderer_webmediaplayer_delegate.cc:119: background_video_allowed_ = false; On 2017/02/15 17:13:12, whywhat_slow_PST_till_Feb_6 wrote: > On 2017/02/14 at 01:39:39, sandersd wrote: > > On 2017/02/14 01:36:28, whywhat_slow_PST_till_Feb_6 wrote: > > > On 2017/02/13 at 22:56:20, sandersd wrote: > > > > This can leave us in a strange state where there is background video > playing, > > > but |background_video_allowed_| is false. This could result in surprising > > > behavior in WMPI if that flag is used in ComputePlayState(). > > > > > > Hm, how? This is called when video is being paused and would prevent the > video > > > to resume in the background, no? > > > > There can be more than one playing video on a page at a time. > > This is already the case with OnMediaDelegatePause. There is a difference, though; OnMediaDelegatePause() is an explicit user gesture. I imagine that the actual user intent would be to pause all playing content in the page when they press pause there, but I suppose that doesn't happen either.
On 2017/02/15 at 21:54:09, sandersd wrote: > https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... > File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): > > https://codereview.chromium.org/2681863005/diff/60001/content/renderer/media/... > content/renderer/media/renderer_webmediaplayer_delegate.cc:119: background_video_allowed_ = false; > On 2017/02/15 17:13:12, whywhat_slow_PST_till_Feb_6 wrote: > > On 2017/02/14 at 01:39:39, sandersd wrote: > > > On 2017/02/14 01:36:28, whywhat_slow_PST_till_Feb_6 wrote: > > > > On 2017/02/13 at 22:56:20, sandersd wrote: > > > > > This can leave us in a strange state where there is background video > > playing, > > > > but |background_video_allowed_| is false. This could result in surprising > > > > behavior in WMPI if that flag is used in ComputePlayState(). > > > > > > > > Hm, how? This is called when video is being paused and would prevent the > > video > > > > to resume in the background, no? > > > > > > There can be more than one playing video on a page at a time. > > > > This is already the case with OnMediaDelegatePause. > > There is a difference, though; OnMediaDelegatePause() is an explicit user gesture. Yes, that's why I check for a user gesture in DidPause() now. > > I imagine that the actual user intent would be to pause all playing content in the page when they press pause there, but I suppose that doesn't happen either.
has_session is true on desktop
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
UpdatePlayState changes are hard to vet, but tests look good, so just a few questions, but otherwise lgtm https://codereview.chromium.org/2681863005/diff/140001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2681863005/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:124: playing_videos_.count(player_id) && IsFrameHidden() && !IsFrameClosed()) { Is playing_videos_.count() what you want? If the page cycles to paused before the user hits pause you won't actually disable background video and the next playlist entry could soon start. https://codereview.chromium.org/2681863005/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (left): https://codereview.chromium.org/2681863005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1487: OnPlay(); // Calls UpdatePlayState() so return afterwards. Comment is still true, so we end up updating play state twice here?
dalecurtis@chromium.org changed required reviewers: + sandersd@chromium.org
https://codereview.chromium.org/2681863005/diff/140001/content/renderer/media... File content/renderer/media/renderer_webmediaplayer_delegate.cc (right): https://codereview.chromium.org/2681863005/diff/140001/content/renderer/media... content/renderer/media/renderer_webmediaplayer_delegate.cc:124: playing_videos_.count(player_id) && IsFrameHidden() && !IsFrameClosed()) { On 2017/02/16 at 02:47:38, DaleCurtis wrote: > Is playing_videos_.count() what you want? If the page cycles to paused before the user hits pause you won't actually disable background video and the next playlist entry could soon start. If the page is paused, the user shouldn't be able to pause it again? Since we don't check for this in OnMediaDelegatePause anymore, should be safe to remove. https://codereview.chromium.org/2681863005/diff/140001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (left): https://codereview.chromium.org/2681863005/diff/140001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1487: OnPlay(); // Calls UpdatePlayState() so return afterwards. On 2017/02/16 at 02:47:38, DaleCurtis wrote: > Comment is still true, so we end up updating play state twice here? Hm, yes, otherwise on Android we may not reenable the video track. Same as in OnFrameHidden above (UpdateBgVideoOptimizationState may call OnPause()) but then we do want to schedule the pause timer. UPS is supposed to be reenterable IIRC and I'm relying on that here instead of extracting the common bits somehow so that UPS is called only once and only when needed. Let me check if I can simply: EnableVideoTrackIfNeeded(); if (paused_when_hidden_) { paused_when_hidden_ = false; OnPlay(); } else { UpdatePlayState(); }
avayvod@chromium.org changed required reviewers: - sandersd@chromium.org
zqzhang@chromium.org changed reviewers: + zqzhang@chromium.org
I verified on the pages which had problems before and they are all working \o/
Addressed Dale's comments
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dalecurtis@chromium.org changed required reviewers: + sandersd@chromium.org
Dan explained some of his concerns for the user gesture processing and I agree. You should sort this out offline with Dan (I intentionally marked him as required for review with my previous approval).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
On 2017/02/16 at 21:32:43, dalecurtis wrote: > Dan explained some of his concerns for the user gesture processing and I agree. You should sort this out offline with Dan (I intentionally marked him as required for review with my previous approval). Ah, that's what the asterisk was for! I thought I typoed it when adding reviewers :)
Move UserGesture logic to HTMLMediaElement
Remove the flag from RendererWMPDelegate
Whitespace polishing
Description was changed from ========== [Video] MediaSession API event handlers can resume background video. MediaSession API handlers will generally call play() and have the user gesture token. HTMLMediaElement would not pass play() to WMPI as WMPI was lying about the playing state. Now WMPI actually pauses the video when backgrounded instead of suspending it and pretending it's paused to the delegate. WMPI would ignore the play() call since the delegate wouldn't allow any videos to be resumed. WMPI now allows resuming videos if there's a user gesture token. RenderWebMediaPlayerDelegate would not allow resuming the playback again so it has to account for the play command coming from the page to track whether background video playback is unlocked. WMPI would then pause the video again when pipeline was resuming so ShouldPauseVideoWhenHidden now asks the delegate if background video playback is unlocked. BUG=689637 TEST=manual (have to update the unittests yet) ========== to ========== [Video] MediaSession API event handlers can resume background video. ShouldPauseVideoWhenHidden is now used for videos with audio too so that HTMLMediaElement::play() actually reaches WMPI because the video is paused. WMPI::UpdatePlayState_ComputePlayState now only cares about the presence of the session and allows playback of anything as long as it got play() command. HTMLMediaElement now controls the user gesture aspects of the background video playback. It also uses play() and pause() to implement the default MediaSession behavior. BUG=689637 TEST=manual+updated tests ==========
avayvod@chromium.org changed required reviewers: + mlamouri@chromium.org
PTAL Need Mounir to take a look now too since I change HTMLMediaElement.
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
avayvod@, I think it would be good if you could walk me through the CL. I don't know much about the crazy WMPI state machine and the changes I see in HTMLMediaElement without this context might not make as much sense as they should :) https://codereview.chromium.org/2681863005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2681863005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3131: DocumentUserGestureToken::create(&document())); You can create the user gesture token from the callers. Why having it here? https://codereview.chromium.org/2681863005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3135: play(); I'm not sure I follow the reason why this is no longer calling the internal methods. https://codereview.chromium.org/2681863005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3199: m_lockedPendingUserGesture = true; We should probably discuss this more but I think this is going to break web pages. For example, what would happen in the case of YouTube that auto-pauses when in the background: will they be able to auto-resume when visible again? In general, we never reset the lock boolean to true so I'm a bit worried about this.
https://codereview.chromium.org/2681863005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2681863005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3131: DocumentUserGestureToken::create(&document())); On 2017/02/17 at 12:31:31, mlamouri wrote: > You can create the user gesture token from the callers. Why having it here? So the callers don't know a user gesture. Ideally we'd implement the default notification in the same way the MediaSession API is implemented - so it would just call play()/pause() on the HTMLMediaElement with the user gesture token. However, that change is for another CL. https://codereview.chromium.org/2681863005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3135: play(); On 2017/02/17 at 12:31:31, mlamouri wrote: > I'm not sure I follow the reason why this is no longer calling the internal methods. So that we respect the user gesture token and unlock the element back. https://codereview.chromium.org/2681863005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3199: m_lockedPendingUserGesture = true; On 2017/02/17 at 12:31:31, mlamouri wrote: > We should probably discuss this more but I think this is going to break web pages. For example, what would happen in the case of YouTube that auto-pauses when in the background: will they be able to auto-resume when visible again? In general, we never reset the lock boolean to true so I'm a bit worried about this. I want to reuse the user gesture lock to enforce the following: - user needs to resume playback for the background video to play - if user pauses the video, website can't override it I could add another flag to indicate with set the lock to true here or in pause() while in the background so we reset it to false when foregrounded to unlock the element back. The current CL indicates that if the user paused the video in any state by pressing pause or hiding it, the page won't be able to resume playback without a gesture. I should at least check if the video is not paused here, perhaps. If YouTube pauses the video before this call, then we'd not lock it.
On 2017/02/17 at 16:30:25, avayvod wrote: > The current CL indicates that if the user paused the video in any state by pressing pause or hiding it, the page won't be able to resume playback without a gesture. That is a significant behaviour change that worries me.
On 2017/02/17 at 17:00:16, mlamouri wrote: > On 2017/02/17 at 16:30:25, avayvod wrote: > > The current CL indicates that if the user paused the video in any state by pressing pause or hiding it, the page won't be able to resume playback without a gesture. > > That is a significant behaviour change that worries me. Well, m.youtube.com doesn't try to auto-resume. We don't set the flag in HTMLME but we do essentially lock the element when it's backgrounded and when the user presses pause when the video is backgrounded. I'd say we should lock on backgrounding (and unlock when foregrounded unless it was locked for any other reason like moving between documents). And we should either always lock on pause with user gesture or never - doing it only when it's in the background seems very confusing.
Tweaked the logic in HTMLME a bit
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2681863005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2681863005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3133: } I know you already replied to this comment but this change will have a side effect on other callers (such as WebMediaPlayerCast). Would it make sense to use WebScopedUserGesture in WebMediaPlayerImpl? https://codereview.chromium.org/2681863005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3205: } Instead of this logic, would it make sense if the autoplay checks (I guess isLockedPendingUserGesture()) would check whether the video is visible and require a user gesture if it is not. Does that make sense? Obviously, we would need to reproduce some of the checks currently in WebMediaPlayerImpl in HTMLME but that's the direction we want to take anyway, right?
Note that this changes the behavior (compared to the previous PS) in the case when the user pauses the video while in background - the page will be able to resume w/o a user gesture (since we don't check in pause() if it had a user gesture and lock the element back). Paused video won't be autoresumed/autounlocked when brought to the foreground. https://codereview.chromium.org/2681863005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2681863005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3133: } On 2017/02/20 at 12:00:17, mlamouri wrote: > I know you already replied to this comment but this change will have a side effect on other callers (such as WebMediaPlayerCast). Would it make sense to use WebScopedUserGesture in WebMediaPlayerImpl? If I understood Dan correctly - in general, no :) How will this affect other callers exactly? I'm not sure the Cast notification call DelegateOnPlay/Pause. It calls playbackStateChanged when switching back from the remote to local player to continue local playback. I don't think it's broken. I think ideally, instead of choosing whether to use MediaSession handler or the renderer webmediaplayer delegate path, play/pause command should always come through HTMLMediaElement. Doing it in this change will make it too big though :/ https://codereview.chromium.org/2681863005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3205: } On 2017/02/20 at 12:00:17, mlamouri wrote: > Instead of this logic, would it make sense if the autoplay checks (I guess isLockedPendingUserGesture()) would check whether the video is visible and require a user gesture if it is not. Does that make sense? Obviously, we would need to reproduce some of the checks currently in WebMediaPlayerImpl in HTMLME but that's the direction we want to take anyway, right? It should only require a user gesture until it's unlocked in the background and before it's foregrounded again. So it would require a boolean bit anyhow. It would also need to know whether the player has to be paused when hidden - we'd have to expose ShouldPauseWhenHidden from WMPI (as we check for audio/video only, keyframe distance, remote, etc).
After a discussion with Mounir we decided we don't want to change the behavior of pause() w.r.t. having a user gesture - if it's during the background video playback, pause() with user gesture will require a user gesture to play().
Moved the lock logic into WMPI
PTAL I moved the logic to keep track of whether the player can be resumed in the background w/o user gesture to WMPI after talking to Mounir. It doesn't affect UPS now though. RendererWebMediaPlayerDelegate emulates user gesture so WMPU doesn't have to distinguish for delegate OnPlay/OnPause and play/pause from MediaSession. I want to leave only one path (MediaSession) but don't want to make the change bigger. Also improved the tests for background behavior - there's no 96 test cases (templated). I need to update the ComputePlayState_FrameHidden tests and add a WMPI test for video_locked_when_paused_when_hidden_.
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Description was changed from ========== [Video] MediaSession API event handlers can resume background video. ShouldPauseVideoWhenHidden is now used for videos with audio too so that HTMLMediaElement::play() actually reaches WMPI because the video is paused. WMPI::UpdatePlayState_ComputePlayState now only cares about the presence of the session and allows playback of anything as long as it got play() command. HTMLMediaElement now controls the user gesture aspects of the background video playback. It also uses play() and pause() to implement the default MediaSession behavior. BUG=689637 TEST=manual+updated tests ========== to ========== [Video] MediaSession API event handlers can resume background video. ShouldPauseVideoWhenHidden is now used for videos with audio too so that HTMLMediaElement::play() actually reaches WMPI because the video is paused. WMPI::UpdatePlayState_ComputePlayState now only cares about the presence of the session and allows playback of anything as long as it got play() command. WMPI now tracks whether background video playback has been unlocked by a user gesture. RendererWebMediaPlayerDelegate emulates user gesture in OnPlay/OnPause to behave the same way as MediaSession. BUG=689637 TEST=manual+updated tests ==========
Added tests
This change is actually looking reasonable now. lgtm % still reviewing tests; and I want to know what your plan is for when we inevitably find a broken edge case. https://codereview.chromium.org/2681863005/diff/280001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2681863005/diff/280001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:423: if (blink::WebUserGestureIndicator::isProcessingUserGesture()) Should we be consuming the gesture? https://codereview.chromium.org/2681863005/diff/280001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:2014: (!hasVideo() || !is_backgrounded || !IsBackgroundedSuspendEnabled() || This last condition isn't obvious, please pull it out into a variable and comment it. Also please make sure the large block comment is updated; it's not obvious how eg. |have_metadata| is relevant when that variable doesn't exist anymore. (But you are depending on it here--|!hasVideo()| is always true before |have_metadata|.)
Updated the comment in ComputePlayState
Thanks for a quick turnaround! https://codereview.chromium.org/2681863005/diff/280001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2681863005/diff/280001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:423: if (blink::WebUserGestureIndicator::isProcessingUserGesture()) On 2017/02/23 at 00:38:31, sandersd wrote: > Should we be consuming the gesture? I don't think so, the page may call play() and pause() in an event handler as many times as it wants. IMO it'd be unexpected to have the first play() work and the rest stop working. https://codereview.chromium.org/2681863005/diff/280001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:2014: (!hasVideo() || !is_backgrounded || !IsBackgroundedSuspendEnabled() || On 2017/02/23 at 00:38:31, sandersd wrote: > This last condition isn't obvious, please pull it out into a variable and comment it. > > Also please make sure the large block comment is updated; it's not obvious how eg. |have_metadata| is relevant when that variable doesn't exist anymore. (But you are depending on it here--|!hasVideo()| is always true before |have_metadata|.) Done. |have_metadata| is implied by |has_future_data| so I removed it. Also renamed has_session to has_remote_controls as session is now also refers to the API.
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
avayvod@chromium.org changed required reviewers: - mlamouri@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm!
The CQ bit was checked by avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2681863005/#ps300001 (title: "Updated the comment in ComputePlayState")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 300001, "attempt_start_ts": 1487897595689930, "parent_rev": "56b4b085f77938133adbc5f80ea499821132f235", "commit_rev": "65fad2795b571f868c559941187f785aaa0414a0"}
Message was sent while issue was closed.
Description was changed from ========== [Video] MediaSession API event handlers can resume background video. ShouldPauseVideoWhenHidden is now used for videos with audio too so that HTMLMediaElement::play() actually reaches WMPI because the video is paused. WMPI::UpdatePlayState_ComputePlayState now only cares about the presence of the session and allows playback of anything as long as it got play() command. WMPI now tracks whether background video playback has been unlocked by a user gesture. RendererWebMediaPlayerDelegate emulates user gesture in OnPlay/OnPause to behave the same way as MediaSession. BUG=689637 TEST=manual+updated tests ========== to ========== [Video] MediaSession API event handlers can resume background video. ShouldPauseVideoWhenHidden is now used for videos with audio too so that HTMLMediaElement::play() actually reaches WMPI because the video is paused. WMPI::UpdatePlayState_ComputePlayState now only cares about the presence of the session and allows playback of anything as long as it got play() command. WMPI now tracks whether background video playback has been unlocked by a user gesture. RendererWebMediaPlayerDelegate emulates user gesture in OnPlay/OnPause to behave the same way as MediaSession. BUG=689637 TEST=manual+updated tests Review-Url: https://codereview.chromium.org/2681863005 Cr-Commit-Position: refs/heads/master@{#452700} Committed: https://chromium.googlesource.com/chromium/src/+/65fad2795b571f868c559941187f... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/65fad2795b571f868c559941187f... |