Change saveLayer() semantics to take unfiltered bounds.

src/core/SkCanvas.cpp

 /*
  * Copyright 2008 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkBitmapDevice.h"
 #include "SkCanvas.h"
 #include "SkCanvasPriv.h"
@@ skipping 394 matching lines @@
     // The paint has both a colorfilter(paintCF) and an imagefilter-which-is-a-colorfilter(imgCF)
     // and we need to combine them into a single colorfilter.
     SkAutoTUnref<SkColorFilter> autoImgCF(imgCF);
     return SkColorFilter::CreateComposeFilter(imgCF, paintCF);
 }
 
 class AutoDrawLooper {
 public:
     AutoDrawLooper(SkCanvas* canvas, const SkSurfaceProps& props, const SkPaint& paint,
                    bool skipLayerForImageFilter = false,
                    const SkRect* bounds = nullptr) : fOrigPaint(paint) {

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

Lets rename this parameter somehow, so it is clear to the next reader what kind
of "bounds" it is. Also, perhaps a comment for the constructor could mention
this as well.

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

Done.

         fCanvas = canvas;
         fFilter = canvas->getDrawFilter();
         fPaint = &fOrigPaint;
         fSaveCount = canvas->getSaveCount();
         fTempLayerForImageFilter = false;
         fDone = false;
 
         SkColorFilter* simplifiedCF = image_to_color_filter(fOrigPaint);
         if (simplifiedCF) {
             SkPaint* paint = set_if_needed(&fLazyPaintInit, fOrigPaint);
@@ skipping 14 matching lines @@
              *      return (fPaint). We then draw the primitive (using srcover) into a cleared
              *      buffer/surface.
              *  3. Restore the layer created in #1
              *      The imagefilter is passed the buffer/surface from the layer (now filled with the
              *      src pixels of the primitive). It returns a new "filtered" buffer, which we
              *      draw onto the previous layer using the xfermode from the original paint.
              */
             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;
             // we remove the imagefilter/xfermode inside doNext()
         }
 
         if (SkDrawLooper* looper = paint.getLooper()) {
             void* buffer = fLooperContextAllocator.reserveT<SkDrawLooper::Context>(
                     looper->contextSize());
             fLooperContext = looper->createContext(canvas, buffer);
@@ skipping 552 matching lines @@
 
 bool SkCanvas::clipRectBounds(const SkRect* bounds, SaveFlags flags,
                               SkIRect* intersection, const SkImageFilter* imageFilter) {
     SkIRect clipBounds;
     if (!this->getClipDeviceBounds(&clipBounds)) {
         return false;
     }
 
     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) {
         SkRect r;
 
         ctm.mapRect(&r, *bounds);
         r.roundOut(&ir);
         // early exit if the layer's bounds are clipped out
         if (!ir.intersect(clipBounds)) {
             if (bounds_affects_clip(flags)) {
@@ skipping 924 matching lines @@
 
     SkRect r, storage;
     const SkRect* bounds = nullptr;
     if (paint.canComputeFastBounds()) {
         // special-case 2 points (common for drawing a single line)
         if (2 == count) {
             r.set(pts[0], pts[1]);
         } 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);
 
     LOOPER_BEGIN(paint, SkDrawFilter::kPoint_Type, bounds)
 
     while (iter.next()) {
         iter.fDevice->drawPoints(iter, mode, count, pts, looper.paint());
     }
 
     LOOPER_END
 }
 
 void SkCanvas::onDrawRect(const SkRect& r, const SkPaint& paint) {
     TRACE_EVENT0("disabled-by-default-skia", "SkCanvas::drawRect()");
     SkRect storage;
     const SkRect* bounds = nullptr;
     if (paint.canComputeFastBounds()) {
         // Skia will draw an inverted rect, because it explicitly "sorts" it downstream.
         // To prevent accidental rejecting at this stage, we have to sort it before we check.
         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)
 
     while (iter.next()) {
         iter.fDevice->drawRect(iter, r, looper.paint());
     }
 
     LOOPER_END
 }
 
 void SkCanvas::onDrawOval(const SkRect& oval, const SkPaint& paint) {
     TRACE_EVENT0("disabled-by-default-skia", "SkCanvas::drawOval()");
     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)
 
     while (iter.next()) {
         iter.fDevice->drawOval(iter, oval, looper.paint());
     }
 
     LOOPER_END
 }
 
 void SkCanvas::onDrawRRect(const SkRRect& rrect, const SkPaint& paint) {
     TRACE_EVENT0("disabled-by-default-skia", "SkCanvas::drawRRect()");
     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()) {
         // call the non-virtual version
         this->SkCanvas::drawRect(rrect.getBounds(), paint);
         return;
     } else if (rrect.isOval()) {
         // call the non-virtual version
         this->SkCanvas::drawOval(rrect.getBounds(), paint);
         return;
     }
 
     LOOPER_BEGIN(paint, SkDrawFilter::kRRect_Type, bounds)
 
     while (iter.next()) {
         iter.fDevice->drawRRect(iter, rrect, looper.paint());
     }
 
     LOOPER_END
 }
 
 void SkCanvas::onDrawDRRect(const SkRRect& outer, const SkRRect& inner,
                             const SkPaint& paint) {
     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)
 
     while (iter.next()) {
         iter.fDevice->drawDRRect(iter, outer, inner, looper.paint());
     }
 
     LOOPER_END
 }
 
 void SkCanvas::onDrawPath(const SkPath& path, const SkPaint& paint) {
     TRACE_EVENT0("disabled-by-default-skia", "SkCanvas::drawPath()");
     if (!path.isFinite()) {
         return;
     }
 
     SkRect storage;
     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();
     if (r.width() <= 0 && r.height() <= 0) {
         if (path.isInverseFillType()) {
             this->internalDrawPaint(paint);
             return;
         }
     }
 
     LOOPER_BEGIN(paint, SkDrawFilter::kPath_Type, bounds)
 
     while (iter.next()) {
         iter.fDevice->drawPath(iter, path, looper.paint());
     }
 
     LOOPER_END
 }
 
 void SkCanvas::onDrawImage(const SkImage* image, SkScalar x, SkScalar y, const SkPaint* paint) {
     TRACE_EVENT0("disabled-by-default-skia", "SkCanvas::drawImage()");
     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;
     if (nullptr == paint) {
         paint = lazy.init();
     }
     
     LOOPER_BEGIN(*paint, SkDrawFilter::kBitmap_Type, &bounds)
     
     while (iter.next()) {
         iter.fDevice->drawImage(iter, image, x, y, looper.paint());
     }
     
     LOOPER_END
 }
 
 void SkCanvas::onDrawImageRect(const SkImage* image, const SkRect* src, const SkRect& dst,
                                const SkPaint* paint, SrcRectConstraint constraint) {
     TRACE_EVENT0("disabled-by-default-skia", "SkCanvas::drawImageRect()");
     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) {
         paint = lazy.init();
     }
     
     LOOPER_BEGIN_CHECK_COMPLETE_OVERWRITE(*paint, SkDrawFilter::kBitmap_Type, bounds,
                                           image->isOpaque())
     
     while (iter.next()) {
@@ skipping 16 matching lines @@
         paint = lazy.init();
     }
 
     const SkMatrix matrix = SkMatrix::MakeTrans(x, y);
 
     SkRect storage;
     const SkRect* bounds = nullptr;
     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)
 
     while (iter.next()) {
         iter.fDevice->drawBitmap(iter, bitmap, matrix, looper.paint());
     }
     
     LOOPER_END
 }
 
 // this one is non-virtual, so it can be called safely by other canvas apis
 void SkCanvas::internalDrawBitmapRect(const SkBitmap& bitmap, const SkRect* src,
                                       const SkRect& dst, const SkPaint* paint,
                                       SrcRectConstraint constraint) {
     if (bitmap.drawsNothing() || dst.isEmpty()) {
         return;
     }
 
     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;
     if (nullptr == paint) {
         paint = lazy.init();
     }
 
     LOOPER_BEGIN_CHECK_COMPLETE_OVERWRITE(*paint, SkDrawFilter::kBitmap_Type, bounds,
                                           bitmap.isOpaque())
 
@@ skipping 11 matching lines @@
     this->internalDrawBitmapRect(bitmap, src, dst, paint, constraint);
 }
 
 void SkCanvas::onDrawImageNine(const SkImage* image, const SkIRect& center, const SkRect& dst,
                                const SkPaint* paint) {
     TRACE_EVENT0("disabled-by-default-skia", "SkCanvas::drawImageNine()");
     
     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;
     if (nullptr == paint) {
         paint = lazy.init();
     }
     
     LOOPER_BEGIN(*paint, SkDrawFilter::kBitmap_Type, bounds)
     
     while (iter.next()) {
         iter.fDevice->drawImageNine(iter, image, center, dst, looper.paint());
     }
     
     LOOPER_END
 }
 
 void SkCanvas::onDrawBitmapNine(const SkBitmap& bitmap, const SkIRect& center, const SkRect& dst,
                                 const SkPaint* paint) {
     TRACE_EVENT0("disabled-by-default-skia", "SkCanvas::drawBitmapNine()");
     SkDEBUGCODE(bitmap.validate();)
 
     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;
     if (nullptr == paint) {
         paint = lazy.init();
     }
     
     LOOPER_BEGIN(*paint, SkDrawFilter::kBitmap_Type, bounds)
     
     while (iter.next()) {
@@ skipping 152 matching lines @@
     LOOPER_END
 }
 
 void SkCanvas::onDrawTextBlob(const SkTextBlob* blob, SkScalar x, SkScalar y,
                               const SkPaint& paint) {
 
     SkRect storage;
     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
     // incomplete at this point (text-related attributes are embedded within blob run paints).
     SkDrawFilter* drawFilter = fMCRec->fFilter;
     fMCRec->fFilter = nullptr;
 
     LOOPER_BEGIN(paint, SkDrawFilter::kText_Type, bounds)
 
     while (iter.next()) {
@@ skipping 408 matching lines @@
     }
 
     if (matrix) {
         canvas->concat(*matrix);
     }
 }
 
 SkAutoCanvasMatrixPaint::~SkAutoCanvasMatrixPaint() {
     fCanvas->restoreToCount(fSaveCount);
 }