Bug 170519 – [MediaStream] Host application should be able to mute and unmute media streams

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

[Eric Carlson]

Host application should be able to mute and unmute media streams

[Eric Carlson]

<rdar://problem/31174326>

[Eric Carlson]

Created attachment 306308 [details]
Proposed patch.

[Eric Carlson]

Created attachment 306321 [details]
Updated patch.

[Eric Carlson]

Created attachment 306323 [details]
Updated patch.

[Build Bot]

Attachment 306323 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:94:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Comment on attachment 306323 [details]
Updated patch.

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

> Source/WebCore/Modules/mediastream/MediaStream.cpp:303
> +    m_private->setExternallyMuted(document->page()->isMediaCaptureMuted());

What would be the difference between setExternallyMuted and setMuted?
Can we rename it to setMuted or setMediaCaptureMuted?

> Source/WebCore/dom/Document.cpp:2270
> +    if (page() && m_mediaState != MediaProducer::IsNotPlaying) {

What does this change is covering?

> Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp:138
> +        track->stopProducingData();

Do we need to store m_externallyMuted in MediaStreamPrivate?
It seems like that muted is really a track thing.

s/m_externallyMuted/m_isExternallyMuted/ also?

> Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp:195
> +    m_externallyMuted = muted;

Why not calling setMuted directly to tracks without using m_externallyMuted?

[Eric Carlson]

Comment on attachment 306323 [details]
Updated patch.

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

>> Source/WebCore/Modules/mediastream/MediaStream.cpp:303
>> +    m_private->setExternallyMuted(document->page()->isMediaCaptureMuted());
> 
> What would be the difference between setExternallyMuted and setMuted?
> Can we rename it to setMuted or setMediaCaptureMuted?

Good point, renamed to setMuted.

>> Source/WebCore/dom/Document.cpp:2270
>> +    if (page() && m_mediaState != MediaProducer::IsNotPlaying) {
> 
> What does this change is covering?

We can't push the new media state to the page after detachFromFrame is called, so it is possible for state changes to be lost (mentioned in the ChangeLog).

>> Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp:138
>> +        track->stopProducingData();
> 
> Do we need to store m_externallyMuted in MediaStreamPrivate?
> It seems like that muted is really a track thing.
> 
> s/m_externallyMuted/m_isExternallyMuted/ also?

If we don't store the state in MediaStreamPrivate we won't be able to mute tracks added after the stream is muted.

[Eric Carlson]

Created attachment 306326 [details]
Patch for landing.

[youenn fablet]

> If we don't store the state in MediaStreamPrivate we won't be able to mute
> tracks added after the stream is muted.

All capture tracks are attached to at least one stream so that the capture tracks should be muted anyway?

What about the case of tracks being muted and another gum is being done, should the new tracks be muted also?

[Build Bot]

Comment on attachment 306323 [details]
Updated patch.

Attachment 306323 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3481181

New failing tests:
imported/w3c/web-platform-tests/html/browsers/windows/browsing-context.html

[Build Bot]

Created attachment 306330 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Eric Carlson]

(In reply to Build Bot from comment #10)
> Comment on attachment 306323 [details]
> Updated patch.
> 
> Attachment 306323 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.webkit.org/results/3481181
> 
> New failing tests:
> imported/w3c/web-platform-tests/html/browsers/windows/browsing-context.html

This test failure is unrelated:

-FAIL Check the history.length of the created browsing context assert_equals: The history.length should be 1. expected 1 but got 100
+FAIL Check the history.length of the created browsing context assert_equals: The history.length should be 1. expected 1 but got 24

[Eric Carlson]

(In reply to youenn fablet from comment #9)
> > If we don't store the state in MediaStreamPrivate we won't be able to mute
> > tracks added after the stream is muted.
> 
> All capture tracks are attached to at least one stream so that the capture
> tracks should be muted anyway?
> 
> What about the case of tracks being muted and another gum is being done,
> should the new tracks be muted also?

Both are good points.

[WebKit Commit Bot]

Comment on attachment 306326 [details]
Patch for landing.

Clearing flags on attachment: 306326

Committed r214976: <http://trac.webkit.org/changeset/214976>

[Eric Carlson]

Created attachment 306334 [details]
Updated patch for landing.

[WebKit Commit Bot]

Comment on attachment 306334 [details]
Updated patch for landing.

Rejecting attachment 306334 [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-02', 'apply-attachment', '--no-update', '--non-interactive', 306334, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
cted.txt
Hunk #1 FAILED at 15.
Hunk #2 FAILED at 26.
2 out of 2 hunks FAILED -- saving rejects to file LayoutTests/fast/mediastream/MediaStream-page-muted-expected.txt.rej
patching file LayoutTests/fast/mediastream/MediaStream-page-muted.html
Hunk #1 FAILED at 27.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/fast/mediastream/MediaStream-page-muted.html.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

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

[Eric Carlson]

(In reply to Eric Carlson from comment #13)
> (In reply to youenn fablet from comment #9)
> > > If we don't store the state in MediaStreamPrivate we won't be able to mute
> > > tracks added after the stream is muted.
> > 
> > All capture tracks are attached to at least one stream so that the capture
> > tracks should be muted anyway?
> > 
> > What about the case of tracks being muted and another gum is being done,
> > should the new tracks be muted also?
> 
> Both are good points.

The bot landed the patch before I could change these, will fix in a follow up.

[Eric Carlson]

Plus https://trac.webkit.org/r214980 to address Youenn's comments.

[Ryan Haddad]

This change appears to have caused LayoutTest http/tests/media/media-stream/disconnected-frame.html to crash:

https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r215038%20(463)/results.html

[Eric Carlson]

(In reply to Ryan Haddad from comment #19)
> This change appears to have caused LayoutTest
> http/tests/media/media-stream/disconnected-frame.html to crash:
> 
> https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/
> r215038%20(463)/results.html

I am fixing this now.

[Eric Carlson]

Plus https://trac.webkit.org/r215042 to fix the test crash.