Bug 210167 – Adopt interface AVAudioRoutingArbiter for Mac

• Component: Media
• Status: RESOLVED FIXED

[Peng Liu]

Adopt interface AVAudioRoutingArbiter for Mac

[Peng Liu]

<rdar://problem/59113994>

[Peng Liu]

Created attachment 395764 [details]
WIP patch

[Peng Liu]

Created attachment 395847 [details]
Fix unified build failures

[Jer Noble]

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.

[Jer Noble]

Created attachment 396133 [details]
Patch

[Sam Weinig]

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.

[Jer Noble]

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

[Sam Weinig]

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

[Jer Noble]

Created attachment 396295 [details]
Patch

[Jer Noble]

Created attachment 396296 [details]
Patch (with rename enabled)

[Peng Liu]

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?

[Jer Noble]

Created attachment 396311 [details]
Patch (with rename enabled)

[Jer Noble]

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

[Eric Carlson]

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.

[Jer Noble]

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

[Jer Noble]

Created attachment 396436 [details]
Patch for landing

[Jer Noble]

Created attachment 396461 [details]
Patch for landing

[Jer Noble]

Comment on attachment 396461 [details]
Patch for landing

Previously failing wincairo bot succeeded. Landing.

[EWS]

Committed r260102: <https://trac.webkit.org/changeset/260102>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396461 [details].

[Peng Liu]

Reopen this bug to fix a build failure.

[Peng Liu]

Created attachment 396508 [details]
Patch

[EWS]

Committed r260128: <https://trac.webkit.org/changeset/260128>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396508 [details].