Public encoding API #239

dfrg · 2023-01-06T21:57:34Z

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.

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.

@dfrg

The area bug was found and fixed by @dfrg, and is adapted from #239. I wanted to move it to a separate PR so that one would be more focused on API. The other bug is currently silent because the two quantities swapped are both 4, but it is triggered when experimenting with tuning for performance.

raphlinus

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.

raphlinus · 2023-01-06T22:00:10Z

shader/fine.wgsl

@@ -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);


I moved this to #241, as I feel it doesn't really belong in this PR, and I had another fine fix.

Sounds good.

raphlinus · 2023-01-08T15:46:19Z

src/encoding/draw.rs

+
+impl DrawTag {
+    /// Returns the size of the info buffer used by this tag.
+    pub fn info_size(self) -> u32 {


This might be a candidate for being marked const fn?

raphlinus · 2023-01-08T15:46:58Z

src/encoding/draw.rs

+#[derive(Clone, Copy, Debug, Default, Zeroable, Pod)]
+#[repr(C)]
+pub struct DrawColor {
+    /// Packed RGBA color.


This would probably be a good place to document the byte order.

Ah, good point since it’s technically ABGR and also premultiplied.

raphlinus · 2023-01-08T15:48:36Z

src/encoding/encoding.rs

+
+    /// Returns true if the encoding is empty.
+    pub fn is_empty(&self) -> bool {
+        self.path_tags.is_empty()


Partly a note to self, this will change when there are rectangles.

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.

raphlinus · 2023-01-08T15:49:34Z

src/encoding/encoding.rs

+    }
+}
+
+fn color_with_alpha(mut color: Color, alpha: f32) -> Color {


This feels like it should be a method on Color?

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.

raphlinus · 2023-01-08T15:50:45Z

src/encoding/monoid.rs

+//
+// Also licensed under MIT license, at your choice.
+
+/// Interface for a monoid.


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.

Associated type seems reasonable to me. Also, always happy to be educated in category theory :)

raphlinus · 2023-01-08T15:52:30Z

src/encoding/path.rs

+
+    /// Returns true if this segment ends a subpath.
+    pub fn is_subpath_end(self) -> bool {
+        (self.0 & Self::SUBPATH_END_BIT) != 0


Parentheses are inconsistent with previous fn.

Will make these consistent.

DJMcNab · 2023-01-08T19:08:14Z

src/encoding/draw.rs

+/// Draw tag representation.
+#[derive(Copy, Clone, PartialEq, Eq, Pod, Zeroable)]
+#[repr(C)]
+pub struct DrawTag(pub u32);


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?

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.

DJMcNab · 2023-01-08T19:11:10Z

src/encoding/draw.rs

+    pub const NOP: Self = Self(0);
+
+    /// Color fill.
+    pub const COLOR: Self = Self(0x44);


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.

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.

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.

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.

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.

lemmih · 2023-01-08T21:23:21Z

src/encoding/path.rs

+
+impl PathTag {
+    /// 32-bit floating point line segment.
+    pub const LINE_TO_F32: Self = Self(0x9);


As an outsider, a comment explaining that 0x9 = 0b1001 = Self::F32_BIT | PathSegmentType::LINE_TO would help me understand these seemingly magic numbers.

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

dfrg · 2023-01-08T22:13:41Z

This should address all of the review feedback. I'll fix color_with_alpha in another PR after the required method is added to peniko. Also adds some additional low level encoding methods for the three supported brush types as I think it could be useful to encode those as well without constructing full brushes.

raphlinus added a commit that referenced this pull request Jan 6, 2023

Typo and link #239

767997e

raphlinus mentioned this pull request Jan 8, 2023

Fixes to fine rasterization #241

Merged

raphlinus approved these changes Jan 8, 2023

View reviewed changes

DJMcNab reviewed Jan 8, 2023

View reviewed changes

lemmih reviewed Jan 8, 2023

View reviewed changes

dfrg added 2 commits January 8, 2023 18:34

add function to render directly from an encoding

ec956d8

better naming

3606184

dfrg merged commit 3ea8eef into main Jan 8, 2023

dfrg deleted the new-encoding branch January 8, 2023 23:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Public encoding API #239

Public encoding API #239

dfrg commented Jan 6, 2023

raphlinus left a comment

raphlinus Jan 6, 2023

dfrg Jan 8, 2023

raphlinus Jan 8, 2023

dfrg Jan 8, 2023

raphlinus Jan 8, 2023

dfrg Jan 8, 2023

raphlinus Jan 8, 2023

dfrg Jan 8, 2023

raphlinus Jan 8, 2023

dfrg Jan 8, 2023

raphlinus Jan 8, 2023

dfrg Jan 8, 2023

raphlinus Jan 8, 2023

dfrg Jan 8, 2023

DJMcNab Jan 8, 2023

dfrg Jan 8, 2023

DJMcNab Jan 8, 2023

dfrg Jan 8, 2023

raphlinus Jan 8, 2023

dfrg Jan 8, 2023

raphlinus Jan 8, 2023 •

edited

lemmih Jan 8, 2023

dfrg Jan 8, 2023

dfrg commented Jan 8, 2023

Public encoding API #239

Public encoding API #239

Conversation

dfrg commented Jan 6, 2023

raphlinus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphlinus Jan 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfrg commented Jan 8, 2023

raphlinus Jan 8, 2023 •

edited