diff options
author | bsalomon <bsalomon@google.com> | 2016-09-23 12:09:16 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2016-09-23 12:09:16 -0700 |
commit | aa840647fc7f057715bce62489b96c4299385135 (patch) | |
tree | 882f53aff47ce041f0a43537d166451ab2a94e6a | |
parent | bf41fa841b19ebab1eea7df573b3456dd6c5cac0 (diff) |
Don't compute path keys for volatile paths in GrShape.
Otherwise, we will compute cache keys for internally transformed paths that don't repeat (e.g. clip paths transformed into device space with a changing view matrix).
BUG=chromium:649562
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2369513002
Review-Url: https://codereview.chromium.org/2369513002
-rw-r--r-- | src/gpu/GrShape.cpp | 8 | ||||
-rw-r--r-- | tests/GpuDrawPathTest.cpp | 18 | ||||
-rw-r--r-- | tests/GrShapeTest.cpp | 44 |
3 files changed, 34 insertions, 36 deletions
diff --git a/src/gpu/GrShape.cpp b/src/gpu/GrShape.cpp index 12fdd3bc14..7e0a3a4449 100644 --- a/src/gpu/GrShape.cpp +++ b/src/gpu/GrShape.cpp @@ -132,13 +132,13 @@ int GrShape::unstyledKeySize() const { // 4 for the end points and 1 for the inverseness return 5; case Type::kPath: { + if (0 == fPathData.fGenID) { + return -1; + } int dataKeySize = path_key_from_data_size(fPathData.fPath); if (dataKeySize >= 0) { return dataKeySize; } - if (0 == fPathData.fGenID) { - return -1; - } // The key is the path ID and fill type. return 2; } @@ -172,12 +172,12 @@ void GrShape::writeUnstyledKey(uint32_t* key) const { *key++ = fLineData.fInverted ? 1 : 0; break; case Type::kPath: { + SkASSERT(fPathData.fGenID); int dataKeySize = path_key_from_data_size(fPathData.fPath); if (dataKeySize >= 0) { write_path_key_from_data(fPathData.fPath, key); return; } - SkASSERT(fPathData.fGenID); *key++ = fPathData.fGenID; // We could canonicalize the fill rule for paths that don't differentiate between // even/odd or winding fill (e.g. convex). diff --git a/tests/GpuDrawPathTest.cpp b/tests/GpuDrawPathTest.cpp index 1a42a49efd..83641cb2ec 100644 --- a/tests/GpuDrawPathTest.cpp +++ b/tests/GpuDrawPathTest.cpp @@ -104,18 +104,15 @@ DEF_GPUTEST(GrPathKeys, reporter, /*factory*/) { for (const GrStyle& style : styles) { // Keys should not ignore conic weights. SkPath path1, path2; - path1.setIsVolatile(true); - path2.setIsVolatile(true); SkPoint p0 = SkPoint::Make(100, 0); SkPoint p1 = SkPoint::Make(100, 100); path1.conicTo(p0, p1, .5f); path2.conicTo(p0, p1, .7f); - bool isVolatile; GrUniqueKey key1, key2; - // Even though the paths are marked volatile, they should have keys based on the path data - // because they have a small amount of data. + // We expect these small paths to be keyed based on their data. + bool isVolatile; GrPath::ComputeKey(GrShape(path1, GrStyle::SimpleFill()), &key1, &isVolatile); REPORTER_ASSERT(reporter, !isVolatile); REPORTER_ASSERT(reporter, key1.isValid()); @@ -123,12 +120,19 @@ DEF_GPUTEST(GrPathKeys, reporter, /*factory*/) { REPORTER_ASSERT(reporter, !isVolatile); REPORTER_ASSERT(reporter, key1.isValid()); REPORTER_ASSERT(reporter, key1 != key2); + { + GrUniqueKey tempKey; + path1.setIsVolatile(true); + GrPath::ComputeKey(GrShape(path1, style), &key1, &isVolatile); + REPORTER_ASSERT(reporter, isVolatile); + REPORTER_ASSERT(reporter, !tempKey.isValid()); + } // Ensure that recreating the GrShape doesn't change the key. { GrUniqueKey tempKey; - GrPath::ComputeKey(GrShape(path1, GrStyle::SimpleFill()), &tempKey, &isVolatile); - REPORTER_ASSERT(reporter, key1 == tempKey); + GrPath::ComputeKey(GrShape(path2, GrStyle::SimpleFill()), &tempKey, &isVolatile); + REPORTER_ASSERT(reporter, key2 == tempKey); } // Try a large path that is too big to be keyed off its data. diff --git a/tests/GrShapeTest.cpp b/tests/GrShapeTest.cpp index 4f96a038e3..ae96c7d7dd 100644 --- a/tests/GrShapeTest.cpp +++ b/tests/GrShapeTest.cpp @@ -1161,14 +1161,8 @@ void test_make_hairline_path_effect(skiatest::Reporter* reporter, const Geo& geo // If the resulting path is small enough then it will have a key. REPORTER_ASSERT(reporter, paths_fill_same(a, b)); REPORTER_ASSERT(reporter, paths_fill_same(a, c)); - if (c.countVerbs() <= GrShape::kMaxKeyFromDataVerbCnt) { - REPORTER_ASSERT(reporter, !peCase.appliedPathEffectKey().empty()); - REPORTER_ASSERT(reporter, peCase.appliedPathEffectKey() == - peCase.appliedFullStyleKey()); - } else { - REPORTER_ASSERT(reporter, peCase.appliedPathEffectKey().empty()); - REPORTER_ASSERT(reporter, peCase.appliedFullStyleKey().empty()); - } + REPORTER_ASSERT(reporter, peCase.appliedPathEffectKey().empty()); + REPORTER_ASSERT(reporter, peCase.appliedFullStyleKey().empty()); } REPORTER_ASSERT(reporter, peCase.appliedPathEffectShape().style().isSimpleHairline()); REPORTER_ASSERT(reporter, peCase.appliedFullStyleShape().style().isSimpleHairline()); @@ -1184,8 +1178,8 @@ void test_volatile_path(skiatest::Reporter* reporter, const Geo& geo) { dashAndStroke.setStyle(SkPaint::kStroke_Style); TestCase volatileCase(reporter, vPath, dashAndStroke); // We expect a shape made from a volatile path to have a key iff the shape is recognized - // as a specialized geometry or it has a small verb count. - if (geo.isNonPath(dashAndStroke) || vPath.countVerbs() <= GrShape::kMaxKeyFromDataVerbCnt) { + // as a specialized geometry. + if (geo.isNonPath(dashAndStroke)) { REPORTER_ASSERT(reporter, SkToBool(volatileCase.baseKey().count())); // In this case all the keys should be identical to the non-volatile case. TestCase nonVolatileCase(reporter, geo.path(), dashAndStroke); @@ -1788,19 +1782,19 @@ static void test_short_path_keys(skiatest::Reporter* r) { paints[3].setStyle(SkPaint::kStrokeAndFill_Style); paints[3].setStrokeWidth(5.f); - auto compare = [r, &paints] (SkPath* pathA, SkPath* pathB, + auto compare = [r, &paints] (const SkPath& pathA, const SkPath& pathB, TestCase::ComparisonExpecation expectation) { + SkPath volatileA = pathA; + SkPath volatileB = pathB; + volatileA.setIsVolatile(true); + volatileB.setIsVolatile(true); for (const SkPaint& paint : paints) { + REPORTER_ASSERT(r, !GrShape(volatileA, paint).hasUnstyledKey()); + REPORTER_ASSERT(r, !GrShape(volatileB, paint).hasUnstyledKey()); for (PathGeo::Invert invert : {PathGeo::Invert::kNo, PathGeo::Invert::kYes}) { - for (bool aIsVolatile : {false, true}) { - for (bool bIsVolatile : {false, true}) { - pathA->setIsVolatile(aIsVolatile); - pathB->setIsVolatile(bIsVolatile); - TestCase caseA(PathGeo(*pathA, invert), paint, r); - TestCase caseB(PathGeo(*pathB, invert), paint, r); - caseA.compare(r, caseB, expectation); - } - } + TestCase caseA(PathGeo(pathA, invert), paint, r); + TestCase caseB(PathGeo(pathB, invert), paint, r); + caseA.compare(r, caseB, expectation); } } }; @@ -1814,33 +1808,33 @@ static void test_short_path_keys(skiatest::Reporter* r) { pathB.lineTo(10.f, 10.f); pathB.conicTo(20.f, 20.f, 20.f, 30.f, 0.7f); - compare(&pathA, &pathB, TestCase::kAllSame_ComparisonExpecation); + compare(pathA, pathB, TestCase::kAllSame_ComparisonExpecation); // Give path b a different point pathB.reset(); pathB.lineTo(10.f, 10.f); pathB.conicTo(21.f, 20.f, 20.f, 30.f, 0.7f); - compare(&pathA, &pathB, TestCase::kAllDifferent_ComparisonExpecation); + compare(pathA, pathB, TestCase::kAllDifferent_ComparisonExpecation); // Give path b a different conic weight pathB.reset(); pathB.lineTo(10.f, 10.f); pathB.conicTo(20.f, 20.f, 20.f, 30.f, 0.6f); - compare(&pathA, &pathB, TestCase::kAllDifferent_ComparisonExpecation); + compare(pathA, pathB, TestCase::kAllDifferent_ComparisonExpecation); // Give path b an extra lineTo verb pathB.reset(); pathB.lineTo(10.f, 10.f); pathB.conicTo(20.f, 20.f, 20.f, 30.f, 0.6f); pathB.lineTo(50.f, 50.f); - compare(&pathA, &pathB, TestCase::kAllDifferent_ComparisonExpecation); + compare(pathA, pathB, TestCase::kAllDifferent_ComparisonExpecation); // Give path b a close pathB.reset(); pathB.lineTo(10.f, 10.f); pathB.conicTo(20.f, 20.f, 20.f, 30.f, 0.7f); pathB.close(); - compare(&pathA, &pathB, TestCase::kAllDifferent_ComparisonExpecation); + compare(pathA, pathB, TestCase::kAllDifferent_ComparisonExpecation); } DEF_TEST(GrShape, reporter) { |