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

Source/core/platform/graphics/filters/FilterEffect.cpp

Index: Source/core/platform/graphics/filters/FilterEffect.cpp
diff --git a/Source/core/platform/graphics/filters/FilterEffect.cpp b/Source/core/platform/graphics/filters/FilterEffect.cpp
index 01e4b0a6278d5a66915688a0e6a3713d52a18670..aece491731aefd3038bae8ef622470ed1f8cde36 100644
--- a/Source/core/platform/graphics/filters/FilterEffect.cpp
+++ b/Source/core/platform/graphics/filters/FilterEffect.cpp
@@ -481,4 +481,21 @@ PassRefPtr<SkImageFilter> FilterEffect::createImageFilter(SkiaImageFilterBuilder
     return 0;
 }
 
+SkIRect FilterEffect::getCropRect(const FloatSize& cropOffset) const
+{
+    SkIRect rect = SkIRect::MakeLargest();

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

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?

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

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.

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

Sounds good, thanks for the explanation.

+    FloatRect boundaries = effectBoundaries();
+    FloatSize resolution = filter()->filterResolution();
+    boundaries.scale(resolution.width(), resolution.height());
+    if (hasX())
+        rect.fLeft = static_cast<int>(boundaries.x()) + cropOffset.width();
+    if (hasY())
+        rect.fTop = static_cast<int>(boundaries.y()) + cropOffset.height();
+    if (hasWidth())
+        rect.fRight = rect.fLeft + static_cast<int>(boundaries.width());
+    if (hasHeight())
+        rect.fBottom = rect.fTop + static_cast<int>(boundaries.height());

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

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]

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

+    return rect;
+}
+
 } // namespace WebCore