Reland "Reland "Use SkImage_Raster's unique ID to cache textures.""

Bug: skia:11983

Change-Id: Ib637af99f4abcd427570bd4dda31488fbcb14ab7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/414876
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>

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 @@ bool SkPictureImageGenerator::onGetPixels(const SkImageInfo& info, void* pixels,
 #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,

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
 

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"
 

src/gpu/SkGr.cpp

@@ -163,10 +163,13 @@ static sk_sp<GrTextureProxy> make_bmp_proxy(GrProxyProvider* proxyProvider,
     return proxy;
 }
 
-std::tuple<GrSurfaceProxyView, GrColorType>
+static std::tuple<GrSurfaceProxyView, GrColorType>
-GrMakeCachedBitmapProxyView(GrRecordingContext* rContext,
+make_cached_bitmap_view(GrRecordingContext* rContext,
-                            const SkBitmap& bitmap,
+                        const SkBitmap& bitmap,
-                            GrMipmapped mipmapped) {
+                        GrMipmapped mipmapped,
+                        uint32_t cacheID) {
+    SkASSERT(cacheID != SK_InvalidUniqueID);
+
     if (!bitmap.peekPixels(nullptr)) {
         return {};
     }
@@ -177,14 +180,17 @@ GrMakeCachedBitmapProxyView(GrRecordingContext* rContext,
     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());
+        if (cacheID == bitmap.getGenerationID()) {
-        bitmap.pixelRef()->addGenIDChangeListener(std::move(listener));
+            auto listener = GrMakeUniqueKeyInvalidationListener(&key,
+                                                                proxyProvider->contextID());
+            bitmap.pixelRef()->addGenIDChangeListener(std::move(listener));
+        }
         proxyProvider->assignUniqueKeyToProxy(key, proxy);
     };
 
@@ -229,6 +235,22 @@ GrMakeCachedBitmapProxyView(GrRecordingContext* rContext,
     return {{std::move(mippedProxy), kTopLeft_GrSurfaceOrigin, swizzle}, ct};
 }
 
+std::tuple<GrSurfaceProxyView, GrColorType>
+GrMakeCachedBitmapProxyViewWithID(GrRecordingContext* rContext,
+                                  const SkBitmap& bitmap,
+                                  GrMipmapped mipmapped,
+                                  uint32_t cacheID) {
+    return make_cached_bitmap_view(rContext, bitmap, mipmapped, cacheID);
+}
+
+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);
+}
+
 std::tuple<GrSurfaceProxyView, GrColorType>
 GrMakeUncachedBitmapProxyView(GrRecordingContext* rContext,
                               const SkBitmap& bitmap,

src/gpu/SkGr.h

@@ -199,7 +199,19 @@ GrSurfaceProxyView GrCopyBaseMipMapToView(GrRecordingContext*,
  * 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 @@ GrMakeCachedBitmapProxyView(GrRecordingContext*,
                             GrMipmapped = GrMipmapped::kNo);
 
 /**
- * Like above but always uploads the bitmap and never inserts into the cache. Unlike above, the
+ * Like GrMakeCachedBitmapProxyView but always uploads the bitmap and never inserts into the cache.
- * texture may be approx or scratch and budgeted or not.
+ * Unlike GrMakeCachedBitmapProxyView, the texture may be approx or scratch and budgeted or not.
  */
 std::tuple<GrSurfaceProxyView, GrColorType>
 GrMakeUncachedBitmapProxyView(GrRecordingContext*,

src/image/SkImage_Raster.cpp

@@ -212,6 +212,7 @@ bool SkImage_Raster::onPinAsTexture(GrRecordingContext* rContext) const {
     } 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);
@@ -429,7 +430,7 @@ std::tuple<GrSurfaceProxyView, GrColorType> SkImage_Raster::onAsView(
         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

tests/PinnedImageTest.cpp

@@ -51,7 +51,9 @@ static void basic_test(skiatest::Reporter* reporter, GrRecordingContext* rContex
     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,31 @@ static void basic_test(skiatest::Reporter* reporter, GrRecordingContext* rContex
         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 the original RED texture should be purged by SkBitmap's gen ID listener when
+    // we first modified it after making the image. We should upload and cache the now GREEN bitmap
+    // backing the image.
     {
         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_ColorGREEN));
     }
 }