Video doesn't resume upon bringing it to the foreground if done after a few seconds |
|||||||||
Issue descriptionVersion: Chrome 54 OS: Android LMY48I What steps will reproduce the problem? (1) Open a page with a video and play it (2) Background the tab (3) Observe the video paused, wait for 10 seconds (4) Tap on the notification to get back to the tab. ER: the video resumes AR: the video is suspended, however, the notification and media element controls are in the playing state.
,
Sep 1 2016
Can only reproduce with Spitzer.
,
Sep 1 2016
Reproduced with a src= mp4 video.
,
Sep 1 2016
I'd guess we're not resuming for some reason, possibly it is rejecting the resume due to being hidden.
,
Sep 1 2016
It does resume if I bring the tab back fast enough. Could it be the background pause timer? https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=1472733616&l=1217
,
Sep 1 2016
That's just a normal pause, so clicking the media notification to unpause should work.
,
Sep 1 2016
It doesn't actually trigger for a paused audible video with notification since the delegate state is not GONE in this case. Will keep digging.
,
Sep 1 2016
It seems like we prefer to pause media when it's idle in the background for a while (to preserve resources and avoid autoplay effect when the tab is not immediately foregrounded). This is achieved by idle players cleanup in RendererWebMediaPlayerDelegate. However, in this case, the audio would be paused explicitly by user via notification, so the element and the delegate would be in the right state and would not show the audio as playing when foregrounded. For video, we pretend it's still playing to the web page but not to the delegate (notification) so that if the video is foregrounded it will keep playing. If we want to preserve the idle video cleanup, we should probably call pause() to sync the HTMLMediaElement's state. Otherwise, we should probably not suspend idle audible video players on Android. +Dan, ComputePlayState expert. WDYT?
,
Sep 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e7120dcf27938c9604ec03b3b574e1b4b3bf6c28 commit e7120dcf27938c9604ec03b3b574e1b4b3bf6c28 Author: dalecurtis <dalecurtis@chromium.org> Date: Sat Sep 03 02:54:35 2016 Allow suspension prior to reaching kHaveFutureData. This change rolls in a few changes: - Removes DelegateState::PAUSED_BUT_NOT_IDLE state. - Removes all blocks against suspending prior to kHaveFutureData. - If suspended, triggers resume when prior to have kHaveFutureData if the didLoadingProgress() timer reports true. - Always notifies the delegate of DelegateState::GONE; this ensures the idle timer is always restarted if we didn't quite get enough data to reach kHaveFutureData. - Fixes pause timer to handle newly supported background video; which doesn't set a state of DelegateState::GONE BUG= 612909 , 643383 TEST=new and updated unittests. Review-Url: https://codereview.chromium.org/2289543005 Cr-Commit-Position: refs/heads/master@{#416431} [modify] https://crrev.com/e7120dcf27938c9604ec03b3b574e1b4b3bf6c28/content/renderer/media/renderer_webmediaplayer_delegate.cc [modify] https://crrev.com/e7120dcf27938c9604ec03b3b574e1b4b3bf6c28/content/renderer/media/renderer_webmediaplayer_delegate_browsertest.cc [modify] https://crrev.com/e7120dcf27938c9604ec03b3b574e1b4b3bf6c28/media/blink/webmediaplayer_impl.cc [modify] https://crrev.com/e7120dcf27938c9604ec03b3b574e1b4b3bf6c28/media/blink/webmediaplayer_impl.h [modify] https://crrev.com/e7120dcf27938c9604ec03b3b574e1b4b3bf6c28/media/blink/webmediaplayer_impl_unittest.cc
,
Sep 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4e8a2b9aabbc3fce83dcd99a57ed7f5b0e633ffe commit 4e8a2b9aabbc3fce83dcd99a57ed7f5b0e633ffe Author: sandersd <sandersd@chromium.org> Date: Sat Sep 03 03:04:14 2016 PipelineController: Fix definition of IsSuspended(). BUG= 643383 Review-Url: https://codereview.chromium.org/2308893002 Cr-Commit-Position: refs/heads/master@{#416432} [modify] https://crrev.com/4e8a2b9aabbc3fce83dcd99a57ed7f5b0e633ffe/media/filters/pipeline_controller.cc [modify] https://crrev.com/4e8a2b9aabbc3fce83dcd99a57ed7f5b0e633ffe/media/filters/pipeline_controller_unittest.cc
,
Sep 6 2016
Thanks Dale & Dan, the issue is fixed by the last two changes. Is this something that could be merged to M54?
,
Sep 6 2016
Your change meets the bar and is auto-approved for M54 (branch: 2840)
,
Sep 6 2016
Anton, we weren't planning to merge these fixes back to M-54. They're part of a pretty long chain of fixes that dramatically change how we suspend / seek in the media pipeline. I'm not 100% sure they can be merged as is w/o consequence. I.e. I think we might need to merge issue 638018, issue 642757, and issue 643441 along with these patches. The issue is pages like vimeo.com which start mse players that never reach the decode state and thus don't suspend properly without the fixes above. It's possible just these two patches won't make the situation any worse. If you're willing to do some extra testing with just those two merges, we can take them early. Alternatively we can set this soak for a couple weeks (at least until M55 android dev) and then take them all back if things still look good.
,
Sep 6 2016
What kind of extra testing we're talking about? :) Could I just build an APK with the changes merged and send to someone on the QA team? I'd be fine with the second approach if I knew ahead of time we would merge the changes :) Otherwise, we'd have to look for a smaller fix for this issue in particular.
,
Sep 6 2016
We can merge the change in a couple weeks if everything still looks good. They fix a couple crashes in M-54 so they're worth taking if possible.
,
Sep 7 2016
[Bulk edit] This issue has been approved for a merge to M54 branch 2840. Please try to complete the merge by tomorrow at 5 PM PT if at all possible. If this has already been merged and this message is in error, please remove the label Merge-Approved-54. Cheers, Alex
,
Sep 10 2016
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 14 2016
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 14 2016
Still no Android dev build.
,
Sep 19 2016
able to repro in M55-55.0.2863.0 build. Looks like fix not merged into M55/M54 builds
,
Sep 19 2016
video at go/chrome-androidlogs1/6/643383 (1) Open a page with a video and play it (2) Background the tab (3) Observe the video paused, wait for 10 seconds (4) Tap on the notification to get back to the tab. Observed behavior -the video is suspended and the notification and media element controls are in the Paused state only
,
Sep 19 2016
Chatted with kravula@ the behavior is working as intended. You must click the play button to resume video in background. Simply clicking the notification is not sufficient if the video has been idle for some time.
,
Sep 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4c621e70b04fd5ae89a95d58c10d040c57af9f12 commit 4c621e70b04fd5ae89a95d58c10d040c57af9f12 Author: Dale Curtis <dalecurtis@chromium.org> Date: Tue Sep 20 23:50:51 2016 Merge M54: "Allow suspension prior to reaching kHaveFutureData." This change rolls in a few changes: - Removes DelegateState::PAUSED_BUT_NOT_IDLE state. - Removes all blocks against suspending prior to kHaveFutureData. - If suspended, triggers resume when prior to have kHaveFutureData if the didLoadingProgress() timer reports true. - Always notifies the delegate of DelegateState::GONE; this ensures the idle timer is always restarted if we didn't quite get enough data to reach kHaveFutureData. - Fixes pause timer to handle newly supported background video; which doesn't set a state of DelegateState::GONE BUG= 612909 , 643383 TEST=new and updated unittests. Review-Url: https://codereview.chromium.org/2289543005 Cr-Commit-Position: refs/heads/master@{#416431} (cherry picked from commit e7120dcf27938c9604ec03b3b574e1b4b3bf6c28) Review URL: https://codereview.chromium.org/2355143002 . Cr-Commit-Position: refs/branch-heads/2840@{#451} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/4c621e70b04fd5ae89a95d58c10d040c57af9f12/content/renderer/media/renderer_webmediaplayer_delegate.cc [modify] https://crrev.com/4c621e70b04fd5ae89a95d58c10d040c57af9f12/content/renderer/media/renderer_webmediaplayer_delegate_browsertest.cc [modify] https://crrev.com/4c621e70b04fd5ae89a95d58c10d040c57af9f12/media/blink/webmediaplayer_impl.cc [modify] https://crrev.com/4c621e70b04fd5ae89a95d58c10d040c57af9f12/media/blink/webmediaplayer_impl.h [modify] https://crrev.com/4c621e70b04fd5ae89a95d58c10d040c57af9f12/media/blink/webmediaplayer_impl_unittest.cc
,
Sep 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b2ce911ccf12bce3d43aa10d5994b94fa116042f commit b2ce911ccf12bce3d43aa10d5994b94fa116042f Author: Dale Curtis <dalecurtis@chromium.org> Date: Wed Sep 21 00:10:58 2016 Merge M54: "PipelineController: Fix definition of IsSuspended()." BUG= 643383 Review-Url: https://codereview.chromium.org/2308893002 Cr-Commit-Position: refs/heads/master@{#416432} (cherry picked from commit 4e8a2b9aabbc3fce83dcd99a57ed7f5b0e633ffe) Review URL: https://codereview.chromium.org/2357033002 . Cr-Commit-Position: refs/branch-heads/2840@{#455} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/b2ce911ccf12bce3d43aa10d5994b94fa116042f/media/filters/pipeline_controller.cc [modify] https://crrev.com/b2ce911ccf12bce3d43aa10d5994b94fa116042f/media/filters/pipeline_controller_unittest.cc
,
Sep 21 2016
,
Sep 21 2016
Verified in M54-54.0.2840.34 build
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4c621e70b04fd5ae89a95d58c10d040c57af9f12 commit 4c621e70b04fd5ae89a95d58c10d040c57af9f12 Author: Dale Curtis <dalecurtis@chromium.org> Date: Tue Sep 20 23:50:51 2016 Merge M54: "Allow suspension prior to reaching kHaveFutureData." This change rolls in a few changes: - Removes DelegateState::PAUSED_BUT_NOT_IDLE state. - Removes all blocks against suspending prior to kHaveFutureData. - If suspended, triggers resume when prior to have kHaveFutureData if the didLoadingProgress() timer reports true. - Always notifies the delegate of DelegateState::GONE; this ensures the idle timer is always restarted if we didn't quite get enough data to reach kHaveFutureData. - Fixes pause timer to handle newly supported background video; which doesn't set a state of DelegateState::GONE BUG= 612909 , 643383 TEST=new and updated unittests. Review-Url: https://codereview.chromium.org/2289543005 Cr-Commit-Position: refs/heads/master@{#416431} (cherry picked from commit e7120dcf27938c9604ec03b3b574e1b4b3bf6c28) Review URL: https://codereview.chromium.org/2355143002 . Cr-Commit-Position: refs/branch-heads/2840@{#451} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/4c621e70b04fd5ae89a95d58c10d040c57af9f12/content/renderer/media/renderer_webmediaplayer_delegate.cc [modify] https://crrev.com/4c621e70b04fd5ae89a95d58c10d040c57af9f12/content/renderer/media/renderer_webmediaplayer_delegate_browsertest.cc [modify] https://crrev.com/4c621e70b04fd5ae89a95d58c10d040c57af9f12/media/blink/webmediaplayer_impl.cc [modify] https://crrev.com/4c621e70b04fd5ae89a95d58c10d040c57af9f12/media/blink/webmediaplayer_impl.h [modify] https://crrev.com/4c621e70b04fd5ae89a95d58c10d040c57af9f12/media/blink/webmediaplayer_impl_unittest.cc
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b2ce911ccf12bce3d43aa10d5994b94fa116042f commit b2ce911ccf12bce3d43aa10d5994b94fa116042f Author: Dale Curtis <dalecurtis@chromium.org> Date: Wed Sep 21 00:10:58 2016 Merge M54: "PipelineController: Fix definition of IsSuspended()." BUG= 643383 Review-Url: https://codereview.chromium.org/2308893002 Cr-Commit-Position: refs/heads/master@{#416432} (cherry picked from commit 4e8a2b9aabbc3fce83dcd99a57ed7f5b0e633ffe) Review URL: https://codereview.chromium.org/2357033002 . Cr-Commit-Position: refs/branch-heads/2840@{#455} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/b2ce911ccf12bce3d43aa10d5994b94fa116042f/media/filters/pipeline_controller.cc [modify] https://crrev.com/b2ce911ccf12bce3d43aa10d5994b94fa116042f/media/filters/pipeline_controller_unittest.cc |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by avayvod@chromium.org
, Sep 1 2016