Refactoring of GPU NormalMap handling out into its own class

src/core/SkLightingShader.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkBitmapProcState.h"
 #include "SkColor.h"
 #include "SkEmptyShader.h"
@@ skipping 34 matching lines @@
         @param diffuse    the diffuse bitmap
         @param normal     the normal map
         @param lights     the lights applied to the normal map
         @param invNormRotation rotation applied to the normal map's normals
         @param diffLocalM the local matrix for the diffuse coordinates
         @param normLocalM the local matrix for the normal coordinates
     */
     SkLightingShaderImpl(const SkBitmap& diffuse, const SkBitmap& normal,
                          const sk_sp<SkLights> lights,
                          const SkVector& invNormRotation,
-                         const SkMatrix* diffLocalM, const SkMatrix* normLocalM)
+                         const SkMatrix* diffLocalM, const SkMatrix* normLocalM,
+                         sk_sp<const SkLightingShader::NormalSource> normalSource)

[egdaniel, 2016/06/10 14:23:28]

putting a const inside of an sk_sp is generally a bad thing to do.

[dvonbeck, 2016/06/10 15:22:29]

Done.

         : INHERITED(diffLocalM)
         , fDiffuseMap(diffuse)
         , fNormalMap(normal)
         , fLights(std::move(lights))
         , fInvNormRotation(invNormRotation) {
 
         if (normLocalM) {
             fNormLocalMatrix = *normLocalM;
         } else {
             fNormLocalMatrix.reset();
         }
         // Pre-cache so future calls to fNormLocalMatrix.getType() are threadsafe.
         (void)fNormLocalMatrix.getType();
 
+        fNormalSource = normalSource;

[egdaniel, 2016/06/10 14:23:28]

use std::move(

[dvonbeck, 2016/06/10 15:22:29]

Done.

     }
 
     bool isOpaque() const override;
 
 #if SK_SUPPORT_GPU
     sk_sp<GrFragmentProcessor> asFragmentProcessor(GrContext*,
                                                    const SkMatrix& viewM,
                                                    const SkMatrix* localMatrix,
                                                    SkFilterQuality,
                                                    SkSourceGammaTreatment) const override;
@@ skipping 30 matching lines @@
 
 private:
     SkBitmap        fDiffuseMap;
     SkBitmap        fNormalMap;
 
     sk_sp<SkLights> fLights;
 
     SkMatrix        fNormLocalMatrix;
     SkVector        fInvNormRotation;
 
+    sk_sp<const SkLightingShader::NormalSource> fNormalSource;

[egdaniel, 2016/06/10 14:23:27]

again const sk_sp is bad.

[dvonbeck, 2016/06/10 15:22:29]

Done.

+
     friend class SkLightingShader;
 
     typedef SkShader INHERITED;
 };
 
 ////////////////////////////////////////////////////////////////////////////
 
 #if SK_SUPPORT_GPU
 
 #include "GrCoordTransform.h"
 #include "GrFragmentProcessor.h"
 #include "GrInvariantOutput.h"
 #include "GrTextureAccess.h"
 #include "glsl/GrGLSLFragmentProcessor.h"
 #include "glsl/GrGLSLFragmentShaderBuilder.h"
 #include "glsl/GrGLSLProgramDataManager.h"
 #include "glsl/GrGLSLUniformHandler.h"
 #include "SkGr.h"
 #include "SkGrPriv.h"
 
 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, sk_sp<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);
         for (int i = 0; i < lights->numLights(); ++i) {
             if (SkLights::Light::kAmbient_LightType == lights->light(i).type()) {
                 fAmbientColor += lights->light(i).color();
             } else {
                 // TODO: handle more than one of these
                 fLightColor = lights->light(i).color();
                 fLightDir = lights->light(i).dir();
             }
         }
 
+        this->registerChildProcessor(normalFP);

[egdaniel, 2016/06/10 14:23:27]

use std::move(noramlFP) here

[dvonbeck, 2016/06/10 15:22:29]

Done.

         this->initClassID<LightingFP>();
     }
 
     class LightingGLFP : public GrGLSLFragmentProcessor {
     public:
         LightingGLFP() {
             fLightDir.fX = 10000.0f;
             fLightColor.fX = 0.0f;
             fAmbientColor.fX = 0.0f;
-            fInvNormRotation.set(0.0f, 0.0f);
         }
 
         void emitCode(EmitArgs& args) override {
 
             GrGLSLFragmentBuilder* fragBuilder = args.fFragBuilder;
             GrGLSLUniformHandler* uniformHandler = args.fUniformHandler;
 
             // add uniforms
             const char* lightDirUniName = nullptr;
             fLightDirUni = uniformHandler->addUniform(kFragment_GrShaderFlag,
                                                       kVec3f_GrSLType, kDefault_GrSLPrecision,
                                                       "LightDir", &lightDirUniName);
 
             const char* lightColorUniName = nullptr;
             fLightColorUni = uniformHandler->addUniform(kFragment_GrShaderFlag,
                                                         kVec3f_GrSLType, kDefault_GrSLPrecision,
                                                         "LightColor", &lightColorUniName);
 
             const char* ambientColorUniName = nullptr;
             fAmbientColorUni = uniformHandler->addUniform(kFragment_GrShaderFlag,
                                                           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(";");
+            SkString dstNormalName("dstNormal");
+            this->emitChild(0, nullptr, &dstNormalName, args);
 
-            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);");
-
+            fragBuilder->codeAppendf("vec3 normal = %s.xyz;", dstNormalName.c_str());
             fragBuilder->codeAppendf("float NdotL = clamp(dot(normal, %s), 0.0, 1.0);",
                                      lightDirUniName);
             // diffuse light
             fragBuilder->codeAppendf("vec3 result = %s*diffuseColor.rgb*NdotL;", lightColorUniName);
             // ambient light
             fragBuilder->codeAppendf("result += %s;", ambientColorUniName);
             fragBuilder->codeAppendf("%s = vec4(result.rgb, diffuseColor.a);", args.fOutputColor);
         }
 
         static void GenKey(const GrProcessor& proc, const GrGLSLCaps&,
@@ skipping 17 matching lines @@
             if (lightColor != fLightColor) {
                 pdman.set3fv(fLightColorUni, 1, &lightColor.fX);
                 fLightColor = lightColor;
             }
 
             const SkColor3f& ambientColor = lightingFP.ambientColor();
             if (ambientColor != fAmbientColor) {
                 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:
         SkVector3 fLightDir;
         GrGLSLProgramDataManager::UniformHandle fLightDirUni;
 
         SkColor3f fLightColor;
         GrGLSLProgramDataManager::UniformHandle fLightColorUni;
 
         SkColor3f fAmbientColor;
         GrGLSLProgramDataManager::UniformHandle fAmbientColorUni;
-
-        SkVector fInvNormRotation;
-        GrGLSLProgramDataManager::UniformHandle fXformUni;
     };
 
     void onGetGLSLProcessorKey(const GrGLSLCaps& caps, GrProcessorKeyBuilder* b) const override {
         LightingGLFP::GenKey(*this, caps, b);
     }
 
     const char* name() const override { return "LightingFP"; }
 
     void onComputeInvariantOutput(GrInvariantOutput* inout) const override {
         inout->mulByUnknownFourComponents();
     }
 
     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; }
 
     bool onIsEqual(const GrFragmentProcessor& proc) const override {
+        // TODO add comparison of NormalFPs

[egdaniel, 2016/06/10 14:23:27]

The base class of isEquals takes care of calling child processor onIsEqual so
this guy really just needs to check his own stuff.

[dvonbeck, 2016/06/10 15:22:29]

Done.

         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;
 };
 
 ////////////////////////////////////////////////////////////////////////////
 
 static bool make_mat(const SkBitmap& bm,
                      const SkMatrix& localMatrix1,
                      const SkMatrix* localMatrix2,
                      SkMatrix* result) {
 
     result->setIDiv(bm.width(), bm.height());
@@ skipping 17 matching lines @@
 sk_sp<GrFragmentProcessor> SkLightingShaderImpl::asFragmentProcessor(
                                                      GrContext* context,
                                                      const SkMatrix& viewM,
                                                      const SkMatrix* localMatrix,
                                                      SkFilterQuality filterQuality,
                                                      SkSourceGammaTreatment gammaTreatment) const {
     // we assume diffuse and normal maps have same width and height
     // 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),
                                         viewM,
                                         this->getLocalMatrix(),
                                         &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,
                                                                     fDiffuseMap, diffParams,
                                                                     gammaTreatment));
     if (!diffuseTexture) {
         SkErrorInternals::SetError(kInternalError_SkError, "Couldn't convert bitmap to texture.");
         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? FPs were changed to use sk_sp so it is different from

[egdaniel, 2016/06/10 14:23:28]

This should still be fine still

[dvonbeck, 2016/06/10 15:22:29]

Done.

+     * last patch
+     */
+    sk_sp<GrFragmentProcessor> normalFP(
+            fNormalSource->asFragmentProcessor(context, viewM, localMatrix, filterQuality,
+                                               gammaTreatment));
+    sk_sp<GrFragmentProcessor> inner (
+            new LightingFP(diffuseTexture, diffM, diffParams, fLights, normalFP));

[egdaniel, 2016/06/10 14:23:27]

I believe you want to use std::move here around normalFP

[dvonbeck, 2016/06/10 15:22:29]

Done.

 
-    sk_sp<GrFragmentProcessor> inner (
-        new LightingFP(diffuseTexture, normalTexture, diffM, normM, diffParams, normParams, fLights,
-                       fInvNormRotation));
     return GrFragmentProcessor::MulOutputByInputAlpha(std::move(inner));
 }
 
 #endif
 
 ////////////////////////////////////////////////////////////////////////////
 
 bool SkLightingShaderImpl::isOpaque() const {
     return fDiffuseMap.isOpaque();
 }
@@ skipping 188 matching lines @@
         }
     }
 
     sk_sp<SkLights> lights(builder.finish());
 
     SkVector invNormRotation = {1,0};
     if (!buf.isVersionLT(SkReadBuffer::kLightingShaderWritesInvNormRotation)) {
         invNormRotation = buf.readPoint();
     }
 
+    sk_sp<SkLightingShader::NormalSource> normalSource(
+            static_cast<SkLightingShader::NormalSource*>(buf.readFlattenable(kNormalSource_Type)));
+
     return sk_make_sp<SkLightingShaderImpl>(diffuse, normal, std::move(lights), invNormRotation,
-                                            &diffLocalM, &normLocalM);
+                                            &diffLocalM, &normLocalM, normalSource);

[egdaniel, 2016/06/10 14:23:27]

wrap normalSouce in std::move

[dvonbeck, 2016/06/10 15:22:29]

Done.

 }
 
 void SkLightingShaderImpl::flatten(SkWriteBuffer& buf) const {
     this->INHERITED::flatten(buf);
 
     bool hasNormLocalM = !fNormLocalMatrix.isIdentity();
     buf.writeBool(hasNormLocalM);
     if (hasNormLocalM) {
         buf.writeMatrix(fNormLocalMatrix);
     }
 
     buf.writeBitmap(fDiffuseMap);
     buf.writeBitmap(fNormalMap);
 
     buf.writeInt(fLights->numLights());
     for (int l = 0; l < fLights->numLights(); ++l) {
         const SkLights::Light& light = fLights->light(l);
 
         bool isAmbient = SkLights::Light::kAmbient_LightType == light.type();
 
         buf.writeBool(isAmbient);
         buf.writeScalarArray(&light.color().fX, 3);
         if (!isAmbient) {
             buf.writeScalarArray(&light.dir().fX, 3);
         }
     }
     buf.writePoint(fInvNormRotation);
+
+    buf.writeFlattenable(fNormalSource.get());
 }
 
 bool SkLightingShaderImpl::computeNormTotalInverse(const ContextRec& rec,
                                                    SkMatrix* normTotalInverse) const {
     SkMatrix total;
     total.setConcat(*rec.fMatrix, fNormLocalMatrix);
 
     const SkMatrix* m = &total;
     if (rec.fLocalMatrix) {
         total.setConcat(*m, *rec.fLocalMatrix);
@@ skipping 57 matching lines @@
                                        const SkVector& invNormRotation,
                                        const SkMatrix* diffLocalM, const SkMatrix* normLocalM) {
     if (diffuse.isNull() || bitmap_is_too_big(diffuse) ||
         normal.isNull() || bitmap_is_too_big(normal) ||
         diffuse.width() != normal.width() ||
         diffuse.height() != normal.height()) {
         return nullptr;
     }
 
     SkASSERT(SkScalarNearlyEqual(invNormRotation.lengthSqd(), SK_Scalar1));
-
+    sk_sp<SkLightingShader::NormalSource> normalSource =
+            SkLightingShader::NormalMapSource::Make(normal, invNormRotation, normLocalM);
     return sk_make_sp<SkLightingShaderImpl>(diffuse, normal, std::move(lights),
-                                            invNormRotation, diffLocalM, normLocalM);
+                                            invNormRotation, diffLocalM, normLocalM, normalSource);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 
 SK_DEFINE_FLATTENABLE_REGISTRAR_GROUP_START(SkLightingShader)
     SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkLightingShaderImpl)
 SK_DEFINE_FLATTENABLE_REGISTRAR_GROUP_END
 
 ///////////////////////////////////////////////////////////////////////////////