[webkit-reviews] review granted: [Bug 233316] Let RemoteAudioMediaStreamTrackRendererInternalUnitManager::Unit switch to VPIO unit if VPIO is running : [Attachment 444950] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 233316: Let RemoteAudioMediaStreamTrackRendererInternalUnitManager::Unit
switch to VPIO unit if VPIO is running
https://bugs.webkit.org/show_bug.cgi?id=233316

Attachment 444950: Patch

https://bugs.webkit.org/attachment.cgi?id=444950&action=review




--- Comment #7 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 444950
  --> https://bugs.webkit.org/attachment.cgi?id=444950
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/RemoteAudioMediaStreamTrackRendererInternalUnit
Manager.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/RemoteAudioMediaStreamTrackRendererInternalUnit
Manager.cpp:261
> +    if (m_isPlaying)
> +	   m_localUnit->start();

Ditto