simplify PeriodicWave::createBandLimitedTables()

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

Trunk
mozilla42
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(8 attachments)

(Assignee)

Description

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

Comment 1

3 years ago
Created attachment 8640261 [details] [diff] [review]
redefine halfSize as fftSize / 2

There are no behavior changes here, just the change in meaning of the variable.
Attachment #8640261 - Flags: review?(giles)
(Assignee)

Comment 2

3 years ago
Created attachment 8640262 [details] [diff] [review]
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
Attachment #8640262 - Flags: review?(giles)
(Assignee)

Comment 3

3 years ago
Created attachment 8640263 [details] [diff] [review]
limit number of Fourier coefficients used to halfSize earlier

realP[halfSize] was always set to zero because numberOfPartials < halfSize + 1
was always true.
Attachment #8640263 - Flags: review?(giles)
(Assignee)

Comment 4

3 years ago
Created attachment 8640264 [details] [diff] [review]
simplify culling of partials

avoiding copying and scaling components that will be zeroed.
Attachment #8640264 - Flags: review?(giles)
(Assignee)

Comment 5

3 years ago
Created attachment 8640265 [details] [diff] [review]
combine scaling with copying
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+
(Assignee)

Comment 8

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

Comment 9

3 years ago
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);
(Assignee)

Comment 10

3 years ago
Created attachment 8640838 [details] [diff] [review]
add accessor functions for setting frequency components for inverse FFT
Attachment #8640838 - Flags: review?(giles)
(Assignee)

Comment 11

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

Comment 12

3 years ago
Created attachment 8640840 [details] [diff] [review]
remove now unused PerformInverseFFT() variation
Attachment #8640840 - 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+
(Assignee)

Comment 15

3 years ago
(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.