diff options
author | bungeman <bungeman@google.com> | 2016-03-18 05:10:23 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2016-03-18 05:10:23 -0700 |
commit | 6f0749cfc7f93880bd6b8acfdc61900cda4a81fe (patch) | |
tree | 0fb054893ced56e90a8baf02400744b45a5b2321 | |
parent | 5e1a24808415df2748822e8082e21a361362cdfe (diff) |
Revert of allow one zero length dash (patchset #8 id:140001 of https://codereview.chromium.org/1805963002/ )
Reason for revert:
Causes the dash bench to crash.
Example crash:
https://build.chromium.org/p/client.skia/builders/Perf-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release/builds/5581/steps/nanobench/logs/stdio
Original issue's description:
> 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
>
> Committed: https://skia.googlesource.com/skia/+/5e1a24808415df2748822e8082e21a361362cdfe
TBR=robertphillips@google.com,reed@google.com,caryclark@google.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=583299
Review URL: https://codereview.chromium.org/1808303004
-rw-r--r-- | gm/arcto.cpp | 21 | ||||
-rw-r--r-- | gm/dashing.cpp | 3 | ||||
-rw-r--r-- | src/effects/SkDashPathEffect.cpp | 14 | ||||
-rw-r--r-- | src/utils/SkDashPath.cpp | 83 | ||||
-rw-r--r-- | src/utils/SkDashPathPriv.h | 16 | ||||
-rw-r--r-- | tests/DashPathEffectTest.cpp | 11 |
6 files changed, 59 insertions, 89 deletions
diff --git a/gm/arcto.cpp b/gm/arcto.cpp index 584fca3c4f..cd26eef35a 100644 --- a/gm/arcto.cpp +++ b/gm/arcto.cpp @@ -205,24 +205,3 @@ 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 c728d15a1a..dfd1b629c1 100644 --- a/gm/dashing.cpp +++ b/gm/dashing.cpp @@ -21,8 +21,7 @@ static void drawline(SkCanvas* canvas, int on, int off, const SkPaint& paint, SkIntToScalar(off), }; - SkAutoTUnref<SkPathEffect> effect(SkDashPathEffect::Create(intervals, 2, phase)); - p.setPathEffect(effect); + p.setPathEffect(SkDashPathEffect::Create(intervals, 2, phase))->unref(); canvas->drawLine(startX, startY, finalX, finalY, p); } diff --git a/src/effects/SkDashPathEffect.cpp b/src/effects/SkDashPathEffect.cpp index 3816499915..ced0aab69a 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(-1) + , fInitialDashLength(0) , fInitialDashIndex(0) , fIntervalLength(0) { SkASSERT(intervals); @@ -23,6 +23,7 @@ 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]; } @@ -37,7 +38,7 @@ SkDashPathEffect::~SkDashPathEffect() { bool SkDashPathEffect::filterPath(SkPath* dst, const SkPath& src, SkStrokeRec* rec, const SkRect* cullRect) const { - return SkDashPath::InternalFilter(dst, src, rec, cullRect, fIntervals, fCount, + return SkDashPath::FilterDashPath(dst, src, rec, cullRect, fIntervals, fCount, fInitialDashLength, fInitialDashIndex, fIntervalLength); } @@ -161,7 +162,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 (0 >= rec.getWidth()) { + if (fInitialDashLength < 0 || 0 >= rec.getWidth()) { return false; } @@ -387,8 +388,13 @@ void SkDashPathEffect::toString(SkString* str) const { ////////////////////////////////////////////////////////////////////////////////////////////////// SkPathEffect* SkDashPathEffect::Create(const SkScalar intervals[], int count, SkScalar phase) { - if (!SkDashPath::ValidDashPath(phase, intervals, count)) { + if ((count < 2) || !SkIsAlign2(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 d766b0d53e..0d2783eba2 100644 --- a/src/utils/SkDashPath.cpp +++ b/src/utils/SkDashPath.cpp @@ -40,35 +40,42 @@ void SkDashPath::CalcDashParameters(SkScalar phase, const SkScalar intervals[], len += intervals[i]; } *intervalLength = 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) { + + // 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) { 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) { - phase = SkScalarMod(phase, len); + *adjustedPhase = phase; } - *adjustedPhase = phase; - } - SkASSERT(phase >= 0 && phase < len); + SkASSERT(phase >= 0 && phase < len); - *initialDashLength = find_first_interval(intervals, phase, - initialDashIndex, count); + *initialDashLength = find_first_interval(intervals, phase, + initialDashIndex, count); - SkASSERT(*initialDashLength >= 0); - SkASSERT(*initialDashIndex >= 0 && *initialDashIndex < count); + SkASSERT(*initialDashLength >= 0); + SkASSERT(*initialDashIndex >= 0 && *initialDashIndex < count); + } else { + *initialDashLength = -1; // signal bad dash intervals + } } static void outset_for_stroke(SkRect* rect, const SkStrokeRec& rec) { @@ -213,13 +220,13 @@ private: }; -bool SkDashPath::InternalFilter(SkPath* dst, const SkPath& src, SkStrokeRec* rec, +bool SkDashPath::FilterDashPath(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 - if (rec->isFillStyle()) { + // we do nothing if the src wants to be filled, or if our dashlength is 0 + if (rec->isFillStyle() || initialDashLength < 0) { return false; } @@ -299,7 +306,7 @@ bool SkDashPath::InternalFilter(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; } @@ -314,29 +321,11 @@ bool SkDashPath::InternalFilter(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 InternalFilter(dst, src, rec, cullRect, info.fIntervals, info.fCount, initialDashLength, + return FilterDashPath(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 54bf9a4870..7453dcec82 100644 --- a/src/utils/SkDashPathPriv.h +++ b/src/utils/SkDashPathPriv.h @@ -16,25 +16,17 @@ 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 fa2395ef75..68fce9a142 100644 --- a/tests/DashPathEffectTest.cpp +++ b/tests/DashPathEffectTest.cpp @@ -12,15 +12,20 @@ #include "SkStrokeRec.h" // crbug.com/348821 was rooted in SkDashPathEffect refusing to flatten and unflatten itself when -// the effect is nonsense. Here we test that it fails when passed nonsense parameters. +// fInitialDashLength < 0 (a signal the effect is nonsense). Here we test that it flattens. 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 a nonsense effect. + SkScalar phase = SK_ScalarInfinity; // Used to force the bad fInitialDashLength = -1 path. SkAutoTUnref<SkPathEffect> dash(SkDashPathEffect::Create(intervals, count, phase)); - REPORTER_ASSERT(r, dash == nullptr); + // 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. } // Test out the asPoint culling behavior. |