Use 'shared point numbers' in gvar #628
Conversation
cmyr
force-pushed
the
only-iup-when-better
branch
from
10b227d to
90f47a1
Compare
cmyr
force-pushed
the
shared-point-numbers
branch
from
20d02e8 to
6bdfafb
Compare
This is a basic initial implementation, that needs a bit of fixing up; in particular we should look into whether or not it is worth sharing the 'all' value (the zero byte) which would only be saving a byte per tuple, and would only make sense if it is common for fonts to often have lots of tuples that use all points? but hey, a byte is a byte. Also I have an idea for a fancy but slow algorithm to compute more efficient share points, and that could be fun to look into.
Our first impl had a little wart, where if a given tuple had any points which would be omitted, we would only consider the sparse representation when choosing a set of packed points to share, which was suboptimal. The solution involves fully building and measuring the tuple at construction time, and deciding which repr is best then.
cmyr
force-pushed
the
shared-point-numbers
branch
from
6bdfafb to
754d31d
Compare
| /// is part of the compileTupleVariationStore method: | ||
| /// <https://github.com/fonttools/fonttools/blob/0a3360e52727cdefce2e9b28286b074faf99033c/Lib/fontTools/ttLib/tables/TupleVariation.py#L641> | ||
| /// | ||
| /// # Note |
There was a problem hiding this comment.
Suggest moving the Note to an issue; buried in comments it's basically lost
write-fonts/src/tables/gvar.rs
Outdated
| let (pts, (_, count)) = point_number_counts | ||
| .into_iter() | ||
| .filter(|(_, (_, count))| *count > 1) | ||
| .max_by_key(|(_, (size, count))| *count * *size)?; |
There was a problem hiding this comment.
Multiply by (*count - 1) here?
There was a problem hiding this comment.
ooooh yea okay I didn't understand the rationale for that at first, I see it now.
|
LGTM except for the one comment. |
write-fonts/src/tables/gvar.rs
Outdated
| .max_by_key(|(_, (size, count))| *count * *size)?; | ||
|
|
||
| // no use sharing points if they only occur once | ||
| (count > 1).then(|| pts.to_owned()) |
There was a problem hiding this comment.
Maybe I'm misreading the sea of brackets and underscroes but it seems like you filter for count > 1 and return None otherwise so this has to be true, the comment should move to the filter and this should just say pts.to_owned()?
| // we finish instantiating self. | ||
| // | ||
| // NOTE: we do a lot of duplicate work here with creating & throwing away | ||
| // buffers, and that can be improved at the cost of a bit more complexity |
There was a problem hiding this comment.
Issue please, comments get lost.
| built.per_tuple_data[1].private_point_numbers, | ||
| Some(PackedPointNumbers::Some(vec![1, 3])) | ||
| ); | ||
| } |
There was a problem hiding this comment.
This looks suspiciously like many tests shoved into one because they share test data. Make a helper to provide the data and split up the tests?
There was a problem hiding this comment.
yes fair. this test is noisy because I was using it to debug something. The only thing we really care about here is the very last assertions, that we use all the points for the first tuple but sparse points for the second; all the intermediate stuff is just leftovers from my debugging, can clean that up.
- fixup how we determine the size savings of shared points - add a test for *not* sharing points when all are unique - cleanup our test of the Lcaron glyph in Oswald - add issue links to notes
in gvar you can optionally store on set of point numbers that are referenced by multiple regions for the same glyph; we were not previously using this very efficiently.