From e75ab4c26a4f661334845f7bc4132011694836a4 Mon Sep 17 00:00:00 2001 From: John Reck Date: Thu, 7 May 2015 13:14:15 -0700 Subject: [PATCH] Remove SkBitmap from ResourceCache Bug: 18928352 Fully-proper refcounting via SkBitmap/SkPixelRef, no more side-channel refcounting via ResourceCache. Makes HWUI more resilient to the bitmap being modified as well as the SkBitmap's info & rowBytes() is updated every time a DisplayList is recorded instead of relying on buggy cache eviction logic Change-Id: I2e8292d62ab6c257a2cfa1542387bf2bf1ade816 --- libs/hwui/DisplayList.cpp | 5 ---- libs/hwui/DisplayListCanvas.h | 7 +++-- libs/hwui/ResourceCache.cpp | 70 ------------------------------------------- libs/hwui/ResourceCache.h | 40 ------------------------- 4 files changed, 4 insertions(+), 118 deletions(-) diff --git a/libs/hwui/DisplayList.cpp b/libs/hwui/DisplayList.cpp index 4540bec77a9e..e679bff18c86 100644 --- a/libs/hwui/DisplayList.cpp +++ b/libs/hwui/DisplayList.cpp @@ -41,10 +41,6 @@ void DisplayListData::cleanupResources() { ResourceCache& resourceCache = ResourceCache::getInstance(); resourceCache.lock(); - for (size_t i = 0; i < bitmapResources.size(); i++) { - resourceCache.decrementRefcountLocked(bitmapResources.itemAt(i)); - } - for (size_t i = 0; i < patchResources.size(); i++) { resourceCache.decrementRefcountLocked(patchResources.itemAt(i)); } @@ -59,7 +55,6 @@ void DisplayListData::cleanupResources() { delete path; } - bitmapResources.clear(); patchResources.clear(); pathResources.clear(); paints.clear(); diff --git a/libs/hwui/DisplayListCanvas.h b/libs/hwui/DisplayListCanvas.h index 0064236a1a98..7f8ae72ad240 100644 --- a/libs/hwui/DisplayListCanvas.h +++ b/libs/hwui/DisplayListCanvas.h @@ -350,9 +350,10 @@ private: // correctly, such as creating the bitmap from scratch, drawing with it, changing its // contents, and drawing again. The only fix would be to always copy it the first time, // which doesn't seem worth the extra cycles for this unlikely case. - const SkBitmap* cachedBitmap = mResourceCache.insert(bitmap); - mDisplayListData->bitmapResources.add(cachedBitmap); - return cachedBitmap; + const SkBitmap* localBitmap = new (alloc()) SkBitmap(bitmap); + alloc().autoDestroy(localBitmap); + mDisplayListData->bitmapResources.push_back(localBitmap); + return localBitmap; } inline const Res_png_9patch* refPatch(const Res_png_9patch* patch) { diff --git a/libs/hwui/ResourceCache.cpp b/libs/hwui/ResourceCache.cpp index 454fedc2b179..75d81346d62a 100644 --- a/libs/hwui/ResourceCache.cpp +++ b/libs/hwui/ResourceCache.cpp @@ -59,21 +59,6 @@ void ResourceCache::unlock() { mLock.unlock(); } -const SkBitmap* ResourceCache::insert(const SkBitmap& bitmapResource) { - Mutex::Autolock _l(mLock); - - BitmapKey bitmapKey(bitmapResource); - ssize_t index = mBitmapCache.indexOfKey(bitmapKey); - if (index == NAME_NOT_FOUND) { - SkBitmap* cachedBitmap = new SkBitmap(bitmapResource); - index = mBitmapCache.add(bitmapKey, cachedBitmap); - return cachedBitmap; - } - - mBitmapCache.keyAt(index).mRefCount++; - return mBitmapCache.valueAt(index); -} - void ResourceCache::incrementRefcount(void* resource, ResourceType resourceType) { Mutex::Autolock _l(mLock); incrementRefcountLocked(resource, resourceType); @@ -98,11 +83,6 @@ void ResourceCache::decrementRefcount(void* resource) { decrementRefcountLocked(resource); } -void ResourceCache::decrementRefcount(const SkBitmap* bitmapResource) { - Mutex::Autolock _l(mLock); - decrementRefcountLocked(bitmapResource); -} - void ResourceCache::decrementRefcount(const Res_png_9patch* patchResource) { decrementRefcount((void*) patchResource); } @@ -120,23 +100,6 @@ void ResourceCache::decrementRefcountLocked(void* resource) { } } -void ResourceCache::decrementRefcountLocked(const SkBitmap* bitmapResource) { - BitmapKey bitmapKey(*bitmapResource); - ssize_t index = mBitmapCache.indexOfKey(bitmapKey); - - LOG_ALWAYS_FATAL_IF(index == NAME_NOT_FOUND, - "Decrementing the reference of an untracked Bitmap"); - - const BitmapKey& cacheEntry = mBitmapCache.keyAt(index); - if (cacheEntry.mRefCount == 1) { - // delete the bitmap and remove it from the cache - delete mBitmapCache.valueAt(index); - mBitmapCache.removeItemsAt(index); - } else { - cacheEntry.mRefCount--; - } -} - void ResourceCache::decrementRefcountLocked(const Res_png_9patch* patchResource) { decrementRefcountLocked((void*) patchResource); } @@ -190,38 +153,5 @@ void ResourceCache::deleteResourceReferenceLocked(const void* resource, Resource delete ref; } -/////////////////////////////////////////////////////////////////////////////// -// Bitmap Key -/////////////////////////////////////////////////////////////////////////////// - -void BitmapKey::operator=(const BitmapKey& other) { - this->mRefCount = other.mRefCount; - this->mBitmapDimensions = other.mBitmapDimensions; - this->mPixelRefOrigin = other.mPixelRefOrigin; - this->mPixelRefStableID = other.mPixelRefStableID; -} - -bool BitmapKey::operator==(const BitmapKey& other) const { - return mPixelRefStableID == other.mPixelRefStableID && - mPixelRefOrigin == other.mPixelRefOrigin && - mBitmapDimensions == other.mBitmapDimensions; -} - -bool BitmapKey::operator<(const BitmapKey& other) const { - if (mPixelRefStableID != other.mPixelRefStableID) { - return mPixelRefStableID < other.mPixelRefStableID; - } - if (mPixelRefOrigin.x() != other.mPixelRefOrigin.x()) { - return mPixelRefOrigin.x() < other.mPixelRefOrigin.x(); - } - if (mPixelRefOrigin.y() != other.mPixelRefOrigin.y()) { - return mPixelRefOrigin.y() < other.mPixelRefOrigin.y(); - } - if (mBitmapDimensions.width() != other.mBitmapDimensions.width()) { - return mBitmapDimensions.width() < other.mBitmapDimensions.width(); - } - return mBitmapDimensions.height() < other.mBitmapDimensions.height(); -} - }; // namespace uirenderer }; // namespace android diff --git a/libs/hwui/ResourceCache.h b/libs/hwui/ResourceCache.h index 6c483faf3508..4583c8de87e8 100644 --- a/libs/hwui/ResourceCache.h +++ b/libs/hwui/ResourceCache.h @@ -51,37 +51,6 @@ public: ResourceType resourceType; }; -class BitmapKey { -public: - BitmapKey(const SkBitmap& bitmap) - : mRefCount(1) - , mBitmapDimensions(bitmap.dimensions()) - , mPixelRefOrigin(bitmap.pixelRefOrigin()) - , mPixelRefStableID(bitmap.pixelRef()->getStableID()) { } - - void operator=(const BitmapKey& other); - bool operator==(const BitmapKey& other) const; - bool operator<(const BitmapKey& other) const; - -private: - // This constructor is only used by the KeyedVector implementation - BitmapKey() - : mRefCount(-1) - , mBitmapDimensions(SkISize::Make(0,0)) - , mPixelRefOrigin(SkIPoint::Make(0,0)) - , mPixelRefStableID(0) { } - - // reference count of all HWUI object using this bitmap - mutable int mRefCount; - - SkISize mBitmapDimensions; - SkIPoint mPixelRefOrigin; - uint32_t mPixelRefStableID; - - friend class ResourceCache; - friend struct android::key_value_pair_t; -}; - class ANDROID_API ResourceCache: public Singleton { ResourceCache(); ~ResourceCache(); @@ -97,18 +66,10 @@ public: void lock(); void unlock(); - /** - * The cache stores a copy of the provided resource or refs an existing resource - * if the bitmap has previously been inserted and returns the cached copy. - */ - const SkBitmap* insert(const SkBitmap& resource); - void incrementRefcount(const Res_png_9patch* resource); - void decrementRefcount(const SkBitmap* resource); void decrementRefcount(const Res_png_9patch* resource); - void decrementRefcountLocked(const SkBitmap* resource); void decrementRefcountLocked(const Res_png_9patch* resource); void destructor(Res_png_9patch* resource); @@ -134,7 +95,6 @@ private: mutable Mutex mLock; KeyedVector* mCache; - KeyedVector mBitmapCache; }; }; // namespace uirenderer -- 2.11.0