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

src/core/SkBitmapProcShader.cpp

Index: src/core/SkBitmapProcShader.cpp
diff --git a/src/core/SkBitmapProcShader.cpp b/src/core/SkBitmapProcShader.cpp
index 55a772ec10ace94578a4be78cedbd508de1deb97..9ba655f240794be9c8a12a47c53e8d866186f2ac 100644
--- a/src/core/SkBitmapProcShader.cpp
+++ b/src/core/SkBitmapProcShader.cpp
@@ -34,18 +34,16 @@ bool SkBitmapProcShader::CanDo(const SkBitmap& bm, TileMode tx, TileMode ty) {
 SkBitmapProcShader::SkBitmapProcShader(const SkBitmap& src,
                                        TileMode tmx, TileMode tmy) {
     fRawBitmap = src;
-    fState.fTileModeX = (uint8_t)tmx;
-    fState.fTileModeY = (uint8_t)tmy;
-    fFlags = 0; // computed in setContext
+    fTileModeX = (uint8_t)tmx;
+    fTileModeY = (uint8_t)tmy;
 }
 
 SkBitmapProcShader::SkBitmapProcShader(SkReadBuffer& buffer)
         : INHERITED(buffer) {
     buffer.readBitmap(&fRawBitmap);
     fRawBitmap.setImmutable();
-    fState.fTileModeX = buffer.readUInt();
-    fState.fTileModeY = buffer.readUInt();
-    fFlags = 0; // computed in setContext
+    fTileModeX = buffer.readUInt();
+    fTileModeY = buffer.readUInt();
 }
 
 SkShader::BitmapType SkBitmapProcShader::asABitmap(SkBitmap* texture,
@@ -58,8 +56,8 @@ SkShader::BitmapType SkBitmapProcShader::asABitmap(SkBitmap* texture,
         texM->reset();
     }
     if (xy) {
-        xy[0] = (TileMode)fState.fTileModeX;
-        xy[1] = (TileMode)fState.fTileModeY;
+        xy[0] = (TileMode)fTileModeX;
+        xy[1] = (TileMode)fTileModeY;
     }
     return kDefault_BitmapType;
 }
@@ -68,8 +66,8 @@ void SkBitmapProcShader::flatten(SkWriteBuffer& buffer) const {
     this->INHERITED::flatten(buffer);
 
     buffer.writeBitmap(fRawBitmap);
-    buffer.writeUInt(fState.fTileModeX);
-    buffer.writeUInt(fState.fTileModeY);
+    buffer.writeUInt(fTileModeX);
+    buffer.writeUInt(fTileModeY);
 }
 
 static bool only_scale_and_translate(const SkMatrix& matrix) {
@@ -98,24 +96,58 @@ static bool valid_for_drawing(const SkBitmap& bm) {
     return true;
 }
 
-bool SkBitmapProcShader::setContext(const SkBitmap& device,
-                                    const SkPaint& paint,
-                                    const SkMatrix& matrix) {
+bool SkBitmapProcShader::validContext(const SkBitmap& device,
+                                      const SkPaint& paint,
+                                      const SkMatrix& matrix) const {
     if (!fRawBitmap.getTexture() && !valid_for_drawing(fRawBitmap)) {
         return false;
     }
 
-    // do this first, so we have a correct inverse matrix
-    if (!this->INHERITED::setContext(device, paint, matrix)) {
+    // Do this first, so we know the matrix can be inverted.
+    if (!this->INHERITED::validContext(device, paint, matrix)) {
         return false;
     }
 
-    fState.fOrigBitmap = fRawBitmap;
-    if (!fState.chooseProcs(this->getTotalInverse(), paint)) {
-        this->INHERITED::endContext();
+    // TODO(dominikg): Can we avoid re-computing the inverse?
+    SkMatrix inverse;
+    SkASSERT(matrix.invert(&inverse));
+
+    SkBitmapProcState state;
+    state.fTileModeX = fTileModeX;
+    state.fTileModeY = fTileModeY;
+    state.fOrigBitmap = fRawBitmap;
+    // TODO(dominikg): Could we have a more light-weight method instead of chooseProcs that only

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

Here's what I might do:

1. Instead of making createContext call this->validContext, make it call
this->INHERITED::validContext.
2. Move this work directly into createContext (operating on an SkBitmapProcState
on the stack as you do now).
3. Update BitmapProcShaderContext's constructor to take an SkBitmapProcState as
a parameter.

Then we won't be repeating work - we'll just have to copy the SkBitmapProcState.

We might even mimic steps 1 and 2 for other classes. The downside to that
approach is that if class A : public SkShader does its checks for validContext
inside createContext, and we want class B : public A (or anyone external) to do
those same checks, we'll need to dupe code (bringing the problems of duping
code).

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

Yeah, that could easily lead to problems, couldn't it? Or are we sure that we
won't have subclasses of SkBitmapProcShader?

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

I don't think we'll subclass SkBitmapProcShader, and it is private (inside
src/core), so our clients shouldn't either.

What if we created a new inline function that took an SkBitmapProcShader as a
parameter, modified it and returned the results of chooseProcs? It could be
called by both validContext and createContext. (I guess we're ending up with
creating an object that has invalid state, but we're kind of already doing that
before your change anyway...)

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

Do you mean SkBitmapProcState rather than Shader? We'd still call chooseProcs
twice then, right? I was hoping we can avoid that or call a more light-weight
method in validContext that returns as soon as we know whether we succeed or not
(without actually setting up the SkBitmapProcState).

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

You are correct; I did mean SkBitmapProcState. I don't know of a way to make
chooseProcs lighter weight (but perhaps it could be done). Here's what I
imagine:

private:
bool SkBitmapProcShader::validInternal(device, paint, matrix,
                                       SkBitmapProcState* state) {
  SkMatrix totalInverse;
  if (!this->INHERITED::validContext(device, paint, matrix, &totalInverse)) {
    return false;
  }

  state->fTileModeX = fTileModeX;
  state->fTileModeY = fTileModeY;
  state.fOrigBitmap = fRawBitmap;

  return state.chooseProcs(totalInverse, paint);
}

bool SkBitmapProcShader::validContext(device, matrix, paint, SkMatrix*) {
  SkBitmapProcState state;
  return this->validInternal(device, matrix, paint, &state);
}

SkShader::Context* SkBitmapProcShader::createContext(device, paint, matrix,
                                                     storage) {
  SkBitmapProcState state;
  if (!this->validInternal(device, matrix, paint, &state)) {
    return false;
  }
  return SkNEW_PLACEMENT_ARGS(storage, BitmapProcShaderContext, (*this, device,
                                                                 paint, matrix,
                                                                 state));
}

In this model, chooseProcs is only called once (since I believe the typical
caller
will skip validContext and skip to createContext), but we still have a useful
validContext call (in case we want to assert, for example, or in
resetShaderContext
- are there other reasons we call it?).

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

Yes, that looks reasonable. The only caveat is that it requires a decent copy
constructor for SkBitmapProcState which currently doesn't exist. Simply copying
each element doesn't work. One problem is, for example, that when copying
fOrigBitmap and fScaledBitmap we don't copy the pixel refs (I believe).
fBitmapFilter also looks problematic.

I don't really understand that code very well, so if you have any insights
please let me know. Otherwise I'll have a closer look next week.

[scroggo, 2014/03/28 20:50:07]

I also don't have much time to look at this until next week. As for copying
fOrigBitmap and fScaledBitmap, I'm assuming we don't want to do a deep copy (the
copy constructor for SkBitmap will ref the pixel ref). You're right though, I'm
sure the copy constructor will be complicated.

On the other hand, we could pass a (SkBitmapProcState) pointer to the
(BitmapProcShader) constructor, and have no need to write the copy constructor.
We can skip the extra call to new by putting it in the storage space.

[Dominik Grewe, 2014/04/01 10:49:55]

Good idea. I'm now placing the SkBitmapProcState on the storage behind the
context. The context just holds a pointer to it.

+    //                 validates the inputs?
+    if (!state.chooseProcs(inverse, paint)) {
         return false;
     }
 
+    return true;
+}
+
+SkShader::Context* SkBitmapProcShader::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, BitmapProcShaderContext, (*this, device, paint, matrix));
+}
+
+size_t SkBitmapProcShader::contextSize() const {
+    return sizeof(BitmapProcShaderContext);
+}
+
+SkBitmapProcShader::BitmapProcShaderContext::BitmapProcShaderContext(
+        const SkBitmapProcShader& shader, const SkBitmap& device,
+        const SkPaint& paint, const SkMatrix& matrix) 
+    : INHERITED(shader, device, paint, matrix) {
+
+    fState.fTileModeX = shader.fTileModeX;
+    fState.fTileModeY = shader.fTileModeY;
+    fState.fOrigBitmap = shader.fRawBitmap;
+    SkASSERT(fState.chooseProcs(this->getTotalInverse(), paint));
+
     const SkBitmap& bitmap = *fState.fBitmap;
     bool bitmapIsOpaque = bitmap.isOpaque();
 
@@ -157,12 +189,9 @@ bool SkBitmapProcShader::setContext(const SkBitmap& device,
     }
 
     fFlags = flags;
-    return true;
 }
 
-void SkBitmapProcShader::endContext() {
-    fState.endContext();
-    this->INHERITED::endContext();
+SkBitmapProcShader::BitmapProcShaderContext::~BitmapProcShaderContext() {

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

Any reason not to just remove this entirely?

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

Just for completeness. I've moved the empty body to the class definition.

 }
 
 #define BUF_MAX     128
@@ -176,7 +205,8 @@ void SkBitmapProcShader::endContext() {
     #define TEST_BUFFER_EXTRA   0
 #endif
 
-void SkBitmapProcShader::shadeSpan(int x, int y, SkPMColor dstC[], int count) {
+void SkBitmapProcShader::BitmapProcShaderContext::shadeSpan(
+        int x, int y, SkPMColor dstC[], int count) {

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

Parameters that fit should be on the same line.

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

If I read the coding style correctly it seems that I can put them on the next
line if indented by 8 spaces, but I'm happy to put the ones that fit on the same
line if that's more conventional.

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

I think it's more conventional, but you're right, the style guide does seem to
allow your version. I'll try to stop being so picky ;)

     const SkBitmapProcState& state = fState;
     if (state.getShaderProc32()) {
         state.getShaderProc32()(state, x, y, dstC, count);
@@ -220,7 +250,7 @@ void SkBitmapProcShader::shadeSpan(int x, int y, SkPMColor dstC[], int count) {
     }
 }
 
-SkShader::ShadeProc SkBitmapProcShader::asAShadeProc(void** ctx) {
+SkShader::Context::ShadeProc SkBitmapProcShader::BitmapProcShaderContext::asAShadeProc(void** ctx) {
     if (fState.getShaderProc32()) {
         *ctx = &fState;
         return (ShadeProc)fState.getShaderProc32();
@@ -228,7 +258,8 @@ SkShader::ShadeProc SkBitmapProcShader::asAShadeProc(void** ctx) {
     return NULL;
 }
 
-void SkBitmapProcShader::shadeSpan16(int x, int y, uint16_t dstC[], int count) {
+void SkBitmapProcShader::BitmapProcShaderContext::shadeSpan16(
+        int x, int y, uint16_t dstC[], int count) {

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

Same

     const SkBitmapProcState& state = fState;
     if (state.getShaderProc16()) {
         state.getShaderProc16()(state, x, y, dstC, count);
@@ -342,8 +373,8 @@ void SkBitmapProcShader::toString(SkString* str) const {
     str->append("BitmapShader: (");
 
     str->appendf("(%s, %s)",
-                 gTileModeName[fState.fTileModeX],
-                 gTileModeName[fState.fTileModeY]);
+                 gTileModeName[fTileModeX],
+                 gTileModeName[fTileModeY]);
 
     str->append(" ");
     fRawBitmap.toString(str);
@@ -384,8 +415,8 @@ GrEffectRef* SkBitmapProcShader::asNewEffect(GrContext* context, const SkPaint&
     matrix.preConcat(lmInverse);
 
     SkShader::TileMode tm[] = {
-        (TileMode)fState.fTileModeX,
-        (TileMode)fState.fTileModeY,
+        (TileMode)fTileModeX,
+        (TileMode)fTileModeY,
     };
 
     // Must set wrap and filter on the sampler before requesting a texture. In two places below