Skip to content

Commit

Permalink
Revert "Create property tree nodes for will-change only when relevant…
Browse files Browse the repository at this point in the history
… property has will-change."

This reverts commit 5d3dd81.

Reason for revert: failure of fast/sub-pixel/transformed-iframe-copy-on-scroll.html on mac-mac11-arm64.

Original change's description:
> Create property tree nodes for will-change only when relevant property has will-change.
>
> Prior to this change, will-change: transform, opacity, filter, or
> backdrop-filter would cause the creation of property tree nodes (in both
> the paint property tree and the cc property tree) for all of them
> (though only a single effect property node for both opacity and
> backdrop-filter).
>
> With this change, will-change: transform only causes the creation of a
> transform node (in the transform tree), will-change: filter only causes
> the creation of a filter node (in the effect tree), and will-change:
> opacity or backdrop-filter only causes the creation of an effect node
> (in the effect tree).  However, *if* the nodes are created by something
> else, the presence of a different will-change still causes any node that
> is created in the paint property tree to be composited (i.e., created in
> the cc property tree).
>
> This provides the basis on which we can separate the transform node into
> multiple transform nodes, for efficient implementation and animation of
> individual transform properties, without causing a performance
> regression.
>
> Earlier versions of this CL (prior to the change that kept nodes being
> composited for an unrelated will-change) caused the test
> external/wpt/css/filter-effects/effect-reference-feimage-002.html to
> start failing (in the same way as -001 and -003) because the filter
> effect is no longer composited.  It creates a copy of the test as -004
> with will-change: filter rather than will-change: transform that
> continues to have a composited filter effect node and thus continues
> passing even with that earlier change.  This test still seems worth
> adding despite no longer strictly being needed.
>
> Bug: 900241
> Change-Id: I18efc630668ea7eac2a8c0a0417b766762e71d9f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3328730
> Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
> Commit-Queue: David Baron <dbaron@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#983821}

Bug: 900241
Change-Id: I8143ff91fa0d856ab189414e44f12ce1bcdf77e8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3544106
Auto-Submit: David Baron <dbaron@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: David Baron <dbaron@chromium.org>
Commit-Queue: David Baron <dbaron@chromium.org>
Cr-Commit-Position: refs/heads/main@{#984017}
  • Loading branch information
dbaron authored and Chromium LUCI CQ committed Mar 22, 2022
1 parent 9e73033 commit 213dde1
Show file tree
Hide file tree
Showing 30 changed files with 92 additions and 67 deletions.
Expand Up @@ -414,9 +414,12 @@ TEST_P(CompositingTest, PaintPropertiesWhenCompositingSVG) {
EXPECT_EQ(svg_root_effect_node->parent_id, ancestor_effect_node->id);

auto* rect = CcLayerByDOMElementId("rect");
const auto* rect_effect_node =
const auto* rect_filter_node =
GetPropertyTrees()->effect_tree().Node(rect->effect_tree_index());
EXPECT_EQ(rect_filter_node->opacity, 1);

const auto* rect_effect_node =
GetPropertyTrees()->effect_tree().Node(rect_filter_node->parent_id);
EXPECT_EQ(rect_effect_node->opacity, 0.7f);
EXPECT_EQ(rect_effect_node->parent_id, svg_root_effect_node->id);
}
Expand Down Expand Up @@ -1142,7 +1145,7 @@ TEST_P(CompositingSimTest, LayerSubtreeEffectPropertyChanged) {
#inner {
width: 100px;
height: 100px;
will-change: transform, filter;
will-change: transform;
background: lightblue;
}
</style>
Expand Down
Expand Up @@ -702,12 +702,18 @@ void FragmentPaintPropertyTreeBuilder::UpdateStickyTranslation() {
context_.current.transform = properties_->StickyTranslation();
}

// TODO(dbaron): Remove this function when we can remove the
// BackfaceVisibilityInteropEnabled() check, and have the caller use
// CompositingReason::kDirectReasonsForTransformProperty directly.
// TODO(crbug.com/900241): Remove this function and let the caller use
// CompositingReason::kDirectReasonForTransformProperty directly.
static CompositingReasons CompositingReasonsForTransformProperty() {
CompositingReasons reasons =
CompositingReason::kDirectReasonsForTransformProperty;
reasons |= CompositingReason::kActiveTransformAnimation;
// We also need to create a transform node if will-change creates other nodes,
// to avoid raster invalidation caused by creating/deleting those nodes when
// starting/stopping an animation. See: https://crbug.com/942681.
reasons |= CompositingReason::kWillChangeOpacity;
reasons |= CompositingReason::kWillChangeFilter;
reasons |= CompositingReason::kWillChangeBackdropFilter;

if (RuntimeEnabledFeatures::BackfaceVisibilityInteropEnabled())
reasons |= CompositingReason::kBackfaceInvisibility3DAncestor;
Expand Down Expand Up @@ -962,13 +968,6 @@ void FragmentPaintPropertyTreeBuilder::UpdateTransform() {
state.direct_compositing_reasons =
full_context_.direct_compositing_reasons &
CompositingReasonsForTransformProperty();

// If a transform node exists, add an additional direct compositing
// reason for 3d transforms and will-change to ensure it is composited.
state.direct_compositing_reasons |=
(full_context_.direct_compositing_reasons &
CompositingReason::kAdditionalCompositingTrigger);

state.flags.flattens_inherited_transform =
context_.should_flatten_inherited_transform;
if (object_.HasHiddenBackface()) {
Expand Down Expand Up @@ -1056,16 +1055,33 @@ static bool NeedsClipPathClip(const LayoutObject& object,
return false;
}

// TODO(crbug.com/900241): When this bug is fixed, we should let NeedsEffect()
// use CompositingReason::kDirectReasonForEffectProperty directly instead of
// calling this function. We should still call this function in UpdateEffect().
static CompositingReasons CompositingReasonsForEffectProperty() {
CompositingReasons reasons =
CompositingReason::kDirectReasonsForEffectProperty;
reasons |= CompositingReason::kActiveOpacityAnimation |
CompositingReason::kActiveBackdropFilterAnimation;
// We also need to create an effect node if will-change creates other nodes,
// to avoid raster invalidation caused by creating/deleting those nodes when
// starting/stopping an animation. See: https://crbug.com/942681.
// This also avoids decomposition of the effect when the object is forced
// compositing with will-change:transform.
reasons |= CompositingReason::kWillChangeTransform;
reasons |= CompositingReason::kWillChangeFilter;
return reasons;
}

static bool NeedsEffect(const LayoutObject& object,
CompositingReasons direct_compositing_reasons) {
if (object.IsText()) {
DCHECK(!(direct_compositing_reasons &
CompositingReason::kDirectReasonsForEffectProperty));
DCHECK(
!(direct_compositing_reasons & CompositingReasonsForEffectProperty()));
return false;
}

if (direct_compositing_reasons &
CompositingReason::kDirectReasonsForEffectProperty)
if (direct_compositing_reasons & CompositingReasonsForEffectProperty())
return true;

const ComputedStyle& style = object.StyleRef();
Expand Down Expand Up @@ -1231,13 +1247,16 @@ void FragmentPaintPropertyTreeBuilder::UpdateEffect() {

state.direct_compositing_reasons =
full_context_.direct_compositing_reasons &
CompositingReason::kDirectReasonsForEffectProperty;
CompositingReasonsForEffectProperty();

// If an effect node exists, add an additional direct compositing reason
// for 3d transforms and will-change to ensure it is composited.
// for 3d transforms to ensure it is composited.
CompositingReasons additional_transform_compositing_trigger =
CompositingReason::k3DTransform |
CompositingReason::kTrivial3DTransform;
state.direct_compositing_reasons |=
(full_context_.direct_compositing_reasons &
CompositingReason::kAdditionalCompositingTrigger);
additional_transform_compositing_trigger);

// We may begin to composite our subtree prior to an animation starts, but
// a compositor element ID is only needed when an animation is current.
Expand Down Expand Up @@ -1376,6 +1395,25 @@ static bool IsLinkHighlighted(const LayoutObject& object) {
object);
}

// TODO(crbug.com/900241): When this bug is fixed, we should let NeedsFilter()
// use CompositingReason::kDirectReasonForFilterProperty directly instead of
// calling this function. We should still call this function in UpdateFilter().
static CompositingReasons CompositingReasonsForFilterProperty() {
CompositingReasons reasons =
CompositingReason::kDirectReasonsForFilterProperty;
reasons |= CompositingReason::kActiveFilterAnimation;

// We also need to create a filter node if will-change creates other nodes,
// to avoid raster invalidation caused by creating/deleting those nodes when
// starting/stopping an animation. See: https://crbug.com/942681.
// This also avoids decomposition of the filter when the object is forced
// compositing with will-change.
reasons |= CompositingReason::kWillChangeTransform |
CompositingReason::kWillChangeOpacity |
CompositingReason::kWillChangeBackdropFilter;
return reasons;
}

static bool IsClipPathDescendant(const LayoutObject& object) {
// If the object itself is a resource container (root of a resource subtree)
// it is not considered a clipPath descendant since it is independent of its
Expand All @@ -1396,7 +1434,7 @@ static bool IsClipPathDescendant(const LayoutObject& object) {
static bool NeedsFilter(const LayoutObject& object,
const PaintPropertyTreeBuilderContext& full_context) {
if (full_context.direct_compositing_reasons &
CompositingReason::kDirectReasonsForFilterProperty)
CompositingReasonsForFilterProperty())
return true;

if (object.IsBoxModelObject() &&
Expand Down Expand Up @@ -1486,13 +1524,16 @@ void FragmentPaintPropertyTreeBuilder::UpdateFilter() {
// current.
state.direct_compositing_reasons =
full_context_.direct_compositing_reasons &
CompositingReason::kDirectReasonsForFilterProperty;
CompositingReasonsForFilterProperty();

// If a filter node exists, add an additional direct compositing reason
// for 3d transforms and will-change to ensure it is composited.
// If an effect node exists, add an additional direct compositing reason
// for 3d transforms to ensure it is composited.
CompositingReasons additional_transform_compositing_trigger =
CompositingReason::k3DTransform |
CompositingReason::kTrivial3DTransform;
state.direct_compositing_reasons |=
(full_context_.direct_compositing_reasons &
CompositingReason::kAdditionalCompositingTrigger);
additional_transform_compositing_trigger);

state.compositor_element_id =
GetCompositorElementId(CompositorElementIdNamespace::kEffectFilter);
Expand Down
Expand Up @@ -5690,7 +5690,7 @@ TEST_P(PaintPropertyTreeBuilderTest,
height: 100px;
}
#target {
will-change: opacity;
will-change: transform;
width: 100px;
height: 100px;
}
Expand Down Expand Up @@ -6518,7 +6518,7 @@ TEST_P(PaintPropertyTreeBuilderTest, MainFrameDoesntClipContent) {

TEST_P(PaintPropertyTreeBuilderTest, SVGRootCompositedClipPath) {
SetBodyInnerHTML(R"HTML(
<svg id='svg' style='clip-path: circle(); will-change: transform, opacity'></svg>
<svg id='svg' style='clip-path: circle(); will-change: transform'></svg>
)HTML");

const auto* properties = PaintPropertiesForElement("svg");
Expand Down
Expand Up @@ -24,7 +24,7 @@ INSTANTIATE_PAINT_TEST_SUITE_P(SVGContainerPainterTest);
TEST_P(SVGContainerPainterTest, FilterPaintProperties) {
SetBodyInnerHTML(R"HTML(
<style>
#container, #before, #after { will-change: filter; }
#container, #before, #after { will-change: transform; }
</style>
<svg id="svg" width="40" height="40">
<g id="container">
Expand Down
10 changes: 0 additions & 10 deletions third_party/blink/renderer/platform/graphics/compositing_reasons.h
Expand Up @@ -128,16 +128,6 @@ class PLATFORM_EXPORT CompositingReason {
kDirectReasonsForBackdropFilter = kBackdropFilter |
kActiveBackdropFilterAnimation |
kWillChangeBackdropFilter,

// These reasons cause any transform, effect, or filter node that
// exists to be composited. They don't cause creation of a node.
// This is because 3D transforms and incorrect use of will-change
// are likely indicators that compositing is expected because
// certain changes will be made.
kAdditionalCompositingTrigger =
k3DTransform | kTrivial3DTransform | kWillChangeTransform |
kWillChangeOpacity | kWillChangeBackdropFilter | kWillChangeFilter,

};
};

Expand Down
1 change: 0 additions & 1 deletion third_party/blink/web_tests/TestExpectations
Expand Up @@ -469,7 +469,6 @@ crbug.com/968791 virtual/scalefactor200/css3/filters/filterRegions.html [ Failur
crbug.com/968791 crbug.com/1051044 virtual/scalefactor200/external/wpt/css/filter-effects/effect-reference-feimage-001.html [ Failure ]
crbug.com/968791 crbug.com/1051044 virtual/scalefactor200/external/wpt/css/filter-effects/effect-reference-feimage-002.html [ Failure ]
crbug.com/968791 crbug.com/1051044 virtual/scalefactor200/external/wpt/css/filter-effects/effect-reference-feimage-003.html [ Failure ]
crbug.com/968791 virtual/scalefactor200/external/wpt/css/filter-effects/effect-reference-feimage-004.html [ Failure ]
crbug.com/968791 virtual/scalefactor200/external/wpt/css/filter-effects/filters-test-brightness-003.html [ Failure ]

crbug.com/916825 external/wpt/css/filter-effects/filter-subregion-01.html [ Failure ]
Expand Down
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Expand Up @@ -10,7 +10,7 @@
}

.compositing {
will-change: transform, opacity, filter;
will-change: transform;
}

.masked {
Expand Down
Expand Up @@ -17,7 +17,7 @@
-webkit-mask-position: 0px 0px;
-webkit-mask-size: 200px 200px;
-webkit-mask-repeat: no-repeat;
will-change: transform, opacity, filter;
will-change: transform;
}

#layers {
Expand Down
Expand Up @@ -16,7 +16,7 @@
}

.composited {
will-change: transform, opacity, filter;
will-change: transform;
}

.masked {
Expand Down
Expand Up @@ -14,7 +14,7 @@
}

.composited {
will-change: transform, filter;
will-change: transform;
}

.alpha-color {
Expand Down
Expand Up @@ -13,7 +13,7 @@
}

.composited {
will-change: transform, filter;
will-change: transform;
}

.alpha-color {
Expand Down
Expand Up @@ -22,7 +22,7 @@
}

.compositing {
will-change: transform, opacity, filter;
will-change: transform;
}

#container .masked {
Expand Down
Expand Up @@ -22,7 +22,7 @@
}

.compositing {
will-change: transform, opacity, filter;
will-change: transform;
}

#container .masked {
Expand Down

This file was deleted.

This file was deleted.

Expand Up @@ -12,6 +12,7 @@
"contentsOpaqueForText": true,
"backgroundColor": "#0000FF",
"invalidations": [
[0, 0, 100, 100],
[50, 50, 75, 75]
],
"transform": 1
Expand Down
Expand Up @@ -12,6 +12,7 @@
"contentsOpaqueForText": true,
"backgroundColor": "#0000FF",
"invalidations": [
[0, 0, 100, 100],
[50, 50, 75, 75]
],
"transform": 1
Expand Down
Expand Up @@ -12,6 +12,7 @@
"contentsOpaque": true,
"backgroundColor": "#0000FF",
"invalidations": [
[0, 0, 100, 100],
[50, 50, 75, 75]
],
"transform": 1
Expand Down
Expand Up @@ -13,6 +13,7 @@ CONSOLE MESSAGE: debug
"contentsOpaque": true,
"backgroundColor": "#0000FF",
"invalidations": [
[0, 0, 100, 100],
[50, 50, 75, 75]
],
"transform": 1
Expand Down
Expand Up @@ -6,6 +6,12 @@
"contentsOpaque": true,
"backgroundColor": "#FFFFFF"
},
{
"name": "LayoutTableRow TR",
"position": [8, 8],
"bounds": [74, 24],
"contentsOpaqueForText": true
},
{
"name": "LayoutTableRow TR",
"position": [8, 8],
Expand Down
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Expand Up @@ -12,6 +12,7 @@
"contentsOpaqueForText": true,
"backgroundColor": "#0000FF",
"invalidations": [
[0, 0, 100, 100],
[50, 50, 75, 75]
],
"transform": 1
Expand Down
Expand Up @@ -12,6 +12,7 @@
"contentsOpaqueForText": true,
"backgroundColor": "#0000FF",
"invalidations": [
[0, 0, 100, 100],
[50, 50, 75, 75]
],
"transform": 1
Expand Down
Expand Up @@ -12,6 +12,7 @@
"contentsOpaque": true,
"backgroundColor": "#0000FF",
"invalidations": [
[0, 0, 100, 100],
[50, 50, 75, 75]
],
"transform": 1
Expand Down
Expand Up @@ -13,6 +13,7 @@ CONSOLE MESSAGE: debug
"contentsOpaque": true,
"backgroundColor": "#0000FF",
"invalidations": [
[0, 0, 100, 100],
[50, 50, 75, 75]
],
"transform": 1
Expand Down
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file not shown.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.

0 comments on commit 213dde1

Please sign in to comment.