[skrifa] more advance width adjustments (#1320)

This contains two fixes for scaler advance width computation.

1. Always round advance widths when hinting is requested, even when disabled by the prep table.
2. Only use the hdmx table when hinting is requested and backward compatibility mode is disabled.

This results in metrics that match FreeType for Arimo, Tinos and Market Sans.

font-test-data/src/lib.rs

@@ -90,6 +90,8 @@ pub static AVAR2_CHECKER: &[u8] = include_bytes!("../test_data/ttf/avar2checker.
 pub static MATERIAL_ICONS_SUBSET: &[u8] =
     include_bytes!("../test_data/ttf/material_icons_subset.ttf");
 
+pub static TINOS_SUBSET: &[u8] = include_bytes!("../test_data/ttf/tinos_subset.ttf");
+
 pub mod varc {
     pub static CJK_6868: &[u8] = include_bytes!("../test_data/ttf/varc-6868.ttf");
     pub static CONDITIONALS: &[u8] = include_bytes!("../test_data/ttf/varc-ac01-conditional.ttf");

font-test-data/test_data/README.md

@@ -89,6 +89,16 @@ Describes the provenance, usage and generation procedures for font data used for
   * license: [Apache 2][Apache2]
   * usage: testing avar2 userspace to normalized coordinate mapping
 
+* _tinos_subset_
+  * font: Tinos Regular
+  * source: https://fonts.google.com/specimen/Tinos
+  * license: [Apache 2][Apache2]
+  * usage: testing hdmx advance widths
+  * subset: a few characters to test hdmx advances
+    ```shell
+    pyftsubset tinos-regular.ttf --text=aAbB --gids=3,5
+    ```
+
 ## rebuilding
 To update the binaries and extracted data, run script located at `resources/test_fonts/rebuild.sh`
 This script will install the correct version of fonttools and FreeType, and then regenerate

skrifa/src/outline/glyf/hint/instance.rs

@@ -235,6 +235,17 @@ impl HintInstance {
     }
 }
 
+#[cfg(test)]
+impl HintInstance {
+    /// Enable instruct control bit 1 which effectively disables hinting.
+    ///
+    /// This mimics what the `prep` table might do for various configurations
+    /// and font sizes. Used for testing.    
+    pub fn simulate_prep_flag_suppress_hinting(&mut self) {
+        self.graphics.instruct_control |= 1;
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::{super::super::Outlines, HintInstance};

skrifa/src/outline/glyf/mod.rs

@@ -456,12 +456,26 @@ impl<'a> FreeTypeScaler<'a> {
         glyph_id: GlyphId,
     ) -> Result<ScaledOutline<'a, F26Dot6>, DrawError> {
         self.load(glyph, glyph_id, 0)?;
+        // Use hdmx if hinting is requested and backward compatibility mode
+        // is not enabled.
+        // <https://gitlab.freedesktop.org/freetype/freetype/-/blob/80a507a6b8e3d2906ad2c8ba69329bd2fb2a85ef/src/truetype/ttgload.c#L2559>
+        let hdmx_width = if self.is_hinted
+            && self
+                .hinter
+                .as_ref()
+                .map(|hinter| !hinter.backward_compatibility())
+                .unwrap_or(true)
+        {
+            self.outlines.hdmx_width(self.ppem, glyph_id)
+        } else {
+            None
+        };
         Ok(ScaledOutline::new(
             &mut self.memory.scaled[..self.point_count],
             self.phantom,
             &mut self.memory.flags[..self.point_count],
             &mut self.memory.contours[..self.contour_count],
-            self.outlines.hdmx_width(self.ppem, glyph_id),
+            hdmx_width,
         ))
     }
 }

skrifa/src/outline/hint.rs

@@ -526,3 +526,74 @@ impl Target {
         )
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::{
+        outline::{pen::NullPen, DrawSettings},
+        raw::TableProvider,
+        FontRef, MetadataProvider,
+    };
+
+    // FreeType ignores the hdmx table when backward compatibility mode
+    // is enabled in the TrueType interpreter.
+    #[test]
+    fn ignore_hdmx_when_back_compat_enabled() {
+        let font = FontRef::new(font_test_data::TINOS_SUBSET).unwrap();
+        let outlines = font.outline_glyphs();
+        // Double quote was the most egregious failure
+        let gid = font.charmap().map('"').unwrap();
+        let font_size = 16;
+        let hinter = HintingInstance::new(
+            &outlines,
+            Size::new(font_size as f32),
+            LocationRef::default(),
+            HintingOptions::default(),
+        )
+        .unwrap();
+        let HinterKind::Glyf(tt_hinter) = &hinter.kind else {
+            panic!("this is definitely a TrueType hinter");
+        };
+        // Make sure backward compatibility mode is enabled
+        assert!(tt_hinter.backward_compatibility());
+        let outline = outlines.get(gid).unwrap();
+        let metrics = outline.draw(&hinter, &mut NullPen).unwrap();
+        // FreeType computes an advance width of 7 when hinting but hdmx contains 5
+        let scaler_advance = metrics.advance_width.unwrap();
+        assert_eq!(scaler_advance, 7.0);
+        let hdmx_advance = font
+            .hdmx()
+            .unwrap()
+            .record_for_size(font_size)
+            .unwrap()
+            .widths()[gid.to_u32() as usize];
+        assert_eq!(hdmx_advance, 5);
+    }
+
+    // When hinting is disabled by the prep table, FreeType still returns
+    // rounded advance widths
+    #[test]
+    fn round_advance_when_prep_disables_hinting() {
+        let font = FontRef::new(font_test_data::TINOS_SUBSET).unwrap();
+        let outlines = font.outline_glyphs();
+        let gid = font.charmap().map('"').unwrap();
+        let size = Size::new(16.0);
+        let location = LocationRef::default();
+        let mut hinter =
+            HintingInstance::new(&outlines, size, location, HintingOptions::default()).unwrap();
+        let HinterKind::Glyf(tt_hinter) = &mut hinter.kind else {
+            panic!("this is definitely a TrueType hinter");
+        };
+        tt_hinter.simulate_prep_flag_suppress_hinting();
+        let outline = outlines.get(gid).unwrap();
+        // And we still have a rounded advance
+        let metrics = outline.draw(&hinter, &mut NullPen).unwrap();
+        assert_eq!(metrics.advance_width, Some(7.0));
+        // Unhinted advance has some fractional bits
+        let metrics = outline
+            .draw(DrawSettings::unhinted(size, location), &mut NullPen)
+            .unwrap();
+        assert_eq!(metrics.advance_width, Some(6.53125));
+    }
+}

skrifa/src/outline/mod.rs

@@ -111,6 +111,10 @@ use core::fmt::Debug;
 use pen::PathStyle;
 use read_fonts::{types::GlyphId, TableProvider};
 
+#[cfg(feature = "libm")]
+#[allow(unused_imports)]
+use core_maths::CoreFloat;
+
 /// Source format for an outline glyph.
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 pub enum OutlineGlyphFormat {
@@ -360,13 +364,19 @@ impl<'a> OutlineGlyph<'a> {
                         is_pedantic,
                     )
                 } else {
-                    self.draw_unhinted(
+                    let mut metrics = self.draw_unhinted(
                         hinting_instance.size(),
                         hinting_instance.location(),
                         settings.memory,
                         settings.path_style,
                         pen,
-                    )
+                    )?;
+                    // Round advance width when hinting is requested, even if
+                    // the instance is disabled.
+                    if let Some(advance) = metrics.advance_width.as_mut() {
+                        *advance = advance.round();
+                    }
+                    Ok(metrics)
                 }
             }
             (DrawInstance::Hinted { .. }, PathStyle::HarfBuzz) => {