Revert "Split up opLists"
This reverts commit bc8ee52d4649afdc972599e5ef2a2a543867985d.
Reason for revert: Instanced rendering is broken
Original change's description:
> Split up opLists
>
> Split into:
> https://skia-review.googlesource.com/c/11793/ (Remove lastProxy capability from GrSurface)
>
> Change-Id: I903ba30e17de4aab8cb0d2cc3281ae5c262142f9
> Reviewed-on: https://skia-review.googlesource.com/11581
> Commit-Queue: Robert Phillips <robertphillips@google.com>
> Reviewed-by: Brian Osman <brianosman@google.com>
>
TBR=egdaniel@google.com,robertphillips@google.com,brianosman@google.com,reviews@skia.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
Change-Id: Ic3ae650630a09730d40da4a9587d9e25a9dd5e6c
Reviewed-on: https://skia-review.googlesource.com/13725
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
diff --git a/include/private/GrSurfaceProxy.h b/include/private/GrSurfaceProxy.h
index 35cff20..efcec13 100644
--- a/include/private/GrSurfaceProxy.h
+++ b/include/private/GrSurfaceProxy.h
@@ -378,9 +378,9 @@
// This back-pointer is required so that we can add a dependancy between
// the opList used to create the current contents of this surface
// and the opList of a destination surface to which this one is being drawn or copied.
- // This pointer is unreffed. OpLists own a ref on their surface proxies.
GrOpList* fLastOpList;
+
typedef GrIORefProxy INHERITED;
};
diff --git a/src/gpu/GrDrawingManager.cpp b/src/gpu/GrDrawingManager.cpp
index 9c5be8e..6b2c5a2 100644
--- a/src/gpu/GrDrawingManager.cpp
+++ b/src/gpu/GrDrawingManager.cpp
@@ -26,10 +26,10 @@
void GrDrawingManager::cleanup() {
for (int i = 0; i < fOpLists.count(); ++i) {
fOpLists[i]->makeClosed(); // no opList should receive a new command after this
+ fOpLists[i]->clearTarget();
// We shouldn't need to do this, but it turns out some clients still hold onto opLists
- // after a cleanup.
- // MDB TODO: is this still true?
+ // after a cleanup
fOpLists[i]->reset();
}
@@ -83,7 +83,6 @@
// but need to be flushed anyway. Closing such GrOpLists here will mean new
// GrOpLists will be created to replace them if the SkGpuDevice(s) write to them again.
fOpLists[i]->makeClosed();
- SkDEBUGCODE(fOpLists[i]->validateTargetsSingleRenderTarget());
}
#ifdef ENABLE_MDB
@@ -116,7 +115,6 @@
if (!opList) {
continue; // Odd - but not a big deal
}
- opList->makeClosed();
SkDEBUGCODE(opList->validateTargetsSingleRenderTarget());
opList->prepareOps(&fFlushState);
if (!opList->executeOps(&fFlushState)) {
@@ -153,7 +151,17 @@
fOpLists[i]->reset();
}
+#ifndef ENABLE_MDB
+ // When MDB is disabled we keep reusing the same GrOpList
+ if (fOpLists.count()) {
+ SkASSERT(fOpLists.count() == 1);
+ // Clear out this flag so the topological sort's SkTTopoSort_CheckAllUnmarked check
+ // won't bark
+ fOpLists[0]->resetFlag(GrOpList::kWasOutput_Flag);
+ }
+#else
fOpLists.reset();
+#endif
fFlushState.reset();
// We always have to notify the cache when it requested a flush so it can reset its state.
@@ -190,12 +198,19 @@
sk_sp<GrRenderTargetOpList> GrDrawingManager::newRTOpList(sk_sp<GrRenderTargetProxy> rtp) {
SkASSERT(fContext);
- // This is a temporary fix for the partial-MDB world. In that world we're not reordering
- // so ops that (in the single opList world) would've just glommed onto the end of the single
- // opList but referred to a far earlier RT need to appear in their own opList.
- if (!fOpLists.empty()) {
- fOpLists.back()->makeClosed();
+#ifndef ENABLE_MDB
+ // When MDB is disabled we always just return the single GrOpList
+ if (fOpLists.count()) {
+ SkASSERT(fOpLists.count() == 1);
+ // In the non-MDB-world the same GrOpList gets reused for multiple render targets.
+ // Update this pointer so all the asserts are happy
+ rtp->setLastOpList(fOpLists[0].get());
+ // DrawingManager gets the creation ref - this ref is for the caller
+
+ // TODO: although this is true right now it isn't cool
+ return sk_ref_sp((GrRenderTargetOpList*) fOpLists[0].get());
}
+#endif
sk_sp<GrRenderTargetOpList> opList(new GrRenderTargetOpList(rtp,
fContext->getGpu(),
@@ -212,21 +227,19 @@
sk_sp<GrTextureOpList> GrDrawingManager::newTextureOpList(sk_sp<GrTextureProxy> textureProxy) {
SkASSERT(fContext);
- // This is a temporary fix for the partial-MDB world. In that world we're not reordering
- // so ops that (in the single opList world) would've just glommed onto the end of the single
- // opList but referred to a far earlier RT need to appear in their own opList.
- if (!fOpLists.empty()) {
- fOpLists.back()->makeClosed();
- }
-
- sk_sp<GrTextureOpList> opList(new GrTextureOpList(textureProxy, fContext->getGpu(),
+ sk_sp<GrTextureOpList> opList(new GrTextureOpList(std::move(textureProxy), fContext->getGpu(),
fContext->getAuditTrail()));
- SkASSERT(textureProxy->getLastOpList() == opList.get());
-
- fOpLists.push_back() = opList;
-
+#ifndef ENABLE_MDB
+ // When MDB is disabled we still create a new GrOpList, but don't store or ref it - we rely
+ // on the caller to immediately execute and free it.
return opList;
+#else
+ *fOpLists.append() = opList;
+
+ // Drawing manager gets the creation ref - this ref is for the caller
+ return opList;
+#endif
}
GrAtlasTextContext* GrDrawingManager::getAtlasTextContext() {
diff --git a/src/gpu/GrOpList.cpp b/src/gpu/GrOpList.cpp
index 5e618a2..91c8c79 100644
--- a/src/gpu/GrOpList.cpp
+++ b/src/gpu/GrOpList.cpp
@@ -21,7 +21,9 @@
}
GrOpList::GrOpList(sk_sp<GrSurfaceProxy> surfaceProxy, GrAuditTrail* auditTrail)
- : fTarget(surfaceProxy)
+ // MDB TODO: in the future opLists will own the GrSurfaceProxy they target.
+ // For now, preserve the status quo.
+ : fTarget(surfaceProxy.get())
, fAuditTrail(auditTrail)
, fUniqueID(CreateUniqueID())
, fFlags(0) {
diff --git a/src/gpu/GrOpList.h b/src/gpu/GrOpList.h
index 352a46c..b7dd7fa 100644
--- a/src/gpu/GrOpList.h
+++ b/src/gpu/GrOpList.h
@@ -29,7 +29,11 @@
virtual bool executeOps(GrOpFlushState* flushState) = 0;
virtual void makeClosed() {
+ // We only close GrOpLists when MDB is enabled. When MDB is disabled there is only
+ // ever one GrOpLists and all calls will be funnelled into it.
+#ifdef ENABLE_MDB
this->setFlag(kClosed_Flag);
+#endif
}
// TODO: it seems a bit odd that GrOpList has nothing to clear on reset
@@ -40,6 +44,10 @@
virtual void abandonGpuResources() = 0;
virtual void freeGpuResources() = 0;
+ // TODO: this entry point is only needed in the non-MDB world. Remove when
+ // we make the switch to MDB
+ void clearTarget() { fTarget = nullptr; }
+
bool isClosed() const { return this->isSetFlag(kClosed_Flag); }
/*
@@ -74,8 +82,8 @@
SkDEBUGCODE(virtual void validateTargetsSingleRenderTarget() const = 0;)
protected:
- sk_sp<GrSurfaceProxy> fTarget;
- GrAuditTrail* fAuditTrail;
+ GrSurfaceProxy* fTarget;
+ GrAuditTrail* fAuditTrail;
private:
friend class GrDrawingManager; // for resetFlag & TopoSortTraits
@@ -127,11 +135,11 @@
void addDependency(GrOpList* dependedOn);
- uint32_t fUniqueID;
- uint32_t fFlags;
+ uint32_t fUniqueID;
+ uint32_t fFlags;
// 'this' GrOpList relies on the output of the GrOpLists in 'fDependencies'
- SkTDArray<GrOpList*> fDependencies;
+ SkTDArray<GrOpList*> fDependencies;
typedef SkRefCnt INHERITED;
};
diff --git a/src/gpu/GrPreFlushResourceProvider.cpp b/src/gpu/GrPreFlushResourceProvider.cpp
index 25033a0..3c6ddc4 100644
--- a/src/gpu/GrPreFlushResourceProvider.cpp
+++ b/src/gpu/GrPreFlushResourceProvider.cpp
@@ -30,6 +30,16 @@
return nullptr;
}
+ // MDB TODO: This explicit resource creation is required in the pre-MDB world so that the
+ // pre-Flush ops are placed in their own opList.
+ sk_sp<GrRenderTargetOpList> opList(new GrRenderTargetOpList(
+ sk_ref_sp(proxy->asRenderTargetProxy()),
+ fDrawingMgr->fContext->getGpu(),
+ fDrawingMgr->fContext->resourceProvider(),
+ fDrawingMgr->fContext->getAuditTrail(),
+ fDrawingMgr->fOptionsForOpLists));
+ proxy->setLastOpList(opList.get());
+
sk_sp<GrRenderTargetContext> renderTargetContext(
fDrawingMgr->makeRenderTargetContext(std::move(proxy),
std::move(colorSpace),
@@ -50,6 +60,16 @@
sk_sp<GrSurfaceProxy> proxy,
sk_sp<SkColorSpace> colorSpace,
const SkSurfaceProps* props) {
+ // MDB TODO: This explicit resource creation is required in the pre-MDB world so that the
+ // pre-Flush ops are placed in their own opList.
+ sk_sp<GrRenderTargetOpList> opList(new GrRenderTargetOpList(
+ sk_ref_sp(proxy->asRenderTargetProxy()),
+ fDrawingMgr->fContext->getGpu(),
+ fDrawingMgr->fContext->resourceProvider(),
+ fDrawingMgr->fContext->getAuditTrail(),
+ fDrawingMgr->fOptionsForOpLists));
+ proxy->setLastOpList(opList.get());
+
sk_sp<GrRenderTargetContext> renderTargetContext(
fDrawingMgr->makeRenderTargetContext(std::move(proxy),
std::move(colorSpace),
diff --git a/src/gpu/GrRenderTargetContext.cpp b/src/gpu/GrRenderTargetContext.cpp
index eada468..34812dd 100644
--- a/src/gpu/GrRenderTargetContext.cpp
+++ b/src/gpu/GrRenderTargetContext.cpp
@@ -128,7 +128,7 @@
return fOpList.get();
}
-// MDB TODO: move this (and GrTextContext::copy) to GrSurfaceContext?
+// TODO: move this (and GrTextContext::copy) to GrSurfaceContext?
bool GrRenderTargetContext::onCopy(GrSurfaceProxy* srcProxy,
const SkIRect& srcRect,
const SkIPoint& dstPoint) {
diff --git a/src/gpu/GrRenderTargetOpList.cpp b/src/gpu/GrRenderTargetOpList.cpp
index 194bc26..4d309f4 100644
--- a/src/gpu/GrRenderTargetOpList.cpp
+++ b/src/gpu/GrRenderTargetOpList.cpp
@@ -87,7 +87,7 @@
#endif
void GrRenderTargetOpList::prepareOps(GrOpFlushState* flushState) {
- SkASSERT(this->isClosed());
+ // MDB TODO: add SkASSERT(this->isClosed());
// Loop over the ops that haven't yet been prepared.
for (int i = 0; i < fRecordedOps.count(); ++i) {
@@ -140,34 +140,34 @@
if (0 == fRecordedOps.count()) {
return false;
}
-
-#ifdef SK_DEBUG
- GrSurface* target = fTarget->instantiate(flushState->resourceProvider());
- if (!target) {
- return false;
- }
- const GrRenderTarget* currentRenderTarget = target->asRenderTarget();
- SkASSERT(currentRenderTarget);
-#endif
-
- std::unique_ptr<GrGpuCommandBuffer> commandBuffer = create_command_buffer(fGpu);
- flushState->setCommandBuffer(commandBuffer.get());
-
// Draw all the generated geometry.
+ const GrRenderTarget* currentRenderTarget = fRecordedOps[0].fRenderTarget.get();
+ SkASSERT(currentRenderTarget);
+ std::unique_ptr<GrGpuCommandBuffer> commandBuffer;
+
for (int i = 0; i < fRecordedOps.count(); ++i) {
if (!fRecordedOps[i].fOp) {
continue;
}
- SkASSERT(fRecordedOps[i].fRenderTarget.get() == currentRenderTarget);
+ SkASSERT(fRecordedOps[i].fRenderTarget.get());
if (fRecordedOps[i].fOp->needsCommandBufferIsolation()) {
// This op is a special snowflake and must occur between command buffers
// TODO: make this go through the command buffer
finish_command_buffer(commandBuffer.get());
+ currentRenderTarget = fRecordedOps[i].fRenderTarget.get();
commandBuffer.reset();
flushState->setCommandBuffer(commandBuffer.get());
+ } else if (fRecordedOps[i].fRenderTarget.get() != currentRenderTarget) {
+ // Changing renderTarget
+ // MDB TODO: this code path goes away
+ finish_command_buffer(commandBuffer.get());
+ currentRenderTarget = fRecordedOps[i].fRenderTarget.get();
+
+ commandBuffer = create_command_buffer(fGpu);
+ flushState->setCommandBuffer(commandBuffer.get());
} else if (!commandBuffer) {
commandBuffer = create_command_buffer(fGpu);
flushState->setCommandBuffer(commandBuffer.get());
@@ -255,7 +255,6 @@
////////////////////////////////////////////////////////////////////////////////
-// MDB TODO: fuse with GrTextureOpList::copySurface
bool GrRenderTargetOpList::copySurface(GrResourceProvider* resourceProvider,
GrRenderTargetContext* dst,
GrSurfaceProxy* src,
@@ -312,13 +311,6 @@
return nullptr;
}
-#ifdef SK_DEBUG
- if (!fRecordedOps.empty()) {
- GrRenderTargetOpList::RecordedOp& back = fRecordedOps.back();
- SkASSERT(back.fRenderTarget == renderTarget);
- }
-#endif
-
// A closed GrOpList should never receive new/more ops
SkASSERT(!this->isClosed());
@@ -418,7 +410,7 @@
fRecordedOps[j].fOp = std::move(fRecordedOps[i].fOp);
break;
}
- // Stop traversing if we would cause a painter's order violation.
+ // Stop going traversing if we would cause a painter's order violation.
if (!can_reorder(fRecordedOps[j].fOp->bounds(), op->bounds())) {
GrOP_INFO("\t\tForward: Intersects with (%s, opID: %u)\n", candidate.fOp->name(),
candidate.fOp->uniqueID());
diff --git a/src/gpu/GrSurfaceProxy.cpp b/src/gpu/GrSurfaceProxy.cpp
index e833eca..f2a90eb 100644
--- a/src/gpu/GrSurfaceProxy.cpp
+++ b/src/gpu/GrSurfaceProxy.cpp
@@ -32,9 +32,10 @@
}
GrSurfaceProxy::~GrSurfaceProxy() {
- // For this to be deleted the opList that held a ref on it (if there was one) must have been
- // deleted. Which would have cleared out this back pointer.
- SkASSERT(!fLastOpList);
+ if (fLastOpList) {
+ fLastOpList->clearTarget();
+ }
+ SkSafeUnref(fLastOpList);
}
GrSurface* GrSurfaceProxy::instantiate(GrResourceProvider* resourceProvider) {
@@ -96,14 +97,15 @@
}
void GrSurfaceProxy::setLastOpList(GrOpList* opList) {
-#ifdef SK_DEBUG
if (fLastOpList) {
+ // The non-MDB world never closes so we can't check this condition
+#ifdef ENABLE_MDB
SkASSERT(fLastOpList->isClosed());
- }
#endif
+ fLastOpList->clearTarget();
+ }
- // Un-reffed
- fLastOpList = opList;
+ SkRefCnt_SafeAssign(fLastOpList, opList);
}
GrRenderTargetOpList* GrSurfaceProxy::getLastRenderTargetOpList() {
diff --git a/src/gpu/GrTextureContext.cpp b/src/gpu/GrTextureContext.cpp
index f41fcd1..68e94f9 100644
--- a/src/gpu/GrTextureContext.cpp
+++ b/src/gpu/GrTextureContext.cpp
@@ -68,16 +68,32 @@
return fOpList.get();
}
-// MDB TODO: move this (and GrRenderTargetContext::copy) to GrSurfaceContext?
+// TODO: move this (and GrRenderTargetContext::copy) to GrSurfaceContext?
bool GrTextureContext::onCopy(GrSurfaceProxy* srcProxy,
const SkIRect& srcRect,
const SkIPoint& dstPoint) {
ASSERT_SINGLE_OWNER
RETURN_FALSE_IF_ABANDONED
SkDEBUGCODE(this->validate();)
- GR_AUDIT_TRAIL_AUTO_FRAME(fAuditTrail, "GrTextureContext::onCopy");
+ GR_AUDIT_TRAIL_AUTO_FRAME(fAuditTrail, "GrTextureContext::copy");
- return this->getOpList()->copySurface(fContext->resourceProvider(),
- fTextureProxy.get(), srcProxy, srcRect, dstPoint);
+#ifndef ENABLE_MDB
+ // We can't yet fully defer copies to textures, so GrTextureContext::copySurface will
+ // execute the copy immediately. Ensure the data is ready.
+ fContext->contextPriv().flushSurfaceWrites(srcProxy);
+#endif
+
+ GrTextureOpList* opList = this->getOpList();
+ bool result = opList->copySurface(fContext->resourceProvider(),
+ fTextureProxy.get(), srcProxy, srcRect, dstPoint);
+
+#ifndef ENABLE_MDB
+ GrOpFlushState flushState(fContext->getGpu(), nullptr);
+ opList->prepareOps(&flushState);
+ opList->executeOps(&flushState);
+ opList->reset();
+#endif
+
+ return result;
}
diff --git a/src/gpu/GrTextureOpList.cpp b/src/gpu/GrTextureOpList.cpp
index de3cacb..689ea44 100644
--- a/src/gpu/GrTextureOpList.cpp
+++ b/src/gpu/GrTextureOpList.cpp
@@ -50,7 +50,7 @@
#endif
void GrTextureOpList::prepareOps(GrOpFlushState* flushState) {
- SkASSERT(this->isClosed());
+ // MDB TODO: add SkASSERT(this->isClosed());
// Loop over the ops that haven't yet generated their geometry
for (int i = 0; i < fRecordedOps.count(); ++i) {
@@ -81,7 +81,6 @@
////////////////////////////////////////////////////////////////////////////////
-// MDB TODO: fuse with GrRenderTargetOpList::copySurface
bool GrTextureOpList::copySurface(GrResourceProvider* resourceProvider,
GrSurfaceProxy* dst,
GrSurfaceProxy* src,
diff --git a/tests/SurfaceTest.cpp b/tests/SurfaceTest.cpp
index 361320d..e236928 100644
--- a/tests/SurfaceTest.cpp
+++ b/tests/SurfaceTest.cpp
@@ -899,8 +899,6 @@
test_surface_creation_and_snapshot_with_color_space(reporter, "wrapped", f16Support,
wrappedSurfaceMaker);
- context->flush();
-
for (auto textureHandle : textureHandles) {
context->getGpu()->deleteTestingOnlyBackendTexture(textureHandle);
}