Skip to content

Commit

Permalink
[skrifa] glyf: use gvar for advance delta when HVAR is missing
Browse files Browse the repository at this point in the history 
We were using gvar to compute deltas for left side-bearings but not for advance widths when the HVAR table is missing.

Fixes #1311
  • Loading branch information
@dfrg
dfrg committed Jan 9, 2025
1 parent 6fba4bb commit 7ecd4d1
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 29 deletions.
34 changes: 23 additions & 11 deletions skrifa/src/outline/glyf/deltas.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<D: PointCoord>(
pub(super) fn composite_glyph<D: PointCoord>(
gvar: &Gvar,
glyph_id: GlyphId,
coords: &[F2Dot14],
Expand All @@ -33,7 +43,7 @@ pub fn composite_glyph<D: PointCoord>(
Ok(())
}

pub struct SimpleGlyph<'a, C: PointCoord> {
pub(super) struct SimpleGlyph<'a, C: PointCoord> {
pub points: &'a [Point<C>],
pub flags: &'a mut [PointFlags],
pub contours: &'a [u16],
Expand All @@ -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<C, D>(
pub(super) fn simple_glyph<C, D>(
gvar: &Gvar,
glyph_id: GlyphId,
coords: &[F2Dot14],
has_var_lsb: bool,
var_metrics: AvailableVarMetrics,
glyph: SimpleGlyph<C>,
iup_buffer: &mut [Point<D>],
deltas: &mut [Point<D>],
Expand All @@ -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| {
Expand Down
82 changes: 64 additions & 18 deletions skrifa/src/outline/glyf/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
}

Expand All @@ -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,
Expand Down Expand Up @@ -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,
})
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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();
Expand All @@ -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
// <https://github.com/googlefonts/fontations/issues/1311>
#[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);
}
}

0 comments on commit 7ecd4d1

Please sign in to comment.