start to remove lockPixels from bitmapshader

src/core/SkBitmapProcShader.cpp

Index: src/core/SkBitmapProcShader.cpp
diff --git a/src/core/SkBitmapProcShader.cpp b/src/core/SkBitmapProcShader.cpp
index 3f657156f4f6f1d6333584a70e88905968d55167..e9d19e4d90c2f91b2b967e09f71ee0f67a287292 100644
--- a/src/core/SkBitmapProcShader.cpp
+++ b/src/core/SkBitmapProcShader.cpp
@@ -80,24 +80,39 @@ bool SkBitmapProcShader::isOpaque() const {
     return fRawBitmap.isOpaque();
 }
 
+static bool validForDrawing(const SkBitmap& bm) {

[scroggo, 2013/09/10 18:08:31]

Style nit: valid_for_drawing

[reed1, 2013/09/10 20:28:11]

Done.

+    if (0 == bm.width() || 0 == bm.height()) {
+        return false;   // nothing to draw
+    }
+    if (NULL == bm.pixelRef()) {
+        return false;   // no pixels to read
+    }
+    if (SkBitmap::kIndex8_Config == bm.config()) {
+        // ugh, I have to lock-pixels to inspect the colortable
+        bm.lockPixels();

[scroggo, 2013/09/10 18:08:31]

AutoLockPixels?

[reed1, 2013/09/10 20:28:11]

Done.

+        SkColorTable* ct = bm.getColorTable();
+        bm.unlockPixels();
+        if (!ct) {
+            return false;
+        }
+    }
+    return true;
+}
+
 bool SkBitmapProcShader::setContext(const SkBitmap& device,
                                     const SkPaint& paint,
                                     const SkMatrix& matrix) {
-    // do this first, so we have a correct inverse matrix
-    if (!this->INHERITED::setContext(device, paint, matrix)) {
+    if (!fRawBitmap.getTexture() && !validForDrawing(fRawBitmap)) {

[scroggo, 2013/09/10 18:08:31]

Should valid_for_drawing check bitmap.getTexture()? It seems like a logical
piece of being valid for drawing.

If we want to take it a bit further, should valid_for_drawing be folded into
SkBitmap::readyToDraw?

[reed1, 2013/09/10 20:28:11]

Yes, though I don't see now why we could work with a texture?
Ready to draw today presumes that we *have* called lockPixels (in that it looks
get getPixels()). I am explicitly trying to *not* call lockPixels at this point,
since I don't know the CTM. Perhaps if this no-need-to-lock idea propagates more
widely, we can fold it into that method.

         return false;
     }
 
-    fState.fOrigBitmap = fRawBitmap;
-    fState.fOrigBitmap.lockPixels();
-    if (!fState.fOrigBitmap.getTexture() && !fState.fOrigBitmap.readyToDraw()) {
-        fState.fOrigBitmap.unlockPixels();
-        this->INHERITED::endContext();
+    // do this first, so we have a correct inverse matrix
+    if (!this->INHERITED::setContext(device, paint, matrix)) {
         return false;
     }
 
+    fState.fOrigBitmap = fRawBitmap;
     if (!fState.chooseProcs(this->getTotalInverse(), paint)) {
-        fState.fOrigBitmap.unlockPixels();
         this->INHERITED::endContext();
         return false;
     }
@@ -147,7 +162,6 @@ bool SkBitmapProcShader::setContext(const SkBitmap& device,
 }
 
 void SkBitmapProcShader::endContext() {
-    fState.fOrigBitmap.unlockPixels();
     fState.endContext();
     this->INHERITED::endContext();
 }