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

diff --git a/libs/hwui/DisplayList.cpp b/libs/hwui/DisplayList.cpp
index 4540bec..e679bff 100644 (file)
--- 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 0064236..7f8ae72 100644 (file)
--- 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 454fedc..75d8134 100644 (file)
--- 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 6c483fa..4583c8d 100644 (file)
--- 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<BitmapKey, SkBitmap*>;
-};
-
 class ANDROID_API ResourceCache: public Singleton<ResourceCache> {
     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<const void*, ResourceReference*>* mCache;
-    KeyedVector<BitmapKey, SkBitmap*> mBitmapCache;
 };
 
 }; // namespace uirenderer