Express (GLSL expression, possibly known value) pairs as a class

src/gpu/gl/GrGLProgram.cpp

Index: src/gpu/gl/GrGLProgram.cpp
diff --git a/src/gpu/gl/GrGLProgram.cpp b/src/gpu/gl/GrGLProgram.cpp
index 2f8e8076636abcfdc7e99117b5b2c532e2fe7771..8ffa472c82233cf00010861d7b2766cc0257783d 100644
--- a/src/gpu/gl/GrGLProgram.cpp
+++ b/src/gpu/gl/GrGLProgram.cpp
@@ -144,13 +144,13 @@ inline void blend_term_string(SkString* str, SkXfermode::Coeff coeff,
         str->printf("(%s * %s)", src, value);
         break;
     case SkXfermode::kISC_Coeff:
-        str->printf("((%s - %s) * %s)", GrGLSLOnesVecf(4), src, value);
+        str->printf("((%s - %s) * %s)", GrGLSLExpr<4>(1).c_str(), src, value);
         break;
     case SkXfermode::kDC_Coeff:
         str->printf("(%s * %s)", dst, value);
         break;
     case SkXfermode::kIDC_Coeff:
-        str->printf("((%s - %s) * %s)", GrGLSLOnesVecf(4), dst, value);
+        str->printf("((%s - %s) * %s)", GrGLSLExpr<4>(1).c_str(), dst, value);
         break;
     case SkXfermode::kSA_Coeff:      /** src alpha */
         str->printf("(%s.a * %s)", src, value);
@@ -182,29 +182,18 @@ void add_color_filter(GrGLShaderBuilder* builder,
     SkString colorStr, constStr;
     blend_term_string(&colorStr, colorCoeff, filterColor, inColor, inColor);
     blend_term_string(&constStr, uniformCoeff, filterColor, inColor, filterColor);
-
-    SkString sum;
-    GrGLSLAddf<4>(&sum, colorStr.c_str(), constStr.c_str());
-    builder->fsCodeAppendf("\t%s = %s;\n", outputVar, sum.c_str());
-}
-}
-
-namespace {
-
-void expand_known_value4f(SkString* string, GrSLConstantVec vec) {
-    SkASSERT(string->isEmpty() == (vec != kNone_GrSLConstantVec));
-    switch (vec) {
-        case kNone_GrSLConstantVec:
-            break;
-        case kZeros_GrSLConstantVec:
-            *string = GrGLSLZerosVecf(4);
-            break;
-        case kOnes_GrSLConstantVec:
-            *string = GrGLSLOnesVecf(4);
-            break;
+    GrGLSLExpr<4> sum;
+    if (colorStr.isEmpty() && constStr.isEmpty()) {
+        sum = GrGLSLExpr<4>(0);
+    } else if (colorStr.isEmpty()) {
+        sum = constStr;
+    } else if (constStr.isEmpty()) {
+        sum = colorStr;
+    } else {
+        sum = GrGLSLExpr<4>(colorStr) + GrGLSLExpr<4>(constStr);
     }
+    builder->fsCodeAppendf("\t%s = %s;\n", outputVar, sum.c_str());
 }
-
 }
 
 bool GrGLProgram::genProgram(const GrEffectStage* colorStages[],
@@ -221,8 +210,7 @@ bool GrGLProgram::genProgram(const GrEffectStage* colorStages[],
     }
 
     // incoming color to current stage being processed.
-    SkString inColor = builder.getInputColor();
-    GrSLConstantVec knownColorValue = builder.getKnownColorValue();
+    GrGLSLExpr<4> inColor = builder.getInputColor();
 
     // Get the coeffs for the Mode-based color filter, determine if color is needed.
     SkXfermode::Coeff colorCoeff;
@@ -249,7 +237,6 @@ bool GrGLProgram::genProgram(const GrEffectStage* colorStages[],
                             fDesc.effectKeys(),
                             fDesc.numColorEffects(),
                             &inColor,
-                            &knownColorValue,
                             effectUniformArrays.get(),
                             glEffects.get());
 
@@ -266,24 +253,14 @@ bool GrGLProgram::genProgram(const GrEffectStage* colorStages[],
                                                              &colorFilterColorUniName);
 
         builder.fsCodeAppend("\tvec4 filteredColor;\n");
-        const char* color;
-        // add_color_filter requires a real input string.
-        if (knownColorValue == kOnes_GrSLConstantVec) {
-            color = GrGLSLOnesVecf(4);
-        } else if (knownColorValue == kZeros_GrSLConstantVec) {
-            color = GrGLSLZerosVecf(4);
-        } else {
-            color = inColor.c_str();
-        }
         add_color_filter(&builder, "filteredColor", filterColorCoeff,
-                         colorCoeff, colorFilterColorUniName, color);
+                         colorCoeff, colorFilterColorUniName, inColor.c_str());
         inColor = "filteredColor";
     }
 
     ///////////////////////////////////////////////////////////////////////////
     // compute the partial coverage
-    SkString inCoverage = builder.getInputCoverage();
-    GrSLConstantVec knownCoverageValue = builder.getKnownCoverageValue();
+    GrGLSLExpr<4> inCoverage = builder.getInputCoverage();
 
     for (int e = 0; e < fDesc.numCoverageEffects(); ++e) {
         effectUniformArrays[e] = &fCoverageEffects[e].fSamplerUnis;
@@ -293,7 +270,6 @@ bool GrGLProgram::genProgram(const GrEffectStage* colorStages[],
                         fDesc.getEffectKeys() + fDesc.numColorEffects(),
                         fDesc.numCoverageEffects(),
                         &inCoverage,
-                        &knownCoverageValue,
                         effectUniformArrays.get(),
                         glEffects.get());
     for (int e = 0; e < fDesc.numCoverageEffects(); ++e) {
@@ -301,8 +277,8 @@ bool GrGLProgram::genProgram(const GrEffectStage* colorStages[],
     }
 
     // discard if coverage is zero
-    if (header.fDiscardIfZeroCoverage && kOnes_GrSLConstantVec != knownCoverageValue) {
-        if (kZeros_GrSLConstantVec == knownCoverageValue) {
+    if (header.fDiscardIfZeroCoverage && !inCoverage.isOnes()) {
+        if (inCoverage.isZeros()) {
             // This is unfortunate.
             builder.fsCodeAppend("\tdiscard;\n");
         } else {
@@ -315,79 +291,30 @@ bool GrGLProgram::genProgram(const GrEffectStage* colorStages[],
         const char* secondaryOutputName = builder.enableSecondaryOutput();
 
         // default coeff to ones for kCoverage_DualSrcOutput
-        SkString coeff;
-        GrSLConstantVec knownCoeffValue = kOnes_GrSLConstantVec;
+        GrGLSLExpr<4> coeff(1);
         if (GrGLProgramDesc::kSecondaryCoverageISA_CoverageOutput == header.fCoverageOutput) {
             // Get (1-A) into coeff
-            SkString inColorAlpha;
-            GrGLSLGetComponent4f(&inColorAlpha,
-                                    inColor.c_str(),
-                                    kA_GrColorComponentFlag,
-                                    knownColorValue,
-                                    true);
-            knownCoeffValue = GrGLSLSubtractf<1>(&coeff,
-                                                 NULL,
-                                                 inColorAlpha.c_str(),
-                                                 kOnes_GrSLConstantVec,
-                                                 knownColorValue,
-                                                 true);
+            coeff = GrGLSLExpr<4>::VectorCast(1 - inColor.a());

[bsalomon, 2013/10/02 15:18:04]

This line looks a bit odd to me. Does the 1 get casted to a GrGLSLExpr<1>?

[Kimmo Kinnunen, 2013/10/08 12:18:29]

It will use the Expr<M> operator-(int, Expr<M>).
This is part of the design where "1" is the tag that means expression with all
ones.

Ints are not convertible to Exprs because const char* are, ie. Expr e = 0 is
ambiguous. Also ints convertible to something else seem to sometimes cause
unforseen problems.

If desired, const char* do not need to be convertible after removing few
instances of passing expressions / variable names as const char*..

[bsalomon, 2013/10/08 14:45:27]

Ah-ha! I missed that operator on first read. This feels a bit too auto-magical
to me. It isn't totally obvious how it works to a causal reader. And going much
further in this direction could lead to code that is pretty hard to read. We
could add float variants and other mixed-type operators and wind up with long
expressions that are mixes of string, exprs, ints, floats that turn into a lot
of conversions or operators. It doesn't seem too onerous to require the caller
to type:

GrGLSLExpr<1>(1) - inColor.a()
instead of
1 - inColor.a()

Maybe another way of stating my concern is that Skia has deliberately steered
clear of a lot of operator overloading in order to make the code more explicit
about what functions are being called to a reader. I think GrGLSLExpr is an
exception because something like

d.div(a.mul(c).add(b)) is so much harder to read than

d / (a*c + b)

but when the types of a, b, c, and d are different I feel like it starts to
become hard to know exactly what's going on. I know that this one operator
doesn't exactly create this situation, but I feel like it is nudging us down
that path.

[Kimmo Kinnunen, 2013/10/09 13:39:59]

..
This was not the rationale. The rationale was the "impossibility" to implement
use-cases that are useful, maybe even the primary rationale for having the Expr,
e.g. to use the algorithms on Expr"ish" objects, in generic way.

Example of the problem:

 template <Expr>
 inv_alpha_adder(const Expr& a, const Expr& b) {
   return a + (ExprWITHDDIMENSIONOFALPHA(1) - b.a());
 }

 SLExpr<4> r = inv_alpha_adder(SLExpr<4>(1), SLExpr<4>(2))
 
Eg. to reduce the dimension, you do as follows -->

 template <Expr, AlphaOner>
 inv_alpha_adder(const Expr& a, const Expr& b, const AlphaOner& one) {
   return a + (one() - b.a());
 }

 class Oner {
 public:
   SLExpr<1> operator() const { return SLExpr<1>(1); }
 }

 Oner oner;
 SLExpr<4> r = inv_alpha_adder(SLExpr<4>(1), SLExpr<4>(2), oner)
 
Though, I'm no template expert. There might be better ways.

Of course, you can also contest the need for the generality ("if you need
inv_alpha_adder with non-SLExpr, re-write it").
 
For the actual content, GLSL, the writer naturally has to know what's going on
wrt the implicit type casts. Most likely how that translates to C++ templates
will surprise him, in all more complex cases.

I'm not saying "exprs all the way", per se, but extending the system with none
of the hairy stuff might be unrealistic. This in turn might make future-proofing
slightly frustrating argument: the future that is used as a premise would be
order of magnitude more hairy or the extension would not happen, both
invalidating the future-proofing as an argument.

I removed the operator and used your suggestion. The problem, if there is one,
is not visible in this patch.

[bsalomon, 2013/10/09 19:53:31]

I'm not 100% sure that I follow the example but I think we should defer until
the need arises for more complexity.

         } else if (GrGLProgramDesc::kSecondaryCoverageISC_CoverageOutput == header.fCoverageOutput) {
             // Get (1-RGBA) into coeff
-            knownCoeffValue = GrGLSLSubtractf<4>(&coeff,
-                                                 NULL,
-                                                 inColor.c_str(),
-                                                 kOnes_GrSLConstantVec,
-                                                 knownColorValue,
-                                                 true);
+            coeff = 1 - inColor;
         }
         // Get coeff * coverage into modulate and then write that to the dual source output.
-        SkString modulate;
-        GrGLSLModulatef<4>(&modulate,
-                           coeff.c_str(),
-                           inCoverage.c_str(),
-                           knownCoeffValue,
-                           knownCoverageValue,
-                           false);
-        builder.fsCodeAppendf("\t%s = %s;\n", secondaryOutputName, modulate.c_str());
+        builder.fsCodeAppendf("\t%s = %s;\n", secondaryOutputName, (coeff * inCoverage).c_str());
     }
 
     ///////////////////////////////////////////////////////////////////////////
     // combine color and coverage as frag color
 
     // Get "color * coverage" into fragColor
-    SkString fragColor;
-    GrSLConstantVec knownFragColorValue = GrGLSLModulatef<4>(&fragColor,
-                                                             inColor.c_str(),
-                                                             inCoverage.c_str(),
-                                                             knownColorValue,
-                                                             knownCoverageValue,
-                                                             true);
+    GrGLSLExpr<4> fragColor = inColor * inCoverage;
     // Now tack on "+(1-coverage)dst onto the frag color if we were asked to do so.
     if (GrGLProgramDesc::kCombineWithDst_CoverageOutput == header.fCoverageOutput) {
-        SkString dstCoeff;
-        GrSLConstantVec knownDstCoeffValue = GrGLSLSubtractf<4>(&dstCoeff,
-                                                                NULL,
-                                                                inCoverage.c_str(),
-                                                                kOnes_GrSLConstantVec,
-                                                                knownCoverageValue,
-                                                                true);
-        SkString dstContribution;
-        GrSLConstantVec knownDstContributionValue = GrGLSLModulatef<4>(&dstContribution,
-                                                                       dstCoeff.c_str(),
-                                                                       builder.dstColor(),
-                                                                       knownDstCoeffValue,
-                                                                       kNone_GrSLConstantVec,
-                                                                       true);
-        SkString oldFragColor = fragColor;
-        fragColor.reset();
-        GrGLSLAddf<4>(&fragColor,
-                      oldFragColor.c_str(),
-                      dstContribution.c_str(),
-                      knownFragColorValue,
-                      knownDstContributionValue,
-                      false);
-    } else {
-        expand_known_value4f(&fragColor, knownFragColorValue);
+        GrGLSLExpr<4> dstCoeff = 1 - inCoverage;
+
+        GrGLSLExpr<4> dstContribution = dstCoeff * GrGLSLExpr<4>(builder.dstColor());
+
+        fragColor = fragColor + dstContribution;
     }
     builder.fsCodeAppendf("\t%s = %s;\n", builder.getColorOutputName(), fragColor.c_str());