Closed Bug 1236937 Opened 8 years ago Closed 8 years ago

Wrong definition of PI in AAFilter.cpp

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: tonymec)

Details

Attachments

(1 file)

http://mxr.mozilla.org/mozilla-central/source/media/libsoundtouch/src/AAFilter.cpp#52
#define PI        3.141592655357989

That's horribly wrong.

According to Wikipedia, the first 50 decimals are:

3.14159265358979323846264338327950288419716939937510...

https://en.wikipedia.org/wiki/Pi
Does that module have access to trigonometric functions? If it does, then inverse trigonometric functions (in radians) are particularly useful. Of course they cannot be used in a define, but maybe to set up a "variable" whose value won't change once it has been computed.

4 * atan(1)
is usually regarded as giving the best precision.

2 * asin(1), or acos(-1), work also, but unless the trigonometric routine shortcuts them to some precalculated pi, the precision isn't as good.

This said, with how much precision is that pi-value stored?

By requesting a precision of %.60G I get the value
3.141592653589793115997963468544185161590576171875000000000000
meaning that I obviously asked for more than the software I used was capable of delivering. It seems that I got about 15 correct digits after the decimal point. Some 50 bits maybe?

I suppose that if AAFilter.cpp keeps its Floats in 25 bits or so (let's say 32 bits including the exponent?) the value would be about right. But does it?
I don't think any human can hear the difference between the two.
I'd take a patch and send it upstream, though.
(In reply to Paul Adenot (:padenot) from comment #3)
> I'd take a patch and send it upstream, though.

According to info, there is a "double" value of pi predefined as M_PI, a compiler macro in math.h, which that source file already includes, provided that the compiler accepts at least Unix 98 (4.4 BSD) source, which is part of the default source featureset.

It ought thus to be enough to simply replace the line in question with

#define PI M_PI

Here is the last paragraph of that info page:

   _Note:_ Some programs use a constant named `PI' which has the same
value as `M_PI'.  This constant is not standard; it may have appeared
in some old AT&T headers, and is mentioned in Stroustrup's book on C++.
It infringes on the user's name space, so the GNU C Library does not
define it.  Fixing programs written to expect it is simple: replace
`PI' with `M_PI' throughout, or put `-DPI=M_PI' on the compiler command
line.

Disclaimer: I haven't checked which source standards are used at Mozilla, but I have noticed in MXR that M_PI is used elsewhere in the source.
Thanks, the problem is that this code has been imported from another repository for use in the Mozilla source. We could certainly fix it in our tree, but it would be nice to send the patch upstream as well.
(In reply to Paul Adenot (:padenot) from comment #5)
> Thanks, the problem is that this code has been imported from another
> repository for use in the Mozilla source. We could certainly fix it in our
> tree, but it would be nice to send the patch upstream as well.

OK, OK, can't you make one? I'll attach a patch in Mozilla format with all due reservations. As I don't know what format is expected upstream, please do whatever is necessary after checking the patch, and in particular, that it gives the right value after preprocessing.
Attached patch patch v0.0Splinter Review
And, Paul, if you don't have reviewer privileges, please treat my r? as f? and find some other reviewer.
Attachment #8706816 - Flags: review?(padenot)
Attachment #8706816 - Flags: feedback?(mats)
Comment on attachment 8706816 [details] [diff] [review]
patch v0.0

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

Thanks !
Attachment #8706816 - Flags: review?(padenot)
Attachment #8706816 - Flags: review+
Attachment #8706816 - Flags: feedback?(mats)
Assignee: nobody → antoine.mechelynck
I've sent the patch upstream, and it's been taken by the maintainer.

Tony, thanks again !
Component: Audio/Video → Audio/Video: Playback
(In reply to Paul Adenot (:padenot) from comment #10)
> I've sent the patch upstream, and it's been taken by the maintainer.
> 
> Tony, thanks again !

I would have liked Mats to have his say about the patch, but otherwise, no problem. :-)
Comment on attachment 8706816 [details] [diff] [review]
patch v0.0

This looks like the correct fix to me, fwiw.

Can we do the same to the #define PI in InterpolateShannon.cpp ?
(see bug 1236939)
I'm hoping that the compiler doesn't warn about redefining it to
the exact same value.  Otherwise, we'll have to exclude these
files from being unified.
(In reply to Mats Palmgren (:mats) from comment #12)
> Comment on attachment 8706816 [details] [diff] [review]
> patch v0.0
> 
> This looks like the correct fix to me, fwiw.
> 
> Can we do the same to the #define PI in InterpolateShannon.cpp ?
> (see bug 1236939)
> I'm hoping that the compiler doesn't warn about redefining it to
> the exact same value.  Otherwise, we'll have to exclude these
> files from being unified.

IIUC, the problem causing the first warning in bug 1236969 comment #0 seems to be that both AAFilter.cpp and InterpolateShannon.cpp are included, in that order, by some file Unified_cpp_libsoundtouch_src0.cpp — which, however, I cannot find in my mozilla-central clone — and that the first definition is still valid (has not been #undef'ined) when the second one is encountered.

There are several possible solutions:
- replace PI by M_PI everywhere, and don't define it (the compiler does it). This, however, is only compatible with compilers supporting Unix-98 (4.4 BSD) [or later] source standards
- move the define to some file which will only be included once
- guard the define as follows, possibly also moving it to the *.h header for every source using it:

#ifndef PI
# define PI M_PI
// or define it as the actual exact numeric value for installations
// using libsoundtouch but not gcc and not a Unix-98-compatible compiler
#endif

I think this latter possibility (in its "numeric" variant) is the one with the most chance of being accepted by the libsoundtouch maintainers. Maybe even

#ifndef PI
# ifdef M_PI
#  define PI M_PI
# else
#  define PI 3.1415926535897932
# endif
#endif
https://hg.mozilla.org/mozilla-central/rev/a88bb7a3700c
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(In reply to Tony Mechelynck [:tonymec] from comment #13)
> IIUC, the problem causing the first warning in bug 1236969 comment #0 seems
> to be that both AAFilter.cpp and InterpolateShannon.cpp are included, in
> that order, by some file Unified_cpp_libsoundtouch_src0.cpp — which,
> however, I cannot find in my mozilla-central clone — and that the first
> definition is still valid (has not been #undef'ined) when the second one is
> encountered.

Yes, this is caused by "unified builds" where we concatenate multiple source
files into one and compile that instead to save compilation time.
You need to log in before you can comment on or make changes to this bug.