Closed Bug 1188704 Opened 4 years ago Closed 4 years ago

simplify PeriodicWave::createBandLimitedTables()

Categories

(Core :: Web Audio, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: karlt, Assigned: karlt)

Details

Attachments

(8 files)

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.
There are no behavior changes here, just the change in meaning of the variable.
Attachment #8640261 - Flags: review?(giles)
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
Attachment #8640262 - Flags: review?(giles)
realP[halfSize] was always set to zero because numberOfPartials < halfSize + 1
was always true.
Attachment #8640263 - Flags: review?(giles)
avoiding copying and scaling components that will be zeroed.
Attachment #8640264 - Flags: review?(giles)
Attachment #8640265 - Flags: review?(giles)
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.
Attachment #8640261 - Flags: review?(giles) → review+
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?
Attachment #8640265 - Flags: review?(giles) → review+
Attachment #8640264 - Flags: review?(giles) → review+
Attachment #8640263 - Flags: review?(giles) → review+
Attachment #8640262 - Flags: review?(giles) → review+
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 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);
(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.
Attachment #8640839 - Flags: review?(giles)
Attachment #8640838 - Flags: review?(giles) → review+
Attachment #8640840 - Flags: review?(giles) → review+
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.
Attachment #8640839 - Flags: review?(giles) → review+
(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) {
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.