Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit e75ab4c2 authored by John Reck's avatar John Reck
Browse files

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
parent b59642bf

libs/hwui/DisplayList.cpp

+0 −5
Original line number Diff line number Diff line
@@ -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();

libs/hwui/DisplayListCanvas.h

+4 −3
Original line number Diff line number Diff line
@@ -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) {

libs/hwui/ResourceCache.cpp

+0 −70
Original line number Diff line number Diff line
@@ -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

libs/hwui/ResourceCache.h

+0 −40
Original line number Diff line number Diff line
@@ -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