From 036b3e6d83ab7ca6e1b1447b487cb4d9e297cb6b Mon Sep 17 00:00:00 2001 From: Chad Brokaw Date: Thu, 9 Jan 2025 13:07:08 -0500 Subject: [PATCH] [skrifa] glyf: use gvar for advance delta when HVAR is missing (#1312) We were using gvar to compute deltas for left side-bearings but not for advance widths when the HVAR table is missing. Fixes #1311 --- skrifa/src/outline/glyf/deltas.rs | 34 ++++++++----- skrifa/src/outline/glyf/mod.rs | 82 ++++++++++++++++++++++++------- 2 files changed, 87 insertions(+), 29 deletions(-) diff --git a/skrifa/src/outline/glyf/deltas.rs b/skrifa/src/outline/glyf/deltas.rs index 237716c03..c9d0e9e51 100644 --- a/skrifa/src/outline/glyf/deltas.rs +++ b/skrifa/src/outline/glyf/deltas.rs @@ -11,11 +11,21 @@ use read_fonts::{ use super::PHANTOM_POINT_COUNT; +#[derive(Copy, Clone, Debug)] +pub(super) enum AvailableVarMetrics { + /// Full variable metrics with advances and side-bearings. + Present, + /// Variable advances but not side-bearings. + MissingSideBearings, + /// No variable metrics table. + Missing, +} + /// Compute a set of deltas for the component offsets of a composite glyph. /// /// Interpolation is meaningless for component offsets so this is a /// specialized function that skips the expensive bits. -pub fn composite_glyph( +pub(super) fn composite_glyph( gvar: &Gvar, glyph_id: GlyphId, coords: &[F2Dot14], @@ -33,7 +43,7 @@ pub fn composite_glyph( Ok(()) } -pub struct SimpleGlyph<'a, C: PointCoord> { +pub(super) struct SimpleGlyph<'a, C: PointCoord> { pub points: &'a [Point], pub flags: &'a mut [PointFlags], pub contours: &'a [u16], @@ -44,11 +54,11 @@ pub struct SimpleGlyph<'a, C: PointCoord> { /// This function will use interpolation to infer missing deltas for tuples /// that contain sparse sets. The `iup_buffer` buffer is temporary storage /// used for this and the length must be >= glyph.points.len(). -pub fn simple_glyph( +pub(super) fn simple_glyph( gvar: &Gvar, glyph_id: GlyphId, coords: &[F2Dot14], - has_var_lsb: bool, + var_metrics: AvailableVarMetrics, glyph: SimpleGlyph, iup_buffer: &mut [Point], deltas: &mut [Point], @@ -73,13 +83,15 @@ where flags, contours, } = glyph; - // Include the first phantom point if the font is missing variable metrics - // for left side bearings. The adjustment made here may affect the final - // shift of the outline. - let actual_len = if has_var_lsb { - points.len() - 4 - } else { - points.len() - 3 + // Determine which phantom points to modify based on the presence and + // content of the HVAR table. + let actual_len = match var_metrics { + // Modify LSB and advance + AvailableVarMetrics::Missing => points.len() - 2, + // Modify only LSB + AvailableVarMetrics::MissingSideBearings => points.len() - 3, + // Modify nothing + AvailableVarMetrics::Present => points.len() - 4, }; let deltas = &mut deltas[..actual_len]; compute_deltas_for_glyph(gvar, glyph_id, coords, deltas, |scalar, tuple, deltas| { diff --git a/skrifa/src/outline/glyf/mod.rs b/skrifa/src/outline/glyf/mod.rs index 31c228fdb..9a644bf77 100644 --- a/skrifa/src/outline/glyf/mod.rs +++ b/skrifa/src/outline/glyf/mod.rs @@ -9,6 +9,7 @@ mod outline; #[allow(unused_imports)] use core_maths::CoreFloat; +use deltas::AvailableVarMetrics; pub use hint::{HintError, HintInstance, HintOutline}; pub use outline::{Outline, ScaledOutline}; use raw::FontRef; @@ -53,7 +54,7 @@ pub struct Outlines<'a> { glyph_count: u16, units_per_em: u16, os2_vmetrics: [i16; 2], - has_var_lsb: bool, + var_metrics: AvailableVarMetrics, prefer_interpreter: bool, } @@ -62,11 +63,16 @@ impl<'a> Outlines<'a> { let loca = font.loca(None).ok()?; let glyf = font.glyf().ok()?; let glyph_metrics = GlyphHMetrics::new(font)?; - let has_var_lsb = glyph_metrics - .hvar - .as_ref() - .map(|hvar| hvar.lsb_mapping().is_some()) - .unwrap_or_default(); + let var_metrics = match glyph_metrics.hvar.as_ref() { + Some(hvar) => { + if hvar.lsb_mapping().is_some() { + AvailableVarMetrics::Present + } else { + AvailableVarMetrics::MissingSideBearings + } + } + None => AvailableVarMetrics::Missing, + }; let ( glyph_count, max_function_defs, @@ -131,7 +137,7 @@ impl<'a> Outlines<'a> { glyph_count, units_per_em: font.head().ok()?.units_per_em(), os2_vmetrics, - has_var_lsb, + var_metrics, prefer_interpreter, }) } @@ -597,7 +603,7 @@ impl Scaler for FreeTypeScaler<'_> { gvar, glyph_id, self.coords, - self.outlines.has_var_lsb, + self.outlines.var_metrics, glyph, iup_buffer, deltas, @@ -742,10 +748,16 @@ impl Scaler for FreeTypeScaler<'_> { .get_mut(delta_base..delta_base + count) .ok_or(InsufficientMemory)?; if deltas::composite_glyph(gvar, glyph_id, self.coords, &mut deltas[..]).is_ok() { - // If the font is missing variation data for LSBs in HVAR then we - // apply the delta to the first phantom point. - if !self.outlines.has_var_lsb { - self.phantom[0].x += F26Dot6::from_bits(deltas[count - 4].x.to_i32()); + // Apply selective deltas to phantom points. + match self.outlines.var_metrics { + AvailableVarMetrics::Missing => { + self.phantom[0].x += F26Dot6::from_bits(deltas[count - 4].x.to_i32()); + self.phantom[1].x += F26Dot6::from_bits(deltas[count - 3].x.to_i32()); + } + AvailableVarMetrics::MissingSideBearings => { + self.phantom[0].x += F26Dot6::from_bits(deltas[count - 4].x.to_i32()); + } + _ => {} } have_deltas = true; } @@ -1107,7 +1119,7 @@ impl Scaler for HarfBuzzScaler<'_> { gvar, glyph_id, self.coords, - self.outlines.has_var_lsb, + self.outlines.var_metrics, glyph, iup_buffer, deltas, @@ -1159,10 +1171,16 @@ impl Scaler for HarfBuzzScaler<'_> { .get_mut(delta_base..delta_base + count) .ok_or(InsufficientMemory)?; if deltas::composite_glyph(gvar, glyph_id, self.coords, &mut deltas[..]).is_ok() { - // If the font is missing variation data for LSBs in HVAR then we - // apply the delta to the first phantom point. - if !self.outlines.has_var_lsb { - self.phantom[0].x += deltas[count - 4].x; + // Apply selective deltas to phantom points. + match self.outlines.var_metrics { + AvailableVarMetrics::Missing => { + self.phantom[0].x += deltas[count - 4].x; + self.phantom[1].x += deltas[count - 3].x; + } + AvailableVarMetrics::MissingSideBearings => { + self.phantom[0].x += deltas[count - 4].x; + } + _ => {} } have_deltas = true; } @@ -1328,8 +1346,9 @@ mod tests { FreeTypeScaler::unhinted(&outlines, &outline, &mut buf, ppem, &coords).unwrap(); let scaled = scaler.scale(&outline.glyph, gid).unwrap(); let advance_hvar = scaled.adjusted_advance_width(); - // Set HVAR table to None to force loading metrics from gvar + // Set HVAR to None and mark var metrics as missing so we pull deltas from gvar outlines.glyph_metrics.hvar = None; + outlines.var_metrics = AvailableVarMetrics::Missing; let scaler = FreeTypeScaler::unhinted(&outlines, &outline, &mut buf, ppem, &coords).unwrap(); let scaled = scaler.scale(&outline.glyph, gid).unwrap(); @@ -1348,4 +1367,31 @@ mod tests { assert!(outline.glyph.is_none()); assert_eq!(outline.points, PHANTOM_POINT_COUNT); } + + // Pull metric deltas from gvar when hvar is not present + // + #[test] + fn missing_hvar_advance() { + let font = FontRef::new(font_test_data::HVAR_WITH_TRUNCATED_ADVANCE_INDEX_MAP).unwrap(); + let mut outlines = Outlines::new(&font).unwrap(); + let coords = [F2Dot14::from_f32(0.5)]; + let ppem = Some(24.0); + let gid = font.charmap().map('A').unwrap(); + let outline = outlines.outline(gid).unwrap(); + let mut buf = [0u8; 1024]; + let scaler = + FreeTypeScaler::unhinted(&outlines, &outline, &mut buf, ppem, &coords).unwrap(); + let scaled = scaler.scale(&outline.glyph, gid).unwrap(); + let advance_hvar = scaled.adjusted_advance_width(); + // Set HVAR to None and mark var metrics as missing so we pull deltas from gvar + outlines.glyph_metrics.hvar = None; + outlines.var_metrics = AvailableVarMetrics::Missing; + let scaler = + FreeTypeScaler::unhinted(&outlines, &outline, &mut buf, ppem, &coords).unwrap(); + let scaled = scaler.scale(&outline.glyph, gid).unwrap(); + let advance_gvar = scaled.adjusted_advance_width(); + // Make sure we have an advance and that the two are the same + assert!(advance_hvar != F26Dot6::ZERO); + assert_eq!(advance_hvar, advance_gvar); + } }