Revert of Matrix convolution bounds fix; affectsTransparentBlack fixes. (patchset #4 id:60001 of https://codereview.chromium.org/1500923004/ )
Reason for revert:
Introduced memory leak; pixel changes in Chrome.
Original issue's description:
> Matrix convolution bounds fix; affectsTransparentBlack fixes.
>
> Because the convolution kernel is (currently) applied in device space,
> there's no way to know which object-space pixels will be touched. So
> return false from canComputeFastBounds().
>
> The results from the matrixconvolution GM were actually wrong, since
> they were showing edge differences on the clip boundaries, where they
> should really only show on crop boundaries. I added a crop to the GM
> to keep the results the same (which are useful to test the different
> convolution tile modes).
>
> While I was at it, SkImageFilter::affectsTransparentBlack() was
> inapplicable on most things except color filters, and its use on
> leaf nodes was confusing. So I removed it, and made
> SkImageFilter::canComputeFastBounds() virtual instead.
>
> BUG=skia:
>
> Committed: https://skia.googlesource.com/skia/+/8705ec80518ef551994b82ca5ccaeb0241d6adec
TBR=reed@google.com,reed@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=skia:
Review URL: https://codereview.chromium.org/1497083005
diff --git a/gm/matrixconvolution.cpp b/gm/matrixconvolution.cpp
index 3b159f4..6d16f8d 100644
--- a/gm/matrixconvolution.cpp
+++ b/gm/matrixconvolution.cpp
@@ -86,22 +86,21 @@
void onDraw(SkCanvas* canvas) override {
canvas->clear(SK_ColorBLACK);
SkIPoint kernelOffset = SkIPoint::Make(1, 0);
- SkImageFilter::CropRect rect(SkRect::Make(fBitmap.bounds()));
for (int x = 10; x < 310; x += 100) {
- this->draw(canvas, x, 10, kernelOffset, MCIF::kClamp_TileMode, true, &rect);
- this->draw(canvas, x, 110, kernelOffset, MCIF::kClampToBlack_TileMode, true, &rect);
- this->draw(canvas, x, 210, kernelOffset, MCIF::kRepeat_TileMode, true, &rect);
+ this->draw(canvas, x, 10, kernelOffset, MCIF::kClamp_TileMode, true);
+ this->draw(canvas, x, 110, kernelOffset, MCIF::kClampToBlack_TileMode, true);
+ this->draw(canvas, x, 210, kernelOffset, MCIF::kRepeat_TileMode, true);
kernelOffset.fY++;
}
kernelOffset.fY = 1;
- SkImageFilter::CropRect smallRect(SkRect::MakeXYWH(10, 5, 60, 60));
- this->draw(canvas, 310, 10, kernelOffset, MCIF::kClamp_TileMode, true, &smallRect);
- this->draw(canvas, 310, 110, kernelOffset, MCIF::kClampToBlack_TileMode, true, &smallRect);
- this->draw(canvas, 310, 210, kernelOffset, MCIF::kRepeat_TileMode, true, &smallRect);
+ SkImageFilter::CropRect rect(SkRect::MakeXYWH(10, 5, 60, 60));
+ this->draw(canvas, 310, 10, kernelOffset, MCIF::kClamp_TileMode, true, &rect);
+ this->draw(canvas, 310, 110, kernelOffset, MCIF::kClampToBlack_TileMode, true, &rect);
+ this->draw(canvas, 310, 210, kernelOffset, MCIF::kRepeat_TileMode, true, &rect);
- this->draw(canvas, 410, 10, kernelOffset, MCIF::kClamp_TileMode, false, &rect);
- this->draw(canvas, 410, 110, kernelOffset, MCIF::kClampToBlack_TileMode, false, &rect);
- this->draw(canvas, 410, 210, kernelOffset, MCIF::kRepeat_TileMode, false, &rect);
+ this->draw(canvas, 410, 10, kernelOffset, MCIF::kClamp_TileMode, false);
+ this->draw(canvas, 410, 110, kernelOffset, MCIF::kClampToBlack_TileMode, false);
+ this->draw(canvas, 410, 210, kernelOffset, MCIF::kRepeat_TileMode, false);
}
private:
diff --git a/include/core/SkImageFilter.h b/include/core/SkImageFilter.h
index 909a2f8..d90df29 100644
--- a/include/core/SkImageFilter.h
+++ b/include/core/SkImageFilter.h
@@ -199,7 +199,12 @@
* replaced by the returned colorfilter. i.e. the two effects will affect drawing in the
* same way.
*/
- bool asAColorFilter(SkColorFilter** filterPtr) const;
+ bool asAColorFilter(SkColorFilter** filterPtr) const {
+ return this->countInputs() > 0 &&
+ NULL == this->getInput(0) &&
+ !this->affectsTransparentBlack() &&
+ this->isColorFilterNode(filterPtr);
+ }
/**
* Returns the number of inputs this filter will accept (some inputs can
@@ -235,7 +240,7 @@
virtual void computeFastBounds(const SkRect&, SkRect*) const;
// Can this filter DAG compute the resulting bounds of an object-space rectangle?
- virtual bool canComputeFastBounds() const;
+ bool canComputeFastBounds() const;
/**
* If this filter can be represented by another filter + a localMatrix, return that filter,
@@ -405,6 +410,14 @@
virtual bool asFragmentProcessor(GrFragmentProcessor**, GrTexture*, const SkMatrix&,
const SkIRect& bounds) const;
+ /**
+ * Returns true if this filter can cause transparent black pixels to become
+ * visible (ie., alpha > 0). The default implementation returns false. This
+ * function is non-recursive, i.e., only queries this filter and not its
+ * inputs.
+ */
+ virtual bool affectsTransparentBlack() const;
+
private:
friend class SkGraphics;
static void PurgeCache();
diff --git a/include/effects/SkColorFilterImageFilter.h b/include/effects/SkColorFilterImageFilter.h
index db5d842..200ec86 100644
--- a/include/effects/SkColorFilterImageFilter.h
+++ b/include/effects/SkColorFilterImageFilter.h
@@ -25,7 +25,7 @@
bool onFilterImage(Proxy*, const SkBitmap& src, const Context&, SkBitmap* result,
SkIPoint* loc) const override;
bool onIsColorFilterNode(SkColorFilter**) const override;
- bool canComputeFastBounds() const override;
+ bool affectsTransparentBlack() const override;
private:
SkColorFilterImageFilter(SkColorFilter* cf,
diff --git a/include/effects/SkLightingImageFilter.h b/include/effects/SkLightingImageFilter.h
index 33cfcec..fb356c5 100644
--- a/include/effects/SkLightingImageFilter.h
+++ b/include/effects/SkLightingImageFilter.h
@@ -49,7 +49,7 @@
void flatten(SkWriteBuffer&) const override;
const SkImageFilterLight* light() const { return fLight.get(); }
SkScalar surfaceScale() const { return fSurfaceScale; }
- bool canComputeFastBounds() const override { return false; }
+ bool affectsTransparentBlack() const override { return true; }
private:
typedef SkImageFilter INHERITED;
diff --git a/include/effects/SkMatrixConvolutionImageFilter.h b/include/effects/SkMatrixConvolutionImageFilter.h
index ef5ff72..76349f4 100644
--- a/include/effects/SkMatrixConvolutionImageFilter.h
+++ b/include/effects/SkMatrixConvolutionImageFilter.h
@@ -80,7 +80,7 @@
bool onFilterImage(Proxy*, const SkBitmap& src, const Context&,
SkBitmap* result, SkIPoint* loc) const override;
bool onFilterBounds(const SkIRect&, const SkMatrix&, SkIRect*) const override;
- bool canComputeFastBounds() const override;
+
#if SK_SUPPORT_GPU
bool asFragmentProcessor(GrFragmentProcessor**, GrTexture*, const SkMatrix&,
diff --git a/include/effects/SkRectShaderImageFilter.h b/include/effects/SkRectShaderImageFilter.h
index 9ac116a..9798af2 100644
--- a/include/effects/SkRectShaderImageFilter.h
+++ b/include/effects/SkRectShaderImageFilter.h
@@ -29,7 +29,7 @@
static SkImageFilter* Create(SkShader* s, const SkRect& rect);
static SkImageFilter* Create(SkShader* s, const CropRect* rect = NULL);
- bool canComputeFastBounds() const override;
+ bool affectsTransparentBlack() const override;
SK_TO_STRING_OVERRIDE()
SK_DECLARE_PUBLIC_FLATTENABLE_DESERIALIZATION_PROCS(SkRectShaderImageFilter)
diff --git a/src/core/SkImageFilter.cpp b/src/core/SkImageFilter.cpp
index 3f33bc3..6a3286e 100644
--- a/src/core/SkImageFilter.cpp
+++ b/src/core/SkImageFilter.cpp
@@ -309,6 +309,9 @@
}
bool SkImageFilter::canComputeFastBounds() const {
+ if (this->affectsTransparentBlack()) {
+ return false;
+ }
for (int i = 0; i < fInputCount; i++) {
SkImageFilter* input = this->getInput(i);
if (input && !input->canComputeFastBounds()) {
@@ -318,6 +321,10 @@
return true;
}
+bool SkImageFilter::affectsTransparentBlack() const {
+ return false;
+}
+
bool SkImageFilter::onFilterImage(Proxy*, const SkBitmap&, const Context&,
SkBitmap*, SkIPoint*) const {
return false;
@@ -384,14 +391,6 @@
return false;
}
-bool SkImageFilter::asAColorFilter(SkColorFilter** filterPtr) const {
- SkASSERT(nullptr != filterPtr);
- return this->countInputs() > 0 &&
- NULL == this->getInput(0) &&
- this->isColorFilterNode(filterPtr) &&
- !(*filterPtr)->affectsTransparentBlack();
-}
-
bool SkImageFilter::applyCropRect(const Context& ctx, const SkBitmap& src,
const SkIPoint& srcOffset, SkIRect* dstBounds,
SkIRect* srcBounds) const {
diff --git a/src/effects/SkColorFilterImageFilter.cpp b/src/effects/SkColorFilterImageFilter.cpp
index b0e4750..8d394aa 100644
--- a/src/effects/SkColorFilterImageFilter.cpp
+++ b/src/effects/SkColorFilterImageFilter.cpp
@@ -99,11 +99,8 @@
return false;
}
-bool SkColorFilterImageFilter::canComputeFastBounds() const {
- if (fColorFilter->affectsTransparentBlack()) {
- return false;
- }
- return INHERITED::canComputeFastBounds();
+bool SkColorFilterImageFilter::affectsTransparentBlack() const {
+ return fColorFilter->affectsTransparentBlack();
}
#ifndef SK_IGNORE_TO_STRING
diff --git a/src/effects/SkMatrixConvolutionImageFilter.cpp b/src/effects/SkMatrixConvolutionImageFilter.cpp
index 7c5dd83..e58eec1 100644
--- a/src/effects/SkMatrixConvolutionImageFilter.cpp
+++ b/src/effects/SkMatrixConvolutionImageFilter.cpp
@@ -335,12 +335,6 @@
return true;
}
-bool SkMatrixConvolutionImageFilter::canComputeFastBounds() const {
- // Because the kernel is applied in device-space, we have no idea what
- // pixels it will affect in object-space.
- return false;
-}
-
#if SK_SUPPORT_GPU
static GrTextureDomain::Mode convert_tilemodes(
diff --git a/src/effects/SkRectShaderImageFilter.cpp b/src/effects/SkRectShaderImageFilter.cpp
index 00964b5..14837d0 100644
--- a/src/effects/SkRectShaderImageFilter.cpp
+++ b/src/effects/SkRectShaderImageFilter.cpp
@@ -79,11 +79,8 @@
return true;
}
-bool SkRectShaderImageFilter::canComputeFastBounds() const {
- // http:skbug.com/4627: "make computeFastBounds and onFilterBounds() CropRect-aware"
- // computeFastBounds() doesn't currently take the crop rect into account,
- // so we can't compute it. If a full crop rect is set, we should return true here.
- return false;
+bool SkRectShaderImageFilter::affectsTransparentBlack() const {
+ return true;
}
#ifndef SK_IGNORE_TO_STRING