From 94bfb1ac61880343377deda2c1bd97a69d913f94 Mon Sep 17 00:00:00 2001 From: Behdad Esfahbod Date: Mon, 5 Oct 2026 14:40:39 -0600 Subject: [PATCH] [gpos] Share cursive attachment across glyph-ID widths Keep the format-specific coverage and anchor-offset readers in their wrappers, and share anchor resolution and attachment adjustment through a non-generic, non-inlined helper. Reuse child and parent position references instead of repeating bounds-checked indexing. Exercise both CursivePos formats in the all-direction attachment test, including the right-to-left lookup flag. No allocations or trait-object dispatch are added. On x86-64 Rust 1.89 release/fat-LTO builds, ELF text plus data shrinks by 920 bytes for hr-shape and 928 bytes for the C API. Stripped file sizes are unchanged because of segment alignment. Alternating pinned shaping benchmarks on Arabic, Urdu and Latin samples remain within about 1%. Tested with the complete all-feature workspace suite (including 6073 shaping regressions), strict Clippy, formatting, and a no-std/libm check. Shaping output and flags match the baseline in all four directions. Assisted-by: OpenAI Codex --- harfrust/src/ot/gpos/cursive.rs | 210 ++++++++++++++++-------------- harfrust/src/ot/tests/extended.rs | 61 +++++---- 2 files changed, 149 insertions(+), 122 deletions(-) diff --git a/harfrust/src/ot/gpos/cursive.rs b/harfrust/src/ot/gpos/cursive.rs index 64d2424e..cf8e1e25 100644 --- a/harfrust/src/ot/gpos/cursive.rs +++ b/harfrust/src/ot/gpos/cursive.rs @@ -4,7 +4,7 @@ use crate::ot::apply::{Apply, SkippingIterator}; use crate::ot::gpos::attach_type; use crate::ot::lookup_flags; use crate::{Direction, GlyphPosition}; -use read_fonts::tables::gpos::{CursivePosFormat1, CursivePosFormat2}; +use read_fonts::tables::gpos::{AnchorTable, CursivePosFormat1, CursivePosFormat2}; macro_rules! impl_cursive_pos { ($format:ident) => { @@ -44,103 +44,7 @@ macro_rules! impl_cursive_pos { return None; }; - let (exit_x, exit_y) = ctx.layout.ot.resolve_anchor(&exit_prev); - let (entry_x, entry_y) = ctx.layout.ot.resolve_anchor(&entry_this); - let exit_x = ctx.scale_x(exit_x); - let exit_y = ctx.scale_y(exit_y); - let entry_x = ctx.scale_x(entry_x); - let entry_y = ctx.scale_y(entry_y); - - let direction = ctx.buffer.direction; - let j = ctx.buffer.idx; - ctx.buffer.unsafe_to_break(Some(i), Some(j + 1)); - - let pos = &mut ctx.buffer.pos; - match direction { - Direction::LeftToRight => { - pos[i].x_advance = exit_x.saturating_add(pos[i].x_offset); - let d = entry_x.saturating_add(pos[j].x_offset); - pos[j].x_advance = pos[j].x_advance.saturating_sub(d); - pos[j].x_offset = pos[j].x_offset.saturating_sub(d); - } - Direction::RightToLeft => { - let d = exit_x.saturating_add(pos[i].x_offset); - pos[i].x_advance = pos[i].x_advance.saturating_sub(d); - pos[i].x_offset = pos[i].x_offset.saturating_sub(d); - pos[j].x_advance = entry_x.saturating_add(pos[j].x_offset); - } - Direction::TopToBottom => { - pos[i].y_advance = exit_y.saturating_add(pos[i].y_offset); - let d = entry_y.saturating_add(pos[j].y_offset); - pos[j].y_advance = pos[j].y_advance.saturating_sub(d); - pos[j].y_offset = pos[j].y_offset.saturating_sub(d); - } - Direction::BottomToTop => { - let d = exit_y.saturating_add(pos[i].y_offset); - pos[i].y_advance = pos[i].y_advance.saturating_sub(d); - pos[i].y_offset = pos[i].y_offset.saturating_sub(d); - pos[j].y_advance = entry_y; - } - Direction::Invalid => {} - } - - // Cross-direction adjustment - - // We attach child to parent (think graph theory and rooted trees whereas - // the root stays on baseline and each node aligns itself against its - // parent. - // - // Optimize things for the case of RightToLeft, as that's most common in - // Arabic. - let mut child = i; - let mut parent = j; - let mut x_offset = entry_x.saturating_sub(exit_x); - let mut y_offset = entry_y.saturating_sub(exit_y); - - // Low bits are lookup flags, so we want to truncate. - if ctx.lookup_props as u16 & lookup_flags::RIGHT_TO_LEFT == 0 { - core::mem::swap(&mut child, &mut parent); - x_offset = x_offset.saturating_neg(); - y_offset = y_offset.saturating_neg(); - } - - // If child was already connected to someone else, walk through its old - // chain and reverse the link direction, such that the whole tree of its - // previous connection now attaches to new parent. Watch out for case - // where new parent is on the path from old chain... - reverse_cursive_minor_offset(pos, child, direction, parent); - - pos[child].set_attach_type(attach_type::CURSIVE); - let chain = parent as isize - child as isize; - pos[child].set_attach_chain(chain as i16); - // If the distance between the two glyphs does not fit in the i16 chain - // field it would be truncated to a bogus value; leave the glyph - // unattached instead of storing a poisoned chain. Matches HarfBuzz. - if isize::from(pos[child].attach_chain()) != chain { - pos[child].set_attach_chain(0); - } - - ctx.buffer.scratch_flags |= HB_BUFFER_SCRATCH_FLAG_HAS_GPOS_ATTACHMENT; - if direction.is_horizontal() { - pos[child].y_offset = y_offset; - } else { - pos[child].x_offset = x_offset; - } - - // If parent was attached to child, separate them. - // https://github.com/harfbuzz/harfbuzz/issues/2469 - if pos[parent].attach_chain() == -pos[child].attach_chain() { - pos[parent].set_attach_chain(0); - - if direction.is_horizontal() { - pos[parent].y_offset = 0; - } else { - pos[parent].x_offset = 0; - } - } - - ctx.buffer.idx += 1; - Some(()) + apply_cursive_attachment(ctx, i, &entry_this, &exit_prev) } } }; @@ -149,6 +53,116 @@ macro_rules! impl_cursive_pos { impl_cursive_pos!(CursivePosFormat1); impl_cursive_pos!(CursivePosFormat2); +// Keep the format-independent attachment logic shared by both widths. +#[inline(never)] +fn apply_cursive_attachment( + ctx: &mut ApplyContext, + i: usize, + entry_anchor: &AnchorTable<'_>, + exit_anchor: &AnchorTable<'_>, +) -> Option<()> { + let (exit_x, exit_y) = ctx.layout.ot.resolve_anchor(exit_anchor); + let (entry_x, entry_y) = ctx.layout.ot.resolve_anchor(entry_anchor); + let exit_x = ctx.scale_x(exit_x); + let exit_y = ctx.scale_y(exit_y); + let entry_x = ctx.scale_x(entry_x); + let entry_y = ctx.scale_y(entry_y); + + let direction = ctx.buffer.direction; + let j = ctx.buffer.idx; + ctx.buffer.unsafe_to_break(Some(i), Some(j + 1)); + + let pos = &mut ctx.buffer.pos; + match direction { + Direction::LeftToRight => { + pos[i].x_advance = exit_x.saturating_add(pos[i].x_offset); + let d = entry_x.saturating_add(pos[j].x_offset); + pos[j].x_advance = pos[j].x_advance.saturating_sub(d); + pos[j].x_offset = pos[j].x_offset.saturating_sub(d); + } + Direction::RightToLeft => { + let d = exit_x.saturating_add(pos[i].x_offset); + pos[i].x_advance = pos[i].x_advance.saturating_sub(d); + pos[i].x_offset = pos[i].x_offset.saturating_sub(d); + pos[j].x_advance = entry_x.saturating_add(pos[j].x_offset); + } + Direction::TopToBottom => { + pos[i].y_advance = exit_y.saturating_add(pos[i].y_offset); + let d = entry_y.saturating_add(pos[j].y_offset); + pos[j].y_advance = pos[j].y_advance.saturating_sub(d); + pos[j].y_offset = pos[j].y_offset.saturating_sub(d); + } + Direction::BottomToTop => { + let d = exit_y.saturating_add(pos[i].y_offset); + pos[i].y_advance = pos[i].y_advance.saturating_sub(d); + pos[i].y_offset = pos[i].y_offset.saturating_sub(d); + pos[j].y_advance = entry_y; + } + Direction::Invalid => {} + } + + // Cross-direction adjustment + + // We attach child to parent (think graph theory and rooted trees whereas + // the root stays on baseline and each node aligns itself against its + // parent. + // + // Optimize things for the case of RightToLeft, as that's most common in + // Arabic. + let mut child = i; + let mut parent = j; + let mut x_offset = entry_x.saturating_sub(exit_x); + let mut y_offset = entry_y.saturating_sub(exit_y); + + // Low bits are lookup flags, so we want to truncate. + if ctx.lookup_props as u16 & lookup_flags::RIGHT_TO_LEFT == 0 { + core::mem::swap(&mut child, &mut parent); + x_offset = x_offset.saturating_neg(); + y_offset = y_offset.saturating_neg(); + } + + // If child was already connected to someone else, walk through its old + // chain and reverse the link direction, such that the whole tree of its + // previous connection now attaches to new parent. Watch out for case + // where new parent is on the path from old chain... + reverse_cursive_minor_offset(pos, child, direction, parent); + + let child_pos = &mut pos[child]; + child_pos.set_attach_type(attach_type::CURSIVE); + let chain = parent as isize - child as isize; + child_pos.set_attach_chain(chain as i16); + // If the distance between the two glyphs does not fit in the i16 chain + // field it would be truncated to a bogus value; leave the glyph + // unattached instead of storing a poisoned chain. Matches HarfBuzz. + if isize::from(child_pos.attach_chain()) != chain { + child_pos.set_attach_chain(0); + } + + ctx.buffer.scratch_flags |= HB_BUFFER_SCRATCH_FLAG_HAS_GPOS_ATTACHMENT; + if direction.is_horizontal() { + child_pos.y_offset = y_offset; + } else { + child_pos.x_offset = x_offset; + } + let child_chain = child_pos.attach_chain(); + + // If parent was attached to child, separate them. + // https://github.com/harfbuzz/harfbuzz/issues/2469 + let parent_pos = &mut pos[parent]; + if parent_pos.attach_chain() == -child_chain { + parent_pos.set_attach_chain(0); + + if direction.is_horizontal() { + parent_pos.y_offset = 0; + } else { + parent_pos.x_offset = 0; + } + } + + ctx.buffer.idx += 1; + Some(()) +} + fn reverse_cursive_minor_offset( pos: &mut [GlyphPosition], i: usize, diff --git a/harfrust/src/ot/tests/extended.rs b/harfrust/src/ot/tests/extended.rs index eed8114c..fedf54d3 100644 --- a/harfrust/src/ot/tests/extended.rs +++ b/harfrust/src/ot/tests/extended.rs @@ -608,8 +608,16 @@ fn single_pos4_rejects_coverage_indices_outside_the_value_array() { } #[test] -fn cursive_pos2_attaches_wide_glyphs_in_all_directions() { - let subtable = [ +fn cursive_pos_formats_attach_glyphs_in_all_directions() { + let narrow = [ + 0, 1, 0, 14, 0, 2, // Header and two records. + 0, 0, 0, 22, // First glyph's exit anchor. + 0, 28, 0, 0, // Second glyph's entry anchor. + 0, 1, 0, 2, 0, 1, 0, 2, // Coverage. + 0, 1, 0, 100, 0, 50, // Exit anchor. + 0, 1, 0, 20, 0, 10, // Entry anchor. + ]; + let wide = [ 0, 2, 0, 0, 0, 21, 0, 0, 2, // Header and two records. 0, 0, 0, 0, 0, 32, // First glyph's exit anchor. 0, 0, 38, 0, 0, 0, // Second glyph's entry anchor. @@ -617,33 +625,38 @@ fn cursive_pos2_attaches_wide_glyphs_in_all_directions() { 0, 1, 0, 100, 0, 50, // Exit anchor. 0, 1, 0, 20, 0, 10, // Entry anchor. ]; - for (direction, expected) in [ - (Direction::LeftToRight, [[0, 0, 100, 0], [-20, 40, -20, 0]]), - (Direction::RightToLeft, [[-100, 0, -100, 0], [0, 40, 20, 0]]), - (Direction::TopToBottom, [[0, 0, 0, 50], [80, -10, 0, -10]]), - (Direction::BottomToTop, [[0, -50, 0, -50], [80, 0, 0, 10]]), + for (subtable, glyphs) in [ + (narrow.as_slice(), [1, 2]), + (wide.as_slice(), [65536, 65537]), ] { - let output = apply_subtable_configured(3, false, &subtable, &[65536, 65537], |ctx| { - ctx.buffer.set_direction(direction); - }); - for (pos, expected) in output.glyph_positions().iter().zip(expected) { + for (direction, expected) in [ + (Direction::LeftToRight, [[0, 0, 100, 0], [-20, 40, -20, 0]]), + (Direction::RightToLeft, [[-100, 0, -100, 0], [0, 40, 20, 0]]), + (Direction::TopToBottom, [[0, 0, 0, 50], [80, -10, 0, -10]]), + (Direction::BottomToTop, [[0, -50, 0, -50], [80, 0, 0, 10]]), + ] { + let output = apply_subtable_configured(3, false, subtable, &glyphs, |ctx| { + ctx.buffer.set_direction(direction); + }); + for (pos, expected) in output.glyph_positions().iter().zip(expected) { + assert_eq!( + [pos.x_offset, pos.y_offset, pos.x_advance, pos.y_advance], + expected + ); + } + assert_eq!(output.glyph_positions()[1].attach_chain(), -1); assert_eq!( - [pos.x_offset, pos.y_offset, pos.x_advance, pos.y_advance], - expected + output.glyph_positions()[1].attach_type(), + gpos::attach_type::CURSIVE ); } - assert_eq!(output.glyph_positions()[1].attach_chain(), -1); - assert_eq!( - output.glyph_positions()[1].attach_type(), - gpos::attach_type::CURSIVE - ); - } - let output = apply_subtable_configured(3, false, &subtable, &[65536, 65537], |ctx| { - ctx.lookup_props |= u32::from(lookup_flags::RIGHT_TO_LEFT); - }); - assert_eq!(output.glyph_positions()[0].attach_chain(), 1); - assert_eq!(output.glyph_positions()[0].y_offset, -40); + let output = apply_subtable_configured(3, false, subtable, &glyphs, |ctx| { + ctx.lookup_props |= u32::from(lookup_flags::RIGHT_TO_LEFT); + }); + assert_eq!(output.glyph_positions()[0].attach_chain(), 1); + assert_eq!(output.glyph_positions()[0].y_offset, -40); + } } #[test]