Fix use of fLastMoveToIndex in computeConvexity
Convexity is determined with a two pass algorithm, first by a sign measure and then by a winding measure. Convexity also is meant to not be affected by leading moveTos (other than the last leading moveTo before real verbs) and not affected by trailing moveTos (since no additional contour has actually started). The old code would incorrectly reduce pointCount when the last moveTo index was greater than 0, so the BySign pass was skipped or calculated on an incomplete set of points. When a path (as the one added in this CL's new test) is convex by winding but not by sign, it would be incorrectly identified as convex. This led to further cascading issues during rasterization. However, the old code also had the effect of correctly ignoring any last trailing moveTo from being included in the BySign test. Without the new loop decrementing pointCount, trailing moveTo locations would incorrectly create concave paths (and would in fact be concave if the verb was anything other than a move). I also realized that if the last moveTo index is not at the end of the initial leading block, or at the end of the path entirely, then it means the path must have multiple contours, at which point the path cannot be convex, so we take the early out. TBR: reed@google.com, bsalomon@google.com Bug: skia:1220754 Change-Id: I9bd38f2eaaa3dbee135c190ade46fce0bd20257a Reviewed-on: https://skia-review.googlesource.com/c/skia/+/420238 Reviewed-by: Michael Ludwig <michaelludwig@google.com> Commit-Queue: Michael Ludwig <michaelludwig@google.com>
This commit is contained in:
parent
6b95a366f5
commit
9f73b04b43
@ -2217,20 +2217,32 @@ SkPathConvexity SkPath::computeConvexity() const {
|
||||
return setFail();
|
||||
}
|
||||
|
||||
// Check to see if path changes direction more than three times as quick concave test
|
||||
// pointCount potentially includes a block of leading moveTos and trailing moveTos. Convexity
|
||||
// only cares about the last of the initial moveTos and the verbs before the final moveTos.
|
||||
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;
|
||||
int skipCount = SkPathPriv::LeadingMoveToCount(*this) - 1;
|
||||
|
||||
if (fLastMoveToIndex >= 0) {
|
||||
if (fLastMoveToIndex == pointCount - 1) {
|
||||
// Find the last real verb that affects convexity
|
||||
auto verbs = fPathRef->verbsEnd() - 1;
|
||||
while(verbs > fPathRef->verbsBegin() && *verbs == Verb::kMove_Verb) {
|
||||
verbs--;
|
||||
pointCount--;
|
||||
}
|
||||
} else if (fLastMoveToIndex != skipCount) {
|
||||
// There's an additional moveTo between two blocks of other verbs, so the path must have
|
||||
// more than one contour and cannot be convex.
|
||||
return setComputedConvexity(SkPathConvexity::kConcave);
|
||||
} // else no trailing or intermediate moveTos to worry about
|
||||
}
|
||||
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;
|
||||
}
|
||||
|
||||
// Check to see if path changes direction more than three times as quick concave test
|
||||
SkPathConvexity convexity = Convexicator::BySign(points, pointCount);
|
||||
if (SkPathConvexity::kConvex != convexity) {
|
||||
return setComputedConvexity(SkPathConvexity::kConcave);
|
||||
|
@ -5834,3 +5834,23 @@ DEF_TEST(path_moveto_addrect, r) {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// crbug.com/1220754
|
||||
DEF_TEST(path_moveto_twopass_convexity, r) {
|
||||
// There had been a bug when the last moveTo index > 0, the calculated point count was incorrect
|
||||
// and the BySign convexity pass would not evaluate the entire path, effectively only using the
|
||||
// winding rule for determining convexity.
|
||||
SkPath path;
|
||||
path.setFillType(SkPathFillType::kWinding);
|
||||
path.moveTo(3.25f, 115.5f);
|
||||
path.conicTo(9.98099e+17f, 2.83874e+15f, 1.75098e-30f, 1.75097e-30f, 1.05385e+18f);
|
||||
path.conicTo(9.96938e+17f, 6.3804e+19f, 9.96934e+17f, 1.75096e-30f, 1.75096e-30f);
|
||||
path.quadTo(1.28886e+10f, 9.9647e+17f, 9.98101e+17f, 2.61006e+15f);
|
||||
REPORTER_ASSERT(r, !path.isConvex());
|
||||
|
||||
SkPath pathWithExtraMoveTo;
|
||||
pathWithExtraMoveTo.setFillType(SkPathFillType::kWinding);
|
||||
pathWithExtraMoveTo.moveTo(5.90043e-39f, 1.34525e-43f);
|
||||
pathWithExtraMoveTo.addPath(path);
|
||||
REPORTER_ASSERT(r, !pathWithExtraMoveTo.isConvex());
|
||||
}
|
||||
|
Loading…
Reference in New Issue
Block a user