Include filter-generated content bounds in visual rects

Include filter-generated content bounds in visual rects

PaintLayer::mapRectForFilter was used to map from changed bounds in the
filtered content to changed bounds once the filter has been applied.
For example, an offset filter (FEOffset) would simply apply an offset
to the input bounds. PaintLayer::mapRectForFilter was used for
calculating the filtered element's visual rect in
LayoutBox::inflateVisualRectForReflectionAndFilter.

Some filters such as FETurbulence [1] generate filtered output that
can fill the entire filtered region (i.e., including object margins).
When these filters are used, the visual rect is independent of the
paint invalidation rect, so mapRectForFilter has been refactored as
"PaintLayer::mapRectToAffectedFilterRegion".

This patch uses FilterEffect::determineFilterPrimitiveSubregion to
calculate the full filter boundaries in cases like FETurbulence,
fixing the visual rect for cases like effect-reference-hidpi.html as
well as similar tests with FEImage.

[1] https://drafts.fxtf.org/filters/#elementdef-feturbulence

BUG=640264
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[pdr., 2016-08-25 06:23:55]

Description was changed from

==========
Include filter-generated content bounds in visual rects

PaintLayer::mapRectForFilter was used to map from changed bounds in the
filtered content to changed bounds once the filter has been applied.
For example, an offset filter (FEOffset) would simply apply an offset
to the input bounds. PaintLayer::mapRectForFilter was used for
calculating the filtered element's visual rect in
LayoutBox::inflateVisualRectForReflectionAndFilter.

Some filters such as FETurbulence [1] generate filtered output that
can fill the entire filtered region (i.e., including object margins).
When these filters are used, the visual rect is independent of the
paint invalidation rect, so mapRectForFilter has been refactored as
"PaintLayer::mapRectToAffectedFilterRegion".

This patch uses FilterEffect::determineFilterPrimitiveSubregion to
calculate the full filter boundaries in cases like FETurbulence,
fixing the visual rect for cases like effect-reference-hidpi.html as
well as similar tests with FEImage.

[1] https://drafts.fxtf.org/filters/#elementdef-feturbulence

BUG=598051
==========

to

==========
Include filter-generated content bounds in visual rects

PaintLayer::mapRectForFilter was used to map from changed bounds in the
filtered content to changed bounds once the filter has been applied.
For example, an offset filter (FEOffset) would simply apply an offset
to the input bounds. PaintLayer::mapRectForFilter was used for
calculating the filtered element's visual rect in
LayoutBox::inflateVisualRectForReflectionAndFilter.

Some filters such as FETurbulence [1] generate filtered output that
can fill the entire filtered region (i.e., including object margins).
When these filters are used, the visual rect is independent of the
paint invalidation rect, so mapRectForFilter has been refactored as
"PaintLayer::mapRectToAffectedFilterRegion".

This patch uses FilterEffect::determineFilterPrimitiveSubregion to
calculate the full filter boundaries in cases like FETurbulence,
fixing the visual rect for cases like effect-reference-hidpi.html as
well as similar tests with FEImage.

[1] https://drafts.fxtf.org/filters/#elementdef-feturbulence

BUG=598051
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2016-08-25 06:24:58]

pdr@chromium.org changed reviewers:
+ senorblanco@chromium.org, wkorman@chromium.org

[pdr., 2016-08-25 06:25:02]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 06:25:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pdr., 2016-08-25 06:26:20]

Description was changed from

==========
Include filter-generated content bounds in visual rects

PaintLayer::mapRectForFilter was used to map from changed bounds in the
filtered content to changed bounds once the filter has been applied.
For example, an offset filter (FEOffset) would simply apply an offset
to the input bounds. PaintLayer::mapRectForFilter was used for
calculating the filtered element's visual rect in
LayoutBox::inflateVisualRectForReflectionAndFilter.

Some filters such as FETurbulence [1] generate filtered output that
can fill the entire filtered region (i.e., including object margins).
When these filters are used, the visual rect is independent of the
paint invalidation rect, so mapRectForFilter has been refactored as
"PaintLayer::mapRectToAffectedFilterRegion".

This patch uses FilterEffect::determineFilterPrimitiveSubregion to
calculate the full filter boundaries in cases like FETurbulence,
fixing the visual rect for cases like effect-reference-hidpi.html as
well as similar tests with FEImage.

[1] https://drafts.fxtf.org/filters/#elementdef-feturbulence

BUG=598051
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Include filter-generated content bounds in visual rects

PaintLayer::mapRectForFilter was used to map from changed bounds in the
filtered content to changed bounds once the filter has been applied.
For example, an offset filter (FEOffset) would simply apply an offset
to the input bounds. PaintLayer::mapRectForFilter was used for
calculating the filtered element's visual rect in
LayoutBox::inflateVisualRectForReflectionAndFilter.

Some filters such as FETurbulence [1] generate filtered output that
can fill the entire filtered region (i.e., including object margins).
When these filters are used, the visual rect is independent of the
paint invalidation rect, so mapRectForFilter has been refactored as
"PaintLayer::mapRectToAffectedFilterRegion".

This patch uses FilterEffect::determineFilterPrimitiveSubregion to
calculate the full filter boundaries in cases like FETurbulence,
fixing the visual rect for cases like effect-reference-hidpi.html as
well as similar tests with FEImage.

[1] https://drafts.fxtf.org/filters/#elementdef-feturbulence

BUG=640264
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2016-08-25 09:16:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 09:16:49]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Stephen White, 2016-08-25 15:03:14]

https://codereview.chromium.org/2276283002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2276283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2758:
filteredRect.unite(lastEffect->determineFilterPrimitiveSubregion(MapRectForward));
I think this is papering over the problem. I think the real problem is that we
need to expose a flavour of FilterOperations::mapRect() which calls
FilterEffect::mapPaintRect().

See the comment here, which explicitly calls out tile and displacement map:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...

OTOH, it would be nice if we could unite FilterEffect::mapRect() and
FilterEffect::mapPaintRect(). The latter was added by someone other than me, and
we should figure out what the differences really are for the paint case and why
they don't apply to the other cases. That's not clear to me.

Also, jbroman@ should probably take a look at this.

[pdr., 2016-08-25 17:34:41]

pdr@chromium.org changed reviewers:
+ jbroman@chromium.org

[pdr., 2016-08-25 17:55:07]

On 2016/08/25 at 15:03:14, senorblanco wrote:
>
https://codereview.chromium.org/2276283002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):
> 
>
https://codereview.chromium.org/2276283002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/PaintLayer.cpp:2758:
filteredRect.unite(lastEffect->determineFilterPrimitiveSubregion(MapRectForward));
> I think this is papering over the problem. I think the real problem is that we
need to expose a flavour of FilterOperations::mapRect() which calls
FilterEffect::mapPaintRect().
> 
> See the comment here, which explicitly calls out tile and displacement map:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
> 
> OTOH, it would be nice if we could unite FilterEffect::mapRect() and
FilterEffect::mapPaintRect(). The latter was added by someone other than me, and
we should figure out what the differences really are for the paint case and why
they don't apply to the other cases. That's not clear to me.
> 
> Also, jbroman@ should probably take a look at this.

Thanks for the review. I think we're on the right track since all the tests now
pass with no over-invalidation, but I agree that this stuff is in serious need
of cleaning up. Let me see what I can do.

Side question you may know about that could help... The
effect-reference-hidpi.html test has an html image element that has an svg
turbulence filter applied to it. In [1], we correctly resolve the Filter's
bounds to the html element's bounds and set the filterRegion on the Filter. In
[2], we try to resolve the FilterEffect's bounds but this works slightly
differently since we use the svg root element for the viewport which is 0,0,
producing empty bounds for the FilterEffect. Due to these differences, Filter
has the correct max bounds whereas FilterEffects do not.
FilterEffect::determineFilterPrimitiveSubregion resolves these differences and
is how we get the correct filter bounds for the filter's SkPicture.

I think we will need to make the turbulence FilterEffect (FETurbulence) have
maxEffectBounds equal to the html image element in this case (i.e., like [1],
not [2]) in order for a modified FETurbulence::mapRect to work. Does that sound
like a reasonable avenue to go down?

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Fil...
FloatRect filterRegion =
SVGLengthContext::resolveRectangle<SVGFilterElement>(&filterElement,
filterElement.filterUnits()->currentValue()->enumValue(), referenceBox);
[2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/svg/graph...
effect->setEffectBoundaries(SVGLengthContext::resolveRectangle<SVGFilterPrimitiveStandardAttributes>(effectElement,
filterElement.primitiveUnits()->currentValue()->enumValue(), referenceBox));

+cc jbroman.

[jbroman, 2016-08-25 23:41:48]

On 2016/08/25 at 17:55:07, pdr wrote:
> On 2016/08/25 at 15:03:14, senorblanco wrote:
> >
https://codereview.chromium.org/2276283002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):
> > 
> >
https://codereview.chromium.org/2276283002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/paint/PaintLayer.cpp:2758:
filteredRect.unite(lastEffect->determineFilterPrimitiveSubregion(MapRectForward));
> > I think this is papering over the problem. I think the real problem is that
we need to expose a flavour of FilterOperations::mapRect() which calls
FilterEffect::mapPaintRect().
> > 
> > See the comment here, which explicitly calls out tile and displacement map:
> > 
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
> > 
> > OTOH, it would be nice if we could unite FilterEffect::mapRect() and
FilterEffect::mapPaintRect(). The latter was added by someone other than me, and
we should figure out what the differences really are for the paint case and why
they don't apply to the other cases. That's not clear to me.
> > 
> > Also, jbroman@ should probably take a look at this.
> 
> Thanks for the review. I think we're on the right track since all the tests
now pass with no over-invalidation, but I agree that this stuff is in serious
need of cleaning up. Let me see what I can do.
> 
> Side question you may know about that could help... The
effect-reference-hidpi.html test has an html image element that has an svg
turbulence filter applied to it. In [1], we correctly resolve the Filter's
bounds to the html element's bounds and set the filterRegion on the Filter. In
[2], we try to resolve the FilterEffect's bounds but this works slightly
differently since we use the svg root element for the viewport which is 0,0,
producing empty bounds for the FilterEffect. Due to these differences, Filter
has the correct max bounds whereas FilterEffects do not.
FilterEffect::determineFilterPrimitiveSubregion resolves these differences and
is how we get the correct filter bounds for the filter's SkPicture.
> 
> I think we will need to make the turbulence FilterEffect (FETurbulence) have
maxEffectBounds equal to the html image element in this case (i.e., like [1],
not [2]) in order for a modified FETurbulence::mapRect to work. Does that sound
like a reasonable avenue to go down?
> 
> [1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Fil...
> FloatRect filterRegion =
SVGLengthContext::resolveRectangle<SVGFilterElement>(&filterElement,
filterElement.filterUnits()->currentValue()->enumValue(), referenceBox);
> [2]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/svg/graph...
>
effect->setEffectBoundaries(SVGLengthContext::resolveRectangle<SVGFilterPrimitiveStandardAttributes>(effectElement,
filterElement.primitiveUnits()->currentValue()->enumValue(), referenceBox));
> 
> +cc jbroman.

I plead ignorance about what determineFilterPrimitiveSubregion does. I agree
that I don't know what mapPaintRect is for or what undesirable effects it
allegedly prevents. It makes sense to me that the filter region might need to be
an input to the calculation for things like <feTile>, but I don't know whether
this is the correct logic to do so.

[pdr., 2016-08-29 17:51:13]

https://codereview.chromium.org/2276283002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2276283002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2758:
filteredRect.unite(lastEffect->determineFilterPrimitiveSubregion(MapRectForward));
On 2016/08/25 at 15:03:14, Stephen White wrote:
> I think this is papering over the problem. I think the real problem is that we
need to expose a flavour of FilterOperations::mapRect() which calls
FilterEffect::mapPaintRect().
> 
> See the comment here, which explicitly calls out tile and displacement map:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
> 
> OTOH, it would be nice if we could unite FilterEffect::mapRect() and
FilterEffect::mapPaintRect(). The latter was added by someone other than me, and
we should figure out what the differences really are for the paint case and why
they don't apply to the other cases. That's not clear to me.
> 
> Also, jbroman@ should probably take a look at this.

I've filed https://crbug.com/642035 as a brain-dump of my findings in this
patch. We do need to combine mapRect and mapPaintRect and use the result in
PaintLayer::mapRectForFilter, but this is a large project.