From fa7fd80ec36103351c32a7a1f235a6095110c39c Mon Sep 17 00:00:00 2001 From: "reed@google.com" Date: Thu, 12 Dec 2013 21:37:25 +0000 Subject: [PATCH] detect if the scaledimagecache returns a purged bitmap BUG= R=scroggo@google.com Review URL: https://codereview.chromium.org/110383005 git-svn-id: http://skia.googlecode.com/svn/trunk@12654 2bbb7eff-a529-9590-31e7-b0007b416f81 --- src/core/SkBitmapProcState.cpp | 76 +++++++++++++++++++++++++++------ src/core/SkBitmapScaler.cpp | 2 + src/core/SkScaledImageCache.cpp | 41 ++++++++++++++++-- src/core/SkScaledImageCache.h | 11 +++++ 4 files changed, 114 insertions(+), 16 deletions(-) diff --git a/src/core/SkBitmapProcState.cpp b/src/core/SkBitmapProcState.cpp index 39f6a78039..cdc582bfee 100644 --- a/src/core/SkBitmapProcState.cpp +++ b/src/core/SkBitmapProcState.cpp @@ -106,11 +106,33 @@ static SkScalar effective_matrix_scale_sqrd(const SkMatrix& mat) { return SkMaxScalar(v1.lengthSqd(), v2.lengthSqd()); } +class AutoScaledCacheUnlocker { +public: + AutoScaledCacheUnlocker(SkScaledImageCache::ID** idPtr) : fIDPtr(idPtr) {} + ~AutoScaledCacheUnlocker() { + if (fIDPtr && *fIDPtr) { + SkScaledImageCache::Unlock(*fIDPtr); + *fIDPtr = NULL; + } + } + + // forgets the ID, so it won't call Unlock + void release() { + fIDPtr = NULL; + } + +private: + SkScaledImageCache::ID** fIDPtr; +}; +#define AutoScaledCacheUnlocker(...) SK_REQUIRE_LOCAL_VAR(AutoScaledCacheUnlocker) + // TODO -- we may want to pass the clip into this function so we only scale // 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. bool SkBitmapProcState::possiblyScaleImage() { + AutoScaledCacheUnlocker unlocker(&fScaledCacheID); + SkASSERT(NULL == fBitmap); SkASSERT(NULL == fScaledCacheID); @@ -132,6 +154,17 @@ bool SkBitmapProcState::possiblyScaleImage() { fScaledCacheID = SkScaledImageCache::FindAndLock(fOrigBitmap, invScaleX, invScaleY, &fScaledBitmap); + if (fScaledCacheID) { + fScaledBitmap.lockPixels(); + if (!fScaledBitmap.getPixels()) { + fScaledBitmap.unlockPixels(); + // found a purged entry (discardablememory?), release it + SkScaledImageCache::Unlock(fScaledCacheID); + fScaledCacheID = NULL; + // fall through to rebuild + } + } + if (NULL == fScaledCacheID) { int dest_width = SkScalarCeilToInt(fOrigBitmap.width() / invScaleX); int dest_height = SkScalarCeilToInt(fOrigBitmap.height() / invScaleY); @@ -154,18 +187,19 @@ bool SkBitmapProcState::possiblyScaleImage() { return false; } + SkASSERT(NULL != fScaledBitmap.getPixels()); fScaledCacheID = SkScaledImageCache::AddAndLock(fOrigBitmap, invScaleX, invScaleY, fScaledBitmap); - } - fScaledBitmap.lockPixels(); // wonder if Resize() should have locked this - if (!fScaledBitmap.getPixels()) { - // TODO: find out how this can happen, and add a unittest to exercise - // inspired by BUG=chromium:295895 - return false; + if (!fScaledCacheID) { + fScaledBitmap.reset(); + return false; + } + SkASSERT(NULL != fScaledBitmap.getPixels()); } + SkASSERT(NULL != fScaledBitmap.getPixels()); fBitmap = &fScaledBitmap; // set the inv matrix type to translate-only; @@ -174,6 +208,7 @@ bool SkBitmapProcState::possiblyScaleImage() { // no need for any further filtering; we just did it! fFilterLevel = SkPaint::kNone_FilterLevel; + unlocker.release(); return true; } @@ -248,6 +283,7 @@ bool SkBitmapProcState::possiblyScaleImage() { fScaledBitmap.setPixels(level.fPixels); fBitmap = &fScaledBitmap; fFilterLevel = SkPaint::kLow_FilterLevel; + unlocker.release(); return true; } } @@ -273,15 +309,34 @@ static bool get_locked_pixels(const SkBitmap& src, int pow2, SkBitmap* dst) { } bool SkBitmapProcState::lockBaseBitmap() { + AutoScaledCacheUnlocker unlocker(&fScaledCacheID); + SkPixelRef* pr = fOrigBitmap.pixelRef(); + SkASSERT(NULL == fScaledCacheID); + if (pr->isLocked() || !pr->implementsDecodeInto()) { // fast-case, no need to look in our cache fScaledBitmap = fOrigBitmap; + fScaledBitmap.lockPixels(); + if (NULL == fScaledBitmap.getPixels()) { + return false; + } } else { fScaledCacheID = SkScaledImageCache::FindAndLock(fOrigBitmap, SK_Scalar1, SK_Scalar1, &fScaledBitmap); + if (fScaledCacheID) { + fScaledBitmap.lockPixels(); + if (!fScaledBitmap.getPixels()) { + fScaledBitmap.unlockPixels(); + // found a purged entry (discardablememory?), release it + SkScaledImageCache::Unlock(fScaledCacheID); + fScaledCacheID = NULL; + // fall through to rebuild + } + } + if (NULL == fScaledCacheID) { if (!get_locked_pixels(fOrigBitmap, 0, &fScaledBitmap)) { return false; @@ -299,13 +354,8 @@ bool SkBitmapProcState::lockBaseBitmap() { } } } - fScaledBitmap.lockPixels(); // just 'cause the cache made a copy :( - if (!fScaledBitmap.getPixels()) { - // TODO: find out how this can happen, and add a unittest to exercise - // inspired by BUG=chromium:295895 - return false; - } fBitmap = &fScaledBitmap; + unlocker.release(); return true; } @@ -334,6 +384,8 @@ bool SkBitmapProcState::chooseProcs(const SkMatrix& inv, const SkPaint& paint) { fInvMatrix = inv; fFilterLevel = paint.getFilterLevel(); + SkASSERT(NULL == fScaledCacheID); + // possiblyScaleImage will look to see if it can rescale the image as a // preprocess; either by scaling up to the target size, or by selecting // a nearby mipmap level. If it does, it will adjust the working diff --git a/src/core/SkBitmapScaler.cpp b/src/core/SkBitmapScaler.cpp index c28d4779c7..67a9508ee9 100644 --- a/src/core/SkBitmapScaler.cpp +++ b/src/core/SkBitmapScaler.cpp @@ -301,6 +301,8 @@ bool SkBitmapScaler::Resize(SkBitmap* resultPtr, convolveProcs, true); *resultPtr = result; + resultPtr->lockPixels(); + SkASSERT(NULL != resultPtr->getPixels()); return true; } diff --git a/src/core/SkScaledImageCache.cpp b/src/core/SkScaledImageCache.cpp index b3956f4a8d..7c8b66498e 100644 --- a/src/core/SkScaledImageCache.cpp +++ b/src/core/SkScaledImageCache.cpp @@ -239,9 +239,18 @@ void* SkOneShotDiscardablePixelRef::onLockPixels(SkColorTable** ctable) { return fDM->data(); } - SkASSERT(!fIsLocked); - fIsLocked = fDM->lock(); - return fIsLocked ? fDM->data() : NULL; + // A previous call to onUnlock may have deleted our DM, so check for that + if (NULL == fDM) { + return NULL; + } + + if (!fDM->lock()) { + // since it failed, we delete it now, to free-up the resource + delete fDM; + fDM = NULL; + return NULL; + } + return fDM->data(); } void SkOneShotDiscardablePixelRef::onUnlockPixels() { @@ -613,6 +622,8 @@ void SkScaledImageCache::addToHead(Rec* rec) { this->validate(); } +/////////////////////////////////////////////////////////////////////////////// + #ifdef SK_DEBUG void SkScaledImageCache::validate() const { if (NULL == fHead) { @@ -658,6 +669,21 @@ void SkScaledImageCache::validate() const { } #endif +void SkScaledImageCache::dump() const { + this->validate(); + + const Rec* rec = fHead; + int locked = 0; + while (rec) { + locked += rec->fLockCount > 0; + rec = rec->fNext; + } + + SkDebugf("SkScaledImageCache: count=%d bytes=%d locked=%d %s\n", + fCount, fBytesUsed, locked, + fDiscardableFactory ? "discardable" : "malloc"); +} + /////////////////////////////////////////////////////////////////////////////// #include "SkThread.h" @@ -730,7 +756,9 @@ SkScaledImageCache::ID* SkScaledImageCache::AddAndLockMip(const SkBitmap& orig, void SkScaledImageCache::Unlock(SkScaledImageCache::ID* id) { SkAutoMutexAcquire am(gMutex); - return get_cache()->unlock(id); + get_cache()->unlock(id); + +// get_cache()->dump(); } size_t SkScaledImageCache::GetBytesUsed() { @@ -753,6 +781,11 @@ SkBitmap::Allocator* SkScaledImageCache::GetAllocator() { return get_cache()->allocator(); } +void SkScaledImageCache::Dump() { + SkAutoMutexAcquire am(gMutex); + get_cache()->dump(); +} + /////////////////////////////////////////////////////////////////////////////// #include "SkGraphics.h" diff --git a/src/core/SkScaledImageCache.h b/src/core/SkScaledImageCache.h index 311db325be..fe072306d3 100644 --- a/src/core/SkScaledImageCache.h +++ b/src/core/SkScaledImageCache.h @@ -66,6 +66,11 @@ public: static SkBitmap::Allocator* GetAllocator(); + /** + * Call SkDebugf() with diagnostic information about the state of the cache + */ + static void Dump(); + /////////////////////////////////////////////////////////////////////////// /** @@ -151,6 +156,11 @@ public: SkBitmap::Allocator* allocator() const { return fAllocator; }; + /** + * Call SkDebugf() with diagnostic information about the state of the cache + */ + void dump() const; + public: struct Rec; struct Key; @@ -174,6 +184,7 @@ private: Rec* findAndLock(const Key& key); ID* addAndLock(Rec* rec); + void purgeRec(Rec*); void purgeAsNeeded(); // linklist management