Don't ref/unref the static src-over xp

GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1540363002

bug=chromium:570301

Review URL: https://codereview.chromium.org/1540363002

diff --git a/include/gpu/effects/GrPorterDuffXferProcessor.h b/include/gpu/effects/GrPorterDuffXferProcessor.h
index be014a6..457c6ea 100644 (file)
--- a/include/gpu/effects/GrPorterDuffXferProcessor.h
+++ b/include/gpu/effects/GrPorterDuffXferProcessor.h
@@ -21,10 +21,16 @@ public:
     void getInvariantBlendedColor(const GrProcOptInfo& colorPOI,
                                   GrXPFactory::InvariantBlendedColor*) const override;
 
+
+    /** Because src-over is so common we special case it for performance reasons. If this returns
+        null then the SimpleSrcOverXP() below should be used. */
     static GrXferProcessor* CreateSrcOverXferProcessor(const GrCaps& caps,
                                                        const GrPipelineOptimizations& optimizations,
                                                        bool hasMixedSamples,
                                                        const GrXferProcessor::DstTexture*);
+    /** This XP implements non-LCD src-over using hw blend with no optimizations. It is returned
+        by reference because it is global and its ref-cnting methods are not thread safe. */
+    static const GrXferProcessor& SimpleSrcOverXP();
 
     static inline void SrcOverInvariantBlendedColor(
                                                 GrColor inputColor,

diff --git a/src/gpu/GrPipeline.cpp b/src/gpu/GrPipeline.cpp
index 02edd42..e1c733a 100644 (file)
--- a/src/gpu/GrPipeline.cpp
+++ b/src/gpu/GrPipeline.cpp
@@ -28,29 +28,30 @@ GrPipeline* GrPipeline::CreateAt(void* memory, const CreateArgs& args,
                                                            builder.hasMixedSamples(),
                                                            &args.fDstTexture,
                                                            *args.fCaps));
+        if (!xferProcessor) {
+            return nullptr;
+        }
     } else {
+        // This may return nullptr in the common case of src-over implemented using hw blending.
         xferProcessor.reset(GrPorterDuffXPFactory::CreateSrcOverXferProcessor(
                                                                         *args.fCaps,
                                                                         args.fOpts,
                                                                         builder.hasMixedSamples(),
                                                                         &args.fDstTexture));
     }
-
-    if (!xferProcessor) {
-        return nullptr;
-    }
-
-    GrColor overrideColor = GrColor_ILLEGAL;
+   GrColor overrideColor = GrColor_ILLEGAL;
     if (args.fOpts.fColorPOI.firstEffectiveProcessorIndex() != 0) {
         overrideColor = args.fOpts.fColorPOI.inputColorToFirstEffectiveProccesor();
     }
 
     GrXferProcessor::OptFlags optFlags = GrXferProcessor::kNone_OptFlags;
 
-    optFlags = xferProcessor->getOptimizations(args.fOpts,
-                                               builder.getStencil().doesWrite(),
-                                               &overrideColor,
-                                               *args.fCaps);
+    const GrXferProcessor* xpForOpts = xferProcessor ? xferProcessor.get() : 
+                                                       &GrPorterDuffXPFactory::SimpleSrcOverXP();
+    optFlags = xpForOpts->getOptimizations(args.fOpts,
+                                           builder.getStencil().doesWrite(),
+                                           &overrideColor,
+                                           *args.fCaps);
 
     // When path rendering the stencil settings are not always set on the GrPipelineBuilder
     // so we must check the draw type. In cases where we will skip drawing we simply return a
@@ -167,14 +168,12 @@ void GrPipeline::addDependenciesTo(GrRenderTarget* rt) const {
         add_dependencies_for_processor(fFragmentProcessors[i].get(), rt);
     }
 
-    if (fXferProcessor.get()) {
-        const GrXferProcessor* xfer = fXferProcessor.get();
+    const GrXferProcessor& xfer = this->getXferProcessor();
 
-        for (int i = 0; i < xfer->numTextures(); ++i) {
-            GrTexture* texture = xfer->textureAccess(i).getTexture();   
-            SkASSERT(rt->getLastDrawTarget());
-            rt->getLastDrawTarget()->addDependency(texture);
-        }
+    for (int i = 0; i < xfer.numTextures(); ++i) {
+        GrTexture* texture = xfer.textureAccess(i).getTexture();   
+        SkASSERT(rt->getLastDrawTarget());
+        rt->getLastDrawTarget()->addDependency(texture);
     }
 }
 
@@ -185,7 +184,7 @@ void GrPipeline::adjustProgramFromOptimizations(const GrPipelineBuilder& pipelin
                                                 int* firstColorProcessorIdx,
                                                 int* firstCoverageProcessorIdx) {
     fIgnoresCoverage = SkToBool(flags & GrXferProcessor::kIgnoreCoverage_OptFlag);
-    fReadsFragPosition = fXferProcessor->willReadFragmentPosition();
+    fReadsFragPosition = this->getXferProcessor().willReadFragmentPosition();
 
     if ((flags & GrXferProcessor::kIgnoreColor_OptFlag) ||
         (flags & GrXferProcessor::kOverrideColor_OptFlag)) {
@@ -221,8 +220,11 @@ bool GrPipeline::AreEqual(const GrPipeline& a, const GrPipeline& b,
         return false;
     }
 
-    if (!a.getXferProcessor()->isEqual(*b.getXferProcessor())) {
-        return false;
+    // Most of the time both are nullptr
+    if (a.fXferProcessor.get() || b.fXferProcessor.get()) {
+        if (!a.getXferProcessor().isEqual(b.getXferProcessor())) {
+            return false;
+        }
     }
 
     for (int i = 0; i < a.numFragmentProcessors(); i++) {

diff --git a/src/gpu/GrPipeline.h b/src/gpu/GrPipeline.h
index fbfe119..60d0cab 100644 (file)
--- a/src/gpu/GrPipeline.h
+++ b/src/gpu/GrPipeline.h
@@ -103,7 +103,15 @@ public:
     }
     int numFragmentProcessors() const { return fFragmentProcessors.count(); }
 
-    const GrXferProcessor* getXferProcessor() const { return fXferProcessor.get(); }
+    const GrXferProcessor& getXferProcessor() const {
+        if (fXferProcessor.get()) {
+            return *fXferProcessor.get();
+        } else {
+            // A null xp member means the common src-over case. GrXferProcessor's ref'ing
+            // mechanism is not thread safe so we do not hold a ref on this global.
+            return GrPorterDuffXPFactory::SimpleSrcOverXP();
+        }
+    }
 
     const GrFragmentProcessor& getColorFragmentProcessor(int idx) const {
         SkASSERT(idx < this->numColorFragmentProcessors());
@@ -136,7 +144,7 @@ public:
     bool snapVerticesToPixelCenters() const { return SkToBool(fFlags & kSnapVertices_Flag); }
 
     GrXferBarrierType xferBarrierType(const GrCaps& caps) const {
-        return fXferProcessor->xferBarrierType(fRenderTarget.get(), caps);
+        return this->getXferProcessor().xferBarrierType(fRenderTarget.get(), caps);
     }
 
     /**

diff --git a/src/gpu/batches/GrDrawBatch.h b/src/gpu/batches/GrDrawBatch.h
index 9401c67..4d4209a 100644 (file)
--- a/src/gpu/batches/GrDrawBatch.h
+++ b/src/gpu/batches/GrDrawBatch.h
@@ -83,7 +83,7 @@ public:
                            this->pipeline()->getCoverageFragmentProcessor(i).name(),
                            this->pipeline()->getCoverageFragmentProcessor(i).dumpInfo().c_str());
         }
-        string.appendf("XP: %s\n", this->pipeline()->getXferProcessor()->name());
+        string.appendf("XP: %s\n", this->pipeline()->getXferProcessor().name());
         return string;
     }
 

diff --git a/src/gpu/effects/GrPorterDuffXferProcessor.cpp b/src/gpu/effects/GrPorterDuffXferProcessor.cpp
index 69e77cf..11af4b6 100644 (file)
--- a/src/gpu/effects/GrPorterDuffXferProcessor.cpp
+++ b/src/gpu/effects/GrPorterDuffXferProcessor.cpp
@@ -850,6 +850,12 @@ void GrPorterDuffXPFactory::TestGetXPOutputTypes(const GrXferProcessor* xp,
 ////////////////////////////////////////////////////////////////////////////////////////////////
 // SrcOver Global functions
 ////////////////////////////////////////////////////////////////////////////////////////////////
+const GrXferProcessor& GrPorterDuffXPFactory::SimpleSrcOverXP() {
+    static BlendFormula gSrcOverBlendFormula = COEFF_FORMULA(kOne_GrBlendCoeff,
+                                                             kISA_GrBlendCoeff);
+    static PorterDuffXferProcessor gSrcOverXP(gSrcOverBlendFormula);
+    return gSrcOverXP;
+}
 
 GrXferProcessor* GrPorterDuffXPFactory::CreateSrcOverXferProcessor(
         const GrCaps& caps,
@@ -860,12 +866,11 @@ GrXferProcessor* GrPorterDuffXPFactory::CreateSrcOverXferProcessor(
         !(optimizations.fCoveragePOI.isSolidWhite() &&
           !hasMixedSamples &&
           optimizations.fColorPOI.isOpaque())) {
-        static BlendFormula gSrcOverBlendFormula = COEFF_FORMULA(kOne_GrBlendCoeff,
-                                                                 kISA_GrBlendCoeff);
-        static PorterDuffXferProcessor gSrcOverXP(gSrcOverBlendFormula);
-        SkASSERT(!dstTexture || !dstTexture->texture());
-        gSrcOverXP.ref();
-        return &gSrcOverXP;
+        // We return nullptr here, which our caller interprets as meaning "use SimpleSrcOverXP".
+        // We don't simply return the address of that XP here because our caller would have to unref
+        // it and since it is a global object and GrProgramElement's ref-cnting system is not thread
+        // safe.
+        return nullptr;
     }
 
     BlendFormula blendFormula;

diff --git a/src/gpu/gl/GrGLGpu.cpp b/src/gpu/gl/GrGLGpu.cpp
index 32242c5..f5c646c 100644 (file)
--- a/src/gpu/gl/GrGLGpu.cpp
+++ b/src/gpu/gl/GrGLGpu.cpp
@@ -1495,7 +1495,7 @@ void GrGLGpu::flushScissor(const GrScissorState& scissorState,
 bool GrGLGpu::flushGLState(const DrawArgs& args) {
     GrXferProcessor::BlendInfo blendInfo;
     const GrPipeline& pipeline = *args.fPipeline;
-    args.fPipeline->getXferProcessor()->getBlendInfo(&blendInfo);
+    args.fPipeline->getXferProcessor().getBlendInfo(&blendInfo);
 
     this->flushColorWrite(blendInfo.fWriteColor);
     this->flushDrawFace(pipeline.getDrawFace());

diff --git a/src/gpu/gl/GrGLProgram.cpp b/src/gpu/gl/GrGLProgram.cpp
index 3cf9e4d..c1f5b37 100644 (file)
--- a/src/gpu/gl/GrGLProgram.cpp
+++ b/src/gpu/gl/GrGLProgram.cpp
@@ -89,7 +89,7 @@ void GrGLProgram::setData(const GrPrimitiveProcessor& primProc,
 
     this->setFragmentData(primProc, pipeline, textureBindings);
 
-    const GrXferProcessor& xp = *pipeline.getXferProcessor();
+    const GrXferProcessor& xp = pipeline.getXferProcessor();
     fXferProcessor->fGLProc->setData(fProgramDataManager, xp);
     append_texture_bindings(fXferProcessor.get(), xp, textureBindings);
 }

diff --git a/src/gpu/gl/GrGLProgramDesc.cpp b/src/gpu/gl/GrGLProgramDesc.cpp
index e3d292c..81f92eb 100644 (file)
--- a/src/gpu/gl/GrGLProgramDesc.cpp
+++ b/src/gpu/gl/GrGLProgramDesc.cpp
@@ -125,7 +125,7 @@ bool GrGLProgramDescBuilder::Build(GrProgramDesc* desc,
         }
     }
 
-    const GrXferProcessor& xp = *pipeline.getXferProcessor();
+    const GrXferProcessor& xp = pipeline.getXferProcessor();
     xp.getGLSLProcessorKey(*gpu->glCaps().glslCaps(), &b);
     //**** use glslCaps here?
     if (!gen_meta_key(xp, gpu->glCaps(), 0, &b)) {

diff --git a/src/gpu/gl/builders/GrGLProgramBuilder.cpp b/src/gpu/gl/builders/GrGLProgramBuilder.cpp
index dc9393d..97fcce5 100644 (file)
--- a/src/gpu/gl/builders/GrGLProgramBuilder.cpp
+++ b/src/gpu/gl/builders/GrGLProgramBuilder.cpp
@@ -93,7 +93,7 @@ bool GrGLProgramBuilder::emitAndInstallProcs(GrGLSLExpr4* inputColor, GrGLSLExpr
     this->emitAndInstallFragProcs(0, this->pipeline().numColorFragmentProcessors(), inputColor);
     this->emitAndInstallFragProcs(this->pipeline().numColorFragmentProcessors(), numProcs,
                                   inputCoverage);
-    this->emitAndInstallXferProc(*this->pipeline().getXferProcessor(), *inputColor, *inputCoverage,
+    this->emitAndInstallXferProc(this->pipeline().getXferProcessor(), *inputColor, *inputCoverage,
                                  this->pipeline().ignoresCoverage());
     return true;
 }