Some platforms do not allow more than one <video> element to play concurrently. Have MediaSessionManager handle this.
Created attachment 220896 [details] Patch for the bots
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.
Created attachment 220902 [details] Updated patch Now with fewer style errors.
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;"
(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.
https://trac.webkit.org/r161899
(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)
Plus https://trac.webkit.org/r161902 to fix the Windows build.
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?
(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.