Bug 126530 – Teach MediaSessionManager to manage interruptions

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

[Eric Carlson]

Create a mechanism for a platform to interrupt and optionally resume audio and video playback.

[Eric Carlson]

Created attachment 220451 [details]
Patch for the bots to chew on

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/video-interruption-active-when-element-created-expected.txt', u'LayoutTests/media/video-interruption-active-when-element-created.html', u'LayoutTests/media/video-interruption-with-resume-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-allowing-play.html', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/html/HTMLVideoElement.cpp', u'Source/WebCore/platform/audio/MediaSession.cpp', u'Source/WebCore/platform/audio/MediaSession.h', u'Source/WebCore/platform/audio/MediaSessionManager.cpp', u'Source/WebCore/platform/audio/MediaSessionManager.h', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.cpp', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.h', u'Source/WebCore/platform/audio/mac/MediaSessionManagerMac.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/html/HTMLMediaElement.cpp:62:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/html/HTMLMediaElement.cpp:4744:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 2 in 27 files


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

[Eric Carlson]

Created attachment 220452 [details]
Fix style errors

[EFL EWS Bot]

Comment on attachment 220452 [details]
Fix style errors

Attachment 220452 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/5040858160168960

[Gyuyoung Kim]

(In reply to comment #4)
> (From update of attachment 220452 [details])
> Attachment 220452 [details] did not pass efl-ews (efl):
> Output: http://webkit-queues.appspot.com/results/5040858160168960

It looks you need to add a include path to efl-wk1 first. Please add "${WEBCORE_DIR}/platform/audio" to below line.

http://trac.webkit.org/browser/trunk/Source/WebKit/CMakeLists.txt#L40


If there is a similar build error on efl-wk2 as well, please add it("${WEBCORE_DIR}/platform/audio") to below as well.

http://trac.webkit.org/browser/trunk/Source/WebKit2/CMakeLists.txt#L96

Now I'm verifying it locally.

[EFL EWS Bot]

Comment on attachment 220452 [details]
Fix style errors

Attachment 220452 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/4755002585251840

[Eric Carlson]

Created attachment 220459 [details]
Updated patch

Patch updated with Gyuyoung's suggested fixes.

[Sam Weinig]

Comment on attachment 220459 [details]
Updated patch

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

> Source/WebCore/platform/audio/MediaSession.h:73
> +    bool playbackPermitted(const HTMLMediaElement&) const;
> +    bool dataLoadingPermitted(const HTMLMediaElement&) const;
> +    bool fullscreenPermitted(const HTMLMediaElement&) const;
> +    bool pageAllowsDataLoading(const HTMLMediaElement&) const;
> +    bool pageAllowsPlaybackAfterResuming(const HTMLMediaElement&) const;
> +#if ENABLE(IOS_AIRPLAY)
> +    bool showingPlaybackTargetPickerPermitted(const HTMLMediaElement&) const;
> +#endif

The platform layer is not allowed to know about HTMLMediaElements.  Is this really a platform layer concept? Is it wrapping an existing platform API?

[Eric Carlson]

(In reply to comment #8)
> (From update of attachment 220459 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220459&action=review
> 
> > Source/WebCore/platform/audio/MediaSession.h:73
> > +    bool playbackPermitted(const HTMLMediaElement&) const;
> > +    bool dataLoadingPermitted(const HTMLMediaElement&) const;
> > +    bool fullscreenPermitted(const HTMLMediaElement&) const;
> > +    bool pageAllowsDataLoading(const HTMLMediaElement&) const;
> > +    bool pageAllowsPlaybackAfterResuming(const HTMLMediaElement&) const;
> > +#if ENABLE(IOS_AIRPLAY)
> > +    bool showingPlaybackTargetPickerPermitted(const HTMLMediaElement&) const;
> > +#endif
> 
> The platform layer is not allowed to know about HTMLMediaElements.  Is this really a platform layer concept? Is it wrapping an existing platform API?

Oops, good point. 

The session only uses an HTMLMediaElement to manage restrictions, which is tangential to the real purpose of this patch. I will move that logic back to HTMLMediaElement.

[Eric Carlson]

Created attachment 220537 [details]
Updated patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/video-interruption-active-when-element-created-expected.txt', u'LayoutTests/media/video-interruption-active-when-element-created.html', u'LayoutTests/media/video-interruption-with-resume-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-allowing-play.html', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/platform/audio/MediaSession.cpp', u'Source/WebCore/platform/audio/MediaSession.h', u'Source/WebCore/platform/audio/MediaSessionManager.cpp', u'Source/WebCore/platform/audio/MediaSessionManager.h', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.cpp', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.h', u'Source/WebCore/platform/audio/mac/MediaSessionManagerMac.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl', u'Source/WebKit/CMakeLists.txt', u'Source/WebKit/ChangeLog', u'Source/WebKit2/CMakeLists.txt', u'Source/WebKit2/ChangeLog', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/audio/MediaSession.h:27:  MediaSession_h is incorrect. #defined constants should use all uppercase names with words separated by underscores.  [readability/naming/define/constants] [4]
Total errors found: 1 in 30 files


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

[Sam Weinig]

Comment on attachment 220537 [details]
Updated patch

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

> Source/WebCore/platform/audio/MediaSessionManager.cpp:52
> +    for (auto it = m_sessions.begin(), end = m_sessions.end(); it != end; ++it) {
>          if ((*it)->mediaType() == type)
>              return true;
>      }

for (auto* session : m_sessions) {
    if (session->mediaType() == type)
        return true;
}

> Source/WebCore/platform/audio/MediaSessionManager.cpp:65
> +    for (auto it = m_sessions.begin(), end = m_sessions.end(); it != end; ++it) {
>          if ((*it)->mediaType() == type)
>              ++count;
>      }

Same here.

> Source/WebCore/platform/audio/MediaSessionManager.cpp:76
> +    for (auto it = m_sessions.begin(), end = m_sessions.end(); it != end; ++it)
> +        (*it)->beginInterruption();

And here.

> Source/WebCore/platform/audio/MediaSessionManager.cpp:86
> +    for (auto it = m_sessions.begin(), end = m_sessions.end(); it != end; ++it)
> +        (*it)->endInterruption(flags);

And here.

[Eric Carlson]

(In reply to comment #11)
> Attachment 220537 [details] did not pass style-queue:
...
> ERROR: Source/WebCore/platform/audio/MediaSession.h:27:  MediaSession_h is incorrect. #defined constants should use all uppercase names with words separated by underscores.  [readability/naming/define/constants] [4]
> 
Huh?

[Eric Carlson]

Created attachment 220548 [details]
Updated to address Sam's comments

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/video-interruption-active-when-element-created-expected.txt', u'LayoutTests/media/video-interruption-active-when-element-created.html', u'LayoutTests/media/video-interruption-with-resume-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-allowing-play.html', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/platform/audio/MediaSession.cpp', u'Source/WebCore/platform/audio/MediaSession.h', u'Source/WebCore/platform/audio/MediaSessionManager.cpp', u'Source/WebCore/platform/audio/MediaSessionManager.h', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.cpp', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.h', u'Source/WebCore/platform/audio/mac/MediaSessionManagerMac.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl', u'Source/WebKit/CMakeLists.txt', u'Source/WebKit/ChangeLog', u'Source/WebKit2/CMakeLists.txt', u'Source/WebKit2/ChangeLog', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/audio/MediaSession.h:27:  MediaSession_h is incorrect. #defined constants should use all uppercase names with words separated by underscores.  [readability/naming/define/constants] [4]
Total errors found: 1 in 30 files


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

[Jer Noble]

Comment on attachment 220548 [details]
Updated to address Sam's comments

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

Even though this patch touches WebKit2 in only the most cursory way, you should probably get Sam or another OWNER to check off on it.

> Source/WebCore/html/HTMLMediaElement.cpp:4116
>      bool pausedToBuffer = m_readyStateMaximum >= HAVE_FUTURE_DATA && m_readyState < HAVE_FUTURE_DATA;
> -    return (pausedToBuffer || m_readyState >= HAVE_FUTURE_DATA) && couldPlayIfEnoughData() && !isBlockedOnMediaController();
> +    
> +    if (!(pausedToBuffer || m_readyState >= HAVE_FUTURE_DATA))
> +        return false;
> +
> +    return couldPlayIfEnoughData() && !isBlockedOnMediaController();

If you're cleaning this up to be more understandable, the if-clause could be further cleaned up:

if (!pausedToBuffer && m_readyState < HAVE_FUTURE_DATA)
    return false;

> Source/WebCore/platform/audio/MediaSessionManager.cpp:29
> +#include "HTMLMediaElement.h"

You should remove this #include now that we don't pass in a HTMLMediaElement.

> LayoutTests/media/video-interruption-active-when-element-created.html:34
> +                setTimeout(checkState, 250);

250ms? Can we bring this down to something slightly smaller?

> LayoutTests/media/video-interruption-active-when-element-created.html:39
> +                if (window.internals) 

We should just fail the test immediately if window.internals isn't present.

> LayoutTests/media/video-interruption-active-when-element-created.html:40
> +                    run("window.internals.beginMediaSessionInterruption()");;

The "window." is unnecessary.

> LayoutTests/media/video-interruption-with-resume-allowing-play.html:10
> +                consoleWrite("250ms timer fired...");

Ditto about 250ms.

> LayoutTests/media/video-interruption-with-resume-allowing-play.html:34
> +                if (window.internals) 

Ditto about failing early.

> LayoutTests/media/video-interruption-with-resume-not-allowing-play.html:23
> +                    setTimeout(checkState, 250);

Again with the 250ms.

> LayoutTests/media/video-interruption-with-resume-not-allowing-play.html:38
> +                if (window.internals) 

Again with the failing early.

[Eric Carlson]

Created attachment 220567 [details]
Another update.

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/video-interruption-active-when-element-created-expected.txt', u'LayoutTests/media/video-interruption-active-when-element-created.html', u'LayoutTests/media/video-interruption-with-resume-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-allowing-play.html', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play-expected.txt', u'LayoutTests/media/video-interruption-with-resume-not-allowing-play.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/html/HTMLMediaElement.cpp', u'Source/WebCore/html/HTMLMediaElement.h', u'Source/WebCore/platform/audio/MediaSession.cpp', u'Source/WebCore/platform/audio/MediaSession.h', u'Source/WebCore/platform/audio/MediaSessionManager.cpp', u'Source/WebCore/platform/audio/MediaSessionManager.h', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.cpp', u'Source/WebCore/platform/audio/mac/AudioDestinationMac.h', u'Source/WebCore/platform/audio/mac/MediaSessionManagerMac.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl', u'Source/WebKit/CMakeLists.txt', u'Source/WebKit/ChangeLog', u'Source/WebKit2/CMakeLists.txt', u'Source/WebKit2/ChangeLog', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/audio/MediaSession.h:27:  MediaSession_h is incorrect. #defined constants should use all uppercase names with words separated by underscores.  [readability/naming/define/constants] [4]
Total errors found: 1 in 30 files


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

[WebKit Commit Bot]

Comment on attachment 220567 [details]
Another update.

Clearing flags on attachment: 220567

Committed r161481: <http://trac.webkit.org/changeset/161481>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #19)
> (From update of attachment 220567 [details])
> Clearing flags on attachment: 220567
> 
> Committed r161481: <http://trac.webkit.org/changeset/161481>

It broke the Windows build:

   1>C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Include\WebCore/HTMLMediaElement.h(36): fatal error C1083: Cannot open include file: 'MediaSession.h': No such file or directory (..\..\win\FullscreenVideoController.cpp)
     1>C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Include\WebCore/HTMLMediaElement.h(36): fatal error C1083: Cannot open include file: 'MediaSession.h': No such file or directory (..\..\win\WebView.cpp)
     1>c:\cygwin\home\buildbot\slave\win-release\build\webkitbuild\release\include\webcore\HTMLMediaElement.h(36): fatal error C1083: Cannot open include file: 'MediaSession.h': No such file or directory (..\..\win\WebCoreSupport\WebChromeClient.cpp)