vello_cpu: Unify render APIs#1665
Conversation
|
@nicoburns If you want to feel free to give it a spin. |
laurenz-canva
force-pushed
the
laurenz/new_render_api
branch
from
2846da5 to
5b7d009
Compare
nicoburns
left a comment
nicoburns
left a comment
There was a problem hiding this comment.
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
glifocould use them without depending on `vello_common)
| pub fn as_mut(&mut self) -> PixmapMut<'_> { | ||
| PixmapMut { | ||
| width: self.width, | ||
| height: self.height, | ||
| buf: bytemuck::cast_slice_mut(&mut self.buf), | ||
| } | ||
| } |
There was a problem hiding this comment.
This should probably be an implementation of the stdlib AsMut trait
There was a problem hiding this comment.
This is not possible because the trait returns a &mut T, while we return a custom wrapper type that only conceptually represents a mutable reference to the pixmap.
| /// Buffer of the pixmap in RGBA8 format. | ||
| buf: &'a mut [u8], |
There was a problem hiding this comment.
Probably not blocking, but it's a little weird that Pixmap uses PremulRgba8 whereas PixmapMut uses u8 (my preference would be u8 everywhere).
| settings: RasterizerSettings, | ||
| encoded_paints: &[EncodedPaint], | ||
| image_resolver: &dyn ImageResolver, | ||
| ); |
There was a problem hiding this comment.
Why are dst_x and dst_y in RasterizerSettings, but scene_width and scene_height are passed standalone? I think they should all be passed in the same way (I don't particularly mind which).
There was a problem hiding this comment.
Because dst_x/dst_y is passed by the user, while the scene width/height is only passed internally directly from the render context.
| if settings.composite_mode == CompositeMode::Replace && !target_fully_covered { | ||
| target.data_mut().fill(0); | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
3 participants
Supersedes #1597, closes #1382.
Motivation
Right now, we have two rendering APIs:
render_to_pixmapandcomposite_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:
rendermethod that can be used to render the contents of aRenderContextintoPixmap-like struct.PixmapMutstruct to allow constructing a render target from a mutable borrowed byte slice. Letrenderalways take aPixmapMut, allowing to pass both, a custom buffer or an owned pixmap as the render target.CompositeModeenum that defines whether the contents of a pixmap should be completely replaced (by default), or whether source-over compositing should be used.PixelFormatstruct, to possibly allowBgra8in the future, which is useful for MacOS.RenderModehas been moved from being stored inRenderContextto instead being a setting during rasterization. I initially stored this inRenderContextbecause 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.RenderContextandPixmapalways 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_winitwith 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.