vello_hybrid: Add inline Pixmap rendering#1459
vello_hybrid: Add inline Pixmap rendering#1459grebmeg wants to merge 1 commit intogemberg/glyph-cache-hybrid-supportfrom
Conversation
grebmeg
force-pushed
the
gemberg/glyph-cache-pixmap-hybrid
branch
from
f494fe8 to
f4f8830
Compare
| Err(e) => { | ||
| log::warn!("Failed to allocate pixmap in atlas: {e:?}"); | ||
| None | ||
| } |
There was a problem hiding this comment.
Should this be a RenderError?
| ImageSource::Pixmap(pixmap) => match self.pixmap_register.get(pixmap) { | ||
| Some(id) => Some(id), | ||
| None => { | ||
| match self.image_cache.allocate(pixmap.width(), pixmap.height()) { |
There was a problem hiding this comment.
How are these sorts of pixmaps de-allocated from the texture?
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
| /// to the GPU atlas on demand. The same `Arc<Pixmap>` is used for two fills in | ||
| /// different locations, verifying that deduplication works (only one atlas upload). |
There was a problem hiding this comment.
"verifying that deduplication works" - is that tested here?
| ImageSource::Pixmap(pixmap) => match self.pixmap_register.get(pixmap) { | ||
| Some(id) => Some(id), | ||
| None => { | ||
| match self.image_cache.allocate(pixmap.width(), pixmap.height()) { | ||
| Ok(id) => { | ||
| self.pixmap_register.insert(pixmap, id); | ||
| self.pending_uploads.push(PendingImageUpload { | ||
| image_id: id, | ||
| pixmap: pixmap.clone(), | ||
| }); | ||
| Some(id) | ||
| } | ||
| Err(e) => { | ||
| log::warn!("Failed to allocate pixmap in atlas: {e:?}"); | ||
| None | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Is there a way to do this without doing 2 lookups on the hash map?
| /// Upload any pixmaps that were allocated during [`Self::prepare_gpu_encoded_paints`] | ||
| /// but haven't been written to the GPU atlas yet. | ||
| fn upload_pending_images(&mut self) { | ||
| let uploads: Vec<_> = self.pending_uploads.drain(..).collect(); |
There was a problem hiding this comment.
Do we need to collect here or can we drain without collecting?
I.e, does this work?
let mut uploads = core::mem::take(&mut self.pending_uploads);
for upload in uploads.drain(..) {
self.write_to_atlas(
device,
queue,
encoder,
upload.image_id,
&upload.pixmap,
None,
);
}
self.pending_uploads = uploads;
| self.programs | ||
| .maybe_resize_atlas_texture_array(&self.gl, self.image_cache.atlas_count() as u32); |
There was a problem hiding this comment.
| self.programs | |
| .maybe_resize_atlas_texture_array(&self.gl, self.image_cache.atlas_count() as u32); |
I think this is called in write_to_atlas
| Programs::maybe_resize_atlas_texture_array( | ||
| device, | ||
| encoder, | ||
| &mut self.programs.resources, | ||
| &self.programs.atlas_bind_group_layout, | ||
| self.image_cache.atlas_count() as u32, | ||
| ); |
There was a problem hiding this comment.
| Programs::maybe_resize_atlas_texture_array( | |
| device, | |
| encoder, | |
| &mut self.programs.resources, | |
| &self.programs.atlas_bind_group_layout, | |
| self.image_cache.atlas_count() as u32, | |
| ); |
I think this is called in write_to_atlas
| let uploads: Vec<_> = self.pending_uploads.drain(..).collect(); | ||
| for upload in uploads { | ||
| Programs::maybe_resize_atlas_texture_array( | ||
| device, | ||
| encoder, | ||
| &mut self.programs.resources, | ||
| &self.programs.atlas_bind_group_layout, | ||
| self.image_cache.atlas_count() as u32, | ||
| ); | ||
| self.write_to_atlas( | ||
| device, | ||
| queue, | ||
| encoder, | ||
| upload.image_id, | ||
| &upload.pixmap, | ||
| None, | ||
| ); | ||
| } |
There was a problem hiding this comment.
| let uploads: Vec<_> = self.pending_uploads.drain(..).collect(); | |
| for upload in uploads { | |
| Programs::maybe_resize_atlas_texture_array( | |
| device, | |
| encoder, | |
| &mut self.programs.resources, | |
| &self.programs.atlas_bind_group_layout, | |
| self.image_cache.atlas_count() as u32, | |
| ); | |
| self.write_to_atlas( | |
| device, | |
| queue, | |
| encoder, | |
| upload.image_id, | |
| &upload.pixmap, | |
| None, | |
| ); | |
| } | |
| let mut uploads = core::mem::take(&mut self.pending_uploads); | |
| for upload in uploads.drain(..) { | |
| self.write_to_atlas( | |
| device, | |
| queue, | |
| encoder, | |
| upload.image_id, | |
| &upload.pixmap, | |
| None, | |
| ); | |
| } | |
| self.pending_uploads = uploads; |
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?
|
Echoing Taj's comment, I would like to understand the motivation further before looking at the code more deeply. |
3 participants
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.