Remove fCacheKeys from SkImageFilter.

No public API changes.

Bug: skia:7666, skia:7887
Change-Id: I8ac4ec37dd3d0fcc050bc977db41439a8e18895f
Reviewed-on: https://skia-review.googlesource.com/125500
Commit-Queue: Ben Wagner <benjaminwagner@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>
Auto-Submit: Ben Wagner <benjaminwagner@google.com>

include/core/SkImageFilter.h

@@ -437,8 +437,7 @@ private:
     bool fUsesSrcInput;
     CropRect fCropRect;
     uint32_t fUniqueID; // Globally unique
-    mutable SkTArray<SkImageFilterCacheKey> fCacheKeys;
+
-    mutable SkMutex fMutex;
     typedef SkFlattenable INHERITED;
 };
 

src/core/SkImageFilter.cpp

@@ -167,8 +167,7 @@ SkImageFilter::SkImageFilter(sk_sp<SkImageFilter> const* inputs,
 }
 
 SkImageFilter::~SkImageFilter() {
-    SkAutoMutexAcquire lock(fMutex);
+    SkImageFilterCache::Get()->purgeByImageFilter(this);
-    SkImageFilterCache::Get()->purgeByKeys(fCacheKeys.begin(), fCacheKeys.count());
 }
 
 SkImageFilter::SkImageFilter(int inputCount, SkReadBuffer& buffer)
@@ -224,30 +223,11 @@ sk_sp<SkSpecialImage> SkImageFilter::filterImage(SkSpecialImage* src, const Cont
 
     if (result && context.cache()) {
         context.cache()->set(key, result.get(), *offset, this);
-        SkAutoMutexAcquire mutex(fMutex);
-        fCacheKeys.push_back(key);
     }
 
     return result;
 }
 
-void SkImageFilter::removeKey(const SkImageFilterCacheKey& key) const {
-    SkAutoMutexAcquire mutex(fMutex);
-    for (int i = 0; i < fCacheKeys.count(); i++) {
-        if (fCacheKeys[i] == key) {
-            fCacheKeys.removeShuffle(i);
-            break;
-        }
-    }
-#ifdef SK_DEBUG
-    for (int i = 0; i < fCacheKeys.count(); i++) {
-        if (fCacheKeys[i] == key) {
-            SkASSERT(false);
-        }
-    }
-#endif
-}
-
 SkIRect SkImageFilter::filterBounds(const SkIRect& src, const SkMatrix& ctm,
                                  MapDirection direction) const {
     if (kReverse_MapDirection == direction) {

src/core/SkImageFilterCache.cpp

@@ -7,6 +7,8 @@
 
 #include "SkImageFilterCache.h"
 
+#include <vector>
+
 #include "SkImageFilter.h"
 #include "SkMutex.h"
 #include "SkOnce.h"
@@ -14,6 +16,7 @@
 #include "SkRefCnt.h"
 #include "SkSpecialImage.h"
 #include "SkTDynamicHash.h"
+#include "SkTHash.h"
 #include "SkTInternalLList.h"
 
 #ifdef SK_BUILD_FOR_IOS
@@ -76,6 +79,12 @@ public:
         fLookup.add(v);
         fLRU.addToHead(v);
         fCurrentBytes += image->getSize();
+        if (auto* values = fImageFilterValues.find(filter)) {
+            values->push_back(v);
+        } else {
+            fImageFilterValues.set(filter, {v});
+        }
+
         while (fCurrentBytes > fMaxBytes) {
             Value* tail = fLRU.tail();
             SkASSERT(tail);
@@ -95,18 +104,19 @@ public:
         }
     }
 
-    void purgeByKeys(const Key keys[], int count) override {
+    void purgeByImageFilter(const SkImageFilter* filter) override {
         SkAutoMutexAcquire mutex(fMutex);
-        // This function is only called in the destructor of SkImageFilter.
+        auto* values = fImageFilterValues.find(filter);
-        // Because the destructor will destroy the fCacheKeys anyway, we set the
+        if (!values) {
-        // filter to be null so that removeInternal() won't call the
+            return;
-        // SkImageFilter::removeKey() function.
-        for (int i = 0; i < count; i++) {
-            if (Value* v = fLookup.find(keys[i])) {
-                v->fFilter = nullptr;
-                this->removeInternal(v);
-            }
         }
+        for (Value* v : *values) {
+            // We set the filter to be null so that removeInternal() won't delete from values while
+            // we're iterating over it.
+            v->fFilter = nullptr;
+            this->removeInternal(v);
+        }
+        fImageFilterValues.remove(filter);
     }
 
     SkDEBUGCODE(int count() const override { return fLookup.count(); })
@@ -114,7 +124,18 @@ private:
     void removeInternal(Value* v) {
         SkASSERT(v->fImage);
         if (v->fFilter) {
-            v->fFilter->removeKey(v->fKey);
+            if (auto* values = fImageFilterValues.find(v->fFilter)) {
+                if (values->size() == 1 && (*values)[0] == v) {
+                    fImageFilterValues.remove(v->fFilter);
+                } else {
+                    for (auto it = values->begin(); it != values->end(); ++it) {
+                        if (*it == v) {
+                            values->erase(it);
+                            break;
+                        }
+                    }
+                }
+            }
         }
         fCurrentBytes -= v->fImage->getSize();
         fLRU.remove(v);
@@ -122,11 +143,13 @@ private:
         delete v;
     }
 private:
-    SkTDynamicHash<Value, Key>            fLookup;
+    SkTDynamicHash<Value, Key>                            fLookup;
-    mutable SkTInternalLList<Value>       fLRU;
+    mutable SkTInternalLList<Value>                       fLRU;
-    size_t                                fMaxBytes;
+    // Value* always points to an item in fLookup.
-    size_t                                fCurrentBytes;
+    SkTHashMap<const SkImageFilter*, std::vector<Value*>> fImageFilterValues;
-    mutable SkMutex                       fMutex;
+    size_t                                                fMaxBytes;
+    size_t                                                fCurrentBytes;
+    mutable SkMutex                                       fMutex;
 };
 
 } // namespace

src/core/SkImageFilterCache.h

@@ -59,7 +59,7 @@ public:
     virtual void set(const SkImageFilterCacheKey& key, SkSpecialImage* image,
                      const SkIPoint& offset, const SkImageFilter* filter) = 0;
     virtual void purge() = 0;
-    virtual void purgeByKeys(const SkImageFilterCacheKey[], int) = 0;
+    virtual void purgeByImageFilter(const SkImageFilter*) = 0;
     SkDEBUGCODE(virtual int count() const = 0;)
 };
 

tests/ImageFilterCacheTest.cpp

@@ -8,6 +8,8 @@
 #include "Test.h"
 
 #include "SkBitmap.h"
+#include "SkColorFilter.h"
+#include "SkColorFilterImageFilter.h"
 #include "SkImage.h"
 #include "SkImageFilter.h"
 #include "SkImageFilterCache.h"
@@ -25,6 +27,12 @@ static SkBitmap create_bm() {
     return bm;
 }
 
+static sk_sp<SkImageFilter> make_filter() {
+    sk_sp<SkColorFilter> filter(SkColorFilter::MakeModeFilter(SK_ColorBLUE,
+                                                              SkBlendMode::kSrcIn));
+    return SkColorFilterImageFilter::Make(std::move(filter), nullptr, nullptr);
+}
+
 // Ensure the cache can return a cached image
 static void test_find_existing(skiatest::Reporter* reporter,
                                const sk_sp<SkSpecialImage>& image,
@@ -37,7 +45,8 @@ static void test_find_existing(skiatest::Reporter* reporter,
     SkImageFilterCacheKey key2(0, SkMatrix::I(), clip, subset->uniqueID(), subset->subset());
 
     SkIPoint offset = SkIPoint::Make(3, 4);
-    cache->set(key1, image.get(), offset, nullptr);
+    auto filter = make_filter();
+    cache->set(key1, image.get(), offset, filter.get());
 
     SkIPoint foundOffset;
 
@@ -66,7 +75,8 @@ static void test_dont_find_if_diff_key(skiatest::Reporter* reporter,
     SkImageFilterCacheKey key4(0, SkMatrix::I(), clip1, subset->uniqueID(), subset->subset());
 
     SkIPoint offset = SkIPoint::Make(3, 4);
-    cache->set(key0, image.get(), offset, nullptr);
+    auto filter = make_filter();
+    cache->set(key0, image.get(), offset, filter.get());
 
     SkIPoint foundOffset;
     REPORTER_ASSERT(reporter, !cache->get(key1, &foundOffset));
@@ -86,14 +96,16 @@ static void test_internal_purge(skiatest::Reporter* reporter, const sk_sp<SkSpec
     SkImageFilterCacheKey key2(1, SkMatrix::I(), clip, image->uniqueID(), image->subset());
 
     SkIPoint offset = SkIPoint::Make(3, 4);
-    cache->set(key1, image.get(), offset, nullptr);
+    auto filter1 = make_filter();
+    cache->set(key1, image.get(), offset, filter1.get());
 
     SkIPoint foundOffset;
 
     REPORTER_ASSERT(reporter, cache->get(key1, &foundOffset));
 
     // This should knock the first one out of the cache
-    cache->set(key2, image.get(), offset, nullptr);
+    auto filter2 = make_filter();
+    cache->set(key2, image.get(), offset, filter2.get());
 
     REPORTER_ASSERT(reporter, cache->get(key2, &foundOffset));
     REPORTER_ASSERT(reporter, !cache->get(key1, &foundOffset));
@@ -111,8 +123,10 @@ static void test_explicit_purging(skiatest::Reporter* reporter,
     SkImageFilterCacheKey key2(1, SkMatrix::I(), clip, subset->uniqueID(), image->subset());
 
     SkIPoint offset = SkIPoint::Make(3, 4);
-    cache->set(key1, image.get(), offset, nullptr);
+    auto filter1 = make_filter();
-    cache->set(key2, image.get(), offset, nullptr);
+    auto filter2 = make_filter();
+    cache->set(key1, image.get(), offset, filter1.get());
+    cache->set(key2, image.get(), offset, filter2.get());
     SkDEBUGCODE(REPORTER_ASSERT(reporter, 2 == cache->count());)
 
     SkIPoint foundOffset;
@@ -120,7 +134,7 @@ static void test_explicit_purging(skiatest::Reporter* reporter,
     REPORTER_ASSERT(reporter, cache->get(key1, &foundOffset));
     REPORTER_ASSERT(reporter, cache->get(key2, &foundOffset));
 
-    cache->purgeByKeys(&key1, 1);
+    cache->purgeByImageFilter(filter1.get());
     SkDEBUGCODE(REPORTER_ASSERT(reporter, 1 == cache->count());)
 
     REPORTER_ASSERT(reporter, !cache->get(key1, &foundOffset));