Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't add empty OpacityLayer to the engine #31520

Merged
merged 3 commits into from
Apr 24, 2019
Merged

Don't add empty OpacityLayer to the engine #31520

merged 3 commits into from
Apr 24, 2019

Conversation

liyuqian
Copy link
Contributor

Fixes #31517

@liyuqian liyuqian requested a review from dnfield April 23, 2019 20:42
dnfield
dnfield approved these changes Apr 23, 2019
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM. I think there's potentially a Fuchsia FL-something open for this, I'll try to find it.

@dnfield
Copy link
Contributor

dnfield commented Apr 23, 2019

Can we document this somehow?

It seems entirely reasonable to say the contract for Opacity is that it must have a child (to keep the layer tree smaller). It may be difficult for consumers to discover that right now though, unless it's documented and I just missed it.

@liyuqian
Copy link
Contributor Author

I'll add docs/comments to the flutter::OpacityLayer in the engine repo. It will be like "Don't push an Opacity layer with no children; painting an Opacity layer is very costly due to the saveLayer call".

@chinmaygarde : is there any other or better place for documenting the engine layers?

@liyuqian liyuqian mentioned this pull request Apr 23, 2019
Merged
dnfield
dnfield reviewed Apr 23, 2019
View reviewed changes
packages/flutter/lib/src/rendering/layer.dart Outdated
// Try to avoid an [OpacityLayer] with no children. Painting an [OpacityLayer]
// in the Flutter engine is very costly due to the creation of an offscreen
// buffer and render target switches. If there's no child, having the
// OpacityLayer or not has the same effect. If [OpacityLayer] has no child, its
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems off to me. Why can't we make it cheap in the engine if there's no child? E.g. if the engine gets an OpacityLayer with no layers, it's a no-op layer.

My understanding is that it's still beneficial to omit such layers from the engine side because it would (slightly) shrink the size of the layer tree and thus the real time it takes to traverse it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean "Try to avoid an [OpacityLayer] with no children. Remove that layer if possible to save some tree walks." ?

I guess that we can make an empty OpacityLayer as a no-op in the engine. We're just not currently doing it, and instead doing FML_DCHECK the nonemptiness.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine for the contract to be Don't send me an empty opacity layer, but the explanation here makes me wonder why that is (e.g. why is it so easy to short-circuit it here and not in the engine?).

Should we be pushing the short-circuiting even further up the call stack to the pushLayer call with opacity? Should we add an assert to OpacityLayer that it has at least one child when it's pushed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Talked offline - let's make this a doc comment (///) and your new text in there should be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doc updated.

@kf6gpe kf6gpe added framework flutter/packages/flutter repository. See also f: labels. engine flutter/engine repository. See also e: labels. and removed framework flutter/packages/flutter repository. See also f: labels. labels Apr 23, 2019
dnfield
dnfield reviewed Apr 24, 2019
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

Would like to get further clarification before submitting this :)

@dnfield dnfield added the framework flutter/packages/flutter repository. See also f: labels. label Apr 24, 2019
@dnfield
Copy link
Contributor

dnfield commented Apr 24, 2019

LGTM once doc is fixed.

@liyuqian liyuqian merged commit e519472 into flutter:master Apr 24, 2019
@liyuqian liyuqian deleted the empty_opacity branch April 29, 2019 17:23
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dnfield dnfield dnfield approved these changes

Assignees
No one assigned
Labels
engine flutter/engine repository. See also e: labels. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Empty opacity crashes with debug_unopt engine
4 participants
@liyuqian @dnfield @kf6gpe @googlebot