Bug 170519 - [MediaStream] Host application should be able to mute and unmute media streams
Summary: [MediaStream] Host application should be able to mute and unmute media streams
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-05 13:30 PDT by Eric Carlson
Modified: 2017-04-06 10:21 PDT (History)
6 users (show)

See Also:


Attachments
Proposed patch. (16.86 KB, patch)
2017-04-05 13:48 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch. (7.74 KB, patch)
2017-04-05 14:57 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch. (19.06 KB, patch)
2017-04-05 15:05 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing. (18.95 KB, patch)
2017-04-05 15:40 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (859.89 KB, application/zip)
2017-04-05 16:09 PDT, Build Bot
no flags Details
Updated patch for landing. (18.63 KB, patch)
2017-04-05 16:33 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2017-04-05 13:30:59 PDT
Host application should be able to mute and unmute media streams
Comment 1 Eric Carlson 2017-04-05 13:31:39 PDT
<rdar://problem/31174326>
Comment 2 Eric Carlson 2017-04-05 13:48:05 PDT
Created attachment 306308 [details]
Proposed patch.
Comment 3 Eric Carlson 2017-04-05 14:57:02 PDT
Created attachment 306321 [details]
Updated patch.
Comment 4 Eric Carlson 2017-04-05 15:05:40 PDT
Created attachment 306323 [details]
Updated patch.
Comment 5 Build Bot 2017-04-05 15:06:44 PDT
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.
Comment 6 youenn fablet 2017-04-05 15:25:03 PDT
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?
Comment 7 Eric Carlson 2017-04-05 15:37:06 PDT
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.
Comment 8 Eric Carlson 2017-04-05 15:40:27 PDT
Created attachment 306326 [details]
Patch for landing.
Comment 9 youenn fablet 2017-04-05 15:41:51 PDT
> 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?
Comment 10 Build Bot 2017-04-05 16:09:01 PDT
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
Comment 11 Build Bot 2017-04-05 16:09:02 PDT
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
Comment 12 Eric Carlson 2017-04-05 16:16:08 PDT
(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
Comment 13 Eric Carlson 2017-04-05 16:17:06 PDT
(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.
Comment 14 WebKit Commit Bot 2017-04-05 16:22:24 PDT
Comment on attachment 306326 [details]
Patch for landing.

Clearing flags on attachment: 306326

Committed r214976: <http://trac.webkit.org/changeset/214976>
Comment 15 Eric Carlson 2017-04-05 16:33:54 PDT
Created attachment 306334 [details]
Updated patch for landing.
Comment 16 WebKit Commit Bot 2017-04-05 16:34:43 PDT
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
Comment 17 Eric Carlson 2017-04-05 16:36:44 PDT
(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.
Comment 18 Eric Carlson 2017-04-05 17:05:15 PDT
Plus https://trac.webkit.org/r214980 to address Youenn's comments.
Comment 19 Ryan Haddad 2017-04-06 10:13:48 PDT
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
Comment 20 Eric Carlson 2017-04-06 10:15:37 PDT
(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.
Comment 21 Eric Carlson 2017-04-06 10:21:57 PDT
Plus https://trac.webkit.org/r215042 to fix the test crash.