Fix key computation for GrPaths

Improve tests to ensure paths are receiving valid keys
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2342873002

Review-Url: https://codereview.chromium.org/2342873002

4 files changed, 111 insertions, 110 deletions

diff --git a/src/gpu/GrPath.cpp b/src/gpu/GrPath.cpp
index 91a245d854..27fbf21a9d 100644
--- a/src/gpu/GrPath.cpp
+++ b/src/gpu/GrPath.cpp
@@ -6,9 +6,8 @@
  */
 
 #include "GrPath.h"
-#include "GrStyle.h"
+#include "GrShape.h"
 
-namespace {
 // Verb count limit for generating path key from content of a volatile path.
 // The value should accomodate at least simple rects and rrects.
 static const int kSimpleVolatilePathVerbLimit = 10;
@@ -26,58 +25,14 @@ static inline void write_style_key(uint32_t* dst, const GrStyle& style) {
     GrStyle::WriteKey(dst, style, GrStyle::Apply::kPathEffectAndStrokeRec, SK_Scalar1);
 }
 
-
-inline static bool compute_key_for_line_path(const SkPath& path, const GrStyle& style,
-                                             GrUniqueKey* key) {
-    SkPoint pts[2];
-    if (!path.isLine(pts)) {
-        return false;
-    }
-    static_assert((sizeof(pts) % sizeof(uint32_t)) == 0 && sizeof(pts) > sizeof(uint32_t),
-                  "pts_needs_padding");
-    int styleDataCnt = style_data_cnt(style);
-
-    const int kBaseData32Cnt = 1 + sizeof(pts) / sizeof(uint32_t);
-    static const GrUniqueKey::Domain kOvalPathDomain = GrUniqueKey::GenerateDomain();
-    GrUniqueKey::Builder builder(key, kOvalPathDomain, kBaseData32Cnt + styleDataCnt);
-    builder[0] = path.getFillType();
-    memcpy(&builder[1], &pts, sizeof(pts));
-    if (styleDataCnt > 0) {
-        write_style_key(&builder[kBaseData32Cnt], style);
-    }
-    return true;
-}
-
-inline static bool compute_key_for_oval_path(const SkPath& path, const GrStyle& style,
-                                             GrUniqueKey* key) {
-    SkRect rect;
-    // Point order is significant when dashing, so we cannot devolve to a rect key.
-    if (style.pathEffect() || !path.isOval(&rect)) {
-        return false;
-    }
-    static_assert((sizeof(rect) % sizeof(uint32_t)) == 0 && sizeof(rect) > sizeof(uint32_t),
-                  "rect_needs_padding");
-
-    const int kBaseData32Cnt = 1 + sizeof(rect) / sizeof(uint32_t);
-    int styleDataCnt = style_data_cnt(style);
-    static const GrUniqueKey::Domain kOvalPathDomain = GrUniqueKey::GenerateDomain();
-    GrUniqueKey::Builder builder(key, kOvalPathDomain, kBaseData32Cnt + styleDataCnt);
-    builder[0] = path.getFillType();
-    memcpy(&builder[1], &rect, sizeof(rect));
-    if (styleDataCnt > 0) {
-        write_style_key(&builder[kBaseData32Cnt], style);
-    }
-    return true;
-}
-
-// Encodes the full path data to the unique key for very small, volatile paths. This is typically
-// hit when clipping stencils the clip stack. Intention is that this handles rects too, since
-// SkPath::isRect seems to do non-trivial amount of work.
-inline static bool compute_key_for_simple_path(const SkPath& path, const GrStyle& style,
-                                               GrUniqueKey* key) {
-    if (!path.isVolatile()) {
+// Encodes the full path data to the unique key for very small paths that wouldn't otherwise have a
+// key. This is typically hit when clipping stencils the clip stack.
+inline static bool compute_key_for_simple_path(const GrShape& shape, GrUniqueKey* key) {
+    if (shape.hasUnstyledKey()) {
         return false;
     }
+    SkPath path;
+    shape.asPath(&path);
     // The check below should take care of negative values casted positive.
     const int verbCnt = path.countVerbs();
     if (verbCnt > kSimpleVolatilePathVerbLimit) {
@@ -124,7 +79,7 @@ inline static bool compute_key_for_simple_path(const SkPath& path, const GrStyle
     // 2) stroke data (varying size)
 
     const int baseData32Cnt = 2 + verbData32Cnt + pointData32Cnt + conicWeightData32Cnt;
-    const int styleDataCnt = style_data_cnt(style);
+    const int styleDataCnt = style_data_cnt(shape.style());
     static const GrUniqueKey::Domain kSimpleVolatilePathDomain = GrUniqueKey::GenerateDomain();
     GrUniqueKey::Builder builder(key, kSimpleVolatilePathDomain, baseData32Cnt + styleDataCnt);
     int i = 0;
@@ -169,45 +124,33 @@ inline static bool compute_key_for_simple_path(const SkPath& path, const GrStyle
     }
     SkASSERT(i == baseData32Cnt);
     if (styleDataCnt > 0) {
-        write_style_key(&builder[baseData32Cnt], style);
+        write_style_key(&builder[baseData32Cnt], shape.style());
     }
     return true;
 }
 
-inline static void compute_key_for_general_path(const SkPath& path, const GrStyle& style,
-                                                GrUniqueKey* key) {
-    const int kBaseData32Cnt = 2;
-    int styleDataCnt = style_data_cnt(style);
+inline static bool compute_key_for_general_shape(const GrShape& shape, GrUniqueKey* key) {
+    int geoCnt = shape.unstyledKeySize();
+    int styleCnt = style_data_cnt(shape.style());
+    if (styleCnt < 0 || geoCnt < 0) {
+        return false;
+    }
     static const GrUniqueKey::Domain kGeneralPathDomain = GrUniqueKey::GenerateDomain();
-    GrUniqueKey::Builder builder(key, kGeneralPathDomain, kBaseData32Cnt + styleDataCnt);
-    builder[0] = path.getGenerationID();
-    builder[1] = path.getFillType();
-    if (styleDataCnt > 0) {
-        write_style_key(&builder[kBaseData32Cnt], style);
+    GrUniqueKey::Builder builder(key, kGeneralPathDomain, geoCnt + styleCnt);
+    shape.writeUnstyledKey(&builder[0]);
+    if (styleCnt) {
+        write_style_key(&builder[geoCnt], shape.style());
     }
+    return true;
 }
 
-}
-
-void GrPath::ComputeKey(const SkPath& path, const GrStyle& style, GrUniqueKey* key,
-                        bool* outIsVolatile) {
-    if (compute_key_for_line_path(path, style, key)) {
-        *outIsVolatile = false;
-        return;
-    }
-
-    if (compute_key_for_oval_path(path, style, key)) {
-        *outIsVolatile = false;
-        return;
-    }
+void GrPath::ComputeKey(const GrShape& shape, GrUniqueKey* key, bool* outIsVolatile) {
 
-    if (compute_key_for_simple_path(path, style, key)) {
+    if (compute_key_for_simple_path(shape, key)) {
         *outIsVolatile = false;
         return;
     }
-
-    compute_key_for_general_path(path, style, key);
-    *outIsVolatile = path.isVolatile();
+    *outIsVolatile = !compute_key_for_general_shape(shape, key);
 }
 
 #ifdef SK_DEBUG
diff --git a/src/gpu/GrPath.h b/src/gpu/GrPath.h
index ee3123f9b5..19538370d5 100644
--- a/src/gpu/GrPath.h
+++ b/src/gpu/GrPath.h
@@ -14,6 +14,8 @@
 #include "SkPath.h"
 #include "SkRect.h"
 
+class GrShape;
+
 class GrPath : public GrGpuResource {
 public:
     /**
@@ -30,8 +32,7 @@ public:
     {
     }
 
-    static void ComputeKey(const SkPath& path, const GrStyle& style, GrUniqueKey* key,
-                           bool* outIsVolatile);
+    static void ComputeKey(const GrShape&, GrUniqueKey* key, bool* outIsVolatile);
 
     const SkRect& getBounds() const { return fBounds; }
 
diff --git a/src/gpu/batches/GrStencilAndCoverPathRenderer.cpp b/src/gpu/batches/GrStencilAndCoverPathRenderer.cpp
index 4f2c6b35b6..69a3142f21 100644
--- a/src/gpu/batches/GrStencilAndCoverPathRenderer.cpp
+++ b/src/gpu/batches/GrStencilAndCoverPathRenderer.cpp
@@ -51,20 +51,28 @@ bool GrStencilAndCoverPathRenderer::onCanDrawPath(const CanDrawPathArgs& args) c
     }
 }
 
-static GrPath* get_gr_path(GrResourceProvider* resourceProvider, const SkPath& skPath,
-                           const GrStyle& style) {
+static GrPath* get_gr_path(GrResourceProvider* resourceProvider, const GrShape& shape) {
     GrUniqueKey key;
     bool isVolatile;
-    GrPath::ComputeKey(skPath, style, &key, &isVolatile);
-    SkAutoTUnref<GrPath> path(
-        static_cast<GrPath*>(resourceProvider->findAndRefResourceByUniqueKey(key)));
+    GrPath::ComputeKey(shape, &key, &isVolatile);
+    sk_sp<GrPath> path;
+    if (!isVolatile) {
+        path.reset(
+            static_cast<GrPath*>(resourceProvider->findAndRefResourceByUniqueKey(key)));
+    }
     if (!path) {
-        path.reset(resourceProvider->createPath(skPath, style));
+        SkPath skPath;
+        shape.asPath(&skPath);
+        path.reset(resourceProvider->createPath(skPath, shape.style()));
         if (!isVolatile) {
-            resourceProvider->assignUniqueKeyToResource(key, path);
+            resourceProvider->assignUniqueKeyToResource(key, path.get());
         }
     } else {
-        SkASSERT(path->isEqualTo(skPath, style));
+#ifdef SK_DEBUG
+        SkPath skPath;
+        shape.asPath(&skPath);
+        SkASSERT(path->isEqualTo(skPath, shape.style()));
+#endif
     }
     return path.release();
 }
@@ -73,10 +81,8 @@ void GrStencilAndCoverPathRenderer::onStencilPath(const StencilPathArgs& args) {
     GR_AUDIT_TRAIL_AUTO_FRAME(args.fDrawContext->auditTrail(),
                               "GrStencilAndCoverPathRenderer::onStencilPath");
     SkASSERT(!args.fIsAA || args.fDrawContext->isStencilBufferMultisampled());
-    SkPath path;
-    args.fShape->asPath(&path);
 
-    SkAutoTUnref<GrPath> p(get_gr_path(fResourceProvider, path, GrStyle::SimpleFill()));
+    SkAutoTUnref<GrPath> p(get_gr_path(fResourceProvider, *args.fShape));
     args.fDrawContext->drawContextPriv().stencilPath(*args.fClip, args.fIsAA, *args.fViewMatrix, p);
 }
 
@@ -91,7 +97,7 @@ bool GrStencilAndCoverPathRenderer::onDrawPath(const DrawPathArgs& args) {
     SkPath path;
     args.fShape->asPath(&path);
 
-    SkAutoTUnref<GrPath> p(get_gr_path(fResourceProvider, path, args.fShape->style()));
+    SkAutoTUnref<GrPath> p(get_gr_path(fResourceProvider, *args.fShape));
 
     if (path.isInverseFillType()) {
         SkMatrix invert = SkMatrix::I();
diff --git a/tests/GpuDrawPathTest.cpp b/tests/GpuDrawPathTest.cpp
index c0b31f0737..1a42a49efd 100644
--- a/tests/GpuDrawPathTest.cpp
+++ b/tests/GpuDrawPathTest.cpp
@@ -11,7 +11,7 @@
 
 #include "GrContext.h"
 #include "GrPath.h"
-#include "GrStyle.h"
+#include "GrShape.h"
 #include "SkBitmap.h"
 #include "SkCanvas.h"
 #include "SkColor.h"
@@ -92,21 +92,72 @@ DEF_GPUTEST_FOR_ALL_GL_CONTEXTS(GpuDrawPath, reporter, ctxInfo) {
 }
 
 DEF_GPUTEST(GrPathKeys, reporter, /*factory*/) {
-    // 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;
-    GrPath::ComputeKey(path1, GrStyle::SimpleFill(), &key1, &isVolatile);
-    GrPath::ComputeKey(path2, GrStyle::SimpleFill(), &key2, &isVolatile);
-    REPORTER_ASSERT(reporter, key1 != key2);
+    SkPaint strokePaint;
+    strokePaint.setStyle(SkPaint::kStroke_Style);
+    strokePaint.setStrokeWidth(10.f);
+    GrStyle styles[] = {
+        GrStyle::SimpleFill(),
+        GrStyle::SimpleHairline(),
+        GrStyle(strokePaint)
+    };
+
+    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.
+        GrPath::ComputeKey(GrShape(path1, GrStyle::SimpleFill()), &key1, &isVolatile);
+        REPORTER_ASSERT(reporter, !isVolatile);
+        REPORTER_ASSERT(reporter, key1.isValid());
+        GrPath::ComputeKey(GrShape(path2, GrStyle::SimpleFill()), &key2, &isVolatile);
+        REPORTER_ASSERT(reporter, !isVolatile);
+        REPORTER_ASSERT(reporter, key1.isValid());
+        REPORTER_ASSERT(reporter, key1 != key2);
+
+        // 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);
+        }
+
+        // Try a large path that is too big to be keyed off its data.
+        SkPath path3;
+        SkPath path4;
+        for (int i = 0; i < 1000; ++i) {
+            SkScalar s = SkIntToScalar(i);
+            path3.conicTo(s, 3.f * s / 4, s + 1.f, s, 0.5f + s / 2000.f);
+            path4.conicTo(s, 3.f * s / 4, s + 1.f, s, 0.3f + s / 2000.f);
+        }
+
+        GrUniqueKey key3, key4;
+        // These aren't marked volatile and so should have keys
+        GrPath::ComputeKey(GrShape(path3, style), &key3, &isVolatile);
+        REPORTER_ASSERT(reporter, !isVolatile);
+        REPORTER_ASSERT(reporter, key3.isValid());
+        GrPath::ComputeKey(GrShape(path4, style), &key4, &isVolatile);
+        REPORTER_ASSERT(reporter, !isVolatile);
+        REPORTER_ASSERT(reporter, key4.isValid());
+        REPORTER_ASSERT(reporter, key3 != key4);
+
+        {
+            GrUniqueKey tempKey;
+            path3.setIsVolatile(true);
+            GrPath::ComputeKey(GrShape(path3, style), &key1, &isVolatile);
+            REPORTER_ASSERT(reporter, isVolatile);
+            REPORTER_ASSERT(reporter, !tempKey.isValid());
+        }
+    }
 }
 
 #endif