Closed
Bug 1166183
Opened 9 years ago
Closed 9 years ago
Back out the direct listener removal landed by mistake in bug 1141781
Categories
(Core :: WebRTC: Audio/Video, defect, P1)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla41
backlog | webrtc/webaudio+ |
People
(Reporter: padenot, Assigned: padenot)
References
Details
(Keywords: regression, Whiteboard: [sync])
Attachments
(3 files)
1.69 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-release+
Sylvestre
:
approval-mozilla-esr38+
|
Details | Diff | Splinter Review |
1.41 KB,
patch
|
pehrsons
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-release+
Sylvestre
:
approval-mozilla-esr38+
|
Details | Diff | Splinter Review |
1.66 KB,
patch
|
bwc
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-release+
Sylvestre
:
approval-mozilla-esr38+
|
Details | Diff | Splinter Review |
I somehow landed a patch I was using for testing the full-duplex stuff, along with the patch from bug 1141781, that disabled the direct listener thing for gUM. I'm going to back myself out on all branches (down to beta, this did not make it to release), since it seems to break rotation on b2g. That said, we should reconsider what we are doing here, because if we have not seen the difference while dog fooding, we might as well remove the direct listener thing altogether. The MSG refactoring completely changed the game here, it's not clear to me it's still a win (although it might be, depending on the platforms). This will get removed when full-duplex supports eventually lands, in any case, since it will clearly don't make sense anymore.
Assignee | ||
Comment 1•9 years ago
|
||
This got irc-r+ed by jesup.
Assignee | ||
Comment 2•9 years ago
|
||
And apparently it might be the cause of other problems like drifts, another reason to backout.
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8607398 [details] [diff] [review] Back out the direct listener removal landed by mistake in bug 1141781 Approval Request Comment [Feature/regressing bug #]: bug 1141781 [User impact if declined]: see comment 1, it break rotation on b2g, and might be the cause for a clock drift leading to a quality issue on some configurations [Describe test coverage new/current, TreeHerder]: same coverage [Risks and why]: little risk, the code did not move much between the unintentional removing and the re-adding [String/UUID change made/needed]: none
Attachment #8607398 -
Flags: approval-mozilla-beta?
Attachment #8607398 -
Flags: approval-mozilla-aurora?
Comment 5•9 years ago
|
||
Comment on attachment 8607398 [details] [diff] [review] Back out the direct listener removal landed by mistake in bug 1141781 Sure. Should be in the next nightly and in the first 39 beta.
Attachment #8607398 -
Flags: approval-mozilla-beta?
Attachment #8607398 -
Flags: approval-mozilla-beta+
Attachment #8607398 -
Flags: approval-mozilla-aurora?
Attachment #8607398 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 6•9 years ago
|
||
This looks like it's permafail on m-3 for some reason. Someone must have landed something in the meantime that makes weird assumptions.
Comment 7•9 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #6) > This looks like it's permafail on m-3 for some reason. Someone must have > landed something in the meantime that makes weird assumptions. yeah the perma failure is/was https://treeherder.mozilla.org/logviewer.html#?job_id=9959839&repo=mozilla-inbound and its backed out now
Assignee | ||
Comment 9•9 years ago
|
||
After a quite painful `hg bisect` session, it came down to this:
> The first bad revision is:
> changeset: 240531:d49f149a7c76
> user: Andreas Pehrson <pehrsons@gmail.com>
> date: Wed Apr 22 11:59:43 2015 +0800
> summary: Bug 1155089 - Part 3: Test replacing with WebAudio track in
> track_peerConnection_replaceTrack.html. r=jib
Andreas, do you know why we would observer a timeout in your test with the patch attached landed? Maybe it's because the bug where you have to addref the gUM stream when using it with Web Audio or it get garbage collected?
Flags: needinfo?(pehrsons)
Assignee | ||
Comment 11•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c234d6821b2
Updated•9 years ago
|
Comment 12•9 years ago
|
||
Confirmed with a local build that this fixes bug 1157932 for me.
Blocks: 1157932
Comment 13•9 years ago
|
||
Comment on attachment 8607632 [details] [diff] [review] Work around bug 934512 in track_peerConnection_replaceTrack.html. r= Review of attachment 8607632 [details] [diff] [review]: ----------------------------------------------------------------- Sure, this works. Any clue to why using the direct listener again would cause this to permafail? ::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html @@ +31,5 @@ > var newtrack; > var audiotrack; > return navigator.mediaDevices.getUserMedia({video:true, audio:true, fake:true}) > .then(newstream => { > + window.grip = newstream; A comment mentioning bug 934512 here would be nice.
Attachment #8607632 -
Flags: review?(pehrsons) → review+
Updated•9 years ago
|
Flags: needinfo?(pehrsons)
Assignee | ||
Comment 14•9 years ago
|
||
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #13) > Comment on attachment 8607632 [details] [diff] [review] > Work around bug 934512 in track_peerConnection_replaceTrack.html. r= > > Review of attachment 8607632 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sure, this works. Any clue to why using the direct listener again would > cause this to permafail? Your new test changes a lot of timing, and new a CC/GC is triggered most of the time, and the gum stream gets collected. > ::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html > @@ +31,5 @@ > > var newtrack; > > var audiotrack; > > return navigator.mediaDevices.getUserMedia({video:true, audio:true, fake:true}) > > .then(newstream => { > > + window.grip = newstream; > > A comment mentioning bug 934512 here would be nice. For sure.
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c163ecde3b7f https://hg.mozilla.org/integration/mozilla-inbound/rev/a243da0786ca
Comment 16•9 years ago
|
||
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9999455&repo=mozilla-inbound
Flags: needinfo?(padenot)
Comment 17•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/c6e91068a086 https://hg.mozilla.org/integration/mozilla-inbound/rev/9ea0669a75d8
Assignee | ||
Comment 18•9 years ago
|
||
Andreas, looking at this deeper after the backout, it appeared to me that replaceTrack does not work with DirectListeners. When running the test, and replacing the gum track by an OscillatorNode at 2kHz, the AnalyserNode rapidly goes silent on the receiving side, while having the correct tone at the sending side. The fact that it goes silent over a period of time is because of the windowing of the AnalyserNode: 359 INFO Comparing maxima; input[93] = 255, output[46] = 200 360 INFO Comparing maxima; input[94] = 255, output[46] = 193 361 INFO Comparing maxima; input[94] = 255, output[46] = 187 362 INFO Comparing maxima; input[94] = 255, output[46] = 179 363 INFO Comparing maxima; input[94] = 255, output[46] = 172 364 INFO Comparing maxima; input[94] = 255, output[46] = 165 365 INFO Comparing maxima; input[94] = 255, output[46] = 158 366 INFO Comparing maxima; input[94] = 255, output[46] = 151 367 INFO Comparing maxima; input[94] = 255, output[46] = 144 368 INFO Comparing maxima; input[94] = 255, output[46] = 137 369 INFO Comparing maxima; input[94] = 255, output[46] = 130 370 INFO Comparing maxima; input[94] = 255, output[46] = 123 371 INFO Comparing maxima; input[94] = 255, output[46] = 116 372 INFO Comparing maxima; input[94] = 255, output[46] = 109 373 INFO Comparing maxima; input[94] = 255, output[46] = 102 374 INFO Comparing maxima; input[94] = 255, output[46] = 95 375 INFO Comparing maxima; input[94] = 255, output[46] = 88 376 INFO Comparing maxima; input[94] = 255, output[46] = 81 377 INFO Comparing maxima; input[94] = 255, output[46] = 74 378 INFO Comparing maxima; input[94] = 255, output[46] = 66 379 INFO Comparing maxima; input[94] = 255, output[46] = 59 380 INFO Comparing maxima; input[94] = 255, output[46] = 52 381 INFO Comparing maxima; input[94] = 255, output[46] = 45 382 INFO Comparing maxima; input[94] = 255, output[46] = 38 383 INFO Comparing maxima; input[94] = 255, output[46] = 31 384 INFO Comparing maxima; input[94] = 255, output[46] = 24 385 INFO Comparing maxima; input[94] = 255, output[46] = 17 386 INFO Comparing maxima; input[94] = 255, output[46] = 10 387 INFO Comparing maxima; input[94] = 255, output[46] = 3 388 INFO Comparing maxima; input[94] = 255, output[1023] = 0 Note the indices: 94 at the sending side, which is probably 2kHz, 46Hz at the receiving side, which is probably 1kHz. Maybe you need to complete your initial patch so that we reroute a direct listener to flow through the MSG when switching from direct gUM -> PC to do WebAudio -> PC ?
Flags: needinfo?(padenot) → needinfo?(pehrsons)
Comment 19•9 years ago
|
||
I see, I'll take a look ASAP. I'll attach a patch to this bug so they can land/be uplifted/whatever else (as needed) together.
Flags: needinfo?(pehrsons)
Comment 20•9 years ago
|
||
We keep the same MediaStreamListener after replacing tracks in MediaPipeline, but we don't reset the flag tracking whether we're in direct mode or not.
Comment 21•9 years ago
|
||
Attachment #8608499 -
Flags: review?(docfaraday)
Updated•9 years ago
|
Attachment #8608499 -
Flags: review?(docfaraday) → review+
Assignee | ||
Updated•9 years ago
|
status-firefox38:
--- → affected
status-firefox38.0.5:
--- → affected
status-firefox39:
--- → affected
status-firefox40:
--- → affected
status-firefox41:
--- → affected
Comment 23•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ea3e2adcdc https://hg.mozilla.org/integration/mozilla-inbound/rev/7f8efd416237 https://hg.mozilla.org/integration/mozilla-inbound/rev/6a60cf57f923
Comment 24•9 years ago
|
||
Comment on attachment 8607398 [details] [diff] [review] Back out the direct listener removal landed by mistake in bug 1141781 Approval Request Comment (for this patch and the minor test/etc patches included here) [Feature/regressing bug #]: bug 1141781 [User impact if declined]: Some users (dependent on hardware, OS, and load) will experience major audio drift in webrtc calls - delays can reach a second in 10-30 minutes, or even more. Users who are affected basically will find webrtc unusable. [Describe test coverage new/current, TreeHerder]: DirectListener can't easily be tested directly because it depends on an input/output drift to exist (and even then would be hard to test). The one in-house person who's been able top reliably reproduce it (drno) has verified this fixes the problem for him. [Risks and why]: exceedingly low risk [String/UUID change made/needed]: none
Attachment #8607398 -
Flags: approval-mozilla-release?
Updated•9 years ago
|
Attachment #8607632 -
Flags: approval-mozilla-release?
Attachment #8607632 -
Flags: approval-mozilla-beta?
Attachment #8607632 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Attachment #8608499 -
Flags: approval-mozilla-release?
Attachment #8608499 -
Flags: approval-mozilla-beta?
Attachment #8608499 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Attachment #8607398 -
Flags: approval-mozilla-release? → approval-mozilla-release+
Updated•9 years ago
|
Attachment #8607632 -
Flags: approval-mozilla-release?
Attachment #8607632 -
Flags: approval-mozilla-release+
Attachment #8607632 -
Flags: approval-mozilla-beta?
Attachment #8607632 -
Flags: approval-mozilla-beta+
Attachment #8607632 -
Flags: approval-mozilla-aurora?
Attachment #8607632 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8608499 -
Flags: approval-mozilla-release?
Attachment #8608499 -
Flags: approval-mozilla-release+
Attachment #8608499 -
Flags: approval-mozilla-beta?
Attachment #8608499 -
Flags: approval-mozilla-beta+
Attachment #8608499 -
Flags: approval-mozilla-aurora?
Attachment #8608499 -
Flags: approval-mozilla-aurora+
Comment 25•9 years ago
|
||
Major regression, taking it in a potential 38.0.5 new RC.
Assignee | ||
Comment 27•9 years ago
|
||
It's in mozilla-release: https://hg.mozilla.org/releases/mozilla-release/file/0aae20c40e62/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#l637
Comment 28•9 years ago
|
||
[Tracking Requested - why for this release]: Seems like we need this on esr38 as well.
status-firefox-esr38:
--- → affected
tracking-firefox-esr38:
--- → ?
Comment 29•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/9cf746bab003 https://hg.mozilla.org/releases/mozilla-aurora/rev/6c4418d3c0ea https://hg.mozilla.org/releases/mozilla-aurora/rev/6c5bd5f85e20
Comment 30•9 years ago
|
||
(In reply to Paul Adenot (:padenot) from comment #27) > It's in mozilla-release: > https://hg.mozilla.org/releases/mozilla-release/file/0aae20c40e62/media/ > webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#l637 When did they land ? I don't see anything here which suggests it had.
Flags: needinfo?(padenot)
Comment 31•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/edd045aad951 https://hg.mozilla.org/releases/mozilla-beta/rev/7311bd491c30 https://hg.mozilla.org/releases/mozilla-beta/rev/d68fc8a46c1a
Comment 32•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-release/rev/9525d8723413 https://hg.mozilla.org/releases/mozilla-release/rev/6ee39f753d19 https://hg.mozilla.org/releases/mozilla-release/rev/19b9ffda6e2f
Comment 33•9 years ago
|
||
This has mochitest-3 permafail on beta and release. Will back out later if nobody fixes in the mean time.
Comment 34•9 years ago
|
||
I have a fix. Effectively it's a mis-merge (newstream->newStream - the test symbol changed between aurora and beta)
Flags: needinfo?(rjesup)
Comment 35•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-release/rev/3f4bd4277a6a Bustage/mis-merge fix (test-only fix) -- in the test, newStream in aurora and later is newstream in beta and release. verified locally.
Comment 36•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f7ea3e2adcdc
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 37•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7f8efd416237 https://hg.mozilla.org/mozilla-central/rev/6a60cf57f923
Assignee | ||
Comment 38•9 years ago
|
||
Clearing NEEDINFO, thanks to everybody involved for the a+ and uplifts!
Flags: needinfo?(padenot)
Comment 39•9 years ago
|
||
Paul, is there anything manual QA could do to help verify this fix? We're currently running a few smoke tests on WebRTC in general, but a few steps to confirm this specific fix would be useful.
Flags: needinfo?(padenot)
Assignee | ||
Comment 40•9 years ago
|
||
Andrei, Nils has confirmed it works, we should be good.
Flags: needinfo?(padenot)
Comment 41•9 years ago
|
||
I've also verified this on nightly and dev-edition by going through a scenario mentioned in bug 1157932.
Assignee | ||
Comment 43•9 years ago
|
||
It does, I thought the landing in 38.0.5 implied that it would go in ESR, but the timing must have been bad.
Flags: needinfo?(padenot)
Updated•9 years ago
|
Attachment #8607398 -
Flags: approval-mozilla-esr38+
Updated•9 years ago
|
Attachment #8607632 -
Flags: approval-mozilla-esr38+
Updated•9 years ago
|
Attachment #8608499 -
Flags: approval-mozilla-esr38+
Updated•9 years ago
|
Comment 44•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr38/rev/b1f5175eb8f2 https://hg.mozilla.org/releases/mozilla-esr38/rev/b6207942c59f https://hg.mozilla.org/releases/mozilla-esr38/rev/917403490208
You need to log in
before you can comment on or make changes to this bug.
Description
•