Disabling/muting Media tracks no longer works

RESOLVED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Core
WebRTC: Audio/Video
P1
normal
Rank:
5
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: standard8, Assigned: pehrsons)

Tracking

({regression})

33 Branch
mozilla45
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 unaffected, firefox44+ fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments, 4 obsolete attachments)

40 bytes, text/x-review-board-request
jib
: review+
jesup
: review+
Details | Review
1.12 KB, patch
pehrsons
: review+
Details | Diff | Splinter Review
40 bytes, text/x-review-board-request
pehrsons
: review+
Details | Review
40 bytes, text/x-review-board-request
pehrsons
: review+
Details | Review
40 bytes, text/x-review-board-request
pehrsons
: review+
Details | Review
40 bytes, text/x-review-board-request
pehrsons
: review+
Details | Review
40 bytes, text/x-review-board-request
padenot
: review+
Details | Review
(Reporter)

Description

2 years ago
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.
[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.
Assignee: nobody → rjesup
backlog: --- → webrtc/webaudio+
Rank: 5
Priority: -- → P1
[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.
tracking-firefox44: --- → ?
(Reporter)

Comment 3

2 years ago
(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.
Flags: needinfo?(bogdan.maris)
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).
Blocks: 1170958
Flags: needinfo?(pehrsons)
(Assignee)

Comment 5

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

Comment 6

2 years ago
Created 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
Attachment #8681087 - Flags: review?(jib)
(Assignee)

Comment 7

2 years ago
Created 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
Attachment #8681088 - Flags: review?(rjesup)
(Assignee)

Comment 8

2 years ago
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=37b5a5e6ce1c
Assignee: rjesup → pehrsons
Status: NEW → ASSIGNED
Flags: needinfo?(pehrsons)
(Assignee)

Updated

2 years ago
See Also: → bug 1220047

Updated

2 years ago
Attachment #8681088 - Flags: review?(rjesup) → review+
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 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?
Attachment #8681087 - Flags: review?(jib) → review+
(Assignee)

Comment 11

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

Comment 12

2 years ago
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.
Flags: needinfo?(rjesup)
Flags: needinfo?(jib)
(Assignee)

Comment 13

2 years ago
Here's a try push to test the 440Hz theory: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e96383c5abaf
fake: true should give you a fake track regardless of whether there is a device available.
(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. :)
Flags: needinfo?(jib)
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

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a196aee79c60
Created attachment 8681635 [details] [diff] [review]
Add mochitest for track disabling over a peer connection

With faketracks:true
Attachment #8681635 - Flags: review?(jib)

Updated

2 years ago
Assignee: pehrsons → rjesup
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a84e7b7c36ef
Flags: needinfo?(rjesup)
jib: no joy with faketracks: true
Flags: needinfo?(jib)
(Assignee)

Comment 21

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

Comment 22

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

Comment 23

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

Comment 24

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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a196aee79c60
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(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.
Flags: needinfo?(jib)
(Assignee)

Comment 27

2 years ago
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.
(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?
https://reviewboard.mozilla.org/r/23759/#review21387
Attachment #8681635 - Flags: review?(jib)
(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?
Flags: needinfo?(pehrsons)
(Assignee)

Comment 31

2 years ago
(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.
Flags: needinfo?(pehrsons)
(Assignee)

Comment 32

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

Updated

2 years ago
Assignee: rjesup → pehrsons
(Assignee)

Comment 33

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

Comment 34

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

Comment 35

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

Comment 36

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

Comment 37

2 years ago
Created 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.
Attachment #8682352 - Flags: review?(jib)
(Assignee)

Comment 38

2 years ago
Created attachment 8682353 [details]
MozReview Request: Bug 1219711 - Remove fakeness from webrtc tests. r?jib

Bug 1219711 - Remove fakeness from webrtc tests. r?jib
Attachment #8682353 - Flags: review?(jib)
(Assignee)

Updated

2 years ago
Attachment #8681635 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8681087 - Flags: review+ → review?(jib)
(Assignee)

Comment 39

2 years ago
try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f70cdffe2fb0
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"));
Attachment #8681087 - Flags: review?(jib) → review+
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.
Attachment #8682352 - Flags: review?(jib)
Attachment #8682353 - Flags: review?(jib) → review+
Comment on attachment 8682353 [details]
MozReview Request: Bug 1219711 - Remove fakeness from webrtc tests. r?jib

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

lgtm.
(Assignee)

Comment 43

2 years ago
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
(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.
Flags: needinfo?(bogdan.maris)
(Assignee)

Updated

2 years ago
Attachment #8682352 - Flags: review?(jib)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #43)
> Your assumption in point 1 is wrong.

Oh, good. Happy to be wrong there. :)
Attachment #8682352 - Flags: review?(jib) → review+
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
(Assignee)

Comment 47

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

Comment 48

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

Updated

2 years ago
Attachment #8681088 - Attachment description: MozReview Request: Bug 1219711 - Ensure MediaStreamTrack.enabled propagates across peer connections. r?jesup → MozReview Request: Bug 1219711 - Refactor captureStream_common.js to accept generic pixel testing method. r?jib
Attachment #8681088 - Flags: review?(jib)
(Assignee)

Comment 49

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

Comment 50

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

Comment 51

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

Comment 52

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

Comment 53

2 years ago
Ugh, now the actual fix disappeared since it already landed on central. I'll put it up separately.
(Assignee)

Updated

2 years ago
Attachment #8681088 - Flags: review+
Attachment #8681088 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 years ago
Attachment #8681087 - Flags: review+ → review?(jib)
(Assignee)

Comment 54

2 years ago
Created attachment 8683026 [details] [diff] [review]
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.
Attachment #8683026 - Flags: review+
Attachment #8683026 - Flags: approval‑mozilla‑b2g44?
Attachment #8683026 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 55

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

Comment 56

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/a196aee79c60
status-b2g-v2.5: --- → fixed
(Assignee)

Updated

2 years ago
Attachment #8683026 - Flags: approval‑mozilla‑b2g44?
(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 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?
Attachment #8681087 - Flags: review?(jib)
(Assignee)

Comment 59

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

Comment 60

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

Updated

2 years ago
Attachment #8681087 - Flags: review?(jib)
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.
Attachment #8681088 - Flags: review?(jib) → review+
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) ?
Attachment #8681087 - Flags: review?(jib) → review+
(Assignee)

Comment 63

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

Comment 64

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

Comment 65

2 years ago
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/
Attachment #8681088 - Flags: review?(rjesup)
(Assignee)

Comment 66

2 years ago
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/
(Assignee)

Comment 67

2 years ago
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/
(Assignee)

Comment 68

2 years ago
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/
(Assignee)

Updated

2 years ago
Attachment #8681088 - Flags: review?(rjesup)
(Assignee)

Comment 69

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

Comment 70

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

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

Comment 72

2 years ago
bugherder
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
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
Flags: needinfo?(pehrsons)
I also see a linux failure: https://treeherder.mozilla.org/logviewer.html#?job_id=16930412&repo=mozilla-inbound
Some of this backed out. Let's make sure to identify the right patches for uplift. 
Ritu can you have a look for 44?
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

Updated

2 years ago
Flags: needinfo?(rkothari)
Tracked for FF44 for obvious reasons.
tracking-firefox44: ? → +
Flags: needinfo?(rkothari)
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.
Attachment #8683026 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Backout merged to m-c: https://hg.mozilla.org/mozilla-central/rev/0d880e3ac292
https://hg.mozilla.org/releases/mozilla-aurora/rev/35e183dfdd7a
status-firefox44: affected → fixed
(Assignee)

Comment 81

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

Comment 83

2 years ago
(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.
Flags: needinfo?(pehrsons)

Comment 84

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/35e183dfdd7a
(Assignee)

Comment 85

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

Comment 86

2 years ago
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/
Attachment #8681088 - Attachment description: MozReview Request: Bug 1219711 - Refactor captureStream_common.js to accept generic pixel testing method. r?jib → MozReview Request: Bug 1219711 - Don't rely on timer interval in fake audio track. r?jesup
Attachment #8681088 - Flags: review?(rjesup)
(Assignee)

Updated

2 years ago
Attachment #8681087 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8682352 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8682353 - Attachment is obsolete: true
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
Attachment #8681088 - Flags: review?(rjesup) → review+
(Assignee)

Comment 88

2 years ago
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/
(Assignee)

Comment 89

2 years ago
Created attachment 8684924 [details]
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
(Assignee)

Comment 90

2 years ago
Created attachment 8684925 [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
(Assignee)

Comment 91

2 years ago
Created attachment 8684926 [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
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.
(Assignee)

Comment 92

2 years ago
Created attachment 8684927 [details]
MozReview Request: Bug 1219711 - Remove fakeness from webrtc tests. r=jib

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

Comment 93

2 years ago
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/
(Assignee)

Comment 94

2 years ago
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.
Attachment #8684924 - Flags: review+
(Assignee)

Comment 95

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

Carries r=jib.
Attachment #8684925 - Flags: review+
(Assignee)

Comment 96

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

Carries r=jib.
Attachment #8684926 - Flags: review+
(Assignee)

Comment 97

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

Carries r=jib.
Attachment #8684927 - Flags: review+
(Assignee)

Comment 98

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

Comment 99

2 years ago
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/
Attachment #8684925 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8684926 - Flags: review+
(Assignee)

Comment 100

2 years ago
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/
(Assignee)

Comment 101

2 years ago
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/
Attachment #8684927 - Flags: review+
(Assignee)

Comment 102

2 years ago
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/
(Assignee)

Comment 103

2 years ago
Created attachment 8685338 [details]
MozReview Request: Bug 1219711 - Lower AudioStreamAnalyser's smoothingTimeConstant for speedier tests. r?padenot

Bug 1219711 - Lower AudioStreamAnalyser's smoothingTimeConstant for speedier tests. r?padenot
Attachment #8685338 - Flags: review?(padenot)
(Assignee)

Comment 104

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

Carries r=jib
Attachment #8684925 - Flags: review+
(Assignee)

Comment 105

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

Carries r=jib
Attachment #8684926 - Flags: review+
(Assignee)

Comment 106

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

Carries r=jib
Attachment #8684927 - Flags: review+
(Assignee)

Comment 107

2 years ago
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=224ef0e009e5
(Assignee)

Comment 108

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

Updated

2 years ago
Attachment #8685338 - Flags: review?(padenot) → review+
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

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

Updated

2 years ago
Depends on: 1223655

Comment 111

2 years ago
bugherder
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
You need to log in before you can comment on or make changes to this bug.