Table splitting for MarkBase #649
Conversation
in the equivalent fonttools PR fonttools/fonttools#1297 from a few years back, some Noto fonts are mentioned. In any case, fontc probably will have to emit some MarkToBase lookups before we can test this with real fonts |
This is a checkpoint; the code has not been tested.
Found & fixed one show-stopper in our base array splitting.
This adds a big smoke test that we are passing, and it does include checks that values are correct, so that's something?
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
There was a problem hiding this comment.
Apologies for letting this one sit so long. There's some realllly annoying inherent complexity here so I both compared to HB and traced the logic according to the spec and... this seems solid, nice work! A couple comments/questions inline but I didn't see anything blocking.
| log::debug!( | ||
| "nothing to split, size '{}'", | ||
| accumulated + partial_coverage_size | ||
| ); |
There was a problem hiding this comment.
Should this be inside the conditional below?
There was a problem hiding this comment.
it's supposed to be a companion to the trace! above, which prints each split; this prints the size of the final subtable (it's sort of a fenceposts-vs-fences thing)
| // because offsets may be null, and there is no pattern, we visit each one | ||
| for base_record in base_array.base_records().iter() { | ||
| let base_record = base_record.unwrap(); | ||
| for (mark_class, offset) in base_record.base_anchor_offsets().iter().enumerate() { | ||
| let has_offset = !offset.get().is_null(); | ||
| let in_range = (start..end).contains(&mark_class); |
There was a problem hiding this comment.
I think this comment should be on the inner for loop.
I'm assuming this means that we iterate all and check the range rather than slicing base_anchor_offsets() on start..end because we need to track next_offset_idx? Could we break out of the loop when mark_class >= end? (not suggesting we do so, just checking my understanding of what's going on)
There was a problem hiding this comment.
noted, reading this back I agree that there could be more explanation here. Basically: what is tricky here is that the anchor tables may or may not have subtables, and there is no pattern around how subtables are distributed across the set of anchor tables, so we need to visit each offset and see if it's null or not to figure out whether we are copying over a subtable.
The reason we can't break early is because even if we're out of range, if we saw an offset we need to increment next_offset_idx so that once we're back in range we are copying the correct subtable. 🤕
There was a problem hiding this comment.
Ohh, yep, I see it now. The comment seems like it's in the proper place then... I was just missing that the offset list is associated with the base array and not the base record (obvious in hindsigh). Thanks!
| objects | ||
| .iter() | ||
| .map(|id| { | ||
| if !visited.insert(*id) { | ||
| return 0; | ||
| } | ||
| // the size of the anchor table | ||
| let base_size = graph.objects[id].bytes.len(); | ||
| // the size of any devices or variation indices. | ||
| let children_size = graph.objects[id] | ||
| .offsets | ||
| .iter() | ||
| .map(|id| { | ||
| // the mark2pos subgraph is only ever two layers deep | ||
| debug_assert!(graph.objects[&id.object].offsets.is_empty()); | ||
| visited | ||
| .insert(id.object) | ||
| .then(|| graph.objects[&id.object].bytes.len()) | ||
| .unwrap_or(0) | ||
| }) | ||
| .sum::<usize>(); | ||
| base_size + children_size | ||
| }) | ||
| .sum() |
There was a problem hiding this comment.
No issues.. just wanted to say that I <3 this code.
This is an initial port of the table splitting impl for this subtable.
Unlike with PairPos, we don't have any real-world failure cases to test this on yet, but I think it's ready as a checkpoint.
This is based on #647, which should go in first. We also need to resolve clippy & rustdoc complains (#648)