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

VERIFIED FIXED in Firefox 25

Status

()

Core
WebRTC: Audio/Video
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

({regression})

Trunk
mozilla26
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox25 verified, firefox26 verified)

Details

(Whiteboard: [getUserMedia], URL)

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
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).
(Assignee)

Comment 1

4 years ago
Created attachment 795253 [details] [diff] [review]
Part 1-Refactor MediaStreamTrack disabling so we can call it directly and access from other threads
(Assignee)

Comment 2

4 years ago
Created attachment 795264 [details] [diff] [review]
Part 2 - Allow DOM MediaStreams to intercept SetTrackEnabled calls
(Assignee)

Comment 3

4 years ago
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.
Attachment #795253 - Flags: review?(roc)
(Assignee)

Comment 4

4 years ago
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.
Attachment #795264 - Flags: review?(roc)
Attachment #795253 - Flags: review?(roc) → review+
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?
Attachment #795264 - Flags: review?(roc) → review+
(Assignee)

Comment 6

4 years ago
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
Version: 22 Branch → Trunk
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
(Assignee)

Comment 8

4 years ago
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
https://hg.mozilla.org/mozilla-central/rev/10949a6eac60
https://hg.mozilla.org/mozilla-central/rev/a5e2ad4508be
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 10

4 years ago
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
Attachment #795253 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

4 years ago
Attachment #795264 - Flags: approval-mozilla-aurora?
Waiting on further comment in bug 884365

Updated

4 years ago
Attachment #795264 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 12

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/b558ce6a7217
https://hg.mozilla.org/releases/mozilla-aurora/rev/e203add2f0eb
status-firefox25: --- → fixed
status-firefox26: --- → fixed
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.
Attachment #795253 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Does this need any testing to verify it's working correctly?
Flags: needinfo?(rjesup)
(Assignee)

Comment 15

4 years ago
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!
Flags: needinfo?(rjesup)
Thanks Randell.

Mihaela, please verify this is fixed using the info in comment 15. Thanks.
Keywords: verifyme
QA Contact: mihaela.velimiroviciu
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?
Flags: needinfo?(rjesup)
(Assignee)

Comment 18

4 years ago
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.
Flags: needinfo?(rjesup)
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
Status: RESOLVED → VERIFIED
status-firefox25: fixed → verified
status-firefox26: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.