GPUProcess should react upon AudioSession interruptions
Created attachment 392722 [details] WIP
I'm already in the middle of implementing something similar, but that also handles route changes as the like. I'll post it when I've got something compiling.
Created attachment 392728 [details] WIP: WebCore - MediaSessionHelper Here's the approach I've been working towards. I plan on subclassing this new helper and XPCing the client callbacks across the process boundary.
https://bugs.webkit.org/attachment.cgi?id=392722 is specifically about AudioSession interruptions. I think it makes sense to have it close to AudioSession with an observer pattern. This allows the remote session to ping the necessary WebProcesses nicely. I agree the other ones also need to move, but they seem more related to NowPlaying really and Eric is working on it I think.
(In reply to youenn fablet from comment #4) > https://bugs.webkit.org/attachment.cgi?id=392722 is specifically about > AudioSession interruptions. > > I think it makes sense to have it close to AudioSession with an observer > pattern. > This allows the remote session to ping the necessary WebProcesses nicely. > > I agree the other ones also need to move, but they seem more related to > NowPlaying really and Eric is working on it I think. That's fine; I'll just remove interruption handling from the helper class I'm making. But I don't understand why we can't use the existing RemoteAudioSession/Proxy object rather than dropping all the way to GPUProcess/Proxy. Rather than add a bunch of interruption code to GPUProcessConnection, it seems like a cleaner approach to just send the interruptions from RemoteAudioSessionManagerProxy -> RemoteAudioSessionProxy -> XPC -> RemoteAudioSession -> observers; that contains the audio-related code in the correct class.
Comment on attachment 392722 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=392722&action=review > Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.cpp:208 > +void RemoteAudioSessionProxyManager::beginAudioSessionInterruption(PlatformMediaSession::InterruptionType type) > +{ > + m_process.forAllWebProcessConnections([type](auto& webProcessConnection) { > + webProcessConnection.connection().send(Messages::GPUProcessConnection::BeginAudioSessionInterruption { type }, 0); > + }); > +} > + > +void RemoteAudioSessionProxyManager::endAudioSessionInterruption(PlatformMediaSession::EndInterruptionFlags flags) > +{ > + m_process.forAllWebProcessConnections([flags](auto& webProcessConnection) { > + webProcessConnection.connection().send(Messages::GPUProcessConnection::EndAudioSessionInterruption { flags }, 0); > + }); > +} I don't think this is correct. We should only send interruptions to processes with active AudioSessions. So I'd use tryToSetActiveForProcess() as an example here and skip sending interruptions to sessions which are not active.
(In reply to Jer Noble from comment #6) > Comment on attachment 392722 [details] > WIP > > View in context: > https://bugs.webkit.org/attachment.cgi?id=392722&action=review > > > Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.cpp:208 > > +void RemoteAudioSessionProxyManager::beginAudioSessionInterruption(PlatformMediaSession::InterruptionType type) > > +{ > > + m_process.forAllWebProcessConnections([type](auto& webProcessConnection) { > > + webProcessConnection.connection().send(Messages::GPUProcessConnection::BeginAudioSessionInterruption { type }, 0); > > + }); > > +} > > + > > +void RemoteAudioSessionProxyManager::endAudioSessionInterruption(PlatformMediaSession::EndInterruptionFlags flags) > > +{ > > + m_process.forAllWebProcessConnections([flags](auto& webProcessConnection) { > > + webProcessConnection.connection().send(Messages::GPUProcessConnection::EndAudioSessionInterruption { flags }, 0); > > + }); > > +} > > I don't think this is correct. We should only send interruptions to > processes with active AudioSessions. So I'd use tryToSetActiveForProcess() > as an example here and skip sending interruptions to sessions which are not > active. Right, that is what I wrote in Source/WebKit/ChangeLog, hence why this is WIP patch :) Agreed with the idea to go directly through RemoteAudioSession to WebProcess AudioSession proxy.
Created attachment 393024 [details] Patch
(In reply to youenn fablet from comment #8) > Created attachment 393024 [details] > Patch Rebased patch on top of newly added MediaSessionHelperClient. It seems RemoteAudioSessionProxyManager should only have a set of active session proxies. That would somehow mirror what the OS is probably doing (only caring about active Audio Sessions).
Created attachment 393026 [details] Patch
Created attachment 393030 [details] Patch
(In reply to youenn fablet from comment #11) > Created attachment 393030 [details] > Patch New patch looks good. I wonder if it's time to make AudioSessionIOS and AudioSessionMac proper subclasses of AudioSession, rather than just "mixins".
Comment on attachment 393030 [details] Patch Will test it tomorrow on iOS. Putting it under review in the meantime. View in context: https://bugs.webkit.org/attachment.cgi?id=393030&action=review > Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:117 > + AudioSession::sharedSession().addInterruptionObserver(*this); I'll remove this unneeded line.
Comment on attachment 393030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393030&action=review r=me with nits: > Source/WebCore/platform/audio/AudioSession.h:103 > + class InterruptionObserver : public CanMakeWeakPtr<InterruptionObserver> { > + public: > + virtual ~InterruptionObserver() = default; > + > + virtual void beginAudioSessionInterruption(PlatformMediaSession::InterruptionType) = 0; > + virtual void endAudioSessionInterruption(PlatformMediaSession::EndInterruptionFlags) = 0; > + }; InterruptionObserver shouldn't use PlatformMediaSession enums; it should export its own. > Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:55 > + // FIXME: We probably want each interested media session to observe audio interruptions directly. > + AudioSession::sharedSession().addInterruptionObserver(*this); I don't think we need this FIXME. The session manager should be the one which, e.g., decides which session is resumed after an interruption ends.
Created attachment 393241 [details] Patch for landing
(In reply to Jer Noble from comment #14) > Comment on attachment 393030 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=393030&action=review > > r=me with nits: > > > Source/WebCore/platform/audio/AudioSession.h:103 > > + class InterruptionObserver : public CanMakeWeakPtr<InterruptionObserver> { > > + public: > > + virtual ~InterruptionObserver() = default; > > + > > + virtual void beginAudioSessionInterruption(PlatformMediaSession::InterruptionType) = 0; > > + virtual void endAudioSessionInterruption(PlatformMediaSession::EndInterruptionFlags) = 0; > > + }; > > InterruptionObserver shouldn't use PlatformMediaSession enums; it should > export its own. Agreed, we do not need PlatformMediaSession::InterruptionType at all here. Will do as a follow-up. > > > Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:55 > > + // FIXME: We probably want each interested media session to observe audio interruptions directly. > > + AudioSession::sharedSession().addInterruptionObserver(*this); > > I don't think we need this FIXME. The session manager should be the one > which, e.g., decides which session is resumed after an interruption ends. I removed it from now. I'll dig in further but it seems going through MediaSessionManager is unnecessary at the moment since it basically iterate over all sessions for both beginInterruption/endInterruption. And it might open up some nice simplification. In addition, we probably want at some point to have Page-level and not Process-level objects for AudioSession and MediaSessionManager in the WebProcess. Decoupling the two might help doing this transition more easily.
Comment on attachment 393241 [details] Patch for landing Clearing flags on attachment: 393241 Committed r258263: <https://trac.webkit.org/changeset/258263>
All reviewed patches have been landed. Closing bug.
<rdar://problem/60324277>
Re-opened since this is blocked by bug 208922
Created attachment 393367 [details] Patch for landing
Comment on attachment 393367 [details] Patch for landing Clearing flags on attachment: 393367 Committed r258324: <https://trac.webkit.org/changeset/258324>