From: bsalomon Date: Thu, 19 Feb 2015 16:24:16 +0000 (-0800) Subject: Allow resources' unique keys to be changed. X-Git-Tag: accepted/tizen/5.0/unified/20181102.025319~3527 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=f99e961f55bb603d099c8cb57d05a2ae52a4e9ca;p=platform%2Fupstream%2FlibSkiaSharp.git 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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());