Skip to content

vello_cpu: Unify render APIs#1665

Merged
LaurenzV merged 4 commits into
mainfrom
laurenz/new_render_api
Jun 10, 2026
Merged

vello_cpu: Unify render APIs#1665
LaurenzV merged 4 commits into
mainfrom
laurenz/new_render_api

Conversation

@LaurenzV

@LaurenzV LaurenzV commented May 23, 2026

Copy link
Copy Markdown
Collaborator

Supersedes #1597, closes #1382.

Motivation

Right now, we have two rendering APIs: render_to_pixmap and composite_to_pixmap_at_offset. We are about to add a third one in #1597. Having this many APIs is very confusing, and after thinking about this more closely I realized that really, what we have been doing is adding methods that do similar things but just with different knobs enabled/disabled. I think it would be much cleaner if we had one single API entry point for rendering to a pixmap, and instead expose the available knobs as a simple settings struct.

Implementation

Therefore, this PR proposes to:

  1. Expose one single render method that can be used to render the contents of a RenderContext into Pixmap-like struct.
  2. Introduce a PixmapMut struct to allow constructing a render target from a mutable borrowed byte slice. Let render always take a PixmapMut, allowing to pass both, a custom buffer or an owned pixmap as the render target.
  3. Introduce a CompositeMode enum that defines whether the contents of a pixmap should be completely replaced (by default), or whether source-over compositing should be used.
  4. Also introduce a PixelFormat struct, to possibly allow Bgra8 in the future, which is useful for MacOS.
  5. RenderMode has been moved from being stored in RenderContext to instead being a setting during rasterization. I initially stored this in RenderContext because I thought it might happen that the setting will also be needed during scene construction, but this turned out to not be the case in the end.
  6. Previously, the implicit assumption was that RenderContext and Pixmap always need to have the same size, although it "accidentally" was possible for them to not have the same size, and some people have made use of that. This PR relaxes the conditions imposed on the size, explicitly allowing size mismatches and also documents what the intended semantics are for each.

Testing

I've added some new tests for the expected behavior. Existing tests pass. I've also ran vello_cpu_winit with the different scenes and didn't notice any regressions.

TODO: I still want to run this version against my PDF test suite, but this shouldn't block a review.

@LaurenzV LaurenzV requested review from grebmeg and nicoburns May 23, 2026 09:20
@LaurenzV

Copy link
Copy Markdown
Collaborator Author

@nicoburns If you want to feel free to give it a spin.

@laurenz-canva laurenz-canva force-pushed the laurenz/new_render_api branch from 2846da5 to 5b7d009 Compare May 23, 2026 09:28

@nicoburns nicoburns left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm onboard with the general concept, but I think it could use some tweaks.

Eventually I would keen on:

  • Adding PixmapRef
  • Moving all of the Pixmap types into their own crate (so that things like glifo could use them without depending on `vello_common)

Comment thread sparse_strips/vello_common/src/pixmap.rs
Comment on lines +39 to +40
/// Buffer of the pixmap in RGBA8 format.
buf: &'a mut [u8],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not blocking, but it's a little weird that Pixmap uses PremulRgba8 whereas PixmapMut uses u8 (my preference would be u8 everywhere).

Comment thread sparse_strips/vello_cpu/src/dispatch/mod.rs
Comment on lines +724 to +726
if settings.composite_mode == CompositeMode::Replace && !target_fully_covered {
target.data_mut().fill(0);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn’t expect the target would be cleared beforehand when using CompositeMode::Replace. I’d interpret that mode as “replace the rendered region with the scene”, not “wipe everything and then draw the scene”. Also, don’t we already clear regions later with fine.clear? Is Replace equivalent to the Copy blend mode? If so, could we just use the two Porter-Duff operators here and leave it up to the consumer to decide whether the pixmap should be cleared beforehand?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could change the semantics. It just seems a bit unexpected to me that if the render context is smaller than the pixmap, any pixels that aren’t touched by the scene are left stale (which might also be relevant for our internal use case, in that case we would have to manually clear the pixmap before rendering into it).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, don’t we already clear regions later with fine.clear?

We do, but if the scene doesn't cover the whole pixmap, as I mentioned this will leave the outside regions in the existing pixmap untouched. This is why this action is only triggered if !target_fully_covered.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could change the semantics. It just seems a bit unexpected to me that if the render context is smaller than the pixmap, any pixels that aren’t touched by the scene are left stale (which might also be relevant for our internal use case, in that case we would have to manually clear the pixmap before rendering into it).

Personally, I would expect Replace to behave as a region-local operator: the rendered region overwrites the destination, while pixels outside that region are left untouched. If the caller really wants a whole-buffer wipe, they can do that explicitly on their side with pixmap.data_as_u8_slice_mut().fill(0). That makes the behaviour opt-in, explicit, and preserves the same cost.

There may also be cases where you want to avoid the cost of SrcOver, especially with unpacking wide tile buffers, but still want to replace only the rendered region without clearing the whole buffer, for example when rendering into a preprocessed image, frame, or another existing target.

@laurenz-canva laurenz-canva force-pushed the laurenz/new_render_api branch 2 times, most recently from b954db3 to a1237b9 Compare June 1, 2026 04:59
Comment thread sparse_strips/vello_cpu/src/render.rs Outdated
@nicoburns

Copy link
Copy Markdown
Contributor

I would also ideally like to test this out, but I'm onboard with the API in principle.

@LaurenzV

LaurenzV commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator Author

Anything needed from my side to facilitate this, or do you mean “I’ll test this out once I find the time to do so”?

@nicoburns

Copy link
Copy Markdown
Contributor

I mean “I’ll test this out once I find the time to do so”. I would also be happy for this to land before that if you're confident in it.

@LaurenzV

LaurenzV commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator Author

I agree that the pixmap API should probably be refactored a bit (I'm leaning towards drawing inspiration from tiny-skia here), but I don't want to make this PR harder to review than it already is, so I will leave this for a follow-up.

@LaurenzV LaurenzV requested a review from grebmeg June 8, 2026 11:55
@laurenz-canva laurenz-canva force-pushed the laurenz/new_render_api branch from e6087b3 to 90e5cb5 Compare June 8, 2026 11:59
@laurenz-canva laurenz-canva force-pushed the laurenz/new_render_api branch from 90e5cb5 to bd98e50 Compare June 8, 2026 12:54

@nicoburns nicoburns left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely have some quibbles with the API, but I've tested this against Blitz and it works fine (no unexpected surprises). So happy for this to land :)

@nicoburns

Copy link
Copy Markdown
Contributor

I agree that the pixmap API should probably be refactored a bit (I'm leaning towards drawing inspiration from tiny-skia here), but I don't want to make this PR harder to review than it already is, so I will leave this for a follow-up.

I'd quite like to see Pixmap/PixmapMut/PixmapRef become it's own mini crate. That could then be used by glifo and could help remove the vello_common dep from glifo.

@LaurenzV

LaurenzV commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator Author

Yeah, as I mentioned, happy to iterate further in a future PR!

(Will still wait for Alex to have had a chance to take a look before merging.)

@grebmeg grebmeg left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

TODO: I still want to run this version against my PDF test suite, but this shouldn't block a review.

Before merging, could you please verify this if you haven’t already?

Comment thread sparse_strips/vello_cpu/README.md
Comment thread sparse_strips/vello_cpu/src/render.rs Outdated
Comment on lines +724 to +726
if settings.composite_mode == CompositeMode::Replace && !target_fully_covered {
target.data_mut().fill(0);
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could change the semantics. It just seems a bit unexpected to me that if the render context is smaller than the pixmap, any pixels that aren’t touched by the scene are left stale (which might also be relevant for our internal use case, in that case we would have to manually clear the pixmap before rendering into it).

Personally, I would expect Replace to behave as a region-local operator: the rendered region overwrites the destination, while pixels outside that region are left untouched. If the caller really wants a whole-buffer wipe, they can do that explicitly on their side with pixmap.data_as_u8_slice_mut().fill(0). That makes the behaviour opt-in, explicit, and preserves the same cost.

There may also be cases where you want to avoid the cost of SrcOver, especially with unpacking wide tile buffers, but still want to replace only the rendered region without clearing the whole buffer, for example when rendering into a preprocessed image, frame, or another existing target.

@LaurenzV

Copy link
Copy Markdown
Collaborator Author

Before merging, could you please verify this if you haven’t already?

Already did yesterday, works fine!

@LaurenzV

Copy link
Copy Markdown
Collaborator Author

Personally, I would expect Replace to behave as a region-local operator: the rendered region overwrites the destination, while pixels outside that region are left untouched. If the caller really wants a whole-buffer wipe, they can do that explicitly on their side with pixmap.data_as_u8_slice_mut().fill(0). That makes the behaviour opt-in, explicit, and preserves the same cost.

Hmm, okay. Let's do it this way: I'll add a TODO for this now to avoid additional churn for #1701, and once that has landed I will try to make that change on top of that. I think it shouldn't be too hard, but I want to avoid making rebasing harder than it needs to be. 😄 I hope that's okay with you.

@LaurenzV LaurenzV added this pull request to the merge queue Jun 10, 2026
Merged via the queue into main with commit 011f4ff Jun 10, 2026
29 of 38 checks passed
@LaurenzV LaurenzV deleted the laurenz/new_render_api branch June 10, 2026 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support PixmapMut<'_> and stride in vello_cpu?

3 participants