Tie mip map cache purging to Images, not Bitmaps/PixelRefs

We were potentially using different keys for lookup vs. add, because we
were adding with a key based on the pixelRef, which may not have the
same uniqueID as the originating image.

Bug: skia:
Change-Id: Ib4d3d5ead9f5a574cf6d1920080bc9c4ae66c1d0
Reviewed-on: https://skia-review.googlesource.com/c/161625
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>

include/core/SkPixelRef.h

@@ -80,12 +80,6 @@ public:
     // Takes ownership of listener.  Threadsafe.
     void addGenIDChangeListener(GenIDChangeListener* listener);
 
-    // Call when this pixelref is part of the key to a resourcecache entry. This allows the cache
-    // to know automatically those entries can be purged when this pixelref is changed or deleted.
-    void notifyAddedToCache() {
-        fAddedToCache.store(true);
-    }
-
     virtual SkDiscardableMemory* diagnostic_only_getDiscardable() const { return nullptr; }
 
 protected:
@@ -104,9 +98,6 @@ private:
     SkMutex                         fGenIDChangeListenersMutex;
     SkTDArray<GenIDChangeListener*> fGenIDChangeListeners;  // pointers are owned
 
-    // Set true by caches when they cache content that's derived from the current pixels.
-    std::atomic<bool> fAddedToCache;
-
     enum Mutability {
         kMutable,               // PixelRefs begin mutable.
         kTemporarilyImmutable,  // Considered immutable, but can revert to mutable.

src/core/SkBitmapCache.cpp

@@ -7,6 +7,7 @@
 
 #include "SkAtomics.h"
 #include "SkBitmapCache.h"
+#include "SkBitmapProvider.h"
 #include "SkImage.h"
 #include "SkResourceCache.h"
 #include "SkMipMap.h"
@@ -342,12 +343,18 @@ static SkResourceCache::DiscardableFactory get_fact(SkResourceCache* localCache)
                       : SkResourceCache::GetDiscardableFactory();
 }
 
-const SkMipMap* SkMipMapCache::AddAndRef(const SkBitmap& src, SkResourceCache* localCache) {
+const SkMipMap* SkMipMapCache::AddAndRef(const SkBitmapProvider& provider,
+                                         SkResourceCache* localCache) {
+    SkBitmap src;
+    if (!provider.asBitmap(&src)) {
+        return nullptr;
+    }
+
     SkMipMap* mipmap = SkMipMap::Build(src, get_fact(localCache));
     if (mipmap) {
-        MipMapRec* rec = new MipMapRec(SkBitmapCacheDesc::Make(src), mipmap);
+        MipMapRec* rec = new MipMapRec(provider.makeCacheDesc(), mipmap);
         CHECK_LOCAL(localCache, add, Add, rec);
-        src.pixelRef()->notifyAddedToCache();
+        provider.notifyAddedToCache();
     }
     return mipmap;
 }

src/core/SkBitmapCache.h

@@ -11,6 +11,7 @@
 #include "SkBitmap.h"
 #include "SkMipMap.h"
 
+class SkBitmapProvider;
 class SkImage;
 class SkResourceCache;
 
@@ -61,7 +62,8 @@ class SkMipMapCache {
 public:
     static const SkMipMap* FindAndRef(const SkBitmapCacheDesc&,
                                       SkResourceCache* localCache = nullptr);
-    static const SkMipMap* AddAndRef(const SkBitmap& src, SkResourceCache* localCache = nullptr);
+    static const SkMipMap* AddAndRef(const SkBitmapProvider&,
+                                     SkResourceCache* localCache = nullptr);
 };
 
 #endif

src/core/SkBitmapController.cpp

@@ -78,11 +78,7 @@ bool SkBitmapController::State::processMediumRequest(const SkBitmapProvider& pro
     if (invScaleSize.width() > SK_Scalar1 || invScaleSize.height() > SK_Scalar1) {
         fCurrMip.reset(SkMipMapCache::FindAndRef(provider.makeCacheDesc()));
         if (nullptr == fCurrMip.get()) {
-            SkBitmap orig;
-            if (!provider.asBitmap(&orig)) {
-                return false;
-            }
-            fCurrMip.reset(SkMipMapCache::AddAndRef(orig));
+            fCurrMip.reset(SkMipMapCache::AddAndRef(provider));
             if (nullptr == fCurrMip.get()) {
                 return false;
             }

src/core/SkPixelRef.cpp

@@ -5,7 +5,6 @@
  * found in the LICENSE file.
  */
 
-#include "SkBitmapCache.h"
 #include "SkMutex.h"
 #include "SkPixelRef.h"
 #include "SkTraceEvent.h"
@@ -42,7 +41,6 @@ SkPixelRef::SkPixelRef(int width, int height, void* pixels, size_t rowBytes)
 
     this->needsNewGenID();
     fMutability = kMutable;
-    fAddedToCache.store(false);
 }
 
 SkPixelRef::~SkPixelRef() {
@@ -101,12 +99,6 @@ void SkPixelRef::callGenIDChangeListeners() {
         for (int i = 0; i < fGenIDChangeListeners.count(); i++) {
             fGenIDChangeListeners[i]->onChange();
         }
-
-        // TODO: SkAtomic could add "old_value = atomic.xchg(new_value)" to make this clearer.
-        if (fAddedToCache.load()) {
-            SkNotifyBitmapGenIDIsStale(this->getGenerationID());
-            fAddedToCache.store(false);
-        }
     }
     // Listeners get at most one shot, so whether these triggered or not, blow them away.
     fGenIDChangeListeners.deleteAll();

tests/SkResourceCacheTest.cpp

@@ -7,6 +7,7 @@
 
 #include "Test.h"
 #include "SkBitmapCache.h"
+#include "SkBitmapProvider.h"
 #include "SkCanvas.h"
 #include "SkDiscardableMemoryPool.h"
 #include "SkGraphics.h"
@@ -43,15 +44,18 @@ static void test_mipmapcache(skiatest::Reporter* reporter, SkResourceCache* cach
     SkBitmap src;
     src.allocN32Pixels(5, 5);
     src.setImmutable();
+    sk_sp<SkImage> img = SkImage::MakeFromBitmap(src);
+    SkBitmapProvider provider(img.get());
+    const auto desc = provider.makeCacheDesc();
 
-    const SkMipMap* mipmap = SkMipMapCache::FindAndRef(SkBitmapCacheDesc::Make(src), cache);
+    const SkMipMap* mipmap = SkMipMapCache::FindAndRef(desc, cache);
     REPORTER_ASSERT(reporter, nullptr == mipmap);
 
-    mipmap = SkMipMapCache::AddAndRef(src, cache);
+    mipmap = SkMipMapCache::AddAndRef(provider, cache);
     REPORTER_ASSERT(reporter, mipmap);
 
     {
-        const SkMipMap* mm = SkMipMapCache::FindAndRef(SkBitmapCacheDesc::Make(src), cache);
+        const SkMipMap* mm = SkMipMapCache::FindAndRef(desc, cache);
         REPORTER_ASSERT(reporter, mm);
         REPORTER_ASSERT(reporter, mm == mipmap);
         mm->unref();
@@ -65,7 +69,7 @@ static void test_mipmapcache(skiatest::Reporter* reporter, SkResourceCache* cach
     check_data(reporter, mipmap, 1, kInCache, kNotLocked);
 
     // find us again
-    mipmap = SkMipMapCache::FindAndRef(SkBitmapCacheDesc::Make(src), cache);
+    mipmap = SkMipMapCache::FindAndRef(desc, cache);
     check_data(reporter, mipmap, 2, kInCache, kLocked);
 
     cache->purgeAll();
@@ -78,25 +82,26 @@ static void test_mipmap_notify(skiatest::Reporter* reporter, SkResourceCache* ca
     const int N = 3;
 
     SkBitmap src[N];
+    sk_sp<SkImage> img[N];
+    SkBitmapCacheDesc desc[N];
     for (int i = 0; i < N; ++i) {
         src[i].allocN32Pixels(5, 5);
         src[i].setImmutable();
-        SkMipMapCache::AddAndRef(src[i], cache)->unref();
+        img[i] = SkImage::MakeFromBitmap(src[i]);
+        SkBitmapProvider provider(img[i].get());
+        SkMipMapCache::AddAndRef(provider, cache)->unref();
+        desc[i] = provider.makeCacheDesc();
     }
 
     for (int i = 0; i < N; ++i) {
-        const auto desc = SkBitmapCacheDesc::Make(src[i]);
-        const SkMipMap* mipmap = SkMipMapCache::FindAndRef(desc, cache);
-        if (cache) {
-            // if cache is null, we're working on the global cache, and other threads might purge
-            // it, making this check fragile.
-            REPORTER_ASSERT(reporter, mipmap);
-        }
+        const SkMipMap* mipmap = SkMipMapCache::FindAndRef(desc[i], cache);
+        // We're always using a local cache, so we know we won't be purged by other threads
+        REPORTER_ASSERT(reporter, mipmap);
         SkSafeUnref(mipmap);
 
-        src[i].reset(); // delete the underlying pixelref, which *should* remove us from the cache
+        img[i].reset(); // delete the image, which *should* remove us from the cache
 
-        mipmap = SkMipMapCache::FindAndRef(desc, cache);
+        mipmap = SkMipMapCache::FindAndRef(desc[i], cache);
         REPORTER_ASSERT(reporter, !mipmap);
     }
 }