Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Fix Opacity performance regression on Fuchsia #15573

Merged
merged 4 commits into from
Jan 16, 2020
Merged

Fix Opacity performance regression on Fuchsia #15573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
935f267
Add more profile markers
arbreng Jan 10, 2020
f7e4c10
Revert FuchsiaSystemCompistedLayer changes
arbreng Jan 14, 2020
b740240
Re-add opacity w/o elevation changes
arbreng Jan 14, 2020
a6040f2
Fix formatting
arbreng Jan 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions ci/licenses_golden/licenses_flutter
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,6 @@ FILE: ../../../flutter/flow/layers/color_filter_layer_unittests.cc
FILE: ../../../flutter/flow/layers/container_layer.cc
FILE: ../../../flutter/flow/layers/container_layer.h
FILE: ../../../flutter/flow/layers/container_layer_unittests.cc
FILE: ../../../flutter/flow/layers/elevated_container_layer.cc
FILE: ../../../flutter/flow/layers/elevated_container_layer.h
FILE: ../../../flutter/flow/layers/fuchsia_system_composited_layer.cc
FILE: ../../../flutter/flow/layers/fuchsia_system_composited_layer.h
FILE: ../../../flutter/flow/layers/image_filter_layer.cc
FILE: ../../../flutter/flow/layers/image_filter_layer.h
FILE: ../../../flutter/flow/layers/image_filter_layer_unittests.cc
Expand Down
4 changes: 0 additions & 4 deletions flow/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ source_set("flow") {
"layers/color_filter_layer.h",
"layers/container_layer.cc",
"layers/container_layer.h",
"layers/elevated_container_layer.cc",
"layers/elevated_container_layer.h",
"layers/image_filter_layer.cc",
"layers/image_filter_layer.h",
"layers/layer.cc",
Expand Down Expand Up @@ -80,8 +78,6 @@ source_set("flow") {
sources += [
"layers/child_scene_layer.cc",
"layers/child_scene_layer.h",
"layers/fuchsia_system_composited_layer.cc",
"layers/fuchsia_system_composited_layer.h",
"scene_update_context.cc",
"scene_update_context.h",
"view_holder.cc",
Expand Down
2 changes: 2 additions & 0 deletions flow/layers/child_scene_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ void ChildSceneLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {
void ChildSceneLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "ChildSceneLayer::Paint");
FML_DCHECK(needs_painting());
FML_DCHECK(needs_system_composite());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to make sure I understand this: So before FuchsiaSystemCompositedLayer would wrap this call. Now we're not constructing it any more, so we're calling it before every paint?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing,

We landed this PR: #14024 in order to have OpacityLayer work on Fuchsia even with cross-process content. This way you could do say Opacity(opacity: 0.1, children: [chromium_child_view]) and the chromium that you embedded would be rendered at 0.1 group opacity.

In order to do so, OpacityLayer's need to be "popped out" as their own system composited layer. We also wanted to remove this same "popping out" from the PhysicalShapeLayer as it is wasteful in terms of resources. needs_system_composite flag indicates that one of the layers children or the layer itself is going to be popped out.

Unfortunately, the PhysicalShapeLayer part of that change caused a large performance regression on Fuchsia. So, this patch reverts the original one and then re-applies it but without this commit: c4df6df

Definitely agreed on the tests, I'm thinking about that right now. Lack of tests is why the perf regression made it through in the first place. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, to your previous question: ChildSceneLayer is a layer to embed cross-process content, so by definition needs_system_composite is true for it. I added that DCHECK as a sanity check while debugging


// If we are being rendered into our own frame using the system compositor,
// then it is neccesary to "punch a hole" in the canvas/frame behind us so
Expand All @@ -42,6 +43,7 @@ void ChildSceneLayer::Paint(PaintContext& context) const {
}

void ChildSceneLayer::UpdateScene(SceneUpdateContext& context) {
TRACE_EVENT0("flutter", "ChildSceneLayer::UpdateScene");
FML_DCHECK(needs_system_composite());

auto* view_holder = ViewHolder::FromId(layer_id_);
Expand Down
9 changes: 8 additions & 1 deletion flow/layers/clip_path_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,14 @@ ClipPathLayer::ClipPathLayer(const SkPath& clip_path, Clip clip_behavior)
}

void ClipPathLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {
TRACE_EVENT0("flutter", "ClipPathLayer::Preroll");

SkRect previous_cull_rect = context->cull_rect;
SkRect clip_path_bounds = clip_path_.getBounds();
children_inside_clip_ = context->cull_rect.intersect(clip_path_bounds);
if (children_inside_clip_) {
TRACE_EVENT_INSTANT0("flutter", "children inside clip rect");

Layer::AutoPrerollSaveLayerState save =
Layer::AutoPrerollSaveLayerState::Create(context, UsesSaveLayer());
context->mutators_stack.PushClipPath(clip_path_);
Expand All @@ -39,6 +43,7 @@ void ClipPathLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {
#if defined(OS_FUCHSIA)

void ClipPathLayer::UpdateScene(SceneUpdateContext& context) {
TRACE_EVENT0("flutter", "ClipPathLayer::UpdateScene");
FML_DCHECK(needs_system_composite());

// TODO(liyuqian): respect clip_behavior_
Expand All @@ -52,8 +57,10 @@ void ClipPathLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "ClipPathLayer::Paint");
FML_DCHECK(needs_painting());

if (!children_inside_clip_)
if (!children_inside_clip_) {
TRACE_EVENT_INSTANT0("flutter", "children not inside clip rect, skipping");
return;
}

SkAutoCanvasRestore save(context.internal_nodes_canvas, true);
context.internal_nodes_canvas->clipPath(clip_path_,
Expand Down
9 changes: 8 additions & 1 deletion flow/layers/clip_rect_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,13 @@ ClipRectLayer::ClipRectLayer(const SkRect& clip_rect, Clip clip_behavior)
}

void ClipRectLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {
TRACE_EVENT0("flutter", "ClipRectLayer::Preroll");

SkRect previous_cull_rect = context->cull_rect;
children_inside_clip_ = context->cull_rect.intersect(clip_rect_);
if (children_inside_clip_) {
TRACE_EVENT_INSTANT0("flutter", "children inside clip rect");

Layer::AutoPrerollSaveLayerState save =
Layer::AutoPrerollSaveLayerState::Create(context, UsesSaveLayer());
context->mutators_stack.PushClipRect(clip_rect_);
Expand All @@ -32,6 +36,7 @@ void ClipRectLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {
#if defined(OS_FUCHSIA)

void ClipRectLayer::UpdateScene(SceneUpdateContext& context) {
TRACE_EVENT0("flutter", "ClipRectLayer::UpdateScene");
FML_DCHECK(needs_system_composite());

// TODO(liyuqian): respect clip_behavior_
Expand All @@ -45,8 +50,10 @@ void ClipRectLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "ClipRectLayer::Paint");
FML_DCHECK(needs_painting());

if (!children_inside_clip_)
if (!children_inside_clip_) {
TRACE_EVENT_INSTANT0("flutter", "children not inside clip rect, skipping");
return;
}

SkAutoCanvasRestore save(context.internal_nodes_canvas, true);
context.internal_nodes_canvas->clipRect(clip_rect_,
Expand Down
9 changes: 8 additions & 1 deletion flow/layers/clip_rrect_layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,14 @@ ClipRRectLayer::ClipRRectLayer(const SkRRect& clip_rrect, Clip clip_behavior)
}

void ClipRRectLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {
TRACE_EVENT0("flutter", "ClipRRectLayer::Preroll");

SkRect previous_cull_rect = context->cull_rect;
SkRect clip_rrect_bounds = clip_rrect_.getBounds();
children_inside_clip_ = context->cull_rect.intersect(clip_rrect_bounds);
if (children_inside_clip_) {
TRACE_EVENT_INSTANT0("flutter", "children inside clip rect");

Layer::AutoPrerollSaveLayerState save =
Layer::AutoPrerollSaveLayerState::Create(context, UsesSaveLayer());
context->mutators_stack.PushClipRRect(clip_rrect_);
Expand All @@ -33,6 +37,7 @@ void ClipRRectLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) {
#if defined(OS_FUCHSIA)

void ClipRRectLayer::UpdateScene(SceneUpdateContext& context) {
TRACE_EVENT0("flutter", "ClipRRectLayer::UpdateScene");
FML_DCHECK(needs_system_composite());

// TODO(liyuqian): respect clip_behavior_
Expand All @@ -46,8 +51,10 @@ void ClipRRectLayer::Paint(PaintContext& context) const {
TRACE_EVENT0("flutter", "ClipRRectLayer::Paint");
FML_DCHECK(needs_painting());

if (!children_inside_clip_)
if (!children_inside_clip_) {
TRACE_EVENT_INSTANT0("flutter", "children not inside clip rect, skipping");
return;
}

SkAutoCanvasRestore save(context.internal_nodes_canvas, true);
context.internal_nodes_canvas->clipRRect(clip_rrect_,
Expand Down
49 changes: 0 additions & 49 deletions flow/layers/elevated_container_layer.cc
View file
Open in desktop

This file was deleted.

34 changes: 0 additions & 34 deletions flow/layers/elevated_container_layer.h
View file
Open in desktop

This file was deleted.

55 changes: 0 additions & 55 deletions flow/layers/fuchsia_system_composited_layer.cc
View file
Open in desktop

This file was deleted.

37 changes: 0 additions & 37 deletions flow/layers/fuchsia_system_composited_layer.h
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion flow/layers/layer_tree.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ void LayerTree::UpdateScene(SceneUpdateContext& context,
context,
SkRRect::MakeRect(
SkRect::MakeWH(frame_size_.width(), frame_size_.height())),
SK_ColorTRANSPARENT);
SK_ColorTRANSPARENT, SK_AlphaOPAQUE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is covering Flutter's UI with an opaque rect. On other platforms Flutter UIs are expected to blend with other backgrounds transparently. Is it safe to assume that's never the case here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is safe to assume. I think we would like to have the same blending on Fuchsia, but unfortunately Scenic doesn't support that at the moment; you get a black square if you try

if (root_layer_->needs_system_composite()) {
root_layer_->UpdateScene(context);
}
Expand Down
1 change: 1 addition & 0 deletions flow/layers/layer_tree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "flutter/fml/macros.h"
#include "flutter/fml/time/time_delta.h"
#include "third_party/skia/include/core/SkPicture.h"
#include "third_party/skia/include/core/SkSize.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Extra import?


namespace flutter {

Expand Down
Loading