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>

include/private/SkPathRef.h

@@ -577,6 +577,7 @@ private:
     friend class PathRefTest_Private;
     friend class ForceIsRRect_Private; // unit test isRRect
     friend class SkPath;
+    friend class SkPathPriv;
 };
 
 #endif

src/core/SkPath.cpp

@@ -1785,6 +1785,15 @@ static void subdivide_cubic_to(SkPath* path, const SkPoint pts[4],
 }
 
 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 @@ void SkPath::transform(const SkMatrix& matrix, SkPath* dst) const {
         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 @@ void SkPath::transform(const SkMatrix& matrix, SkPath* dst) const {
             } 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;
             }
         }

src/core/SkPathPriv.h

@@ -260,6 +260,11 @@ public:
         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

tests/PathTest.cpp

@@ -5204,3 +5204,110 @@ DEF_TEST(Path_increserve_handle_neg_crbug_883666, r) {
     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; });
+}
+