1219711 - Disabling/muting Media tracks no longer works

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

STR:

1) Plug in some headphones
2) Load http://mozilla.github.io/webrtc-landing/pc_test.html
3) Click start and share a camera & microphone
4) On the big remote video, click the volume button to unmute it.

=> Audio & video is playing fine.

5) Select either the "Disable audio" or "Disable video" options

Expected Results

=> Audio is stopped, or Video is black (depending on which was pressed)

Actual Results

=> Audio keeps playing, or Video keeps playing

Similar steps also work in Firefox Hello, where I've also verified that when using FF 44 at one end of the call and any of FF 41 - 43 at the other, muting on the 44 end doesn't stop the audio being sent, but muting the other end (FF 41, 42 or 43) mutes that side.

Comment 1

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:

Thanks for filing this, Mark.  Do you know if the smoke tests for Hello (done by softvision) include checking mute functionality?  If not, we should add it -- or I can ask for it to be added to the WebRTC smoke tests that get run when a release goes to Dev Edition.

Randell -- Can you help me dig into this?  We'll need to get this fixed ASAP (it's a privacy issue) -- before Dev Edition gets pushed out to users next week.

Comment 2

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:  Mute functionality in WebRTC broke during the Fx44 Nightly cycle.  We'll need to fix this ASAP and get it uplifted into Fx44.

Comment 3

[Mark Banner (:standard8)]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #1)
> Thanks for filing this, Mark.  Do you know if the smoke tests for Hello
> (done by softvision) include checking mute functionality?  If not, we should
> add it -- or I can ask for it to be added to the WebRTC smoke tests that get
> run when a release goes to Dev Edition.

I didn't know softvision did general smoke tests for Hello, so I'll pass this onto Bogdan who probably does know.

Comment 4

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

Looks like bug 1170958 broke track disabling for DirectListeners (i.e. PeerConnections).  I find that the disabling of the track is only recorded for the TrackUnion, and not the SourceMediaStream, so applying the disabling at AppendToTrack does nothing.  This worked until the Sep 30th checkins it appears.

Disabling DirectListeners resolves this (though uncovers a bug that after disable/enable, there's a fractional-second delay in remote video).

The disabling has to filter down to the source (or be grabbed and applied to the source instead), or we need to remove DirectListeners (and resolve all implications of this).

Comment 5

[Andreas Pehrson [:pehrsons]]

Do we not have any automated tests of this? :(

Anyway, I'll take a look. And throw a test in.

Keeping the ni? as a reminder.

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1219711 - Add mochitest for track disabling over a peer connection. r?jib

Bug 1219711 - Add mochitest for track disabling over a peer connection. r?jib

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1219711 - Don't rely on timer interval in fake audio track. r?jesup

Bug 1219711 - Ensure MediaStreamTrack.enabled propagates across peer connections. r?jesup

Comment 8

[Andreas Pehrson [:pehrsons]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37b5a5e6ce1c

Comment 9

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

Comment on attachment 8681088 [details]
MozReview Request: Bug 1219711 - Don't rely on timer interval in fake audio track. r?jesup

https://reviewboard.mozilla.org/r/23761/#review21239

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8681087 [details]
MozReview Request: Bug 1219711 - Add mochitest for track disabling over a peer connection. r?jib

https://reviewboard.mozilla.org/r/23759/#review21237

::: dom/media/tests/mochitest/test_peerConnection_trackDisabling.html:28
(Diff revision 1)
> +  test.chain.append([

Do we need to run the entire chain for this?

::: dom/media/tests/mochitest/test_peerConnection_trackDisabling.html:29
(Diff revision 1)
> +    function CHECK_ASSUMPTIONS() {

Why not have a single function instead of 11? There's no reuse here, and the combination of named functions and promises has shown itself to be hazardous (forgetting `return`). 

This would also let us define variables where they are initialized.

::: dom/media/tests/mochitest/test_peerConnection_trackDisabling.html:49
(Diff revision 1)
> +      return h.waitForPixel(video, h.green, 128,
> +                            "Local video should be green before disabling");
> +    },
> +    function CHECK_ENABLED_VIDEO_REMOTE() {
> +      var video = test.pcRemote.mediaElements[0];
> +      return h.waitForPixel(video, h.green, 128,
> +                            "Remote video should be green before disabling");

Neat, though couldn't these run in parallel?

Comment 11

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/23759/#review21237

> Do we need to run the entire chain for this?

We have to ensure that the connection is established and live at least. What's in there that we shouldn't run? Do we care?

> Why not have a single function instead of 11? There's no reuse here, and the combination of named functions and promises has shown itself to be hazardous (forgetting `return`). 
> 
> This would also let us define variables where they are initialized.

When debugging a log I really want to easily track where in the test we are, and the named functions provide that by the framework's step logging. I also had some more monolithic functions pushed back in favour of more named functions before (though not by you).

> Neat, though couldn't these run in parallel?

Maybe. I think the helper uses one canvas for drawing all the waitForPixel video element tests, but that's probably synchronous and ok. It doesn't take long to run though so I'd prefer the improved logging of running them in sequence over doing it in parallel.

Comment 12

[Andreas Pehrson [:pehrsons]]

Try is failing on linux x64 when checking for enabled local audio: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37b5a5e6ce1c

Is an explicit `fake: true` not enough to get the fake track when there's a fake device available? jib, jesup do any of you know?

The fake audio track has 1kHz audio (though not very smooth for some reason, should suffice for this test), the media device has 440Hz audio according to the log. I see a hint of the debug canvas in the screenshot; it looks like less than the 1k I get when running it locally, I suspect it's at 440Hz.

Comment 13

[Andreas Pehrson [:pehrsons]]

Here's a try push to test the 440Hz theory: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e96383c5abaf

Comment 14

[Eric Rescorla (:ekr)]

fake: true should give you a fake track regardless of whether there is a device available.

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #11)
> > Do we need to run the entire chain for this?
> 
> We have to ensure that the connection is established and live at least.
> What's in there that we shouldn't run? Do we care?

Some stats and sdp validation. I usually cut things off after PC_REMOTE_WAIT_FOR_MEDIA_FLOW.

> I also had some more monolithic functions pushed back in favour of more
> named functions before (though not by you).

Different strokes... info() works for me, though I would burn down the chain in favor of promise chains if they let me. :)

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Note that on Linux only, mochitests use special loopback devices instead of the regular fake devices. They hook in at a lower level of the stack for deeper testing.

To circumvent their use, use { fake: true, faketracks: true } as a workaround. This works because faketracks is a feature exclusive to the regular built-in fake devices, so specifying faketracks ensures you get the built-in fake ones instead of the loopback ones (not great, but there you go).

This culminates on code here [1].

[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp?rev=b53b26799619&mark=1418-1437#1403

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a196aee79c60

Comment 18

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

Attachment: Add mochitest for track disabling over a peer connection

With faketracks:true

Comment 19

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a84e7b7c36ef

Comment 20

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

jib: no joy with faketracks: true

Comment 21

[Andreas Pehrson [:pehrsons]]

I thought faketracks: true was only to get a stream with more than one audio track. And the same for video. I still don't understand how my test that also accepts 440Hz is passing while it doesn't for 1kHz-only. It seems to take quite a while to finish as well (7s opt, 13s debug, in that try run) so I suspect that we might be eventually passing on some noise, but it could also be the machine being a bit slow. I just know it's a lot faster on my machine.

Comment 22

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #21) It seems to
> take quite a while to finish as well (7s opt, 13s debug, in that try run) so
> I suspect that we might be eventually passing on some noise, but it could
> also be the machine being a bit slow. I just know it's a lot faster on my
> machine.

To compare, MacOSX debug was <5s, Win8 debug was 3.5s.

Comment 23

[Andreas Pehrson [:pehrsons]]

For comparison, this forces fake tracks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60c41534909d

Comment 24

[Andreas Pehrson [:pehrsons]]

After chatting with jib on IRC I now understand that the presence of loopback devices will take precedence over fake: true unless you also have faketracks: true, in which case you also end up with a stream with multiple audio and video tracks.

Thus it sounds like there is no convenient way of ensuring that you get a media stream with one fake audio and one fake video track, which is what I want here.

I'm proposing to turn things around so that fake: true (controllable by the test writer, who knows best what he needs) takes precedence over loopback devices (set by the test framework, so they always exist for linux in automation) and I'll soon put up a patch to prove it. I just want to see it pass try first: https://treeherder.mozilla.org/#/jobs?repo=try&revision=29e117d4230b

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a196aee79c60

Comment 26

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #24)
> I'm proposing to turn things around so that fake: true (controllable by the
> test writer, who knows best what he needs) takes precedence over loopback
> devices (set by the test framework, so they always exist for linux in
> automation) and I'll soon put up a patch to prove it.

I like the idea, but I don't think the code you have will work. I remember it took me 10+ tries to get that if statement down to where it satisfied every test.

I propose we distinguish between the two ways of turning on fake:

 1. Caller passes in { fake: true } constraint
 2. Environment pref "media.navigator.streams.fake"

by excluding loopback devices in the former case, but not the latter. I also think we should keep the existing faketracks: behavior, to minimize upheaval right now.

This should give us the control we need. We'd want to go through and weed out any remaining places that set { fake: true } needlessly (remember "media.navigator.streams.fake" is on by default in the mochitest harness).

Once that is done, we can use { fake: true } in your test only, to make sure you don't suffer the loopback devices. Does that sound good?

To execute this, we'll need to change the two places MediaManager reads "media.navigator.streams.fake" (one in gUM the other in EnumerateDevices):

http://mxr.mozilla.org/mozilla-central/search?string=media.navigator.streams.fake&filter=[Mm]edia.navigator.streams.fake

One remaining caveat is that this leaves no way to exclude loopback devices for enumerateDevices since that method doesn't take constraints, but hope that's not a problem.

Comment 27

[Andreas Pehrson [:pehrsons]]

I'm OK with that.

A comment on getting my code passing all tests; I had a bug in it that I fixed in this try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=acd61ea31f5f

Now it looks pretty consistent, and we're only failing the constraints test (which I haven't looked at yet).

My biggest concern however is that it takes 30+ seconds for my new test to pass on linux32-opt, linux32-debug and linux64-debug.

Comment 28

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #27)
> My biggest concern however is that it takes 30+ seconds for my new test to
> pass on linux32-opt, linux32-debug and linux64-debug.

How does that compare to average? Would cutting off the tail of the chain help?

Comment 29

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/23759/#review21387

Comment 30

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #27)
> I'm OK with that.

OK are you making the changes, or do you want me to?

Comment 31

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #28)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #27)
> > My biggest concern however is that it takes 30+ seconds for my new test to
> > pass on linux32-opt, linux32-debug and linux64-debug.
> 
> How does that compare to average? Would cutting off the tail of the chain
> help?

For linux32-opt in [1] this test took 8-9 seconds (OK, but still a bit high) 7 times and ~30 seconds 8 times. The only thing I can really attribute the long times to is audio noise. That the 1kHz tone is a bit weakened after AEC or something like that.

I'll increase the tolerance to see how that performs.. That might give some clues.


(In reply to Jan-Ivar Bruaroey [:jib] from comment #30)
> (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #27)
> > I'm OK with that.
> 
> OK are you making the changes, or do you want me to?

I can do it. Though first I'd like to try to fix the last test failure I'm seeing on try. It sounded in comment 26 like you are in favor of my proposed behavior if only I can get it to work.

Comment 32

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681088 [details]
MozReview Request: Bug 1219711 - Don't rely on timer interval in fake audio track. r?jesup

Approval Request Comment
[Feature/regressing bug #]: Bug 1170958
[User impact if declined]: Audio and video streams appear to have been blocked (locally they are) while the remote side is still receiving them when streamed over WebRTC.
[Describe test coverage new/current, TreeHerder]: Manual testing. We're working out the wrinkles for landing a mochitest on central, but aurora should be stable enough to just take the fix.
[Risks and why]: Low. Fix is simple and comprehensible.
[String/UUID change made/needed]: None

Comment 33

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #31)
> (In reply to Jan-Ivar Bruaroey [:jib] from comment #28)
> > (In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #27)
> > > My biggest concern however is that it takes 30+ seconds for my new test to
> > > pass on linux32-opt, linux32-debug and linux64-debug.
> > 
> > How does that compare to average? Would cutting off the tail of the chain
> > help?
> 
> For linux32-opt in [1] this test took 8-9 seconds (OK, but still a bit high)
> 7 times and ~30 seconds 8 times. The only thing I can really attribute the
> long times to is audio noise. That the 1kHz tone is a bit weakened after AEC
> or something like that.

It could actually be the video taking that much time. I'm comparing to green with a pretty high tolerance, but if the video goes completely blue or red we have to wait for it to come back to green... Maybe a not-black test is more appropriate. I'll see how easy that is to write.

Comment 34

[Andreas Pehrson [:pehrsons]]

Screwed up a comma here, but now it looks good (all linux runs are 6-10s so far): https://treeherder.mozilla.org/#/jobs?repo=try&revision=19638dd23e98

I was right in comment 33 then.

Comment 35

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681088 [details]
MozReview Request: Bug 1219711 - Don't rely on timer interval in fake audio track. r?jesup

Bug 1219711 - Ensure MediaStreamTrack.enabled propagates across peer connections. r?jesup

Comment 36

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681087 [details]
MozReview Request: Bug 1219711 - Add mochitest for track disabling over a peer connection. r?jib

Bug 1219711 - Add mochitest for track disabling over a peer connection. r?jib

Comment 37

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1219711 - Let fake stream take precedence in testing. r?jib

Bug 1219711 - Let fake stream take precedence in testing. r?jib
This is how I think fake streams should work.

TL;DR requesting a fake stream always gives you a fake stream. No magic.

The gUMConstraint `fake: true` should take precedence and if set always
use MediaEngineDefault.
If it is set the state of `faketracks` is passed
on to MediaEngineDefault.
If it is not set, but (any of) audio/video loopback devices are set, the
device enumeration will filter out only those.

Comment 38

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1219711 - Remove fakeness from webrtc tests. r?jib

Bug 1219711 - Remove fakeness from webrtc tests. r?jib

Comment 39

[Andreas Pehrson [:pehrsons]]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f70cdffe2fb0

Comment 40

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8681087 [details]
MozReview Request: Bug 1219711 - Add mochitest for track disabling over a peer connection. r?jib

https://reviewboard.mozilla.org/r/23759/#review21511

::: dom/canvas/test/captureStream_common.js:67
(Diff revision 2)
> -  waitForPixel: function (video, refColor, threshold, infoString) {
> +  testPixelNot: function (video, refData, threshold) {
> +    var ctxout = this.cout.getContext('2d');
> +    ctxout.drawImage(video, 0, 0);
> +    var pixel = ctxout.getImageData(0, 0, 1, 1).data;
> +    return pixel.some((val, i) => Math.abs(val - refData[i]) <= 255 - threshold);
> +  },

Refactor suggestion: I would cut the callback API at var pixel, since the compare function on var pixel seems to be the only difference between testPixel and testPixelNot, and would fit neatly in an arrow function passed in at the call site, and remove the need for bind.

::: dom/canvas/test/captureStream_common.js:79
(Diff revision 2)
> -        if (this.testPixel(video, refColor.data, threshold)) {
> +        if (fun.bind(this)(video, refColor.data, threshold)) {

I dislike bind in general, and especially bind at a distance. Refactor above would take care of this, or...

::: dom/canvas/test/captureStream_common.js:94
(Diff revision 2)
> +    return this.waitForPixelImpl(this.testPixel, video, refColor, threshold, infoString);

If we don't refactor, we could avoid bind-at-a-distance like this (though ugly):

    return this.waitForPixelImpl((a,b,c) => this.testPixel(a,b,c), video, refColor, threshold, infoString);

::: dom/media/tests/mochitest/test_peerConnection_trackDisabling.html:75
(Diff revision 2)
> +    function DISABLE_VIDEO() {
> +      localStream.getVideoTracks()[0].enabled = false;
> +    },
> +    function CHECK_DISABLED_VIDEO_LOCAL() {
> +      var video = test.pcLocal.mediaElements[0];
> +      // Threshold has to be low due to fake tracks having a semi-transparent
> +      // time indicator sometimes filling the top left corner.
> +      return h.waitForPixel(video, h.black, 10,
> +                            "Local video should become black after disabling");
> +    },
> +    function CHECK_DISABLED_VIDEO_REMOTE() {
> +      var video = test.pcRemote.mediaElements[0];
> +      // Threshold has to be low due to fake tracks having a semi-transparent
> +      // time indicator sometimes filling the top left corner.
> +      return h.waitForPixel(video, h.black, 10,
> +                            "Remote video should become black after disabling");
> +    },

Refactor suggestion: This breakdown seems excessive to me, and everything is done twice. Why not:

    localStream.getVideoTracks()[0].enabled = false;

    // Threshold has to be low due to fake tracks having a semi-transparent
    // time indicator sometimes filling the top left corner.
    [test.pcLocal, test.pcRemote].forEach(pc =>
      h.waitForPixel(pc.mediaElements[0], h.black, 10,
                     pc.label + " should become black after disabling"));

Comment 41

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8682352 [details]
MozReview Request: Bug 1219711 - Let fake stream take precedence in testing. r?jib

https://reviewboard.mozilla.org/r/24057/#review21523

::: dom/media/MediaManager.cpp:1419
(Diff revision 1)
> -  if (!aFakeTracks) {
> -    if (aVideoType == dom::MediaSourceEnum::Camera) {
> -      audioLoopDev = Preferences::GetCString("media.audio_loopback_dev");
> +  if (!aFake) {
> +    // Fake stream not requested. The entire device stack is available.
> +    // Loop in loopback devices if they are set, and their respective type is

As I tried to explain in comment 26, I don't think this will work, because:

 1. "media.navigator.streams.fake" defaults to true in the tree (all platforms).
 2. "media.(video|audio)_loopback_dev" are set on linux only.

The intent is for the majority of tests to use the "built-in" fake devices on all platforms except linux, where loopback devices are to be used instead. Said differently: the loopback devices are also "fake".

With this change the loopback devices will never run, I fear, because "fake" is always set. See comment 26 for my recommendation.

::: dom/media/MediaManager.cpp:1426
(Diff revision 1)
> -    } else {
> -      aFake = false;
> +    if (aAudioType == MediaSourceEnum::Microphone) {
> +      audioLoopDev = Preferences::GetCString("media.audio_loopback_dev");
>      }

This is good.

Comment 42

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8682353 [details]
MozReview Request: Bug 1219711 - Remove fakeness from webrtc tests. r?jib

https://reviewboard.mozilla.org/r/24059/#review21527

lgtm.

Comment 43

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/24057/#review21523

> As I tried to explain in comment 26, I don't think this will work, because:
> 
>  1. "media.navigator.streams.fake" defaults to true in the tree (all platforms).
>  2. "media.(video|audio)_loopback_dev" are set on linux only.
> 
> The intent is for the majority of tests to use the "built-in" fake devices on all platforms except linux, where loopback devices are to be used instead. Said differently: the loopback devices are also "fake".
> 
> With this change the loopback devices will never run, I fear, because "fake" is always set. See comment 26 for my recommendation.

Your assumption in point 1 is wrong. See https://dxr.mozilla.org/mozilla-central/rev/9f69202d82752e093a653a8f15b0274e347db33a/dom/media/tests/mochitest/head.js#276

Comment 44

[Bogdan Maris, Desktop Test Engineering]

(In reply to Mark Banner (:standard8) from comment #3)
> (In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #1)
> > Thanks for filing this, Mark.  Do you know if the smoke tests for Hello
> > (done by softvision) include checking mute functionality?  If not, we should
> > add it -- or I can ask for it to be added to the WebRTC smoke tests that get
> > run when a release goes to Dev Edition.
> 
> I didn't know softvision did general smoke tests for Hello, so I'll pass
> this onto Bogdan who probably does know.

Yes, we are doing some general Hello smoke tests for Hello but only at the beginning of a new beta cycle. We don't have a specific Mute test but I can make sure it will be added.

Comment 45

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #43)
> Your assumption in point 1 is wrong.

Oh, good. Happy to be wrong there. :)

Comment 46

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8682352 [details]
MozReview Request: Bug 1219711 - Let fake stream take precedence in testing. r?jib

https://reviewboard.mozilla.org/r/24057/#review21637

Comment 47

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/23759/#review21511

> Refactor suggestion: I would cut the callback API at var pixel, since the compare function on var pixel seems to be the only difference between testPixel and testPixelNot, and would fit neatly in an arrow function passed in at the call site, and remove the need for bind.

I did a slight overhaul, see what you think.

Comment 48

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #42)
> Comment on attachment 8682353 [details]
> MozReview Request: Bug 1219711 - Remove fakeness from webrtc tests. r?jib
> 
> https://reviewboard.mozilla.org/r/24059/#review21527
> 
> lgtm.

I had to add fake: false to the basicTabShare test to make it pass. Not taking it for re-review though, so just FYI.

Comment 49

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681088 [details]
MozReview Request: Bug 1219711 - Don't rely on timer interval in fake audio track. r?jesup

Bug 1219711 - Refactor captureStream_common.js to accept generic pixel testing method. r?jib

Comment 50

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681087 [details]
MozReview Request: Bug 1219711 - Add mochitest for track disabling over a peer connection. r?jib

Bug 1219711 - Add mochitest for track disabling over a peer connection. r?jib

Comment 51

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8682352 [details]
MozReview Request: Bug 1219711 - Let fake stream take precedence in testing. r?jib

Bug 1219711 - Let fake stream take precedence in testing. r?jib
This is how I think fake streams should work.

TL;DR requesting a fake stream always gives you a fake stream. No magic.

The gUMConstraint `fake: true` should take precedence and if set always
use MediaEngineDefault.
If it is set the state of `faketracks` is passed
on to MediaEngineDefault.
If it is not set, but (any of) audio/video loopback devices are set, the
device enumeration will filter out only those.

Comment 52

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8682353 [details]
MozReview Request: Bug 1219711 - Remove fakeness from webrtc tests. r?jib

Bug 1219711 - Remove fakeness from webrtc tests. r?jib

Comment 53

[Andreas Pehrson [:pehrsons]]

Ugh, now the actual fix disappeared since it already landed on central. I'll put it up separately.

Comment 54

[Andreas Pehrson [:pehrsons]]

Attachment: Ensure MediaStreamTrack.enabled propagates across peer connections

This already landed on central (comment 25), and disappeared on the bug when I pushed the test patches to mozreview. See comment 32 for the original uplift approval request. It only applies to this patch.

Comment 55

[Andreas Pehrson [:pehrsons]]

Try push for the tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7cee67e5ad8

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/a196aee79c60

Comment 57

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

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #53)
> Ugh, now the actual fix disappeared since it already landed on central. I'll
> put it up separately.

Isn't it still in the first patch here?  Though it doesn't matter really...

Comment 58

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8681087 [details]
MozReview Request: Bug 1219711 - Add mochitest for track disabling over a peer connection. r?jib

https://reviewboard.mozilla.org/r/23759/#review21675

I'm having a bit of trouble reviewing this because of bug 1221849. Tips welcome.

::: dom/canvas/test/captureStream_common.js:74
(Diff revisions 2 - 3)
> -    var ctxout = this.cout.getContext('2d');
> +    threshold = threshold || 255; // Default to 255 (exact) if not passed in.

maybe check against undefined in case someone passes in a threshold of 0?

Comment 59

[Andreas Pehrson [:pehrsons]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #58)
> Comment on attachment 8681087 [details]
> MozReview Request: Bug 1219711 - Add mochitest for track disabling over a
> peer connection. r?jib
> 
> https://reviewboard.mozilla.org/r/23759/#review21675
> 
> I'm having a bit of trouble reviewing this because of bug 1221849. Tips
> welcome.

I'll see if I can get a manual interdiff out of git.


> ::: dom/canvas/test/captureStream_common.js:74
> (Diff revisions 2 - 3)
> > -    var ctxout = this.cout.getContext('2d');
> > +    threshold = threshold || 255; // Default to 255 (exact) if not passed in.
> 
> maybe check against undefined in case someone passes in a threshold of 0?

Oh, yep. Went a bit fast here.

Comment 60

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #59)
> (In reply to Jan-Ivar Bruaroey [:jib] from comment #58)
> > Comment on attachment 8681087 [details]
> > MozReview Request: Bug 1219711 - Add mochitest for track disabling over a
> > peer connection. r?jib
> > 
> > https://reviewboard.mozilla.org/r/23759/#review21675
> > 
> > I'm having a bit of trouble reviewing this because of bug 1221849. Tips
> > welcome.
> 
> I'll see if I can get a manual interdiff out of git.

No need, you should look at the (new) first patch for those changes. Somehow it ended up after the test in bugzilla but mozreview shows the correct order.

Comment 61

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8681088 [details]
MozReview Request: Bug 1219711 - Don't rely on timer interval in fake audio track. r?jesup

https://reviewboard.mozilla.org/r/23761/#review21805

with timeout addressed.

::: dom/canvas/test/test_capture.html:91
(Diff revision 3)
> -    .then(() => h.waitForPixel(vmanual, h.green, 0, "should still be green"))
> +    .then(() => h.waitForPixelColorTimeout(vrate, h.red, 0, 0,

0 timeout?

::: dom/canvas/test/webgl-mochitest/test_capture.html:57
(Diff revision 3)
> -  ok(h.testPixel(vauto, [0, 0, 0, 0], 0), "Should not be drawn to before stable state");
> -  ok(h.testPixel(vrate, [0, 0, 0, 0], 0), "Should not be drawn to before stable state");
> -  ok(h.testPixel(vmanual, [0, 0, 0, 0], 0), "Should not be drawn to before stable state");
> +  ok(h.getPixel(vauto).every(c => c == 0),
> +     "vauto should not be drawn to before stable state");
> +  ok(h.getPixel(vrate).every(c => c == 0),
> +     "vrate should not be drawn to before stable state");
> +  ok(h.getPixel(vmanual).every(c => c == 0),
> +     "vmanual should not be drawn to before stable state");

You're a tough wordwrapper.

Comment 62

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8681087 [details]
MozReview Request: Bug 1219711 - Add mochitest for track disabling over a peer connection. r?jib

https://reviewboard.mozilla.org/r/23759/#review21803

I pulled the commit to compare. Looks like mozReview got the commits confused. Love the refactor!

::: dom/canvas/test/captureStream_common.js:92
(Diff revisions 2 - 3)
>    waitForPixel: function (video, offsetX, offsetY, test, timeout) {
>      return new Promise(resolve => {
> -      info("Testing " + video.id + " against [" + refColor.data.join(',') + "]");
> +      const startTime = video.currentTime;
>        CaptureStreamTestHelper2D.prototype.clear.call(this, this.cout);
> -      video.ontimeupdate = () => {
> -        if (fun.bind(this)(video, refColor.data, threshold)) {
> -          ok(true, video.id + " " + infoString);
> -          video.ontimeupdate = null;
> -          resolve();
> +      var ontimeupdate = () => {
> +        const pixelMatch = test(this.getPixel(video, offsetX, offsetY));
> +        if (!pixelMatch &&
> +            (!timeout || video.currentTime < startTime + (timeout / 1000.0))) {
> +          // No match yet and,
>            // No timeout (waiting indefinitely) or |timeout| has not passed yet.
>            return;

Not holding anything up over this, but why not timeout(waitForPixel(a,b,c,d), 128) ?

Comment 63

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/23761/#review21805

> 0 timeout?

I think the rationale was that it should not currently be red since we waited for `vauto` already. Should just check that instead of doing a timeout thing... I'll fix.

> You're a tough wordwrapper.

I'll put it on my CV.

Comment 64

[Andreas Pehrson [:pehrsons]]

https://reviewboard.mozilla.org/r/23759/#review21803

> Not holding anything up over this, but why not timeout(waitForPixel(a,b,c,d), 128) ?

I considered it but had to let it go since in the case of a timeout there won't be anything stopping the waitForPixel-internal callbacks from happening on every "timeupdate".

Comment 65

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681088 [details]
MozReview Request: Bug 1219711 - Don't rely on timer interval in fake audio track. r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23761/diff/3-4/

Comment 66

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681087 [details]
MozReview Request: Bug 1219711 - Add mochitest for track disabling over a peer connection. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23759/diff/3-4/

Comment 67

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8682352 [details]
MozReview Request: Bug 1219711 - Let fake stream take precedence in testing. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24057/diff/2-3/

Comment 68

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8682353 [details]
MozReview Request: Bug 1219711 - Remove fakeness from webrtc tests. r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24059/diff/2-3/

Comment 69

[Andreas Pehrson [:pehrsons]]

Try push looks good with the latest fixes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c827bef5fa2

Comment 70

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a21d79a94307
https://hg.mozilla.org/integration/mozilla-inbound/rev/65abe62869df
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eaa8cf9a752
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf14a6e5f340

Comment 71

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d5f09d89424

Comment 72

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a21d79a94307
https://hg.mozilla.org/mozilla-central/rev/65abe62869df
https://hg.mozilla.org/mozilla-central/rev/4eaa8cf9a752
https://hg.mozilla.org/mozilla-central/rev/cf14a6e5f340

Comment 73

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/aaee8ec22e5f for very frequent android m(9) failures: https://treeherder.mozilla.org/logviewer.html#?job_id=16899978&repo=mozilla-inbound

Comment 74

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

I also see a linux failure: https://treeherder.mozilla.org/logviewer.html#?job_id=16930412&repo=mozilla-inbound

Comment 75

[Liz Henry (:lizzard) (relman/hg->git project)]

Some of this backed out. Let's make sure to identify the right patches for uplift. 
Ritu can you have a look for 44?

Comment 76

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

This is the correct patch for uplift (mozreview did something stupid and it's not visible there):
https://hg.mozilla.org/mozilla-central/rev/a196aee79c60

Comment 77

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for FF44 for obvious reasons.

Comment 78

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8683026 [details] [diff] [review]
Ensure MediaStreamTrack.enabled propagates across peer connections

This patch has been in Nightly for a few days, it seems safe to uplift to Aurora44.

Comment 79

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

Backout merged to m-c: https://hg.mozilla.org/mozilla-central/rev/0d880e3ac292

Comment 80

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/35e183dfdd7a

Comment 81

[Andreas Pehrson [:pehrsons]]

It's timing out while checking for enabled local audio. I'm thinking it could be because of audio coming from MediaEngineDefault and high CPU load. We have a canvas for debugging this (causing more CPU load on main thread) but the captured screenshot is useless here.

The MediaEngineDefaultAudioSource runs off a regular nsTimer and appends data to the source track every 10ms, see [1]. Sound like it's asking for trouble if you ask me, as audio normally runs with highest possible priorities (it has to stay realtime after all!). I wonder if rewriting the audio source to be pull based and run in the MediaStreamGraph would improve things. We just precalc a buffer of one period of the 1kHz sine tone and feed that into the source stream on NotifyPull.

[1] https://dxr.mozilla.org/mozilla-central/rev/e2a910c048dc82fc3be53475f18e7f81f03e377b/dom/media/webrtc/MediaEngineDefault.cpp#457

Comment 82

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

Currently (i.e. without full-duplex) audio sources run asynchronously to the MSG and push data in (and even with full-duplex, you may route a mic in output domain 1 to output domain 2 (a separate MSG), and thus it will be asynchronous - or synchronized through a buffer to hide the asynchronous aspect.

The other complication is that we run DirectListeners on the input, and we really don't want to invoke them on the MSG thread, so if we do this (which may help tests) it will remove testing of audio DirectListeners, and not test the code paths used in production as well.

(I'll note DirectListeners are a problem for Full-Duplex as well.)

Still: it's probably worth trying, and combine with disabling DirectListeners for fake inputs somehow.

However to the larger picture: testing realtime code at a CPU overload condition is going to invoke a bunch of non-normal pathways and behaviors, and while interesting in some ways for testing those, it's not a good test of normal operation.

Comment 83

[Andreas Pehrson [:pehrsons]]

(In reply to Randell Jesup [:jesup] from comment #82)
> Currently (i.e. without full-duplex) audio sources run asynchronously to the
> MSG and push data in (and even with full-duplex, you may route a mic in
> output domain 1 to output domain 2 (a separate MSG), and thus it will be
> asynchronous - or synchronized through a buffer to hide the asynchronous
> aspect.
> 
> The other complication is that we run DirectListeners on the input, and we
> really don't want to invoke them on the MSG thread, so if we do this (which
> may help tests) it will remove testing of audio DirectListeners, and not
> test the code paths used in production as well.
> 
> (I'll note DirectListeners are a problem for Full-Duplex as well.)
> 
> Still: it's probably worth trying, and combine with disabling
> DirectListeners for fake inputs somehow.
> 
> However to the larger picture: testing realtime code at a CPU overload
> condition is going to invoke a bunch of non-normal pathways and behaviors,
> and while interesting in some ways for testing those, it's not a good test
> of normal operation.

I.e., we should have machines for all platforms capable of handling our realtime code. Even if main thread is overloaded we should be able to handle underlying low-level audio stuff.

I see two options here,

1. Appending audio data in MSG's NotifyPull. But like you said (and reminded me of), ugh, DirectListeners.

2. Increasing the thread priority of MediaEngineDefaultAudioSource so it properly simulates an audio callback thread. Do the MediaEngineDefault timers even run on its own thread, or do they run on main thread? Maybe a dedicated thread is enough.

Comment 84

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/35e183dfdd7a

Comment 85

[Andreas Pehrson [:pehrsons]]

Here's why MediaEngineDefaultAudioSource is bad. On every Notify() we append exactly 10ms of audio data, but the interval at which Notify() is called are typically what I logged here:

> Last Notify() 12,042ms ago
> Last Notify() 11,327ms ago
> Last Notify() 11,097ms ago
> Last Notify() 11,601ms ago
> Last Notify() 11,694ms ago
> Last Notify() 11,593ms ago
> Last Notify() 11,698ms ago
> Last Notify() 12,492ms ago


Trying to fix so that we at least take the time since the last append into account.

Comment 86

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681088 [details]
MozReview Request: Bug 1219711 - Don't rely on timer interval in fake audio track. r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23761/diff/4-5/

Comment 87

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

Comment on attachment 8681088 [details]
MozReview Request: Bug 1219711 - Don't rely on timer interval in fake audio track. r?jesup

https://reviewboard.mozilla.org/r/23761/#review22213

Comment 88

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681088 [details]
MozReview Request: Bug 1219711 - Don't rely on timer interval in fake audio track. r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23761/diff/5-6/

Comment 89

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1219711 - Refactor captureStream_common.js to accept generic pixel testing method. r=jib

Bug 1219711 - Refactor captureStream_common.js to accept generic pixel testing method. r=jib

Comment 90

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1219711 - Add mochitest for track disabling over a peer connection. r=jib

Bug 1219711 - Add mochitest for track disabling over a peer connection. r=jib

Comment 91

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1219711 - Let fake stream take precedence in testing. r=jib

Bug 1219711 - Let fake stream take precedence in testing. r=jib
TL;DR requesting a fake stream always gives you a fake stream. No magic.

The gUMConstraint `fake: true` should take precedence and if set always
use MediaEngineDefault.
If it is set the state of `faketracks` is passed
on to MediaEngineDefault.
If it is not set, but (any of) audio/video loopback devices are set, the
device enumeration will filter out only those.

Comment 92

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1219711 - Remove fakeness from webrtc tests. r=jib

Bug 1219711 - Remove fakeness from webrtc tests. r=jib

Comment 93

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681088 [details]
MozReview Request: Bug 1219711 - Don't rely on timer interval in fake audio track. r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23761/diff/6-7/

Comment 94

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8684924 [details]
MozReview Request: Bug 1219711 - Refactor captureStream_common.js to accept generic pixel testing method. r=jib

I'm getting good at breaking bugs with mozreview. Now order is at least somewhat restored. Carries r=jib.

Comment 95

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8684925 [details]
MozReview Request: Bug 1219711 - Add mochitest for track disabling over a peer connection. r=jib

Carries r=jib.

Comment 96

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8684926 [details]
MozReview Request: Bug 1219711 - Let fake stream take precedence in testing. r=jib

Carries r=jib.

Comment 97

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8684927 [details]
MozReview Request: Bug 1219711 - Remove fakeness from webrtc tests. r=jib

Carries r=jib.

Comment 98

[Andreas Pehrson [:pehrsons]]

Some diagnostic logging from the timeout on Android 4.3 debug:

> 21:41:16     INFO -  402 INFO Run step 54: CHECK_VIDEO
> 21:41:16     INFO -  403 INFO Checking local video enabled
> 21:41:16     INFO -  404 INFO Time taken so far: 140.544s
> 21:41:16     INFO -  405 INFO Checking remote video enabled
> 21:41:16     INFO -  406 INFO Time taken so far: 143.611s
> 21:41:16     INFO -  407 INFO Checking local video disabled
> 21:41:16     INFO -  408 INFO Time taken so far: 145.458s
> 21:41:16     INFO -  409 INFO Checking remote video disabled
> 21:41:16     INFO -  410 INFO Time taken so far: 147.559s
> 21:41:16     INFO -  411 INFO Run step 55: CHECK_AUDIO
> 21:41:16     INFO -  412 INFO Checking local audio enabled
> 21:41:16     INFO -  413 INFO Time taken so far: 184.747s
> 21:41:16     INFO -  414 INFO Checking remote audio enabled
> 21:41:16     INFO -  415 INFO Time taken so far: 217.245s
> 21:41:16     INFO -  416 INFO Checking local audio disabled
> 21:41:16     INFO -  417 INFO Time taken so far: 231.964s
> 21:41:16     INFO -  418 INFO Checking remote audio disabled
> 21:41:16     INFO -  419 INFO Time taken so far: 292.504s


Though most of the time ICE fails, not much I can fix about that in this bug.

I'll disable the test on Android 4.3 debug. Opt is now OK, though really slow at around 2.5 minutes. For comparison test_peerConnection_basicAudioVideo.html takes 1 minute.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f89d53ae8e3

I have one more trick up my sleeve on speeding up the audio analysis part. Patch coming.

Comment 99

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8684925 [details]
MozReview Request: Bug 1219711 - Add mochitest for track disabling over a peer connection. r=jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24671/diff/1-2/

Comment 100

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8684926 [details]
MozReview Request: Bug 1219711 - Let fake stream take precedence in testing. r=jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24673/diff/1-2/

Comment 101

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8684927 [details]
MozReview Request: Bug 1219711 - Remove fakeness from webrtc tests. r=jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/24675/diff/1-2/

Comment 102

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8681088 [details]
MozReview Request: Bug 1219711 - Don't rely on timer interval in fake audio track. r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23761/diff/7-8/

Comment 103

[Andreas Pehrson [:pehrsons]]

Attachment: MozReview Request: Bug 1219711 - Lower AudioStreamAnalyser's smoothingTimeConstant for speedier tests. r?padenot

Bug 1219711 - Lower AudioStreamAnalyser's smoothingTimeConstant for speedier tests. r?padenot

Comment 104

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8684925 [details]
MozReview Request: Bug 1219711 - Add mochitest for track disabling over a peer connection. r=jib

Carries r=jib

Comment 105

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8684926 [details]
MozReview Request: Bug 1219711 - Let fake stream take precedence in testing. r=jib

Carries r=jib

Comment 106

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8684927 [details]
MozReview Request: Bug 1219711 - Remove fakeness from webrtc tests. r=jib

Carries r=jib

Comment 107

[Andreas Pehrson [:pehrsons]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=224ef0e009e5

Comment 108

[Andreas Pehrson [:pehrsons]]

Looks like running time on android opt was cut by around a minute. I observed one run before the smoothingTimeConstant patch at 163s and the first one to finish after that patch came in at 107s.

Comment 109

[Paul Adenot (:padenot)]

Comment on attachment 8685338 [details]
MozReview Request: Bug 1219711 - Lower AudioStreamAnalyser's smoothingTimeConstant for speedier tests. r?padenot

https://reviewboard.mozilla.org/r/24773/#review22291

Comment 110

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50412f7bafbf
https://hg.mozilla.org/integration/mozilla-inbound/rev/07dce87de29b
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b8755f0ccf5
https://hg.mozilla.org/integration/mozilla-inbound/rev/9267c84ef58c
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0e38d45f7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/39d1c1826d6f

Comment 111

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/50412f7bafbf
https://hg.mozilla.org/mozilla-central/rev/07dce87de29b
https://hg.mozilla.org/mozilla-central/rev/7b8755f0ccf5
https://hg.mozilla.org/mozilla-central/rev/9267c84ef58c
https://hg.mozilla.org/mozilla-central/rev/4c0e38d45f7b
https://hg.mozilla.org/mozilla-central/rev/39d1c1826d6f