Bug 171999 - Simplify RealtimeMediaSource data production and state
Summary: Simplify RealtimeMediaSource data production and state
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-11 16:38 PDT by youenn fablet
Modified: 2017-05-16 14:21 PDT (History)
3 users (show)

See Also:


Attachments
Patch (48.97 KB, patch)
2017-05-11 17:15 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (48.93 KB, patch)
2017-05-11 17:30 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Trying to fix GTK (51.00 KB, patch)
2017-05-12 10:51 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (51.27 KB, patch)
2017-05-12 12:17 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (51.84 KB, patch)
2017-05-12 13:11 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (56.15 KB, patch)
2017-05-15 13:58 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (51.78 KB, patch)
2017-05-15 16:35 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-05-11 16:38:44 PDT
RealtimeMediaSource production of data is complex.
We should try to simplify it.
Comment 1 youenn fablet 2017-05-11 17:15:21 PDT
Created attachment 309829 [details]
Patch
Comment 2 youenn fablet 2017-05-11 17:30:38 PDT
Created attachment 309832 [details]
Patch
Comment 3 youenn fablet 2017-05-12 10:51:21 PDT
Created attachment 309915 [details]
Trying to fix GTK
Comment 4 youenn fablet 2017-05-12 12:17:34 PDT
Created attachment 309925 [details]
Patch
Comment 5 youenn fablet 2017-05-12 13:11:58 PDT
Created attachment 309933 [details]
Patch
Comment 6 Eric Carlson 2017-05-15 09:54:41 PDT
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?
Comment 7 youenn fablet 2017-05-15 09:59:16 PDT
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.
Comment 8 youenn fablet 2017-05-15 13:58:36 PDT
Created attachment 310161 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2017-05-15 14:00:09 PDT
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
Comment 10 youenn fablet 2017-05-15 16:35:08 PDT
Created attachment 310181 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2017-05-15 19:45:32 PDT
Comment on attachment 310181 [details]
Patch for landing

Clearing flags on attachment: 310181

Committed r216898: <http://trac.webkit.org/changeset/216898>
Comment 12 WebKit Commit Bot 2017-05-15 19:45:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2017-05-16 14:21:47 PDT
<rdar://problem/32231650>