Change saveLayer() semantics to take unfiltered bounds.

src/core/SkCanvas.cpp

Index: src/core/SkCanvas.cpp
diff --git a/src/core/SkCanvas.cpp b/src/core/SkCanvas.cpp
index 6dd8d0472136ef1ca0690e5420e8688167b952f4..e0d89079832d334789c2c52b8a140fa5e8780952 100644
--- a/src/core/SkCanvas.cpp
+++ b/src/core/SkCanvas.cpp
@@ -447,6 +447,16 @@ public:
             SkPaint tmp;
             tmp.setImageFilter(fPaint->getImageFilter());
             tmp.setXfermode(fPaint->getXfermode());
+#ifndef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED
+            SkRect storage;

[reed1, 2015/10/27 20:05:40]

one way to make the intent of this block clear, could be to create a small
helper function that just does this.

e.g.

/**
 * There are many bounds in skia. A circle's bounds is just its center extended
by its radius. However, if we stroke
 * a circle, then the "bounds" of that is larger, since it will now draw outside
of its raw-bounds by 1/2 the stroke width.
 * SkPaint has lots of optional effects/attributes that can modify the effective
bounds of a given primitive
 *   - maskfilters, patheffects, stroking, etc.
 *
 *  This function takes a raw bounds and a paint, and returns the conservative
"effective" bounds based on the 
 * settings in the paint... with one exception. This function does *not* look at
the imagefilter, which can also modify
 * the effective bounds. It is deliberately ignored.
 */
static SkRect apply_paint_to_bounds_sans_imagefilter(const SkPaint& paint, const
SkRect& rawBounds) {
 ....
    return effectiveBounds;
}

[Stephen White, 2015/10/27 20:59:55]

Done.

+            if (bounds) {
+                SkPaint tmpUnfiltered(*fPaint);
+                tmpUnfiltered.setImageFilter(nullptr);
+                if (tmpUnfiltered.canComputeFastBounds()) {
+                    bounds = &tmpUnfiltered.computeFastBounds(*bounds, &storage);
+                }
+            }
+#endif
             (void)canvas->internalSaveLayer(bounds, &tmp, SkCanvas::kARGB_ClipLayer_SaveFlag,
                                             SkCanvas::kFullLayer_SaveLayerStrategy);
             fTempLayerForImageFilter = true;
@@ -1019,8 +1029,19 @@ bool SkCanvas::clipRectBounds(const SkRect* bounds, SaveFlags flags,
 
     const SkMatrix& ctm = fMCRec->fMatrix;  // this->getTotalMatrix()
 
+#ifndef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED
+    SkRect storage;
+#endif
     if (imageFilter) {
         imageFilter->filterBounds(clipBounds, ctm, &clipBounds);
+#ifndef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED

[reed1, 2015/10/27 20:05:40]

This bock of code exists solely as a temporary hack to maintain results, until
we can update the actual filters. Lets make that obvious to the caller.

1. a comment to that effect
2. Perhaps a different #ifdef FLAG as well, like
SK_SUPPORT_SRC_BOUNDS_BLOAT_FOR_IMAGEFILERS. This can naturally only be valid if
we're not in SK_SAVE_LAYER_BOUNDS_ARE_FILTERED, but adding a different flag is
another way to document this, and remind us to remove it eventually.

[Stephen White, 2015/10/27 20:59:55]

Done.

+        if (bounds && imageFilter->canComputeFastBounds()) {
+            imageFilter->computeFastBounds(*bounds, &storage);
+            bounds = &storage;
+        } else {
+            bounds = nullptr;

[reed1, 2015/10/27 20:05:40]

I presume we set this to null because we assume that the filter is telling us
that it affects all of space, by returning false for canComputeFastBounds? If
so, that connection isn't necessarily obvious. I wonder if a different (helper)
method could make that clearer.

e.g.
    bool SkImageFilter::affectsAllSpaceOrSomethingLikeThat() const { return
this->canComputeFastBounds(); }

[Stephen White, 2015/10/27 20:59:55]

Should we just rename canComputeFastBounds(), then? It seems kind of
silly two have two methods.

+        }
+#endif
     }
     SkIRect ir;
     if (bounds) {
@@ -1965,10 +1986,19 @@ void SkCanvas::onDrawPoints(PointMode mode, size_t count, const SkPoint pts[],
         } else {
             r.set(pts, SkToInt(count));
         }
+#ifdef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED
         bounds = &paint.computeFastStrokeBounds(r, &storage);
         if (this->quickReject(*bounds)) {
             return;
         }
+#else
+        if (this->quickReject(paint.computeFastStrokeBounds(r, &storage))) {
+            return;
+        }
+        SkPaint unfiltered(paint);
+        unfiltered.setImageFilter(nullptr);
+        bounds = &unfiltered.computeFastStrokeBounds(r, &storage);

[reed1, 2015/10/27 20:05:40]

don't we just want r here?

bounds = &r;

[Stephen White, 2015/10/27 20:59:55]

Hmm, yeah. Done.

+#endif
     }
 
     SkASSERT(pts != nullptr);
@@ -1992,10 +2022,17 @@ void SkCanvas::onDrawRect(const SkRect& r, const SkPaint& paint) {
         SkRect tmp(r);
         tmp.sort();
 
+#ifdef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED
         bounds = &paint.computeFastBounds(tmp, &storage);
         if (this->quickReject(*bounds)) {
             return;
         }
+#else
+        if (this->quickReject(paint.computeFastBounds(tmp, &storage))) {
+            return;
+        }
+        bounds = &r;
+#endif
     }
 
     LOOPER_BEGIN_CHECK_COMPLETE_OVERWRITE(paint, SkDrawFilter::kRect_Type, bounds, false)
@@ -2012,10 +2049,17 @@ void SkCanvas::onDrawOval(const SkRect& oval, const SkPaint& paint) {
     SkRect storage;
     const SkRect* bounds = nullptr;
     if (paint.canComputeFastBounds()) {
+#ifdef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED
         bounds = &paint.computeFastBounds(oval, &storage);
         if (this->quickReject(*bounds)) {
             return;
         }
+#else
+        if (this->quickReject(paint.computeFastBounds(oval, &storage))) {
+            return;
+        }
+        bounds = &oval;
+#endif
     }
 
     LOOPER_BEGIN(paint, SkDrawFilter::kOval_Type, bounds)
@@ -2032,10 +2076,17 @@ void SkCanvas::onDrawRRect(const SkRRect& rrect, const SkPaint& paint) {
     SkRect storage;
     const SkRect* bounds = nullptr;
     if (paint.canComputeFastBounds()) {
+#ifdef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED
         bounds = &paint.computeFastBounds(rrect.getBounds(), &storage);
         if (this->quickReject(*bounds)) {
             return;
         }
+#else
+        if (this->quickReject(paint.computeFastBounds(rrect.getBounds(), &storage))) {
+            return;
+        }
+        bounds = &rrect.getBounds();
+#endif
     }
 
     if (rrect.isRect()) {
@@ -2062,10 +2113,17 @@ void SkCanvas::onDrawDRRect(const SkRRect& outer, const SkRRect& inner,
     SkRect storage;
     const SkRect* bounds = nullptr;
     if (paint.canComputeFastBounds()) {
+#ifdef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED
         bounds = &paint.computeFastBounds(outer.getBounds(), &storage);
         if (this->quickReject(*bounds)) {
             return;
         }
+#else
+        if (this->quickReject(paint.computeFastBounds(outer.getBounds(), &storage))) {
+            return;
+        }
+        bounds = &outer.getBounds();
+#endif
     }
 
     LOOPER_BEGIN(paint, SkDrawFilter::kRRect_Type, bounds)
@@ -2087,10 +2145,17 @@ void SkCanvas::onDrawPath(const SkPath& path, const SkPaint& paint) {
     const SkRect* bounds = nullptr;
     if (!path.isInverseFillType() && paint.canComputeFastBounds()) {
         const SkRect& pathBounds = path.getBounds();
+#ifdef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED
         bounds = &paint.computeFastBounds(pathBounds, &storage);
         if (this->quickReject(*bounds)) {
             return;
         }
+#else
+        if (this->quickReject(paint.computeFastBounds(pathBounds, &storage))) {
+            return;
+        }
+        bounds = &pathBounds;
+#endif
     }
 
     const SkRect& r = path.getBounds();
@@ -2115,12 +2180,22 @@ void SkCanvas::onDrawImage(const SkImage* image, SkScalar x, SkScalar y, const S
     SkRect bounds = SkRect::MakeXYWH(x, y,
                                      SkIntToScalar(image->width()), SkIntToScalar(image->height()));
     if (nullptr == paint || paint->canComputeFastBounds()) {
+#ifdef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED
         if (paint) {
             paint->computeFastBounds(bounds, &bounds);
         }
         if (this->quickReject(bounds)) {
             return;
         }
+#else
+        SkRect tmp = bounds;
+        if (paint) {
+            paint->computeFastBounds(tmp, &tmp);
+        }
+        if (this->quickReject(tmp)) {
+            return;
+        }
+#endif
     }
     
     SkLazyPaint lazy;
@@ -2143,12 +2218,22 @@ void SkCanvas::onDrawImageRect(const SkImage* image, const SkRect* src, const Sk
     SkRect storage;
     const SkRect* bounds = &dst;
     if (nullptr == paint || paint->canComputeFastBounds()) {
+#ifdef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED
         if (paint) {
             bounds = &paint->computeFastBounds(dst, &storage);
         }
         if (this->quickReject(*bounds)) {
             return;
         }
+#else
+        storage = dst;
+        if (paint) {
+            paint->computeFastBounds(dst, &storage);
+        }
+        if (this->quickReject(storage)) {
+            return;
+        }
+#endif
     }
     SkLazyPaint lazy;
     if (nullptr == paint) {
@@ -2185,10 +2270,18 @@ void SkCanvas::onDrawBitmap(const SkBitmap& bitmap, SkScalar x, SkScalar y, cons
     if (paint->canComputeFastBounds()) {
         bitmap.getBounds(&storage);
         matrix.mapRect(&storage);
+#ifdef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED
         bounds = &paint->computeFastBounds(storage, &storage);
         if (this->quickReject(*bounds)) {
             return;
         }
+#else
+        SkRect tmp = storage;
+        if (this->quickReject(paint->computeFastBounds(tmp, &tmp))) {
+            return;
+        }
+        bounds = &storage;
+#endif
     }
 
     LOOPER_BEGIN(*paint, SkDrawFilter::kBitmap_Type, bounds)
@@ -2211,12 +2304,18 @@ void SkCanvas::internalDrawBitmapRect(const SkBitmap& bitmap, const SkRect* src,
     SkRect storage;
     const SkRect* bounds = &dst;
     if (nullptr == paint || paint->canComputeFastBounds()) {
+#ifdef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED
         if (paint) {
             bounds = &paint->computeFastBounds(dst, &storage);
         }
         if (this->quickReject(*bounds)) {
             return;
         }
+#else
+        if (this->quickReject(paint ? paint->computeFastBounds(dst, &storage) : dst)) {
+            return;
+        }
+#endif
     }
 
     SkLazyPaint lazy;
@@ -2248,12 +2347,18 @@ void SkCanvas::onDrawImageNine(const SkImage* image, const SkIRect& center, cons
     SkRect storage;
     const SkRect* bounds = &dst;
     if (nullptr == paint || paint->canComputeFastBounds()) {
+#ifdef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED
         if (paint) {
             bounds = &paint->computeFastBounds(dst, &storage);
         }
         if (this->quickReject(*bounds)) {
             return;
         }
+#else
+        if (this->quickReject(paint ? paint->computeFastBounds(dst, &storage) : dst)) {
+            return;
+        }
+#endif
     }
     
     SkLazyPaint lazy;
@@ -2278,12 +2383,18 @@ void SkCanvas::onDrawBitmapNine(const SkBitmap& bitmap, const SkIRect& center, c
     SkRect storage;
     const SkRect* bounds = &dst;
     if (nullptr == paint || paint->canComputeFastBounds()) {
+#ifdef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED
         if (paint) {
             bounds = &paint->computeFastBounds(dst, &storage);
         }
         if (this->quickReject(*bounds)) {
             return;
         }
+#else
+        if (this->quickReject(paint ? paint->computeFastBounds(dst, &storage) : dst)) {
+            return;
+        }
+#endif
     }
     
     SkLazyPaint lazy;
@@ -2456,11 +2567,19 @@ void SkCanvas::onDrawTextBlob(const SkTextBlob* blob, SkScalar x, SkScalar y,
     const SkRect* bounds = nullptr;
     if (paint.canComputeFastBounds()) {
         storage = blob->bounds().makeOffset(x, y);
+#ifdef SK_SAVE_LAYER_BOUNDS_ARE_FILTERED
         bounds = &paint.computeFastBounds(storage, &storage);
 
         if (this->quickReject(*bounds)) {
             return;
         }
+#else
+        SkRect tmp;
+        if (this->quickReject(paint.computeFastBounds(storage, &tmp))) {
+            return;
+        }
+        bounds = &storage;
+#endif
     }
 
     // We cannot filter in the looper as we normally do, because the paint is