Revert "Never share pixels when we make a subset image"
This reverts commit 3eff2459d4a19462a8f24fb0a39a1d11a2487b9c.
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>
diff --git a/gm/image_pict.cpp b/gm/image_pict.cpp
index de5c670..b399a89 100644
--- a/gm/image_pict.cpp
+++ b/gm/image_pict.cpp
@@ -264,8 +264,7 @@
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));
diff --git a/include/core/SkImage.h b/include/core/SkImage.h
index 4a7ef38..1c1fef0 100644
--- a/include/core/SkImage.h
+++ b/include/core/SkImage.h
@@ -1188,20 +1188,20 @@
*/
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
*/
diff --git a/src/core/SkImagePriv.h b/src/core/SkImagePriv.h
index 3b34d80..03e0c75 100644
--- a/src/core/SkImagePriv.h
+++ b/src/core/SkImagePriv.h
@@ -85,4 +85,10 @@
*/
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
diff --git a/src/core/SkWriteBuffer.cpp b/src/core/SkWriteBuffer.cpp
index ca6ab36..40a6a46 100644
--- a/src/core/SkWriteBuffer.cpp
+++ b/src/core/SkWriteBuffer.cpp
@@ -151,6 +151,10 @@
*/
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 @@
}
this->writeDataAsByteArray(data.get());
+ if (flags & SkWriteBufferImageFlags::kHasSubsetRect) {
+ this->writeIRect(r);
+ }
if (flags & SkWriteBufferImageFlags::kHasMipmap) {
this->writeDataAsByteArray(mips->serialize().get());
+
}
}
diff --git a/src/image/SkImage.cpp b/src/image/SkImage.cpp
index 23eb777..3f5655b 100644
--- a/src/image/SkImage.cpp
+++ b/src/image/SkImage.cpp
@@ -654,6 +654,11 @@
as_IB(image)->onUnpinAsTexture(rContext);
}
+SkIRect SkImage_getSubset(const SkImage* image) {
+ SkASSERT(image);
+ return as_IB(image)->onGetSubset();
+}
+
///////////////////////////////////////////////////////////////////////////////////////////////////
SkMipmapBuilder::SkMipmapBuilder(const SkImageInfo& info) {
diff --git a/src/image/SkImage_Base.h b/src/image/SkImage_Base.h
index f6e6fca..3796a8d 100644
--- a/src/image/SkImage_Base.h
+++ b/src/image/SkImage_Base.h
@@ -36,6 +36,10 @@
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; }
diff --git a/src/image/SkImage_Lazy.cpp b/src/image/SkImage_Lazy.cpp
index 231cf46..8b2677a 100644
--- a/src/image/SkImage_Lazy.cpp
+++ b/src/image/SkImage_Lazy.cpp
@@ -61,8 +61,8 @@
///////////////////////////////////////////////////////////////////////////////
-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;
@@ -70,18 +70,29 @@
// 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 @@
///////////////////////////////////////////////////////////////////////////////
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 @@
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 @@
}
#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 @@
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 @@
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::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;
}
- return img ? img->makeRasterImage() : 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;
+ }
+ 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 @@
// 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);
diff --git a/src/image/SkImage_Lazy.h b/src/image/SkImage_Lazy.h
index a2a5818..6ba3ef6 100644
--- a/src/image/SkImage_Lazy.h
+++ b/src/image/SkImage_Lazy.h
@@ -22,18 +22,24 @@
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 @@
// 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.