Revert "Never share pixels when we make a subset image"

This reverts commit 3eff245.

Reason for revert: did I break abandongpu bot?

Original change's description:
> Never share pixels when we make a subset image
> 
> - mostly just affects Lazy images (raster was already always copying)
> - issue with other factories that offer subsetting
>    - (e.g. MakeFromGenerator)
>    - can we remove those options, and require makeSubset() afterwards?
> - greatly simplifies dealing with mipmaps
> 
> Landing this would obsolete
> https://skia-review.googlesource.com/c/skia/+/305960
> 
> Related: https://skia-review.googlesource.com/c/skia/+/306136
> 
> Related: https://bugs.chromium.org/p/skia/issues/detail?id=10544
> 
> Change-Id: I074420543bd2fcd46ed1620bbf0eed00371ab018
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/305970
> Commit-Queue: Mike Reed <reed@google.com>
> Reviewed-by: Leon Scroggins <scroggo@google.com>

TBR=mtklein@google.com,bsalomon@google.com,robertphillips@google.com,scroggo@google.com,brianosman@google.com,reed@google.com

Change-Id: Iab2f92855fe61e48f0e432b7257eb7ddef78fcfa
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/306377
Reviewed-by: Mike Reed <reed@google.com>

Author: reed-at-google

gm/image_pict.cpp

@@ -264,8 +264,7 @@ class ImageCacheratorGM : public skiagm::GM {
         if (!gen) {
             return false;
         }
-        fImageSubset = SkImage::MakeFromGenerator(std::move(gen))->makeSubset(subset,
-                                                                              dContext);
+        fImageSubset = SkImage::MakeFromGenerator(std::move(gen))->makeSubset(subset);
 
         SkASSERT(fImage->dimensions() == SkISize::Make(100, 100));
         SkASSERT(fImageSubset->dimensions() == SkISize::Make(50, 50));

include/core/SkImage.h

@@ -1188,20 +1188,20 @@ class SK_API SkImage : public SkRefCnt {
     */
     sk_sp<SkData> refEncodedData() const;
 
-    /** Returns subset of this image.
+    /** Returns subset of SkImage. subset must be fully contained by SkImage dimensions().
+        The implementation may share pixels, or may copy them.
 
         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
+          - Subset is not contained by bounds
+          - Pixels in SkImage could not be read or copied
 
         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.
+        context of the source image.
 
         @param subset  bounds of returned SkImage
         @param context the GrDirectContext in play, if it exists
-        @return        the subsetted image, or nullptr
+        @return        partial or full SkImage, or nullptr
 
         example: https://fiddle.skia.org/c/@Image_makeSubset
     */

src/core/SkImagePriv.h

@@ -85,4 +85,10 @@ bool SkImage_pinAsTexture(const SkImage*, GrRecordingContext*);
  */
 void SkImage_unpinAsTexture(const SkImage*, GrRecordingContext*);
 
+/**
+ *  Returns the bounds of the image relative to its encoded buffer. For all non-lazy images,
+ *  this returns (0,0,width,height). For a lazy-image, it may return a subset of that rect.
+ */
+SkIRect SkImage_getSubset(const SkImage*);
+
 #endif

src/core/SkWriteBuffer.cpp

@@ -151,6 +151,10 @@ bool SkBinaryWriteBuffer::writeToStream(SkWStream* stream) const {
  */
 void SkBinaryWriteBuffer::writeImage(const SkImage* image) {
     uint32_t flags = 0;
+    const SkIRect r = SkImage_getSubset(image);
+    if (r.x() != 0 || r.y() != 0 || r.width() != image->width() || r.height() != image->height()) {
+        flags |= SkWriteBufferImageFlags::kHasSubsetRect;
+    }
     const SkMipmap* mips = as_IB(image)->onPeekMips();
     if (mips) {
         flags |= SkWriteBufferImageFlags::kHasMipmap;
@@ -167,8 +171,12 @@ void SkBinaryWriteBuffer::writeImage(const SkImage* image) {
     }
     this->writeDataAsByteArray(data.get());
 
+    if (flags & SkWriteBufferImageFlags::kHasSubsetRect) {
+        this->writeIRect(r);
+    }
     if (flags & SkWriteBufferImageFlags::kHasMipmap) {
         this->writeDataAsByteArray(mips->serialize().get());
+
     }
 }
 

src/image/SkImage.cpp

@@ -654,6 +654,11 @@ void SkImage_unpinAsTexture(const SkImage* image, GrRecordingContext* rContext)
     as_IB(image)->onUnpinAsTexture(rContext);
 }
 
+SkIRect SkImage_getSubset(const SkImage* image) {
+    SkASSERT(image);
+    return as_IB(image)->onGetSubset();
+}
+
 ///////////////////////////////////////////////////////////////////////////////////////////////////
 
 SkMipmapBuilder::SkMipmapBuilder(const SkImageInfo& info) {

src/image/SkImage_Base.h

@@ -36,6 +36,10 @@ class SkImage_Base : public SkImage {
 public:
     ~SkImage_Base() override;
 
+    virtual SkIRect onGetSubset() const {
+        return { 0, 0, this->width(), this->height() };
+    }
+
     virtual bool onPeekPixels(SkPixmap*) const { return false; }
 
     virtual const SkBitmap* onPeekBitmap() const { return nullptr; }

src/image/SkImage_Lazy.cpp

@@ -61,27 +61,38 @@ class SharedGenerator final : public SkNVRefCnt<SharedGenerator> {
 
 ///////////////////////////////////////////////////////////////////////////////
 
-SkImage_Lazy::Validator::Validator(sk_sp<SharedGenerator> gen, const SkColorType* colorType,
-                                   sk_sp<SkColorSpace> colorSpace)
+SkImage_Lazy::Validator::Validator(sk_sp<SharedGenerator> gen, const SkIRect* subset,
+                                   const SkColorType* colorType, sk_sp<SkColorSpace> colorSpace)
         : fSharedGenerator(std::move(gen)) {
     if (!fSharedGenerator) {
         return;
     }
 
     // The following generator accessors are safe without acquiring the mutex (const getters).
     // TODO: refactor to use a ScopedGenerator instead, for clarity.
-    fInfo = fSharedGenerator->fGenerator->getInfo();
-    if (fInfo.isEmpty()) {
+    const SkImageInfo& info = fSharedGenerator->fGenerator->getInfo();
+    if (info.isEmpty()) {
         fSharedGenerator.reset();
         return;
     }
 
     fUniqueID = fSharedGenerator->fGenerator->uniqueID();
-
-    if (colorType && (*colorType == fInfo.colorType())) {
-        colorType = nullptr;
+    const SkIRect bounds = SkIRect::MakeWH(info.width(), info.height());
+    if (subset) {
+        if (!bounds.contains(*subset)) {
+            fSharedGenerator.reset();
+            return;
+        }
+        if (*subset != bounds) {
+            // we need a different uniqueID since we really are a subset of the raw generator
+            fUniqueID = SkNextID::ImageID();
+        }
+    } else {
+        subset = &bounds;
     }
 
+    fInfo   = info.makeDimensions(subset->size());
+    fOrigin = SkIPoint::Make(subset->x(), subset->y());
     if (colorType || colorSpace) {
         if (colorType) {
             fInfo = fInfo.makeColorType(*colorType);
@@ -120,15 +131,51 @@ class SkImage_Lazy::ScopedGenerator {
 ///////////////////////////////////////////////////////////////////////////////
 
 SkImage_Lazy::SkImage_Lazy(Validator* validator)
-    : INHERITED(validator->fInfo, validator->fUniqueID)
-    , fSharedGenerator(std::move(validator->fSharedGenerator))
-{
+        : INHERITED(validator->fInfo, validator->fUniqueID)
+        , fSharedGenerator(std::move(validator->fSharedGenerator))
+        , fOrigin(validator->fOrigin) {
     SkASSERT(fSharedGenerator);
 }
 
 
 //////////////////////////////////////////////////////////////////////////////////////////////////
 
+static bool generate_pixels(SkImageGenerator* gen, const SkPixmap& pmap, int originX, int originY) {
+    const int genW = gen->getInfo().width();
+    const int genH = gen->getInfo().height();
+    const SkIRect srcR = SkIRect::MakeWH(genW, genH);
+    const SkIRect dstR = SkIRect::MakeXYWH(originX, originY, pmap.width(), pmap.height());
+    if (!srcR.contains(dstR)) {
+        return false;
+    }
+
+    // If they are requesting a subset, we have to have a temp allocation for full image, and
+    // then copy the subset into their allocation
+    SkBitmap full;
+    SkPixmap fullPM;
+    const SkPixmap* dstPM = &pmap;
+    if (srcR != dstR) {
+        if (!full.tryAllocPixels(pmap.info().makeWH(genW, genH))) {
+            return false;
+        }
+        if (!full.peekPixels(&fullPM)) {
+            return false;
+        }
+        dstPM = &fullPM;
+    }
+
+    if (!gen->getPixels(dstPM->info(), dstPM->writable_addr(), dstPM->rowBytes())) {
+        return false;
+    }
+
+    if (srcR != dstR) {
+        if (!full.readPixels(pmap, originX, originY)) {
+            return false;
+        }
+    }
+    return true;
+}
+
 bool SkImage_Lazy::getROPixels(SkBitmap* bitmap, SkImage::CachingHint chint) const {
     auto check_output_bitmap = [bitmap]() {
         SkASSERT(bitmap->isImmutable());
@@ -145,14 +192,17 @@ bool SkImage_Lazy::getROPixels(SkBitmap* bitmap, SkImage::CachingHint chint) con
     if (SkImage::kAllow_CachingHint == chint) {
         SkPixmap pmap;
         SkBitmapCache::RecPtr cacheRec = SkBitmapCache::Alloc(desc, this->imageInfo(), &pmap);
-        if (!cacheRec || !ScopedGenerator(fSharedGenerator)->getPixels(pmap)) {
+        if (!cacheRec ||
+            !generate_pixels(ScopedGenerator(fSharedGenerator), pmap,
+                             fOrigin.x(), fOrigin.y())) {
             return false;
         }
         SkBitmapCache::Add(std::move(cacheRec), bitmap);
         this->notifyAddedToRasterCache();
     } else {
         if (!bitmap->tryAllocPixels(this->imageInfo()) ||
-            !ScopedGenerator(fSharedGenerator)->getPixels(bitmap->pixmap())) {
+            !generate_pixels(ScopedGenerator(fSharedGenerator), bitmap->pixmap(), fOrigin.x(),
+                             fOrigin.y())) {
             return false;
         }
         bitmap->setImmutable();
@@ -196,13 +246,14 @@ GrSurfaceProxyView SkImage_Lazy::refView(GrRecordingContext* context, GrMipmappe
 }
 #endif
 
-sk_sp<SkImage> SkImage_Lazy::onMakeSubset(const SkIRect& subset, GrDirectContext* direct) const {
-    // TODO: can we do this more efficiently, by telling the generator we want to
-    //       "realize" a subset?
+sk_sp<SkImage> SkImage_Lazy::onMakeSubset(const SkIRect& subset, GrDirectContext*) const {
+    SkASSERT(this->bounds().contains(subset));
+    SkASSERT(this->bounds() != subset);
 
-    auto pixels = direct ? this->makeTextureImage(direct)
-                         : this->makeRasterImage();
-    return pixels ? pixels->makeSubset(subset, direct) : nullptr;
+    const SkIRect generatorSubset = subset.makeOffset(fOrigin);
+    const SkColorType colorType = this->colorType();
+    Validator validator(fSharedGenerator, &generatorSubset, &colorType, this->refColorSpace());
+    return validator ? sk_sp<SkImage>(new SkImage_Lazy(&validator)) : nullptr;
 }
 
 sk_sp<SkImage> SkImage_Lazy::onMakeColorTypeAndColorSpace(SkColorType targetCT,
@@ -214,7 +265,9 @@ sk_sp<SkImage> SkImage_Lazy::onMakeColorTypeAndColorSpace(SkColorType targetCT,
         SkColorSpace::Equals(targetCS.get(), fOnMakeColorTypeAndSpaceResult->colorSpace())) {
         return fOnMakeColorTypeAndSpaceResult;
     }
-    Validator validator(fSharedGenerator, &targetCT, targetCS);
+    const SkIRect generatorSubset =
+            SkIRect::MakeXYWH(fOrigin.x(), fOrigin.y(), this->width(), this->height());
+    Validator validator(fSharedGenerator, &generatorSubset, &targetCT, targetCS);
     sk_sp<SkImage> result = validator ? sk_sp<SkImage>(new SkImage_Lazy(&validator)) : nullptr;
     if (result) {
         fOnMakeColorTypeAndSpaceResult = result;
@@ -232,7 +285,7 @@ sk_sp<SkImage> SkImage_Lazy::onReinterpretColorSpace(sk_sp<SkColorSpace> newCS)
     if (bitmap.tryAllocPixels(this->imageInfo().makeColorSpace(std::move(newCS)))) {
         SkPixmap pixmap = bitmap.pixmap();
         pixmap.setColorSpace(this->refColorSpace());
-        if (ScopedGenerator(fSharedGenerator)->getPixels(pixmap)) {
+        if (generate_pixels(ScopedGenerator(fSharedGenerator), pixmap, fOrigin.x(), fOrigin.y())) {
             bitmap.setImmutable();
             return SkImage::MakeFromBitmap(bitmap);
         }
@@ -242,17 +295,47 @@ sk_sp<SkImage> SkImage_Lazy::onReinterpretColorSpace(sk_sp<SkColorSpace> newCS)
 
 sk_sp<SkImage> SkImage::MakeFromGenerator(std::unique_ptr<SkImageGenerator> generator) {
     SkImage_Lazy::Validator
-            validator(SharedGenerator::Make(std::move(generator)), nullptr, nullptr);
+            validator(SharedGenerator::Make(std::move(generator)), nullptr, nullptr, nullptr);
 
     return validator ? sk_make_sp<SkImage_Lazy>(&validator) : nullptr;
 }
 
 sk_sp<SkImage> SkImage::DecodeToRaster(const void* encoded, size_t length, const SkIRect* subset) {
-    auto img = SkImage::MakeFromEncoded(SkData::MakeWithoutCopy(encoded, length));
-    if (img && subset) {
-        img = img->makeSubset(*subset);
+    // The generator will not outlive this function, so we can wrap the encoded data without copy
+    auto gen = SkImageGenerator::MakeFromEncoded(SkData::MakeWithoutCopy(encoded, length));
+    if (!gen) {
+        return nullptr;
+    }
+    SkImageInfo info = gen->getInfo();
+    if (info.isEmpty()) {
+        return nullptr;
+    }
+
+    SkIPoint origin = {0, 0};
+    if (subset) {
+        if (!SkIRect::MakeWH(info.width(), info.height()).contains(*subset)) {
+            return nullptr;
+        }
+        info = info.makeDimensions(subset->size());
+        origin = {subset->x(), subset->y()};
+    }
+
+    size_t rb = info.minRowBytes();
+    if (rb == 0) {
+        return nullptr; // rb was too big
+    }
+    size_t size = info.computeByteSize(rb);
+    if (size == SIZE_MAX) {
+        return nullptr;
     }
-    return img ? img->makeRasterImage() : nullptr;
+    auto data = SkData::MakeUninitialized(size);
+
+    SkPixmap pmap(info, data->writable_data(), rb);
+    if (!generate_pixels(gen.get(), pmap, origin.x(), origin.y())) {
+        return nullptr;
+    }
+
+    return SkImage::MakeRasterData(info, data, rb);
 }
 
 //////////////////////////////////////////////////////////////////////////////////////////////////
@@ -514,7 +597,7 @@ GrSurfaceProxyView SkImage_Lazy::lockTextureProxyView(GrRecordingContext* ctx,
     // 2. Ask the generator to natively create one.
     {
         ScopedGenerator generator(fSharedGenerator);
-        if (auto view = generator->generateTexture(ctx, this->imageInfo(), {0,0}, mipMapped,
+        if (auto view = generator->generateTexture(ctx, this->imageInfo(), fOrigin, mipMapped,
                                                    texGenPolicy)) {
             SK_HISTOGRAM_ENUMERATION("LockTexturePath", kNative_LockTexturePath,
                                      kLockTexturePathCount);

src/image/SkImage_Lazy.h

@@ -22,18 +22,24 @@ class SharedGenerator;
 class SkImage_Lazy : public SkImage_Base {
 public:
     struct Validator {
-        Validator(sk_sp<SharedGenerator>, const SkColorType*, sk_sp<SkColorSpace>);
+        Validator(sk_sp<SharedGenerator>, const SkIRect* subset, const SkColorType* colorType,
+                  sk_sp<SkColorSpace> colorSpace);
 
         operator bool() const { return fSharedGenerator.get(); }
 
         sk_sp<SharedGenerator> fSharedGenerator;
         SkImageInfo            fInfo;
+        SkIPoint               fOrigin;
         sk_sp<SkColorSpace>    fColorSpace;
         uint32_t               fUniqueID;
     };
 
     SkImage_Lazy(Validator* validator);
 
+    SkIRect onGetSubset() const override {
+        return SkIRect::MakeXYWH(fOrigin.fX, fOrigin.fY, this->width(), this->height());
+    }
+
     bool onReadPixels(const SkImageInfo&, void*, size_t, int srcX, int srcY,
                       CachingHint) const override;
 #if SK_SUPPORT_GPU
@@ -79,6 +85,7 @@ class SkImage_Lazy : public SkImage_Base {
     // cropped by onMakeSubset and its color type/space may be changed by
     // onMakeColorTypeAndColorSpace.
     sk_sp<SharedGenerator> fSharedGenerator;
+    const SkIPoint         fOrigin;
 
     // Repeated calls to onMakeColorTypeAndColorSpace will result in a proliferation of unique IDs
     // and SkImage_Lazy instances. Cache the result of the last successful call.