Implement correct clipping for image filters.

Implement correct clipping for image filters.

Image filters in Skia currently clip the size of the the offscreen
bitmap used for filtering to the device clip bounds. This means that
any pixel-moving filter (e.g., blur) has edge artifacts at the clip
boundaries. This is problematic for tiling, where a single SkPicture
is played back with a clip set to the tile boundaries.

By implementing the onFilterBounds() traversal, and using it in
saveLayer() when a filter is present, we can clip the layer to the
expanded clip rect. Note that this requires that the traversal be
performed in reverse as compared to computeFastBounds().  (It's also
done in device space, unlike computeFastBounds()).

New test imagefiltersclipped tests pixel-moving filters when clipped
by various clip rects.
New test imageblurtiled tests tiled (compositor-style) rendering of
blurred text. There should be no artifacts at the tile boundaries.

BUG=337831
R=reed@google.com

Committed: https://code.google.com/p/skia/source/detail?r=13323

[Stephen White, 2014-02-03 23:59:37]

PTAL. Thanks!

[reed1, 2014-02-04 15:30:07]

https://codereview.chromium.org/23011012/diff/156001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/23011012/diff/156001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:1029: // returns false if the entire rectangle is
entirely clipped out
Lets add dox for how the optional imageFilter parameter is interpreted

https://codereview.chromium.org/23011012/diff/156001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:1031: SkIRect* intersection, const SkImageFilter*
imageFilter = NULL);
nit: SkCanvas.h is predominantly 80-col

https://codereview.chromium.org/23011012/diff/156001/include/core/SkImageFilt...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/23011012/diff/156001/include/core/SkImageFilt...
include/core/SkImageFilter.h:192: // if no inputs.
Perhaps augment dox:

- Given the bounds of the src image to be filtered (first parmeter) in device
coordinates, and the CTM, compute (conservatively) the resulting
device-coordinates bounds that this filter will produce. Used for culling and
temp-buffer allocations, so the result need not be exact, but should never be
smaller than the real answer.

https://codereview.chromium.org/23011012/diff/156001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/23011012/diff/156001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:804: op = SkRegion::kReplace_Op;
Hmm, this make me nervous. I understand that the offscreen needs to be larger
sometimes for the imagefilter to avoid edge-artifacts. It seems odd that we are
expanding "the" clip, when all we really want to do is expand the offscreen's
bounds (and keep its clip open), since on restore() the original clip is fine
(need not be expanded).

[Stephen White, 2014-02-04 16:01:40]

Thanks for your review. New patch is up.

https://codereview.chromium.org/23011012/diff/156001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/23011012/diff/156001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:1029: // returns false if the entire rectangle is
entirely clipped out
On 2014/02/04 15:30:08, reed1 wrote:
> Lets add dox for how the optional imageFilter parameter is interpreted

Done.

https://codereview.chromium.org/23011012/diff/156001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:1031: SkIRect* intersection, const SkImageFilter*
imageFilter = NULL);
On 2014/02/04 15:30:08, reed1 wrote:
> nit: SkCanvas.h is predominantly 80-col

Fixed.

https://codereview.chromium.org/23011012/diff/156001/include/core/SkImageFilt...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/23011012/diff/156001/include/core/SkImageFilt...
include/core/SkImageFilter.h:192: // if no inputs.
On 2014/02/04 15:30:08, reed1 wrote:
> Perhaps augment dox:
> 
> - Given the bounds of the src image to be filtered (first parmeter) in device
> coordinates, and the CTM, compute (conservatively) the resulting
> device-coordinates bounds that this filter will produce. Used for culling and
> temp-buffer allocations, so the result need not be exact, but should never be
> smaller than the real answer.

Done, although I've changed the wording slightly. This one is not actually used
for
culling: computeFastBounds() is. This one applies the inverse transformation 
("what pixels of the source image would be required to fill the given 
destination rect").

https://codereview.chromium.org/23011012/diff/156001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/23011012/diff/156001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:804: op = SkRegion::kReplace_Op;
On 2014/02/04 15:30:08, reed1 wrote:
> Hmm, this make me nervous. I understand that the offscreen needs to be larger
> sometimes for the imagefilter to avoid edge-artifacts. It seems odd that we
are
> expanding "the" clip, when all we really want to do is expand the offscreen's
> bounds (and keep its clip open), since on restore() the original clip is fine
> (need not be expanded).

I'm pretty sure we also need to expand the clip inside the saveLayer(), since 
otherwise the primitives that are drawn there will be clipped against the 
original device bounds. However, it shouldn't affect the main clip, since the 
saveLayer() for image filters always saves the clip and restores it before the 
drawDevice() which applies the filters.

However, if there's a better way of accomplishing this, I'm open to suggestions.

[Stephen White, 2014-02-04 18:05:26]

Note to Robert: when this lands, the following Blink layout tests will need to
be suppressed for rebaselining in skia_test_expectations.txt:

  svg/W3C-SVG-1.1/filters-blend-01-b.svg [ Failure ]
  css3/filters/effect-brightness-clamping-hw.html [ ImageOnlyFailure ]
  css3/filters/effect-drop-shadow-hw.html [ ImageOnlyFailure ]
  css3/filters/effect-reference-hidpi-hw.html [ ImageOnlyFailure ]
  css3/filters/effect-reference-subregion-hw.html [ ImageOnlyFailure ]
  css3/filters/effect-reference-subregion.html [ ImageOnlyFailure ]
  css3/filters/filter-change-repaint-composited.html [ ImageOnlyFailure ]
  css3/filters/filter-change-repaint.html [ ImageOnlyFailure ]
  fast/box-shadow/scaled-box-shadow.html [ ImageOnlyFailure ]
  svg/W3C-SVG-1.1/filters-gauss-01-b.svg [ ImageOnlyFailure ]
  svg/W3C-SVG-1.1/filters-morph-01-f.svg [ ImageOnlyFailure ]
  svg/dynamic-updates/SVGFEGaussianBlurElement-dom-in-attr.html [
ImageOnlyFailure ]
  svg/dynamic-updates/SVGFEGaussianBlurElement-dom-stdDeviation-attr.html [
ImageOnlyFailure ]
  svg/dynamic-updates/SVGFEGaussianBlurElement-dom-stdDeviation-call.html [
ImageOnlyFailure ]
  svg/dynamic-updates/SVGFEGaussianBlurElement-svgdom-in-prop.html [
ImageOnlyFailure ]
  svg/filters/feDropShadow-zero-deviation.svg [ ImageOnlyFailure ]
  svg/filters/feDropShadow.svg [ ImageOnlyFailure ]
  svg/filters/feGaussianBlur.svg [ ImageOnlyFailure ]
  svg/filters/filterRes.svg [ ImageOnlyFailure ]
  svg/filters/shadow-on-filter.svg [ ImageOnlyFailure ]
  virtual/gpu/fast/canvas/canvas-incremental-repaint.html [ ImageOnlyFailure ]

[reed1, 2014-02-04 21:14:05]

Still am curious if we're still right to (re)use saveLayer for filtering, but I
agree that, since we've already said save(), it may be OK to wack (enlarge) the
current clip as needed. lgtm

[Stephen White, 2014-02-05 17:51:31]

Committed patchset #11 manually as r13323 (presubmit successful).

[robertphillips, 2014-02-06 13:41:39]

From your suppression list, I also had to suppress:

  svg/batik/text/smallFonts.svg [ ImageOnlyFailure ]
  svg/batik/text/textFeatures.svg [ ImageOnlyFailure ]

(which were Mac-only failures) but didn't have to suppress:

svg/W3C-SVG-1.1/filters-blend-01-b.svg [ Failure ]
fast/box-shadow/scaled-box-shadow.html [ ImageOnlyFailure ]
virtual/gpu/fast/canvas/canvas-incremental-repaint.html [ ImageOnlyFailure ]

Both scaled-box-shadow and canvas-incremental-repaint are already suppressed for
r13238 (Fast blurred rectangles on GPU).

[Stephen White, 2014-02-06 13:45:07]

On 2014/02/06 13:41:39, robertphillips wrote:
> From your suppression list, I also had to suppress:
> 
>   svg/batik/text/smallFonts.svg [ ImageOnlyFailure ]
>   svg/batik/text/textFeatures.svg [ ImageOnlyFailure ]
> 
> (which were Mac-only failures) but didn't have to suppress:
> 
> svg/W3C-SVG-1.1/filters-blend-01-b.svg [ Failure ]
> fast/box-shadow/scaled-box-shadow.html [ ImageOnlyFailure ]
> virtual/gpu/fast/canvas/canvas-incremental-repaint.html [ ImageOnlyFailure ]
> 
> Both scaled-box-shadow and canvas-incremental-repaint are already suppressed
for
> r13238 (Fast blurred rectangles on GPU).

OK, sounds reasonable. Thanks!

[tomhudson, 2014-02-11 12:18:08]

This patch brought in a 3x performance regression on
blur_image_filter_{small,large}_80.00_80.00

Was that expected?

[Stephen White, 2014-02-11 15:02:33]

On 2014/02/11 12:18:08, tomhudson wrote:
> This patch brought in a 3x performance regression on
> blur_image_filter_{small,large}_80.00_80.00
> 
> Was that expected?

Yes, that's expected. With no bounds set in the saveLayer() call, the blur is
not clipped in order to get the edges correct (see the corresponding changes in
the GMs), which means larger offscreens and more processing.

This is needed to avoid artifacts on tile boundaries in impl-side painting. If
the caller wants to constrain the offscreen size to the clip rect, they can
specify smaller bounds in saveLayer(), or use the blur filter's crop rect.