Allow client to force an SkImage snapshot to be unique (and uniquely own its backing store).
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1686163002
Review URL: https://codereview.chromium.org/1686163002
diff --git a/include/core/SkSurface.h b/include/core/SkSurface.h
index 45262d7..d5a66b4 100644
--- a/include/core/SkSurface.h
+++ b/include/core/SkSurface.h
@@ -237,6 +237,18 @@
SkImage* newImageSnapshot(Budgeted = kYes_Budgeted);
/**
+ * In rare instances a client may want a unique copy of the SkSurface's contents in an image
+ * snapshot. This enum can be used to enforce that the image snapshot's backing store is not
+ * shared with another image snapshot or the surface's backing store. This is generally more
+ * expensive. This was added for Chromium bug 585250.
+ */
+ enum ForceUnique {
+ kNo_ForceUnique,
+ kYes_ForceUnique
+ };
+ SkImage* newImageSnapshot(Budgeted, ForceUnique);
+
+ /**
* Though the caller could get a snapshot image explicitly, and draw that,
* it seems that directly drawing a surface into another canvas might be
* a common pattern, and that we could possibly be more efficient, since
diff --git a/src/image/SkSurface.cpp b/src/image/SkSurface.cpp
index fed13a2..711dfda 100644
--- a/src/image/SkSurface.cpp
+++ b/src/image/SkSurface.cpp
@@ -164,9 +164,13 @@
}
SkImage* SkSurface::newImageSnapshot(Budgeted budgeted) {
- SkImage* image = asSB(this)->getCachedImage(budgeted);
- SkSafeRef(image); // the caller will call unref() to balance this
- return image;
+ // the caller will call unref() to balance this
+ return asSB(this)->refCachedImage(budgeted, kNo_ForceUnique);
+}
+
+SkImage* SkSurface::newImageSnapshot(Budgeted budgeted, ForceUnique unique) {
+ // the caller will call unref() to balance this
+ return asSB(this)->refCachedImage(budgeted, unique);
}
SkSurface* SkSurface::newSurface(const SkImageInfo& info) {
diff --git a/src/image/SkSurface_Base.h b/src/image/SkSurface_Base.h
index 7658409..aaa19cf 100644
--- a/src/image/SkSurface_Base.h
+++ b/src/image/SkSurface_Base.h
@@ -9,6 +9,7 @@
#define SkSurface_Base_DEFINED
#include "SkCanvas.h"
+#include "SkImagePriv.h"
#include "SkSurface.h"
#include "SkSurfacePriv.h"
@@ -42,7 +43,7 @@
* must faithfully represent the current contents, even if the surface
* is changed after this called (e.g. it is drawn to via its canvas).
*/
- virtual SkImage* onNewImageSnapshot(Budgeted) = 0;
+ virtual SkImage* onNewImageSnapshot(Budgeted, ForceCopyMode) = 0;
/**
* Default implementation:
@@ -75,7 +76,7 @@
virtual void onRestoreBackingMutability() {}
inline SkCanvas* getCachedCanvas();
- inline SkImage* getCachedImage(Budgeted);
+ inline SkImage* refCachedImage(Budgeted, ForceUnique);
bool hasCachedImage() const { return fCachedImage != nullptr; }
@@ -108,12 +109,23 @@
return fCachedCanvas;
}
-SkImage* SkSurface_Base::getCachedImage(Budgeted budgeted) {
- if (nullptr == fCachedImage) {
- fCachedImage = this->onNewImageSnapshot(budgeted);
- SkASSERT(!fCachedCanvas || fCachedCanvas->getSurfaceBase() == this);
+SkImage* SkSurface_Base::refCachedImage(Budgeted budgeted, ForceUnique unique) {
+ SkImage* snap = fCachedImage;
+ if (kYes_ForceUnique == unique && snap && !snap->unique()) {
+ snap = nullptr;
}
- return fCachedImage;
+ if (snap) {
+ return SkRef(snap);
+ }
+ ForceCopyMode fcm = (kYes_ForceUnique == unique) ? kYes_ForceCopyMode :
+ kNo_ForceCopyMode;
+ snap = this->onNewImageSnapshot(budgeted, fcm);
+ if (kNo_ForceUnique == unique) {
+ SkASSERT(!fCachedImage);
+ fCachedImage = SkSafeRef(snap);
+ }
+ SkASSERT(!fCachedCanvas || fCachedCanvas->getSurfaceBase() == this);
+ return snap;
}
#endif
diff --git a/src/image/SkSurface_Gpu.cpp b/src/image/SkSurface_Gpu.cpp
index 24ac6d3..03fdece 100644
--- a/src/image/SkSurface_Gpu.cpp
+++ b/src/image/SkSurface_Gpu.cpp
@@ -76,10 +76,27 @@
&this->props());
}
-SkImage* SkSurface_Gpu::onNewImageSnapshot(Budgeted budgeted) {
+SkImage* SkSurface_Gpu::onNewImageSnapshot(Budgeted budgeted, ForceCopyMode forceCopyMode) {
+ GrRenderTarget* rt = fDevice->accessRenderTarget();
+ SkASSERT(rt);
+ GrTexture* tex = rt->asTexture();
+ SkAutoTUnref<GrTexture> copy;
+ // TODO: Force a copy when the rt is an external resource.
+ if (kYes_ForceCopyMode == forceCopyMode || !tex) {
+ GrSurfaceDesc desc = fDevice->accessRenderTarget()->desc();
+ GrContext* ctx = fDevice->context();
+ desc.fFlags = desc.fFlags & !kRenderTarget_GrSurfaceFlag;
+ copy.reset(ctx->textureProvider()->createTexture(desc, kYes_Budgeted == budgeted));
+ if (!copy) {
+ return nullptr;
+ }
+ if (!ctx->copySurface(copy, rt)) {
+ return nullptr;
+ }
+ tex = copy;
+ }
const SkImageInfo info = fDevice->imageInfo();
SkImage* image = nullptr;
- GrTexture* tex = fDevice->accessRenderTarget()->asTexture();
if (tex) {
image = new SkImage_Gpu(info.width(), info.height(), kNeedNewImageUniqueID,
info.alphaType(), tex, budgeted);
@@ -94,7 +111,7 @@
GrRenderTarget* rt = fDevice->accessRenderTarget();
// are we sharing our render target with the image? Note this call should never create a new
// image because onCopyOnWrite is only called when there is a cached image.
- SkImage* image = this->getCachedImage(kNo_Budgeted);
+ SkAutoTUnref<SkImage> image(this->refCachedImage(kNo_Budgeted, kNo_ForceUnique));
SkASSERT(image);
if (rt->asTexture() == as_IB(image)->getTexture()) {
this->fDevice->replaceRenderTarget(SkSurface::kRetain_ContentChangeMode == mode);
diff --git a/src/image/SkSurface_Gpu.h b/src/image/SkSurface_Gpu.h
index 2324154..23aed2c 100644
--- a/src/image/SkSurface_Gpu.h
+++ b/src/image/SkSurface_Gpu.h
@@ -23,7 +23,7 @@
bool onGetRenderTargetHandle(GrBackendObject*, BackendHandleAccess) override;
SkCanvas* onNewCanvas() override;
SkSurface* onNewSurface(const SkImageInfo&) override;
- SkImage* onNewImageSnapshot(Budgeted) override;
+ SkImage* onNewImageSnapshot(Budgeted, ForceCopyMode) override;
void onCopyOnWrite(ContentChangeMode) override;
void onDiscard() override;
diff --git a/src/image/SkSurface_Raster.cpp b/src/image/SkSurface_Raster.cpp
index d9763c0..37790a0 100644
--- a/src/image/SkSurface_Raster.cpp
+++ b/src/image/SkSurface_Raster.cpp
@@ -24,7 +24,7 @@
SkCanvas* onNewCanvas() override;
SkSurface* onNewSurface(const SkImageInfo&) override;
- SkImage* onNewImageSnapshot(Budgeted) override;
+ SkImage* onNewImageSnapshot(Budgeted, ForceCopyMode) override;
void onDraw(SkCanvas*, SkScalar x, SkScalar y, const SkPaint*) override;
void onCopyOnWrite(ContentChangeMode) override;
void onRestoreBackingMutability() override;
@@ -118,18 +118,20 @@
canvas->drawBitmap(fBitmap, x, y, paint);
}
-SkImage* SkSurface_Raster::onNewImageSnapshot(Budgeted) {
+SkImage* SkSurface_Raster::onNewImageSnapshot(Budgeted, ForceCopyMode forceCopyMode) {
if (fWeOwnThePixels) {
// SkImage_raster requires these pixels are immutable for its full lifetime.
// We'll undo this via onRestoreBackingMutability() if we can avoid the COW.
if (SkPixelRef* pr = fBitmap.pixelRef()) {
pr->setTemporarilyImmutable();
}
+ } else {
+ forceCopyMode = kYes_ForceCopyMode;
}
+
// Our pixels are in memory, so read access on the snapshot SkImage could be cheap.
// Lock the shared pixel ref to ensure peekPixels() is usable.
- return SkNewImageFromRasterBitmap(fBitmap,
- fWeOwnThePixels ? kNo_ForceCopyMode : kYes_ForceCopyMode);
+ return SkNewImageFromRasterBitmap(fBitmap, forceCopyMode);
}
void SkSurface_Raster::onRestoreBackingMutability() {
@@ -141,8 +143,9 @@
void SkSurface_Raster::onCopyOnWrite(ContentChangeMode mode) {
// are we sharing pixelrefs with the image?
- SkASSERT(this->getCachedImage(kNo_Budgeted));
- if (SkBitmapImageGetPixelRef(this->getCachedImage(kNo_Budgeted)) == fBitmap.pixelRef()) {
+ SkAutoTUnref<SkImage> cached(this->refCachedImage(kNo_Budgeted, kNo_ForceUnique));
+ SkASSERT(cached);
+ if (SkBitmapImageGetPixelRef(cached) == fBitmap.pixelRef()) {
SkASSERT(fWeOwnThePixels);
if (kDiscard_ContentChangeMode == mode) {
fBitmap.allocPixels();
diff --git a/tests/SurfaceTest.cpp b/tests/SurfaceTest.cpp
index d9138eb..41fef61 100644
--- a/tests/SurfaceTest.cpp
+++ b/tests/SurfaceTest.cpp
@@ -323,6 +323,133 @@
}
#endif
+static bool same_image(SkImage* a, SkImage* b,
+ std::function<intptr_t(SkImage*)> getImageBackingStore) {
+ return getImageBackingStore(a) == getImageBackingStore(b);
+}
+
+static bool same_image_surf(SkImage* a, SkSurface* b,
+ std::function<intptr_t(SkImage*)> getImageBackingStore,
+ std::function<intptr_t(SkSurface*)> getSurfaceBackingStore) {
+ return getImageBackingStore(a) == getSurfaceBackingStore(b);
+}
+
+static void test_unique_image_snap(skiatest::Reporter* reporter, SkSurface* surface,
+ bool surfaceIsDirect,
+ std::function<intptr_t(SkImage*)> imageBackingStore,
+ std::function<intptr_t(SkSurface*)> surfaceBackingStore) {
+ std::function<intptr_t(SkImage*)> ibs = imageBackingStore;
+ std::function<intptr_t(SkSurface*)> sbs = surfaceBackingStore;
+ static const SkSurface::Budgeted kB = SkSurface::kNo_Budgeted;
+ {
+ SkAutoTUnref<SkImage> image(surface->newImageSnapshot(kB, SkSurface::kYes_ForceUnique));
+ REPORTER_ASSERT(reporter, !same_image_surf(image, surface, ibs, sbs));
+ REPORTER_ASSERT(reporter, image->unique());
+ }
+ {
+ SkAutoTUnref<SkImage> image1(surface->newImageSnapshot(kB, SkSurface::kYes_ForceUnique));
+ REPORTER_ASSERT(reporter, !same_image_surf(image1, surface, ibs, sbs));
+ REPORTER_ASSERT(reporter, image1->unique());
+ SkAutoTUnref<SkImage> image2(surface->newImageSnapshot(kB, SkSurface::kYes_ForceUnique));
+ REPORTER_ASSERT(reporter, !same_image_surf(image2, surface, ibs, sbs));
+ REPORTER_ASSERT(reporter, !same_image(image1, image2, ibs));
+ REPORTER_ASSERT(reporter, image2->unique());
+ }
+ {
+ SkAutoTUnref<SkImage> image1(surface->newImageSnapshot(kB, SkSurface::kNo_ForceUnique));
+ SkAutoTUnref<SkImage> image2(surface->newImageSnapshot(kB, SkSurface::kYes_ForceUnique));
+ SkAutoTUnref<SkImage> image3(surface->newImageSnapshot(kB, SkSurface::kNo_ForceUnique));
+ SkAutoTUnref<SkImage> image4(surface->newImageSnapshot(kB, SkSurface::kYes_ForceUnique));
+ // Image 1 and 3 ought to be the same (or we're missing an optimization).
+ REPORTER_ASSERT(reporter, same_image(image1, image3, ibs));
+ // If the surface is not direct then images 1 and 3 should alias the surface's
+ // store.
+ REPORTER_ASSERT(reporter, !surfaceIsDirect == same_image_surf(image1, surface, ibs, sbs));
+ // Image 2 should not be shared with any other image.
+ REPORTER_ASSERT(reporter, !same_image(image1, image2, ibs) &&
+ !same_image(image3, image2, ibs) &&
+ !same_image(image4, image2, ibs));
+ REPORTER_ASSERT(reporter, image2->unique());
+ REPORTER_ASSERT(reporter, !same_image_surf(image2, surface, ibs, sbs));
+ // Image 4 should not be shared with any other image.
+ REPORTER_ASSERT(reporter, !same_image(image1, image4, ibs) &&
+ !same_image(image3, image4, ibs));
+ REPORTER_ASSERT(reporter, !same_image_surf(image4, surface, ibs, sbs));
+ REPORTER_ASSERT(reporter, image4->unique());
+ }
+}
+
+DEF_TEST(UniqueImageSnapshot, reporter) {
+ auto getImageBackingStore = [reporter](SkImage* image) {
+ SkPixmap pm;
+ bool success = image->peekPixels(&pm);
+ REPORTER_ASSERT(reporter, success);
+ return reinterpret_cast<intptr_t>(pm.addr());
+ };
+ auto getSufaceBackingStore = [reporter](SkSurface* surface) {
+ SkImageInfo info;
+ size_t rowBytes;
+ const void* pixels = surface->getCanvas()->peekPixels(&info, &rowBytes);
+ REPORTER_ASSERT(reporter, pixels);
+ return reinterpret_cast<intptr_t>(pixels);
+ };
+
+ SkAutoTUnref<SkSurface> surface(create_surface());
+ test_unique_image_snap(reporter, surface, false, getImageBackingStore, getSufaceBackingStore);
+ surface.reset(create_direct_surface());
+ test_unique_image_snap(reporter, surface, true, getImageBackingStore, getSufaceBackingStore);
+}
+
+#if SK_SUPPORT_GPU
+DEF_GPUTEST_FOR_RENDERING_CONTEXTS(UniqueImageSnapshot_Gpu, reporter, context) {
+ for (auto& surface_func : { &create_gpu_surface, &create_gpu_scratch_surface }) {
+ SkAutoTUnref<SkSurface> surface(surface_func(context, kOpaque_SkAlphaType, nullptr));
+
+ auto imageBackingStore = [reporter](SkImage* image) {
+ GrTexture* texture = as_IB(image)->peekTexture();
+ if (!texture) {
+ ERRORF(reporter, "Not texture backed.");
+ return static_cast<intptr_t>(0);
+ }
+ return static_cast<intptr_t>(texture->getUniqueID());
+ };
+
+ auto surfaceBackingStore = [reporter](SkSurface* surface) {
+ GrRenderTarget* rt =
+ surface->getCanvas()->internal_private_accessTopLayerRenderTarget();
+ if (!rt) {
+ ERRORF(reporter, "Not render target backed.");
+ return static_cast<intptr_t>(0);
+ }
+ return static_cast<intptr_t>(rt->getUniqueID());
+ };
+
+ test_unique_image_snap(reporter, surface, false, imageBackingStore, surfaceBackingStore);
+
+ // Test again with a "direct" render target;
+ GrBackendObject textureObject = context->getGpu()->createTestingOnlyBackendTexture(nullptr,
+ 10, 10, kRGBA_8888_GrPixelConfig);
+ GrBackendTextureDesc desc;
+ desc.fConfig = kRGBA_8888_GrPixelConfig;
+ desc.fWidth = 10;
+ desc.fHeight = 10;
+ desc.fFlags = kRenderTarget_GrBackendTextureFlag;
+ desc.fTextureHandle = textureObject;
+ GrTexture* texture = context->textureProvider()->wrapBackendTexture(desc);
+ {
+ SkAutoTUnref<SkSurface> 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,
+ surfaceBackingStore);
+ }
+ texture->unref();
+ context->getGpu()->deleteTestingOnlyBackendTexture(textureObject);
+ }
+}
+#endif
+
#if SK_SUPPORT_GPU
// May we (soon) eliminate the need to keep testing this, by hiding the bloody device!
static uint32_t get_legacy_gen_id(SkSurface* surface) {