lock pixels in image when bitmap is immutable and not-lazy
BUG=skia:
Review URL: https://codereview.chromium.org/1266143003
diff --git a/include/core/SkPixelRef.h b/include/core/SkPixelRef.h
index a67855d..1dc01f7 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 @@
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 0edad83..43e2884 100644
--- a/src/core/SkPixelRef.cpp
+++ b/src/core/SkPixelRef.cpp
@@ -173,7 +173,6 @@
}
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 @@
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 a17f336..241904b 100644
--- a/src/image/SkImage.cpp
+++ b/src/image/SkImage.cpp
@@ -253,7 +253,7 @@
#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 0626d6a..bc4a7b0 100644
--- a/src/image/SkImagePriv.h
+++ b/src/image/SkImagePriv.h
@@ -35,16 +35,12 @@
* 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 6629593..120c397 100644
--- a/src/image/SkImage_Raster.cpp
+++ b/src/image/SkImage_Raster.cpp
@@ -81,10 +81,13 @@
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* 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 @@
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 d0a6553..bcf6da7 100644
--- a/src/image/SkSurface_Raster.cpp
+++ b/src/image/SkSurface_Raster.cpp
@@ -128,7 +128,7 @@
}
// 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 882871f..eb2263e 100644
--- a/tests/DeferredCanvasTest.cpp
+++ b/tests/DeferredCanvasTest.cpp
@@ -64,7 +64,7 @@
}
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 5da8342..ba071bb 100644
--- a/tests/ImageTest.cpp
+++ b/tests/ImageTest.cpp
@@ -245,9 +245,7 @@
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 d5c7eaa..fabc1fd 100644
--- a/tests/SkImageTest.cpp
+++ b/tests/SkImageTest.cpp
@@ -26,7 +26,7 @@
canvas.drawIRect(r, p);
SkBitmap dstBitmap;
srcBitmap.extractSubset(&dstBitmap, r);
- image.reset(SkNewImageFromRasterBitmap(dstBitmap, NULL, kUnlocked_SharedPixelRefMode));
+ image.reset(SkImage::NewFromBitmap(dstBitmap));
}
SkBitmap tgt;