Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2039793005:
Don't resume paused media. Don't resume playing media after timeout. (Closed)
|
Created:
4 years, 6 months ago by DaleCurtis Modified:
4 years, 6 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't resume paused media. Don't resume playing media after timeout.
As titled. We shouldn't resume paused media because there's no
reason to not wait for the user interaction. We shouldn't resume
playing media if the user has moved on to other tasks and comes
back some time later.
The idle timeout for resuming playing media is 5 seconds as chosen
by a dice roll.
BUG=617815
TEST=media suspends and resumes as expected.
Committed: https://crrev.com/8b8505e78452b365b861025d7b3364ca4b1e5849
Cr-Commit-Position: refs/heads/master@{#399287}
Patch Set 1 #Patch Set 2 : Ensure OnHidden() actually happened. #
Total comments: 4
Patch Set 3 : Reimplement. Add tests. #
Total comments: 8
Patch Set 4 : Comments. #Patch Set 5 : Comments. #
Total comments: 4
Patch Set 6 : Condense. #Patch Set 7 : Remove now incorrect test. #
Messages
Total messages: 25 (9 generated)
dalecurtis@chromium.org changed reviewers: + sandersd@chromium.org, watk@chromium.org
This exposes a crash with the deferred renderer, something to do with the copied image. Abort message: '[FATAL:android_deferred_rendering_backing_strategy.cc(389)] Check failed: static_cast<GLenum>(0x0) == ::gl::g_current_gl_context_tls->Get()->glGetErrorFn() (0 vs. 1282) Doesn't happen with the copying strategy and will go away after watk@ removes this path. +watk in case he wants to take a look and see what's going on.
(Confirmed no problem w/ watk@'s patch to kill copying)
https://codereview.chromium.org/2039793005/diff/20001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2039793005/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1117: // this here since |delegate_state_| is generally cleared during suspend. It's worth noting here that PAUSED_BUT_NOT_IDLE is purposefully excluded. https://codereview.chromium.org/2039793005/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1131: // and will get into a bad state if we try and suspend. "try to suspend." https://codereview.chromium.org/2039793005/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1132: if (!paused_ && !seeking_ && !last_hidden_time_.is_null() && If this is intended to match DelegateState::PAUSED, there are a few more conditions that UpdatePlayState() checks: - !IsNetworkStateError(network_state_) We probably shouldn't change the player state when there is an error. - !isRemote() Same for casting. - highest_ready_state_ >= WebMediaPlayer::ReadyStateHaveFutureData We want to resume if this isn't true yet. - !must_suspend_ Presumably we don't get OnShown() while the tab is still closed? - ready_state_ >= WebMediaPlayer::ReadyStateHaveMetadata (Just a clarifies the hasVideo() check, not actually required.) - hasVideo() We don't want to be pausing audio that is playing. - !seeking() This is the same as |seeking_| as long as the |ready_state_| check passes. https://codereview.chromium.org/2039793005/diff/20001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1135: OnPause(); Since OnPause() calls UpdatePlayState(), it's probably better to set |is_idle_| first.
Patchset #3 (id:40001) has been deleted
Redone as we discussed, please take a look.
https://codereview.chromium.org/2039793005/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2039793005/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1119: if (pipeline_controller_.IsSuspended() && !paused_) { We shouldn't schedule this while casting either. Probably the timer callback should be through a helper that checks the conditions, so that we don't need to think about whether the conditions can change. I'm also not sure if we want to be setting a |paused_time_| before HAVE_METADATA. It looks like playbackStateChanged() reads the paused state even before HAVE_FUTURE_DATA, so it looks like any call (even without your changes) results in the media becoming paused. Perhaps a larger bug? https://codereview.chromium.org/2039793005/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1532: bool background_audio_suspended = is_backgrounded && have_metadata && paused_; Should be |have_future_data| as a prerequisite for inspecting |paused_|. To confirm: this will result in the delegate state becoming GONE for backgrounded paused audio, so the system-level play control will disappear upon backgrounding. Is that the intended behavior? https://codereview.chromium.org/2039793005/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1552: if (is_suspended && !result.is_suspended && paused_) Better to move all the conditions up so that the assignment is just |result.is_suspended = true|. Perhaps there is a computation that can be pulled out and shared below?
https://codereview.chromium.org/2039793005/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2039793005/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1119: if (pipeline_controller_.IsSuspended() && !paused_) { On 2016/06/08 at 23:16:27, sandersd wrote: > We shouldn't schedule this while casting either. Probably the timer callback should be through a helper that checks the conditions, so that we don't need to think about whether the conditions can change. > > I'm also not sure if we want to be setting a |paused_time_| before HAVE_METADATA. > > It looks like playbackStateChanged() reads the paused state even before HAVE_FUTURE_DATA, so it looks like any call (even without your changes) results in the media becoming paused. Perhaps a larger bug? Done. I don't understand your concerns about HAVE_METADATA though, we must have reached that state to be in the playing state, so we should be fine to call pause (which could have happened from JS anyways). https://codereview.chromium.org/2039793005/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1532: bool background_audio_suspended = is_backgrounded && have_metadata && paused_; On 2016/06/08 at 23:16:27, sandersd wrote: > Should be |have_future_data| as a prerequisite for inspecting |paused_|. > > To confirm: this will result in the delegate state becoming GONE for backgrounded paused audio, so the system-level play control will disappear upon backgrounding. Is that the intended behavior? That's not what will happen since background_audio_suspended is not considered in the has_session calculation, just the is_suspended one. I added tests explicitly for this case. https://codereview.chromium.org/2039793005/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1552: if (is_suspended && !result.is_suspended && paused_) On 2016/06/08 at 23:16:27, sandersd wrote: > Better to move all the conditions up so that the assignment is just |result.is_suspended = true|. Perhaps there is a computation that can be pulled out and shared below? I prefer the current phrasing for readability, but will change if you feel strongly.
https://codereview.chromium.org/2039793005/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2039793005/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1552: if (is_suspended && !result.is_suspended && paused_) On 2016/06/10 00:02:11, DaleCurtis wrote: > On 2016/06/08 at 23:16:27, sandersd wrote: > > Better to move all the conditions up so that the assignment is just > |result.is_suspended = true|. Perhaps there is a computation that can be pulled > out and shared below? > > I prefer the current phrasing for readability, but will change if you feel > strongly. I feel strongly because the this version doesn't make it trivially obvious that this will never unset |result.is_suspended|.
https://codereview.chromium.org/2039793005/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2039793005/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:1552: if (is_suspended && !result.is_suspended && paused_) On 2016/06/10 at 01:15:09, sandersd wrote: > On 2016/06/10 00:02:11, DaleCurtis wrote: > > On 2016/06/08 at 23:16:27, sandersd wrote: > > > Better to move all the conditions up so that the assignment is just > > |result.is_suspended = true|. Perhaps there is a computation that can be pulled > > out and shared below? > > > > I prefer the current phrasing for readability, but will change if you feel > > strongly. > > I feel strongly because the this version doesn't make it trivially obvious that this will never unset |result.is_suspended|. Done.
lgtm https://codereview.chromium.org/2039793005/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2039793005/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1529: // The |paused_| state is not reliable until we |have_future_data|. Newline before. https://codereview.chromium.org/2039793005/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1552: !seeking_) { Doesn't actually need the |!result.is_suspended| condition. With that change it actually looks exactly like the other variables. I'll let you decide if it should be turned into one.
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by dalecurtis@chromium.org
https://codereview.chromium.org/2039793005/diff/100001/media/blink/webmediapl... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2039793005/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1529: // The |paused_| state is not reliable until we |have_future_data|. On 2016/06/10 at 18:27:12, sandersd wrote: > Newline before. Done. https://codereview.chromium.org/2039793005/diff/100001/media/blink/webmediapl... media/blink/webmediaplayer_impl.cc:1552: !seeking_) { On 2016/06/10 at 18:27:12, sandersd wrote: > Doesn't actually need the |!result.is_suspended| condition. > > With that change it actually looks exactly like the other variables. I'll let you decide if it should be turned into one. Done.
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2039793005/#ps140001 (title: "Condense.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2039793005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by dalecurtis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2039793005/#ps160001 (title: "Remove now incorrect test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2039793005/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Don't resume paused media. Don't resume playing media after timeout. As titled. We shouldn't resume paused media because there's no reason to not wait for the user interaction. We shouldn't resume playing media if the user has moved on to other tasks and comes back some time later. The idle timeout for resuming playing media is 5 seconds as chosen by a dice roll. BUG=617815 TEST=media suspends and resumes as expected. ========== to ========== Don't resume paused media. Don't resume playing media after timeout. As titled. We shouldn't resume paused media because there's no reason to not wait for the user interaction. We shouldn't resume playing media if the user has moved on to other tasks and comes back some time later. The idle timeout for resuming playing media is 5 seconds as chosen by a dice roll. BUG=617815 TEST=media suspends and resumes as expected. Committed: https://crrev.com/8b8505e78452b365b861025d7b3364ca4b1e5849 Cr-Commit-Position: refs/heads/master@{#399287} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/8b8505e78452b365b861025d7b3364ca4b1e5849 Cr-Commit-Position: refs/heads/master@{#399287} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
