start to remove lockPixels from bitmapshader

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 "SkFlattenableBuffers.h"
 #include "SkPixelRef.h"
@@ skipping 62 matching lines @@
 
 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 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) {
+    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;
+    }
+
     // do this first, so we have a correct inverse matrix
     if (!this->INHERITED::setContext(device, paint, matrix)) {
         return false;
     }
 
     fState.fOrigBitmap = fRawBitmap;
-    fState.fOrigBitmap.lockPixels();
-    if (!fState.fOrigBitmap.getTexture() && !fState.fOrigBitmap.readyToDraw()) {
-        fState.fOrigBitmap.unlockPixels();
+    if (!fState.chooseProcs(this->getTotalInverse(), paint)) {
         this->INHERITED::endContext();
         return false;
     }
-
-    if (!fState.chooseProcs(this->getTotalInverse(), paint)) {
-        fState.fOrigBitmap.unlockPixels();
-        this->INHERITED::endContext();
-        return false;
-    }
 
     const SkBitmap& bitmap = *fState.fBitmap;
     bool bitmapIsOpaque = bitmap.isOpaque();
 
     // update fFlags
     uint32_t flags = 0;
     if (bitmapIsOpaque && (255 == this->getPaintAlpha())) {
         flags |= kOpaqueAlpha_Flag;
     }
 
@@ skipping 26 matching lines @@
         if (flags & kHasSpan16_Flag) {
             flags |= kConstInY16_Flag;
         }
     }
 
     fFlags = flags;
     return true;
 }
 
 void SkBitmapProcShader::endContext() {
-    fState.fOrigBitmap.unlockPixels();
     fState.endContext();
     this->INHERITED::endContext();
 }
 
 #define BUF_MAX     128
 
 #define TEST_BUFFER_OVERRITEx
 
 #ifdef TEST_BUFFER_OVERRITE
     #define TEST_BUFFER_EXTRA   32
@@ skipping 236 matching lines @@
     GrEffectRef* effect = NULL;
     if (paintFilterLevel == SkPaint::kHigh_FilterLevel) {
         effect = GrBicubicEffect::Create(texture, matrix, params);
     } else {
         effect = GrSimpleTextureEffect::Create(texture, matrix, params);
     }
     GrUnlockAndUnrefCachedBitmapTexture(texture);
     return effect;
 }
 #endif