Allow client to force an SkImage snapshot to be unique (and uniquely own its backing...

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 45262d78e63d3d30351c43ccd22ae3c2442ad0a3..d5a66b4bb7f8fb0a5417fada09ff08aad8bd6e5a 100644 (file)
--- a/include/core/SkSurface.h
+++ b/include/core/SkSurface.h
@@ -236,6 +236,18 @@ public:
      */
     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

diff --git a/src/image/SkSurface.cpp b/src/image/SkSurface.cpp
index fed13a284beaed2202f5d2bde92f71032dcc777f..711dfda6a43c9662d9ef6c1586f5a5e87619b014 100644 (file)
--- a/src/image/SkSurface.cpp
+++ b/src/image/SkSurface.cpp
@@ -164,9 +164,13 @@ SkCanvas* SkSurface::getCanvas() {
 }
 
 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 7658409ad764eb78cdcb330d5ea13897d986c83f..aaa19cf5922d7f3ac5e0788a561dbea387d0ca67 100644 (file)
--- 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 @@ public:
      *  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 @@ public:
     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 @@ SkCanvas* SkSurface_Base::getCachedCanvas() {
     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 24ac6d33124b82a237f3bfffd6a07d25bcdb8e64..03fdecef3a720115cf531c22bdf69c8e1df1cba2 100644 (file)
--- a/src/image/SkSurface_Gpu.cpp
+++ b/src/image/SkSurface_Gpu.cpp
@@ -76,10 +76,27 @@ SkSurface* SkSurface_Gpu::onNewSurface(const SkImageInfo& info) {
                                       &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 @@ void SkSurface_Gpu::onCopyOnWrite(ContentChangeMode mode) {
     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 23241540bfe3d95d7f550480204d230f77f14f06..23aed2cd3c5d020688b70724d82c7919df1619c8 100644 (file)
--- a/src/image/SkSurface_Gpu.h
+++ b/src/image/SkSurface_Gpu.h
@@ -23,7 +23,7 @@ public:
     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 d9763c0c9566eedc10f47cb70ca8c12ce9eb5da6..37790a0dd93bd7a0e2ce1dd3e24139fc8bdb5cd9 100644 (file)
--- a/src/image/SkSurface_Raster.cpp
+++ b/src/image/SkSurface_Raster.cpp
@@ -24,7 +24,7 @@ public:
 
     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 @@ void SkSurface_Raster::onDraw(SkCanvas* canvas, SkScalar x, SkScalar y,
     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::onRestoreBackingMutability() {
 
 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 d9138ebca73cc7a0676c32d6c5c9a84181a1060c..41fef617bca9e37669a26dc5716833ff0dde5300 100644 (file)
--- a/tests/SurfaceTest.cpp
+++ b/tests/SurfaceTest.cpp
@@ -323,6 +323,133 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SurfaceBackendHandleAccessCopyOnWrite_Gpu, re
 }
 #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) {