1131529 - Update the maximum FFT size in AnalyserNode to be 2^15

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Paul Adenot (:padenot)]

Spec discussion, and rationale for this change: https://github.com/WebAudio/web-audio-api/issues/375
Patch: https://github.com/WebAudio/web-audio-api/pull/463

Comment 1

[Paul Adenot (:padenot)]

Code is here [0], this is just a matter of changing the maximum of the range to be 2^15, really.

Then, there will probably be tests to adjust to reflect this change, a quick `grep fftSize dom/media/webaudio/test/*` will probably reveal those.

[0]: https://dxr.mozilla.org/mozilla-central/source/dom/media/webaudio/AnalyserNode.cpp#123

Comment 2

[Ankit Goyal]

hey paul, i am working on it.

Comment 3

[Ankit Goyal]

Attachment: Bug 1131529 - Update the maximum FFT size in AnalyserNode to be 2^15. r=padenot

Comment 4

[Paul Adenot (:padenot)]

Comment on attachment 8563227 [details] [diff] [review]
Bug 1131529 - Update the maximum FFT size in AnalyserNode to be 2^15. r=padenot

Review of attachment 8563227 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, thanks!

Now can you write a second patch to fix dom/media/webaudio/test/test_analyserNode.html? it checks that the max value is 2048, this should be updated to 2^15 as well.

Comment 5

[Ankit Goyal]

Attachment: Bug 1131529 - Update the maximum FFT size in AnalyserNode to be 2^15. r=padenot

Comment 6

[Paul Adenot (:padenot)]

Comment on attachment 8564007 [details] [diff] [review]
Bug 1131529 - Update the maximum FFT size in AnalyserNode to be 2^15. r=padenot

Review of attachment 8564007 [details] [diff] [review]:
-----------------------------------------------------------------

Overall good, but the default value does not need to change, would you mind sending an updated patch with the comments addressed ?

::: dom/media/webaudio/test/test_analyserNode.html
@@ -14,5 @@
>  addLoadEvent(function() {
>  
>    var context = new AudioContext();
> -  var buffer = context.createBuffer(1, 2048, context.sampleRate);
> -  for (var i = 0; i < 2048; ++i) {

This does not need to change.

@@ +34,4 @@
>    is(analyser.channelCountMode, "max", "Correct channelCountMode for the analyser node");
>    is(analyser.channelInterpretation, "speakers", "Correct channelCountInterpretation for the analyser node");
>  
> +  is(analyser.fftSize, 32768, "Correct default value for fftSize");

Nope, the default value should still be 2048, it's only the max value that changes.

Comment 7

[Ankit Goyal]

Attachment: Bug 1131529 - Update the maximum FFT size in AnalyserNode to be 2^15. r=padenot

Comment 8

[Paul Adenot (:padenot)]

Thanks, pushed!

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4141f98a5df

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4141f98a5df