Open Bug 1406910 Opened 7 years ago Updated 1 year ago

Add a mochitest for input-only audio capture

Categories

(Core :: Audio/Video: MediaStreamGraph, enhancement, P2)

58 Branch
enhancement

Tracking

()

ASSIGNED

People

(Reporter: pehrsons, Assigned: pehrsons)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

This is a followup to bug 1406027 to land the test there once we have a virtual input device on linux with frequent enough callbacks. See bug 1406027 comment 15.
Rank: 19
Priority: P3 → P2
It works! Except for Android simulator debug where we're still timing out. I'll see if I can get it reliable with looping playback of the recording, or whether we should disable it.
Comment on attachment 8918286 [details]
Bug 1406910 - Break out getSineWaveFile to head.js.

https://reviewboard.mozilla.org/r/189118/#review197868

lgtm with some comments.

::: dom/media/tests/mochitest/head.js:36
(Diff revision 1)
> +
> +  osc.frequency.value = frequency;
> +  osc.start();
> +  rec.start();
> +
> +  off.startRendering().then(() => rec.stop());

getSineWaveFile returns before rec.stop() is called here. Intentional?

Any reason not to await here until the recording part is done?

::: dom/media/tests/mochitest/head.js:38
(Diff revision 1)
> +  let {data:blob} = await haveEvent(rec, "dataavailable");
> +  return blob;

Nit: space after :

or

    let {data} = await haveEvent(rec, "dataavailable");
    return data;

or

    return (await haveEvent(rec, "dataavailable")).data;

::: dom/media/tests/mochitest/test_getUserMedia_audioCapture.html:60
(Diff revision 1)
> -    getSineWaveFile(10000, 10, function(blob) {
> +    getSineWaveFile(10000, 10).then(blob => {
>        wavtone.src = URL.createObjectURL(blob);
>        oscThroughMediaElement.start();
>        oscThroughAudioDestinationNode.start();
>        wavtone.loop = true;
>        wavtone.play();
>        acTone.play();
>      });
>  
>      var constraints = {audio: {mediaSource: "audioCapture"}};
>  
>      return getUserMedia(constraints).then((stream) => {

Are we concerned that getSineWave() and gUM are racing here?

Should we use async/await to join error handling?

If not, please add a .catch(e => ok(false)) at least to the former's chain.
Attachment #8918286 - Flags: review?(jib) → review+
Comment on attachment 8916591 [details]
Bug 1406910 - Add mochitest for audio gUM and no output.

https://reviewboard.mozilla.org/r/187718/#review197860

Lgtm. Just some questions and ideas.

::: dom/media/tests/mochitest/test_getUserMedia_audio_inputOnly.html:11
(Diff revision 3)
> +<body>
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    title: "getUserMedia Audio Test with no audio output present",
> +    bug: "1406027",

bug 1406910?

::: dom/media/tests/mochitest/test_getUserMedia_audio_inputOnly.html:18
(Diff revision 3)
> +    // We want to test audio capture by a MediaStreamGraph that has no audio
> +    // outputs. Playing a file uses a separate path for output so this is ok
> +    // to feed the MSG input track through our loopback device.

Might this accidentally change in the future? Is there a way to self-test this assumption here?

::: dom/media/tests/mochitest/test_getUserMedia_audio_inputOnly.html:26
(Diff revision 3)
> +    let stream = await getUserMedia({audio: true});
> +    let recorder = new MediaRecorder(stream);
> +    recorder.start();
> +
> +    // We record 5 seconds of audio for the analysis pass later.
> +    await wait(5000);

I worry the gUM setup time might eat into the 5 seconds here on slow test machines.

Could we add the stream to a video element and wait for loadedmetadata before counting the 5 seconds, to prevent this?

::: dom/media/tests/mochitest/test_getUserMedia_audio_inputOnly.html:27
(Diff revision 3)
> +    let recorder = new MediaRecorder(stream);
> +    recorder.start();
> +
> +    // We record 5 seconds of audio for the analysis pass later.
> +    await wait(5000);
> +
> +    let dataavailable = haveEvent(recorder, "dataavailable");
> +    recorder.stop();
> +    let {data: blob} = await dataavailable;

I like this tight use of MediaRecorder, and maybe error-checking isn't crucial here, but would it simplify to put this in a `record(stream, ms)` sub-function for re-use, something like in https://jsfiddle.net/jib1/nrrcrewf/ ?

::: dom/media/tests/mochitest/test_getUserMedia_audio_inputOnly.html:33
(Diff revision 3)
> +    let dataavailable = haveEvent(recorder, "dataavailable");
> +    recorder.stop();
> +    let {data: blob} = await dataavailable;

FWIW should work to do:

    recorder.stop();
    let {data: blob} = await haveEvent(recorder, "dataavailable");

::: dom/media/tests/mochitest/test_getUserMedia_audio_inputOnly.html:62
(Diff revision 3)
> +            if (array[analyser.binIndexForFrequency(frequency / 2)] < 50 &&
> +                array[analyser.binIndexForFrequency(frequency)] > 200 &&
> +                array[analyser.binIndexForFrequency(frequency * 1.5)] < 50) {
> +              // Audio input should be smooth enough to pass the check 10
> +              // iterations in a row
> +              return (++count) == 10;

Is this comparable to calling waitForAnalysisSuccess() 10 times in a for-loop?

::: dom/media/tests/mochitest/test_getUserMedia_audio_inputOnly.html:69
(Diff revision 3)
> +    } catch (e) {
> +      ok(false, `${e.name}, ${e.message}`);
> +    }

Doesn't runtest() try-catch?

::: dom/media/tests/mochitest/test_getUserMedia_audio_inputOnly.html:73
(Diff revision 3)
> +      ok(true, "Analysis OK");
> +    } catch (e) {
> +      ok(false, `${e.name}, ${e.message}`);
> +    }
> +
> +    analyser.disableDebugCanvas();

cruft?
Attachment #8916591 - Flags: review?(jib) → review+
Comment on attachment 8918286 [details]
Bug 1406910 - Break out getSineWaveFile to head.js.

https://reviewboard.mozilla.org/r/189118/#review197868

> getSineWaveFile returns before rec.stop() is called here. Intentional?
> 
> Any reason not to await here until the recording part is done?

"dataavailable" below will only happen after stop() since we didn't pass a timeslice to start(). That basically waits until the recording is done.

> Are we concerned that getSineWave() and gUM are racing here?
> 
> Should we use async/await to join error handling?
> 
> If not, please add a .catch(e => ok(false)) at least to the former's chain.

The race doesn't matter much since the test can only pass after this is setup. It could affect determinism if one of the paths [getSineWave-then-gUM, gUM-then-getSineWave] were broken. Neither this or rewriting to async/await is within scope of this bug however.

I'll add a catch to get better visibility than a test timeout if `getSineWaveFile()` fails.
Comment on attachment 8916591 [details]
Bug 1406910 - Add mochitest for audio gUM and no output.

https://reviewboard.mozilla.org/r/187718/#review197860

> bug 1406910?

For the test fix yeah, but we're really testing behavior per bug 1406027.

> Might this accidentally change in the future? Is there a way to self-test this assumption here?

It certainly could, but I'm not aware of a way to self-test this other than exposing a chrome-only api specifically for this purpose.

> I worry the gUM setup time might eat into the 5 seconds here on slow test machines.
> 
> Could we add the stream to a video element and wait for loadedmetadata before counting the 5 seconds, to prevent this?

HTMLMediaElement doesn't wait for actual audio data before "loadedmetadata" (or "loadeddata" for that matter). Only for video. I don't think any API waits for proper audio samples to come through before an event. MediaRecorder used to wait for channels to be known, but that is limited to 1 second now, so it can't generally be relied on.

gUM setup time should be covered by the gUM promise anyway, no?

> I like this tight use of MediaRecorder, and maybe error-checking isn't crucial here, but would it simplify to put this in a `record(stream, ms)` sub-function for re-use, something like in https://jsfiddle.net/jib1/nrrcrewf/ ?

I put in a `record` but instead of milliseconds it takes a Promise. The drawback is that with a timeout we start counting when creating the recorder, not when it has reported "start", but the advantage is that we can base it off any events we like.

> FWIW should work to do:
> 
>     recorder.stop();
>     let {data: blob} = await haveEvent(recorder, "dataavailable");

True. I think I had a more generic thought that it's possible that it stops before stop() if the tracks end. That would in any case cause stop() to throw...
It is broken out into a generic function (and rewritten) now anyways.

> Is this comparable to calling waitForAnalysisSuccess() 10 times in a for-loop?

No. `waitForAnalysisSuccess` will respond to failures by just waiting longer. I.e., it would validate the sequence [t,t,f,t,t,f,t,t,f,t,t,f,t,t] which was exactly the kind of thing I wanted to keep out (because we're highly dependant on the length of the audio callback buffer).
Comment on attachment 8916591 [details]
Bug 1406910 - Add mochitest for audio gUM and no output.

https://reviewboard.mozilla.org/r/187718/#review197860

> cruft?

Actually no, I think this is good to keep. It stops the requestAnimationFrame loop drawing into the debug canvas.
Comment on attachment 8916591 [details]
Bug 1406910 - Add mochitest for audio gUM and no output.

https://reviewboard.mozilla.org/r/187718/#review197860

> HTMLMediaElement doesn't wait for actual audio data before "loadedmetadata" (or "loadeddata" for that matter). Only for video. I don't think any API waits for proper audio samples to come through before an event. MediaRecorder used to wait for channels to be known, but that is limited to 1 second now, so it can't generally be relied on.
> 
> gUM setup time should be covered by the gUM promise anyway, no?

Ok, I've verified we get a 5 second audio recording regardless of this event here https://jsfiddle.net/jib1/sL8rvb9L/

The same is NOT true for video! Change audio to video in that fiddle and you get a smaller 3 second recording.

From an API perspective this is perhaps concerning, but for this test it's fine. Worth a comment maybe.

> gUM setup time should be covered by the gUM promise anyway, no?

Depends on what we mean. If it means my workaround above for video should not be necessary, then no. Should gUM wait for first frame?

> Actually no, I think this is good to keep. It stops the requestAnimationFrame loop drawing into the debug canvas.

What requestAnimationFrame?
Comment on attachment 8916591 [details]
Bug 1406910 - Add mochitest for audio gUM and no output.

https://reviewboard.mozilla.org/r/187718/#review197860

> Ok, I've verified we get a 5 second audio recording regardless of this event here https://jsfiddle.net/jib1/sL8rvb9L/
> 
> The same is NOT true for video! Change audio to video in that fiddle and you get a smaller 3 second recording.
> 
> From an API perspective this is perhaps concerning, but for this test it's fine. Worth a comment maybe.
> 
> > gUM setup time should be covered by the gUM promise anyway, no?
> 
> Depends on what we mean. If it means my workaround above for video should not be necessary, then no. Should gUM wait for first frame?

What about audio+video? That should become 5 seconds too, with a 2 second gap until the first frame. For video only I think the MediaRecorder ignores the time when the input was null.

Or perhaps it's playback that ignores the time before the first track to start starts.

> What requestAnimationFrame?

The one driving the debug canvas.
Status: NEW → ASSIGNED
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.