Bug 151412 – Add a setting and restriction which will pause invisible autoplaying video

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Add a setting and restriction which will pause invisible autoplaying video

[Jer Noble]

Created attachment 265789 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Simon Fraser (smfr)]

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&

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Eric Carlson]

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?

[Jer Noble]

(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.

[Jer Noble]

(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.

[Jer Noble]

Created attachment 265990 [details]
Patch

[WebKit Commit Bot]

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.

[Simon Fraser (smfr)]

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).

[Eric Carlson]

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?

[Jer Noble]

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().

[Jer Noble]

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 &.

[WebKit Commit Bot]

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.

[Jer Noble]

Created attachment 265997 [details]
Patch

[WebKit Commit Bot]

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.

[Jer Noble]

Created attachment 266002 [details]
Patch

One more patch to unbreak EWS

[WebKit Commit Bot]

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.

[WebKit Commit Bot]

Comment on attachment 266002 [details]
Patch

Clearing flags on attachment: 266002

Committed r192966: <http://trac.webkit.org/changeset/192966>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/25090531>