vello: Keep image atlas residency across renders#1558
Conversation
|
As with everything, this was done with AI assistance, but I did review it and we iterated on it some. |
|
This helps address the performance problem described in #1545. |
|
We probably need a way to mark images as dirty (mainly |
raphlinus
left a comment
There was a problem hiding this comment.
I checked this over fairly carefully. Generally, it looks good, but I did have some comments; my biggest single concern is the potential panic in repack_to_size.
Just sketching, there's an alternative to the dirty bool in residents. Rather, you can have a resize_gen for the atlas, incremented on each resize. Instead of dirty, track the resize_gen for which the image was last uploaded. Then in the occupied case of get_or_insert, push & update resize_gen if resize_gen doesn't match. That avoids the full map iteration in restart_resolve_pass and eliminates the need for finish_resolve. I personally find this easier to reason about: the resize_gen in a resident identifies which gen it was uploaded to. But basically comes down to a style issue; I did verify the current logic and am satisfied.
| for (id, mut resident) in entries { | ||
| let alloc = atlas | ||
| .allocate(size2(resident.image.width as _, resident.image.height as _)) | ||
| .expect("resident image must fit after atlas growth"); |
There was a problem hiding this comment.
I'm a little worried about panics here. Does guillotiere give guarantees that allocation will succeed if size is sufficient? It seems like fragmentation etc might cause a failure under reasonable circumstances.
| if self.image_cache.can_fit_image(&pending_image.image) | ||
| && self.image_cache.evict_stale_entries() | ||
| { | ||
| continue 'outer; |
There was a problem hiding this comment.
I think continue 'outer will work here, but isn't necessarily optimal; it should be possible to retry the get_or_insert and move on. But it makes the logic a bit messier, so not sure it's worth changing.
Keep the single-atlas allocator and residency map alive in vello_encoding::ImageCache, reuse resident entries across resolves, and use generation-based stale eviction before atlas growth under allocation pressure.
5e1bc79 to
2ddbc34
Compare
|
@raphlinus I've updated this per your feedback, I think. Want to take a fresh look? |
|
Yup, looks like you addressed all my comments. Thanks! |
Followup to #1558, which caches images. There's a risk that we'll render stale cached date from dynamic external textures, so we provide a method to mark those as dirty.
This changes Vello’s image atlas handling from per-render rebuilds to persistent residency.
Before this series, image atlas placement was recomputed on every resolve, and the renderer recreated and rewrote the atlas texture every render. That meant even steady-state scenes that only moved already-uploaded images still paid repeated atlas upload cost. This series keeps atlas residency metadata alive in
vello_encoding, keeps the atlas texture proxy alive invello, and only uploads images that are newly inserted or dirtied by atlas growth.What changed:
vello_encodingnow keeps the single-atlas allocator and resident image map across resolves.Use this to see the trace log messages: