Closed Bug 1131529 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Web Audio, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: padenot, Assigned: ankit.goyal90, Mentored)

Details

(Whiteboard: [lang=c++])

Attachments

(2 files, 1 obsolete file)

Mentor: padenot
Whiteboard: [lang=c++]
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
hey paul, i am working on it.
Flags: needinfo?(padenot)
Flags: needinfo?(padenot)
Attachment #8563227 - Flags: review?(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.
Attachment #8563227 - Flags: review?(padenot) → review+
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.
Attachment #8564007 - Flags: feedback?(padenot) → feedback-
Attachment #8564007 - Attachment is obsolete: true
Attachment #8564952 - Flags: review?(padenot)
Attachment #8564952 - Flags: review?(padenot) → review+
https://hg.mozilla.org/mozilla-central/rev/c4141f98a5df
Assignee: nobody → ankit.goyal90
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.