Bug 228753 - [iOS] getUserMedia sometimes doesn't capture from specified microphone
Summary: [iOS] getUserMedia sometimes doesn't capture from specified microphone
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-08-03 14:49 PDT by Eric Carlson
Modified: 2021-08-06 12:01 PDT (History)
10 users (show)

See Also:


Attachments
Patch (22.40 KB, patch)
2021-08-03 15:19 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (19.15 KB, patch)
2021-08-04 17:24 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (19.17 KB, patch)
2021-08-05 09:40 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing (19.07 KB, patch)
2021-08-05 10:57 PDT, Eric Carlson
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2021-08-03 14:49:46 PDT
getUserMedia sometimes doesn't capture from the non-default microphone.
Comment 1 Eric Carlson 2021-08-03 14:50:08 PDT
rdar://79704226
Comment 2 Eric Carlson 2021-08-03 15:19:58 PDT
Created attachment 434863 [details]
Patch
Comment 3 youenn fablet 2021-08-04 01:34:13 PDT
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.
Comment 4 Eric Carlson 2021-08-04 12:41:58 PDT
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.
Comment 5 Eric Carlson 2021-08-04 13:22:54 PDT
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.
Comment 6 Eric Carlson 2021-08-04 14:07:30 PDT
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
Comment 7 Eric Carlson 2021-08-04 17:24:15 PDT
Created attachment 434954 [details]
Patch
Comment 8 Eric Carlson 2021-08-05 09:40:58 PDT
Created attachment 434997 [details]
Patch
Comment 9 youenn fablet 2021-08-05 09:47:51 PDT
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?
Comment 10 Eric Carlson 2021-08-05 10:57:04 PDT
Created attachment 435007 [details]
Patch for landing
Comment 11 Eric Carlson 2021-08-05 11:01:13 PDT
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.
Comment 12 EWS 2021-08-05 12:29:15 PDT
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].
Comment 14 Eric Carlson 2021-08-06 12:01:44 PDT
(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)