Bug 208714 - Move AudioSession interruption listener code to AudioSession
Summary: Move AudioSession interruption listener code to AudioSession
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 208922
Blocks:
  Show dependency treegraph
 
Reported: 2020-03-06 08:00 PST by youenn fablet
Modified: 2020-03-12 07:05 PDT (History)
8 users (show)

See Also:


Attachments
WIP (23.03 KB, patch)
2020-03-06 08:06 PST, youenn fablet
no flags Details | Formatted Diff | Diff
WIP: WebCore - MediaSessionHelper (57.38 KB, patch)
2020-03-06 08:58 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (35.53 KB, patch)
2020-03-09 06:43 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (36.34 KB, patch)
2020-03-09 07:12 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (36.99 KB, patch)
2020-03-09 07:32 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (36.51 KB, patch)
2020-03-11 07:44 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (36.55 KB, patch)
2020-03-12 06:19 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2020-03-06 08:00:19 PST
GPUProcess should react upon AudioSession interruptions
Comment 1 youenn fablet 2020-03-06 08:06:36 PST
Created attachment 392722 [details]
WIP
Comment 2 Jer Noble 2020-03-06 08:52:20 PST
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.
Comment 3 Jer Noble 2020-03-06 08:58:01 PST
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.
Comment 4 youenn fablet 2020-03-06 09:07:43 PST
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.
Comment 5 Jer Noble 2020-03-06 09:57:28 PST
(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 6 Jer Noble 2020-03-06 09:59:53 PST
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.
Comment 7 youenn fablet 2020-03-09 02:02:15 PDT
(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.
Comment 8 youenn fablet 2020-03-09 06:43:45 PDT
Created attachment 393024 [details]
Patch
Comment 9 youenn fablet 2020-03-09 06:46:12 PDT
(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).
Comment 10 youenn fablet 2020-03-09 07:12:58 PDT
Created attachment 393026 [details]
Patch
Comment 11 youenn fablet 2020-03-09 07:32:26 PDT
Created attachment 393030 [details]
Patch
Comment 12 Jer Noble 2020-03-09 08:10:29 PDT
(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 13 youenn fablet 2020-03-09 10:39:43 PDT
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 14 Jer Noble 2020-03-10 07:39:52 PDT
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.
Comment 15 youenn fablet 2020-03-11 07:44:00 PDT
Created attachment 393241 [details]
Patch for landing
Comment 16 youenn fablet 2020-03-11 07:57:22 PDT
(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 17 WebKit Commit Bot 2020-03-11 08:46:59 PDT
Comment on attachment 393241 [details]
Patch for landing

Clearing flags on attachment: 393241

Committed r258263: <https://trac.webkit.org/changeset/258263>
Comment 18 WebKit Commit Bot 2020-03-11 08:47:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Radar WebKit Bug Importer 2020-03-11 08:47:43 PDT
<rdar://problem/60324277>
Comment 20 WebKit Commit Bot 2020-03-11 09:46:41 PDT
Re-opened since this is blocked by bug 208922
Comment 21 youenn fablet 2020-03-12 06:19:32 PDT
Created attachment 393367 [details]
Patch for landing
Comment 22 WebKit Commit Bot 2020-03-12 07:05:19 PDT
Comment on attachment 393367 [details]
Patch for landing

Clearing flags on attachment: 393367

Committed r258324: <https://trac.webkit.org/changeset/258324>
Comment 23 WebKit Commit Bot 2020-03-12 07:05:21 PDT
All reviewed patches have been landed.  Closing bug.