Bug 220471

Summary: [HANG] 496ms to 1360ms in WebCore::AVAudioSessionCaptureDeviceManager::refreshAudioCaptureDevices()
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, ews-watchlist, glenn, hta, peng.liu6, philipj, sergio, tommyw, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=223476
Attachments:
Description Flags
Patch
none
Patch
youennf: review+
Patch for landing none

Description Jer Noble 2021-01-08 11:35:31 PST
[HANG] 496ms to 1360ms in WebCore::AVAudioSessionCaptureDeviceManager::refreshAudioCaptureDevices()
Comment 1 Jer Noble 2021-01-08 11:46:00 PST
<rdar://72349642>
Comment 2 Jer Noble 2021-01-08 11:54:22 PST
Created attachment 417285 [details]
Patch
Comment 3 Peng Liu 2021-01-08 14:56:55 PST
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.
Comment 4 Jer Noble 2021-01-08 15:34:23 PST
(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.
Comment 5 youenn fablet 2021-01-11 08:31:00 PST
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&&
Comment 6 Jer Noble 2021-01-11 10:09:11 PST
(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.
Comment 7 Jer Noble 2021-01-11 12:55:03 PST
(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.
Comment 8 youenn fablet 2021-01-11 12:57:06 PST
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.
Comment 9 Jer Noble 2021-01-12 23:47:28 PST
Created attachment 417512 [details]
Patch
Comment 10 youenn fablet 2021-01-13 01:34:08 PST
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.
Comment 11 Jer Noble 2021-01-13 09:50:06 PST
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.
Comment 12 Jer Noble 2021-01-13 13:18:58 PST
(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.
Comment 13 Jer Noble 2021-01-13 13:57:44 PST
(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.
Comment 14 Jer Noble 2021-01-13 14:00:16 PST
Created attachment 417560 [details]
Patch for landing
Comment 15 EWS 2021-01-13 16:01:42 PST
Committed r271471: <https://trac.webkit.org/changeset/271471>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417560 [details].
Comment 16 youenn fablet 2021-01-15 02:02:18 PST
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.