-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Public encoding API #239
Public encoding API #239
Conversation
This exposes significantly more of the previously internal encoding guts. This is beneficial for its own sake as there always seem to be new and interesting things that are supported by the encoding and GPU pipeline that are not directly exposed by the scene builder. The secondary purpose is laying the groundwork for scene *de*coding and processing. This will enable us to implement various stages of the pipeline on the CPU.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it! It seems clean, and in particular I like the way the tag values are encapsulated, it's a big improvement over having magic numbers scattered in the source. It will be very interesting to see how people use this.
Most of the comments are minor documentation nits.
shader/fine.wgsl
Outdated
@@ -138,7 +138,7 @@ fn fill_path(tile: Tile, xy: vec2<f32>) -> array<f32, PIXELS_PER_THREAD> { | |||
} | |||
// nonzero winding rule | |||
for (var i = 0u; i < PIXELS_PER_THREAD; i += 1u) { | |||
area[i] = abs(area[i]); | |||
area[i] = min(abs(area[i]), 1.0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this to #241, as I feel it doesn't really belong in this PR, and I had another fine fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
src/encoding/draw.rs
Outdated
|
||
impl DrawTag { | ||
/// Returns the size of the info buffer used by this tag. | ||
pub fn info_size(self) -> u32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a candidate for being marked const fn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
src/encoding/draw.rs
Outdated
#[derive(Clone, Copy, Debug, Default, Zeroable, Pod)] | ||
#[repr(C)] | ||
pub struct DrawColor { | ||
/// Packed RGBA color. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would probably be a good place to document the byte order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point since it’s technically ABGR and also premultiplied.
|
||
/// Returns true if the encoding is empty. | ||
pub fn is_empty(&self) -> bool { | ||
self.path_tags.is_empty() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partly a note to self, this will change when there are rectangles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was originally a hack to detect empty glyphs when that would break the path encoding but I think it’s probably useful regardless.
} | ||
} | ||
|
||
fn color_with_alpha(mut color: Color, alpha: f32) -> Color { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it should be a method on Color?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I’ll update add it to my pending peniko patch. For context, lottie has an additional opacity modifier and I wanted to be able to encode this without rebuilding brushes.
src/encoding/monoid.rs
Outdated
// | ||
// Also licensed under MIT license, at your choice. | ||
|
||
/// Interface for a monoid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably good to mention that default is the identity of the monoid.
I'm also curious why T is a generic parameter rather than an associated type.
The existence of the new
method makes this a monoid homomorphism rather than a vanilla monoid, but for the purposes of this module we don't have to go full math nerd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Associated type seems reasonable to me. Also, always happy to be educated in category theory :)
src/encoding/path.rs
Outdated
|
||
/// Returns true if this segment ends a subpath. | ||
pub fn is_subpath_end(self) -> bool { | ||
(self.0 & Self::SUBPATH_END_BIT) != 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parentheses are inconsistent with previous fn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will make these consistent.
/// Draw tag representation. | ||
#[derive(Copy, Clone, PartialEq, Eq, Pod, Zeroable)] | ||
#[repr(C)] | ||
pub struct DrawTag(pub u32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it meaningful for this to be public (rather than having a getter?)
That is, is there any valid case where this could be set to a value other than one of the below constants?
I suppose it being Pod
is equivalent to this being pub
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the full encoding exposed, any number of invariants can be broken so I see little value in hiding some arbitrary bits of data behind getters. For those wanting a “safe” interface, SceneBuilder should be used.
pub const NOP: Self = Self(0); | ||
|
||
/// Color fill. | ||
pub const COLOR: Self = Self(0x44); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values seem very arbitrary. I see that 4 bits of the value is info_size
. 6 is an awkward number of bits for that to be down, if these are being written out as (non-binary) literals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raph can give a more informed rationale for the specific packing but various bits are used on the GPU side for computing offsets and more efficient branching on clips.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a good point. Right now it's tightly packed rightward, but there's probably no compelling reason for that to be the case. If all our shifts were multiples of 4 then it would read as a hex value. One minor consideration is whether to count bytes or u32 words; the latter is more
Another approach to this would be to have a const fn builder that takes is_clip, scene_size (maybe "scene_words"), and info_size as args. That feels Rust-like and less magical, though unfortunately the current state of naga is that it might be clearer to use hex values as those would match CPU and GPU sides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had considered the const fn approach as well but felt it better if these values just matched those in the shader for now. I expect these to change again when we actually handle extend modes for gradients (scene size will change) so it may be worth reconsidering the packing at 4-bit granularity then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, the encoding will also change with rectangles, and it's also potentially a place to stash even/odd vs nonzero fill rules, though in the current architecture linewidth is probably a more natural fit.
|
||
impl PathTag { | ||
/// 32-bit floating point line segment. | ||
pub const LINE_TO_F32: Self = Self(0x9); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an outsider, a comment explaining that 0x9 = 0b1001 = Self::F32_BIT | PathSegmentType::LINE_TO
would help me understand these seemingly magic numbers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll add a comment.
* remove area clamp in fine (now in #241) * make DrawTag::info_size() const fn * document DrawColor rgba field * change Monoid type parameter to an associated type and describe the additional Default constraint * consistent parens in PathTag::is_subpath_end * add comments for 32-bit path segment types in PathTag * also adds low level encoding functions for the three currently supported brushes
This should address all of the review feedback. I'll fix |
This exposes significantly more of the previously internal encoding guts.
This is beneficial for its own sake as there always seem to be new and interesting things that are supported by the encoding and GPU pipeline that are not directly exposed by the scene builder.
The secondary purpose is laying the groundwork for scene decoding and processing. This will enable us to implement various stages of the pipeline on the CPU and hopefully allow development of better debugging tools.