Bug 172897 – [MediaStream] Page capture state not reported correctly

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

[Eric Carlson]

Page capture state is not reported correctly when only audio or video is muted.

[Eric Carlson]

<rdar://problem/32493318>

[Eric Carlson]

Created attachment 311935 [details]
Proposed patch.

[youenn fablet]

Comment on attachment 311935 [details]
Proposed patch.

Looks like an improvement.
Would we have an issue if the capturing track is no longer part of the media stream using removeTrack?
Maybe that calls for a more direct access to the capturing sources of the document or maybe better of the WebProcess.

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

> Source/WebCore/Modules/mediastream/MediaStream.cpp:348
> +        if (track->kind() == "audio") {

Probably more efficient to use track->source().type()

> LayoutTests/fast/mediastream/media-stream-track-muted.html:5
> +    <title>Mute video and audio tracks independently, make sure page state updates correctly.</title>

Cool test!

> LayoutTests/fast/mediastream/media-stream-track-muted.html:41
> +            assert_false(internals.pageMediaState().includes('HasMutedAudioCaptureDevice'));

Might want to reject if window.internals is undefined.

> LayoutTests/fast/mediastream/media-stream-track-muted.html:77
> +            testTrack(stream.getAudioTracks()[0], resolve, reject);

You could probably create the promise_test inside testTrack function and then have:
testTrack(stream.getVideoTracks()[0],  "Mute video track only")
testTrack(stream.getAudioTracks()[0],  "Mute audio track only")

[Eric Carlson]

Comment on attachment 311935 [details]
Proposed patch.

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

>> Source/WebCore/Modules/mediastream/MediaStream.cpp:348
>> +        if (track->kind() == "audio") {
> 
> Probably more efficient to use track->source().type()

Fixed.

>> LayoutTests/fast/mediastream/media-stream-track-muted.html:41
>> +            assert_false(internals.pageMediaState().includes('HasMutedAudioCaptureDevice'));
> 
> Might want to reject if window.internals is undefined.

I have written it this way because it is so ridiculously difficult to debug these w3c-style tests. Even though the test can't really run in Safari, this allows me to open it in Safari and find and fix syntax errors.

>> LayoutTests/fast/mediastream/media-stream-track-muted.html:77
>> +            testTrack(stream.getAudioTracks()[0], resolve, reject);
> 
> You could probably create the promise_test inside testTrack function and then have:
> testTrack(stream.getVideoTracks()[0],  "Mute video track only")
> testTrack(stream.getAudioTracks()[0],  "Mute audio track only")

Good idea, that also let me get rid of the global variable.

[Eric Carlson]

Created attachment 311969 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 311969 [details]
Patch for landing.

Clearing flags on attachment: 311969

Committed r217775: <http://trac.webkit.org/changeset/217775>

[Ahmad Saleem]

Checked on Webkit GitHub with BugID and seems to have landed and not backed out:

https://github.com/WebKit/WebKit/commit/d04d046fcb16fe79974831443f8aa4495552eff6

Marking this as "RESOLVED FIXED". Thanks!