Bug 192829 – [WebKitLegacy] Media playback pauses on scroll

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jer Noble]

[WebKitLegacy] Media playback pauses on scroll

[Jer Noble]

<rdar://problem/46420245>

[Jer Noble]

Created attachment 357610 [details]
Patch

[EWS Watchlist]

Attachment 357610 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:48:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Created attachment 357615 [details]
Patch

[EWS Watchlist]

Attachment 357615 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:48:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Comment on attachment 357615 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357615&action=review

> Source/WebCore/dom/Document.cpp:1727
> +        platformMediaSessionManager->stopAllMediaPlaybackForDocument(this);

Nit: you might as well update stopAllMediaPlaybackForDocument to take a Document reference.

> Source/WebCore/html/HTMLMediaElement.cpp:834
> +        INFO_LOG(LOGIDENTIFIER, "src = ", value);

We shouldn't log urls in release builds. You can have this only log in a debug build by changing it to something like 

    INFO_LOG(LOGIDENTIFIER, "src = ", URL { URL { }, value });

> Source/WebCore/page/Page.cpp:1738
> +    for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) {
> +        if (auto* document = frame->document())
> +            document->suspendAllMediaPlayback();
> +    }
> +
> +    m_mediaPlaybackIsSuspended = true;

Nit: it is probably slightly safer to set m_mediaPlaybackIsSuspended before suspending the documents, in case they do something that causes this to be called again.

[Jer Noble]

(In reply to Eric Carlson from comment #6)
> Comment on attachment 357615 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357615&action=review
> 
> > Source/WebCore/dom/Document.cpp:1727
> > +        platformMediaSessionManager->stopAllMediaPlaybackForDocument(this);
> 
> Nit: you might as well update stopAllMediaPlaybackForDocument to take a
> Document reference.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:834
> > +        INFO_LOG(LOGIDENTIFIER, "src = ", value);
> 
> We shouldn't log urls in release builds. You can have this only log in a
> debug build by changing it to something like 
> 
>     INFO_LOG(LOGIDENTIFIER, "src = ", URL { URL { }, value });

Yeah, this was a debugging statement that shouldn't have gone into this patch; I'll delete it.

> > Source/WebCore/page/Page.cpp:1738
> > +    for (Frame* frame = &mainFrame(); frame; frame = frame->tree().traverseNext()) {
> > +        if (auto* document = frame->document())
> > +            document->suspendAllMediaPlayback();
> > +    }
> > +
> > +    m_mediaPlaybackIsSuspended = true;
> 
> Nit: it is probably slightly safer to set m_mediaPlaybackIsSuspended before
> suspending the documents, in case they do something that causes this to be
> called again.

No, it needs to happen afterwards, because pause() checks whether playback is allowed, which it won't be if playback is suspended.

[Jer Noble]

Created attachment 358337 [details]
Patch for landing

[EWS Watchlist]

Attachment 358337 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/ScrollingDoesNotPauseMedia.mm:48:  This { should be at the end of the previous line  [whitespace/braces] [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 358337 [details]
Patch for landing

Clearing flags on attachment: 358337

Committed r239640: <https://trac.webkit.org/changeset/239640>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 358337 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=358337&action=review

> Source/WebCore/ChangeLog:15
> +        Do not use suspendActiveDOMObjects(ReasonForSuspension::PageWillBeSuspended) to pause
> +        video. Roll back the changes to HTMLMediaElement, and introduce a new set of Page calls
> +        suspendAllMediaPlayback() & resumeAllMediaPlayback() which replaces the removed bahavior.

What was the cause of the bug though?

[Jer Noble]

(In reply to Simon Fraser (smfr) from comment #12)
> Comment on attachment 358337 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=358337&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        Do not use suspendActiveDOMObjects(ReasonForSuspension::PageWillBeSuspended) to pause
> > +        video. Roll back the changes to HTMLMediaElement, and introduce a new set of Page calls
> > +        suspendAllMediaPlayback() & resumeAllMediaPlayback() which replaces the removed bahavior.
> 
> What was the cause of the bug though?

WebKitLegacy uses suspendActiveDOMObjects(PageWillBeSuspended) during active scroll operations.

[Aakash Jain]

The ScrollingDoesNotPauseMedia API Test added in this patch seems to be failing consistently. It would be nice to fix that soon to keep the automation green/happy.