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

Wire up Opacity on Fuchsia, round 2 #12984

Closed
wants to merge 11 commits into from
Closed

Wire up Opacity on Fuchsia, round 2 #12984

wants to merge 11 commits into from

Conversation

arbreng
Copy link
Contributor

@arbreng arbreng commented Oct 7, 2019

[fuchsia] Wire up OpacityLayer to Scenic

Remove the opacity exposed from ChildView, as that was added mistakenly.

Make the layer stack explicit in SceneBuilder instead of implicit in Layer, and remove
parent().

On Fuchsia, add a code-path for compositing OpacityLayers using the system
compositor, which exposes a fastpath for opacity via Scenic.
This will only work under certain circumstances, in particular nested
Opacity() widgets will not render correctly!

On Fuchsia, add a code path for compositing PhysicalShapeLayers using
the system compositor, which allows cross-process shadows. Set to off by default,
which restores performant shadows on Fuchsia.

Finally, we centralize the logic for switching between the system-composited
and flutter-composited paths using SystemCompositedContainerLayer. We also
centralize the logic for computing elevation there. This allows the removal of several
OS_FUCHSIA-specific code-paths.

Test: Ran workstation on Fuchsia; ran flow_unittests; ran flutter macrobenchmarks
Bug: https://bugs.fuchsia.dev/p/fuchsia/issues/list/23711
Bug: https://bugs.fuchsia.dev/p/fuchsia/issues/list/24163

@arbreng arbreng requested review from amirh and liyuqian October 7, 2019 22:15
@liyuqian
Copy link
Contributor

liyuqian commented Oct 8, 2019

What are the links to the following bugs?

Bug: 23711
Bug: 24163

Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

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

It might be nice to add some layer unit tests like flow/layers/opacity_layer_unittests.cc for the modified layers (see flow/layers/physical_shape_layer_unittests.cc for an example).

BTW, it might be nice to measure the engine size change by this PR. The device lab test hello_world_android__compile might be handy. (See https://github.com/flutter/flutter/tree/master/dev/devicelab#running-tests-against-a-local-engine-build on how to run it.)

@arbreng
Copy link
Contributor Author

arbreng commented Oct 9, 2019

What are the links to the following bugs?

Bug: 23711
Bug: 24163

Updated CL description

@cbracken
Copy link
Member

@arbreng is this still WIP? Looks like we're waiting on addressing @liyuqian 's comments.

@arbreng
Copy link
Contributor Author

arbreng commented Oct 23, 2019

Was a lot of work just to rebase on 3 weeks worth of changes. Writing tests now

@cbracken
Copy link
Member

cbracken commented Nov 4, 2019

@arbreng is this ready for review, or are you still working on it?

@cbracken cbracken added the Work in progress (WIP) Not ready (yet) for review! label Nov 4, 2019
@arbreng arbreng added affects: engine code health and removed Work in progress (WIP) Not ready (yet) for review! labels Nov 11, 2019
Copy link
Contributor

@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

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

Thanks for adding some excellent tests. Can you add these to flutter_runner_unittests? I'd like us to test system composition.

}
ClearChildren();
Add(new_child);
void OpacityLayer::Add(std::shared_ptr<Layer> layer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add an FML_DCHECK to ensure that this only gets called once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is perfectly legal to call this method N times though; its ContainerLayer::Add that should only be called once. I don't think I can add an FML_DCHECK to that without add a should_have_one_child() virtual, or something similar

@cbracken
Copy link
Member

@arbreng, I assume this is still being worked on?

@arbreng
Copy link
Contributor Author

arbreng commented Nov 26, 2019

@arbreng, I assume this is still being worked on?

Yep! Currently updating this to use the tests from #13986 and fixing the last few review comments

@arbreng
Copy link
Contributor Author

arbreng commented Nov 27, 2019

Closing this PR in favor of #14024 because I'm working out of flutter repo directly now.

I fixed all of iskakaushik's and liyuqian's feedback

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants