Fix overdraw from unstable perspective math
There were two issues leading to the corruption seen in the linked
chromium issue.
1. The draw's bounds were calculated based on the quad being clipped
to w>= epsilon, which is what happens when the AA inset/outset is done.
But for non-aa quads, the fillrect and texture ops did no clipping,
assuming that the GPU would be sufficient. However, this can produce
non-aa draws that exceed the calculated bounds, misleading the clip
stack into incorrectly removing the scissor, etc.
2. Precision issues within CropToRect meant some perspective quads'
barycentric coordinates would become degenerate and compute to (0,0,1),
making it appear as if the render target/scissor were contained within
it. This meant we'd turn it into a rectangular clear.
These changes appear to address the corruption on Linux and Windows, but
there are still rendering artifacts from poor aa inset/outset
calculations. These artifacts are at least limited to the clip properly.
A better rendering method that does not rely on line intersections
will address these artifacts, but this CL is a reasonable temporary
mitigation.
Bug: chromium:1204347
Change-Id: I3c67d4efe70313ae7c98abc0a57b5b047c83890d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/407821
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
diff --git a/src/gpu/geometry/GrQuadUtils.cpp b/src/gpu/geometry/GrQuadUtils.cpp
index d8c48c7..f30f2a4 100644
--- a/src/gpu/geometry/GrQuadUtils.cpp
+++ b/src/gpu/geometry/GrQuadUtils.cpp
@@ -574,24 +574,16 @@
return true;
}
- if (computeLocal) {
+ if (computeLocal || quad->fDevice.quadType() == GrQuad::Type::kPerspective) {
// FIXME (michaelludwig) Calculate cropped local coordinates when not kAxisAligned
+ // FIXME (michaelludwig) crbug.com/1204347 and skbug.com/9906 - disable this when there's
+ // perspective; it does not prove numerical robust enough in the wild and should be
+ // revisited.
return false;
}
V4f devX = quad->fDevice.x4f();
V4f devY = quad->fDevice.y4f();
- // Project the 3D coordinates to 2D
- if (quad->fDevice.quadType() == GrQuad::Type::kPerspective) {
- V4f devW = quad->fDevice.w4f();
- if (any(devW < SkPathPriv::kW0PlaneDistance)) {
- // The rest of this function assumes the quad is in front of w = 0
- return false;
- }
- devW = 1.f / devW;
- devX *= devW;
- devY *= devW;
- }
V4f clipX = {cropRect.fLeft, cropRect.fLeft, cropRect.fRight, cropRect.fRight};
V4f clipY = {cropRect.fTop, cropRect.fBottom, cropRect.fTop, cropRect.fBottom};
diff --git a/src/gpu/ops/GrFillRectOp.cpp b/src/gpu/ops/GrFillRectOp.cpp
index 93052d2..d24b360 100644
--- a/src/gpu/ops/GrFillRectOp.cpp
+++ b/src/gpu/ops/GrFillRectOp.cpp
@@ -88,10 +88,8 @@
IsHairline::kNo);
DrawQuad extra;
- // Only clip when there's anti-aliasing. When non-aa, the GPU clips just fine and there's
- // no inset/outset math that requires w > 0.
- int count = quad->fEdgeFlags != GrQuadAAFlags::kNone ? GrQuadUtils::ClipToW0(quad, &extra)
- : 1;
+ // Always crop to W>0 to remain consistent with GrQuad::bounds()
+ int count = GrQuadUtils::ClipToW0(quad, &extra);
if (count == 0) {
// We can't discard the op at this point, but disable AA flags so it won't go through
// inset/outset processing
diff --git a/src/gpu/ops/GrTextureOp.cpp b/src/gpu/ops/GrTextureOp.cpp
index 844b425..ee268c3 100644
--- a/src/gpu/ops/GrTextureOp.cpp
+++ b/src/gpu/ops/GrTextureOp.cpp
@@ -624,11 +624,8 @@
int appendQuad(DrawQuad* quad, const SkPMColor4f& color, const SkRect& subset) {
DrawQuad extra;
- // Only clip when there's anti-aliasing. When non-aa, the GPU clips just fine and there's
- // no inset/outset math that requires w > 0.
- int quadCount = quad->fEdgeFlags != GrQuadAAFlags::kNone
- ? GrQuadUtils::ClipToW0(quad, &extra)
- : 1;
+ // Always clip to W0 to stay consistent with GrQuad::bounds
+ int quadCount = GrQuadUtils::ClipToW0(quad, &extra);
if (quadCount == 0) {
// We can't discard the op at this point, but disable AA flags so it won't go through
// inset/outset processing
diff --git a/tests/GrQuadCropTest.cpp b/tests/GrQuadCropTest.cpp
index 0b65df4..2807632 100644
--- a/tests/GrQuadCropTest.cpp
+++ b/tests/GrQuadCropTest.cpp
@@ -134,10 +134,22 @@
}
} else {
// Since no local coordinates were provided, and the input draw geometry is known to
- // fully cover the crop rect, the quad should be updated to match cropRect exactly
+ // fully cover the crop rect, the quad should be updated to match cropRect exactly,
+ // unless it's perspective in which case we don't do anything since the code isn't
+ // numerically robust enough.
+ DrawQuad originalQuad = quad;
bool exact = GrQuadUtils::CropToRect(kDrawRect, clipAA, &quad, /* calc. local */ false);
- ASSERTF(exact, "Expected crop to be exact");
+ if (originalQuad.fDevice.quadType() == GrQuad::Type::kPerspective) {
+ ASSERTF(!exact, "Expected no change for perspective");
+ for (int i = 0; i < 4; ++i) {
+ ASSERTF(originalQuad.fDevice.x(i) == quad.fDevice.x(i));
+ ASSERTF(originalQuad.fDevice.y(i) == quad.fDevice.y(i));
+ ASSERTF(originalQuad.fDevice.w(i) == quad.fDevice.w(i));
+ }
+ return;
+ }
+ ASSERTF(exact, "Expected crop to be exact");
GrQuadAAFlags expectedFlags = clipAA == GrAA::kYes ? GrQuadAAFlags::kAll
: GrQuadAAFlags::kNone;
ASSERTF(expectedFlags == quad.fEdgeFlags,
