test_peerConnection_addSecondAudioStream.html needs verification of second media flow

RESOLVED FIXED in Firefox 55

Status

()

P3
normal
Rank:
35
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: bwc, Assigned: pehrsons)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(14 attachments)

59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
59 bytes, text/x-review-board-request
jib
: review+
Details
Comment hidden (empty)
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3
(Assignee)

Comment 1

2 years ago
I've done some work to get rid of all test comments about this bug.

It's almost done, but for now I'm blocked by bug 1328142 for the simulcast test.
Assignee: nobody → pehrson
Status: NEW → ASSIGNED
Depends on: 1328142
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

2 years ago
mozreview-review
Comment on attachment 8863675 [details]
Bug 1093835 - Check audio flow stopping after removing audio track and renegotiating.

https://reviewboard.mozilla.org/r/134688/#review139058

::: dom/media/tests/mochitest/test_peerConnection_removeAudioTrack.html:17
(Diff revision 1)
>    });
>  
>    var test;
>    runNetworkTest(function (options) {
>      test = new PeerConnectionTest(options);
> +    var receivedTrack, analyser, freq;

Use let in file scope.

::: dom/media/tests/mochitest/test_peerConnection_removeAudioTrack.html:28
(Diff revision 1)
> +          const ac = new AudioContext();
> +          analyser = new AudioStreamAnalyser(ac,
> +              new MediaStream([receivedTrack]));

ac can be inlined as well.

::: dom/media/tests/mochitest/test_peerConnection_removeAudioTrack.html:43
(Diff revision 1)
>          },
> +      ],
> +      [
> +        function PC_REMOTE_CHECK_FLOW_STOPPED(test) {
> +          is(test.pcRemote._pc.getReceivers().length, 0,
> +             "pcRemote should still have no more receivers");

still?
Attachment #8863675 - Flags: review?(jib) → review+

Comment 18

2 years ago
mozreview-review
Comment on attachment 8863676 [details]
Bug 1093835 - Check audio flow in test_pc_addSecondAudioStream.html.

https://reviewboard.mozilla.org/r/134690/#review139066

::: dom/media/tests/mochitest/test_peerConnection_addSecondAudioStream.html:14
(Diff revision 1)
>    runNetworkTest(function (options) {
> -    test = new PeerConnectionTest(options);
> +    const test = new PeerConnectionTest(options);

I've found no mozilla guidance on const vs. let...

I've been a traditionalist so far, resisting using const for mutable object references, reserving the semantic for immutable values. [1]

But it's hard to argue with the benefit of invariants. [2]

Let's see how it goes.

[1] http://stackoverflow.com/questions/41319111/choosing-how-to-better-take-advantage-of-es6-const-and-let
[2] https://softwareengineering.stackexchange.com/questions/278652/how-much-should-i-be-using-let-vs-const-in-es6

::: dom/media/tests/mochitest/test_peerConnection_addSecondAudioStreamNoBundle.html:14
(Diff revision 1)
>    runNetworkTest(function (options) {
>      options = options || { };
>      options.bundle = false;

While you're in here, pretty please do me a favor and change the || {} to:

    runNetworkTest(function (options = {}) {
        options.bundle = false;
        ...
Attachment #8863676 - Flags: review?(jib) → review+

Comment 19

2 years ago
mozreview-review
Comment on attachment 8863677 [details]
Bug 1093835 - Check audio flow in test_pc_addSecondAudioStreamNoBundle.html.

https://reviewboard.mozilla.org/r/134692/#review139070
Attachment #8863677 - Flags: review?(jib) → review+

Comment 20

2 years ago
mozreview-review
Comment on attachment 8863678 [details]
Bug 1093835 - Check audio flow in test_pc_removeThenAddAudioTrack.html.

https://reviewboard.mozilla.org/r/134694/#review139072

::: dom/media/tests/mochitest/test_peerConnection_removeThenAddAudioTrack.html:16
(Diff revision 1)
>    });
>  
> -  var test;
>    runNetworkTest(function (options) {
> -    test = new PeerConnectionTest(options);
> +    const test = new PeerConnectionTest(options);
> +    var originalTrack;

let

::: dom/media/tests/mochitest/test_peerConnection_removeThenAddAudioTrack.html:42
(Diff revision 1)
> +          const ac = new AudioContext();
> +          const analyser =
> +              new AudioStreamAnalyser(ac, new MediaStream([track]));

ac can be inlined as well
Attachment #8863678 - Flags: review?(jib) → review+

Comment 21

2 years ago
mozreview-review
Comment on attachment 8863679 [details]
Bug 1093835 - Check audio flow in test_pc_removeThenAddAudioTrackNoBundle.html.

https://reviewboard.mozilla.org/r/134696/#review139076

Earlier comments seem to apply throughout, so I'll avoid repeating them from here on.
Attachment #8863679 - Flags: review?(jib) → review+

Comment 22

2 years ago
mozreview-review
Comment on attachment 8863680 [details]
Bug 1093835 - Check video flow stopping after removing video track and renegotiating.

https://reviewboard.mozilla.org/r/134698/#review139078

::: dom/media/tests/mochitest/test_peerConnection_removeVideoTrack.html:30
(Diff revision 1)
> +          return new Promise(resolve =>
> +              element.addEventListener("loadeddata", resolve));

Is it worth reusing:

    return haveEvent(element, "loadeddata");

?
Attachment #8863680 - Flags: review?(jib) → review+

Comment 23

2 years ago
mozreview-review
Comment on attachment 8863681 [details]
Bug 1093835 - Commoditize checkVideoPlaying/checkVideoPaused.

https://reviewboard.mozilla.org/r/134700/#review139080

::: dom/media/tests/mochitest/head.js:911
(Diff revision 1)
>      var freq = analyser.binIndexForFrequency(TEST_AUDIO_FREQ);
>      return this.checkAudio(stream, analyser, array => array[freq] < 50);
>    }
>  }
>  
> -function VideoStreamHelper() {
> +function VideoFrameEmitter(color1, color2) {

Since you're redoing this, have you considered using JS class syntax for VideoFrameEmitter and VideoStreamHelper?

We have that in PeerConnection.js now [1] and like it.

[1] https://dxr.mozilla.org/mozilla-central/rev/a748acbebbde373a88868dc02910fb2bc5e6a023/dom/media/PeerConnection.js#1610

::: dom/media/tests/mochitest/head.js:933
(Diff revision 1)
>      var helper = this;
> -    return setInterval(function() {
> +    this._started = true;
> +    this._intervalId = setInterval(function() {
>        try {
>          helper._helper.drawColor(helper._canvas,
> -                                 i ? helper._helper.green : helper._helper.red);
> +                                 i ? helper._color1: helper._color2);

Use an arrow function in setInterval and you won't need helper anymore.

::: dom/media/tests/mochitest/head.js:972
(Diff revision 1)
> +           ". Pass=" + result);
> +      return result;
> +    });
> +  },
> +
> +  checkVideoPlaying: function(video, offsetX, offsetY, threshold) {

Have you considered rewriting checkVideoPlaying and checkVideoPaused with async/await?
Attachment #8863681 - Flags: review?(jib) → review+

Comment 24

2 years ago
mozreview-review
Comment on attachment 8863682 [details]
Bug 1093835 - Check video flow in test_pc_addSecondVideoStream.html.

https://reviewboard.mozilla.org/r/134702/#review139082
Attachment #8863682 - Flags: review?(jib) → review+

Comment 25

2 years ago
mozreview-review
Comment on attachment 8863683 [details]
Bug 1093835 - Check video flow in test_pc_addSecondVideoStreamNoBundle.html.

https://reviewboard.mozilla.org/r/134704/#review139084
Attachment #8863683 - Flags: review?(jib) → review+

Comment 26

2 years ago
mozreview-review
Comment on attachment 8863684 [details]
Bug 1093835 - Check video flow in test_pc_removeThenAddVideoTrack.html

https://reviewboard.mozilla.org/r/134706/#review139086
Attachment #8863684 - Flags: review?(jib) → review+

Comment 27

2 years ago
mozreview-review
Comment on attachment 8863685 [details]
Bug 1093835 - Check video flow in test_pc_removeThenAddVideoTrackNoBundle.html.

https://reviewboard.mozilla.org/r/134708/#review139088
Attachment #8863685 - Flags: review?(jib) → review+

Comment 28

2 years ago
mozreview-review
Comment on attachment 8863686 [details]
Bug 1093835 - Check video flow in test_pc_replaceVideoThenRenegotiate.html.

https://reviewboard.mozilla.org/r/134710/#review139090
Attachment #8863686 - Flags: review?(jib) → review+

Comment 29

2 years ago
mozreview-review
Comment on attachment 8863687 [details]
Bug 1093835 - Check video flow in test_pc_videoRenegotiationInactiveAnswer.html.

https://reviewboard.mozilla.org/r/134712/#review139092
Attachment #8863687 - Flags: review?(jib) → review+

Comment 30

2 years ago
mozreview-review
Comment on attachment 8863688 [details]
Bug 1093835 - Turn unnecessary array.map to array.forEach in pc.js.

https://reviewboard.mozilla.org/r/134714/#review139094
Attachment #8863688 - Flags: review?(jib) → review+
(Assignee)

Comment 31

2 years ago
mozreview-review-reply
Comment on attachment 8863680 [details]
Bug 1093835 - Check video flow stopping after removing video track and renegotiating.

https://reviewboard.mozilla.org/r/134698/#review139078

> Is it worth reusing:
> 
>     return haveEvent(element, "loadeddata");
> 
> ?

Sure is! (I wrote this patch many moons ago)
(Assignee)

Comment 32

2 years ago
mozreview-review-reply
Comment on attachment 8863681 [details]
Bug 1093835 - Commoditize checkVideoPlaying/checkVideoPaused.

https://reviewboard.mozilla.org/r/134700/#review139080

> Since you're redoing this, have you considered using JS class syntax for VideoFrameEmitter and VideoStreamHelper?
> 
> We have that in PeerConnection.js now [1] and like it.
> 
> [1] https://dxr.mozilla.org/mozilla-central/rev/a748acbebbde373a88868dc02910fb2bc5e6a023/dom/media/PeerConnection.js#1610

Sure, will do.

> Have you considered rewriting checkVideoPlaying and checkVideoPaused with async/await?

Didn't strike me yet, but that should be a fun exercise.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 47

2 years ago
Comment on attachment 8863681 [details]
Bug 1093835 - Commoditize checkVideoPlaying/checkVideoPaused.

A double-include of head.js in test_pc_close.html (redef of VideoStreamHelper) and simulcast tests being reenabled (used the old VideoStreamHelper) warrant a review of the interdiff IMHO.

Thanks!
Attachment #8863681 - Flags: review+ → review?(jib)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8863681 - Flags: review?(jib)

Comment 56

2 years ago
mozreview-review
Comment on attachment 8863681 [details]
Bug 1093835 - Commoditize checkVideoPlaying/checkVideoPaused.

https://reviewboard.mozilla.org/r/134700/#review139230

lgtm. Just two non-issue musings.

::: dom/media/tests/mochitest/head.js:977
(Diff revisions 1 - 3)
> -    return this.checkHasFrame(video, offsetX, offsetY, threshold).then(() => {
> +    await this.checkHasFrame(video, offsetX, offsetY, threshold);
> -      let startPixel = { data: h.getPixel(video, offsetX, offsetY)
> +    let startPixel = { data: h.getPixel(video, offsetX, offsetY)
> -                       , name: "startcolor"
> +                     , name: "startcolor"
> -                       };
> +                     };
> -      return h.waitForPixel(video, offsetX, offsetY, px => {
> +    return h.waitForPixel(video, offsetX, offsetY, px => {
> -        let result = h.isPixelNot(px, startPixel, threshold)
> +      let result = h.isPixelNot(px, startPixel, threshold)

missing semicolon (not that we need them)

::: dom/media/tests/mochitest/test_peerConnection_simulcastOffer.html:132
(Diff revision 3)
>          // the resolution isn't updated on the video element on the first frame.
> -        function PC_REMOTE_WAIT_FOR_FRAMES_3() {
> -          var vremote = test.pcRemote.remoteMediaElements[0];
> +        async function PC_REMOTE_WAIT_FOR_FRAMES_3() {
> +          const vremote = test.pcRemote.remoteMediaElements[0];
>            ok(vremote, "Should have remote video element for pcRemote");
> -          return helper.waitForFrames(vremote);
> +          emitter.start();
> +          await helper.checkVideoPlaying(vremote, 10, 10, 16);

I'm seeing 10, 10, 16 copied copiously throughout. Would it be helpful if helper.checkVideoPlaying() defaulted to 10, 10, 16 as defaults, or maybe if they need to be tuned to the video in question, allow you to pass in default values in the helper's constructor?
Attachment #8863681 - Flags: review?(jib) → review+
(Assignee)

Comment 57

2 years ago
mozreview-review-reply
Comment on attachment 8863681 [details]
Bug 1093835 - Commoditize checkVideoPlaying/checkVideoPaused.

https://reviewboard.mozilla.org/r/134700/#review139230

> I'm seeing 10, 10, 16 copied copiously throughout. Would it be helpful if helper.checkVideoPlaying() defaulted to 10, 10, 16 as defaults, or maybe if they need to be tuned to the video in question, allow you to pass in default values in the helper's constructor?

Defaults would be ok, but on the other hand I always found it useful to be explicit.
Implicitly (defined in nested callees) the defaults are, checkVideoPlaying: 0, 0, 0, checkVideoPaused: 0, 0, 127.

I'll defer it to another patch in case someone has a strong opinion about it.

Comment 58

2 years ago
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9983a4f7101f
Check audio flow stopping after removing audio track and renegotiating. r=jib
https://hg.mozilla.org/integration/autoland/rev/3825c6856dd1
Check audio flow in test_pc_addSecondAudioStream.html. r=jib
https://hg.mozilla.org/integration/autoland/rev/deed247384e1
Check audio flow in test_pc_addSecondAudioStreamNoBundle.html. r=jib
https://hg.mozilla.org/integration/autoland/rev/6c4403d7b9c5
Check audio flow in test_pc_removeThenAddAudioTrack.html. r=jib
https://hg.mozilla.org/integration/autoland/rev/18156171c142
Check audio flow in test_pc_removeThenAddAudioTrackNoBundle.html. r=jib
https://hg.mozilla.org/integration/autoland/rev/b5140fba95ef
Check video flow stopping after removing video track and renegotiating. r=jib
https://hg.mozilla.org/integration/autoland/rev/734031e48e90
Commoditize checkVideoPlaying/checkVideoPaused. r=jib
https://hg.mozilla.org/integration/autoland/rev/9f875647e245
Check video flow in test_pc_addSecondVideoStream.html. r=jib
https://hg.mozilla.org/integration/autoland/rev/a76b96c86835
Check video flow in test_pc_addSecondVideoStreamNoBundle.html. r=jib
https://hg.mozilla.org/integration/autoland/rev/a8d125ad8699
Check video flow in test_pc_removeThenAddVideoTrack.html r=jib
https://hg.mozilla.org/integration/autoland/rev/f58e870d151a
Check video flow in test_pc_removeThenAddVideoTrackNoBundle.html. r=jib
https://hg.mozilla.org/integration/autoland/rev/f19c2d8aa1fe
Check video flow in test_pc_replaceVideoThenRenegotiate.html. r=jib
https://hg.mozilla.org/integration/autoland/rev/a8a8fabfec7f
Check video flow in test_pc_videoRenegotiationInactiveAnswer.html. r=jib
https://hg.mozilla.org/integration/autoland/rev/d6aa0f1171bc
Turn unnecessary array.map to array.forEach in pc.js. r=jib
(Assignee)

Comment 59

2 years ago
Forgot to push the semicolon before autolanding. Oh well.
You need to log in before you can comment on or make changes to this bug.