Fix handling of radii scaling to force the result to always be less
than a given side.
BUG=472147
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1567723004
Review URL: https://codereview.chromium.org/1567723004
diff --git a/src/core/SkRRect.cpp b/src/core/SkRRect.cpp
index ad62e5b..c8b3a6b 100644
--- a/src/core/SkRRect.cpp
+++ b/src/core/SkRRect.cpp
@@ -5,6 +5,7 @@
* found in the LICENSE file.
*/
+#include <cmath>
#include "SkRRect.h"
#include "SkMatrix.h"
@@ -109,28 +110,6 @@
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.
@@ -141,6 +120,48 @@
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.
+// Scale must be less than or equal to the ratio limit / (*a + *b).
+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) {
+ float* minRadius = a;
+ float* maxRadius = b;
+ // Force minRadius to be the smaller of the two.
+ if (*minRadius > *maxRadius) {
+ SkTSwap(minRadius, maxRadius);
+ }
+ // newMinRadius must be float in order to give the actual value of the radius.
+ // The newMinRadius will always be smaller than limit. The largest that minRadius can be
+ // is 1/2 the ratio of minRadius : (minRadius + maxRadius), therefore in the resulting
+ // division, minRadius can be no larger than 1/2 limit + ULP.
+ float newMinRadius = *minRadius * scale;
+ *minRadius = newMinRadius;
+ // Because newMaxRadius is the result of a double to float conversion, it can be larger
+ // than limit, but only by one ULP.
+ float newMaxRadius = (float)(limit - newMinRadius);
+ // If newMaxRadius is larger than the same value as a double, then it needs to be
+ // reduced by one ULP to be less than limit - newMinRadius.
+ // Note: nexttowardf is a c99 call and should be std::nexttoward, but this is not
+ // implemented in the ARM compiler.
+ if (newMaxRadius > limit - newMinRadius) {
+ newMaxRadius = nexttowardf(newMaxRadius, limit - newMinRadius);
+ }
+ // This handles the case where both sets of radii are larger than a side by differing
+ // scale factors. The one that needs the larger scale factor (the radii with less
+ // overlap) will produce radii that are short enough just using the smaller scale factor
+ // from the side where the radii overlap is larger.
+ *maxRadius = SkMinScalar(scale * *maxRadius, newMaxRadius);
+ } else {
+ *a *= scale;
+ *b *= scale;
+ }
+ SkASSERT(*a >= 0.0f && *b >= 0.0f);
+ SkASSERT((*a + *b) <= limit);
+}
+
void SkRRect::setRectRadii(const SkRect& rect, const SkVector radii[4]) {
fRect = rect;
fRect.sort();
@@ -190,29 +211,21 @@
// 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();
diff --git a/tests/DrawPathTest.cpp b/tests/DrawPathTest.cpp
index 364a297..e9aa449 100644
--- a/tests/DrawPathTest.cpp
+++ b/tests/DrawPathTest.cpp
@@ -313,6 +313,39 @@
REPORTER_ASSERT(reporter, filteredPath.isEmpty());
}
+// http://crbug.com/472147
+// This is a simplified version from the bug. RRect radii not properly scaled.
+static void test_crbug_472147_simple(skiatest::Reporter* reporter) {
+ SkAutoTUnref<SkSurface> surface(SkSurface::NewRasterN32Premul(1000, 1000));
+ SkCanvas* canvas = surface->getCanvas();
+ SkPaint p;
+ SkRect r = SkRect::MakeLTRB(-246.0f, 33.0f, 848.0f, 33554464.0f);
+ SkVector radii[4] = {
+ { 13.0f, 8.0f }, { 170.0f, 2.0 }, { 256.0f, 33554430.0f }, { 120.0f, 5.0f }
+ };
+ SkRRect rr;
+ rr.setRectRadii(r, radii);
+ canvas->drawRRect(rr, p);
+}
+
+// http://crbug.com/472147
+// RRect radii not properly scaled.
+static void test_crbug_472147_actual(skiatest::Reporter* reporter) {
+ SkAutoTUnref<SkSurface> surface(SkSurface::NewRasterN32Premul(1000, 1000));
+ SkCanvas* canvas = surface->getCanvas();
+ SkPaint p;
+ SkRect r = SkRect::MakeLTRB(-246.0f, 33.0f, 848.0f, 33554464.0f);
+ SkVector radii[4] = {
+ { 13.0f, 8.0f }, { 170.0f, 2.0 }, { 256.0f, 33554430.0f }, { 120.0f, 5.0f }
+ };
+ SkRRect rr;
+ rr.setRectRadii(r, radii);
+ canvas->clipRRect(rr, SkRegion::kIntersect_Op, false);
+
+ SkRect r2 = SkRect::MakeLTRB(0, 33, 1102, 33554464);
+ canvas->drawRect(r2, p);
+}
+
DEF_TEST(DrawPath, reporter) {
test_giantaa();
test_bug533();
@@ -325,6 +358,8 @@
if (false) test_crbug131181();
test_infinite_dash(reporter);
test_crbug_165432(reporter);
+ test_crbug_472147_simple(reporter);
+ test_crbug_472147_actual(reporter);
test_big_aa_rect(reporter);
test_halfway();
}