Closed Bug 1265405 Opened 4 years ago Closed 4 years ago

Use a dictionary to specify how PeriodicWave should be normalized (or not)

Categories

(Core :: Web Audio, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: padenot, Assigned: dminor)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files)

It's now normalized on creation only, if normalization is enabled, and uses a dictionary.

Spec changes at https://github.com/webaudio/web-audio-api/commit/a04cdabe1e98f054137e9ec58141e1c479d446ae.
Priority: -- → P2
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8744337 [details]
MozReview Request: Bug 1265405 - Add a dictionary to specify how PeriodicWave should be normalized (or not); r?smaug

https://reviewboard.mozilla.org/r/48469/#review45215
Attachment #8744337 - Flags: review?(bugs) → review+
Attachment #8744339 - Flags: review?(padenot) → review+
Comment on attachment 8744339 [details]
MozReview Request: Bug 1265405 - Add test case for PeriodicWave normalization; r?padenot

https://reviewboard.mozilla.org/r/48473/#review45435

::: dom/media/webaudio/test/test_periodicWaveDisableNormalization.html:4
(Diff revision 1)
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test the PeriodicWave interface</title>

Maybe adjust this title.

::: dom/media/webaudio/test/test_periodicWaveDisableNormalization.html:18
(Diff revision 1)
> +SimpleTest.waitForExplicitFinish();
> +
> +const realMax = 99;
> +var real = new Float32Array(realMax + 1);
> +real[1] = 2.0; // fundamental
> +real[realMax] = 0.25;

Write a comment about choosing these "magic" value, and maybe a bit more about the test: "create a periodic wave that contains two tones, ...".

::: dom/media/webaudio/test/test_periodicWaveDisableNormalization.html:70
(Diff revision 1)
> +                            context.sampleRate) +
> +         real[realMax] * Math.cos(2 * Math.PI * realMax * realFundamental * i /
> +                            context.sampleRate));
> +
> +      // TODO: We need to scale by a factor of two to make the results work
> +      //       out here. This seems suspicious, see Bug 1266737.

Yeah not sure about this one, I'm curious what you find in 1266737.
Comment on attachment 8744338 [details]
MozReview Request: Bug 1265405 - Use a dictionary to specify how PeriodicWave should be normalized (or not); r?padenot

https://reviewboard.mozilla.org/r/48471/#review45441
Attachment #8744338 - Flags: review?(padenot) → review+
https://reviewboard.mozilla.org/r/48473/#review45443

Also can you add a test case that tests that default value for `disableNormalization` is false (when the dictionnary is not passed in) ?
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/11de193d3777
https://hg.mozilla.org/mozilla-central/rev/94189d37e581
https://hg.mozilla.org/mozilla-central/rev/632811bf4b6e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
I've made sure this is documented on

https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/createPeriodicWave

I've also included a note in the relevant developer notes:

https://developer.mozilla.org/en-US/Firefox/Releases/49#Others

Let me know if these read ok. Thanks!
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #11)
> I've made sure this is documented on
> 
> https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/
> createPeriodicWave
> 
> I've also included a note in the relevant developer notes:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/49#Others
> 
> Let me know if these read ok. Thanks!

Thanks! It might be helpful to say that if normalized, the resulting wave will have a "maximum absolute peak value of 1" as the spec puts it, otherwise the results will depend upon the fourier coefficients, just so people don't have to think too hard about what normalization means in this context.
(In reply to Dan Minor [:dminor] from comment #12)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #11)
> > I've made sure this is documented on
> > 
> > https://developer.mozilla.org/en-US/docs/Web/API/AudioContext/
> > createPeriodicWave
> > 
> > I've also included a note in the relevant developer notes:
> > 
> > https://developer.mozilla.org/en-US/Firefox/Releases/49#Others
> > 
> > Let me know if these read ok. Thanks!
> 
> Thanks! It might be helpful to say that if normalized, the resulting wave
> will have a "maximum absolute peak value of 1" as the spec puts it,
> otherwise the results will depend upon the fourier coefficients, just so
> people don't have to think too hard about what normalization means in this
> context.

Done.
Duplicate of this bug: 1186345
You need to log in before you can comment on or make changes to this bug.