Add a setting and restriction which will pause invisible autoplaying video
Created attachment 265789 [details] Patch
Attachment 265789 [details] did not pass style-queue: ERROR: Source/WebCore/platform/audio/PlatformMediaSession.cpp:58: Non-label code inside switch statements should be indented. [whitespace/indent] [4] Total errors found: 1 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 265789 [details] Patch Attachment 265789 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/451459 New failing tests: media/video-interruption-with-resume-not-allowing-play.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-video.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-move-to-other-document.html media/video-background-playback.html webgl/1.0.2/conformance/extensions/oes-texture-float-with-video.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-video-rgba5551.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-video-rgb565.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-move-within-document.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-video-rgba4444.html media/video-interruption-with-resume-allowing-play.html webgl/1.0.3/conformance/extensions/oes-texture-half-float-with-video.html
Created attachment 265906 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 265789 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265789&action=review > Source/WebCore/html/HTMLMediaElement.cpp:6716 > +static bool mediaElementIsAllowedToAutoplay(HTMLMediaElement& element) const HTMLMediaElement& > Source/WebCore/rendering/RenderElement.h:336 > + unsigned m_isRegisteredForVisibleInViewportCallback : 1; Please put this into RenderObject's rareData. > Source/WebCore/rendering/RenderElement.h:344 > + VisibleInViewportState m_visibleInViewportState { VisibilityUnknown }; How much does this increase the size of RenderElement? > Source/WebCore/rendering/RenderView.cpp:1348 > +void RenderView::unregisterForVisibleInViewportCallback(RenderElement& renderer) const RenderElement& > Source/WebCore/rendering/RenderView.cpp:1354 > +void RenderView::updateVisibleViewportRect(IntRect visibleRect) const IntRect&
Comment on attachment 265789 [details] Patch Attachment 265789 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/451882 New failing tests: media/video-interruption-with-resume-not-allowing-play.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-video.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-move-to-other-document.html media/video-background-playback.html webgl/1.0.2/conformance/extensions/oes-texture-float-with-video.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-video-rgba5551.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-video-rgb565.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-move-within-document.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-video-rgba4444.html media/video-interruption-with-resume-allowing-play.html webgl/1.0.3/conformance/extensions/oes-texture-half-float-with-video.html
Created attachment 265919 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 265789 [details] Patch Attachment 265789 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/451886 New failing tests: media/video-interruption-with-resume-not-allowing-play.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-video.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-move-to-other-document.html media/video-background-playback.html webgl/1.0.2/conformance/extensions/oes-texture-float-with-video.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-video-rgba5551.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-video-rgb565.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document.html imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-move-within-document.html webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-video-rgba4444.html media/video-interruption-with-resume-allowing-play.html webgl/1.0.3/conformance/extensions/oes-texture-half-float-with-video.html
Created attachment 265921 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 265789 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265789&action=review > Source/WebCore/ChangeLog:14 > + Drive-by fix: m_autoplaying is reset in many places by calling pause() or play(), where those > + calls did not originate from an explicit request to pause or play, e.g., during an interruption. > + This causes m_autoplaying to be set to false, thus breaking resumption of autoplaying when the > + interruption ends. Change these calls to play() and pause() to playInternal() and pauseInternal(), > + respectively, and only clear m_autoplaying in the explicit play() and pause() methods. Can you add a test, or modify an existing test, for this fix? > Source/WebCore/rendering/RenderElement.cpp:1477 > +void RenderElement::registerForVisibleInViewportCallback() > +{ > + if (m_isRegisteredForVisibleInViewportCallback) > + return; > + m_isRegisteredForVisibleInViewportCallback = true; > + > + view().registerForVisibleInViewportCallback(*this); > +} Shouldn't you set m_visibleInViewportState to unknown here? > Source/WebCore/rendering/RenderElement.h:226 > + bool suspendImageAnimationIfNeeded(); Is this used?
(In reply to comment #10) > Comment on attachment 265789 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265789&action=review > > > Source/WebCore/ChangeLog:14 > > + Drive-by fix: m_autoplaying is reset in many places by calling pause() or play(), where those > > + calls did not originate from an explicit request to pause or play, e.g., during an interruption. > > + This causes m_autoplaying to be set to false, thus breaking resumption of autoplaying when the > > + interruption ends. Change these calls to play() and pause() to playInternal() and pauseInternal(), > > + respectively, and only clear m_autoplaying in the explicit play() and pause() methods. > > Can you add a test, or modify an existing test, for this fix? It will be very timing sensitive. We would have to trigger an interruption after the video had begun loading, but before it had begun auto-playing, just to set up the state where the interruption would (previously) clear m_autoplaying. However, I'm re-thinking this. The spec says: "When the pause() method is invoked, and when the user agent is required to pause the media element [...] set the media element's autoplaying flag to false." So it sounds like we should be clearing this flag even during interruptions, and other times when we call pauseInternal(). I'll come up with a better fix here. > > Source/WebCore/rendering/RenderElement.cpp:1477 > > +void RenderElement::registerForVisibleInViewportCallback() > > +{ > > + if (m_isRegisteredForVisibleInViewportCallback) > > + return; > > + m_isRegisteredForVisibleInViewportCallback = true; > > + > > + view().registerForVisibleInViewportCallback(*this); > > +} > > Shouldn't you set m_visibleInViewportState to unknown here? Yes! > > Source/WebCore/rendering/RenderElement.h:226 > > + bool suspendImageAnimationIfNeeded(); > > Is this used? Probably not.
(In reply to comment #11) > (In reply to comment #10) > > Comment on attachment 265789 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=265789&action=review > > > > > Source/WebCore/ChangeLog:14 > > > + Drive-by fix: m_autoplaying is reset in many places by calling pause() or play(), where those > > > + calls did not originate from an explicit request to pause or play, e.g., during an interruption. > > > + This causes m_autoplaying to be set to false, thus breaking resumption of autoplaying when the > > > + interruption ends. Change these calls to play() and pause() to playInternal() and pauseInternal(), > > > + respectively, and only clear m_autoplaying in the explicit play() and pause() methods. > > > > Can you add a test, or modify an existing test, for this fix? > > It will be very timing sensitive. We would have to trigger an interruption > after the video had begun loading, but before it had begun auto-playing, > just to set up the state where the interruption would (previously) clear > m_autoplaying. > > However, I'm re-thinking this. The spec says: "When the pause() method is > invoked, and when the user agent is required to pause the media element > [...] set the media element's autoplaying flag to false." So it sounds like > we should be clearing this flag even during interruptions, and other times > when we call pauseInternal(). I'll come up with a better fix here. > > > > Source/WebCore/rendering/RenderElement.cpp:1477 > > > +void RenderElement::registerForVisibleInViewportCallback() > > > +{ > > > + if (m_isRegisteredForVisibleInViewportCallback) > > > + return; > > > + m_isRegisteredForVisibleInViewportCallback = true; > > > + > > > + view().registerForVisibleInViewportCallback(*this); > > > +} > > > > Shouldn't you set m_visibleInViewportState to unknown here? > > Yes! Wait! No! It will already be "unknown", either because that's the initial state, or because we set that state when we unregister. > > > Source/WebCore/rendering/RenderElement.h:226 > > > + bool suspendImageAnimationIfNeeded(); > > > > Is this used? > > Probably not.
Created attachment 265990 [details] Patch
Attachment 265990 [details] did not pass style-queue: ERROR: Source/WebCore/platform/audio/PlatformMediaSession.cpp:59: Non-label code inside switch statements should be indented. [whitespace/indent] [4] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 265990 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265990&action=review > Source/WebCore/html/HTMLMediaElement.cpp:6747 > + if (renderer->style().visibility() != VISIBLE) > + return false; Probably want a "FIXME: look at ancestor visibility" here. > Source/WebCore/html/HTMLMediaElement.cpp:6752 > + return true; What about being zero sized, or having opacity: 0? > Source/WebCore/rendering/RenderObject.cpp:2208 > + if (visible != VisibilityUnknown || hasRareData()) > + ensureRareData().setVisibleInViewportState(visible); And other style stuff (opacity). Some stuff you can only check after layout (size).
Comment on attachment 265990 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265990&action=review the media-element-specific parts look good, but I will let Simon give it an official r+. > Source/WebCore/html/HTMLMediaElement.cpp:6492 > + if (m_readyState >= HAVE_ENOUGH_DATA > + && paused() > + && autoplay() > + && !document().isSandboxed(SandboxAutomaticFeatures) > + && m_mediaSession->playbackPermitted(*this)) it would be nice to not duplicate this logic here and in HTMLMediaElement::setReadyState, can it be shared?
Comment on attachment 265990 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265990&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:6747 >> + return false; > > Probably want a "FIXME: look at ancestor visibility" here. So I thought about this, and if an ancestor is marked as "visibility:hidden", its children will inherit that style, unless it's explicitly overridden. So we should be safe here without checking ancestors. >> Source/WebCore/html/HTMLMediaElement.cpp:6752 >> + return true; > > What about being zero sized, or having opacity: 0? Right now, I'm shooting for GIF parity. We can add more conditions in the future (such as "transform:rotate-y(90deg)".) >> Source/WebCore/rendering/RenderObject.cpp:2208 >> + ensureRareData().setVisibleInViewportState(visible); > > And other style stuff (opacity). Some stuff you can only check after layout (size). We have a good place to hook off that already, in HTMLMediaElement::layoutSizeChanged().
Created attachment 265996 [details] Patch Simon's suggestion to make unregister...() take a const& causes a compile error when trying to remove a const pointer from the HashSet; reverting to a non-const &.
Attachment 265996 [details] did not pass style-queue: ERROR: Source/WebCore/platform/audio/PlatformMediaSession.cpp:59: Non-label code inside switch statements should be indented. [whitespace/indent] [4] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 265997 [details] Patch
Attachment 265997 [details] did not pass style-queue: ERROR: Source/WebCore/platform/audio/PlatformMediaSession.cpp:59: Non-label code inside switch statements should be indented. [whitespace/indent] [4] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 266002 [details] Patch One more patch to unbreak EWS
Attachment 266002 [details] did not pass style-queue: ERROR: Source/WebCore/platform/audio/PlatformMediaSession.cpp:59: Non-label code inside switch statements should be indented. [whitespace/indent] [4] Total errors found: 1 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 266002 [details] Patch Clearing flags on attachment: 266002 Committed r192966: <http://trac.webkit.org/changeset/192966>
All reviewed patches have been landed. Closing bug.
<rdar://problem/25090531>