Bug 233316 – Let RemoteAudioMediaStreamTrackRendererInternalUnitManager::Unit switch to VPIO unit if VPIO is running

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

[youenn fablet]

Let RemoteAudioMediaStreamTrackRendererInternalUnitManager::Unit switch to VPIO unit if VPIO is running

[youenn fablet]

Created attachment 444668 [details]
Patch

[youenn fablet]

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.

[youenn fablet]

Created attachment 444845 [details]
Patch

[youenn fablet]

Created attachment 444950 [details]
Patch

[Phong Le]

Not only remote audio stream but also playing a media url by audio, video element during a call

[Radar WebKit Bug Importer]

<rdar://problem/85750694>

[Eric Carlson]

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

[youenn fablet]

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.

[youenn fablet]

Created attachment 445565 [details]
Patch

[EWS]

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].