Optimize inverse FFT scaling during convolution

RESOLVED FIXED in mozilla29

Status

()

Core
Web Audio
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

({perf})

Trunk
mozilla29
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 8355931 [details] [diff] [review]
patch

This reduces MSG CPU usage by 2% on Demo 4 of http://webaudiodemos.appspot.com/MIDIDrums/index.html
Attachment #8355931 - Flags: review?(paul)
(Assignee)

Updated

4 years ago
Keywords: perf
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.
Attachment #8355931 - Flags: review?(paul) → review+
(Assignee)

Comment 3

4 years ago
Created attachment 8356269 [details] [diff] [review]
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.
Attachment #8356269 - Flags: review?(paul)
(Assignee)

Comment 4

4 years ago
Created attachment 8356271 [details] [diff] [review]
Remove unnecessary channel count parameter from AudioBufferInPlaceScale

Makes the interface more consistent with other AudioBuffer functions.
Attachment #8356271 - Flags: review?(paul)

Updated

4 years ago
Attachment #8356269 - Flags: review?(paul) → review+
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?
Attachment #8356271 - Flags: review?(paul) → review+
(Assignee)

Comment 6

4 years ago
Created attachment 8356499 [details] [diff] [review]
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?
Attachment #8356499 - Flags: review?(paul)

Updated

4 years ago
Attachment #8356499 - Flags: review?(paul) → review+
(Assignee)

Updated

4 years ago
Flags: in-testsuite-
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 ?
Flags: needinfo?(karlt)
(Assignee)

Comment 11

4 years ago
(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.
Flags: needinfo?(karlt)
(Assignee)

Comment 12

4 years ago
Bug 957691 doesn't involve web audio and is likely the same bug as the assertions in comments 9 and 10.
You need to log in before you can comment on or make changes to this bug.