Open
Conversation
grebmeg
force-pushed
the
gemberg/glyph-cache-pixmap-hybrid
branch
from
f494fe8 to
f4f8830
Compare
taj-p
reviewed
Feb 20, 2026
taj-p
reviewed
Feb 20, 2026
Contributor
taj-p
left a comment
taj-p
left a comment
There was a problem hiding this comment.
Code looks good! Just asking some questions for my understanding
Contributor
There was a problem hiding this comment.
For what types of use cases would we use this over asking the caller to manage the lifetime of pixmaps?
I'm a little wary of using this API because it means the caller needs to keep the pixmap in memory - is that right? The caller can't deallocate the pixmap if they pass it as an ImageSource::Pixmap because the next time they use it, they need to pass the Arc<Pixmap>? Or is there a certain use case for glyph caching that this is used?
Collaborator
Author
There was a problem hiding this comment.
Echoing Taj's comment, I would like to understand the motivation further before looking at the code more deeply.
I added this code with the bitmap glyphs case in mind, specifically when caching is disabled, so that instead of panicking now we can at least render something. Manually uploading all glyphs is also a tedious step, and ideally that should be hidden if possible especially for glyphs.
That said, it now seems like we’ve reached a complete model here. I do agree with your concern though, the caller would still need to keep pixmaps in memory, similar to the CPU case when the glyph cache is disabled. But this current implementation can be considered the least recommended option and a starting point, so that we can evolve it further if needed.
Collaborator
|
Echoing Taj's comment, I would like to understand the motivation further before looking at the code more deeply. |
grebmeg
force-pushed
the
gemberg/glyph-cache-hybrid-support
branch
4 times, most recently
from
0284ce9 to
b861095
Compare
grebmeg
force-pushed
the
gemberg/glyph-cache-pixmap-hybrid
branch
from
f4f8830 to
9685e59
Compare
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Apr 14, 2026
# Overview This PR attempts to integrate glifo into vello_hybrid and vello_cpu. As mentioned, the git diff is a bit messy because 1) it's based on an old version of main and 2) some commits that are needed have been cherry-picked, bust should ideally be merged separately (i.e. the other currently open PRs). # Blockers PRs I’d consider blocking are: ~#1553~ ~#1555~ Semi-blocking is also #1459, but for now we could just accept that (non-cached) bitmap glyphs panic in vello_hybrid and not render them for now. # Limitations / TODOs Since this PR by itself is already big by itself, it still some TODOs and definitely doesn’t represent the ideal end state we want to have: - glifo still has the dependency on vello_common. - While I agree that there are probably improvements that can be made to the API (see the [Zulip thread](https://xi.zulipchat.com/#narrow/channel/197075-vello/topic/Hooking.20up.20glifo.20in.20vello_hybrid.2Fvello_cpu/with/583030800)), I have not investigated those yet and just implemented the minimum necessary to get tests to run. For now, core text rendering logic still lives in vello_hybrid/vello_cpu instead of an external crate or somewhere else. I also haven’t introduced any “render from sub-rect of atlas” API, instead just sticking as closely as possible to what Alex previously implemented. Some other TODOs: - When caching is enabled, zooming is currently extremely slow. Looking at the profile, it becomes clear that this is because deallocating and reallocating cached glyphs with guillotierre becomes a huge bottleneck with many glyphs. So we either need to explore using etagere for that, or investigate whether there is something else that causes the issue in our case. See also the video: https://github.com/user-attachments/assets/1692d83f-29e9-436f-9c0b-2a0af2445bd5 - There also seems to be some other bug where some glyphs become corrupted at certain zoom levels, but it's not easily reproducible. I am not sure yet whether this is a regression introduced in this PR or already existed previously. If I find the time I might piggy-back a fix into this PR, otherwise it will be a follow-up: https://github.com/user-attachments/assets/33ec8245-922b-47d6-b14f-314f11a19217 - It seems like when caching is enabled, the glyphs have some kind of funny "jump" when panning the viewport. I suspect that the subpixel offsets are somehow applied inversely, but I need to look into it. **EDIT: Nevermind, I realized that the answer is very simple, it's because we don't do any subpixel rendering in the y direction at all. Only in the x direction.** https://github.com/user-attachments/assets/5c847d29-0f77-4620-b669-36742ec62604 So overall, there are definitely more things to investigate, but as you can see the integration seems to work overall! # Changes - The biggest change is probably to resources handling. Both renderers now require the user to pass a `Resources` (open for better name suggestions) for certain operations (anything text-related on the scene, and the final rendering operation). In vello_cpu, this basically only contains stuff relevant for glyph caching. In vello_hybrid, this additionally now contains the image cache, which previously lived in the “Renderer”. This allows us to share resources between scene encoding and the final rendering operation. The disadvantage is that it puts the burden on the user to always pass the correct resources, but hopefully this shouldn’t be too hard to get wrong. I don’t really see any better way of handling this otherwise. Perhaps this API can in the future be extended to allow the user passing custom resources (e.g. custom texture views for sampling). In vello_cpu, we could _probably_ get away with hiding this behind `RenderContext`, but it also gets a bit messy and I think having this explicit consistently is better. - The default size of an atlas on Vello CPU was changed to 4096, I hope that’s a reasonable default? Or should it be smaller? - I’ve made slight changes to how the glyph run builder API works. In the previous parley version, GlyphRunBuilder was a simple struct with a builder pattern, where you needed to manually the image cache and glyph caches to the method. In order to make it work better with the `Resources` struct used by both renders, I’ve made the builder generic over a backend. This way, the user simply has to call `glyph_run` once, passing the resources to it, and the rest then happens automatically. - The previous benchmarks had some benches for measuring specific parts of the pipeline (e.g. maintain), since those are now more hidden away it’s not easy to benchmark anymore. Therefore, I’ve changed the benchmarks to simply be “integration” benchmarks for Vello CPU. - I split up the previous `GlyphCaches` struct into a separate struct for outline/hint caches and atlas caching. The reason for this change is that I want it to be possible to lazily initialize glyph atlases and glyph renderers (especially in vello_cpu), such that the user doesn’t have to pay the cost for constructing those and keeping them in memory, if they don’t make use of the feature at all. This required making it easier to pass no atlas cache to all of the glyph drawing methods, and splitting those two up seemed like the easiest way. This way, you can still use the outline/hint cache even if atlas caching itself is disabled. - I’m not sure what the best way of handling this is, but obviously we want to exercise the caching path in the tests. Unfortunately, caching simply can result in quite large pixel differences, so reusing existing snapshots is not really an option, and we need dedicated ones for new ones. Right now, I went down the path of simply adding an attribute that parametrizes the test on whether glyph caching should be enabled or not. I’m not sure if the better option here is to just manually hardcode the variants for each test, or something else. Open for suggestions here! # Other Notes - Something that make things awkward are recordings. Ideally, glifo would take care of handling and re-exporting all text-related dependencies, and vello_common doesn’t have any of that. However, since the `Recording` type lives in vello_common and we need to have the abilities to replay glyph drawing instructions, some of it _has_ to live in vello_common for now. In particular, I had to move the `Glyph` type to vello_common, in the long term this should probably move back to glifo. - To expand on that, the fact that we have recordings makes things even harder. On current main, we are able to record normal outline glyphs but will panic for COLR and bitmap glyphs. However that exact setup is unfortunately difficult to replicate now. The reason being that the version currently on main relies on the ability to lower glyph runs into strip commands directly in vello_common. However, now that logic lives in glifo, and vello_common can’t take a dependency on glifo. Therefore, for now I’ve opted to simply record glyph render commmands as they are, but not actually provide any caching. - For similar reasons, we need to duplicate the glyph run builder API in vello_common, which is also not ideal, but I wouldn't know how to deal with that better. --------- Co-authored-by: Laurenz Stampfl <laurenz@canva.com> Co-authored-by: Alex Gemberg <gemberg@canva.com> Co-authored-by: Taj Pereira <taj@canva.com>
taj-p
requested changes
Apr 19, 2026
Contributor
There was a problem hiding this comment.
In office hours, we discussed the direction of Vello Hybrid and textures. It seems like the direction is towards consumers owning textures, atlasing, and lifetimes of images in those atlases (moving ownership out of hybrid and to the caller).
At the same time, the direction for glifo seems to be further decoupling from renderers.
When you put these 2 together, I think this PR, which evolves the image cache and hybrid to own more of texture atlasing and lifetimes, conflicts with that direction.
Collaborator
|
Does that mean the user would be responsible for manually uploading and destroying the bitmaps for bitmap glyphs? I think that would be a bit unfortunate. |
Contributor
No. It would be glifo's responsibility |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 PR adds support for rendering
Arc<Pixmap>images directly viaImageSource::Pixmapinvello_hybrid, without requiring callers to manually pre-upload them. The renderer automatically deduplicates pixmaps by pointer identity across frames and lazily uploads them to the GPU atlas on demand, with periodic eviction of stale entries.