Fix handling of radii scaling to force the result to always be less

src/core/SkRRect.cpp

 /*
  * Copyright 2012 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
+#include <cmath>
 #include "SkRRect.h"
 #include "SkMatrix.h"
 
 ///////////////////////////////////////////////////////////////////////////////
 
 void SkRRect::setRectXY(const SkRect& rect, SkScalar xRad, SkScalar yRad) {
     fRect = rect;
     fRect.sort();
 
     if (fRect.isEmpty() || !fRect.isFinite()) {
@@ skipping 84 matching lines @@
     }
 
     fRadii[kUpperLeft_Corner].set(leftRad, topRad);
     fRadii[kUpperRight_Corner].set(rightRad, topRad);
     fRadii[kLowerRight_Corner].set(rightRad, bottomRad);
     fRadii[kLowerLeft_Corner].set(leftRad, bottomRad);
 
     SkDEBUGCODE(this->validate();)
 }
 
-/*
- *  TODO: clean this guy up and possibly add to SkScalar.h
- */
-static inline SkScalar SkScalarDecULP(SkScalar value) {
-#if SK_SCALAR_IS_FLOAT
-        return SkBits2Float(SkFloat2Bits(value) - 1);
-#else
-    #error "need impl for doubles"
-#endif
-}
-
- /**
- *  We need all combinations of predicates to be true to have a "safe" radius value.
- */
-static SkScalar clamp_radius_check_predicates(SkScalar rad, SkScalar min, SkScalar max) {
-    SkASSERT(min < max);
-    if (rad > max - min || min + rad > max || max - rad < min) {
-        rad = SkScalarDecULP(rad);
-    }
-    return rad;
-}
-
 // These parameters intentionally double. Apropos crbug.com/463920, if one of the
 // radii is huge while the other is small, single precision math can completely
 // miss the fact that a scale is required.
 static double compute_min_scale(double rad1, double rad2, double limit, double curMin) {
     if ((rad1 + rad2) > limit) {
         return SkTMin(curMin, limit / (rad1 + rad2));
     }
     return curMin;
 }
 
+// This code assumes that a and b fit in in a float, and therefore the resulting smaller value of
+// a and b will fit in a float. The side of the rectangle may be larger than a float.
+static void adjust_radii(double limit, double scale, float* a, float* b) {
+    SkASSERT(scale < 1.0 && scale > 0.0);
+    // This check is conservative. (double)*a + (double)*b >= (double)(*a + *b)
+    if ((double)*a + (double)*b > limit) {

[robertphillips, 2016/01/07 17:56:41]

minRadius & maxRadius ?

[herb_g, 2016/01/07 18:45:32]

Done.

+        float* min_radius = a;
+        float* max_radius = b;
+        // force min_radius to be the smaller of the two.
+        if (*min_radius > *max_radius) {
+            SkTSwap(min_radius, max_radius);
+        }
+        // new_min_radius must be float in order to give the actual value of the radius.

[robertphillips, 2016/01/07 17:56:41]

newMinRadius ?

[herb_g, 2016/01/07 18:45:32]

Done.

+        float new_min_radius = *min_radius * scale;

[robertphillips, 2016/01/07 17:56:41]

Can it ever happen that newMinRadius is larger than limit ?

[herb_g, 2016/01/07 18:45:32]

Done.

+        *min_radius = new_min_radius;
+        // Because new_max_radius is the result of a double to float conversion, it can be larger
+        // than limit, but only by one ULP.

[robertphillips, 2016/01/07 17:56:41]

newMaxRadius ?

[herb_g, 2016/01/07 18:45:32]

Done.

+        float new_max_radius = (float)(limit - new_min_radius);
+        // If new_max_radius is larger than the same value as a double, then it needs to be
+        // reduced by one ULP to be less than limit - new_min_radius.
+        // Note: nexttowardf is a c99 call and should be std::nexttoward, but this is not
+        // implemented in the ARM compiler.
+        if (new_max_radius > limit - new_min_radius) {
+            new_max_radius = nexttowardf(new_max_radius, limit - new_min_radius);
+        }
+        // This handles the case where both sets of radii are larger than a side by differing
+        // scale factors. The one that needs the smaller scale factor will produce short enough
+        // radii in the other side just using the scale factor.
+        *max_radius = SkMinScalar(scale * *max_radius, new_max_radius);
+    } else {
+        *a *= scale;
+        *b *= scale;
+    }

[robertphillips, 2016/01/07 17:56:41]

SkASSERT(*a >= 0.0f && *b >= 0.0f); ?

[herb_g, 2016/01/07 18:45:32]

Done.

+    SkASSERT((*a + *b) <= limit);
+}
+
 void SkRRect::setRectRadii(const SkRect& rect, const SkVector radii[4]) {
     fRect = rect;
     fRect.sort();
 
     if (fRect.isEmpty() || !fRect.isFinite()) {
         this->setEmpty();
         return;
     }
 
     if (!SkScalarsAreFinite(&radii[0].fX, 8)) {
@@ skipping 29 matching lines @@
     // that to scale down _all_ the radii. This algorithm is from the
     // W3 spec (http://www.w3.org/TR/css3-background/) section 5.5 - Overlapping
     // Curves:
     // "Let f = min(Li/Si), where i is one of { top, right, bottom, left },
     //   Si is the sum of the two corresponding radii of the corners on side i,
     //   and Ltop = Lbottom = the width of the box,
     //   and Lleft = Lright = the height of the box.
     // If f < 1, then all corner radii are reduced by multiplying them by f."
     double scale = 1.0;
 
-    scale = compute_min_scale(fRadii[0].fX, fRadii[1].fX, fRect.width(),  scale);
-    scale = compute_min_scale(fRadii[1].fY, fRadii[2].fY, fRect.height(), scale);
-    scale = compute_min_scale(fRadii[2].fX, fRadii[3].fX, fRect.width(),  scale);
-    scale = compute_min_scale(fRadii[3].fY, fRadii[0].fY, fRect.height(), scale);
+    // The sides of the rectangle may be larger than a float.
+    double width = (double)fRect.fRight - (double)fRect.fLeft;
+    double height = (double)fRect.fBottom - (double)fRect.fTop;
+    scale = compute_min_scale(fRadii[0].fX, fRadii[1].fX, width,  scale);
+    scale = compute_min_scale(fRadii[1].fY, fRadii[2].fY, height, scale);
+    scale = compute_min_scale(fRadii[2].fX, fRadii[3].fX, width,  scale);
+    scale = compute_min_scale(fRadii[3].fY, fRadii[0].fY, height, scale);
 
     if (scale < 1.0) {
-        for (int i = 0; i < 4; ++i) {
-            fRadii[i].fX *= scale;
-            fRadii[i].fY *= scale;
-        }
+        adjust_radii(width,  scale, &fRadii[0].fX, &fRadii[1].fX);
+        adjust_radii(height, scale, &fRadii[1].fY, &fRadii[2].fY);
+        adjust_radii(width,  scale, &fRadii[2].fX, &fRadii[3].fX);
+        adjust_radii(height, scale, &fRadii[3].fY, &fRadii[0].fY);
     }
 
-    // https://bug.skia.org/3239 -- its possible that we can hit the following inconsistency:
-    //     rad == bounds.bottom - bounds.top
-    //     bounds.bottom - radius < bounds.top
-    //     YIKES
-    // We need to detect and "fix" this now, otherwise we can have the following wackiness:
-    //     path.addRRect(rrect);
-    //     rrect.rect() != path.getBounds()
-    for (int i = 0; i < 4; ++i) {
-        fRadii[i].fX = clamp_radius_check_predicates(fRadii[i].fX, fRect.fLeft, fRect.fRight);
-        fRadii[i].fY = clamp_radius_check_predicates(fRadii[i].fY, fRect.fTop, fRect.fBottom);
-    }
     // At this point we're either oval, simple, or complex (not empty or rect).
     this->computeType();
 
     SkDEBUGCODE(this->validate();)
 }
 
 // This method determines if a point known to be inside the RRect's bounds is
 // inside all the corners.
 bool SkRRect::checkCornerContainment(SkScalar x, SkScalar y) const {
     SkPoint canonicalPt; // (x,y) translated to one of the quadrants
@@ skipping 366 matching lines @@
     }
 
     for (int i = 0; i < 4; ++i) {
         validate_radius_check_predicates(fRadii[i].fX, fRect.fLeft, fRect.fRight);
         validate_radius_check_predicates(fRadii[i].fY, fRect.fTop, fRect.fBottom);
     }
 }
 #endif // SK_DEBUG
 
 ///////////////////////////////////////////////////////////////////////////////