SkLS accepts nullptr for pointer args, handles alpha accurately, has new GM

src/core/SkLightingShader.cpp

Index: src/core/SkLightingShader.cpp
diff --git a/src/core/SkLightingShader.cpp b/src/core/SkLightingShader.cpp
index 02f14b34ddc9215c72070aeedc769a8f36418cd2..52d9e987fa8ded6f5ded09e8de97fdb25ffaaf17 100644
--- a/src/core/SkLightingShader.cpp
+++ b/src/core/SkLightingShader.cpp
@@ -21,15 +21,11 @@
 
 /*
    SkLightingShader TODOs:
-        support other than clamp mode
-        allow 'diffuse' & 'normal' to be of different dimensions?
         support different light types
         support multiple lights
-        enforce normal map is 4 channel
-        use SkImages instead if SkBitmaps
+        fix non-opaque diffuse textures
 
     To Test:
-        non-opaque diffuse textures
         A8 diffuse textures
         down & upsampled draws
 */
@@ -81,6 +77,7 @@ public:
     private:
         SkShader::Context*        fDiffuseContext;
         SkNormalSource::Provider* fNormalProvider;
+        SkColor                   fPaintColor;
         uint32_t                  fFlags;
 
         void* fHeapAllocated;
@@ -270,18 +267,21 @@ sk_sp<GrFragmentProcessor> SkLightingShaderImpl::asFragmentProcessor(
         return nullptr;
     }
 
-    sk_sp<GrFragmentProcessor> fpPipeline[] = {
-            fDiffuseShader->asFragmentProcessor(context, viewM, localMatrix, filterQuality,
-                                                gammaTreatment),
-            sk_make_sp<LightingFP>(std::move(normalFP), fLights)
-    };
-    if(!fpPipeline[0]) {
-        return nullptr;
-    }
-
-    sk_sp<GrFragmentProcessor> inner(GrFragmentProcessor::RunInSeries(fpPipeline, 2));
+    if (fDiffuseShader) {
+        sk_sp<GrFragmentProcessor> fpPipeline[] = {

[robertphillips, 2016/07/11 14:02:04]

this seems over tabbed

[dvonbeck, 2016/07/11 19:37:29]

Done.

+                fDiffuseShader->asFragmentProcessor(context, viewM, localMatrix, filterQuality,
+                                                    gammaTreatment),
+                sk_make_sp<LightingFP>(std::move(normalFP), fLights)
+        };
+        if(!fpPipeline[0]) {
+            return nullptr;
+        }
 
-    return GrFragmentProcessor::MulOutputByInputAlpha(std::move(inner));
+        sk_sp<GrFragmentProcessor> innerLightFP = GrFragmentProcessor::RunInSeries(fpPipeline, 2);
+        return GrFragmentProcessor::MulOutputByInputAlpha(std::move(innerLightFP));
+    } else {
+        return sk_make_sp<LightingFP>(std::move(normalFP), fLights);

[egdaniel, 2016/07/11 18:00:59]

I think we still need to do the premul of alpha after this right? As in do the
same MulOutputByInputAlpha as above

[dvonbeck, 2016/07/11 19:37:29]

The alpha of the input color gets passed down to the output color (see
SkLightingShader.cpp:182). In the diffuseShader==nullptr case, this input color
is the paint's color, so I believe this to be correct. In any case, the GPU side
has never handled alpha correctly for some reason, so this need some looking
into either way. I would leave it for later.

+    }
 }
 
 #endif
@@ -289,7 +289,7 @@ sk_sp<GrFragmentProcessor> SkLightingShaderImpl::asFragmentProcessor(
 ////////////////////////////////////////////////////////////////////////////
 
 bool SkLightingShaderImpl::isOpaque() const {

[robertphillips, 2016/07/11 14:02:04]

Hmmm, what happens if the paint's color is transparent ?

[dvonbeck, 2016/07/11 19:37:29]

I don't know :S, we don't know the paint at shader initalization time. I changed
the result to false since I checked and this is used to verify overdraw. Please
let me know if you think this is wrong because I'm not very sure about this.

[robertphillips, 2016/07/11 20:51:42]

We can't pull the alpha out of 'fPaintColor' ?

[dvonbeck, 2016/07/12 21:01:44]

No, fPaintColor is a field of LightingShaderContext. LightingShaderContext gets
created at draw time so it has access to the paint. SkLightingShaderImpl does
not.

-    return fDiffuseShader->isOpaque();
+    return (fDiffuseShader ? fDiffuseShader->isOpaque() : true);
 }
 
 SkLightingShaderImpl::LightingShaderContext::LightingShaderContext(
@@ -308,13 +308,16 @@ SkLightingShaderImpl::LightingShaderContext::LightingShaderContext(
         flags |= kOpaqueAlpha_Flag;
     }
 
+    fPaintColor = rec.fPaint->getColor();
     fFlags = flags;
 }
 
 SkLightingShaderImpl::LightingShaderContext::~LightingShaderContext() {
     // The dependencies have been created outside of the context on memory that was allocated by
     // the onCreateContext() method. Call the destructors and free the memory.
-    fDiffuseContext->~Context();
+    if (fDiffuseContext) {
+        fDiffuseContext->~Context();
+    }
     fNormalProvider->~Provider();
 
     sk_free(fHeapAllocated);
@@ -352,15 +355,21 @@ void SkLightingShaderImpl::LightingShaderContext::shadeSpan(int x, int y,
     SkPMColor diffuse[BUFFER_MAX];
     SkPoint3 normals[BUFFER_MAX];
 
+    SkColor diffColor = fPaintColor;
+
     do {
         int n = SkTMin(count, BUFFER_MAX);
 
-        fDiffuseContext->shadeSpan(x, y, diffuse, n);
         fNormalProvider->fillScanLine(x, y, normals, n);
 
-        for (int i = 0; i < n; ++i) {
+        if (fDiffuseContext) {
+            fDiffuseContext->shadeSpan(x, y, diffuse, n);
+        }
 
-            SkColor diffColor = SkUnPreMultiply::PMColorToColor(diffuse[i]);
+        for (int i = 0; i < n; ++i) {
+            if (fDiffuseContext) {
+                diffColor = SkUnPreMultiply::PMColorToColor(diffuse[i]);
+            }
 
             SkColor3f accum = SkColor3f::Make(0.0f, 0.0f, 0.0f);
             // This is all done in linear unpremul color space (each component 0..255.0f though)
@@ -430,7 +439,12 @@ sk_sp<SkFlattenable> SkLightingShaderImpl::CreateProc(SkReadBuffer& buf) {
     sk_sp<SkLights> lights(builder.finish());
 
     sk_sp<SkNormalSource> normalSource(buf.readFlattenable<SkNormalSource>());
-    sk_sp<SkShader> diffuseShader(buf.readFlattenable<SkShader>());
+
+    bool hasDiffuse = buf.readBool();
+    sk_sp<SkShader> diffuseShader = nullptr;
+    if (hasDiffuse) {
+        diffuseShader = buf.readFlattenable<SkShader>();
+    }
 
     return sk_make_sp<SkLightingShaderImpl>(std::move(diffuseShader), std::move(normalSource),
                                             std::move(lights));
@@ -453,7 +467,10 @@ void SkLightingShaderImpl::flatten(SkWriteBuffer& buf) const {
     }
 
     buf.writeFlattenable(fNormalSource.get());

[robertphillips, 2016/07/11 14:02:04]

Normally this would require a bump in the .skp version number. I think we're
okay though since no one should be saving these out yet.

[dvonbeck, 2016/07/11 19:37:29]

Acknowledged. It's changed quite a few times by now so that's good, hahaha.

-    buf.writeFlattenable(fDiffuseShader.get());
+    buf.writeBool(fDiffuseShader);
+    if (fDiffuseShader) {
+        buf.writeFlattenable(fDiffuseShader.get());
+    }
 }
 
 size_t SkLightingShaderImpl::onContextSize(const ContextRec& rec) const {
@@ -462,18 +479,23 @@ size_t SkLightingShaderImpl::onContextSize(const ContextRec& rec) const {
 
 SkShader::Context* SkLightingShaderImpl::onCreateContext(const ContextRec& rec,
                                                          void* storage) const {
-    size_t heapRequired = fDiffuseShader->contextSize(rec) +
+    size_t heapRequired = (fDiffuseShader ? fDiffuseShader->contextSize(rec) : 0) +
                           fNormalSource->providerSize(rec);
     void* heapAllocated = sk_malloc_throw(heapRequired);
 
     void* diffuseContextStorage = heapAllocated;
-    SkShader::Context* diffuseContext = fDiffuseShader->createContext(rec, diffuseContextStorage);
-    if (!diffuseContext) {
-        sk_free(heapAllocated);
-        return nullptr;
+    void* normalProviderStorage = (char*) diffuseContextStorage +
+                                  (fDiffuseShader ? fDiffuseShader->contextSize(rec) : 0);
+
+    SkShader::Context *diffuseContext = nullptr;
+    if (fDiffuseShader) {
+        diffuseContext = fDiffuseShader->createContext(rec, diffuseContextStorage);
+        if (!diffuseContext) {
+            sk_free(heapAllocated);
+            return nullptr;
+        }
     }
 
-    void* normalProviderStorage = (char*)heapAllocated + fDiffuseShader->contextSize(rec);
     SkNormalSource::Provider* normalProvider = fNormalSource->asProvider(rec,
                                                                          normalProviderStorage);
     if (!normalProvider) {
@@ -491,10 +513,8 @@ SkShader::Context* SkLightingShaderImpl::onCreateContext(const ContextRec& rec,
 sk_sp<SkShader> SkLightingShader::Make(sk_sp<SkShader> diffuseShader,
                                        sk_sp<SkNormalSource> normalSource,
                                        sk_sp<SkLights> lights) {
-    if (!diffuseShader || !normalSource) {
-        // TODO: Use paint's color in absence of a diffuseShader
-        // TODO: Use a default implementation of normalSource instead
-        return nullptr;
+    if (!normalSource) {
+        normalSource = SkNormalSource::MakeFlat();
     }
 
     return sk_make_sp<SkLightingShaderImpl>(std::move(diffuseShader), std::move(normalSource),