lock pixels in image when bitmap is immutable and not-lazy

BUG=skia:

Review URL: https://codereview.chromium.org/1266143003

9 files changed, 16 insertions, 32 deletions

diff --git a/include/core/SkPixelRef.h b/include/core/SkPixelRef.h
index a67855de5f..1dc01f79ab 100644
--- a/include/core/SkPixelRef.h
+++ b/include/core/SkPixelRef.h
@@ -19,20 +19,6 @@
 #include "SkString.h"
 #include "SkTDArray.h"
 
-#ifdef SK_DEBUG
-    /**
-     *  Defining SK_IGNORE_PIXELREF_SETPRELOCKED will force all pixelref
-     *  subclasses to correctly handle lock/unlock pixels. For performance
-     *  reasons, simple malloc-based subclasses call setPreLocked() to skip
-     *  the overhead of implementing these calls.
-     *
-     *  This build-flag disables that optimization, to add in debugging our
-     *  call-sites, to ensure that they correctly balance their calls of
-     *  lock and unlock.
-     */
-//    #define SK_IGNORE_PIXELREF_SETPRELOCKED
-#endif
-
 class SkColorTable;
 class SkData;
 struct SkIRect;
@@ -385,6 +371,9 @@ private:
     void restoreMutability();
     friend class SkSurface_Raster;   // For the two methods above.
 
+    bool isPreLocked() const { return fPreLocked; }
+    friend class SkImage_Raster;
+
     // When copying a bitmap to another with the same shape and config, we can safely
     // clone the pixelref generation ID too, which makes them equivalent under caching.
     friend class SkBitmap;  // only for cloneGenID
diff --git a/src/core/SkPixelRef.cpp b/src/core/SkPixelRef.cpp
index 0edad83028..43e2884bdb 100644
--- a/src/core/SkPixelRef.cpp
+++ b/src/core/SkPixelRef.cpp
@@ -173,7 +173,6 @@ static void validate_pixels_ctable(const SkImageInfo& info, const SkColorTable*
 }
 
 void SkPixelRef::setPreLocked(void* pixels, size_t rowBytes, SkColorTable* ctable) {
-#ifndef SK_IGNORE_PIXELREF_SETPRELOCKED
     SkASSERT(pixels);
     validate_pixels_ctable(fInfo, ctable);
     // only call me in your constructor, otherwise fLockCount tracking can get
@@ -183,7 +182,6 @@ void SkPixelRef::setPreLocked(void* pixels, size_t rowBytes, SkColorTable* ctabl
     fRec.fRowBytes = rowBytes;
     fLockCount = SKPIXELREF_PRELOCKED_LOCKCOUNT;
     fPreLocked = true;
-#endif
 }
 
 // Increments fLockCount only on success
diff --git a/src/image/SkImage.cpp b/src/image/SkImage.cpp
index a17f336c9c..241904b3a3 100644
--- a/src/image/SkImage.cpp
+++ b/src/image/SkImage.cpp
@@ -253,7 +253,7 @@ SkImage* SkImage::NewFromBitmap(const SkBitmap& bm) {
 #endif
 
     // This will check for immutable (share or copy)
-    return SkNewImageFromRasterBitmap(bm, nullptr, kUnlocked_SharedPixelRefMode);
+    return SkNewImageFromRasterBitmap(bm, nullptr);
 }
 
 bool SkImage::asLegacyBitmap(SkBitmap* bitmap, LegacyBitmapMode mode) const {
diff --git a/src/image/SkImagePriv.h b/src/image/SkImagePriv.h
index 0626d6a3ce..bc4a7b04ad 100644
--- a/src/image/SkImagePriv.h
+++ b/src/image/SkImagePriv.h
@@ -35,16 +35,12 @@ extern SkImage* SkNewImageFromPixelRef(const SkImageInfo&, SkPixelRef*,
  *  SkImageInfo, or the bitmap's pixels cannot be accessed, this will return
  *  NULL.
  */
-enum SharedPixelRefMode {
-    kLocked_SharedPixelRefMode,
-    kUnlocked_SharedPixelRefMode
-};
 enum ForceCopyMode {
     kNo_ForceCopyMode,
     kYes_ForceCopyMode, // must copy the pixels even if the bitmap is immutable
 };
 extern SkImage* SkNewImageFromRasterBitmap(const SkBitmap&, const SkSurfaceProps*,
-                                           SharedPixelRefMode, ForceCopyMode = kNo_ForceCopyMode);
+                                           ForceCopyMode = kNo_ForceCopyMode);
 
 static inline size_t SkImageMinRowBytes(const SkImageInfo& info) {
     size_t minRB = info.minRowBytes();
diff --git a/src/image/SkImage_Raster.cpp b/src/image/SkImage_Raster.cpp
index 6629593286..120c397e82 100644
--- a/src/image/SkImage_Raster.cpp
+++ b/src/image/SkImage_Raster.cpp
@@ -81,10 +81,13 @@ public:
     bool isOpaque() const override;
     bool onAsLegacyBitmap(SkBitmap*, LegacyBitmapMode) const override;
 
-    SkImage_Raster(const SkBitmap& bm, const SkSurfaceProps* props, bool lockPixels = false)
+    SkImage_Raster(const SkBitmap& bm, const SkSurfaceProps* props)
         : INHERITED(bm.width(), bm.height(), bm.getGenerationID(), props)
-        , fBitmap(bm) {
-        if (lockPixels) {
+        , fBitmap(bm)
+    {
+        if (bm.pixelRef()->isPreLocked()) {
+            // we only preemptively lock if there is no chance of triggering something expensive
+            // like a lazy decode or imagegenerator. PreLocked means it is flat pixels already.
             fBitmap.lockPixels();
         }
         SkASSERT(fBitmap.isImmutable());
@@ -237,7 +240,7 @@ SkImage* SkNewImageFromPixelRef(const SkImageInfo& info, SkPixelRef* pr,
 }
 
 SkImage* SkNewImageFromRasterBitmap(const SkBitmap& bm, const SkSurfaceProps* props,
-                                    SharedPixelRefMode mode, ForceCopyMode forceCopy) {
+                                    ForceCopyMode forceCopy) {
     SkASSERT(NULL == bm.getTexture());
 
     if (!SkImage_Raster::ValidArgs(bm.info(), bm.rowBytes(), NULL, NULL)) {
@@ -258,7 +261,7 @@ SkImage* SkNewImageFromRasterBitmap(const SkBitmap& bm, const SkSurfaceProps* pr
             as_IB(image)->initWithProps(*props);
         }
     } else {
-        image = SkNEW_ARGS(SkImage_Raster, (bm, props, kLocked_SharedPixelRefMode == mode));
+        image = SkNEW_ARGS(SkImage_Raster, (bm, props));
     }
     return image;
 }
diff --git a/src/image/SkSurface_Raster.cpp b/src/image/SkSurface_Raster.cpp
index d0a65530f0..bcf6da7d18 100644
--- a/src/image/SkSurface_Raster.cpp
+++ b/src/image/SkSurface_Raster.cpp
@@ -128,7 +128,7 @@ SkImage* SkSurface_Raster::onNewImageSnapshot(Budgeted) {
     }
     // Our pixels are in memory, so read access on the snapshot SkImage could be cheap.
     // Lock the shared pixel ref to ensure peekPixels() is usable.
-    return SkNewImageFromRasterBitmap(fBitmap, &this->props(), kLocked_SharedPixelRefMode,
+    return SkNewImageFromRasterBitmap(fBitmap, &this->props(),
                                       fWeOwnThePixels ? kNo_ForceCopyMode : kYes_ForceCopyMode);
 }
 
diff --git a/tests/DeferredCanvasTest.cpp b/tests/DeferredCanvasTest.cpp
index 882871fa4d..eb2263ef2b 100644
--- a/tests/DeferredCanvasTest.cpp
+++ b/tests/DeferredCanvasTest.cpp
@@ -64,7 +64,7 @@ public:
     }
 
     SkImage* onNewImageSnapshot(Budgeted) override {
-        return SkNewImageFromRasterBitmap(fBitmap, &this->props(), kUnlocked_SharedPixelRefMode);
+        return SkNewImageFromRasterBitmap(fBitmap, &this->props());
     }
 
     void onCopyOnWrite(ContentChangeMode mode) override {
diff --git a/tests/ImageTest.cpp b/tests/ImageTest.cpp
index 5da8342995..ba071bb1f7 100644
--- a/tests/ImageTest.cpp
+++ b/tests/ImageTest.cpp
@@ -245,9 +245,7 @@ DEF_TEST(image_newfrombitmap, reporter) {
         const bool sharedID = (image->uniqueID() == bm.getGenerationID());
         REPORTER_ASSERT(reporter, sharedID == rec[i].fExpectSharedID);
 
-#if 0   // TODO: fix so that peek will succeed in the immutable case
         const bool peekSuccess = image->peekPixels(&pmap);
         REPORTER_ASSERT(reporter, peekSuccess == rec[i].fExpectPeekSuccess);
-#endif
     }
 }
diff --git a/tests/SkImageTest.cpp b/tests/SkImageTest.cpp
index d5c7eaa819..fabc1fd146 100644
--- a/tests/SkImageTest.cpp
+++ b/tests/SkImageTest.cpp
@@ -26,7 +26,7 @@ DEF_TEST(SkImageFromBitmap_extractSubset, reporter) {
         canvas.drawIRect(r, p);
         SkBitmap dstBitmap;
         srcBitmap.extractSubset(&dstBitmap, r);
-        image.reset(SkNewImageFromRasterBitmap(dstBitmap, NULL, kUnlocked_SharedPixelRefMode));
+        image.reset(SkImage::NewFromBitmap(dstBitmap));
     }
 
     SkBitmap tgt;