Bug 220471 – [HANG] 496ms to 1360ms in WebCore::AVAudioSessionCaptureDeviceManager::refreshAudioCaptureDevices()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

[HANG] 496ms to 1360ms in WebCore::AVAudioSessionCaptureDeviceManager::refreshAudioCaptureDevices()

[Jer Noble]

<rdar://72349642>

[Jer Noble]

Created attachment 417285 [details]
Patch

[Peng Liu]

Comment on attachment 417285 [details]
Patch

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

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:146
> +        unsigned m_resultsRemaining { 1 };

Maybe my understanding is wrong. I feel the initial value here needs to be 0?

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:150
> +    auto accumulator = CaptureDeviceAccumulator::create([completion = WTFMove(completion)] (Vector<CaptureDevice> devices) mutable {

Nit. The space between square brackets and parentheses can be removed.

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:153
> +    getCaptureDevices([] (Vector<CaptureDevice>) { });

Ditto.

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:183
> +    dispatch_async(m_dispatchQueue, makeBlockPtr([this, completion = WTFMove(completion)] () mutable {

Ditto.

[Jer Noble]

(In reply to Peng Liu from comment #3)
> Comment on attachment 417285 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417285&action=review
> 
> > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:146
> > +        unsigned m_resultsRemaining { 1 };
> 
> Maybe my understanding is wrong. I feel the initial value here needs to be 0?

Nope. We need to handle the case where all the callbacks are called synchronously. If we start at zero, the master callback will be called if the first callback is called before the second one is added.

[youenn fablet]

Comment on attachment 417285 [details]
Patch

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

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:115
> +void RealtimeMediaSourceCenter::getMediaStreamDevices(CompletionHandler<void(Vector<CaptureDevice>)>&& completion)

Should probably be CompletionHandler<void(Vector<CaptureDevice>&&)>&&.
Here and below

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:117
> +    class CaptureDeviceAccumulator : public RefCounted<CaptureDeviceAccumulator> {

Can we use CallbackAggregator instead?

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:136
> +                m_completion(m_results);

WTFMove()

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:140
> +        CaptureDeviceAccumulator(CompletionHandler<void(Vector<CaptureDevice>)>&& completion)

explicit

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:145
> +        CompletionHandler<void(Vector<CaptureDevice>)> m_completion;

s/m_completion/m_completionHandler/

>> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:150
>> +    auto accumulator = CaptureDeviceAccumulator::create([completion = WTFMove(completion)] (Vector<CaptureDevice> devices) mutable {
> 
> Nit. The space between square brackets and parentheses can be removed.

s/(Vector<CaptureDevice> devices)/(auto&& devices)

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:154
> +        completion(devices);

WTFMove(devices)

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.h:69
> +    WEBCORE_EXPORT void getMediaStreamDevices(CompletionHandler<void(Vector<CaptureDevice>)>&&);

&&

>> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:153
>> +    getCaptureDevices([] (Vector<CaptureDevice>) { });
> 
> Ditto.

auto

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:185
> +        callOnWebThreadOrDispatchAsyncOnMainThread(makeBlockPtr([this, completion = WTFMove(completion), newAudioDevices = WTFMove(newAudioDevices)] () mutable {

Why not simply callOnMainThread? This code is not expected to work in WebKit 1 anyway.
It might be safer also to isolate copy newAudioDevices.
Something like newAudioDevices = crossThreadCopy(WTFMove(newAudioDevices)) after adding a CaptureDevice::isolatedCopy().

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:662
> +    RealtimeMediaSourceCenter::singleton().getMediaStreamDevices([this, weakThis = makeWeakPtr(this), revealIdsAndLabels, completion = WTFMove(completion)] (Vector<CaptureDevice> devices) mutable {

auto&&

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:664
> +        int microphoneCount = 0;

unsigned maybe

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:743
> +            computeFilteredDeviceList(revealIdsAndLabels, [completionHandler = WTFMove(completionHandler), deviceIDHashSalt] (Vector<CaptureDevice>&& devices) mutable {

deviceIDHashSalt = WTFMove(deviceIDHashSalt)

> Source/WebKit/UIProcess/UserMediaProcessManager.cpp:222
> +    WebCore::RealtimeMediaSourceCenter::singleton().getMediaStreamDevices([this, shouldNotify] (Vector<WebCore::CaptureDevice>&& newDevices) mutable {

auto&&

[Jer Noble]

(In reply to youenn fablet from comment #5)
> Comment on attachment 417285 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417285&action=review
> 
> > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:115
> > +void RealtimeMediaSourceCenter::getMediaStreamDevices(CompletionHandler<void(Vector<CaptureDevice>)>&& completion)
> 
> Should probably be CompletionHandler<void(Vector<CaptureDevice>&&)>&&.
> Here and below

Sure.

> > Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:117
> > +    class CaptureDeviceAccumulator : public RefCounted<CaptureDeviceAccumulator> {
> 
> Can we use CallbackAggregator instead?

No, despite its name, CallbackAggregator doesn't aggregate the results passed into the callbacks.

[Jer Noble]

(In reply to youenn fablet from comment #5)
> Comment on attachment 417285 [details]
> > Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:185
> > +        callOnWebThreadOrDispatchAsyncOnMainThread(makeBlockPtr([this, completion = WTFMove(completion), newAudioDevices = WTFMove(newAudioDevices)] () mutable {
> 
> Why not simply callOnMainThread? This code is not expected to work in WebKit
> 1 anyway.

I'm not looking to make any significant architectural changes here; the existing code calls to the WebThread, so I'm going to keep that code in place.

[youenn fablet]

Comment on attachment 417285 [details]
Patch

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

>>> Source/WebCore/platform/mediastream/RealtimeMediaSourceCenter.cpp:117
>>> +    class CaptureDeviceAccumulator : public RefCounted<CaptureDeviceAccumulator> {
>> 
>> Can we use CallbackAggregator instead?
> 
> No, despite its name, CallbackAggregator doesn't aggregate the results passed into the callbacks.

That is a bit sad we do not have a pattern for it.
In any case, I think we should stick with the idea of calling the completion handler when CaptureDeviceAccumulator gets destroyed.
That should remove the need for m_resultsRemaining.

[Jer Noble]

Created attachment 417512 [details]
Patch

[youenn fablet]

Comment on attachment 417512 [details]
Patch

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

> Source/WebCore/platform/mediastream/CaptureDeviceManager.h:37
>      virtual const Vector<CaptureDevice>& captureDevices() = 0;

Can we make captureDevices() private or protected at least?

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:184
> +        auto newAudioDevices = retrieveAudioSessionCaptureDevices();

This is fine as is now but could easily break in the future if we decide to ref/unref some strings there.
I would tend to isolateCopy just in case.
Or we could make retrieveAudioSessionCaptureDevices a free function and pass it the audio session.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:662
> +    RealtimeMediaSourceCenter::singleton().getMediaStreamDevices([this, weakThis = makeWeakPtr(this), revealIdsAndLabels, completion = WTFMove(completion)] (auto&& devices) mutable {

We probably need a weakThis check here.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:743
> +            computeFilteredDeviceList(revealIdsAndLabels, [completionHandler = WTFMove(completionHandler), deviceIDHashSalt = WTFMove(deviceIDHashSalt)] (Vector<CaptureDevice>&& devices) mutable {

auto&&?

> Source/WebKit/UIProcess/UserMediaProcessManager.cpp:222
> +    WebCore::RealtimeMediaSourceCenter::singleton().getMediaStreamDevices([this, shouldNotify] (Vector<WebCore::CaptureDevice>&& newDevices) mutable {

auto&&?

> LayoutTests/ChangeLog:12
> +        does), then the test turns that into a video capture constraint, which fails.

I wonder why we have this change of behavior.
It seems the audiooutput was filtered out previously and is included now. This seems like a progression to me.

As a side bonus, we could make MockAudioCaptureDeviceManager implement getCaptureDevices and return the list of devices asynchronously to validate this code path.
That will make it closer to the real manager and might catch future bugs.

[Jer Noble]

Comment on attachment 417512 [details]
Patch

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

>> Source/WebCore/platform/mediastream/CaptureDeviceManager.h:37
>>      virtual const Vector<CaptureDevice>& captureDevices() = 0;
> 
> Can we make captureDevices() private or protected at least?

Private, probably not. Protected, maybe. I'll see.

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:662
>> +    RealtimeMediaSourceCenter::singleton().getMediaStreamDevices([this, weakThis = makeWeakPtr(this), revealIdsAndLabels, completion = WTFMove(completion)] (auto&& devices) mutable {
> 
> We probably need a weakThis check here.

Ok.

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:743
>> +            computeFilteredDeviceList(revealIdsAndLabels, [completionHandler = WTFMove(completionHandler), deviceIDHashSalt = WTFMove(deviceIDHashSalt)] (Vector<CaptureDevice>&& devices) mutable {
> 
> auto&&?

Sure.

[Jer Noble]

(In reply to Jer Noble from comment #11)
> Comment on attachment 417512 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417512&action=review
> 
> >> Source/WebCore/platform/mediastream/CaptureDeviceManager.h:37
> >>      virtual const Vector<CaptureDevice>& captureDevices() = 0;
> > 
> > Can we make captureDevices() private or protected at least?
> 
> Private, probably not. Protected, maybe. I'll see.

Nope: 

```
./platform/mediastream/RealtimeMediaSourceCenter.cpp:211:79: error: 'captureDevices' is a protected member of 'WebCore::CaptureDeviceManager'
```

I think we'll need to go even further to purge all the synchronous calls from the rest of WebKit before we can do this.

[Jer Noble]

(In reply to youenn fablet from comment #10)
> Comment on attachment 417512 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417512&action=review
> 
> > Source/WebCore/platform/mediastream/CaptureDeviceManager.h:37
> >      virtual const Vector<CaptureDevice>& captureDevices() = 0;
> 
> Can we make captureDevices() private or protected at least?
> 
> > Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:184
> > +        auto newAudioDevices = retrieveAudioSessionCaptureDevices();
> 
> This is fine as is now but could easily break in the future if we decide to
> ref/unref some strings there.
> I would tend to isolateCopy just in case.

```
usr/local/include/wtf/Vector.h:1610:38: No member named 'isolatedCopy' in 'WebCore::AVAudioSessionCaptureDevice'
```

We'd need to implement an isolatedCopy() method on the capture device first; we can do that as a follow up.

[Jer Noble]

Created attachment 417560 [details]
Patch for landing

[EWS]

Committed r271471: <https://trac.webkit.org/changeset/271471>

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

[youenn fablet]

Comment on attachment 417560 [details]
Patch for landing

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

> LayoutTests/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=220493

The title and bug number is wrong here.