allow one zero length dash
If the constructed stroke that represents a dash has a
single dash of length zero, and the end cap is square or
round, draw the cap.
The old code initialized the initial dash length to zero,
making it ambiguous whether the first length is zero or
not.
R=robertphillips@google.com
BUG=583299
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1805963002
Review URL: https://codereview.chromium.org/1805963002
diff --git a/gm/arcto.cpp b/gm/arcto.cpp
index cd26eef..584fca3 100644
--- a/gm/arcto.cpp
+++ b/gm/arcto.cpp
@@ -205,3 +205,24 @@
canvas->drawPath(p, paint);
}
+
+#include "SkDashPathEffect.h"
+#include "SkPathMeasure.h"
+
+DEF_SIMPLE_GM(bug583299, canvas, 300, 300) {
+ const char* d="M60,60 A50,50 0 0 0 160,60 A50,50 0 0 0 60,60z";
+ SkPaint p;
+ p.setStyle(SkPaint::kStroke_Style);
+ p.setStrokeWidth(100);
+ p.setAntiAlias(true);
+ p.setColor(0xFF008200);
+ p.setStrokeCap(SkPaint::kSquare_Cap);
+ SkPath path;
+ SkParsePath::FromSVGString(d, &path);
+ SkPathMeasure meas(path, false);
+ SkScalar length = meas.getLength();
+ SkScalar intervals[] = {0, length };
+ int intervalCount = (int) SK_ARRAY_COUNT(intervals);
+ p.setPathEffect(SkDashPathEffect::Create(intervals, intervalCount, 0))->unref();
+ canvas->drawPath(path, p);
+}
diff --git a/gm/dashing.cpp b/gm/dashing.cpp
index dfd1b62..c728d15 100644
--- a/gm/dashing.cpp
+++ b/gm/dashing.cpp
@@ -21,7 +21,8 @@
SkIntToScalar(off),
};
- p.setPathEffect(SkDashPathEffect::Create(intervals, 2, phase))->unref();
+ SkAutoTUnref<SkPathEffect> effect(SkDashPathEffect::Create(intervals, 2, phase));
+ p.setPathEffect(effect);
canvas->drawLine(startX, startY, finalX, finalY, p);
}
diff --git a/src/effects/SkDashPathEffect.cpp b/src/effects/SkDashPathEffect.cpp
index ced0aab..3816499 100644
--- a/src/effects/SkDashPathEffect.cpp
+++ b/src/effects/SkDashPathEffect.cpp
@@ -14,7 +14,7 @@
SkDashPathEffect::SkDashPathEffect(const SkScalar intervals[], int count, SkScalar phase)
: fPhase(0)
- , fInitialDashLength(0)
+ , fInitialDashLength(-1)
, fInitialDashIndex(0)
, fIntervalLength(0) {
SkASSERT(intervals);
@@ -23,7 +23,6 @@
fIntervals = (SkScalar*)sk_malloc_throw(sizeof(SkScalar) * count);
fCount = count;
for (int i = 0; i < count; i++) {
- SkASSERT(intervals[i] >= 0);
fIntervals[i] = intervals[i];
}
@@ -38,7 +37,7 @@
bool SkDashPathEffect::filterPath(SkPath* dst, const SkPath& src,
SkStrokeRec* rec, const SkRect* cullRect) const {
- return SkDashPath::FilterDashPath(dst, src, rec, cullRect, fIntervals, fCount,
+ return SkDashPath::InternalFilter(dst, src, rec, cullRect, fIntervals, fCount,
fInitialDashLength, fInitialDashIndex, fIntervalLength);
}
@@ -162,7 +161,7 @@
const SkMatrix& matrix,
const SkRect* cullRect) const {
// width < 0 -> fill && width == 0 -> hairline so requiring width > 0 rules both out
- if (fInitialDashLength < 0 || 0 >= rec.getWidth()) {
+ if (0 >= rec.getWidth()) {
return false;
}
@@ -388,13 +387,8 @@
//////////////////////////////////////////////////////////////////////////////////////////////////
SkPathEffect* SkDashPathEffect::Create(const SkScalar intervals[], int count, SkScalar phase) {
- if ((count < 2) || !SkIsAlign2(count)) {
+ if (!SkDashPath::ValidDashPath(phase, intervals, count)) {
return nullptr;
}
- for (int i = 0; i < count; i++) {
- if (intervals[i] < 0) {
- return nullptr;
- }
- }
return new SkDashPathEffect(intervals, count, phase);
}
diff --git a/src/utils/SkDashPath.cpp b/src/utils/SkDashPath.cpp
index 0d2783e..d766b0d 100644
--- a/src/utils/SkDashPath.cpp
+++ b/src/utils/SkDashPath.cpp
@@ -40,42 +40,35 @@
len += intervals[i];
}
*intervalLength = len;
-
- // watch out for values that might make us go out of bounds
- if ((len > 0) && SkScalarIsFinite(phase) && SkScalarIsFinite(len)) {
-
- // Adjust phase to be between 0 and len, "flipping" phase if negative.
- // e.g., if len is 100, then phase of -20 (or -120) is equivalent to 80
- if (adjustedPhase) {
- if (phase < 0) {
- phase = -phase;
- if (phase > len) {
- phase = SkScalarMod(phase, len);
- }
- phase = len - phase;
-
- // Due to finite precision, it's possible that phase == len,
- // even after the subtract (if len >>> phase), so fix that here.
- // This fixes http://crbug.com/124652 .
- SkASSERT(phase <= len);
- if (phase == len) {
- phase = 0;
- }
- } else if (phase >= len) {
+ // Adjust phase to be between 0 and len, "flipping" phase if negative.
+ // e.g., if len is 100, then phase of -20 (or -120) is equivalent to 80
+ if (adjustedPhase) {
+ if (phase < 0) {
+ phase = -phase;
+ if (phase > len) {
phase = SkScalarMod(phase, len);
}
- *adjustedPhase = phase;
+ phase = len - phase;
+
+ // Due to finite precision, it's possible that phase == len,
+ // even after the subtract (if len >>> phase), so fix that here.
+ // This fixes http://crbug.com/124652 .
+ SkASSERT(phase <= len);
+ if (phase == len) {
+ phase = 0;
+ }
+ } else if (phase >= len) {
+ phase = SkScalarMod(phase, len);
}
- SkASSERT(phase >= 0 && phase < len);
-
- *initialDashLength = find_first_interval(intervals, phase,
- initialDashIndex, count);
-
- SkASSERT(*initialDashLength >= 0);
- SkASSERT(*initialDashIndex >= 0 && *initialDashIndex < count);
- } else {
- *initialDashLength = -1; // signal bad dash intervals
+ *adjustedPhase = phase;
}
+ SkASSERT(phase >= 0 && phase < len);
+
+ *initialDashLength = find_first_interval(intervals, phase,
+ initialDashIndex, count);
+
+ SkASSERT(*initialDashLength >= 0);
+ SkASSERT(*initialDashIndex >= 0 && *initialDashIndex < count);
}
static void outset_for_stroke(SkRect* rect, const SkStrokeRec& rec) {
@@ -220,13 +213,13 @@
};
-bool SkDashPath::FilterDashPath(SkPath* dst, const SkPath& src, SkStrokeRec* rec,
+bool SkDashPath::InternalFilter(SkPath* dst, const SkPath& src, SkStrokeRec* rec,
const SkRect* cullRect, const SkScalar aIntervals[],
int32_t count, SkScalar initialDashLength, int32_t initialDashIndex,
SkScalar intervalLength) {
- // we do nothing if the src wants to be filled, or if our dashlength is 0
- if (rec->isFillStyle() || initialDashLength < 0) {
+ // we do nothing if the src wants to be filled
+ if (rec->isFillStyle()) {
return false;
}
@@ -306,7 +299,7 @@
// extend if we ended on a segment and we need to join up with the (skipped) initial segment
if (meas.isClosed() && is_even(initialDashIndex) &&
- initialDashLength > 0) {
+ initialDashLength >= 0) {
meas.getSegment(0, initialDashLength, dst, !addedSegment);
++segCount;
}
@@ -321,11 +314,29 @@
bool SkDashPath::FilterDashPath(SkPath* dst, const SkPath& src, SkStrokeRec* rec,
const SkRect* cullRect, const SkPathEffect::DashInfo& info) {
+ if (!ValidDashPath(info.fPhase, info.fIntervals, info.fCount)) {
+ return false;
+ }
SkScalar initialDashLength = 0;
int32_t initialDashIndex = 0;
SkScalar intervalLength = 0;
CalcDashParameters(info.fPhase, info.fIntervals, info.fCount,
&initialDashLength, &initialDashIndex, &intervalLength);
- return FilterDashPath(dst, src, rec, cullRect, info.fIntervals, info.fCount, initialDashLength,
+ return InternalFilter(dst, src, rec, cullRect, info.fIntervals, info.fCount, initialDashLength,
initialDashIndex, intervalLength);
}
+
+bool SkDashPath::ValidDashPath(SkScalar phase, const SkScalar intervals[], int32_t count) {
+ if (count < 2 || !SkIsAlign2(count)) {
+ return false;
+ }
+ SkScalar length = 0;
+ for (int i = 0; i < count; i++) {
+ if (intervals[i] < 0) {
+ return false;
+ }
+ length += intervals[i];
+ }
+ // watch out for values that might make us go out of bounds
+ return length > 0 && SkScalarIsFinite(phase) && SkScalarIsFinite(length);
+}
diff --git a/src/utils/SkDashPathPriv.h b/src/utils/SkDashPathPriv.h
index 7453dce..54bf9a4 100644
--- a/src/utils/SkDashPathPriv.h
+++ b/src/utils/SkDashPathPriv.h
@@ -16,17 +16,25 @@
* inputed phase and intervals. If adjustedPhase is passed in, then the phase will be
* adjusted to be between 0 and intervalLength. The result will be stored in adjustedPhase.
* If adjustedPhase is nullptr then it is assumed phase is already between 0 and intervalLength
+ *
+ * Caller should have already used ValidDashPath to exclude invalid data.
*/
void CalcDashParameters(SkScalar phase, const SkScalar intervals[], int32_t count,
SkScalar* initialDashLength, int32_t* initialDashIndex,
SkScalar* intervalLength, SkScalar* adjustedPhase = nullptr);
bool FilterDashPath(SkPath* dst, const SkPath& src, SkStrokeRec*, const SkRect*,
- const SkScalar aIntervals[], int32_t count, SkScalar initialDashLength,
- int32_t initialDashIndex, SkScalar intervalLength);
-
- bool FilterDashPath(SkPath* dst, const SkPath& src, SkStrokeRec*, const SkRect*,
const SkPathEffect::DashInfo& info);
+
+ /*
+ * Caller should have already used ValidDashPath to exclude invalid data.
+ */
+ bool InternalFilter(SkPath* dst, const SkPath& src, SkStrokeRec* rec,
+ const SkRect* cullRect, const SkScalar aIntervals[],
+ int32_t count, SkScalar initialDashLength, int32_t initialDashIndex,
+ SkScalar intervalLength);
+
+ bool ValidDashPath(SkScalar phase, const SkScalar intervals[], int32_t count);
}
#endif
diff --git a/tests/DashPathEffectTest.cpp b/tests/DashPathEffectTest.cpp
index 68fce9a..fa2395e 100644
--- a/tests/DashPathEffectTest.cpp
+++ b/tests/DashPathEffectTest.cpp
@@ -12,20 +12,15 @@
#include "SkStrokeRec.h"
// crbug.com/348821 was rooted in SkDashPathEffect refusing to flatten and unflatten itself when
-// fInitialDashLength < 0 (a signal the effect is nonsense). Here we test that it flattens.
+// the effect is nonsense. Here we test that it fails when passed nonsense parameters.
DEF_TEST(DashPathEffectTest_crbug_348821, r) {
SkScalar intervals[] = { 1.76934361e+36f, 2.80259693e-45f }; // Values from bug.
const int count = 2;
- SkScalar phase = SK_ScalarInfinity; // Used to force the bad fInitialDashLength = -1 path.
+ SkScalar phase = SK_ScalarInfinity; // Used to force a nonsense effect.
SkAutoTUnref<SkPathEffect> dash(SkDashPathEffect::Create(intervals, count, phase));
- // nullptr -> refuses to work with flattening framework.
- REPORTER_ASSERT(r, dash->getFactory() != nullptr);
-
- SkWriteBuffer buffer;
- buffer.writeFlattenable(dash);
- REPORTER_ASSERT(r, buffer.bytesWritten() > 12); // We'd write 12 if broken, >=40 if not.
+ REPORTER_ASSERT(r, dash == nullptr);
}
// Test out the asPoint culling behavior.