Resuming background video playback on Android is broken when using the MediaSession API |
||||||
Issue description1. Go to https://xxyzzzq.github.io/sandbox/media-session/full-test-video.html 2. Play the video and background the tab. 3. Click on the play button in the notification or lock screen to resume the video ER: the video resumes OR: the video doesn't resume This breaks an existing feature so I consider it to be a stable blocker.
,
Feb 8 2017
Hm, I tried to just check if there's a user gesture being processed in WMPI::UpdatePlayState_ComputePlayState and ORed it with delegate_->IsBackgroundVideoPlaybackAllowed(). It didn't help however, seems like HTMLMediaElement::play() is not always on the same call stack as WMPI::UpdatePlayState_ComputePlayState() that actually resumes the playback. Logging shows that sometimes WebUserGestureIndicator::IsProcessingUserGesture() returns false in WMPI::UPS_CPS but true in HTMLMediaElement::play() so the video is not allowed to resume but sometimes the method returns true but WMPI doesn't resume playback (presumably because UPS_CPS() is called a few times when play() is called on HTMLMediaElement) so background playback doesn't resume still...
,
Feb 8 2017
Ah, no, that actually works as expected. RenderWebMediaPlayerDelegate didn't update background_video_allowed_ in response to DidPlay in this case so subsequent WMPI::UPS_CPS would suspend the video.
,
Feb 8 2017
Except that WMPI doesn't really pause the video when backgrounded on Android so play() from the page doesn't actually call WMPI play() until background pause timer kicks in and pauses the video for real. Then I can resume background playback using the notification and the new MediaSession path. Seems like on Android we need to honestly pause the video when hidden, however that interleaves with the bg playback optimization logic now :/
,
Feb 16 2017
Issue 692983 has been merged into this issue.
,
Feb 22 2017
WIP patch is here: https://codereview.chromium.org/2681863005
,
Feb 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/65fad2795b571f868c559941187f785aaa0414a0 commit 65fad2795b571f868c559941187f785aaa0414a0 Author: avayvod <avayvod@chromium.org> Date: Fri Feb 24 01:00:48 2017 [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} [modify] https://crrev.com/65fad2795b571f868c559941187f785aaa0414a0/content/browser/media/session/media_session_impl_visibility_browsertest.cc [modify] https://crrev.com/65fad2795b571f868c559941187f785aaa0414a0/content/renderer/media/renderer_webmediaplayer_delegate.cc [modify] https://crrev.com/65fad2795b571f868c559941187f785aaa0414a0/content/renderer/media/renderer_webmediaplayer_delegate.h [modify] https://crrev.com/65fad2795b571f868c559941187f785aaa0414a0/content/renderer/media/renderer_webmediaplayer_delegate_browsertest.cc [modify] https://crrev.com/65fad2795b571f868c559941187f785aaa0414a0/content/renderer/media/webmediaplayer_ms_unittest.cc [modify] https://crrev.com/65fad2795b571f868c559941187f785aaa0414a0/media/blink/webmediaplayer_delegate.h [modify] https://crrev.com/65fad2795b571f868c559941187f785aaa0414a0/media/blink/webmediaplayer_impl.cc [modify] https://crrev.com/65fad2795b571f868c559941187f785aaa0414a0/media/blink/webmediaplayer_impl.h [modify] https://crrev.com/65fad2795b571f868c559941187f785aaa0414a0/media/blink/webmediaplayer_impl_unittest.cc
,
Feb 24 2017
After an offline discussion, Mounir and I decided that if the change breaks some unnoticed edge case in 57, we would roll back the merge of the CL and let the bug exist in 57. Disabling background playback if the site uses MediaSession was suggested but it would be yet another risky change to write and merge.
,
Feb 24 2017
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a8ce00278da90b8abbdf39b0f74ddbb417e0035e commit a8ce00278da90b8abbdf39b0f74ddbb417e0035e Author: Anton Vayvod <avayvod@google.com> Date: Fri Feb 24 21:51:01 2017 [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} (cherry picked from commit 65fad2795b571f868c559941187f785aaa0414a0) R=sandersd@chromium.org Review-Url: https://codereview.chromium.org/2705213007 . Cr-Commit-Position: refs/branch-heads/2987@{#682} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/a8ce00278da90b8abbdf39b0f74ddbb417e0035e/content/browser/media/session/media_session_impl_visibility_browsertest.cc [modify] https://crrev.com/a8ce00278da90b8abbdf39b0f74ddbb417e0035e/content/renderer/media/android/webmediaplayer_android.cc [modify] https://crrev.com/a8ce00278da90b8abbdf39b0f74ddbb417e0035e/content/renderer/media/android/webmediaplayer_android.h [modify] https://crrev.com/a8ce00278da90b8abbdf39b0f74ddbb417e0035e/content/renderer/media/renderer_webmediaplayer_delegate.cc [modify] https://crrev.com/a8ce00278da90b8abbdf39b0f74ddbb417e0035e/content/renderer/media/renderer_webmediaplayer_delegate.h [modify] https://crrev.com/a8ce00278da90b8abbdf39b0f74ddbb417e0035e/content/renderer/media/renderer_webmediaplayer_delegate_browsertest.cc [modify] https://crrev.com/a8ce00278da90b8abbdf39b0f74ddbb417e0035e/content/renderer/media/webmediaplayer_ms_unittest.cc [modify] https://crrev.com/a8ce00278da90b8abbdf39b0f74ddbb417e0035e/media/blink/webmediaplayer_delegate.h [modify] https://crrev.com/a8ce00278da90b8abbdf39b0f74ddbb417e0035e/media/blink/webmediaplayer_impl.cc [modify] https://crrev.com/a8ce00278da90b8abbdf39b0f74ddbb417e0035e/media/blink/webmediaplayer_impl.h [modify] https://crrev.com/a8ce00278da90b8abbdf39b0f74ddbb417e0035e/media/blink/webmediaplayer_impl_unittest.cc
,
Feb 24 2017
,
Mar 2 2017
Verified in 57.0.2987.88 build |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by avayvod@chromium.org
, Feb 7 2017