[write-fonts] Fix miscalculation of ppf2 subgraph size

I had run into a problem here that I didn't have time to dig into last
week.

It turns out were incrementing the current index only when the subtable had
not been seen before, which meant we would never advance the index and would
not visit all subtables.

write-fonts/src/graph/splitting.rs

@@ -204,6 +204,7 @@ fn split_off_ppf1(graph: &mut Graph, subtable: ObjectId, start: u16, end: u16) -
 // based off of
 // <https://github.com/harfbuzz/harfbuzz/blob/f380a32825a1b2c51bbe21dc7acb9b3cc0921f69/src/graph/pairpos-graph.hh#L207>
 fn split_pair_pos_format_2(graph: &mut Graph, subtable: ObjectId) -> Option<Vec<ObjectId>> {
+    // the minimum size of a format 2 subtable
     const BASE_SIZE: usize = 8 * u16::RAW_BYTE_LEN;
     let data = &graph.objects[&subtable];
 
@@ -279,12 +280,10 @@ fn split_pair_pos_format_2(graph: &mut Graph, subtable: ObjectId) -> Option<Vec<
         }
 
         accumulated += accumulated_delta;
-        let _largest_obj = coverage_size.max(class_def_1_size).max(class_def2_size);
-        let total = accumulated + coverage_size + class_def_1_size + class_def2_size;
-        //NOTE: harfbuzz has this optimization but I was seeing an overflow with
-        //the last object in some cases, so currently disabled
-        // largest obj packs last and can overflow
-        //- _largest_obj;
+        let largest_obj = coverage_size.max(class_def_1_size).max(class_def2_size);
+        let total = accumulated + coverage_size + class_def_1_size + class_def2_size
+        // largest obj packs last and can overflow (we only point to the start)
+        - largest_obj;
 
         if total > MAX_TABLE_SIZE {
             split_points.push(idx);
@@ -630,12 +629,10 @@ fn size_of_value_record_children(
         .filter_map(|offset| (!offset.is_null()).then_some(*offset.offset()))
         .map(|_| {
             let obj = offsets[*next_offset_idx].object;
-            if seen.insert(obj) {
-                *next_offset_idx += 1;
-                graph.objects[&obj].bytes.len()
-            } else {
-                0
-            }
+            *next_offset_idx += 1;
+            seen.insert(obj)
+                .then(|| graph.objects[&obj].bytes.len())
+                .unwrap_or(0)
         })
         .sum()
 }
@@ -668,7 +665,7 @@ mod tests {
                 Class1Record, Class2Record, PairPos, PairSet, PairValueRecord, PositionLookup,
                 ValueRecord,
             },
-            layout::{CoverageTableBuilder, DeviceOrVariationIndex, VariationIndex},
+            layout::{CoverageTableBuilder, Device, DeviceOrVariationIndex, VariationIndex},
         },
         FontWrite, TableWriter,
     };
@@ -915,6 +912,20 @@ mod tests {
         assert_eq!(count_num_ranges(&make_input(&[1, 2, 3, 5, 6, 7, 10])), 3);
     }
 
+    fn dummy_class_def(
+        n_classes: u16,
+        n_glyphs_per_class: u16,
+        first_gid: u16,
+    ) -> wlayout::ClassDef {
+        let n_glyphs = n_classes * n_glyphs_per_class;
+        (first_gid..first_gid + n_glyphs)
+            .map(|gid| {
+                let class = (gid - 1) / n_glyphs_per_class;
+                (GlyphId::new(gid), class)
+            })
+            .collect()
+    }
+
     fn make_pairpos2() -> PositionLookup {
         fn next_class2_rec(i: usize) -> Class2Record {
             // idk how better to cast bits directly
@@ -945,24 +956,11 @@ mod tests {
             }
         }
 
-        fn make_class_def(
-            n_classes: u16,
-            n_glyphs_per_class: u16,
-            first_gid: u16,
-        ) -> wlayout::ClassDef {
-            let n_glyphs = n_classes * n_glyphs_per_class;
-            (first_gid..first_gid + n_glyphs)
-                .map(|gid| {
-                    let class = (gid - 1) / n_glyphs_per_class;
-                    (GlyphId::new(gid), class)
-                })
-                .collect()
-        }
         const CLASS1_COUNT: u16 = 100;
         const CLASS2_COUNT: u16 = 100;
 
-        let class_def1 = make_class_def(CLASS1_COUNT, 4, 1);
-        let class_def2 = make_class_def(CLASS2_COUNT, 3, 1);
+        let class_def1 = dummy_class_def(CLASS1_COUNT, 4, 1);
+        let class_def2 = dummy_class_def(CLASS2_COUNT, 3, 1);
 
         assert_eq!(class_def1.class_count(), CLASS1_COUNT);
         assert_eq!(class_def2.class_count(), CLASS2_COUNT);
@@ -1066,4 +1064,79 @@ mod tests {
             .sum();
         assert_eq!(total_c2recs, expected_n_c2_recs);
     }
+
+    #[test]
+    fn size_of_value_record_children_sanity() {
+        // let's have  single class1class, and three class2 classes
+        // we want a duplicate varidx, a null varidx, and a device table?
+
+        fn val_record_with_xadv(x_advance: i16) -> ValueRecord {
+            let format = ValueFormat::X_ADVANCE | ValueFormat::X_ADVANCE_DEVICE;
+            ValueRecord::new()
+                .with_explicit_value_format(format)
+                .with_x_advance(x_advance)
+        }
+
+        // number of classes, number of glyphs per class, GID of first glyph
+        let class_def1 = dummy_class_def(1, 4, 1);
+        let class_def2 = dummy_class_def(3, 3, 1);
+        let coverage = class_def1.iter().map(|(gid, _)| gid).collect();
+        let actual_device_table = Device::new(12, 15, &[118, 119, 127, 99]);
+        // sanity check the size of the device table, these are weird:
+        assert_eq!(crate::dump_table(&actual_device_table).unwrap().len(), 10);
+        let class1_records = vec![Class1Record::new(vec![
+            Class2Record::new(
+                val_record_with_xadv(5),
+                val_record_with_xadv(6)
+                    .with_x_advance_device(DeviceOrVariationIndex::variation_index(4, 20)),
+            ),
+            Class2Record::new(
+                val_record_with_xadv(7)
+                    .with_x_advance_device(DeviceOrVariationIndex::Device(actual_device_table)),
+                // a duplicate table
+                val_record_with_xadv(8)
+                    .with_x_advance_device(DeviceOrVariationIndex::variation_index(4, 20)),
+            ),
+            Class2Record::new(
+                val_record_with_xadv(9)
+                    .with_x_advance_device(DeviceOrVariationIndex::variation_index(6, 9)),
+                val_record_with_xadv(10),
+            ),
+        ])];
+        let ppf2 = PairPos::format_2(coverage, class_def1, class_def2, class1_records);
+
+        // now we need to pretend we're in the split_pair_pos_format_2 fn
+        let mut graph = TableWriter::make_graph(&ppf2);
+        assert!(graph.pack_objects());
+        let root_id = graph.root;
+        let ppf2_data = &graph.objects[&root_id];
+        let ppf2 = ppf2_data.reparse::<rgpos::PairPosFormat2>().unwrap();
+        assert_eq!(ppf2.class1_records().len(), 1);
+        let c1rec = ppf2.class1_records().get(0).unwrap();
+        let mut visited = HashSet::new();
+        let mut next_device_offset = 3;
+        assert_eq!(c1rec.class2_records.len(), 3);
+
+        // a little helper so we don't have to have this huge fn call in each assert
+        let mut children_size = |record: &rgpos::ValueRecord| -> usize {
+            size_of_value_record_children(
+                record,
+                &graph,
+                &ppf2_data.offsets,
+                &mut next_device_offset,
+                &mut visited,
+            )
+        };
+
+        let c2rec1 = c1rec.class2_records().get(0).unwrap();
+        assert_eq!(children_size(c2rec1.value_record1()), 0, "no subtables");
+        assert_eq!(children_size(c2rec1.value_record2()), 6, "one new varidx");
+        let c2rec2 = c1rec.class2_records().get(1).unwrap();
+        assert_eq!(children_size(c2rec2.value_record1()), 10, "a device table");
+        assert_eq!(children_size(c2rec2.value_record2()), 0, "duplicate table");
+        let c2rec3 = c1rec.class2_records().get(2).unwrap();
+        assert_eq!(children_size(c2rec3.value_record1()), 6, "new varidx table");
+        assert_eq!(children_size(c2rec3.value_record2()), 0, "a null offset");
+        assert_eq!(next_device_offset, 7, "we visited all offsets");
+    }
 }