Bug 34538 – audio engine: add Complex number class

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Chris Rogers]

The audio engine needs a class representing a complex number.  It's used in various DSP / math algorithms.

[Sam Weinig]

Are there reasons we can't use std::complex other than the normal WebCore rule of not using stdlib types. If it is usable for your purposes, I think we should consider augmenting the rule for this, like we do for std::pair.

[Chris Rogers]

Created attachment 48062 [details]
patch for Complex number class

[Chris Rogers]

Hi Sam, I anticipate extending this class in the future for various DSP algorithms, so I'd prefer to use our own implementation.

[Darin Adler]

Comment on attachment 48062 [details]
patch for Complex number class

> +    Complex() : m_real(0.0), m_imag(0.0) { }

We try to keep abbreviations to a minimum in WebKit and use words instead. So this would normally be called "imaginary", not "imag".

> +    double abs() const { return sqrt(m_real*m_real + m_imag*m_imag); }

We put spaces around operators like "*".

> +    double magnitude() const { return abs(); }

Do we really need both the names "abs()" and "magnitude()"? How about choosing just one? I suggest "magnitude".

> +    void setMagnitudePhase(double mag, double phase)
> +    {
> +        m_real = mag * cos(phase);
> +        m_imag = mag * sin(phase);
> +    }

It seems like we'd want this to be a function that returns a Complex, rather than a setter.

> +protected:
> +    double m_real;
> +    double m_imag;

private, not protected.

[Chris Rogers]

Created attachment 48065 [details]
addresses Darin Adler's initial comments

Hi Darin, thanks for having such a quick look!

[Darin Adler]

(In reply to comment #3)
> Hi Sam, I anticipate extending this class in the future for various DSP
> algorithms, so I'd prefer to use our own implementation.

I think we can add lots of functions that work with std::complex too, though, so I’m not sure a new class is the right way to go.

[Chris Rogers]

subclassing std::complex to add additional methods looks like a problem since the ivars are private.  I'd prefer, if at all possible, to have something lightweight and more easily modifiable to our needs.

[Darin Adler]

(In reply to comment #7)
> subclassing std::complex to add additional methods looks like a problem since
> the ivars are private.  I'd prefer, if at all possible, to have something
> lightweight and more easily modifiable to our needs.

The technique is not subclassing. It's functions that take and return complex objects.

I hear you expressing your preference. But I don’t see a good reason to consider std::complex heavyweight. And it comes with many of the functions you’d want to use already implemented. We should really consider it seriously before adding our own.

Maybe you could show some coding examples to clarify why the code ends up looking so much cleaner with your class than if you used std::complex?

[Chris Rogers]

OK - don't want to rock the boat too much :)
I'm sure it will be possible to adapt to std::complex.  This will probably be fine for the current engine code.  If it starts to get too messy, as I've been fearing, then we can revisit it later.

Thanks for having a look - I guess we can close out this bug...

[Eric Seidel (no email)]

Closing as wontfix per Chris' comment above.

[Eric Seidel (no email)]

Comment on attachment 48065 [details]
addresses Darin Adler's initial comments

Clearing review flag per Chris's above comment.

[Chris Rogers]

Sorry, I'm re-opening this.  I posted to webkit-dev with the problems with std::complex - re-pasting here:

In the process of switching my code over to use std::complex I noticed a conflict with isinf(), isnan(), etc.

The problem is that simply including:

#include <complex>

breaks the isinf(), isnan() functions (and some others I think).  So now I'm getting compile errors in any header files which use these functions, such as WebGLFloatArray.h (which I need to include for music visualizer stuff).
I'm a bit queasy about all the side-effects of simply including <complex> and am not even sure how to address the current situation, short of switching all of webkit over to using std::isinf, std::isnan, etc.

Now I remember having similar problems with this in other codebases I've worked on, as the effects of <complex> seem to be viral...

Unless somebody has a reasonable fix for the sinister side-effects of including
<complex> I'd like my patch to be re-considered.

Thanks,
Chris

[Darin Adler]

(In reply to comment #12)
> So now I'm
> getting compile errors in any header files which use these functions, such as
> WebGLFloatArray.h

Lets start by fixing those -- if it really ends up being too much of a pain we can fall back. I don’t want to give up at the first sign of trouble. It’s great to not have to reinvent the C++ library.

[Chris Rogers]

Hi Darin, I've been trying to move forward by changing all the uses of isnan() in the WebGL header files to std::isnan().  This also required including <cmath> in all of these header files so that std::isnan() is defined.

But now this appears to be having a cascading effect, and this creates a compile error in
JSImmediate.h (a use of the signbit() function) in JavaScriptCore.  After including <cmath> here and using std::signbit(), I'm now getting a compile error in CachedResource ( function isfinite() ).  I've found that the isfinite() function is used many places in the codebase, as well as isinf(), isnan(), etc. etc.

So, I'm not sure what to do.  I'm very uncomfortable trying to do a massive munge to convert the whole codebase to be <cmath> friendly since I'm very new here, especially considering possible differences across platforms.  I think it would be a very large number of changes.

What do you think we should do?

[Darin Adler]

(In reply to comment #14)
> Hi Darin, I've been trying to move forward by changing all the uses of isnan()
> in the WebGL header files to std::isnan().  This also required including
> <cmath> in all of these header files so that std::isnan() is defined.

Could you post your patch in progress somewhere so I can spend a little time on this?

[Darin Adler]

One of the first things I noticed is that many of those isnan checks are unneeded, because converting NaN to an integer already yields a zero. Not directly relevant, but something I spotted while looking at this.

[Chris Rogers]

Created attachment 48516 [details]
Patch

[Chris Rogers]

Comment on attachment 48516 [details]
Patch

I've removed my original patch where I created a new complex number class which was defined in Complex.h.  Instead, I've added a utilities header for adding new methods on std::complex as recommended by Darin Adler.

[Darin Adler]

Comment on attachment 48516 [details]
Patch

I like everything about this except for the suffix Utilities. I think this file would be just fine named Complex.h -- named after the typedef that's in it.

> +#include <stdio.h>

Why include this? We should not.

I'm going to say review- because I think we should not choose this filename.

[Chris Rogers]

Created attachment 48533 [details]
Patch

[Chris Rogers]

Comment on attachment 48533 [details]
Patch

changed filename from ComplexUtilities.h to Complex.h
also removed #include <stdio.h>

[WebKit Commit Bot]

Comment on attachment 48533 [details]
Patch

Clearing flags on attachment: 48533

Committed r54684: <http://trac.webkit.org/changeset/54684>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.