Work (in progress) to make SkShader immutable.

src/core/SkShader.cpp

Index: src/core/SkShader.cpp
diff --git a/src/core/SkShader.cpp b/src/core/SkShader.cpp
index b06e0c26ff6391e3125d21d847746b05d76c7aa0..29757306840dcc29986306ad2b19122d5ba0229e 100644
--- a/src/core/SkShader.cpp
+++ b/src/core/SkShader.cpp
@@ -13,27 +13,23 @@
 #include "SkShader.h"
 #include "SkWriteBuffer.h"
 
-SkShader::SkShader() {
+SkShaderGenerator::SkShaderGenerator() {
     fLocalMatrix.reset();
-    SkDEBUGCODE(fInSetContext = false;)
 }
 
-SkShader::SkShader(SkReadBuffer& buffer)
+SkShaderGenerator::SkShaderGenerator(SkReadBuffer& buffer)
         : INHERITED(buffer) {
     if (buffer.readBool()) {
         buffer.readMatrix(&fLocalMatrix);
     } else {
         fLocalMatrix.reset();
     }
-
-    SkDEBUGCODE(fInSetContext = false;)
 }
 
-SkShader::~SkShader() {
-    SkASSERT(!fInSetContext);
+SkShaderGenerator::~SkShaderGenerator() {
 }
 
-void SkShader::flatten(SkWriteBuffer& buffer) const {
+void SkShaderGenerator::flatten(SkWriteBuffer& buffer) const {
     this->INHERITED::flatten(buffer);
     bool hasLocalM = this->hasLocalMatrix();
     buffer.writeBool(hasLocalM);
@@ -42,39 +38,51 @@ void SkShader::flatten(SkWriteBuffer& buffer) const {
     }
 }
 
-bool SkShader::setContext(const SkBitmap& device,
-                          const SkPaint& paint,
-                          const SkMatrix& matrix) {
-    SkASSERT(!this->setContextHasBeenCalled());
-
+bool SkShaderGenerator::validContext(const SkBitmap& device,
+                                     const SkPaint& paint,
+                                     const SkMatrix& matrix) const
+{
     const SkMatrix* m = &matrix;
     SkMatrix        total;
 
-    fPaintAlpha = paint.getAlpha();
     if (this->hasLocalMatrix()) {
         total.setConcat(matrix, this->getLocalMatrix());
         m = &total;
     }
     if (m->invert(&fTotalInverse)) {
         fTotalInverseClass = (uint8_t)ComputeMatrixClass(fTotalInverse);

[Dominik Grewe, 2014/03/14 16:00:40]

These variables should be temporary rather than member variables right?

[scroggo, 2014/03/17 15:11:56]

Oh yes, of course.

-        SkDEBUGCODE(fInSetContext = true;)
         return true;
     }
     return false;
 }
 
-void SkShader::endContext() {
-    SkASSERT(fInSetContext);
-    SkDEBUGCODE(fInSetContext = false;)
+SkShaderGenerator::ShaderImpl::ShaderImpl(
+        const SkShaderGenerator& shader, const SkBitmap& unused,
+        const SkPaint& paint const SkMatrix& matrix)
+    : fShader(shader)
+{
+    SkASSERT(fShader.validContext(device, paint, matrix));
+
+    const SkMatrix* m = &matrix;
+    SkMatrix        total;
+
+    fPaintAlpha = paint.getAlpha();
+    if (fShader.hasLocalMatrix()) {
+        total.setConcat(matrix, fShader.getLocalMatrix());
+        m = &total;
+    }
+    // If this fails, SkShaderGenerator::validContext was not called.
+    SkAssertResult(m->invert(&fTotalInverse));
+    fTotalInverseClass = (uint8_t)ComputeMatrixClass(fTotalInverse);
 }
 
-SkShader::ShadeProc SkShader::asAShadeProc(void** ctx) {
+SkShaderGenerator::ShadeProc SkShaderGenerator::asAShadeProc(void** ctx) {
     return NULL;
 }
 
 #include "SkColorPriv.h"
 
-void SkShader::shadeSpan16(int x, int y, uint16_t span16[], int count) {
+void ShaderImpl::shadeSpan16(int x, int y, uint16_t span16[], int count) {
     SkASSERT(span16);
     SkASSERT(count > 0);
     SkASSERT(this->canCallShadeSpan16());
@@ -92,7 +100,7 @@ void SkShader::shadeSpan16(int x, int y, uint16_t span16[], int count) {
     #define SkU32BitShiftToByteOffset(shift)    ((shift) >> 3)
 #endif
 
-void SkShader::shadeSpanAlpha(int x, int y, uint8_t alpha[], int count) {
+void ShaderImpl::shadeSpanAlpha(int x, int y, uint8_t alpha[], int count) {
     SkASSERT(count > 0);
 
     SkPMColor   colors[kTempColorCount];
@@ -146,7 +154,8 @@ void SkShader::shadeSpanAlpha(int x, int y, uint8_t alpha[], int count) {
 #endif
 }
 
-SkShader::MatrixClass SkShader::ComputeMatrixClass(const SkMatrix& mat) {
+SkShaderGenerator::ShaderImpl::MatrixClass
+SkShaderGenerator::ShaderImpl::ComputeMatrixClass(const SkMatrix& mat) {
     MatrixClass mc = kLinear_MatrixClass;
 
     if (mat.hasPerspective()) {
@@ -161,26 +170,26 @@ SkShader::MatrixClass SkShader::ComputeMatrixClass(const SkMatrix& mat) {
 
 //////////////////////////////////////////////////////////////////////////////
 
-SkShader::BitmapType SkShader::asABitmap(SkBitmap*, SkMatrix*,
+SkShaderGenerator::BitmapType SkShaderGenerator::asABitmap(SkBitmap*, SkMatrix*,
                                          TileMode*) const {
     return kNone_BitmapType;
 }
 
-SkShader::GradientType SkShader::asAGradient(GradientInfo* info) const {
+SkShaderGenerator::GradientType SkShaderGenerator::asAGradient(GradientInfo* info) const {
     return kNone_GradientType;
 }
 
-GrEffectRef* SkShader::asNewEffect(GrContext*, const SkPaint&) const {
+GrEffectRef* SkShaderGenerator::asNewEffect(GrContext*, const SkPaint&) const {
     return NULL;
 }
 
-SkShader* SkShader::CreateBitmapShader(const SkBitmap& src,
+SkShaderGenerator* SkShaderGenerator::CreateBitmapShader(const SkBitmap& src,
                                        TileMode tmx, TileMode tmy) {
     return ::CreateBitmapShader(src, tmx, tmy, NULL);
 }
 
 #ifdef SK_DEVELOPER
-void SkShader::toString(SkString* str) const {
+void SkShaderGenerator::toString(SkString* str) const {
     if (this->hasLocalMatrix()) {
         str->append(" ");
         this->getLocalMatrix().toString(str);
@@ -193,19 +202,16 @@ void SkShader::toString(SkString* str) const {
 #include "SkColorShader.h"
 #include "SkUtils.h"
 
-SkColorShader::SkColorShader() {
-    fFlags = 0;
-    fInheritColor = true;
+SkColorShader::SkColorShader()
+    : fColor()
+    , fInheritColor(true) {
 }
 
-SkColorShader::SkColorShader(SkColor c) {
-    fFlags = 0;
-    fColor = c;
-    fInheritColor = false;
+SkColorShader::SkColorShader(SkColor c)
+    : fColor(c)
+    , fInheritColor(false) {
 }
 
-SkColorShader::~SkColorShader() {}
-
 bool SkColorShader::isOpaque() const {
     if (fInheritColor) {
         return true; // using paint's alpha
@@ -213,14 +219,11 @@ bool SkColorShader::isOpaque() const {
     return SkColorGetA(fColor) == 255;
 }
 
-SkColorShader::SkColorShader(SkReadBuffer& b) : INHERITED(b) {
-    fFlags = 0; // computed in setContext
-
-    fInheritColor = b.readBool();
-    if (fInheritColor) {
-        return;
+SkColorShader* SkColorShader::CreateProc(SkReadBuffer& b) {
+    if (b.readBool()) {
+        return SkNEW(SkColorShader);
     }
-    fColor = b.readColor();
+    return SkNEW_ARGS(SkColorShader, (b.readColor()));
 }
 
 void SkColorShader::flatten(SkWriteBuffer& buffer) const {
@@ -232,32 +235,42 @@ void SkColorShader::flatten(SkWriteBuffer& buffer) const {
     buffer.writeColor(fColor);
 }
 
-uint32_t SkColorShader::getFlags() {
+uint32_t SkColorShader::CSImpl::getFlags() {
     return fFlags;
 }
 
-uint8_t SkColorShader::getSpan16Alpha() const {
+uint8_t SkColorShader::CSImpl::getSpan16Alpha() const {
     return SkGetPackedA32(fPMColor);
 }
 
-bool SkColorShader::setContext(const SkBitmap& device, const SkPaint& paint,
-                               const SkMatrix& matrix) {
-    if (!this->INHERITED::setContext(device, paint, matrix)) {
-        return false;
-    }
+SkColorShader::CSImpl* SkColorShader::createShaderImpl(const SkBitmap& device,
+                                                       const SkPaint& paint,
+                                                       const SkMatrix& matrix,
+                                                       void* storage) const
+{
+    SkASSERT(this->contextValid(device, paint, matrix));

[Dominik Grewe, 2014/03/14 16:00:40]

Could we just call validContext() here and return NULL if it returns false?

[scroggo, 2014/03/17 15:11:56]

The problem with that is that SkSmallAllocator has no way of knowing that you
did not create an object in the space provided by reserveT, so it will still
call the destructor. One hacky way to solve that is if this function returns
NULL to create a dummy one to call the destructor on. Or maybe SkSmallAllocator
needs a way to just tell it to not call the destructor. That may not be too bad.

[Dominik Grewe, 2014/03/17 16:07:07]

Good point. Not sure which way is best. If we have a reserveT that doesn't call
a constructor maybe it's okay to add a freeT that makes sure the destructor
isn't called, as you suggested.

+
+    return SkNEW_ARGS(SkColorShader::CSImpl, (*this, device, paint, matrix));
+}
 
+SkColorShader::CSImpl::CSImpl(const SkColorShader& shader, const SkBitmap& device,
+                              const SkPaint& paint, const SkMatrix& matrix)
+    : INHERITED(shader, device, paint, matrix)
+{
     unsigned a;
 
-    if (fInheritColor) {
-        fColor = paint.getColor();
-        a = SkColorGetA(fColor);
+    SkColor color
+    if (shader.fInheritColor) {
+        color = paint.getColor();
+        a = SkColorGetA(color);
     } else {
-        a = SkAlphaMul(SkColorGetA(fColor), SkAlpha255To256(paint.getAlpha()));
+        color = shader.fColor;
+        a = SkAlphaMul(SkColorGetA(color), SkAlpha255To256(paint.getAlpha()));
     }
 
-    unsigned r = SkColorGetR(fColor);
-    unsigned g = SkColorGetG(fColor);
-    unsigned b = SkColorGetB(fColor);
+    unsigned r = SkColorGetR(color);
+    unsigned g = SkColorGetG(color);
+    unsigned b = SkColorGetB(cnolor);
 
     // we want this before we apply any alpha
     fColor16 = SkPack888ToRGB16(r, g, b);
@@ -280,31 +293,31 @@ bool SkColorShader::setContext(const SkBitmap& device, const SkPaint& paint,
     return true;
 }
 
-void SkColorShader::shadeSpan(int x, int y, SkPMColor span[], int count) {
+void SkColorShader::CSImpl::shadeSpan(int x, int y, SkPMColor span[], int count) {
     sk_memset32(span, fPMColor, count);
 }
 
-void SkColorShader::shadeSpan16(int x, int y, uint16_t span[], int count) {
+void SkColorShader::CSImpl::shadeSpan16(int x, int y, uint16_t span[], int count) {
     sk_memset16(span, fColor16, count);
 }
 
-void SkColorShader::shadeSpanAlpha(int x, int y, uint8_t alpha[], int count) {
+void SkColorShader::CSImpl::shadeSpanAlpha(int x, int y, uint8_t alpha[], int count) {
     memset(alpha, SkGetPackedA32(fPMColor), count);
 }
 
 // if we had a asAColor method, that would be more efficient...
-SkShader::BitmapType SkColorShader::asABitmap(SkBitmap* bitmap, SkMatrix* matrix,
+SkShaderGenerator::BitmapType SkColorShader::asABitmap(SkBitmap* bitmap, SkMatrix* matrix,
                                               TileMode modes[]) const {
     return kNone_BitmapType;
 }
 
-SkShader::GradientType SkColorShader::asAGradient(GradientInfo* info) const {
+SkShaderGenerator::GradientType SkColorShader::asAGradient(GradientInfo* info) const {
     if (info) {
         if (info->fColors && info->fColorCount >= 1) {
             info->fColors[0] = fColor;
         }
         info->fColorCount = 1;
-        info->fTileMode = SkShader::kRepeat_TileMode;
+        info->fTileMode = SkShaderGenerator::kRepeat_TileMode;
     }
     return kColor_GradientType;
 }
@@ -330,24 +343,6 @@ void SkColorShader::toString(SkString* str) const {
 
 #include "SkEmptyShader.h"
 
-uint32_t SkEmptyShader::getFlags() { return 0; }
-uint8_t SkEmptyShader::getSpan16Alpha() const { return 0; }
-
-bool SkEmptyShader::setContext(const SkBitmap&, const SkPaint&,
-                               const SkMatrix&) { return false; }
-
-void SkEmptyShader::shadeSpan(int x, int y, SkPMColor span[], int count) {
-    SkDEBUGFAIL("should never get called, since setContext() returned false");
-}
-
-void SkEmptyShader::shadeSpan16(int x, int y, uint16_t span[], int count) {
-    SkDEBUGFAIL("should never get called, since setContext() returned false");
-}
-
-void SkEmptyShader::shadeSpanAlpha(int x, int y, uint8_t alpha[], int count) {
-    SkDEBUGFAIL("should never get called, since setContext() returned false");
-}
-
 #ifdef SK_DEVELOPER
 void SkEmptyShader::toString(SkString* str) const {
     str->append("SkEmptyShader: (");