From 4fa31782fc55c43e0ba1566034207fa18089c0e7 Mon Sep 17 00:00:00 2001 From: Robert Phillips Date: Tue, 17 Jul 2018 12:27:26 +0000 Subject: [PATCH] Revert "Reduce arbitrary opList splitting when sorting" This reverts commit 94fee93c9b23bd1a32604753da8bef755d6c8a95. Reason for revert: Android (and Chromecast) woes Original change's description: > Reduce arbitrary opList splitting when sorting > > Change-Id: I49a47672600f72dc46f27462a2c344e77a06a659 > Reviewed-on: https://skia-review.googlesource.com/141243 > Reviewed-by: Brian Salomon > Commit-Queue: Robert Phillips TBR=bsalomon@google.com,robertphillips@google.com Change-Id: Ic4fd4ab17bb15bef35dcbf852e0f8ad99ee45e8f No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://skia-review.googlesource.com/141760 Reviewed-by: Robert Phillips Commit-Queue: Robert Phillips --- include/private/GrOpList.h | 21 +++--- src/gpu/GrDrawingManager.cpp | 57 +++++---------- src/gpu/GrOpList.cpp | 71 ++----------------- .../GrGaussianConvolutionFragmentProcessor.h | 21 ++---- 4 files changed, 41 insertions(+), 129 deletions(-) diff --git a/include/private/GrOpList.h b/include/private/GrOpList.h index a4e541c81d..0d5a1a245a 100644 --- a/include/private/GrOpList.h +++ b/include/private/GrOpList.h @@ -67,7 +67,15 @@ public: /* * Does this opList depend on 'dependedOn'? */ - bool dependsOn(const GrOpList* dependedOn) const; + bool dependsOn(GrOpList* dependedOn) const { + for (int i = 0; i < fDependencies.count(); ++i) { + if (fDependencies[i] == dependedOn) { + return true; + } + } + + return false; + } /* * Safely cast this GrOpList to a GrTextureOpList (if possible). @@ -112,13 +120,6 @@ protected: private: friend class GrDrawingManager; // for resetFlag, TopoSortTraits & gatherProxyIntervals - void addDependency(GrOpList* dependedOn); - void addDependent(GrOpList* dependent); - SkDEBUGCODE(bool isDependedent(const GrOpList* dependent) const); - SkDEBUGCODE(void validate() const); - - void closeThoseWhoDependOnMe(const GrCaps&); - // Remove all Ops which reference proxies that have not been instantiated. virtual void purgeOpsWithUninstantiatedProxies() = 0; @@ -173,13 +174,13 @@ private: virtual void onPrepare(GrOpFlushState* flushState) = 0; virtual bool onExecute(GrOpFlushState* flushState) = 0; + void addDependency(GrOpList* dependedOn); + uint32_t fUniqueID; uint32_t fFlags; // 'this' GrOpList relies on the output of the GrOpLists in 'fDependencies' SkSTArray<1, GrOpList*, true> fDependencies; - // 'this' GrOpList's output is relied on by the GrOpLists in 'fDependents' - SkSTArray<1, GrOpList*, true> fDependents; typedef SkRefCnt INHERITED; }; diff --git a/src/gpu/GrDrawingManager.cpp b/src/gpu/GrDrawingManager.cpp index 366f285cf3..0df9b34468 100644 --- a/src/gpu/GrDrawingManager.cpp +++ b/src/gpu/GrDrawingManager.cpp @@ -32,6 +32,11 @@ #include "ccpr/GrCoverageCountingPathRenderer.h" #include "text/GrTextContext.h" +// Turn on/off the sorting of opLists at flush time +#ifndef SK_DISABLE_RENDER_TARGET_SORTING + #define SK_DISABLE_RENDER_TARGET_SORTING +#endif + GrDrawingManager::GrDrawingManager(GrContext* context, const GrPathRendererChain::Options& optionsForPathRendererChain, const GrTextContext::Options& optionsForTextContext, @@ -127,11 +132,6 @@ GrSemaphoresSubmitted GrDrawingManager::internalFlush(GrSurfaceProxy*, fOpLists[i]->makeClosed(*fContext->contextPriv().caps()); } - if (fSortRenderTargets) { - SkDEBUGCODE(bool result =) SkTTopoSort(&fOpLists); - SkASSERT(result); - } - #ifdef SK_DEBUG // This block checks for any unnecessary splits in the opLists. If two sequential opLists // share the same backing GrSurfaceProxy it means the opList was artificially split. @@ -149,6 +149,11 @@ GrSemaphoresSubmitted GrDrawingManager::internalFlush(GrSurfaceProxy*, } #endif + if (fSortRenderTargets) { + SkDEBUGCODE(bool result =) SkTTopoSort(&fOpLists); + SkASSERT(result); + } + GrOpFlushState flushState(gpu, fContext->contextPriv().resourceProvider(), &fTokenTracker); @@ -273,7 +278,7 @@ bool GrDrawingManager::executeOpLists(int startIndex, int stopIndex, GrOpFlushSt SkDebugf("Flushing opLists: %d to %d out of [%d, %d]\n", startIndex, stopIndex, 0, fOpLists.count()); for (int i = startIndex; i < stopIndex; ++i) { - fOpLists[i]->dump(true); + fOpLists[i]->dump(false); } #endif @@ -426,23 +431,11 @@ sk_sp GrDrawingManager::newRTOpList(GrRenderTargetProxy* r bool managedOpList) { 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()) { - if (fSortRenderTargets) { - // In this case we need to close all the opLists that rely on the current contents of - // 'rtp'. That is bc we're going to update the content of the proxy so they need to be - // split in case they use both the old and new content. (This is a bit of an overkill: - // they really only need to be split if they ever reference proxy's contents again but - // that is hard to predict/handle). - if (GrOpList* lastOpList = rtp->getLastOpList()) { - lastOpList->closeThoseWhoDependOnMe(*fContext->contextPriv().caps()); - } - } else { - // 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. - fOpLists.back()->makeClosed(*fContext->contextPriv().caps()); - } + fOpLists.back()->makeClosed(*fContext->contextPriv().caps()); } auto resourceProvider = fContext->contextPriv().resourceProvider(); @@ -464,23 +457,11 @@ sk_sp GrDrawingManager::newRTOpList(GrRenderTargetProxy* r sk_sp GrDrawingManager::newTextureOpList(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()) { - if (fSortRenderTargets) { - // In this case we need to close all the opLists that rely on the current contents of - // 'rtp'. That is bc we're going to update the content of the proxy so they need to be - // split in case they use both the old and new content. (This is a bit of an overkill: - // they really only need to be split if they ever reference proxy's contents again but - // that is hard to predict/handle). - if (GrOpList* lastOpList = textureProxy->getLastOpList()) { - lastOpList->closeThoseWhoDependOnMe(*fContext->contextPriv().caps()); - } - } else { - // 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. - fOpLists.back()->makeClosed(*fContext->contextPriv().caps()); - } + fOpLists.back()->makeClosed(*fContext->contextPriv().caps()); } sk_sp opList(new GrTextureOpList(fContext->contextPriv().resourceProvider(), diff --git a/src/gpu/GrOpList.cpp b/src/gpu/GrOpList.cpp index 12d15afe6a..b9f425576e 100644 --- a/src/gpu/GrOpList.cpp +++ b/src/gpu/GrOpList.cpp @@ -96,9 +96,6 @@ void GrOpList::addDependency(GrOpList* dependedOn) { } fDependencies.push_back(dependedOn); - dependedOn->addDependent(this); - - SkDEBUGCODE(this->validate()); } // Convert from a GrSurface-based dependency to a GrOpList one @@ -109,13 +106,11 @@ void GrOpList::addDependency(GrSurfaceProxy* dependedOn, const GrCaps& caps) { GrOpList* opList = dependedOn->getLastOpList(); if (opList == this) { - // self-read - presumably for dst reads. We can't make it closed in the self-read case. + // self-read - presumably for dst reads } else { this->addDependency(opList); - // We are closing 'opList' here bc the current contents of it are what 'this' opList - // depends on. We need a break in 'opList' so that the usage of that state has a - // chance to execute. + // Can't make it closed in the self-read case opList->makeClosed(caps); } } @@ -127,78 +122,22 @@ void GrOpList::addDependency(GrSurfaceProxy* dependedOn, const GrCaps& caps) { } } -bool GrOpList::dependsOn(const GrOpList* dependedOn) const { - for (int i = 0; i < fDependencies.count(); ++i) { - if (fDependencies[i] == dependedOn) { - return true; - } - } - - return false; -} - -void GrOpList::addDependent(GrOpList* dependent) { - fDependents.push_back(dependent); -} - -#ifdef SK_DEBUG -bool GrOpList::isDependedent(const GrOpList* dependent) const { - for (int i = 0; i < fDependents.count(); ++i) { - if (fDependents[i] == dependent) { - return true; - } - } - - return false; -} - -void GrOpList::validate() const { - // TODO: check for loops and duplicates - - for (int i = 0; i < fDependencies.count(); ++i) { - SkASSERT(fDependencies[i]->isDependedent(this)); - } -} -#endif - -void GrOpList::closeThoseWhoDependOnMe(const GrCaps& caps) { - for (int i = 0; i < fDependents.count(); ++i) { - if (!fDependents[i]->isClosed()) { - fDependents[i]->makeClosed(caps); - } - } -} - bool GrOpList::isInstantiated() const { return fTarget.get()->priv().isInstantiated(); } #ifdef SK_DEBUG -static const char* op_to_name(GrLoadOp op) { - return GrLoadOp::kLoad == op ? "load" : GrLoadOp::kClear == op ? "clear" : "discard"; -} - void GrOpList::dump(bool printDependencies) const { SkDebugf("--------------------------------------------------------------\n"); - SkDebugf("opListID: %d -> proxyID: %d\n", fUniqueID, - fTarget.get() ? fTarget.get()->uniqueID().asUInt() : -1); - SkDebugf("ColorLoadOp: %s %x StencilLoadOp: %s\n", - op_to_name(fColorLoadOp), - GrLoadOp::kClear == fColorLoadOp ? fLoadClearColor : 0x0, - op_to_name(fStencilLoadOp)); + SkDebugf("opList: %d -> RT: %d\n", fUniqueID, fTarget.get() ? fTarget.get()->uniqueID().asUInt() + : -1); if (printDependencies) { - SkDebugf("I rely On (%d): ", fDependencies.count()); + SkDebugf("relies On (%d): ", fDependencies.count()); for (int i = 0; i < fDependencies.count(); ++i) { SkDebugf("%d, ", fDependencies[i]->fUniqueID); } SkDebugf("\n"); - - SkDebugf("(%d) Rely On Me: ", fDependents.count()); - for (int i = 0; i < fDependents.count(); ++i) { - SkDebugf("%d, ", fDependents[i]->fUniqueID); - } - SkDebugf("\n"); } } #endif diff --git a/src/gpu/effects/GrGaussianConvolutionFragmentProcessor.h b/src/gpu/effects/GrGaussianConvolutionFragmentProcessor.h index 6d68292616..9850e605fb 100644 --- a/src/gpu/effects/GrGaussianConvolutionFragmentProcessor.h +++ b/src/gpu/effects/GrGaussianConvolutionFragmentProcessor.h @@ -44,15 +44,6 @@ public: const char* name() const override { return "GaussianConvolution"; } - SkString dumpInfo() const override { - SkString str; - str.appendf("dir: %s radius: %d bounds: [%d %d]", - Direction::kX == fDirection ? "X" : "Y", - fRadius, - fBounds[0], fBounds[1]); - return str; - } - std::unique_ptr clone() const override { return std::unique_ptr( new GrGaussianConvolutionFragmentProcessor(*this)); @@ -83,14 +74,14 @@ private: GR_DECLARE_FRAGMENT_PROCESSOR_TEST - GrCoordTransform fCoordTransform; - TextureSampler fTextureSampler; + GrCoordTransform fCoordTransform; + TextureSampler fTextureSampler; // TODO: Inline the kernel constants into the generated shader code. This may involve pulling // some of the logic from SkGpuBlurUtils into this class related to radius/sigma calculations. - float fKernel[kMaxKernelWidth]; - int fBounds[2]; - int fRadius; - Direction fDirection; + float fKernel[kMaxKernelWidth]; + int fBounds[2]; + int fRadius; + Direction fDirection; GrTextureDomain::Mode fMode; typedef GrFragmentProcessor INHERITED;