Harmonize FilterEffect::mapRect and mapPaintRect

Harmonize FilterEffect::mapRect and mapPaintRect

This CL terminates the FilterEffect::determineAbsolutePaintRect codepath,
replacing the last user (SVGFilterPainter) with mapRect().
A new structure for FilterEffect::mapRect is introduced (taking some
hints from the SkImageFilter implementation), where mapRect is the entry-
point (replacing mapRectRecursive), which in turn calls: 1) mapInputs,
2) mapEffect and 3) applyBounds. The last simply applies any clip bounds
set on the effect, while also adjusting for affectsTransparentPixels().
mapInputs() computes the contribution from inputs to the current effect,
while mapEffect() applies the contribution from the current effect
itself. The notion of forward/reverse is not retained since only forward
mapping is used.

BUG=642035, 640264, 611674, 600430
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/fbeebf2905898067b4def80b945aacabe9d413e9
Cr-Commit-Position: refs/heads/master@{#420293}

[fs, 2016-09-14 21:23:34]

Description was changed from

==========
FilterEffect::mapRect harmonization

BUG=642035
==========

to

==========
FilterEffect::mapRect harmonization

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

[fs, 2016-09-14 21:24:02]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 21:24:46]

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

[commit-bot: I haz the power, 2016-09-14 22:17:00]

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

[commit-bot: I haz the power, 2016-09-14 22:17:01]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[fs, 2016-09-14 22:19:49]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 22:20:25]

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

[commit-bot: I haz the power, 2016-09-14 23:09:41]

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

[commit-bot: I haz the power, 2016-09-14 23:09:42]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[fs, 2016-09-15 19:04:51]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 19:05:39]

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

[commit-bot: I haz the power, 2016-09-15 20:16:26]

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

[commit-bot: I haz the power, 2016-09-15 20:16:27]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[fs, 2016-09-15 21:02:25]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 21:03:21]

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

[fs, 2016-09-15 21:18:30]

Description was changed from

==========
FilterEffect::mapRect harmonization

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

to

==========
Harmonize FilterEffect::mapRect and mapPaintRect

This CL terminates the FilterEffect::determineAbsolutePaintRect codepath,
replacing the last user (SVGFilterPainter) with mapRect().
A new structure for FilterEffect::mapRect is introduced (taking some
hints from the SkImageFilter implementation), where mapRect is the entry-
point (replacing mapRectRecursive), which in turn calls: 1) mapInputs,
2) mapEffect and 3) applyBounds. The last simply applies any clip bounds
set on the effect, while also adjusting for affectsTransparentPixels().
mapInputs() computes the contribution from inputs to the current effect,
while mapEffect() applies the contribution from the current effect
itself. The notion of forward/reverse is not retained since only forward
mapping is used.

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

[commit-bot: I haz the power, 2016-09-15 22:37:52]

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

[commit-bot: I haz the power, 2016-09-15 22:37:53]

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_...)

[fs, 2016-09-16 09:12:42]

Description was changed from

==========
Harmonize FilterEffect::mapRect and mapPaintRect

This CL terminates the FilterEffect::determineAbsolutePaintRect codepath,
replacing the last user (SVGFilterPainter) with mapRect().
A new structure for FilterEffect::mapRect is introduced (taking some
hints from the SkImageFilter implementation), where mapRect is the entry-
point (replacing mapRectRecursive), which in turn calls: 1) mapInputs,
2) mapEffect and 3) applyBounds. The last simply applies any clip bounds
set on the effect, while also adjusting for affectsTransparentPixels().
mapInputs() computes the contribution from inputs to the current effect,
while mapEffect() applies the contribution from the current effect
itself. The notion of forward/reverse is not retained since only forward
mapping is used.

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

to

==========
Harmonize FilterEffect::mapRect and mapPaintRect

This CL terminates the FilterEffect::determineAbsolutePaintRect codepath,
replacing the last user (SVGFilterPainter) with mapRect().
A new structure for FilterEffect::mapRect is introduced (taking some
hints from the SkImageFilter implementation), where mapRect is the entry-
point (replacing mapRectRecursive), which in turn calls: 1) mapInputs,
2) mapEffect and 3) applyBounds. The last simply applies any clip bounds
set on the effect, while also adjusting for affectsTransparentPixels().
mapInputs() computes the contribution from inputs to the current effect,
while mapEffect() applies the contribution from the current effect
itself. The notion of forward/reverse is not retained since only forward
mapping is used.

BUG=642035, 611674
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fs, 2016-09-16 09:17:36]

Description was changed from

==========
Harmonize FilterEffect::mapRect and mapPaintRect

This CL terminates the FilterEffect::determineAbsolutePaintRect codepath,
replacing the last user (SVGFilterPainter) with mapRect().
A new structure for FilterEffect::mapRect is introduced (taking some
hints from the SkImageFilter implementation), where mapRect is the entry-
point (replacing mapRectRecursive), which in turn calls: 1) mapInputs,
2) mapEffect and 3) applyBounds. The last simply applies any clip bounds
set on the effect, while also adjusting for affectsTransparentPixels().
mapInputs() computes the contribution from inputs to the current effect,
while mapEffect() applies the contribution from the current effect
itself. The notion of forward/reverse is not retained since only forward
mapping is used.

BUG=642035, 611674
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Harmonize FilterEffect::mapRect and mapPaintRect

This CL terminates the FilterEffect::determineAbsolutePaintRect codepath,
replacing the last user (SVGFilterPainter) with mapRect().
A new structure for FilterEffect::mapRect is introduced (taking some
hints from the SkImageFilter implementation), where mapRect is the entry-
point (replacing mapRectRecursive), which in turn calls: 1) mapInputs,
2) mapEffect and 3) applyBounds. The last simply applies any clip bounds
set on the effect, while also adjusting for affectsTransparentPixels().
mapInputs() computes the contribution from inputs to the current effect,
while mapEffect() applies the contribution from the current effect
itself. The notion of forward/reverse is not retained since only forward
mapping is used.

BUG=642035, 640264, 611674
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fs, 2016-09-16 09:20:21]

Description was changed from

==========
Harmonize FilterEffect::mapRect and mapPaintRect

This CL terminates the FilterEffect::determineAbsolutePaintRect codepath,
replacing the last user (SVGFilterPainter) with mapRect().
A new structure for FilterEffect::mapRect is introduced (taking some
hints from the SkImageFilter implementation), where mapRect is the entry-
point (replacing mapRectRecursive), which in turn calls: 1) mapInputs,
2) mapEffect and 3) applyBounds. The last simply applies any clip bounds
set on the effect, while also adjusting for affectsTransparentPixels().
mapInputs() computes the contribution from inputs to the current effect,
while mapEffect() applies the contribution from the current effect
itself. The notion of forward/reverse is not retained since only forward
mapping is used.

BUG=642035, 640264, 611674
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Harmonize FilterEffect::mapRect and mapPaintRect

This CL terminates the FilterEffect::determineAbsolutePaintRect codepath,
replacing the last user (SVGFilterPainter) with mapRect().
A new structure for FilterEffect::mapRect is introduced (taking some
hints from the SkImageFilter implementation), where mapRect is the entry-
point (replacing mapRectRecursive), which in turn calls: 1) mapInputs,
2) mapEffect and 3) applyBounds. The last simply applies any clip bounds
set on the effect, while also adjusting for affectsTransparentPixels().
mapInputs() computes the contribution from inputs to the current effect,
while mapEffect() applies the contribution from the current effect
itself. The notion of forward/reverse is not retained since only forward
mapping is used.

BUG=642035, 640264, 611674, 600430
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[fs, 2016-09-16 15:09:33]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 15:09:55]

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

[commit-bot: I haz the power, 2016-09-16 16:16:44]

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

[commit-bot: I haz the power, 2016-09-16 16:16:46]

Dry run: This issue passed the CQ dry run.

[fs, 2016-09-16 16:19:41]

fs@opera.com changed reviewers:
+ pdr@chromium.org, senorblanco@chromium.org

[pdr., 2016-09-16 18:29:18]

So harmonious! I like this at a high level, just some minor nits and questions
and requests for comments whose absence isn't entirely your fault.

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FEComposite.cpp
(right):

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FEComposite.cpp:118: return
k4() > 0;
Can you add a comment explaining this line?

There's this comment in mapInputs but it doesn't really explain why. The spec's
equation is "result = k1*i1*i2 + k2*i1 + k3*i2 + k4" and it's not clear how k4
different from k1-k3 in terms of affecting transparent pixels.

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FEComposite.cpp:132: //
Arithmetic with non-zero k4 may influnce the complete filter primitive
Nit: "influence"

If possible, can you add comments above each of these conditionals explaining
why?

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FELighting.cpp (left):

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FELighting.cpp:56: // The
areas affected need to be a pixel bigger to accommodate the Sobel kernel.
Don't we still need this inflation?

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FilterEffect.h (right):

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FilterEffect.h:111: virtual
FloatRect mapInputs(const FloatRect&) const;
Can you add comments above these like you did for mapRect above?

[fs, 2016-09-16 18:53:28]

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FEComposite.cpp
(right):

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FEComposite.cpp:118: return
k4() > 0;
On 2016/09/16 at 18:29:18, pdr. wrote:
> Can you add a comment explaining this line?

Sure.

> There's this comment in mapInputs but it doesn't really explain why. The
spec's equation is "result = k1*i1*i2 + k2*i1 + k3*i2 + k4" and it's not clear
how k4 different from k1-k3 in terms of affecting transparent pixels.

i1=i2=0, k1=k2=k3=x, k4>0 => result > 0 (not a transparent pixel)

More generally "(transparent <op> transparent) != transparent".

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FEComposite.cpp:132: //
Arithmetic with non-zero k4 may influnce the complete filter primitive
On 2016/09/16 at 18:29:18, pdr. wrote:
> Nit: "influence"
> 
> If possible, can you add comments above each of these conditionals explaining
why?

I'll see if can express this in a not too verbose way.

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FELighting.cpp (left):

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FELighting.cpp:56: // The
areas affected need to be a pixel bigger to accommodate the Sobel kernel.
On 2016/09/16 at 18:29:18, pdr. wrote:
> Don't we still need this inflation?

That would only be relevant for a reverse-mapping - i.e "to produce the correct
destination we need one extra border pixel", but for "forward" we'll only
produce the same amount of pixels.

[fs, 2016-09-16 20:16:21]

Comments added (and one small fix)

(I need to re-do and re-upload the new baselines unfortunately, because PS5 was
uploaded from my office machine, and binary files don't work with "git cl
patch"...)

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FEComposite.cpp
(right):

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FEComposite.cpp:118: return
k4() > 0;
On 2016/09/16 at 18:53:28, fs wrote:
> On 2016/09/16 at 18:29:18, pdr. wrote:
> > Can you add a comment explaining this line?
> 
> Sure.
> 
> > There's this comment in mapInputs but it doesn't really explain why. The
spec's equation is "result = k1*i1*i2 + k2*i1 + k3*i2 + k4" and it's not clear
how k4 different from k1-k3 in terms of affecting transparent pixels.
> 
> i1=i2=0, k1=k2=k3=x, k4>0 => result > 0 (not a transparent pixel)
> 
> More generally "(transparent <op> transparent) != transparent".

Noticed that I should probably have checked m_type here as well to not pessimize
unnecessarily, so thank for prodding! =)

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FEComposite.cpp:132: //
Arithmetic with non-zero k4 may influnce the complete filter primitive
On 2016/09/16 at 18:53:28, fs wrote:
> On 2016/09/16 at 18:29:18, pdr. wrote:
> > Nit: "influence"
> > 
> > If possible, can you add comments above each of these conditionals
explaining why?
> 
> I'll see if can express this in a not too verbose way.

Comments added.

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FilterEffect.h (right):

https://codereview.chromium.org/2341923002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FilterEffect.h:111: virtual
FloatRect mapInputs(const FloatRect&) const;
On 2016/09/16 at 18:29:18, pdr. wrote:
> Can you add comments above these like you did for mapRect above?

Added.

[pdr., 2016-09-16 20:45:25]

On 2016/09/16 at 20:16:21, fs wrote:
> Comments added (and one small fix)
> 
> (I need to re-do and re-upload the new baselines unfortunately, because PS5
was uploaded from my office machine, and binary files don't work with "git cl
patch"...)

Fellow traveler, I know your pain and present to you the gift of apply_issue.py!
The following will guide you towards your destination:
apply_issue --root_dir . --issue [issue#] --server
https://codereview.chromium.org --ignore_deps -v

Fore more:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/971ksZLKW3s/3ou_ZgIv...

[pdr., 2016-09-16 20:48:15]

LGTM here

Please give señorblanco a chance to review this as well.

[fs, 2016-09-16 21:00:34]

On 2016/09/16 at 20:45:25, pdr wrote:
> On 2016/09/16 at 20:16:21, fs wrote:
> > Comments added (and one small fix)
> > 
> > (I need to re-do and re-upload the new baselines unfortunately, because PS5
was uploaded from my office machine, and binary files don't work with "git cl
patch"...)
> 
> Fellow traveler, I know your pain and present to you the gift of
apply_issue.py! The following will guide you towards your destination:
> apply_issue --root_dir . --issue [issue#] --server
https://codereview.chromium.org --ignore_deps -v
> 
> Fore more:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/971ksZLKW3s/3ou_ZgIv...

@ pickup apply_issue.py

apply_issue.py was added to your inventory.

@ attack issue 2341923002 with apply_issue.py

You wield your mighty apply_issue.py, but it is heavy, and instead of getting
the patch you end up cutting off your toe. There can only be less blood.

@ use bandage on toe

No bandage in inventory.

@ look

You see several pairs of yellow glints. The are eyes. Of ogres. You are doomed.

THE END.

[fs, 2016-09-16 22:36:09]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 22:36:39]

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

[commit-bot: I haz the power, 2016-09-16 23:16:42]

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

[commit-bot: I haz the power, 2016-09-16 23:16:43]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[fs, 2016-09-20 11:15:34]

On 2016/09/16 at 20:48:15, pdr wrote:
> LGTM here
> 
> Please give señorblanco a chance to review this as well.

señor, would you like me to sit on this for a while longer?

[Stephen White, 2016-09-20 15:36:19]

Sorry for the delay.

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-gaussianblur-expected.txt
(right):

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-gaussianblur-expected.txt:13:
"rect": [38, 30, 240, 240],
OK, so thanks to your change, this is now more precise, since we're invalidating
only the filter region in this case (-10%, -10%, 120%, 120%). But the test is
less useful, since it doesn't really exercise what it was designed to, which is
FEGaussianBlur::mapRect() (now mapEffect()).

Could you modify the test so the <filter> primitive has something super-large,
like x=-50%, y=-50%, width=200%, height=200%? Then we should see the actual blur
radius show up here, approx. -60, -60, 320 x 320.

We should probably do the same for all filter-effect-repaint-* tests.

(This is probably my fault, since I implemented filter subregion correctly for
CSS filters, these tests are clipping to the filter region where they weren't
before, but were still using an unclipped paint invalidation region.)

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp (right):

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp:63:
context.clipRect(lastEffect->mapRect(object.strokeBoundingBox()));
Does mapRect() now take into effect filters which affect transparent black?
(This may become clear after I review more of this patch.)

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp:66:
context.beginLayer(1, SkXfermode::kSrcOver_Mode, &boundaries, ColorFilterNone,
std::move(imageFilter));
Not new to this patch, but Skia may ignore the boundaries passed to saveLayer()
with a filter which affects transparent black. So it looks like we may be
ignoring the filter region here. Perhaps we should be intersecting the bounds
computed by mapRect() above with the filter region, instead of passing it as
boundaries here, to ensure that it is properly clipped. I probably need to think
about this some more.

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FEBoxReflect.cpp
(right):

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FEBoxReflect.cpp:25: //
Reflection about any line is self-inverse, so this for both forward and
Nit: probably could remove this comment, since we don't have reverse mapping
anymore.

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FEComposite.cpp
(right):

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FEComposite.cpp:121: return
m_type == FECOMPOSITE_OPERATOR_ARITHMETIC && k4() > 0;
Could you add some repaint tests for the different cases (here and below)?

Something in the style of
paint/invalidation/filters/effect-reference-repaint-gaussianblur.html would be
great.

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FEDisplacementMap.cpp
(right):

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FEDisplacementMap.cpp:56:
FloatRect FEDisplacementMap::mapInputs(const FloatRect& rect) const
New repaint test for this node?

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FELighting.cpp (left):

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FELighting.cpp:57:
result.inflate(1);
pdr@ attempted to remove this before. I don't think we can remove it, since it
seems like invalidating the interior of a rect should invalidate the pixels
bordering that rect, due to the Sobel kernel. Either I'm wrong, or our testing
is insufficient here (probably needs something where the inner border of the
invalidation region contains some interesting pixels (ie., not solid colour)).

[fs, 2016-09-20 19:13:59]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[fs, 2016-09-20 19:14:15]

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-gaussianblur-expected.txt
(right):

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-gaussianblur-expected.txt:13:
"rect": [38, 30, 240, 240],
On 2016/09/20 at 15:36:19, Stephen White wrote:
> OK, so thanks to your change, this is now more precise, since we're
invalidating only the filter region in this case (-10%, -10%, 120%, 120%). But
the test is less useful, since it doesn't really exercise what it was designed
to, which is FEGaussianBlur::mapRect() (now mapEffect()).
> 
> Could you modify the test so the <filter> primitive has something super-large,
like x=-50%, y=-50%, width=200%, height=200%? Then we should see the actual blur
radius show up here, approx. -60, -60, 320 x 320.
> 
> We should probably do the same for all filter-effect-repaint-* tests.
> 
> (This is probably my fault, since I implemented filter subregion correctly for
CSS filters, these tests are clipping to the filter region where they weren't
before, but were still using an unclipped paint invalidation region.)

Changed per suggestion above.

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp (right):

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp:63:
context.clipRect(lastEffect->mapRect(object.strokeBoundingBox()));
On 2016/09/20 at 15:36:19, Stephen White wrote:
> Does mapRect() now take into effect filters which affect transparent black?
(This may become clear after I review more of this patch.)

Yes, it should.

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp:66:
context.beginLayer(1, SkXfermode::kSrcOver_Mode, &boundaries, ColorFilterNone,
std::move(imageFilter));
On 2016/09/20 at 15:36:19, Stephen White wrote:
> Not new to this patch, but Skia may ignore the boundaries passed to
saveLayer() with a filter which affects transparent black. So it looks like we
may be ignoring the filter region here. Perhaps we should be intersecting the
bounds computed by mapRect() above with the filter region, instead of passing it
as boundaries here, to ensure that it is properly clipped. I probably need to
think about this some more.

mapRect() should apply that, which should hopefully be enough. Eventually we
won't be able to specify this here like this, so I guess we should ponder what
can be passed instead.

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FEBoxReflect.cpp
(right):

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FEBoxReflect.cpp:25: //
Reflection about any line is self-inverse, so this for both forward and
On 2016/09/20 at 15:36:19, Stephen White wrote:
> Nit: probably could remove this comment, since we don't have reverse mapping
anymore.

Removed.

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FEComposite.cpp
(right):

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FEComposite.cpp:121: return
m_type == FECOMPOSITE_OPERATOR_ARITHMETIC && k4() > 0;
On 2016/09/20 at 15:36:19, Stephen White wrote:
> Could you add some repaint tests for the different cases (here and below)?
> 
> Something in the style of
paint/invalidation/filters/effect-reference-repaint-gaussianblur.html would be
great.

Added a few tests.

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FEDisplacementMap.cpp
(right):

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FEDisplacementMap.cpp:56:
FloatRect FEDisplacementMap::mapInputs(const FloatRect& rect) const
On 2016/09/20 at 15:36:19, Stephen White wrote:
> New repaint test for this node?

Added a (one) test.

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FELighting.cpp (left):

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FELighting.cpp:57:
result.inflate(1);
On 2016/09/20 at 15:36:19, Stephen White wrote:
> pdr@ attempted to remove this before. I don't think we can remove it, since it
seems like invalidating the interior of a rect should invalidate the pixels
bordering that rect, due to the Sobel kernel. Either I'm wrong, or our testing
is insufficient here (probably needs something where the inner border of the
invalidation region contains some interesting pixels (ie., not solid colour)).

And he didn't leave a trail of regression tests after (trying) that? =)

Not particularly easy to dream up a reasonable test for this, but added
something. I don't carry high hopes that one will ever catch anything though
(given how the lighting filters will essentially be returning the full region.)
It's there for discussion and tweaks (and "somethingness") though.

[commit-bot: I haz the power, 2016-09-20 19:14:19]

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

[Stephen White, 2016-09-20 19:46:39]

Thanks for taking this on, BTW. This is good stuff.

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp (right):

https://codereview.chromium.org/2341923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/SVGFilterPainter.cpp:66:
context.beginLayer(1, SkXfermode::kSrcOver_Mode, &boundaries, ColorFilterNone,
std::move(imageFilter));
On 2016/09/20 19:14:14, fs wrote:
> On 2016/09/20 at 15:36:19, Stephen White wrote:
> > Not new to this patch, but Skia may ignore the boundaries passed to
> saveLayer() with a filter which affects transparent black. So it looks like we
> may be ignoring the filter region here. Perhaps we should be intersecting the
> bounds computed by mapRect() above with the filter region, instead of passing
it
> as boundaries here, to ensure that it is properly clipped. I probably need to
> think about this some more.
> 
> mapRect() should apply that, which should hopefully be enough. Eventually we
> won't be able to specify this here like this, so I guess we should ponder what
> can be passed instead.

Eventually, we can probably pass nullptr, and just use the clip. Or even get rid
of saveLayer() altogether, and just use drawPaint() at the Skia level.

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1.html
(right):

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1.html:1:
<!DOCTYPE html>
Thanks for adding these. Shouldn't they also have .txt results, though?

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-gaussianblur-expected.txt
(right):

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-gaussianblur-expected.txt:13:
"rect": [38, 30, 240, 240],
This still looks suspiciously small to me. Shouldn't it be 320x320-ish?

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-lighting.html
(right):

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-lighting.html:20:
filter: url(#lighting);
I think we'll need to figure out how to get something other than a solid (and
possibly transparent) primitive here. Maybe gradients or something could be used
to get some non-uniform normals?

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FELighting.cpp (left):

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FELighting.cpp:53: FloatRect
FELighting::mapPaintRect(const FloatRect& rect, bool) const
Given that you can still remove this code, I think the test isn't exercising it
correctly.

[commit-bot: I haz the power, 2016-09-20 19:57:53]

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

[commit-bot: I haz the power, 2016-09-20 19:57:54]

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

[fs, 2016-09-20 21:39:05]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 21:39:48]

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

[fs, 2016-09-20 21:50:26]

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1.html
(right):

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-composite-1.html:1:
<!DOCTYPE html>
On 2016/09/20 at 19:46:38, Stephen White wrote:
> Thanks for adding these. Shouldn't they also have .txt results, though?

(See below, should be there now.)

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-gaussianblur-expected.txt
(right):

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-gaussianblur-expected.txt:13:
"rect": [38, 30, 240, 240],
On 2016/09/20 at 19:46:38, Stephen White wrote:
> This still looks suspiciously small to me. Shouldn't it be 320x320-ish?

I thought would be able to pick up the new result from the bots, but that didn't
work out. Should be back to the original (314x314) now though.

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-lighting.html
(right):

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/paint/invalidation/filters/effect-reference-repaint-lighting.html:20:
filter: url(#lighting);
On 2016/09/20 at 19:46:38, Stephen White wrote:
> I think we'll need to figure out how to get something other than a solid (and
possibly transparent) primitive here. Maybe gradients or something could be used
to get some non-uniform normals?

I don't think that's the major problem - that would rather be to get a partial
repaint. I changed to use a gradient and then "clip" that with the
SourceGraphic.

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/filters/FELighting.cpp (left):

https://codereview.chromium.org/2341923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/filters/FELighting.cpp:53: FloatRect
FELighting::mapPaintRect(const FloatRect& rect, bool) const
On 2016/09/20 at 19:46:39, Stephen White wrote:
> Given that you can still remove this code, I think the test isn't exercising
it correctly.

Innocent until otherwise proven? =)

What I think makes this not have any impact is that
affectsTransparentPixels=true (and that we're no longer computing the same thing
as this used to; to produce R we need R.inflate(1), but regardless we'll only
produce output in R.)

[commit-bot: I haz the power, 2016-09-20 22:52:29]

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

[commit-bot: I haz the power, 2016-09-20 22:52:30]

Dry run: This issue passed the CQ dry run.

[Stephen White, 2016-09-21 18:01:12]

LGTM

Awesome! My heartfelt appreciation for this work!

[fs, 2016-09-21 18:27:11]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 18:27:37]

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

[commit-bot: I haz the power, 2016-09-21 18:34:17]

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

[commit-bot: I haz the power, 2016-09-21 18:34:19]

Dry run: This issue passed the CQ dry run.

[fs, 2016-09-21 20:40:40]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 20:41:09]

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

[commit-bot: I haz the power, 2016-09-21 22:35:06]

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

[commit-bot: I haz the power, 2016-09-21 22:35:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[fs, 2016-09-22 09:03:13]

The CQ bit was checked by fs@opera.com

[fs, 2016-09-22 09:03:14]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
senorblanco@chromium.org
Link to the patchset: https://codereview.chromium.org/2341923002/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-22 09:03:26]

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

[commit-bot: I haz the power, 2016-09-22 09:34:41]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-22 09:38:45]

Description was changed from

==========
Harmonize FilterEffect::mapRect and mapPaintRect

This CL terminates the FilterEffect::determineAbsolutePaintRect codepath,
replacing the last user (SVGFilterPainter) with mapRect().
A new structure for FilterEffect::mapRect is introduced (taking some
hints from the SkImageFilter implementation), where mapRect is the entry-
point (replacing mapRectRecursive), which in turn calls: 1) mapInputs,
2) mapEffect and 3) applyBounds. The last simply applies any clip bounds
set on the effect, while also adjusting for affectsTransparentPixels().
mapInputs() computes the contribution from inputs to the current effect,
while mapEffect() applies the contribution from the current effect
itself. The notion of forward/reverse is not retained since only forward
mapping is used.

BUG=642035, 640264, 611674, 600430
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Harmonize FilterEffect::mapRect and mapPaintRect

This CL terminates the FilterEffect::determineAbsolutePaintRect codepath,
replacing the last user (SVGFilterPainter) with mapRect().
A new structure for FilterEffect::mapRect is introduced (taking some
hints from the SkImageFilter implementation), where mapRect is the entry-
point (replacing mapRectRecursive), which in turn calls: 1) mapInputs,
2) mapEffect and 3) applyBounds. The last simply applies any clip bounds
set on the effect, while also adjusting for affectsTransparentPixels().
mapInputs() computes the contribution from inputs to the current effect,
while mapEffect() applies the contribution from the current effect
itself. The notion of forward/reverse is not retained since only forward
mapping is used.

BUG=642035, 640264, 611674, 600430
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/fbeebf2905898067b4def80b945aacabe9d413e9
Cr-Commit-Position: refs/heads/master@{#420293}
==========

[commit-bot: I haz the power, 2016-09-22 09:38:49]

Patchset 10 (id:??) landed as
https://crrev.com/fbeebf2905898067b4def80b945aacabe9d413e9
Cr-Commit-Position: refs/heads/master@{#420293}