getUserMedia sometimes doesn't capture from the non-default microphone.
rdar://79704226
Created attachment 434863 [details] Patch
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 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 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 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
Created attachment 434954 [details] Patch
Created attachment 434997 [details] Patch
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?
Created attachment 435007 [details] Patch for landing
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.
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].
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
(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)