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

src/core/SkPictureShader.cpp

 /*
  * Copyright 2014 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkPictureShader.h"
 
 #include "SkBitmap.h"
@@ skipping 37 matching lines @@
     this->INHERITED::flatten(buffer);
 
     buffer.write32(fTmx);
     buffer.write32(fTmy);
     buffer.writeBool(NULL != fPicture);
     if (fPicture) {
         fPicture->flatten(buffer);
     }
 }
 
-bool SkPictureShader::buildBitmapShader(const SkMatrix& matrix) const {
+SkShader* SkPictureShader::buildBitmapShader(const SkMatrix& matrix) const {
     if (!fPicture || (0 == fPicture->width() && 0 == fPicture->height())) {
-        return false;
+        return NULL;
     }
 
     SkMatrix m;
     if (this->hasLocalMatrix()) {
         m.setConcat(matrix, this->getLocalMatrix());
     } else {
         m = matrix;
     }
 
     // Use a rotation-invariant scale
     SkPoint scale;
     if (!SkDecomposeUpper2x2(m, NULL, &scale, NULL)) {
         // Decomposition failed, use an approximation.
         scale.set(SkScalarSqrt(m.getScaleX() * m.getScaleX() + m.getSkewX() * m.getSkewX()),
                   SkScalarSqrt(m.getScaleY() * m.getScaleY() + m.getSkewY() * m.getSkewY()));
     }
     SkSize scaledSize = SkSize::Make(scale.x() * fPicture->width(), scale.y() * fPicture->height());
 
     SkISize tileSize = scaledSize.toRound();
     if (tileSize.isEmpty()) {
-        return false;
+        return NULL;
     }
 
     // The actual scale, compensating for rounding.
     SkSize tileScale = SkSize::Make(SkIntToScalar(tileSize.width()) / fPicture->width(),
                                     SkIntToScalar(tileSize.height()) / fPicture->height());
 
-    if (!fCachedShader || tileScale != fCachedTileScale) {
+    SK_DECLARE_STATIC_MUTEX(cachedShaderMutex);

[scroggo, 2014/04/11 15:51:45]

Why is this static?

[Dominik Grewe, 2014/04/11 17:08:22]

Good point. It should be a regular class member.

+    SkAutoMutexAcquire ama(cachedShaderMutex);

[scroggo, 2014/04/11 15:51:45]

It seems like we're doing an awful lot inside this mutex. What if we created the
shader outside the mutex, and only cached it inside the mutex?

Here are the pros and cons of my approach, as far as I can tell (perhaps there
are more):
PRO:
  - If two threads want to create different shaders, the second thread won't
have
    to wait as long to create its own.
CON:
  - If two threads want to create identical shaders, we could end up making each
    thread create the same one.

Maybe the right choice depends on whether we expect two threads drawing the
picture shader simultaneously to be drawing with the same tile size/local matrix
or not.

[Dominik Grewe, 2014/04/11 17:08:22]

We always have to lock though before we access the cached shader, even if we
don't modify it. Otherwise we may think that we don't need to modify the shader,
but before reffing and returning it it's already been changed. So we'd need two
locks: one for the early out, another for overwriting the shader.

So I'm not sure either which would be faster. Like you said, probably depends on
how it's being used.

+
+    if (!fCachedBitmapShader || tileScale != fCachedTileScale) {

[scroggo, 2014/04/11 15:51:45]

Sort of tied into my last comment, if the cached shader has already been
created, and thread A needs to make no modifications to it, it still locks the
mutex, computes the new (potentially the same) matrix and sets it before
returning. Meanwhile thread B could be waiting at the mutex when it also needed
to make no changes.

[Dominik Grewe, 2014/04/11 17:08:22]

I didn't even realize the setLocalMatrix below... If we change the matrix, all
other references to the cached shader become invalid, right? So we need to
create a new shader each time the shaderMatrix changes, don't we?

[scroggo, 2014/04/11 17:51:36]

Yes. It may make more sense to make that change along with the overall
setLocalMatrix changes. That said, we should probably add a TODO.

         SkBitmap bm;
         if (!bm.allocN32Pixels(tileSize.width(), tileSize.height())) {
-            return false;
+            return NULL;
         }
         bm.eraseColor(SK_ColorTRANSPARENT);
 
         SkCanvas canvas(bm);
         canvas.scale(tileScale.width(), tileScale.height());
         canvas.drawPicture(*fPicture);
 
-        fCachedShader.reset(CreateBitmapShader(bm, fTmx, fTmy));
+        fCachedBitmapShader.reset(CreateBitmapShader(bm, fTmx, fTmy));
         fCachedTileScale = tileScale;
     }
 
     SkMatrix shaderMatrix = this->getLocalMatrix();
     shaderMatrix.preScale(1 / tileScale.width(), 1 / tileScale.height());
-    fCachedShader->setLocalMatrix(shaderMatrix);
+    fCachedBitmapShader->setLocalMatrix(shaderMatrix);
 
-    return true;
+    fCachedBitmapShader.get()->ref();

[scroggo, 2014/04/11 15:51:45]

Typically the caller refs the object that gets returned. Is this done here so it
can be done inside the mutex?

If we do need to go against the typical usage pattern, there should be a comment
explaining why.

[Dominik Grewe, 2014/04/11 17:08:22]

Yes, we have to do it inside the mutex cause otherwise the shader may not exist
any more when we try to ref it. Will add a comment.

+    return fCachedBitmapShader;
 }
 
-bool SkPictureShader::setContext(const SkBitmap& device,
-                                 const SkPaint& paint,
-                                 const SkMatrix& matrix) {
-    if (!this->buildBitmapShader(matrix)) {
-        return false;
+SkShader* SkPictureShader::validInternal(const SkBitmap& device, const SkPaint& paint,
+                                         const SkMatrix& matrix, SkMatrix* totalInverse) const {
+    if (!this->INHERITED::validContext(device, paint, matrix, totalInverse)) {
+        return NULL;
     }
 
-    if (!this->INHERITED::setContext(device, paint, matrix)) {
-        return false;
+    SkShader* bitmapShader = this->buildBitmapShader(matrix);
+    if (!bitmapShader) {
+        return NULL;
     }
 
-    SkASSERT(fCachedShader);
-    if (!fCachedShader->setContext(device, paint, matrix)) {
-        this->INHERITED::endContext();
-        return false;
+    if (!bitmapShader->validContext(device, paint, matrix)) {
+        bitmapShader->unref();
+        return NULL;
     }
 
-    return true;
+    return bitmapShader;
 }
 
-void SkPictureShader::endContext() {
-    SkASSERT(fCachedShader);
-    fCachedShader->endContext();
-
-    this->INHERITED::endContext();
+bool SkPictureShader::validContext(const SkBitmap& device, const SkPaint& paint,
+                                   const SkMatrix& matrix, SkMatrix* totalInverse) const {
+    SkAutoTUnref<SkShader> shader(this->validInternal(device, paint, matrix, totalInverse));
+    return shader != NULL;
 }
 
-uint32_t SkPictureShader::getFlags() {
-    if (NULL != fCachedShader) {
-        return fCachedShader->getFlags();
+SkShader::Context* SkPictureShader::createContext(const SkBitmap& device, const SkPaint& paint,
+                                                  const SkMatrix& matrix, void* storage) const {
+    SkShader* bitmapShader = this->validInternal(device, paint, matrix, NULL);
+    if (!bitmapShader) {
+        return NULL;
+    }
+
+    return SkNEW_PLACEMENT_ARGS(storage, PictureShaderContext,
+                                (*this, device, paint, matrix, bitmapShader));
+}
+
+size_t SkPictureShader::contextSize() const {
+    return sizeof(PictureShaderContext);
+}
+
+SkPictureShader::PictureShaderContext::PictureShaderContext(
+        const SkPictureShader& shader, const SkBitmap& device,
+        const SkPaint& paint, const SkMatrix& matrix, SkShader* bitmapShader)
+    : INHERITED(shader, device, paint, matrix)
+    , fBitmapShader(bitmapShader)
+{
+    // TODO(dominikg): Can we avoid heap alloc here and instead put it in the same
+    //                 storage as this? The problem is that SkPicutreShader::contextSize()
+    //                 doesn't know the size of the bitmap shader's context.

[Dominik Grewe, 2014/04/11 12:39:52]

Any ideas?

[scroggo, 2014/04/11 15:51:45]

No. I guess we don't know because we don't what type of shader will be returned
by CreateBitmapShader?

[Dominik Grewe, 2014/04/11 17:08:22]

Yeah, we could hard-code a maximum size, but that's ugly. Just gonna keep the
malloc here then.

+    fBitmapShaderContextStorage = sk_malloc_throw(fBitmapShader->contextSize());
+    fBitmapShaderContext = fBitmapShader->createContext(
+            device, paint, matrix, fBitmapShaderContextStorage);
+    SkASSERT(fBitmapShaderContext);
+}
+
+SkPictureShader::PictureShaderContext::~PictureShaderContext() {
+    fBitmapShaderContext->SkShader::Context::~Context();
+    sk_free(fBitmapShaderContextStorage);
+}
+
+uint32_t SkPictureShader::PictureShaderContext::getFlags() {
+    if (fBitmapShaderContext) {

[scroggo, 2014/04/11 15:51:45]

I don't think this can ever be NULL, since it's created in the constructor (and
we assert that it's non NULL), and it's deleted in the destructor.

[Dominik Grewe, 2014/04/11 17:08:22]

Done.

+        return fBitmapShaderContext->getFlags();
     }
     return 0;
 }
 
-SkShader::ShadeProc SkPictureShader::asAShadeProc(void** ctx) {
-    if (fCachedShader) {
-        return fCachedShader->asAShadeProc(ctx);
+SkShader::Context::ShadeProc SkPictureShader::PictureShaderContext::asAShadeProc(void** ctx) {
+    if (fBitmapShaderContext) {

[scroggo, 2014/04/11 15:51:45]

This should never be NULL.

[Dominik Grewe, 2014/04/11 17:08:22]

Done.

+        return fBitmapShaderContext->asAShadeProc(ctx);
     }
     return NULL;
 }
 
-void SkPictureShader::shadeSpan(int x, int y, SkPMColor dstC[], int count) {
-    SkASSERT(fCachedShader);
-    fCachedShader->shadeSpan(x, y, dstC, count);
+void SkPictureShader::PictureShaderContext::shadeSpan(int x, int y, SkPMColor dstC[], int count) {
+    SkASSERT(fBitmapShaderContext);
+    fBitmapShaderContext->shadeSpan(x, y, dstC, count);
 }
 
-void SkPictureShader::shadeSpan16(int x, int y, uint16_t dstC[], int count) {
-    SkASSERT(fCachedShader);
-    fCachedShader->shadeSpan16(x, y, dstC, count);
+void SkPictureShader::PictureShaderContext::shadeSpan16(int x, int y, uint16_t dstC[], int count) {
+    SkASSERT(fBitmapShaderContext);
+    fBitmapShaderContext->shadeSpan16(x, y, dstC, count);
 }
 
 #ifndef SK_IGNORE_TO_STRING
 void SkPictureShader::toString(SkString* str) const {
     static const char* gTileModeName[SkShader::kTileModeCount] = {
         "clamp", "repeat", "mirror"
     };
 
     str->appendf("PictureShader: [%d:%d] ",
                  fPicture ? fPicture->width() : 0,
                  fPicture ? fPicture->height() : 0);
 
     str->appendf("(%s, %s)", gTileModeName[fTmx], gTileModeName[fTmy]);
 
     this->INHERITED::toString(str);
 }
 #endif
 
 #if SK_SUPPORT_GPU
 GrEffectRef* SkPictureShader::asNewEffect(GrContext* context, const SkPaint& paint) const {
-    if (!this->buildBitmapShader(context->getMatrix())) {
+    SkAutoTUnref<SkShader> bitmapShader(this->buildBitmapShader(context->getMatrix()));
+    if (!bitmapShader) {
         return NULL;
     }
-    SkASSERT(fCachedShader);
-    return fCachedShader->asNewEffect(context, paint);
+    return bitmapShader->asNewEffect(context, paint);
 }
 #endif