From 48f106501cd2ce75aeba17907051160528a53f8f Mon Sep 17 00:00:00 2001 From: Michael Ludwig Date: Thu, 22 Apr 2021 09:34:45 -0400 Subject: [PATCH] Reland "Stop using copying SkPath::Iter for convexity and contains checks" This reverts commit 29c06bc82a6f650715e069ed84d5ae26bb036caf. Reason for revert: Convexicator::BySign did not handle count <= 3, which it previously never encountered because a path with leading moveTos would actually turn into a sequence of moveTo+close by the forceClose SkPath::Iter so it'd never actually skip anything. I updated the code so that BySign checks for count <= 3 after we've skipped leading moveTos. This means computeConvexity's logic can get a little simpler, just checking isFinite(), calling into BySign, and then going into the second pass. Previously, it skipped the first pass if pointCount <= 3 (using the pointCount before leading moveTos were skipped). Lastly, I removed SkPathPriv::IsConvex. It was the other user of BySign but it was only used in PathTest. I figured it's best to have a single source of convexity definition rather than having two code paths that both need to implement the same two-pass behavior. Original change's description: > Revert "Stop using copying SkPath::Iter for convexity and contains checks" > > This reverts commit 37527601577a0468e3d2c5be90a4e6dc2816f546. > > Reason for revert: asan failures > > Original change's description: > > Stop using copying SkPath::Iter for convexity and contains checks > > > > This also ensures that consecutive moveTos at the start and end of the > > path do not affect convexity, and updates AutoBoundsUpdate respects > > that as well. > > > > Bug: 1187385 > > Change-Id: I9d9d7ab7f268003ff12e46873d7b98d993db47fe > > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/396056 > > Commit-Queue: Michael Ludwig > > Reviewed-by: Mike Reed > > TBR=csmartdalton@google.com,reed@google.com,michaelludwig@google.com > > Change-Id: I46aaca9c709be7124fc3933f5d02f20f5d2b42ea > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: 1187385 > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/399376 > Reviewed-by: Michael Ludwig > Commit-Queue: Michael Ludwig # Not skipping CQ checks because this is a reland. Bug: 1187385 Change-Id: I21159915839911225440c2f65da9bbbd22b77ab3 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/399377 Reviewed-by: Chris Dalton Commit-Queue: Michael Ludwig --- src/core/SkPath.cpp | 227 ++++++++++++++++++------------------------ src/core/SkPathPriv.h | 23 +++-- tests/PathTest.cpp | 55 ++++++---- 3 files changed, 147 insertions(+), 158 deletions(-) diff --git a/src/core/SkPath.cpp b/src/core/SkPath.cpp index aa5be38fe2..6c83029bdd 100644 --- a/src/core/SkPath.cpp +++ b/src/core/SkPath.cpp @@ -61,7 +61,7 @@ static void joinNoEmptyChecks(SkRect* dst, const SkRect& src) { } static bool is_degenerate(const SkPath& path) { - return path.countVerbs() <= 1; + return (path.countVerbs() - SkPathPriv::LeadingMoveToCount(path)) == 0; } class SkAutoDisableDirectionCheck { @@ -291,69 +291,51 @@ bool SkPath::conservativelyContainsRect(const SkRect& rect) const { SkPoint firstPt; SkPoint prevPt; - SkPath::Iter iter(*this, true); - SkPath::Verb verb; - SkPoint pts[4]; int segmentCount = 0; SkDEBUGCODE(int moveCnt = 0;) - SkDEBUGCODE(int closeCount = 0;) - while ((verb = iter.next(pts)) != kDone_Verb) { - int nextPt = -1; - switch (verb) { - case kMove_Verb: - SkASSERT(!segmentCount && !closeCount); - SkDEBUGCODE(++moveCnt); - firstPt = prevPt = pts[0]; - break; - case kLine_Verb: - if (!SkPathPriv::AllPointsEq(pts, 2)) { - nextPt = 1; - SkASSERT(moveCnt && !closeCount); - ++segmentCount; - } - break; - case kQuad_Verb: - case kConic_Verb: - if (!SkPathPriv::AllPointsEq(pts, 3)) { - SkASSERT(moveCnt && !closeCount); - ++segmentCount; - nextPt = 2; - } - break; - case kCubic_Verb: - if (!SkPathPriv::AllPointsEq(pts, 4)) { - SkASSERT(moveCnt && !closeCount); - ++segmentCount; - nextPt = 3; - } - break; - case kClose_Verb: - SkDEBUGCODE(++closeCount;) - break; - default: - SkDEBUGFAIL("unknown verb"); - } - if (-1 != nextPt) { - if (SkPath::kConic_Verb == verb) { - SkConic orig; - orig.set(pts, iter.conicWeight()); - SkPoint quadPts[5]; - int count = orig.chopIntoQuadsPOW2(quadPts, 1); - SkASSERT_RELEASE(2 == count); + for (auto [verb, pts, weight] : SkPathPriv::Iterate(*this)) { + if (verb == SkPathVerb::kClose || (segmentCount > 0 && verb == SkPathVerb::kMove)) { + // Closing the current contour; but since convexity is a precondition, it's the only + // contour that matters. + SkASSERT(moveCnt); + segmentCount++; + break; + } else if (verb == SkPathVerb::kMove) { + // A move at the start of the contour (or multiple leading moves, in which case we + // keep the last one before a non-move verb). + SkASSERT(!segmentCount); + SkDEBUGCODE(++moveCnt); + firstPt = prevPt = pts[0]; + } else { + int pointCount = SkPathPriv::PtsInVerb((unsigned) verb); + SkASSERT(pointCount > 0); - if (!check_edge_against_rect(quadPts[0], quadPts[2], rect, direction)) { - return false; - } - if (!check_edge_against_rect(quadPts[2], quadPts[4], rect, direction)) { - return false; - } - } else { - if (!check_edge_against_rect(prevPt, pts[nextPt], rect, direction)) { - return false; + if (!SkPathPriv::AllPointsEq(pts, pointCount + 1)) { + SkASSERT(moveCnt); + int nextPt = pointCount; + segmentCount++; + + if (SkPathVerb::kConic == verb) { + SkConic orig; + orig.set(pts, *weight); + SkPoint quadPts[5]; + int count = orig.chopIntoQuadsPOW2(quadPts, 1); + SkASSERT_RELEASE(2 == count); + + if (!check_edge_against_rect(quadPts[0], quadPts[2], rect, direction)) { + return false; + } + if (!check_edge_against_rect(quadPts[2], quadPts[4], rect, direction)) { + return false; + } + } else { + if (!check_edge_against_rect(prevPt, pts[nextPt], rect, direction)) { + return false; + } } + prevPt = pts[nextPt]; } - prevPt = pts[nextPt]; } } @@ -2108,6 +2090,11 @@ struct Convexicator { } static SkPathConvexity BySign(const SkPoint points[], int count) { + if (count <= 3) { + // point, line, or triangle are always convex + return SkPathConvexity::kConvex; + } + const SkPoint* last = points + count; SkPoint currPt = *points++; SkPoint firstPt = currPt; @@ -2216,10 +2203,6 @@ private: }; SkPathConvexity SkPath::computeConvexity() const { - SkPoint pts[4]; - SkPath::Verb verb; - SkPath::Iter iter(*this, true); - auto setComputedConvexity = [=](SkPathConvexity convexity){ SkASSERT(SkPathConvexity::kUnknown != convexity); this->setConvexity(convexity); @@ -2230,70 +2213,76 @@ SkPathConvexity SkPath::computeConvexity() const { return setComputedConvexity(SkPathConvexity::kConcave); }; + if (!this->isFinite()) { + return setFail(); + } + // Check to see if path changes direction more than three times as quick concave test int pointCount = this->countPoints(); // last moveTo index may exceed point count if data comes from fuzzer (via SkImageFilter) if (0 < fLastMoveToIndex && fLastMoveToIndex < pointCount) { pointCount = fLastMoveToIndex; } - if (pointCount > 3) { - const SkPoint* points = fPathRef->points(); - const SkPoint* last = &points[pointCount]; - // only consider the last of the initial move tos - while (SkPath::kMove_Verb == iter.next(pts)) { - ++points; - } - --points; - SkPathConvexity convexity = Convexicator::BySign(points, (int) (last - points)); - if (SkPathConvexity::kConvex != convexity) { - return setComputedConvexity(SkPathConvexity::kConcave); - } - iter.setPath(*this, true); - } else if (!this->isFinite()) { - return setFail(); + const SkPoint* points = fPathRef->points(); + // only consider the last of the initial move tos + int skipCount = SkPathPriv::LeadingMoveToCount(*this) - 1; + if (skipCount > 0) { + points += skipCount; + pointCount -= skipCount; } - int contourCount = 0; - int count; - Convexicator state; + SkPathConvexity convexity = Convexicator::BySign(points, pointCount); + if (SkPathConvexity::kConvex != convexity) { + return setComputedConvexity(SkPathConvexity::kConcave); + } - while ((verb = iter.next(pts)) != SkPath::kDone_Verb) { - switch (verb) { - case kMove_Verb: - if (++contourCount > 1) { - return setComputedConvexity(SkPathConvexity::kConcave); - } + int contourCount = 0; + bool needsClose = false; + Convexicator state; + + for (auto [verb, pts, wt] : SkPathPriv::Iterate(*this)) { + // Looking for the last moveTo before non-move verbs start + if (contourCount == 0) { + if (verb == SkPathVerb::kMove) { state.setMovePt(pts[0]); - count = 0; - break; - case kLine_Verb: - count = 1; - break; - case kQuad_Verb: - // fall through - case kConic_Verb: - count = 2; - break; - case kCubic_Verb: - count = 3; - break; - case kClose_Verb: + } else { + // Starting the actual contour, fall through to c=1 to add the points + contourCount++; + needsClose = true; + } + } + // Accumulating points into the Convexicator until we hit a close or another move + if (contourCount == 1) { + if (verb == SkPathVerb::kClose || verb == SkPathVerb::kMove) { if (!state.close()) { return setFail(); } - count = 0; - break; - default: - SkDEBUGFAIL("bad verb"); - return setComputedConvexity(SkPathConvexity::kConcave); - } - for (int i = 1; i <= count; i++) { - if (!state.addPt(pts[i])) { + needsClose = false; + contourCount++; + } else { + // lines add 1 point, cubics add 3, conics and quads add 2 + int count = SkPathPriv::PtsInVerb((unsigned) verb); + SkASSERT(count > 0); + for (int i = 1; i <= count; ++i) { + if (!state.addPt(pts[i])) { + return setFail(); + } + } + } + } else { + // The first contour has closed and anything other than spurious trailing moves means + // there's multiple contours and the path can't be convex + if (verb != SkPathVerb::kMove) { return setFail(); } } } + // If the path isn't explicitly closed do so implicitly + if (needsClose && !state.close()) { + return setFail(); + } + if (this->getFirstDirection() == SkPathFirstDirection::kUnknown) { if (state.getFirstDirection() == SkPathFirstDirection::kUnknown && !this->getBounds().isEmpty()) { @@ -2305,28 +2294,6 @@ SkPathConvexity SkPath::computeConvexity() const { return setComputedConvexity(SkPathConvexity::kConvex); } -bool SkPathPriv::IsConvex(const SkPoint points[], int count) { - SkPathConvexity convexity = Convexicator::BySign(points, count); - if (SkPathConvexity::kConvex != convexity) { - return false; - } - Convexicator state; - state.setMovePt(points[0]); - for (int i = 1; i < count; i++) { - if (!state.addPt(points[i])) { - return false; - } - } - if (!state.addPt(points[0])) { - return false; - } - if (!state.close()) { - return false; - } - return state.getFirstDirection() != SkPathFirstDirection::kUnknown - || state.reversals() < 3; -} - /////////////////////////////////////////////////////////////////////////////// class ContourIter { diff --git a/src/core/SkPathPriv.h b/src/core/SkPathPriv.h index c8c5e67603..fa299f885e 100644 --- a/src/core/SkPathPriv.h +++ b/src/core/SkPathPriv.h @@ -76,6 +76,20 @@ public: return false; } + // In some scenarios (e.g. fill or convexity checking all but the last leading move to are + // irrelevant to behavior). SkPath::injectMoveToIfNeeded should ensure that this is always at + // least 1. + static int LeadingMoveToCount(const SkPath& path) { + int verbCount = path.countVerbs(); + auto verbs = path.fPathRef->verbsBegin(); + for (int i = 0; i < verbCount; i++) { + if (verbs[i] != SkPath::Verb::kMove_Verb) { + return i; + } + } + return verbCount; // path is all move verbs + } + static void AddGenIDChangeListener(const SkPath& path, sk_sp listener) { path.fPathRef->addGenIDChangeListener(std::move(listener)); } @@ -188,15 +202,6 @@ public: return path.fPathRef->conicWeights(); } - /** Returns true if path formed by pts is convex. - - @param pts SkPoint array of path - @param count number of entries in array - - @return true if pts represent a convex geometry - */ - static bool IsConvex(const SkPoint pts[], int count); - /** Returns true if the underlying SkPathRef has one single owner. */ static bool TestingOnly_unique(const SkPath& path) { return path.fPathRef->unique(); diff --git a/tests/PathTest.cpp b/tests/PathTest.cpp index 86ba4063c6..3262fa96fa 100644 --- a/tests/PathTest.cpp +++ b/tests/PathTest.cpp @@ -1314,22 +1314,6 @@ static void check_convexity(skiatest::Reporter* reporter, const SkPath& path, SkPath copy(path); // NOLINT(performance-unnecessary-copy-initialization) bool convexity = copy.isConvex(); REPORTER_ASSERT(reporter, convexity == expectedConvexity); - - // test points-by-array interface - SkPath::Iter iter(path, true); - int initialMoves = 0; - SkPoint pts[4]; - while (SkPath::kMove_Verb == iter.next(pts)) { - ++initialMoves; - } - if (initialMoves > 0) { - std::vector points; - points.resize(path.getPoints(nullptr, 0)); - (void) path.getPoints(&points.front(), points.size()); - int skip = initialMoves - 1; - bool isConvex = SkPathPriv::IsConvex(&points.front() + skip, points.size() - skip); - REPORTER_ASSERT(reporter, isConvex == expectedConvexity); - } } static void test_path_crbug389050(skiatest::Reporter* reporter) { @@ -1984,9 +1968,9 @@ static void test_conservativelyContains(skiatest::Reporter* reporter) { path.moveTo(0, 0); path.lineTo(SkIntToScalar(100), 0); path.lineTo(0, SkIntToScalar(100)); - // Convexity logic is now more conservative, so that multiple (non-trailing) moveTos make a - // path non-convex. - REPORTER_ASSERT(reporter, !path.conservativelyContainsRect( + // Convexity logic treats a path as filled and closed, so that multiple (non-trailing) moveTos + // have no effect on convexity + REPORTER_ASSERT(reporter, path.conservativelyContainsRect( SkRect::MakeXYWH(SkIntToScalar(50), 0, SkIntToScalar(10), SkIntToScalar(10)))); @@ -5817,3 +5801,36 @@ DEF_TEST(path_convexity_scale_way_down, r) { SkPathPriv::ForceComputeConvexity(path2); REPORTER_ASSERT(r, path2.isConvex()); } + +// crbug.com/1187385 +DEF_TEST(path_moveto_addrect, r) { + // Test both an empty and non-empty rect passed to SkPath::addRect + SkRect rects[] = {{207.0f, 237.0f, 300.0f, 237.0f}, + {207.0f, 237.0f, 300.0f, 267.0f}}; + + for (SkRect rect: rects) { + for (int numExtraMoveTos : {0, 1, 2, 3}) { + SkPath path; + // Convexity and contains functions treat the path as a simple fill, so consecutive + // moveTos are collapsed together. + for (int i = 0; i < numExtraMoveTos; ++i) { + path.moveTo(i, i); + } + path.addRect(rect); + + REPORTER_ASSERT(r, (numExtraMoveTos + 1) == SkPathPriv::LeadingMoveToCount(path)); + + // addRect should mark the path as known convex automatically (i.e. it wasn't set + // to unknown after edits) + SkPathConvexity origConvexity = SkPathPriv::GetConvexityOrUnknown(path); + REPORTER_ASSERT(r, origConvexity == SkPathConvexity::kConvex); + + // but it should also agree with the regular convexity computation + SkPathPriv::ForceComputeConvexity(path); + REPORTER_ASSERT(r, path.isConvex()); + + SkRect query = rect.makeInset(10.f, 0.f); + REPORTER_ASSERT(r, path.conservativelyContainsRect(query)); + } + } +}