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

Issue 658680 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Player in background won't start when started from media notification

Project Member Reported by zqzh...@chromium.org, Oct 24 2016

Issue description

Using media controls to switch tracks does not work when the page is invisible.
 
Status: Available (was: Untriaged)
Summary: Player in background won't start when started from media notification (was: MediaSession controls is not sent to background pages)
Turns out to be we are blocking players to start when the page is invisible.
The page did receive the event but the play request is pending.

Need to look into WMPL
Owner: zqzh...@chromium.org
Description: Show this description
Cc: dalecur...@chromium.org
I haven't found anything yet. Dale, do you have any quick answer?

So basically, we have an <audio> playing on a background page, and then the page calls:

audio.src = "<new-src>";
audio.play();  // Gesture requirement fulfilled

The playback does not begin until the page becomes visible again.
Do you have a repro page?
I think we have a browser intervention for play() although thought it was desktop only?
Or was it for autoplay only?
BTW, I've checked the network log, the new audio has been downloaded already.
Seems like we haven't received readyState HAVE_ENOUGH_DATA before the page becomes visible.
I have a test page, but it only works on my local build (since I only have next/previous track in the notification locally).

http://xxyzzzq.github.io/sandbox/media-session/full-test.html

You could apply the following CLs to make the build (also, you need --enable-blink-features=MediaSession):
https://codereview.chromium.org/2442303002/
https://codereview.chromium.org/2441403002/

>I think we have a browser intervention for play() although thought it was desktop only?
>Or was it for autoplay only?

Did you mean the autoplay muted video starts playing until visible?

I think it's unrelated, since the test page only have audio.
This is likely due to load() being blocked in the background. This is the same behavior we have on desktop and required to ensure that 'prerender' type links don't start playing incorrectly.

https://cs.chromium.org/chromium/src/chrome/renderer/chrome_content_renderer_client.cc?l=602

This is handled by the defer_load_cb in the WebMediaPlayers.
It's surprising that has_played_media() would be false in this case though. Load is allowed if media has been played before.
Cc: sande...@chromium.org
I found the reason why it does not play. It's not related to has_played_media(). I checked the log and it is true at the moment.

So the reason is in WMPL::UpdatePlayState_ComputePlayState. The method computes the player state.
The following line makes the new state suspended:
  bool background_pause_suspended = is_backgrounded && paused_;
since |is_backgrounded| and |paused_| are both true at the moment.

However, if we do play/pause in the media notification while the page is in background, |paused_| never changes (seems like a bug?)

We should refine the logic. We could probably have a "suspend lock" and set it when going background. When the player to allowed play (either background video playback or background audio), we unlock it so there are no further restrictions. Or maybe we could remove this check since we checked has_played_media() already.

I'm not very familiar with suspend logic, maybe you have better ideas.
I confirm that DoLoad is called, however I don't see play() called ever when the page is backgrounded. This is what WMPI logs show when the page is in the foreground and I play the first audio:

webmediaplayer_impl.cc(545)] setVolume(1)
webmediaplayer_impl.cc(575)] setPreload(2)
webmediaplayer_impl.cc(302)] load(0, http://xxyzzzq.github.io/sandbox/media-session/media/acousticbreeze.mp3, 0)
chrome_content_renderer_client.cc(606)] DeferMediaLoad: 0
webmediaplayer_impl.cc(359)] DoLoad
webmediaplayer_impl.cc(1565)] SetNetworkState(2)
webmediaplayer_impl.cc(1573)] SetReadyState(0)
webmediaplayer_impl.cc(1447)] NotifyDownloading
webmediaplayer_impl.cc(1447)] NotifyDownloading
webmediaplayer_impl.cc(1413)] DataSourceInitialized
webmediaplayer_impl.cc(1118)] OnMetadata
webmediaplayer_impl.cc(1573)] SetReadyState(1)
webmediaplayer_impl.cc(1150)] OnBufferingStateChange(1)
webmediaplayer_impl.cc(1573)] SetReadyState(4)
webmediaplayer_impl.cc(520)] setRate(1)
webmediaplayer_impl.cc(545)] setVolume(1)
webmediaplayer_impl.cc(398)] play
webmediaplayer_impl.cc(545)] setVolume(1)
webmediaplayer_impl.cc(1447)] NotifyDownloading
webmediaplayer_impl.cc(1565)] SetNetworkState(1)

And here's what I see when I background the page and tap the next track button:

webmediaplayer_impl.cc(545)] setVolume(1)
webmediaplayer_impl.cc(575)] setPreload(2)
webmediaplayer_impl.cc(302)] load(0, http://xxyzzzq.github.io/sandbox/media-session/media/buddy.mp3, 0)
chrome_content_renderer_client.cc(606)] DeferMediaLoad: 1
webmediaplayer_impl.cc(359)] DoLoad
webmediaplayer_impl.cc(1565)] SetNetworkState(2)
webmediaplayer_impl.cc(1573)] SetReadyState(0)
webmediaplayer_impl.cc(1447)] NotifyDownloading
webmediaplayer_impl.cc(1447)] NotifyDownloading
webmediaplayer_impl.cc(1413)] DataSourceInitialized
webmediaplayer_impl.cc(1118)] OnMetadata
webmediaplayer_impl.cc(1573)] SetReadyState(1)
.... missing events ....
webmediaplayer_impl.cc(1447)] NotifyDownloading
webmediaplayer_impl.cc(1565)] SetNetworkState(1)


Anton, maybe you could try changing this line:

bool background_pause_suspended = is_backgrounded && paused_;

to:

bool background_pause_suspended = true;

to bypass the check. It should work.
I added "&& have_future_data" to the condition instead which fixed the issue for me.

Dan, Dale, wdyt?
Cc: w...@chromium.org
+watk who is working on a similar issue i think. it's possible the player is being incorrectly idle suspended and the idle suspend should instead be deferred slightly until the demuxer is actually idle.

i'm surprised to see it idle suspend so quickly after a new tag is created though, so i'd guess something stranger is happening.
> I added "&& have_future_data" to the condition instead which fixed the issue for me.

Found the code was exactly what you said in the past, but was changed in this CL:
https://codereview.chromium.org/2289543005

I'm still trying to understand the logic.
Found out why play/pause from the notification works but invoking play() does not.

When play/pause from the notification, |WMPI::paused_| is set as true in OnPlay() when receiving the MediaPlayerDelegateMsg_Pause IPC message, which happens before ComputePlayState() is called.

When play() from the page, |WMPI::paused_| is false at the moment, so it won't play.

Basically we are implementing an API that adds previous/next track in Android media notification to allow the page to switch tracks.
So I think there are two options we can do:
1) Loosen the restriction for play, or
2) Pass a flag from MediaSession to WMPI to tell we pressed previous/next track and allow the player to start.

FWIW 1 is better. But maybe there are better options. WDYT?
Labels: M-55
Owner: sande...@chromium.org
Hm, Zhiqiang, can you confirm play/pause from the notification works and only prev/next track doesn't? That's what I'm seeing.

When the page handles prev/next track it calls play() on HTMLMediaElement which waits until there's enough data to start playback. Unfortunately, WMPI::UpdatePlayState_ComputePlayState suspends the pipeline since the media is paused and backgrounded. As a result, no data is being downloaded so HTMLMediaElement never calls play() on WMPI.

So we probably don't need to suspend media pipeline in WMPI if it has more than no data at all but not enough data yet (or whatever condition HTMLMediaElement uses to request playback start).

I've created a smaller version of the page that allows reproducing the bug without patches and experimental Web APIs: http://avayvod.github.io/media/background-switch-media.html

Next/prev track fire with a timeout of 5s so one can background Chrome before the track is actually switched to reproduce. I can reproduce in Dev and Stable.
BTW, thanks for the test page.

> Hm, Zhiqiang, can you confirm play/pause from the notification works and only prev/next track doesn't? That's what I'm seeing.

Correct, this is also what I see.

You are right, it's not because whether play() is called from the page or from content/browser. I tried delaying play() on the page while not switching track and it works. So it should be a data issue.

Cc: -dalecur...@chromium.org
Owner: dalecur...@chromium.org
I think the fix in c#15 is correct. We can't just always background pause suspend, https://codereview.chromium.org/2449873006 
Project Member

Comment 22 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc8baf72a3cfc63b1bd39710b8999b04508e43c8

commit cc8baf72a3cfc63b1bd39710b8999b04508e43c8
Author: dalecurtis <dalecurtis@chromium.org>
Date: Thu Oct 27 01:49:44 2016

Don't background pause suspend until we have future data.

Otherwise the pause state is not reliable and we may incorrectly
prevent background playback attempts.

We can't always force background pause suspend since background
playback may trigger a play() but we don't know the correct paused
state yet.

BUG= 658680 
TEST=new test.

Review-Url: https://codereview.chromium.org/2449873006
Cr-Commit-Position: refs/heads/master@{#427910}

[modify] https://crrev.com/cc8baf72a3cfc63b1bd39710b8999b04508e43c8/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/cc8baf72a3cfc63b1bd39710b8999b04508e43c8/media/blink/webmediaplayer_impl_unittest.cc

Status: Fixed (was: Available)
The patch did fix the issue. Thanks Dale :)
Labels: Merge-Request-55
Status: Started (was: Fixed)
Dale, what do you think about merging this to M-55?

Comment 25 by dimu@chromium.org, Oct 27 2016

Labels: -Merge-Request-55 Merge-Approved-55 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M55 (branch: 2883)
Project Member

Comment 26 by bugdroid1@chromium.org, Oct 27 2016

Labels: -merge-approved-55 merge-merged-2883
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0ed1f1da97ec0eda271fc183d738de4018d0d0c9

commit 0ed1f1da97ec0eda271fc183d738de4018d0d0c9
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Thu Oct 27 18:12:05 2016

Merge M55: "Don't background pause suspend until we have future data."

Otherwise the pause state is not reliable and we may incorrectly
prevent background playback attempts.

We can't always force background pause suspend since background
playback may trigger a play() but we don't know the correct paused
state yet.

BUG= 658680 
TEST=new test.

Review-Url: https://codereview.chromium.org/2449873006
Cr-Commit-Position: refs/heads/master@{#427910}
(cherry picked from commit cc8baf72a3cfc63b1bd39710b8999b04508e43c8)

Review URL: https://codereview.chromium.org/2456613003 .

Cr-Commit-Position: refs/branch-heads/2883@{#340}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/0ed1f1da97ec0eda271fc183d738de4018d0d0c9/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/0ed1f1da97ec0eda271fc183d738de4018d0d0c9/media/blink/webmediaplayer_impl_unittest.cc

Status: Fixed (was: Started)
Project Member

Comment 28 by bugdroid1@chromium.org, Oct 27 2016

Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0ed1f1da97ec0eda271fc183d738de4018d0d0c9

commit 0ed1f1da97ec0eda271fc183d738de4018d0d0c9
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Thu Oct 27 18:12:05 2016

Merge M55: "Don't background pause suspend until we have future data."

Otherwise the pause state is not reliable and we may incorrectly
prevent background playback attempts.

We can't always force background pause suspend since background
playback may trigger a play() but we don't know the correct paused
state yet.

BUG= 658680 
TEST=new test.

Review-Url: https://codereview.chromium.org/2449873006
Cr-Commit-Position: refs/heads/master@{#427910}
(cherry picked from commit cc8baf72a3cfc63b1bd39710b8999b04508e43c8)

Review URL: https://codereview.chromium.org/2456613003 .

Cr-Commit-Position: refs/branch-heads/2883@{#340}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/0ed1f1da97ec0eda271fc183d738de4018d0d0c9/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/0ed1f1da97ec0eda271fc183d738de4018d0d0c9/media/blink/webmediaplayer_impl_unittest.cc

Comment 29 by dimu@google.com, Nov 4 2016

[Automated comment] removing mislabelled merge-merged-2840

Comment 30 by dimu@google.com, Nov 4 2016

Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840

Sign in to add a comment