diff --git a/src/gpu/ccpr/GrCoverageCountingPathRenderer.cpp b/src/gpu/ccpr/GrCoverageCountingPathRenderer.cpp index c6e5a0ecf2..b4eb89a1a3 100644 --- a/src/gpu/ccpr/GrCoverageCountingPathRenderer.cpp +++ b/src/gpu/ccpr/GrCoverageCountingPathRenderer.cpp @@ -104,6 +104,7 @@ GrCoverageCountingPathRenderer::DrawPathsOp::DrawPathsOp(GrCoverageCountingPathR , fProcessors(std::move(args.fPaint)) , fTailDraw(&fHeadDraw) , fOwningRTPendingOps(nullptr) { + SkDEBUGCODE(++fCCPR->fPendingDrawOpsCount); SkDEBUGCODE(fBaseInstance = -1); SkDEBUGCODE(fDebugInstanceCount = 1;) SkDEBUGCODE(fDebugSkippedInstances = 0;) @@ -139,20 +140,34 @@ GrCoverageCountingPathRenderer::DrawPathsOp::DrawPathsOp(GrCoverageCountingPathR this->setBounds(devBounds, GrOp::HasAABloat::kYes, GrOp::IsZeroArea::kNo); } +GrCoverageCountingPathRenderer::DrawPathsOp::~DrawPathsOp() { + if (fOwningRTPendingOps) { + // Remove CCPR's dangling pointer to this Op before deleting it. + SkASSERT(!fCCPR->fFlushing); + fOwningRTPendingOps->fOpList.remove(this); + } + SkDEBUGCODE(--fCCPR->fPendingDrawOpsCount); +} + GrDrawOp::RequiresDstTexture DrawPathsOp::finalize(const GrCaps& caps, const GrAppliedClip* clip, GrPixelConfigIsClamped dstIsClamped) { - SingleDraw& onlyDraw = this->getOnlyPathDraw(); + SkASSERT(!fCCPR->fFlushing); + // There should only be one single path draw in this Op right now. + SkASSERT(1 == fDebugInstanceCount); + SkASSERT(&fHeadDraw == fTailDraw); GrProcessorSet::Analysis analysis = fProcessors.finalize( - onlyDraw.fColor, GrProcessorAnalysisCoverage::kSingleChannel, clip, false, caps, - dstIsClamped, &onlyDraw.fColor); + fHeadDraw.fColor, GrProcessorAnalysisCoverage::kSingleChannel, clip, false, caps, + dstIsClamped, &fHeadDraw.fColor); return analysis.requiresDstTexture() ? RequiresDstTexture::kYes : RequiresDstTexture::kNo; } bool DrawPathsOp::onCombineIfPossible(GrOp* op, const GrCaps& caps) { DrawPathsOp* that = op->cast(); SkASSERT(fCCPR == that->fCCPR); + SkASSERT(!fCCPR->fFlushing); SkASSERT(fOwningRTPendingOps); SkASSERT(fDebugInstanceCount); + SkASSERT(!that->fOwningRTPendingOps || that->fOwningRTPendingOps == fOwningRTPendingOps); SkASSERT(that->fDebugInstanceCount); if (this->getFillType() != that->getFillType() || @@ -161,20 +176,8 @@ bool DrawPathsOp::onCombineIfPossible(GrOp* op, const GrCaps& caps) { return false; } - if (RTPendingOps* owningRTPendingOps = that->fOwningRTPendingOps) { - SkASSERT(owningRTPendingOps == fOwningRTPendingOps); - owningRTPendingOps->fOpList.remove(that); - } else { - // The Op is being combined immediately after creation, before a call to wasRecorded. In - // this case wasRecorded will not be called. So we count its path here instead. - const SingleDraw& onlyDraw = that->getOnlyPathDraw(); - ++fOwningRTPendingOps->fNumTotalPaths; - fOwningRTPendingOps->fNumSkPoints += onlyDraw.fPath.countPoints(); - fOwningRTPendingOps->fNumSkVerbs += onlyDraw.fPath.countVerbs(); - } - fTailDraw->fNext = &fOwningRTPendingOps->fDrawsAllocator.push_back(that->fHeadDraw); - fTailDraw = that->fTailDraw == &that->fHeadDraw ? fTailDraw->fNext : that->fTailDraw; + fTailDraw = (that->fTailDraw == &that->fHeadDraw) ? fTailDraw->fNext : that->fTailDraw; this->joinBounds(*that); @@ -184,12 +187,9 @@ bool DrawPathsOp::onCombineIfPossible(GrOp* op, const GrCaps& caps) { } void DrawPathsOp::wasRecorded(GrRenderTargetOpList* opList) { + SkASSERT(!fCCPR->fFlushing); SkASSERT(!fOwningRTPendingOps); - const SingleDraw& onlyDraw = this->getOnlyPathDraw(); fOwningRTPendingOps = &fCCPR->fRTPendingOpsMap[opList->uniqueID()]; - ++fOwningRTPendingOps->fNumTotalPaths; - fOwningRTPendingOps->fNumSkPoints += onlyDraw.fPath.countPoints(); - fOwningRTPendingOps->fNumSkVerbs += onlyDraw.fPath.countVerbs(); fOwningRTPendingOps->fOpList.addToTail(this); } @@ -224,22 +224,29 @@ void GrCoverageCountingPathRenderer::setupPerFlushResources(GrOnFlushResourcePro fPerFlushResourcesAreValid = false; - SkTInternalLList flushingOps; + // Gather the Ops that are being flushed. int maxTotalPaths = 0, numSkPoints = 0, numSkVerbs = 0; - + SkTInternalLList flushingOps; for (int i = 0; i < numOpListIDs; ++i) { auto it = fRTPendingOpsMap.find(opListIDs[i]); - if (fRTPendingOpsMap.end() != it) { - RTPendingOps& rtPendingOps = it->second; - SkASSERT(!rtPendingOps.fOpList.isEmpty()); - flushingOps.concat(std::move(rtPendingOps.fOpList)); - maxTotalPaths += rtPendingOps.fNumTotalPaths; - numSkPoints += rtPendingOps.fNumSkPoints; - numSkVerbs += rtPendingOps.fNumSkVerbs; + if (fRTPendingOpsMap.end() == it) { + continue; } + SkTInternalLList::Iter iter; + SkTInternalLList& rtFlushingOps = it->second.fOpList; + iter.init(rtFlushingOps, SkTInternalLList::Iter::kHead_IterStart); + while (DrawPathsOp* flushingOp = iter.get()) { + for (const auto* draw = &flushingOp->fHeadDraw; draw; draw = draw->fNext) { + ++maxTotalPaths; + numSkPoints += draw->fPath.countPoints(); + numSkVerbs += draw->fPath.countVerbs(); + } + flushingOp->fOwningRTPendingOps = nullptr; // Owner is about to change to 'flushingOps'. + iter.next(); + } + flushingOps.concat(std::move(rtFlushingOps)); } - SkASSERT(flushingOps.isEmpty() == !maxTotalPaths); if (flushingOps.isEmpty()) { return; // Nothing to draw. } @@ -376,6 +383,8 @@ void DrawPathsOp::onExecute(GrOpFlushState* flushState) { return; // Setup failed. } + SkASSERT(fBaseInstance >= 0); // Make sure setupPerFlushResources has set us up. + GrPipeline::InitArgs initArgs; initArgs.fFlags = fSRGBFlags; initArgs.fProxy = flushState->drawOpArgs().fProxy; diff --git a/src/gpu/ccpr/GrCoverageCountingPathRenderer.h b/src/gpu/ccpr/GrCoverageCountingPathRenderer.h index 7d613f90e5..a6295a5a7c 100644 --- a/src/gpu/ccpr/GrCoverageCountingPathRenderer.h +++ b/src/gpu/ccpr/GrCoverageCountingPathRenderer.h @@ -35,6 +35,12 @@ public: static sk_sp CreateIfSupported(const GrCaps&, bool drawCachablePaths); + ~GrCoverageCountingPathRenderer() override { + // Ensure nothing exists that could have a dangling pointer back into this class. + SkASSERT(fRTPendingOpsMap.empty()); + SkASSERT(0 == fPendingDrawOpsCount); + } + // GrPathRenderer overrides. StencilSupport onGetStencilSupport(const GrShape&) const override { return GrPathRenderer::kNoSupport_StencilSupport; @@ -55,6 +61,7 @@ public: SK_DECLARE_INTERNAL_LLIST_INTERFACE(DrawPathsOp); DrawPathsOp(GrCoverageCountingPathRenderer*, const DrawPathArgs&, GrColor); + ~DrawPathsOp() override; const char* name() const override { return "GrCoverageCountingPathRenderer::DrawPathsOp"; } @@ -87,12 +94,6 @@ public: SingleDraw* fNext = nullptr; }; - SingleDraw& getOnlyPathDraw() { - SkASSERT(&fHeadDraw == fTailDraw); - SkASSERT(1 == fDebugInstanceCount); - return fHeadDraw; - } - struct AtlasBatch { const GrCCPRAtlas* fAtlas; int fEndInstanceIdx; @@ -130,14 +131,12 @@ private: struct RTPendingOps { SkTInternalLList fOpList; - int fNumTotalPaths = 0; - int fNumSkPoints = 0; - int fNumSkVerbs = 0; GrSTAllocator<256, DrawPathsOp::SingleDraw> fDrawsAllocator; }; // Map from render target ID to the individual render target's pending path ops. std::map fRTPendingOpsMap; + SkDEBUGCODE(int fPendingDrawOpsCount = 0;) sk_sp fPerFlushIndexBuffer; sk_sp fPerFlushVertexBuffer; diff --git a/tests/GrCCPRTest.cpp b/tests/GrCCPRTest.cpp index f56298be5a..b8e3db9c10 100644 --- a/tests/GrCCPRTest.cpp +++ b/tests/GrCCPRTest.cpp @@ -154,6 +154,28 @@ class GrCCPRTest_cleanup : public CCPRTest { }; DEF_CCPR_TEST(GrCCPRTest_cleanup) +class GrCCPRTest_unregisterCulledOps : public CCPRTest { + void onRun(skiatest::Reporter* reporter, CCPRPathDrawer& ccpr) override { + REPORTER_ASSERT(reporter, SkPathPriv::TestingOnly_unique(fPath)); + + // Ensure Ops get unregistered from CCPR when culled early. + ccpr.drawPath(fPath); + REPORTER_ASSERT(reporter, !SkPathPriv::TestingOnly_unique(fPath)); + ccpr.clear(); // Clear should delete the CCPR Op. + REPORTER_ASSERT(reporter, SkPathPriv::TestingOnly_unique(fPath)); + ccpr.flush(); // Should not crash (DrawPathsOp should have unregistered itself). + + // Ensure Op unregisters work when we delete the context without flushing. + ccpr.drawPath(fPath); + REPORTER_ASSERT(reporter, !SkPathPriv::TestingOnly_unique(fPath)); + ccpr.clear(); // Clear should delete the CCPR DrawPathsOp. + REPORTER_ASSERT(reporter, SkPathPriv::TestingOnly_unique(fPath)); + ccpr.abandonGrContext(); + fMockContext.reset(); // Should not crash (DrawPathsOp should have unregistered itself). + } +}; +DEF_CCPR_TEST(GrCCPRTest_unregisterCulledOps) + class CCPRRenderingTest { public: void run(skiatest::Reporter* reporter, GrContext* ctx) const {