Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit 764e546

Browse files
bsalomonSkia Commit-Bot
authored andcommitted
Fix op chaining painter's order violation in GrRenderTargetOpList.
Add unit test of op chaining. Relax bounds checks in op merging/chaining to only check bounds against heads of op chains. Change-Id: I714435913b901c0a068bc7233ca30f2ab7916c2e Reviewed-on: https://skia-review.googlesource.com/148380 Reviewed-by: Brian Osman <brianosman@google.com> Commit-Queue: Brian Salomon <bsalomon@google.com>
1 parent a5a2622 commit 764e546

File tree

4 files changed

+187
-8
lines changed

4 files changed

+187
-8
lines changed

gn/tests.gni

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,7 @@ tests_sources = [
146146
"$_tests/MipMapTest.cpp",
147147
"$_tests/NonlinearBlendingTest.cpp",
148148
"$_tests/OnceTest.cpp",
149+
"$_tests/OpChainTest.cpp",
149150
"$_tests/OSPathTest.cpp",
150151
"$_tests/OverAlignedTest.cpp",
151152
"$_tests/PackBitsTest.cpp",

src/gpu/GrRenderTargetOpList.cpp

Lines changed: 38 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -379,8 +379,11 @@ uint32_t GrRenderTargetOpList::recordOp(std::unique_ptr<GrOp> op,
379379
case GrOp::CombineResult::kCannotCombine:
380380
break;
381381
}
382-
// Stop going backwards if we would cause a painter's order violation.
383-
if (!can_reorder(fRecordedOps.fromBack(i).fOp->bounds(), op->bounds())) {
382+
// Stop going backwards if we would cause a painter's order violation. We only need to
383+
// test against chain heads as elements of a chain always draw in their chain head's
384+
// slot.
385+
if (candidate.fOp->isChainHead() &&
386+
!can_reorder(candidate.fOp->bounds(), op->bounds())) {
384387
GrOP_INFO("\t\tBackward: Intersects with (%s, opID: %u)\n", candidate.fOp->name(),
385388
candidate.fOp->uniqueID());
386389
break;
@@ -400,8 +403,33 @@ uint32_t GrRenderTargetOpList::recordOp(std::unique_ptr<GrOp> op,
400403
SkDEBUGCODE(fNumClips++;)
401404
}
402405
if (firstChainableIdx >= 0) {
403-
GrOP_INFO("\t\t\tBackward: Chained\n");
404-
fRecordedOps.fromBack(firstChainableIdx).fOp->setNextInChain(op.get());
406+
// If we chain this op it will draw in the slot of the head of the chain. We have to check
407+
// that the new op's bounds don't intersect any of the other ops between firstChainableIdx
408+
// and the head of that op's chain. We only need to test against chain heads as elements of
409+
// a chain always draw in their chain head's slot.
410+
const GrOp* chainHead = fRecordedOps.fromBack(firstChainableIdx).fOp->chainHead();
411+
int idx = firstChainableIdx;
412+
bool chain = true;
413+
while (fRecordedOps.fromBack(idx).fOp.get() != chainHead) {
414+
// If idx is not in the same chain then we have to check against its bounds as we will
415+
// draw before it (when chainHead draws).
416+
const GrOp* testOp = fRecordedOps.fromBack(idx).fOp.get();
417+
if (testOp->isChainHead() && !can_reorder(testOp->bounds(), op->bounds())) {
418+
GrOP_INFO("\t\tBackward: Intersects with (%s, opID: %u). Cannot chain.\n",
419+
testOp->name(), testOp->uniqueID());
420+
chain = false;
421+
break;
422+
}
423+
++idx;
424+
// We must encounter the chain head before running off the beginning of the list.
425+
SkASSERT(idx < fRecordedOps.count());
426+
}
427+
if (chain) {
428+
GrOp* prevOp = fRecordedOps.fromBack(firstChainableIdx).fOp.get();
429+
GrOP_INFO("\t\t\tBackward: Chained to (%s, opID: %u)\n", prevOp->name(),
430+
prevOp->uniqueID());
431+
prevOp->setNextInChain(op.get());
432+
}
405433
}
406434
fRecordedOps.emplace_back(std::move(op), clip, dstProxy);
407435
return this->uniqueID();
@@ -447,7 +475,8 @@ void GrRenderTargetOpList::forwardCombine(const GrCaps& caps) {
447475
break;
448476
}
449477
// Stop traversing if we would cause a painter's order violation.
450-
if (!can_reorder(fRecordedOps[j].fOp->bounds(), op->bounds())) {
478+
if (candidate.fOp->isChainHead() &&
479+
!can_reorder(candidate.fOp->bounds(), op->bounds())) {
451480
GrOP_INFO("\t\t%d: (%s opID: %u) -> Intersects with (%s, opID: %u)\n",
452481
i, op->name(), op->uniqueID(),
453482
candidate.fOp->name(), candidate.fOp->uniqueID());
@@ -456,10 +485,12 @@ void GrRenderTargetOpList::forwardCombine(const GrCaps& caps) {
456485
++j;
457486
if (j > maxCandidateIdx) {
458487
if (firstChainableIdx >= 0) {
459-
GrOP_INFO("\t\t\tForward: Chained\n");
488+
GrOp* nextOp = fRecordedOps[firstChainableIdx].fOp.get();
489+
GrOP_INFO("\t\t\tForward: Chained to (%s, opID: %u)\n", nextOp->name(),
490+
nextOp->uniqueID());
460491
// We have to chain i before firstChainableIdx in order to preserve their
461492
// relative order as they may overlap.
462-
fRecordedOps[i].fOp->setNextInChain(fRecordedOps[firstChainableIdx].fOp.get());
493+
fRecordedOps[i].fOp->setNextInChain(nextOp);
463494
// However we want to draw them *after* any ops that occur between them. So move
464495
// the head of the new chain to the later slot as we only execute chain heads.
465496
std::swap(fRecordedOps[i].fOp, fRecordedOps[firstChainableIdx].fOp);

src/gpu/ops/GrOp.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ class GrOp : private SkNoncopyable {
184184
public:
185185
explicit Iter(const GrOp* head) : fCurr(head) {}
186186
inline Iter& operator++() { return *this = Iter(fCurr->nextInChain()); }
187-
const OpSubclass& operator*() const { return *static_cast<const OpSubclass*>(fCurr); }
187+
const OpSubclass& operator*() const { return fCurr->cast<OpSubclass>(); }
188188
bool operator!=(const Iter& that) const { return fCurr != that.fCurr; }
189189

190190
private:
@@ -201,6 +201,8 @@ class GrOp : private SkNoncopyable {
201201
void setNextInChain(GrOp*);
202202
/** Returns true if this is the head of a chain (including a length 1 chain). */
203203
bool isChainHead() const { return !fChainHead || (fChainHead == this); }
204+
/** Gets the head op of the chain. */
205+
const GrOp* chainHead() const { return fChainHead ? fChainHead : this; }
204206
/** Returns true if this is the tail of a chain (including a length 1 chain). */
205207
bool isChainTail() const { return !fNextInChain; }
206208
/** Returns true if this is part of chain with length > 1. */

tests/OpChainTest.cpp

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
/*
2+
* Copyright 2018 Google Inc.
3+
*
4+
* Use of this source code is governed by a BSD-style license that can be
5+
* found in the LICENSE file.
6+
*/
7+
8+
#include "GrContext.h"
9+
#include "GrContextPriv.h"
10+
#include "GrMemoryPool.h"
11+
#include "GrOpFlushState.h"
12+
#include "GrRenderTargetOpList.h"
13+
#include "Test.h"
14+
#include "ops/GrOp.h"
15+
16+
// The number of configurations is 2^Permutations(kNumOps, 2) * kNumOps!. To make this any larger
17+
// we'd have to probabilistically sample the space. At four this finishes in a very reasonable
18+
// amount of time but at five it takes nearly 2 minutes in a Release build on a Z840. Four seems
19+
// like it generates sufficiently complex test cases.
20+
static constexpr int kNumOps = 4;
21+
22+
static constexpr int fact(int n) {
23+
assert(n > 0);
24+
return n > 1 ? n * fact(n - 1) : 1;
25+
}
26+
27+
// Number of possible allowable binary chainings among the kNumOps ops.
28+
static constexpr int kNumChainabilityBits = fact(kNumOps) / fact(kNumOps - 2);
29+
30+
// We store the chainability booleans as a 32 bit bitfield.
31+
GR_STATIC_ASSERT(kNumChainabilityBits <= 32);
32+
33+
static constexpr uint64_t kNumChainabilityPermutations = 1 << kNumChainabilityBits;
34+
35+
/** Is op a chainable to op b as indicated by the bitfield? */
36+
static bool is_chainable(int a, int b, uint32_t chainabilityBits) {
37+
SkASSERT(b != a);
38+
// Each index gets kNumOps - 1 contiguous bits
39+
int aOffset = a * (kNumOps - 1);
40+
// Within a's range we give one bit each other op, but not one for itself.
41+
int bIdxInA = b < a ? b : b - 1;
42+
return chainabilityBits & (1 << (aOffset + bIdxInA));
43+
}
44+
45+
namespace {
46+
/**
47+
* A simple test op. It has an integer position, p. When it executes it writes p into an array
48+
* of ints at index p and p+1. It takes a bitfield that indicates allowed pair-wise chainings.
49+
*/
50+
class TestOp : public GrOp {
51+
public:
52+
DEFINE_OP_CLASS_ID
53+
54+
static std::unique_ptr<TestOp> Make(GrContext* context, int pos, int result[],
55+
uint32_t chainability) {
56+
GrOpMemoryPool* pool = context->contextPriv().opMemoryPool();
57+
return pool->allocate<TestOp>(pos, result, chainability);
58+
}
59+
60+
const char* name() const override { return "TestOp"; }
61+
62+
void writeResult(int result[]) const { result[fPos + 1] = result[fPos] = fPos; }
63+
64+
private:
65+
friend class ::GrOpMemoryPool; // for ctor
66+
67+
TestOp(int pos, int result[], uint32_t chainability)
68+
: INHERITED(ClassID()), fPos(pos), fResult(result), fChainability(chainability) {
69+
// Each op writes 2 values (at pos and pos+1) in a (kNumOps + 1) x 1 buffer.
70+
this->setBounds(SkRect::MakeXYWH(pos, 0, 2, 1), HasAABloat::kNo, IsZeroArea::kNo);
71+
}
72+
73+
void onPrepare(GrOpFlushState*) override {}
74+
75+
void onExecute(GrOpFlushState*) override {
76+
for (auto& op : ChainRange<TestOp>(this)) {
77+
op.writeResult(fResult);
78+
}
79+
}
80+
81+
CombineResult onCombineIfPossible(GrOp* that, const GrCaps&) override {
82+
return is_chainable(fPos, that->cast<TestOp>()->fPos, fChainability)
83+
? CombineResult::kMayChain
84+
: CombineResult::kCannotCombine;
85+
}
86+
87+
int fPos;
88+
int* fResult;
89+
uint32_t fChainability;
90+
91+
typedef GrOp INHERITED;
92+
};
93+
} // namespace
94+
95+
/**
96+
* Tests adding kNumOps to an op list with all possible allowed chaining configurations. Tests
97+
* adding the ops in all possible orders and verifies that the chained executions don't violate
98+
* painter's order.
99+
*/
100+
DEF_GPUTEST(OpChainTest, reporter, /*ctxInfo*/) {
101+
auto context = GrContext::MakeMock(nullptr);
102+
SkASSERT(context);
103+
GrSurfaceDesc desc;
104+
desc.fConfig = kRGBA_8888_GrPixelConfig;
105+
desc.fWidth = kNumOps + 1;
106+
desc.fHeight = 1;
107+
desc.fFlags = kRenderTarget_GrSurfaceFlag;
108+
109+
auto proxy = context->contextPriv().proxyProvider()->createProxy(
110+
desc, kTopLeft_GrSurfaceOrigin, GrMipMapped::kNo, SkBackingFit::kExact, SkBudgeted::kNo,
111+
GrInternalSurfaceFlags::kNone);
112+
SkASSERT(proxy);
113+
proxy->instantiate(context->contextPriv().resourceProvider());
114+
int result[kNumOps + 1];
115+
int validResult[kNumOps + 1];
116+
117+
int permutation[kNumOps];
118+
for (int i = 0; i < kNumOps; ++i) {
119+
permutation[i] = i;
120+
}
121+
do {
122+
for (uint32_t chainabilityBits = 0; chainabilityBits < kNumChainabilityPermutations;
123+
++chainabilityBits) {
124+
GrTokenTracker tracker;
125+
GrOpFlushState flushState(context->contextPriv().getGpu(),
126+
context->contextPriv().resourceProvider(), &tracker);
127+
GrRenderTargetOpList opList(context->contextPriv().resourceProvider(),
128+
sk_ref_sp(context->contextPriv().opMemoryPool()),
129+
proxy->asRenderTargetProxy(),
130+
context->contextPriv().getAuditTrail());
131+
std::fill(result, result + kNumOps + 1, -1);
132+
std::fill(validResult, validResult + kNumOps + 1, -1);
133+
for (int i = 0; i < kNumOps; ++i) {
134+
auto op = TestOp::Make(context.get(), permutation[i], result, chainabilityBits);
135+
op->writeResult(validResult);
136+
opList.addOp(std::move(op), *context->contextPriv().caps());
137+
}
138+
opList.makeClosed(*context->contextPriv().caps());
139+
opList.prepare(&flushState);
140+
opList.execute(&flushState);
141+
opList.endFlush();
142+
REPORTER_ASSERT(reporter, std::equal(result, result + kNumOps + 1, validResult));
143+
}
144+
} while (std::next_permutation(permutation, permutation + kNumOps));
145+
}

0 commit comments

Comments
 (0)