Fix MatrixImageFilter computeFastBounds

Fix MatrixImageFilter computeFastBounds

This CL makes the bounding box returned from SkMatrixImageFilter::computeFastBounds actually contain the final result pixels. This, in turn, fixes the first two rows of the filterfastbounds GM and the SVG web page cited in the bug.

BUG=418417
Committed: https://skia.googlesource.com/skia/+/8b7b9c2e804b54a8301363091f418cfeda719fe3

[robertphillips, 2014-12-08 17:13:45]

robertphillips@google.com changed reviewers:
+ reed@google.com, senorblanco@google.com

[Stephen White, 2014-12-08 17:20:51]

senorblanco@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Stephen White, 2014-12-08 17:20:51]

https://codereview.chromium.org/785893004/diff/1/src/effects/SkMatrixImageFil...
File src/effects/SkMatrixImageFilter.cpp (right):

https://codereview.chromium.org/785893004/diff/1/src/effects/SkMatrixImageFil...
src/effects/SkMatrixImageFilter.cpp:102: dst->join(bounds);   // Work around for
skia:3194
This really doesn't seem right to me, and we could come up with cases where
it'll fail. I think we really need the CTM here to do this correctly.

Granted, the original implementation wasn't correct either, but I'd rather get
us closer to correct than just paper over individual cases.

[robertphillips, 2014-12-08 17:32:46]

> This really doesn't seem right to me, and we could come up with cases where
> it'll fail. I think we really need the CTM here to do this correctly.

What are the cases where the CTM is required? I will add them to the
filterfastbounds GM.

[Stephen White, 2014-12-08 18:55:57]

On 2014/12/08 17:32:46, robertphillips wrote:
> > This really doesn't seem right to me, and we could come up with cases where
> > it'll fail. I think we really need the CTM here to do this correctly.
> 
> What are the cases where the CTM is required? I will add them to the
> filterfastbounds GM.

(This is all off the top of my head, so may be wrong.)

Basically anything where the srcOffset in onFilterImage() is non-zero, since
computeFastBounds() has no way to know what the srcOffset is going to be. I
think something with a crop rect set on an upstream filter feeding into an
SkMatrixImageFilter should show the problem.

It may even be as simple as setting a clip rect to successive tiles and drawing
a primitive with an SkMatrixImageFilter.

The general problem is that the fTransform is applied relative to the offscreen
(saveLayer) origin, not the primitive bounds origin. It has to be this way,
since cropping the primitive (or drawing a different primitive with the same
filter) shouldn't move the origin of the transform. Since computeFastBounds()
doesn't know what that origin is, it tries to make a guess based on the upstream
offset, which is wrong. Your patch makes it ignore the offset altogether, which
I think is also wrong.

[robertphillips, 2014-12-08 19:26:09]

In the case of the saveLayer isn't the origin of the transform the UL corner of
the saveLayer? In the case of drawing a primitive isn't the origin of the
transform the origin of the local coordinate space in which it is being drawn?

I will try creating a test case with a crop rect.

[Stephen White, 2014-12-08 19:43:32]

On 2014/12/08 19:26:09, robertphillips wrote:
> In the case of the saveLayer isn't the origin of the transform the UL corner
of
> the saveLayer? In the case of drawing a primitive isn't the origin of the
> transform the origin of the local coordinate space in which it is being drawn?
> 
> I will try creating a test case with a crop rect.

Try your patch against GM:imagefiltersclipped.

[robertphillips, 2014-12-08 20:08:35]

All the images remain the same except for filterfastbounds (where the rotated
and scaled matriximagefilter results appear).

[Stephen White, 2014-12-08 20:46:00]

On 2014/12/08 20:08:35, robertphillips wrote:
> All the images remain the same except for filterfastbounds (where the rotated
> and scaled matriximagefilter results appear).

Sorry, my mistake. OK, let's proceed with this for now and see if we can come up

with a fix for 3194.

LGTM

[robertphillips, 2014-12-08 20:49:44]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2014-12-08 20:50:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/785893004/1

[commit-bot: I haz the power, 2014-12-09 13:28:29]

Committed patchset #1 (id:1) as
https://skia.googlesource.com/skia/+/8b7b9c2e804b54a8301363091f418cfeda719fe3