Closed Bug 1420322 Opened 2 years ago Closed 2 years ago

Test for audio seamless looping

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: chunmin, Assigned: padenot)

Details

Attachments

(3 files, 8 obsolete files)

We need a test for audio seamless looping(bug 654787).
Assignee: nobody → cchang
Attached patch [WIP] Draft (obsolete) — Splinter Review
mochitest is probably a wrong choice. I tried to calculate how much time we need for seamlessly looping an audio n times by `performance.now()`, but the time varied. It may lead to an intermittent fail. Perhaps we could create an test-only idl and insert it into `AudioStream::Callback` to check it's called within 10~12ms every time.
Attached patch [WIP] Test for seamless looping (obsolete) — Splinter Review
Hi Paul,
Does WebAudio has API that can check if there is a gap in the audio output data? 

I tried using `getByteTimeDomainData()`[0] but it doesn't work. The dumped data is same no matter seamless looping is on or off. (I guess the data is captured from audio data callback so it won't append zero into buffer when the audio element stop playing.) Do we have API that can help us to check if there is a silence while looping? 

[0] https://developer.mozilla.org/en-US/docs/Web/API/AnalyserNode/getByteTimeDomainData
Attachment #8931562 - Attachment is obsolete: true
Flags: needinfo?(padenot)
Use a ScriptProcessorNode, that lets you get raw buffer of audio.

AnalyzerNode is not good enough because it applies a Blackman window, so everything is smoothed out.
Flags: needinfo?(padenot)
Comment on attachment 8934087 [details]
Bug 1420322 - Test for seamless looping;

https://reviewboard.mozilla.org/r/205036/#review212350

::: dom/media/test/mochitest.ini:554
(Diff revision 2)
>    seek_support.js
>    seekLies.sjs
>    seek_with_sound.ogg^headers^
>    sequential.vtt
>    short-cenc.mp4
> +  sine-440-500ms.wav

This should be generated at run time. Why is it a wav file?

::: dom/media/test/test_seamless_loop.html:51
(Diff revision 2)
> +// that follows FIFO policy. When the window is full, the oldest data will be
> +// popped before the new data is pushed. We use a sequence number to track how
> +// many input data is same as 0. If the data is same as 0, then the sequence
> +// number is accumulated. Otherwise, it's reset to 0. If the sequence number is
> +// equal or greater than the window's size, then the data in the window is all
> +// same as 0.

Can't we simply record the audio that we get in the ScriptProcessorNode, and then compare it to a sine wave computed out-of-band, and check if there are gaps?

::: dom/media/test/test_seamless_loop.html:118
(Diff revision 2)
> +// ============================================================================
> +// Check if the test fails by disabling seamless-looping.
> +// SpecialPowers.setBoolPref("media.seamless-looping", false);
> +
> +// The looping time for the test.
> +const TIMES = 1;

Are we sure this is enough?

::: dom/media/test/test_seamless_loop.html:125
(Diff revision 2)
> +const BUF_SIZE = 4096;
> +// The exclusive upper bound of the total number of a series of 0s in the
> +// audio data. That is, it's ok to have a 1023 successive 0s in one audio data,
> +// but it will be considered as a silence gap once the number of successive 0s
> +// is larger or equal than 1024.
> +const ZERO_TOLERANCE = 1024;

How have we decided that this is an acceptable value?

More broadly, I'm having a hard time reconciling the concept of "seamless" and the concept of "tolerance". Looping is either seamless or not seamless.

Is our implementation not able to do true seamless looping?
Attachment #8934087 - Flags: review?(padenot) → review-
(In reply to Paul Adenot (:padenot) from comment #6)
> Comment on attachment 8934087 [details]
> Bug 1420322 - Test for seamless looping;
> 
> https://reviewboard.mozilla.org/r/205036/#review212350
> 
> ::: dom/media/test/mochitest.ini:554
> (Diff revision 2)
> >    seek_support.js
> >    seekLies.sjs
> >    seek_with_sound.ogg^headers^
> >    sequential.vtt
> >    short-cenc.mp4
> > +  sine-440-500ms.wav
> 
> This should be generated at run time. Why is it a wav file?
Is there any API can allow us to push the generated audio data into the audio element's buffer? The seamless looping is based on the audio element's loop attribute[0], so the wav file is used to be the source of audio element. If we can use the computed sine wave data as the audio element's source, then we can get rid of the wav file.

[0] https://searchfox.org/mozilla-central/rev/9d920555ec81f1c9d4b7fa3b08e23eb88efb60e1/dom/media/MediaDecoderStateMachine.cpp#3674
(In reply to Chun-Min Chang[:chunmin] from comment #7)
Thanks for Paul's advice: We can use an OscillatorNode to generate sine waves and feed them into MediaRecorder, then pass the data to the source of audio element. To analyze whether the audio is seamless, we can convert the data from time-domain to frequency-domain by FFT to see the values on frequency-axis. If it's seamless, then we will have *only one* frequency value in that axis. If not, then we will have other values beside the frequency of the sine wave.

Examples:
[0] https://searchfox.org/mozilla-central/rev/110706c3c09d457dc70293b213d7bccb4f6f5643/dom/media/tests/mochitest/test_getUserMedia_audioCapture.html#59-81
[1] https://developer.mozilla.org/en-US/docs/Web/API/MediaStream_Recording_API/Using_the_MediaStream_Recording_API
Attached patch [WIP] Test for seamless looping (obsolete) — Splinter Review
Try to implement Comment 8.
Attachment #8933175 - Attachment is obsolete: true
Attached patch [WIP] Test for seamless looping (obsolete) — Splinter Review
Switch to an easier implementation:
1) Get the decoded data from a .wav file
2) Loop the audio element whose source is the .wav file
3) Use ScriptProcesserNode to get the buffer data of the the audio element
4) Compare the buffer data captured by ScriptProcesserNode and the original decoded data of the .wav file to check if the second play matches perfectly
Attachment #8934087 - Attachment is obsolete: true
Attachment #8934087 - Flags: review?(jwwang)
Attached patch [WIP] Test for seamless looping (obsolete) — Splinter Review
Attachment #8940641 - Attachment is obsolete: true
Attachment #8940994 - Attachment is obsolete: true
(In reply to Chun-Min Chang[:chunmin] from comment #10)
> Created attachment 8940994 [details] [diff] [review]
> [WIP] Test for seamless looping
> 
> Switch to an easier implementation:
> 1) Get the decoded data from a .wav file
> 2) Loop the audio element whose source is the .wav file
> 3) Use ScriptProcesserNode to get the buffer data of the the audio element
> 4) Compare the buffer data captured by ScriptProcesserNode and the original
> decoded data of the .wav file to check if the second play matches perfectly

This is not reliable. There are no guarantees that the ScriptProcessorNode will see all the samples, and this will cause intermittent failures.
Comment on attachment 8934087 [details]
Bug 1420322 - Test for seamless looping;

https://reviewboard.mozilla.org/r/205036/#review217408

::: dom/media/test/test_seamless_loop.html:5
(Diff revision 3)
> +<!DOCTYPE HTML>
> +<html>
> +<meta charset="utf-8">
> +<head>
> +  <title>Test for seamlee looping</title>

seamless.

::: dom/media/test/test_seamless_loop.html:40
(Diff revision 3)
> +
> +var seamless = true;    // Set to false if the test fails in seamless check.
> +var sourceBuffer;       // The decoded audio buffer used to compare with the
> +                        // captured buffer data.
> +
> +var audioElement = new Audio();

Use document.createElement("audio") instead.

::: dom/media/test/test_seamless_loop.html:121
(Diff revision 3)
> +function eventCounter(evt) {
> +  ++eventCount[evt.type];
> +  info(evt.type + ": " + eventCount[evt.type]);
> +}
> +
> +// Event callback for timeupdate.

It would be easier to check how many 'seeked' are received.

::: dom/media/test/test_seamless_loop.html:166
(Diff revision 3)
> +  for (evt in listeners) {
> +    audio.addEventListener(evt, listeners[evt], false);
> +  }
> +}
> +
> +setupEnvironment()

Use async/await to make the syntax compact:

await setupEnvironment();
let buffer = await decodeAudioData();
(In reply to Paul Adenot (:padenot) from comment #12)
> (In reply to Chun-Min Chang[:chunmin] from comment #10)
> > Created attachment 8940994 [details] [diff] [review]
> > [WIP] Test for seamless looping
> > 
> > Switch to an easier implementation:
> > 1) Get the decoded data from a .wav file
> > 2) Loop the audio element whose source is the .wav file
> > 3) Use ScriptProcesserNode to get the buffer data of the the audio element
> > 4) Compare the buffer data captured by ScriptProcesserNode and the original
> > decoded data of the .wav file to check if the second play matches perfectly
> 
> This is not reliable. There are no guarantees that the ScriptProcessorNode
> will see all the samples, and this will cause intermittent failures.
I switch the method since I see some noise around the frequency of the sine wave when I use an OscillatorNode to connect an AnalyserNode and then show its real-time frequency-domain data. Does our FFT work perfectly?

demo here: http://chunminchang.github.io/playground/webaudio/FFT.html
(In reply to Paul Adenot (:padenot) from comment #12)
> This is not reliable. There are no guarantees that the ScriptProcessorNode
> will see all the samples, and this will cause intermittent failures.
When the samples will be ignored by ScriptProcessorNode?
I noticed that the data captured by ScriptProcessorNode at the beginning are all zero, but it's fine in this test. We start comparing and checking the samples after the first-round play ends so it's ok if ScriptProcessorNode ignores some samples when the audio just starts playing. We only care if there is a delay between the first-round play and the second-round play.
Flags: needinfo?(padenot)
(In reply to Chun-Min Chang[:chunmin] from comment #16)
> (In reply to Paul Adenot (:padenot) from comment #12)
> > This is not reliable. There are no guarantees that the ScriptProcessorNode
> > will see all the samples, and this will cause intermittent failures.
> When the samples will be ignored by ScriptProcessorNode?

https://searchfox.org/mozilla-central/source/dom/media/webaudio/ScriptProcessorNode.cpp#133
Flags: needinfo?(padenot)
Attached file FFT.html (obsolete) —
A test page to check WebAudio FFT.
(In reply to Paul Adenot (:padenot) from comment #18)
To avoid using ScriptProcessorNode, we need to change the solution again.

Although the FFT is not *perfect*, it seems like the FFT result of playing a sine is stable(after play about 1sec) if frequency of the sine wave is higher than 2000hz(check it on attachment 8942111 [details]). we could use this fact to to write a test. For a seamless-looping sine wave, the wave should be connected perfectly. As a result, its FFT result should be the same no matter how many times it loops. If there is a discontinuity in the loop, then the FFT result will change.

- *perfect* means the Fourier transform works perfectly. For 440hz sine, the frequency-domain data should be all zero except 440Hz. However, we have a few noise around 440hz. See attachment 8941355 [details] or check it in attachment 8942111 [details].
(In reply to Chun-Min Chang[:chunmin] from comment #15)
> I switch the method since I see some noise around the frequency of the sine
> wave when I use an OscillatorNode to connect an AnalyserNode and then show
> its real-time frequency-domain data. Does our FFT work perfectly?
> 
> demo here: http://chunminchang.github.io/playground/webaudio/FFT.html

Yes, this is the expected result of a discrete Fourier transforms. The "noise" you see is very very low energy.
(In reply to Chun-Min Chang[:chunmin] from comment #20)
> (In reply to Paul Adenot (:padenot) from comment #18)
> To avoid using ScriptProcessorNode, we need to change the solution again.
> 
> Although the FFT is not *perfect*, it seems like the FFT result of playing a
> sine is stable(after play about 1sec) if frequency of the sine wave is
> higher than 2000hz(check it on attachment 8942111 [details]). we could use
> this fact to to write a test. For a seamless-looping sine wave, the wave
> should be connected perfectly. As a result, its FFT result should be the
> same no matter how many times it loops. If there is a discontinuity in the
> loop, then the FFT result will change.

A discontinuity would provoke heavy noise across a rather wide range of frequency (try it!), very easy to detect in the frequency domain.
 
> - *perfect* means the Fourier transform works perfectly. For 440hz sine, the
> frequency-domain data should be all zero except 440Hz. However, we have a
> few noise around 440hz. See attachment 8941355 [details] or check it in
> attachment 8942111 [details].

We can't really do this "perfect" transform, because we don't have an infinite sample-rate and an Fourier transform of infinite size. We're real-time here so we have to bound things somehow.
Attachment #8934087 - Attachment is obsolete: true
Attachment #8934087 - Flags: review?(suro001)
Attachment #8934087 - Flags: review?(padenot)
Attachment #8940995 - Attachment is obsolete: true
Attachment #8941355 - Attachment is obsolete: true
Attachment #8942111 - Attachment is obsolete: true
Attached file wav.html
Picking this one up.

This is creating a perfectly looped sine wave of one second, at 44100Hz, convert it to a blob, play it in an HTMLMediaElement with `loop = true`, feed that into an AudioContext with an AnalyserNode, getting the FFT, checking that there is no discontinuity in the frequency domain.
Assignee: chun.m.chang → padenot
Missing an important comma here:

> checking that there is no discontinuity, in the frequency domain.
Attachment #8948449 - Attachment mime type: text/plain → text/html
Screencast that show the approach works well. Turning this into a media mochitest now.
Comment on attachment 8948672 [details]
Bug 1420322 - Add a test for seamless looping.

https://reviewboard.mozilla.org/r/218074/#review223950

::: dom/media/test/test_seamless_looping.html:7
(Diff revision 2)
> +<html>
> +<head>
> +  <title>Test for seamless loop of HTMLMediaElements</title>
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +  <script type="text/javascript" src="manifest.js"></script>

what's manifest.js needed for?

::: dom/media/test/test_seamless_looping.html:15
(Diff revision 2)
> +  // Adds a canvas with a little drawing of what is going on, and actually
> +  // outputs the audio.
> +  var DEBUG = false;

If I read this correctly:

s/Adds a canvas/Set DEBUG to true to add a canvas/, and
s/outputs the audio/outputs the audio to speakers/

::: dom/media/test/test_seamless_looping.html:22
(Diff revision 2)
> +    var binIndex440 = 1 + Math.round(tone_frequency * buf.length / ctxSampleRate);
> +    ok(buf[binIndex440] > -100,
> +       "Could not find a peak: " + buf[binIndex440] + "db at 440Hz");
> +
> +    // check that the energy at other freqs are very very low: a click will
> +    // increase the energy there.
> +    var binIndex4k = 1 + Math.round(4000 * buf.length / ctxSampleRate);
> +    ok(buf[binIndex4k] < -110,
> +       "Found unexpected high frequency content: " + buf[binIndex4k] + "db at 4000Hz");

You have `tone_frequency` but then the code assumes 440Hz. Shall we remove this assumption? The 4k is probably high enough but it could also be `tone_frequency * 2`, `tone_frequency / 2`, etc.

::: dom/media/test/test_seamless_looping.html:51
(Diff revision 2)
> +    // Make a RIFF header, it's 23 bytes
> +    var buf = new Int16Array(buffer.length + 23);
> +    buf[0] = 0x4952;
> +    buf[1] = 0x4646;
> +    buf[2] = (2 * buffer.length + 15) & 0x0000ffff;
> +    buf[3] = ((2 * buffer.length + 15) & 0xffff0000) >> 16;
> +    buf[4] = 0x4157;
> +    buf[5] = 0x4556;
> +    buf[6] = 0x6d66;
> +    buf[7] = 0x2074;
> +    buf[8] = 0x0012;
> +    buf[9] = 0x0000;
> +    buf[10] = 0x0001;
> +    buf[11] = 1;
> +    buf[12] = 44100 & 0x0000ffff;
> +    buf[13] = (44100 & 0xffff0000) >> 16;
> +    buf[14] = (2 * channels * sampleRate) & 0x0000ffff;
> +    buf[15] = ((2 * channels * sampleRate) & 0xffff0000) >> 16;
> +    buf[16] = 0x0004;
> +    buf[17] = 0x0010;
> +    buf[18] = 0x0000;
> +    buf[19] = 0x6164;
> +    buf[20] = 0x6174;
> +    buf[21] = (2 * buffer.length) & 0x0000ffff;
> +    buf[22] = ((2 * buffer.length) & 0xffff0000) >> 16;

I'll trust you on this :-)

::: dom/media/test/test_seamless_looping.html:82
(Diff revision 2)
> +    var b = new Blob([buf], { type: 'audio/wav' });
> +
> +    var media = document.createElement("audio");
> +    media.src = URL.createObjectURL(b);
> +    media.controls = true;
> +    media.loop = true;

Would looping over a blob act differently from looping over an audio file in any way? Do we need to test both? Does container matter?

::: dom/media/test/test_seamless_looping.html:85
(Diff revision 2)
> +    }
> +
> +    var b = new Blob([buf], { type: 'audio/wav' });
> +
> +    var media = document.createElement("audio");
> +    media.src = URL.createObjectURL(b);

Will the window or document going away revoke the object url or should we do it manually?

::: dom/media/test/test_seamless_looping.html:122
(Diff revision 2)
> +        source.disconnect();
> +        SimpleTest.finish();
> +      }
> +    }
> +
> +    function r() {

Can we give this a more useful name?

::: dom/media/test/test_seamless_looping.html:124
(Diff revision 2)
> +      // There is an unrelated click at start, skip it.
> +      if (loopCount) {

The semantics are clearer with `loopCount == 0` IMO.
Attachment #8948672 - Flags: review?(apehrson)
Comment on attachment 8948672 [details]
Bug 1420322 - Add a test for seamless looping.

https://reviewboard.mozilla.org/r/218074/#review223950

> what's manifest.js needed for?

Nothing, copy-paste artifact.

> Would looping over a blob act differently from looping over an audio file in any way? Do we need to test both? Does container matter?

If those two things matter, we have bigger problems. I now we have an issue with pre-roll on Opus, but it's a different issue altogether.

> Will the window or document going away revoke the object url or should we do it manually?

Yes, navigating away from the document will clean this.
Comment on attachment 8948672 [details]
Bug 1420322 - Add a test for seamless looping.

https://reviewboard.mozilla.org/r/218074/#review224450

::: dom/media/test/test_seamless_looping.html:23
(Diff revisions 5 - 6)
>  
>    function doAnalysis(buf, ctxSampleRate) {
> -    // first find a peak at 440
> -    var binIndex440 = 1 + Math.round(tone_frequency * buf.length / ctxSampleRate);
> -    ok(buf[binIndex440] > -100,
> -       "Could not find a peak: " + buf[binIndex440] + "db at 440Hz");
> +    // first find a peak where we expect one.
> +    var binIndexTone = 1 + Math.round(tone_frequency * buf.length / ctxSampleRate);
> +    ok(buf[binIndexTone] > -100,
> +       "Could not find a peak: " + buf[binIndexTone] + "db at "+ tone_frequency +"Hz");

You might find template strings to be nice, i.e., ```
`Could not find a peak: ${buf[binIndexTone]}db at ${tone_frequency}Hz`
```

::: dom/media/test/test_seamless_looping.html:20
(Diff revisions 6 - 7)
> +    // The size of an FFT is twice the number of bins in its output.
> +    var fftSize = 2 * buf.length;
>      // first find a peak where we expect one.
> -    var binIndexTone = 1 + Math.round(tone_frequency * buf.length / ctxSampleRate);
> +    var binIndexTone = 1 + Math.round(tone_frequency * fftSize / ctxSampleRate);

Or the analyser will tell you, like in https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/dom/media/tests/mochitest/head.js#205-207
Attachment #8948672 - Flags: review?(apehrson) → review+
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/autoland/rev/3129374e4781
Add a test for seamless looping. r=pehrsons
https://hg.mozilla.org/mozilla-central/rev/3129374e4781
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.