Re-enable peerIdentity test

NEW
Assigned to

Status

()

Core
WebRTC
P5
normal
Rank:
45
4 years ago
10 months ago

People

(Reporter: mt, Assigned: mt)

Tracking

({leave-open})

unspecified
All
Gonk (Firefox OS)
leave-open
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Bug 1021776 noted an intermittent failure caused by poor emulator performance.  This was initially at low rates, but it increased over time to fail on about 30-40% of runs.

Aggressively backing off on the timers seems to alleviate the problems: https://tbpl.mozilla.org/?tree=Try&rev=7a4d7c1f6b47
(Assignee)

Comment 1

4 years ago
Created attachment 8446214 [details] [diff] [review]
Attempting to ease load on B2G emulator for tests
(Assignee)

Comment 2

4 years ago
Created attachment 8446215 [details] [diff] [review]
Re-enabling gUM peerIdentity test
(Assignee)

Comment 3

4 years ago
Comment on attachment 8446214 [details] [diff] [review]
Attempting to ease load on B2G emulator for tests

This one exponentially backs off the timers so that the emulator has a chance.  It also suppresses rendering and disconnects nodes that aren't in use.  This seems to do the trick.

Maybe when we get principal-based events on tracks this can be removed.  Until then, I think that this is safe to proceed with.
Attachment #8446214 - Flags: review?(jib)
(Assignee)

Updated

4 years ago
Attachment #8446215 - Flags: review?(jib)
Comment on attachment 8446214 [details] [diff] [review]
Attempting to ease load on B2G emulator for tests

Review of attachment 8446214 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm.

::: dom/media/tests/mochitest/blacksilence.js
@@ +35,5 @@
>        }
> +    }
> +    function setupNext() {
> +      timeout = setTimeout(periodic, 200 << num);
> +      num++;

Maybe add a comment explaining why this approach is here and seems to work.

@@ +81,5 @@
>      var e = document.createElement(type);
>      e.width = 32;
>      e.height = 24;
> +    // var display = document.getElementById('display');
> +    // display.appendChild(e);

I think commented-out code frowned on. Maybe add a comment explaining what needs to happen to re-enable i? It's not clear.
Attachment #8446214 - Flags: review?(jib) → review+
> >      var e = document.createElement(type);
> >      e.width = 32;
> >      e.height = 24;
> > +    // var display = document.getElementById('display');
> > +    // display.appendChild(e);

Also, is it a problem that mkElement() now returns something not attached to anything?
Attachment #8446215 - Flags: review?(jib) → review+
(Assignee)

Comment 6

4 years ago
(In reply to Jan-Ivar Bruaroey [:jib] from comment #4)
> I think commented-out code frowned on. Maybe add a comment explaining what
> needs to happen to re-enable i? It's not clear.

That's just an accident.  I thought that I'd cut that out.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #5)
> Also, is it a problem that mkElement() now returns something not attached to
> anything?

The tests pass :)  The basic idea here is that the element is still attached to the document, but since it's not in the tree it doesn't incur any rendering costs.  This is as aggressive a reduction in cycles as I could manage without being too disruptive.
(Assignee)

Comment 7

4 years ago
Created attachment 8446647 [details] [diff] [review]
Attempting to ease load on B2G emulator for tests r=jib
(Assignee)

Updated

4 years ago
Attachment #8446214 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Comment on attachment 8446647 [details] [diff] [review]
Attempting to ease load on B2G emulator for tests r=jib

Carrying r=jib
Attachment #8446647 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Assignee: nobody → martin.thomson
Still hitting intermittent failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42643186&tree=Mozilla-Inbound

I backed out the re-enabling part (but left in the first patch).
https://hg.mozilla.org/integration/mozilla-inbound/rev/4179297b82a3
Keywords: leave-open
(Assignee)

Comment 11

4 years ago
Thanks, that was the right call.  It looks like my try runs didn't hit the slowest machines.
Whoops, that's not the gUM test, so maybe unrelated.
(Assignee)

Comment 14

4 years ago
Yeah, that looks pretty serious.  But it isn't really related to this bug.  Definitely a bug, but it's beyond my modest debugging abilities to diagnose faults that occur in the kernel.
Still relevant?  Overtaken by work on other identity bugs?
backlog: --- → webRTC+
Rank: 45
Flags: needinfo?(martin.thomson)
Priority: -- → P4
(Assignee)

Comment 17

3 years ago
This still needs to be done, sadly.  But performance on the emulator tests is still too poor.  Byron has suggested that we might reduce frame rate (and maybe also resolution) on the fake source to alleviate some of the burden.
Flags: needinfo?(martin.thomson)
Mass change P4->P5 to align with new Mozilla triage process.
Priority: P4 → P5
You need to log in before you can comment on or make changes to this bug.