Bug 192829

Summary: [WebKitLegacy] Media playback pauses on scroll
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: MediaAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, commit-queue, eric.carlson, ews-watchlist, simon.fraser, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=193286
https://bugs.webkit.org/show_bug.cgi?id=195137
https://bugs.webkit.org/show_bug.cgi?id=210013
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Jer Noble 2018-12-18 13:58:58 PST
[WebKitLegacy] Media playback pauses on scroll
Comment 1 Jer Noble 2018-12-18 14:01:00 PST
<rdar://problem/46420245>
Comment 2 Jer Noble 2018-12-18 14:02:00 PST
Created attachment 357610 [details]
Patch
Comment 3 EWS Watchlist 2018-12-18 14:06:39 PST
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.
Comment 4 Jer Noble 2018-12-18 14:52:46 PST
Created attachment 357615 [details]
Patch
Comment 5 EWS Watchlist 2018-12-18 14:55:04 PST
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.
Comment 6 Eric Carlson 2018-12-21 13:16:45 PST
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.
Comment 7 Jer Noble 2018-12-21 13:32:33 PST
(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.
Comment 8 Jer Noble 2019-01-04 11:04:22 PST
Created attachment 358337 [details]
Patch for landing
Comment 9 EWS Watchlist 2019-01-04 11:07:32 PST
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.
Comment 10 WebKit Commit Bot 2019-01-04 14:23:24 PST
Comment on attachment 358337 [details]
Patch for landing

Clearing flags on attachment: 358337

Committed r239640: <https://trac.webkit.org/changeset/239640>
Comment 11 WebKit Commit Bot 2019-01-04 14:23:26 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Simon Fraser (smfr) 2019-01-05 10:43:50 PST
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?
Comment 13 Jer Noble 2019-01-07 09:22:30 PST
(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.
Comment 14 Aakash Jain 2019-02-28 07:33:55 PST
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.