From ebaf6a69bf604c85185e23aca3fb93308e747ff5 Mon Sep 17 00:00:00 2001 From: bsalomon Date: Wed, 1 Jul 2015 11:02:50 -0700 Subject: [PATCH] Revert of Fix SkTileImageFilter clipping/cropRect interaction issue (patchset #2 id:30001 of https://codereview.chromium.org/1210053003/) Reason for revert: Perf regression: https://code.google.com/p/chromium/issues/detail?id=505564 Original issue's description: > Fix SkTileImageFilter clipping/cropRect interaction issue > > BUG=499499 > > Committed: https://skia.googlesource.com/skia/+/157bcd0840b578060dbc3365daafffc6837da391 TBR=reed@google.com,senorblanco@google.com,senorblanco@chromium.org,robertphillips@google.com NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=499499 Review URL: https://codereview.chromium.org/1219193002 --- gm/cropdisp.cpp | 105 ----------------------- include/core/SkImageFilter.h | 3 - src/core/SkImageFilter.cpp | 31 ------- src/effects/SkColorFilterImageFilter.cpp | 1 - src/effects/SkDisplacementMapEffect.cpp | 3 +- src/effects/SkTileImageFilter.cpp | 31 +++---- 6 files changed, 12 insertions(+), 162 deletions(-) delete mode 100644 gm/cropdisp.cpp diff --git a/gm/cropdisp.cpp b/gm/cropdisp.cpp deleted file mode 100644 index 6cc0d5fe96..0000000000 --- a/gm/cropdisp.cpp +++ /dev/null @@ -1,105 +0,0 @@ -/* - * Copyright 2015 Google Inc. - * - * Use of this source code is governed by a BSD-style license that can be - * found in the LICENSE file. - */ - - -#include "SkBitmapSource.h" -#include "SkColorFilter.h" -#include "SkColorFilterImageFilter.h" -#include "SkDisplacementMapEffect.h" -#include "SkTileImageFilter.h" -#include "SkXfermode.h" -#include "gm.h" - -namespace skiagm { - -// This tests the image filter graph: -// -// BitmapSource1 -- all red 512x512 -// | -// ColorFilterImageFilter -- with a 64x64 crop rect - makes the pixels green -// | -// TileImageFilter -- which tiles the 64x64 green pixels across 512x512 -// | -// | BitmapSource1 -- all red 512x512 -// | displacement | color -// | | -// DisplacementMapEffect -- this is only necessary to preserve the clip in the computed bounds -// TileImageFilter by itself bloats the bounds to include the src -// It has the TileImageFilter as the offset input. -// -// What was going on was that the clipRect being applied to the draw (64, 64, 512, 512) -// was eliminating the "displacement" chain due to the crop rect. -// This reproduces crbug/499499 -class CroppedDisplacementGM : public GM { -public: - CroppedDisplacementGM() { } - -protected: - - SkString onShortName() override { - return SkString("cropped-displacement"); - } - - SkISize onISize() override { - return SkISize::Make(kWidth, kHeight); - } - - void onOnceBeforeDraw() override { - fRedBitmap.allocN32Pixels(kWidth, kHeight); - SkCanvas canvas(fRedBitmap); - canvas.clear(SK_ColorRED); - } - - void onDraw(SkCanvas* canvas) override { - - SkPaint p; - - const SkRect smRect = SkRect::MakeWH(SkIntToScalar(kSmallSize), SkIntToScalar(kSmallSize)); - SkImageFilter::CropRect cr(smRect); - - SkAutoTUnref bms(SkBitmapSource::Create(fRedBitmap)); - SkAutoTUnref cf(SkColorFilter::CreateModeFilter(SK_ColorGREEN, - SkXfermode::kSrc_Mode)); - SkAutoTUnref cfif(SkColorFilterImageFilter::Create(cf, bms, &cr)); - - SkAutoTUnref tif(SkTileImageFilter::Create( - SkRect::MakeWH(SkIntToScalar(kSmallSize), SkIntToScalar(kSmallSize)), - SkRect::MakeWH(SkIntToScalar(kWidth), SkIntToScalar(kHeight)), - cfif)); - - static const SkScalar kScale = 20.0f; - - SkAutoTUnref dif(SkDisplacementMapEffect::Create( - SkDisplacementMapEffect::kB_ChannelSelectorType, - SkDisplacementMapEffect::kB_ChannelSelectorType, - kScale, - tif, bms)); - - p.setImageFilter(dif); - - canvas->clipRect(SkRect::MakeLTRB(kSmallSize+kScale/2.0f, - kSmallSize+kScale/2.0f, - SkIntToScalar(kWidth), SkIntToScalar(kHeight))); - canvas->saveLayer(NULL, &p); - canvas->restore(); - } - -private: - static const int kWidth = 512; - static const int kHeight = 512; - static const int kSmallSize = 64; - - SkBitmap fRedBitmap; - - typedef GM INHERITED; -}; - -////////////////////////////////////////////////////////////////////////////// - -DEF_GM( return SkNEW(CroppedDisplacementGM); ) - -} diff --git a/include/core/SkImageFilter.h b/include/core/SkImageFilter.h index 188905e5d7..4125db3734 100644 --- a/include/core/SkImageFilter.h +++ b/include/core/SkImageFilter.h @@ -44,9 +44,6 @@ public: explicit CropRect(const SkRect& rect, uint32_t flags = kHasAll_CropEdge) : fRect(rect), fFlags(flags) {} uint32_t flags() const { return fFlags; } const SkRect& rect() const { return fRect; } -#ifndef SK_IGNORE_TO_STRING - void toString(SkString* str) const; -#endif private: SkRect fRect; uint32_t fFlags; diff --git a/src/core/SkImageFilter.cpp b/src/core/SkImageFilter.cpp index f63d1bf13b..7233ec6706 100644 --- a/src/core/SkImageFilter.cpp +++ b/src/core/SkImageFilter.cpp @@ -32,37 +32,6 @@ enum { kDefaultCacheSize = 128 * 1024 * 1024 }; #endif -#ifndef SK_IGNORE_TO_STRING -void SkImageFilter::CropRect::toString(SkString* str) const { - if (!fFlags) { - return; - } - - str->appendf("cropRect ("); - if (fFlags & CropRect::kHasLeft_CropEdge) { - str->appendf("%.2f, ", fRect.fLeft); - } else { - str->appendf("X, "); - } - if (fFlags & CropRect::kHasTop_CropEdge) { - str->appendf("%.2f, ", fRect.fTop); - } else { - str->appendf("X, "); - } - if (fFlags & CropRect::kHasRight_CropEdge) { - str->appendf("%.2f, ", fRect.fRight); - } else { - str->appendf("X, "); - } - if (fFlags & CropRect::kHasBottom_CropEdge) { - str->appendf("%.2f", fRect.fBottom); - } else { - str->appendf("X"); - } - str->appendf(") "); -} -#endif - static int32_t next_image_filter_unique_id() { static int32_t gImageFilterUniqueID; diff --git a/src/effects/SkColorFilterImageFilter.cpp b/src/effects/SkColorFilterImageFilter.cpp index d7a5ff848c..2eb720e5c4 100755 --- a/src/effects/SkColorFilterImageFilter.cpp +++ b/src/effects/SkColorFilterImageFilter.cpp @@ -101,7 +101,6 @@ bool SkColorFilterImageFilter::onIsColorFilterNode(SkColorFilter** filter) const #ifndef SK_IGNORE_TO_STRING void SkColorFilterImageFilter::toString(SkString* str) const { str->appendf("SkColorFilterImageFilter: ("); - this->getCropRect().toString(str); str->appendf("input: ("); diff --git a/src/effects/SkDisplacementMapEffect.cpp b/src/effects/SkDisplacementMapEffect.cpp index 0eac80ff8a..d7d92c82b9 100644 --- a/src/effects/SkDisplacementMapEffect.cpp +++ b/src/effects/SkDisplacementMapEffect.cpp @@ -269,7 +269,7 @@ void SkDisplacementMapEffect::computeFastBounds(const SkRect& src, SkRect* dst) } bool SkDisplacementMapEffect::onFilterBounds(const SkIRect& src, const SkMatrix& ctm, - SkIRect* dst) const { + SkIRect* dst) const { SkIRect bounds = src; SkVector scale = SkVector::Make(fScale, fScale); ctm.mapVectors(&scale, 1); @@ -285,7 +285,6 @@ bool SkDisplacementMapEffect::onFilterBounds(const SkIRect& src, const SkMatrix& #ifndef SK_IGNORE_TO_STRING void SkDisplacementMapEffect::toString(SkString* str) const { str->appendf("SkDisplacementMapEffect: ("); - this->getCropRect().toString(str); str->appendf("scale: %f ", fScale); str->appendf("displacement: ("); if (this->getDisplacementInput()) { diff --git a/src/effects/SkTileImageFilter.cpp b/src/effects/SkTileImageFilter.cpp index 64e9a43f5a..2b7ed940d2 100644 --- a/src/effects/SkTileImageFilter.cpp +++ b/src/effects/SkTileImageFilter.cpp @@ -27,27 +27,19 @@ SkTileImageFilter* SkTileImageFilter::Create(const SkRect& srcRect, const SkRect bool SkTileImageFilter::onFilterImage(Proxy* proxy, const SkBitmap& src, const Context& ctx, SkBitmap* dst, SkIPoint* offset) const { + SkBitmap source = src; + SkImageFilter* input = getInput(0); + SkIPoint srcOffset = SkIPoint::Make(0, 0); + if (input && !input->filterImage(proxy, src, ctx, &source, &srcOffset)) { + return false; + } SkRect dstRect; ctx.ctm().mapRect(&dstRect, fDstRect); const SkIRect dstIRect = dstRect.roundOut(); - if (fSrcRect.isEmpty() || dstIRect.isEmpty()) { - return false; - } - - // TODO: the actual clip that needs to be applied to the src should be (roughly) determined by: - // intersect ctx.clip and dstIRect - // determine if that rect lies wholly inside fSrcRect - // if so pass it on as the clip - // if not pass the entire fSrcRect as the clip - // For now don't apply any clip to the source (since it is usually very small and all of it - // will be required anyway). - Context srcCtx(ctx.ctm(), SkIRect::MakeLargest(), ctx.cache()); - - SkBitmap source = src; - SkImageFilter* input = this->getInput(0); - SkIPoint srcOffset = SkIPoint::Make(0, 0); - if (input && !input->filterImage(proxy, src, srcCtx, &source, &srcOffset)) { + int w = dstIRect.width(); + int h = dstIRect.height(); + if (!fSrcRect.width() || !fSrcRect.height() || !w || !h) { return false; } @@ -67,7 +59,7 @@ bool SkTileImageFilter::onFilterImage(Proxy* proxy, const SkBitmap& src, return false; } - SkAutoTUnref device(proxy->createDevice(dstIRect.width(), dstIRect.height())); + SkAutoTUnref device(proxy->createDevice(w, h)); if (NULL == device.get()) { return false; } @@ -124,13 +116,12 @@ void SkTileImageFilter::flatten(SkWriteBuffer& buffer) const { #ifndef SK_IGNORE_TO_STRING void SkTileImageFilter::toString(SkString* str) const { str->appendf("SkTileImageFilter: ("); - this->getCropRect().toString(str); str->appendf("src: %.2f %.2f %.2f %.2f", fSrcRect.fLeft, fSrcRect.fTop, fSrcRect.fRight, fSrcRect.fBottom); str->appendf(" dst: %.2f %.2f %.2f %.2f", fDstRect.fLeft, fDstRect.fTop, fDstRect.fRight, fDstRect.fBottom); if (this->getInput(0)) { - str->appendf(" input: ("); + str->appendf("input: ("); this->getInput(0)->toString(str); str->appendf(")"); }