Bug 221608 – Split RemoteRealtimeMediaSource in two audio-specific and video-specific classes

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

[youenn fablet]

Split RemoteRealtimeMediaSource in two audio-specific and video-specific classes

[youenn fablet]

Created attachment 419711 [details]
Patch

[youenn fablet]

Created attachment 419725 [details]
Patch

[Eric Carlson]

Comment on attachment 419725 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419725&action=review

> Source/WebKit/WebProcess/cocoa/RemoteRealtimeAudioSource.cpp:131
> +void RemoteRealtimeAudioSource::whenReady(CompletionHandler<void(String)>&& callback)
> +{
> +    if (m_isReady)
> +        return callback(WTFMove(m_errorMessage));
> +    m_callback = WTFMove(callback);
> +}
> +
> +void RemoteRealtimeAudioSource::didFail(String&& errorMessage)
> +{
> +    m_isReady = true;
> +    m_errorMessage = WTFMove(errorMessage);
> +    if (m_callback)
> +        m_callback(m_errorMessage);
> +}
> +
> +void RemoteRealtimeAudioSource::setAsReady()
> +{
> +    m_isReady = true;
> +    if (m_callback)
> +        m_callback({ });
> +}
> +
> +void RemoteRealtimeAudioSource::setCapabilities(RealtimeMediaSourceCapabilities&& capabilities)
> +{
> +    m_capabilities = WTFMove(capabilities);
> +}
> +
> +void RemoteRealtimeAudioSource::setSettings(RealtimeMediaSourceSettings&& settings)
> +{
> +    auto changed = m_settings.difference(settings);
> +    m_settings = WTFMove(settings);
> +    notifySettingsDidChangeObservers(changed);
> +}
> +

These methods are identical in the Audio and Video classes, can they be moved into a base class?

> Source/WebKit/WebProcess/cocoa/RemoteRealtimeAudioSource.cpp:182
> +IPC::Connection* RemoteRealtimeAudioSource::connection()
> +{
> +    ASSERT(isMainThread());
> +#if ENABLE(GPU_PROCESS)
> +    if (m_shouldCaptureInGPUProcess)
> +        return &WebProcess::singleton().ensureGPUProcessConnection().connection();
> +#endif
> +    return WebProcess::singleton().parentProcessConnection();
> +}
> +
> +void RemoteRealtimeAudioSource::startProducingData()
> +{
> +    connection()->send(Messages::UserMediaCaptureManagerProxy::StartProducingData { m_identifier }, 0);
> +}
> +
> +void RemoteRealtimeAudioSource::stopProducingData()
> +{
> +    connection()->send(Messages::UserMediaCaptureManagerProxy::StopProducingData { m_identifier }, 0);
> +}
> +
> +const WebCore::RealtimeMediaSourceCapabilities& RemoteRealtimeAudioSource::capabilities()
> +{
> +    return m_capabilities;
> +}
> +
> +void RemoteRealtimeAudioSource::applyConstraints(const MediaConstraints& constraints, ApplyConstraintsHandler&& completionHandler)
> +{
> +    m_pendingApplyConstraintsCallbacks.append(WTFMove(completionHandler));
> +    // FIXME: Use sendAsyncWithReply.
> +    connection()->send(Messages::UserMediaCaptureManagerProxy::ApplyConstraints { m_identifier, constraints }, 0);
> +}
> +
> +void RemoteRealtimeAudioSource::applyConstraintsSucceeded(RealtimeMediaSourceSettings&& settings)
> +{
> +    setSettings(WTFMove(settings));
> +
> +    auto callback = m_pendingApplyConstraintsCallbacks.takeFirst();
> +    callback({ });
> +}
> +
> +void RemoteRealtimeAudioSource::applyConstraintsFailed(String&& failedConstraint, String&& errorMessage)
> +{
> +    auto callback = m_pendingApplyConstraintsCallbacks.takeFirst();
> +    callback(ApplyConstraintsError { WTFMove(failedConstraint), WTFMove(errorMessage) });
> +}

Ditto

> Source/WebKit/WebProcess/cocoa/RemoteRealtimeAudioSource.cpp:215
> +void RemoteRealtimeAudioSource::captureStopped()
> +{
> +    stop();
> +    hasEnded();
> +}
> +
> +void RemoteRealtimeAudioSource::captureFailed()
> +{
> +    RealtimeMediaSource::captureFailed();
> +    hasEnded();
> +}
> +
> +#if ENABLE(GPU_PROCESS)
> +void RemoteRealtimeAudioSource::gpuProcessConnectionDidClose(GPUProcessConnection&)
> +{
> +    ASSERT(m_shouldCaptureInGPUProcess);
> +    if (isEnded())
> +        return;
> +
> +    m_manager.remoteCaptureSampleManager().didUpdateSourceConnection(*this);
> +    createRemoteMediaSource();
> +    // FIXME: We should update the track according current settings.
> +    if (isProducingData())
> +        startProducingData();
> +}

Ditto.

> Source/WebKit/WebProcess/cocoa/UserMediaCaptureManager.cpp:118
> +    if (auto source = m_audioSources.get(id))
>          source->captureStopped();
> +    else if (auto source = m_videoSources.get(id))
> +        source->captureStopped();

This pattern is used in enough places this it would be good to have a method that returned the source given an identifier.

[youenn fablet]

I will look at sharing more code in a follow-up patch since the plan is for RemoteRealtimeVideoSource to become a RealtimeVideoCaptureSource.

[EWS]

Committed r272719: <https://commits.webkit.org/r272719>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 419725 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74227645>