Refactoring of GPU NormalMap handling out into its own class

src/core/SkLightingShader.cpp

Index: src/core/SkLightingShader.cpp
diff --git a/src/core/SkLightingShader.cpp b/src/core/SkLightingShader.cpp
index 542d0f3e673a43b03e3c6569875f5164d4b02cbe..a476d7e5799068c2f7e63f9b7c3ce628cd3ae1a8 100644
--- a/src/core/SkLightingShader.cpp
+++ b/src/core/SkLightingShader.cpp
@@ -14,6 +14,7 @@
 #include "SkPoint3.h"
 #include "SkReadBuffer.h"
 #include "SkWriteBuffer.h"
+#include "SkNormalSourceImpl.h"

[egdaniel, 2016/06/09 16:56:37]

we place include in alphabetical order, so move this up between SkMathPriv and
SkPoint3

[dvonbeck, 2016/06/09 18:51:29]

Done.

 
 ////////////////////////////////////////////////////////////////////////////
 
@@ -67,6 +68,7 @@ public:
         // Pre-cache so future calls to fNormLocalMatrix.getType() are threadsafe.
         (void)fNormLocalMatrix.getType();
 
+        fNormalSource = NormalMapSource::Make(fNormalMap, fInvNormRotation, &fNormLocalMatrix);
     }
 
     bool isOpaque() const override;
@@ -119,6 +121,8 @@ private:
 
     friend class SkLightingShader;
 
+    sk_sp<NormalSource> fNormalSource;
+
     typedef SkShader INHERITED;
 };
 
@@ -139,19 +143,12 @@ private:
 
 class LightingFP : public GrFragmentProcessor {
 public:
-    LightingFP(GrTexture* diffuse, GrTexture* normal, const SkMatrix& diffMatrix,
-               const SkMatrix& normMatrix, const GrTextureParams& diffParams,
-               const GrTextureParams& normParams, sk_sp<SkLights> lights,
-               const SkVector& invNormRotation)
+    LightingFP(GrTexture* diffuse, const SkMatrix& diffMatrix, const GrTextureParams& diffParams,
+               sk_sp<SkLights> lights, const GrFragmentProcessor* normalFP)
         : fDiffDeviceTransform(kLocal_GrCoordSet, diffMatrix, diffuse, diffParams.filterMode())
-        , fNormDeviceTransform(kLocal_GrCoordSet, normMatrix, normal, normParams.filterMode())
-        , fDiffuseTextureAccess(diffuse, diffParams)
-        , fNormalTextureAccess(normal, normParams)
-        , fInvNormRotation(invNormRotation) {
+        , fDiffuseTextureAccess(diffuse, diffParams) {
         this->addCoordTransform(&fDiffDeviceTransform);
-        this->addCoordTransform(&fNormDeviceTransform);
         this->addTextureAccess(&fDiffuseTextureAccess);
-        this->addTextureAccess(&fNormalTextureAccess);
 
         // fuse all ambient lights into a single one
         fAmbientColor.set(0.0f, 0.0f, 0.0f);
@@ -165,6 +162,7 @@ public:
             }
         }
 
+        this->registerChildProcessor(normalFP);
         this->initClassID<LightingFP>();
     }
 
@@ -174,7 +172,6 @@ public:
             fLightDir.fX = 10000.0f;
             fLightColor.fX = 0.0f;
             fAmbientColor.fX = 0.0f;
-            fInvNormRotation.set(0.0f, 0.0f);
         }
 
         void emitCode(EmitArgs& args) override {
@@ -198,33 +195,16 @@ public:
                                                           kVec3f_GrSLType, kDefault_GrSLPrecision,
                                                           "AmbientColor", &ambientColorUniName);
 
-            const char* xformUniName = nullptr;
-            fXformUni = uniformHandler->addUniform(kFragment_GrShaderFlag,
-                                                   kVec2f_GrSLType, kDefault_GrSLPrecision,
-                                                   "Xform", &xformUniName);
-
             fragBuilder->codeAppend("vec4 diffuseColor = ");
             fragBuilder->appendTextureLookupAndModulate(args.fInputColor, args.fTexSamplers[0],
                                                 args.fCoords[0].c_str(),
                                                 args.fCoords[0].getType());
             fragBuilder->codeAppend(";");
 
-            fragBuilder->codeAppend("vec4 normalColor = ");
-            fragBuilder->appendTextureLookup(args.fTexSamplers[1],
-                                     args.fCoords[1].c_str(),
-                                     args.fCoords[1].getType());
-            fragBuilder->codeAppend(";");
-
-            fragBuilder->codeAppend("vec3 normal = normalColor.rgb - vec3(0.5);");
-
-            fragBuilder->codeAppendf(
-                                 "mat3 m = mat3(%s.x, -%s.y, 0.0, %s.y, %s.x, 0.0, 0.0, 0.0, 1.0);",
-                                 xformUniName, xformUniName, xformUniName, xformUniName);
-
-            // TODO: inverse map the light direction vectors in the vertex shader rather than
-            // transforming all the normals here!
-            fragBuilder->codeAppend("normal = normalize(m*normal);");
+            SkString dstNormalName("dstNormal");
+            this->emitChild(0, nullptr, &dstNormalName, args);
 
+            fragBuilder->codeAppendf("vec3 normal = %s.xyz;", dstNormalName.c_str());
             fragBuilder->codeAppendf("float NdotL = clamp(dot(normal, %s), 0.0, 1.0);",
                                      lightDirUniName);
             // diffuse light
@@ -234,6 +214,7 @@ public:
             fragBuilder->codeAppendf("%s = vec4(result.rgb, diffuseColor.a);", args.fOutputColor);
         }
 
+        // TODO does this need changing now?

[egdaniel, 2016/06/09 16:56:37]

It shouldn't need to. The parent and child processors emit their own keys
independent of each other.

[dvonbeck, 2016/06/09 18:51:28]

Done.

         static void GenKey(const GrProcessor& proc, const GrGLSLCaps&,
                            GrProcessorKeyBuilder* b) {
 //            const LightingFP& lightingFP = proc.cast<LightingFP>();
@@ -262,12 +243,6 @@ public:
                 pdman.set3fv(fAmbientColorUni, 1, &ambientColor.fX);
                 fAmbientColor = ambientColor;
             }
-
-            const SkVector& invNormRotation = lightingFP.invNormRotation();
-            if (invNormRotation != fInvNormRotation) {
-                pdman.set2fv(fXformUni, 1, &invNormRotation.fX);
-                fInvNormRotation = invNormRotation;
-            }
         }
 
     private:
@@ -279,9 +254,6 @@ public:
 
         SkColor3f fAmbientColor;
         GrGLSLProgramDataManager::UniformHandle fAmbientColorUni;
-
-        SkVector fInvNormRotation;
-        GrGLSLProgramDataManager::UniformHandle fXformUni;
     };
 
     void onGetGLSLProcessorKey(const GrGLSLCaps& caps, GrProcessorKeyBuilder* b) const override {
@@ -297,7 +269,6 @@ public:
     const SkVector3& lightDir() const { return fLightDir; }
     const SkColor3f& lightColor() const { return fLightColor; }
     const SkColor3f& ambientColor() const { return fAmbientColor; }
-    const SkVector& invNormRotation() const { return fInvNormRotation; }
 
 private:
     GrGLSLFragmentProcessor* onCreateGLSLInstance() const override { return new LightingGLFP; }
@@ -305,24 +276,17 @@ private:
     bool onIsEqual(const GrFragmentProcessor& proc) const override {
         const LightingFP& lightingFP = proc.cast<LightingFP>();
         return fDiffDeviceTransform == lightingFP.fDiffDeviceTransform &&
-               fNormDeviceTransform == lightingFP.fNormDeviceTransform &&
                fDiffuseTextureAccess == lightingFP.fDiffuseTextureAccess &&
-               fNormalTextureAccess == lightingFP.fNormalTextureAccess &&
                fLightDir == lightingFP.fLightDir &&
                fLightColor == lightingFP.fLightColor &&
-               fAmbientColor == lightingFP.fAmbientColor &&
-               fInvNormRotation == lightingFP.fInvNormRotation;
+               fAmbientColor == lightingFP.fAmbientColor;
     }
 
     GrCoordTransform fDiffDeviceTransform;
-    GrCoordTransform fNormDeviceTransform;
     GrTextureAccess  fDiffuseTextureAccess;
-    GrTextureAccess  fNormalTextureAccess;
     SkVector3        fLightDir;
     SkColor3f        fLightColor;
     SkColor3f        fAmbientColor;
-
-    SkVector         fInvNormRotation;
 };
 
 ////////////////////////////////////////////////////////////////////////////
@@ -350,6 +314,8 @@ static bool make_mat(const SkBitmap& bm,
     return true;
 }
 
+//const GrFragmentProcessor*
+
 const GrFragmentProcessor* SkLightingShaderImpl::asFragmentProcessor(
                                                      GrContext* context,
                                                      const SkMatrix& viewM,
@@ -360,16 +326,12 @@ const GrFragmentProcessor* SkLightingShaderImpl::asFragmentProcessor(
     // TODO: support different sizes
     SkASSERT(fDiffuseMap.width() == fNormalMap.width() &&
              fDiffuseMap.height() == fNormalMap.height());
-    SkMatrix diffM, normM;
+    SkMatrix diffM;
 
     if (!make_mat(fDiffuseMap, this->getLocalMatrix(), localMatrix, &diffM)) {
         return nullptr;
     }
 
-    if (!make_mat(fNormalMap, fNormLocalMatrix, localMatrix, &normM)) {
-        return nullptr;
-    }
-
     bool doBicubic;
     GrTextureParams::FilterMode diffFilterMode = GrSkFilterQualityToGrFilterMode(
                                         SkTMin(filterQuality, kMedium_SkFilterQuality),
@@ -378,13 +340,6 @@ const GrFragmentProcessor* SkLightingShaderImpl::asFragmentProcessor(
                                         &doBicubic);
     SkASSERT(!doBicubic);
 
-    GrTextureParams::FilterMode normFilterMode = GrSkFilterQualityToGrFilterMode(
-                                        SkTMin(filterQuality, kMedium_SkFilterQuality),
-                                        viewM,
-                                        fNormLocalMatrix,
-                                        &doBicubic);
-    SkASSERT(!doBicubic);
-
     // TODO: support other tile modes
     GrTextureParams diffParams(kClamp_TileMode, diffFilterMode);
     SkAutoTUnref<GrTexture> diffuseTexture(GrRefCachedBitmapTexture(context,
@@ -395,18 +350,13 @@ const GrFragmentProcessor* SkLightingShaderImpl::asFragmentProcessor(
         return nullptr;
     }
 
-    GrTextureParams normParams(kClamp_TileMode, normFilterMode);
-    SkAutoTUnref<GrTexture> normalTexture(GrRefCachedBitmapTexture(context,
-                                                                   fNormalMap, normParams,
-                                                                   gammaTreatment));
-    if (!normalTexture) {
-        SkErrorInternals::SetError(kInternalError_SkError, "Couldn't convert bitmap to texture.");
-        return nullptr;
-    }
-
+    // TODO is this correct memory handling?

[egdaniel, 2016/06/09 16:56:37]

should work. When you pass it into LightingFP and you call registerChild it
should ref it there. Thus you want to then remove your ref from this version.

[dvonbeck, 2016/06/09 18:51:28]

The master upstream changed and asFragmentProcessor() now returns an sk_sp<>. I
assume the memory handling is still correct since it will unref when the sk_sp
goes out of scope.

+    SkAutoTUnref<const GrFragmentProcessor> normalFP(
+            fNormalSource->asFragmentProcessor(context, viewM, localMatrix, filterQuality,
+                                               gammaTreatment)
+    );
     SkAutoTUnref<const GrFragmentProcessor> inner (
-        new LightingFP(diffuseTexture, normalTexture, diffM, normM, diffParams, normParams, fLights,
-                       fInvNormRotation));
+        new LightingFP(diffuseTexture, diffM, diffParams, fLights, normalFP));
     return GrFragmentProcessor::MulOutputByInputAlpha(inner);
 }