This repository was archived by the owner on Feb 25, 2025. It is now read-only.
[Impeller] Reland DlAiksCanvas, again. #45386
Closed
Conversation
This file contains hidden or 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
This reverts commit 7a2eaf4.
jonahwilliams
approved these changes
Sep 1, 2023
|
This needs to pull in latest main |
This comment was marked as resolved.
This comment was marked as resolved.
|
It looks like opacity isn't working in a number of cases. This might be a paint pass delegate issue? or some case where the subpass expects the parent to apply the opacity but it doesn't? Also see the gallery app, the shrine transitions and page transitions don't show any opacity. |
|
Something funky with the page transitions too: foo.mp4 |
chinmaygarde
changed the title
|
The Perf overlay was unrelated, I'll help look at the opacity issues next week. |
|
One possibility is that the picture is supposed to be drawn iwth opacity and right now we're always treating it as one in the DrawImpellerPicture call.. |
|
There were some other regressions on the tessellation benchmarks on this change too. I think this is generally related to us computing the bounds of paths many times, so I opened #45456 to address that. |
jonahwilliams
removed
the
autosubmit
jonahwilliams
suggested changes
Sep 5, 2023
|
Ah, misunderstood what correctness issues remained. Are there any issues filed? |
|
Still trying to figure it out, but I think entitypass clone is still buggy |
|
I verified this locally and found it might have been due to an interaction with the DL split, but trying again on ToT I don't see any issues. |
|
Actually we just found one issue. Investigating. |
|
Looking at wonderous, the icon in the top left is fading out the text too soon/too late on the main page - the advanced blends also look off. This could be an issue with |
auto-submit bot
pushed a commit
that referenced
this pull request
Sep 6, 2023
Conversion of SkTextBlobs to impeller::TextFrame objects is one of the most expensive operations in display list dispatching. While the rest of the engine and framework makes a reasonable attempt to cache the SkTextBlobs generated during paragraph construction, the design of the dl dispatcher means that these the Impeller backend will always reconstruct all text frames on each frame - even if the display list/picture that contained those text frames was unchanged. Removing this overhead is one of the goals of #45386 , however this patch is also fairly risky and will be difficult to land. As a more incremental solution, we can instead construct the impeller::TextFrame objects when performing paragraph painting and record them in the display list. This both moves the text frame construction to the UI thread and allows the framework/engine to cache unchanged text frames. This also does not conflict with the dl_aiks_canvas patch directly, and is fine to land before or after it does. (though I'd argue we should land this first). To compare the current performance levels, I ran the complex_layout_scroll perf test, since this is fairly text filled. On a Pixel 6 pro. Across several runs this is a fairly consistent ~1ms raster time improvement. ### Skia ``` "average_frame_build_time_millis": 1.497333333333333, "90th_percentile_frame_build_time_millis": 2.038, "99th_percentile_frame_build_time_millis": 17.686, "worst_frame_build_time_millis": 23.095, "missed_frame_build_budget_count": 3, "average_frame_rasterizer_time_millis": 5.5078589743589745, "stddev_frame_rasterizer_time_millis": 2.226343414420338, "90th_percentile_frame_rasterizer_time_millis": 7.481, "99th_percentile_frame_rasterizer_time_millis": 19.11, "worst_frame_rasterizer_time_millis": 79.799, "missed_frame_rasterizer_budget_count": 7, "frame_count": 234, "frame_rasterizer_count": 234, "new_gen_gc_count": 10, "old_gen_gc_count": 2, ``` ### Impeller (ToT) ``` "average_frame_build_time_millis": 1.431575000000001, "90th_percentile_frame_build_time_millis": 2.196, "99th_percentile_frame_build_time_millis": 14.486, "worst_frame_build_time_millis": 23.728, "missed_frame_build_budget_count": 2, "average_frame_rasterizer_time_millis": 6.536087499999999, "stddev_frame_rasterizer_time_millis": 1.9902712500000004, "90th_percentile_frame_rasterizer_time_millis": 9.705, "99th_percentile_frame_rasterizer_time_millis": 14.727, "worst_frame_rasterizer_time_millis": 17.838, "missed_frame_rasterizer_budget_count": 1, "frame_count": 240, "frame_rasterizer_count": 240, "new_gen_gc_count": 10, "old_gen_gc_count": 2, ``` ### Impeller (Patched) ``` "average_frame_build_time_millis": 1.4500167364016743, "90th_percentile_frame_build_time_millis": 2.478, "99th_percentile_frame_build_time_millis": 14.883, "worst_frame_build_time_millis": 18.782, "missed_frame_build_budget_count": 1, "average_frame_rasterizer_time_millis": 5.023033333333336, "stddev_frame_rasterizer_time_millis": 1.6445388888888894, "90th_percentile_frame_rasterizer_time_millis": 7.814, "99th_percentile_frame_rasterizer_time_millis": 13.497, "worst_frame_rasterizer_time_millis": 15.008, "missed_frame_rasterizer_budget_count": 0, "frame_count": 239, "frame_rasterizer_count": 240, "new_gen_gc_count": 8, "old_gen_gc_count": 0, ```
auto-submit bot
pushed a commit
that referenced
this pull request
Sep 22, 2023
Due to flutter/flutter#127500 , we can get in a state where enable-impeller is true but we're using Skia. We need to either fall back completely to Skia, make this configuration fatal, or remote the check ---------------- Conversion of SkTextBlobs to impeller::TextFrame objects is one of the most expensive operations in display list dispatching. While the rest of the engine and framework makes a reasonable attempt to cache the SkTextBlobs generated during paragraph construction, the design of the dl dispatcher means that these the Impeller backend will always reconstruct all text frames on each frame - even if the display list/picture that contained those text frames was unchanged. Removing this overhead is one of the goals of #45386 , however this patch is also fairly risky and will be difficult to land. As a more incremental solution, we can instead construct the impeller::TextFrame objects when performing paragraph painting and record them in the display list. This both moves the text frame construction to the UI thread and allows the framework/engine to cache unchanged text frames. This also does not conflict with the dl_aiks_canvas patch directly, and is fine to land before or after it does. (though I'd argue we should land this first). To compare the current performance levels, I ran the complex_layout_scroll perf test, since this is fairly text filled. On a Pixel 6 pro. Across several runs this is a fairly consistent ~1ms raster time improvement. Fixes flutter/flutter#133204
harryterkelsen
pushed a commit
that referenced
this pull request
Oct 23, 2023
Due to flutter/flutter#127500 , we can get in a state where enable-impeller is true but we're using Skia. We need to either fall back completely to Skia, make this configuration fatal, or remote the check ---------------- Conversion of SkTextBlobs to impeller::TextFrame objects is one of the most expensive operations in display list dispatching. While the rest of the engine and framework makes a reasonable attempt to cache the SkTextBlobs generated during paragraph construction, the design of the dl dispatcher means that these the Impeller backend will always reconstruct all text frames on each frame - even if the display list/picture that contained those text frames was unchanged. Removing this overhead is one of the goals of #45386 , however this patch is also fairly risky and will be difficult to land. As a more incremental solution, we can instead construct the impeller::TextFrame objects when performing paragraph painting and record them in the display list. This both moves the text frame construction to the UI thread and allows the framework/engine to cache unchanged text frames. This also does not conflict with the dl_aiks_canvas patch directly, and is fine to land before or after it does. (though I'd argue we should land this first). To compare the current performance levels, I ran the complex_layout_scroll perf test, since this is fairly text filled. On a Pixel 6 pro. Across several runs this is a fairly consistent ~1ms raster time improvement. Fixes flutter/flutter#133204
|
This pretty stale, going to close for now. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This is a partial revert of #45326.
The DlAiksCanvas implementation was reverted there because it caused merge conflicts. I've fixed those up.
This did introduce some regressions that are fixed by #45313
This also depends on #45355 to avoid dangling pointer issues with
DrawVertices.