Transform CFF glyphs by their matrix

[tbodt]

Some fonts use the shear part of this matrix to do an oblique version.

See discussion at typst/pixglyph#3

[RazrFalcon]

Can you run benchmarks? How much does it affect performance? It would be nice if we could skip this step for a default transform.

[tbodt]

before:

     Running methods_perf.rs (target/release/deps/methods_perf-1edc13985d9c68a9)

running 15 tests
test family_name                 ... bench:         107 ns/iter (+/- 4)
test from_data_otf_cff           ... bench:         690 ns/iter (+/- 36)
test from_data_otf_cff2          ... bench:         746 ns/iter (+/- 55)
test from_data_ttf               ... bench:         280 ns/iter (+/- 30)
test glyph_index_u41             ... bench:          13 ns/iter (+/- 4)
test glyph_name_cff_276          ... bench:          70 ns/iter (+/- 2)
test glyph_name_cff_8            ... bench:           4 ns/iter (+/- 0)
test glyph_name_post_276         ... bench:       1,036 ns/iter (+/- 27)
test glyph_name_post_8           ... bench:           2 ns/iter (+/- 0)
test outline_glyph_276_from_cff  ... bench:         589 ns/iter (+/- 15)
test outline_glyph_276_from_cff2 ... bench:         807 ns/iter (+/- 17)
test outline_glyph_276_from_glyf ... bench:         508 ns/iter (+/- 10)
test outline_glyph_8_from_cff    ... bench:         257 ns/iter (+/- 15)
test outline_glyph_8_from_cff2   ... bench:         495 ns/iter (+/- 19)
test outline_glyph_8_from_glyf   ... bench:         226 ns/iter (+/- 77)

test result: ok. 0 passed; 0 failed; 0 ignored; 15 measured

     Running methods_perf_x1000.rs (target/release/deps/methods_perf_x1000-50bae7bd03e09487)

running 9 tests
test ascender               ... bench:         491 ns/iter (+/- 23)
test glyph_hor_advance      ... bench:         333 ns/iter (+/- 16)
test glyph_hor_side_bearing ... bench:         334 ns/iter (+/- 12)
test strikeout_metrics      ... bench:         335 ns/iter (+/- 14)
test subscript_metrics      ... bench:       2,052 ns/iter (+/- 68)
test underline_metrics      ... bench:         334 ns/iter (+/- 31)
test units_per_em           ... bench:         332 ns/iter (+/- 9)
test width                  ... bench:         332 ns/iter (+/- 11)
test x_height               ... bench:         333 ns/iter (+/- 12)

test result: ok. 0 passed; 0 failed; 0 ignored; 9 measured

after:

     Running methods_perf.rs (target/release/deps/methods_perf-1edc13985d9c68a9)

running 15 tests
test family_name                 ... bench:         106 ns/iter (+/- 7)
test from_data_otf_cff           ... bench:         663 ns/iter (+/- 21)
test from_data_otf_cff2          ... bench:         705 ns/iter (+/- 12)
test from_data_ttf               ... bench:         270 ns/iter (+/- 22)
test glyph_index_u41             ... bench:          12 ns/iter (+/- 3)
test glyph_name_cff_276          ... bench:          68 ns/iter (+/- 2)
test glyph_name_cff_8            ... bench:           4 ns/iter (+/- 0)
test glyph_name_post_276         ... bench:       1,068 ns/iter (+/- 28)
test glyph_name_post_8           ... bench:           2 ns/iter (+/- 0)
test outline_glyph_276_from_cff  ... bench:         634 ns/iter (+/- 51)
test outline_glyph_276_from_cff2 ... bench:         789 ns/iter (+/- 14)
test outline_glyph_276_from_glyf ... bench:         496 ns/iter (+/- 17)
test outline_glyph_8_from_cff    ... bench:         273 ns/iter (+/- 11)
test outline_glyph_8_from_cff2   ... bench:         502 ns/iter (+/- 11)
test outline_glyph_8_from_glyf   ... bench:         223 ns/iter (+/- 4)

test result: ok. 0 passed; 0 failed; 0 ignored; 15 measured

     Running methods_perf_x1000.rs (target/release/deps/methods_perf_x1000-50bae7bd03e09487)

running 9 tests
test ascender               ... bench:         484 ns/iter (+/- 15)
test glyph_hor_advance      ... bench:         328 ns/iter (+/- 8)
test glyph_hor_side_bearing ... bench:         328 ns/iter (+/- 15)
test strikeout_metrics      ... bench:         328 ns/iter (+/- 11)
test subscript_metrics      ... bench:       2,005 ns/iter (+/- 20)
test underline_metrics      ... bench:         328 ns/iter (+/- 11)
test units_per_em           ... bench:         328 ns/iter (+/- 8)
test width                  ... bench:         329 ns/iter (+/- 15)
test x_height               ... bench:         328 ns/iter (+/- 16)

test result: ok. 0 passed; 0 failed; 0 ignored; 9 measured

so, seems to be a slight measurable impact on a couple of outline_glyph benchmarks. I'll throw in an Option and see what happens

[tbodt]

Option gets back about half the performance

     Running methods_perf.rs (target/release/deps/methods_perf-1edc13985d9c68a9)

running 15 tests
test family_name                 ... bench:         104 ns/iter (+/- 3)
test from_data_otf_cff           ... bench:         660 ns/iter (+/- 54)
test from_data_otf_cff2          ... bench:         703 ns/iter (+/- 34)
test from_data_ttf               ... bench:         271 ns/iter (+/- 9)
test glyph_index_u41             ... bench:          12 ns/iter (+/- 6)
test glyph_name_cff_276          ... bench:          67 ns/iter (+/- 1)
test glyph_name_cff_8            ... bench:           4 ns/iter (+/- 0)
test glyph_name_post_276         ... bench:       1,009 ns/iter (+/- 58)
test glyph_name_post_8           ... bench:           2 ns/iter (+/- 1)
test outline_glyph_276_from_cff  ... bench:         610 ns/iter (+/- 8)
test outline_glyph_276_from_cff2 ... bench:         773 ns/iter (+/- 202)
test outline_glyph_276_from_glyf ... bench:         496 ns/iter (+/- 18)
test outline_glyph_8_from_cff    ... bench:         273 ns/iter (+/- 19)
test outline_glyph_8_from_cff2   ... bench:         485 ns/iter (+/- 13)
test outline_glyph_8_from_glyf   ... bench:         222 ns/iter (+/- 16)

test result: ok. 0 passed; 0 failed; 0 ignored; 15 measured

     Running methods_perf_x1000.rs (target/release/deps/methods_perf_x1000-50bae7bd03e09487)

running 9 tests
test ascender               ... bench:         484 ns/iter (+/- 8)
test glyph_hor_advance      ... bench:         328 ns/iter (+/- 14)
test glyph_hor_side_bearing ... bench:         329 ns/iter (+/- 7)
test strikeout_metrics      ... bench:         328 ns/iter (+/- 7)
test subscript_metrics      ... bench:       2,007 ns/iter (+/- 62)
test underline_metrics      ... bench:         328 ns/iter (+/- 8)
test units_per_em           ... bench:         328 ns/iter (+/- 12)
test width                  ... bench:         328 ns/iter (+/- 10)
test x_height               ... bench:         327 ns/iter (+/- 2)

test result: ok. 0 passed; 0 failed; 0 ignored; 9 measured

pushed this along with test fixes

[tbodt]

I'm not happy with how units_per_em is getting passed around, particularly how I needed to type 1000 on each call to cff::Table::parse in the test - and oh no that's public API! but it does seem to be necessary to make some kind of public API change because outline and glyph_width aren't actually possible to implement correctly without knowledge of font upem...

• is it preferable to add it as a parameter to table constructor, or to outline and glyph_width the functions that actually need it?
• make it an option and default to 1000 in the implementation? but making it too easy to just pass None might make it too easy to forget to pass the correct value

[LaurenzV]

This is definitely unfortunate, for one thing because it's annoying to have to pass this manually, but also because this would be a breaking change. I'm honestly not sure what the best way forward is here. :/

[tbodt]

Well, while messy, it's always possible to add new methods for outline and glyph_width and deprecate the old broken ones, without strictly being a breaking change.

But also I'm curious has this library made breaking changes before? is there established process?

[LaurenzV]

But also I'm curious has this library made breaking changes before? is there established process?

Sure, we are at version 0.25, so there have been many breaking changes before. However, the problem is that at this point, this library is more or less in maintenance-only mode as most of the momentum is shifting towards skrifa, and there are some adjacent crates that depend on ttf-parser like fontdb which, if I understood correctly, won't be updated anymore, so we definitely need to be more mindful about making breaking releases. I guess I just want to hear some other voices before making a decision on that.

Sorry that the small bug you wanted to fix ends up being so hard to actually get merged. :(

[tbodt]

If adjacent crates won't be updated anymore and will stay on the current version forever then there's no problem with adding new breaking versions because they will never have to update to those 🤔

[LaurenzV]

new breaking versions because they will never have to update to those

Well, it's desirable to avoid that because then you end up depending on two or more versions of ttf-parser in Typst, which means more binary bloat. If it were non-breaking, then we can just update the ttf-parser version in Typst and all downstream crates that use the same major version will just use that.

[LaurenzV]

As mentioned, I'm not strictly opposed to merging this, I just think it warrants some more opinons from the other maintainers before doing so.

[RazrFalcon]

The patch looks good to me.

As for breaking changes... we could wing it. Direct CFF parsing is technically a semi-private API and 99% of projects do not use it. And since bumping ttf-parser is kinda impossible at this points - that's the only thing we can do.
If we would get a lot of complains we could always yank it and do a proper version bump.

PS: you should fix older Rust support. You use newer Rust features. Specifically if-let.

[tbodt]

Will fix for older rust. Aside, I'd still like thoughts on which API is better - add units_per_em argument to cff table constructor, or to outline + glyph_width?

[RazrFalcon]

To the CFF table constructor. This is how other tables are implemented as well.

[tbodt]

I wasn't sure because the upem is not part of the table, just information useful for interpreting the table, but if that's how the other constructors are then makes sense

[LaurenzV]

Thanks, I will land this soon but I’m not yet convinced about just landing this in a patch release, so we will see how we end up doing it.

[LaurenzV]

Thanks!