Optimizations and more documentation of SkShadowShader

src/core/SkShadowShader.cpp

Index: src/core/SkShadowShader.cpp
diff --git a/src/core/SkShadowShader.cpp b/src/core/SkShadowShader.cpp
index b1f49b851d775877d6c711db3089a5456abf44a6..141128f2a4434695aaa35fc2ce09bef7162dc2a2 100644
--- a/src/core/SkShadowShader.cpp
+++ b/src/core/SkShadowShader.cpp
@@ -118,12 +118,12 @@ public:
             if (fNumNonAmbLights < SkShadowShader::kMaxNonAmbientLights) {
                 fLightColor[fNumNonAmbLights] = lights->light(i).color();
 
-                if (SkLights::Light::kDirectional_LightType == lights->light(i).type()) {
-                    fLightDirOrPos[fNumNonAmbLights] = lights->light(i).dir();
-                    fLightIntensity[fNumNonAmbLights] = 0.0f;
-                } else if (SkLights::Light::kPoint_LightType == lights->light(i).type()) {
+                if (SkLights::Light::kPoint_LightType == lights->light(i).type()) {
                     fLightDirOrPos[fNumNonAmbLights] = lights->light(i).pos();
                     fLightIntensity[fNumNonAmbLights] = lights->light(i).intensity();

[jvanverth1, 2016/08/30 15:55:42]

Suggestion: allow SkLight to have color and intensity (because it's a clearer
API), but when you create the uniforms, you only need fLightColor. For point
lights, multiply light.color() and light.intensity() together and store that in
fLightColor. That avoids a multiply for every fragment.

[vjiaoblack, 2016/08/30 16:52:18]

Done.

+                } else {
+                    fLightDirOrPos[fNumNonAmbLights] = lights->light(i).dir();
+                    fLightIntensity[fNumNonAmbLights] = 0.0f;
                 }
 
                 fIsPointLight[fNumNonAmbLights] =
@@ -174,11 +174,20 @@ public:
 
             const char* lightDirOrPosUniName[SkShadowShader::kMaxNonAmbientLights] = {nullptr};
             const char* lightColorUniName[SkShadowShader::kMaxNonAmbientLights] = {nullptr};
-            const char* lightIntensityUniName[SkShadowShader::kMaxNonAmbientLights] = {nullptr};
+            const char* lightIntensityUniName[SkShadowShader::kMaxNonAmbientLights] =
+                    {nullptr};

[jvanverth1, 2016/08/30 15:55:42]

Why is this on a different line?

[vjiaoblack, 2016/08/30 16:52:18]

Done.

[vjiaoblack, 2016/08/30 16:52:18]

Acknowledged.

+
+            const char* ambientColorUniName = nullptr;
 
             const char* depthMapWidthUniName[SkShadowShader::kMaxNonAmbientLights] = {nullptr};
             const char* depthMapHeightUniName[SkShadowShader::kMaxNonAmbientLights] = {nullptr};
+            const char* widthUniName = nullptr; // dimensions of povDepth
+            const char* heightUniName = nullptr;
+
+            const char* shBiasUniName = nullptr;
+            const char* minVarianceUniName = nullptr;
 
+            // setting uniforms
             for (int i = 0; i < shadowFP.fNumNonAmbLights; i++) {
                 SkString lightDirOrPosUniNameStr("lightDir");
                 lightDirOrPosUniNameStr.appendf("%d", i);
@@ -221,9 +230,6 @@ public:
                                                    &depthMapHeightUniName[i]);
             }
 
-            const char* shBiasUniName = nullptr;
-            const char* minVarianceUniName = nullptr;
-
             fBiasingConstantUni = uniformHandler->addUniform(kFragment_GrShaderFlag,
                                                              kFloat_GrSLType,
                                                              kDefault_GrSLPrecision,
@@ -233,9 +239,6 @@ public:
                                                          kDefault_GrSLPrecision,
                                                          "minVariance", &minVarianceUniName);
 
-            const char* widthUniName = nullptr;
-            const char* heightUniName = nullptr;
-
             fWidthUni = uniformHandler->addUniform(kFragment_GrShaderFlag,
                                                    kInt_GrSLType,
                                                    kDefault_GrSLPrecision,
@@ -245,18 +248,37 @@ public:
                                                     kDefault_GrSLPrecision,
                                                     "height", &heightUniName);
 
+            fAmbientColorUni = uniformHandler->addUniform(kFragment_GrShaderFlag,
+                                                          kVec3f_GrSLType, kDefault_GrSLPrecision,
+                                                          "AmbientColor", &ambientColorUniName);
+
+            SkString povDepthSampler("_povDepth");
             SkString povDepth("povDepth");
-            this->emitChild(0, nullptr, &povDepth, args);
+            this->emitChild(0, nullptr, &povDepthSampler, args);
+            fragBuilder->codeAppendf("vec4 %s = %s;", povDepth.c_str(), povDepthSampler.c_str());
 
+            SkString diffuseColorSampler("_inDiffuseColor");
             SkString diffuseColor("inDiffuseColor");
-            this->emitChild(1, nullptr, &diffuseColor, args);
+            this->emitChild(1, nullptr, &diffuseColorSampler, args);
+            fragBuilder->codeAppendf("vec4 %s = %s;", diffuseColor.c_str(),
+                                     diffuseColorSampler.c_str());
 
             SkString depthMaps[SkShadowShader::kMaxNonAmbientLights];
 
+            fragBuilder->codeAppendf("vec4 resultDiffuseColor = %s;", diffuseColor.c_str());
+            fragBuilder->codeAppend ("vec3 totalLightColor = vec3(0);");
+
+            // probability that a fragment is lit. For each light, we multiply this by the
+            // light's color to get its contribution to totalLightColor.
+            fragBuilder->codeAppend ("float lightProbability;");
+
+            // coordinates of current fragment in world space
+            fragBuilder->codeAppend ("vec3 worldCor;");
+
             // Multiply by 255 to transform from sampler coordinates to world
             // coordinates (since 1 channel is 0xFF)
-            fragBuilder->codeAppendf("vec3 worldCor = vec3(vMatrixCoord_0_1_Stage0 * "
-                                                     "vec2(%s, %s), %s.b * 255);",
+            fragBuilder->codeAppendf("worldCor = vec3(vMatrixCoord_0_1_Stage0 * "

[jvanverth1, 2016/08/30 15:55:42]

You might want to put a note in the header that the shadow shader is required to
be in Stage0. Otherwise this won't work -- you'll get the wrong texture coords
or compilation will fail because vMatrixCoord_0_1_Stage0 isn't defined.

[vjiaoblack, 2016/08/30 16:52:18]

Done.

Know of any way to do this in a non hacky way?

+                                                "vec2(%s, %s), %s.b * 255);",
                                      widthUniName, heightUniName, povDepth.c_str());
 
             // Applies the offset indexing that goes from our view space into the light's space.
@@ -273,72 +295,63 @@ public:
                 SkString scaleVec("scaleVec");
                 scaleVec.appendf("%d", i);
 
-                SkString scaleOffsetVec("scaleOffsetVec");
-                scaleOffsetVec.appendf("%d", i);
-
                 fragBuilder->codeAppendf("vec2 %s;", offset.c_str());
 
+                // note that we flip the y-coord of the offset and then later add
+                // a value just to the y-coord of povCoord. This is to account for
+                // the shifted origins from switching from raster into GPU.
                 if (shadowFP.fIsPointLight[i]) {
                     fragBuilder->codeAppendf("vec3 fragToLight%d = %s - worldCor;",
                                              i, lightDirOrPosUniName[i]);
                     fragBuilder->codeAppendf("float distsq%d = dot(fragToLight%d, fragToLight%d);"
                                              "fragToLight%d = normalize(fragToLight%d);",
                                              i, i, i, i, i);
-                    fragBuilder->codeAppendf("%s = -vec2(%s.x - worldCor.x, worldCor.y - %s.y)*"
-                                                    "(povDepth.b) / vec2(%s, %s);",
+                    fragBuilder->codeAppendf("%s = vec2(worldCor.x - %s.x, %s.y - worldCor.y) * "
+                                                  "povDepth.b;",
                                              offset.c_str(), lightDirOrPosUniName[i],
-                                             lightDirOrPosUniName[i],
-                                             widthUniName, heightUniName);
+                                             lightDirOrPosUniName[i]);
                 } else {
-                    fragBuilder->codeAppendf("%s = vec2(%s) * povDepth.b * 255 / vec2(%s, %s);",
-                                             offset.c_str(), lightDirOrPosUniName[i],
-                                             widthUniName, heightUniName);
+                    fragBuilder->codeAppendf("%s = vec2(%s) * povDepth.b * vec2(255.0, -255.0);",
+                                             offset.c_str(), lightDirOrPosUniName[i]);
                 }
-                fragBuilder->codeAppendf("vec2 %s = (vec2(%s, %s) / vec2(%s, %s));",
-                                         scaleVec.c_str(),
+                fragBuilder->codeAppendf("vec2 %s = (vec2(%s, %s) *"
+                                                    "vMatrixCoord_0_1_Stage0 +"
+                                                    "vec2(0,%s - %s)"
+                                                    " + "
+                                                    "%s) / vec2(%s, %s);",
+                                         povCoord.c_str(),
                                          widthUniName, heightUniName,
+                                         depthMapHeightUniName[i], heightUniName,
+                                         offset.c_str(),
                                          depthMapWidthUniName[i], depthMapHeightUniName[i]);
 
-                fragBuilder->codeAppendf("vec2 %s = 1 - %s;\n",
-                                         scaleOffsetVec.c_str(),
-                                         scaleVec.c_str());
-
-                fragBuilder->codeAppendf("vec2 %s = (vMatrixCoord_0_1_Stage0 + "
-                                                   "vec2(%s.x, 0 - %s.y)) "
-                                                   " * %s + vec2(0,1) * %s;",
-                                         povCoord.c_str(), offset.c_str(), offset.c_str(),
-                                         scaleVec.c_str(), scaleOffsetVec.c_str());
-
                 fragBuilder->appendTextureLookup(&depthMaps[i], args.fTexSamplers[i],
                                                  povCoord.c_str(),
                                                  kVec2f_GrSLType);
 
             }
 
-            const char* ambientColorUniName = nullptr;
-            fAmbientColorUni = uniformHandler->addUniform(kFragment_GrShaderFlag,
-                                                          kVec3f_GrSLType, kDefault_GrSLPrecision,
-                                                          "AmbientColor", &ambientColorUniName);
-
-            fragBuilder->codeAppendf("vec4 resultDiffuseColor = %s;", diffuseColor.c_str());
-
-            SkString totalLightColor("totalLightColor");
-            fragBuilder->codeAppendf("vec3 %s = vec3(0,0,0);", totalLightColor.c_str());
+            // helper variables for calculating shadowing
 
-            fragBuilder->codeAppendf("float lightProbability;");
+            // variance of depth at this fragment in the context of surrounding area
+            // (area size and weighting dependent on blur size and type)
             fragBuilder->codeAppendf("float variance;");
+
+            // the difference in depth between the user POV and light POV.
             fragBuilder->codeAppendf("float d;");
 
+            // add up light contributions from all lights to totalLightColor
             for (int i = 0; i < numLights; i++) {
                 if (!shadowFP.isPointLight(i)) {
                     fragBuilder->codeAppendf("lightProbability = 1;");
 
-                    // 1/512 is less than half a pixel; imperceptible
-                    fragBuilder->codeAppendf("if (%s.b <= %s.b + 1/512) {",
+                    // 1/512 == .00195... is less than half a pixel; imperceptible
+                    fragBuilder->codeAppendf("if (%s.b <= %s.b + .001953125) {",
                                              povDepth.c_str(), depthMaps[i].c_str());
                     if (blurAlgorithm == SkShadowParams::kVariance_ShadowType) {
-                        fragBuilder->codeAppendf("vec2 moments%d = vec2(%s.b * 255,"
-                                                                       "%s.g * 255 * 256 );",
+                        // We mess with depth and depth^2 in their given scales.

[jvanverth1, 2016/08/30 15:55:42]

Since you reverted the variance change, do you need to revert this too?

[vjiaoblack, 2016/08/30 16:52:18]

I think this is fine. Everything also works as expected.

What variance change do you mean?

+                        // (i.e. between 0 and 1)
+                        fragBuilder->codeAppendf("vec2 moments%d = vec2(%s.b, %s.g);",
                                                  i, depthMaps[i].c_str(), depthMaps[i].c_str());
 
                         // variance biasing lessens light bleeding
@@ -347,7 +360,7 @@ public:
                                                                 "%s);", i, i, i,
                                                  minVarianceUniName);
 
-                        fragBuilder->codeAppendf("d = (%s.b * 255) - moments%d.x;",
+                        fragBuilder->codeAppendf("d = (%s.b) - moments%d.x;",
                                                  povDepth.c_str(), i);
                         fragBuilder->codeAppendf("lightProbability = "
                                                          "(variance / (variance + d * d));");
@@ -358,7 +371,7 @@ public:
                         // choosing between light artifacts or correct shape shadows
                         // linstep
                         fragBuilder->codeAppendf("float %s = clamp((lightProbability - %s) /"
-                                                         "(1 - %s), 0, 1);",
+                                                                  "(1 - %s), 0, 1);",
                                                  clamp.c_str(), shBiasUniName, shBiasUniName);
 
                         fragBuilder->codeAppendf("lightProbability = %s;", clamp.c_str());
@@ -371,28 +384,25 @@ public:
 
                     // VSM: The curved shadows near plane edges are artifacts from blurring
                     fragBuilder->codeAppendf("}");
-                    fragBuilder->codeAppendf("%s += dot(vec3(0,0,1), %s) * %s * "
-                                                   "lightProbability;",
-                                             totalLightColor.c_str(),
+                    fragBuilder->codeAppendf("totalLightColor += %s.z * %s * "
+                                                                "lightProbability;",
                                              lightDirOrPosUniName[i],
                                              lightColorUniName[i]);
                 } else {
                     // fragToLight%d.z is equal to the fragToLight dot the surface normal.
-                    fragBuilder->codeAppendf("%s += max(fragToLight%d.z, 0) * %s /"
-                                                   "(1 + distsq%d / (%s * %s));",
-                                             totalLightColor.c_str(), i,
-                                             lightColorUniName[i], i,
-                                             lightIntensityUniName[i],
-                                             lightIntensityUniName[i]);
-                }
-            }
+                    fragBuilder->codeAppendf("totalLightColor += max(fragToLight%d.z, 0) * "
+                                                                "%s * %s * 2 /"

[jvanverth1, 2016/08/30 15:55:42]

Why multiply by 2?

[vjiaoblack, 2016/08/30 16:52:18]

Done.

+                                                                "(1 + distsq%d);",
+                                             i, lightColorUniName[i],
+                                             lightIntensityUniName[i], i);
 
-            fragBuilder->codeAppendf("%s += %s;", totalLightColor.c_str(), ambientColorUniName);
 
-            fragBuilder->codeAppendf("resultDiffuseColor *= vec4(%s, 1);",
-                                     totalLightColor.c_str());
+                }
+            }
 
-            fragBuilder->codeAppendf("%s = resultDiffuseColor;", args.fOutputColor);
+            fragBuilder->codeAppendf("totalLightColor += %s;", ambientColorUniName);
+            fragBuilder->codeAppendf("%s = resultDiffuseColor * vec4(totalLightColor, 1);",
+                                     args.fOutputColor);
         }
 
         static void GenKey(const GrProcessor& proc, const GrGLSLCaps&,
@@ -450,8 +460,8 @@ public:
 
             SkScalar minVariance = shadowFP.shadowParams().fMinVariance;
             if (minVariance != fMinVariance) {
-                pdman.set1f(fMinVarianceUni, minVariance);
-                fMinVariance = minVariance;
+                pdman.set1f(fMinVarianceUni, minVariance / 65536.0f);

[jvanverth1, 2016/08/30 15:55:42]

Should this be reverted?

[vjiaoblack, 2016/08/30 16:52:18]

What revert are you talking about?

+                fMinVariance = minVariance / 65536.0f;
             }
 
             int width = shadowFP.width();