Implement shaping in `parley_core` by tomcur · Pull Request #675 · linebender/parley

tomcur · 2026-07-06T22:11:36Z

This implements shaping in parley_core, taking the result of the new itemization that just landed, some additional shaping options, plus an API that keeps specifics of font selection up to the caller. Taking this form, we should be able to drop the fontique dependency from parley_core, but I've not done so here as it inflates the diff and would benefit from separate discussion.

The next step after landing this would be the introduction of a ShapedText. I've left some notes in parley_core/src/shaped_text.rs about what that may look like. A ShapedText is going to be useful for reshaping; I have something in mind that I think is better than what I proposed in #634, but more on that later/elsewhere!

The ShaperContext::shape_item method is somewhat callback-heavy. I wonder if we should start introducing traits instead. (Though the |shaped_run| callback is going to become a &mut ShapedText soon enough I suppose.)

This benches neutral to ~+1.5% on my machine.

$ cargo bench -q --bench=main -- compare ../target/benchmarks/main -t 8.
Default Style - arabic 20 characters               [   8.7 us ...   8.9 us ]      +1.50%*
Default Style - latin 20 characters                [   4.2 us ...   4.2 us ]      +0.29%
Default Style - japanese 20 characters             [   8.1 us ...   8.1 us ]      +0.01%
Default Style - arabic 1 paragraph                 [  47.6 us ...  47.9 us ]      +0.53%
Default Style - latin 1 paragraph                  [  16.2 us ...  16.5 us ]      +1.37%*
Default Style - japanese 1 paragraph               [  69.4 us ...  69.0 us ]      -0.50%
Default Style - arabic 4 paragraph                 [ 203.4 us ... 205.8 us ]      +1.16%*
Default Style - latin 4 paragraph                  [  61.7 us ...  62.2 us ]      +0.87%
Default Style - japanese 4 paragraph               [  97.9 us ...  97.7 us ]      -0.25%
Styled - arabic 20 characters                      [   9.7 us ...   9.9 us ]      +1.28%*
Styled - latin 20 characters                       [   5.3 us ...   5.4 us ]      +0.49%
Styled - japanese 20 characters                    [   8.6 us ...   8.6 us ]      -0.73%
Styled - arabic 1 paragraph                        [  50.2 us ...  50.5 us ]      +0.68%
Styled - latin 1 paragraph                         [  20.9 us ...  20.9 us ]      +0.09%
Styled - japanese 1 paragraph                      [  75.0 us ...  75.1 us ]      +0.11%
Styled - arabic 4 paragraph                        [ 219.9 us ... 223.9 us ]      +1.80%*
Styled - latin 4 paragraph                         [  81.1 us ...  81.5 us ]      +0.50%
Styled - japanese 4 paragraph                      [ 106.7 us ... 106.3 us ]      -0.31%

Slight changes are getting compilation and the benches to fall somewhat differently, e.g., taking a trait for font selection seems to have a very slight positive effect, but it seems mostly up to the whims of the compiler. (As a performance aside, the whole font fallback code is quite hot as we're calling it for each character, and we could optimize, e.g., under the assumption that the preferred font is likely to match.)

Small review guide: parley's behavior should be unchanged, and I copied the shaping code verbatim. The first commit just moves blocks around (it compiles and passes tests). The second commit moves some blocks around, with some changes to get stuff to compile, but does not wire up parley to use the shaping that's been moved out. The third commit wires everything up and passes tests, and the commits after that are clean up, more docs, etc.

tomcur · 2026-07-06T22:17:51Z

+    pub fn shape_item(
+        &mut self,
+        text: &str,
+        analysis: &Analysis,
+        item: &Item,
+        options: &ShapeOptions<'_>,
+        select_font: impl FnMut(&mut CharCluster) -> Option<FontInstance>,
+        analysis_data_sources: &AnalysisDataSources,
+        shaped_runs: impl FnMut(ShapedRun<'_>),
+    ) {


For font selection specifically, using a trait instead of the callback would have the potential benefit of being able to take the font Blob by reference, which could be useful in case users are doing multi-threaded layout (as the Blob's Arc refcount bumps may then be contended). Perhaps something like the following.

pub trait SelectFont { fn select_font(&mut self, char_cluster: &mut CharCluster) -> Option<FontInstanceRef<'_>>; }

(Also, and very much separately, I believe parley's charmap test is not quite sufficient, and glyphs can still become .notdef. I think we need to try to shape with the font and move to the next candidate font if that fails.)

tomcur · 2026-07-06T22:20:52Z

-                        font: font.clone(),
-                        attrs: self.attrs,
-                    });
+                    selected_font = Some(SelectedFont { font: font.clone() });


I've dropped the SelectedFont::attrs field (a bit of a drive-by) as these are just properties taken from the run's style, so are already available there.

Hmm. I believed that they were useful for something like "how much fake bold do I need", but I'm possibly thinking of something else?

We're still passing the attributes through, but the old code was getting the attributes from the run's style, then storing them in the selected font, and then pushing them to the layout from there. The new code gets them directly from the run's style when pushing the run to the layout. Compare old vs new. Actually, sticking close to the old version would be a bit messy as parley_core would have to plumb it through (and it only cares about the selected font, not the attributes).

(For e.g. emboldening, you'd perhaps only check the Synthesis::embolden flag, though Attributes::weight is still available if you want to calculate a delta. (Maybe fontique should store the delta in Synthesis...))

tomcur · 2026-07-06T22:23:35Z

-    NFD,
-    NFC,
+    Nfd,
+    Nfc,


(This was simpler than figuring out text for the reason field that parley_core's lints require. 🙃 )

tomcur · 2026-07-06T22:39:32Z

+    // TODO: For `select_font`, on `None`, the previous font is taken (and the run is dropped if
+    // `None` is returned on the first call). This is identical to Parley's old behavior, but we
+    // probably want the commented-out documented behavior that follows, as returning a font is
+    // cheap and it probably doesn't make a ton of sense to hardcode some font fallback behavior
+    // here.
+    //
+    // /// Return `None` if there are no fonts available at all. The character cluster's text will be
+    // /// omitted from the shaped result. Instead, you probably want to render a `.notdef` glyph from a
+    // /// font you do have available, in which case you can return the previous font or some
+    // /// last-resort fallback font instead.


The behavior of using the result of font selection is copied from parley in this PR, but I think if the user's font selection returns None, we should not assume they want the previous font used, and instead should not render the text.

At some point, we'll need to work out how to tofu (and/or TOFU with number).

At some point, we'll need to work out how to tofu (and/or TOFU with number).

I believe we currently tofu from the primary font! But tofu'ing with a number could be nice (or in general letting the user render some arbitrary tofu). I suppose we could let users pass in an optional callback to replace .notdef with something returning metrics instead, and at rendering-time they'd render whatever path they want.

tomcur · 2026-07-06T22:50:33Z

+    /// The item is broken into runs of maximal sequences of character clusters for which
+    /// `select_font` returns the same font. The `shaped_runs` callback is called with each shaped
+    /// run.


We could probably start guaranteeing these "character clusters" to be graphemes, but that requires a careful pass.

I would love that. But yes, definitely out of scope of this PR.

nicoburns · 2026-07-07T10:16:21Z

Re: performance. We should probably be adding #[inline] to a lot of the public methods in parley_core (any small functions).

Font selection remains in `parley`.

taj-p

Nice!

If the intent is to re-audit the public API surface of parley_core in a separate PR to keep this PR minimal, LGTM.

If you want to re-think the API surface within this PR, please re-request review after those changes are in place.

taj-p · 2026-07-11T21:07:57Z

 #[derive(Debug, Default)]
-pub(crate) struct CharCluster {
+pub struct CharCluster {
    pub chars: Vec<Char>,


I'm not sure we want to allow consumers to mutate chars because it would invalidate our other fields within CharCluster

Fair! I wanted to keep the diff small, but probably better to get visibility more correct. I've made the fields private with read accessors, but will do a bit more clean up as a new PR.

taj-p · 2026-07-11T21:18:21Z

Moving ClusterData as is into parley_core means that we've increased the effective surface area of ClusterData from pub(crate) to pub. This means that its internal details like glyph_len == 0xFF => inline glyph in glyph_offsetnow need to be parsed by external consumers and itspub` fields can be writable.

To what extent do we want to address this increase in surface area in this PR? IMO, I think ClusterData shouldn't have writable fields for consumers outside the crate and we could reduce the number of footguns across its API.

For example, this is an example that currently lives in parley/src/layout:

pub fn glyphs(&self) -> impl Iterator<Item = Glyph> + 'a + Clone + use<'a, B> { if self.data.glyph_len == 0xFF { GlyphIter::Single(Some(Glyph { id: self.data.glyph_offset, x: 0., y: 0., advance: self.data.advance, })) } else { let start = self.run.data.glyph_start + self.data.glyph_offset as usize; GlyphIter::Slice( self.run.layout.data.glyphs[start..start + self.data.glyph_len as usize].iter(), ) } }

I think GlyphIter might be more a parley_core struct than a parley struct. I'm also unsure whether we want consumers to branch on glyph_len == 0xFF.

The same goes for many of the newly pub structs exposed from parley_core - but I assume that some of these will become pub(crate) again after the migration is complete? I'm happy to audit the newly exposed structs, but I'm assuming that the plan is to perform that audit in a separate pass?

Moving ClusterData as is into parley_core means that we've increased the effective surface area of ClusterData from pub(crate) to pub. This means that its internal details like glyph_len == 0xFF => inline glyph in glyph_offsetnow need to be parsed by external consumers and itspub` fields can be writable.

To what extent do we want to address this increase in surface area in this PR? IMO, I think ClusterData shouldn't have writable fields for consumers outside the crate and we could reduce the number of footguns across its API.

I also prefer an API for parley_core that exposes just what's necessary or potentially useful, and ideally core's invariants can't be broken in subtle ways by users. There's a somewhat similar discussion here: #679 (comment).

In opening this PR my intention was to introduce as little changes as possible to ease review of the migration, but we definitely need to do a pass over what we expose.

taj-p · 2026-07-11T21:23:11Z

I'm assuming this was copied verbatim 🙏

taj-p · 2026-07-11T21:28:35Z

@@ -44,13 +44,13 @@ core_maths = { version = "0.1.1", optional = true }
 accesskit = { workspace = true, optional = true }
 hashbrown = { workspace = true }
 harfrust = { workspace = true }


Will we need to continue depending on harfrust in parley following the migration to parley_core?

Nope! Currently we just need to name some types. In the ShapedText PR (#675), parley drop the harfrust dependency (along with some icu4x crates, all have moved to parley_core).

taj-p · 2026-07-11T21:37:26Z

+    /// The item is broken into runs of maximal sequences of character clusters for which
+    /// `select_font` returns the same font. The `shaped_runs` callback is called with each shaped
+    /// run.


I would love that. But yes, definitely out of scope of this PR.

taj-p · 2026-07-11T21:50:18Z

+
+fn variations_iter<'a>(
+    synthesis: &'a fontique::Synthesis,
+    item: Option<&'a [FontVariation]>,


Every consumer of variations_iter (just the 1) passes a Some(...) item, so I think we can drop the Option here

Suggested change

item: Option<&'a [FontVariation]>,

item: &'a [FontVariation],

Nice! (Though note I didn't look too closely at the code, it was copied from parley, and there may be more clean ups possible!)

taj-p · 2026-07-11T22:00:28Z

+                    &char_style_indices[shaped_run.range.char_range.clone()];
+                layout.data.push_run(
+                    FontData::new(shaped_run.font.blob, shaped_run.font.index),
+                    style.font_size,


Suggested change

style.font_size,

run_style.font_size,

taj-p · 2026-07-11T22:00:40Z

+                    style.word_spacing,
+                    style.letter_spacing,


Suggested change

style.word_spacing,

style.letter_spacing,

run_style.word_spacing,

run_style.letter_spacing,

I believe these fields are guaranteed to be the same as run_style?

Yes, but specifically they're guaranteed to be nearly_eq. I think it's perhaps better to take them from style. We itemize based on these (and also the font_size you marked above):

parley/parley/src/shape/mod.rs

Lines 118 to 129 in a5ea9a7

if style_index != item_style_index {

let item_style = &styles[usize::from(item_style_index)];

let style = &styles[usize::from(style_index)];

!nearly_eq(style.font_size, item_style.font_size)

|| style.locale != item_style.locale

|| style.font_variations != item_style.font_variations

|| style.font_features != item_style.font_features

|| !nearly_eq(style.letter_spacing, item_style.letter_spacing)

|| !nearly_eq(style.word_spacing, item_style.word_spacing)

} else {

false

}

That means that these are constant (within the nearly_eq decision to put them into a single item) over all the runs produced for this item (and new runs are created due to font selection falling differently and such). That means that taking them from the run could potentially get a slightly different result, which sort of conflicts with putting them in a single item.

I'm thinking to leave it as this for now, but I'm not too attached to it, and am happy for suggestions or revisiting this.

taj-p · 2026-07-11T22:04:33Z

-            font.font.synthesis,
-            &glyph_buffer,
-            item.level,
-            item.style_index,


I think we are producing different behaviour with these changes. Before, we only considered the item's style index, but today we're considering the run's style index. I think the proposal is more correct because it means that "the big text" resolves a different run for "big" so if it's line height, for example, changes, we use the new line height rather than the line height of "the".

Does that make sense?

Ah, I don't now remember whether I changed the item.style_index to run_style_index purposefully in this PR (I was sticking close to the original behavior), but the run style index is in principle the correct style index for things like font weight. There's a caveat that we nearly_eq to itemize for some shaping-relevant styles (more on that in the other comment: #675 (comment)). That means that we need to be careful to take the styles that were considered equal for itemization (and thus considered equal for shaping) from the item's style, and not the run's.

(And perhaps the simplest way to get trivial correctness is just dropping the nearly_eq!)

tomcur · 2026-07-12T22:04:48Z

I believe I've addressed the feedback as it relates to this PR in the narrow sense; thanks a lot for the careful review! 🙏

Merging, but expect some follow up!

tomcur commented Jul 6, 2026

View reviewed changes

Comment thread parley_core/src/shape/shaper.rs

tomcur commented Jul 6, 2026

View reviewed changes

tomcur force-pushed the parley-core-shaping branch 2 times, most recently from b336e70 to 0f94d2f Compare July 8, 2026 16:05

tomcur added 11 commits July 9, 2026 16:28

Move some data and shaping algorithms into parley_core

8c860f9

Move ShapeContext and segment shaping into parley_core

f291825

Font selection remains in `parley`.

Wire up parley_core's shaping in parley

99126c6

Docs

33e8372

Clippy and fix/suppress lints

32f08e4

Inline

25fd23f

Re-export

2e85934

Add font select docs and TODO note

227c8e0

Fmt

4c39147

Docs

6382ee8

Drop superfluous param

e4b3d91

tomcur force-pushed the parley-core-shaping branch from 0f94d2f to e4b3d91 Compare July 9, 2026 14:28

tomcur mentioned this pull request Jul 9, 2026

Implement parley_core::ShapedText #679

Merged

taj-p approved these changes Jul 11, 2026

View reviewed changes

taj-p reviewed Jul 11, 2026

View reviewed changes

Comment thread parley_core/src/shape/shaper.rs

taj-p reviewed Jul 11, 2026

View reviewed changes

tomcur added 2 commits July 12, 2026 17:37

Make CharCluster fields crate-private

8908996

Rename ShapeContext => Shaper

6e1a32e

tomcur force-pushed the parley-core-shaping branch from 4392464 to 6e1a32e Compare July 12, 2026 15:41

tomcur added 4 commits July 12, 2026 21:27

Inline CluserInfo methods

d23cb3d

Inline fill_cluster

d03a72a

Inline callbacks

4e18933

Simplify variations_iter

d0b2c0c

tomcur force-pushed the parley-core-shaping branch from becc100 to d0b2c0c Compare July 12, 2026 20:47

tomcur added this pull request to the merge queue Jul 12, 2026

Merged via the queue into linebender:main with commit c96d22f Jul 12, 2026
24 checks passed

tomcur deleted the parley-core-shaping branch July 12, 2026 22:15

	item: Option<&'a [FontVariation]>,
	item: &'a [FontVariation],

	if style_index != item_style_index {
	let item_style = &styles[usize::from(item_style_index)];
	let style = &styles[usize::from(style_index)];
	!nearly_eq(style.font_size, item_style.font_size)
	\|\| style.locale != item_style.locale
	\|\| style.font_variations != item_style.font_variations
	\|\| style.font_features != item_style.font_features
	\|\| !nearly_eq(style.letter_spacing, item_style.letter_spacing)
	\|\| !nearly_eq(style.word_spacing, item_style.word_spacing)
	} else {
	false
	}

Uh oh!

Conversation

tomcur commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomcur Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicoburns commented Jul 7, 2026

Uh oh!

taj-p left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomcur Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomcur Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taj-p Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomcur Jul 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

tomcur commented Jul 6, 2026 •

edited

Loading

tomcur Jul 7, 2026 •

edited

Loading

tomcur Jul 12, 2026 •

edited

Loading

tomcur Jul 12, 2026 •

edited

Loading

taj-p Jul 11, 2026 •

edited

Loading

tomcur Jul 12, 2026 •

edited

Loading