[SPv2] Create effect nodes for CSS filter (issue 2428513004 by trchen@chromium.org)

[trc...@chromium.org]

Reviewers: chrishtr, ajuma (slow till Oct 24), pdr.
CL: https://chromiumcodereview.appspot.com/2428513004/

Message:
This is a WIP for SPv2 filter implementation. Right now I'm still blocked by a
DCHECK in PaintLayerClipper / GeometryMapper. Uploaded for early preview.

To ajuma:

How the generated effect node would interact with clip tree will be slightly
different from SPv1.

Previously the effect's output clip must be a common ancestor of all compositing
children (child effects, layers painting into this effect)'s clip, and when the
compositing children draw into the surface, only clips below the output clip
will be applied.

In this CL, children of a filter will be explicitly unclipped (i.e. their clip
node parented to root). This should bring back some sanity.

To chrishtr:

The filter & clip interaction was inconsistent between composited and
non-composited path. For composited path, only the filter output is clipped,
while the inputs skip all accumulated clip. For non-composited path, both output
and inputs are clipped. This is probably a bug due to not expanding Skia layer
bound when spatial filter (e.g. blur) exists. This CL follows the composited
behavior, which IMO makes more sense when interacts with scrolling.

This clip escaping behavior triggered a DCHECK in GeometryMapper.

Another thing I noticed is that currently GeometryMapper doesn't distinguish
between cull rect and clip rect computation.

Although computation is similar, I think they are very different concept and
needs to be explicitly spelled out in code.

Cull rect allows false positive, and should take animation & filter expanding
into account.

One the other hand, clip rect should be exact rect. If a clip chain contains
rect that are not axis aligned to target space, or contains rounded rect, those
should be DCHECK'd or reported to caller by a flag.

For SPv2 only the cull rect is useful in Blink, while clip rect may be useful in
CC. For SPv1 only the clip rect is used.

Description:
WIP WIP WIP WIP WIP WIP WIP WIP WIP WIP WIP WIP WIP WIP WIP WIP WIP WIP WIP WIP

[SPv2] Create effect nodes for CSS filter

This CL does the following two things:
* Add filter related properties to EffectPaintPropertyNode
* Add CSS effect support to PaintPropertyTreeBuilder

What's not in the scope of this CL:
* Compile filter property from Blink effect node to CC effect node

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

Affected files (+209, -38 lines):
M third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
M third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
M third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
M third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h
M third_party/WebKit/Source/platform/graphics/paint/GeometryMapperTest.cpp
M third_party/WebKit/Source/platform/graphics/paint/PaintArtifactToSkCanvasTest.cpp
M third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp
M third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp

[aj...@chromium.org]

On 2016/10/19 02:50:48, trchen wrote:
> This is a WIP for SPv2 filter implementation. Right now I'm still blocked by a
> DCHECK in PaintLayerClipper / GeometryMapper. Uploaded for early preview.
>
> To ajuma:
>
> How the generated effect node would interact with clip tree will be slightly
> different from SPv1.
>
> Previously the effect's output clip must be a common ancestor of all
compositing
> children (child effects, layers painting into this effect)'s clip, and when
the
> compositing children draw into the surface, only clips below the output clip
> will be applied.
>
> In this CL, children of a filter will be explicitly unclipped (i.e. their clip
> node parented to root). This should bring back some sanity.
>
> To chrishtr:
>
> The filter & clip interaction was inconsistent between composited and
> non-composited path. For composited path, only the filter output is clipped,
> while the inputs skip all accumulated clip.

I believe the composited behavior (at least in terms of what happens in cc)
depends on whether you're talking about computing visible rects or about
applying clips at draw time. For the latter, yes, we clip the output of the
filter, not its inputs. But for computing visible rects, we use the accumulated
clip including the surface clip (unless the surface is an "unclipped surface",
having no clip in between it and its target, but we're working towards removing
that distinction for visible rects in order to compute more realistic visible
rects).

I'd worry that if we always treat the inputs to filters as unclipped for the
purposes of computing visible rects (and that's what setting their clip tree
parent to the root will do), we'll end up with overly-large visible rects and
raster a lot more than we need to.

https://codereview.chromium.org/2428513004/

[p...@chromium.org]

https://chromiumcodereview.appspot.com/2428513004/diff/1/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):

https://chromiumcodereview.appspot.com/2428513004/diff/1/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp#newcode387
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:387:
// In this example "A" should be clipped if the filter did not present.
Nit: did not -> is not

https://chromiumcodereview.appspot.com/2428513004/diff/1/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp#newcode392
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:392:
// clipped, so a blurred "B" may still be invisible.
This description is good and helped me understand what the code is
doing. I agree that matching the current behavior is the right thing to
do.

https://chromiumcodereview.appspot.com/2428513004/diff/1/third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h
File
third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h
(right):

https://chromiumcodereview.appspot.com/2428513004/diff/1/third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h#newcode76
third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h:76:
// The chain of effects are applied in reverse order of the following
list:
Nit: can we list these in order instead of reverse order?

https://chromiumcodereview.appspot.com/2428513004/

[trc...@chromium.org]

https://codereview.chromium.org/2428513004/diff/1/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):

https://codereview.chromium.org/2428513004/diff/1/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp#newcode387

third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:387:
// In this example "A" should be clipped if the filter did not present.

On 2016/10/19 17:44:43, pdr. wrote:
> Nit: did not -> is not

Done.

https://codereview.chromium.org/2428513004/diff/1/third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h
File
third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h
(right):

https://codereview.chromium.org/2428513004/diff/1/third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h#newcode76

third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h:76:
// The chain of effects are applied in reverse order of the following
list:

On 2016/10/19 17:44:43, pdr. wrote:
> Nit: can we list these in order instead of reverse order?

Done.

By the way every time we added a new property, all
EffectPaintPropertyNode::create() invocation in the unit tests need to
be updated. This results in O(n^2) effort and won't scale. I think we
should adopt keyword-based optional arguments (which can be implemented
with variadic templates), if that's accepted by style guide.

https://codereview.chromium.org/2428513004/

[p...@chromium.org]

Just a few couple for cleaning up updateEffect. After that I think we can land
this.


https://codereview.chromium.org/2428513004/diff/40001/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):

https://codereview.chromium.org/2428513004/diff/40001/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp#newcode354
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:354:
if (!style.isStackingContext()) {
Is this needed? It seems to include hasOpacity() and
hasFilterInducingProperty().

https://codereview.chromium.org/2428513004/diff/40001/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp#newcode369
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:369:
CompositorFilterOperations filter;
Could the following avoid the duplicated clearing code?
if (style.opacity() == 1.0f && !object->hasFilterInducingProperty())
// clear effect and return.
// ...continue constructing effect node.

Alternatively something like:
bool effectNodeNeeded = false;
if (style.opacity() != 1.0f)
effectNodeNeeded = true;
if (object->hasFilterInducingProperty())
effectNodeNeeded = true;

// TODO(trchen): Can't omit effect node if we have 3D children.
// TODO(trchen): Can't omit effect node if we have blending children.

if (!effectNodeNeeded)
// clear effect and return
// ...continue constructing effect node.

https://codereview.chromium.org/2428513004/diff/40001/third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp
File
third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp
(right):

https://codereview.chromium.org/2428513004/diff/40001/third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp#newcode20
third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp:20:
TransformPaintPropertyNode* dummyRootTransform() {
Potential followup: WDYT of adding a superclass that all spv2 tests
inherit from, and defining these once there? We could also add helpers
such as createFilterEffectNode, createOpacityEffectNode, etc, to avoid
needing to update all tests when adding new effect subtypes.

https://codereview.chromium.org/2428513004/

[trc...@chromium.org]

https://codereview.chromium.org/2428513004/diff/40001/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
(right):

https://codereview.chromium.org/2428513004/diff/40001/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp#newcode354
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:354:
if (!style.isStackingContext()) {

On 2016/10/22 03:58:37, pdr. wrote:
> Is this needed? It seems to include hasOpacity() and
> hasFilterInducingProperty().

I've thought about whether to make it a DCHECK or an early out, and
decided early out is probably a better choice.

The logic goes like:
1. An effect node is the abstraction of an isolated group. Create an
effect node whenever there is an isolated group. In HTML/CSS, every
stacking context is isolated (#1).
2. That said, we can omit creation of effect nodes if we can prove the
rendering will be equivalent. Things require proper isolation include
effects applied on the current group, children that interact with
backdrop (i.e. mix-blend-mode, backdrop-filter), and depth sorting of 3D
children.

Note #1: Which is not true in SVG. SVG can actually have non-isolated
stacking context.

https://codereview.chromium.org/2428513004/diff/40001/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp#newcode369
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:369:
CompositorFilterOperations filter;

On 2016/10/22 03:58:37, pdr. wrote:
> Could the following avoid the duplicated clearing code?

I'm thinking something like this:

bool PPTB::findIsolationReasonAndUpdateEffectNode() {
bool effectNodeNeeded = false;
if (opacity != 1.0)
effectNodeNeeded = true;
if (!filter.isEmpty())
effectNodeNeeded = true;

if (!effectNodeNeeded)
return false;

createOrUpdateEffect(...);
return true;
}

void PaintPropertyTreeBuilder::updateEffect() {
if (!object.styleRef().isStackingContext() ||
!findIsolationReasonAndUpdateEffectNode())
properties->clearEffect();

}

https://codereview.chromium.org/2428513004/diff/40001/third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp
File
third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp
(right):

https://codereview.chromium.org/2428513004/diff/40001/third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp#newcode20
third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp:20:
TransformPaintPropertyNode* dummyRootTransform() {

On 2016/10/22 03:58:37, pdr. wrote:
> Potential followup: WDYT of adding a superclass that all spv2 tests
inherit
> from, and defining these once there? We could also add helpers such as
> createFilterEffectNode, createOpacityEffectNode, etc, to avoid needing
to update
> all tests when adding new effect subtypes.

I'm thinking to move the static root nodes to
Source/platform/graphics/paint/*PaintPropertyNode.cpp
Is there a reason why that'd be a bad idea?

I think adding the helper function is a good idea.

https://codereview.chromium.org/2428513004/

[p...@chromium.org]

LGTM

> third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp:20:
TransformPaintPropertyNode* dummyRootTransform() {
> On 2016/10/22 03:58:37, pdr. wrote:
> > Potential followup: WDYT of adding a superclass that all spv2 tests inherit
> > from, and defining these once there? We could also add helpers such as
> > createFilterEffectNode, createOpacityEffectNode, etc, to avoid needing to
update
> > all tests when adding new effect subtypes.
>
> I'm thinking to move the static root nodes to
Source/platform/graphics/paint/*PaintPropertyNode.cpp
> Is there a reason why that'd be a bad idea?
>
> I think adding the helper function is a good idea.

Statics are local to compilation units so I think we would end up with multiple
root nodes in that case.

Modulo c++ issues though, I think it's a good idea.

https://codereview.chromium.org/2428513004/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2428513004/80001


https://codereview.chromium.org/2428513004/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

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_tests_slimming_paint_v2/builds/1068)

https://codereview.chromium.org/2428513004/

[p...@chromium.org]

On 2016/10/25 at 00:27:25, commit-bot wrote:
> 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_tests_slimming_paint_v2/builds/1068)

These failures seem real. Are they just simple crashes?

https://codereview.chromium.org/2428513004/

[trc...@chromium.org]

I think they are real. Still investigating.

https://codereview.chromium.org/2428513004/

[trc...@chromium.org]

How did the other CLs passed linux_layout_tests_slimming_paint_v2?

I tried vanilla ToT on my local machine with
--enable-blink-features=PaintUnderInvalidationChecking,SlimmingPaintV2 , there
are still under invalidation errors. I'll clobber my local build and try again.

https://codereview.chromium.org/2428513004/

[p...@chromium.org]

On 2016/10/25 at 22:03:38, trchen wrote:
> How did the other CLs passed linux_layout_tests_slimming_paint_v2?
>
> I tried vanilla ToT on my local machine with
--enable-blink-features=PaintUnderInvalidationChecking,SlimmingPaintV2 , there
are still under invalidation errors. I'll clobber my local build and try again.

If there are existing failures, they will be ignored. The following look like
new failures:
fast/css/invalidation/link-pseudo.html
fast/css/all-inherit-or-unset-color.html
fast/css/getComputedStyle/getComputedStyle-all.html
fast/layers/transform-inline-no-display.html
fast/css/containment/paint-contained-tablerow-with-fixed-children.html
fast/css/unset-keyword.html
fast/writing-mode/orthogonal-writing-modes-in-layoutview-with-floats.html
fast/layers/perspective-relpos-image-no-display.html
fast/css/all-shorthand-css-text.html
fast/css/crash-on-csstext.html
fast/transforms/transform-on-inline.html
fast/css/getComputedStyle/getComputedStyle-transform.html
fast/css/invalidation/visited-pseudo.html
fast/layers/perspective-inline-no-display.html
fast/transforms/container-transform-crash.html
fast/css/all-shorthand.html
fast/css/webkit-empty-transform-preserve3d-crash.html
fast/reflections/inline-crash.html
fast/css/serialize-style-with-all-crash.html

https://codereview.chromium.org/2428513004/

[trc...@chromium.org]

I was referring to the previous run. The list failures in the last re-run you
posted does make sense. I assumed that every stacking context should have a
PaintLayer, turns out that is not true. (Very surprising but I'll investigate
later.)

https://codereview.chromium.org/2428513004/

[trc...@chromium.org]

Okay without the DCHECK it passed. I also figured out why the DCHECK failed in
the first place. Do you mind me delay landing this CL to fix another bug that
blocked this DCHECK?

https://codereview.chromium.org/2428513004/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2428513004/100001


https://codereview.chromium.org/2428513004/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

Committed patchset #6 (id:100001)

https://codereview.chromium.org/2428513004/

[commit-bot@chromium.org via chromiumcodereview-hr.appspot.com]

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

https://codereview.chromium.org/2428513004/