start to remove lockPixels from bitmapshader

src/core/SkBitmapProcState.cpp

Index: src/core/SkBitmapProcState.cpp
diff --git a/src/core/SkBitmapProcState.cpp b/src/core/SkBitmapProcState.cpp
index 6ab3126113340a9ff59ef6da4ae048b24ca557f2..174e63eca09bae349665e546f323471b91565f7c 100644
--- a/src/core/SkBitmapProcState.cpp
+++ b/src/core/SkBitmapProcState.cpp
@@ -13,6 +13,7 @@
 #include "SkUtilsArm.h"
 #include "SkBitmapScaler.h"
 #include "SkMipMap.h"
+#include "SkPixelRef.h"
 #include "SkScaledImageCache.h"
 
 #if !SK_ARM_NEON_IS_NONE
@@ -109,15 +110,14 @@ static SkScalar effective_matrix_scale_sqrd(const SkMatrix& mat) {
 // the portion of the image that we're going to need.  This will complicate
 // the interface to the cache, but might be well worth it.
 
-void SkBitmapProcState::possiblyScaleImage() {
+bool SkBitmapProcState::possiblyScaleImage() {
+    SkASSERT(NULL == fBitmap);
+    SkASSERT(NULL == fScaledCacheID);
 
     if (fFilterLevel <= SkPaint::kLow_FilterLevel) {
-        // none or low (bilerp) does not need to look any further
-        return;
+        return false;
     }
 
-    // STEP 1: Highest quality direct scale?
-
     // Check to see if the transformation matrix is simple, and if we're
     // doing high quality scaling.  If so, do the bitmap scale here and
     // remove the scaling component from the matrix.
@@ -129,7 +129,6 @@ void SkBitmapProcState::possiblyScaleImage() {
         SkScalar invScaleX = fInvMatrix.getScaleX();
         SkScalar invScaleY = fInvMatrix.getScaleY();
 
-        SkASSERT(NULL == fScaledCacheID);
         fScaledCacheID = SkScaledImageCache::FindAndLock(fOrigBitmap,
                                                          invScaleX, invScaleY,
                                                          &fScaledBitmap);
@@ -151,7 +150,7 @@ void SkBitmapProcState::possiblyScaleImage() {
                                         simd)) {
                 // we failed to create fScaledBitmap, so just return and let
                 // the scanline proc handle it.
-                return;
+                return true;
 
             }
             fScaledCacheID = SkScaledImageCache::AddAndLock(fOrigBitmap,
@@ -159,25 +158,19 @@ void SkBitmapProcState::possiblyScaleImage() {
                                                             invScaleY,
                                                             fScaledBitmap);
         }
-        fScaledBitmap.lockPixels();
-
+        fScaledBitmap.lockPixels(); // wonder if Resize() should have locked this
         fBitmap = &fScaledBitmap;
 
         // set the inv matrix type to translate-only;
-
         fInvMatrix.setTranslate(fInvMatrix.getTranslateX() / fInvMatrix.getScaleX(),
                                 fInvMatrix.getTranslateY() / fInvMatrix.getScaleY());
 
         // no need for any further filtering; we just did it!
-
         fFilterLevel = SkPaint::kNone_FilterLevel;
-
-        return;
+        return true;
     }
 
     /*
-     *  If we get here, the caller has requested either Med or High filter-level

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

Do these comments no longer apply?

-     *
      *  If High, then our special-case for scale-only did not take, and so we
      *  have to make a choice:
      *      1. fall back on mipmaps + bilerp
@@ -202,7 +195,7 @@ void SkBitmapProcState::possiblyScaleImage() {
         const SkScalar bicubicLimit = SkFloatToScalar(4.0f);
         const SkScalar bicubicLimitSqd = bicubicLimit * bicubicLimit;
         if (scaleSqd < bicubicLimitSqd) {  // use bicubic scanline
-            return;
+            return false;
         }
 
         // else set the filter-level to Medium, since we're scaling down and
@@ -247,15 +240,54 @@ void SkBitmapProcState::possiblyScaleImage() {
                                         level.fRowBytes);
                 fScaledBitmap.setPixels(level.fPixels);
                 fBitmap = &fScaledBitmap;
+                fFilterLevel = SkPaint::kLow_FilterLevel;
+                return true;
             }
         }
     }
 
-    /*
-     *  At this point, we may or may not have built a mipmap. Regardless, we
-     *  now fall back on Low so will bilerp whatever fBitmap now points at.
-     */
-    fFilterLevel = SkPaint::kLow_FilterLevel;

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

This appears to be a change in behavior: if !mip || scaleSqd >= SK_Scalar1, we
no longer set fFilterLevel to SkPaint::kLow_FilterLevel. Is this intended?

+    return false;
+}
+
+static bool decodeInto(const SkBitmap& src, int pow2, SkBitmap* dst) {

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

Style nit: decode_into

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

Done.

+    // new way to decodeInto...

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

Should there be some more explicit comments, eg FIXME, in progress etc?

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

Done.

+    SkPixelRef* pr = src.pixelRef();
+    if (pr && pr->decodeInto(pow2, dst)) {
+        return true;
+    }
+
+    // olde school impl... ignores pow2
+    src.lockPixels();
+    if (!src.getPixels()) {
+        return false;
+    }
+    *dst = src;
+    dst->lockPixels();
+    src.unlockPixels();
+    return SkToBool(dst->getPixels());
+}
+
+bool SkBitmapProcState::lockBaseBitmap() {

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

Will this function become more general to lock other versions besides the base
bitmap?

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

I think not. I think that functionality belongs in possiblyScaleBitmap, where we
worry about mipmaps etc.

+    fScaledCacheID = SkScaledImageCache::FindAndLock(fOrigBitmap,
+                                                     SK_Scalar1, SK_Scalar1,
+                                                     &fScaledBitmap);
+    if (NULL == fScaledCacheID) {
+        if (!decodeInto(fOrigBitmap, 0, &fScaledBitmap)) {
+            return false;
+        }
+
+        fScaledCacheID = SkScaledImageCache::AddAndLock(fOrigBitmap,
+                                                        SK_Scalar1, SK_Scalar1,
+                                                        fScaledBitmap);
+        if (!fScaledCacheID) {
+            fScaledBitmap.reset();
+            return false;
+        }
+    }
+
+    fScaledBitmap.lockPixels(); // just 'cause the cache made a copy :(
+    fBitmap = &fScaledBitmap;
+    return true;
 }
 
 void SkBitmapProcState::endContext() {
@@ -277,17 +309,10 @@ SkBitmapProcState::~SkBitmapProcState() {
 }
 
 bool SkBitmapProcState::chooseProcs(const SkMatrix& inv, const SkPaint& paint) {
-    if (fOrigBitmap.width() == 0 || fOrigBitmap.height() == 0) {
-        return false;
-    }
+    SkASSERT(fOrigBitmap.width() && fOrigBitmap.height());
 
-    fBitmap = &fOrigBitmap;
+    fBitmap = NULL;
     fInvMatrix = inv;
-
-    // initialize our filter quality to the one requested by the caller.
-    // We may downgrade it later if we determine that we either don't need
-    // or can't provide as high a quality filtering as the user requested.
-
     fFilterLevel = paint.getFilterLevel();
 
     // possiblyScaleImage will look to see if it can rescale the image as a
@@ -295,8 +320,13 @@ bool SkBitmapProcState::chooseProcs(const SkMatrix& inv, const SkPaint& paint) {
     // a nearby mipmap level.  If it does, it will adjust the working
     // matrix as well as the working bitmap.  It may also adjust the filter
     // quality to avoid re-filtering an already perfectly scaled image.
-
-    this->possiblyScaleImage();
+    if (!this->possiblyScaleImage()) {
+        if (!this->lockBaseBitmap()) {
+            return false;
+        }
+    }
+    
+    SkASSERT(fBitmap);
 
     bool trivialMatrix = (fInvMatrix.getType() & ~SkMatrix::kTranslate_Mask) == 0;
     bool clampClamp = SkShader::kClamp_TileMode == fTileModeX &&
@@ -322,7 +352,6 @@ bool SkBitmapProcState::chooseProcs(const SkMatrix& inv, const SkPaint& paint) {
                 SkScalar tx = -SkScalarRoundToScalar(forward.getTranslateX());
                 SkScalar ty = -SkScalarRoundToScalar(forward.getTranslateY());
                 fInvMatrix.setTranslate(tx, ty);
-
             }
         }
     }