Revert "Split up opLists (take 2)"

This reverts commit df2bf213649e0b2bcb9402548af9976bbdf7a218.

Reason for revert: Maybe AndroidOne timing out

Original change's description:
> Split up opLists (take 2)
> 
> Reland of: https://skia-review.googlesource.com/c/11581/ (Split up opLists)
> 
> https://skia-review.googlesource.com/c/13860/ (Make InstancedRendering more opList-splitting friendly) has landed so this should be good for another attempt.
> 
> Change-Id: Icc9998196587510328e0a9ca1b2ce42013a86c6c
> Reviewed-on: https://skia-review.googlesource.com/13802
> Reviewed-by: Brian Salomon <bsalomon@google.com>
> Commit-Queue: Robert Phillips <robertphillips@google.com>
> 

TBR=bsalomon@google.com,robertphillips@google.com,reviews@skia.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

Change-Id: I744f2a3145b294e5911862bb39d57ca33a1b9a5a
Reviewed-on: https://skia-review.googlesource.com/14184
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>

9 files changed, 115 insertions, 63 deletions

diff --git a/src/gpu/GrDrawingManager.cpp b/src/gpu/GrDrawingManager.cpp
index 48d19f296e..282226c748 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();
     }
 
@@ -92,7 +92,6 @@ void GrDrawingManager::internalFlush(GrSurfaceProxy*, GrResourceCache::FlushType
         // 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
@@ -125,7 +124,6 @@ void GrDrawingManager::internalFlush(GrSurfaceProxy*, GrResourceCache::FlushType
                 if (!opList) {
                     continue;   // Odd - but not a big deal
                 }
-                opList->makeClosed();
                 SkDEBUGCODE(opList->validateTargetsSingleRenderTarget());
                 opList->prepareOps(&fFlushState);
                 if (!opList->executeOps(&fFlushState)) {
@@ -162,7 +160,17 @@ void GrDrawingManager::internalFlush(GrSurfaceProxy*, GrResourceCache::FlushType
         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.
@@ -199,12 +207,19 @@ void GrDrawingManager::addPreFlushCallbackObject(sk_sp<GrPreFlushCallbackObject>
 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(),
@@ -221,21 +236,19 @@ sk_sp<GrRenderTargetOpList> GrDrawingManager::newRTOpList(sk_sp<GrRenderTargetPr
 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 5e618a21df..91c8c7942c 100644
--- a/src/gpu/GrOpList.cpp
+++ b/src/gpu/GrOpList.cpp
@@ -21,7 +21,9 @@ uint32_t GrOpList::CreateUniqueID() {
 }
 
 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 352a46c2a9..b7dd7fa62f 100644
--- a/src/gpu/GrOpList.h
+++ b/src/gpu/GrOpList.h
@@ -29,7 +29,11 @@ public:
     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 @@ public:
     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 @@ public:
     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 @@ private:
 
     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 25033a0e39..3c6ddc4ab7 100644
--- a/src/gpu/GrPreFlushResourceProvider.cpp
+++ b/src/gpu/GrPreFlushResourceProvider.cpp
@@ -30,6 +30,16 @@ sk_sp<GrRenderTargetContext> GrPreFlushResourceProvider::makeRenderTargetContext
         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<GrRenderTargetContext> GrPreFlushResourceProvider::makeRenderTargetContext
                                                         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 9b188f450b..41fa6dce0d 100644
--- a/src/gpu/GrRenderTargetContext.cpp
+++ b/src/gpu/GrRenderTargetContext.cpp
@@ -126,7 +126,7 @@ GrRenderTargetOpList* GrRenderTargetContext::getOpList() {
     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 4c4e2ad5d3..0bc6708f3a 100644
--- a/src/gpu/GrRenderTargetOpList.cpp
+++ b/src/gpu/GrRenderTargetOpList.cpp
@@ -87,7 +87,7 @@ void GrRenderTargetOpList::validateTargetsSingleRenderTarget() const {
 #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 @@ bool GrRenderTargetOpList::executeOps(GrOpFlushState* flushState) {
     if (0 == fRecordedOps.count()) {
         return false;
     }
-
-#ifdef SK_DEBUG
-    GrSurface* target = fTarget->instantiate(flushState->resourceProvider());
-    if (!target) {
-        return false;
-    }
-    const GrRenderTarget* currentRenderTarget = target->asRenderTarget();
+    // Draw all the generated geometry.
+    const GrRenderTarget* currentRenderTarget = fRecordedOps[0].fRenderTarget.get();
     SkASSERT(currentRenderTarget);
-#endif
+    std::unique_ptr<GrGpuCommandBuffer> commandBuffer;
 
-    std::unique_ptr<GrGpuCommandBuffer> commandBuffer = create_command_buffer(fGpu);
-    flushState->setCommandBuffer(commandBuffer.get());
-
-    // Draw all the generated geometry.
     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 @@ void GrRenderTargetOpList::fullClear(GrRenderTargetContext* renderTargetContext,
 
 ////////////////////////////////////////////////////////////////////////////////
 
-// MDB TODO: fuse with GrTextureOpList::copySurface
 bool GrRenderTargetOpList::copySurface(GrResourceProvider* resourceProvider,
                                        GrRenderTargetContext* dst,
                                        GrSurfaceProxy* src,
@@ -312,13 +311,6 @@ GrOp* GrRenderTargetOpList::recordOp(std::unique_ptr<GrOp> op,
         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 @@ void GrRenderTargetOpList::forwardCombine() {
                 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 8451409eae..b55d12a124 100644
--- a/src/gpu/GrSurfaceProxy.cpp
+++ b/src/gpu/GrSurfaceProxy.cpp
@@ -32,9 +32,10 @@ GrSurfaceProxy::GrSurfaceProxy(sk_sp<GrSurface> surface, SkBackingFit fit)
 }
 
 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 @@ int GrSurfaceProxy::worstCaseHeight(const GrCaps& caps) const {
 }
 
 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 f41fcd1837..68e94f9be3 100644
--- a/src/gpu/GrTextureContext.cpp
+++ b/src/gpu/GrTextureContext.cpp
@@ -68,16 +68,32 @@ GrTextureOpList* GrTextureContext::getOpList() {
     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 de3cacb480..689ea44783 100644
--- a/src/gpu/GrTextureOpList.cpp
+++ b/src/gpu/GrTextureOpList.cpp
@@ -50,7 +50,7 @@ void GrTextureOpList::validateTargetsSingleRenderTarget() const {
 #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 @@ void GrTextureOpList::reset() {
 
 ////////////////////////////////////////////////////////////////////////////////
 
-// MDB TODO: fuse with GrRenderTargetOpList::copySurface
 bool GrTextureOpList::copySurface(GrResourceProvider* resourceProvider,
                                   GrSurfaceProxy* dst,
                                   GrSurfaceProxy* src,