Remove unnecessary saveLayer #5420
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
saveLayer is slow so we should avoid it as much as possible. If there are artifacts without saveLayer, then we should report that to Skia for the fix instead of slowing the performance with saveLayer. This PQ makes average rasterizer time drop from 25ms to 18ms in flutter_gallery transitions perf test on a Nexus 5X.
chinmaygarde
approved these changes
May 30, 2018
|
Awesome! Looking forward to the benchmark of the next roll. |
Hixie
reviewed
May 31, 2018
| @@ -48,7 +48,7 @@ void ClipPathLayer::Paint(PaintContext& context) const { | |||
| TRACE_EVENT0("flutter", "ClipPathLayer::Paint"); | |||
| FXL_DCHECK(needs_painting()); | |||
|
|
|||
| Layer::AutoSaveLayer save(context, paint_bounds(), nullptr); | |||
| SkAutoCanvasRestore save(&context.canvas, true); | |||
There was a problem hiding this comment.
This won't work. It will cause artifacts.
See this article for a discussion of why: https://master-docs-flutter-io.firebaseapp.com/flutter/dart-ui/Canvas/saveLayer.html
|
There will definitely be artifacts. For example, if a clip region contains a red rectangle with a white rectangle on top of it, then at the edge of the clip region, this will result in red-tinted pixels, but they should be white. |
|
Interestingly, I just tried this on the beta channel and I get the red outlines already, so while I don't think this PR is correct, it seems something else had already broken this before. |
|
Filed flutter/flutter#18057 to track this. I marked in "regression" because I'm sure this used to work. |
najeira
pushed a commit
to najeira/flutter-engine
that referenced
this pull request
Jun 5, 2018
saveLayer is slow so we should avoid it as much as possible. If there are artifacts without saveLayer, then we should report that to Skia for the fix instead of slowing the performance with saveLayer. This PQ makes average rasterizer time drop from 25ms to 18ms in flutter_gallery transitions perf test on a Nexus 5X. This partially fixes flutter/flutter#13736 . We probably still need some work in the opacity layer to squize all the performance improvements.
liyuqian
added a commit
to liyuqian/flutter
that referenced
this pull request
Jun 20, 2018
This is a follow up on flutter/engine#5420 and flutter#18057 As you can see from the diff, we also mistakenly saveLayer before the clip at some places previously.
liyuqian
added a commit
to flutter/flutter
that referenced
this pull request
Jun 21, 2018
This is a follow up on flutter/engine#5420 and #18057 As you can see from the diff, we also mistakenly saveLayer before the clip at some places previously.
najeira
pushed a commit
to najeira/flutter-engine
that referenced
this pull request
Jul 19, 2018
saveLayer is slow so we should avoid it as much as possible. If there are artifacts without saveLayer, then we should report that to Skia for the fix instead of slowing the performance with saveLayer. This PQ makes average rasterizer time drop from 25ms to 18ms in flutter_gallery transitions perf test on a Nexus 5X. This partially fixes flutter/flutter#13736 . We probably still need some work in the opacity layer to squize all the performance improvements.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
saveLayer is slow so we should avoid it as much as possible. If
there are artifacts without saveLayer, then we should report that
to Skia for the fix instead of slowing the performance with
saveLayer.
This PQ makes average rasterizer time drop from 25ms to 18ms in
flutter_gallery transitions perf test on a Nexus 5X.
This partially fixes flutter/flutter#13736 .
We probably still need some work in the opacity layer to squize
all the performance improvements.