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

src/core/SkPictureShader.cpp

Index: src/core/SkPictureShader.cpp
diff --git a/src/core/SkPictureShader.cpp b/src/core/SkPictureShader.cpp
index 15df3a37a587f253f0634558517a09e141bce63a..98669ac4f39734ea6fdc3bd584eaf2633e7ac437 100644
--- a/src/core/SkPictureShader.cpp
+++ b/src/core/SkPictureShader.cpp
@@ -55,9 +55,9 @@ void SkPictureShader::flatten(SkWriteBuffer& buffer) const {
     }
 }
 
-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;
@@ -78,17 +78,20 @@ bool SkPictureShader::buildBitmapShader(const SkMatrix& matrix) const {
 
     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);
 
@@ -96,66 +99,100 @@ bool SkPictureShader::buildBitmapShader(const SkMatrix& matrix) const {
         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;
+    }
+
+    SkShader* bitmapShader = this->buildBitmapShader(matrix);
+    if (!bitmapShader) {
+        return NULL;
     }
 
-    if (!this->INHERITED::setContext(device, paint, matrix)) {
-        return false;
+    if (!bitmapShader->validContext(device, paint, matrix)) {
+        bitmapShader->unref();
+        return NULL;
     }
 
-    SkASSERT(fCachedShader);
-    if (!fCachedShader->setContext(device, paint, matrix)) {
-        this->INHERITED::endContext();
-        return false;
+    return bitmapShader;
+}
+
+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;
+}
+
+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 true;
+    return SkNEW_PLACEMENT_ARGS(storage, PictureShaderContext,
+                                (*this, device, paint, matrix, bitmapShader));
+}
+
+size_t SkPictureShader::contextSize() const {
+    return sizeof(PictureShaderContext);
 }
 
-void SkPictureShader::endContext() {
-    SkASSERT(fCachedShader);
-    fCachedShader->endContext();
+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);
+}
 
-    this->INHERITED::endContext();
+SkPictureShader::PictureShaderContext::~PictureShaderContext() {
+    fBitmapShaderContext->SkShader::Context::~Context();
+    sk_free(fBitmapShaderContextStorage);
 }
 
-uint32_t SkPictureShader::getFlags() {
-    if (NULL != fCachedShader) {
-        return fCachedShader->getFlags();
+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
@@ -176,10 +213,10 @@ void SkPictureShader::toString(SkString* str) const {
 
 #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