[Impeller] refactor clip rendering.#56030
Conversation
impeller/display_list/canvas.cc
Outdated
| geometry.IsAxisAlignedRect() && | ||
| GetCurrentTransform().IsTranslationScaleOnly()); |
There was a problem hiding this comment.
nit: /*is_axis_aligned_rect=*/
| SetClipScissor(clip_coverage_stack_.CurrentClipCoverage(), | ||
| *render_passes_.back().inline_pass_context->GetRenderPass(), | ||
| GetGlobalPassPosition()); |
There was a problem hiding this comment.
Just food for thought -- I think we could skip rendering clips when all the following are true:
- geometry.IsAxisAlignedRect()
- GetCurrentTransform().IsTranslationScaleOnly()
- geometry screen space coverage is all integers +- epsilon
My guess is this would be pretty common skip, since anecdotally most clips added by the framework end up being integer translated integer rectangles, and on high DPI devices we just scale up the transform 3x.
There was a problem hiding this comment.
There are a lot of devices with something like a 2.65 DPI, but for the integrer variants yeah. Will do in follow up?
There was a problem hiding this comment.
Eh rounded rect clips are pretty common too. So maybe not most clips, but...
There was a problem hiding this comment.
a lot of the "to be safe" clips are clip rects set to hardEdge
Actually, we could honor the hardEdge setting for axis aligned and discard the factional coverage?
There was a problem hiding this comment.
There are a lot of devices with something like a 2.65 DPI, but for the integrer variants yeah. Will do in follow up?
Yeah of course, just thinking aloud. No need to do it in this patch.
a lot of the "to be safe" clips are clip rects set to hardEdge
Actually, we could honor the hardEdge setting for axis aligned and discard the factional coverage?
That seems like a good idea. From the descriptions here and here, it sounds like hardEdge clips should always round inwards on all sides to intentionally avoid anti-aliasing on long box edges?
There was a problem hiding this comment.
not sure if round in or round out. The SkCanvas API just has a bool doAntiAlias
jonahwilliams
added
the
autosubmit
flutter/engine@2d3f0f4...50f6d98 2024-10-22 [email protected] [Impeller] refactor clip rendering. (flutter/engine#56030) 2024-10-22 [email protected] Fix macos xcprivacy manifest copy location (flutter/engine#56010) 2024-10-22 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Copy gen_snapshots using python's shutil.copy, avoid links (#55830)" (flutter/engine#56034) 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] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…7389) flutter/engine@2d3f0f4...50f6d98 2024-10-22 [email protected] [Impeller] refactor clip rendering. (flutter/engine#56030) 2024-10-22 [email protected] Fix macos xcprivacy manifest copy location (flutter/engine#56010) 2024-10-22 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Copy gen_snapshots using python's shutil.copy, avoid links (flutter#55830)" (flutter/engine#56034) 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] 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Now that we don't have the entity pass structure, clips can be treated as a kind of structured data and not a generic entity. Doing so lets us simplify some of the clip management and remove some usage of raw pointers. This change also removes the ClipRestoreContents, which are only ever created and then immediately rendered - we can instead replace this with a function. The ClipCoverageStack now has explicit Clip/Restore methods that internally apply the same logic, but without involving the entity object. Redo of flutter/engine#55784
2 participants
Now that we don't have the entity pass structure, clips can be treated as a kind of structured data and not a generic entity. Doing so lets us simplify some of the clip management and remove some usage of raw pointers. This change also removes the ClipRestoreContents, which are only ever created and then immediately rendered - we can instead replace this with a function.
The ClipCoverageStack now has explicit Clip/Restore methods that internally apply the same logic, but without involving the entity object.
Redo of #55784