[Reland] Fix NVPR assert for equivalent ovals

For oval paths, GrPath ignores the point order and only uses the bounds
when building its key.  This is problematic because

1) point order is important when dashing
2) GrStencilAndCoverPathRenderer asserts that the lookup SkPath is equal
   to the cached SkPath - which is not the case for ovals with different
   directions/different point order.

With this CL we no longer use the reduced oval key when dashing, and
instead fall through to the more general path cases.  The assert is
adjusted to accommodate "equivalent" ovals (when not dashing).

Also re-enabled & updated the GpuDrawPath unit test (disabled in
https://codereview.chromium.org/1456463003/, presumably due to the use
of uninitialized SkRects).

R=bsalomon@google.com,robertphillips@google.com,cdalton@nvidia.com

Review URL: https://codereview.chromium.org/1457073002

diff --git a/src/gpu/GrPath.cpp b/src/gpu/GrPath.cpp
index 4e1119d..8ac356d 100644 (file)
--- a/src/gpu/GrPath.cpp
+++ b/src/gpu/GrPath.cpp
@@ -36,7 +36,8 @@ inline static bool compute_key_for_line_path(const SkPath& path, const GrStrokeI
 inline static bool compute_key_for_oval_path(const SkPath& path, const GrStrokeInfo& stroke,
                                              GrUniqueKey* key) {
     SkRect rect;
-    if (!path.isOval(&rect)) {
+    // Point order is significant when dashing, so we cannot devolve to a rect key.
+    if (stroke.isDashed() || !path.isOval(&rect)) {
         return false;
     }
     static_assert((sizeof(rect) % sizeof(uint32_t)) == 0 && sizeof(rect) > sizeof(uint32_t),
@@ -171,3 +172,20 @@ void GrPath::ComputeKey(const SkPath& path, const GrStrokeInfo& stroke, GrUnique
     *outIsVolatile = path.isVolatile();
 }
 
+#ifdef SK_DEBUG
+bool GrPath::isEqualTo(const SkPath& path, const GrStrokeInfo& stroke) const {
+    if (!fStroke.hasEqualEffect(stroke)) {
+        return false;
+    }
+
+    // We treat same-rect ovals as identical - but only when not dashing.
+    SkRect ovalBounds;
+    if (!fStroke.isDashed() && fSkPath.isOval(&ovalBounds)) {
+        SkRect otherOvalBounds;
+        return path.isOval(&otherOvalBounds) && ovalBounds == otherOvalBounds;
+    }
+
+    return fSkPath == path;
+}
+#endif
+

diff --git a/src/gpu/GrPath.h b/src/gpu/GrPath.h
index f74baf3..2edfd4c 100644 (file)
--- a/src/gpu/GrPath.h
+++ b/src/gpu/GrPath.h
@@ -36,9 +36,7 @@ public:
     const SkRect& getBounds() const { return fBounds; }
 
 #ifdef SK_DEBUG
-    bool isEqualTo(const SkPath& path, const GrStrokeInfo& stroke) {
-        return fSkPath == path && fStroke.hasEqualEffect(stroke);
-    }
+    bool isEqualTo(const SkPath& path, const GrStrokeInfo& stroke) const;
 #endif
 
 protected:

diff --git a/tests/GpuDrawPathTest.cpp b/tests/GpuDrawPathTest.cpp
index f23f5ef..3702a7c 100644 (file)
--- a/tests/GpuDrawPathTest.cpp
+++ b/tests/GpuDrawPathTest.cpp
@@ -15,6 +15,8 @@
 #include "SkCanvas.h"
 #include "SkColor.h"
 #include "SkPaint.h"
+#include "SkPath.h"
+#include "SkDashPathEffect.h"
 #include "SkRRect.h"
 #include "SkRect.h"
 #include "SkSurface.h"
@@ -23,11 +25,12 @@
 static void test_drawPathEmpty(skiatest::Reporter*, SkCanvas* canvas) {
     // Filling an empty path should not crash.
     SkPaint paint;
-    canvas->drawRect(SkRect(), paint);
+    SkRect emptyRect = SkRect::MakeEmpty();
+    canvas->drawRect(emptyRect, paint);
     canvas->drawPath(SkPath(), paint);
-    canvas->drawOval(SkRect(), paint);
-    canvas->drawRect(SkRect(), paint);
-    canvas->drawRRect(SkRRect(), paint);
+    canvas->drawOval(emptyRect, paint);
+    canvas->drawRect(emptyRect, paint);
+    canvas->drawRRect(SkRRect::MakeRect(emptyRect), paint);
 
     // Stroking an empty path should not crash.
     paint.setAntiAlias(true);
@@ -35,15 +38,44 @@ static void test_drawPathEmpty(skiatest::Reporter*, SkCanvas* canvas) {
     paint.setColor(SK_ColorGRAY);
     paint.setStrokeWidth(SkIntToScalar(20));
     paint.setStrokeJoin(SkPaint::kRound_Join);
-    canvas->drawRect(SkRect(), paint);
+    canvas->drawRect(emptyRect, paint);
     canvas->drawPath(SkPath(), paint);
-    canvas->drawOval(SkRect(), paint);
-    canvas->drawRect(SkRect(), paint);
-    canvas->drawRRect(SkRRect(), paint);
+    canvas->drawOval(emptyRect, paint);
+    canvas->drawRect(emptyRect, paint);
+    canvas->drawRRect(SkRRect::MakeRect(emptyRect), paint);
 }
 
+static void fill_and_stroke(SkCanvas* canvas, const SkPath& p1, const SkPath& p2,
+                            SkPathEffect* effect) {
+    SkPaint paint;
+    paint.setAntiAlias(true);
+    paint.setPathEffect(effect);
+
+    canvas->drawPath(p1, paint);
+    canvas->drawPath(p2, paint);
+
+    paint.setStyle(SkPaint::kStroke_Style);
+    canvas->drawPath(p1, paint);
+    canvas->drawPath(p2, paint);
+}
+
+static void test_drawSameRectOvals(skiatest::Reporter*, SkCanvas* canvas) {
+    // Drawing ovals with similar bounds but different points order should not crash.
+
+    SkPath oval1, oval2;
+    const SkRect rect = SkRect::MakeWH(100, 50);
+    oval1.addOval(rect, SkPath::kCW_Direction);
+    oval2.addOval(rect, SkPath::kCCW_Direction);
+
+    fill_and_stroke(canvas, oval1, oval2, nullptr);
+
+    const SkScalar intervals[] = { 1, 1 };
+    SkAutoTUnref<SkPathEffect> dashEffect(SkDashPathEffect::Create(intervals, 2, 0));
+    fill_and_stroke(canvas, oval1, oval2, dashEffect);
+}
 
 DEF_GPUTEST(GpuDrawPath, reporter, factory) {
+    // https://bugs.chromium.org/p/skia/issues/detail?id=4581
     return;
 
     for (int type = 0; type < GrContextFactory::kLastGLContextType; ++type) {
@@ -66,4 +98,16 @@ DEF_GPUTEST(GpuDrawPath, reporter, factory) {
     }
 }
 
+DEF_GPUTEST(GpuDrawPathSameRectOvals, reporter, factory) {
+    GrContext* grContext = factory->get(GrContextFactory::kNVPR_GLContextType);
+    if (!grContext) {
+        return;
+    }
+
+    SkAutoTUnref<SkSurface> surface(
+        SkSurface::NewRenderTarget(grContext, SkSurface::kNo_Budgeted,
+                                   SkImageInfo::MakeN32Premul(255, 255), 4));
+    test_drawSameRectOvals(reporter, surface->getCanvas());
+}
+
 #endif