AuroraMiddleware/skia2
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Reland "Stop using copying SkPath::Iter for convexity and contains checks"

Browse Source 
This reverts commit 29c06bc82a.

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 3752760157.
>
> 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 <michaelludwig@google.com>
> > Reviewed-by: Mike Reed <reed@google.com>
>
> 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 <michaelludwig@google.com>
> Commit-Queue: Michael Ludwig <michaelludwig@google.com>

# 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 <csmartdalton@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
This commit is contained in:
Michael Ludwig 2021-04-22 09:34:45 -04:00 committed by Skia Commit-Bot
parent 1034425bc1
commit 48f106501c
3 changed files with 147 additions and 158 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

227
src/core/SkPath.cpp
View File

@ -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 {

23
src/core/SkPathPriv.h
View File

@ -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<SkIDChangeListener> 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();

55
tests/PathTest.cpp
View File

@ -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<SkPoint> 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));
}
}
}