Bug 208714 – Move AudioSession interruption listener code to AudioSession

• Component: Media
• Status: RESOLVED FIXED

[youenn fablet]

GPUProcess should react upon AudioSession interruptions

[youenn fablet]

Created attachment 392722 [details]
WIP

[Jer Noble]

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.

[Jer Noble]

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.

[youenn fablet]

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.

[Jer Noble]

(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.

[Jer Noble]

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.

[youenn fablet]

(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.

[youenn fablet]

Created attachment 393024 [details]
Patch

[youenn fablet]

(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).

[youenn fablet]

Created attachment 393026 [details]
Patch

[youenn fablet]

Created attachment 393030 [details]
Patch

[Jer Noble]

(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".

[youenn fablet]

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.

[Jer Noble]

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.

[youenn fablet]

Created attachment 393241 [details]
Patch for landing

[youenn fablet]

(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.

[WebKit Commit Bot]

Comment on attachment 393241 [details]
Patch for landing

Clearing flags on attachment: 393241

Committed r258263: <https://trac.webkit.org/changeset/258263>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60324277>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 208922

[youenn fablet]

Created attachment 393367 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 393367 [details]
Patch for landing

Clearing flags on attachment: 393367

Committed r258324: <https://trac.webkit.org/changeset/258324>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.