From 7cf12ddb8c1c2cf7a0eec63439148cc6b2bc104a Mon Sep 17 00:00:00 2001 From: herb Date: Mon, 11 Jan 2016 08:08:56 -0800 Subject: [PATCH] Fix radii calculation code to handle large radii. BUG=472147 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1569403002 Review URL: https://codereview.chromium.org/1569403002 --- src/core/SkRRect.cpp | 54 ++++++++------------------------- src/core/SkScaleToSides.h | 61 ++++++++++++++++++++++++++++++++++++++ tests/DrawPathTest.cpp | 35 ++++++++++++++++++++++ tests/ScaleToSidesTest.cpp | 49 ++++++++++++++++++++++++++++++ 4 files changed, 158 insertions(+), 41 deletions(-) create mode 100644 src/core/SkScaleToSides.h create mode 100644 tests/ScaleToSidesTest.cpp diff --git a/src/core/SkRRect.cpp b/src/core/SkRRect.cpp index ad62e5bbae..ca4fd56152 100644 --- a/src/core/SkRRect.cpp +++ b/src/core/SkRRect.cpp @@ -5,8 +5,10 @@ * found in the LICENSE file. */ +#include #include "SkRRect.h" #include "SkMatrix.h" +#include "SkScaleToSides.h" /////////////////////////////////////////////////////////////////////////////// @@ -109,28 +111,6 @@ void SkRRect::setNinePatch(const SkRect& rect, SkScalar leftRad, SkScalar topRad 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. @@ -190,29 +170,21 @@ void SkRRect::setRectRadii(const SkRect& rect, const SkVector radii[4]) { // 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; - } + ScaleToSides::AdjustRadii(width, scale, &fRadii[0].fX, &fRadii[1].fX); + ScaleToSides::AdjustRadii(height, scale, &fRadii[1].fY, &fRadii[2].fY); + ScaleToSides::AdjustRadii(width, scale, &fRadii[2].fX, &fRadii[3].fX); + ScaleToSides::AdjustRadii(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/src/core/SkScaleToSides.h b/src/core/SkScaleToSides.h new file mode 100644 index 0000000000..77637a3ac2 --- /dev/null +++ b/src/core/SkScaleToSides.h @@ -0,0 +1,61 @@ +/* + * Copyright 2016 Google Inc. + * + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +#ifndef SkScaleToSides_DEFINED +#define SkScaleToSides_DEFINED + +#include +#include "SkScalar.h" +#include "SkTypes.h" + +class ScaleToSides { +public: + // 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). + // This code assumes that NaN and Inf are never passed in. + static void AdjustRadii(double limit, double scale, SkScalar* a, SkScalar* b) { + SkASSERTF(scale < 1.0 && scale > 0.0, "scale: %g", scale); + + *a = (float)((double)*a * scale); + *b = (float)((double)*b * scale); + + // 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; + + // 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 forces the total over the limit, 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 ((double)newMaxRadius + (double)newMinRadius > limit) { + newMaxRadius = nexttowardf(newMaxRadius, 0.0); + } + *maxRadius = newMaxRadius; + } + + SkASSERTF(*a >= 0.0f && *b >= 0.0f, "a: %g, b: %g", *a, *b); + SkASSERTF((*a + *b) <= limit, "limit: %g, a: %g, b: %g", limit, *a, *b); + } +}; +#endif // ScaleToSides_DEFINED diff --git a/tests/DrawPathTest.cpp b/tests/DrawPathTest.cpp index 364a297123..e9aa4499d9 100644 --- a/tests/DrawPathTest.cpp +++ b/tests/DrawPathTest.cpp @@ -313,6 +313,39 @@ static void test_crbug_165432(skiatest::Reporter* reporter) { 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 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 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 @@ DEF_TEST(DrawPath, reporter) { 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(); } diff --git a/tests/ScaleToSidesTest.cpp b/tests/ScaleToSidesTest.cpp new file mode 100644 index 0000000000..60e82be529 --- /dev/null +++ b/tests/ScaleToSidesTest.cpp @@ -0,0 +1,49 @@ +/* + * Copyright 2016 Google Inc. + * + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +#include "SkScaleToSides.h" + +#include +#include "Test.h" + +DEF_TEST(ScaleToSides, reporter) { + float interestingValues[] = { + 0.0f, + 0.5f, + 1.0f, + 2.0f, + 3.0f, + 33.0f, + 33554430.0f, + 33554431.0f, + 33554464.0f, + 333333332.0f, + 333333333.0f, + 333333334.0f, + FLT_MAX, + FLT_EPSILON, + FLT_MIN + }; + + int numInterestingValues = (int)SK_ARRAY_COUNT(interestingValues); + + for (int i = 0; i < numInterestingValues; i++) { + for (int j = 0; j < numInterestingValues; j++) { + for (int k = 0; k < numInterestingValues; k++) { + float radius1 = interestingValues[i]; + float radius2 = interestingValues[j]; + float width = interestingValues[k]; + if (width > 0.0f) { + double scale = (double)width / ((double)radius1 + (double)radius2); + if (scale < 1.0) { + ScaleToSides::AdjustRadii(width, scale, &radius1, &radius2); + } + } + } + } + } +}