From ae4f96a9e06df44f70c3d5f7324f5a7fabcd1026 Mon Sep 17 00:00:00 2001 From: "bsalomon@google.com" Date: Fri, 18 May 2012 19:54:48 +0000 Subject: [PATCH] Some refactoring of GrCustomStage and friends Review URL: http://codereview.appspot.com/6209071/ git-svn-id: http://skia.googlecode.com/svn/trunk@4003 2bbb7eff-a529-9590-31e7-b0007b416f81 --- gyp/gpu.gyp | 3 +- include/gpu/GrCustomStage.h | 30 ++++++--- include/gpu/GrProgramStageFactory.h | 106 ++++++++++++++++++++++++++++++++ src/gpu/GrCustomStage.cpp | 4 +- src/gpu/GrProgramStageFactory.cpp | 17 ----- src/gpu/GrProgramStageFactory.h | 50 --------------- src/gpu/effects/GrConvolutionEffect.cpp | 102 +++++++++--------------------- src/gpu/effects/GrConvolutionEffect.h | 15 +++-- src/gpu/gl/GrGLProgram.cpp | 8 +-- src/gpu/gl/GrGLProgramStage.cpp | 2 +- src/gpu/gl/GrGLProgramStage.h | 5 +- src/gpu/gl/GrGpuGLShaders.cpp | 6 +- 12 files changed, 183 insertions(+), 165 deletions(-) create mode 100644 include/gpu/GrProgramStageFactory.h delete mode 100644 src/gpu/GrProgramStageFactory.cpp delete mode 100644 src/gpu/GrProgramStageFactory.h diff --git a/gyp/gpu.gyp b/gyp/gpu.gyp index 04cb4e3..621ea02 100644 --- a/gyp/gpu.gyp +++ b/gyp/gpu.gyp @@ -187,6 +187,7 @@ '../include/gpu/GrNoncopyable.h', '../include/gpu/GrPaint.h', '../include/gpu/GrPoint.h', + '../include/gpu/GrProgramStageFactory.h', '../include/gpu/GrRect.h', '../include/gpu/GrRefCnt.h', '../include/gpu/GrRenderTarget.h', @@ -247,8 +248,6 @@ '../src/gpu/GrPathUtils.cpp', '../src/gpu/GrPathUtils.h', '../src/gpu/GrPlotMgr.h', - '../src/gpu/GrProgramStageFactory.cpp', - '../src/gpu/GrProgramStageFactory.h', '../src/gpu/GrRandom.h', '../src/gpu/GrRectanizer.cpp', '../src/gpu/GrRectanizer.h', diff --git a/include/gpu/GrCustomStage.h b/include/gpu/GrCustomStage.h index 64bcce3..908b10d 100644 --- a/include/gpu/GrCustomStage.h +++ b/include/gpu/GrCustomStage.h @@ -9,16 +9,18 @@ #define GrCustomStage_DEFINED #include "GrRefCnt.h" +#include "GrNoncopyable.h" +#include "GrProgramStageFactory.h" +#include "SkTemplates.h" class GrContext; -class GrProgramStageFactory; /** Provides custom vertex shader, fragment shader, uniform data for a - particular stage of the Ganesh shading pipeline. - TODO: may want to refcount these? */ + particular stage of the Ganesh shading pipeline. */ class GrCustomStage : public GrRefCnt { public: + typedef GrProgramStageFactory::StageKey StageKey; GrCustomStage(); virtual ~GrCustomStage(); @@ -31,10 +33,24 @@ public: stage guaranteed to produce an opaque output? */ virtual bool isOpaque(bool inputTextureIsOpaque) const; - /** This pointer, besides creating back-end-specific helper - objects, is used for run-time-type-identification. Every - subclass must return a consistent unique value for it. */ - virtual GrProgramStageFactory* getFactory() const = 0; + /** This object, besides creating back-end-specific helper + objects, is used for run-time-type-identification. The factory should be + an instance of templated class, GrTProgramStageFactory. It is templated + on the subclass of GrCustomStage. The subclass must have a nested type + (or typedef) named GLProgramStage which will be the subclass of + GrGLProgramStage created by the factory. + + Example: + class MyCustomStage : public GrCustomStage { + ... + virtual const GrProgramStageFactory& getFactory() const + SK_OVERRIDE { + return GrTProgramStageFactory::getInstance(); + } + ... + }; + */ + virtual const GrProgramStageFactory& getFactory() const = 0; /** Returns true if the other custom stage will generate equal output. diff --git a/include/gpu/GrProgramStageFactory.h b/include/gpu/GrProgramStageFactory.h new file mode 100644 index 0000000..e73b9ba --- /dev/null +++ b/include/gpu/GrProgramStageFactory.h @@ -0,0 +1,106 @@ +/* + * Copyright 2012 Google Inc. + * + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +#ifndef GrProgramStageFactory_DEFINED +#define GrProgramStageFactory_DEFINED + +#include "GrTypes.h" + +/** Given a GrCustomStage of a particular type, creates the corresponding + graphics-backend-specific GrProgramStage. Also tracks equivalence + of shaders generated via a key. + */ + +class GrCustomStage; +class GrGLProgramStage; + +class GrProgramStageFactory : public GrNoncopyable { +public: + typedef uint16_t StageKey; + enum { + kProgramStageKeyBits = 10, + }; + + virtual StageKey stageKey(const GrCustomStage* stage) const = 0; + virtual GrGLProgramStage* createGLInstance( + const GrCustomStage* stage) const = 0; + + bool operator ==(const GrProgramStageFactory& b) const { + return fStageClassID == b.fStageClassID; + } + bool operator !=(const GrProgramStageFactory& b) const { + return !(*this == b); + } + +protected: + enum { + kIllegalStageClassID = 0, + }; + + GrProgramStageFactory() { + fStageClassID = kIllegalStageClassID; + } + + static StageKey GenID() { + // fCurrStageClassID has been initialized to kIllegalStageClassID. The + // atomic inc returns the old value not the incremented value. So we add + // 1 to the returned value. + int32_t id = sk_atomic_inc(&fCurrStageClassID) + 1; + GrAssert(id < (1 << (8 * sizeof(StageKey) - kProgramStageKeyBits))); + return id; + } + + StageKey fStageClassID; + +private: + static int32_t fCurrStageClassID; +}; + +template +class GrTProgramStageFactory : public GrProgramStageFactory { + +public: + typedef typename StageClass::GLProgramStage GLProgramStage; + + /** Returns an value that idenitifes the shader code generated by + a GrCustomStage. This enables caching of generated shaders. Part of the + id identifies the GrCustomShader subclass. The remainder is based + on the aspects of the GrCustomStage object's configuration that affect + code generation. */ + virtual StageKey stageKey(const GrCustomStage* stage) const SK_OVERRIDE { + GrAssert(kIllegalStageClassID != fStageClassID); + StageKey stageID = GLProgramStage::GenKey(stage); + static const StageKey kIllegalIDMask = + ~((1 << kProgramStageKeyBits) - 1); + GrAssert(!(kIllegalIDMask & stageID)); + return fStageClassID | stageID; + } + + /** Returns a new instance of the appropriate *GL* implementation class + for the given GrCustomStage; caller is responsible for deleting + the object. */ + virtual GLProgramStage* createGLInstance( + const GrCustomStage* stage) const SK_OVERRIDE { + return new GLProgramStage(stage); + } + + static const GrProgramStageFactory& getInstance() { + static SkAlignedSTStorage<1, GrTProgramStageFactory> gInstanceMem; + static const GrTProgramStageFactory* gInstance; + if (!gInstance) { + gInstance = new (gInstanceMem.get()) GrTProgramStageFactory(); + } + return *gInstance; + } + +protected: + GrTProgramStageFactory() { + fStageClassID = GenID() << kProgramStageKeyBits; + } +}; + +#endif diff --git a/src/gpu/GrCustomStage.cpp b/src/gpu/GrCustomStage.cpp index f63c79a..c7d0844 100644 --- a/src/gpu/GrCustomStage.cpp +++ b/src/gpu/GrCustomStage.cpp @@ -8,8 +8,10 @@ #include "GrContext.h" #include "GrCustomStage.h" -GrCustomStage::GrCustomStage() { +int32_t GrProgramStageFactory::fCurrStageClassID = + GrProgramStageFactory::kIllegalStageClassID; +GrCustomStage::GrCustomStage() { } GrCustomStage::~GrCustomStage() { diff --git a/src/gpu/GrProgramStageFactory.cpp b/src/gpu/GrProgramStageFactory.cpp deleted file mode 100644 index 4bacf92..0000000 --- a/src/gpu/GrProgramStageFactory.cpp +++ /dev/null @@ -1,17 +0,0 @@ -/* - * Copyright 2012 Google Inc. - * - * Use of this source code is governed by a BSD-style license that can be - * found in the LICENSE file. - */ - -#include "GrProgramStageFactory.h" - -GrProgramStageFactory::~GrProgramStageFactory(void) { - -} - -uint16_t GrProgramStageFactory::stageKey(const GrCustomStage*) { - return 0; -} - diff --git a/src/gpu/GrProgramStageFactory.h b/src/gpu/GrProgramStageFactory.h deleted file mode 100644 index d3580d5..0000000 --- a/src/gpu/GrProgramStageFactory.h +++ /dev/null @@ -1,50 +0,0 @@ -/* - * Copyright 2012 Google Inc. - * - * Use of this source code is governed by a BSD-style license that can be - * found in the LICENSE file. - */ - -#ifndef GrProgramStageFactory_DEFINED -#define GrProgramStageFactory_DEFINED - -#include "GrTypes.h" - -class GrCustomStage; -class GrGLProgramStage; - -/** Given a GrCustomStage of a particular type, creates the corresponding - graphics-backend-specific GrProgramStage. Also tracks equivalence - of shaders generated via stageKey(). - - TODO: most of this class' subclasses are boilerplate and ought to be - templateable? -*/ - -class GrProgramStageFactory { - -public: - - virtual ~GrProgramStageFactory(); - - /** Returns a short unique identifier for this subclass x its - parameters. If the key differs, different shader code must - be generated; if the key matches, shader code can be reused. - 0 == no custom stage. */ - virtual uint16_t stageKey(const GrCustomStage*); - - /** Returns a new instance of the appropriate *GL* implementation class - for the given GrCustomStage; caller is responsible for deleting - the object. */ - virtual GrGLProgramStage* createGLInstance(GrCustomStage*) = 0; - -protected: - - /** Disable default constructor - instances should be singletons - with static factory functions: our test examples are all stateless, - but we suspect that future implementations may want to cache data? */ - GrProgramStageFactory() { } -}; - - -#endif diff --git a/src/gpu/effects/GrConvolutionEffect.cpp b/src/gpu/effects/GrConvolutionEffect.cpp index d63b023..563d1a4 100644 --- a/src/gpu/effects/GrConvolutionEffect.cpp +++ b/src/gpu/effects/GrConvolutionEffect.cpp @@ -17,7 +17,7 @@ class GrGLConvolutionEffect : public GrGLProgramStage { public: - GrGLConvolutionEffect(GrConvolutionEffect* data); + GrGLConvolutionEffect(const GrCustomStage* stage); virtual const char* name() const SK_OVERRIDE; virtual void setupVSUnis(VarArray* vsUnis, int stage) SK_OVERRIDE; virtual void setupFSUnis(VarArray* fsUnis, int stage) SK_OVERRIDE; @@ -30,13 +30,14 @@ public: const char* sampleCoords) SK_OVERRIDE; virtual void initUniforms(const GrGLInterface*, int programID) SK_OVERRIDE; - virtual void setData(const GrGLInterface*, GrCustomStage*, + virtual void setData(const GrGLInterface*, const GrCustomStage*, const GrGLTexture*) SK_OVERRIDE; + static inline StageKey GenKey(const GrCustomStage* s); + protected: - GrConvolutionEffect* fData; - + int fKernelWidth; GrGLShaderVar* fKernelVar; GrGLShaderVar* fImageIncrementVar; @@ -48,18 +49,16 @@ private: typedef GrGLProgramStage INHERITED; }; -GrGLConvolutionEffect::GrGLConvolutionEffect(GrConvolutionEffect* data) - : fData(data) - , fKernelVar(NULL) +GrGLConvolutionEffect::GrGLConvolutionEffect(const GrCustomStage* data) + : fKernelVar(NULL) , fImageIncrementVar(NULL) , fKernelLocation(0) - , fImageIncrementLocation(0) -{ - + , fImageIncrementLocation(0) { + fKernelWidth = static_cast(data)->width(); } const char* GrGLConvolutionEffect::name() const { - return fData->name(); + return GrConvolutionEffect::Name(); } void GrGLConvolutionEffect::setupVSUnis(VarArray* vsUnis, @@ -81,7 +80,7 @@ void GrGLConvolutionEffect::setupFSUnis(VarArray* fsUnis, fKernelVar->setType(kFloat_GrSLType); fKernelVar->setTypeModifier( GrGLShaderVar::kUniform_TypeModifier); - fKernelVar->setArrayCount(fData->fKernelWidth); + fKernelVar->setArrayCount(fKernelWidth); (*fKernelVar->accessName()) = "uKernel"; fKernelVar->accessName()->appendS32(stage); @@ -93,7 +92,7 @@ void GrGLConvolutionEffect::setupFSUnis(VarArray* fsUnis, void GrGLConvolutionEffect::emitVS(GrStringBuilder* code, const char* vertexCoords) { - float scale = (fData->fKernelWidth - 1) * 0.5f; + float scale = (fKernelWidth - 1) * 0.5f; code->appendf("\t\t%s -= vec2(%g, %g) * %s;\n", vertexCoords, scale, scale, fImageIncrementVar->getName().c_str()); @@ -121,7 +120,7 @@ void GrGLConvolutionEffect::emitFS(GrStringBuilder* code, code->appendf("\t\tvec4 sum = vec4(0, 0, 0, 0);\n"); code->appendf("\t\tvec2 coord = %s;\n", sampleCoords); code->appendf("\t\tfor (int i = 0; i < %d; i++) {\n", - fData->fKernelWidth); + fKernelWidth); code->appendf("\t\t\tsum += "); this->emitTextureLookup(code, samplerName, "coord"); @@ -143,14 +142,17 @@ void GrGLConvolutionEffect::initUniforms(const GrGLInterface* gl, } void GrGLConvolutionEffect::setData(const GrGLInterface* gl, - GrCustomStage* data, + const GrCustomStage* data, const GrGLTexture* texture) { - fData = static_cast(data); + const GrConvolutionEffect* conv = + static_cast(data); + // the code we generated was for a specific kernel width + GrAssert(conv->width() == fKernelWidth); GR_GL_CALL(gl, Uniform1fv(fKernelLocation, - fData->fKernelWidth, - fData->fKernel)); + fKernelWidth, + conv->kernel())); float imageIncrement[2] = { 0 }; - switch (fData->fDirection) { + switch (conv->direction()) { case GrSamplerState::kX_FilterDirection: imageIncrement[0] = 1.0f / texture->width(); break; @@ -163,58 +165,9 @@ void GrGLConvolutionEffect::setData(const GrGLInterface* gl, GR_GL_CALL(gl, Uniform2fv(fImageIncrementLocation, 1, imageIncrement)); } -///////////////////////////////////////////////////////////////////// -// TODO: stageKey() and sEffectId are the only non-boilerplate in -// this class; we ought to be able to templatize? - -class GrConvolutionEffectFactory : public GrProgramStageFactory { - -public: - - virtual ~GrConvolutionEffectFactory(); - - virtual uint16_t stageKey(const GrCustomStage* s) SK_OVERRIDE; - virtual GrGLProgramStage* createGLInstance(GrCustomStage* s) SK_OVERRIDE; - - static GrConvolutionEffectFactory* getInstance(); - -protected: - - GrConvolutionEffectFactory(); - - // TODO: find a more reliable installation than hand-coding - // id values like '1'. - static const int sEffectId = 1; - -private: - - typedef GrProgramStageFactory INHERITED; -}; - -GrConvolutionEffectFactory::~GrConvolutionEffectFactory() { - -} - -uint16_t GrConvolutionEffectFactory::stageKey(const GrCustomStage* s) { - const GrConvolutionEffect* c = - static_cast(s); - GrAssert(c->width() < 256); - return (sEffectId << 8) | (c->width() & 0xff); -} - -GrGLProgramStage* GrConvolutionEffectFactory::createGLInstance( - GrCustomStage* s) { - return new GrGLConvolutionEffect(static_cast(s)); -} - -GrConvolutionEffectFactory* GrConvolutionEffectFactory::getInstance() { - static GrConvolutionEffectFactory* instance = - new GrConvolutionEffectFactory; - return instance; -} - -GrConvolutionEffectFactory::GrConvolutionEffectFactory() { - +GrGLProgramStage::StageKey GrGLConvolutionEffect::GenKey( + const GrCustomStage* s) { + return static_cast(s)->width(); } ///////////////////////////////////////////////////////////////////// @@ -236,11 +189,12 @@ GrConvolutionEffect::~GrConvolutionEffect() { } const char* GrConvolutionEffect::name() const { - return "Convolution"; + return Name(); } -GrProgramStageFactory* GrConvolutionEffect::getFactory() const { - return GrConvolutionEffectFactory::getInstance(); + +const GrProgramStageFactory& GrConvolutionEffect::getFactory() const { + return GrTProgramStageFactory::getInstance(); } bool GrConvolutionEffect::isEqual(const GrCustomStage * sBase) const { diff --git a/src/gpu/effects/GrConvolutionEffect.h b/src/gpu/effects/GrConvolutionEffect.h index e3c3aa9..4fdddd9 100644 --- a/src/gpu/effects/GrConvolutionEffect.h +++ b/src/gpu/effects/GrConvolutionEffect.h @@ -11,6 +11,8 @@ #include "GrCustomStage.h" #include "GrSamplerState.h" // for MAX_KENEL_WIDTH, FilterDirection +class GrGLConvolutionEffect; + class GrConvolutionEffect : public GrCustomStage { public: @@ -19,19 +21,24 @@ public: unsigned int kernelWidth, const float* kernel); virtual ~GrConvolutionEffect(); + unsigned int width() const { return fKernelWidth; } + const float* kernel() const { return fKernel; } + GrSamplerState::FilterDirection direction() const { return fDirection; } + + static const char* Name() { return "Convolution"; } + + typedef GrGLConvolutionEffect GLProgramStage; + virtual const char* name() const SK_OVERRIDE; - virtual GrProgramStageFactory* getFactory() const SK_OVERRIDE; + virtual const GrProgramStageFactory& getFactory() const SK_OVERRIDE; virtual bool isEqual(const GrCustomStage *) const SK_OVERRIDE; - unsigned int width() const { return fKernelWidth; } - protected: GrSamplerState::FilterDirection fDirection; unsigned int fKernelWidth; float fKernel[MAX_KERNEL_WIDTH]; - friend class GrGLConvolutionEffect; private: diff --git a/src/gpu/gl/GrGLProgram.cpp b/src/gpu/gl/GrGLProgram.cpp index e576163..755a2f8 100644 --- a/src/gpu/gl/GrGLProgram.cpp +++ b/src/gpu/gl/GrGLProgram.cpp @@ -629,10 +629,10 @@ bool GrGLProgram::genProgram(const GrGLContextInfo& gl, } if (NULL != customStages[s]) { - GrProgramStageFactory* factory = + const GrProgramStageFactory& factory = customStages[s]->getFactory(); programData->fCustomStage[s] = - factory->createGLInstance(customStages[s]); + factory.createGLInstance(customStages[s]); } this->genStageCode(gl, s, @@ -755,10 +755,10 @@ bool GrGLProgram::genProgram(const GrGLContextInfo& gl, } if (NULL != customStages[s]) { - GrProgramStageFactory* factory = + const GrProgramStageFactory& factory = customStages[s]->getFactory(); programData->fCustomStage[s] = - factory->createGLInstance(customStages[s]); + factory.createGLInstance(customStages[s]); } this->genStageCode(gl, s, fProgramDesc.fStages[s], diff --git a/src/gpu/gl/GrGLProgramStage.cpp b/src/gpu/gl/GrGLProgramStage.cpp index 06c4d93..28a711d 100644 --- a/src/gpu/gl/GrGLProgramStage.cpp +++ b/src/gpu/gl/GrGLProgramStage.cpp @@ -25,7 +25,7 @@ void GrGLProgramStage::initUniforms(const GrGLInterface*, int progID) { } -void GrGLProgramStage::setData(const GrGLInterface*, GrCustomStage*, +void GrGLProgramStage::setData(const GrGLInterface*, const GrCustomStage*, const GrGLTexture*) { } diff --git a/src/gpu/gl/GrGLProgramStage.h b/src/gpu/gl/GrGLProgramStage.h index 61e62e5..2141640 100644 --- a/src/gpu/gl/GrGLProgramStage.h +++ b/src/gpu/gl/GrGLProgramStage.h @@ -9,12 +9,12 @@ #define GrGLCustomStage_DEFINED #include "GrAllocator.h" +#include "GrCustomStage.h" #include "GrGLShaderBuilder.h" #include "GrGLShaderVar.h" #include "GrGLSL.h" #include "GrStringBuilder.h" -class GrCustomStage; struct GrGLInterface; class GrGLTexture; @@ -31,6 +31,7 @@ class GrGLTexture; class GrGLProgramStage { public: + typedef GrCustomStage::StageKey StageKey ; // TODO: redundant with GrGLProgram.cpp enum { kUnusedUniform = -1, @@ -90,7 +91,7 @@ public: are to be read. TODO: since we don't have a factory, we can't assert to enforce this. Shouldn't we? */ - virtual void setData(const GrGLInterface*, GrCustomStage*, + virtual void setData(const GrGLInterface*, const GrCustomStage*, const GrGLTexture*); // TODO: needs a better name diff --git a/src/gpu/gl/GrGpuGLShaders.cpp b/src/gpu/gl/GrGpuGLShaders.cpp index d32c57e..08cfaf7 100644 --- a/src/gpu/gl/GrGpuGLShaders.cpp +++ b/src/gpu/gl/GrGpuGLShaders.cpp @@ -302,7 +302,7 @@ bool GrGpuGLShaders::programUnitTest() { (GrSamplerState::FilterDirection)direction, stage.fKernelWidth, kernel); stage.fCustomStageKey = - customStages[s]->getFactory()->stageKey(customStages[s]); + customStages[s]->getFactory().stageKey(customStages[s]); } } CachedData cachedData; @@ -933,8 +933,8 @@ void setup_custom_stage(GrGLProgram::ProgramDesc::StageDesc* stage, GrGLProgram* program, int index) { GrCustomStage* customStage = sampler.getCustomStage(); if (customStage) { - GrProgramStageFactory* factory = customStage->getFactory(); - stage->fCustomStageKey = factory->stageKey(customStage); + const GrProgramStageFactory& factory = customStage->getFactory(); + stage->fCustomStageKey = factory.stageKey(customStage); customStages[index] = customStage; } else { stage->fCustomStageKey = 0; -- 2.7.4