Fix the VARC review findings of #298

crates/sigilbuzz-subset/src/lib.rs

@@ -706,16 +706,22 @@ fn push_layout_and_variation_tables(
     // are rewritten to point at the new gid namespace; the
     // MultiVarStore is pruned to the entries the kept records use. The
     // core parse only validates the table: `subset_varc` walks the raw
-    // bytes itself.
-    if face.varc()?.is_some() {
-        let varc_bytes = face.table_bytes(tag::VARC).map_err(SubsetError::from)?;
-        let lookup = |old: GlyphId| -> Option<GlyphId> {
-            let i = gid_map.binary_search_by_key(&old, |(o, _)| *o).ok()?;
-            gid_map.get(i).map(|&(_, new)| new)
-        };
-        if let Some(b) = varc::subset_varc(varc_bytes, kept, &lookup)? {
-            tables.push((tag::VARC, b));
+    // bytes itself. A VARC that does not parse counts as absent, as the
+    // face's outlines read it and as HarfBuzz drops it: it is left out
+    // with a warning, and its glyphs keep their glyf or CFF outlines.
+    match face.varc() {
+        Ok(Some(_)) => {
+            let varc_bytes = face.table_bytes(tag::VARC).map_err(SubsetError::from)?;
+            let lookup = |old: GlyphId| -> Option<GlyphId> {
+                let i = gid_map.binary_search_by_key(&old, |(o, _)| *o).ok()?;
+                gid_map.get(i).map(|&(_, new)| new)
+            };
+            if let Some(b) = varc::subset_varc(varc_bytes, kept, &lookup)? {
+                tables.push((tag::VARC, b));
+            }
         }
+        Ok(None) => {}
+        Err(_) => warnings.push(tag::VARC, 0, "VARC that does not parse", "the whole table"),
     }
     Ok(())
 }

crates/sigilbuzz-subset/tests/varc_unparsable.rs

@@ -0,0 +1,134 @@
+//! A `VARC` table the core cannot parse is left out of a subset with a
+//! warning, as the face's outlines read the font without it and as
+//! HarfBuzz drops it. Before, `subset` failed on such a font.
+//!
+//! The table splices in a MultiItemVariationStore in the layout HarfBuzz
+//! adopted after 14.5.0 (a 32-bit region count, and an Offset32 from each
+//! MultiVarData to its delta-set INDEX), which the core rejects.
+
+use sigilbuzz::Face;
+use sigilbuzz_subset::{subset, SubsetInput};
+
+const GLYF_FONT: &[u8] = include_bytes!("../../../tests/fixtures/varc_parity.ttf");
+
+/// A `VARC` table covering `covered`, each glyph one component naming
+/// glyph 1, over a MultiItemVariationStore in the post-14.5.0 layout.
+fn new_layout_varc(covered: &[u16]) -> Vec<u8> {
+    let mut coverage = vec![0, 1];
+    coverage.extend_from_slice(&(covered.len() as u16).to_be_bytes());
+    for g in covered {
+        coverage.extend_from_slice(&g.to_be_bytes());
+    }
+
+    let mut store = Vec::new();
+    store.extend_from_slice(&1u16.to_be_bytes()); // format
+    store.extend_from_slice(&12u32.to_be_bytes()); // region list
+    store.extend_from_slice(&1u16.to_be_bytes()); // one MultiVarData
+    store.extend_from_slice(&30u32.to_be_bytes()); // at 30
+    store.extend_from_slice(&1u32.to_be_bytes()); // region count (u32)
+    store.extend_from_slice(&8u32.to_be_bytes()); // Offset32 to the region
+    store.extend_from_slice(&1u16.to_be_bytes()); // axis count
+    store.extend_from_slice(&0u16.to_be_bytes()); // axis 0
+    store.extend_from_slice(&0i16.to_be_bytes());
+    store.extend_from_slice(&0x4000i16.to_be_bytes());
+    store.extend_from_slice(&0x4000i16.to_be_bytes());
+    assert_eq!(store.len(), 30);
+    store.push(1); // MultiVarData format
+    store.extend_from_slice(&1u16.to_be_bytes());
+    store.extend_from_slice(&0u16.to_be_bytes());
+    store.extend_from_slice(&9u32.to_be_bytes()); // Offset32 to the INDEX
+    store.extend_from_slice(&1u32.to_be_bytes());
+    store.extend_from_slice(&[1, 1, 3, 0x00, 100]);
+
+    let record = [0x18, 0x00, 0x01, 0x00, 0x00, 0x00];
+    let mut records = (covered.len() as u32).to_be_bytes().to_vec();
+    records.push(1); // offSize
+    for i in 0..=covered.len() {
+        records.push((1 + i * record.len()) as u8);
+    }
+    for _ in covered {
+        records.extend_from_slice(&record);
+    }
+
+    let mut out = Vec::new();
+    out.extend_from_slice(&[0, 1, 0, 0]); // version 1.0
+    let mut at = 24u32;
+    for part in [&coverage, &store] {
+        out.extend_from_slice(&at.to_be_bytes());
+        at += part.len() as u32;
+    }
+    out.extend_from_slice(&0u32.to_be_bytes()); // no conditions
+    out.extend_from_slice(&0u32.to_be_bytes()); // no axis indices
+    out.extend_from_slice(&at.to_be_bytes()); // glyph records
+    out.extend_from_slice(&coverage);
+    out.extend_from_slice(&store);
+    out.extend_from_slice(&records);
+    out
+}
+
+/// `font` with its tables rewritten into a new directory, `VARC`
+/// replaced by `varc`, or dropped when `varc` is `None`.
+fn with_varc(font: &[u8], varc: Option<&[u8]>) -> Vec<u8> {
+    let face = Face::parse_bytes(font, 0).unwrap();
+    let mut tables: Vec<([u8; 4], &[u8])> = face
+        .records()
+        .iter()
+        .filter(|r| &r.tag != b"VARC")
+        .map(|r| (r.tag, face.table_bytes(r.tag).unwrap()))
+        .collect();
+    if let Some(varc) = varc {
+        tables.push((*b"VARC", varc));
+    }
+    tables.sort_by_key(|t| t.0);
+    let header = 12 + 16 * tables.len();
+    let mut out = Vec::new();
+    out.extend_from_slice(&font[..4]);
+    out.extend_from_slice(&(tables.len() as u16).to_be_bytes());
+    out.extend_from_slice(&[0; 6]);
+    let mut body = Vec::new();
+    for (tag, data) in &tables {
+        out.extend_from_slice(tag);
+        out.extend_from_slice(&0u32.to_be_bytes());
+        out.extend_from_slice(&((header + body.len()) as u32).to_be_bytes());
+        out.extend_from_slice(&(data.len() as u32).to_be_bytes());
+        body.extend_from_slice(data);
+        while body.len() % 4 != 0 {
+            body.push(0);
+        }
+    }
+    out.extend_from_slice(&body);
+    out
+}
+
+#[test]
+fn a_subset_leaves_out_a_varc_that_does_not_parse() {
+    let bad_varc = new_layout_varc(&[2, 3]);
+    let with_bad = with_varc(GLYF_FONT, Some(&bad_varc));
+    let without = with_varc(GLYF_FONT, None);
+    let bad_face = Face::parse_bytes(&with_bad, 0).unwrap();
+    assert!(bad_face.varc().is_err(), "the spliced VARC must not parse");
+    let plain_face = Face::parse_bytes(&without, 0).unwrap();
+
+    for drop_unhandled in [false, true] {
+        let input = SubsetInput {
+            gids: vec![0, 1, 2, 3],
+            drop_unhandled,
+            ..Default::default()
+        };
+        let got = subset(&bad_face, &input).expect("subset succeeds without VARC");
+        let want = subset(&plain_face, &input).expect("subset of the font without VARC");
+        assert_eq!(
+            got.bytes, want.bytes,
+            "the subset equals the subset of the font without VARC (drop_unhandled {drop_unhandled})"
+        );
+        let out = Face::parse_bytes(&got.bytes, 0).unwrap();
+        assert!(out.record(*b"VARC").is_none(), "VARC is left out");
+        assert!(
+            got.warnings
+                .iter()
+                .any(|w| w.table == *b"VARC" && w.dropped == "the whole table"),
+            "a warning names the dropped VARC: {:?}",
+            got.warnings
+        );
+    }
+}

src/face/glyph_outlines.rs

@@ -24,7 +24,8 @@ use crate::tables::{tag, Cff, Cff2, Glyf, Gvar, Hmtx, Loca, Outline, OutlineSink
 /// them: the `CFF2` or `CFF ` table, or `glyf` with `loca`, `gvar` and
 /// the metrics its phantom points read, and `VARC`. Each glyph comes out
 /// exactly as [`Face::glyph_outline_at_coords`] draws it, errors
-/// included: a table that does not read fails every glyph that needs it.
+/// included: a table that does not read fails every glyph that needs it,
+/// except `VARC`, which counts as absent then, as for the face.
 ///
 /// It caches through cells that are not thread-safe, so it is `Send` but
 /// not `Sync`: build one per thread.
@@ -48,7 +49,8 @@ pub struct GlyphOutlines<'a> {
     face: Face<'a>,
     /// The coords as shaping reads them, rounded to F2DOT14.
     coords: Vec<f32>,
-    varc: OnceCell<Result<Option<Varc<'a>>>>,
+    /// The face's `VARC`, `None` when it has none or it does not parse.
+    varc: OnceCell<Option<Varc<'a>>>,
     tables: OnceCell<Result<Tables<'a>>>,
 }
 
@@ -111,11 +113,7 @@ impl<'a> GlyphOutlines<'a> {
     /// The errors [`Face::glyph_outline_at_coords`] returns for the
     /// glyph.
     pub fn draw<S: OutlineSink>(&self, glyph_id: u16, sink: &mut S) -> Result<bool> {
-        let varc = self
-            .varc
-            .get_or_init(|| self.face.varc())
-            .as_ref()
-            .map_err(Clone::clone)?;
+        let varc = self.varc.get_or_init(|| self.face.drawable_varc());
         // A VARC composite is drawn by the walk the face draws it with:
         // its components carry coords of their own, so its leaves come
         // from tables the walk reads for them.

src/face/outlines.rs

@@ -175,16 +175,24 @@ impl<'a> Face<'a> {
     /// given.
     ///
     /// A glyph the font's `VARC` table covers is drawn from its
-    /// components, as HarfBuzz draws it:
+    /// components, as HarfBuzz draws it (a `VARC` table that does not
+    /// parse counts as absent, as HarfBuzz drops it):
     ///
     /// - Each component is drawn at its own coords, which start from
     ///   the coords of the glyph it belongs to, or from the font's
     ///   (`coords`) when it sets `RESET_UNSPECIFIED_AXES`, however deep
     ///   it sits.
     /// - A component that names the glyph it belongs to draws that
-    ///   glyph's `glyf` or CFF outline instead of recursing. A longer
-    ///   cycle is cut where HarfBuzz's decycler cuts it, and a covered
+    ///   glyph's `glyf` or CFF outline instead of recursing. A covered
     ///   glyph past the end of the glyph records draws nothing.
+    /// - A cycle of `n` glyphs is cut where HarfBuzz's decycler cuts it,
+    ///   `2n - 1` levels down. A cycle of more than 32 glyphs reaches
+    ///   the 64-level cap first and fails the glyph, where HarfBuzz stops
+    ///   expanding at that depth and draws the rest.
+    /// - Leaves (glyphs drawn from `glyf` or CFF) share HarfBuzz's 2^24
+    ///   glyph budget for their `gvar` and charstring work, and draw
+    ///   nothing once it runs out, as HarfBuzz's do; a leaf reached again
+    ///   at the same coords is drawn once.
     /// - At the default instance of a font with `fvar` (`coords` empty or
     ///   all zero), VARC reads one zero coord per axis, as HarfBuzz's
     ///   font holds them, so deltas from a region that constrains no
@@ -201,7 +209,7 @@ impl<'a> Face<'a> {
         coords: &[f32],
     ) -> Result<Option<Outline>> {
         let coords = f2dot14_coords(coords);
-        if let Some(varc) = self.varc()? {
+        if let Some(varc) = self.drawable_varc() {
             let mut out = Outline::new();
             if VarcDraw::new(self, &varc, &coords).draw(glyph_id, &mut out)? {
                 return Ok(Some(out));