Bug 220010

Summary: [iOS] Do extensive search for microphone devices when trying to capture
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebRTCAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, ews-watchlist, glenn, hta, jer.noble, peng.liu6, philipj, sergio, tommyw, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Updated patch
none
Updated patch
none
Patch none

Description youenn fablet 2020-12-18 00:42:35 PST
[iOS] Do extensive search for microphone devices when trying to capture.
Currently, we do an extensive search immediately when enumerateDevices is called.
It is better to wait for getUserMedia calls.
Comment 1 youenn fablet 2020-12-18 03:19:55 PST
Created attachment 416506 [details]
Patch
Comment 2 youenn fablet 2020-12-18 03:29:25 PST
Created attachment 416507 [details]
Patch
Comment 3 youenn fablet 2020-12-18 04:53:00 PST
Created attachment 416509 [details]
Patch
Comment 4 youenn fablet 2020-12-18 09:22:10 PST
Created attachment 416523 [details]
Patch
Comment 5 youenn fablet 2020-12-18 10:10:39 PST
Created attachment 416527 [details]
Patch
Comment 6 Peng Liu 2020-12-18 10:51:36 PST
Comment on attachment 416527 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        exhaustive audio device query if no other page is needed it.

s/is needed/needs/
Comment 7 Eric Carlson 2020-12-18 11:11:35 PST
Comment on attachment 416527 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Extensive audio device query needs to enable the AudioSession.

s/needs to enable the AudioSession/needs to have an active AudioSession/

> Source/WebCore/ChangeLog:10
> +        To work around these side effects, we add an API that allows to control whether activating the audio session or not.

s/control whether activating the audio session or not/control whether the AudioSession is active or not/

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:219
> +        RELEASE_LOG_ERROR(WebRTC, "Failed to disactivate audio session with error: %@.", error.localizedDescription);

s/disactivate/deactivate/
Comment 8 Eric Carlson 2020-12-18 16:58:43 PST
<rdar://problem/72036034>
Comment 9 Eric Carlson 2020-12-18 17:02:27 PST
Created attachment 416559 [details]
Updated patch
Comment 10 Eric Carlson 2020-12-18 17:12:53 PST
Created attachment 416560 [details]
Updated patch
Comment 11 youenn fablet 2020-12-21 02:30:06 PST
Comment on attachment 416560 [details]
Updated patch

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

> Source/WebCore/PAL/pal/cocoa/AVFoundationSoftLink.h:290
> +#define AVAudioSessionModeVideoRecording PAL::get_AVFoundation_AVAudioSessionModeVideoRecording()

Unclear why we want this change.

> Source/WebCore/PAL/pal/cocoa/AVFoundationSoftLink.mm:216
> +SOFT_LINK_CONSTANT_FOR_SOURCE_WITH_EXPORT(PAL, AVFoundation, AVAudioSessionModeVideoRecording, NSString *, PAL_EXPORT)

Unclear why we want this change.

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:92
> +    dispatch_async(m_dispatchQueue, makeBlockPtr([this, locker = holdLock(m_lock)] {

Sounds good enough for now. We might want to document this in ChangeLog though.
Ideally, we would use a completion handler.

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:104
> +            RELEASE_LOG_ERROR(WebRTC, "Failed to set audio session category with error: %@.", error.localizedDescription);

In theory, we could release the dispatch queue once we are done there.

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:160
> +    auto locker = holdLock(m_lock);

We could reduce the time where we hold the lock by doing
{
    auto locker = holdLock(m_lock);
}

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:163
> +    do {

I do not think we want this do loop here since we are locking above.
Comment 12 youenn fablet 2020-12-21 06:58:58 PST
Created attachment 416599 [details]
Patch
Comment 13 EWS 2020-12-22 02:11:49 PST
Committed r271049: <https://trac.webkit.org/changeset/271049>

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