-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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.
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
[Impeller] grow glyph atlas instead of resizing when rect packer is full. #52849
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
I need to test the overflow behavior, I think I can do that just by creating giant glyphs. I'm also expecting many tests to fail, so lets see... |
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. |
3c36c6c
to
a569767
Compare
Golden file changes are available for triage from new commit, Click here to view. |
Couldn't it also expand horizontally when the max atlas size is reached (for instance 4096) and use a height-width adjustment? Then it could be 4096x4096. |
definitely, we could keep expanding it in that direction. Initially I was trying to keep the same skyline rect packer so I wanted one dimension to stay constant for ease of use but I dropped that. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry @chinmaygarde , you're it |
Why clear the texture? The blit is going to achieve the same thing. And if not, then it indicates we are sampling from outside of the glyph region. |
We do sample from outside the glyph region when the glyph is rotated/skewed. I tried clearing a 1px border around each glyph too but that still showed leaks. We have bugs in our glyph rendering logic :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't grasp point 5 of the strategy. But the implementation of the stated strategy lgtm.
BufferView buffer_view = | ||
host_buffer.Emplace(nullptr, byte_size, DefaultUniformAlignment()); | ||
|
||
::memset(buffer_view.buffer->OnGetContents() + buffer_view.range.offset, 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear about why this is necessary as the blit will clear the contents and we shouldn't be sampling from locations that aren't cleared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this is necessary, let's add a trace around this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
// Because we can't grow the skyline packer horizontally, pick a reasonable | ||
// large width for all atlases. | ||
static constexpr auto kAtlasWidth = 4096u; | ||
static constexpr auto kMinAtlasHeight = 1024; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Missing u
. Especially since you are also using auto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually these should be signed since we pass them to ISize. Will remove auto.
lol, fair. |
Added more context to pt 5. What this does is allow us to preserve all previously rendered glyphs without using any additional software rasterization |
Golden file changes are available for triage from new commit, Click here to view. |
…148529) flutter/engine@8d1a1d8...d575e51 2024-05-17 skia-flutter-autoroll@skia.org Roll Dart SDK from a68a3967e3d9 to ecc0d32e29cd (1 revision) (flutter/engine#52893) 2024-05-17 jonahwilliams@google.com [Impeller] grow glyph atlas instead of resizing when rect packer is full. (flutter/engine#52849) 2024-05-16 jonahwilliams@google.com [Impeller] use smaller SkFont size for determining glyph bounds. (flutter/engine#52868) 2024-05-16 skia-flutter-autoroll@skia.org Roll Skia from 7536d3b590b8 to 5f094e6de863 (2 revisions) (flutter/engine#52891) 2024-05-16 skia-flutter-autoroll@skia.org Roll Dart SDK from d174ead78d98 to a68a3967e3d9 (1 revision) (flutter/engine#52889) 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 bdero@google.com,rmistry@google.com,zra@google.com 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
Fixes flutter/flutter#138798
As far as I can tell, this gets us about as good as Skia, modulo the actual cost of rasterizing one of these glyphs is much more expensive for us.
The basic strategy is: