Bug 171999 – Simplify RealtimeMediaSource data production and state

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[youenn fablet]

RealtimeMediaSource production of data is complex.
We should try to simplify it.

[youenn fablet]

Created attachment 309829 [details]
Patch

[youenn fablet]

Created attachment 309832 [details]
Patch

[youenn fablet]

Created attachment 309915 [details]
Trying to fix GTK

[youenn fablet]

Created attachment 309925 [details]
Patch

[youenn fablet]

Created attachment 309933 [details]
Patch

[Eric Carlson]

Comment on attachment 309933 [details]
Patch

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

> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:89
>      m_muted = muted;
>  
> -    if (muted) {
> -        // FIXME: We need to figure out how to guarantee that at least one black video frame is
> -        // emitted after being muted.
> -        stopProducingData();
> -    } else
> -        startProducingData();
> +    if (muted)
> +        stop();
> +    else
> +        start();
> +
> +    notifyMutedObservers();

It is slightly confusing that we modify m_muted and call notifyMutedObservers() here and in notifyMutedChange. Is there a reason to not call notifyMutedChange from here and have it do both?

[youenn fablet]

Comment on attachment 309933 [details]
Patch

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

>> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:89
>> +    notifyMutedObservers();
> 
> It is slightly confusing that we modify m_muted and call notifyMutedObservers() here and in notifyMutedChange. Is there a reason to not call notifyMutedChange from here and have it do both?

Yes. The only reason is that we preserve setting m_muted before calling start/stop.
But maybe it is better to call start/stop unconditionally and then call notifyMutedChange to do the reset of the work.
I'll try this and see whether that breaks anything.

[youenn fablet]

Created attachment 310161 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 310161 [details]
Patch for landing

Rejecting attachment 310161 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 310161, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Tools/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/3745960

[youenn fablet]

Created attachment 310181 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 310181 [details]
Patch for landing

Clearing flags on attachment: 310181

Committed r216898: <http://trac.webkit.org/changeset/216898>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/32231650>