Remove Ganesh and Graphite code from SkImage::subset

This makes both implementations more consistent and easier to decouple if there is no GPU backend compiled in. Graphite images are no longer uploaded as a texture-backed image if a recorder is passed in and Lazy images are turned into a raster image before being subset (instead of a texture-backed image). In practice, this should only impact skp images (which were further cleaned up in [1]). In some cases, we want to be able to directly create a texture- based subset, so a new static function does that SkImages::SubsetTextureFrom. This currently has a trivial implementation, but we might do something smarter, especially with larger textures. [1] https://skia-review.googlesource.com/c/skia/+/678656 Change-Id: I28f06a96fa451272c77a5fd06e97b2a4ad5b77b3 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/671679 Commit-Queue: Kevin Lubick <[email protected]> Reviewed-by: Brian Osman <[email protected]>
armansito · Apr 28, 2023 · 14d319b · 14d319b
1 parent 6858863
commit 14d319b
Show file tree

Hide file tree

Showing 42 changed files with 318 additions and 167 deletions.
diff --git a/docs/examples/Image_encodeToData.cpp b/docs/examples/Image_encodeToData.cpp
@@ -11,7 +11,8 @@ void draw(SkCanvas* canvas) {
         SkJpegEncoder::Options options;
         options.fQuality = quality;
         sk_sp<SkData> data(SkJpegEncoder::Encode(nullptr, image.get(), options));
-        sk_sp<SkImage> filtered = SkImages::DeferredFromEncodedData(data)->makeSubset(subset);
+        sk_sp<SkImage> filtered = SkImages::DeferredFromEncodedData(data)->
+                makeSubset(nullptr, subset);
         canvas->drawImage(filtered, x, 0);
         x += 16;
     }

diff --git a/docs/examples/Image_encodeToData_2.cpp b/docs/examples/Image_encodeToData_2.cpp
@@ -12,7 +12,7 @@ void draw(SkCanvas* canvas) {
     if (!data) {
         data = SkPngEncoder::Encode(nullptr, image.get(), {});
     }
-    sk_sp<SkImage> eye = SkImages::DeferredFromEncodedData(data)->makeSubset(subset);
+    sk_sp<SkImage> eye = SkImages::DeferredFromEncodedData(data)->makeSubset(nullptr, subset);
     canvas->drawImage(eye, 0, 0);
 }
 }  // END FIDDLE
diff --git a/docs/examples/Image_makeSubset.cpp b/docs/examples/Image_makeSubset.cpp
@@ -9,7 +9,7 @@ void draw(SkCanvas* canvas) {
     const int height = 64;
     for (int y = 0; y < 512; y += height ) {
         for (int x = 0; x < 512; x += width ) {
-            sk_sp<SkImage> subset(image->makeSubset({x, y, x + width, y + height}));
+            sk_sp<SkImage> subset(image->makeSubset(nullptr, {x, y, x + width, y + height}));
             canvas->drawImage(subset, x * 3 / 2, y * 3 / 2);
         }
     }

diff --git a/docs/examples/image_subsets_get_different_uids.cpp b/docs/examples/image_subsets_get_different_uids.cpp
@@ -5,8 +5,8 @@ REG_FIDDLE(image_subsets_get_different_uids, 128, 128, true, 5) {
 // image_subsets_get_different_uids
 void draw(SkCanvas*) {
     SkIRect r{0, 0, 10, 10};
-    auto s1 = image->makeSubset(r);
-    auto s2 = image->makeSubset(r);
+    auto s1 = image->makeSubset(nullptr, r);
+    auto s2 = image->makeSubset(nullptr, r);
     SkDebugf("%u\n%u\n", s1->uniqueID(), s2->uniqueID());
 }
 }  // END FIDDLE
diff --git a/docs/examples/skbug_237_drawImage_with_blur.cpp b/docs/examples/skbug_237_drawImage_with_blur.cpp
@@ -7,7 +7,7 @@ void draw(SkCanvas* canvas) {
     paint.setMaskFilter(SkMaskFilter::MakeBlur(kNormal_SkBlurStyle, 5.0f, false));
     canvas->clear(0xFF88FF88);
     canvas->scale(4, 4);
-    auto subset = image->makeSubset({16, 16, 48, 48});
+    auto subset = image->makeSubset(nullptr, {16, 16, 48, 48});
     canvas->drawImage(subset, 16, 16, SkSamplingOptions(), &paint);
 }
 }  // END FIDDLE
diff --git a/gm/coordclampshader.cpp b/gm/coordclampshader.cpp
@@ -21,7 +21,7 @@ DEF_SIMPLE_GM(coordclampshader, canvas, 1074, 795) {
         return;
     }
     // The mandrill_512 image has a bottom row of mostly black pixels. Remove it.
-    image = image->makeSubset(SkIRect::MakeWH(image->width(), image->height() - 1));
+    image = image->makeSubset(nullptr, SkIRect::MakeWH(image->width(), image->height() - 1));
     image = image->withDefaultMipmaps();
 
     auto imageShader = image->makeShader(SkSamplingOptions{SkFilterMode::kLinear});

diff --git a/gm/crop_imagefilter.cpp b/gm/crop_imagefilter.cpp
@@ -182,7 +182,7 @@ void draw_example(
         SkRect clippedContentBounds;
         if (clippedContentBounds.intersect(contentBounds, kExampleBounds)) {
             auto contentImage = ToolUtils::MakeTextureImage(
-                    canvas, image->makeSubset(clippedContentBounds.roundOut()));
+                    canvas, image->makeSubset(nullptr, clippedContentBounds.roundOut()));
             if (contentImage) {
                 SkPaint tiledPaint;
                 tiledPaint.setShader(contentImage->makeShader(

diff --git a/gm/crosscontextimage.cpp b/gm/crosscontextimage.cpp
@@ -54,8 +54,9 @@ DEF_SIMPLE_GPU_GM_CAN_FAIL(cross_context_image, rContext, canvas, errorMsg,
         canvas->drawImage(images[i], 0, 0);
         canvas->translate(0, 256 + 10);
 
-        canvas->drawImage(images[i]->makeSubset(SkIRect::MakeXYWH(64, 64, 128, 128), dContext),
-                          0, 0);
+        auto subset = images[i]->makeSubset(dContext, SkIRect::MakeXYWH(64, 64, 128, 128));
+        SkASSERTF(subset, "subset was null");
+        canvas->drawImage(subset, 0, 0);
         canvas->translate(128, 0);
 
         canvas->drawImageRect(images[i], SkRect::MakeWH(128, 128),

diff --git a/gm/drawbitmaprect.cpp b/gm/drawbitmaprect.cpp
@@ -145,11 +145,11 @@ static void imagesubsetproc(SkCanvas* canvas, sk_sp<SkImage> image, const SkBitm
     }
 
     auto direct = GrAsDirectContext(canvas->recordingContext());
-    if (sk_sp<SkImage> subset = image->makeSubset(srcR, direct)) {
+    if (sk_sp<SkImage> subset = image->makeSubset(direct, srcR)) {
         canvas->drawImageRect(subset, dstR, sampling, paint);
     }
 #if defined(SK_GRAPHITE)
-    if (sk_sp<SkImage> subset = image->makeSubset(srcR, canvas->recorder())) {
+    if (sk_sp<SkImage> subset = image->makeSubset(canvas->recorder(), srcR, {})) {
         canvas->drawImageRect(subset, dstR, sampling, paint);
     }
 #endif

diff --git a/gm/drawimageset.cpp b/gm/drawimageset.cpp
@@ -92,7 +92,7 @@ static void make_image_tiles(int tileW, int tileH, int m, int n, const SkColor c
                 subset.fBottom = h;
                 set[y * m + x].fAAFlags |= SkCanvas::kBottom_QuadAAFlag;
             }
-            set[y * m + x].fImage = fullImage->makeSubset(subset);
+            set[y * m + x].fImage = fullImage->makeSubset(nullptr, subset);
             set[y * m + x].fSrcRect =
                     SkRect::MakeXYWH(x == 0 ? 0 : 1, y == 0 ? 0 : 1, tileW, tileH);
             set[y * m + x].fDstRect = SkRect::MakeXYWH(x * tileW, y * tileH, tileW, tileH);

diff --git a/gm/image.cpp b/gm/image.cpp
@@ -508,11 +508,11 @@ DEF_SIMPLE_GM_CAN_FAIL(image_subset, canvas, errorMsg, 440, 220) {
 
 #if defined(SK_GRAPHITE)
     if (recorder) {
-        subset = img->makeSubset({100, 100, 200, 200}, recorder);
+        subset = img->makeSubset(recorder, {100, 100, 200, 200}, {});
     } else
 #endif
     {
-        subset = img->makeSubset({100, 100, 200, 200}, dContext);
+        subset = img->makeSubset(dContext, {100, 100, 200, 200});
     }
 
     canvas->drawImage(subset, 220, 10);

diff --git a/gm/image_pict.cpp b/gm/image_pict.cpp
@@ -317,16 +317,16 @@ class ImageCacheratorGM : public skiagm::GM {
                     auto textureGen = std::unique_ptr<GrTextureGenerator>(
                         static_cast<GrTextureGenerator*>(gen.release()));
                     fImageSubset = SkImages::DeferredFromTextureGenerator(std::move(textureGen))
-                            ->makeSubset(subset, dContext);
+                            ->makeSubset(dContext, subset);
                 } else {
                     fImageSubset = SkImages::DeferredFromGenerator(std::move(gen))
-                            ->makeSubset(subset, dContext);
+                            ->makeSubset(dContext, subset);
                 }
             } else {
 #if defined(SK_GRAPHITE)
                 auto recorder = canvas->recorder();
                 fImageSubset = SkImages::DeferredFromGenerator(std::move(gen))
-                                       ->makeSubset(subset, recorder);
+                                       ->makeSubset(recorder, subset, {});
 #endif
             }
             if (!fImageSubset) {

diff --git a/gm/imagemasksubset.cpp b/gm/imagemasksubset.cpp
@@ -94,10 +94,10 @@ DEF_SIMPLE_GM(imagemasksubset, canvas, 480, 480) {
             sk_sp<SkImage> subset;
 
             if (auto direct = GrAsDirectContext(canvas->recordingContext())) {
-                subset = image->makeSubset(kSubset, direct);
+                subset = image->makeSubset(direct, kSubset);
             } else {
 #if defined(SK_GRAPHITE)
-                subset = image->makeSubset(kSubset, canvas->recorder());
+                subset = image->makeSubset(canvas->recorder(), kSubset, {});
 #endif
             }
 

diff --git a/gm/perspimages.cpp b/gm/perspimages.cpp
@@ -26,7 +26,8 @@ using namespace skia_private;
 static sk_sp<SkImage> make_image1() { return GetResourceAsImage("images/mandrill_128.png"); }
 
 static sk_sp<SkImage> make_image2() {
-    return GetResourceAsImage("images/brickwork-texture.jpg")->makeSubset({0, 0, 128, 128});
+    return GetResourceAsImage("images/brickwork-texture.jpg")->
+            makeSubset(nullptr, {0, 0, 128, 128});
 }
 
 namespace skiagm {

diff --git a/gm/wacky_yuv_formats.cpp b/gm/wacky_yuv_formats.cpp
@@ -1134,7 +1134,7 @@ class YUVMakeColorSpaceGM : public GM {
                     y += kTileWidthHeight + kPad;
 
                     SkIRect bounds = SkIRect::MakeWH(kTileWidthHeight / 2, kTileWidthHeight / 2);
-                    auto subset = yuv->makeSubset(bounds, dContext);
+                    auto subset = SkImages::SubsetTextureFrom(dContext, yuv.get(), bounds);
                     SkASSERT(subset);
                     canvas->drawImage(subset, x, y);
                     y += kTileWidthHeight + kPad;

diff --git a/gm/yuv420_odd_dim.cpp b/gm/yuv420_odd_dim.cpp
@@ -102,7 +102,7 @@ DEF_SIMPLE_GM_CAN_FAIL(yuv420_odd_dim_repeat, canvas, errMsg,
     // Make sure the image is odd dimensioned.
     int w = image->width()  & 0b1 ? image->width()  : image->width()  - 1;
     int h = image->height() & 0b1 ? image->height() : image->height() - 1;
-    image = image->makeSubset(SkIRect::MakeWH(w, h));
+    image = image->makeSubset(nullptr, SkIRect::MakeWH(w, h));
 
     auto [planes, yuvaInfo] = sk_gpu_test::MakeYUVAPlanesAsA8(image.get(),
                                                               kJPEG_SkYUVColorSpace,

diff --git a/include/core/SkImage.h b/include/core/SkImage.h
@@ -680,19 +680,27 @@ class SK_API SkImage : public SkRefCnt {
         Returns nullptr if any of the following are true:
           - Subset is empty
           - Subset is not contained inside the image's bounds
-          - Pixels in the image could not be read or copied
+          - Pixels in the source image could not be read or copied
+          - This image is texture-backed and the provided context is null or does not match
+            the source image's context.
 
-        If this image is texture-backed, the context parameter is required and must match the
-        context of the source image. If the context parameter is provided, and the image is
-        raster-backed, the subset will be converted to texture-backed.
+        If the source image was texture-backed, the resulting image will be texture-backed also.
+        Otherwise, the returned image will be raster-backed.
 
+        @param direct  the GrDirectContext of the source image (nullptr is ok if the source image
+                       is not texture-backed).
         @param subset  bounds of returned SkImage
-        @param context the GrDirectContext in play, if it exists
         @return        the subsetted image, or nullptr
 
         example: https://fiddle.skia.org/c/@Image_makeSubset
     */
-    sk_sp<SkImage> makeSubset(const SkIRect& subset, GrDirectContext* direct = nullptr) const;
+    virtual sk_sp<SkImage> makeSubset(GrDirectContext* direct, const SkIRect& subset) const = 0;
+
+#if !defined(SK_DISABLE_LEGACY_IMAGE_SUBSET_METHODS)
+    sk_sp<SkImage> makeSubset(const SkIRect& subset, GrDirectContext* direct = nullptr) const {
+        return this->makeSubset(direct, subset);
+    }
+#endif
 
     /**
      *  Returns true if the image has mipmap levels.
@@ -882,20 +890,29 @@ class SK_API SkImage : public SkRefCnt {
           - Subset is empty
           - Subset is not contained inside the image's bounds
           - Pixels in the image could not be read or copied
+          - This image is texture-backed and the provided context is null or does not match
+            the source image's context.
 
-        If this image is texture-backed, the recorder parameter is required.
-        If the recorder parameter is provided, and the image is raster-backed, the subset will
-        be converted to texture-backed.
+        If the source image was texture-backed, the resulting image will be texture-backed also.
+        Otherwise, the returned image will be raster-backed.
 
+        @param recorder                 the recorder of the source image (nullptr is ok if the
+                                        source image was texture-backed).
         @param subset                   bounds of returned SkImage
-        @param recorder                 the recorder in which to create the new image
-        @param RequiredImageProperties  properties the returned SkImage must possess (e.g.,
-                                        mipmaps)
+        @param RequiredImageProperties  properties the returned SkImage must possess (e.g. mipmaps)
         @return                         the subsetted image, or nullptr
     */
+    sk_sp<SkImage> makeSubset(skgpu::graphite::Recorder*,
+                              const SkIRect& subset,
+                              RequiredImageProperties) const;
+
+#if !defined(SK_DISABLE_LEGACY_IMAGE_SUBSET_METHODS)
     sk_sp<SkImage> makeSubset(const SkIRect& subset,
-                              skgpu::graphite::Recorder*,
-                              RequiredImageProperties = {}) const;
+                              skgpu::graphite::Recorder* r,
+                              RequiredImageProperties props = {}) const {
+        return this->makeSubset(r, subset, props);
+    }
+#endif
 
     /** Creates SkImage in target SkColorSpace.
         Returns nullptr if SkImage could not be created.
@@ -948,7 +965,7 @@ class SK_API SkImage : public SkRefCnt {
 
         example: https://fiddle.skia.org/c/@Image_makeNonTextureImage
     */
-    sk_sp<SkImage> makeNonTextureImage() const;
+    sk_sp<SkImage> makeNonTextureImage(GrDirectContext* = nullptr) const;
 
     /** Returns raster image. Copies SkImage backed by GPU texture into CPU memory,
         or decodes SkImage from lazy image. Returns original SkImage if decoded in
@@ -963,7 +980,14 @@ class SK_API SkImage : public SkRefCnt {
 
         example: https://fiddle.skia.org/c/@Image_makeRasterImage
     */
-    sk_sp<SkImage> makeRasterImage(CachingHint cachingHint = kDisallow_CachingHint) const;
+    sk_sp<SkImage> makeRasterImage(GrDirectContext*,
+                                   CachingHint cachingHint = kDisallow_CachingHint) const;
+
+#if !defined(SK_IMAGE_READ_PIXELS_DISABLE_LEGACY_API)
+    sk_sp<SkImage> makeRasterImage(CachingHint cachingHint = kDisallow_CachingHint) const {
+        return this->makeRasterImage(nullptr, cachingHint);
+    }
+#endif
 
     /** Creates filtered SkImage. filter processes original SkImage, potentially changing
         color, position, and size. subset is the bounds of original SkImage processed

diff --git a/include/gpu/ganesh/SkImageGanesh.h b/include/gpu/ganesh/SkImageGanesh.h
@@ -32,6 +32,7 @@ class SkYUVAPixmaps;
 enum SkAlphaType : int;
 enum SkColorType : int;
 enum class SkTextureCompressionType;
+struct SkIRect;
 struct SkISize;
 
 /**
@@ -380,6 +381,22 @@ inline bool GetBackendTextureFromImage(GrDirectContext* context,
                                        backendTextureReleaseProc);
 }
 
+/** Returns subset of this image as a texture-backed image.
+
+    Returns nullptr if any of the following are true:
+      - Subset is empty
+      - Subset is not contained inside the image's bounds
+      - Pixels in the source image could not be read or copied
+      - The source image is texture-backed and context does not match the source image's context.
+
+    @param context the non-null GrDirectContext to which the subset should be uploaded.
+    @param subset  bounds of returned SkImage
+    @return        the subsetted image, uploaded as a texture, or nullptr
+*/
+SK_API sk_sp<SkImage> SubsetTextureFrom(GrDirectContext* context,
+                                        const SkImage* img,
+                                        const SkIRect& subset);
+
 }  // namespace SkImages
 
 #endif
diff --git a/include/gpu/graphite/Image.h b/include/gpu/graphite/Image.h
@@ -0,0 +1,42 @@
+/*
+ * Copyright 2023 Google LLC
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#ifndef skgpu_graphite_Image_DEFINED
+#define skgpu_graphite_Image_DEFINED
+
+#include "include/core/SkImage.h"
+#include "include/core/SkRefCnt.h"
+
+struct SkIRect;
+
+namespace skgpu::graphite {
+    class Recorder;
+}
+
+namespace SkImages {
+/** Returns subset of this image as a texture-backed image.
+
+    Returns nullptr if any of the following are true:
+      - Subset is empty
+      - Subset is not contained inside the image's bounds
+      - Pixels in the source image could not be read or copied
+      - The source image is texture-backed and context does not match the source image's context.
+
+    @param recorder the non-null recorder in which to create the new image.
+    @param img     Source image
+    @param subset  bounds of returned SkImage
+    @param props   properties the returned SkImage must possess (e.g. mipmaps)
+    @return        the subsetted image, uploaded as a texture, or nullptr
+*/
+SK_API sk_sp<SkImage> SubsetTextureFrom(skgpu::graphite::Recorder* recorder,
+                                        const SkImage* img,
+                                        const SkIRect& subset,
+                                        SkImage::RequiredImageProperties props = {});
+} // namespace SkImages
+
+
+#endif // skgpu_graphite_Image_DEFINED
diff --git a/relnotes/skimage_subset.md b/relnotes/skimage_subset.md
@@ -0,0 +1,14 @@
+`SkImage::subset` now takes a `GrDirectContext*` as its first parameter (this can be `nullptr` for
+non-gpu backed images. Images which are backed by a codec or picture will not be turned into a GPU
+texture before being read. This should only impact picture-backed images, which may not be read
+correctly if the picture contain nested texture-backed images itself. To force a conversion to
+a texture, clients should call `SkImages::TextureFromImage`, passing in the image, and then call
+subset on the result. Documentation has been clarified that `SkImage::subset` will return a raster-
+backed image if the source is not backed by a texture, and texture-otherwise.
+
+`SkImages::SubsetTextureFrom` has been added to subset an image and explicitly return a texture-
+backed image. This allows some optimizations, especially for large images that exceed a maximum
+texture size of a GPU.
+
+`SkImage::makeRasterImage` and `SkImage::makeNonTextureImage` now take a `GrDirectContext*` which
+clients should supply for reading-back pixels from texture-backed images.
diff --git a/src/core/SkReadBuffer.cpp b/src/core/SkReadBuffer.cpp
@@ -412,7 +412,7 @@ sk_sp<SkImage> SkReadBuffer::readImage() {
         SkIRect subset;
         this->readIRect(&subset);
         if (image) {
-            image = image->makeSubset(subset);
+            image = image->makeSubset(nullptr, subset);
         }
     }