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

Conversation

flar
Copy link
Contributor

@flar flar commented Aug 24, 2023

@flar
Copy link
Contributor Author

flar commented Aug 24, 2023

The original was reverted due to rendering issues with some Flare assets. The problem was that a layer could decide that it was a "NOP" due to the paint it will use to blend in with the scene - but clip and transform methods could also become a NOP due to setting the transform or clip to empty/non-invertible. Both conditions can happen independently and so neither should override the other's conclusion, but that is exactly what the clip/transform methods were doing.

The fix is in the last commmit - only have clip/transform methods set the NOP flag to true when they cause a NOP condition. It can only become false on a restore that ends the NOP layer.

@flar
Copy link
Contributor Author

flar commented Aug 24, 2023

Verified with internal testing that the project that discovered the bug is now working with this fix.

@flar flar requested review from dnfield and chinmaygarde August 24, 2023 23:08
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

RSLGTM

@flar flar added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 25, 2023
@auto-submit auto-submit bot merged commit 1382d6d into flutter:main Aug 25, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 25, 2023
zanderso pushed a commit to flutter/flutter that referenced this pull request Aug 25, 2023
…133298)

flutter/engine@b8ec4da...1382d6d

2023-08-25 [email protected] Reland "Split DisplayListBuilder into
DlCanvas optimizer and DlOp recorder classes #44718"
(flutter/engine#45085)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the
revert to ensure that a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@flar
Copy link
Contributor Author

flar commented Aug 27, 2023

Some benchmark improvements from relanding this work:

@chinmaygarde
Copy link
Member

chinmaygarde commented Aug 28, 2023

Awesome. I am assuming the preview dip was from the last attempt that got reverted?
Screenshot 2023-08-28 at 1 10 21 PM

@flar
Copy link
Contributor Author

flar commented Aug 28, 2023

Awesome. I am assuming the preview dip was from the last attempt that got reverted?

Yes, it does line up perfectly with those commits.

@flar
Copy link
Contributor Author

flar commented Aug 28, 2023

My guess is that I removed some cases where I avoided an extra "pointer to shared_ptr" copy that was independent from the actual work being done in this PR. It could have been patched in independently.

(DlPaint has both Foo* getFooPtr() and shared_ptr getFoo() methods as well as set-by-pointer and set-by-shared equivalents. When you transfer a value from one paint to another using the Ptr versions you end up needing to create a copy rather than sharing the value. I found a couple of those in the builder code while I was implementing this change.)

gaaclarke pushed a commit to gaaclarke/engine that referenced this pull request Aug 30, 2023
zanderso added a commit to zanderso/engine that referenced this pull request Aug 31, 2023
@flar flar mentioned this pull request Sep 8, 2023
XilaiZhang added a commit to XilaiZhang/engine that referenced this pull request Sep 11, 2023
XilaiZhang added a commit to XilaiZhang/engine that referenced this pull request Sep 11, 2023
XilaiZhang added a commit that referenced this pull request Sep 11, 2023
…as optimizer and … (#45680)

…DlOp recorder classes #44718" (#45085)"

This reverts commit
1382d6d.

context: b/299886812

manual revert. the revert in main contains two commits and cannot be
cherry picked cleanly.
XilaiZhang added a commit that referenced this pull request Sep 11, 2023
…s optimizer and … (#45678)

…DlOp recorder classes #44718" (#45085)"

This reverts commit 1382d6d.

context: b/299837297

manual revert. the revert in main contains two commits and cannot be
cherry picked cleanly.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Reland "Split DisplayListBuilder into DlCanvas optimizer and DlOp recorder classes".
3 participants