change setLength and Normalize to handle when mag2 overflows a float, but the

actual lenght does not.

R=caryclark@google.com

Review URL: https://codereview.chromium.org/14838006

git-svn-id: http://skia.googlecode.com/svn/trunk@8988 2bbb7eff-a529-9590-31e7-b0007b416f81

diff --git a/src/core/SkPoint.cpp b/src/core/SkPoint.cpp
index ebbe3a4..bf3affa 100644 (file)
--- a/src/core/SkPoint.cpp
+++ b/src/core/SkPoint.cpp
@@ -107,30 +107,74 @@ static inline bool isLengthNearlyZero(float dx, float dy,
 }
 
 SkScalar SkPoint::Normalize(SkPoint* pt) {
+    float x = pt->fX;
+    float y = pt->fY;
     float mag2;
-    if (!isLengthNearlyZero(pt->fX, pt->fY, &mag2)) {
-        float mag = sk_float_sqrt(mag2);
-        float scale = 1.0f / mag;
-        pt->fX = pt->fX * scale;
-        pt->fY = pt->fY * scale;
-        return mag;
+    if (isLengthNearlyZero(x, y, &mag2)) {
+        return 0;
     }
-    return 0;
+
+    float mag, scale;
+    if (SkScalarIsFinite(mag2)) {
+        mag = sk_float_sqrt(mag2);
+        scale = 1 / mag;
+    } else {
+        // our mag2 step overflowed to infinity, so use doubles instead.
+        // much slower, but needed when x or y are very large, other wise we
+        // divide by inf. and return (0,0) vector.
+        double xx = x;
+        double yy = y;
+        double magmag = sqrt(xx * xx + yy * yy);
+        mag = (float)magmag;
+        // we perform the divide with the double magmag, to stay exactly the
+        // same as setLength. It would be faster to perform the divide with
+        // mag, but it is possible that mag has overflowed to inf. but still
+        // have a non-zero value for scale (thanks to denormalized numbers).
+        scale = (float)(1 / magmag);
+    }
+    pt->set(x * scale, y * scale);
+    return mag;
 }
 
 SkScalar SkPoint::Length(SkScalar dx, SkScalar dy) {
-    return sk_float_sqrt(getLengthSquared(dx, dy));
+    float mag2 = dx * dx + dy * dy;
+    if (SkScalarIsFinite(mag2)) {
+        return sk_float_sqrt(mag2);
+    } else {
+        double xx = dx;
+        double yy = dy;
+        return (float)sqrt(xx * xx + yy * yy);
+    }
 }
 
+/*
+ *  We have to worry about 2 tricky conditions:
+ *  1. underflow of mag2 (compared against nearlyzero^2)
+ *  2. overflow of mag2 (compared w/ isfinite)
+ *
+ *  If we underflow, we return false. If we overflow, we compute again using
+ *  doubles, which is much slower (3x in a desktop test) but will not overflow.
+ */
 bool SkPoint::setLength(float x, float y, float length) {
     float mag2;
-    if (!isLengthNearlyZero(x, y, &mag2)) {
-        float scale = length / sk_float_sqrt(mag2);
-        fX = x * scale;
-        fY = y * scale;
-        return true;
+    if (isLengthNearlyZero(x, y, &mag2)) {
+        return false;
     }
-    return false;
+
+    float scale;
+    if (SkScalarIsFinite(mag2)) {
+        scale = length / sk_float_sqrt(mag2);
+    } else {
+        // our mag2 step overflowed to infinity, so use doubles instead.
+        // much slower, but needed when x or y are very large, other wise we
+        // divide by inf. and return (0,0) vector.
+        double xx = x;
+        double yy = y;
+        scale = (float)(length / sqrt(xx * xx + yy * yy));
+    }
+    fX = x * scale;
+    fY = y * scale;
+    return true;
 }
 
 #else

diff --git a/tests/PointTest.cpp b/tests/PointTest.cpp
index b8f4398..9d4bdfd 100644 (file)
--- a/tests/PointTest.cpp
+++ b/tests/PointTest.cpp
@@ -22,6 +22,18 @@ static void test_casts(skiatest::Reporter* reporter) {
     REPORTER_ASSERT(reporter, r.asScalars() == rPtr);
 }
 
+// Tests SkPoint::Normalize() for this (x,y)
+static void test_Normalize(skiatest::Reporter* reporter,
+                           SkScalar x, SkScalar y) {
+    SkPoint point;
+    point.set(x, y);
+    SkScalar oldLength = point.length();
+    SkScalar returned = SkPoint::Normalize(&point);
+    SkScalar newLength = point.length();
+    REPORTER_ASSERT(reporter, SkScalarNearlyEqual(returned, oldLength));
+    REPORTER_ASSERT(reporter, SkScalarNearlyEqual(newLength, SK_Scalar1));
+}
+
 // Tests that SkPoint::length() and SkPoint::Length() both return
 // approximately expectedLength for this (x,y).
 static void test_length(skiatest::Reporter* reporter, SkScalar x, SkScalar y,
@@ -34,28 +46,57 @@ static void test_length(skiatest::Reporter* reporter, SkScalar x, SkScalar y,
     //See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=323
     REPORTER_ASSERT(reporter, SkScalarNearlyEqual(s1, s2));
     REPORTER_ASSERT(reporter, SkScalarNearlyEqual(s1, expectedLength));
+    
+    test_Normalize(reporter, x, y);
 }
 
-// Tests SkPoint::Normalize() for this (x,y)
-static void test_Normalize(skiatest::Reporter* reporter,
-                           SkScalar x, SkScalar y) {
-    SkPoint point;
-    point.set(x, y);
-    SkScalar oldLength = point.length();
-    SkScalar returned = SkPoint::Normalize(&point);
-    SkScalar newLength = point.length();
-    REPORTER_ASSERT(reporter, SkScalarNearlyEqual(returned, oldLength));
-    REPORTER_ASSERT(reporter, SkScalarNearlyEqual(newLength, SK_Scalar1));
+// test that we handle very large values correctly. i.e. that we can
+// successfully normalize something whose mag overflows a float.
+static void test_overflow(skiatest::Reporter* reporter) {
+    SkPoint pt = { SkFloatToScalar(3.4e38f), SkFloatToScalar(3.4e38f) };
+    
+    SkScalar length = pt.length();
+    REPORTER_ASSERT(reporter, !SkScalarIsFinite(length));
+
+    // this should succeed, even though we can't represent length
+    REPORTER_ASSERT(reporter, pt.setLength(SK_Scalar1));
+
+    // now that pt is normalized, we check its length
+    length = pt.length();
+    REPORTER_ASSERT(reporter, SkScalarNearlyEqual(length, SK_Scalar1));
+}
+
+// test that we handle very small values correctly. i.e. that we can
+// report failure if we try to normalize them.
+static void test_underflow(skiatest::Reporter* reporter) {
+    SkPoint pt = { SkFloatToScalar(1.0e-37f), SkFloatToScalar(1.0e-37f) };
+    SkPoint copy = pt;
+
+    REPORTER_ASSERT(reporter, 0 == SkPoint::Normalize(&pt));
+    REPORTER_ASSERT(reporter, pt == copy);  // pt is unchanged
+
+    REPORTER_ASSERT(reporter, !pt.setLength(SK_Scalar1));
+    REPORTER_ASSERT(reporter, pt == copy);  // pt is unchanged
 }
 
 static void PointTest(skiatest::Reporter* reporter) {
     test_casts(reporter);
 
-    test_length(reporter, SkIntToScalar(3), SkIntToScalar(4), SkIntToScalar(5));
-    test_length(reporter, SkFloatToScalar(0.6f), SkFloatToScalar(0.8f),
-                SK_Scalar1);
-    test_Normalize(reporter, SkIntToScalar(3), SkIntToScalar(4));
-    test_Normalize(reporter, SkFloatToScalar(0.6f), SkFloatToScalar(0.8f));
+    static const struct {
+        SkScalar fX;
+        SkScalar fY;
+        SkScalar fLength;
+    } gRec[] = {
+        { SkIntToScalar(3), SkIntToScalar(4), SkIntToScalar(5) },
+        { SkFloatToScalar(0.6f), SkFloatToScalar(0.8f), SK_Scalar1 },
+    };
+    
+    for (size_t i = 0; i < SK_ARRAY_COUNT(gRec); ++i) {
+        test_length(reporter, gRec[i].fX, gRec[i].fY, gRec[i].fLength);
+    }
+
+    test_underflow(reporter);
+    test_overflow(reporter);
 }
 
 #include "TestClassDef.h"