Allow resources' unique keys to be changed.

Review URL: https://codereview.chromium.org/938943002

diff --git a/include/gpu/GrContext.h b/include/gpu/GrContext.h
index 712eeb8..846cd0f 100644 (file)
--- a/include/gpu/GrContext.h
+++ b/include/gpu/GrContext.h
@@ -169,10 +169,11 @@ public:
      */
     void purgeAllUnlockedResources();
 
-    /** Sets a unique key on the resource. The resource must not already have a unique key and the
-     *  key must not already be in use for this to succeed.
+    /**
+     * Sets a unique key on the resource. Upon key collision this resource takes the place of the
+     * previous resource that had the key.
      */
-    bool addResourceToCache(const GrUniqueKey&, GrGpuResource*);
+    void addResourceToCache(const GrUniqueKey&, GrGpuResource*);
 
     /**
      * Finds a resource in the cache, based on the specified key. This is intended for use in

diff --git a/include/gpu/GrGpuResource.h b/include/gpu/GrGpuResource.h
index 80b50e3..e2f23e0 100644 (file)
--- a/include/gpu/GrGpuResource.h
+++ b/include/gpu/GrGpuResource.h
@@ -269,7 +269,7 @@ private:
     virtual size_t onGpuMemorySize() const = 0;
 
     // See comments in CacheAccess and ResourcePriv.
-    bool setUniqueKey(const GrUniqueKey&);
+    void setUniqueKey(const GrUniqueKey&);
     void removeUniqueKey();
     void notifyIsPurgeable() const;
     void removeScratchKey();

diff --git a/include/gpu/GrResourceKey.h b/include/gpu/GrResourceKey.h
index ce3e76e..aecdc70 100644 (file)
--- a/include/gpu/GrResourceKey.h
+++ b/include/gpu/GrResourceKey.h
@@ -45,7 +45,6 @@ protected:
     }
 
     bool operator==(const GrResourceKey& that) const {
-        SkASSERT(this->isValid() && that.isValid());
         return 0 == memcmp(fKey.get(), that.fKey.get(), this->size());
     }
 

diff --git a/src/effects/SkBlurMaskFilter.cpp b/src/effects/SkBlurMaskFilter.cpp
index c16cc2d..ae4f756 100644 (file)
--- a/src/effects/SkBlurMaskFilter.cpp
+++ b/src/effects/SkBlurMaskFilter.cpp
@@ -777,7 +777,7 @@ bool GrRectBlurEffect::CreateBlurProfileTexture(GrContext *context, float sigma,
         if (NULL == *blurProfileTexture) {
             return false;
         }
-        SkAssertResult(context->addResourceToCache(key, *blurProfileTexture));
+        context->addResourceToCache(key, *blurProfileTexture);
     }
 
     return true;
@@ -962,7 +962,7 @@ GrFragmentProcessor* GrRRectBlurEffect::Create(GrContext* context, float sigma,
         texDesc.fConfig = kAlpha_8_GrPixelConfig;
 
         blurNinePatchTexture = context->createTexture(texDesc, true, blurred_mask.fImage, 0);
-        SkAssertResult(context->addResourceToCache(key, blurNinePatchTexture));
+        context->addResourceToCache(key, blurNinePatchTexture);
 
         SkMask::FreeImage(blurred_mask.fImage);
     }

diff --git a/src/effects/SkColorCubeFilter.cpp b/src/effects/SkColorCubeFilter.cpp
index 552d000..f71f471 100644 (file)
--- a/src/effects/SkColorCubeFilter.cpp
+++ b/src/effects/SkColorCubeFilter.cpp
@@ -354,7 +354,7 @@ GrFragmentProcessor* SkColorCubeFilter::asFragmentProcessor(GrContext* context)
     if (!textureCube) {
         textureCube.reset(context->createTexture(desc, true, fCubeData->data(), 0));
         if (textureCube) {
-            SkAssertResult(context->addResourceToCache(key, textureCube));
+            context->addResourceToCache(key, textureCube);
         }
     }
 

diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp
index ef80b5b..a4679a9 100755 (executable)
--- a/src/gpu/GrContext.cpp
+++ b/src/gpu/GrContext.cpp
@@ -1582,12 +1582,12 @@ void GrContext::setResourceCacheLimits(int maxTextures, size_t maxTextureBytes)
     fResourceCache->setLimits(maxTextures, maxTextureBytes);
 }
 
-bool GrContext::addResourceToCache(const GrUniqueKey& key, GrGpuResource* resource) {
+void GrContext::addResourceToCache(const GrUniqueKey& key, GrGpuResource* resource) {
     ASSERT_OWNED_RESOURCE(resource);
-    if (!resource || resource->wasDestroyed()) {
-        return false;
+    if (!resource) {
+        return;
     }
-    return resource->resourcePriv().setUniqueKey(key);
+    resource->resourcePriv().setUniqueKey(key);
 }
 
 bool GrContext::isResourceInCache(const GrUniqueKey& key) const {

diff --git a/src/gpu/GrGpuResource.cpp b/src/gpu/GrGpuResource.cpp
index 0c2c9a1..a2fc7b3 100644 (file)
--- a/src/gpu/GrGpuResource.cpp
+++ b/src/gpu/GrGpuResource.cpp
@@ -86,31 +86,23 @@ void GrGpuResource::didChangeGpuMemorySize() const {
 
 void GrGpuResource::removeUniqueKey() {
     SkASSERT(fUniqueKey.isValid());
-    get_resource_cache(fGpu)->resourceAccess().willRemoveUniqueKey(this);
-    fUniqueKey.reset();
+    get_resource_cache(fGpu)->resourceAccess().removeUniqueKey(this);
 }
 
-bool GrGpuResource::setUniqueKey(const GrUniqueKey& key) {
-    // Currently this can only be called once and can't be called when the resource is scratch.
+void GrGpuResource::setUniqueKey(const GrUniqueKey& key) {
     SkASSERT(this->internalHasRef());
     SkASSERT(key.isValid());
 
     // Wrapped and uncached resources can never have a unique key.
     if (!this->resourcePriv().isBudgeted()) {
-        return false;
+        return;
     }
 
-    if (fUniqueKey.isValid() || this->wasDestroyed()) {
-        return false;
+    if (this->wasDestroyed()) {
+        return;
     }
 
-    fUniqueKey = key;
-
-    if (!get_resource_cache(fGpu)->resourceAccess().didSetUniqueKey(this)) {
-        fUniqueKey.reset();
-        return false;
-    }
-    return true;
+    get_resource_cache(fGpu)->resourceAccess().changeUniqueKey(this, key);
 }
 
 void GrGpuResource::notifyIsPurgeable() const {

diff --git a/src/gpu/GrGpuResourceCacheAccess.h b/src/gpu/GrGpuResourceCacheAccess.h
index df0ca34..4f38fc6 100644 (file)
--- a/src/gpu/GrGpuResourceCacheAccess.h
+++ b/src/gpu/GrGpuResourceCacheAccess.h
@@ -55,6 +55,12 @@ private:
         }
     }
 
+    /** Called by the cache to assign a new unique key. */
+    void setUniqueKey(const GrUniqueKey& key) { fResource->fUniqueKey = key; }
+
+    /** Called by the cache to make the unique key invalid. */
+    void removeUniqueKey() { fResource->fUniqueKey.reset(); }
+
     uint32_t timestamp() const { return fResource->fTimestamp; }
     void setTimestamp(uint32_t ts) { fResource->fTimestamp = ts; }
 

diff --git a/src/gpu/GrGpuResourcePriv.h b/src/gpu/GrGpuResourcePriv.h
index 92c53da..5324dcc 100644 (file)
--- a/src/gpu/GrGpuResourcePriv.h
+++ b/src/gpu/GrGpuResourcePriv.h
@@ -19,17 +19,15 @@ class GrGpuResource::ResourcePriv {
 public:
     /**
      * Sets a unique key for the resource. If the resource was previously cached as scratch it will
-     * be converted to a uniquely-keyed resource. Currently this may only be called once per
-     * resource. It fails if there is already a resource with the same unique key. TODO: make this
-     * supplant the resource that currently is using the unique key, allow resources' unique keys
-     * to change, and allow removal of a unique key to convert a resource back to scratch.
+     * be converted to a uniquely-keyed resource. If the key is invalid then this is equivalent to
+     * removeUniqueKey(). If another resource is using the key then its unique key is removed and
+     * this resource takes over the key.
      */
-    bool setUniqueKey(const GrUniqueKey& key) {
-        return fResource->setUniqueKey(key);
-    }
+    void setUniqueKey(const GrUniqueKey& key) { fResource->setUniqueKey(key); }
 
-    /** Removes the unique key from a resource */
-    void removeUniqueKey() { return fResource->removeUniqueKey(); }
+    /** Removes the unique key from a resource. If the resource has a scratch key, it may be
+        preserved for recycling as scratch. */
+    void removeUniqueKey() { fResource->removeUniqueKey(); }
 
     /**
      * If the resource is uncached make it cached. Has no effect on resources that are wrapped or

diff --git a/src/gpu/GrResourceCache.cpp b/src/gpu/GrResourceCache.cpp
index 8658744..a2fde2f 100644 (file)
--- a/src/gpu/GrResourceCache.cpp
+++ b/src/gpu/GrResourceCache.cpp
@@ -237,26 +237,50 @@ void GrResourceCache::willRemoveScratchKey(const GrGpuResource* resource) {
     fScratchMap.remove(resource->resourcePriv().getScratchKey(), resource);
 }
 
-void GrResourceCache::willRemoveUniqueKey(const GrGpuResource* resource) {
+void GrResourceCache::removeUniqueKey(GrGpuResource* resource) {
     // Someone has a ref to this resource in order to invalidate it. When the ref count reaches
     // zero we will get a notifyPurgable() and figure out what to do with it.
-    SkASSERT(resource->getUniqueKey().isValid());
-    fUniqueHash.remove(resource->getUniqueKey());
+    if (resource->getUniqueKey().isValid()) {
+        SkASSERT(resource == fUniqueHash.find(resource->getUniqueKey()));
+        fUniqueHash.remove(resource->getUniqueKey());
+    }
+    resource->cacheAccess().removeUniqueKey();
+    this->validate();
 }
 
-bool GrResourceCache::didSetUniqueKey(GrGpuResource* resource) {
+void GrResourceCache::changeUniqueKey(GrGpuResource* resource, const GrUniqueKey& newKey) {
     SkASSERT(resource);
     SkASSERT(this->isInCache(resource));
-    SkASSERT(resource->getUniqueKey().isValid());
 
-    GrGpuResource* res = fUniqueHash.find(resource->getUniqueKey());
-    if (NULL != res) {
-        return false;
+    // Remove the entry for this resource if it already has a unique key.
+    if (resource->getUniqueKey().isValid()) {
+        SkASSERT(resource == fUniqueHash.find(resource->getUniqueKey()));
+        fUniqueHash.remove(resource->getUniqueKey());
+        SkASSERT(NULL == fUniqueHash.find(resource->getUniqueKey()));
+    }
+
+    // If another resource has the new key, remove its key then install the key on this resource.
+    if (newKey.isValid()) {
+        if (GrGpuResource* old = fUniqueHash.find(newKey)) {
+            // If the old resource using the key is purgeable and is unreachable, then remove it.
+            if (!old->resourcePriv().getScratchKey().isValid() && old->isPurgeable()) {
+                // release may call validate() which will assert that resource is in fUniqueHash
+                // if it has a valid key. So in debug reset the key here before we assign it.
+                SkDEBUGCODE(resource->cacheAccess().removeUniqueKey();)
+                old->cacheAccess().release();
+            } else {
+                fUniqueHash.remove(newKey);
+                old->cacheAccess().removeUniqueKey();
+            }
+        }
+        SkASSERT(NULL == fUniqueHash.find(newKey));
+        resource->cacheAccess().setUniqueKey(newKey);
+        fUniqueHash.add(resource);
+    } else {
+        resource->cacheAccess().removeUniqueKey();
     }
 
-    fUniqueHash.add(resource);
     this->validate();
-    return true;
 }
 
 void GrResourceCache::refAndMakeResourceMRU(GrGpuResource* resource) {

diff --git a/src/gpu/GrResourceCache.h b/src/gpu/GrResourceCache.h
index a908140..3e5df38 100644 (file)
--- a/src/gpu/GrResourceCache.h
+++ b/src/gpu/GrResourceCache.h
@@ -32,9 +32,9 @@ class SkString;
  *         between two temporary surfaces). The scratch key is set at resource creation time and
  *         should never change. Resources need not have a scratch key.
  *      2) A unique key. This key's meaning is specific to the domain that created the key. Only one
- *         resource may have a given unique key. The unique key can be set after resource creation
- *         creation. Currently it may only be set once and cannot be cleared. This restriction will
- *         be removed.
+ *         resource may have a given unique key. The unique key can be set, cleared, or changed
+ *         anytime after resource creation.
+ *
  * A unique key always takes precedence over a scratch key when a resource has both types of keys.
  * If a resource has neither key type then it will be deleted as soon as the last reference to it
  * is dropped.
@@ -179,9 +179,9 @@ private:
     void removeResource(GrGpuResource*);
     void notifyPurgeable(GrGpuResource*);
     void didChangeGpuMemorySize(const GrGpuResource*, size_t oldSize);
-    bool didSetUniqueKey(GrGpuResource*);
+    void changeUniqueKey(GrGpuResource*, const GrUniqueKey&);
+    void removeUniqueKey(GrGpuResource*);
     void willRemoveScratchKey(const GrGpuResource*);
-    void willRemoveUniqueKey(const GrGpuResource*);
     void didChangeBudgetStatus(GrGpuResource*);
     void refAndMakeResourceMRU(GrGpuResource*);
     /// @}
@@ -297,20 +297,16 @@ private:
     }
 
     /**
-     * Called by GrGpuResources when their unique keys change.
-     *
-     * This currently returns a bool and fails when an existing resource has a key that collides
-     * with the new key. In the future it will null out the unique key for the existing resource.
-     * The failure is a temporary measure which will be fixed soon.
+     * Called by GrGpuResources to change their unique keys.
      */
-    bool didSetUniqueKey(GrGpuResource* resource) { return fCache->didSetUniqueKey(resource); }
+    void changeUniqueKey(GrGpuResource* resource, const GrUniqueKey& newKey) {
+         fCache->changeUniqueKey(resource, newKey);
+    }
 
     /**
-     * Called by a GrGpuResource when it removes its unique key.
+     * Called by a GrGpuResource to remove its unique key.
      */
-    void willRemoveUniqueKey(GrGpuResource* resource) {
-        return fCache->willRemoveUniqueKey(resource);
-    }
+    void removeUniqueKey(GrGpuResource* resource) { fCache->removeUniqueKey(resource); }
 
     /**
      * Called by a GrGpuResource when it removes its scratch key.

diff --git a/src/gpu/SkGr.cpp b/src/gpu/SkGr.cpp
index e353d41..e13c2c7 100644 (file)
--- a/src/gpu/SkGr.cpp
+++ b/src/gpu/SkGr.cpp
@@ -180,7 +180,7 @@ static GrTexture* create_texture_for_bmp(GrContext* ctx,
     if (result && optionalKey.isValid()) {
         BitmapInvalidator* listener = SkNEW_ARGS(BitmapInvalidator, (optionalKey));
         pixelRefForInvalidationNotification->addGenIDChangeListener(listener);
-        SkAssertResult(ctx->addResourceToCache(optionalKey, result));
+        ctx->addResourceToCache(optionalKey, result);
     }
     return result;
 }

diff --git a/src/gpu/effects/GrTextureStripAtlas.cpp b/src/gpu/effects/GrTextureStripAtlas.cpp
index 7376419..51aaf53 100644 (file)
--- a/src/gpu/effects/GrTextureStripAtlas.cpp
+++ b/src/gpu/effects/GrTextureStripAtlas.cpp
@@ -204,7 +204,7 @@ void GrTextureStripAtlas::lockTexture() {
     fTexture = fDesc.fContext->findAndRefCachedTexture(key);
     if (NULL == fTexture) {
         fTexture = fDesc.fContext->createTexture(texDesc, true, NULL, 0);
-        SkAssertResult(fDesc.fContext->addResourceToCache(key, fTexture));
+        fDesc.fContext->addResourceToCache(key, fTexture);
         // This is a new texture, so all of our cache info is now invalid
         this->initLRU();
         fKeyTable.rewind();

diff --git a/tests/GLProgramsTest.cpp b/tests/GLProgramsTest.cpp
index 7736653..aeb97c4 100644 (file)
--- a/tests/GLProgramsTest.cpp
+++ b/tests/GLProgramsTest.cpp
@@ -118,7 +118,7 @@ static GrRenderTarget* random_render_target(GrContext* context, SkRandom* random
     if (!texture) {
         texture = context->createTexture(texDesc, true);
         if (texture) {
-            SkAssertResult(context->addResourceToCache(key, texture));            
+            context->addResourceToCache(key, texture);
         }
     }
     return texture ? texture->asRenderTarget() : NULL;

diff --git a/tests/ResourceCacheTest.cpp b/tests/ResourceCacheTest.cpp
index 574d713..64cd327 100644 (file)
--- a/tests/ResourceCacheTest.cpp
+++ b/tests/ResourceCacheTest.cpp
@@ -245,7 +245,7 @@ static void test_budgeting(skiatest::Reporter* reporter) {
     scratch->setSize(10);
     TestResource* unique = SkNEW_ARGS(TestResource, (context->getGpu()));
     unique->setSize(11);
-    REPORTER_ASSERT(reporter, unique->resourcePriv().setUniqueKey(uniqueKey));
+    unique->resourcePriv().setUniqueKey(uniqueKey);
     TestResource* wrapped = SkNEW_ARGS(TestResource,
                                        (context->getGpu(), GrGpuResource::kWrapped_LifeCycle));
     wrapped->setSize(12);
@@ -256,7 +256,7 @@ static void test_budgeting(skiatest::Reporter* reporter) {
     // Make sure we can't add a unique key to the wrapped resource
     GrUniqueKey uniqueKey2;
     make_unique_key<0>(&uniqueKey2, 1);
-    REPORTER_ASSERT(reporter, !wrapped->resourcePriv().setUniqueKey(uniqueKey2));
+    wrapped->resourcePriv().setUniqueKey(uniqueKey2);
     REPORTER_ASSERT(reporter, NULL == cache->findAndRefUniqueResource(uniqueKey2));
 
     // Make sure sizes are as we expect
@@ -340,7 +340,7 @@ static void test_unbudgeted(skiatest::Reporter* reporter) {
 
     unique = SkNEW_ARGS(TestResource, (context->getGpu()));
     unique->setSize(11);
-    REPORTER_ASSERT(reporter, unique->resourcePriv().setUniqueKey(uniqueKey));
+    unique->resourcePriv().setUniqueKey(uniqueKey);
     unique->unref();
     REPORTER_ASSERT(reporter, 2 == cache->getResourceCount());
     REPORTER_ASSERT(reporter, 21 == cache->getResourceBytes());
@@ -500,7 +500,6 @@ static void test_remove_scratch_key(skiatest::Reporter* reporter) {
     a->unref();
     b->unref();
 
-
     GrScratchKey scratchKey;
     // Ensure that scratch key lookup is correct for negative case.
     TestResource::ComputeScratchKey(TestResource::kA_SimulatedProperty, &scratchKey);
@@ -616,38 +615,72 @@ static void test_duplicate_unique_key(skiatest::Reporter* reporter) {
     
     // Create two resources that we will attempt to register with the same unique key.
     TestResource* a = SkNEW_ARGS(TestResource, (context->getGpu()));
-    TestResource* b = SkNEW_ARGS(TestResource, (context->getGpu()));
     a->setSize(11);
-    b->setSize(12);
     
-    // Can't set the same unique key on two resources.
-    REPORTER_ASSERT(reporter, a->resourcePriv().setUniqueKey(key));
-    REPORTER_ASSERT(reporter, !b->resourcePriv().setUniqueKey(key));
+    // Set key on resource a.
+    a->resourcePriv().setUniqueKey(key);
+    REPORTER_ASSERT(reporter, a == cache->findAndRefUniqueResource(key));
+    a->unref();
+
+    // Make sure that redundantly setting a's key works.
+    a->resourcePriv().setUniqueKey(key);
+    REPORTER_ASSERT(reporter, a == cache->findAndRefUniqueResource(key));
+    a->unref();
+    REPORTER_ASSERT(reporter, 1 == cache->getResourceCount());
+    REPORTER_ASSERT(reporter, a->gpuMemorySize() == cache->getResourceBytes());
+    REPORTER_ASSERT(reporter, 1 == TestResource::NumAlive());
+
+    // Create resource b and set the same key. It should replace a's unique key cache entry.
+    TestResource* b = SkNEW_ARGS(TestResource, (context->getGpu()));
+    b->setSize(12);
+    b->resourcePriv().setUniqueKey(key);
+    REPORTER_ASSERT(reporter, b == cache->findAndRefUniqueResource(key));
+    b->unref();
 
-    // Still have two resources because b is still reffed.
+    // Still have two resources because a is still reffed.
     REPORTER_ASSERT(reporter, 2 == cache->getResourceCount());
-    REPORTER_ASSERT(reporter, a->gpuMemorySize() + b->gpuMemorySize() ==
-                              cache->getResourceBytes());
+    REPORTER_ASSERT(reporter, a->gpuMemorySize() + b->gpuMemorySize() == cache->getResourceBytes());
     REPORTER_ASSERT(reporter, 2 == TestResource::NumAlive());
 
+    a->unref();
+    // Now a should be gone.
+    REPORTER_ASSERT(reporter, 1 == cache->getResourceCount());
+    REPORTER_ASSERT(reporter, b->gpuMemorySize() == cache->getResourceBytes());
+    REPORTER_ASSERT(reporter, 1 == TestResource::NumAlive());
+
+    // Now replace b with c, but make sure c can start with one unique key and change it to b's key.
+    // Also make b be unreffed when replacement occurs.
     b->unref();
-    // Now b should be gone.
+    TestResource* c = SkNEW_ARGS(TestResource, (context->getGpu()));
+    GrUniqueKey differentKey;
+    make_unique_key<0>(&differentKey, 1);
+    c->setSize(13);
+    c->resourcePriv().setUniqueKey(differentKey);
+    REPORTER_ASSERT(reporter, 2 == cache->getResourceCount());
+    REPORTER_ASSERT(reporter, b->gpuMemorySize() + c->gpuMemorySize() == cache->getResourceBytes());
+    REPORTER_ASSERT(reporter, 2 == TestResource::NumAlive());
+    // c replaces b and b should be immediately purged.
+    c->resourcePriv().setUniqueKey(key);
     REPORTER_ASSERT(reporter, 1 == cache->getResourceCount());
-    REPORTER_ASSERT(reporter, a->gpuMemorySize() == cache->getResourceBytes());
+    REPORTER_ASSERT(reporter, c->gpuMemorySize() == cache->getResourceBytes());
     REPORTER_ASSERT(reporter, 1 == TestResource::NumAlive());
 
+    // c shouldn't be purged because it is ref'ed.
     cache->purgeAllUnlocked();
     REPORTER_ASSERT(reporter, 1 == cache->getResourceCount());
-    REPORTER_ASSERT(reporter, a->gpuMemorySize() == cache->getResourceBytes());
+    REPORTER_ASSERT(reporter, c->gpuMemorySize() == cache->getResourceBytes());
     REPORTER_ASSERT(reporter, 1 == TestResource::NumAlive());
 
-    // Drop the ref on a but it isn't immediately purged as it still has a valid scratch key.
-    a->unref();
+    // Drop the ref on c, it should be kept alive because it has a unique key.
+    c->unref();
     REPORTER_ASSERT(reporter, 1 == cache->getResourceCount());
-    REPORTER_ASSERT(reporter, a->gpuMemorySize() == cache->getResourceBytes());
+    REPORTER_ASSERT(reporter, c->gpuMemorySize() == cache->getResourceBytes());
     REPORTER_ASSERT(reporter, 1 == TestResource::NumAlive());
 
-    cache->purgeAllUnlocked();
+    // Verify that we can find c, then remove its unique key. It should get purged immediately.
+    REPORTER_ASSERT(reporter, c == cache->findAndRefUniqueResource(key));
+    c->resourcePriv().removeUniqueKey();
+    c->unref();
     REPORTER_ASSERT(reporter, 0 == cache->getResourceCount());
     REPORTER_ASSERT(reporter, 0 == cache->getResourceBytes());
     REPORTER_ASSERT(reporter, 0 == TestResource::NumAlive());