Use RenderContext to own resource lifecycle outside of scene rendering

[taj-p]

Refactors RenderContext from being owned by renderer internals into a standalone, consumer-owned object passed by reference. This makes resource (image) lifetime management explicit and removes the burden of managing resource maps and ID counters from consumers.

Previously, Vello Hybrid would leak images. This changes enables consumers for explicitly managing image resource lifecycle.

Design Decisions

Deferred vs Eager Image Upload

There is a tension with the initial implementation and the introduction of RenderContext. The anyrender_svg crate was uploading images to the GPU as soon as it encountered an image node. It is likely preferential to bulk/batch upload images to the GPU rather than creating a per-image command encoder.

For now, we've stuck with the eager strategy, but I imagine there's an opportunity here to iterate the design to support deferred upload. I think this is a step in the right direction, regardless.

It should be noted that the current design supports both eager and deferred upload. To defer upload, simply register_image in Vello Hybrid prior to rendering some scene.

[nicoburns]

This is not a fully comphrensive review, but I've left some thoughts

[nicoburns]

I think this ought to check whether the image is already registered (using the ImageData's id as a key). And avoid re-registering it in that case. That would save (each time an image is re-rendered):

• An expensive premultiply and reallocate for Vello CPU
• Texture upload for Vello Hybrid
• Conversion to SkImage for Skia

[taj-p]

That would save (each time an image is re-rendered):

This part isn't quite true. This would only occur if a caller re-registers the same image duplicately - not for every render. The caller shouldn't be calling these duplicately (and to do so might imply incorrect usage).

Happy to add in a defensive layer. From my perspective of using AnyRender for testing/benchmarking, it doesn't impact my usage.

[nicoburns]

Ah, yes I'd read this wrong. I see that the resource map is only being included in the ScenePainter to map back the other way. And this could go away (at least for the renderers we control) if we implement this mapping (/resource registration) in the underlying renderers.

[nicoburns]

Would it make sense to have the WindowRenderer/ImageRenderer own the impl RenderContext rather than passing it in here? Then (with the current design), it wouldn't need to be in the API of these traits at all.

[nicoburns]

Then (with the current design), it wouldn't need to be in the API of these traits at all.

Having said that, I wonder whether the "draw callback" (FnOnce(&mut Self::ScenePainter<'_>)) ought to be bounded by PaintScene + RenderContext rather than just PaintScene. Then it would be on callers to register resources rather than something handled internally by the ScenePainters...

But maybe that doesn't make sense if you then need a type that impl's PaintScene + RenderContext anyway?

[taj-p]

I think I might not understand this comment. Are we asking whether it makes sense for RenderContext to be implemented by WindowRenderer and ImageRenderer like:

pub trait WindowRenderer: RenderContext {
    ...
    fn render<F: FnOnce(&mut Self::ScenePainter<'_>)>(&mut self, draw_fn: F);
}

pub trait ImageRenderer: RenderContext {
    ...
    fn render<F: FnOnce(&mut Self::ScenePainter<'_>)>(&mut self, draw_fn: F, buffer: &mut [u8]);
}

// And, for Skia:

pub struct SkiaImageRenderer {
    ...
    ctx: SkiaRenderContext,
}

impl RenderContext for SkiaImageRenderer {
    fn register_image(&mut self, image: ImageData) -> ImageResource {
        self.ctx.register_image(image)
    }

    fn unregister_resource(&mut self, id: ResourceId) {
        self.ctx.unregister_resource(id)
    }
}

impl ImageRenderer for SkiaImageRenderer {

    fn new(width: u32, height: u32) -> Self {
        ...

        Self {
            ...
            ctx: SkiaRenderContext::new(),
        }
    }
}

[nicoburns]

That's what I was asking, yes.

[nicoburns]

In the current design I don't think draw_fn has access to the impl RenderContext at all (I think that's what lead me to think that the impl PaintScene was registering the resources). One way or another, I would expect the draw_fn to be passed the impl RenderContext parameter (and be responsible for registering the resources).

To be concrete: In Blitz, the draw_fn corresponds to a very thin (1 line) wrapper (which does have the possibility of doing closure capture is needed) around the blitz-paint crate which does interop between blitz-dom and anyrender (and is generally responsible for transforming a styled and layouted documen

[taj-p]

Yup - makes sense.

Can you let me know if bbfa9e1 is what you meant? If so, I can merge with main and do a re-review of the code tomorrow morning before re-requesting review