956604 - Optimize inverse FFT scaling during convolution

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Karl Tomlinson (:karlt)]

Conventions for forward and inverse FFTs usually scale by the reciprocal of
the size (1/N) when performing the inverse.

When convolving, the forward FFT of the impulse response buffer is performed
only once but used for each block in the audio stream.  By moving the 1/N
scaling to the DFT from the impulse response, the scaling need only be
performed once, instead of for each inverse FFT.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: patch

This reduces MSG CPU usage by 2% on Demo 4 of http://webaudiodemos.appspot.com/MIDIDrums/index.html

Comment 2

[Paul Adenot (:padenot)]

Comment on attachment 8355931 [details] [diff] [review]
patch

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

Makes sense, thanks.

::: content/media/webaudio/FFTBlock.cpp
@@ +49,5 @@
> +    newBlock->GetInverseWithoutScaling(buffer.Elements());
> +    const float scale = 1.0f / fftSize;
> +    for (int i = 0; i < fftSize / 2; ++i) {
> +      buffer[i] *= scale;
> +    }

AudioBufferInPlaceScale (it has a variant with an arbitrary size array), please, so we can be faster on ARM, and on x86 when I finally land the SSE patches.

::: content/media/webaudio/FFTBlock.h
@@ +52,5 @@
> +    GetInverseWithoutScaling(aDataOut);
> +    const float scale = 1.0f / mFFTSize;
> +    for (uint32_t i = 0; i < mFFTSize; ++i) {
> +      aDataOut[i] *= scale;
> +    }

Same remark.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: part 1: Rename block version of AudioBufferInPlaceScale to AudioBlockInPlaceScale

(In reply to Paul Adenot (:padenot) from comment #2)
> AudioBufferInPlaceScale (it has a variant with an arbitrary size array),
> please, so we can be faster on ARM, and on x86 when I finally land the SSE
> patches.

Ah, yes.  I looked for that but somehow looked over it.  Perhaps I got confused by the naming.  There is almost a convention in the naming of block and arbitrary size methods.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: Remove unnecessary channel count parameter from AudioBufferInPlaceScale

Makes the interface more consistent with other AudioBuffer functions.

Comment 5

[Paul Adenot (:padenot)]

Comment on attachment 8356271 [details] [diff] [review]
Remove unnecessary channel count parameter from AudioBufferInPlaceScale

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

::: content/media/AudioNodeEngineNEON.cpp
@@ +135,5 @@
>    float32x4_t vin0, vin1, vin2, vin3;
>    float32x4_t vout0, vout1, vout2, vout3;
>    float32x4_t vscale = vmovq_n_f32(aScale);
>  
> +  uint32_t totalSize = aSize;

Could you just use aSize below instead of copying it to another variable?

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: part 3: renaming totalSize to vectorSize

(In reply to Paul Adenot (:padenot) from comment #5)
> Could you just use aSize below instead of copying it to another variable?

It would work, but the a- prefix on the variable suggests that it is the value passed to the function, so I would prefer not to modify aSize.

How about this?

Comment 7

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b73bf63d70c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/3555983eb8d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/59b94799b064

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b73bf63d70c8
https://hg.mozilla.org/mozilla-central/rev/3555983eb8d2
https://hg.mozilla.org/mozilla-central/rev/59b94799b064

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Any chance these patches could be causing all of the asserts in https://tbpl.mozilla.org/php/getParsedLog.php?id=32720120&tree=Fx-Team and https://tbpl.mozilla.org/php/getParsedLog.php?id=32725298&tree=Fx-Team ?

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Wes Kocher (:KWierso) from comment #9)
> Any chance these patches could be causing all of the asserts in
> https://tbpl.mozilla.org/php/getParsedLog.php?id=32720120&tree=Fx-Team and
> https://tbpl.mozilla.org/php/getParsedLog.php?id=32725298&tree=Fx-Team ?

And https://tbpl.mozilla.org/php/getParsedLog.php?id=32724152&tree=Mozilla-Central ?

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to Wes Kocher (:KWierso) from comment #10)

Those are not likely the patches in this bug, which change values rather than lengths, but there have been other webaudio and MediaStreamGraph changes that may be involved.

I'll see if I can do some retriggers tomorrow if we don't have more info then.

Comment 12

[Karl Tomlinson (:karlt)]

Bug 957691 doesn't involve web audio and is likely the same bug as the assertions in comments 9 and 10.