Reland "Use SkImage_Raster's unique ID to cache textures."
This is a reland of 8005007e9879f2778189e6505d70c3b2ce83452c
Original change's description:
> Use SkImage_Raster's unique ID to cache textures.
>
> SkImages can share SkBitmaps and have different unique IDs/mipmap
> status. Currently we cache the texture version using the bitmap's
> unique ID. Instead use the image's ID so different images produce
> different textures (e.g. mipped and nonmipped).
>
> Bug: skia:11983
> Change-Id: Ic37564186f675277e5a9de1bcf36b40a19c3a3de
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/407356
> Commit-Queue: Brian Salomon <bsalomon@google.com>
> Reviewed-by: Robert Phillips <robertphillips@google.com>
Bug: skia:11983
Change-Id: I63e9d15ffdf6b6769c9b0b97d9aa30f353e1a3a3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/409376
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
diff --git a/src/core/SkPictureImageGenerator.cpp b/src/core/SkPictureImageGenerator.cpp
index 04cc519..845971e 100644
--- a/src/core/SkPictureImageGenerator.cpp
+++ b/src/core/SkPictureImageGenerator.cpp
@@ -12,7 +12,6 @@
#include "include/core/SkPicture.h"
#include "include/core/SkSurface.h"
#include "src/core/SkTLazy.h"
-#include "src/gpu/SkGr.h"
#include "src/image/SkImage_Base.h"
class SkPictureImageGenerator : public SkImageGenerator {
@@ -93,6 +92,7 @@
#if SK_SUPPORT_GPU
#include "include/gpu/GrRecordingContext.h"
#include "src/gpu/GrRecordingContextPriv.h"
+#include "src/gpu/SkGr.h"
GrSurfaceProxyView SkPictureImageGenerator::onGenerateTexture(GrRecordingContext* ctx,
const SkImageInfo& info,
diff --git a/src/effects/imagefilters/SkMatrixConvolutionImageFilter.cpp b/src/effects/imagefilters/SkMatrixConvolutionImageFilter.cpp
index 3c14293..c90f743 100644
--- a/src/effects/imagefilters/SkMatrixConvolutionImageFilter.cpp
+++ b/src/effects/imagefilters/SkMatrixConvolutionImageFilter.cpp
@@ -16,11 +16,11 @@
#include "src/core/SkReadBuffer.h"
#include "src/core/SkSpecialImage.h"
#include "src/core/SkWriteBuffer.h"
-#include "src/gpu/SkGr.h"
#if SK_SUPPORT_GPU
#include "src/gpu/GrRecordingContextPriv.h"
#include "src/gpu/GrTextureProxy.h"
+#include "src/gpu/SkGr.h"
#include "src/gpu/effects/GrMatrixConvolutionEffect.h"
#endif
diff --git a/src/gpu/GrTextureProxy.h b/src/gpu/GrTextureProxy.h
index a0ac99a..37ac1b1 100644
--- a/src/gpu/GrTextureProxy.h
+++ b/src/gpu/GrTextureProxy.h
@@ -8,6 +8,7 @@
#ifndef GrTextureProxy_DEFINED
#define GrTextureProxy_DEFINED
+#include "include/gpu/GrBackendSurface.h"
#include "src/gpu/GrSamplerState.h"
#include "src/gpu/GrSurfaceProxy.h"
diff --git a/src/gpu/SkGr.cpp b/src/gpu/SkGr.cpp
index 7a593bb..41a2ede 100644
--- a/src/gpu/SkGr.cpp
+++ b/src/gpu/SkGr.cpp
@@ -163,10 +163,14 @@
return proxy;
}
-std::tuple<GrSurfaceProxyView, GrColorType>
-GrMakeCachedBitmapProxyView(GrRecordingContext* rContext,
- const SkBitmap& bitmap,
- GrMipmapped mipmapped) {
+static std::tuple<GrSurfaceProxyView, GrColorType>
+make_cached_bitmap_view(GrRecordingContext* rContext,
+ const SkBitmap& bitmap,
+ GrMipmapped mipmapped,
+ uint32_t cacheID,
+ bool addListenerToBitmap) {
+ SkASSERT(cacheID != SK_InvalidUniqueID);
+
if (!bitmap.peekPixels(nullptr)) {
return {};
}
@@ -177,14 +181,19 @@
GrUniqueKey key;
SkIPoint origin = bitmap.pixelRefOrigin();
SkIRect subset = SkIRect::MakePtSize(origin, bitmap.dimensions());
- GrMakeKeyFromImageID(&key, bitmap.pixelRef()->getGenerationID(), subset);
+ GrMakeKeyFromImageID(&key, cacheID, subset);
mipmapped = adjust_mipmapped(mipmapped, bitmap, caps);
GrColorType ct = choose_bmp_texture_colortype(caps, bitmap);
auto installKey = [&](GrTextureProxy* proxy) {
- auto listener = GrMakeUniqueKeyInvalidationListener(&key, proxyProvider->contextID());
- bitmap.pixelRef()->addGenIDChangeListener(std::move(listener));
+ if (cacheID == bitmap.getGenerationID()) {
+ if (addListenerToBitmap) {
+ auto listener = GrMakeUniqueKeyInvalidationListener(&key,
+ proxyProvider->contextID());
+ bitmap.pixelRef()->addGenIDChangeListener(std::move(listener));
+ }
+ }
proxyProvider->assignUniqueKeyToProxy(key, proxy);
};
@@ -230,6 +239,22 @@
}
std::tuple<GrSurfaceProxyView, GrColorType>
+GrMakeCachedBitmapProxyViewWithID(GrRecordingContext* rContext,
+ const SkBitmap& bitmap,
+ GrMipmapped mipmapped,
+ uint32_t cacheID) {
+ return make_cached_bitmap_view(rContext, bitmap, mipmapped, cacheID, /*add listener*/ false);
+}
+
+std::tuple<GrSurfaceProxyView, GrColorType>
+GrMakeCachedBitmapProxyView(GrRecordingContext* rContext,
+ const SkBitmap& bitmap,
+ GrMipmapped mipmapped) {
+ uint32_t cacheID = bitmap.getGenerationID();
+ return make_cached_bitmap_view(rContext, bitmap, mipmapped, cacheID, /*add listener*/ true);
+}
+
+std::tuple<GrSurfaceProxyView, GrColorType>
GrMakeUncachedBitmapProxyView(GrRecordingContext* rContext,
const SkBitmap& bitmap,
GrMipmapped mipmapped,
diff --git a/src/gpu/SkGr.h b/src/gpu/SkGr.h
index 8dba066..d14cfca 100644
--- a/src/gpu/SkGr.h
+++ b/src/gpu/SkGr.h
@@ -199,7 +199,19 @@
* if mipmapping isn't supported or for a 1x1 bitmap. If GrMipmapped is kNo it indicates mipmaps
* aren't required but a previously created mipmapped texture may still be returned. A color type is
* returned as color type conversion may be performed if there isn't a texture format equivalent of
- * the bitmap's color type.
+ * the bitmap's color type. cacheID is used in the cache key and is from the shared namespace of
+ * SkBitmap generation IDs and SkImage unique IDs. It must be valid.
+ */
+std::tuple<GrSurfaceProxyView, GrColorType>
+GrMakeCachedBitmapProxyViewWithID(GrRecordingContext*,
+ const SkBitmap&,
+ GrMipmapped,
+ uint32_t cacheID);
+
+/**
+ * Like GrMakeCachedBitmapProxyViewWithID but uses the bitmap's generation ID as the cache ID and
+ * also may add a listener to the SkBitmap that will invalidate the cached texture if the bitmap
+ * is deleted or its contents change.
*/
std::tuple<GrSurfaceProxyView, GrColorType>
GrMakeCachedBitmapProxyView(GrRecordingContext*,
@@ -207,8 +219,8 @@
GrMipmapped = GrMipmapped::kNo);
/**
- * Like above but always uploads the bitmap and never inserts into the cache. Unlike above, the
- * texture may be approx or scratch and budgeted or not.
+ * Like GrMakeCachedBitmapProxyView but always uploads the bitmap and never inserts into the cache.
+ * Unlike GrMakeCachedBitmapProxyView, the texture may be approx or scratch and budgeted or not.
*/
std::tuple<GrSurfaceProxyView, GrColorType>
GrMakeUncachedBitmapProxyView(GrRecordingContext*,
diff --git a/src/image/SkImage_Raster.cpp b/src/image/SkImage_Raster.cpp
index 52fe5ee..08b2545 100644
--- a/src/image/SkImage_Raster.cpp
+++ b/src/image/SkImage_Raster.cpp
@@ -212,6 +212,7 @@
} else {
SkASSERT(fPinnedCount == 0);
SkASSERT(fPinnedUniqueID == 0);
+ // It's important that we use the bitmap's gen ID here and not the image's unique ID.
std::tie(fPinnedView, fPinnedColorType) = GrMakeCachedBitmapProxyView(rContext,
fBitmap,
GrMipmapped::kNo);
@@ -428,7 +429,7 @@
return {fPinnedView, fPinnedColorType};
}
if (policy == GrImageTexGenPolicy::kDraw) {
- return GrMakeCachedBitmapProxyView(rContext, fBitmap, mipmapped);
+ return GrMakeCachedBitmapProxyViewWithID(rContext, fBitmap, mipmapped, this->uniqueID());
}
auto budgeted = (policy == GrImageTexGenPolicy::kNew_Uncached_Unbudgeted)
? SkBudgeted::kNo
diff --git a/tests/PinnedImageTest.cpp b/tests/PinnedImageTest.cpp
index d29fdd1..de9e788 100644
--- a/tests/PinnedImageTest.cpp
+++ b/tests/PinnedImageTest.cpp
@@ -51,7 +51,9 @@
sk_sp<SkSurface> gpuSurface = SkSurface::MakeRenderTarget(rContext, SkBudgeted::kYes, ii);
SkCanvas* canvas = gpuSurface->getCanvas();
- // w/o pinning - the gpu draw always reflects the current state of the underlying bitmap
+ // w/o pinning - the gpu caches the contents of the image and assumes the bitmap is immutable.
+ // This is actually undefined but we're assuming the GPU backend will cache the original and
+ // that it won't be purged before this test ends.
{
canvas->drawImage(img, 0, 0);
REPORTER_ASSERT(reporter, surface_is_expected_color(gpuSurface.get(), ii, SK_ColorRED));
@@ -59,28 +61,29 @@
bmCanvas.clear(SK_ColorGREEN);
canvas->drawImage(img, 0, 0);
- REPORTER_ASSERT(reporter, surface_is_expected_color(gpuSurface.get(), ii, SK_ColorGREEN));
+ REPORTER_ASSERT(reporter, surface_is_expected_color(gpuSurface.get(), ii, SK_ColorRED));
}
// w/ pinning - the gpu draw is stuck at the pinned state
{
+ bmCanvas.clear(SK_ColorBLUE);
SkImage_pinAsTexture(img.get(), rContext); // pin at blue
canvas->drawImage(img, 0, 0);
- REPORTER_ASSERT(reporter, surface_is_expected_color(gpuSurface.get(), ii, SK_ColorGREEN));
+ REPORTER_ASSERT(reporter, surface_is_expected_color(gpuSurface.get(), ii, SK_ColorBLUE));
- bmCanvas.clear(SK_ColorBLUE);
+ bmCanvas.clear(SK_ColorGREEN);
canvas->drawImage(img, 0, 0);
- REPORTER_ASSERT(reporter, surface_is_expected_color(gpuSurface.get(), ii, SK_ColorGREEN));
+ REPORTER_ASSERT(reporter, surface_is_expected_color(gpuSurface.get(), ii, SK_ColorBLUE));
SkImage_unpinAsTexture(img.get(), rContext);
}
- // once unpinned local changes will be picked up
+ // once unpinned revert to original cached texture
{
canvas->drawImage(img, 0, 0);
- REPORTER_ASSERT(reporter, surface_is_expected_color(gpuSurface.get(), ii, SK_ColorBLUE));
+ REPORTER_ASSERT(reporter, surface_is_expected_color(gpuSurface.get(), ii, SK_ColorRED));
}
}