Bug 228753 – [iOS] getUserMedia sometimes doesn't capture from specified microphone

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[Eric Carlson]

getUserMedia sometimes doesn't capture from the non-default microphone.

[Eric Carlson]

rdar://79704226

[Eric Carlson]

Created attachment 434863 [details]
Patch

[youenn fablet]

Comment on attachment 434863 [details]
Patch

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

> Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:130
> +    else if (captureCount || preparingToCapture) {

Is there a possibility we go through that state:
preparingToCapture = true, captureCount = false -> PlayAndRecord
preparingToCapture = false, captureCount = false -> Not PlayAndRecord
preparingToCapture = false, captureCount = true -> PlayAndRecord again

I wonder whether preparingToCapture mechanism might not get sometimes broken leading to flaky PlayAndRecord value in GPUProcess.

Now that we are doing capture in GPUProcess, maybe we should let know the GPUProcess AudioSession that we are doing capture (using startProducing/stopProducing signal).
Then the GPUProcess AudioSession would actually set its parameters based on what WebProcess and GPUProcess capture say.
It seems easier to manipulate the real AudioSession to handle capture in GPUProcess than at distance in WebProcess.

> Source/WebCore/platform/audio/ios/AudioSessionIOS.mm:303
> +        RELEASE_LOG_ERROR(Media, "failed to set preferred buffer duration to %f with error: %@", duration, error.localizedDescription);

RELEASE_LOG_ERROR_IF(error, ...)

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:151
> +        if (deviceUID == String(portDescription.UID)) {

Maybe better to convert deviceUID to NSString or const char* once instead of converting every portDescription.UID to String?

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:158
> +        RELEASE_LOG_ERROR(WebRTC, "failed to find preferred input '%s'", deviceUID.ascii().data());

Do we want %{public}s?

> Source/WebCore/platform/mediastream/ios/AVAudioSessionCaptureDeviceManager.mm:164
> +        RELEASE_LOG_ERROR(WebRTC, "failed to set preferred input to '%s' with error: %@", deviceUID.ascii().data(), error.localizedDescription);

Do we want %{public}s?

> Source/WebKit/ChangeLog:12
> +        capture begins.

The Mac wk2 debug tests failures seem relevant, it seems the PlayAndRecord category is not always set with this code path.

[Eric Carlson]

Comment on attachment 434863 [details]
Patch

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

>> Source/WebKit/ChangeLog:12
>> +        capture begins.
> 
> The Mac wk2 debug tests failures seem relevant, it seems the PlayAndRecord category is not always set with this code path.

The assert:

    ASSERT(!DeprecatedGlobalSettings::shouldManageAudioSessionCategory() || AudioSession::sharedSession().category() == AudioSession::CategoryType::PlayAndRecord);

is invalid because `AudioSession::sharedSession()` doesn't return the audio session in use by the GPU Process, where the RemoteAudioSessionProxyManager which owns an audio session. It looks like this is a more general problem.

[Eric Carlson]

Comment on attachment 434863 [details]
Patch

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

>>> Source/WebKit/ChangeLog:12
>>> +        capture begins.
>> 
>> The Mac wk2 debug tests failures seem relevant, it seems the PlayAndRecord category is not always set with this code path.
> 
> The assert:
> 
>     ASSERT(!DeprecatedGlobalSettings::shouldManageAudioSessionCategory() || AudioSession::sharedSession().category() == AudioSession::CategoryType::PlayAndRecord);
> 
> is invalid because `AudioSession::sharedSession()` doesn't return the audio session in use by the GPU Process, where the RemoteAudioSessionProxyManager which owns an audio session. It looks like this is a more general problem.

Actually, the assert is valid, the problem is that RemoteAudioSessionProxyManager creates an uses an AudioSession. This is bad because any code that moves from the WP to the GPUP will continue to use `AudioSession::sharedSession` so we'll end up with two session objects in the GPUP.

[Eric Carlson]

Comment on attachment 434863 [details]
Patch

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

>>>> Source/WebKit/ChangeLog:12
>>>> +        capture begins.
>>> 
>>> The Mac wk2 debug tests failures seem relevant, it seems the PlayAndRecord category is not always set with this code path.
>> 
>> The assert:
>> 
>>     ASSERT(!DeprecatedGlobalSettings::shouldManageAudioSessionCategory() || AudioSession::sharedSession().category() == AudioSession::CategoryType::PlayAndRecord);
>> 
>> is invalid because `AudioSession::sharedSession()` doesn't return the audio session in use by the GPU Process, where the RemoteAudioSessionProxyManager which owns an audio session. It looks like this is a more general problem.
> 
> Actually, the assert is valid, the problem is that RemoteAudioSessionProxyManager creates an uses an AudioSession. This is bad because any code that moves from the WP to the GPUP will continue to use `AudioSession::sharedSession` so we'll end up with two session objects in the GPUP.

https://bugs.webkit.org/show_bug.cgi?id=228795

[Eric Carlson]

Created attachment 434954 [details]
Patch

[Eric Carlson]

Created attachment 434997 [details]
Patch

[youenn fablet]

Comment on attachment 434997 [details]
Patch

We need to think of whether changing the default device should fire a devicechange event.
As per spec, I would say yes, and it might make sense in MacOS. But let's leave that to a follow-up

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

> Source/WebCore/platform/audio/PlatformMediaSessionManager.cpp:122
> +    for (const auto& source : m_audioCaptureSources) {

Could we use anyOf maybe?

[Eric Carlson]

Created attachment 435007 [details]
Patch for landing

[Eric Carlson]

Comment on attachment 434997 [details]
Patch

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

Thanks for the review!

>> Source/WebCore/platform/audio/PlatformMediaSessionManager.cpp:122
>> +    for (const auto& source : m_audioCaptureSources) {
> 
> Could we use anyOf maybe?

Good idea, changed.

[EWS]

Committed r280702 (240298@main): <https://commits.webkit.org/240298@main>

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

[Truitt Savell]

it looks like the changes in https://trac.webkit.org/changeset/280702/webkit

broke two tests:
fast/speechrecognition/start-recognition-twice-exception.html [ Crash ]
fast/text/international/system-language/navigator-language/navigator-language-en-GB.html [ Crash ]

History:
https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=fast%2Fspeechrecognition%2Fstart-recognition-twice-exception.html&test=fast%2Ftext%2Finternational%2Fsystem-language%2Fnavigator-language%2Fnavigator-language-en-GB.html

Log: https://ews-build.s3-us-west-2.amazonaws.com/macOS-AppleSilicon-Big-Sur-Debug-WK2-Tests-EWS/r435044-9395-rerun/fast/speechrecognition/start-recognition-twice-exception-crash-log.txt

[Eric Carlson]

(In reply to Truitt Savell from comment #13)
> it looks like the changes in https://trac.webkit.org/changeset/280702/webkit
> 
> broke two tests:
> fast/speechrecognition/start-recognition-twice-exception.html [ Crash ]
> fast/text/international/system-language/navigator-language/navigator-
> language-en-GB.html [ Crash ]
> 
> History:
> https://results.webkit.org/?suite=layout-tests&suite=layout-
> tests&test=fast%2Fspeechrecognition%2Fstart-recognition-twice-exception.
> html&test=fast%2Ftext%2Finternational%2Fsystem-language%2Fnavigator-
> language%2Fnavigator-language-en-GB.html
> 
> Log:
> https://ews-build.s3-us-west-2.amazonaws.com/macOS-AppleSilicon-Big-Sur-
> Debug-WK2-Tests-EWS/r435044-9395-rerun/fast/speechrecognition/start-
> recognition-twice-exception-crash-log.txt

That was fixed by r280722 (bug 228847)