776429 - Unify definitions of M_PI

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Doug Sherk (:drs) (inactive)]

We have several definitions of M_PI floating around in our code. We should unify our them, probably into somewhere like mfbt/Math.h

Comment 1

[Sergey Glushchenko]

Attachment: Move most of M_PI definitions to mfbt/Math.h

Hello, this is my first bug, so please correct me if I'm not following the process.

grepping through the sources for the definitions of M_PI showed the following:

$ grep -RI 'define M_PI' .
./gfx/2d/Blur.cpp:#define M_PI 3.14159265358979323846 (+)
./gfx/2d/DrawTargetD2D.cpp:#define M_PI 3.14159265358979323846 (+)
./gfx/2d/PathD2D.cpp:#define M_PI 3.14159265358979323846 (+)
./gfx/2d/PathHelpers.h:#define M_PI 3.14159265358979323846 (+)
./gfx/cairo/cairo/src/cairoint.h:#define M_PI 3.14159265358979323846
./gfx/cairo/libpixman/src/pixman-compiler.h:# define M_PI           3.14159265358979323846
./gfx/cairo/nonfatal-assertions.patch: #define M_PI 3.14159265358979323846
./gfx/layers/ipc/AsyncPanZoomController.cpp:#define M_PI 3.14159265358979323846 (+)
./gfx/thebes/gfxContext.cpp:#define M_PI 3.14159265358979323846 (+)
./js/src/jsmath.cpp:#define M_PI            3.14159265358979323846 (+)
./layout/svg/base/src/nsSVGUtils.h:#define M_PI 3.14159265358979323846 (+)
./media/libopus/celt/modes.c:#define M_PI 3.141592653
./media/libspeex_resampler/src/resample.c:#define M_PI 3.14159263
./media/libtremor/lib/os.h:#  define M_PI (3.1415926536f)
./media/libvorbis/lib/os.h:#  define M_PI (3.1415926536f)
./media/webrtc/trunk/src/modules/audio_coding/codecs/iSAC/main/source/fft.c:# define M_PI 3.14159265358979323846264338327950288
./widget/windows/nsWinGesture.cpp:#define M_PI 3.14159265358979323846 (+)

It is my understanding that we shouldn't touch any third-party libs like cairo and ones in media/. So I've marked with (+) places where Math.h should be used instead of in-place #define's.

I've successfully built up-to-date Nightly with these changes.

Thank you!

Comment 2

[Doug Sherk (:drs) (inactive)]

Comment on attachment 646917 [details] [diff] [review]
Move most of M_PI definitions to mfbt/Math.h

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

Overall great work! Just some nits to do with styling. Please fix these and resubmit.

Will you need me to land this for you?

::: gfx/2d/Blur.cpp
@@ +10,4 @@
>  
>  #include "mozilla/CheckedInt.h"
>  #include "mozilla/Util.h"
> +#include "mozilla/Math.h"

Please keep these in alphabetical order.

::: gfx/2d/DrawTargetD2D.cpp
@@ +17,4 @@
>  
>  #include <dwrite.h>
>  
> +#include "mozilla/Math.h"

This should go below the bigger set of headers. Unfortunately I don't know anything about why dwrite.h was separated like this but I think it's safe to say that mozilla/Math.h fits in more nicely with the other headers.

::: gfx/2d/PathHelpers.h
@@ +8,4 @@
>  
>  #include "2D.h"
>  
> +#include "mozilla/Math.h"

I'm being really picky here, but no need for a new line above this :)

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ -18,5 @@
>  namespace layers {
>  
> -#ifndef M_PI
> -#define M_PI 3.14159265358979323846
> -#endif

There's an extra line here now, between "namespace layers {" and "static const float EPSILON = 0.0001";

::: gfx/thebes/gfxContext.cpp
@@ +2097,4 @@
>    }
>  
>    mDT->SetTransform(aNewMatrix);
> +}

I'm not sure what happened here, but please remove this.

::: mfbt/Math.h
@@ +8,5 @@
> +#ifndef mozilla_Math_h_
> +#define mozilla_Math_h_
> +
> +#ifndef M_PI
> +#define M_PI 3.14159265358979323846

It seems to be a mfbt convention to indent within preprocessor blocks, so this would be "#  define ..."

::: widget/windows/nsWinGesture.cpp
@@ +20,1 @@
>  

Extra line.

Comment 3

[Sergey Glushchenko]

Doug,

I've updated the patch according to your comments. However, during the last week some changes have been made to the codebase which cause build to fail with the patch on OSX with default filesystem setup (file names are case-insensitive).

make -C mfbt libs
bignum-dtoa.cc
/usr/bin/clang++ -o bignum-dtoa.o -c  -fvisibility=hidden -DIMPL_MFBT  -I/Users/sergey/work/firefox/mozilla/mfbt -I. -I../dist/include  -I/Users/sergey/work/firefox/mozilla/obj-x86_64-apple-darwin11.4.0/dist/include/nspr -I/Users/sergey/work/firefox/mozilla/obj-x86_64-apple-darwin11.4.0/dist/include/nss      -fPIC -Qunused-arguments  -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -DNO_X11 -pipe  -DNDEBUG -DTRIMMED -g -O3 -fomit-frame-pointer  -Qunused-arguments  -DMOZILLA_CLIENT -include ../mozilla-config.h -MD -MF .deps/bignum-dtoa.o.pp /Users/sergey/work/firefox/mozilla/mfbt/double-conversion/bignum-dtoa.cc
/Users/sergey/work/firefox/mozilla/mfbt/double-conversion/bignum-dtoa.cc:410:21: error: use of undeclared identifier 'ceil'
  double estimate = ceil((exponent + kSignificandSize - 1) * k1Log10 - 1e-10);
                    ^
1 error generated.

Because of -I/Users/sergey/work/firefox/mozilla/mfbt, "#include <math.h>" at the beginning of bignum-dtoa.cc gets substituted by mfbt/Math.h which is also mfbt/math.h in OSX.

To get around this issue, I propose to simply rename mfbt/Math.h to mfbt/MathHelper.h. What do you think?

Comment 4

[Doug Sherk (:drs) (inactive)]

Since this is currently restricted to a single constant and I don't know how likely it is to have new stuff added to it, I would actually prefer to rename it to Constants.h. I feel like "Math.h" is already kind of misleading (I would assume it had trig functions, etc.), so "MathHelper.h" is too.

Comment 5

[Sergey Glushchenko]

Attachment: Updated patch

Comment 6

[Sergey Glushchenko]

I will need you to land this for me. Thanks.

Comment 7

[Doug Sherk (:drs) (inactive)]

Comment on attachment 649227 [details] [diff] [review]
Updated patch

Nice!

Comment 8

[Doug Sherk (:drs) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6beb523ebcb5

Comment 9

[David Zbarsky (:dzbarsky)]

You probably also want to use M_PI in 
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/DOMSVGMatrix.cpp#12
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGTransform.cpp#12

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6beb523ebcb5

Comment 11

[Doug Sherk (:drs) (inactive)]

(In reply to David Zbarsky (:dzbarsky) (away until August 20) from comment #9)
> You probably also want to use M_PI in 
> http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/
> DOMSVGMatrix.cpp#12
> http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/
> SVGTransform.cpp#12

Sergey, are you going to make a patch for these too?

Comment 12

[Sergey Glushchenko]

Attachment: A few more clean-ups.

I've got a small concern with regard to converting all the uses of PI throughout the code to a single definition in mfbt/Constants.h. First of all, PI was defined with different precision in different places, and by using a unified constant we change the precision level in some places. Second, even now in some places our M_PI gets substituted by the one from <math.h>, i.e. in DOMSVGMatrix.cpp in this patch, and that M_PI from <math.h> is also different (at least on OSX). I'm just hoping that these precision "fluctuations" of the value of PI won't hurt in sensitive areas like graphics.

Comment 13

[Doug Sherk (:drs) (inactive)]

(In reply to Sergey Glushchenko from comment #12)
> Created attachment 649859 [details] [diff] [review]
> A few more clean-ups.
> 
> I've got a small concern with regard to converting all the uses of PI
> throughout the code to a single definition in mfbt/Constants.h. First of
> all, PI was defined with different precision in different places, and by
> using a unified constant we change the precision level in some places.
> Second, even now in some places our M_PI gets substituted by the one from
> <math.h>, i.e. in DOMSVGMatrix.cpp in this patch, and that M_PI from
> <math.h> is also different (at least on OSX). I'm just hoping that these
> precision "fluctuations" of the value of PI won't hurt in sensitive areas
> like graphics.

That's a good point. In this case, we can throw this patch at automated testing before pushing it. If I had to guess, I'm pretty sure it'll be fine, but we'll see for sure once the tests are done:

https://tbpl.mozilla.org/?tree=Try&rev=4e3141b1d679

If it looks ok, I'll r+ it and push it.

Comment 14

[Doug Sherk (:drs) (inactive)]

Ok, it looks like this broke something in SVG, on Android R3. I'd be fine with not making this code use M_PI, but if you want, you can look into it some more.

Comment 15

[Sergey Glushchenko]

Attachment: A few more clean-ups.

It's amazing how such a tiny thing can cause so much trouble.

When I first looked at this bug, I thought why not simply use M_PI from <math.h>. After some research I found out that the standard does not require definition of M_PI (and some people say that this definition is even discouraged by the standard), so portable code should not rely on it. However, it seems like most (if not all) libc's actually define it.

My guess w.r.t. failed SVG test would be that radPerDegree is initialized with different values in DOMSVGMatrix.cpp and SVGTransform.cpp. I've added a little hack to #undef M_PI in DOMSVGMatrix.cpp before including mfbt/Constants.h. Even if this won't get into the codebase, it'd still be interesting to see if it solves the issue, so please throw this patch at automated testing again. Thank you.

Comment 16

[Chris Peterson [:cpeterson]]

(In reply to Sergey Glushchenko from comment #15)
> When I first looked at this bug, I thought why not simply use M_PI from
> <math.h>. After some research I found out that the standard does not require
> definition of M_PI (and some people say that this definition is even
> discouraged by the standard), so portable code should not rely on it.
> However, it seems like most (if not all) libc's actually define it.

It also seems that M_PI's precision varies by platform. Android bionic and Linux glibc use:

  #define M_PI    3.14159265358979323846

And OS X uses: 

  #define M_PI    3.14159265358979323846264338327950288

Both definitions differ from the pi value that DOMSVGMatrix.cpp had been using:

  -const double radPerDegree = 2.0*3.1415926535 / 360.0;
  +const double radPerDegree = 2.0*M_PI / 360.0;

Comment 17

[Sergey Glushchenko]

My guess is that in case of DOMSVGMatrix.cpp and DOMSVGMatrix.cpp it's not the precision that matters, but the consistency of the M_PI value across these files.

Comment 18

[Sergey Glushchenko]

DOMSVGMatrix.cpp and SVGTransform.cpp, sorry.

Comment 19

[Doug Sherk (:drs) (inactive)]

(In reply to Sergey Glushchenko from comment #15)
> Created attachment 650341 [details] [diff] [review]
> A few more clean-ups.
> 
> It's amazing how such a tiny thing can cause so much trouble.
> 
> When I first looked at this bug, I thought why not simply use M_PI from
> <math.h>. After some research I found out that the standard does not require
> definition of M_PI (and some people say that this definition is even
> discouraged by the standard), so portable code should not rely on it.
> However, it seems like most (if not all) libc's actually define it.
> 
> My guess w.r.t. failed SVG test would be that radPerDegree is initialized
> with different values in DOMSVGMatrix.cpp and SVGTransform.cpp. I've added a
> little hack to #undef M_PI in DOMSVGMatrix.cpp before including
> mfbt/Constants.h. Even if this won't get into the codebase, it'd still be
> interesting to see if it solves the issue, so please throw this patch at
> automated testing again. Thank you.

I'm not comfortable landing patches that require undef'ing (especially when the purpose of the patch is to improve code organization and clarify), but having said that I pushed this to try to see what happens:

https://tbpl.mozilla.org/?tree=Try&rev=55bc8fc4c984

Comment 20

[Doug Sherk (:drs) (inactive)]

(In reply to Sergey Glushchenko from comment #17)
> My guess is that in case of DOMSVGMatrix.cpp and DOMSVGMatrix.cpp it's not
> the precision that matters, but the consistency of the M_PI value across
> these files.

We usually fuzz checks in the case of rounding errors between platforms (especially in the case of hardware rendering) so I'm surprised that a difference this small actually caused a test failure.

Comment 21

[Doug Sherk (:drs) (inactive)]

Looks like it failed again.

Comment 22

[Sergey Glushchenko]

Interesting. Do you have a scheduled daily test runs? It would be interesting to see the results without the patch.

Comment 23

[Doug Sherk (:drs) (inactive)]

We have automated tests whenever someone pushes something to mozilla-central:
https://tbpl.mozilla.org/

In addition, people can fire off their own tests ad-hoc, like I've done in this bug. These go to a tree called "try":
https://tbpl.mozilla.org/?tree=Try

We get things called random failures, which can manifest themselves as oranges, reds, etc. For example, if you look at this push:
https://tbpl.mozilla.org/?rev=257777cf58fe

You can see Linux64 debug failed, and philor marked it as orangefactor:
"[philor - Wed Aug 8 20:42:23 2012 PDT]
Bug 777875 [orangefactor]"

That means that this push didn't cause that problem, but some glitch on the computer it was running on or a different way of rounding things (for example) caused it.

I went back and starred our previous push to try, which is unusual and nobody really does it since the parent of every push is completely different, but this will help you see what you caused. If we pushed this patch, the tree might look something like this, and you would be backed out due to the R3 failures.

You'll notice that Windows builds all burned; that's because I used an, unknown to me, broken mozilla-inbound (an unstable version of mozilla-central you can find at https://tbpl.mozilla.org/?tree=Mozilla-Inbound) as my parent, since it was the only tree I had kicking around unused. I think Linux opt Cipc was also broken at the time.

tl;dr You really want to be looking at the Android R3 failures; the rest are noise. :) I hope that helps.

Comment 24

[Jeff Walden [:Waldo]]

I'm a bit surprised M_PI isn't actually a standard thing that we can rely on.  Madness.  :-(

mfbt convention is to avoid using macros except when absolutely necessary, so it would be better to have static const double mozilla::Pi or somesuch, ideally.  Is that possible?  I think you're only touching C++ files right now, so there's no C dependency that would mandate a macro.

Also, ordinarily mfbt macros should be MOZ_-prefixed.  But this might be an exception to that rule; I'm not sure offhand.

Also you should run mfbt patches past an mfbt peer; we try to run a pretty tight ship to maximize consistency, understandability, and so on.  Feel free to r? me as needed here, once the latest patch moves forward.

Comment 25

[Raymond Heldt]

Attachment: Simple fix - hardcoded to 20 decimal places everywhere

Perhaps this wasn't the most efficient way to do this bug, but it seemed like a rather small amount of code to define into a new header file "PiValue.h" and then include that file in every file that uses pi's value. Anyway, I think the main reason for keeping code in one place is so if it has to be changed later on, it only has to be changed once. But pi is a constant value (albeit irrational and never-ending), so I figured there shouldn't be concerns about having to change it to say, 3.18 or something in the future, so the value is hard-coded into each file instead.

Comment 26

[Raymond Heldt]

Attachment: Fixed mistake in previous patch

Okay, THIS would be the final version of this patch. I left out the last few digits in libtremor/lib/os.h

Comment 27

[Doug Sherk (:drs) (inactive)]

(In reply to Raymond Heldt from comment #26)
> Perhaps this wasn't the most efficient way to do this bug, but it seemed like a 
> rather small amount of code to define into a new header file "PiValue.h" and then 
> include that file in every file that uses pi's value.

We put M_PI in a file called "Constants.h", not "PiValue.h", so we can add more constants in the future if we need them.

> But pi is a constant value (albeit irrational and never-ending), so I figured there 
> shouldn't be concerns about having to change it to say, 3.18 or something in the 
> future, so the value is hard-coded into each file instead.

Actually, that would be pretty bad and could screw up reftests depending on a tighter range than that would allow.

> Okay, THIS would be the final version of this patch. I left out the last few digits 
> in libtremor/lib/os.h

We don't edit libraries, which is all this patch hits. There's a specific process for patching each library. If we patch them willy-nilly, the patches will be lifted when the libraries are upgraded. Also note that you're replacing a bunch of floats with (implicit) doubles, which could have unintended consequences.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 702485 [details] [diff] [review]
Fixed mistake in previous patch

feedback- based on the feedback that Doug already gave in the previous two comments. I don't know what more is expected in this bug.

Doug, are we happy with the changes already in this bug? Do we need to keep it open for those other two cases that Sergey was working on? This bug has been in [leave open] long enough that I think it would be better to mark this bug as RESO-FIXED and either file a followup for the remaining two or just leave them be based on the troubles that have been seen when changing those two values.

Comment 29

[Doug Sherk (:drs) (inactive)]

(In reply to Jared Wein [:jaws] from comment #28)
> Doug, are we happy with the changes already in this bug? Do we need to keep
> it open for those other two cases that Sergey was working on? This bug has
> been in [leave open] long enough that I think it would be better to mark
> this bug as RESO-FIXED and either file a followup for the remaining two or
> just leave them be based on the troubles that have been seen when changing
> those two values.

Agreed, I'll close it.

Comment 30

[Doug Sherk (:drs) (inactive)]

Comment on attachment 650341 [details] [diff] [review]
A few more clean-ups.

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

It seems like we don't want to do this, based on test results and overall gains.