Refactoring of CPU 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 c80f26884ae1f2b89b04d293f53fc3695db08f77..e45c17f3b1b78050a96a6d1c6644821ced5723b7 100644
--- a/src/core/SkLightingShader.cpp
+++ b/src/core/SkLightingShader.cpp
@@ -88,7 +88,8 @@ public:
         // The context takes ownership of the states. It will call their destructors
         // but will NOT free the memory.
         LightingShaderContext(const SkLightingShaderImpl&, const ContextRec&,
-                              SkBitmapProcState* diffuseState, SkBitmapProcState* normalState);
+                              SkBitmapProcState* diffuseState,
+                              SkLightingShader::NormalSource::Provider*);
         ~LightingShaderContext() override;
 
         void shadeSpan(int x, int y, SkPMColor[], int count) override;
@@ -96,9 +97,9 @@ public:
         uint32_t getFlags() const override { return fFlags; }
 
     private:
-        SkBitmapProcState* fDiffuseState;
-        SkBitmapProcState* fNormalState;
-        uint32_t           fFlags;
+        SkBitmapProcState*                        fDiffuseState;
+        SkLightingShader::NormalSource::Provider* fNormalProvider;
+        uint32_t                                  fFlags;
 
         typedef SkShader::Context INHERITED;
     };
@@ -110,7 +111,6 @@ protected:
     void flatten(SkWriteBuffer&) const override;
     size_t onContextSize(const ContextRec&) const override;
     Context* onCreateContext(const ContextRec&, void*) const override;
-    bool computeNormTotalInverse(const ContextRec& rec, SkMatrix* normTotalInverse) const;
 
 private:
     SkBitmap        fDiffuseMap;
@@ -367,13 +367,11 @@ bool SkLightingShaderImpl::isOpaque() const {
 }
 
 SkLightingShaderImpl::LightingShaderContext::LightingShaderContext(
-                                                                const SkLightingShaderImpl& shader,
-                                                                const ContextRec& rec,
-                                                                SkBitmapProcState* diffuseState,
-                                                                SkBitmapProcState* normalState)
+        const SkLightingShaderImpl& shader, const ContextRec& rec, SkBitmapProcState* diffuseState,
+        SkLightingShader::NormalSource::Provider* normalProvider)
     : INHERITED(shader, rec)
     , fDiffuseState(diffuseState)
-    , fNormalState(normalState) {
+    , fNormalProvider(normalProvider) {
     const SkPixmap& pixmap = fDiffuseState->fPixmap;
     bool isOpaque = pixmap.isOpaque();
 
@@ -390,7 +388,7 @@ SkLightingShaderImpl::LightingShaderContext::~LightingShaderContext() {
     // The bitmap proc states have been created outside of the context on memory that will be freed
     // elsewhere. Call the destructors but leave the freeing of the memory to the caller.
     fDiffuseState->~SkBitmapProcState();
-    fNormalState->~SkBitmapProcState();
+    fNormalProvider->~Provider();
 }
 
 static inline SkPMColor convert(SkColor3f color, U8CPU a) {
@@ -417,32 +415,32 @@ static inline SkPMColor convert(SkColor3f color, U8CPU a) {
 
 // larger is better (fewer times we have to loop), but we shouldn't
 // take up too much stack-space (each one here costs 16 bytes)
+// TODO: these defines are redundant with the ones in SkLightingShader_NormalSource.cpp, will
+// consolidate in a future CL when the source of diffuse colores is factored out
 #define TMP_COUNT     16
+#define BUFFER_MAX    (TMP_COUNT * sizeof(uint32_t))
 
 void SkLightingShaderImpl::LightingShaderContext::shadeSpan(int x, int y,
                                                             SkPMColor result[], int count) {
     const SkLightingShaderImpl& lightShader = static_cast<const SkLightingShaderImpl&>(fShader);
 
-    uint32_t  tmpColor[TMP_COUNT], tmpNormal[TMP_COUNT];
-    SkPMColor tmpColor2[2*TMP_COUNT], tmpNormal2[2*TMP_COUNT];
+    uint32_t  tmpColor[TMP_COUNT];
+    SkPMColor tmpColor2[2*TMP_COUNT];
 
     SkBitmapProcState::MatrixProc   diffMProc = fDiffuseState->getMatrixProc();
     SkBitmapProcState::SampleProc32 diffSProc = fDiffuseState->getSampleProc32();
 
-    SkBitmapProcState::MatrixProc   normalMProc = fNormalState->getMatrixProc();
-    SkBitmapProcState::SampleProc32 normalSProc = fNormalState->getSampleProc32();
-
-    int diffMax = fDiffuseState->maxCountForBufferSize(sizeof(tmpColor[0]) * TMP_COUNT);
-    int normMax = fNormalState->maxCountForBufferSize(sizeof(tmpNormal[0]) * TMP_COUNT);
-    int max = SkTMin(diffMax, normMax);
+    int diffMax = fDiffuseState->maxCountForBufferSize(BUFFER_MAX);
+    int normMax = fNormalProvider->maxCountForFillScanLine(TMP_COUNT);
+    size_t max = SkTMin(diffMax, normMax);
 
     SkASSERT(fDiffuseState->fPixmap.addr());
-    SkASSERT(fNormalState->fPixmap.addr());
 
-    SkPoint3 norm, xformedNorm;
+    SkASSERT(max <= BUFFER_MAX);
+    SkPoint3 normals[BUFFER_MAX];
 
     do {
-        int n = count;
+        size_t n = count;

[egdaniel, 2016/06/16 13:48:10]

why the change to size_t here? Quick scan seems to be using it as int it most
cases like in normalProvider->fillScanLine(). This may cause a compiler to give
a warning about loss of data going from size_t to int.

[dvonbeck, 2016/06/16 22:05:40]

I was avoiding a compiler error "comparison between unsigned and signed", since
BUFFER_MAX was unsigned. I circumvented it some other way, so it is back to
being an int.

         if (n > max) {
             n = max;
         }
@@ -450,21 +448,9 @@ void SkLightingShaderImpl::LightingShaderContext::shadeSpan(int x, int y,
         diffMProc(*fDiffuseState, tmpColor, n, x, y);
         diffSProc(*fDiffuseState, tmpColor, n, tmpColor2);
 
-        normalMProc(*fNormalState, tmpNormal, n, x, y);
-        normalSProc(*fNormalState, tmpNormal, n, tmpNormal2);
-
-        for (int i = 0; i < n; ++i) {
-            SkASSERT(0xFF == SkColorGetA(tmpNormal2[i]));  // opaque -> unpremul
-            norm.set(SkIntToScalar(SkGetPackedR32(tmpNormal2[i]))-127.0f,
-                     SkIntToScalar(SkGetPackedG32(tmpNormal2[i]))-127.0f,
-                     SkIntToScalar(SkGetPackedB32(tmpNormal2[i]))-127.0f);
-            norm.normalize();
+        fNormalProvider->fillScanLine(x, y, normals, n);
 
-            xformedNorm.fX = lightShader.fInvNormRotation.fX * norm.fX +
-                             lightShader.fInvNormRotation.fY * norm.fY;
-            xformedNorm.fY = -lightShader.fInvNormRotation.fY * norm.fX +
-                             lightShader.fInvNormRotation.fX * norm.fY;
-            xformedNorm.fZ = norm.fZ;
+        for (size_t i = 0; i < n; ++i) {
 
             SkColor diffColor = SkUnPreMultiply::PMColorToColor(tmpColor2[i]);
 
@@ -476,7 +462,7 @@ void SkLightingShaderImpl::LightingShaderContext::shadeSpan(int x, int y,
                 if (SkLights::Light::kAmbient_LightType == light.type()) {
                     accum += light.color().makeScale(255.0f);
                 } else {
-                    SkScalar NdotL = xformedNorm.dot(light.dir());
+                    SkScalar NdotL = normals[i].dot(light.dir());
                     if (NdotL < 0.0f) {
                         NdotL = 0.0f;
                     }
@@ -599,21 +585,10 @@ void SkLightingShaderImpl::flatten(SkWriteBuffer& buf) const {
     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);
-        m = &total;
-    }
-    return m->invert(normTotalInverse);
-}
-
 size_t SkLightingShaderImpl::onContextSize(const ContextRec&) const {
-    return 2 * sizeof(SkBitmapProcState) + sizeof(LightingShaderContext);
+    return sizeof(LightingShaderContext) +
+           sizeof(SkBitmapProcState)     +
+           fNormalSource->providerSize();
 }
 
 SkShader::Context* SkLightingShaderImpl::onCreateContext(const ContextRec& rec,
@@ -623,11 +598,6 @@ SkShader::Context* SkLightingShaderImpl::onCreateContext(const ContextRec& rec,
     // computeTotalInverse was called in SkShader::createContext so we know it will succeed
     SkAssertResult(this->computeTotalInverse(rec, &diffTotalInv));
 
-    SkMatrix normTotalInv;
-    if (!this->computeNormTotalInverse(rec, &normTotalInv)) {
-        return nullptr;
-    }
-
     void* diffuseStateStorage = (char*)storage + sizeof(LightingShaderContext);
     SkBitmapProcState* diffuseState = new (diffuseStateStorage) SkBitmapProcState(fDiffuseMap,
                                               SkShader::kClamp_TileMode, SkShader::kClamp_TileMode,
@@ -637,21 +607,18 @@ SkShader::Context* SkLightingShaderImpl::onCreateContext(const ContextRec& rec,
         diffuseState->~SkBitmapProcState();
         return nullptr;
     }
+    void* normalProviderStorage = (char*)storage +
+                                  sizeof(LightingShaderContext) +
+                                  sizeof(SkBitmapProcState);
 
-    void* normalStateStorage = (char*)storage +
-                                sizeof(LightingShaderContext) +
-                                sizeof(SkBitmapProcState);
-    SkBitmapProcState* normalState = new (normalStateStorage) SkBitmapProcState(fNormalMap,
-                                            SkShader::kClamp_TileMode, SkShader::kClamp_TileMode,
-                                                                    SkMipMap::DeduceTreatment(rec));
-    SkASSERT(normalState);
-    if (!normalState->setup(normTotalInv, *rec.fPaint)) {
+    SkLightingShader::NormalSource::Provider* normalProvider =
+            fNormalSource->asProvider(rec, normalProviderStorage);
+    if (!normalProvider) {
         diffuseState->~SkBitmapProcState();
-        normalState->~SkBitmapProcState();
         return nullptr;
     }
 
-    return new (storage) LightingShaderContext(*this, rec, diffuseState, normalState);
+    return new (storage) LightingShaderContext(*this, rec, diffuseState, normalProvider);
 }
 
 ///////////////////////////////////////////////////////////////////////////////