New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 689637 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Resuming background video playback on Android is broken when using the MediaSession API

Project Member Reported by avayvod@chromium.org, Feb 7 2017

Issue description

1. 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.
 
On a first glance, it seems we need to somehow set RenderWebMediaPlayerImpl::background_video_allowed_ to true in case of play() with a user gesture.

Or make the delegate be aware of the MediaSession events coming through and allow bg playback during that time.
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...
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.
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 :/
Issue 692983 has been merged into this issue.
WIP patch is here: https://codereview.chromium.org/2681863005
Project Member

Comment 7 by bugdroid1@chromium.org, 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

Labels: Merge-Request-57
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.
Project Member

Comment 9 by sheriffbot@chromium.org, Feb 24 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
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
Project Member

Comment 10 by bugdroid1@chromium.org, Feb 24 2017

Labels: -merge-approved-57 merge-merged-2987
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

Status: Fixed (was: Assigned)
Status: Verified (was: Fixed)
Verified in 57.0.2987.88 build

Sign in to add a comment