909187 - MediaStreamTrack::enabled doesn't work with direct listeners (regression caused by bug 884365)

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

MediaStreamTrack::enabled doesn't work for direct listeners (eg PeerConnection connected to getUserMedia() streams).  To fix this, we need to intercept SetTrackEnable calls applied to the TrackUnion of a gUM stream, and apply the disabling instead to the SourceMediaStream of the UserMediaStream (gUM stream).

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Part 1-Refactor MediaStreamTrack disabling so we can call it directly and access from other threads

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Part 2 - Allow DOM MediaStreams to intercept SetTrackEnabled calls

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 795253 [details] [diff] [review]
Part 1-Refactor MediaStreamTrack disabling so we can call it directly and access from other threads

It seemed cleaner to push the disabling down into the Segment code.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 795264 [details] [diff] [review]
Part 2 - Allow DOM MediaStreams to intercept SetTrackEnabled calls

ForwardTrackEnabled() seemed a reasonable solution to handle the trackid mapping needed.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 795264 [details] [diff] [review]
Part 2 - Allow DOM MediaStreams to intercept SetTrackEnabled calls

Review of attachment 795264 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/TrackUnionStream.h
@@ +120,5 @@
> +  virtual void ForwardTrackEnabled(TrackID aOutputID, bool aEnabled) {
> +    for (int32_t i = mTrackMap.Length() - 1; i >= 0; --i) {
> +      if (mTrackMap[i].mOutputTrackID == aOutputID) {
> +        mTrackMap[i].mInputPort->GetSource()->
> +          SetTrackEnabled(mTrackMap[i].mInputTrackID, aEnabled);

We shouldn't do this in general.

We need a way to just forward this back in the gUM case. Maybe add a flag to TrackUnionStream to indicate whether that should be done, and set the flag when gUM creates its TrackUnionStream?

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Roc and I talked on IRC to explain why this was needed, and that it's only for the GUM TrackUnionStream case, which answers his initial review comment.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0601038e2a31
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b1a4a62635

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b0a6d97b6e26 for causing multiple test failures on Windows:
Mochitest-1: https://tbpl.mozilla.org/php/getParsedLog.php?id=26992620&tree=Mozilla-Inbound
Mochitest-3: https://tbpl.mozilla.org/php/getParsedLog.php?id=26992593&tree=Mozilla-Inbound
Crashtest: https://tbpl.mozilla.org/php/getParsedLog.php?id=26992471&tree=Mozilla-Inbound

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Reland: crashes were due to a push by glandium needing a clobber

https://hg.mozilla.org/integration/mozilla-inbound/rev/10949a6eac60
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5e2ad4508be

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/10949a6eac60
https://hg.mozilla.org/mozilla-central/rev/a5e2ad4508be

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 795253 [details] [diff] [review]
Part 1-Refactor MediaStreamTrack disabling so we can call it directly and access from other threads

[Approval Request Comment]
Regression fix for bug 884365's landing

Bug caused by (feature/regressing bug #): 884365
 
User impact if declined: See bug 884365. If that is landed without this, MediaStreamTrack.enabled = false will appear to work locally, but won't disable sending the track to a remote participant in the call.

Testing completed (on m-c, etc.): On M/C for a while now; tested on a bunch of platforms.

Risk to taking this patch (and alternatives if risky): Very low; just refactors the muting code so we can use it in the new path added by bug 884365.

String or IDL/UUID changes made by this patch: None

Comment 11

[Alex Keybl [:akeybl]]

Waiting on further comment in bug 884365

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b558ce6a7217
https://hg.mozilla.org/releases/mozilla-aurora/rev/e203add2f0eb

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 795253 [details] [diff] [review]
Part 1-Refactor MediaStreamTrack disabling so we can call it directly and access from other threads

Looks like this was missed by accident in Akeybl's last pass through.

Comment 14

[u279076]

Does this need any testing to verify it's working correctly?

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

it can be tested at http://mozilla.github.com/webrtc-landing/pc_test.html (for audio, turn off muting of the large image on the left (yourself) - use a headset!

Comment 16

[u279076]

Thanks Randell.

Mihaela, please verify this is fixed using the info in comment 15. Thanks.

Comment 17

[Mihaela Velimiroviciu (:mihaelav)]

I'm not sure what the expected result should be using the steps from comment #15. After selecting "Unmute" in the left large image, I can hear myself, but that happens also on build prior the fix (Aug 25 Nightly).
Can you please provide more info about the expected result in this case?

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

8/25 wouldn't have the patch for bug 884365.  8/26 might/should have it (and would not have this patch as it uplifted to m-c the morning of 8/26).  Otherwise you'd have to look for non-nightly builds from FTP that include 884265 and not this regression fix.

Comment 19

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:26.0) Gecko/20100101 Firefox/26.0

Mozilla/5.0 (X11; Linux i686; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:25.0) Gecko/20100101 Firefox/25.0

Verified on the latest Nightly and Aurora builds that mute and unmute options work using the guidelines from comment #15.
Marking the bug VERIFIED