Bug 151412 - Add a setting and restriction which will pause invisible autoplaying video
Summary: Add a setting and restriction which will pause invisible autoplaying video
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-11-18 15:15 PST by Jer Noble
Modified: 2016-03-10 11:45 PST (History)
5 users (show)

See Also:


Attachments
Patch (33.47 KB, patch)
2015-11-18 15:40 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (855.41 KB, application/zip)
2015-11-19 14:43 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.13 MB, application/zip)
2015-11-19 16:47 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (900.92 KB, application/zip)
2015-11-19 16:52 PST, Build Bot
no flags Details
Patch (36.85 KB, patch)
2015-11-20 13:36 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (37.93 KB, patch)
2015-11-20 14:00 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (38.71 KB, patch)
2015-11-20 14:10 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (38.71 KB, patch)
2015-11-20 14:43 PST, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2015-11-18 15:15:13 PST
Add a setting and restriction which will pause invisible autoplaying video
Comment 1 Jer Noble 2015-11-18 15:40:00 PST
Created attachment 265789 [details]
Patch
Comment 2 WebKit Commit Bot 2015-11-18 15:42:35 PST
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 3 Build Bot 2015-11-19 14:43:10 PST
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
Comment 4 Build Bot 2015-11-19 14:43:12 PST
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 5 Simon Fraser (smfr) 2015-11-19 14:47:07 PST
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 6 Build Bot 2015-11-19 16:47:05 PST
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
Comment 7 Build Bot 2015-11-19 16:47:08 PST
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 8 Build Bot 2015-11-19 16:52:06 PST
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
Comment 9 Build Bot 2015-11-19 16:52:08 PST
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 10 Eric Carlson 2015-11-20 07:25:05 PST
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?
Comment 11 Jer Noble 2015-11-20 08:35:38 PST
(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.
Comment 12 Jer Noble 2015-11-20 08:36:50 PST
(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.
Comment 13 Jer Noble 2015-11-20 13:36:48 PST
Created attachment 265990 [details]
Patch
Comment 14 WebKit Commit Bot 2015-11-20 13:37:45 PST
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 15 Simon Fraser (smfr) 2015-11-20 13:49:17 PST
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 16 Eric Carlson 2015-11-20 13:51:00 PST
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 17 Jer Noble 2015-11-20 13:52:32 PST
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().
Comment 18 Jer Noble 2015-11-20 14:00:02 PST
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 &.
Comment 19 WebKit Commit Bot 2015-11-20 14:01:30 PST
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.
Comment 20 Jer Noble 2015-11-20 14:10:19 PST
Created attachment 265997 [details]
Patch
Comment 21 WebKit Commit Bot 2015-11-20 14:12:48 PST
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.
Comment 22 Jer Noble 2015-11-20 14:43:58 PST
Created attachment 266002 [details]
Patch

One more patch to unbreak EWS
Comment 23 WebKit Commit Bot 2015-11-20 14:45:27 PST
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 24 WebKit Commit Bot 2015-12-02 14:10:13 PST
Comment on attachment 266002 [details]
Patch

Clearing flags on attachment: 266002

Committed r192966: <http://trac.webkit.org/changeset/192966>
Comment 25 WebKit Commit Bot 2015-12-02 14:10:17 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Radar WebKit Bug Importer 2016-03-10 11:45:45 PST
<rdar://problem/25090531>