vello_hybrid: Deallocate filter regions at the end of each frame instead of the start of next#1734
vello_hybrid: Deallocate filter regions at the end of each frame instead of the start of next#1734LaurenzV wants to merge 1 commit into
Conversation
| let canvas = web_sys::window() | ||
| .and_then(|window| window.document()) | ||
| .and_then(|document| document.create_element("canvas").ok()) | ||
| .and_then(|element| element.dyn_into::<HtmlCanvasElement>().ok()) | ||
| .ok_or(WebGlProbeError::CanvasCreationError)?; |
There was a problem hiding this comment.
@taj-p Is this fine to do? Can we assume that this will always yield a canvas from our side?
2506488 to
600b43f
Compare
| let renderer = Self::new_with( | ||
| &canvas, | ||
| RenderSettings { | ||
| atlas_config: PROBE_ATLAS_CONFIG, | ||
| ..RenderSettings::default() | ||
| }, | ||
| ); |
There was a problem hiding this comment.
I think this is the biggest concern in this PR, since webgl contexts are a limited resource. You mentioned this will go away with the rewrite, but still would it be worth just adding a temporary bandaid fix?
I can think of a few possible approaches:
- the one you mentioned: add another swap for
filter_textures(it might be the other resources wouldn’t need swapping); - move filter image cache deallocation to the end of
render_scene; - make deallocation unable to free Ids from the wrong cache by associating each cache with a generation or identity.
My concern is that if a user opens a page with multiple vello_hybrid instances running, we could see more webgl context losses, especially since other parts of the page may also consume additional contexts. What do you think?
There was a problem hiding this comment.
Yeah sure, we can do 1) or 2). Not so sure about 3) for now. If we can convince ourselves that 1) is enough, this is probably the easiest. But I would need to take some time to look into whether this is actually enough again.
…f the start of next
600b43f to
2bcf515
Compare
LaurenzV
changed the title
2 participants
Currently, the
probemethod piggybacks an existingWebGlRendererand does somemem::swaps in an effort to not dirty the existing renderer state. Unfortunately, it turns out that this is not enough. Currently, filter handling is written in such a way that at the beginning of the render operation (i.e. when callingrender_scene), we check the filter context and clear/deallocate image regions from the last frame. Obviously, this doesn't work well with probing: If we rendered a filter in the previous frame and are about to render the probe image next, it thinks it still needs to deallocate an image from the last frame, and if the image IDs happen to be the same it will instead deallocate the image that is needed by the probe itself. The fact that filter regions are currently deallocated this way is more than dubious, but I won't bother fixing this now since it will go away with the Vello Hybrid rewrite.However, I also don't want to fix this issue with a bandaid fix by just trying to
mem::swapthe filter context and any other associated state that we missed, since this is still fragile. Therefore, we instead simply makeprobecreate its ownWebGlRenderContextand therefore leave any other existing contexts untouched. I initially didn't do this because I wanted to avoid having to allocate those additional buffers, but I now think that this is the best and cleanest way.As a nice side effect, this also seems to fix the CI flakiness, so we can run the probe in CI again.
I verified on a number of low-tier devices that this does not seem to regress the probe time itself.
This PR was done with assistance of GPT5.6-Sol.