Fix rrects that are large enough that we lose/gain a bit when we add the radius to a bounds coordinate.
add test that triggers assert in addRRect
BUG=skia:3239
Review URL: https://codereview.chromium.org/803153003
diff --git a/include/core/SkPathRef.h b/include/core/SkPathRef.h
index 78c6cd5..ded8e3f 100644
--- a/include/core/SkPathRef.h
+++ b/include/core/SkPathRef.h
@@ -253,6 +253,8 @@
*/
uint32_t genID() const;
+ SkDEBUGCODE(void validate() const;)
+
private:
enum SerializationOffsets {
kIsFinite_SerializationShift = 25, // requires 1 bit
@@ -415,8 +417,6 @@
return reinterpret_cast<intptr_t>(fVerbs) - reinterpret_cast<intptr_t>(fPoints);
}
- SkDEBUGCODE(void validate() const;)
-
/**
* Called the first time someone calls CreateEmpty to actually create the singleton.
*/
diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp
index f8280bd..6dc056e 100644
--- a/src/core/SkPath.cpp
+++ b/src/core/SkPath.cpp
@@ -1118,6 +1118,7 @@
}
this->close();
}
+ SkDEBUGCODE(fPathRef->validate();)
}
bool SkPath::hasOnlyMoveTos() const {
diff --git a/src/core/SkRRect.cpp b/src/core/SkRRect.cpp
index d3a571f..58e7de7 100644
--- a/src/core/SkRRect.cpp
+++ b/src/core/SkRRect.cpp
@@ -97,6 +97,32 @@
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
+}
+
+static SkScalar clamp_radius_add(SkScalar rad, SkScalar min, SkScalar max) {
+ SkASSERT(rad <= max - min);
+ if (min + rad > max) {
+ rad = SkScalarDecULP(rad);
+ }
+ return rad;
+}
+
+static SkScalar clamp_radius_sub(SkScalar rad, SkScalar min, SkScalar max) {
+ SkASSERT(rad <= max - min);
+ if (max - rad < min) {
+ rad = SkScalarDecULP(rad);
+ }
+ return rad;
+}
void SkRRect::setRectRadii(const SkRect& rect, const SkVector radii[4]) {
if (rect.isEmpty()) {
@@ -159,11 +185,27 @@
if (scale < SK_Scalar1) {
for (int i = 0; i < 4; ++i) {
- fRadii[i].fX = SkScalarMul(fRadii[i].fX, scale);
- fRadii[i].fY = SkScalarMul(fRadii[i].fY, scale);
+ fRadii[i].fX *= scale;
+ fRadii[i].fY *= scale;
}
}
+ // skbug.com/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()
+ fRadii[0].fX = clamp_radius_add(fRadii[0].fX, rect.fLeft, rect.fRight);
+ fRadii[0].fY = clamp_radius_add(fRadii[0].fY, rect.fTop, rect.fBottom);
+ fRadii[1].fX = clamp_radius_sub(fRadii[1].fX, rect.fLeft, rect.fRight);
+ fRadii[1].fY = clamp_radius_add(fRadii[1].fY, rect.fTop, rect.fBottom);
+ fRadii[2].fX = clamp_radius_sub(fRadii[2].fX, rect.fLeft, rect.fRight);
+ fRadii[2].fY = clamp_radius_sub(fRadii[2].fY, rect.fTop, rect.fBottom);
+ fRadii[3].fX = clamp_radius_add(fRadii[3].fX, rect.fLeft, rect.fRight);
+ fRadii[3].fY = clamp_radius_sub(fRadii[3].fY, rect.fTop, rect.fBottom);
+
// At this point we're either oval, simple, or complex (not empty or rect)
// but we lazily resolve the type to avoid the work if the information
// isn't required.
diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp
index 1a74682..4bd4538 100644
--- a/tests/PathTest.cpp
+++ b/tests/PathTest.cpp
@@ -21,6 +21,41 @@
#include "SkWriter32.h"
#include "Test.h"
+static void set_radii(SkVector radii[4], int index, float rad) {
+ sk_bzero(radii, sizeof(SkVector) * 4);
+ radii[index].set(rad, rad);
+}
+
+static void test_add_rrect(skiatest::Reporter* reporter, const SkRect& bounds,
+ const SkVector radii[4]) {
+ SkRRect rrect;
+ rrect.setRectRadii(bounds, radii);
+ REPORTER_ASSERT(reporter, bounds == rrect.rect());
+
+ SkPath path;
+ // this line should not assert in the debug build (from validate)
+ path.addRRect(rrect);
+ REPORTER_ASSERT(reporter, bounds == path.getBounds());
+}
+
+static void test_skbug_3239(skiatest::Reporter* reporter) {
+ const float min = SkBits2Float(0xcb7f16c8); /* -16717512.000000 */
+ const float max = SkBits2Float(0x4b7f1c1d); /* 16718877.000000 */
+ const float big = SkBits2Float(0x4b7f1bd7); /* 16718807.000000 */
+
+ const float rad = 33436320;
+
+ const SkRect rectx = SkRect::MakeLTRB(min, min, max, big);
+ const SkRect recty = SkRect::MakeLTRB(min, min, big, max);
+
+ SkVector radii[4];
+ for (int i = 0; i < 4; ++i) {
+ set_radii(radii, i, rad);
+ test_add_rrect(reporter, rectx, radii);
+ test_add_rrect(reporter, recty, radii);
+ }
+}
+
static void make_path_crbug364224(SkPath* path) {
path->reset();
path->moveTo(3.747501373f, 2.724499941f);
@@ -3729,4 +3764,5 @@
PathRefTest_Private::TestPathRef(reporter);
test_dump(reporter);
test_path_crbugskia2820(reporter);
+ test_skbug_3239(reporter);
}