Make SkImageFilter crop rects relative to the primitive origin, instead of relative to their parent

Make SkImageFilter crop rects relative to the primitive origin, instead of relative to their parent's crop rect.  This is required by SVG semantics, and is more sane anyway.

To do this, this patch changes the "offset/loc" parameter in filterImage() / onFilterImage() from an inout-param to an out-param only, so that the calling filter can know how much the input filter wants its result offset (and doesn't include the original primitive position). This offset can then be applied to the current filter's crop rect. (I've renamed the parameter "offset" in all cases to make this clear.) This makes the call sites in SkCanvas/SkGpuDevice responsible for applying the resulting offset to the primitive's position, which is actually a fairly small change.

This change also fixes SkTileImageFilter and SkOffsetImageFilter to correctly handle an input offset, which they weren't before. This required modifying the GM's, since they assumed the broken behaviour.

NOTE: this will require rebaselining the imagefiltersgraph test, since it has a new test case.

NOTE: this will "break" the Blink layout tests css3/filters/effect-reference-subregion-chained-hw.html and css3/filters/effect-reference-subregion-hw.html, but it actually makes them give correct results. It should be suppressed on the skia roll, and I'll rebaseline it.

R=reed@google.com

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

[Stephen White, 2014-01-03 16:26:04]

reed@: PTAL. Thanks!

[reed1, 2014-01-03 16:42:07]

perhaps we could add some dox to onFilterImage now, esp. to clarify this
parameter.

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

https://codereview.chromium.org/112803004/diff/260001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:1001: SkIPoint offset = SkIPoint::Make(0, 0);
Why is this initialized if it is purely an out-parameter now?

https://codereview.chromium.org/112803004/diff/260001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/112803004/diff/260001/src/gpu/SkGpuDevice.cpp...
src/gpu/SkGpuDevice.cpp:1501: SkIPoint offset = SkIPoint::Make(0, 0);
same question about initializing it when that isn't necessary

[Stephen White, 2014-01-03 16:48:35]

Thanks for your review; will modify the docs.

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

https://codereview.chromium.org/112803004/diff/260001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:1001: SkIPoint offset = SkIPoint::Make(0, 0);
On 2014/01/03 16:42:07, reed1 wrote:
> Why is this initialized if it is purely an out-parameter now?

Before this patch, some of the callees were leaving it unchanged (i.e.,
uninitialized), and I was nervous that I might have missed some cases, so I left
this in as a just-in-case.

If you like, we can remove it, and rely on valgrind to let me know if I screwed
up.

[Stephen White, 2014-01-03 17:15:04]

New patch up: fixed docs; tried to find all the places where a filter returns
"true" and initialize the offset.

[reed1, 2014-01-03 17:47:11]

lgtm

[Stephen White, 2014-01-03 19:26:40]

I've added a new unit test to exercise all the filters using a cropped input,
and I'm glad I did. It exposed a bug: the bounds passed to the pixel loops have
to be offset back by the srcOffset, since they refer to pixels in the source
bitmap.

reed@: PTAL. Thanks!

[reed1, 2014-01-03 20:05:27]

it still feels weird to me, that you always init an output-only param. I wonder
if that will allow some subclasses to rely on this accidentally, so they can
ignore that parameter, and suddenly you have changed back to an input/output
param.

A different idea might be to, in debug mode, init to 0x80000000, and then assert
that the values are not that if the filter returned true.

anyway, lgtm

[Stephen White, 2014-01-03 21:48:29]

Committed patchset #13 manually as r12895 (presubmit successful).