Implement underline ink skipping

[valadaptive]

I still expect the parley_draw API surface to change quite a bit, so this API is a bit awkward for the time being.

This PR implements a "draw decoration with ink skipping" function in parley_draw, using GlyphRunBuilder. It takes the underline offset, thickness, and X-position range as arguments, and draws a bunch of rectangles. It defers to the new decoration_spans method, which returns an iterator of rectangles. This means that if we want to add new decoration types (squiggly underlines!) in the future, we can just draw them inside the rectangles that decoration_spans yields.

One annoying thing is that we have to store the cut-out spans in a Vec in order to properly merge them all together--theoretically, the very last glyph in the run could have a really long swash or something that completely wipes out all the previous spans. The cut-out spans are kept in left-to-right order and merged together using an insertion-sort-like algorithm that should perform well since most spans should be left-to-right and mostly non-overlapping in practice.

Instead of caching the underline spans per glyph, I've opted instead to add the glyphs' bounding boxes to the existing OutlineCache. Glyph bounds are relatively cheap to compute, and seem like they could be broadly useful (they're necessary for atlas-based drawing, for instance), so I don't think calculating them unconditionally is a big deal. If the underline intersects a glyph's bounding box, only then do we compute the precise span. Most glyphs don't have descenders, so this should be a decent speedup in and of itself.

I don't think there's any benchmarking infrastructure for parley_draw at the moment; it'll be necessary to add some before trying more complicated caching strategy.

Since parley_draw doesn't actually depend on Parley, we can't use the Decoration type there, and I haven't made any modifications to it. I feel like there was some previous discussion on whether parley_draw should depend on Parley, but can't find it right now.

[waywardmonkeys]

Excited to see this happening.

[taj-p]

This is amazing work!! 🙌

I've left this as a comment to get your feedback on:

• the API direction (happy for this to be a follow up)
• whether you want to proceed now or wait until we have tests

[taj-p]

Why not the below? Is that to disambiguate from GlyphRenderer::fill_rect?

Suggested change

	Self::fill_rect(self, &rect);
	self.fill_rect(&rect);

[valadaptive]

I thought this needed <Self as RenderContext>::fill_rect, but found the deref was enough to disambiguate it. I've cleaned this up.

[taj-p]

I worry that a reader of this trait might assume that fill_rect is used in some way to render a glyph.

Suggested change

	/// Fill a rectangle with the current paint.
	/// Fill a rectangle with the current paint used for decorations like underline.

[valadaptive]

Clarified the doc comment.

[taj-p]

Could we store this in GlyphCache to allow for re-using the allocation between runs?

[valadaptive]

Yes, we can! It requires adding an extra lifetime parameter to decoration_spans, which now returns an iterator referencing the passed caches, but I think that's okay.

[taj-p]

I'm pretty unsure about this API. Invoking the API in this way necessitates the re-creation/re-fetch-from-cache of the hinting instance, retrieval of the OutlineGlyphCollection (see font_ref.outline_glyphs(), which isn't free) and redundant lookups into outlines and the outline cache.

To be clear, fill_glyphs → calls render() which does:

• FontRef::from_index(...)
• font_ref.outline_glyphs()
• prepare_glyph_run() (hinting instance)
• OutlineCacheSession::new()
• Per-glyph outline lookups

render_decoration → calls decoration_spans() which does the same setup again.

I'm wondering whether a more efficient API would be a builder pattern where we re-use this logic from within render:

/// Configuration for a decoration that skips ink.
#[derive(Clone, Copy)]
pub struct InkSkipDecoration {
    pub x_range_start: f32,
    pub x_range_end: f32,
    pub baseline_y: f32,
    pub offset: f32,
    pub size: f32,
    pub buffer: f32,
}

pub struct GlyphRunBuilder<'a, T: GlyphRenderer + 'a> {
    run: GlyphRun<'a>,
    renderer: &'a mut T,
    // I think we could use a `SmallVec` here of 2 capacity (or simply a 2 sized array) to enable more than 1 decoration.
    decoration: Option<InkSkipDecoration>,
}

impl<'a, T: GlyphRenderer + 'a> GlyphRunBuilder<'a, T> {
    /// Configure an ink-skipping decoration to render alongside the glyphs.
    pub fn with_decoration(
        mut self,
        x_range: RangeInclusive<f32>,
        baseline_y: f32,
        offset: f32,
        size: f32,
        buffer: f32,
    ) -> Self {
        self.decoration = Some(InkSkipDecoration {
            x_range_start: *x_range.start(),
            x_range_end: *x_range.end(),
            baseline_y,
            offset,
            size,
            buffer,
        });
        self
    }
    // ...
}

The downside is that you can't render decorations separately from glyphs.

My understanding is that, on a paragraph basis, Parley should be fast enough to re-render the entire paragraph for any change without resorting to per-glyph surgical updates (i.e. we don't want to support fast paths for inserting a character in the middle of a paragraph - we just re-render the paragraph entirely).

I think that principle can be extended here where, if you need to re-render the decoration, you can also re-render the glyphs. That said, I imagine the goal is for changes to paint styles (like underline/strikethrough) to skip reshaping via fast paths.

I'm happy if you'd prefer to address this as a follow up or here.

profile with significant time spent in setup

[taj-p]

CAUTION DRAFT CODE: I implemented a POC here (for a single decoration)

[valadaptive]

This also applies to fill_glyphs vs. stroke_glyphs, right? If you want to draw filled, outlined text, then you have to do the setup twice already.

I agree that we should come up with an API that solves this issue. I think GlyphRunBuilder should return some sort of "ready-to-render glyph run" type, which the API consumer can then use to render fills, strokes, or decorations in whatever combination or order they want. I think it should be able to store the iterator returned by GlyphRun::positioned_glyphs, and just clone it on each render. Not sure if that iterator's anonymous lifetime will pose an issue; I'll prototype something out.

Regardless, I think this is probably best left to a follow-up, since it will require changing the existing parley_draw API surface.

[taj-p]

This also applies to fill_glyphs vs. stroke_glyphs, right?

Not necessarily. For the same global position, you are unlikely to want to both stroke and fill glyphs. But, for the same glyph run global position, you are likely going to want to render glyphs and/or decorations.

Since the current outputs of the GlyphRunBuilder are/depend on the global transform, I'm not sure how a "ready-to-render" glyph run could be reused in its current form except for re-rendering identical outputs.

which the API consumer can then use to render fills, strokes, or decorations in whatever combination or order they want

Did you have a use case in mind for this?

I'll have to think about this because I'm not sure whether that API could provide the same performance as a build and then render all at once approach. As we iterate the paths for filling or stroking some glyphs, for example, we would want to also prepare the decorations if they are requested (but that's only possible if we know ahead of time).

Separately, I'm not so sure about the performance cost of cloning out all every glyph position for every render. I think we'd prefer to avoid that - perhaps benchmarks could prove otherwise, however!

[taj-p]

Oops - I just read your DM (after posting the above!). Let me check that out first 🙏 .

[taj-p]

Would you believe this is showing up on perf profiles?

I guess because it's the fast path for all glyphs, it's pretty hot code. Since we're only evaluating the y coordinates, we could halve the computations by ignoring x. I think the below is valid:

            // We only need the y-extent of the transformed bbox, so we compute it directly using the formula y' = b*x + d*y + f
            let [_, b, _, d, _, f] = outline_transform.as_coeffs();
            let (y_min, y_max) = {
                let bx0 = b * path.bbox.x0;
                let bx1 = b * path.bbox.x1;
                let dy0 = d * path.bbox.y0;
                let dy1 = d * path.bbox.y1;
                (
                    f + bx0.min(bx1) + dy0.min(dy1),
                    f + bx0.max(bx1) + dy0.max(dy1),
                )
            };
            if y_max < layout_y0 || y_min > layout_y1 {
                continue;
            }

But, also happy to defer this until we have benchmarks and tests in place.

[valadaptive]

I've implemented that optimization.

[taj-p]

Running the intersection code seems to regularly dominate performance profiles. I'm wondering whether it's worth calculating the y bounds at the same time and fast path out of intersect_line:

    let (mut x_bounds, y_bounds) = match seg {
        PathSeg::Line(line) => (
            (line.p0.x.min(line.p1.x), line.p0.x.max(line.p1.x)),
            (line.p0.y.min(line.p1.y), line.p0.y.max(line.p1.y)),
        ),
        PathSeg::Quad(quad) => (
            (
                quad.p0.x.min(quad.p1.x).min(quad.p2.x),
                quad.p0.x.max(quad.p1.x).max(quad.p2.x),
            ),
            (
                quad.p0.y.min(quad.p1.y).min(quad.p2.y),
                quad.p0.y.max(quad.p1.y).max(quad.p2.y),
            ),
        ),
        PathSeg::Cubic(cubic) => (
            (
                cubic.p0.x.min(cubic.p1.x).min(cubic.p2.x).min(cubic.p3.x),
                cubic.p0.x.max(cubic.p1.x).max(cubic.p2.x).max(cubic.p3.x),
            ),
            (
                cubic.p0.y.min(cubic.p1.y).min(cubic.p2.y).min(cubic.p3.y),
                cubic.p0.y.max(cubic.p1.y).max(cubic.p2.y).max(cubic.p3.y),
            ),
        ),
    };

    // Fast path: skip segments whose y-extent doesn't intersect the y_span.
    if y_bounds.1 < *y_span.start() || y_bounds.0 > *y_span.end() {
        return;
    }

Happy to leave this until we have benchmarks.

[valadaptive]

I've implemented that optimization.

[taj-p]

On naming and parameterisation, I'm wondering whether you're imagining this evolve to take a should_ink or ink_skip parameter so we can support strikethrough as well?

[valadaptive]

I think as part of Bruce's future "attributed text" work + moving the non-layout style types out of Parley, there'll probably be a "decoration" type somewhere else. Once we get there, we can just take that type.

[valadaptive]

I've addressed the review feedback and implemented the optimizations you suggested. What benchmark harness have you been using?

Also left my thoughts on what a better API could look like here; we're definitely doing a lot of redundant work as it currently stands. I can rework the API as a follow-up.

Let's wait for some tests; I'm working on moving the test suite out of the parley crate, but some of them reach into private APIs so it'll be a little tricky.

[taj-p]

I think we might want the buffer to scale with the scene's transform in some way. I tried to test this, but discovered that there is likely a bug with handling the current transform of the scene:

This is the same text layout but the second typography text has a 2x scale transform applied to it.

reprod

// Copyright 2024 the Parley Authors
// SPDX-License-Identifier: Apache-2.0 OR MIT

//! A simple example that lays out some text using Parley, extracts outlines using Skrifa and
//! then paints those outlines using Vello CPU through Parley Draw.

#![expect(clippy::cast_possible_truncation, reason = "Deferred")]

use parley::{
    Alignment, AlignmentOptions, FontContext, GenericFamily, GlyphRun, Layout, LayoutContext,
    LineHeight, PositionedLayoutItem, StyleProperty,
};
use parley_draw::{GlyphCaches, GlyphRunBuilder};
use vello_cpu::{Pixmap, RenderContext, kurbo, peniko::Color};

#[derive(Clone, Copy, Debug, PartialEq)]
struct ColorBrush {
    color: Color,
}

impl Default for ColorBrush {
    fn default() -> Self {
        Self {
            color: Color::BLACK,
        }
    }
}

fn main() {
    let text = String::from("Typography pqgy");

    // The display scale for HiDPI rendering
    let display_scale = 1.0;

    // Whether to automatically align the output to pixel boundaries, to avoid blurry text.
    let quantize = true;

    // The width for line wrapping
    let max_advance: Option<f32> = None;

    // Colours for rendering
    let foreground_color = Color::BLACK;
    let background_color = Color::WHITE;

    // Padding around the output image
    let padding = 20;

    // Create a FontContext, LayoutContext
    let mut font_cx = FontContext::new();
    let mut layout_cx = LayoutContext::new();

    // Create a RangedBuilder
    let mut builder = layout_cx.ranged_builder(&mut font_cx, &text, display_scale, quantize);

    // Set default text colour styles (set foreground text color)
    let foreground_brush = ColorBrush {
        color: foreground_color,
    };
    builder.push_default(StyleProperty::Brush(foreground_brush));
    builder.push_default(GenericFamily::SystemUi);
    builder.push_default(LineHeight::FontSizeRelative(1.3));
    builder.push_default(StyleProperty::FontSize(16.0));
    builder.push_default(StyleProperty::Underline(true));

    // Build the builder into a Layout
    let mut layout: Layout<ColorBrush> = builder.build(&text);

    // Perform layout
    layout.break_all_lines(max_advance);
    layout.align(max_advance, Alignment::Start, AlignmentOptions::default());

    // Calculate canvas size to fit both 1x and 2x scaled text
    let layout_width = layout.width().ceil() as u16;
    let layout_height = layout.height().ceil() as u16;
    let canvas_width =
        (layout_width as f64 * 2.0 + padding as f64 * 3.0).ceil() as u16 + layout_width;
    let canvas_height = (layout_height as f64 * 2.0 + padding as f64 * 3.0).ceil() as u16;

    // The renderer and glyph caches should be created once per app (or per thread).
    let mut renderer = RenderContext::new(canvas_width, canvas_height);
    let mut glyph_caches = GlyphCaches::new();

    renderer.set_paint(background_color);
    renderer.fill_rect(&kurbo::Rect::new(
        0.0,
        0.0,
        canvas_width as f64,
        canvas_height as f64,
    ));

    // I believe this value doesn't scale with the scene transform (?), so the gap around descenders
    // appears different at different zoom levels.
    let exclusion_buffer = 1.0;

    // Render at 1x scale
    renderer.set_transform(kurbo::Affine::translate(kurbo::Vec2::new(
        padding as f64,
        padding as f64,
    )));
    render_layout(
        &mut renderer,
        &layout,
        foreground_color,
        exclusion_buffer,
        &mut glyph_caches,
    );

    // Render at 2x scale
    // When we apply a 2x scale, the exclusion_buffer should ideally also scale 2x,
    // but it doesn't because it's specified in layout coordinates.
    renderer.set_transform(
        kurbo::Affine::translate(kurbo::Vec2::new(
            padding as f64 * 2.0 + layout_width as f64,
            padding as f64,
        )) * kurbo::Affine::scale(2.0),
    );
    render_layout(
        &mut renderer,
        &layout,
        foreground_color,
        exclusion_buffer,
        &mut glyph_caches,
    );

    let mut pixmap = Pixmap::new(canvas_width, canvas_height);
    renderer.render_to_pixmap(&mut pixmap);
    glyph_caches.maintain();

    // Write image to PNG file in examples/_output dir
    let output_path = {
        let path = std::path::PathBuf::from(file!());
        let mut path = std::fs::canonicalize(path).unwrap();
        path.pop();
        path.pop();
        path.pop();
        path.push("_output");
        drop(std::fs::create_dir(path.clone()));
        path.push("vello_cpu_render.png");
        path
    };
    let png = pixmap.into_png().unwrap();
    std::fs::write(output_path, png).unwrap();
}

fn render_layout(
    renderer: &mut RenderContext,
    layout: &Layout<ColorBrush>,
    foreground_color: Color,
    exclusion_buffer: f32,
    glyph_caches: &mut GlyphCaches,
) {
    for line in layout.lines() {
        for item in line.items() {
            match item {
                PositionedLayoutItem::GlyphRun(glyph_run) => {
                    renderer.set_paint(glyph_run.style().brush.color);
                    let run = glyph_run.run();
                    GlyphRunBuilder::new(run.font().clone(), *renderer.transform(), renderer)
                        .font_size(run.font_size())
                        .hint(true)
                        .normalized_coords(run.normalized_coords())
                        .fill_glyphs(
                            glyph_run
                                .positioned_glyphs()
                                .map(|glyph| parley_draw::Glyph {
                                    id: glyph.id,
                                    x: glyph.x,
                                    y: glyph.y,
                                }),
                            glyph_caches,
                        );

                    let style = glyph_run.style();
                    if let Some(decoration) = &style.underline {
                        let offset = decoration.offset.unwrap_or(run.metrics().underline_offset);
                        let size = decoration.size.unwrap_or(run.metrics().underline_size);

                        renderer.set_paint(decoration.brush.color);
                        let x = glyph_run.offset();
                        let x1 = x + glyph_run.advance();
                        let baseline = glyph_run.baseline();

                        GlyphRunBuilder::new(run.font().clone(), *renderer.transform(), renderer)
                            .font_size(run.font_size())
                            .normalized_coords(run.normalized_coords())
                            .render_decoration(
                                glyph_run
                                    .positioned_glyphs()
                                    .map(|glyph| parley_draw::Glyph {
                                        id: glyph.id,
                                        x: glyph.x,
                                        y: glyph.y,
                                    }),
                                x..=x1,
                                baseline,
                                offset,
                                size,
                                exclusion_buffer,
                                glyph_caches,
                            );
                    }
                    if let Some(decoration) = &style.strikethrough {
                        let offset = decoration
                            .offset
                            .unwrap_or(run.metrics().strikethrough_offset);
                        let size = decoration.size.unwrap_or(run.metrics().strikethrough_size);

                        render_decoration(renderer, &decoration.brush, &glyph_run, offset, size);
                    }
                }
                PositionedLayoutItem::InlineBox(inline_box) => {
                    renderer.set_paint(foreground_color);
                    let (x0, y0) = (inline_box.x as f64, inline_box.y as f64);
                    let (x1, y1) = (x0 + inline_box.width as f64, y0 + inline_box.height as f64);
                    renderer.fill_rect(&kurbo::Rect::new(x0, y0, x1, y1));
                }
            }
        }
    }
}

fn render_decoration(
    renderer: &mut RenderContext,
    brush: &ColorBrush,
    glyph_run: &GlyphRun<'_, ColorBrush>,
    offset: f32,
    size: f32,
) {
    renderer.set_paint(brush.color);
    let y = glyph_run.baseline() - offset;
    let x = glyph_run.offset();
    let x1 = x + glyph_run.advance();
    let y1 = y + size;
    renderer.fill_rect(&kurbo::Rect::new(x as f64, y as f64, x1 as f64, y1 as f64));
}

[valadaptive]

This was a coordinate-space issue, which turned out to be a bit of a can of worms. The GlyphRenderer trait's existing methods work in a global coordinate space (the vello_cpu implementation clears and restores the renderer transform), but fill_rect implicitly uses local space.

The two diverge when hinting is enabled. prepare_glyph_run has Skrifa scale the glyphs on the hinted path and strips the scale from the global transform.

I chose to keep fill_rect in local space. I think we should probably use local space for everything going forward, since it's necessary for proper stroke scaling when applying a stroke to a glyph run (right now, that's irrelevant since the stroke_glyph implementation just...applies a fill instead???) I think it's also required for gradient fills to be in the correct coordinate space. The GlyphRenderer API should probably also handle the global transform for us, so we don't have to pass renderer.transform() into GlyphRunBuilder manually.

I noticed another bug, not related to this change. The glyph_transform seems to be applied in the wrong order when hinting is enabled. Look at this render with a 2x scale transform and an X skew of 1.0:

It looks like the underline gaps are in the wrong place! But actually, it's the glyphs that are wrong. With hinting disabled, we can see what it's supposed to look like:

This matches the 1x scale version:

So in a follow-up, I'll also need to address the incorrect per-glyph transform (and add tests for that too, once the infrastructure is in place).

[taj-p]

What benchmark harness have you been using?

Sorry, I don't have a good answer for you. I wrapped the render logic in vello_cpu_render in a loop, removed the file generation, and profiled it in a release build with samply. I tried out a variety of inputs like the existing text style, enabling underline by default, and enabling more underline of the existing text.

[conor-93]

Amazing work Val!

Agreed that the general API issue affecting decorations/fills/strokes should be addressed as a follow-up. Separating GlyphRunRenderer from GlyphRunBuilder looks like it’ll be a good way to handle this.

cLGTM on Taj’s full approval, and assuming exclusion zone scaling is addressed before merge.

[xStrom]

I believe this was closed by accident.