Closed Bug 1093835 Opened 10 years ago Closed 8 years ago

test_peerConnection_addSecondAudioStream.html needs verification of second media flow

Categories

(Core :: WebRTC, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bwc, Assigned: pehrsons)

References

Details

Attachments

(14 files)

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

Created:
Updated:
Size: