Implement filter primitive subregion on accelerated reference filters (FEColorMatrix a…

Implement filter primitive subregion on accelerated reference filters for FEColorMatrix, FEGaussianBlur and FE*Lighting. This patch also adds crop tests for all the filters, both software and hardware versions. The software results are correct; bringing the remainder of the hardware results into alignment with software results will be the work of future patches (Skia and Blink-side).

The patch adds a crop offset to the SkiaImageFilterBuilder, which is used to communicate the offset computed by the Blink filter chain when inflating its results for filter bounds. This code may be removed in the future once Skia takes on this responsibility and knows how much each filter inflates its result.

BUG=229910
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157529

[Stephen White, 2013-09-09 18:16:35]

Florin: PTAL. Thanks!

[Stephen White, 2013-09-09 18:19:39]

https://codereview.chromium.org/22794007/diff/27001/LayoutTests/css3/filters/...
File LayoutTests/css3/filters/effect-reference-subregion.html (left):

https://codereview.chromium.org/22794007/diff/27001/LayoutTests/css3/filters/...
LayoutTests/css3/filters/effect-reference-subregion.html:3: <filter id="f1"
color-interpolation-filters="sRGB">
Note that the old version of this file has essentially be renamed to
effect-reference-subregion-colormatrix.html, and its contents have been replaced
by a new test which tests all filters when cropped.

[f(malita), 2013-09-09 21:37:43]

https://codereview.chromium.org/22794007/diff/27001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FilterEffect.cpp (right):

https://codereview.chromium.org/22794007/diff/27001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FilterEffect.cpp:486: SkIRect rect =
SkIRect::MakeLargest();
Hmm, starting off with SkIRect::MakeLargest() doesn't seem right: IIRC the
default SVG filter region is something like (-10%, -10%) - (110%, 110%) relative
to the target bounding box (or is it 0%,0% - 100%,100% for filter effect
subregions?). Shouldn't we default to that in the absence of (sub)region
attributes?

https://codereview.chromium.org/22794007/diff/27001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FilterEffect.cpp:497: rect.fBottom =
rect.fTop + static_cast<int>(boundaries.height());
Selectively overwriting edges like this seems problematic: given a MakeLargest()
default, I think this will yield incorrect results when only certain attributes
are present.

For example, if one only specifies width="100", it seems we'd end up with a rect
of {left:SK_MinS32, top:SK_MinS32, right:SK_MinS32 + 100, bottom:SK_MaxS32}, no?

[Stephen White, 2013-09-10 14:43:41]

Fixed computation of default filter primitive subregion.

https://codereview.chromium.org/22794007/diff/27001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FilterEffect.cpp (right):

https://codereview.chromium.org/22794007/diff/27001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FilterEffect.cpp:486: SkIRect rect =
SkIRect::MakeLargest();
On 2013/09/09 21:37:43, Florin Malita wrote:
> Hmm, starting off with SkIRect::MakeLargest() doesn't seem right: IIRC the
> default SVG filter region is something like (-10%, -10%) - (110%, 110%)
relative
> to the target bounding box (or is it 0%,0% - 100%,100% for filter effect
> subregions?). Shouldn't we default to that in the absence of (sub)region
> attributes?

When omitted, filter primitive subregion is supposed to fall back to 100% of the
filter region. The filter region does default to -10%, -10%, 120%, 120% in SVG
as you describe, but filter region is currently unimplemented for reference
filters (see crbug.com/240827 and crbug.com/240845).

However, this 120% behaviour is frankly idiotic, when it's easy to compute the
actual required filter bounds. Dirk Schulze has promised to change the spec for
FilterEffects, since there's still time to fix it. So I'm doing my part to nudge
things in the direction of sanity by implementing auto-sizing. This is already
done for the raster path.

My plan is to implement the filter region fallback in a future patch, and then
have SVG have the legacy 120% behaviour by always setting it directly to those
values when unspecified, and have CSS filter effects use auto-sizing when
unspecified.

https://codereview.chromium.org/22794007/diff/27001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FilterEffect.cpp:497: rect.fBottom =
rect.fTop + static_cast<int>(boundaries.height());
On 2013/09/09 21:37:43, Florin Malita wrote:
> Selectively overwriting edges like this seems problematic: given a
MakeLargest()
> default, I think this will yield incorrect results when only certain
attributes
> are present.
> 
> For example, if one only specifies width="100", it seems we'd end up with a
rect
> of {left:SK_MinS32, top:SK_MinS32, right:SK_MinS32 + 100, bottom:SK_MaxS32},
no?

You're right. (I knew there was a reason I didn't submit this patch before going
off on vacation..)

[f(malita), 2013-09-10 15:37:10]

lgtm

https://codereview.chromium.org/22794007/diff/27001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FilterEffect.cpp (right):

https://codereview.chromium.org/22794007/diff/27001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FilterEffect.cpp:486: SkIRect rect =
SkIRect::MakeLargest();
On 2013/09/10 14:43:41, Stephen White wrote:
> On 2013/09/09 21:37:43, Florin Malita wrote:
> > Hmm, starting off with SkIRect::MakeLargest() doesn't seem right: IIRC the
> > default SVG filter region is something like (-10%, -10%) - (110%, 110%)
> relative
> > to the target bounding box (or is it 0%,0% - 100%,100% for filter effect
> > subregions?). Shouldn't we default to that in the absence of (sub)region
> > attributes?
> 
> When omitted, filter primitive subregion is supposed to fall back to 100% of
the
> filter region. The filter region does default to -10%, -10%, 120%, 120% in SVG
> as you describe, but filter region is currently unimplemented for reference
> filters (see crbug.com/240827 and crbug.com/240845).
> 
> However, this 120% behaviour is frankly idiotic, when it's easy to compute the
> actual required filter bounds. Dirk Schulze has promised to change the spec
for
> FilterEffects, since there's still time to fix it. So I'm doing my part to
nudge
> things in the direction of sanity by implementing auto-sizing. This is already
> done for the raster path.
> 
> My plan is to implement the filter region fallback in a future patch, and then
> have SVG have the legacy 120% behaviour by always setting it directly to those
> values when unspecified, and have CSS filter effects use auto-sizing when
> unspecified.

Sounds good, thanks for the explanation.

[commit-bot: I haz the power, 2013-09-10 15:38:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/senorblanco@chromium.org/22794007/45001

[commit-bot: I haz the power, 2013-09-10 17:14:28]

Change committed as 157529