Optimizations and more documentation of SkShadowShader

src/core/SkShadowShader.cpp

 /*
  * Copyright 2016 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkCanvas.h"
 #include "SkReadBuffer.h"
 #include "SkShadowShader.h"
@@ skipping 100 matching lines @@
              const SkShadowParams& params,
              GrContext* context) {
 
         fAmbientColor = lights->ambientLightColor();
 
         fNumNonAmbLights = 0; // count of non-ambient lights
         for (int i = 0; i < lights->numLights(); ++i) {
             if (fNumNonAmbLights < SkShadowShader::kMaxNonAmbientLights) {
                 fLightColor[fNumNonAmbLights] = lights->light(i).color();
 
-                if (SkLights::Light::kDirectional_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;
-                } else if (SkLights::Light::kPoint_LightType == lights->light(i).type()) {
-                    fLightDirOrPos[fNumNonAmbLights] = lights->light(i).pos();
-                    fLightIntensity[fNumNonAmbLights] = lights->light(i).intensity();
                 }
 
                 fIsPointLight[fNumNonAmbLights] =
                         SkLights::Light::kPoint_LightType == lights->light(i).type();
 
                 SkImage_Base* shadowMap = ((SkImage_Base*)lights->light(i).getShadowMap());
 
                 // gets deleted when the ShadowFP is destroyed, and frees the GrTexture*
                 fTexture[fNumNonAmbLights] = sk_sp<GrTexture>(shadowMap->asTextureRef(context,
                                                            GrTextureParams::ClampNoFilter(),
@@ skipping 30 matching lines @@
             SkASSERT(shadowFP.fNumNonAmbLights <= SkShadowShader::kMaxNonAmbientLights);
 
             // add uniforms
             int32_t numLights = shadowFP.fNumNonAmbLights;
             SkASSERT(numLights <= SkShadowShader::kMaxNonAmbientLights);
 
             int blurAlgorithm = shadowFP.fShadowParams.fType;
 
             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);
                 SkString lightColorUniNameStr("lightColor");
                 lightColorUniNameStr.appendf("%d", i);
                 SkString lightIntensityUniNameStr("lightIntensity");
                 lightIntensityUniNameStr.appendf("%d", i);
 
                 SkString depthMapWidthUniNameStr("dmapWidth");
                 depthMapWidthUniNameStr.appendf("%d", i);
@@ skipping 22 matching lines @@
                                                    kDefault_GrSLPrecision,
                                                    depthMapWidthUniNameStr.c_str(),
                                                    &depthMapWidthUniName[i]);
                 fDepthMapHeightUni[i] = uniformHandler->addUniform(kFragment_GrShaderFlag,
                                                    kInt_GrSLType,
                                                    kDefault_GrSLPrecision,
                                                    depthMapHeightUniNameStr.c_str(),
                                                    &depthMapHeightUniName[i]);
             }
 
-            const char* shBiasUniName = nullptr;
-            const char* minVarianceUniName = nullptr;
-
             fBiasingConstantUni = uniformHandler->addUniform(kFragment_GrShaderFlag,
                                                              kFloat_GrSLType,
                                                              kDefault_GrSLPrecision,
                                                              "shadowBias", &shBiasUniName);
             fMinVarianceUni = uniformHandler->addUniform(kFragment_GrShaderFlag,
                                                          kFloat_GrSLType,
                                                          kDefault_GrSLPrecision,
                                                          "minVariance", &minVarianceUniName);
 
-            const char* widthUniName = nullptr;
-            const char* heightUniName = nullptr;
-
             fWidthUni = uniformHandler->addUniform(kFragment_GrShaderFlag,
                                                    kInt_GrSLType,
                                                    kDefault_GrSLPrecision,
                                                    "width", &widthUniName);
             fHeightUni = uniformHandler->addUniform(kFragment_GrShaderFlag,
                                                     kInt_GrSLType,
                                                     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.
             for (int i = 0; i < shadowFP.fNumNonAmbLights; i++) {
                 SkString povCoord("povCoord");
                 povCoord.appendf("%d", i);
 
                 // vMatrixCoord_0_1_Stage0 is the texture sampler coordinates.

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

This comment should move up to where you first use vMatrixCoord_0_1_Stage0.

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

Done.

                 // povDepth.b * 255 scales it to 0 - 255, bringing it to world space,
                 // and the / vec2(width, height) brings it back to a sampler coordinate
                 SkString offset("offset");
                 offset.appendf("%d", i);
 
                 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);
+            // helper variables for calculating shadowing
 
-            fragBuilder->codeAppendf("vec4 resultDiffuseColor = %s;", diffuseColor.c_str());
+            // 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;");
 
-            SkString totalLightColor("totalLightColor");
-            fragBuilder->codeAppendf("vec3 %s = vec3(0,0,0);", totalLightColor.c_str());
-
-            fragBuilder->codeAppendf("float lightProbability;");
-            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
                         fragBuilder->codeAppendf("variance = max(moments%d.y - "
                                                                 "(moments%d.x * moments%d.x),"
                                                                 "%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));");
 
                         SkString clamp("clamp");
                         clamp.appendf("%d", i);
 
                         // 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());
                     } else {
                         fragBuilder->codeAppendf("if (%s.b >= %s.b) {",
                                                  povDepth.c_str(), depthMaps[i].c_str());
                         fragBuilder->codeAppendf("lightProbability = 1;");
                         fragBuilder->codeAppendf("} else { lightProbability = 0; }");
                     }
 
                     // 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&,
                            GrProcessorKeyBuilder* b) {
             const ShadowFP& shadowFP = proc.cast<ShadowFP>();
             b->add32(shadowFP.fNumNonAmbLights);
             int isPL = 0;
             for (int i = 0; i < SkShadowShader::kMaxNonAmbientLights; i++) {
                 isPL = isPL | ((shadowFP.fIsPointLight[i] ? 1 : 0) << i);
             }
@@ skipping 37 matching lines @@
             }
 
             SkScalar biasingConstant = shadowFP.shadowParams().fBiasingConstant;
             if (biasingConstant != fBiasingConstant) {
                 pdman.set1f(fBiasingConstantUni, biasingConstant);
                 fBiasingConstant = biasingConstant;
             }
 
             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();
             if (width != fWidth) {
                 pdman.set1i(fWidthUni, width);
                 fWidth = width;
             }
             int height = shadowFP.height();
             if (height != fHeight) {
                 pdman.set1i(fHeightUni, height);
@@ skipping 371 matching lines @@
 
 ///////////////////////////////////////////////////////////////////////////////
 
 SK_DEFINE_FLATTENABLE_REGISTRAR_GROUP_START(SkShadowShader)
     SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkShadowShaderImpl)
 SK_DEFINE_FLATTENABLE_REGISTRAR_GROUP_END
 
 ///////////////////////////////////////////////////////////////////////////////
 
 #endif