Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Impeller] grow glyph atlas instead of resizing when rect packer is full. #52849

Merged
merged 12 commits into from
May 17, 2024
Merged

[Impeller] grow glyph atlas instead of resizing when rect packer is full. #52849

merged 12 commits into from
May 17, 2024
+242 −253

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented May 15, 2024 •
edited

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:

  1. If the existing atlas texture has room (defined by the rect packer addRect call succeeding) append as many glyphs as we can to the old texture.
  2. Check if there were any remaining glyphs. If not return.
  3. Otherwise, double the size of the texture and create a new rect packer that represents only the increased area. So for example if you doubled a 100x100 texture to 100x200, then you'd create another 100x100 rect packer, assuming the old one is full.
  4. Grow this size until all remaining glyphs fit. Store the new rect packer and the "height adjustment", which basically tells you where along the y-axis the rect packer offset starts.
  5. Allocate this new texture, clear it, and then blit the contents of the old texture into the top left corner. Append all additional glyphs. The allows us to recycle all previously rendered glyphs and keep them in the texture, reducing the amount of software rasterization per frame.
  6. Note; if the max atlas size would be exceeded, throw it away and start again.

@flutter-dashboard
Copy link

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.

@jonahwilliams
Copy link
Member Author

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...

@flutter-dashboard
Copy link

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.

Changes reported for pull request #52849 at sha fbbe668

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #52849 at sha d34694a

@cmkweber
Copy link

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.

@jonahwilliams
Copy link
Member Author

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.

@jonahwilliams

This comment was marked as resolved.

Sign in to view
@jonahwilliams
Copy link
Member Author

Sorry @chinmaygarde , you're it

@chinmaygarde
Copy link
Member

Allocate this new texture, clear it, and then blit the contents of the old texture into the top left corner.

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.

@jonahwilliams
Copy link
Member Author

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 :(

chinmaygarde
chinmaygarde approved these changes May 16, 2024
View reviewed changes
Copy link
Member

@chinmaygarde chinmaygarde left a 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.

impeller/typographer/backends/skia/typographer_context_skia.cc Outdated
BufferView buffer_view =
host_buffer.Emplace(nullptr, byte_size, DefaultUniformAlignment());

::memset(buffer_view.buffer->OnGetContents() + buffer_view.range.offset, 0,
Copy link
Member

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.

Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

impeller/typographer/backends/skia/typographer_context_skia.cc Outdated
// 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;
Copy link
Member

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.

Copy link
Member Author

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.

@chinmaygarde
Copy link
Member

We have bugs in our glyph rendering logic :(

lol, fair.

@jonahwilliams
Copy link
Member Author

Added more context to pt 5. What this does is allow us to preserve all previously rendered glyphs without using any additional software rasterization

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label May 17, 2024
@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #52849 at sha d7bfa23

@auto-submit auto-submit bot merged commit 7e87cfd into flutter:main May 17, 2024
29 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 17, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 17, 2024
@engine-flutter-autoroll
7e87cfd35 [Impeller] grow glyph atlas instead of resizing when rect p…
aa19ef0 
…acker is full. (flutter/engine#52849)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 17, 2024
@engine-flutter-autoroll
Roll Flutter Engine from 8d1a1d8d7b48 to d575e511f9e5 (5 revisions) (#…
7f32df4 
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chinmaygarde chinmaygarde chinmaygarde approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller will affect goldens
Projects
None yet
Milestone
No milestone
3 participants
@jonahwilliams @cmkweber @chinmaygarde