[M85 Cherry-pick] Fix bug(s) in combining and chaining GrTextureOps
The repro case for this bug is where we have ~700 non-AA texture ops
with a few coverage-AA texture ops sprinkled throughout. In the old
system this could result in the quad limits being exceeded bc the
AA type for the entire run was believed to be non-AA during the creation
phase only to find that it was actually coverage-AA at execution time.
This problem manifested in both the bulk creation method and the
1-by-1/chaining creation method.
Note: in the original repro case every texture op has its own separate
texture so the problem manifests in the chaining case.
Bug: 1108475
Change-Id: Ie443af4fb8cbaf5d1b3807931ceb8b687d7870ad
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/307697
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
diff --git a/src/gpu/ops/GrTextureOp.cpp b/src/gpu/ops/GrTextureOp.cpp
index ee71291..94bd158 100644
--- a/src/gpu/ops/GrTextureOp.cpp
+++ b/src/gpu/ops/GrTextureOp.cpp
@@ -740,34 +740,45 @@
}
#ifdef SK_DEBUG
+ static int validate_op(GrTextureType textureType,
+ GrAAType aaType,
+ GrSwizzle swizzle,
+ const TextureOp* op) {
+ SkASSERT(op->fMetadata.fSwizzle == swizzle);
+
+ int quadCount = 0;
+ for (unsigned p = 0; p < op->fMetadata.fProxyCount; ++p) {
+ auto* proxy = op->fViewCountPairs[p].fProxy->asTextureProxy();
+ quadCount += op->fViewCountPairs[p].fQuadCnt;
+ SkASSERT(proxy);
+ SkASSERT(proxy->textureType() == textureType);
+ }
+
+ SkASSERT(aaType == op->fMetadata.aaType());
+ return quadCount;
+ }
+
void validate() const override {
// NOTE: Since this is debug-only code, we use the virtual asTextureProxy()
auto textureType = fViewCountPairs[0].fProxy->asTextureProxy()->textureType();
GrAAType aaType = fMetadata.aaType();
+ GrSwizzle swizzle = fMetadata.fSwizzle;
- int quadCount = 0;
- for (const auto& op : ChainRange<TextureOp>(this)) {
- SkASSERT(op.fMetadata.fSwizzle == fMetadata.fSwizzle);
+ int quadCount = validate_op(textureType, aaType, swizzle, this);
- for (unsigned p = 0; p < op.fMetadata.fProxyCount; ++p) {
- auto* proxy = op.fViewCountPairs[p].fProxy->asTextureProxy();
- quadCount += op.fViewCountPairs[p].fQuadCnt;
- SkASSERT(proxy);
- SkASSERT(proxy->textureType() == textureType);
- }
+ for (const GrOp* tmp = this->prevInChain(); tmp; tmp = tmp->prevInChain()) {
+ quadCount += validate_op(textureType, aaType, swizzle,
+ static_cast<const TextureOp*>(tmp));
+ }
- // Each individual op must be a single aaType. kCoverage and kNone ops can chain
- // together but kMSAA ones do not.
- if (aaType == GrAAType::kCoverage || aaType == GrAAType::kNone) {
- SkASSERT(op.fMetadata.aaType() == GrAAType::kCoverage ||
- op.fMetadata.aaType() == GrAAType::kNone);
- } else {
- SkASSERT(aaType == GrAAType::kMSAA && op.fMetadata.aaType() == GrAAType::kMSAA);
- }
+ for (const GrOp* tmp = this->nextInChain(); tmp; tmp = tmp->nextInChain()) {
+ quadCount += validate_op(textureType, aaType, swizzle,
+ static_cast<const TextureOp*>(tmp));
}
SkASSERT(quadCount == this->numChainedQuads());
}
+
#endif
#if GR_TEST_UTILS
@@ -775,6 +786,8 @@
#endif
void characterize(Desc* desc) const {
+ SkDEBUGCODE(this->validate();)
+
GrQuad::Type quadType = GrQuad::Type::kAxisAligned;
ColorType colorType = ColorType::kNone;
GrQuad::Type srcQuadType = GrQuad::Type::kAxisAligned;
@@ -926,11 +939,32 @@
SkASSERT(numDraws == fDesc->fNumProxies);
}
+ void propagateCoverageAAThroughoutChain() {
+ fMetadata.fAAType = static_cast<uint16_t>(GrAAType::kCoverage);
+
+ for (GrOp* tmp = this->prevInChain(); tmp; tmp = tmp->prevInChain()) {
+ TextureOp* tex = static_cast<TextureOp*>(tmp);
+ SkASSERT(tex->fMetadata.aaType() == GrAAType::kCoverage ||
+ tex->fMetadata.aaType() == GrAAType::kNone);
+ tex->fMetadata.fAAType = static_cast<uint16_t>(GrAAType::kCoverage);
+ }
+
+ for (GrOp* tmp = this->nextInChain(); tmp; tmp = tmp->nextInChain()) {
+ TextureOp* tex = static_cast<TextureOp*>(tmp);
+ SkASSERT(tex->fMetadata.aaType() == GrAAType::kCoverage ||
+ tex->fMetadata.aaType() == GrAAType::kNone);
+ tex->fMetadata.fAAType = static_cast<uint16_t>(GrAAType::kCoverage);
+ }
+ }
+
CombineResult onCombineIfPossible(GrOp* t, GrRecordingContext::Arenas*,
const GrCaps& caps) override {
TRACE_EVENT0("skia.gpu", TRACE_FUNC);
const auto* that = t->cast<TextureOp>();
+ SkDEBUGCODE(this->validate();)
+ SkDEBUGCODE(that->validate();)
+
if (fDesc || that->fDesc) {
// This should never happen (since only DDL recorded ops should be prePrepared)
// but, in any case, we should never combine ops that that been prePrepared
@@ -976,7 +1010,16 @@
thisProxy != thatProxy) {
// We can't merge across different proxies. Check if 'this' can be chained with 'that'.
if (GrTextureProxy::ProxiesAreCompatibleAsDynamicState(thisProxy, thatProxy) &&
- caps.dynamicStateArrayGeometryProcessorTextureSupport()) {
+ caps.dynamicStateArrayGeometryProcessorTextureSupport() &&
+ fMetadata.aaType() == that->fMetadata.aaType()) {
+ // We only allow chaining when the aaTypes match bc otherwise the AA type
+ // reported by the chain can be inconsistent. That is, since chaining doesn't
+ // propagate revised AA information throughout the chain, the head of the chain
+ // could have an AA setting of kNone while the chain as a whole could have a
+ // setting of kCoverage. This inconsistency would then interfere with the validity
+ // of the CombinedQuadCountWillOverflow calls.
+ // This problem doesn't occur w/ merging bc we do propagate the AA information
+ // (in propagateCoverageAAThroughoutChain) below.
return CombineResult::kMayChain;
}
return CombineResult::kCannotCombine;
@@ -984,18 +1027,20 @@
fMetadata.fSubset |= that->fMetadata.fSubset;
fMetadata.fColorType = std::max(fMetadata.fColorType, that->fMetadata.fColorType);
- if (upgradeToCoverageAAOnMerge) {
- fMetadata.fAAType = static_cast<uint16_t>(GrAAType::kCoverage);
- }
// Concatenate quad lists together
fQuads.concat(that->fQuads);
fViewCountPairs[0].fQuadCnt += that->fQuads.count();
fMetadata.fTotalQuadCount += that->fQuads.count();
+ if (upgradeToCoverageAAOnMerge) {
+ this->propagateCoverageAAThroughoutChain();
+ }
+
+ SkDEBUGCODE(this->validate();)
+
return CombineResult::kMerged;
}
-
GrQuadBuffer<ColorSubsetAndAA> fQuads;
sk_sp<GrColorSpaceXform> fTextureColorSpaceXform;
// Most state of TextureOp is packed into these two field to minimize the op's size.
@@ -1218,7 +1263,8 @@
for (int i = 0; i < state.numLeft(); ++i) {
int absIndex = state.baseIndex() + i;
- if (set[absIndex].fAAFlags != GrQuadAAFlags::kNone) {
+ if (set[absIndex].fAAFlags != GrQuadAAFlags::kNone ||
+ runningAA == GrAAType::kCoverage) {
if (i >= GrResourceProvider::MaxNumAAQuads()) {
// Here we either need to boost the AA type to kCoverage, but doing so with
diff --git a/tests/BulkRectTest.cpp b/tests/BulkRectTest.cpp
index c65d8ae..71532b5 100644
--- a/tests/BulkRectTest.cpp
+++ b/tests/BulkRectTest.cpp
@@ -4,10 +4,12 @@
* Use of this source code is governed by a BSD-style license that can be
* found in the LICENSE file.
*/
+
#include "src/core/SkBlendModePriv.h"
#include "src/gpu/GrContextPriv.h"
#include "src/gpu/GrProxyProvider.h"
#include "src/gpu/GrRenderTargetContext.h"
+#include "src/gpu/GrRenderTargetContextPriv.h"
#include "src/gpu/ops/GrFillRectOp.h"
#include "src/gpu/ops/GrTextureOp.h"
#include "tests/Test.h"
@@ -30,15 +32,26 @@
typedef GrQuadAAFlags (*PerQuadAAFunc)(int i);
-typedef void (*BulkRectTest)(skiatest::Reporter* reporter, GrContext* context,
- PerQuadAAFunc perQuadAA, GrAAType overallAA, SkBlendMode blendMode,
- int requestedTotNumQuads, int expectedNumOps);
+typedef void (*BulkRectTest)(skiatest::Reporter*,
+ GrContext*,
+ PerQuadAAFunc,
+ GrAAType overallAA,
+ SkBlendMode,
+ bool addOneByOne,
+ bool allUniqueProxies,
+ int requestedTotNumQuads,
+ int expectedNumOps);
//-------------------------------------------------------------------------------------------------
-static void bulk_fill_rect_create_test(skiatest::Reporter* reporter, GrContext* context,
- PerQuadAAFunc perQuadAA, GrAAType overallAA,
- SkBlendMode blendMode,
- int requestedTotNumQuads, int expectedNumOps) {
+static void fillrectop_creation_test(skiatest::Reporter* reporter, GrContext* context,
+ PerQuadAAFunc perQuadAA, GrAAType overallAA,
+ SkBlendMode blendMode, bool addOneByOne,
+ bool allUniqueProxies,
+ int requestedTotNumQuads, int expectedNumOps) {
+
+ if (addOneByOne || allUniqueProxies) {
+ return;
+ }
std::unique_ptr<GrRenderTargetContext> rtc = new_RTC(context);
@@ -53,6 +66,7 @@
GrPaint paint;
paint.setXPFactory(SkBlendMode_AsXPFactory(blendMode));
+
GrFillRectOp::AddFillRectOps(rtc.get(), nullptr, context, std::move(paint), overallAA,
SkMatrix::I(), quads, requestedTotNumQuads);
@@ -77,24 +91,42 @@
}
//-------------------------------------------------------------------------------------------------
-static void bulk_texture_rect_create_test(skiatest::Reporter* reporter, GrContext* context,
- PerQuadAAFunc perQuadAA, GrAAType overallAA,
- SkBlendMode blendMode,
- int requestedTotNumQuads, int expectedNumOps) {
+static void textureop_creation_test(skiatest::Reporter* reporter, GrContext* context,
+ PerQuadAAFunc perQuadAA, GrAAType overallAA,
+ SkBlendMode blendMode, bool addOneByOne,
+ bool allUniqueProxies,
+ int requestedTotNumQuads, int expectedNumOps) {
std::unique_ptr<GrRenderTargetContext> rtc = new_RTC(context);
- sk_sp<GrSurfaceProxy> proxyA = create_proxy(context);
- sk_sp<GrSurfaceProxy> proxyB = create_proxy(context);
- GrSurfaceProxyView proxyViewA(std::move(proxyA), kTopLeft_GrSurfaceOrigin, GrSwizzle::RGBA());
- GrSurfaceProxyView proxyViewB(std::move(proxyB), kTopLeft_GrSurfaceOrigin, GrSwizzle::RGBA());
+ GrSurfaceProxyView proxyViewA, proxyViewB;
+
+ if (!allUniqueProxies) {
+ sk_sp<GrSurfaceProxy> proxyA = create_proxy(context);
+ sk_sp<GrSurfaceProxy> proxyB = create_proxy(context);
+ proxyViewA = GrSurfaceProxyView(std::move(proxyA),
+ kTopLeft_GrSurfaceOrigin,
+ GrSwizzle::RGBA());
+ proxyViewB = GrSurfaceProxyView(std::move(proxyB),
+ kTopLeft_GrSurfaceOrigin,
+ GrSwizzle::RGBA());
+ }
auto set = new GrRenderTargetContext::TextureSetEntry[requestedTotNumQuads];
for (int i = 0; i < requestedTotNumQuads; ++i) {
- // Alternate between two proxies to prevent op merging if the batch API was forced to submit
- // one op at a time (to work, this does require that all fDstRects overlap).
- set[i].fProxyView = i % 2 == 0 ? proxyViewA : proxyViewB;
+ if (!allUniqueProxies) {
+ // Alternate between two proxies to prevent op merging if the batch API was forced to
+ // submit one op at a time (to work, this does require that all fDstRects overlap).
+ set[i].fProxyView = i % 2 == 0 ? proxyViewA : proxyViewB;
+ } else {
+ // Each op gets its own proxy to force chaining only
+ sk_sp<GrSurfaceProxy> proxyA = create_proxy(context);
+ set[i].fProxyView = GrSurfaceProxyView(std::move(proxyA),
+ kTopLeft_GrSurfaceOrigin,
+ GrSwizzle::RGBA());
+ }
+
set[i].fSrcAlphaType = kPremul_SkAlphaType;
set[i].fSrcRect = SkRect::MakeWH(100.0f, 100.0f);
set[i].fDstRect = SkRect::MakeWH(100.5f, 100.5f); // prevent the int non-AA optimization
@@ -104,14 +136,42 @@
set[i].fAAFlags = perQuadAA(i);
}
- GrTextureOp::AddTextureSetOps(rtc.get(), nullptr, context, set, requestedTotNumQuads,
- requestedTotNumQuads, // We alternate so proxyCnt == cnt
- GrSamplerState::Filter::kNearest,
- GrTextureOp::Saturate::kYes,
- blendMode,
- overallAA,
- SkCanvas::kStrict_SrcRectConstraint,
- SkMatrix::I(), nullptr);
+ if (addOneByOne) {
+ for (int i = 0; i < requestedTotNumQuads; ++i) {
+ DrawQuad quad;
+
+ quad.fDevice = GrQuad::MakeFromRect(set[i].fDstRect, SkMatrix::I());
+ quad.fLocal = GrQuad(set[i].fSrcRect);
+ quad.fEdgeFlags = set[i].fAAFlags;
+
+ std::unique_ptr<GrDrawOp> op = GrTextureOp::Make(context,
+ set[i].fProxyView,
+ set[i].fSrcAlphaType,
+ nullptr,
+ GrSamplerState::Filter::kNearest,
+ set[i].fColor,
+ GrTextureOp::Saturate::kYes,
+ blendMode,
+ overallAA,
+ &quad,
+ nullptr);
+ rtc->priv().testingOnly_addDrawOp(nullptr, std::move(op));
+ }
+ } else {
+ GrTextureOp::AddTextureSetOps(rtc.get(),
+ nullptr,
+ context,
+ set,
+ requestedTotNumQuads,
+ requestedTotNumQuads, // We alternate so proxyCnt == cnt
+ GrSamplerState::Filter::kNearest,
+ GrTextureOp::Saturate::kYes,
+ blendMode,
+ overallAA,
+ SkCanvas::kStrict_SrcRectConstraint,
+ SkMatrix::I(),
+ nullptr);
+ }
GrOpsTask* opsTask = rtc->testingOnly_PeekLastOpsTask();
int actualNumOps = opsTask->numOpChains();
@@ -129,9 +189,12 @@
: GrFillRectOp::ClassID();
for (int i = 0; i < actualNumOps; ++i) {
const GrOp* tmp = opsTask->getChain(i);
- REPORTER_ASSERT(reporter, tmp->classID() == expectedOpID);
- REPORTER_ASSERT(reporter, tmp->isChainTail());
- actualTotNumQuads += ((GrDrawOp*) tmp)->numQuads();
+ REPORTER_ASSERT(reporter, allUniqueProxies || tmp->isChainTail());
+ while (tmp) {
+ REPORTER_ASSERT(reporter, tmp->classID() == expectedOpID);
+ actualTotNumQuads += ((GrDrawOp*) tmp)->numQuads();
+ tmp = tmp->nextInChain();
+ }
}
REPORTER_ASSERT(reporter, expectedNumOps == actualNumOps);
@@ -144,6 +207,7 @@
//-------------------------------------------------------------------------------------------------
static void run_test(GrContext* context, skiatest::Reporter* reporter, BulkRectTest test) {
+
// This is the simple case where there is no AA at all. We expect 2 non-AA clumps of quads.
{
auto noAA = [](int i) -> GrQuadAAFlags {
@@ -153,7 +217,7 @@
static const int kNumExpectedOps = 2;
test(reporter, context, noAA, GrAAType::kNone, SkBlendMode::kSrcOver,
- 2*GrResourceProvider::MaxNumNonAAQuads(), kNumExpectedOps);
+ false, false, 2*GrResourceProvider::MaxNumNonAAQuads(), kNumExpectedOps);
}
// This is the same as the above case except the overall AA is kCoverage. However, since
@@ -166,7 +230,7 @@
static const int kNumExpectedOps = 2;
test(reporter, context, noAA, GrAAType::kCoverage, SkBlendMode::kSrcOver,
- 2*GrResourceProvider::MaxNumNonAAQuads(), kNumExpectedOps);
+ false, false, 2*GrResourceProvider::MaxNumNonAAQuads(), kNumExpectedOps);
}
// This case has an overall AA of kCoverage but the per-quad AA alternates.
@@ -180,7 +244,7 @@
GrResourceProvider::MaxNumAAQuads();
test(reporter, context, alternateAA, GrAAType::kCoverage, SkBlendMode::kSrcOver,
- 2*GrResourceProvider::MaxNumNonAAQuads(), numExpectedOps);
+ false, false, 2*GrResourceProvider::MaxNumNonAAQuads(), numExpectedOps);
}
// In this case we have a run of MaxNumAAQuads non-AA quads and then AA quads. This
@@ -195,7 +259,7 @@
static const int kNumExpectedOps = 2;
test(reporter, context, runOfNonAA, GrAAType::kCoverage, SkBlendMode::kSrcOver,
- 2*GrResourceProvider::MaxNumAAQuads(), kNumExpectedOps);
+ false, false, 2*GrResourceProvider::MaxNumAAQuads(), kNumExpectedOps);
}
// In this case we use a blend mode other than src-over, which hits the GrFillRectOp fallback
@@ -210,14 +274,42 @@
static const int kNumExpectedOps = 2;
test(reporter, context, fixedAA, GrAAType::kCoverage, SkBlendMode::kSrcATop,
- 2*GrResourceProvider::MaxNumAAQuads(), kNumExpectedOps);
+ false, false, 2*GrResourceProvider::MaxNumAAQuads(), kNumExpectedOps);
}
+
+ // This repros crbug.com/1108475, where we create 1024 non-AA texture ops w/ one coverage-AA
+ // texture op in the middle. Because each op has its own texture, all the texture ops
+ // get chained together so the quad count can exceed the AA maximum.
+ {
+ auto onlyOneAA = [](int i) -> GrQuadAAFlags {
+ return i == 256 ? GrQuadAAFlags::kAll : GrQuadAAFlags::kNone;
+ };
+
+ static const int kNumExpectedOps = 3;
+
+ test(reporter, context, onlyOneAA, GrAAType::kCoverage, SkBlendMode::kSrcOver,
+ true, true, 1024, kNumExpectedOps);
+ }
+
+ // This repros a problem related to crbug.com/1108475. In this case, the bulk creation
+ // method had no way to break up the set of texture ops at the AA quad limit.
+ {
+ auto onlyOneAA = [](int i) -> GrQuadAAFlags {
+ return i == 256 ? GrQuadAAFlags::kAll : GrQuadAAFlags::kNone;
+ };
+
+ static const int kNumExpectedOps = 2;
+
+ test(reporter, context, onlyOneAA, GrAAType::kCoverage, SkBlendMode::kSrcOver,
+ false, true, 1024, kNumExpectedOps);
+ }
+
}
DEF_GPUTEST_FOR_RENDERING_CONTEXTS(BulkFillRectTest, reporter, ctxInfo) {
- run_test(ctxInfo.grContext(), reporter, bulk_fill_rect_create_test);
+ run_test(ctxInfo.grContext(), reporter, fillrectop_creation_test);
}
DEF_GPUTEST_FOR_RENDERING_CONTEXTS(BulkTextureRectTest, reporter, ctxInfo) {
- run_test(ctxInfo.grContext(), reporter, bulk_texture_rect_create_test);
+ run_test(ctxInfo.grContext(), reporter, textureop_creation_test);
}