Skip to content

GitLab

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

[BlinkGenPropertyTrees] Stop incorrect clearing of property tree changed 

PaintPropertyTreeNode::Changed is used for in-layer raster invalidation,
and cc damage for both SPV2 and BGPT. https://crrev.com/606472 failed
to account for the case when non-transient paint controllers in
GraphicsLayers would clear property tree changed values before the
values could be used for damage.

As a reminder, with BlinkGenPropertyTrees, each GraphicsLayer keeps a
non-transient paint controller, and at the end of painting a transient
paint controller is created and each GraphicsLayer's cc::Layer is
inserted as a foreign layer into the transient paint controller.

This patch changes BGPT to clear all property tree changed bits after
damage has been calculated in PaintArtifactCompositor. It is implemented
with a recursive walk of the GraphicsLayer tree.

Bug: 899628
Change-Id: Ibc7d65d6ab8875508b9c3dd18ab6db1230bf1308
Reviewed-on: https://chromium-review.googlesource.com/c/1327804
Commit-Queue: Philip Rogers <pdr@chromium.org>
Reviewed-by: default avatarXianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606874}
parent 1dc99fe0
Show whitespace changes
Inline Side-by-side
Showing with 117 additions and 14 deletions
third_party/WebKit/LayoutTests/flag-specific/enable-blink-gen-property-trees/paint/invalidation/clip/clip-path-constant-repaint-expected.txt
View file @ 6f5a1bd0
...@@ -26,7 +26,14 @@ ...@@ -26,7 +26,14 @@
{ {
"name": "Mask Layer", "name": "Mask Layer",
"bounds": [800, 300], "bounds": [800, 300],
"backfaceVisibility": "hidden" "backfaceVisibility": "hidden",
"paintInvalidations": [
{
"object": "Mask Layer",
"rect": [0, 0, 800, 300],
"reason": "paint property change"
}
]
} }
], ],
"transform": 2 "transform": 2
......
third_party/blink/renderer/core/exported/web_layer_test.cc
View file @ 6f5a1bd0
...@@ -579,4 +579,62 @@ TEST_P(WebLayerListSimTest, LayerSubtreeClipPropertyChanged) { ...@@ -579,4 +579,62 @@ TEST_P(WebLayerListSimTest, LayerSubtreeClipPropertyChanged) {
EXPECT_FALSE(inner_element_layer->subtree_property_changed()); EXPECT_FALSE(inner_element_layer->subtree_property_changed());
} }
TEST_P(WebLayerListSimTest, LayerSubtreeOverflowClipPropertyChanged) {
// TODO(crbug.com/765003): SPV2 may make different layerization decisions and
// we cannot guarantee that both divs will be composited in this test. When
// SPV2 gets closer to launch, this test should be updated to pass.
if (RuntimeEnabledFeatures::SlimmingPaintV2Enabled())
return;
InitializeWithHTML(R"HTML(
<!DOCTYPE html>
<style>
html { overflow: hidden; }
#outer {
width: 100px;
height: 100px;
will-change: transform;
position: absolute;
overflow: hidden;
}
#inner {
width: 200px;
height: 100px;
will-change: transform;
background: lightblue;
}
</style>
<div id='outer'>
<div id='inner'></div>
</div>
)HTML");
Compositor().BeginFrame();
auto* outer_element = GetElementById("outer");
auto* outer_element_layer = ContentLayerAt(ContentLayerCount() - 2);
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));
// Initially, no layer should have |subtree_property_changed| set.
EXPECT_FALSE(outer_element_layer->subtree_property_changed());
EXPECT_FALSE(inner_element_layer->subtree_property_changed());
// Modifying the clip width should set |subtree_property_changed| on
// both layers.
outer_element->setAttribute(html_names::kStyleAttr, "width: 200px;");
WebView().MainFrameWidget()->UpdateAllLifecyclePhases();
EXPECT_TRUE(outer_element_layer->subtree_property_changed());
EXPECT_TRUE(inner_element_layer->subtree_property_changed());
// After a frame the |subtree_property_changed| value should be reset.
Compositor().BeginFrame();
EXPECT_FALSE(outer_element_layer->subtree_property_changed());
EXPECT_FALSE(inner_element_layer->subtree_property_changed());
}
} // namespace blink } // namespace blink
third_party/blink/renderer/core/frame/local_frame_view.cc
View file @ 6f5a1bd0
...@@ -2558,6 +2558,14 @@ bool LocalFrameView::RunPrePaintLifecyclePhase( ...@@ -2558,6 +2558,14 @@ bool LocalFrameView::RunPrePaintLifecyclePhase(
return target_state > DocumentLifecycle::kPrePaintClean; return target_state > DocumentLifecycle::kPrePaintClean;
} }
template <typename Function>
static void ForAllGraphicsLayers(GraphicsLayer& layer,
const Function& function) {
function(layer);
for (auto* child : layer.Children())
ForAllGraphicsLayers(*child, function);
}
void LocalFrameView::RunPaintLifecyclePhase() { void LocalFrameView::RunPaintLifecyclePhase() {
TRACE_EVENT0("blink,benchmark", "LocalFrameView::RunPaintLifecyclePhase"); TRACE_EVENT0("blink,benchmark", "LocalFrameView::RunPaintLifecyclePhase");
// While printing a document, the paint walk is done by the printing // While printing a document, the paint walk is done by the printing
...@@ -2603,8 +2611,18 @@ void LocalFrameView::RunPaintLifecyclePhase() { ...@@ -2603,8 +2611,18 @@ void LocalFrameView::RunPaintLifecyclePhase() {
// Property tree changed state is typically cleared through // Property tree changed state is typically cleared through
// |PaintController::FinishCycle| but that will be a no-op because // |PaintController::FinishCycle| but that will be a no-op because
// the paint controller is transient, so force the changed state to be // the paint controller is transient, so force the changed state to be
// updated here. // cleared here.
paint_controller_->ClearPropertyTreeChangedState(); paint_controller_->ClearPropertyTreeChangedStateTo(
PropertyTreeState::Root());
auto* root = GetLayoutView()->Compositor()->PaintRootGraphicsLayer();
if (root) {
ForAllGraphicsLayers(*root, [](GraphicsLayer& layer) {
if (layer.DrawsContent() && layer.HasLayerState()) {
layer.GetPaintController().ClearPropertyTreeChangedStateTo(
layer.GetPropertyTreeState());
}
});
}
paint_controller_ = nullptr; paint_controller_ = nullptr;
} }
} }
......
third_party/blink/renderer/platform/graphics/graphics_layer_test.cc
View file @ 6f5a1bd0
...@@ -139,8 +139,11 @@ TEST_P(GraphicsLayerTest, PaintRecursively) { ...@@ -139,8 +139,11 @@ TEST_P(GraphicsLayerTest, PaintRecursively) {
layers_.graphics_layer_client().SetNeedsRepaint(true); layers_.graphics_layer_client().SetNeedsRepaint(true);
layers_.graphics_layer().PaintRecursively(); layers_.graphics_layer().PaintRecursively();
// With BlinkGenPropertyTrees, these are not cleared until after paint.
if (!RuntimeEnabledFeatures::BlinkGenPropertyTreesEnabled()) {
EXPECT_FALSE(transform1->Changed(transform_root)); EXPECT_FALSE(transform1->Changed(transform_root));
EXPECT_FALSE(transform2->Changed(transform_root)); EXPECT_FALSE(transform2->Changed(transform_root));
}
} }
TEST_P(GraphicsLayerTest, SetDrawsContentFalse) { TEST_P(GraphicsLayerTest, SetDrawsContentFalse) {
......
third_party/blink/renderer/platform/graphics/paint/paint_artifact.cc
View file @ 6f5a1bd0
...@@ -103,8 +103,14 @@ void PaintArtifact::AppendToDisplayItemList(const FloatSize& visual_rect_offset, ...@@ -103,8 +103,14 @@ void PaintArtifact::AppendToDisplayItemList(const FloatSize& visual_rect_offset,
} }
void PaintArtifact::FinishCycle() { void PaintArtifact::FinishCycle() {
// BlinkGenPropertyTrees uses PaintController::ClearPropertyTreeChangedStateTo
// for clearing the property tree changed state at the end of paint instead of
// in FinishCycle. See: LocalFrameView::RunPaintLifecyclePhase.
bool clear_property_tree_changed =
!RuntimeEnabledFeatures::BlinkGenPropertyTreesEnabled();
for (auto& chunk : chunks_) { for (auto& chunk : chunks_) {
chunk.client_is_just_created = false; chunk.client_is_just_created = false;
if (clear_property_tree_changed)
chunk.properties.ClearChangedToRoot(); chunk.properties.ClearChangedToRoot();
} }
} }
......
third_party/blink/renderer/platform/graphics/paint/paint_controller.cc
View file @ 6f5a1bd0
...@@ -562,14 +562,17 @@ void PaintController::FinishCycle() { ...@@ -562,14 +562,17 @@ void PaintController::FinishCycle() {
#endif #endif
} }
void PaintController::ClearPropertyTreeChangedState() { void PaintController::ClearPropertyTreeChangedStateTo(
const PropertyTreeState& to) {
DCHECK(RuntimeEnabledFeatures::BlinkGenPropertyTreesEnabled()); DCHECK(RuntimeEnabledFeatures::BlinkGenPropertyTreesEnabled());
DCHECK(usage_ == kTransient);
// Calling |ClearChangedToRoot| for every chunk is O(|property nodes|^2) and // Calling |ClearChangedTo| for every chunk is O(|property nodes|^2) and
// could be optimized by caching which nodes that have already been cleared. // could be optimized by caching which nodes that have already been cleared.
for (const auto& chunk : current_paint_artifact_->PaintChunks()) for (const auto& chunk : current_paint_artifact_->PaintChunks()) {
chunk.properties.ClearChangedToRoot(); chunk.properties.Transform()->ClearChangedTo(to.Transform());
chunk.properties.Clip()->ClearChangedTo(to.Clip());
chunk.properties.Effect()->ClearChangedTo(to.Effect());
}
} }
size_t PaintController::ApproximateUnsharedMemoryUsage() const { size_t PaintController::ApproximateUnsharedMemoryUsage() const {
......
third_party/blink/renderer/platform/graphics/paint/paint_controller.h
View file @ 6f5a1bd0
...@@ -178,7 +178,7 @@ class PLATFORM_EXPORT PaintController { ...@@ -178,7 +178,7 @@ class PLATFORM_EXPORT PaintController {
// BlinkGenPropertyTrees and this function provides a hook for clearing // BlinkGenPropertyTrees and this function provides a hook for clearing
// the property tree changed state after paint. // the property tree changed state after paint.
// TODO(pdr): Remove this when BlinkGenPropertyTrees ships. // TODO(pdr): Remove this when BlinkGenPropertyTrees ships.
void ClearPropertyTreeChangedState(); void ClearPropertyTreeChangedStateTo(const PropertyTreeState&);
// Returns the approximate memory usage, excluding memory likely to be // Returns the approximate memory usage, excluding memory likely to be
// shared with the embedder after copying to WebPaintController. // shared with the embedder after copying to WebPaintController.
......
third_party/blink/renderer/platform/graphics/paint/paint_property_node.h
View file @ 6f5a1bd0
...@@ -74,8 +74,9 @@ class PaintPropertyNode : public RefCounted<NodeType> { ...@@ -74,8 +74,9 @@ class PaintPropertyNode : public RefCounted<NodeType> {
return true; return true;
} }
void ClearChangedToRoot() const { void ClearChangedToRoot() const { ClearChangedTo(nullptr); }
for (auto* n = this; n; n = n->Parent()) void ClearChangedTo(const NodeType* node) const {
for (auto* n = this; n && n != node; n = n->Parent())
n->changed_ = false; n->changed_ = false;
} }
...@@ -143,6 +144,13 @@ class PaintPropertyNode : public RefCounted<NodeType> { ...@@ -143,6 +144,13 @@ class PaintPropertyNode : public RefCounted<NodeType> {
// nodes that do not affect rendering and are ignored for the purposes of // nodes that do not affect rendering and are ignored for the purposes of
// display item list generation. // display item list generation.
bool is_parent_alias_ = false; bool is_parent_alias_ = false;
// Indicates that the paint property value changed in the last update in the
// prepaint lifecycle step. This is used for raster invalidation and damage
// in the compositor. This value is cleared through |ClearChangedTo*|. With
// BlinkGenPropertyTrees, this is cleared explicitly at the end of paint (see:
// LocalFrameView::RunPaintLifecyclePhase), otherwise this is cleared through
// PaintController::FinishCycle.
mutable bool changed_ = true; mutable bool changed_ = true;
#if DCHECK_IS_ON() #if DCHECK_IS_ON()
......
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