Back out the direct listener removal landed by mistake in bug 1141781

RESOLVED FIXED in Firefox 38.0.5

Status

()

defect
P1
normal
Rank:
5
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

({regression})

unspecified
mozilla41
Points:
---

Firefox Tracking Flags

(firefox38 wontfix, firefox38.0.5 fixed, firefox39 fixed, firefox40 verified, firefox41 verified, firefox-esr3839+ fixed)

Details

(Whiteboard: [sync])

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
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 2

4 years ago
And apparently it might be the cause of other problems like drifts, another reason to backout.
(Assignee)

Comment 4

4 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 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

4 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.
(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

4 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)

Updated

4 years ago
backlog: --- → webRTC+
Rank: 5
Keywords: regression
Priority: -- → P1
Whiteboard: [sync]
Confirmed with a local build that this fixes bug 1157932 for me.
Blocks: 1157932
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+
Flags: needinfo?(pehrsons)
(Assignee)

Comment 14

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

Comment 18

4 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)
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)
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.
Attachment #8608499 - Flags: review?(docfaraday) → review+
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?
Attachment #8607632 - Flags: approval-mozilla-release?
Attachment #8607632 - Flags: approval-mozilla-beta?
Attachment #8607632 - Flags: approval-mozilla-aurora?
Attachment #8608499 - Flags: approval-mozilla-release?
Attachment #8608499 - Flags: approval-mozilla-beta?
Attachment #8608499 - Flags: approval-mozilla-aurora?
Attachment #8607398 - Flags: approval-mozilla-release? → approval-mozilla-release+
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+
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+
Major regression, taking it in a potential 38.0.5 new RC.
I think this regression is only in 39. Jesup?
Flags: needinfo?(rjesup)
[Tracking Requested - why for this release]: Seems like we need this on esr38 as well.
(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)
This has mochitest-3 permafail on beta and release. Will back out later if nobody fixes in the mean time.
I have a fix.  Effectively it's a mis-merge (newstream->newStream - the test symbol changed between aurora and beta)
Flags: needinfo?(rjesup)
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.
https://hg.mozilla.org/mozilla-central/rev/f7ea3e2adcdc
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Comment 38

4 years ago
Clearing NEEDINFO, thanks to everybody involved for the a+ and uplifts!
Flags: needinfo?(padenot)
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

4 years ago
Andrei, Nils has confirmed it works, we should be good.
Flags: needinfo?(padenot)
I've also verified this on nightly and dev-edition by going through a scenario mentioned in bug 1157932.
Does this need to land on esr38?
Flags: needinfo?(padenot)
(Assignee)

Comment 43

4 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)
Attachment #8607398 - Flags: approval-mozilla-esr38+
Attachment #8607632 - Flags: approval-mozilla-esr38+
Attachment #8608499 - Flags: approval-mozilla-esr38+
Duplicate of this bug: 1151060
You need to log in before you can comment on or make changes to this bug.