Closed Bug 1166832 Opened 5 years ago Closed 4 years ago

Verify that mochitest checks for stream liveness actually detect failures after renegotiation

Categories

(Core :: WebRTC: Signaling, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox41 --- affected
firefox46 --- fixed
Blocking Flags:

People

(Reporter: bwc, Assigned: mjf)

Details

(Whiteboard: [investigation])

Attachments

(11 files)

58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
58 bytes, text/x-review-board-request
bwc
: review+
Details
We need to check that these checks actually can detect failures when they happen.
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3
Assignee: nobody → mfroman
Attachment #8706472 - Flags: review?(docfaraday)
Attachment #8706473 - Flags: review?(docfaraday)
Attachment #8706472 - Flags: review?(docfaraday)
Comment on attachment 8706472 [details]
MozReview Request: Bug 1166832 - Adding test for canvas capture on multiple streams.

https://reviewboard.mozilla.org/r/30345/#review27281

::: dom/media/tests/mochitest/test_peerConnection_multiple_captureStream_canvas_2d.html:55
(Diff revision 1)
> +      ok(!!testremote, "Should have remote1 video element for pcRemote");

The '!!' idiom is not necessary in JS; it will convert anything to a boolean implicitly whether you want it to or not.

::: dom/media/tests/mochitest/test_peerConnection_multiple_captureStream_canvas_2d.html:57
(Diff revision 1)
> +      if (h.isPixel(h.getPixel(testremote), h.green, 128)) {

I worry that this step might execute before anything is being rendered. We probably want something like a Promise.race([waitForPixelColor(green), waitForPixelColor(blue)]) before trying to do this. Perhaps even better would be using Promise.race and Promise.all like:

    // Either vremote1 turns green and vremote2 turns blue, or vice versa
    Promise.race([
       Promise.all([
          waitForPixelColor(vremote1, green),
          waitForPixelColor(vremote2, blue)]),
       Promise.all([
          waitForPixelColor(vremote2, green),
          waitForPixelColor(vremote1, blue)]),
    ])
    
After this, you could draw red on both, and make sure both turn red; I'm not too worried about mixups once we've established that one drew green and the other blue.
Comment on attachment 8706473 [details]
MozReview Request: Bug 1166832 - Adding test to verify video (using capture stream) after renegotiation.

https://reviewboard.mozilla.org/r/30347/#review27283
Attachment #8706473 - Flags: review?(docfaraday) → review+
Attachment #8707728 - Flags: review?(docfaraday)
Attachment #8707729 - Flags: review?(docfaraday)
Attachment #8707730 - Flags: review?(docfaraday)
Attachment #8707731 - Flags: review?(docfaraday)
Attachment #8707732 - Flags: review?(docfaraday)
Comment on attachment 8707728 [details]
MozReview Request: Bug 1166832 - Add test to verify audio (using AudioStreamAnalyser) after renegotiation.

https://reviewboard.mozilla.org/r/30845/#review27697

::: dom/media/tests/mochitest/test_peerConnection_verifyAudioAfterRenegotiation.html:50
(Diff revision 1)
> +          .then(() => info("Checking local audio enabled"))
> +          .then(() => checkAudioEnabled(local1Analyser, freq1k))
> +          .then(() => info("Checking remote audio enabled"))
> +          .then(() => checkAudioEnabled(remote1Analyser, freq1k))
> +
> +          .then(() => test.pcLocal.streams[0].getAudioTracks()[0].enabled = false)
> +
> +          .then(() => info("Checking local audio disabled"))
> +          .then(() => checkAudioDisabled(local1Analyser, freq1k))
> +          .then(() => info("Checking remote audio disabled"))
> +          .then(() => checkAudioDisabled(remote1Analyser, freq1k))

I haven't seen info() calls put in a promise chain like this before, but it is pretty readable. You might want to ask either jib or mt.

::: dom/media/tests/mochitest/test_peerConnection_verifyAudioAfterRenegotiation.html:117
(Diff revision 1)
> +    // TODO(bug 1093835): figure out how to verify if media flows through the new stream

We don't need this TODO, since we're doing this.
Attachment #8707728 - Flags: review?(docfaraday) → review+
Comment on attachment 8707729 [details]
MozReview Request: Bug 1166832 - Remove usage of '!!' idiom.

https://reviewboard.mozilla.org/r/30847/#review27701
Attachment #8707729 - Flags: review?(docfaraday) → review+
Comment on attachment 8707731 [details]
MozReview Request: Bug 1166832 - Fix html bug title.

https://reviewboard.mozilla.org/r/30851/#review27703

::: dom/media/tests/mochitest/test_peerConnection_verifyAfterRenegotiation.html:11
(Diff revision 1)
>      bug: "1017888",

Update this too.
Attachment #8707731 - Flags: review?(docfaraday) → review+
https://reviewboard.mozilla.org/r/30845/#review27705

::: dom/media/tests/mochitest/test_peerConnection_verifyAudioAfterRenegotiation.html:10
(Diff revision 1)
> +    bug: "1017888",

Update this bug number.
Comment on attachment 8707732 [details]
MozReview Request: Bug 1166832 - Rename test to better show that it is video related.

https://reviewboard.mozilla.org/r/30853/#review27709
Attachment #8707732 - Flags: review?(docfaraday) → review+
Attachment #8707730 - Flags: review?(docfaraday)
Comment on attachment 8707730 [details]
MozReview Request: Bug 1166832 - Address review, using a promise to avoid a rendering race.

https://reviewboard.mozilla.org/r/30849/#review27713

::: dom/media/tests/mochitest/test_peerConnection_multiple_captureStream_canvas_2d.html:70
(Diff revision 1)
> -    },
> +             ])

Don't forget the ';', I know JS implicitly inserts these but it is good to make sure they are there.
Comment on attachment 8707914 [details]
MozReview Request: Bug 1166832 - Fix ordering, draw both local red then test remotes to avoid ordering issues.

https://reviewboard.mozilla.org/r/30905/#review27715
Attachment #8707914 - Flags: review+
https://reviewboard.mozilla.org/r/30845/#review27697

> I haven't seen info() calls put in a promise chain like this before, but it is pretty readable. You might want to ask either jib or mt.

I directly copied this from https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/test_peerConnection_trackDisabling.html#76-87
Attachment #8707925 - Flags: review?(docfaraday)
Attachment #8707926 - Flags: review?(docfaraday)
Attachment #8707928 - Flags: review?(docfaraday)
Comment on attachment 8707925 [details]
MozReview Request: Bug 1166832 - Fix bug number in test and remove TODO.

https://reviewboard.mozilla.org/r/30907/#review27719
Attachment #8707925 - Flags: review?(docfaraday) → review+
Attachment #8707926 - Flags: review?(docfaraday) → review+
Comment on attachment 8707926 [details]
MozReview Request: Bug 1166832 - Fix missing semicolon on return.

https://reviewboard.mozilla.org/r/30909/#review27721
Attachment #8707928 - Flags: review?(docfaraday)
Comment on attachment 8707928 [details]
MozReview Request: Bug 1166832 - Fix bug number on test_peerConnection_multiple_captureStream_canvas_2d.html.

https://reviewboard.mozilla.org/r/30913/#review27723

::: dom/media/tests/mochitest/test_peerConnection_multiple_captureStream_canvas_2d.html:11
(Diff revision 1)
> -  bug: "1032848",
> +  bug: "1166832",

Actually, let's leave this bug number alone. Just remove this changeset.
Comment on attachment 8707730 [details]
MozReview Request: Bug 1166832 - Address review, using a promise to avoid a rendering race.

https://reviewboard.mozilla.org/r/30849/#review27725

All issues look resolved in subsequent changesets.
Attachment #8707730 - Flags: review+
https://reviewboard.mozilla.org/r/30913/#review27723

> Actually, let's leave this bug number alone. Just remove this changeset.

I forgot that this was a new test, disregard.
Comment on attachment 8707928 [details]
MozReview Request: Bug 1166832 - Fix bug number on test_peerConnection_multiple_captureStream_canvas_2d.html.

https://reviewboard.mozilla.org/r/30913/#review27729
Attachment #8707928 - Flags: review+
Comment on attachment 8706472 [details]
MozReview Request: Bug 1166832 - Adding test for canvas capture on multiple streams.

https://reviewboard.mozilla.org/r/30345/#review27731

All issues look resolved in subsequent changesets
Attachment #8706472 - Flags: review+
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #30)
> https://hg.mozilla.org/mozilla-central/rev/bd18104e0d2c
> https://hg.mozilla.org/mozilla-central/rev/5a7e55e8c0b3
> https://hg.mozilla.org/mozilla-central/rev/64d267bd0527

It seems that not all of the changesets were pushed to inbound.  Critical changes are missing.  Please advise how to get all the changes pushed.
(In reply to Michael Froman [:mjf] from comment #31)
> (In reply to Carsten Book [:Tomcat] from comment #30)
> > https://hg.mozilla.org/mozilla-central/rev/bd18104e0d2c
> > https://hg.mozilla.org/mozilla-central/rev/5a7e55e8c0b3
> > https://hg.mozilla.org/mozilla-central/rev/64d267bd0527
> 
> It seems that not all of the changesets were pushed to inbound.  Critical
> changes are missing.  Please advise how to get all the changes pushed.

Never mind - I see they were just condensed down into three commits.  Handy!
You need to log in before you can comment on or make changes to this bug.