[webkit-reviews] review granted: [Bug 220471] [HANG] 496ms to 1360ms in WebCore::AVAudioSessionCaptureDeviceManager::refreshAudioCaptureDevices() : [Attachment 417512] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jan 13 01:34:08 PST 2021


youenn fablet <youennf at gmail.com> has granted Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 220471: [HANG] 496ms to 1360ms in
WebCore::AVAudioSessionCaptureDeviceManager::refreshAudioCaptureDevices()
https://bugs.webkit.org/show_bug.cgi?id=220471

Attachment 417512: Patch

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




--- Comment #10 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 417512
  --> https://bugs.webkit.org/attachment.cgi?id=417512
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:1
84
> +	   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.


More information about the webkit-reviews mailing list