Fix issue mipmapped SkImages made from SkSurface_raster snapshots.

This fixes a bug with the following sequence:

Make SkSurface_Raster, draw to it
snap an image, A, from the surface
make image B by calling withDefaultMipmaps() on image A
Let image A be destroyed.
draw to surface again
snap another image, C, from the surface

Image C and image B would now share the same SkPixelRef, reflecting
the final contents of the SkSurface.

Add a GM that exercises the bug when run through either the pic- or
serialize- vias and DDL.

Bug: skia:13111
Change-Id: Ib079163c84f420baf62fa7960110386303b8fb93
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/525517
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
This commit is contained in:
Brian Salomon 2022-03-30 10:12:28 -04:00 committed by SkCQ
parent 3f5184cfa6
commit e4601b0864
4 changed files with 59 additions and 5 deletions

View File

@ -298,6 +298,47 @@ DEF_SURFACE_TESTS(simple_snap_image2, canvas, 256, 256) {
canvas->drawImage(std::move(image), 0, 0);
}
DEF_SIMPLE_GM(snap_with_mips, canvas, 80, 75) {
auto ct = canvas->imageInfo().colorType() == kUnknown_SkColorType
? kRGBA_8888_SkColorType
: canvas->imageInfo().colorType();
auto ii = SkImageInfo::Make({32, 32},
ct,
kPremul_SkAlphaType,
canvas->imageInfo().refColorSpace());
auto surface = SkSurface::MakeRaster(ii);
auto nextImage = [&](SkColor color) {
surface->getCanvas()->clear(color);
SkPaint paint;
paint.setColor(~color | 0xFF000000);
surface->getCanvas()->drawRect(SkRect::MakeLTRB(surface->width() *2/5.f,
surface->height()*2/5.f,
surface->width() *3/5.f,
surface->height()*3/5.f),
paint);
return surface->makeImageSnapshot()->withDefaultMipmaps();
};
static constexpr int kPad = 8;
static const SkSamplingOptions kSampling{SkFilterMode::kLinear, SkMipmapMode::kLinear};
canvas->save();
for (int y = 0; y < 3; ++y) {
canvas->save();
SkColor kColors[] = {0xFFF0F0F0, SK_ColorBLUE};
for (int x = 0; x < 2; ++x) {
auto image = nextImage(kColors[x]);
canvas->drawImage(image, 0, 0, kSampling);
canvas->translate(ii.width() + kPad, 0);
}
canvas->restore();
canvas->translate(0, ii.width() + kPad);
canvas->scale(.4f, .4f);
}
canvas->restore();
}
DEF_SURFACE_TESTS(copy_on_write_savelayer, canvas, 256, 256) {
const SkImageInfo info = SkImageInfo::MakeN32Premul(256, 256);
sk_sp<SkSurface> surf = make(info);

View File

@ -112,7 +112,9 @@ private:
void setTemporarilyImmutable();
void restoreMutability();
friend class SkSurface_Raster; // For the two methods above.
bool isTemporarilyImmutable();
friend class SkSurface_Raster; // For temporary immutable methods above.
friend class SkImage_Raster; // For temporary immutable methods above.
void setImmutableWithID(uint32_t genID);
friend void SkBitmapCache_setImmutableWithID(SkPixelRef*, uint32_t);

View File

@ -132,6 +132,8 @@ void SkPixelRef::restoreMutability() {
fMutability = kMutable;
}
bool SkPixelRef::isTemporarilyImmutable() { return fMutability == kTemporarilyImmutable; }
sk_sp<SkPixelRef> SkMakePixelRefWithProc(int width, int height, size_t rowBytes, void* addr,
void (*releaseProc)(void* addr, void* ctx), void* ctx) {
SkASSERT(width >= 0 && height >= 0);

View File

@ -135,13 +135,22 @@ public:
SkMipmap* onPeekMips() const override { return fBitmap.fMips.get(); }
sk_sp<SkImage> onMakeWithMipmaps(sk_sp<SkMipmap> mips) const override {
auto img = new SkImage_Raster(fBitmap);
// This SkImage may be a SkSurface's cached image snapshot. If so, we can't make a new image
// that shares the original image's backing SkPixelRef. This is because when copy-on-write
// is not triggered we just keep updating the backing SkPixelRef's contents. The SkPixelRefs
// that back cached SkImage snapshots are marked "temporarily immutable" so we look for that
// as our signal that we must copy.
auto copyMode = fBitmap.fPixelRef->isTemporarilyImmutable()
? SkCopyPixelsMode::kAlways_SkCopyPixelsMode
: SkCopyPixelsMode::kIfMutable_SkCopyPixelsMode;
sk_sp<SkImage> img = SkMakeImageFromRasterBitmap(fBitmap, copyMode);
auto imgRaster = static_cast<SkImage_Raster*>(img.get());
if (mips) {
img->fBitmap.fMips = std::move(mips);
imgRaster->fBitmap.fMips = std::move(mips);
} else {
img->fBitmap.fMips.reset(SkMipmap::Build(fBitmap.pixmap(), nullptr));
imgRaster->fBitmap.fMips.reset(SkMipmap::Build(fBitmap.pixmap(), nullptr));
}
return sk_sp<SkImage>(img);
return img;
}
private: