Implement filter primitive subregion for reference CSS filters. This required refactoring determin…

Implement filter primitive subregion for reference CSS filters.  This required copying determineFilterPrimitiveSubregion() from RenderSVGResourceFilter out into FilterEffect, and modifying it slightly (to accommodate CSS's inflatable result rects; see below). This also required refactoring m_filterRegion and m_absoluteFilterRegion into the Filter base class, and giving its accessors sane names.

There was also a call to setClipsToBounds(false) for the FilterOperations nodes converted to FilterEffects.  However, this is not correct for reference filters -- they should clip to their filter primitive subregion.  (In fact, it was only being called on the final node in a reference effect chain, just to be super-confusing.)  This was fixed by calling setEffectBoundaries() while building the reference filter, and skipping the call to setClipsToBounds(false).

The filter outsets for reference filters are now computed by recursively traversing the DAG, so the graphics layers will be created at the appropriately-inflated size, as is done for shorthand filters. The actual per-FilterEffect-subclass computation (mapRect) was refactored from the existing code to compute absolutePaintRects, and made bidirectional. It's used for computing outsets in the forward direction, and source rects in the reverse direction.

We now no longer call setMaxEffectsRects() directly, but instead call determineFilterPrimitiveSubregion() recursively on the output node. This will set the maxEffectRect as well as the absolutePaintRect, inflating and clipping as necessary. This uses the filter primitive subregion (if set, as in the reference filter case), or the source rect (if not).

Note that this patch only supports filter primitive subregion, not filter effect region, which still defaults to 100% of the filterBoxRect.

Finally, I noticed that the colorspace for reference filters was not being correctly set either (same place as the setClipsToBounds() call).  That will require a bunch of rebaselines, however, so it'll be fixed in a separate patch.

BUG=237518
R=schenney@chromium.org

First committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150342
Reverted:  http://src.chromium.org/viewvc/blink?view=rev&rev=150369

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

[Stephen White, 2013-05-02 16:24:05]

schenney@: PTAL.  Thanks!

[Stephen Chennney, 2013-05-02 16:53:55]

I mostly wondering about testing, plus our desire to avoid all static_cast for
security reasons.

The basic changes look good to me. I do rely very heavily on the tests to catch
problems in this area, given the confusing nature of the spec and code.

https://codereview.chromium.org/14652016/diff/1/LayoutTests/css3/filters/effe...
File LayoutTests/css3/filters/effect-reference-subregion.html (right):

https://codereview.chromium.org/14652016/diff/1/LayoutTests/css3/filters/effe...
LayoutTests/css3/filters/effect-reference-subregion.html:8: <img
style="-webkit-filter: url(#f1); filter: url(#f1);"
src="resources/reference.png"></img>
There are also rules for how filterEffectRegion is 
"inherited" when applied to some filter effects but not others in the same
filter, and also how it interacts with the filterRegion on the top level
<filter> element. I wouldl recommend at least one test checking that the regions
are correct in such cases. I believe we have SVG tests for this, but I don't
know if they are sufficient to cover the CSS code paths.

https://codereview.chromium.org/14652016/diff/1/Source/core/rendering/FilterE...
File Source/core/rendering/FilterEffectRenderer.cpp (right):

https://codereview.chromium.org/14652016/diff/1/Source/core/rendering/FilterE...
Source/core/rendering/FilterEffectRenderer.cpp:150: SVGFilterElement*
filterElement = static_cast<SVGFilterElement*>(filter);
To we have a "toSVGFilterElement" method to avoid the static_cast?

[Stephen White, 2013-05-02 18:00:20]

On 2013/05/02 16:53:55, Stephen Chenney wrote:
> I mostly wondering about testing, plus our desire to avoid all static_cast for
> security reasons.
> 
> The basic changes look good to me. I do rely very heavily on the tests to
catch
> problems in this area, given the confusing nature of the spec and code.
> 
>
https://codereview.chromium.org/14652016/diff/1/LayoutTests/css3/filters/effe...
> File LayoutTests/css3/filters/effect-reference-subregion.html (right):
> 
>
https://codereview.chromium.org/14652016/diff/1/LayoutTests/css3/filters/effe...
> LayoutTests/css3/filters/effect-reference-subregion.html:8: <img
> style="-webkit-filter: url(#f1); filter: url(#f1);"
> src="resources/reference.png"></img>
> There are also rules for how filterEffectRegion is 
> "inherited" when applied to some filter effects but not others in the same
> filter, and also how it interacts with the filterRegion on the top level
> <filter> element. I wouldl recommend at least one test checking that the
regions
> are correct in such cases. I believe we have SVG tests for this, but I don't
> know if they are sufficient to cover the CSS code paths.

Will do.

https://codereview.chromium.org/14652016/diff/1/Source/core/rendering/FilterE...
> File Source/core/rendering/FilterEffectRenderer.cpp (right):
> 
>
https://codereview.chromium.org/14652016/diff/1/Source/core/rendering/FilterE...
> Source/core/rendering/FilterEffectRenderer.cpp:150: SVGFilterElement*
> filterElement = static_cast<SVGFilterElement*>(filter);
> To we have a "toSVGFilterElement" method to avoid the static_cast?

Sadly, no (I looked).  I'll add one.

[Stephen White, 2013-05-10 20:41:54]

PTAL. I've reworked the code significantly, and added more tests. I think it
should be easier to reason about correctness now.

[krit, 2013-05-10 20:50:58]

On 2013/05/10 20:41:54, Stephen White wrote:
> PTAL. I've reworked the code significantly, and added more tests. I think it
> should be easier to reason about correctness now.

I'll take a look on Monday. Won't work today for me :(

[Stephen White, 2013-05-13 18:12:03]

On 2013/05/10 20:50:58, krit wrote:
> I'll take a look on Monday. Won't work today for me :(

No problem, Dirk. I appreciate your expertise whenever it's available.

[Stephen White, 2013-05-13 21:54:26]

Stephen, would you mind taking another look?

[Stephen Chennney, 2013-05-14 17:07:13]

Good tests, good refactor.

Please add OVERRIDE FINAL on all the mapRect() implementations and then I think
we're good to go.

https://codereview.chromium.org/14652016/diff/96001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FEDropShadow.h (right):

https://codereview.chromium.org/14652016/diff/96001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FEDropShadow.h:55: virtual FloatRect
mapRect(const FloatRect&, bool forward = true);
OVERRIDE FINAL

All of the implementations of mapRect in the filter primitives should be marked
this way.

[Stephen White, 2013-05-14 17:41:00]

Thanks for your review.

This also fixes the Debug build, which I broke a few patches back and hadn't
checked.

https://codereview.chromium.org/14652016/diff/96001/Source/core/platform/grap...
File Source/core/platform/graphics/filters/FEDropShadow.h (right):

https://codereview.chromium.org/14652016/diff/96001/Source/core/platform/grap...
Source/core/platform/graphics/filters/FEDropShadow.h:55: virtual FloatRect
mapRect(const FloatRect&, bool forward = true);
On 2013/05/14 17:07:13, Stephen Chenney wrote:
> OVERRIDE FINAL
> 
> All of the implementations of mapRect in the filter primitives should be
marked
> this way.

Done.

[Stephen Chennney, 2013-05-14 18:02:28]

lgtm

[commit-bot: I haz the power, 2013-05-14 18:31:50]

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

[Stephen White, 2013-05-14 20:07:44]

Committed patchset #19 manually as r150342 (presubmit successful).

[eseidel, 2013-05-14 21:46:40]

This may have caused 2 crashes:
css3/filters/effect-reference-ordering-hw.html
css3/filters/effect-reference-composite.html

[Stephen White, 2013-05-15 01:22:28]

On 2013/05/14 21:46:40, eseidel wrote:
> This may have caused 2 crashes:
> css3/filters/effect-reference-ordering-hw.html
> css3/filters/effect-reference-composite.html

Good thing these were caught -- this is a pretty serious bug.

The problem is that ReferenceFilterOperation takes a ref on the last effect in
the FilterEffect DAG (and transitively, the whole DAG), but each of the filters
in the DAG have a bare backpointer to their Filter (in this case,
FilterEffectRenderer). However, ReferenceFilterOperation can outlive the
FilterEffectRenderer. This wasn't a problem before, since
ReferenceFilterOperation never did anything unless it had a
FilterEffectRenderer. Now it computes the outsets, which refer back to the
owning Filter. Fixed by taking a ref on the owning Filter as well.

Note that this is just an interim measure, until we create separate Filters for
each ReferenceFilterOperation, as in https://codereview.chromium.org/14793008/,
but that patch still needs a bit of work.

[Stephen Chennney, 2013-05-15 13:50:56]

LGTM.

My primary concern is that we'll create circular references, and it seems there
are no loops here.

[commit-bot: I haz the power, 2013-05-15 13:55:43]

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

[commit-bot: I haz the power, 2013-05-15 14:55:25]

Change committed as 150433