[GPUP] Implement RemoteAudioSession
<rdar://problem/60047827>
Is it the same as https://bugs.webkit.org/show_bug.cgi?id=208578?
Created attachment 392445 [details] Patch
(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.
(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.
Created attachment 392454 [details] Patch
Created attachment 392457 [details] Patch
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?
(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.
(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().
Created attachment 392460 [details] Patch
Created attachment 392461 [details] Patch
(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.
(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?
(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.
(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.
(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.
Created attachment 392471 [details] Patch
Created attachment 392482 [details] Patch
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?
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.
Created attachment 392518 [details] Patch
Created attachment 392520 [details] Patch
Created attachment 392524 [details] Patch
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.
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.
(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().
Created attachment 392570 [details] Patch
Created attachment 392574 [details] Patch
Created attachment 392575 [details] Patch
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.
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.
Created attachment 392578 [details] Patch for landing
Created attachment 392579 [details] Patch for landing
Created attachment 392584 [details] Patch for landing
Created attachment 392585 [details] Patch for landing
Created attachment 392600 [details] Patch for landing
Comment on attachment 392600 [details] Patch for landing Clearing flags on attachment: 392600 Committed r257936: <https://trac.webkit.org/changeset/257936>
All reviewed patches have been landed. Closing bug.