Player in background won't start when started from media notification |
||||||||||||||||
Issue descriptionUsing media controls to switch tracks does not work when the page is invisible.
,
Oct 24 2016
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
,
Oct 24 2016
,
Oct 24 2016
,
Oct 24 2016
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.
,
Oct 24 2016
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?
,
Oct 24 2016
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.
,
Oct 24 2016
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/
,
Oct 24 2016
>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.
,
Oct 24 2016
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.
,
Oct 24 2016
It's surprising that has_played_media() would be false in this case though. Load is allowed if media has been played before.
,
Oct 25 2016
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.
,
Oct 25 2016
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)
,
Oct 25 2016
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.
,
Oct 25 2016
I added "&& have_future_data" to the condition instead which fixed the issue for me. Dan, Dale, wdyt?
,
Oct 25 2016
+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.
,
Oct 26 2016
> 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.
,
Oct 26 2016
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?
,
Oct 26 2016
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.
,
Oct 26 2016
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.
,
Oct 26 2016
I think the fix in c#15 is correct. We can't just always background pause suspend, https://codereview.chromium.org/2449873006
,
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
,
Oct 27 2016
The patch did fix the issue. Thanks Dale :)
,
Oct 27 2016
Dale, what do you think about merging this to M-55?
,
Oct 27 2016
Your change meets the bar and is auto-approved for M55 (branch: 2883)
,
Oct 27 2016
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
,
Oct 27 2016
,
Oct 27 2016
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
,
Nov 4 2016
[Automated comment] removing mislabelled merge-merged-2840
,
Nov 4 2016
[Automated comment] removing mislabelled merge-merged-2840 |
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by zqzh...@chromium.org
, Oct 24 2016