Adopt interface AVAudioRoutingArbiter for Mac
<rdar://problem/59113994>
Created attachment 395764 [details] WIP patch
Created attachment 395847 [details] Fix unified build failures
Comment on attachment 395847 [details] Fix unified build failures View in context: https://bugs.webkit.org/attachment.cgi?id=395847&action=review Looks like WebGLLayer.mm was fixed since you uploaded this patch, so you'll need to rebase before landing. > Source/WebCore/platform/audio/mac/AudioSessionMac.mmSource/WebCore/platform/audio/mac/AudioSessionMac.cpp:121 > + [[PAL::getAVAudioRoutingArbiterClass() sharedRoutingArbiter] beginArbitrationWithCategory:arbitrationCategory completionHandler:[this](BOOL defaultDeviceChanged, NSError * _Nullable error) { I know `this` is safe because the class is a singleton, but it still makes me nervous. I don't really have a better suggestion though.
Created attachment 396133 [details] Patch
Comment on attachment 396133 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396133&action=review > Source/WebKit/ChangeLog:9 > + Add a new XPC object, AudioSessionRouterAbitrator/Proxy which passes routing arbitration I don't think you mean XPC here. > Source/WebKit/ChangeLog:12 > + Can you add a little bit of detail about what exactly audio routing arbitration is and why WebCore needs to be involved? I think that would aid reviewing the design here. > Source/WebKit/UIProcess/WebProcessProxy.h:542 > +#if HAVE(AVAUDIO_ROUTING_ARBITER) > + UniqueRef<AudioSessionRoutingArbitratorProxy> m_routingArbitrator; > +#endif Is there a way this can be made non-platform specific? The goal has been to try and create abstractions for things like this that other ports could then implement.
(In reply to Sam Weinig from comment #6) > Comment on attachment 396133 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396133&action=review > > > Source/WebKit/ChangeLog:9 > > + Add a new XPC object, AudioSessionRouterAbitrator/Proxy which passes routing arbitration > > I don't think you mean XPC here. Sure, it's an object that facilitates XPC by bridging the UIProcess/WebContent process boundary. So, I guess it's a cross-process object pair. > > Source/WebKit/UIProcess/WebProcessProxy.h:542 > > +#if HAVE(AVAUDIO_ROUTING_ARBITER) > > + UniqueRef<AudioSessionRoutingArbitratorProxy> m_routingArbitrator; > > +#endif > > Is there a way this can be made non-platform specific? The goal has been to > try and create abstractions for things like this that other ports could then > implement. Sure, I'll make AudioSessionRoutingArbitratorProxy the abstraction.
(In reply to Jer Noble from comment #7) > (In reply to Sam Weinig from comment #6) > > Comment on attachment 396133 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=396133&action=review > > > > > Source/WebKit/ChangeLog:9 > > > + Add a new XPC object, AudioSessionRouterAbitrator/Proxy which passes routing arbitration > > > > I don't think you mean XPC here. > > Sure, it's an object that facilitates XPC by bridging the > UIProcess/WebContent process boundary. So, I guess it's a cross-process > object pair. > I really just meant that "XPC object" has an existing meaning in Darwin of an instance of "xpc_object_t"
Created attachment 396295 [details] Patch
Created attachment 396296 [details] Patch (with rename enabled)
Comment on attachment 396296 [details] Patch (with rename enabled) View in context: https://bugs.webkit.org/attachment.cgi?id=396296&action=review > Source/WebKit/UIProcess/Media/AudioSessionRoutingArbitratorProxy.cpp:29 > +#if ENABLE(ROUTING_ARBITRATION) && !HAVE(AVAUDIO_ROUTING_ARBITER) Do we need to use both two macros here? > Source/WebKit/UIProcess/Media/AudioSessionRoutingArbitratorProxy.messages.in:26 > + LeaveRoutingArbitration(); EndRoutingArbitration sounds better?
Created attachment 396311 [details] Patch (with rename enabled)
(In reply to Peng Liu from comment #11) > Comment on attachment 396296 [details] > Patch (with rename enabled) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396296&action=review > > > Source/WebKit/UIProcess/Media/AudioSessionRoutingArbitratorProxy.cpp:29 > > +#if ENABLE(ROUTING_ARBITRATION) && !HAVE(AVAUDIO_ROUTING_ARBITER) > > Do we need to use both two macros here? Yes, because otherwise we will end up with duplicate symbols. > > Source/WebKit/UIProcess/Media/AudioSessionRoutingArbitratorProxy.messages.in:26 > > + LeaveRoutingArbitration(); > > EndRoutingArbitration sounds better? Sure.
Comment on attachment 396311 [details] Patch (with rename enabled) View in context: https://bugs.webkit.org/attachment.cgi?id=396311&action=review > Source/WebKit/UIProcess/Media/cocoa/AudioSessionRoutingArbitratorProxyCocoa.mm:122 > + for (auto& callback : m_enqueuedCallbacks) > + callback(error ? RoutingArbitrationError::Failed : RoutingArbitrationError::None, defaultDeviceChanged ? DefaultRouteChanged::Yes : DefaultRouteChanged::No); > + > + m_enqueuedCallbacks.clear(); Is there any chance that calling the callback could allow pending messages to be processed? If so, it would be safer to copy the callbacks to a new vector and clear m_enqueuedCallbacks first.
(In reply to Eric Carlson from comment #14) > Comment on attachment 396311 [details] > Patch (with rename enabled) > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396311&action=review > > > Source/WebKit/UIProcess/Media/cocoa/AudioSessionRoutingArbitratorProxyCocoa.mm:122 > > + for (auto& callback : m_enqueuedCallbacks) > > + callback(error ? RoutingArbitrationError::Failed : RoutingArbitrationError::None, defaultDeviceChanged ? DefaultRouteChanged::Yes : DefaultRouteChanged::No); > > + > > + m_enqueuedCallbacks.clear(); > > Is there any chance that calling the callback could allow pending messages > to be processed? If so, it would be safer to copy the callbacks to a new > vector and clear m_enqueuedCallbacks first. Since we're always clearing the callbacks afterwards, we should absolutely WTFMove them into a new vector first. Good catch.
Created attachment 396436 [details] Patch for landing
Created attachment 396461 [details] Patch for landing
Comment on attachment 396461 [details] Patch for landing Previously failing wincairo bot succeeded. Landing.
Committed r260102: <https://trac.webkit.org/changeset/260102> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396461 [details].
Reopen this bug to fix a build failure.
Created attachment 396508 [details] Patch
Committed r260128: <https://trac.webkit.org/changeset/260128> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396508 [details].