Refactoring of GPU NormalMap handling out into its own class

src/core/SkNormalSourceImpl.cpp

Index: src/core/SkNormalSourceImpl.cpp
diff --git a/src/core/SkNormalSourceImpl.cpp b/src/core/SkNormalSourceImpl.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..3cfbdf555e7420b101ea4b0b6b855652ee0b60ed
--- /dev/null
+++ b/src/core/SkNormalSourceImpl.cpp
@@ -0,0 +1,226 @@
+/*
+ * 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 "SkNormalSourceImpl.h"
+#include "SkShader.h"
+
+class NormalMapFP : public GrFragmentProcessor {
+public:
+    NormalMapFP(GrTexture* normal, const SkMatrix& normMatrix, const GrTextureParams& normParams,
+                const SkVector& invNormRotation)
+            : fNormDeviceTransform(kLocal_GrCoordSet, normMatrix, normal, normParams.filterMode())

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

Skia's style has these initiliazers only tabbed in once

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

Done.

+            , fNormalTextureAccess(normal, normParams)
+            , fInvNormRotation(invNormRotation) {
+        this->addCoordTransform(&fNormDeviceTransform);
+        this->addTextureAccess(&fNormalTextureAccess);
+
+        this->initClassID<NormalMapFP>();
+    }
+
+    class NormalMapGLFP : public GrGLSLFragmentProcessor {

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

Usually we put the GL in front of the class name, and actaully using GL is an
old style and should be GLSL at this point. So lets call this GLSLNormalMapFP

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

Done.

+    public:
+        NormalMapGLFP() {
+            fInvNormRotation.set(0.0f, 0.0f);
+        }
+
+        void emitCode(EmitArgs& args) override {
+
+            GrGLSLFragmentBuilder* fragBuilder = args.fFragBuilder;
+            GrGLSLUniformHandler* uniformHandler = args.fUniformHandler;
+
+            // add uniform
+            const char* xformUniName = nullptr;
+            fXformUni = uniformHandler->addUniform(kFragment_GrShaderFlag,
+                                                   kVec2f_GrSLType, kDefault_GrSLPrecision,
+                                                   "Xform", &xformUniName);
+
+            fragBuilder->codeAppend("vec4 normalColor = ");
+            fragBuilder->appendTextureLookup(args.fTexSamplers[0],
+                                             args.fCoords[0].c_str(),
+                                             args.fCoords[0].getType());
+            fragBuilder->codeAppend(";");
+
+            fragBuilder->codeAppend("vec3 normal = normalColor.rgb - vec3(0.5);");
+
+            // TODO: inverse map the light direction vectors in the vertex shader rather than
+            // transforming all the normals here!
+            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);
+
+            fragBuilder->codeAppend("normal = normalize(m*normal);");
+            fragBuilder->codeAppendf("%s = vec4(normal, 0);", args.fOutputColor);
+        }
+
+        // TODO does this need changing now?
+        static void GenKey(const GrProcessor& proc, const GrGLSLCaps&,
+                           GrProcessorKeyBuilder* b) {
+//            const LightingFP& lightingFP = proc.cast<LightingFP>();
+            // only one shader generated currently
+            b->add32(0x0);

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

This should be fine outputing zero. Clean up the comment since it still refers
to Lighting

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

Done.

+        }
+
+    protected:
+        void onSetData(const GrGLSLProgramDataManager& pdman, const GrProcessor& proc) override {
+            const NormalMapFP& normalMapFP = proc.cast<NormalMapFP>();
+
+            const SkVector& invNormRotation = normalMapFP.invNormRotation();
+            if (invNormRotation != fInvNormRotation) {
+                pdman.set2fv(fXformUni, 1, &invNormRotation.fX);
+                fInvNormRotation = invNormRotation;
+            }
+        }
+
+    private:
+        SkVector fInvNormRotation;
+        GrGLSLProgramDataManager::UniformHandle fXformUni;
+    };
+
+    void onGetGLSLProcessorKey(const GrGLSLCaps& caps, GrProcessorKeyBuilder* b) const override {
+        NormalMapGLFP::GenKey(*this, caps, b);
+    }
+
+    const char* name() const override { return "NormalMapFP"; }
+
+    // TODO what is this? keep? delete?
+    void onComputeInvariantOutput(GrInvariantOutput* inout) const override {
+        inout->mulByUnknownFourComponents();

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

Since this guy doesn't actually use his input, and we don't know what the
normals he will produce are, I would do inout->setUnknownFourComponents()

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

The object doesn't have a setUnknownFourComponents() method.

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

Sorry that was the function on the inner struct. This should be a setToUnknown
or something similar to that.

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

Done.

+    }
+
+    const SkVector& invNormRotation() const { return fInvNormRotation; }
+
+private:
+    GrGLSLFragmentProcessor* onCreateGLSLInstance() const override { return new NormalMapGLFP; }
+
+    bool onIsEqual(const GrFragmentProcessor& proc) const override {
+        const NormalMapFP& normalMapFP = proc.cast<NormalMapFP>();
+        return fNormDeviceTransform == normalMapFP.fNormDeviceTransform &&
+               fNormalTextureAccess == normalMapFP.fNormalTextureAccess &&
+               fInvNormRotation == normalMapFP.fInvNormRotation;
+    }
+
+    GrCoordTransform fNormDeviceTransform;
+    GrTextureAccess  fNormalTextureAccess;
+
+    SkVector         fInvNormRotation;
+};
+
+// TODO same code at SkLightingShader.cpp. Refactor to common source!
+static bool make_mat(const SkBitmap& bm,
+                     const SkMatrix& localMatrix1,
+                     const SkMatrix* localMatrix2,
+                     SkMatrix* result) {
+
+    result->setIDiv(bm.width(), bm.height());
+
+    SkMatrix lmInverse;
+    if (!localMatrix1.invert(&lmInverse)) {
+        return false;
+    }
+    if (localMatrix2) {
+        SkMatrix inv;
+        if (!localMatrix2->invert(&inv)) {
+            return false;
+        }
+        lmInverse.postConcat(inv);
+    }
+    result->preConcat(lmInverse);
+
+    return true;
+}
+
+class NormalMapSourceImpl : public NormalSource {
+public:
+    NormalMapSourceImpl(const SkBitmap &normal, const SkVector &invNormRotation,
+                     const SkMatrix *normLocalM)

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

alight with other parameters

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

Done.

+            : fNormalMap(normal)

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

again remove one tab

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

Done.

+            , fInvNormRotation(invNormRotation) {
+
+        if (normLocalM) {
+            fNormLocalMatrix = *normLocalM;
+        } else {
+            fNormLocalMatrix.reset();
+        }
+        // Pre-cache so future calls to fNormLocalMatrix.getType() are threadsafe.
+        (void)fNormLocalMatrix.getType();
+    }
+
+    const GrFragmentProcessor* asFragmentProcessor(
+                                             GrContext* context,
+                                             const SkMatrix& viewM,
+                                             const SkMatrix* localMatrix,
+                                             SkFilterQuality filterQuality,
+                                             SkSourceGammaTreatment gammaTreatment) const override;
+
+private:
+    SkBitmap        fNormalMap;
+    SkMatrix        fNormLocalMatrix;
+    SkVector        fInvNormRotation;
+};
+
+const GrFragmentProcessor* NormalMapSourceImpl::asFragmentProcessor(
+                                                     GrContext *context,
+                                                     const SkMatrix &viewM,
+                                                     const SkMatrix *localMatrix,
+                                                     SkFilterQuality filterQuality,
+                                                     SkSourceGammaTreatment gammaTreatment) const {
+
+    // TODO Here, the old code was checking that diffuse map and normal map are same size, that

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

We should really have a way so that the diffuse bitmap isn't required and
instead is treated more as a color input to the lighting shader

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

Acknowledged.

+    // TODO needs to happen somewhere again!
+
+    SkMatrix normM;
+    if (!make_mat(fNormalMap, fNormLocalMatrix, localMatrix, &normM)) {
+        return nullptr;
+    }
+
+    bool doBicubic;
+    GrTextureParams::FilterMode normFilterMode = GrSkFilterQualityToGrFilterMode(
+            SkTMin(filterQuality, kMedium_SkFilterQuality),
+            viewM,
+            fNormLocalMatrix,
+            &doBicubic);
+    SkASSERT(!doBicubic);
+
+    // TODO: support other tile modes
+    GrTextureParams normParams(SkShader::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:38]

yes this is fine here. It is the callers responsibility to handle unreffing of
the object

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

Done.

+    return new NormalMapFP(normalTexture, normM, normParams, fInvNormRotation);
+}
+
+// TODO This function is also in SkLightingShader. Refactor to common source?
+// TODO what does it mean for the funciton to be static
+static bool bitmap_is_too_big(const SkBitmap& bm) {
+    // SkBitmapProcShader stores bitmap coordinates in a 16bit buffer, as it
+    // communicates between its matrix-proc and its sampler-proc. Until we can
+    // widen that, we have to reject bitmaps that are larger.
+    //
+    static const int kMaxSize = 65535;
+
+    return bm.width() > kMaxSize || bm.height() > kMaxSize;
+}
+
+sk_sp<NormalSource> NormalMapSource::Make(const SkBitmap &normal, const SkVector &invNormRotation,
+                                          const SkMatrix *normLocalM) {
+
+    // TODO not checking normal and diffuse maps to be same size
+    if (normal.isNull() || bitmap_is_too_big(normal)) {
+        return nullptr;
+    }
+
+    SkASSERT(SkScalarNearlyEqual(invNormRotation.lengthSqd(), SK_Scalar1));
+
+    return sk_make_sp<NormalMapSourceImpl>(normal, invNormRotation, normLocalM);
+}