From b7a2d2849fb458aa5bb80d100cb3d7d644e7403c Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Tue, 12 Sep 2023 17:14:18 -0400 Subject: [PATCH] [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 | 129 ++++++++++++++++++++++------- 1 file changed, 101 insertions(+), 28 deletions(-) diff --git a/write-fonts/src/graph/splitting.rs b/write-fonts/src/graph/splitting.rs index b1ea6970c..b623264f0 100644 --- a/write-fonts/src/graph/splitting.rs +++ b/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 // fn split_pair_pos_format_2(graph: &mut Graph, subtable: ObjectId) -> Option> { + // 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 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::().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"); + } }