Fix variable collisions in processors when a processor has child proc…

…essors. (#113)

src/gpu/FragmentShaderBuilder.cpp

@@ -21,7 +21,6 @@
 
 namespace tgfx {
 FragmentShaderBuilder::FragmentShaderBuilder(ProgramBuilder* program) : ShaderBuilder(program) {
-  subStageIndices.push_back(0);
 }
 
 void FragmentShaderBuilder::onFinalize() {
@@ -32,18 +31,11 @@ void FragmentShaderBuilder::declareCustomOutputColor() {
   outputs.emplace_back(CustomColorOutputName(), SLType::Float4, ShaderVar::TypeModifier::Out);
 }
 
-void FragmentShaderBuilder::onBeforeChildProcEmitCode() {
-  subStageIndices.push_back(0);
-  // second-to-last value in the subStageIndices stack is the index of the child proc
-  // at that level which is currently emitting code.
-  _mangleString.append("_c");
-  _mangleString += std::to_string(subStageIndices[subStageIndices.size() - 2]);
+void FragmentShaderBuilder::onBeforeChildProcEmitCode(const FragmentProcessor* child) {
+  programBuilder->currentProcessors.push_back(child);
 }
 
 void FragmentShaderBuilder::onAfterChildProcEmitCode() {
-  subStageIndices.pop_back();
-  subStageIndices.back()++;
-  auto removeAt = _mangleString.rfind('_');
-  _mangleString.erase(removeAt, _mangleString.size() - removeAt);
+  programBuilder->currentProcessors.pop_back();
 }
 }  // namespace tgfx

src/gpu/FragmentShaderBuilder.h

@@ -21,17 +21,15 @@
 #include "ShaderBuilder.h"
 
 namespace tgfx {
+class FragmentProcessor;
+
 class FragmentShaderBuilder : public ShaderBuilder {
  public:
   explicit FragmentShaderBuilder(ProgramBuilder* program);
 
-  const std::string& mangleString() const {
-    return _mangleString;
-  }
-
   virtual std::string dstColor() = 0;
 
-  void onBeforeChildProcEmitCode();
+  void onBeforeChildProcEmitCode(const FragmentProcessor* child);
 
   void onAfterChildProcEmitCode();
 
@@ -46,26 +44,6 @@ class FragmentShaderBuilder : public ShaderBuilder {
 
   void onFinalize() override;
 
-  /**
-   * State that tracks which child proc in the proc tree is currently emitting code.  This is
-   * used to update the _mangleString, which is used to mangle the names of uniforms and functions
-   * emitted by the proc. subStageIndices is a stack: its count indicates how many levels deep
-   * we are in the tree, and its second-to-last value is the index of the child proc at that
-   * level which is currently emitting code. For example, if subStageIndices = [3, 1, 2, 0], that
-   * means we're currently emitting code for the base proc's 3rd child's 1st child's 2nd child.
-   */
-  std::vector<int> subStageIndices;
-
-  /**
-   * The mangle string is used to mangle the names of uniforms/functions emitted by the child
-   * procs so no duplicate uniforms/functions appear in the generated shader program. The mangle
-   * string is simply based on subStageIndices. For example, if subStageIndices = [3, 1, 2, 0],
-   * then the _mangleString will be "_c3_c1_c2", and any uniform/function emitted by that proc will
-   * have "_c3_c1_c2" appended to its name, which can be interpreted as "base proc's 3rd child's
-   * 1st child's 2nd child".
-   */
-  std::string _mangleString;
-
   friend class ProgramBuilder;
 };
 }  // namespace tgfx

src/gpu/Pipeline.cpp

@@ -18,7 +18,6 @@
 
 #include "Pipeline.h"
 #include "gpu/ProgramBuilder.h"
-#include "gpu/StagedUniformBuffer.h"
 #include "gpu/TextureSampler.h"
 #include "gpu/processors/PorterDuffXferProcessor.h"
 
@@ -33,6 +32,21 @@ Pipeline::Pipeline(std::unique_ptr<GeometryProcessor> geometryProcessor,
   if (!BlendModeAsCoeff(blendMode, &_blendInfo)) {
     xferProcessor = PorterDuffXferProcessor::Make(blendMode);
   }
+  updateProcessorIndices();
+}
+
+void Pipeline::updateProcessorIndices() {
+  int index = 0;
+  processorIndices[geometryProcessor.get()] = index++;
+  for (auto& fragmentProcessor : fragmentProcessors) {
+    FragmentProcessor::Iter iter(fragmentProcessor.get());
+    const FragmentProcessor* fp = iter.next();
+    while (fp) {
+      processorIndices[fp] = index++;
+      fp = iter.next();
+    }
+  }
+  processorIndices[getXferProcessor()] = index++;
 }
 
 const XferProcessor* Pipeline::getXferProcessor() const {
@@ -43,24 +57,23 @@ const XferProcessor* Pipeline::getXferProcessor() const {
 }
 
 void Pipeline::getUniforms(UniformBuffer* uniformBuffer) const {
-  auto buffer = static_cast<StagedUniformBuffer*>(uniformBuffer);
-  buffer->advanceStage();
+  uniformBuffer->nameSuffix = getMangledSuffix(geometryProcessor.get());
   FragmentProcessor::CoordTransformIter coordTransformIter(this);
-  geometryProcessor->setData(buffer, &coordTransformIter);
+  geometryProcessor->setData(uniformBuffer, &coordTransformIter);
   for (auto& fragmentProcessor : fragmentProcessors) {
-    buffer->advanceStage();
     FragmentProcessor::Iter iter(fragmentProcessor.get());
     const FragmentProcessor* fp = iter.next();
     while (fp) {
-      fp->setData(buffer);
+      uniformBuffer->nameSuffix = getMangledSuffix(fp);
+      fp->setData(uniformBuffer);
       fp = iter.next();
     }
   }
-  if (dstTextureInfo.texture != nullptr) {
-    buffer->advanceStage();
-    xferProcessor->setData(buffer, dstTextureInfo.texture.get(), dstTextureInfo.offset);
-  }
-  buffer->resetStage();
+  auto processor = getXferProcessor();
+  uniformBuffer->nameSuffix = getMangledSuffix(processor);
+  auto texture = dstTextureInfo.texture ? dstTextureInfo.texture.get() : nullptr;
+  processor->setData(uniformBuffer, texture, dstTextureInfo.offset);
+  uniformBuffer->nameSuffix = "";
 }
 
 std::vector<SamplerInfo> Pipeline::getSamplers() const {
@@ -96,4 +109,20 @@ void Pipeline::computeUniqueKey(Context* context, BytesKey* uniqueKey) const {
 std::unique_ptr<Program> Pipeline::createProgram(Context* context) const {
   return ProgramBuilder::CreateProgram(context, this);
 }
+
+int Pipeline::getProcessorIndex(const Processor* processor) const {
+  auto result = processorIndices.find(processor);
+  if (result == processorIndices.end()) {
+    return -1;
+  }
+  return result->second;
+}
+
+std::string Pipeline::getMangledSuffix(const Processor* processor) const {
+  auto processorIndex = getProcessorIndex(processor);
+  if (processorIndex == -1) {
+    return "";
+  }
+  return "_P" + std::to_string(processorIndex);
+}
 }  // namespace tgfx

src/gpu/Pipeline.h

@@ -18,6 +18,7 @@
 
 #pragma once
 
+#include <unordered_map>
 #include "Swizzle.h"
 #include "gpu/Blend.h"
 #include "gpu/ProgramInfo.h"
@@ -89,14 +90,25 @@ class Pipeline : public ProgramInfo {
 
   std::unique_ptr<Program> createProgram(Context* context) const override;
 
+  /**
+   * Returns the index of the processor in the pipeline. Returns -1 if the processor is not in the
+   * pipeline.
+   */
+  int getProcessorIndex(const Processor* processor) const;
+
+  std::string getMangledSuffix(const Processor* processor) const;
+
  private:
-  std::unique_ptr<GeometryProcessor> geometryProcessor;
-  std::vector<std::unique_ptr<FragmentProcessor>> fragmentProcessors;
+  std::unique_ptr<GeometryProcessor> geometryProcessor = {};
+  std::vector<std::unique_ptr<FragmentProcessor>> fragmentProcessors = {};
+  std::unordered_map<const Processor*, int> processorIndices = {};
   // This value is also the index in fragmentProcessors where coverage processors begin.
   size_t numColorProcessors = 0;
   std::unique_ptr<XferProcessor> xferProcessor;
   BlendInfo _blendInfo = {};
   DstTextureInfo dstTextureInfo = {};
   const Swizzle* _outputSwizzle = nullptr;
+
+  void updateProcessorIndices();
 };
 }  // namespace tgfx

src/gpu/ProgramBuilder.cpp

@@ -20,6 +20,20 @@
 #include "gpu/processors/FragmentProcessor.h"
 
 namespace tgfx {
+class ProcessorGuard {
+ public:
+  ProcessorGuard(ProgramBuilder* builder, const Processor* processor) : builder(builder) {
+    builder->currentProcessors.push_back(processor);
+  }
+
+  ~ProcessorGuard() {
+    builder->currentProcessors.pop_back();
+  }
+
+ private:
+  ProgramBuilder* builder = nullptr;
+};
+
 ProgramBuilder::ProgramBuilder(Context* context, const Pipeline* pipeline)
     : context(context), pipeline(pipeline) {
 }
@@ -35,22 +49,21 @@ bool ProgramBuilder::emitAndInstallProcessors() {
   return checkSamplerCounts();
 }
 
-void ProgramBuilder::advanceStage() {
-  _stageIndex++;
-  // Each output to the fragment processor gets its own code section
-  fragmentShaderBuilder()->nextStage();
-}
-
 void ProgramBuilder::emitAndInstallGeoProc(std::string* outputColor, std::string* outputCoverage) {
+  // We don't want the RTAdjustName to be mangled, so we add it to the uniform handler before the
+  // processor guard.
   uniformHandler()->addUniform(ShaderFlags::Vertex, SLType::Float4, RTAdjustName);
-  advanceStage();
+  auto geometryProcessor = pipeline->getGeometryProcessor();
+  // Set the current processor so that all variable names will be mangled correctly.
+  ProcessorGuard processorGuard(this, geometryProcessor);
   nameExpression(outputColor, "outputColor");
   nameExpression(outputCoverage, "outputCoverage");
-  auto geometryProcessor = pipeline->getGeometryProcessor();
+
+  auto processorIndex = pipeline->getProcessorIndex(geometryProcessor);
   // Enclose custom code in a block to avoid namespace conflicts
-  fragmentShaderBuilder()->codeAppendf("{ // Stage %d %s\n", _stageIndex,
+  fragmentShaderBuilder()->codeAppendf("{ // Processor%d : %s\n", processorIndex,
                                        geometryProcessor->name().c_str());
-  vertexShaderBuilder()->codeAppendf("// Geometry Processor %s\n",
+  vertexShaderBuilder()->codeAppendf("// Processor%d : %s\n", processorIndex,
                                      geometryProcessor->name().c_str());
 
   GeometryProcessor::FPCoordTransformHandler transformHandler(pipeline, &transformedCoordVars);
@@ -89,13 +102,13 @@ static const T* GetPointer(const std::vector<T>& vector, size_t atIndex) {
 std::string ProgramBuilder::emitAndInstallFragProc(const FragmentProcessor* processor,
                                                    size_t transformedCoordVarsIdx,
                                                    const std::string& input) {
-  advanceStage();
+  ProcessorGuard processorGuard(this, processor);
   std::string output;
   nameExpression(&output, "output");
 
   // Enclose custom code in a block to avoid namespace conflicts
-  fragmentShaderBuilder()->codeAppendf("{ // Stage %d %s\n", _stageIndex,
-                                       processor->name().c_str());
+  fragmentShaderBuilder()->codeAppendf(
+      "{ // Processor%d : %s\n", pipeline->getProcessorIndex(processor), processor->name().c_str());
 
   std::vector<SamplerHandle> texSamplers;
   FragmentProcessor::Iter fpIter(processor);
@@ -115,17 +128,17 @@ std::string ProgramBuilder::emitAndInstallFragProc(const FragmentProcessor* proc
                                    &coords, &textureSamplers);
 
   processor->emitCode(args);
-
   fragmentShaderBuilder()->codeAppend("}");
   return output;
 }
 
 void ProgramBuilder::emitAndInstallXferProc(const std::string& colorIn,
                                             const std::string& coverageIn) {
-  advanceStage();
-
   auto xferProcessor = pipeline->getXferProcessor();
-  fragmentShaderBuilder()->codeAppendf("{ // Xfer Processor %s\n", xferProcessor->name().c_str());
+  ProcessorGuard processorGuard(this, xferProcessor);
+  fragmentShaderBuilder()->codeAppendf("{ // Processor%d : %s\n",
+                                       pipeline->getProcessorIndex(xferProcessor),
+                                       xferProcessor->name().c_str());
 
   SamplerHandle dstTextureSamplerHandle;
   if (auto dstTexture = pipeline->dstTexture()) {
@@ -156,32 +169,23 @@ void ProgramBuilder::emitFSOutputSwizzle() {
   fragBuilder->codeAppendf("%s = %s.%s;", output.c_str(), output.c_str(), swizzle.c_str());
 }
 
-std::string ProgramBuilder::nameVariable(char prefix, const std::string& name, bool mangle) const {
-  std::string out;
-  if ('\0' != prefix) {
-    out += prefix;
-  }
-  out += name;
-  if (mangle && _stageIndex >= 0) {
-    if (out.rfind('_') == out.length() - 1) {
-      // Names containing "__" are reserved.
-      out += "x";
-    }
-    out += "_Stage";
-    out += std::to_string(_stageIndex);
+std::string ProgramBuilder::nameVariable(const std::string& name) const {
+  auto processor = currentProcessors.empty() ? nullptr : currentProcessors.back();
+  if (processor == nullptr) {
+    return name;
   }
-  return out;
+  return name + pipeline->getMangledSuffix(processor);
 }
 
 void ProgramBuilder::nameExpression(std::string* output, const std::string& baseName) {
-  // create var to hold stage result. If we already have a valid output name, just use that
+  // Create var to hold the stage result. If we already have a valid output name, just use that
   // otherwise create a new mangled one. This name is only valid if we are reordering stages
   // and have to tell stage exactly where to put its output.
   std::string outName;
   if (!output->empty()) {
     outName = *output;
   } else {
-    outName = nameVariable('\0', baseName);
+    outName = nameVariable(baseName);
   }
   fragmentShaderBuilder()->codeAppendf("vec4 %s;", outName.c_str());
   *output = outName;

src/gpu/ProgramBuilder.h

@@ -39,6 +39,10 @@ class ProgramBuilder {
     return context;
   }
 
+  const Pipeline* getPipeline() const {
+    return pipeline;
+  }
+
   virtual std::string versionDeclString() = 0;
 
   virtual std::string textureFuncName() const = 0;
@@ -56,15 +60,9 @@ class ProgramBuilder {
   }
 
   /**
-   * Generates a name for a variable. The generated string will be named prefixed by the prefix
-   * char (unless the prefix is '\0'). It also will mangle the name to be stage-specific unless
-   * explicitly asked not to.
+   * Generates a name for a variable. The generated string will be mangled to be processor-specific.
    */
-  std::string nameVariable(char prefix, const std::string& name, bool mangle = true) const;
-
-  int stageIndex() const {
-    return _stageIndex;
-  }
+  std::string nameVariable(const std::string& name) const;
 
   virtual UniformHandler* uniformHandler() = 0;
 
@@ -79,6 +77,7 @@ class ProgramBuilder {
  protected:
   Context* context = nullptr;
   const Pipeline* pipeline = nullptr;
+  int numFragmentSamplers = 0;
 
   ProgramBuilder(Context* context, const Pipeline* pipeline);
 
@@ -88,15 +87,9 @@ class ProgramBuilder {
 
   virtual bool checkSamplerCounts() = 0;
 
-  int numFragmentSamplers = 0;
-
  private:
-  /**
-   * advanceStage is called by program creator between each processor's emit code. It increments
-   * the stage offset for variable name mangling, and also ensures verification variables in the
-   * fragment shader are cleared.
-   */
-  void advanceStage();
+  std::vector<const Processor*> currentProcessors = {};
+  std::vector<ShaderVar> transformedCoordVars = {};
 
   /**
    * Generates a possibly mangled name for a stage variable and writes it to the fragment shader.
@@ -116,7 +109,7 @@ class ProgramBuilder {
 
   void emitFSOutputSwizzle();
 
-  int _stageIndex = -1;
-  std::vector<ShaderVar> transformedCoordVars = {};
+  friend class FragmentShaderBuilder;
+  friend class ProcessorGuard;
 };
 }  // namespace tgfx