Let RemoteAudioMediaStreamTrackRendererInternalUnitManager::Unit switch to VPIO unit if VPIO is running
Created attachment 444668 [details] Patch
There are some issues with this approach. In particular, when microphone is muted through WebKit, we keep using PlayAndRecord but switch to RemoteIO for audio rendering. We then end up in ducked audio, because of PlayAndRecord. We need to investigate this issue further, as it seems setting PlayAndRecord but not starting VPIO should not trigger these audio level jumps.
Created attachment 444845 [details] Patch
Created attachment 444950 [details] Patch
Not only remote audio stream but also playing a media url by audio, video element during a call
<rdar://problem/85750694>
Comment on attachment 444950 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444950&action=review > Source/WebCore/ChangeLog:9 > + We use this in WebKit to render the MediaStreamTrackis of the process doing capture through VPIO when running. s/MediaStreamTrackis/MediaStreamTracks/ > Source/WebCore/platform/mediastream/mac/BaseAudioSharedUnit.cpp:87 > + setShouldProduceMicrophoneSamples(true); Nit: it not only _should_ produce microphone samples, it _is_ producing them, so maybe `setIsProducingMicrophoneSamples` instead? > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:499 > + m_speakerSamplesProducer->captureUnitIsStopped(); Nit: The producer is called after the unit has stopped, so I think `captureUnitHasStopped` would be clearer. > Source/WebKit/GPUProcess/GPUProcess.cpp:516 > +void GPUProcess::notifyStartCapturingAudio(GPUConnectionToWebProcess& process) Nit: this name is slightly awkward, maybe `audioCaptureIsStarting`, or `processIsStartingToCaptureAudio` instead? > Source/WebKit/GPUProcess/webrtc/RemoteAudioMediaStreamTrackRendererInternalUnitManager.cpp:255 > + if (m_isPlaying) > + m_localUnit->stop(); This made me scratch my head for a minute so it might be worth adding a comment about why we stop the local unit when the capture unit starts. > Source/WebKit/GPUProcess/webrtc/RemoteAudioMediaStreamTrackRendererInternalUnitManager.cpp:261 > + if (m_isPlaying) > + m_localUnit->start(); Ditto
Thanks for the review. (In reply to Eric Carlson from comment #7) > Comment on attachment 444950 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=444950&action=review > > > Source/WebCore/ChangeLog:9 > > + We use this in WebKit to render the MediaStreamTrackis of the process doing capture through VPIO when running. > > s/MediaStreamTrackis/MediaStreamTracks/ OK > > Source/WebCore/platform/mediastream/mac/BaseAudioSharedUnit.cpp:87 > > + setShouldProduceMicrophoneSamples(true); > > Nit: it not only _should_ produce microphone samples, it _is_ producing > them, so maybe `setIsProducingMicrophoneSamples` instead? Sounds good. > > Source/WebCore/platform/mediastream/mac/CoreAudioCaptureSource.cpp:499 > > + m_speakerSamplesProducer->captureUnitIsStopped(); > > Nit: The producer is called after the unit has stopped, so I think > `captureUnitHasStopped` would be clearer. OK > > Source/WebKit/GPUProcess/GPUProcess.cpp:516 > > +void GPUProcess::notifyStartCapturingAudio(GPUConnectionToWebProcess& process) > > Nit: this name is slightly awkward, maybe `audioCaptureIsStarting`, or > `processIsStartingToCaptureAudio` instead? processIsStartingToCaptureAudio sounds much better. > > Source/WebKit/GPUProcess/webrtc/RemoteAudioMediaStreamTrackRendererInternalUnitManager.cpp:255 > > + if (m_isPlaying) > > + m_localUnit->stop(); > > This made me scratch my head for a minute so it might be worth adding a > comment about why we stop the local unit when the capture unit starts. > > > Source/WebKit/GPUProcess/webrtc/RemoteAudioMediaStreamTrackRendererInternalUnitManager.cpp:261 > > + if (m_isPlaying) > > + m_localUnit->start(); > > Ditto OK, will add a comment.
Created attachment 445565 [details] Patch
Committed r286650 (244963@main): <https://commits.webkit.org/244963@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 445565 [details].