Closed
Bug 909187
Opened 11 years ago
Closed 11 years ago
MediaStreamTrack::enabled doesn't work with direct listeners (regression caused by bug 884365)
Categories
(Core :: WebRTC: Audio/Video, defect)
Core
WebRTC: Audio/Video
Tracking
()
VERIFIED
FIXED
mozilla26
People
(Reporter: jesup, Assigned: jesup)
References
()
Details
(Keywords: regression, Whiteboard: [getUserMedia])
Attachments
(2 files)
5.98 KB,
patch
|
roc
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
6.21 KB,
patch
|
roc
:
review+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 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•11 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•11 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•11 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
Comment 9•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/10949a6eac60 https://hg.mozilla.org/mozilla-central/rev/a5e2ad4508be
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•11 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•11 years ago
|
Attachment #795264 -
Flags: approval-mozilla-aurora?
Comment 11•11 years ago
|
||
Waiting on further comment in bug 884365
Updated•11 years ago
|
Attachment #795264 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 12•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/b558ce6a7217 https://hg.mozilla.org/releases/mozilla-aurora/rev/e203add2f0eb
status-firefox25:
--- → fixed
Updated•11 years ago
|
status-firefox26:
--- → fixed
Comment 13•11 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 Looks like this was missed by accident in Akeybl's last pass through.
Attachment #795253 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 14•11 years ago
|
||
Does this need any testing to verify it's working correctly?
Flags: needinfo?(rjesup)
Assignee | ||
Comment 15•11 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)
Comment 16•11 years ago
|
||
Thanks Randell. Mihaela, please verify this is fixed using the info in comment 15. Thanks.
Keywords: verifyme
QA Contact: mihaela.velimiroviciu
Comment 17•11 years ago
|
||
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•11 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)
Comment 19•11 years ago
|
||
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
Keywords: verifyme
You need to log in
before you can comment on or make changes to this bug.
Description
•