[write-fonts] Use short offsets in gvar where appropriate #639
Conversation
write-fonts/src/tables/gvar.rs
Outdated
| .iter() | ||
| .fold(0, |acc, val| acc + val.length); | ||
|
|
||
| if max_offset / 2 < (u16::MAX as u32) { |
There was a problem hiding this comment.
This doesn't consider the padding, does it?
cmyr
force-pushed
the
gvar-short-offsets
branch
from
eb4cfbb to
093d7f6
Compare
|
Thanks. Logic LGTM. |
write-fonts/src/tables/gvar.rs
Outdated
|
|
||
| let bytes = sink.into_data().bytes; | ||
| let expected_len = 6 * u16::RAW_BYTE_LEN // offsets: n_glyphs + 1 | ||
| + 32 * 5; // rounded size of each glyph, * 5 typhs |
There was a problem hiding this comment.
um yes, that is supposed to say 'glyphs', i imagine..
This was something I skipped in my initial implementation, but should save us a few kb in the common case.
cmyr
force-pushed
the
gvar-short-offsets
branch
from
093d7f6 to
916fbcf
Compare
write-fonts/src/tables/gvar.rs
Outdated
| @@ -934,4 +969,120 @@ mod tests { | |||
| Some(PackedPointNumbers::Some(vec![1, 3])) | |||
| ); | |||
| } | |||
|
|
|||
| fn make_39_bytes_of_variation_data() -> Vec<GlyphDeltas> { | |||
There was a problem hiding this comment.
Why does it matter that it's 39 bytes?
There was a problem hiding this comment.
guess because it's % 2 != 0 so must be 2-byte aligned
write-fonts/src/tables/gvar.rs
Outdated
| tupl_map.insert(&variations.variations[1].peak_tuple, 2); | ||
|
|
||
| let built = variations.clone().build(&tupl_map); | ||
| // we need an odd number for this to make sense |
write-fonts/src/tables/gvar.rs
Outdated
|
|
||
| #[test] | ||
| fn use_long_offsets_when_necessary() { | ||
| // 2**16 * 2 / (31 + 1 padding) (bytes per tuple) = 4096 so this just overflows |
There was a problem hiding this comment.
I suggest you test the most possible short offsets as well, for example loop over N_GLYPHS 4099, 4100 and confirm that their respective flags are SHORT_OFFSETS, LONG_OFFSETS with the expected sizes
write-fonts/src/tables/gvar.rs
Outdated
| .map(|off| off.unwrap().get()) | ||
| .collect::<Vec<_>>(), | ||
| vec![0u32, 31, 62, 93, 124, 155] | ||
| ) |
There was a problem hiding this comment.
The majority of prefer_short_offsets and use_long_offsets_when_necessary is duplicative noise; if you diff them they differ only in a few key values. Make a helper, e.g. assert_gvar(num glyphs, expected flags, expected offset type, expected first values)
That will make testing the maximum number of short offsets as it's own test trivial too.
write-fonts/src/tables/gvar.rs
Outdated
| @@ -254,11 +262,15 @@ impl GlyphVariations { | |||
| .map(|tup| tup.build(shared_tuple_map, shared_points.as_ref())) | |||
| .unzip(); | |||
|
|
|||
| GlyphVariationData { | |||
| let mut this = GlyphVariationData { | |||
There was a problem hiding this comment.
Nit: the name "this" is perhaps suboptimal to readers with a C++ or Java background.
| /// | ||
| /// we compute this once up front because we need to know it in a bunch | ||
| /// of different places | ||
| length: u32, |
There was a problem hiding this comment.
usize seems the more natural type, I suppose you eventually want to assign it somewhere u32. Perhaps note why it's u32 here?
| } | ||
| // then write the actual data | ||
| for glyph in self.data { | ||
| if !glyph.is_empty() { | ||
| glyph.write_into(writer); | ||
| if !self.long_offsets && glyph.length % 2 != 0 { | ||
| writer.pad_to_2byte_aligned(); | ||
| } |
There was a problem hiding this comment.
You dont' need && glyph.length % 2 here as pad_to_2byte_aligned will happily nop if htat's true?
- Reuse test code - more testing of the short/long boundary condition - clarify some comments
- Reuse test code - more testing of the short/long boundary condition - clarify some comments
cmyr
force-pushed
the
gvar-short-offsets
branch
from
716f724 to
306c78d
Compare
This was something I skipped in my initial implementation, but should save us a few kb in the common case.
closes #615