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

src/core/SkDraw.cpp

 /*
  * Copyright 2006 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkDraw.h"
 #include "SkBlitter.h"
 #include "SkBounder.h"
@@ skipping 2337 matching lines @@
 
     src[0] = texs[state.f0];
     src[1] = texs[state.f1];
     src[2] = texs[state.f2];
     dst[0] = verts[state.f0];
     dst[1] = verts[state.f1];
     dst[2] = verts[state.f2];
     return matrix->setPolyToPoly(src, dst, 3);
 }
 
+/**
+ *  This class is NOT immutable due to the setup() method being called repeatedly after
+ *  the shader has been associated with a paint. This is okay because we only ever
+ *  associate this shader with temporary paints.
+ */
 class SkTriColorShader : public SkShader {
 public:
     SkTriColorShader() {}
 
-    bool setup(const SkPoint pts[], const SkColor colors[], int, int, int);
+    // TODO(dominikg): Feels like this should be part of the context, but we don't have
+    //                 access to it where we need it.
+    bool setup(const SkPoint pts[], const SkColor colors[], int, int, int, const SkMatrix&);
 
-    virtual void shadeSpan(int x, int y, SkPMColor dstC[], int count) SK_OVERRIDE;
+    virtual SkShader::Context* createContext(
+            const SkBitmap&, const SkPaint&, const SkMatrix&, void*) const SK_OVERRIDE;
+    virtual size_t contextSize() const SK_OVERRIDE;
+
+    class TriColorShaderContext : public SkShader::Context {
+    public:
+        TriColorShaderContext(const SkTriColorShader& shader, const SkBitmap& device,
+                              const SkPaint& paint, const SkMatrix& matrix);
+        virtual ~TriColorShaderContext();
+
+        virtual void shadeSpan(int x, int y, SkPMColor dstC[], int count) SK_OVERRIDE;
+    private:
+        const SkTriColorShader& fTriColorShader;

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

Any reason SkTriColorShader does not simply static cast fShader?

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

Done.

+
+        typedef SkShader::Context INHERITED;
+    };
 
     SK_DEVELOPER_TO_STRING()
     SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(SkTriColorShader)
 
 protected:
     SkTriColorShader(SkReadBuffer& buffer) : SkShader(buffer) {}
 
 private:
     SkMatrix    fDstToUnit;
     SkPMColor   fColors[3];
 
     typedef SkShader INHERITED;
 };
 
+SkShader::Context* SkTriColorShader::createContext(const SkBitmap& device, const SkPaint& paint,
+                                                   const SkMatrix& matrix, void* storage) const {
+    if (!this->validContext(device, paint, matrix)) {
+        return NULL;
+    }
+
+    return SkNEW_PLACEMENT_ARGS(storage, TriColorShaderContext, (*this, device, paint, matrix));
+}
+
 bool SkTriColorShader::setup(const SkPoint pts[], const SkColor colors[],
-                             int index0, int index1, int index2) {
+                             int index0, int index1, int index2, const SkMatrix& matrix) {

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

Maybe rename this parameter to make it clear what it's used for. totalInverse?

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

It's not the totalInverse though, is it? We use it to compute the inverse but I
don't know what it represents by itself. It's the same matrix we pass into
createContext().

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

Oh, I misread the code below...

 
     fColors[0] = SkPreMultiplyColor(colors[index0]);
     fColors[1] = SkPreMultiplyColor(colors[index1]);
     fColors[2] = SkPreMultiplyColor(colors[index2]);
 
     SkMatrix m, im;
     m.reset();
     m.set(0, pts[index1].fX - pts[index0].fX);
     m.set(1, pts[index2].fX - pts[index0].fX);
     m.set(2, pts[index0].fX);
     m.set(3, pts[index1].fY - pts[index0].fY);
     m.set(4, pts[index2].fY - pts[index0].fY);
     m.set(5, pts[index0].fY);
     if (!m.invert(&im)) {
         return false;
     }
-    return fDstToUnit.setConcat(im, this->getTotalInverse());
+    SkMatrix inverse;
+    SkASSERT(matrix.invert(&inverse));

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

I seemed to have overlooked this part. I take it this is mimicking the work that
is  done in setContext/createContext? A few comments:

- This appears to ignore the local matrix.
- Code inside SkASSERT is removed from Release builds. To do what you want
(assert that the code returns true in debug, and just run the code ignoring the
result in release), you can SkAssertResult.
- It's not obvious why you're asserting that it must succeed - presumably
because validContext would have returned NULL previously so we would not have
reached here. If that's the case, it could use a comment.
- Why don't we pass the Context to this function, or Context::getTotalInverse?
(It would mean adding a getter to the blitter, maybe that's okay?) Then we know
we have the value we want.

As suggested in SkBitmapProcShader, perhaps validContext should have an out
parameter that returns the total inverse?

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

Actually, if we add a SkBlitter::getShaderContext(), we can move this method to
TriColorShaderContext where we have access to totalInverse. It also means that
SkTriColorShader is immutable.

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

That seems even better to me.

+    return fDstToUnit.setConcat(im, inverse);
 }
 
 #include "SkColorPriv.h"
 #include "SkComposeShader.h"
 
 static int ScalarTo256(SkScalar v) {
     int scale = SkScalarToFixed(v) >> 8;
     if (scale < 0) {
         scale = 0;
     }
     if (scale > 255) {
         scale = 255;
     }
     return SkAlpha255To256(scale);
 }
 
-void SkTriColorShader::shadeSpan(int x, int y, SkPMColor dstC[], int count) {
+
+SkTriColorShader::TriColorShaderContext::TriColorShaderContext(
+        const SkTriColorShader& shader, const SkBitmap& device,

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

nit: Parameters that fit on the same line should go on the same line.

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

If I move the first one to the same line and line up all the others below it, it
takes up more space than this way. I think the way it is is cleaner and the
style guide seems to support it iiuc.

+        const SkPaint& paint, const SkMatrix& matrix)
+    : INHERITED(shader, device, paint, matrix)
+    , fTriColorShader(shader) {}
+
+SkTriColorShader::TriColorShaderContext::~TriColorShaderContext() {}
+
+size_t SkTriColorShader::contextSize() const {
+    return sizeof(TriColorShaderContext);
+}
+void SkTriColorShader::TriColorShaderContext::shadeSpan(int x, int y, SkPMColor dstC[], int count) {
     SkPoint src;
 
     for (int i = 0; i < count; i++) {
-        fDstToUnit.mapXY(SkIntToScalar(x), SkIntToScalar(y), &src);
+        fTriColorShader.fDstToUnit.mapXY(SkIntToScalar(x), SkIntToScalar(y), &src);
         x += 1;
 
         int scale1 = ScalarTo256(src.fX);
         int scale2 = ScalarTo256(src.fY);
         int scale0 = 256 - scale1 - scale2;
         if (scale0 < 0) {
             if (scale1 > scale2) {
                 scale2 = 256 - scale1;
             } else {
                 scale1 = 256 - scale2;
             }
             scale0 = 0;
         }
 
-        dstC[i] = SkAlphaMulQ(fColors[0], scale0) +
-        SkAlphaMulQ(fColors[1], scale1) +
-        SkAlphaMulQ(fColors[2], scale2);
+        dstC[i] = SkAlphaMulQ(fTriColorShader.fColors[0], scale0) +
+        SkAlphaMulQ(fTriColorShader.fColors[1], scale1) +
+        SkAlphaMulQ(fTriColorShader.fColors[2], scale2);
     }
 }
 
 #ifdef SK_DEVELOPER
 void SkTriColorShader::toString(SkString* str) const {
     str->append("SkTriColorShader: (");
 
     this->INHERITED::toString(str);
 
     str->append(")");
@@ skipping 65 matching lines @@
             SkShader* compose = SkNEW_ARGS(SkComposeShader,
                                            (&triShader, shader, xmode));
             p.setShader(compose)->unref();
             if (releaseMode) {
                 xmode->unref();
             }
         }
     }
 
     SkAutoBlitterChoose blitter(*fBitmap, *fMatrix, p);
-    // important that we abort early, as below we may manipulate the shader
-    // and that is only valid if the shader returned true from setContext.
+    // Important that we abort early, as below we may manipulate the shader context
+    // and that is only valid if a context was successfully created from the shader.
     // If it returned false, then our blitter will be the NullBlitter.
     if (blitter->isNullBlitter()) {
         return;
     }
 
     // setup our state and function pointer for iterating triangles
     VertState       state(count, indices, indexCount);
     VertState::Proc vertProc = state.chooseProc(vmode);
 
     if (NULL != textures || NULL != colors) {
         SkMatrix  tempM;
         SkMatrix  savedLocalM;
         if (shader) {
             savedLocalM = shader->getLocalMatrix();
         }
 
-        // setContext has already been called and verified to return true
-        // by the constructor of SkAutoBlitterChoose
-        bool prevContextSuccess = true;
         while (vertProc(&state)) {
             if (NULL != textures) {
                 if (texture_to_matrix(state, vertices, textures, &tempM)) {
                     tempM.postConcat(savedLocalM);
                     shader->setLocalMatrix(tempM);
-                    // Need to recall setContext since we changed the local matrix.
-                    // However, we also need to balance the calls this with a
-                    // call to endContext which requires tracking the result of
-                    // the previous call to setContext.
-                    if (prevContextSuccess) {
-                        shader->endContext();
-                    }
-                    prevContextSuccess = shader->setContext(*fBitmap, p, *fMatrix);
-                    if (!prevContextSuccess) {
+                    if (!blitter->resetShaderContext(*fBitmap, p, *fMatrix)) {

[Dominik Grewe, 2014/03/21 12:52:50]

Afaict, we want to create a new shader context here, but we don't have access to
either the shader context or the allocator. The solution here seems to work but
maybe there's a better way?

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

This seems like a fine way to do it to me.

                         continue;
                     }
                 }
             }
             if (NULL != colors) {
                 if (!triShader.setup(vertices, colors,
-                                     state.f0, state.f1, state.f2)) {
+                                     state.f0, state.f1, state.f2, *fMatrix)) {
                     continue;
                 }
             }
 
             SkPoint tmp[] = {
                 devVerts[state.f0], devVerts[state.f1], devVerts[state.f2]
             };
             SkScan::FillTriangle(tmp, *fRC, blitter.get());
         }
 
         // now restore the shader's original local matrix
         if (NULL != shader) {
             shader->setLocalMatrix(savedLocalM);
         }
-
-        // If the final call to setContext fails we must make it suceed so that the
-        // call to endContext in the destructor for SkAutoBlitterChoose is balanced.
-        if (!prevContextSuccess) {
-            prevContextSuccess = shader->setContext(*fBitmap, paint, SkMatrix::I());
-            SkASSERT(prevContextSuccess);
-        }
     } else {
         // no colors[] and no texture
         HairProc hairProc = ChooseHairProc(paint.isAntiAlias());
         const SkRasterClip& clip = *fRC;
         while (vertProc(&state)) {
             hairProc(devVerts[state.f0], devVerts[state.f1], clip, blitter.get());
             hairProc(devVerts[state.f1], devVerts[state.f2], clip, blitter.get());
             hairProc(devVerts[state.f2], devVerts[state.f0], clip, blitter.get());
         }
     }
@@ skipping 211 matching lines @@
         mask->fImage = SkMask::AllocImage(size);
         memset(mask->fImage, 0, mask->computeImageSize());
     }
 
     if (SkMask::kJustComputeBounds_CreateMode != mode) {
         draw_into_mask(*mask, devPath, style);
     }
 
     return true;
 }