Enforce saveLayer ignoring restore paint's mask filter
Documentation specifies that only the alpha, color filter, image filter
and blend mode are used when restoring the saved layer. The coverage that
would be sent to the mask filter is ill-defined, whereas the explicit
boundary of the separate clip image gets around this.
This also removes the issue of the order in which the mask filter and
image filter would be applied.
Some GMs exercised the save layer with mask filter, but those have
been updated to either expect the MF to be ignored, or modified to
no longer use saveLayer+MF. While Android and Flutter expose a saveLayer
with general Paint API, their documentation matches SkCanvas so hopefully
no one is actually relying on the mask filter in those cases. It does
not appear as if Chrome ever tries to use a saveLayer+MF.
Bug: skia:9561
Change-Id: I67a350fbf565683ea60ea88f83a91270e7dcd187
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/274508
Reviewed-by: Mike Reed <reed@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
diff --git a/src/core/SkBitmapDevice.cpp b/src/core/SkBitmapDevice.cpp
index 587a6db..04677bc 100644
--- a/src/core/SkBitmapDevice.cpp
+++ b/src/core/SkBitmapDevice.cpp
@@ -559,13 +559,10 @@
void SkBitmapDevice::drawDevice(SkBaseDevice* device, int x, int y, const SkPaint& origPaint) {
SkASSERT(!origPaint.getImageFilter());
+ SkASSERT(!origPaint.getMaskFilter());
// todo: can we unify with similar adjustment in SkGpuDevice?
SkTCopyOnFirstWrite<SkPaint> paint(origPaint);
- if (paint->getMaskFilter()) {
- paint.writable()->setMaskFilter(
- paint->getMaskFilter()->makeWithMatrix(this->localToDevice()));
- }
// hack to test coverage
SkBitmapDevice* src = static_cast<SkBitmapDevice*>(device);
@@ -574,10 +571,9 @@
draw.fDst = fBitmap.pixmap();
draw.fMatrix = &SkMatrix::I();
draw.fRC = &fRCStack.rc();
- SkPaint paint(origPaint);
- paint.setShader(src->fBitmap.makeShader());
+ paint.writable()->setShader(src->fBitmap.makeShader());
draw.drawBitmap(*src->fCoverage.get(),
- SkMatrix::MakeTrans(SkIntToScalar(x),SkIntToScalar(y)), nullptr, paint);
+ SkMatrix::MakeTrans(SkIntToScalar(x),SkIntToScalar(y)), nullptr, *paint);
} else {
this->drawSprite(src->fBitmap, x, y, *paint);
}
@@ -623,6 +619,7 @@
void SkBitmapDevice::drawSpecial(SkSpecialImage* src, int x, int y, const SkPaint& origPaint,
SkImage* clipImage, const SkMatrix& clipMatrix) {
SkASSERT(!src->isTextureBacked());
+ SkASSERT(!origPaint.getMaskFilter());
sk_sp<SkSpecialImage> filteredImage;
SkTCopyOnFirstWrite<SkPaint> paint(origPaint);
@@ -647,11 +644,6 @@
y += offset.y();
}
- if (paint->getMaskFilter()) {
- paint.writable()->setMaskFilter(
- paint->getMaskFilter()->makeWithMatrix(this->localToDevice()));
- }
-
if (!clipImage) {
SkBitmap resultBM;
if (src->getROPixels(&resultBM)) {
diff --git a/src/core/SkCanvas.cpp b/src/core/SkCanvas.cpp
index 7d5a475..e9cf7e3 100644
--- a/src/core/SkCanvas.cpp
+++ b/src/core/SkCanvas.cpp
@@ -1083,17 +1083,21 @@
void SkCanvas::internalSaveLayer(const SaveLayerRec& rec, SaveLayerStrategy strategy) {
TRACE_EVENT0("skia", TRACE_FUNC);
const SkRect* bounds = rec.fBounds;
- const SkPaint* paint = rec.fPaint;
SaveLayerFlags saveLayerFlags = rec.fSaveLayerFlags;
+ SkTCopyOnFirstWrite<SkPaint> paint(rec.fPaint);
+ // saveLayer ignores mask filters, so force it to null
+ if (paint.get() && paint->getMaskFilter()) {
+ paint.writable()->setMaskFilter(nullptr);
+ }
+
// If we have a backdrop filter, then we must apply it to the entire layer (clip-bounds)
// regardless of any hint-rect from the caller. skbug.com/8783
if (rec.fBackdrop) {
bounds = nullptr;
}
- SkLazyPaint lazyP;
- SkImageFilter* imageFilter = paint ? paint->getImageFilter() : nullptr;
+ SkImageFilter* imageFilter = paint.get() ? paint->getImageFilter() : nullptr;
SkMatrix stashedMatrix = fMCRec->fMatrix;
MCRec* modifiedRec = nullptr;
@@ -1124,10 +1128,8 @@
SkASSERT(modifiedCTM.isScaleTranslate() || as_IFB(imageFilter)->canHandleComplexCTM());
modifiedRec = fMCRec;
this->internalSetMatrix(modifiedCTM);
- SkPaint* p = lazyP.set(*paint);
- p->setImageFilter(std::move(modifiedFilter));
- imageFilter = p->getImageFilter();
- paint = p;
+ imageFilter = modifiedFilter.get();
+ paint.writable()->setImageFilter(std::move(modifiedFilter));
}
// Else the filter didn't change, so modifiedCTM == stashedMatrix and there's nothing
// left to do since the stack already has that as the CTM.
diff --git a/src/gpu/SkGpuDevice.cpp b/src/gpu/SkGpuDevice.cpp
index f051151..4c413c3 100644
--- a/src/gpu/SkGpuDevice.cpp
+++ b/src/gpu/SkGpuDevice.cpp
@@ -982,6 +982,8 @@
void SkGpuDevice::drawSpecial(SkSpecialImage* special, int left, int top, const SkPaint& paint,
SkImage* clipImage, const SkMatrix& clipMatrix) {
+ SkASSERT(!paint.getMaskFilter());
+
ASSERT_SINGLE_OWNER
GR_CREATE_TRACE_MARKER_CONTEXT("SkGpuDevice", "drawSpecial", fContext.get());
@@ -1010,10 +1012,6 @@
ctm.postTranslate(-SkIntToScalar(left), -SkIntToScalar(top));
SkPaint tmpUnfiltered(paint);
- if (tmpUnfiltered.getMaskFilter()) {
- tmpUnfiltered.setMaskFilter(tmpUnfiltered.getMaskFilter()->makeWithMatrix(ctm));
- }
-
tmpUnfiltered.setImageFilter(nullptr);
auto fp = GrTextureEffect::Make(std::move(view), special->alphaType());
@@ -1260,6 +1258,7 @@
void SkGpuDevice::drawDevice(SkBaseDevice* device,
int left, int top, const SkPaint& paint) {
SkASSERT(!paint.getImageFilter());
+ SkASSERT(!paint.getMaskFilter());
ASSERT_SINGLE_OWNER
// clear of the source device must occur before CHECK_SHOULD_DRAW
diff --git a/src/pdf/SkPDFDevice.cpp b/src/pdf/SkPDFDevice.cpp
index f36ee0a..4704720 100644
--- a/src/pdf/SkPDFDevice.cpp
+++ b/src/pdf/SkPDFDevice.cpp
@@ -961,6 +961,7 @@
void SkPDFDevice::drawDevice(SkBaseDevice* device, int x, int y, const SkPaint& paint) {
SkASSERT(!paint.getImageFilter());
+ SkASSERT(!paint.getMaskFilter());
// Check if the source device is really a bitmapdevice (because that's what we returned
// from createDevice (likely due to an imagefilter)
@@ -1707,6 +1708,7 @@
return;
}
SkASSERT(!srcImg->isTextureBacked());
+ SkASSERT(!paint.getMaskFilter());
//TODO: clipImage support