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

src/core/SkBitmapProcShader.cpp

 
 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 #include "SkColorPriv.h"
 #include "SkReadBuffer.h"
 #include "SkWriteBuffer.h"
@@ skipping 16 matching lines @@
                 return true;
         default:
             break;
     }
     return false;
 }
 
 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,
                                                    SkMatrix* texM,
                                                    TileMode xy[]) const {
     if (texture) {
         *texture = fRawBitmap;
     }
     if (texM) {
         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;
 }
 
 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) {
     unsigned mask = SkMatrix::kTranslate_Mask | SkMatrix::kScale_Mask;
     return (matrix.getType() & ~mask) == 0;
 }
 
 bool SkBitmapProcShader::isOpaque() const {
     return fRawBitmap.isOpaque();
 }
 
 static bool valid_for_drawing(const SkBitmap& bm) {
     if (0 == bm.width() || 0 == bm.height()) {
         return false;   // nothing to draw
     }
     if (NULL == bm.pixelRef()) {
         return false;   // no pixels to read
     }
     if (kIndex_8_SkColorType == bm.colorType()) {
         // ugh, I have to lock-pixels to inspect the colortable
         SkAutoLockPixels alp(bm);
         if (!bm.getColorTable()) {
             return false;
         }
     }
     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();
 
     // update fFlags
     uint32_t flags = 0;
     if (bitmapIsOpaque && (255 == this->getPaintAlpha())) {
         flags |= kOpaqueAlpha_Flag;
     }
 
     switch (bitmap.colorType()) {
@@ skipping 21 matching lines @@
     // if we're only 1-pixel high, and we don't rotate, then we can claim this
     if (1 == bitmap.height() &&
             only_scale_and_translate(this->getTotalInverse())) {
         flags |= kConstInY32_Flag;
         if (flags & kHasSpan16_Flag) {
             flags |= kConstInY16_Flag;
         }
     }
 
     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
 
 #define TEST_BUFFER_OVERRITEx
 
 #ifdef TEST_BUFFER_OVERRITE
     #define TEST_BUFFER_EXTRA   32
     #define TEST_PATTERN    0x88888888
 #else
     #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);
         return;
     }
 
     uint32_t buffer[BUF_MAX + TEST_BUFFER_EXTRA];
     SkBitmapProcState::MatrixProc   mproc = state.getMatrixProc();
     SkBitmapProcState::SampleProc32 sproc = state.getSampleProc32();
     int max = fState.maxCountForBufferSize(sizeof(buffer[0]) * BUF_MAX);
@@ skipping 23 matching lines @@
 
         if ((count -= n) == 0) {
             break;
         }
         SkASSERT(count > 0);
         x += n;
         dstC += n;
     }
 }
 
-SkShader::ShadeProc SkBitmapProcShader::asAShadeProc(void** ctx) {
+SkShader::Context::ShadeProc SkBitmapProcShader::BitmapProcShaderContext::asAShadeProc(void** ctx) {
     if (fState.getShaderProc32()) {
         *ctx = &fState;
         return (ShadeProc)fState.getShaderProc32();
     }
     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);
         return;
     }
 
     uint32_t buffer[BUF_MAX];
     SkBitmapProcState::MatrixProc   mproc = state.getMatrixProc();
     SkBitmapProcState::SampleProc16 sproc = state.getSampleProc16();
     int max = fState.maxCountForBufferSize(sizeof(buffer));
@@ skipping 93 matching lines @@
 
 #ifdef SK_DEVELOPER
 void SkBitmapProcShader::toString(SkString* str) const {
     static const char* gTileModeName[SkShader::kTileModeCount] = {
         "clamp", "repeat", "mirror"
     };
 
     str->append("BitmapShader: (");
 
     str->appendf("(%s, %s)",
-                 gTileModeName[fState.fTileModeX],
-                 gTileModeName[fState.fTileModeY]);
+                 gTileModeName[fTileModeX],
+                 gTileModeName[fTileModeY]);
 
     str->append(" ");
     fRawBitmap.toString(str);
 
     this->INHERITED::toString(str);
 
     str->append(")");
 }
 #endif
 
@@ skipping 20 matching lines @@
     SkMatrix matrix;
     matrix.setIDiv(fRawBitmap.width(), fRawBitmap.height());
 
     SkMatrix lmInverse;
     if (!this->getLocalMatrix().invert(&lmInverse)) {
         return NULL;
     }
     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
     // we check the matrix scale factors to determine how to interpret the filter quality setting.
     // This completely ignores the complexity of the drawVertices case where explicit local coords
     // are provided by the caller.
     SkPaint::FilterLevel paintFilterLevel = paint.getFilterLevel();
     GrTextureParams::FilterMode textureFilterMode;
     switch(paintFilterLevel) {
         case SkPaint::kNone_FilterLevel:
@@ skipping 45 matching lines @@
     GrEffectRef* effect = NULL;
     if (paintFilterLevel == SkPaint::kHigh_FilterLevel) {
         effect = GrBicubicEffect::Create(texture, matrix, tm);
     } else {
         effect = GrSimpleTextureEffect::Create(texture, matrix, params);
     }
     GrUnlockAndUnrefCachedBitmapTexture(texture);
     return effect;
 }
 #endif