don't trust convexity after a transform
In theory, a convex shape is still convex if transformed by an affine
matrix. However, SkPath segments are specified using floats, and attributes
like collinearity can break under some transforms due to finite precision.
Computing convexity is non-trivial, so there is value in SkPath caching this
calculation. Convexity is useful, as both the CPU and GPU backends can draw
convex shapes faster than non-convex.
To balance these two (fragile float math and value of caching convexity),
this CL invalidates this cached state if the transform could change convexity.
In the general case, it is assumed that convexity could change. Special cases
where it is safe to keep the cached state after transform are:
- identity transform
- scale/translate transform if the path is known to be axis-aligned
All other combinations invalidate the cached state, forcing it to be
recomputed.
"axis-aligned" means the segments in the path are all axis-aligned, horizontal
or vertical (e.g. a rect or rrect)
Bug: 899689
Change-Id: I1381273eaff61d6b7134ae94b4f251c69991081a
Reviewed-on: https://skia-review.googlesource.com/c/173226
Commit-Queue: Ravi Mistry <rmistry@google.com>
Reviewed-by: Cary Clark <caryclark@google.com>
diff --git a/include/private/SkPathRef.h b/include/private/SkPathRef.h
index d250fc3..5678eb7 100644
--- a/include/private/SkPathRef.h
+++ b/include/private/SkPathRef.h
@@ -577,6 +577,7 @@
friend class PathRefTest_Private;
friend class ForceIsRRect_Private; // unit test isRRect
friend class SkPath;
+ friend class SkPathPriv;
};
#endif
diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp
index af8cf9e..468ea56 100644
--- a/src/core/SkPath.cpp
+++ b/src/core/SkPath.cpp
@@ -1785,6 +1785,15 @@
}
void SkPath::transform(const SkMatrix& matrix, SkPath* dst) const {
+#ifndef SK_SUPPORT_LEGACY_CACHE_CONVEXITY
+ if (matrix.isIdentity()) {
+ if (dst != nullptr && dst != this) {
+ *dst = *this;
+ }
+ return;
+ }
+#endif
+
SkDEBUGCODE(this->validate();)
if (dst == nullptr) {
dst = (SkPath*)this;
@@ -1832,15 +1841,33 @@
matrix.mapPoints(ed.points(), ed.pathRef()->countPoints());
dst->fFirstDirection = SkPathPriv::kUnknown_FirstDirection;
} else {
+#ifndef SK_SUPPORT_LEGACY_CACHE_CONVEXITY
+ Convexity convexity = fConvexity;
+#endif
+
SkPathRef::CreateTransformedCopy(&dst->fPathRef, *fPathRef.get(), matrix);
if (this != dst) {
dst->fLastMoveToIndex = fLastMoveToIndex;
dst->fFillType = fFillType;
+#ifdef SK_SUPPORT_LEGACY_CACHE_CONVEXITY
dst->fConvexity.store(fConvexity);
+#endif
dst->fIsVolatile = fIsVolatile;
}
+#ifndef SK_SUPPORT_LEGACY_CACHE_CONVEXITY
+ // Due to finite/fragile float numerics, we can't assume that a convex path remains
+ // convex after a transformation, so mark it as unknown here.
+ // However, some transformations are thought to be safe:
+ // axis-aligned values under scale/translate.
+ if (matrix.isScaleTranslate() && SkPathPriv::IsAxisAligned(*this)) {
+ dst->fConvexity = convexity;
+ } else {
+ dst->fConvexity = kUnknown_Convexity;
+ }
+#endif
+
if (SkPathPriv::kUnknown_FirstDirection == fFirstDirection) {
dst->fFirstDirection = SkPathPriv::kUnknown_FirstDirection;
} else {
@@ -1853,7 +1880,9 @@
} else if (det2x2 > 0) {
dst->fFirstDirection = fFirstDirection.load();
} else {
+#ifdef SK_SUPPORT_LEGACY_CACHE_CONVEXITY
dst->fConvexity = kUnknown_Convexity;
+#endif
dst->fFirstDirection = SkPathPriv::kUnknown_FirstDirection;
}
}
diff --git a/src/core/SkPathPriv.h b/src/core/SkPathPriv.h
index b6f6ff6..8962d0e 100644
--- a/src/core/SkPathPriv.h
+++ b/src/core/SkPathPriv.h
@@ -260,6 +260,11 @@
SkASSERT(verb < SK_ARRAY_COUNT(gPtsInVerb));
return gPtsInVerb[verb];
}
+
+ static bool IsAxisAligned(const SkPath& path) {
+ SkRect tmp;
+ return (path.fPathRef->fIsRRect | path.fPathRef->fIsOval) || path.isRect(&tmp);
+ }
};
#endif
diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp
index febfd4f..6809f15 100644
--- a/tests/PathTest.cpp
+++ b/tests/PathTest.cpp
@@ -5204,3 +5204,110 @@
shallowPath.incReserve(0xffffffff);
}
+////////////////////////////////////////////////////////////////////////////////////////////////
+
+/*
+ * For speed, we tried to preserve useful/expensive attributes about paths,
+ * - convexity, isrect, isoval, ...
+ * Axis-aligned shapes (rect, oval, rrect) should survive, including convexity if the matrix
+ * is axis-aligned (e.g. scale+translate)
+ */
+
+struct Xforms {
+ SkMatrix fIM, fTM, fSM, fRM;
+
+ Xforms() {
+ fIM.reset();
+ fTM.setTranslate(10, 20);
+ fSM.setScale(2, 3);
+ fRM.setRotate(30);
+ }
+};
+
+#ifndef SK_SUPPORT_LEGACY_CACHE_CONVEXITY
+static bool conditional_convex(const SkPath& path, bool is_convex) {
+ SkPath::Convexity c = path.getConvexityOrUnknown();
+ return is_convex ? (c == SkPath::kConvex_Convexity) : (c != SkPath::kConvex_Convexity);
+}
+#endif
+
+// expect axis-aligned shape to survive assignment, identity and scale/translate matrices
+template <typename ISA>
+void survive(SkPath* path, const Xforms& x, bool isAxisAligned, skiatest::Reporter* reporter,
+ ISA isa_proc) {
+ REPORTER_ASSERT(reporter, isa_proc(*path));
+ // force the issue (computing convexity) the first time.
+ REPORTER_ASSERT(reporter, path->getConvexity() == SkPath::kConvex_Convexity);
+
+ SkPath path2;
+
+ // a path's isa and convexity should survive assignment
+ path2 = *path;
+ REPORTER_ASSERT(reporter, isa_proc(path2));
+ REPORTER_ASSERT(reporter, path2.getConvexityOrUnknown() == SkPath::kConvex_Convexity);
+
+ // a path's isa and convexity should identity transform
+ path->transform(x.fIM, &path2);
+ path->transform(x.fIM);
+ REPORTER_ASSERT(reporter, isa_proc(path2));
+ REPORTER_ASSERT(reporter, path2.getConvexityOrUnknown() == SkPath::kConvex_Convexity);
+ REPORTER_ASSERT(reporter, isa_proc(*path));
+ REPORTER_ASSERT(reporter, path->getConvexityOrUnknown() == SkPath::kConvex_Convexity);
+
+ // a path's isa should survive translation, convexity depends on axis alignment
+ path->transform(x.fTM, &path2);
+ path->transform(x.fTM);
+ REPORTER_ASSERT(reporter, isa_proc(path2));
+ REPORTER_ASSERT(reporter, isa_proc(*path));
+#ifndef SK_SUPPORT_LEGACY_CACHE_CONVEXITY
+ REPORTER_ASSERT(reporter, conditional_convex(path2, isAxisAligned));
+ REPORTER_ASSERT(reporter, conditional_convex(*path, isAxisAligned));
+#endif
+
+ // a path's isa should survive scaling, convexity depends on axis alignment
+ path->transform(x.fSM, &path2);
+ path->transform(x.fSM);
+ REPORTER_ASSERT(reporter, isa_proc(path2));
+ REPORTER_ASSERT(reporter, isa_proc(*path));
+#ifndef SK_SUPPORT_LEGACY_CACHE_CONVEXITY
+ REPORTER_ASSERT(reporter, conditional_convex(path2, isAxisAligned));
+ REPORTER_ASSERT(reporter, conditional_convex(*path, isAxisAligned));
+#endif
+
+ // For security, post-rotation, we can't assume we're still convex. It might prove to be,
+ // in fact, still be convex, be we can't have cached that setting, hence the call to
+ // getConvexityOrUnknown() instead of getConvexity().
+ path->transform(x.fRM, &path2);
+ path->transform(x.fRM);
+ if (isAxisAligned) {
+ REPORTER_ASSERT(reporter, !isa_proc(path2));
+ REPORTER_ASSERT(reporter, !isa_proc(*path));
+ }
+#ifndef SK_SUPPORT_LEGACY_CACHE_CONVEXITY
+ REPORTER_ASSERT(reporter, path2.getConvexityOrUnknown() != SkPath::kConvex_Convexity);
+ REPORTER_ASSERT(reporter, path->getConvexityOrUnknown() != SkPath::kConvex_Convexity);
+#endif
+}
+
+DEF_TEST(Path_survive_transform, r) {
+ const Xforms x;
+
+ SkPath path;
+ path.addRect({10, 10, 40, 50});
+ survive(&path, x, true, r, [](const SkPath& p) { return p.isRect(nullptr); });
+
+ path.reset();
+ path.addOval({10, 10, 40, 50});
+ survive(&path, x, true, r, [](const SkPath& p) { return p.isOval(nullptr); });
+
+ path.reset();
+ path.addRRect(SkRRect::MakeRectXY({10, 10, 40, 50}, 5, 5));
+ survive(&path, x, true, r, [](const SkPath& p) { return p.isRRect(nullptr); });
+
+ // make a trapazoid; definitely convex, but not marked as axis-aligned (e.g. oval, rrect)
+ path.reset();
+ path.moveTo(0, 0).lineTo(100, 0).lineTo(70, 100).lineTo(30, 100);
+ REPORTER_ASSERT(r, path.getConvexity() == SkPath::kConvex_Convexity);
+ survive(&path, x, false, r, [](const SkPath& p) { return true; });
+}
+
