1188704 - simplify PeriodicWave::createBandLimitedTables()

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Karl Tomlinson (:karlt)]

I found it hard to work out exactly which partials were culled in the tables
of createBandLimitedTables().  I think some changes can make this clearer.
The goal here is not to change the end result but to get there more directly
and clearly, which may also happen to remove some unnecessary operations.
See what you think.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: redefine halfSize as fftSize / 2

There are no behavior changes here, just the change in meaning of the variable.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: trim unnecessary extra basic waveform coeffient

The built-in waveforms are all odd and so imagP[halfSize] was ignored.

This returns the code to that prior to
https://hg.mozilla.org/mozilla-central/diff/5377bce3b478/content/media/webaudio/blink/PeriodicWave.cpp#l1.276

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: limit number of Fourier coefficients used to halfSize earlier

realP[halfSize] was always set to zero because numberOfPartials < halfSize + 1
was always true.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: simplify culling of partials

avoiding copying and scaling components that will be zeroed.

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: combine scaling with copying

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8640261 [details] [diff] [review]
redefine halfSize as fftSize / 2

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

I don't really think this is better, but I suppose I have no objection.

Maybe halfSize is a poor name. The +1 comes from the properties of the fft giving us dc+nyquist values, so we need N/2 + 1 complex values to pass to the ifft to get back an N-point real function. A comment could explain that without needing to add +1 everwhere, but this does emphasize that this realP[] and imagP[] use closed rather than the usual half-open C array bounds.

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8640262 [details] [diff] [review]
trim unnecessary extra basic waveform coeffient

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

::: dom/media/webaudio/blink/PeriodicWave.cpp
@@ +269,2 @@
>  
> +    for (unsigned n = 1; n < halfSize; ++n) {

What about realP[halfsize]. Is that not still meaningful?

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8640262 [details] [diff] [review]
trim unnecessary extra basic waveform coeffient

(In reply to Ralph Giles (:rillian) from comment #7)
> What about realP[halfsize]. Is that not still meaningful?

It's not.  I've changed the first paragraph in the commit message to:

The built-in waveforms are all odd and so realP[halfSize] was zero, and it
would have been ignored in createBandLimitedTables even if it was non-zero.
imagP[halfSize] was ignored as it was not involved in the inverse FFT.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8640263 [details] [diff] [review]
limit number of Fourier coefficients used to halfSize earlier

After irc discussion, I've added this comment:

+    // Limit the number of components used to those for frequencies below the
+    // Nyquist of the fixed length inverse FFT.
     numberOfComponents = std::min(numberOfComponents, halfSize);

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: add accessor functions for setting frequency components for inverse FFT

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: use existing FFTBlock arrays instead of allocating and copying and scaling

(In reply to Ralph Giles (:rillian) from comment #6)
> Comment on attachment 8640261 [details] [diff] [review]
> I don't really think this is better, but I suppose I have no objection.
> 
> Maybe halfSize is a poor name. The +1 comes from the properties of the fft
> giving us dc+nyquist values, so we need N/2 + 1 complex values to pass to
> the ifft to get back an N-point real function. A comment could explain that
> without needing to add +1 everwhere,

With this change, there is no need to add +1 to halfSize anywhere and it is
only used where we want N/2.

It is now no longer necessary to clear zero bins because these are zeroed
in the FFTBlock constructor.

nsTArray bounds assertions now apply.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: remove now unused PerformInverseFFT() variation

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 8640839 [details] [diff] [review]
use existing FFTBlock arrays instead of allocating and copying and scaling

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

::: dom/media/webaudio/blink/PeriodicWave.cpp
@@ +201,5 @@
> +        // Copy from loaded frequency data and generate complex conjugate
> +        // because of the way the inverse FFT is defined.
> +        for (i = 0; i < numberOfPartials + 1; ++i) {
> +            frame.RealData(i) = realData[i];
> +            frame.ImagData(i) = -imagData[i];

IIRC we had ambitions of accelerating AudioBufferCopyWithScale, but it looks like this hasn't happened. I suppose this function shouldn't be called very often even if the compiler doesn't do as well as memcpy here.

@@ +206,1 @@
>          }

Might be worth commmenting here that FFTBlock zeros its coefficients on construction, so the reset of the above-nyquist partials will be null.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de9cf19f0a9f
https://hg.mozilla.org/integration/mozilla-inbound/rev/595664043330
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6feac456665
https://hg.mozilla.org/integration/mozilla-inbound/rev/d647fe2abdc9
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf6f07f8060
https://hg.mozilla.org/integration/mozilla-inbound/rev/b70aa71884d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/91d2934096a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e16627c8968

Comment 15

[Karl Tomlinson (:karlt)]

(In reply to Ralph Giles (:rillian) on PTO until Aug 18. from comment #13)
> Comment on attachment 8640839 [details] [diff] [review]

> ::: dom/media/webaudio/blink/PeriodicWave.cpp
> @@ +201,5 @@
> > +        // Copy from loaded frequency data and generate complex conjugate
> > +        // because of the way the inverse FFT is defined.
> > +        for (i = 0; i < numberOfPartials + 1; ++i) {
> > +            frame.RealData(i) = realData[i];
> > +            frame.ImagData(i) = -imagData[i];
> 
> IIRC we had ambitions of accelerating AudioBufferCopyWithScale, but it looks
> like this hasn't happened. I suppose this function shouldn't be called very
> often even if the compiler doesn't do as well as memcpy here.

It's not costing us more to do this here because this copy was happening in
PerformInverseFFT() anyway.  Now we skip the AudioBufferCopyWithScale
altogether.

https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/dom/media/webaudio/FFTBlock.h#l159
https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/dom/media/webaudio/FFTBlock.h#l147

> Might be worth commmenting here that FFTBlock zeros its coefficients on
> construction, so the reset of the above-nyquist partials will be null.

Thanks.  Added this:

         // Copy from loaded frequency data and generate complex conjugate
         // because of the way the inverse FFT is defined.
+        // The coefficients of higher partials remain zero, as initialized in
+        // the FFTBlock constructor.
         for (i = 0; i < numberOfPartials + 1; ++i) {

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de9cf19f0a9f
https://hg.mozilla.org/mozilla-central/rev/595664043330
https://hg.mozilla.org/mozilla-central/rev/f6feac456665
https://hg.mozilla.org/mozilla-central/rev/d647fe2abdc9
https://hg.mozilla.org/mozilla-central/rev/2bf6f07f8060
https://hg.mozilla.org/mozilla-central/rev/b70aa71884d9
https://hg.mozilla.org/mozilla-central/rev/91d2934096a1
https://hg.mozilla.org/mozilla-central/rev/3e16627c8968