Skip to content

GitLab

T
tangled
Commit b3af3441 authored by Philip Rogers's avatar Philip Rogers Committed by Commit Bot
Browse files
Options

Directly composited value updates should set property node changed bits 

When a property node changes in blink, changed bits need to be set on
the associated cc property node for damage. Blink has a fast-path for
directly updating composited property nodes
(see: PropertyTreeManager::DirectlyUpdateScrollOffsetTransform) that
marks the cc property node as changed but does not cause a full update.
Code was recently added when doing a full update that would unset the
changed bit on a cc property node that was directly updated. This patch
fixes that bug by not clearing the changed bit of a directly-updated
node.

This situation could occur if a fast-path direct update and a slow-path
update occur in the same frame. This was only noticed on Windows because
partial swap is used there which makes the damage bug more visible.

Bug: 985729
Change-Id: I483305477d47aef095591c0f7e54720eb06f7827
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1721197Reviewed-by: default avatarChris Harrelson <chrishtr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682708}
parent 179e0ba2
Hide whitespace changes
Inline Side-by-side
Showing with 91 additions and 5 deletions
third_party/blink/renderer/core/exported/web_layer_test.cc
View file @ b3af3441
...@@ -571,6 +571,91 @@ TEST_P(WebLayerListSimTest, DirectTransformPropertyUpdate) { ...@@ -571,6 +571,91 @@ TEST_P(WebLayerListSimTest, DirectTransformPropertyUpdate) {
EXPECT_FALSE(transform_node->transform_changed); EXPECT_FALSE(transform_node->transform_changed);
} }
// This test is similar to |DirectTransformPropertyUpdate| but tests that
// the changed value of a directly updated transform is still set if some other
// change causes PaintArtifactCompositor to run and do non-direct updates.
TEST_P(WebLayerListSimTest, DirectTransformPropertyUpdateCausesChange) {
// TODO(crbug.com/765003): CAP may make different layerization decisions and
// we cannot guarantee that both divs will be composited in this test. When
// CAP gets closer to launch, this test should be updated to pass.
if (RuntimeEnabledFeatures::CompositeAfterPaintEnabled())
return;
InitializeWithHTML(R"HTML(
<!DOCTYPE html>
<style>
html { overflow: hidden; }
#outer {
width: 100px;
height: 100px;
will-change: transform;
transform: translate(1px, 2px);
}
#inner {
width: 100px;
height: 100px;
will-change: transform;
background: lightblue;
transform: translate(3px, 4px);
}
</style>
<div id='outer'>
<div id='inner'></div>
</div>
)HTML");
Compositor().BeginFrame();
auto* outer_element = GetElementById("outer");
auto* outer_element_layer = ContentLayerAt(ContentLayerCount() - 2);
DCHECK_EQ(outer_element_layer->element_id(),
CompositorElementIdFromUniqueObjectId(
outer_element->GetLayoutObject()->UniqueId(),
CompositorElementIdNamespace::kPrimary));
auto outer_transform_tree_index = outer_element_layer->transform_tree_index();
auto* outer_transform_node =
GetPropertyTrees()->transform_tree.Node(outer_transform_tree_index);
auto* inner_element = GetElementById("inner");
auto* inner_element_layer = ContentLayerAt(ContentLayerCount() - 1);
DCHECK_EQ(inner_element_layer->element_id(),
CompositorElementIdFromUniqueObjectId(
inner_element->GetLayoutObject()->UniqueId(),
CompositorElementIdNamespace::kPrimary));
auto inner_transform_tree_index = inner_element_layer->transform_tree_index();
auto* inner_transform_node =
GetPropertyTrees()->transform_tree.Node(inner_transform_tree_index);
// Initially, the transforms should be unchanged.
EXPECT_FALSE(outer_transform_node->transform_changed);
EXPECT_FALSE(inner_transform_node->transform_changed);
EXPECT_FALSE(paint_artifact_compositor()->NeedsUpdate());
// Modifying the outer transform in a simple way should allow for a direct
// update of the outer transform. Modifying the inner transform in a
// non-simple way should not allow for a direct update of the inner transform.
outer_element->setAttribute(html_names::kStyleAttr,
"transform: translate(5px, 6px)");
inner_element->setAttribute(html_names::kStyleAttr,
"transform: rotate(30deg)");
UpdateAllLifecyclePhasesExceptPaint();
EXPECT_TRUE(outer_transform_node->transform_changed);
EXPECT_FALSE(inner_transform_node->transform_changed);
EXPECT_TRUE(paint_artifact_compositor()->NeedsUpdate());
// After a PaintArtifactCompositor update, which was needed due to the inner
// element's transform change, both the inner and outer transform nodes
// should be marked as changed to ensure they result in damage.
UpdateAllLifecyclePhases();
EXPECT_TRUE(outer_transform_node->transform_changed);
EXPECT_TRUE(inner_transform_node->transform_changed);
// After a frame the |transform_changed| values should be reset.
Compositor().BeginFrame();
EXPECT_FALSE(outer_transform_node->transform_changed);
EXPECT_FALSE(inner_transform_node->transform_changed);
}
// This test ensures that the correct transform nodes are created and bits set // This test ensures that the correct transform nodes are created and bits set
// so that the browser controls movement adjustments needed by bottom-fixed // so that the browser controls movement adjustments needed by bottom-fixed
// elements will work. // elements will work.
......
third_party/blink/renderer/platform/graphics/compositing/property_tree_manager.cc
View file @ b3af3441
...@@ -419,9 +419,7 @@ int PropertyTreeManager::EnsureCompositorTransformNode( ...@@ -419,9 +419,7 @@ int PropertyTreeManager::EnsureCompositorTransformNode(
compositor_node.source_node_id = parent_id; compositor_node.source_node_id = parent_id;
UpdateCcTransformLocalMatrix(compositor_node, transform_node); UpdateCcTransformLocalMatrix(compositor_node, transform_node);
compositor_node.transform_changed = compositor_node.transform_changed = transform_node.NodeChangeAffectsRaster();
transform_node.NodeChanged() >=
PaintPropertyChangeType::kChangedOnlySimpleValues;
compositor_node.flattens_inherited_transform = compositor_node.flattens_inherited_transform =
transform_node.FlattensInheritedTransform(); transform_node.FlattensInheritedTransform();
compositor_node.sorting_context_id = transform_node.RenderingContextId(); compositor_node.sorting_context_id = transform_node.RenderingContextId();
...@@ -1111,8 +1109,7 @@ void PropertyTreeManager::PopulateCcEffectNode( ...@@ -1111,8 +1109,7 @@ void PropertyTreeManager::PopulateCcEffectNode(
} }
effect_node.blend_mode = blend_mode; effect_node.blend_mode = blend_mode;
effect_node.double_sided = !effect.LocalTransformSpace().IsBackfaceHidden(); effect_node.double_sided = !effect.LocalTransformSpace().IsBackfaceHidden();
effect_node.effect_changed = effect_node.effect_changed = effect.NodeChangeAffectsRaster();
effect.NodeChanged() >= PaintPropertyChangeType::kChangedOnlySimpleValues;
} }
} // namespace blink } // namespace blink
third_party/blink/renderer/platform/graphics/paint/paint_property_node.h
View file @ b3af3441
...@@ -147,6 +147,10 @@ class PaintPropertyNode : public RefCounted<NodeType> { ...@@ -147,6 +147,10 @@ class PaintPropertyNode : public RefCounted<NodeType> {
} }
PaintPropertyChangeType NodeChanged() const { return changed_; } PaintPropertyChangeType NodeChanged() const { return changed_; }
bool NodeChangeAffectsRaster() const {
return changed_ != PaintPropertyChangeType::kUnchanged &&
changed_ != PaintPropertyChangeType::kChangedOnlyNonRerasterValues;
}
#if DCHECK_IS_ON() #if DCHECK_IS_ON()
String ToTreeString() const; String ToTreeString() const;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment