Closed
Bug 1265405
Opened 9 years ago
Closed 9 years ago
Use a dictionary to specify how PeriodicWave should be normalized (or not)
Categories
(Core :: Web Audio, defect, P1)
Core
Web Audio
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.
|
Assignee | |
Updated•9 years ago
|
Priority: -- → P2
|
|
Assignee | |
Updated•9 years ago
|
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Priority: P2 → P1
|
|
Assignee | |
Comment 1•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/48469/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48469/
Attachment #8744337 -
Flags: review?(bugs)
Attachment #8744338 -
Flags: review?(padenot)
Attachment #8744339 -
Flags: review?(padenot)
|
|
Assignee | |
Comment 2•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/48471/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48471/
|
|
Assignee | |
Comment 3•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/48473/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48473/
|
||
Comment 4•9 years ago
|
||
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+
|
Reporter | |
Updated•9 years ago
|
Attachment #8744339 -
Flags: review?(padenot) → review+
|
|
Reporter | |
Comment 5•9 years ago
|
||
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.
|
|
Reporter | |
Comment 6•9 years ago
|
||
|
|
Reporter | |
Comment 7•9 years ago
|
||
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+
|
|
Reporter | |
Comment 8•9 years ago
|
||
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) ?
|
|
Assignee | |
Updated•9 years ago
|
Keywords: dev-doc-needed
|
||
Comment 10•9 years ago
|
||
| bugherder | ||
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: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
|
||
Comment 11•9 years ago
|
||
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!
Keywords: dev-doc-needed → dev-doc-complete
|
|
Assignee | |
Comment 12•9 years ago
|
||
(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.
|
|
||
Comment 13•9 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•