From b2c0133caf0f03462385c19634281c351355c979 Mon Sep 17 00:00:00 2001 From: bsalomon Date: Fri, 26 Feb 2016 10:37:26 -0800 Subject: [PATCH] When a surface is backed by an external render target force a copy on image snap BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1694943002 Review URL: https://codereview.chromium.org/1694943002 --- src/gpu/GrGpuResource.cpp | 2 +- src/gpu/GrGpuResourceCacheAccess.h | 5 ----- src/gpu/GrGpuResourcePriv.h | 7 ++++++- src/gpu/GrResourceCache.cpp | 10 +++++----- src/gpu/GrResourceCache.h | 2 +- src/image/SkSurface_Gpu.cpp | 6 ++++-- tests/SurfaceTest.cpp | 4 +--- 7 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/gpu/GrGpuResource.cpp b/src/gpu/GrGpuResource.cpp index 98cea1c9a8..7c053b31d7 100644 --- a/src/gpu/GrGpuResource.cpp +++ b/src/gpu/GrGpuResource.cpp @@ -169,7 +169,7 @@ void GrGpuResource::setScratchKey(const GrScratchKey& scratchKey) { SkASSERT(!fScratchKey.isValid()); SkASSERT(scratchKey.isValid()); // Wrapped resources can never have a scratch key. - if (this->cacheAccess().isExternal()) { + if (this->resourcePriv().isExternal()) { return; } fScratchKey = scratchKey; diff --git a/src/gpu/GrGpuResourceCacheAccess.h b/src/gpu/GrGpuResourceCacheAccess.h index 70fe08503e..4b47a1c932 100644 --- a/src/gpu/GrGpuResourceCacheAccess.h +++ b/src/gpu/GrGpuResourceCacheAccess.h @@ -30,11 +30,6 @@ private: SkBudgeted::kYes == fResource->resourcePriv().isBudgeted(); } - /** - * Is the resource object wrapping an externally allocated GPU resource? - */ - bool isExternal() const { return fResource->isExternal(); } - /** * Is the resource object wrapping an externally allocated GPU resource that Skia has not taken * ownership of. diff --git a/src/gpu/GrGpuResourcePriv.h b/src/gpu/GrGpuResourcePriv.h index 62dc85059a..5da1c94878 100644 --- a/src/gpu/GrGpuResourcePriv.h +++ b/src/gpu/GrGpuResourcePriv.h @@ -50,7 +50,12 @@ public: return SkBudgeted(ret); } - /** + /** + * Is the resource object wrapping an externally allocated GPU resource? + */ + bool isExternal() const { return fResource->isExternal(); } + + /** * If this resource can be used as a scratch resource this returns a valid scratch key. * Otherwise it returns a key for which isNullScratch is true. The resource may currently be * used as a uniquely keyed resource rather than scratch. Check isScratch(). diff --git a/src/gpu/GrResourceCache.cpp b/src/gpu/GrResourceCache.cpp index 005b6c4ccd..087e762592 100644 --- a/src/gpu/GrResourceCache.cpp +++ b/src/gpu/GrResourceCache.cpp @@ -147,7 +147,7 @@ void GrResourceCache::insertResource(GrGpuResource* resource) { #endif } if (resource->resourcePriv().getScratchKey().isValid()) { - SkASSERT(!resource->cacheAccess().isExternal()); + SkASSERT(!resource->resourcePriv().isExternal()); fScratchMap.insert(resource->resourcePriv().getScratchKey(), resource); } @@ -377,7 +377,7 @@ void GrResourceCache::notifyCntReachedZero(GrGpuResource* resource, uint32_t fla if (SkBudgeted::kNo == resource->resourcePriv().isBudgeted()) { // Check whether this resource could still be used as a scratch resource. - if (!resource->cacheAccess().isExternal() && + if (!resource->resourcePriv().isExternal() && resource->resourcePriv().getScratchKey().isValid()) { // We won't purge an existing resource to make room for this one. if (fBudgetedCount < fMaxCount && @@ -662,19 +662,19 @@ void GrResourceCache::validate() const { SkASSERT(!resource->getUniqueKey().isValid()); ++fScratch; SkASSERT(fScratchMap->countForKey(resource->resourcePriv().getScratchKey())); - SkASSERT(!resource->cacheAccess().isExternal()); + SkASSERT(!resource->resourcePriv().isExternal()); } else if (resource->resourcePriv().getScratchKey().isValid()) { SkASSERT(SkBudgeted::kNo == resource->resourcePriv().isBudgeted() || resource->getUniqueKey().isValid()); ++fCouldBeScratch; SkASSERT(fScratchMap->countForKey(resource->resourcePriv().getScratchKey())); - SkASSERT(!resource->cacheAccess().isExternal()); + SkASSERT(!resource->resourcePriv().isExternal()); } const GrUniqueKey& uniqueKey = resource->getUniqueKey(); if (uniqueKey.isValid()) { ++fContent; SkASSERT(fUniqueHash->find(uniqueKey) == resource); - SkASSERT(!resource->cacheAccess().isExternal()); + SkASSERT(!resource->resourcePriv().isExternal()); SkASSERT(SkBudgeted::kYes == resource->resourcePriv().isBudgeted()); } diff --git a/src/gpu/GrResourceCache.h b/src/gpu/GrResourceCache.h index 8321289dff..fb13dde88e 100644 --- a/src/gpu/GrResourceCache.h +++ b/src/gpu/GrResourceCache.h @@ -211,7 +211,7 @@ public: if (resource->cacheAccess().isScratch()) { ++fScratch; } - if (resource->cacheAccess().isExternal()) { + if (resource->resourcePriv().isExternal()) { ++fExternal; } if (resource->cacheAccess().isBorrowed()) { diff --git a/src/image/SkSurface_Gpu.cpp b/src/image/SkSurface_Gpu.cpp index 5345ceee20..173a0d81d7 100644 --- a/src/image/SkSurface_Gpu.cpp +++ b/src/image/SkSurface_Gpu.cpp @@ -82,8 +82,10 @@ SkImage* SkSurface_Gpu::onNewImageSnapshot(SkBudgeted budgeted, ForceCopyMode fo SkASSERT(rt); GrTexture* tex = rt->asTexture(); SkAutoTUnref copy; - // TODO: Force a copy when the rt is an external resource. - if (kYes_ForceCopyMode == forceCopyMode || !tex) { + // If the original render target is a buffer originally created by the client, then we don't + // want to ever retarget the SkSurface at another buffer we create. Force a copy now to avoid + // copy-on-write. + if (kYes_ForceCopyMode == forceCopyMode || !tex || rt->resourcePriv().isExternal()) { GrSurfaceDesc desc = fDevice->accessRenderTarget()->desc(); GrContext* ctx = fDevice->context(); desc.fFlags = desc.fFlags & ~kRenderTarget_GrSurfaceFlag; diff --git a/tests/SurfaceTest.cpp b/tests/SurfaceTest.cpp index 7449f211b5..8166bba9bd 100644 --- a/tests/SurfaceTest.cpp +++ b/tests/SurfaceTest.cpp @@ -439,9 +439,7 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(UniqueImageSnapshot_Gpu, reporter, context) { { SkAutoTUnref surface( SkSurface::NewRenderTargetDirect(texture->asRenderTarget())); - // We should be able to pass true here, but disallowing copy on write for direct GPU - // surfaces is not yet implemented. - test_unique_image_snap(reporter, surface, false, imageBackingStore, + test_unique_image_snap(reporter, surface, true, imageBackingStore, surfaceBackingStore); } texture->unref();