diff --git a/gm/arcto.cpp b/gm/arcto.cpp index cd26eef35a..584fca3c4f 100644 --- a/gm/arcto.cpp +++ b/gm/arcto.cpp @@ -205,3 +205,24 @@ DEF_SIMPLE_GM(bug593049, canvas, 300, 300) { 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 dfd1b629c1..c728d15a1a 100644 --- a/gm/dashing.cpp +++ b/gm/dashing.cpp @@ -21,7 +21,8 @@ static void drawline(SkCanvas* canvas, int on, int off, const SkPaint& paint, SkIntToScalar(off), }; - p.setPathEffect(SkDashPathEffect::Create(intervals, 2, phase))->unref(); + SkAutoTUnref 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 ced0aab69a..3816499915 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 @@ SkDashPathEffect::SkDashPathEffect(const SkScalar intervals[], int count, SkScal 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 @@ SkDashPathEffect::~SkDashPathEffect() { 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 @@ bool SkDashPathEffect::asPoints(PointData* results, 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 @@ void SkDashPathEffect::toString(SkString* str) const { ////////////////////////////////////////////////////////////////////////////////////////////////// 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 0d2783eba2..d766b0d53e 100644 --- a/src/utils/SkDashPath.cpp +++ b/src/utils/SkDashPath.cpp @@ -40,42 +40,35 @@ void SkDashPath::CalcDashParameters(SkScalar phase, const SkScalar intervals[], 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 @@ private: }; -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 @@ bool SkDashPath::FilterDashPath(SkPath* dst, const SkPath& src, SkStrokeRec* rec // 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 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 7453dcec82..54bf9a4870 100644 --- a/src/utils/SkDashPathPriv.h +++ b/src/utils/SkDashPathPriv.h @@ -16,17 +16,25 @@ namespace SkDashPath { * 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 68fce9a142..fa2395ef75 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 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.