Fix DMSAA RRectOp bug with skew matrices
This fixes both the new GM (Justin's reduced repro case) and the original Chrome repro case (viewing a specific PDF).
Bug: 1442854
Change-Id: I5861b39556922617ed4b6569d0db51fa8ab33d79
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/702841
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
diff --git a/gm/scaledrects.cpp b/gm/scaledrects.cpp
new file mode 100644
index 0000000..81fb3e7
--- /dev/null
+++ b/gm/scaledrects.cpp
@@ -0,0 +1,67 @@
+/*
+ * Copyright 2023 Google LLC
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "gm/gm.h"
+#include "include/core/SkCanvas.h"
+#include "include/core/SkPaint.h"
+#include "include/core/SkRect.h"
+#include "include/core/SkString.h"
+
+namespace skiagm {
+
+// From crbug.com/1442854. Draws two rects (equivalent in device space) but which vary wildly
+// in their sizes and scales. In both cases the clip is what is actually determining the
+// final drawn geometry. For the red rectangle case the inverted skews were becoming very small and
+// ran afoul of some logic in the DMSAA code that zeroed them out.
+class ScaledRectsGM : public GM {
+public:
+ ScaledRectsGM() {
+ this->setBGColor(0xFFCCCCCC);
+ }
+
+protected:
+ SkString onShortName() override {
+ return SkString("scaledrects");
+ }
+
+ SkISize onISize() override {
+ return SkISize::Make(128, 64);
+ }
+
+ void onDraw(SkCanvas* canvas) override {
+ canvas->clipRect(SkRect::MakeXYWH(10, 50, 100, 10));
+
+ {
+ SkPaint blue;
+ blue.setColor(SK_ColorBLUE);
+
+ canvas->setMatrix(SkMatrix::MakeAll( 3.0f, -0.5f, 0.0f,
+ -0.5f, -3.0f, 0.0f,
+ 0.0f, 0.0f, 1.0f));
+
+ canvas->drawRect(SkRect::MakeXYWH(-1000, -1000, 2000, 2000), blue);
+ }
+
+ {
+ SkPaint red;
+ red.setColor(SK_ColorRED);
+ red.setBlendMode(SkBlendMode::kPlus);
+
+ canvas->setMatrix(SkMatrix::MakeAll(3000.0f, -500.0f, 0.0f,
+ -500.0f, -3000.0f, 0.0f,
+ 0.0f, 0.0f, 1.0f));
+
+ canvas->drawRect(SkRect::MakeXYWH(-1, -1, 2, 2), red);
+ }
+ }
+};
+
+//////////////////////////////////////////////////////////////////////////////
+
+DEF_GM(return new ScaledRectsGM;)
+
+} // namespace skiagm
diff --git a/gn/gm.gni b/gn/gm.gni
index a8838b3..9016af8 100644
--- a/gn/gm.gni
+++ b/gn/gm.gni
@@ -330,6 +330,7 @@
"$_gm/savelayer.cpp",
"$_gm/scaledemoji.cpp",
"$_gm/scaledemoji_rendering.cpp",
+ "$_gm/scaledrects.cpp",
"$_gm/scaledstrokes.cpp",
"$_gm/shadermaskfilter.cpp",
"$_gm/shadertext3.cpp",
diff --git a/src/gpu/ganesh/GrDrawingManager.cpp b/src/gpu/ganesh/GrDrawingManager.cpp
index 9207b10..bea464b 100644
--- a/src/gpu/ganesh/GrDrawingManager.cpp
+++ b/src/gpu/ganesh/GrDrawingManager.cpp
@@ -229,11 +229,11 @@
bool GrDrawingManager::executeRenderTasks(GrOpFlushState* flushState) {
#if GR_FLUSH_TIME_OP_SPEW
- SkDebugf("Flushing %d opsTasks\n", fDAG.count());
- for (int i = 0; i < fDAG.count(); ++i) {
+ SkDebugf("Flushing %d opsTasks\n", fDAG.size());
+ for (int i = 0; i < fDAG.size(); ++i) {
if (fDAG[i]) {
SkString label;
- label.printf("task %d/%d", i, fDAG.count());
+ label.printf("task %d/%d", i, fDAG.size());
fDAG[i]->dump(label, {}, true, true);
}
}
diff --git a/src/gpu/ganesh/ops/FillRRectOp.cpp b/src/gpu/ganesh/ops/FillRRectOp.cpp
index 5ed3b34..601aa2c 100644
--- a/src/gpu/ganesh/ops/FillRRectOp.cpp
+++ b/src/gpu/ganesh/ops/FillRRectOp.cpp
@@ -33,6 +33,26 @@
namespace {
+// Note: Just checking m.restStaysRect is not sufficient
+bool skews_are_relevant(const SkMatrix& m) {
+ SkASSERT(!m.hasPerspective());
+
+ if (m[SkMatrix::kMSkewX] == 0.0f && m[SkMatrix::kMSkewY] == 0.0f) {
+ return false;
+ }
+
+ static constexpr float kTol = SK_ScalarNearlyZero;
+ float absScaleX = SkScalarAbs(m[SkMatrix::kMScaleX]);
+ float absSkewX = SkScalarAbs(m[SkMatrix::kMSkewX]);
+ float absScaleY = SkScalarAbs(m[SkMatrix::kMScaleY]);
+ float absSkewY = SkScalarAbs(m[SkMatrix::kMSkewY]);
+
+ // The maximum absolute column sum norm of the upper left 2x2
+ float norm = std::max(absScaleX + absSkewY, absSkewX + absScaleY);
+
+ return absSkewX > kTol * norm || absSkewY > kTol * norm;
+}
+
class FillRRectOpImpl final : public GrMeshDrawOp {
private:
using Helper = GrSimpleMeshDrawOpHelper;
@@ -76,6 +96,10 @@
GrProcessorSet::Analysis finalize(const GrCaps&, const GrAppliedClip*, GrClampType) override;
CombineResult onCombineIfPossible(GrOp*, SkArenaAlloc*, const GrCaps&) override;
+#if GR_TEST_UTILS
+ SkString onDumpInfo() const override;
+#endif
+
void visitProxies(const GrVisitProxyFunc& func) const override {
if (fProgramInfo) {
fProgramInfo->visitFPProxies(func);
@@ -259,8 +283,8 @@
}
clipToView.preConcat(clipMatrix);
SkASSERT(!clipToView.hasPerspective());
- if (!SkScalarNearlyZero(clipToView.getSkewX()) ||
- !SkScalarNearlyZero(clipToView.getSkewY())) {
+
+ if (skews_are_relevant(clipToView)) {
// A rect in "clipMatrix" space is not a rect in "viewMatrix" space.
return ClipResult::kFail;
}
@@ -355,6 +379,24 @@
return CombineResult::kMerged;
}
+#if GR_TEST_UTILS
+SkString FillRRectOpImpl::onDumpInfo() const {
+ SkString str = SkStringPrintf("# instances: %u\n", fInstanceCount);
+ str += fHelper.dumpInfo();
+ int i = 0;
+ for (Instance* tmp = fHeadInstance; tmp; tmp = tmp->fNext, ++i) {
+ str.appendf("%d: Color: [%.2f, %.2f, %.2f, %.2f] ",
+ i, tmp->fColor.fR, tmp->fColor.fG, tmp->fColor.fB, tmp->fColor.fA);
+ SkMatrix m = tmp->fViewMatrix;
+ str.appendf("ViewMatrix: [%.2f, %.2f, %.2f, %.2f, %.2f, %.2f, %.2f, %.2f, %.2f] ",
+ m[0], m[1], m[2], m[3], m[4], m[5], m[6], m[7], m[8]);
+ SkRect r = tmp->fRRect.rect();
+ str.appendf("Rect: [%f %f %f %f]\n", r.fLeft, r.fTop, r.fRight, r.fBottom);
+ }
+ return str;
+}
+#endif
+
class FillRRectOpImpl::Processor final : public GrGeometryProcessor {
public:
static GrGeometryProcessor* Make(SkArenaAlloc* arena, GrAAType aaType, ProcessorFlags flags) {
diff --git a/tools/flags/CommonFlagsConfig.cpp b/tools/flags/CommonFlagsConfig.cpp
index 0e9a24de..bdedb97 100644
--- a/tools/flags/CommonFlagsConfig.cpp
+++ b/tools/flags/CommonFlagsConfig.cpp
@@ -123,6 +123,7 @@
{ "mtlf16norm", "gpu", "api=metal,color=f16norm" },
{ "mtlsrgba", "gpu", "api=metal,color=srgba"},
{ "mtl1010102", "gpu", "api=metal,color=1010102" },
+ { "mtl_dmsaa", "gpu", "api=metal,dmsaa=true" },
{ "mtlmsaa4", "gpu", "api=metal,samples=4" },
{ "mtlmsaa8", "gpu", "api=metal,samples=8" },
{ "mtlddl", "gpu", "api=metal,useDDLSink=true" },
