commit 5449aade6227a1e701ebc24ee683d94a810c9a70
author Florin Malita <fmalita@chromium.org> Mon Jul 10 11:14:40 2017 -0400
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Mon Jul 10 16:17:45 2017 +0000
tree 34fe52f49f780e231cebaeac81e603a90dd3f9d6
parent 81995592d8a941fcf79ba5cee71a37510fa8d6e5

[Reland] More SkColorSpaceXformer caching

  * apply(SkColorFilter*)
  * apply(SkImage*)

Also add purge logic to minimize caching scope.

Change-Id: I295d20f760f8be0c3746858d6f9c73e351f10a36
Reviewed-on: https://skia-review.googlesource.com/22030
Commit-Queue: Florin Malita <fmalita@chromium.org>
Reviewed-by: Mike Reed <reed@google.com>

src/core/SkColorSpaceXformer.cpp

diff --git a/src/core/SkColorSpaceXformer.cpp b/src/core/SkColorSpaceXformer.cpp
index 71715ad..58b3f49 100644
--- a/src/core/SkColorSpaceXformer.cpp
+++ b/src/core/SkColorSpaceXformer.cpp
@@ -19,7 +19,8 @@
 SkColorSpaceXformer::SkColorSpaceXformer(sk_sp<SkColorSpace> dst,
                                          std::unique_ptr<SkColorSpaceXform> fromSRGB)
     : fDst(std::move(dst))
-    , fFromSRGB(std::move(fromSRGB)) {}
+    , fFromSRGB(std::move(fromSRGB))
+    , fReentryCount(0) {}
 
 SkColorSpaceXformer::~SkColorSpaceXformer() {}
 
@@ -33,11 +34,76 @@
         : nullptr;
 }
 
+// So what's up with these caches?
+//
+// We want to cache transformed objects for a couple of reasons:
+//
+// 1) to avoid redundant work - the inputs are a DAG, not a tree (e.g. same SkImage drawn multiple
+//    times in a SkPicture), so if we blindly recurse we could end up transforming the same objects
+//    repeatedly.
+//
+// 2) to avoid topology changes - we want the output to remain isomorphic with the input -- this is
+//    particularly important for image filters (to maintain their original DAG structure in order
+//    to not defeat their own/internal caching), but also for avoiding unnecessary cloning
+//    (e.g. duplicated SkImages allocated for the example in #1 above).
+//
+// The caching scope is naturaly bound by the lifetime of the SkColorSpaceXformer object, but
+// clients may choose to not discard xformers immediately - in which case, caching indefinitely
+// is problematic.  The solution is to limit the cache scope to the top level apply() call
+// (i.e. we only keep cached objects alive while transforming).
+
+class SkColorSpaceXformer::AutoCachePurge {
+public:
+    AutoCachePurge(SkColorSpaceXformer* xformer)
+        : fXformer(xformer) {
+        fXformer->fReentryCount++;
+    }
+
+    ~AutoCachePurge() {
+        SkASSERT(fXformer->fReentryCount > 0);
+        if (--fXformer->fReentryCount == 0) {
+            fXformer->purgeCaches();
+        }
+    }
+
+private:
+    SkColorSpaceXformer* fXformer;
+};
+
+template <typename T>
+sk_sp<T> SkColorSpaceXformer::cachedApply(const T* src, Cache<T>* cache,
+                                          sk_sp<T> (*applyFunc)(const T*, SkColorSpaceXformer*)) {
+    if (!src) {
+        return nullptr;
+    }
+
+    auto key = sk_ref_sp(const_cast<T*>(src));
+    if (auto* xformed = cache->find(key)) {
+        return sk_ref_sp(xformed->get());
+    }
+
+    auto xformed = applyFunc(src, this);
+    cache->set(std::move(key), xformed);
+
+    return xformed;
+}
+
+void SkColorSpaceXformer::purgeCaches() {
+    fImageCache.reset();
+    fColorFilterCache.reset();
+    fImageFilterCache.reset();
+}
+
 sk_sp<SkImage> SkColorSpaceXformer::apply(const SkImage* src) {
-    return src->makeColorSpace(fDst, SkTransferFunctionBehavior::kIgnore);
+    const AutoCachePurge autoPurge(this);
+    return this->cachedApply<SkImage>(src, &fImageCache,
+        [](const SkImage* img, SkColorSpaceXformer* xformer) {
+            return img->makeColorSpace(xformer->fDst, SkTransferFunctionBehavior::kIgnore);
+        });
 }
 
 sk_sp<SkImage> SkColorSpaceXformer::apply(const SkBitmap& src) {
+    const AutoCachePurge autoPurge(this);
     sk_sp<SkImage> image = SkMakeImageFromRasterBitmap(src, kNever_SkCopyPixelsMode);
     if (!image) {
         return nullptr;
@@ -50,25 +116,23 @@
 }
 
 sk_sp<SkColorFilter> SkColorSpaceXformer::apply(const SkColorFilter* colorFilter) {
-    return colorFilter->makeColorSpace(this);
+    const AutoCachePurge autoPurge(this);
+    return this->cachedApply<SkColorFilter>(colorFilter, &fColorFilterCache,
+        [](const SkColorFilter* f, SkColorSpaceXformer* xformer) {
+            return f->makeColorSpace(xformer);
+        });
 }
 
 sk_sp<SkImageFilter> SkColorSpaceXformer::apply(const SkImageFilter* imageFilter) {
-    if (!imageFilter) {
-        return nullptr;
-    }
-
-    if (auto* xformedFilter = fFilterCache.find(imageFilter->fUniqueID)) {
-        return sk_ref_sp(xformedFilter->get());
-    }
-
-    auto xformedFilter = imageFilter->makeColorSpace(this);
-    fFilterCache.set(imageFilter->fUniqueID, xformedFilter);
-
-    return xformedFilter;
+    const AutoCachePurge autoPurge(this);
+    return this->cachedApply<SkImageFilter>(imageFilter, &fImageFilterCache,
+        [](const SkImageFilter* f, SkColorSpaceXformer* xformer) {
+            return f->makeColorSpace(xformer);
+        });
 }
 
 sk_sp<SkShader> SkColorSpaceXformer::apply(const SkShader* shader) {
+    const AutoCachePurge autoPurge(this);
     return as_SB(shader)->makeColorSpace(this);
 }
 
@@ -85,6 +149,8 @@
 }
 
 SkPaint SkColorSpaceXformer::apply(const SkPaint& src) {
+    const AutoCachePurge autoPurge(this);
+
     SkPaint dst = src;
 
     // All SkColorSpaces have the same black point.