Extract most of the mutable state of SkShader into a separate Context object.

src/core/SkBlitter.cpp

Index: src/core/SkBlitter.cpp
diff --git a/src/core/SkBlitter.cpp b/src/core/SkBlitter.cpp
index f925eaa7b2d7f193ef076a7b696ef0db42c339ca..53366c685ba8438b8f8e2d342f67c734d7bd7c81 100644
--- a/src/core/SkBlitter.cpp
+++ b/src/core/SkBlitter.cpp
@@ -26,6 +26,11 @@ SkBlitter::~SkBlitter() {}
 
 bool SkBlitter::isNullBlitter() const { return false; }
 
+bool SkBlitter::resetShaderContext(const SkBitmap& device, const SkPaint& paint,
+                                   const SkMatrix& matrix) {
+    return true;
+}
+
 const SkBitmap* SkBlitter::justAnOpaqueColor(uint32_t* value) {
     return NULL;
 }
@@ -568,102 +573,147 @@ class Sk3DShader : public SkShader {
 public:
     Sk3DShader(SkShader* proxy) : fProxy(proxy) {
         SkSafeRef(proxy);
-        fMask = NULL;
     }
 
     virtual ~Sk3DShader() {
         SkSafeUnref(fProxy);
     }
 
-    void setMask(const SkMask* mask) { fMask = mask; }
+    virtual size_t contextSize() const SK_OVERRIDE {
+        size_t size = sizeof(SkShader::Context);

[scroggo, 2014/03/24 21:24:46]

I think this should be sizeof(Sk3DShaderContext).

reed@ had suggested we pass the size to createContext. I was initially against
it, but maybe it would have helped us find a bug like this?

[Dominik Grewe, 2014/03/26 17:22:22]

Yes.
Maybe, though we'd check for exactly what we return here, so if we have a bug
here we're likely to have the same bug in createContext :)

[scroggo, 2014/03/26 23:13:09]

I guess we could assert that the size of the returned object in createContext is
smaller than this->contextSize(). It's a bit ugly and requires extra code in
each class though :(

+        if (fProxy) {
+            size += fProxy->contextSize();
+        }
+        return size;
+    }
 
-    virtual bool setContext(const SkBitmap& device, const SkPaint& paint,
-                            const SkMatrix& matrix) SK_OVERRIDE {
-        if (!this->INHERITED::setContext(device, paint, matrix)) {
+    virtual bool validContext(const SkBitmap& device, const SkPaint& paint,
+                              const SkMatrix& matrix) const SK_OVERRIDE
+    {
+        if (!this->INHERITED::validContext(device, paint, matrix)) {
             return false;
         }
         if (fProxy) {
-            if (!fProxy->setContext(device, paint, matrix)) {
-                // must keep our set/end context calls balanced
-                this->INHERITED::endContext();
-                return false;
-            }
-        } else {
-            fPMColor = SkPreMultiplyColor(paint.getColor());
+            return fProxy->validContext(device, paint, matrix);
         }
         return true;
     }
 
-    virtual void endContext() SK_OVERRIDE {
+    virtual SkShader::Context* createContext(const SkBitmap& device,
+                                             const SkPaint& paint,
+                                             const SkMatrix& matrix,
+                                             void* storage) const SK_OVERRIDE
+    {
+        if (!this->validContext(device, paint, matrix)) {
+            return NULL;
+        }
+
+        SkShader::Context* proxyContext;
         if (fProxy) {
-            fProxy->endContext();
+            char* proxyContextStorage = (char*) storage + sizeof(Sk3DShaderContext);
+            proxyContext = fProxy->createContext(device, paint, matrix, proxyContextStorage);
+            SkASSERT(proxyContext);
+        } else {
+            proxyContext = NULL;
         }
-        this->INHERITED::endContext();
+        return SkNEW_PLACEMENT_ARGS(storage, Sk3DShaderContext, (*this, device, paint, matrix,
+                                                                 proxyContext));
     }
 
-    virtual void shadeSpan(int x, int y, SkPMColor span[], int count) SK_OVERRIDE {
-        if (fProxy) {
-            fProxy->shadeSpan(x, y, span, count);
+    class Sk3DShaderContext : public SkShader::Context {
+    public:
+        // Takes ownership of proxyContext and calls its destructor.
+        Sk3DShaderContext(const Sk3DShader& shader, const SkBitmap& device, const SkPaint& paint,
+                          const SkMatrix& matrix, SkShader::Context* proxyContext)
+            : INHERITED(shader, device, paint, matrix)
+            , fMask(NULL)
+            , fProxyContext(proxyContext)
+        {
+            if (!fProxyContext) {
+                fPMColor = SkPreMultiplyColor(paint.getColor());
+            }
         }
 
-        if (fMask == NULL) {
-            if (fProxy == NULL) {
-                sk_memset32(span, fPMColor, count);
+        virtual ~Sk3DShaderContext() {
+            if (fProxyContext) {
+                fProxyContext->SkShader::Context::~Context();
             }
-            return;
         }
 
-        SkASSERT(fMask->fBounds.contains(x, y));
-        SkASSERT(fMask->fBounds.contains(x + count - 1, y));
+        void setMask(const SkMask* mask) { fMask = mask; }
 
-        size_t          size = fMask->computeImageSize();
-        const uint8_t*  alpha = fMask->getAddr8(x, y);
-        const uint8_t*  mulp = alpha + size;
-        const uint8_t*  addp = mulp + size;
+        virtual void shadeSpan(int x, int y, SkPMColor span[], int count) SK_OVERRIDE {
+            if (fProxyContext) {
+                fProxyContext->shadeSpan(x, y, span, count);
+            }
 
-        if (fProxy) {
-            for (int i = 0; i < count; i++) {
-                if (alpha[i]) {
-                    SkPMColor c = span[i];
-                    if (c) {
-                        unsigned a = SkGetPackedA32(c);
-                        unsigned r = SkGetPackedR32(c);
-                        unsigned g = SkGetPackedG32(c);
-                        unsigned b = SkGetPackedB32(c);
+            if (fMask == NULL) {
+                if (fProxyContext == NULL) {
+                    sk_memset32(span, fPMColor, count);
+                }
+                return;
+            }
 
+            SkASSERT(fMask->fBounds.contains(x, y));
+            SkASSERT(fMask->fBounds.contains(x + count - 1, y));
+
+            size_t          size = fMask->computeImageSize();
+            const uint8_t*  alpha = fMask->getAddr8(x, y);
+            const uint8_t*  mulp = alpha + size;
+            const uint8_t*  addp = mulp + size;
+
+            if (fProxyContext) {
+                for (int i = 0; i < count; i++) {
+                    if (alpha[i]) {
+                        SkPMColor c = span[i];
+                        if (c) {
+                            unsigned a = SkGetPackedA32(c);
+                            unsigned r = SkGetPackedR32(c);
+                            unsigned g = SkGetPackedG32(c);
+                            unsigned b = SkGetPackedB32(c);
+
+                            unsigned mul = SkAlpha255To256(mulp[i]);
+                            unsigned add = addp[i];
+
+                            r = SkFastMin32(SkAlphaMul(r, mul) + add, a);
+                            g = SkFastMin32(SkAlphaMul(g, mul) + add, a);
+                            b = SkFastMin32(SkAlphaMul(b, mul) + add, a);
+
+                            span[i] = SkPackARGB32(a, r, g, b);
+                        }
+                    } else {
+                        span[i] = 0;
+                    }
+                }
+            } else {    // color
+                unsigned a = SkGetPackedA32(fPMColor);
+                unsigned r = SkGetPackedR32(fPMColor);
+                unsigned g = SkGetPackedG32(fPMColor);
+                unsigned b = SkGetPackedB32(fPMColor);
+                for (int i = 0; i < count; i++) {
+                    if (alpha[i]) {
                         unsigned mul = SkAlpha255To256(mulp[i]);
                         unsigned add = addp[i];
 
-                        r = SkFastMin32(SkAlphaMul(r, mul) + add, a);
-                        g = SkFastMin32(SkAlphaMul(g, mul) + add, a);
-                        b = SkFastMin32(SkAlphaMul(b, mul) + add, a);
-
-                        span[i] = SkPackARGB32(a, r, g, b);
+                        span[i] = SkPackARGB32( a,
+                                        SkFastMin32(SkAlphaMul(r, mul) + add, a),
+                                        SkFastMin32(SkAlphaMul(g, mul) + add, a),
+                                        SkFastMin32(SkAlphaMul(b, mul) + add, a));
+                    } else {
+                        span[i] = 0;
                     }
-                } else {
-                    span[i] = 0;
-                }
-            }
-        } else {    // color
-            unsigned a = SkGetPackedA32(fPMColor);
-            unsigned r = SkGetPackedR32(fPMColor);
-            unsigned g = SkGetPackedG32(fPMColor);
-            unsigned b = SkGetPackedB32(fPMColor);
-            for (int i = 0; i < count; i++) {
-                if (alpha[i]) {
-                    unsigned mul = SkAlpha255To256(mulp[i]);
-                    unsigned add = addp[i];
-
-                    span[i] = SkPackARGB32( a,
-                                    SkFastMin32(SkAlphaMul(r, mul) + add, a),
-                                    SkFastMin32(SkAlphaMul(g, mul) + add, a),
-                                    SkFastMin32(SkAlphaMul(b, mul) + add, a));
-                } else {
-                    span[i] = 0;
                 }
             }
         }
-    }
+    private:
+        // Unowned.
+        const SkMask*   fMask;
+        // Memory is unowned, but we need to call the destructor.
+        SkShader::Context*     fProxyContext;

[scroggo, 2014/03/24 21:24:46]

nit: The variable names should align.

[Dominik Grewe, 2014/03/26 17:22:22]

Done.

+        SkPMColor       fPMColor;
+
+        typedef SkShader::Context INHERITED;
+    };
 
 #ifdef SK_DEVELOPER
     virtual void toString(SkString* str) const SK_OVERRIDE {
@@ -685,29 +735,30 @@ public:
 protected:
     Sk3DShader(SkReadBuffer& buffer) : INHERITED(buffer) {
         fProxy = buffer.readShader();
-        fPMColor = buffer.readColor();
-        fMask = NULL;
+        // Leaving this here until we bump the picture version, though this
+        // shader should never be recorded.
+        buffer.readColor();
     }
 
     virtual void flatten(SkWriteBuffer& buffer) const SK_OVERRIDE {
         this->INHERITED::flatten(buffer);
         buffer.writeFlattenable(fProxy);
-        buffer.writeColor(fPMColor);
+        // Leaving this here until we bump the picture version, though this
+        // shader should never be recorded.
+        buffer.writeColor(SkColor());
     }
 
 private:
     SkShader*       fProxy;
-    SkPMColor       fPMColor;
-    const SkMask*   fMask;
 
     typedef SkShader INHERITED;
 };
 
 class Sk3DBlitter : public SkBlitter {
 public:
-    Sk3DBlitter(SkBlitter* proxy, Sk3DShader* shader)
+    Sk3DBlitter(SkBlitter* proxy, Sk3DShader::Sk3DShaderContext* shaderContext)
         : fProxy(proxy)
-        , f3DShader(SkRef(shader))
+        , f3DShaderContext(shaderContext)
     {}
 
     virtual void blitH(int x, int y, int width) {
@@ -729,22 +780,22 @@ public:
 
     virtual void blitMask(const SkMask& mask, const SkIRect& clip) {
         if (mask.fFormat == SkMask::k3D_Format) {
-            f3DShader->setMask(&mask);
+            f3DShaderContext->setMask(&mask);
 
             ((SkMask*)&mask)->fFormat = SkMask::kA8_Format;
             fProxy->blitMask(mask, clip);
             ((SkMask*)&mask)->fFormat = SkMask::k3D_Format;
 
-            f3DShader->setMask(NULL);
+            f3DShaderContext->setMask(NULL);
         } else {
             fProxy->blitMask(mask, clip);
         }
     }
 
 private:
-    // fProxy is unowned. It will be deleted by SkSmallAllocator.
-    SkBlitter*               fProxy;
-    SkAutoTUnref<Sk3DShader> f3DShader;
+    // Both pointers are unowned. They will be deleted by SkSmallAllocator.
+    SkBlitter*                     fProxy;
+    Sk3DShader::Sk3DShaderContext* f3DShaderContext;
 };
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -754,8 +805,11 @@ private:
 static bool just_solid_color(const SkPaint& paint) {
     if (paint.getAlpha() == 0xFF && paint.getColorFilter() == NULL) {
         SkShader* shader = paint.getShader();
-        if (NULL == shader ||
-            (shader->getFlags() & SkShader::kOpaqueAlpha_Flag)) {
+        // FIXME: This is ONLY called BEFORE setContext, when the flags are not
+        // supposed to be meaningful. Do we need flags on the shader as well as
+        // the impl?
+        if (NULL == shader /*||
+            (shader->getFlags() & SkShader::kOpaqueAlpha_Flag)*/) {
             return true;
         }
     }
@@ -893,16 +947,22 @@ SkBlitter* SkBlitter::Choose(const SkBitmap& device,
     }
 
     /*
-     *  We need to have balanced calls to the shader:
-     *      setContext
-     *      endContext
-     *  We make the first call here, in case it fails we can abort the draw.
-     *  The endContext() call is made by the blitter (assuming setContext did
-     *  not fail) in its destructor.
+     *  We create a SkShader::Context object, and store it on the blitter.
      */
-    if (shader && !shader->setContext(device, *paint, matrix)) {
-        blitter = allocator->createT<SkNullBlitter>();
-        return blitter;
+    SkShader::Context* shaderContext;
+    if (shader) {
+        // Try to create the ShaderContext
+        void* storage = allocator->reserveT<SkShader::Context>(shader->contextSize());
+        shaderContext = shader->createContext(device, *paint, matrix, storage);
+        if (!shaderContext) {
+            allocator->free(storage);
+            blitter = allocator->createT<SkNullBlitter>();
+            return blitter;
+        }
+        SkASSERT(shaderContext);
+        SkASSERT((void*) shaderContext == storage);
+    } else {
+        shaderContext = NULL;
     }
 
 
@@ -913,19 +973,19 @@ SkBlitter* SkBlitter::Choose(const SkBitmap& device,
                 SkASSERT(NULL == paint->getXfermode());
                 blitter = allocator->createT<SkA8_Coverage_Blitter>(device, *paint);
             } else if (shader) {
-                blitter = allocator->createT<SkA8_Shader_Blitter>(device, *paint);
+                blitter = allocator->createT<SkA8_Shader_Blitter>(device, *paint, shaderContext);
             } else {
                 blitter = allocator->createT<SkA8_Blitter>(device, *paint);
             }
             break;
 
         case kRGB_565_SkColorType:
-            blitter = SkBlitter_ChooseD565(device, *paint, allocator);
+            blitter = SkBlitter_ChooseD565(device, *paint, shaderContext, allocator);
             break;
 
         case kPMColor_SkColorType:
             if (shader) {
-                blitter = allocator->createT<SkARGB32_Shader_Blitter>(device, *paint);
+                blitter = allocator->createT<SkARGB32_Shader_Blitter>(device, *paint, shaderContext);
             } else if (paint->getColor() == SK_ColorBLACK) {
                 blitter = allocator->createT<SkARGB32_Black_Blitter>(device, *paint);
             } else if (paint->getAlpha() == 0xFF) {
@@ -944,7 +1004,8 @@ SkBlitter* SkBlitter::Choose(const SkBitmap& device,
     if (shader3D) {
         SkBlitter* innerBlitter = blitter;
         // innerBlitter was allocated by allocator, which will delete it.
-        blitter = allocator->createT<Sk3DBlitter>(innerBlitter, shader3D);
+        blitter = allocator->createT<Sk3DBlitter>(innerBlitter,
+                static_cast<Sk3DShader::Sk3DShaderContext*>(shaderContext));

[scroggo, 2014/03/24 21:24:46]

Maybe add a comment here that we know shaderContext *must* be an instance of
Sk3DShaderContext? (It's too bad we can't assert that.) It's not obvious unless
you delve into the code above.

[Dominik Grewe, 2014/03/26 17:22:22]

Done.

     }
     return blitter;
 }
@@ -956,18 +1017,29 @@ const uint32_t gMask_00FF00FF = 0xFF00FF;
 
 ///////////////////////////////////////////////////////////////////////////////
 
-SkShaderBlitter::SkShaderBlitter(const SkBitmap& device, const SkPaint& paint)
-        : INHERITED(device) {
-    fShader = paint.getShader();
+SkShaderBlitter::SkShaderBlitter(const SkBitmap& device, const SkPaint& paint,
+                                 SkShader::Context* shaderContext)
+        : INHERITED(device), fShader(paint.getShader()), fShaderContext(shaderContext) {
     SkASSERT(fShader);
-    SkASSERT(fShader->setContextHasBeenCalled());
+    SkASSERT(fShaderContext);
 
-    fShader->ref();
-    fShaderFlags = fShader->getFlags();
+    fShaderFlags = fShaderContext->getFlags();
 }
 
-SkShaderBlitter::~SkShaderBlitter() {
-    SkASSERT(fShader->setContextHasBeenCalled());
-    fShader->endContext();
-    fShader->unref();
+SkShaderBlitter::~SkShaderBlitter() {}
+
+bool SkShaderBlitter::resetShaderContext(const SkBitmap& device, const SkPaint& paint,
+                                         const SkMatrix& matrix) {
+    bool newContext = false;

[scroggo, 2014/03/24 21:24:46]

Why do you need the variable newContext?

Why not just say:

if (!fShader->validContext(device, paint, matrix)) {
    return false;
}
.
.
.
return true;

[Dominik Grewe, 2014/03/26 17:22:22]

Good point. I rewrote this function a few times and I should have removed it.

+    if (fShader->validContext(device, paint, matrix)) {
+        // Only destroy the old context if we have a new one. We need to ensure to have a
+        // live context in fShaderContext because the storage is owned by an SkSmallAllocator
+        // outside of this class.
+        fShaderContext->SkShader::Context::~Context();
+        fShaderContext = fShader->createContext(device, paint, matrix, (void*)fShaderContext);
+        SkASSERT(fShaderContext);

[scroggo, 2014/03/24 21:24:46]

This makes me nervous. It means we have to enforce that no shader returns true
for validContext and NULL for createContext using the same parameters. Maybe
that's the right behavior though, and any shaders that are broken should fail
here and result in an assert.

[Dominik Grewe, 2014/03/26 17:22:22]

I agree. I was hoping we'd never have to call validContext outside of the
shaders, but unless we give the blitter its own SkSmallAllocator (wdyt?) I don't
see a way around it.

I assumed that createContext returns NULL iff validContext returns false, but
that wouldn't be the case any more if we changed SkBitmapProcShader the way you
suggested.

[scroggo, 2014/03/26 23:13:09]

Who else is calling validContext? (I was thinking someone might call it as an
assert, but I don't remember if anyone else calls it.)

Why do you want to give the blitter its own allocator? It would be kind of
ironic because the blitter is created by an SkSmallAllocator.

[Dominik Grewe, 2014/03/27 14:27:20]

This is the only caller outside SkShader afaict.
It should work in principle, right? :) Though it seems like we're creating the
shader context before creating the blitter, so maybe we can't do it here...

The idea was this:
If the blitter has its own allocator, we could reset it here (might need a new
method for that) instead of calling the destructor without actually owning the
storage. It's a weird contract at the moment, where SkBlitter may change the
object itself but has to ensure that there's always some object so the owner of
the storage can destroy it.

[scroggo, 2014/03/27 18:05:33]

I actually think it makes sense that blitter has to call the destructor of the
object it replaces. We're assuming that the object is owned, and it was going to
have its destructor called. So the blitter just called it early, and put a new
one there (whose destructor will be called when the original one would have
originally).

Back to the blitter having its own SkSmallAllocator, I'm not against it, now
that I think about it. In that case, we could almost convert the
SkSmallAllocator to only have the ability to create one object (so long as we
don't mind going back to heap allocating the Sk3DBlitter in SkBlitter::Choose,
which should be fine).

[Dominik Grewe, 2014/03/28 18:16:31]

The only problem is that the first SkShader::Context is created before the
blitter is created. Maybe that can be changed but if you're happy with the
solution here it may not be worth it.

+        newContext = true;
+    }
+
+    return newContext;
 }