From 50c044b9addac613189d6378641c65721e16b2ed Mon Sep 17 00:00:00 2001 From: senorblanco Date: Sat, 5 Dec 2015 05:59:41 -0800 Subject: [PATCH] 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 --- gm/matrixconvolution.cpp | 21 +++++++++---------- include/core/SkImageFilter.h | 17 +++++++++++++-- include/effects/SkColorFilterImageFilter.h | 2 +- include/effects/SkLightingImageFilter.h | 2 +- .../effects/SkMatrixConvolutionImageFilter.h | 2 +- include/effects/SkRectShaderImageFilter.h | 2 +- src/core/SkImageFilter.cpp | 15 +++++++------ src/effects/SkColorFilterImageFilter.cpp | 7 ++----- .../SkMatrixConvolutionImageFilter.cpp | 6 ------ src/effects/SkRectShaderImageFilter.cpp | 7 ++----- 10 files changed, 40 insertions(+), 41 deletions(-) diff --git a/gm/matrixconvolution.cpp b/gm/matrixconvolution.cpp index 3b159f455b..6d16f8dfa7 100644 --- a/gm/matrixconvolution.cpp +++ b/gm/matrixconvolution.cpp @@ -86,22 +86,21 @@ protected: 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 909a2f8284..d90df29c8b 100644 --- a/include/core/SkImageFilter.h +++ b/include/core/SkImageFilter.h @@ -199,7 +199,12 @@ public: * 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 @@ public: 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 @@ protected: 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 db5d842c2f..200ec86500 100644 --- a/include/effects/SkColorFilterImageFilter.h +++ b/include/effects/SkColorFilterImageFilter.h @@ -25,7 +25,7 @@ protected: 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 33cfceccb2..fb356c52e4 100644 --- a/include/effects/SkLightingImageFilter.h +++ b/include/effects/SkLightingImageFilter.h @@ -49,7 +49,7 @@ protected: 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 ef5ff72494..76349f430c 100644 --- a/include/effects/SkMatrixConvolutionImageFilter.h +++ b/include/effects/SkMatrixConvolutionImageFilter.h @@ -80,7 +80,7 @@ protected: 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 9ac116ae5f..9798af2d64 100644 --- a/include/effects/SkRectShaderImageFilter.h +++ b/include/effects/SkRectShaderImageFilter.h @@ -29,7 +29,7 @@ public: 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 3f33bc3876..6a3286ed67 100644 --- a/src/core/SkImageFilter.cpp +++ b/src/core/SkImageFilter.cpp @@ -309,6 +309,9 @@ void SkImageFilter::computeFastBounds(const SkRect& src, SkRect* dst) const { } 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 @@ bool SkImageFilter::canComputeFastBounds() const { 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 @@ bool SkImageFilter::filterImageGPU(Proxy* proxy, const SkBitmap& src, const Cont 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 b0e47505a7..8d394aa17b 100644 --- a/src/effects/SkColorFilterImageFilter.cpp +++ b/src/effects/SkColorFilterImageFilter.cpp @@ -99,11 +99,8 @@ bool SkColorFilterImageFilter::onIsColorFilterNode(SkColorFilter** filter) const 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 7c5dd8368f..e58eec10f4 100644 --- a/src/effects/SkMatrixConvolutionImageFilter.cpp +++ b/src/effects/SkMatrixConvolutionImageFilter.cpp @@ -335,12 +335,6 @@ bool SkMatrixConvolutionImageFilter::onFilterBounds(const SkIRect& src, const Sk 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 00964b5ffb..14837d02b1 100644 --- a/src/effects/SkRectShaderImageFilter.cpp +++ b/src/effects/SkRectShaderImageFilter.cpp @@ -79,11 +79,8 @@ bool SkRectShaderImageFilter::onFilterImage(Proxy* proxy, 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