pin before calling acos

pin before calling acos

Adobe reports some user crashes in acos(). While the cause is
unknown, it's safe and may help stability to pin the input
in case the arguments drifted slightly outside [-1, 1].

R=reed@google.com
BUG=skia:5222
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2006653006

Committed: https://skia.googlesource.com/skia/+/93ca884879e3469b46d32c36deb7b46f2fff1c0c

[caryclark, 2016-05-25 12:12:44]

Description was changed from

==========
pin before calling acos

Adobe reports some user crashes in acos(). While the cause is
unknown, it's safe and may help stability to pin the input
in case the arguments drifted slightly outside [-1, 1].

R=reed@google.com
BUG=skia:5252
==========

to

==========
pin before calling acos

Adobe reports some user crashes in acos(). While the cause is
unknown, it's safe and may help stability to pin the input
in case the arguments drifted slightly outside [-1, 1].

R=reed@google.com
BUG=skia:5252
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2006653006
==========

[caryclark, 2016-05-25 12:13:37]

Description was changed from

==========
pin before calling acos

Adobe reports some user crashes in acos(). While the cause is
unknown, it's safe and may help stability to pin the input
in case the arguments drifted slightly outside [-1, 1].

R=reed@google.com
BUG=skia:5252
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2006653006
==========

to

==========
pin before calling acos

Adobe reports some user crashes in acos(). While the cause is
unknown, it's safe and may help stability to pin the input
in case the arguments drifted slightly outside [-1, 1].

R=reed@google.com
BUG=skia:5222
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2006653006
==========

[caryclark, 2016-05-25 12:13:50]

The CQ bit was checked by caryclark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-25 12:13:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2006653006/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2006653006/1

[commit-bot: I haz the power, 2016-05-25 12:26:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-25 12:26:49]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-25 12:26:57]

Dry run: None

[reed1, 2016-05-25 17:12:52]

lgtm w/ request for comment(s)

https://codereview.chromium.org/2006653006/diff/1/src/core/SkGeometry.cpp
File src/core/SkGeometry.cpp (right):

https://codereview.chromium.org/2006653006/diff/1/src/core/SkGeometry.cpp#new...
src/core/SkGeometry.cpp:746: SkScalar theta = SkScalarACos(SkScalarPin(R /
SkScalarSqrt(Q3), -1, 1));  // bug.skia.org/5222
Perhaps ah comment as to why we pin ...
1. the divide/root can be anything
2. the divide/root can, due to finite precisions, be slightly outside of -1...1
3. the divide/root can occasionally generate inf. or Nan.

[caryclark, 2016-05-26 16:38:53]

I'm unclear on what you would like me to do.

This data may help me unmuddle my thinking. This code:

   static SkScalar dataset[] = {
       -SK_ScalarInfinity, SK_ScalarMin, -1 - FLT_EPSILON, -1, 0, 1, 
       1 + FLT_EPSILON, SK_ScalarMax, SK_ScalarInfinity, SK_ScalarNaN
   };
   for (int index = 0; index < SK_ARRAY_COUNT(dataset); ++index) {
        SkScalar nopin = SkScalarACos(dataset[index]);
        SkScalar pin =  SkScalarACos(SkScalarPin(dataset[index], -1, 1));
        double nopin_d = acos(dataset[index]);
        double pin_d = acos(SkTPin(dataset[index], -1.f, 1.f));
        SkDebugf("%g nopin=%g pin=%g nopin_d=%g pin_d=%g\n", dataset[index],
                nopin, pin, nopin_d, pin_d);
   }

generates:

-inf nopin=-nan(ind) pin=3.14159 nopin_d=-nan(ind) pin_d=3.14159
-3.40282e+38 nopin=-nan(ind) pin=3.14159 nopin_d=-nan(ind) pin_d=3.14159
-1 nopin=-nan(ind) pin=3.14159 nopin_d=-nan(ind) pin_d=3.14159
-1 nopin=3.14159 pin=3.14159 nopin_d=3.14159 pin_d=3.14159
0 nopin=1.5708 pin=1.5708 nopin_d=1.5708 pin_d=1.5708
1 nopin=0 pin=0 nopin_d=0 pin_d=0
1 nopin=-nan(ind) pin=0 nopin_d=-nan(ind) pin_d=0
3.40282e+38 nopin=-nan(ind) pin=0 nopin_d=-nan(ind) pin_d=0
inf nopin=-nan(ind) pin=0 nopin_d=-nan(ind) pin_d=0
nan nopin=nan pin=0 nopin_d=nan pin_d=0

So: 
  - All values outside of [-1, 1] generate NaN.
  - On some platforms, intermediate calculations fed into acos/SkScalarACos
    may be nearly [-1, 1] but generate NaN instead of 3.14159
  - Pinning fixes these cases, but may inadvertently hide error cases
    (e.g., number not nearly -1 / +1).

[caryclark, 2016-05-27 12:10:59]

The CQ bit was checked by caryclark@google.com

[caryclark, 2016-05-27 12:11:00]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/2006653006/#ps20001
(title: "add comment")

[commit-bot: I haz the power, 2016-05-27 12:11:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2006653006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2006653006/20001

[commit-bot: I haz the power, 2016-05-27 12:24:39]

Description was changed from

==========
pin before calling acos

Adobe reports some user crashes in acos(). While the cause is
unknown, it's safe and may help stability to pin the input
in case the arguments drifted slightly outside [-1, 1].

R=reed@google.com
BUG=skia:5222
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2006653006
==========

to

==========
pin before calling acos

Adobe reports some user crashes in acos(). While the cause is
unknown, it's safe and may help stability to pin the input
in case the arguments drifted slightly outside [-1, 1].

R=reed@google.com
BUG=skia:5222
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2006653006

Committed:
https://skia.googlesource.com/skia/+/93ca884879e3469b46d32c36deb7b46f2fff1c0c
==========

[commit-bot: I haz the power, 2016-05-27 12:24:46]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/93ca884879e3469b46d32c36deb7b46f2fff1c0c