Reland "Combine snapBackImage and snapSpecial"
This reverts commit fd849a537a4bbb38114f94be8ffd0d359ebfa6f3.
Reason for revert: Magnifier filter fixed, layout tests shouldn't break on this change anymore.
Original change's description:
> Revert "Combine snapBackImage and snapSpecial"
>
> This reverts commit 45739aa1d8ca1b58e354fe82776d2264936c2f72.
>
> Reason for revert: Looks like the magnifier filter doesn't handle subset origins correctly either, not caught by our tests, but does in layout tests.
>
> Original change's description:
> > Combine snapBackImage and snapSpecial
> >
> > Previously, snapBackImage always made a copy since that is required for
> > its use in saveBehind(). Backdrop filters also used snapBackImage because
> > it relied on its subset SkIRect argument to avoid using the entire
> > layer as input to the backdrop filter. The regular snapSpecial() originally
> > did not take a subset at all. The GPU implementations of snapSpecial()
> > and snapBackImage() were very similar. This merges them into a single call
> > that takes a subset SkIRect and a boolean to control if the copy is required.
> >
> > Only saveBehind() requires that the copy is made, due to how it bypasses
> > the regular copy-on-write behavior managed by SkCanvas and SkSurface. The
> > no-argument saveSpecial() is still provided, but it is defined to just snap
> > the bounds of the device.
> >
> > Flutter noticed a fairly serious performance regression on iOS that went
> > back to this CL: https://github.com/google/skia/commit/08b260c27b519ac8fb17b3ff8ddccf4a2ac0e278. It was determined that the only significant change in
> > behavior that was active given their minimum working example was switching
> > from snapSpecial() to snapBackImage(). Hopefully moving to a snapSpecial that
> > takes a subset, but doesn't copy when there is a texture already available
> > will restore the performance.
> >
> > Bug: skia:9283, https://github.com/flutter/flutter/issues/36352, https://github.com/flutter/flutter/issues/37541, https://github.com/flutter/flutter/issues/36064
> > Change-Id: I448f8886347a1e0f4da8f61279744bb9254f7752
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/237127
> > Commit-Queue: Michael Ludwig <michaelludwig@google.com>
> > Reviewed-by: Brian Salomon <bsalomon@google.com>
> > Reviewed-by: Brian Osman <brianosman@google.com>
>
> TBR=bsalomon@google.com,brianosman@google.com,michaelludwig@google.com
>
> Change-Id: I7fd897da8a50b2fc0079e2aed9b6ad4bed6d3879
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: skia:9283, https://github.com/flutter/flutter/issues/36352, https://github.com/flutter/flutter/issues/37541, https://github.com/flutter/flutter/issues/36064
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/237616
> Reviewed-by: Michael Ludwig <michaelludwig@google.com>
> Commit-Queue: Michael Ludwig <michaelludwig@google.com>
TBR=bsalomon@google.com,brianosman@google.com,michaelludwig@google.com
Change-Id: Ieeb7e8c3b3261d9e900fea6e83a5726b4b6c86cd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:9283, https://github.com/flutter/flutter/issues/36352, https://github.com/flutter/flutter/issues/37541, https://github.com/flutter/flutter/issues/36064
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/237904
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
diff --git a/src/core/SkBitmapDevice.cpp b/src/core/SkBitmapDevice.cpp
index 0910587..a5f6653 100644
--- a/src/core/SkBitmapDevice.cpp
+++ b/src/core/SkBitmapDevice.cpp
@@ -716,12 +716,12 @@
image->makeNonTextureImage());
}
-sk_sp<SkSpecialImage> SkBitmapDevice::snapSpecial() {
- return this->makeSpecial(fBitmap);
-}
-
-sk_sp<SkSpecialImage> SkBitmapDevice::snapBackImage(const SkIRect& bounds) {
- return SkSpecialImage::CopyFromRaster(bounds, fBitmap, &this->surfaceProps());
+sk_sp<SkSpecialImage> SkBitmapDevice::snapSpecial(const SkIRect& bounds, bool forceCopy) {
+ if (forceCopy) {
+ return SkSpecialImage::CopyFromRaster(bounds, fBitmap, &this->surfaceProps());
+ } else {
+ return SkSpecialImage::MakeFromRaster(bounds, fBitmap);
+ }
}
///////////////////////////////////////////////////////////////////////////////
diff --git a/src/core/SkBitmapDevice.h b/src/core/SkBitmapDevice.h
index 2f961ab..e0a777f 100644
--- a/src/core/SkBitmapDevice.h
+++ b/src/core/SkBitmapDevice.h
@@ -111,11 +111,9 @@
SkImage*, const SkMatrix&) override;
sk_sp<SkSpecialImage> makeSpecial(const SkBitmap&) override;
sk_sp<SkSpecialImage> makeSpecial(const SkImage*) override;
- sk_sp<SkSpecialImage> snapSpecial() override;
+ sk_sp<SkSpecialImage> snapSpecial(const SkIRect&, bool = false) override;
void setImmutable() override { fBitmap.setImmutable(); }
- sk_sp<SkSpecialImage> snapBackImage(const SkIRect&) override;
-
///////////////////////////////////////////////////////////////////////////
bool onReadPixels(const SkPixmap&, int x, int y) override;
diff --git a/src/core/SkCanvas.cpp b/src/core/SkCanvas.cpp
index ff9970e..7c05e11 100644
--- a/src/core/SkCanvas.cpp
+++ b/src/core/SkCanvas.cpp
@@ -920,7 +920,7 @@
p.setImageFilter(modifiedFilter->makeWithLocalMatrix(localCTM));
}
- auto special = src->snapBackImage(snapBounds);
+ auto special = src->snapSpecial(snapBounds);
if (special) {
dst->drawSpecial(special.get(), x, y, p, nullptr, SkMatrix::I());
}
@@ -1111,7 +1111,11 @@
// need the bounds relative to the device itself
devBounds.offset(-device->fOrigin.fX, -device->fOrigin.fY);
- auto backImage = device->snapBackImage(devBounds);
+ // This is getting the special image from the current device, which is then drawn into (both by
+ // a client, and the drawClippedToSaveBehind below). Since this is not saving a layer, with its
+ // own device, we need to explicitly copy the back image contents so that its original content
+ // is available when we splat it back later during restore.
+ auto backImage = device->snapSpecial(devBounds, /* copy */ true);
if (!backImage) {
return;
}
diff --git a/src/core/SkDevice.cpp b/src/core/SkDevice.cpp
index da29c70..094f273 100644
--- a/src/core/SkDevice.cpp
+++ b/src/core/SkDevice.cpp
@@ -320,7 +320,10 @@
SkImage*, const SkMatrix&) {}
sk_sp<SkSpecialImage> SkBaseDevice::makeSpecial(const SkBitmap&) { return nullptr; }
sk_sp<SkSpecialImage> SkBaseDevice::makeSpecial(const SkImage*) { return nullptr; }
-sk_sp<SkSpecialImage> SkBaseDevice::snapSpecial() { return nullptr; }
+sk_sp<SkSpecialImage> SkBaseDevice::snapSpecial(const SkIRect&, bool) { return nullptr; }
+sk_sp<SkSpecialImage> SkBaseDevice::snapSpecial() {
+ return this->snapSpecial(SkIRect::MakeWH(this->width(), this->height()));
+}
///////////////////////////////////////////////////////////////////////////////////////////////////
@@ -415,10 +418,6 @@
return nullptr;
}
-sk_sp<SkSpecialImage> SkBaseDevice::snapBackImage(const SkIRect&) {
- return nullptr;
-}
-
//////////////////////////////////////////////////////////////////////////////////////////
void SkBaseDevice::LogDrawScaleFactor(const SkMatrix& view, const SkMatrix& srcToDst,
diff --git a/src/core/SkDevice.h b/src/core/SkDevice.h
index c19ac0b..2e49fa6 100644
--- a/src/core/SkDevice.h
+++ b/src/core/SkDevice.h
@@ -249,13 +249,18 @@
SkImage* clipImage, const SkMatrix& clipMatrix);
virtual sk_sp<SkSpecialImage> makeSpecial(const SkBitmap&);
virtual sk_sp<SkSpecialImage> makeSpecial(const SkImage*);
- virtual sk_sp<SkSpecialImage> snapSpecial();
+ // Get a view of the entire device's current contents as an image.
+ sk_sp<SkSpecialImage> snapSpecial();
+ // Snap the 'subset' contents from this device, possibly as a read-only view. If 'forceCopy'
+ // is true then the returned image's pixels must not be affected by subsequent draws into the
+ // device. When 'forceCopy' is false, the image can be a view into the device's pixels
+ // (avoiding a copy for performance, at the expense of safety). Default returns null.
+ virtual sk_sp<SkSpecialImage> snapSpecial(const SkIRect& subset, bool forceCopy = false);
+
virtual void setImmutable() {}
bool readPixels(const SkPixmap&, int x, int y);
- virtual sk_sp<SkSpecialImage> snapBackImage(const SkIRect&); // default returns null
-
///////////////////////////////////////////////////////////////////////////
virtual GrContext* context() const { return nullptr; }
diff --git a/src/gpu/SkGpuDevice.cpp b/src/gpu/SkGpuDevice.cpp
index 50cfce3..1919248 100644
--- a/src/gpu/SkGpuDevice.cpp
+++ b/src/gpu/SkGpuDevice.cpp
@@ -1225,42 +1225,7 @@
}
}
-sk_sp<SkSpecialImage> SkGpuDevice::snapSpecial() {
- // If we are wrapping a vulkan secondary command buffer, then we can't snap off a special image
- // since it would require us to make a copy of the underlying VkImage which we don't have access
- // to. Additionaly we can't stop and start the render pass that is used with the secondary
- // command buffer.
- if (this->accessRenderTargetContext()->wrapsVkSecondaryCB()) {
- return nullptr;
- }
-
- sk_sp<GrTextureProxy> proxy(this->accessRenderTargetContext()->asTextureProxyRef());
- if (!proxy) {
- // When the device doesn't have a texture, we create a temporary texture.
- // TODO: we should actually only copy the portion of the source needed to apply the image
- // filter
- proxy = GrSurfaceProxy::Copy(fContext.get(),
- this->accessRenderTargetContext()->asSurfaceProxy(),
- GrMipMapped::kNo,
- SkBackingFit::kApprox,
- SkBudgeted::kYes);
- if (!proxy) {
- return nullptr;
- }
- }
-
- const SkImageInfo ii = this->imageInfo();
- const SkIRect srcRect = SkIRect::MakeWH(ii.width(), ii.height());
-
- return SkSpecialImage::MakeDeferredFromGpu(fContext.get(),
- srcRect,
- kNeedNewImageUniqueID_SpecialImage,
- std::move(proxy),
- ii.refColorSpace(),
- &this->surfaceProps());
-}
-
-sk_sp<SkSpecialImage> SkGpuDevice::snapBackImage(const SkIRect& subset) {
+sk_sp<SkSpecialImage> SkGpuDevice::snapSpecial(const SkIRect& subset, bool forceCopy) {
GrRenderTargetContext* rtc = this->accessRenderTargetContext();
// If we are wrapping a vulkan secondary command buffer, then we can't snap off a special image
@@ -1271,22 +1236,32 @@
return nullptr;
}
-
- GrContext* ctx = this->context();
SkASSERT(rtc->asSurfaceProxy());
- auto srcProxy =
- GrSurfaceProxy::Copy(ctx, rtc->asSurfaceProxy(), rtc->mipMapped(), subset,
- SkBackingFit::kApprox, rtc->asSurfaceProxy()->isBudgeted());
- if (!srcProxy) {
- return nullptr;
+ SkIRect finalSubset = subset;
+ sk_sp<GrTextureProxy> proxy(rtc->asTextureProxyRef());
+ if (forceCopy || !proxy) {
+ // When the device doesn't have a texture, or a copy is requested, we create a temporary
+ // texture that matches the device contents
+ proxy = GrSurfaceProxy::Copy(fContext.get(),
+ rtc->asSurfaceProxy(),
+ GrMipMapped::kNo, // Don't auto generate mips
+ subset,
+ SkBackingFit::kApprox,
+ SkBudgeted::kYes); // Always budgeted
+ if (!proxy) {
+ return nullptr;
+ }
+
+ // Since this copied only the requested subset, the special image wrapping the proxy no
+ // longer needs the original subset.
+ finalSubset = SkIRect::MakeSize(proxy->isize());
}
- // Note, can't move srcProxy since we also refer to this in the 2nd parameter
return SkSpecialImage::MakeDeferredFromGpu(fContext.get(),
- SkIRect::MakeSize(srcProxy->isize()),
+ finalSubset,
kNeedNewImageUniqueID_SpecialImage,
- srcProxy,
+ std::move(proxy),
this->imageInfo().refColorSpace(),
&this->surfaceProps());
}
@@ -1301,7 +1276,7 @@
// drawDevice is defined to be in device coords.
SkGpuDevice* dev = static_cast<SkGpuDevice*>(device);
- sk_sp<SkSpecialImage> srcImg(dev->snapSpecial());
+ sk_sp<SkSpecialImage> srcImg(dev->snapSpecial(SkIRect::MakeWH(dev->width(), dev->height())));
if (!srcImg) {
return;
}
diff --git a/src/gpu/SkGpuDevice.h b/src/gpu/SkGpuDevice.h
index 81a19ef..a37d752 100644
--- a/src/gpu/SkGpuDevice.h
+++ b/src/gpu/SkGpuDevice.h
@@ -118,8 +118,7 @@
sk_sp<SkSpecialImage> makeSpecial(const SkBitmap&) override;
sk_sp<SkSpecialImage> makeSpecial(const SkImage*) override;
- sk_sp<SkSpecialImage> snapSpecial() override;
- sk_sp<SkSpecialImage> snapBackImage(const SkIRect&) override;
+ sk_sp<SkSpecialImage> snapSpecial(const SkIRect&, bool = false) override;
void flush() override;
GrSemaphoresSubmitted flush(SkSurface::BackendSurfaceAccess access, const GrFlushInfo&);
diff --git a/src/pdf/SkPDFDevice.cpp b/src/pdf/SkPDFDevice.cpp
index 5c032c0..3ed6f797 100644
--- a/src/pdf/SkPDFDevice.cpp
+++ b/src/pdf/SkPDFDevice.cpp
@@ -1755,10 +1755,6 @@
return SkSpecialImage::MakeFromImage(nullptr, image->bounds(), image->makeNonTextureImage());
}
-sk_sp<SkSpecialImage> SkPDFDevice::snapSpecial() {
- return nullptr;
-}
-
SkImageFilterCache* SkPDFDevice::getImageFilterCache() {
// We always return a transient cache, so it is freed after each
// filter traversal.
diff --git a/src/pdf/SkPDFDevice.h b/src/pdf/SkPDFDevice.h
index 5984c1c..ad8c12a 100644
--- a/src/pdf/SkPDFDevice.h
+++ b/src/pdf/SkPDFDevice.h
@@ -118,7 +118,6 @@
SkImage*, const SkMatrix&) override;
sk_sp<SkSpecialImage> makeSpecial(const SkBitmap&) override;
sk_sp<SkSpecialImage> makeSpecial(const SkImage*) override;
- sk_sp<SkSpecialImage> snapSpecial() override;
SkImageFilterCache* getImageFilterCache() override;
private: