Create a loopback device that will be installed during mochitest installation and will allow to use for automated testing.

I am writing to provide an update and mention all the challenges that I face on that: Good news are that the desired loopback behavior can be achieved by installing module-null-source, making it the default output and using null.monitor device for capture. With this setup anything we playback will directed to the input device and can be captured. The above changes can be implemented in 'testing/mochitest/runtests.py' to apply every time test is executed with --use-fake-device-for-media-stream. The bad news are that I'm not sure this is the right approach. Some tests want to playback the captured stream [1]. In this case we loopback that extra playback, so the incoming stream become something unexpected. Any playback from the test destroys the expected input audio. Should we make tests not playback anything because it's convenience to the new setup? In addition to that the incoming audio is very bad when processing is on. There is no way to apply any audio analysis on that audio. One solution on the first issue could be to create a new execution argument (instead of using the existing --use-fake-device-for-media-stream) and wrap all the logic around. It requires more effort but we do not mess with the existing tests. For the second issue we can simple turn off processing especially AEC. Please tell me thoughts. [1] http://searchfox.org/mozilla-central/source/dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_audio.html Current try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ed973042fcdbb582dcbae01c8e5021ed60143ea

I think in the long term we'll make the null sink the non-default output device, so anything we want to go through the capture device can still be routed to the null sink with setSinkId. In the short term I think it's fine to switch tests that are inherently hard to fix (when it's not enough to mute playback of the gUM stream we're testing) to force our built-in fake device by adding fake:true to the constraints and s/TEST_AUDIO_FREQ/1000/. That said, shouldn't it be fine to mute the input in test [1]?

After discussion with jib we agreed on using the "fake: true" approach to workaround the existing failing tests. So I will add "fake: true" on all failed test to use the fake device and on Linux. New tests without "fake: true" will use the new virtual device configuration. Feel free to comment.

Wasn't it enough to add muting in http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/dom/media/tests/mochitest/head.js#247 ?

I'll check that option too. I am looking for a solution that will work for all failed test and will have the minimum effect on them.

Failed tests: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_audio.html dom/media/tests/mochitest/test_getUserMedia_peerIdentity.html dom/media/tests/mochitest/test_peerConnection_addAudioTrackToExistingVideoStream.html dom/media/tests/mochitest/test_peerConnection_addSecondAudioStream.html dom/media/tests/mochitest/test_peerConnection_removeAudioTrack.html dom/media/tests/mochitest/test_peerConnection_removeThenAddAudioTrack.html dom/media/tests/mochitest/test_peerConnection_addSecondAudioStreamNoBundle.html dom/media/tests/mochitest/test_peerConnection_removeThenAddAudioTrackNoBundle.html dom/media/tests/mochitest/test_peerConnection_verifyAudioAfterRenegotiation.html dom/media/tests/mochitest/test_peerConnection_audioRenegotiationInactiveAnswer.html

(In reply to Alex Chronopoulos [:achronop] from comment #6) > Failed tests: > dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_audio.html Media element playing a MediaStream gets mozCaptureStream()ed, so keeps outputting audio. Should be ok with mute. > dom/media/tests/mochitest/test_getUserMedia_peerIdentity.html The audio check uses an audio element that is not used, [1]. Let's remove that. To unify code here we should rewrite this to use AudioStreamAnalyser instead, [2]. > dom/media/tests/mochitest/test_peerConnection_addAudioTrackToExistingVideoStream.html > dom/media/tests/mochitest/test_peerConnection_addSecondAudioStream.html > dom/media/tests/mochitest/test_peerConnection_removeAudioTrack.html > dom/media/tests/mochitest/test_peerConnection_removeThenAddAudioTrack.html > dom/media/tests/mochitest/test_peerConnection_addSecondAudioStreamNoBundle.html > dom/media/tests/mochitest/test_peerConnection_removeThenAddAudioTrackNoBundle.html > dom/media/tests/mochitest/test_peerConnection_verifyAudioAfterRenegotiation.html > dom/media/tests/mochitest/test_peerConnection_audioRenegotiationInactiveAnswer.html These are all just using the webrtc testing framework so muting in createMediaElement should be fine. They do have TEST_AUDIO_FREQ in common though, but we should be able to set that correctly in the setup code in head.js. [1] http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/dom/media/tests/mochitest/blacksilence.js#67 [2] http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/dom/media/tests/mochitest/head.js#33

Tested the first option which is to set "fake: true" on the constraints and I get a green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=57f2b2cd63248ddbfbddff3edac962058581ed1a I will continue with the other one.

This loopback setup has an important disadvantage. Since the output becomes the input when processing (especially echo canceling) is on, the the spectrum of the input signal is altered significantly. For example Andreas's works well _but_ you have to disable processing first. Otherwise tests are not stable and fail mostly. This is something we must keep in mind when we create test using the new setup.

s/Andreas's works well/Andreas's idea works well/

If we have setSinkId we could have one sink that bypasses the AEC, or even tell setSinkId with a chrome-only thing to bypass the AEC, (the null-sink) which we also catch on the input. Another sink could be default and be fed to the AEC as far-end data. So I agree it's a bit of a hassle now, but in the long run the situation will be better. The latter is more important IMO.

Comment on attachment 8925098 [details] Bug 1406350 - part1: Update runtest.py to new loopback setup. https://reviewboard.mozilla.org/r/196330/#review202156 ::: testing/mochitest/runtests.py:755 (Diff revision 1) > - if not sine_source_loaded(): > - log.error('Couldn\'t load module-sine-source') > + > + if not null_sink_loaded(): > + log.error('Couldn\'t load module-null-sink') > return None > > + # Either was loaded or not make it the default output nit: s/Either/Whether it/

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review202160 ::: dom/media/tests/mochitest/head.js (Diff revision 1) > var audioDevice = SpecialPowers.getCharPref('media.audio_loopback_dev'); > var videoDevice = SpecialPowers.getCharPref('media.video_loopback_dev'); > dump('TEST DEVICES: Using media devices:\n'); > dump('audio: ' + audioDevice + '\nvideo: ' + videoDevice + '\n'); > FAKE_ENABLED = false; > - TEST_AUDIO_FREQ = 440; We still want a distinguishable frequency for the output monitor IMO. Perhaps pass the frequency to the utility function and it will update TEST_AUDIO_FREQ? (if we use 1k for both we could miss if gUM uses the fake device instead of the null monitor) ::: dom/media/tests/mochitest/head.js:24 (Diff revision 1) > } catch (e) { > dump('TEST DEVICES: No test devices found (in media.{audio,video}_loopback_dev, using fake streams.\n'); > FAKE_ENABLED = true; > } > > +function play_sine_output(frequency) { I think we should have something that better explains the intention of this function. How about `setup_loopback_capture_sine`? ::: dom/media/tests/mochitest/head.js:32 (Diff revision 1) > + let osc = ac.createOscillator(); > + osc.connect(ac.destination); > + osc.frequency.value = frequency; > + osc.start(); > +} > +play_sine_output(TEST_AUDIO_FREQ); Don't call this on load. Tests must be able to opt in or out. It doesn't have to called explicitly from each test either. You could pass something to `createHTML()` to configure the basic case, and leave it to the test when something more advanced is needed. ::: dom/media/tests/mochitest/mochitest.ini:53 (Diff revision 1) > +[test_getUserMedia_basicAudio_loopback.html] > +skip-if = os == 'mac' || os == 'win' || toolkit == 'android' # Bug xxxxxx use virtual devices available on linux Needs bug numbers ::: dom/media/tests/mochitest/test_getUserMedia_basicAudio_loopback.html:10 (Diff revision 1) > +</head> > +<body> > +<pre id="test"> > + > +<script> > + createHTML({ title: "getUserMedia Basic Audio Test Loopback", bug: "xxxxx", visible: true }); s/xxxxx/1406350/ ::: dom/media/tests/mochitest/test_getUserMedia_basicAudio_loopback.html:11 (Diff revision 1) > + /** > + * Run a test to verify that we record audio of the giver frequency. > + */ > + let constraints = {audio: {autoGainControl: false, > + echoCancellation: false, > + noiseSuppression: false}}; // Remove processing > + > + scriptsReady > + .then(() => FAKE_ENABLED = false) > + .then(() => runTestWhenReady( async () => { > + let stream = await getUserMedia(constraints); > + > + let analyser = new AudioStreamAnalyser(new AudioContext(), stream); > + analyser.enableDebugCanvas(); > + await analyser.waitForAnalysisSuccess( array => { > + // High energy on 1000 Hz low energy around that > + const freg_50Hz = array[analyser.binIndexForFrequency(50)]; > + const freq = array[analyser.binIndexForFrequency(TEST_AUDIO_FREQ)]; > + const freq_2000Hz = array[analyser.binIndexForFrequency(2000)]; > + > + info("Analysing audio frequency - lower:target:hi = " + freg_50Hz + ':' + freq + ':' + freq_2000Hz); > + return freg_50Hz < 50 && freq > 200 && freq_2000Hz < 50; > + }) > + })) > + .then(() => finish()) If we want an explicit test to check that the loopback device works we should make the output (what is fed to the loopback device) explicit as well, and a bit more elaborate, to check that various output paths work. At a minimum I think we want a tone through MSG (WebAudio) at != TEST_AUDIO_FREQ Hz, and another through playback (so record an offline audio node with MediaRecorder and loop the blob in an audio element).

Comment on attachment 8925100 [details] Bug 1406350 - part3: Add fake input in mda2 and mda3 mochitests. https://reviewboard.mozilla.org/r/196334/#review202164 We want to keep these tests on the loopback device unless it's really impossible. Does it work if you update `createMediaElement()`, [1], to return a muted element instead? That should leave the individual tests intact. [1] http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/dom/media/tests/mochitest/head.js#247

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review202160 > If we want an explicit test to check that the loopback device works we should make the output (what is fed to the loopback device) explicit as well, and a bit more elaborate, to check that various output paths work. > > At a minimum I think we want a tone through MSG (WebAudio) at != TEST_AUDIO_FREQ Hz, and another through playback (so record an offline audio node with MediaRecorder and loop the blob in an audio element). Do you mean additional tests here? Will the test create their own output with loopback_tone=false and check that output through gUM + analyzer? Is it ok to create that on a follow up bug?

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review202262 ::: dom/media/tests/mochitest/head.js:19 (Diff revision 2) > var audioDevice = SpecialPowers.getCharPref('media.audio_loopback_dev'); > var videoDevice = SpecialPowers.getCharPref('media.video_loopback_dev'); > dump('TEST DEVICES: Using media devices:\n'); > dump('audio: ' + audioDevice + '\nvideo: ' + videoDevice + '\n'); > FAKE_ENABLED = false; > TEST_AUDIO_FREQ = 440; Make `TEST_AUDIO_FREQ` `undefined`, `-1` or similar here, so we can detect there's no tone active. ::: dom/media/tests/mochitest/head.js:25 (Diff revision 2) > +function setup_loopback_capture_tone(frequency) { > + dump("Start output tone at " + frequency); > + let ac = new AudioContext(); > + let osc = ac.createOscillator(); > + osc.connect(ac.destination); > + osc.frequency.value = frequency; > + osc.start(); > +} Also test that `TEST_AUDIO_FREQ` was unset here, and set it to `frequency`. ::: dom/media/tests/mochitest/head.js:26 (Diff revision 2) > dump('TEST DEVICES: No test devices found (in media.{audio,video}_loopback_dev, using fake streams.\n'); > FAKE_ENABLED = true; > } > > +function setup_loopback_capture_tone(frequency) { > + dump("Start output tone at " + frequency); s/dump/info/ Also you can write ``` info(`Start output tone at ${frequency}`); ``` ::: dom/media/tests/mochitest/head.js:213 (Diff revision 2) > + * @param {boolean} [meta.loopback_tone=true] > + * Setup test loopback capture tone I think defaulting to `false` is more natural, but we shouldn't have negation in the name (like "disable_loopback_tone"). How about `meta.loopback_explicit` or so? Where `true` means the test will be responsible to take care of it. ::: dom/media/tests/mochitest/head.js:248 (Diff revision 2) > + if (!meta.loopback_sine) { > + meta.loopback_sine = true; > + } If we default to false, it'll already be falsy here so this check is unnecessary. ::: dom/media/tests/mochitest/head.js:252 (Diff revision 2) > + > + if (!meta.loopback_sine) { > + meta.loopback_sine = true; > + } > + if (meta.loopback_sine) { > + setup_loopback_capture_tone(TEST_AUDIO_FREQ); Hey, a tab! ::: dom/media/tests/mochitest/head.js:252 (Diff revision 2) > + > + if (!meta.loopback_sine) { > + meta.loopback_sine = true; > + } > + if (meta.loopback_sine) { > + setup_loopback_capture_tone(TEST_AUDIO_FREQ); s/TEST_AUDIO_FREQ/440/ ::: dom/media/tests/mochitest/mochitest.ini:54 (Diff revision 2) > [test_getUserMedia_addTrackRemoveTrack.html] > skip-if = android_version == '18' || os == 'linux' # android(Bug 1189784, timeouts on 4.3 emulator), linux bug 1377450 > [test_getUserMedia_addtrack_removetrack_events.html] > skip-if = os == 'linux' && debug # Bug 1389983 > +[test_getUserMedia_basicAudio_loopback.html] > +skip-if = os == 'mac' || os == 'win' || toolkit == 'android' # Bug 1406350 use virtual devices available on linux This should list the bug numbers for getting loopback devices on mac, windows and android respectively.

Comment on attachment 8925100 [details] Bug 1406350 - part3: Add fake input in mda2 and mda3 mochitests. https://reviewboard.mozilla.org/r/196334/#review202276 ::: dom/media/tests/mochitest/test_getUserMedia_mediaElementCapture_audio.html:19 (Diff revision 2) > }); > > var audioContext; > var gUMAudioElement; > var analyser; > -runTest(() => getUserMedia({audio: true}) > +runTest(() => getUserMedia({fake: true, audio: true}) I guess my previous feedback didn't show up as an issue. Apologies. Here it is again. We want to keep these tests on the loopback device unless it's really impossible. Does it work if you update createMediaElement(), [1], to return a muted element instead? That should leave the individual tests intact. [1] http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/dom/media/tests/mochitest/head.js#247

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review202262 > Make `TEST_AUDIO_FREQ` `undefined`, `-1` or similar here, so we can detect there's no tone active. I'm not sure how this will work with the rest of mochitests. Many of them make use of that variable and this will break them. This is the list: test_getUserMedia_basicAudio_loopback.html test_getUserMedia_mediaElementCapture_audio.html test_peerConnection_addSecondAudioStream.html test_peerConnection_addSecondAudioStreamNoBundle.html test_peerConnection_removeAudioTrack.html test_peerConnection_removeThenAddAudioTrack.html test_peerConnection_removeThenAddAudioTrackNoBundle.html

Comment on attachment 8925100 [details] Bug 1406350 - part3: Add fake input in mda2 and mda3 mochitests. https://reviewboard.mozilla.org/r/196334/#review202276 > I guess my previous feedback didn't show up as an issue. Apologies. Here it is again. > > > We want to keep these tests on the loopback device unless it's really impossible. > > Does it work if you update createMediaElement(), [1], to return a muted element instead? That should leave the individual tests intact. > > [1] http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/dom/media/tests/mochitest/head.js#247 Yes, in addition we need to turn off processing. Let me know if you want that.

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review202262 > I'm not sure how this will work with the rest of mochitests. Many of them make use of that variable and this will break them. This is the list: > > test_getUserMedia_basicAudio_loopback.html > test_getUserMedia_mediaElementCapture_audio.html > test_peerConnection_addSecondAudioStream.html > test_peerConnection_addSecondAudioStreamNoBundle.html > test_peerConnection_removeAudioTrack.html > test_peerConnection_removeThenAddAudioTrack.html > test_peerConnection_removeThenAddAudioTrackNoBundle.html It would still be set in `setup_loopback_capture_tone`. It's just to catch tests relying on TEST_AUDIO_FREQ but not setting up the infra to actually emit a tone at that frequency.

Comment on attachment 8925100 [details] Bug 1406350 - part3: Add fake input in mda2 and mda3 mochitests. https://reviewboard.mozilla.org/r/196334/#review202276 > Yes, in addition we need to turn off processing. Let me know if you want that. Right, we can use `meta.loopback_explicit == false` as a flag to turn off processing in `getUserMedia()`, [1]. But make sure to fail the test if a constraint to enable processing was explicitly passed in when using the implicit loopback setup. [1] http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/dom/media/tests/mochitest/head.js#298

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review202262 > It would still be set in `setup_loopback_capture_tone`. It's just to catch tests relying on TEST_AUDIO_FREQ but not setting up the infra to actually emit a tone at that frequency. Yeah, got it when I read the rest of the comments :)

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review202262 > Also test that `TEST_AUDIO_FREQ` was unset here, and set it to `frequency`. Actually this is not my intention here. I want to leave `setup_loopback_capture_tone()` method available as an easy way for the user to creatr one or more loopback tones. I do want to allow user calling it more than once and I do not want to reset TEST_AUDIO_FREQ on any additional call of the method. We could identify the case from `meta.loopback_explicit`.

Comment on attachment 8925100 [details] Bug 1406350 - part3: Add fake input in mda2 and mda3 mochitests. https://reviewboard.mozilla.org/r/196334/#review202276 > Right, we can use `meta.loopback_explicit == false` as a flag to turn off processing in `getUserMedia()`, [1]. > > But make sure to fail the test if a constraint to enable processing was explicitly passed in when using the implicit loopback setup. > > [1] http://searchfox.org/mozilla-central/rev/7e090b227f7a0ec44d4ded604823d48823158c51/dom/media/tests/mochitest/head.js#298 That should work but please note that I am planning to chage the test itself and not the `head.js`. That said, it seems better to do a follow up patch about it. In general my plan is to focus on the basic functionality now and not on invidual tests.

(In reply to Alex Chronopoulos [:achronop] from comment #3) > After discussion with jib we agreed on using the "fake: true" approach to > workaround the existing failing tests. So I will add "fake: true" on all > failed test to use the fake device and on Linux. New tests without "fake: > true" will use the new virtual device configuration. Feel free to comment. FWIW I disagree with the general use of this approach. It should be a last resort only. We're doing big changes to the capture side in tests. We shouldn't remove tests from this path in order to fix shortcomings in our test setup (polluted output). Instead we should just fix these shortcomings. With my suggestion in comment 4 I don't think we need to set the 10 tests in comment 6 to fake:true. If we indeed don't, that would be a great win. If we still do, we need to figure out why and see if that's easily fixable. It's clear that audio processing can distort the captured audio, but then I think the more sensible alternative is to disable processing in the tests that rely on audio analyses. I suggest how we can plumb this in comment 21. The other issue I have with adding fake:true when it is not inherently needed. Nothing will regress it so it may be forgotten. This happens way too often so should be disallowed IMO. Again, as a last resort it would be all right. But we need to go through some hoops to establish that it is the last resort. I don't see that having happened yet.

I think comment 30 makes sense, for the tests where muting the element(s) is an option. When Alex and I last discussed this we were looking at a test that used element.captureStream which didn't work if we muted the element, so that one at least still needs fake:true. If we can avoid fake:true on the other 9 tests then I agree that's a win. I like Andreas's idea of muting by default in head.js, and then having tests that rely on elements not being muted to turn muting off explicitly.

New set of error using the new configuration: mda2: dom/media/tests/mochitest/test_getUserMedia_GC_MediaStream.html | Original gUM stream should be collectable dom/media/tests/mochitest/test_getUserMedia_audioCapture.html | Error executing test: Error: Audio analysis timed out waitForAnalysisSuccess@http://mochi. dom/media/tests/mochitest/test_getUserMedia_constraints.html | Triggering mock failure in default audio device fails - got null, expected "NotReadableError" [log…] dom/media/tests/mochitest/test_peerConnection_addSecondAudioStream.html | Error in test execution: Error: Audio analysis timed out waitForAnalysisSuccess@http://mochi. dom/media/tests/mochitest/test_peerConnection_addSecondAudioStream.html | application timed out after 330 seconds with no output [ 0.11 ] dom/media/tests/mochitest/test_peerConnection_removeAudioTrack.html | Error in test execution: Error: Audio analysis timed out waitForAnalysisSuccess dom/media/tests/mochitest/test_peerConnection_removeThenAddAudioTrack.html | Error in test execution: Error: Audio analysis timed out waitForAnalysisSuccess@http://mochi mda3: TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_addSecondAudioStreamNoBundle.html | Error in test execution: Error: Audio analysis timed out TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_audioRenegotiationInactiveAnswer.html | Error in test execution: Error: Audio analysis timed out TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_removeThenAddAudioTrackNoBundle.html | Error in test execution: Error: Audio analysis timed out TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_verifyAudioAfterRenegotiation.html | Error in test execution: Error: Audio analysis timed out

New try, the following 2 test cases expected to fail. Still working on them: dom/media/tests/mochitest/test_getUserMedia_GC_MediaStream.html dom/media/tests/mochitest/test_getUserMedia_constraints.html try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5960d8a32362a917a9696354313393d09074b8ec

Better than expected. One failed test: [task 2017-11-20T16:13:03.439Z] 16:13:03 INFO - TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_getUserMedia_GC_MediaStream.html | Original gUM stream should be collectable - got 2, expected +0 [task 2017-11-20T16:13:03.442Z] 16:13:03 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:312:5 [task 2017-11-20T16:13:03.443Z] 16:13:03 INFO - @dom/media/tests/mochitest/test_getUserMedia_GC_MediaStream.html:62:5 [task 2017-11-20T16:13:03.445Z] 16:13:03 INFO - async*runTestWhenReady/<@dom/media/tests/mochitest/head.js:402:41 [task 2017-11-20T16:13:03.446Z] 16:13:03 INFO - promise callback*runTestWhenReady@dom/media/tests/mochitest/head.js:402:10 [task 2017-11-20T16:13:03.447Z] 16:13:03 INFO - runTest/<@dom/media/tests/mochitest/mediaStreamPlayback.js:250:15 [task 2017-11-20T16:13:03.448Z] 16:13:03 INFO - promise callback*runTest@dom/media/tests/mochitest/mediaStreamPlayback.js:249:31 [task 2017-11-20T16:13:03.450Z] 16:13:03 INFO - @dom/media/tests/mochitest/test_getUserMedia_GC_MediaStream.html:31:3 Probably an error in MSG or GraphDriver. And one leak on debug probably side effect of the above: [task 2017-11-20T16:21:32.596Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 1 BackstagePass [task 2017-11-20T16:21:32.596Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 1 CondVar [task 2017-11-20T16:21:32.596Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 1 IdlePeriod [task 2017-11-20T16:21:32.596Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 2 Mutex [task 2017-11-20T16:21:32.597Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 1 ThreadEventTarget [task 2017-11-20T16:21:32.597Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 1 ThreadTargetSink [task 2017-11-20T16:21:32.598Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 2 XPCNativeInterface [task 2017-11-20T16:21:32.598Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 2 XPCNativeMember [task 2017-11-20T16:21:32.598Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 2 XPCNativeSet [task 2017-11-20T16:21:32.599Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 2 XPCWrappedNative [task 2017-11-20T16:21:32.599Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 1 XPCWrappedNativeProto [task 2017-11-20T16:21:32.599Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 2 XPCWrappedNativeTearOff [task 2017-11-20T16:21:32.600Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsJSPrincipals [task 2017-11-20T16:21:32.600Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsStringBuffer [task 2017-11-20T16:21:32.601Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 5 nsTArray_base [task 2017-11-20T16:21:32.601Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsThread [task 2017-11-20T16:21:32.601Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsTimer [task 2017-11-20T16:21:32.601Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsTimerImpl [task 2017-11-20T16:21:32.601Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsXPCWrappedJS [task 2017-11-20T16:21:32.602Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 1 nsXPCWrappedJSClass [task 2017-11-20T16:21:32.602Z] 16:21:32 INFO - TEST-INFO | leakcheck | default process: leaked 3 xptiInterfaceInfo [task 2017-11-20T16:21:32.603Z] 16:21:32 ERROR - TEST-UNEXPECTED-FAIL | leakcheck | default process: 1724 bytes leaked (BackstagePass, CondVar, IdlePeriod, Mutex, ThreadEventTarget, ...)

The problem was that the loopback tone created the extra 2 streams which made the stream count fail. Solved by disabling explicitly the loopback tone in createHTML(). Test makes use of video stream only so it does not use loopback tone anyway. In addition no leak error any more. Green try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=03129562338069d720bfc9c96cc92ae62649c5f9

A wider try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=23ca91f26ac5e2eea567f455377d45cdc428deb9

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review208134 ::: dom/media/tests/mochitest/head.js:26 (Diff revision 3) > dump('TEST DEVICES: No test devices found (in media.{audio,video}_loopback_dev, using fake streams.\n'); > FAKE_ENABLED = true; > } > > /** > + * This class provides helpers to create a sine tone of a given frequency. It looks more like it is that helper itself. ::: dom/media/tests/mochitest/head.js:27 (Diff revision 3) > FAKE_ENABLED = true; > } > > /** > + * This class provides helpers to create a sine tone of a given frequency. > + * Class should be used when FAKE_ENABLED is false. Hmm, no, it's always instantiated. ::: dom/media/tests/mochitest/head.js:29 (Diff revision 3) > +class LoopbackTone { > + constructor() { > + var _audioContext = null; > + var _oscNode = null; > + > + this.skip = false; > + > + this.start = function(frequency) { > + if (this.skip == true) { > + return; > + } > + if (_audioContext == null) { > + dump(`Start loopback tone at ${frequency}`); > + _audioContext = new AudioContext(); > + _oscNode = _audioContext.createOscillator(); > + _oscNode.connect(_audioContext.destination); > + _oscNode.frequency.value = frequency; > + _oscNode.start(); > + } > + } > + > + this.stop = function() { > + if (_audioContext) { > + _oscNode.stop(); > + _audioContext = null; > + } > + } > + } > +}; How about we make the constructor take/create all that is needed to create this tone, and start() just starts it? That seems more semantically correct. And we shouldn't be creating new AudioContexts all the time. So, let the constructor take an audio context and the frequency (or do you foresee this being reused?), then it creates and sets up the Oscillator. start() will then connect the oscillator to the destination and start() it. stop() will then disconnect and stop(). ::: dom/media/tests/mochitest/head.js:31 (Diff revision 3) > + var _audioContext = null; > + var _oscNode = null; these are members, so `this._audioContext = null;`, etc. ::: dom/media/tests/mochitest/head.js:36 (Diff revision 3) > + var _audioContext = null; > + var _oscNode = null; > + > + this.skip = false; > + > + this.start = function(frequency) { See how to define methods at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes ::: dom/media/tests/mochitest/head.js:40 (Diff revision 3) > + > + this.start = function(frequency) { > + if (this.skip == true) { > + return; > + } > + if (_audioContext == null) { This type of check should be ``` if (!_audioContext) { throw new Error("Wut, no AudioContext!?"); } ``` unless you want this to be possible: ``` tone.start(1000); tone.start(2000); ``` but what would the second call be expected to do? and how should this behave? ``` tone.start(1000); tone.start(2000); tone.stop(); ``` ::: dom/media/tests/mochitest/head.js:59 (Diff revision 3) > + } > + } > + } > +}; > +// The object that provide the tone the tone automatically by default. > +var DefaultLoopbackTone = new LoopbackTone; I think we should follow the precedence in this file for global state. I.e., make it an ALL_CAPS name and a primitive type. I guess you need to know globally if it's on or not too, so it'd make sense to flex the global state type rule for that. ::: dom/media/tests/mochitest/head.js:61 (Diff revision 3) > +function OptOutDefaultLoopbackTone() { > + DefaultLoopbackTone.skip = true; > +} I'm not sure of this terminology since it's negative (OptIn would be the positive version). That's why I named it `request...()` in the pastebin I sent you. You might want to consult jib too, he's usually better than me at getting names right :-) ::: dom/media/tests/mochitest/head.js:64 (Diff revision 3) > +// Helper function to set a tone at a given frequency > +function setPersistentLoopbackTone(frequency) { > + (new LoopbackTone).start(frequency); > +} This is not used. Also, it would get garbage collected pretty much immediately. I'd say, either use this global function to set up the tone, or the LoopbackTone class. Not both. ::: dom/media/tests/mochitest/head.js:298 (Diff revision 3) > element.setAttribute('id', id); > element.setAttribute('height', 100); > element.setAttribute('width', 150); > element.setAttribute('controls', 'controls'); > element.setAttribute('autoplay', 'autoplay'); > + element.muted = true; Use `setAttribute` like the others. We want to set the html attribute, not the js one. The former becomes the default of the latter. ::: dom/media/tests/mochitest/head.js:351 (Diff revision 3) > + && constraints.audio > + && !DefaultLoopbackTone.skip) { > + // Loopback device is configured, start the default loopback tone > + DefaultLoopbackTone.start(TEST_AUDIO_FREQ) > + // Disable input audio processing to avoid distortion of the loopback tone > + constraints.audio = Object.assign({}, constraints.audio, {autoGainControl: false}, {echoCancellation: false}, {noiseSuppression: false}); You need to treat these individually, or what happens if the test is deliberately passing in one of them set to true? ::: dom/media/tests/mochitest/head.js:389 (Diff revision 3) > + > + one empty line should be enough ::: dom/media/tests/mochitest/mochitest.ini:55 (Diff revision 3) > [test_getUserMedia_addTrackRemoveTrack.html] > skip-if = android_version == '18' || os == 'linux' # android(Bug 1189784, timeouts on 4.3 emulator), linux bug 1377450 > [test_getUserMedia_addtrack_removetrack_events.html] > skip-if = os == 'linux' && debug # Bug 1389983 > +[test_getUserMedia_basicAudio_loopback.html] > +skip-if = os == 'mac' || os == 'win' || toolkit == 'android' # one general bug for all OSs Bug 1404995 Say the reason why they're disabled (no loopback devices on some platforms) and put it after the bug number ::: dom/media/tests/mochitest/test_getUserMedia_GC_MediaStream.html:32 (Diff revision 3) > is(await SpecialStream.countUnderlyingStreams(), startStreams, > "MediaStreams should have been collected"); > } > > runTest(async () => { > + OptOutDefaultLoopbackTone(); Please add a comment on why this is needed for this test. ::: dom/media/tests/mochitest/test_getUserMedia_basicAudio_loopback.html:10 (Diff revision 3) > +</head> > +<body> > +<pre id="test"> > + > +<script> > + createHTML({ title: "getUserMedia Basic Audio Test Loopback", bug: "1406350", visible: true }); We cap at 80 columns https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style ::: dom/media/tests/mochitest/test_getUserMedia_basicAudio_loopback.html:12 (Diff revision 3) > +<pre id="test"> > + > +<script> > + createHTML({ title: "getUserMedia Basic Audio Test Loopback", bug: "1406350", visible: true }); > + /** > + * Run a test to verify that we record audio of the giver frequency. This should mention the purpose of this test. I can see what you're testing but I'm not sure it's all we want to test, so having this description to compare against would be useful. (I could see value in making this use both the default path and add an explicit frequency on the side to make sure we're not falling back on the internal fake device, for instance) ::: dom/media/tests/mochitest/test_getUserMedia_basicAudio_loopback.html:27 (Diff revision 3) > + // High energy on 1000 Hz low energy around that > + const freg_50Hz = array[analyser.binIndexForFrequency(50)]; > + const freq = array[analyser.binIndexForFrequency(TEST_AUDIO_FREQ)]; > + const freq_2000Hz = array[analyser.binIndexForFrequency(2000)]; > + > + info("Analysing audio frequency - lower:target:hi = " + freg_50Hz + ':' + freq + ':' + freq_2000Hz); We cap at 80 columns

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review202160 > Do you mean additional tests here? Will the test create their own output with loopback_tone=false and check that output through gUM + analyzer? Is it ok to create that on a follow up bug? Seems like I forgot to publish this, and now the answer was basically obsolete, but I think I answered this in my latest re-review of the test. So ask again if something is unclear.

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review208188 ::: dom/media/tests/mochitest/head.js:29 (Diff revision 3) > +class LoopbackTone { > + constructor() { > + var _audioContext = null; > + var _oscNode = null; > + > + this.skip = false; > + > + this.start = function(frequency) { > + if (this.skip == true) { > + return; > + } > + if (_audioContext == null) { > + dump(`Start loopback tone at ${frequency}`); > + _audioContext = new AudioContext(); > + _oscNode = _audioContext.createOscillator(); > + _oscNode.connect(_audioContext.destination); > + _oscNode.frequency.value = frequency; > + _oscNode.start(); > + } > + } > + > + this.stop = function() { > + if (_audioContext) { > + _oscNode.stop(); > + _audioContext = null; > + } > + } > + } > +}; I considered that approach but I believe this one is better. We do not create additional members when we do not need them. This is not e mechanism that will be used heavily, I expect a couple of calls at most and not on every test. We can create everything on start. Minor but it is also sematically better to pair the frequency argument with the start method. In that way you can also clean up on stop, otherwise you don't know when to clenup. ::: dom/media/tests/mochitest/head.js:31 (Diff revision 3) > + var _audioContext = null; > + var _oscNode = null; This is a way to make "private" members of an object in order to stay untouched from the user of the object. They are intentional like that. ::: dom/media/tests/mochitest/head.js:36 (Diff revision 3) > + var _audioContext = null; > + var _oscNode = null; > + > + this.skip = false; > + > + this.start = function(frequency) { I know how to define methods. This is intentional in order to have "private" members. ::: dom/media/tests/mochitest/head.js:40 (Diff revision 3) > + > + this.start = function(frequency) { > + if (this.skip == true) { > + return; > + } > + if (_audioContext == null) { Your pruposal is different than my intetion here. I want to create an AudioContext only when this is null. If the user needs a second tone can either stop this one and re-start using a new frequency or create a second instanse of that obejct. ::: dom/media/tests/mochitest/head.js:59 (Diff revision 3) > + } > + } > + } > +}; > +// The object that provide the tone the tone automatically by default. > +var DefaultLoopbackTone = new LoopbackTone; That's an object not a flag. Is there another object that it is ALL CAPS? I can create a global method to check if the _default_ tone is on. ::: dom/media/tests/mochitest/head.js:61 (Diff revision 3) > +function OptOutDefaultLoopbackTone() { > + DefaultLoopbackTone.skip = true; > +} I cab change it to SkipDefaultLoopbackTone() ... or ask jib for a better one :) ::: dom/media/tests/mochitest/head.js:298 (Diff revision 3) > element.setAttribute('id', id); > element.setAttribute('height', 100); > element.setAttribute('width', 150); > element.setAttribute('controls', 'controls'); > element.setAttribute('autoplay', 'autoplay'); > + element.muted = true; setAttribute() does not work! Unfortunately I can't really explain why. I used it on an early version of this patch but I got failures. Check here: https://jsfiddle.net/achronop/g7hbL23y/ ::: dom/media/tests/mochitest/head.js:351 (Diff revision 3) > + && constraints.audio > + && !DefaultLoopbackTone.skip) { > + // Loopback device is configured, start the default loopback tone > + DefaultLoopbackTone.start(TEST_AUDIO_FREQ) > + // Disable input audio processing to avoid distortion of the loopback tone > + constraints.audio = Object.assign({}, constraints.audio, {autoGainControl: false}, {echoCancellation: false}, {noiseSuppression: false}); Sematically it is better to bind them together since they are the two elements of the same piece. I can see a value break it if we want a way to activate them individually (maybe that's the case of an EC test). Is this wat you mean?

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review208134 > How about we make the constructor take/create all that is needed to create this tone, and start() just starts it? > > That seems more semantically correct. > > And we shouldn't be creating new AudioContexts all the time. > > So, let the constructor take an audio context and the frequency (or do you foresee this being reused?), then it creates and sets up the Oscillator. > > start() will then connect the oscillator to the destination and start() it. > > stop() will then disconnect and stop(). I considered that approach but I believe this one is better. We do not create additional members when we do not need them. This is not e mechanism that will be used heavily, I expect a couple of calls at most and not on every test. We can create everything on start. Minor but it is also sematically better to pair the frequency argument with the start method. In that way you can also clean up on stop, otherwise you don't know when to clenup. > these are members, so `this._audioContext = null;`, etc. This is a way to make "private" members of an object in order to stay untouched from the user of the object. They are intentional like that. > See how to define methods at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes I know how to define methods. This is intentional in order to have "private" members. > This type of check should be > ``` > if (!_audioContext) { > throw new Error("Wut, no AudioContext!?"); > } > ``` > > unless you want this to be possible: > ``` > tone.start(1000); > tone.start(2000); > ``` > > but what would the second call be expected to do? > and how should this behave? > ``` > tone.start(1000); > tone.start(2000); > tone.stop(); > ``` Your pruposal is different than my intetion here. I want to create an AudioContext only when this is null. If the user needs a second tone can either stop this one and re-start using a new frequency or create a second instanse of that obejct. > I think we should follow the precedence in this file for global state. I.e., make it an ALL_CAPS name and a primitive type. I guess you need to know globally if it's on or not too, so it'd make sense to flex the global state type rule for that. That's an object not a flag. Is there another object that it is ALL CAPS? I can create a global method to check if the _default_ tone is on. > I'm not sure of this terminology since it's negative (OptIn would be the positive version). > That's why I named it `request...()` in the pastebin I sent you. > > You might want to consult jib too, he's usually better than me at getting names right :-) I can change it to SkipDefaultLoopbackTone() ... or ask jib for a better one :) > Use `setAttribute` like the others. We want to set the html attribute, not the js one. The former becomes the default of the latter. setAttribute() does not work! Unfortunately I can't really explain why. I used it on an early version of this patch but I got failures. Check here: https://jsfiddle.net/achronop/g7hbL23y/ > You need to treat these individually, or what happens if the test is deliberately passing in one of them set to true? Sematically it is better to bind them together since they are the two elements of the same piece. I can see a value break it if we want a way to activate them individually (maybe that's the case of an EC test). Is this wat you mean?

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review208134 > I considered that approach but I believe this one is better. We do not create additional members when we do not need them. > > This is not e mechanism that will be used heavily, I expect a couple of calls at most and not on every test. We can create everything on start. Minor but it is also sematically better to pair the frequency argument with the start method. > > In that way you can also clean up on stop, otherwise you don't know when to clenup. I disagree. If you want two tones they should share AudioContext, period. Whether there is usually only one tone present or not is irrelevant. It's easy for a test that knows it will only have one tone to just pass `new AudioContext()`, but hard for one that needs two to make them share it. How is pairing frequency with start() more semantically correct? I'd argue for the opposite because it leaves holes in the API (per my other comment, [1]). Cleanup of an audio context is done by the garbage collector. Thus removing the reference holding this object does the same amount of cleanup as your stop(). [1] https://reviewboard.mozilla.org/r/196332/#comment263564 > This is a way to make "private" members of an object in order to stay untouched from the user of the object. They are intentional like that. Is there precedence in the code base for this pattern? I can't say I've seen it before. > That's an object not a flag. Is there another object that it is ALL CAPS? > > I can create a global method to check if the _default_ tone is on. That's true. It'd be great if we could avoid having an object as a global altogether. > setAttribute() does not work! Unfortunately I can't really explain why. I used it on an early version of this patch but I got failures. > > Check here: > https://jsfiddle.net/achronop/g7hbL23y/ You're right. Doing `setAttribute('muted', 'muted')` makes `element.defaultMuted` `true`, but doesn't touch `.muted`. Oddly the spec doesn't say when `muted` should be instantiated to `defaultMuted`. For completion though, we should set both. > Sematically it is better to bind them together since they are the two elements of the same piece. I can see a value break it if we want a way to activate them individually (maybe that's the case of an EC test). Is this wat you mean? Yeah, a test that does `getUserMedia({audio: {echoCancellation: true, noiseSuppression: "bananas"}})` should result in a call to `navigator.mediaDevices.getUserMedia({audio: {echoCancellation: true, noiseSuppression: "bananas", autoGainControl: false}})`. Or perhaps if we set all processing to off by default (by prefs) we don't have to do this at all?

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review208134 > You're right. Doing `setAttribute('muted', 'muted')` makes `element.defaultMuted` `true`, but doesn't touch `.muted`. Oddly the spec doesn't say when `muted` should be instantiated to `defaultMuted`. For completion though, we should set both. Demonstrated in https://jsfiddle.net/pehrsons/g7hbL23y/1/

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review208134 > I disagree. If you want two tones they should share AudioContext, period. Whether there is usually only one tone present or not is irrelevant. It's easy for a test that knows it will only have one tone to just pass `new AudioContext()`, but hard for one that needs two to make them share it. > > How is pairing frequency with start() more semantically correct? > I'd argue for the opposite because it leaves holes in the API (per my other comment, [1]). > > Cleanup of an audio context is done by the garbage collector. Thus removing the reference holding this object does the same amount of cleanup as your stop(). > > > [1] https://reviewboard.mozilla.org/r/196332/#comment263564 Why should they share AudioContext? Is that a real requirement or the way that you want it? I cannot be in your mind. All we want is a tone that will be detectable by the analyzer and we can have many AudioContext-s in a page. This way the user does not bother by the implementation details. Pairing with frequency is more intuitive because you bind it with the beginnign of the tone. If the object created earlier in the code you have the frequency at the point you use it. In addition, this way allow a flow like the following in case you need two tones: ``` lt.start(1000); lt.stop(); lt.start(2000); ``` That said I see value on your example in [1]. The second `start()` fails silently which is bad. Maybe we can throw an error then. I guess this is on implentation decision level. > Is there precedence in the code base for this pattern? I can't say I've seen it before. Is that a real argument why I shouldn't use that pattern? > That's true. It'd be great if we could avoid having an object as a global altogether. Well yeah, it would be better but if we want to be able to stop the tone is better to have an object. That's one reason I avoid to instantiate additional stuff on the constructor.

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review208134 > Why should they share AudioContext? Is that a real requirement or the way that you want it? I cannot be in your mind. All we want is a tone that will be detectable by the analyzer and we can have many AudioContext-s in a page. This way the user does not bother by the implementation details. > > Pairing with frequency is more intuitive because you bind it with the beginnign of the tone. If the object created earlier in the code you have the frequency at the point you use it. In addition, this way allow a flow like the following in case you need two tones: > > ``` > lt.start(1000); > lt.stop(); > lt.start(2000); > ``` > > That said I see value on your example in [1]. The second `start()` fails silently which is bad. Maybe we can throw an error then. I guess this is on implentation decision level. I don't know the inner parts of AudioContexts to answer detailed, but I have been pushed back on this before, as I recall the reasons were perf related. Let's ask Paul. > Is that a real argument why I shouldn't use that pattern? Yes. Readability, consistency. The same reason we have a coding style document.

Paul, could you shed some light on what the penalties would be to having one AudioContext per oscillator node when setting up loopback tests, if any? Or to phrase it differently, the reasons for AudioStreamAnalyser, [1], taking an AudioContext instead of creating one itself? Ref our discussion in [2]. [1] https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/dom/media/tests/mochitest/head.js#33 [2] https://reviewboard.mozilla.org/r/196332/#comment263550

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review210228 ::: dom/media/tests/mochitest/head.js:29 (Diff revision 3) > +class LoopbackTone { > + constructor() { > + var _audioContext = null; > + var _oscNode = null; > + > + this.skip = false; > + > + this.start = function(frequency) { > + if (this.skip == true) { > + return; > + } > + if (_audioContext == null) { > + dump(`Start loopback tone at ${frequency}`); > + _audioContext = new AudioContext(); > + _oscNode = _audioContext.createOscillator(); > + _oscNode.connect(_audioContext.destination); > + _oscNode.frequency.value = frequency; > + _oscNode.start(); > + } > + } > + > + this.stop = function() { > + if (_audioContext) { > + _oscNode.stop(); > + _audioContext = null; > + } > + } > + } > +}; Barring some domain-specific reason or evidence to the contrary, pairing the lifetime of the AudioContext with the lifetime of the helper class seems in keeping with the design of context objects in the first plac. ::: dom/media/tests/mochitest/head.js:31 (Diff revision 3) > + var _audioContext = null; > + var _oscNode = null; It's a real pattern for something the JavaScript language really doesn't support well [1][2]. That said, it has trade-offs (readability, prevents using modern JS) which must be weighed against the value of privacy enforcement, which I don't see the need for in a test. If there's no precedence in the mozilla codebase, then please make it consistent with the other code in this directory. FWIW PeerConnection.js uses this._privateMember (it relies on webidl for privacy from content JS). [1] http://2ality.com/2016/01/private-data-classes.html (mute noisy add) [2] https://stackoverflow.com/a/6472397/918910 ::: dom/media/tests/mochitest/head.js:40 (Diff revision 3) > + > + this.start = function(frequency) { > + if (this.skip == true) { > + return; > + } > + if (_audioContext == null) { Code style [1] says: "Do not compare x == true or x == false. Use (x) or (!x) instead." [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_practices ::: dom/media/tests/mochitest/head.js:351 (Diff revision 3) > + && constraints.audio > + && !DefaultLoopbackTone.skip) { > + // Loopback device is configured, start the default loopback tone > + DefaultLoopbackTone.start(TEST_AUDIO_FREQ) > + // Disable input audio processing to avoid distortion of the loopback tone > + constraints.audio = Object.assign({}, constraints.audio, {autoGainControl: false}, {echoCancellation: false}, {noiseSuppression: false}); I don't know about individually, but in order to act as defaults, I believe the arguments to Object.assign() need to be applied in the opposite order, e.g.: Object.assign({ autoGainControl: false, echoCancellation: false, noiseSuppression: false }, constraints.audio);

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review208134 > Your pruposal is different than my intetion here. I want to create an AudioContext only when this is null. > > If the user needs a second tone can either stop this one and re-start using a new frequency or create a second instanse of that obejct. An OscillatorNode can update its frequency on the fly. The second start can do that. It looks convenient because we do not have to stop and restart anything. Let me know what you think. > I can change it to SkipDefaultLoopbackTone() ... or ask jib for a better one :) I changed a little and make use of global flag. > This should mention the purpose of this test. I can see what you're testing but I'm not sure it's all we want to test, so having this description to compare against would be useful. > > (I could see value in making this use both the default path and add an explicit frequency on the side to make sure we're not falling back on the internal fake device, for instance) Added more test, let me know if this is what you mean.

(In reply to Andreas Pehrson [:pehrsons] from comment #49) > Paul, could you shed some light on what the penalties would be to having one > AudioContext per oscillator node when setting up loopback tests, if any? > > Or to phrase it differently, the reasons for AudioStreamAnalyser, [1], > taking an AudioContext instead of creating one itself? It does not matter much. Maybe having a single one is slightly faster and uses slightly less memory. If it matters there is another, more fundamental problem.

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review214860 Sorry for the long delay. Looks good! Almost there. ::: dom/media/tests/mochitest/head.js:43 (Diff revision 5) > + if (DISABLE_LOOPBACK_TONE) { > + return; > + } The flag is good, but I'd like it to only affect the default case. A manual call from a test to set up a tone should still work IMO. Looks like you have the default case covered, so it should be all right to just remove this check. ::: dom/media/tests/mochitest/head.js:70 (Diff revision 5) > + > + nit: double-linebreak ::: dom/media/tests/mochitest/head.js:357 (Diff revision 5) > + // Loopback device is configured, start the default loopback tone > + if (!DefaultLoopbackTone) { > + DefaultLoopbackTone = new LoopbackTone(new AudioContext); > + DefaultLoopbackTone.start(TEST_AUDIO_FREQ); > + } > + // Disable input audio processing to avoid distortion of the loopback tone nit: could you mention that it will only disable the processing modes that were not explicitly enabled? ::: dom/media/tests/mochitest/test_getUserMedia_basicAudio_loopback.html:61 (Diff revision 5) > + info("Disable loopback tone"); > + DISABLE_LOOPBACK_TONE = true; > + DefaultLoopbackTone.start(TEST_AUDIO_FREQ); > + await analyser.waitForAnalysisSuccess( array => { > + const freg_50Hz = array[analyser.binIndexForFrequency(50)]; > + const freq = array[analyser.binIndexForFrequency(TEST_AUDIO_FREQ)]; > + const freq_2000Hz = array[analyser.binIndexForFrequency(2000)]; > + > + info("Analysing audio frequency - lower:target:high = " > + + freg_50Hz + ':' + freq + ':' + freq_2000Hz); > + return freg_50Hz < 50 && freq < 50 && freq_2000Hz < 50; > + }) Per my other comment on the DISABLE var this would go away. If you can instead add a case where we have two tones at the same time at different frequencies we'll be golden.

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review202262 > Actually this is not my intention here. I want to leave `setup_loopback_capture_tone()` method available as an easy way for the user to creatr one or more loopback tones. I do want to allow user calling it more than once and I do not want to reset TEST_AUDIO_FREQ on any additional call of the method. We could identify the case from `meta.loopback_explicit`. I think it could still be good if the default tone setup by head.js sets up TEST_AUDIO_FREQ when it gets initiated. So that if the test sets up everything manually it would be something invalid.

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review208134 > I don't know the inner parts of AudioContexts to answer detailed, but I have been pushed back on this before, as I recall the reasons were perf related. Let's ask Paul. All right, so this issue would be ok to drop. > An OscillatorNode can update its frequency on the fly. The second start can do that. It looks convenient because we do not have to stop and restart anything. Let me know what you think. I agree it's convenient. But it seems like unexpected semantics to me ot update the frequency in start(). Keep start() clean and let it call through to oscillator.start(). Then expose the frequency on the side. Getter/setter perhaps? > Added more test, let me know if this is what you mean. I added a new issue for the latest test code so I'll drop this one. It wouldn't hurt to be more detailed of the test's intentions in the comment, but I won't r- you for it.

Comment on attachment 8925099 [details] Bug 1406350 - part2: Create new gUM basic audio test using loopback setup. https://reviewboard.mozilla.org/r/196332/#review215864 Thanks! The only concern left I could think of was who cleans up the default tone. My take on it is that each test runs in a separate document and when finishing a test, its document is unloaded, which according to spec shuts down the AudioContext. ::: dom/media/tests/mochitest/head.js:48 (Diff revisions 5 - 6) > // Method should be used when FAKE_ENABLED is false. > - start(frequency) { > - if (DISABLE_LOOPBACK_TONE) { > - return; > + start() { > + if (!this.oscNode) { > + throw new Error("Attempt to start a stopped LoopbackTone"); > } > - if (!this.audioContext) { > + dump(`Start loopback tone at ${this.oscNode.frequency.value}\n`); Use `info()` instead of `dump()` and remove the newline. ::: dom/media/tests/mochitest/head.js:58 (Diff revisions 5 - 6) > + if (frequency && frequency <= 0) { > + throw new Error("Frequency must must be a positive number"); > } Do we need this check? The oscillator node should handle range checking already. ::: dom/media/tests/mochitest/test_getUserMedia_basicAudio_loopback.html:59 (Diff revisions 5 - 6) > > - info("Stop loopback tone"); > - DefaultLoopbackTone.stop(); > - await analyser.waitForAnalysisSuccess( array => { > - const freg_50Hz = array[analyser.binIndexForFrequency(50)]; > - const freq = array[analyser.binIndexForFrequency(800)]; > + // Create a second tone at a different frequency. > + // Verify that both tones are detected. > + info("Multiple loopback tones"); > + DefaultLoopbackTone.changeFrequency(TEST_AUDIO_FREQ); > + var second_tone = new LoopbackTone(audioContext, 2000); s/var/let/

I am working 2 try issues after latest rebase. First issue: https://treeherder.mozilla.org/logviewer.html#?job_id=154143962&repo=try&lineNumber=2499 Two tones test fails because the side frequencies (50Hz and 4000Hz) have volume greater than 50. In general the spectrum graph is not very clean. There is energy in form of spikes throughout the spectrum range. This must be due to implementation limitations. The easy solution is to expect energy for the side frequencies lower than 100 instead of 50. Also I experiment if there are combinations of target frequencies that do not produce harmonics throughout the spectrum range e.g. one frequency exact multiple of the other. Second issue: https://treeherder.mozilla.org/logviewer.html#?job_id=154419774&repo=try&lineNumber=43756 It's still under investigation. I believe that the default loopback tone does not start at all. I pushed a new run adding debug logs.

Hmm, for the first one the screenshot looks *very* bad: https://public-artifacts.taskcluster.net/QJWEMR0PTRyWjzkoUomHoA/0/public/test_info//mozilla-test-fail-screenshot_7hQuuY.png Must be more than implementation limitations. What about Processing? Underruns? Clipping?

Are we sure we're disabling the AEC and other processing stuff here ?

Yeah it's bad. Processing is disabled. To elaborate more about how it works, we do a gUM call at the beginning of the test with processing disabled. After that we change the output tone, only, and we check the input stream. The output tone passes through loopback devices in the input and gives us the results. If processing was enabled all checks before that would have failed. Most of the loopback happens in PA code which is out of our control.

A quick update on the try issues. The first one solved by lowering the volume of the loopback tone. The second issue happens because getUserMedia request hangs. It fails silently at the following check: https://searchfox.org/mozilla-central/source/dom/media/MediaManager.cpp#2805

More details about the second issue: Repeat here that getUserMedia request fails silently in the following check https://searchfox.org/mozilla-central/source/dom/media/MediaManager.cpp#2805 This is because `MediaManager::EnumerateRawDevices()` method takes a lot of time to be completed, more precisely to resolve the pledge that it returns. At the time of resolve (~5 mins) the current test has already failed due to timeout and check above is failing due to invalid window id. Responsible for the delay is the `cubeb_enumerate_device()` method in `AudioInputCubeb::UpdateDeviceList()`. The following push log contains extra log-messages before and after that method: https://public-artifacts.taskcluster.net/ZOrChPDETla2gv0ic-0TTQ/0/public/logs/live_backing.log Logs show that "before" happens at 2018-01-15T13:20:01.270Z and "after" takes place at 2018-01-15T13:25:12.374Z. I tried with cubeb sandbox disabled but I still get the issue.

I ping 2 more people to check this because I am not able to find something by myself. The error appears in Linux back end, both native and rust, when patches are applied. More info for the problem in the comments above. You can see the error report on the link: https://treeherder.mozilla.org/#/jobs?repo=try&author=achronop@gmail.com&selectedJob=156566878 Added extra debug logs in enumerate device method: https://hg.mozilla.org/try/rev/ede7cd196d648c9f9548c805e02a793e34dc029b Looking at the following log file: https://public-artifacts.taskcluster.net/eIj916cnTHKIuHnhgnZAww/0/public/logs/live_backing.log The events, at time order - Test started: [task 2018-01-16T12:39:20.215Z] 12:39:20 INFO - TEST-START | dom/media/tests/mochitest/test_peerConnection_replaceTrack.html - gUM with audio: true: [task 2018-01-16T12:39:22.535Z] 12:39:22 INFO - GECKO(3150) | alex - enumerate device enter [task 2018-01-16T12:39:22.537Z] 12:39:22 INFO - GECKO(3150) | alex - enumerate device: after lock loop [task 2018-01-16T12:39:22.538Z] 12:39:22 INFO - GECKO(3150) | alex - enumerate devices: after default device [task 2018-01-16T12:39:22.539Z] 12:39:22 INFO - GECKO(3150) | alex - enumerate devices: after input devices - later another gUM with audio:true [task 2018-01-16T12:39:29.529Z] 12:39:29 INFO - GECKO(3150) | alex - enumerate device enter - enumerate device method blocked at this point, test failed due to timeout after ~5 mins [task 2018-01-16T12:44:39.490Z] 12:44:39 INFO - TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_replaceTrack.html | Test timed out. - enumerate device method continues after the fail. [task 2018-01-16T12:44:45.186Z] 12:44:45 INFO - GECKO(3150) | alex - enumerate device: after lock loop On other gUM requests the enumerate device method is executed instantly. For this one the execution in blocked when we try to lock the mainloop of PA. It never fails locally and I am not able to repro in a loaner too. Any idea would be helpful.

That seems to imply something else is holding the mainloop lock for long durations. Can you reproduce it inside a loaner and use gdb to backtrace the other threads? That might be sufficient to find the culprit. If you can't repro in a loaner, logging the thread id and location of every point the lock is taken might narrow things down.

If you look at thread 11 in the log, cubeb is waiting on the drain timer to fire at (https://hg.mozilla.org/try/file/ede7cd196d648c9f9548c805e02a793e34dc029b/media/libcubeb/cubeb-pulse-rs/src/backend/stream.rs#l377)

This issue looks unrelated to the intention of this bug. Probably is the result of timing between threads. In order to unblock landing of loopback devices I will use fake constraints to make the test green and I will raise a follow up issue for that.

Comment on attachment 8943227 [details] Bug 1406350 - part3: Use fake constraints to avoid enumeration error in one mochitest. https://reviewboard.mozilla.org/r/213560/#review219290 ::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:42 (Diff revision 1) > ok(allRemoteStreamsHaveReceiver(pc), > "Shouldn't have any remote streams without a corresponding receiver"); > > var newTrack; > var audiotrack; > - return getUserMedia({video:true, audio:true}) > + return getUserMedia({fake:true, video:true, audio:true}) Please add a comment saying to remove `fake:true` with bug 1431056. ::: dom/media/tests/mochitest/test_peerConnection_replaceTrack.html:72 (Diff revision 1) > } > > runNetworkTest(function () { > test = new PeerConnectionTest(); > test.audioCtx = new AudioContext(); > - test.setMediaConstraints([{video: true, audio: true}], [{video: true}]); > + test.setMediaConstraints([{fake: true, video: true, audio: true}], [{fake: true, video: true}]); Please add a comment saying to remove both `fake:true` with bug 1431056.

Pushed by achronop@gmail.com: https://hg.mozilla.org/integration/autoland/rev/629c42087f29 part1: Update runtest.py to new loopback setup. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/85ec8f3d3fb2 part2: Create new gUM basic audio test using loopback setup. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/1a202fc6df1d part3: Use fake constraints to avoid enumeration error in one mochitest. r=pehrsons

https://hg.mozilla.org/mozilla-central/rev/629c42087f29 https://hg.mozilla.org/mozilla-central/rev/85ec8f3d3fb2 https://hg.mozilla.org/mozilla-central/rev/1a202fc6df1d