Closed Bug 1093835 Opened 10 years ago Closed 7 years ago

test_peerConnection_addSecondAudioStream.html needs verification of second media flow

Categories

(Core :: WebRTC, defect, P3)

Product:
Core
Component:
WebRTC
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: bwc, Assigned: pehrsons)

References

Details

Attachments

(14 files)

Bug 1093835 - Check audio flow stopping after removing audio track and renegotiating.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1093835 - Check audio flow in test_pc_addSecondAudioStream.html.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1093835 - Check audio flow in test_pc_addSecondAudioStreamNoBundle.html.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1093835 - Check audio flow in test_pc_removeThenAddAudioTrack.html.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1093835 - Check audio flow in test_pc_removeThenAddAudioTrackNoBundle.html.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1093835 - Check video flow stopping after removing video track and renegotiating.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1093835 - Commoditize checkVideoPlaying/checkVideoPaused.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1093835 - Check video flow in test_pc_addSecondVideoStream.html.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1093835 - Check video flow in test_pc_addSecondVideoStreamNoBundle.html.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1093835 - Check video flow in test_pc_removeThenAddVideoTrack.html
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1093835 - Check video flow in test_pc_removeThenAddVideoTrackNoBundle.html.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1093835 - Check video flow in test_pc_replaceVideoThenRenegotiate.html.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1093835 - Check video flow in test_pc_videoRenegotiationInactiveAnswer.html.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1093835 - Turn unnecessary array.map to array.forEach in pc.js.
59 bytes, text/x-review-board-request
jib
: review+
Details 
      No description provided.
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3
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 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 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 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 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 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 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 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 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 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 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 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 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 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 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+
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)
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 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)
Attachment #8863681 - Flags: review?(jib)
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+
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.
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: