From 9d31d42ab06a591275e69a70292487363d4e5c08 Mon Sep 17 00:00:00 2001 From: Colin Rofls Date: Fri, 6 Oct 2023 12:09:27 -0400 Subject: [PATCH] [write-fonts] Full pack/split test for MarkToBase This caught one typo in our code that decides what tables we should attempt to split, but otherwise it seems like this worked first try, which is slightly scary --- write-fonts/src/graph/splitting/mark2base.rs | 131 +++++++++++++++++-- write-fonts/src/table_type.rs | 2 +- 2 files changed, 123 insertions(+), 10 deletions(-) diff --git a/write-fonts/src/graph/splitting/mark2base.rs b/write-fonts/src/graph/splitting/mark2base.rs index 1b35d2b21..c5a20dc47 100644 --- a/write-fonts/src/graph/splitting/mark2base.rs +++ b/write-fonts/src/graph/splitting/mark2base.rs @@ -293,7 +293,7 @@ mod tests { use crate::{ tables::{ gpos::{AnchorTable, BaseArray, BaseRecord, MarkArray, MarkBasePosFormat1, MarkRecord}, - layout::Lookup, + layout::{DeviceOrVariationIndex, Lookup, LookupList}, }, TableWriter, }; @@ -315,19 +315,37 @@ mod tests { } } - fn make_mark_array(class_count: u16, glyphs_per_class: u16) -> MarkArray { + fn make_mark_array( + class_count: u16, + glyphs_per_class: u16, + include_var_index_tables: bool, + ) -> MarkArray { let mark_glyph_count = class_count * glyphs_per_class; let records = (0..mark_glyph_count) .map(|cov_idx| { let mark_class = cov_idx / glyphs_per_class; let anchor_val = u16_to_i16(cov_idx); - MarkRecord::new(mark_class, AnchorTable::format_1(anchor_val, anchor_val)) + let anchor = if include_var_index_tables { + AnchorTable::format_3( + anchor_val, + anchor_val, + Some(DeviceOrVariationIndex::variation_index(cov_idx, cov_idx)), + None, + ) + } else { + AnchorTable::format_1(anchor_val, anchor_val) + }; + MarkRecord::new(mark_class, anchor) }) .collect(); MarkArray::new(records) } - fn make_base_array(base_count: u16, mark_class_count: u16) -> BaseArray { + fn make_base_array( + base_count: u16, + mark_class_count: u16, + include_var_index_tables: bool, + ) -> BaseArray { let base_records = (0..base_count) .map(|i| { let mark_anchors = (0..mark_class_count) @@ -335,7 +353,18 @@ mod tests { // even anchors exist, odd anchors are null (j % 2 == 0).then(|| { let val = u16_to_i16(i * mark_class_count + j); - AnchorTable::format_1(val, val) + if include_var_index_tables { + AnchorTable::format_3( + val, + val, + // we want duplicate variation index tables, since it + // poses different challanges when resolving offsets + Some(DeviceOrVariationIndex::variation_index(j, j)), + None, + ) + } else { + AnchorTable::format_1(val, val) + } }) }) .collect(); @@ -362,8 +391,8 @@ mod tests { let base_coverage = (FIRST_BASE_GLYPH..FIRST_BASE_GLYPH + N_BASES) .map(GlyphId::new) .collect(); - let mark_array = make_mark_array(MARK_CLASS_COUNT, MARKS_PER_CLASS); - let base_array = make_base_array(N_BASES, MARK_CLASS_COUNT); + let mark_array = make_mark_array(MARK_CLASS_COUNT, MARKS_PER_CLASS, false); + let base_array = make_base_array(N_BASES, MARK_CLASS_COUNT, false); let table = MarkBasePosFormat1::new(mark_coverage, base_coverage, mark_array, base_array); let lookup = Lookup::new(LookupFlag::empty(), vec![table], 0); @@ -475,6 +504,90 @@ mod tests { } } + #[test] + fn full_split_including_variation_index_tables() { + let _ = env_logger::builder().is_test(true).try_init(); + + const MARK_CLASS_COUNT: u16 = 120; + const MARKS_PER_CLASS: u16 = 4; + const N_BASES: u16 = 500; + const N_MARKS: u16 = MARK_CLASS_COUNT * MARKS_PER_CLASS; + const FIRST_BASE_GLYPH: u16 = 2; + const FIRST_MARK_GLYPH: u16 = 2000; + + let mark_coverage = (FIRST_MARK_GLYPH..FIRST_MARK_GLYPH + N_MARKS) + .map(GlyphId::new) + .collect(); + let base_coverage = (FIRST_BASE_GLYPH..FIRST_BASE_GLYPH + N_BASES) + .map(GlyphId::new) + .collect(); + let mark_array = make_mark_array(MARK_CLASS_COUNT, MARKS_PER_CLASS, true); + let base_array = make_base_array(N_BASES, MARK_CLASS_COUNT, true); + + let table = MarkBasePosFormat1::new(mark_coverage, base_coverage, mark_array, base_array); + let lookup = Lookup::new(LookupFlag::empty(), vec![table], 0); + let lookup_list = LookupList::new(vec![lookup]); + let bytes = crate::dump_table(&lookup_list).unwrap(); + let read_back = rgpos::PositionLookupList::read(bytes.as_slice().into()).unwrap(); + assert_eq!(read_back.lookup_count(), 1); + let lookup = read_back.lookups().get(0).unwrap(); + + let subtables: Vec<_> = match lookup { + rgpos::PositionLookup::MarkToBase(lookup) => { + lookup.subtables().iter().map(|sub| sub.unwrap()).collect() + } + rgpos::PositionLookup::Extension(lookup) => lookup + .subtables() + .iter() + .map(|sub| { + let Ok(rgpos::ExtensionSubtable::MarkToBase(ext_sub)) = sub else { + panic!("wrong extension type"); + }; + ext_sub.extension().unwrap() + }) + .collect(), + _ => panic!("bad lookup type"), + }; + assert_eq!(subtables.len(), 7); + + // now we want to check to see that one of our variation index tables is correct. + // in the markarray, each mark has a varidx table, with both values equal + // to that mark's original coverage index. + + let mark_cov_idx_to_test = 72; + let orig_anchor = &lookup_list.lookups[0].subtables[0].mark_array.mark_records + [mark_cov_idx_to_test] + .mark_anchor; + // sanity check that our logic makes sense + match orig_anchor.as_ref() { + AnchorTable::Format3(anchor) => match anchor.x_device.as_ref() { + Some(DeviceOrVariationIndex::VariationIndex(varidx)) + if varidx.delta_set_outer_index == mark_cov_idx_to_test as u16 => + { + () + } + _ => panic!("bad assumptions"), + }, + _ => panic!("very bad assumptions"), + }; + + let gid = GlyphId::new(FIRST_MARK_GLYPH + mark_cov_idx_to_test as u16); + let subtable = subtables + .iter() + .find(|sub| sub.mark_coverage().unwrap().get(gid).is_some()) + .expect("should exist in some subtable"); + let new_cov_idx = subtable.mark_coverage().unwrap().get(gid).unwrap(); + let mark_array = subtable.mark_array().unwrap(); + let mark_record = &mark_array.mark_records()[new_cov_idx as usize]; + let mark_anchor = mark_record.mark_anchor(mark_array.offset_data()).unwrap(); + let rgpos::AnchorTable::Format3(mark_anchor) = mark_anchor else { + panic!("wrong format") + }; + assert!( + matches!(mark_anchor.x_device().transpose().unwrap(), Some(rgpos::DeviceOrVariationIndex::VariationIndex(varidx)) if varidx.delta_set_outer_index() == mark_cov_idx_to_test as u16) + ); + } + #[test] fn test_my_test_helper() { assert_eq!(u16_to_i16(1), 1); @@ -490,7 +603,7 @@ mod tests { const GLYPHS_PER_CLASS: u16 = N_GLYPHS / N_CLASSES; const SPLIT_CLASS_RANGE: Range = 20..25; - let mark_array = make_mark_array(N_CLASSES, GLYPHS_PER_CLASS); + let mark_array = make_mark_array(N_CLASSES, GLYPHS_PER_CLASS, false); let graph = TableWriter::make_graph(&mark_array); let data = &graph.objects[&graph.root]; @@ -537,7 +650,7 @@ mod tests { const N_BASES: u16 = 10; const SPLIT_CLASS_RANGE: Range = 15..20; - let base_array = make_base_array(N_BASES, N_CLASSES); + let base_array = make_base_array(N_BASES, N_CLASSES, false); let graph = TableWriter::make_graph(&base_array); let data = &graph.objects[&graph.root]; diff --git a/write-fonts/src/table_type.rs b/write-fonts/src/table_type.rs index 79a96f905..7fe9d8073 100644 --- a/write-fonts/src/table_type.rs +++ b/write-fonts/src/table_type.rs @@ -48,7 +48,7 @@ impl TableType { matches!( self, TableType::GposLookup(LookupType::PAIR_POS) - | TableType::GsubLookup(LookupType::MARK_TO_BASE) + | TableType::GposLookup(LookupType::MARK_TO_BASE) ) }