From afc7cce5d68663934128d76963cd501f771d71de Mon Sep 17 00:00:00 2001 From: senorblanco Date: Tue, 2 Feb 2016 18:44:15 -0800 Subject: [PATCH] Fix for rounded-rect clips with filters. Don't use the base canvas size to limit raster of complex clips, since the top canvas size may actually be larger (e.g., a blur filter which expands the clip bounds to accommodate filter margins). Use the top canvas bounds instead. BUG=skia:4879,471212 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1657333002 Review URL: https://codereview.chromium.org/1657333002 --- gm/complexclip_blur_tiled.cpp | 76 +++++++++++++++++++++++++++++++++++ include/core/SkCanvas.h | 5 +-- src/core/SkCanvas.cpp | 31 ++++++-------- src/core/SkRasterClip.cpp | 16 ++++---- src/core/SkRasterClip.h | 6 +-- src/pdf/SkPDFDevice.cpp | 3 +- src/utils/SkLua.cpp | 5 +-- tests/AAClipTest.cpp | 10 ++--- 8 files changed, 109 insertions(+), 43 deletions(-) create mode 100644 gm/complexclip_blur_tiled.cpp diff --git a/gm/complexclip_blur_tiled.cpp b/gm/complexclip_blur_tiled.cpp new file mode 100644 index 0000000000..aac04167b7 --- /dev/null +++ b/gm/complexclip_blur_tiled.cpp @@ -0,0 +1,76 @@ +/* + * Copyright 2016 Google Inc. + * + * Use of this source code is governed by a BSD-style license that can be + * found in the LICENSE file. + */ + +#include "gm.h" +#include "SkBlurImageFilter.h" +#include "SkRRect.h" +#include "SkSurface.h" + +#define WIDTH 512 +#define HEIGHT 512 + +namespace skiagm { + +class ComplexClipBlurTiledGM : public GM { +public: + ComplexClipBlurTiledGM() { + } + +protected: + SkString onShortName() override { + return SkString("complexclip_blur_tiled"); + } + + SkISize onISize() override { + return SkISize::Make(WIDTH, HEIGHT); + } + + void onDraw(SkCanvas* canvas) override { + SkPaint blurPaint; + SkAutoTUnref blur(SkBlurImageFilter::Create(5.0f, 5.0f)); + blurPaint.setImageFilter(blur); + const SkScalar tile_size = SkIntToScalar(128); + SkRect bounds; + if (!canvas->getClipBounds(&bounds)) { + bounds.setEmpty(); + } + int ts = SkScalarCeilToInt(tile_size); + SkImageInfo info = SkImageInfo::MakeN32Premul(ts, ts); + SkAutoTUnref tileSurface(canvas->newSurface(info)); + if (!tileSurface.get()) { + tileSurface.reset(SkSurface::NewRaster(info)); + } + SkCanvas* tileCanvas = tileSurface->getCanvas(); + for (SkScalar y = bounds.top(); y < bounds.bottom(); y += tile_size) { + for (SkScalar x = bounds.left(); x < bounds.right(); x += tile_size) { + tileCanvas->save(); + tileCanvas->clear(0); + tileCanvas->translate(-x, -y); + SkRect rect = SkRect::MakeWH(WIDTH, HEIGHT); + tileCanvas->saveLayer(&rect, &blurPaint); + SkRRect rrect = SkRRect::MakeRectXY(rect.makeInset(20, 20), 25, 25); + tileCanvas->clipRRect(rrect, SkRegion::kDifference_Op, true); + SkPaint paint; + tileCanvas->drawRect(rect, paint); + tileCanvas->restore(); + tileCanvas->restore(); + SkAutoTUnref tileImage(tileSurface->newImageSnapshot()); + canvas->drawImage(tileImage, x, y); + } + } + } + +private: + typedef GM INHERITED; +}; + +////////////////////////////////////////////////////////////////////////////// + +static GM* MyFactory1(void*) { return new ComplexClipBlurTiledGM(); } +static GMRegistry reg1(MyFactory1); + +} diff --git a/include/core/SkCanvas.h b/include/core/SkCanvas.h index 2812b32856..4ad915c641 100644 --- a/include/core/SkCanvas.h +++ b/include/core/SkCanvas.h @@ -1379,11 +1379,10 @@ private: SkBaseDevice* init(SkBaseDevice*, InitFlags); /** - * Gets the size/origin of the top level layer in global canvas coordinates. We don't want this + * Gets the bounds of the top level layer in global canvas coordinates. We don't want this * to be public because it exposes decisions about layer sizes that are internal to the canvas. */ - SkISize getTopLayerSize() const; - SkIPoint getTopLayerOrigin() const; + SkIRect getTopLayerBounds() const; void internalDrawBitmapRect(const SkBitmap& bitmap, const SkRect* src, const SkRect& dst, const SkPaint* paint, diff --git a/src/core/SkCanvas.cpp b/src/core/SkCanvas.cpp index ce08d41a1f..c8c4b8749b 100644 --- a/src/core/SkCanvas.cpp +++ b/src/core/SkCanvas.cpp @@ -812,21 +812,19 @@ void SkCanvas::flush() { } } -SkISize SkCanvas::getTopLayerSize() const { - SkBaseDevice* d = this->getTopDevice(); - return d ? SkISize::Make(d->width(), d->height()) : SkISize::Make(0, 0); -} - -SkIPoint SkCanvas::getTopLayerOrigin() const { - SkBaseDevice* d = this->getTopDevice(); - return d ? d->getOrigin() : SkIPoint::Make(0, 0); -} - SkISize SkCanvas::getBaseLayerSize() const { SkBaseDevice* d = this->getDevice(); return d ? SkISize::Make(d->width(), d->height()) : SkISize::Make(0, 0); } +SkIRect SkCanvas::getTopLayerBounds() const { + SkBaseDevice* d = this->getTopDevice(); + if (!d) { + return SkIRect::MakeEmpty(); + } + return SkIRect::MakeXYWH(d->getOrigin().x(), d->getOrigin().y(), d->width(), d->height()); +} + SkBaseDevice* SkCanvas::getDevice() const { // return root device MCRec* rec = (MCRec*) fMCStack.front(); @@ -1516,7 +1514,7 @@ void SkCanvas::onClipRect(const SkRect& rect, SkRegion::Op op, ClipEdgeStyle edg if (rectStaysRect) { const bool isAA = kSoft_ClipEdgeStyle == edgeStyle; fClipStack->clipDevRect(devR, op, isAA); - fMCRec->fRasterClip.op(devR, this->getBaseLayerSize(), op, isAA); + fMCRec->fRasterClip.op(devR, this->getTopLayerBounds(), op, isAA); } else { // since we're rotated or some such thing, we convert the rect to a path // and clip against that, since it can handle any matrix. However, to @@ -1529,11 +1527,6 @@ void SkCanvas::onClipRect(const SkRect& rect, SkRegion::Op op, ClipEdgeStyle edg } } -static void rasterclip_path(SkRasterClip* rc, const SkCanvas* canvas, const SkPath& devPath, - SkRegion::Op op, bool doAA) { - rc->op(devPath, canvas->getBaseLayerSize(), op, doAA); -} - void SkCanvas::clipRRect(const SkRRect& rrect, SkRegion::Op op, bool doAA) { this->checkForDeferredSave(); ClipEdgeStyle edgeStyle = doAA ? kSoft_ClipEdgeStyle : kHard_ClipEdgeStyle; @@ -1557,7 +1550,7 @@ void SkCanvas::onClipRRect(const SkRRect& rrect, SkRegion::Op op, ClipEdgeStyle fClipStack->clipDevRRect(transformedRRect, op, kSoft_ClipEdgeStyle == edgeStyle); - fMCRec->fRasterClip.op(transformedRRect, this->getBaseLayerSize(), op, + fMCRec->fRasterClip.op(transformedRRect, this->getTopLayerBounds(), op, kSoft_ClipEdgeStyle == edgeStyle); return; } @@ -1643,7 +1636,7 @@ void SkCanvas::onClipPath(const SkPath& path, SkRegion::Op op, ClipEdgeStyle edg op = SkRegion::kReplace_Op; } - rasterclip_path(&fMCRec->fRasterClip, this, devPath, op, edgeStyle); + fMCRec->fRasterClip.op(devPath, this->getTopLayerBounds(), op, edgeStyle); } void SkCanvas::clipRegion(const SkRegion& rgn, SkRegion::Op op) { @@ -1691,7 +1684,7 @@ void SkCanvas::validateClip() const { default: { SkPath path; element->asPath(&path); - rasterclip_path(&tmpClip, this, path, element->getOp(), element->isAA()); + tmpClip.op(path, this->getTopLayerBounds(), element->getOp(), element->isAA()); break; } } diff --git a/src/core/SkRasterClip.cpp b/src/core/SkRasterClip.cpp index ea1a7db429..5afe279b95 100644 --- a/src/core/SkRasterClip.cpp +++ b/src/core/SkRasterClip.cpp @@ -161,18 +161,18 @@ bool SkRasterClip::setPath(const SkPath& path, const SkRegion& clip, bool doAA) return this->updateCacheAndReturnNonEmpty(); } -bool SkRasterClip::op(const SkRRect& rrect, const SkISize& size, SkRegion::Op op, bool doAA) { +bool SkRasterClip::op(const SkRRect& rrect, const SkIRect& bounds, SkRegion::Op op, bool doAA) { if (fForceConservativeRects) { - return this->op(rrect.getBounds(), size, op, doAA); + return this->op(rrect.getBounds(), bounds, op, doAA); } SkPath path; path.addRRect(rrect); - return this->op(path, size, op, doAA); + return this->op(path, bounds, op, doAA); } -bool SkRasterClip::op(const SkPath& path, const SkISize& size, SkRegion::Op op, bool doAA) { +bool SkRasterClip::op(const SkPath& path, const SkIRect& bounds, SkRegion::Op op, bool doAA) { AUTO_RASTERCLIP_VALIDATE(*this); if (fForceConservativeRects) { @@ -181,7 +181,7 @@ bool SkRasterClip::op(const SkPath& path, const SkISize& size, SkRegion::Op op, case kDoNothing_MutateResult: return !this->isEmpty(); case kReplaceClippedAgainstGlobalBounds_MutateResult: - ir = SkIRect::MakeSize(size); + ir = bounds; break; case kContinue_MutateResult: ir = path.getBounds().roundOut(); @@ -210,7 +210,7 @@ bool SkRasterClip::op(const SkPath& path, const SkISize& size, SkRegion::Op op, return this->op(clip, op); } } else { - base.setRect(0, 0, size.width(), size.height()); + base.setRect(bounds); if (SkRegion::kReplace_Op == op) { return this->setPath(path, base, doAA); @@ -285,7 +285,7 @@ static bool nearly_integral(SkScalar x) { return x - SkScalarFloorToScalar(x) < domain; } -bool SkRasterClip::op(const SkRect& r, const SkISize& size, SkRegion::Op op, bool doAA) { +bool SkRasterClip::op(const SkRect& r, const SkIRect& bounds, SkRegion::Op op, bool doAA) { AUTO_RASTERCLIP_VALIDATE(*this); if (fForceConservativeRects) { @@ -294,7 +294,7 @@ bool SkRasterClip::op(const SkRect& r, const SkISize& size, SkRegion::Op op, boo case kDoNothing_MutateResult: return !this->isEmpty(); case kReplaceClippedAgainstGlobalBounds_MutateResult: - ir = SkIRect::MakeSize(size); + ir = bounds; break; case kContinue_MutateResult: ir = r.roundOut(); diff --git a/src/core/SkRasterClip.h b/src/core/SkRasterClip.h index 4e2b1416e8..8f05bb9b3d 100644 --- a/src/core/SkRasterClip.h +++ b/src/core/SkRasterClip.h @@ -45,9 +45,9 @@ public: bool op(const SkIRect&, SkRegion::Op); bool op(const SkRegion&, SkRegion::Op); - bool op(const SkRect&, const SkISize&, SkRegion::Op, bool doAA); - bool op(const SkRRect&, const SkISize&, SkRegion::Op, bool doAA); - bool op(const SkPath&, const SkISize&, SkRegion::Op, bool doAA); + bool op(const SkRect&, const SkIRect&, SkRegion::Op, bool doAA); + bool op(const SkRRect&, const SkIRect&, SkRegion::Op, bool doAA); + bool op(const SkPath&, const SkIRect&, SkRegion::Op, bool doAA); void translate(int dx, int dy, SkRasterClip* dst) const; void translate(int dx, int dy) { diff --git a/src/pdf/SkPDFDevice.cpp b/src/pdf/SkPDFDevice.cpp index 17092bd2a0..0e11f61708 100644 --- a/src/pdf/SkPDFDevice.cpp +++ b/src/pdf/SkPDFDevice.cpp @@ -1708,7 +1708,8 @@ bool SkPDFDevice::handlePathAnnotation(const SkPath& path, SkPath transformedPath = path; transformedPath.transform(*d.fMatrix); SkRasterClip clip = *d.fRC; - clip.op(transformedPath, SkISize::Make(width(), height()), SkRegion::kIntersect_Op, false); + clip.op(transformedPath, SkIRect::MakeWH(width(), height()), SkRegion::kIntersect_Op, + false); SkRect transformedRect = SkRect::Make(clip.getBounds()); SkData* urlData = annotation->find(SkAnnotationKeys::URL_Key()); diff --git a/src/utils/SkLua.cpp b/src/utils/SkLua.cpp index edd336afdf..dd74640401 100644 --- a/src/utils/SkLua.cpp +++ b/src/utils/SkLua.cpp @@ -632,10 +632,7 @@ static int lcanvas_getClipStack(lua_State* L) { int SkLua::lcanvas_getReducedClipStack(lua_State* L) { #if SK_SUPPORT_GPU const SkCanvas* canvas = get_ref(L, 1); - SkISize layerSize = canvas->getTopLayerSize(); - SkIPoint layerOrigin = canvas->getTopLayerOrigin(); - SkIRect queryBounds = SkIRect::MakeXYWH(layerOrigin.fX, layerOrigin.fY, - layerSize.fWidth, layerSize.fHeight); + SkIRect queryBounds = canvas->getTopLayerBounds(); GrReducedClip::ElementList elements; GrReducedClip::InitialState initialState; diff --git a/tests/AAClipTest.cpp b/tests/AAClipTest.cpp index 1ee6358a6e..faf45ed70b 100644 --- a/tests/AAClipTest.cpp +++ b/tests/AAClipTest.cpp @@ -370,7 +370,7 @@ static bool operator==(const SkRasterClip& a, const SkRasterClip& b) { static void did_dx_affect(skiatest::Reporter* reporter, const SkScalar dx[], size_t count, bool changed) { - const SkISize baseSize = SkISize::Make(10, 10); + const SkIRect baseBounds = SkIRect::MakeXYWH(0, 0, 10, 10); SkIRect ir = { 0, 0, 10, 10 }; for (size_t i = 0; i < count; ++i) { @@ -381,11 +381,11 @@ static void did_dx_affect(skiatest::Reporter* reporter, const SkScalar dx[], SkRasterClip rc1(ir); SkRasterClip rc2(ir); - rc0.op(r, baseSize, SkRegion::kIntersect_Op, false); + rc0.op(r, baseBounds, SkRegion::kIntersect_Op, false); r.offset(dx[i], 0); - rc1.op(r, baseSize, SkRegion::kIntersect_Op, true); + rc1.op(r, baseBounds, SkRegion::kIntersect_Op, true); r.offset(-2*dx[i], 0); - rc2.op(r, baseSize, SkRegion::kIntersect_Op, true); + rc2.op(r, baseBounds, SkRegion::kIntersect_Op, true); REPORTER_ASSERT(reporter, changed != (rc0 == rc1)); REPORTER_ASSERT(reporter, changed != (rc0 == rc2)); @@ -431,7 +431,7 @@ static void test_crbug_422693(skiatest::Reporter* reporter) { SkRasterClip rc(SkIRect::MakeLTRB(-25000, -25000, 25000, 25000)); SkPath path; path.addCircle(50, 50, 50); - rc.op(path, rc.getBounds().size(), SkRegion::kIntersect_Op, true); + rc.op(path, rc.getBounds(), SkRegion::kIntersect_Op, true); } DEF_TEST(AAClip, reporter) {