Bug 126780 – Allow MediaSessionManager to restrict media playback

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

[Eric Carlson]

Some platforms do not allow more than one <video> element to play concurrently. Have MediaSessionManager handle this.

[Eric Carlson]

Created attachment 220896 [details]
Patch for the bots

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/media/video-concurrent-playback-expected.txt', u'LayoutTests/media/video-concurrent-playback.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.xcodeproj/project.pbxproj', u'Source/WebCore/html/ElementMediaSession.cpp', u'Source/WebCore/html/ElementMediaSession.h', 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.h', 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/ElementMediaSession.cpp:131:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/testing/Internals.h:337:  The parameter name "ec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 22 files


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

[Eric Carlson]

Created attachment 220902 [details]
Updated patch

Now with fewer style errors.

[Jer Noble]

Comment on attachment 220902 [details]
Updated patch

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

I don't really like the name "ElementMediaSession".  If we move WebAudio out of modules, most of it will end up in html/, not platform/, and conceivably we could want to add a html/-level MediaSession for something which is not an Element.

Aside from that, this looks good with a couple of nits:

> Source/WebCore/html/ElementMediaSession.cpp:125
> +void ElementMediaSession::clientWillBeginPlayback()
> +{
> +    MediaSessionManager::sharedManager().sessionWillBeginPlayback(*this);
> +}

This doesn't appear to have any HTMLMediaElement specific knowledge.  Can we move this into MediaSession?

> Source/WebCore/html/ElementMediaSession.cpp:130
> +void ElementMediaSession::pauseSession()
> +{
> +    client().pausePlayback();
> +}

This looks like it does exactly the same thing as MediaSession::pauseSession().  Can we get rid of this and make MediaSession::pauseSession() non-virtual?

> Source/WebCore/platform/audio/MediaSessionManager.cpp:142
> +    for (auto* oneSession : m_sessions) {
> +        if (oneSession == &session)
> +            continue;
> +        if (oneSession->mediaType() != sessionType)
> +            continue;
> +        if (restrictions & ConcurrentPlaybackNotPermitted)
> +            oneSession->pauseSession();
> +    }

It's a tiny optimization, but to benefit those platforms which will never set the ConcurrentPlaybackNotPermitted restriction, the first check should be: "if (!(restrictions & ConcurrentPlaybackNotPermitted)) continue;"

[Eric Carlson]

(In reply to comment #4)
> (From update of attachment 220902 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220896&action=review
> 
> I don't really like the name "ElementMediaSession".  If we move WebAudio out of modules, most of it will end up in html/, not platform/, and conceivably we could want to add a html/-level MediaSession for something which is not an Element.
> 

As discussed on irc, I will change it to HTMLMediaSession.

> Aside from that, this looks good with a couple of nits:
> 
> > Source/WebCore/html/ElementMediaSession.cpp:125
> > +void ElementMediaSession::clientWillBeginPlayback()
> > +{
> > +    MediaSessionManager::sharedManager().sessionWillBeginPlayback(*this);
> > +}
> 
> This doesn't appear to have any HTMLMediaElement specific knowledge.  Can we move this into MediaSession?
> 

Done.

> > Source/WebCore/html/ElementMediaSession.cpp:130
> > +void ElementMediaSession::pauseSession()
> > +{
> > +    client().pausePlayback();
> > +}
> 
> This looks like it does exactly the same thing as MediaSession::pauseSession().  Can we get rid of this and make MediaSession::pauseSession() non-virtual?
> 

Done.


> > Source/WebCore/platform/audio/MediaSessionManager.cpp:142
> > +    for (auto* oneSession : m_sessions) {
> > +        if (oneSession == &session)
> > +            continue;
> > +        if (oneSession->mediaType() != sessionType)
> > +            continue;
> > +        if (restrictions & ConcurrentPlaybackNotPermitted)
> > +            oneSession->pauseSession();
> > +    }
> 
> It's a tiny optimization, but to benefit those platforms which will never set the ConcurrentPlaybackNotPermitted restriction, the first check should be: "if (!(restrictions & ConcurrentPlaybackNotPermitted)) continue;"

There is already an early return before entering the loop if the session has no restrictions.

[Eric Carlson]

https://trac.webkit.org/r161899

[Csaba Osztrogonác]

(In reply to comment #6)
> https://trac.webkit.org/r161899

It broke the Apple Windows build:
     1>WebCoreTestSupport.lib(Internals.obj) : error LNK2019: unresolved external symbol "public: void __thiscall WebCore::MediaSessionManager::addRestriction(enum WebCore::MediaSession::MediaType,unsigned int)" (?addRestriction@MediaSessionManager@WebCore@@QAEXW4MediaType@MediaSession@2@I@Z) referenced in function "public: void __thiscall WebCore::Internals::setMediaSessionRestrictions(class WTF::String const &,class WTF::String const &,int &)" (?setMediaSessionRestrictions@Internals@WebCore@@QAEXABVString@WTF@@0AAH@Z)
     1>WebCoreTestSupport.lib(Internals.obj) : error LNK2019: unresolved external symbol "public: void __thiscall WebCore::MediaSessionManager::removeRestriction(enum WebCore::MediaSession::MediaType,unsigned int)" (?removeRestriction@MediaSessionManager@WebCore@@QAEXW4MediaType@MediaSession@2@I@Z) referenced in function "public: void __thiscall WebCore::Internals::setMediaSessionRestrictions(class WTF::String const &,class WTF::String const &,int &)" (?setMediaSessionRestrictions@Internals@WebCore@@QAEXABVString@WTF@@0AAH@Z)

[Eric Carlson]

Plus https://trac.webkit.org/r161902 to fix the Windows build.

[Sam Weinig]

Comment on attachment 220902 [details]
Updated patch

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

> Source/WebCore/platform/audio/MediaSessionManager.h:71
> +    std::map<MediaSession::MediaType, SessionRestrictions> m_restrictions;

Why have not historically used std::map.  Is there a reason you are using this over a WTF data structure?

[Eric Carlson]

(In reply to comment #9)
> (From update of attachment 220902 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220902&action=review
> 
> > Source/WebCore/platform/audio/MediaSessionManager.h:71
> > +    std::map<MediaSession::MediaType, SessionRestrictions> m_restrictions;
> 
> Why have not historically used std::map.  Is there a reason you are using this over a WTF data structure?

Not a good reason anyway, I will fix it.