Make sure to always reset the cached filter in ReferenceFilterOperation

Make sure to always reset the cached filter in ReferenceFilterOperation

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

Committed: https://crrev.com/3353a7a6003262aae1515ac9541a519ec23c8c1c
Cr-Commit-Position: refs/heads/master@{#428678}

[fs, 2016-10-27 16:53:31]

Description was changed from

==========
Make sure to always reset the cached filter in ReferenceFilterOperation

BUG=658305
==========

to

==========
Make sure to always reset the cached filter in ReferenceFilterOperation

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

[fs, 2016-10-27 16:53:37]

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

[commit-bot: I haz the power, 2016-10-27 16:54:05]

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

[fs, 2016-10-27 16:59:16]

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

[fs, 2016-10-27 16:59:16]

Not sure if this actually helps with the bug in question, but it seemed to have
an effect on the test I was looking at (effect-reference-delete) - on my
workbranch I should say. It doesn't appear to be a bad thing in general though,
or?

[Stephen White, 2016-10-27 17:10:51]

Seems mostly harmless, except for the lastEffect() (see comment).

It would be nice if we could generate a problematic test case, perhaps changing
from a non-empty to an empty filter region via JS? Or something else that causes
a null return from buildReferenceFilter(), such as a reference to a missing
element?

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

https://codereview.chromium.org/2453033004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/FilterEffectBuilder.cpp:154: effect =
referenceFilter->lastEffect();
I'm not sure I understand why we now grab the last effect here, from the
first-generated reference filter, when the one we're going to use in the
operation is the one we generate below. (Then again, I don't totally understand
why we generate it twice, either.)

[commit-bot: I haz the power, 2016-10-27 18:57:38]

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

[commit-bot: I haz the power, 2016-10-27 18:57:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[fs, 2016-10-28 08:46:31]

On 2016/10/27 at 17:10:51, senorblanco wrote:
> Seems mostly harmless, except for the lastEffect() (see comment).
> 
> It would be nice if we could generate a problematic test case, perhaps
changing from a non-empty to an empty filter region via JS? Or something else
that causes a null return from buildReferenceFilter(), such as a reference to a
missing element?

effect-reference-delete does the latter =). The former actually won't trigger
for the PaintLayer code-path (because of the nodeMap-part of the check.) There
remains one additional way to trigger it I think, which would be certain
primitives (feFlood, fe*Lighting) in an external document.

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

https://codereview.chromium.org/2453033004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/FilterEffectBuilder.cpp:154: effect =
referenceFilter->lastEffect();
On 2016/10/27 at 17:10:51, Stephen White wrote:
> I'm not sure I understand why we now grab the last effect here, from the
first-generated reference filter, when the one we're going to use in the
operation is the one we generate below. (Then again, I don't totally understand
why we generate it twice, either.)

That's what we did previously as well (I only reordered the to lines - modulo
the comment), only the first call to buildReferenceFilter takes the previous
effect as an argument, while the second takes null. The second call is there to
please PaintLayer::mapRectForFilter.

[Stephen White, 2016-10-28 14:58:06]

LGTM

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

https://codereview.chromium.org/2453033004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/FilterEffectBuilder.cpp:154: effect =
referenceFilter->lastEffect();
On 2016/10/28 08:46:31, fs wrote:

> That's what we did previously as well (I only reordered the to lines - modulo
> the comment), only the first call to buildReferenceFilter takes the previous
> effect as an argument, while the second takes null.

Oh right; I misread.

> The second call is there to
> please PaintLayer::mapRectForFilter.

I understand what, I'm just not clear on why. Different SourceGraphic 
so that mapRectForFilter doesn't traverse into the non-reference-filter 
inputs? If that's the case, another option here might be to defer 
connecting reference filters to the rest of the filter chain until 
we convert it to SkImageFilters. IIRC, that's what the accelerated 
path does, using SkComposeImageFilter to hook them up. Then we'd only 
have to build the reference filter once. The downside is that there 
are some optimizations that Skia can't apply across an 
SkComposeImageFilter, such as collapsing matrices. But I don't 
imagine that's a very common case (reference filter plus shorthand 
filters). Anyway all of that is unrelated to this patch.

[Stephen White, 2016-10-28 15:01:19]

On 2016/10/28 08:46:31, fs wrote:

> effect-reference-delete does the latter =). The former actually won't trigger
> for the PaintLayer code-path (because of the nodeMap-part of the check.) There
> remains one additional way to trigger it I think, which would be certain
> primitives (feFlood, fe*Lighting) in an external document.

So why does effect-reference-delete not fail? Due to flake?

Could we perhaps rewrite it with displayAsyncThen() instead of onload() to
de-flake it?

[fs, 2016-10-31 09:19:12]

On 2016/10/28 at 15:01:19, senorblanco wrote:
> On 2016/10/28 08:46:31, fs wrote:
> 
> > effect-reference-delete does the latter =). The former actually won't
trigger
> > for the PaintLayer code-path (because of the nodeMap-part of the check.)
There
> > remains one additional way to trigger it I think, which would be certain
> > primitives (feFlood, fe*Lighting) in an external document.
> 
> So why does effect-reference-delete not fail? Due to flake?

Flake, yes. -delete is not as flaky as -after though (at least for me locally).
I've looked some more at -after, and it appears to flake (fail) every time a
layout happens before the image resource finishes loading. Will dig some more
into that.
 
> Could we perhaps rewrite it with displayAsyncThen() instead of onload() to
> de-flake it?

This is what my other CL (https://codereview.chromium.org/2453403002) does, so
yes we can! =)

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

https://codereview.chromium.org/2453033004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/FilterEffectBuilder.cpp:154: effect =
referenceFilter->lastEffect();
On 2016/10/28 at 14:58:06, Stephen White wrote:
> On 2016/10/28 08:46:31, fs wrote:
> 
> > That's what we did previously as well (I only reordered the to lines -
modulo
> > the comment), only the first call to buildReferenceFilter takes the previous
> > effect as an argument, while the second takes null.
> 
> Oh right; I misread.
> 
> > The second call is there to
> > please PaintLayer::mapRectForFilter.
> 
> I understand what, I'm just not clear on why. Different SourceGraphic 
> so that mapRectForFilter doesn't traverse into the non-reference-filter 
> inputs?

I'd expect something along those lines to be the reason at least.

> If that's the case, another option here might be to defer 
> connecting reference filters to the rest of the filter chain until 
> we convert it to SkImageFilters. IIRC, that's what the accelerated 
> path does, using SkComposeImageFilter to hook them up. Then we'd only 
> have to build the reference filter once. The downside is that there 
> are some optimizations that Skia can't apply across an 
> SkComposeImageFilter, such as collapsing matrices. But I don't 
> imagine that's a very common case (reference filter plus shorthand 
> filters). Anyway all of that is unrelated to this patch.

That might be nice. I'll try to keep that in mind to have a look at a solution
like that.

[fs, 2016-10-31 09:19:52]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-10-31 09:20:01]

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-10-31 10:35:10]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-31 10:37:27]

Description was changed from

==========
Make sure to always reset the cached filter in ReferenceFilterOperation

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

to

==========
Make sure to always reset the cached filter in ReferenceFilterOperation

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

Committed: https://crrev.com/3353a7a6003262aae1515ac9541a519ec23c8c1c
Cr-Commit-Position: refs/heads/master@{#428678}
==========

[commit-bot: I haz the power, 2016-10-31 10:37:27]

Patchset 1 (id:??) landed as
https://crrev.com/3353a7a6003262aae1515ac9541a519ec23c8c1c
Cr-Commit-Position: refs/heads/master@{#428678}