Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2654123002:
Autoplay muted video when visible and pause when invisible (Closed)
|
Created:
3 years, 11 months ago by Zhiqiang Zhang (Slow) Modified:
3 years, 10 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri (slow - plz ping), mlamouri+watch-blink_chromium.org, nessy, Srirama Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAutoplay muted video when visible and pause when invisible
In this CL, when a muted video is autoplaying from attribute, we
pause it when it comes off screen.
Intent to implement and ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/UtFM-kndhaI
Spec PR: https://github.com/whatwg/html/pull/2324
BUG=683141
Review-Url: https://codereview.chromium.org/2654123002
Cr-Commit-Position: refs/heads/master@{#450707}
Committed: https://chromium.googlesource.com/chromium/src/+/6b261e240392eadde78e85ae750d9ba2f51f2239
Patch Set 1 #Patch Set 2 : finalizing implementation and tests #
Total comments: 8
Patch Set 3 : improved tests #
Total comments: 18
Patch Set 4 : addressed mlamouri's comments on tests #
Total comments: 4
Patch Set 5 : fixed tests #
Messages
Total messages: 30 (16 generated)
Description was changed from ========== Autoplay muted video when visible and pause when invisible BUG=683141 ========== to ========== Autoplay muted video when visible and pause when invisible In this CL, when a muted video is autoplaying from attribute, we pause it when it comes off screen. Intent to implement and ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/UtFM-kndhaI Spec PR: https://github.com/whatwg/html/pull/2324 BUG=683141 ==========
zqzhang@chromium.org changed reviewers: + avayvod@chromium.org
avayvod@: PTAL CC: mlamouri@
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
impl looks good but the test seems improvable https://codereview.chromium.org/2654123002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html (right): https://codereview.chromium.org/2654123002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:11: var expectations = [ 'paused', 'playing', 'paused', 'playing', 'paused', 'playing', 'paused' ]; I don't think this is needed at all: - you know what to expect when visible/invisible - you can setup the onplay/onpause handlers in runWhenVisible and runWhenInvisible to guarantee that the right event happens - perhaps just two transitions is enough (hidden & paused -> visible & playing -> hidden & paused again)? https://codereview.chromium.org/2654123002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:24: checkExpectations(t, 'paused'); you could simplify these two lines (ditto below) by making the expectations[] a bool array as in expectedPausedValue = [ true, false, true, ..., true ] and do assert_equals(expectedPausedValue[id++], video.paused) in the check above. https://codereview.chromium.org/2654123002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:47: video.addEventListener('canplay', t.step_func(_ => runStepsWhenInvisible(t))); will this event fire only once during this test? maybe you should remove the handler on the first fire? https://codereview.chromium.org/2654123002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:48: video.addEventListener('play', t.step_func(_ => runStepsWhenVisible(t))); I'd rather have a visibility observer to check the paused state instead of relying on the code under test. When the video is visible, check it's not paused and make it invisible, when invisible, check it's paused and make it visible.
PTAL https://codereview.chromium.org/2654123002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html (right): https://codereview.chromium.org/2654123002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:11: var expectations = [ 'paused', 'playing', 'paused', 'playing', 'paused', 'playing', 'paused' ]; On 2017/02/13 16:50:15, whywhat_slow_PST_till_Feb_6 wrote: > I don't think this is needed at all: > - you know what to expect when visible/invisible > - you can setup the onplay/onpause handlers in runWhenVisible and > runWhenInvisible to guarantee that the right event happens > - perhaps just two transitions is enough (hidden & paused -> visible & playing > -> hidden & paused again)? Now add/remove event listeners to ensure the events happen in the correct order. I still prefer test multiple times. Now I'm using a counter and the test passes if the counter reaches 3. https://codereview.chromium.org/2654123002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:24: checkExpectations(t, 'paused'); On 2017/02/13 16:50:15, whywhat_slow_PST_till_Feb_6 wrote: > you could simplify these two lines (ditto below) by making the expectations[] a > bool array as in expectedPausedValue = [ true, false, true, ..., true ] and do > assert_equals(expectedPausedValue[id++], video.paused) in the check above. This no longer applies as removed. https://codereview.chromium.org/2654123002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:47: video.addEventListener('canplay', t.step_func(_ => runStepsWhenInvisible(t))); On 2017/02/13 16:50:15, whywhat_slow_PST_till_Feb_6 wrote: > will this event fire only once during this test? maybe you should remove the > handler on the first fire? Done. https://codereview.chromium.org/2654123002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:48: video.addEventListener('play', t.step_func(_ => runStepsWhenVisible(t))); On 2017/02/13 16:50:15, whywhat_slow_PST_till_Feb_6 wrote: > I'd rather have a visibility observer to check the paused state instead of > relying on the code under test. When the video is visible, check it's not paused > and make it invisible, when invisible, check it's paused and make it visible. I prefer not to use visibility observer as the internal observer in HTMLMediaElement might receive the event later, so it's not guaranteed that onVisibilityChangedForAutoplay() be called. Listening to "play" and "pause" is fine. If there's a bug, the test should time out.
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Can we prevent it even from loading when offscreen? That's what ends up taking resources.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/13 19:14:01, DaleCurtis wrote: > Can we prevent it even from loading when offscreen? That's what ends up taking > resources. I guess then we need to do another spec change. Currently in the spec, the load algorithm is called in a number of situations, such as "setting the src attribute". We should probably allow the UA to delay the step until the element becomes visible.
lgtm
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
sgtm but I would like to look at the tests again :) https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html (right): https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:11: var cycleCount = 0; gCyclecount to make it clear it's global maybe? https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:12: var testInstance; I don't think you need this. https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:14: function neverHappens(description) { Usage of neverHappens should be: `t.unreached_func()` https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:24: video.removeEventListener('pause', runStepsWhenInvisible); To avoid the `removeEventListener`, you can add `{ once: true }` when `addEventListener` https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:25: video.removeEventListener('play', neverHappens); Maybe use `onplay` here and below to be able to reset easily. https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:27: video.addEventListener('play', runStepsWhenVisible); You should pass `t` to `runStepsWhenINvisible` then pass it again to `runStepsWhenVisible`. Also call it with: ``` video.addEventListener('play', t.step_func(() => { runStepsWhenVisible(t); })); ``` https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:34: testInstance.done(); If `t` is passed, you should be able to do `t.done()` https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:41: video.addEventListener('play', neverHappens); See comments above. https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4015: if (m_canAutoplay && autoplay()) { Would it make sense to add a DCHECK(isAutoplayMuted())?
mlamouri: PTAL https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html (right): https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:11: var cycleCount = 0; On 2017/02/14 20:59:02, mlamouri wrote: > gCyclecount to make it clear it's global maybe? Done. https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:12: var testInstance; On 2017/02/14 20:59:01, mlamouri wrote: > I don't think you need this. Done. https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:14: function neverHappens(description) { On 2017/02/14 20:59:02, mlamouri wrote: > Usage of neverHappens should be: `t.unreached_func()` Done. https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:24: video.removeEventListener('pause', runStepsWhenInvisible); On 2017/02/14 20:59:02, mlamouri wrote: > To avoid the `removeEventListener`, you can add `{ once: true }` when > `addEventListener` Done. https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:25: video.removeEventListener('play', neverHappens); On 2017/02/14 20:59:02, mlamouri wrote: > Maybe use `onplay` here and below to be able to reset easily. Done. https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:27: video.addEventListener('play', runStepsWhenVisible); On 2017/02/14 20:59:02, mlamouri wrote: > You should pass `t` to `runStepsWhenINvisible` then pass it again to > `runStepsWhenVisible`. Also call it with: > ``` > video.addEventListener('play', t.step_func(() => { > runStepsWhenVisible(t); > })); > ``` Done. https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:34: testInstance.done(); On 2017/02/14 20:59:02, mlamouri wrote: > If `t` is passed, you should be able to do `t.done()` Done. https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:41: video.addEventListener('play', neverHappens); On 2017/02/14 20:59:02, mlamouri wrote: > See comments above. Done. https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2654123002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:4015: if (m_canAutoplay && autoplay()) { On 2017/02/14 20:59:02, mlamouri wrote: > Would it make sense to add a DCHECK(isAutoplayMuted())? Hmm, it might hit this DCHECK when the page calls `video.autoplay = false` after it starts autoplaying. Currently I think we do nothing when unsetting the attribute.
lgtm. Thanks! :) https://codereview.chromium.org/2654123002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html (right): https://codereview.chromium.org/2654123002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:28: t.done(); Can you return too just to make sure we don't run extra code that might end up running asserts after done() ;) https://codereview.chromium.org/2654123002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:38: testInstance = t; drop this?
https://codereview.chromium.org/2654123002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html (right): https://codereview.chromium.org/2654123002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:28: t.done(); On 2017/02/15 10:46:02, mlamouri wrote: > Can you return too just to make sure we don't run extra code that might end up > running asserts after done() ;) Done. https://codereview.chromium.org/2654123002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/autoplay-when-visible-multiple-times.html:38: testInstance = t; On 2017/02/15 10:46:02, mlamouri wrote: > drop this? Done.
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avayvod@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2654123002/#ps80001 (title: "fixed tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487158017484000,
"parent_rev": "af71c6b09dc5bd882f2f2018050c2902a92e024b", "commit_rev":
"6b261e240392eadde78e85ae750d9ba2f51f2239"}
Message was sent while issue was closed.
Description was changed from ========== Autoplay muted video when visible and pause when invisible In this CL, when a muted video is autoplaying from attribute, we pause it when it comes off screen. Intent to implement and ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/UtFM-kndhaI Spec PR: https://github.com/whatwg/html/pull/2324 BUG=683141 ========== to ========== Autoplay muted video when visible and pause when invisible In this CL, when a muted video is autoplaying from attribute, we pause it when it comes off screen. Intent to implement and ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/UtFM-kndhaI Spec PR: https://github.com/whatwg/html/pull/2324 BUG=683141 Review-Url: https://codereview.chromium.org/2654123002 Cr-Commit-Position: refs/heads/master@{#450707} Committed: https://chromium.googlesource.com/chromium/src/+/6b261e240392eadde78e85ae750d... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/6b261e240392eadde78e85ae750d...
Message was sent while issue was closed.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Message was sent while issue was closed.
Is it possible to get a spec bug for stopping load? Delaying autoplay until visibility won't help page jank too much since we'll still be required to load metadata, etc.
Message was sent while issue was closed.
On 2017/02/15 17:38:43, DaleCurtis wrote: > Is it possible to get a spec bug for stopping load? Delaying autoplay until > visibility won't help page jank too much since we'll still be required to load > metadata, etc. Done. https://github.com/whatwg/html/issues/2364 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
