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

2 files changed, 30 insertions, 32 deletions

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) {