Bug 208583 – [GPUP] Implement RemoteAudioSession

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[GPUP] Implement RemoteAudioSession

[Radar WebKit Bug Importer]

<rdar://problem/60047827>

[youenn fablet]

Is it the same as https://bugs.webkit.org/show_bug.cgi?id=208578?

[Jer Noble]

Created attachment 392445 [details]
Patch

[Jer Noble]

(In reply to youenn fablet from comment #2)
> Is it the same as https://bugs.webkit.org/show_bug.cgi?id=208578?

Oof, looks like it is.

[Jer Noble]

(In reply to Jer Noble from comment #4)
> (In reply to youenn fablet from comment #2)
> > Is it the same as https://bugs.webkit.org/show_bug.cgi?id=208578?
> 
> Oof, looks like it is.

That said, I think it's more useful to have an entire remote type rather than doing the commands ad-hoc. Also, the other patch doesn't seem to do the right thing when multiple WebContent processes have different audio categories or buffer sizes.

[Jer Noble]

Created attachment 392454 [details]
Patch

[Jer Noble]

Created attachment 392457 [details]
Patch

[Eric Carlson]

Comment on attachment 392454 [details]
Patch

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

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:363
> +#if ENABLE(LEGACY_ENCRYPTED_MEDIA)
> +    if (decoder.messageReceiverName() == Messages::RemoteLegacyCDMFactoryProxy::messageReceiverName()) {
> +        legacyCdmFactoryProxy().didReceiveMessageFromWebProcess(connection, decoder);
> +        return true;
> +    }
> +
> +    if (decoder.messageReceiverName() == Messages::RemoteLegacyCDMProxy::messageReceiverName()) {
> +        legacyCdmFactoryProxy().didReceiveCDMMessage(connection, decoder);
> +        return true;
> +    }
> +
> +    if (decoder.messageReceiverName() == Messages::RemoteLegacyCDMSessionProxy::messageReceiverName()) {
> +        legacyCdmFactoryProxy().didReceiveCDMSessionMessage(connection, decoder);
> +        return true;
> +    }
> +#endif

Does this belong in another patch?

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:432
> +#if ENABLE(LEGACY_ENCRYPTED_MEDIA)
> +    if (decoder.messageReceiverName() == Messages::RemoteLegacyCDMFactoryProxy::messageReceiverName()) {
> +        legacyCdmFactoryProxy().didReceiveSyncMessageFromWebProcess(connection, decoder, replyEncoder);
> +        return true;
> +    }
> +
> +    if (decoder.messageReceiverName() == Messages::RemoteLegacyCDMProxy::messageReceiverName()) {
> +        legacyCdmFactoryProxy().didReceiveSyncCDMMessage(connection, decoder, replyEncoder);
> +        return true;
> +    }
> +
> +    if (decoder.messageReceiverName() == Messages::RemoteLegacyCDMSessionProxy::messageReceiverName()) {
> +        legacyCdmFactoryProxy().didReceiveSyncCDMSessionMessage(connection, decoder, replyEncoder);
> +        return true;
> +    }
> +#endif

Ditto

> Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.cpp:108
> +    m_session->setCategory(category, policy);

We should log this change.

> Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.cpp:145
> +        // This proxy wants to de-activate, as is the last remaining active

s/as is/and is/

> Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.cpp:158
> +    // proxy with mix with the active proxies. No-op, and return activation

s/with mix/will mix/

> Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.cpp:173
> +        if (otherProxy->category() == AudioSession::AmbientSound)

Nit: it would be nice to encapsulate this logic in a a proxy method. Something like "canMixWithOthers()"?

> Source/WebKit/WebProcess/GPU/media/RemoteAudioSession.cpp:77
> +void RemoteAudioSession::configurationChanged(RemoteAudioSessionConfiguration&& configuration)

This is never called?

[Jer Noble]

(In reply to Eric Carlson from comment #8)
> Comment on attachment 392454 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392454&action=review
> 
> > Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:363
> > +#if ENABLE(LEGACY_ENCRYPTED_MEDIA)
> > +    if (decoder.messageReceiverName() == Messages::RemoteLegacyCDMFactoryProxy::messageReceiverName()) {
> > +        legacyCdmFactoryProxy().didReceiveMessageFromWebProcess(connection, decoder);
> > +        return true;
> > +    }
> > +
> > +    if (decoder.messageReceiverName() == Messages::RemoteLegacyCDMProxy::messageReceiverName()) {
> > +        legacyCdmFactoryProxy().didReceiveCDMMessage(connection, decoder);
> > +        return true;
> > +    }
> > +
> > +    if (decoder.messageReceiverName() == Messages::RemoteLegacyCDMSessionProxy::messageReceiverName()) {
> > +        legacyCdmFactoryProxy().didReceiveCDMSessionMessage(connection, decoder);
> > +        return true;
> > +    }
> > +#endif
> 
> Does this belong in another patch?

It does; it got mixed in with the Legacy EME patch. Will remove.

> > +    m_session->setCategory(category, policy);
> 
> We should log this change.

Ok. I don't know how we're doing logging in the GPU Process yet, but I'll just do a standard INFO_LOG() statement here.

> > Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.cpp:145
> > +        // This proxy wants to de-activate, as is the last remaining active
> 
> s/as is/and is/

Changed.

> > Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.cpp:158
> > +    // proxy with mix with the active proxies. No-op, and return activation
> 
> s/with mix/will mix/

Changed

> > Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.cpp:173
> > +        if (otherProxy->category() == AudioSession::AmbientSound)
> 
> Nit: it would be nice to encapsulate this logic in a a proxy method.
> Something like "canMixWithOthers()"?

Ok.

> > Source/WebKit/WebProcess/GPU/media/RemoteAudioSession.cpp:77
> > +void RemoteAudioSession::configurationChanged(RemoteAudioSessionConfiguration&& configuration)
> 
> This is never called?

Not yet. Planning for the future.

[Jer Noble]

(In reply to Jer Noble from comment #9)
> (In reply to Eric Carlson from comment #8)
> > Comment on attachment 392454 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=392454&action=review
> > 
> 
> > > +    m_session->setCategory(category, policy);
> > 
> > We should log this change.
> 
> Ok. I don't know how we're doing logging in the GPU Process yet, but I'll
> just do a standard INFO_LOG() statement here.

Wait, this will be logged by AudioSession; we should just be able to look through GPUProcess logs for AudioSession::setCategory().

[Jer Noble]

Created attachment 392460 [details]
Patch

[Jer Noble]

Created attachment 392461 [details]
Patch

[Eric Carlson]

(In reply to Jer Noble from comment #10)
> (In reply to Jer Noble from comment #9)
> > (In reply to Eric Carlson from comment #8)
> > > Comment on attachment 392454 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=392454&action=review
> > > 
> > 
> > > > +    m_session->setCategory(category, policy);
> > > 
> > > We should log this change.
> > 
> > Ok. I don't know how we're doing logging in the GPU Process yet, but I'll
> > just do a standard INFO_LOG() statement here.
> 

Use GPUConnectionToWebProcess.logger() and WebKit2LogMedia.

> Wait, this will be logged by AudioSession; we should just be able to look
> through GPUProcess logs for AudioSession::setCategory().

AudioSession still uses debug-only logging.

[Jer Noble]

(In reply to Eric Carlson from comment #13)
> (In reply to Jer Noble from comment #10)
> > (In reply to Jer Noble from comment #9)
> > > (In reply to Eric Carlson from comment #8)
> > > > Comment on attachment 392454 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=392454&action=review
> > > > 
> > > 
> > > > > +    m_session->setCategory(category, policy);
> > > > 
> > > > We should log this change.
> > > 
> > > Ok. I don't know how we're doing logging in the GPU Process yet, but I'll
> > > just do a standard INFO_LOG() statement here.
> > 
> 
> Use GPUConnectionToWebProcess.logger() and WebKit2LogMedia.

That won't work; this class isn't associated with a specific GPUConnectionToWebProcess. Is there something similar on GPUProcess?

[Jer Noble]

(In reply to Jer Noble from comment #14)
> That won't work; this class isn't associated with a specific
> GPUConnectionToWebProcess. Is there something similar on GPUProcess?

To answer my own question, no, there isn't. GPUProcess just uses RELEASE_LOG.

[Eric Carlson]

(In reply to Jer Noble from comment #15)
> (In reply to Jer Noble from comment #14)
> > That won't work; this class isn't associated with a specific
> > GPUConnectionToWebProcess. Is there something similar on GPUProcess?
> 
> To answer my own question, no, there isn't. GPUProcess just uses RELEASE_LOG.

Because we need a session ID to know if logging is allowed.

[Jer Noble]

(In reply to Eric Carlson from comment #16)
> (In reply to Jer Noble from comment #15)
> > (In reply to Jer Noble from comment #14)
> > > That won't work; this class isn't associated with a specific
> > > GPUConnectionToWebProcess. Is there something similar on GPUProcess?
> > 
> > To answer my own question, no, there isn't. GPUProcess just uses RELEASE_LOG.
> 
> Because we need a session ID to know if logging is allowed.

Okay, I don't think that we're going to be able to solve this with Logger currently; we can't aggregate logs across multiple loggers in a way that is compatible with the goal of disabling logging when not allowed.

[Jer Noble]

Created attachment 392471 [details]
Patch

[Jer Noble]

Created attachment 392482 [details]
Patch

[Jer Noble]

The Mac-wk2 test failures seem to be because MockAudioSharedUnit() is calling into AudioSession inside the UIProcess. On one hand, platformStrategies() should never be null, even in the UIProcess. On the other, why is MockAudioSharedUnit() using AudioSession directly in the first place?

[Jer Noble]

It definitely doesn't seem to be correct that platformStrategies() returns a nullable pointer that is never null-checked anywhere. It should either be guaranteed non null (i.e., a reference), or should be null-checked in every place it's called. For expediency's sake, I null-checked inside AudioSession::sharedSession(), but I don't think this is correct. I'll file a different bug on what the correct behavior should be.

[Jer Noble]

Created attachment 392518 [details]
Patch

[Jer Noble]

Created attachment 392520 [details]
Patch

[Jer Noble]

Created attachment 392524 [details]
Patch

[Alex Christensen]

Comment on attachment 392524 [details]
Patch

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

> Source/WebKit/GPUProcess/GPUPlatformStrategies.h:42
> +class GPUPlatformStrategies : public WebCore::PlatformStrategies {

Oh please no.  I have worked long and hard to move PlatformStrategies to PageConfiguration, which is the direction they should be going.  If you can't get to or from a Page, fail to do whatever you were about to do.  We should rename PlatformStrategies to LegacyPlatformStrategies.

[youenn fablet]

I am removing the usage of MediaSessionManagerCocoa in GPUProcess for now in https://bugs.webkit.org/show_bug.cgi?id=208568, which might help a little.

Ideally, we would like AudioSession::sharedSession() to only be called in GPUProcess when media for GPUProcess is enabled.
There is a small number of non-platform calls to AudioSession::sharedSession() that we could probably update easily.

When we discussed with Eric, the idea was to use PlatformMediaSessionManager as the interface between WebProcess and GPUProcess. This would handle AudioSession management.
That is made somehow harder given PlatformMediaSessionManager unfortunate tight relationships with HTMLMediaElement.

[Jer Noble]

(In reply to Alex Christensen from comment #25)
> Comment on attachment 392524 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392524&action=review
> 
> > Source/WebKit/GPUProcess/GPUPlatformStrategies.h:42
> > +class GPUPlatformStrategies : public WebCore::PlatformStrategies {
> 
> Oh please no.  I have worked long and hard to move PlatformStrategies to
> PageConfiguration, which is the direction they should be going.  If you
> can't get to or from a Page, fail to do whatever you were about to do.  We
> should rename PlatformStrategies to LegacyPlatformStrategies.

AudioSession is a global; Page is the wrong level of abstraction. 

That said, this setting should be a WebProcessCreationParameter, not a setting or a page configuration, which should also be global in the WebProcess. I'll switch this around so that the AudioSession global allows you to set the global object, which will accomplish the same goal without using platformStrategies().

[Jer Noble]

Created attachment 392570 [details]
Patch

[Jer Noble]

Created attachment 392574 [details]
Patch

[Jer Noble]

Created attachment 392575 [details]
Patch

[Alex Christensen]

Comment on attachment 392575 [details]
Patch

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

Much better.

> Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.h:51
> +    using SetActiveCompletion = CompletionHandler<void(bool)>;

I think this is unused.

> Source/WebKit/Shared/WebCoreArgumentCoders.h:1035
> +    WebCore::AudioSession::CategoryType,

We usually try to put these in the same header as the enum class.

> Source/WebKit/WebProcess/GPU/media/RemoteAudioSessionConfiguration.h:39
> +    float sampleRate;

These should probably have default initializers to prevent use of uninitialized memory if we make a mistake and don't initialize everything somewhere.

[Jer Noble]

Comment on attachment 392575 [details]
Patch

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

>> Source/WebKit/GPUProcess/media/RemoteAudioSessionProxyManager.h:51
>> +    using SetActiveCompletion = CompletionHandler<void(bool)>;
> 
> I think this is unused.

Whoops, will remove.

>> Source/WebKit/Shared/WebCoreArgumentCoders.h:1035
>> +    WebCore::AudioSession::CategoryType,
> 
> We usually try to put these in the same header as the enum class.

Ok, will move.

>> Source/WebKit/WebProcess/GPU/media/RemoteAudioSessionConfiguration.h:39
>> +    float sampleRate;
> 
> These should probably have default initializers to prevent use of uninitialized memory if we make a mistake and don't initialize everything somewhere.

Will do.

[Jer Noble]

Created attachment 392578 [details]
Patch for landing

[Jer Noble]

Created attachment 392579 [details]
Patch for landing

[Jer Noble]

Created attachment 392584 [details]
Patch for landing

[Jer Noble]

Created attachment 392585 [details]
Patch for landing

[Jer Noble]

Created attachment 392600 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 392600 [details]
Patch for landing

Clearing flags on attachment: 392600

Committed r257936: <https://trac.webkit.org/changeset/257936>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.