Bug 210167 - Adopt interface AVAudioRoutingArbiter for Mac
Summary: Adopt interface AVAudioRoutingArbiter for Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Mac Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-07 17:15 PDT by Peng Liu
Modified: 2020-04-15 08:29 PDT (History)
14 users (show)

See Also:


Attachments
WIP patch (19.82 KB, patch)
2020-04-07 18:20 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Fix unified build failures (20.71 KB, patch)
2020-04-08 11:42 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (77.28 KB, patch)
2020-04-10 16:37 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (81.25 KB, patch)
2020-04-13 09:42 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (with rename enabled) (84.29 KB, patch)
2020-04-13 09:44 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (with rename enabled) (84.20 KB, patch)
2020-04-13 11:26 PDT, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff
Patch for landing (70.32 KB, patch)
2020-04-14 10:34 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (70.32 KB, patch)
2020-04-14 14:20 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (1.64 KB, patch)
2020-04-15 00:17 PDT, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2020-04-07 17:15:18 PDT
Adopt interface AVAudioRoutingArbiter for Mac
Comment 1 Peng Liu 2020-04-07 17:16:25 PDT
<rdar://problem/59113994>
Comment 2 Peng Liu 2020-04-07 18:20:01 PDT
Created attachment 395764 [details]
WIP patch
Comment 3 Peng Liu 2020-04-08 11:42:14 PDT
Created attachment 395847 [details]
Fix unified build failures
Comment 4 Jer Noble 2020-04-10 10:21:18 PDT
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.
Comment 5 Jer Noble 2020-04-10 16:37:38 PDT
Created attachment 396133 [details]
Patch
Comment 6 Sam Weinig 2020-04-12 11:57:44 PDT
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.
Comment 7 Jer Noble 2020-04-13 09:32:26 PDT
(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.
Comment 8 Sam Weinig 2020-04-13 09:36:16 PDT
(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"
Comment 9 Jer Noble 2020-04-13 09:42:41 PDT
Created attachment 396295 [details]
Patch
Comment 10 Jer Noble 2020-04-13 09:44:42 PDT
Created attachment 396296 [details]
Patch (with rename enabled)
Comment 11 Peng Liu 2020-04-13 10:57:06 PDT
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?
Comment 12 Jer Noble 2020-04-13 11:26:42 PDT
Created attachment 396311 [details]
Patch (with rename enabled)
Comment 13 Jer Noble 2020-04-13 15:22:04 PDT
(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 14 Eric Carlson 2020-04-13 15:49:01 PDT
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.
Comment 15 Jer Noble 2020-04-14 09:21:20 PDT
(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.
Comment 16 Jer Noble 2020-04-14 10:34:55 PDT
Created attachment 396436 [details]
Patch for landing
Comment 17 Jer Noble 2020-04-14 14:20:22 PDT
Created attachment 396461 [details]
Patch for landing
Comment 18 Jer Noble 2020-04-14 15:11:44 PDT
Comment on attachment 396461 [details]
Patch for landing

Previously failing wincairo bot succeeded. Landing.
Comment 19 EWS 2020-04-14 15:35:52 PDT
Committed r260102: <https://trac.webkit.org/changeset/260102>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396461 [details].
Comment 20 Peng Liu 2020-04-15 00:13:45 PDT
Reopen this bug to fix a build failure.
Comment 21 Peng Liu 2020-04-15 00:17:09 PDT
Created attachment 396508 [details]
Patch
Comment 22 EWS 2020-04-15 08:29:33 PDT
Committed r260128: <https://trac.webkit.org/changeset/260128>

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