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

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

We need to check that these checks actually can detect failures when they happen.

Comment 1

[Michael Froman [:mjf]]

Attachment: MozReview Request: Bug 1166832 - Adding test for canvas capture on multiple streams.

Review commit: https://reviewboard.mozilla.org/r/30345/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30345/

Comment 2

[Michael Froman [:mjf]]

Attachment: MozReview Request: Bug 1166832 - Adding test to verify video (using capture stream) after renegotiation.

Review commit: https://reviewboard.mozilla.org/r/30347/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30347/

Comment 3

[Byron Campen [:bwc]]

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 4

[Byron Campen [:bwc]]

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

Comment 5

[Michael Froman [:mjf]]

Attachment: MozReview Request: Bug 1166832 - Add test to verify audio (using AudioStreamAnalyser) after renegotiation.

Review commit: https://reviewboard.mozilla.org/r/30845/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30845/

Comment 6

[Michael Froman [:mjf]]

Attachment: MozReview Request: Bug 1166832 - Remove usage of '!!' idiom.

Review commit: https://reviewboard.mozilla.org/r/30847/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30847/

Comment 7

[Michael Froman [:mjf]]

Attachment: MozReview Request: Bug 1166832 - Address review, using a promise to avoid a rendering race.

Review commit: https://reviewboard.mozilla.org/r/30849/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30849/

Comment 8

[Michael Froman [:mjf]]

Attachment: MozReview Request: Bug 1166832 - Fix html bug title.

Review commit: https://reviewboard.mozilla.org/r/30851/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30851/

Comment 9

[Michael Froman [:mjf]]

Attachment: MozReview Request: Bug 1166832 - Rename test to better show that it is video related.

Review commit: https://reviewboard.mozilla.org/r/30853/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30853/

Comment 10

[Byron Campen [:bwc]]

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.

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8707729 [details]
MozReview Request: Bug 1166832 - Remove usage of '!!' idiom.

https://reviewboard.mozilla.org/r/30847/#review27701

Comment 12

[Byron Campen [:bwc]]

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.

Comment 13

[Byron Campen [:bwc]]

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 14

[Byron Campen [:bwc]]

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

Comment 15

[Byron Campen [:bwc]]

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 16

[Michael Froman [:mjf]]

Attachment: MozReview Request: Bug 1166832 - Fix ordering, draw both local red then test remotes to avoid ordering issues.

Review commit: https://reviewboard.mozilla.org/r/30905/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30905/

Comment 17

[Byron Campen [:bwc]]

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

Comment 18

[Michael Froman [:mjf]]

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

Comment 19

[Michael Froman [:mjf]]

Attachment: MozReview Request: Bug 1166832 - Fix bug number in test and remove TODO.

Review commit: https://reviewboard.mozilla.org/r/30907/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30907/

Comment 20

[Michael Froman [:mjf]]

Attachment: MozReview Request: Bug 1166832 - Fix missing semicolon on return.

Review commit: https://reviewboard.mozilla.org/r/30909/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30909/

Comment 21

[Michael Froman [:mjf]]

Attachment: MozReview Request: Bug 1166832 - Fix bug number on test_peerConnection_multiple_captureStream_canvas_2d.html.

Review commit: https://reviewboard.mozilla.org/r/30913/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30913/

Comment 22

[Byron Campen [:bwc]]

Comment on attachment 8707925 [details]
MozReview Request: Bug 1166832 - Fix bug number in test and remove TODO.

https://reviewboard.mozilla.org/r/30907/#review27719

Comment 23

[Byron Campen [:bwc]]

Comment on attachment 8707926 [details]
MozReview Request: Bug 1166832 - Fix missing semicolon on return.

https://reviewboard.mozilla.org/r/30909/#review27721

Comment 24

[Byron Campen [:bwc]]

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 25

[Byron Campen [:bwc]]

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.

Comment 26

[Byron Campen [:bwc]]

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 27

[Byron Campen [:bwc]]

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

Comment 28

[Byron Campen [:bwc]]

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

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd18104e0d2c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a7e55e8c0b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/64d267bd0527

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd18104e0d2c
https://hg.mozilla.org/mozilla-central/rev/5a7e55e8c0b3
https://hg.mozilla.org/mozilla-central/rev/64d267bd0527

Comment 31

[Michael Froman [:mjf]]

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

Comment 32

[Michael Froman [:mjf]]

(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!