Revert of Revert of Fix SkTileImageFilter clipping/cropRect interaction issue (patchset #1 id:1 of https://codereview.chromium.org/1219193002/)
Reason for revert:
Blocking the roll
Original issue's description:
> 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
>
> Committed: https://skia.googlesource.com/skia/+/ebaf6a69bf604c85185e23aca3fb93308e747ff5
TBR=reed@google.com,senorblanco@google.com,senorblanco@chromium.org,robertphillips@google.com,reed@chromium.org,bsalomon@google.com
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=499499
Review URL: https://codereview.chromium.org/1207353004
diff --git a/gm/cropdisp.cpp b/gm/cropdisp.cpp
new file mode 100644
index 0000000..6cc0d5f
--- /dev/null
+++ b/gm/cropdisp.cpp
@@ -0,0 +1,105 @@
+/*
+ * 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<SkBitmapSource> bms(SkBitmapSource::Create(fRedBitmap));
+ SkAutoTUnref<SkColorFilter> cf(SkColorFilter::CreateModeFilter(SK_ColorGREEN,
+ SkXfermode::kSrc_Mode));
+ SkAutoTUnref<SkColorFilterImageFilter> cfif(SkColorFilterImageFilter::Create(cf, bms, &cr));
+
+ SkAutoTUnref<SkTileImageFilter> tif(SkTileImageFilter::Create(
+ SkRect::MakeWH(SkIntToScalar(kSmallSize), SkIntToScalar(kSmallSize)),
+ SkRect::MakeWH(SkIntToScalar(kWidth), SkIntToScalar(kHeight)),
+ cfif));
+
+ static const SkScalar kScale = 20.0f;
+
+ SkAutoTUnref<SkDisplacementMapEffect> 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 4125db3..188905e 100644
--- a/include/core/SkImageFilter.h
+++ b/include/core/SkImageFilter.h
@@ -44,6 +44,9 @@
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 7233ec6..f63d1bf 100644
--- a/src/core/SkImageFilter.cpp
+++ b/src/core/SkImageFilter.cpp
@@ -32,6 +32,37 @@
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 2eb720e..d7a5ff8 100755
--- a/src/effects/SkColorFilterImageFilter.cpp
+++ b/src/effects/SkColorFilterImageFilter.cpp
@@ -101,6 +101,7 @@
#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 d7d92c8..0eac80f 100644
--- a/src/effects/SkDisplacementMapEffect.cpp
+++ b/src/effects/SkDisplacementMapEffect.cpp
@@ -269,7 +269,7 @@
}
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,6 +285,7 @@
#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 2b7ed94..64e9a43 100644
--- a/src/effects/SkTileImageFilter.cpp
+++ b/src/effects/SkTileImageFilter.cpp
@@ -27,19 +27,27 @@
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();
- int w = dstIRect.width();
- int h = dstIRect.height();
- if (!fSrcRect.width() || !fSrcRect.height() || !w || !h) {
+ 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)) {
return false;
}
@@ -59,7 +67,7 @@
return false;
}
- SkAutoTUnref<SkBaseDevice> device(proxy->createDevice(w, h));
+ SkAutoTUnref<SkBaseDevice> device(proxy->createDevice(dstIRect.width(), dstIRect.height()));
if (NULL == device.get()) {
return false;
}
@@ -116,12 +124,13 @@
#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(")");
}