[read-fonts] Add scaler for CFF/CFF2 #520
Conversation
This is a 60% gain on scaling all glyphs in Source Sans Pro
| /// [`subfont`](Self::subfont) to create an instance of the subfont for a | ||
| /// particular size and location in variation space. | ||
| /// Creating subfont instances is not free, so this process is exposed in | ||
| /// discrete steps to allow for caching. |
There was a problem hiding this comment.
This seems like it's exposing exactly the sort of detail Context claims to abstract away from me? - not necessarily a problem but perhaps we should explain why.
| font_test_data::NOTO_SERIF_DISPLAY_TRIMMED, | ||
| font_test_data::NOTO_SERIF_DISPLAY_TRIMMED_GLYPHS, | ||
| ); | ||
| } |
There was a problem hiding this comment.
I cannot readily tell where the expected values come from when I read the unit test. I assume the answer is freetype, perhaps through naming or comment we could clarify. For example, I can follow this somewhat more easily:
#[test]
fn cff_static_outline_matches_freetype() {
compare_glyphs(
font_test_data::NOTO_SERIF_DISPLAY_TRIMMED,
font_test_data::NOTO_SERIF_DISPLAY_TRIMMED_GLYPHS,
);
}
#[test]
fn cff2_static_outline_matches_freetype() { ... }
// etc...that's making me realize, it looks like we test nothing more than some outline for each outline format. Shouldn't we build up test assets that use all the drawing commands? E.g. test that a cff with an hstem matches the freetype outline with an hstem, perhaps making a test font with many test glyphs such that each glyph uses "basic" (line, curve, etc) commands plus one unusual (hstem, vstem, etc) command . We could them compare those glyphs for (cff, cff2) x (static, variable).
There was a problem hiding this comment.
We actually test a number of outlines at multiple sizes and (for VFs) locations in variation space. That was the whole point of adding the extract glyphs script.
The charstring module has a handcrafted test that contains all operators except hinting. We can’t test those until hinting support lands.
There was a problem hiding this comment.
Renamed these as suggested. We can increase coverage by adding more glyphs or test fonts to font-test-data. Or additionally, but adjusting the outline instances generated by the extract glyphs script.
There was a problem hiding this comment.
Code lgtm but I did have a fair number of questions and I'm skeptical the unit tests are adequate (details in comment). I realize the "real" test is a large corpus of glyphs but I think we can get more mileage out of unit testing by constructing a targeted corpus.
Some of the questions might imply additional work that doesn't make sense to do in this PR (assuming it makes sense at all); feel free to file issues for followon PRs if you prefer not to address here. I'm fond of this approach when the core of the thing seems good and I need to make followon changes, (e.g. googlefonts/fontc#362.
| struct Outlines<'a> { | ||
| glyf: Option<(glyf::Scaler<'a>, &'a mut glyf::Outline)>, | ||
| // Clippy doesn't like the size discrepancy between the two variants. Ignore | ||
| // for now: we'll replace this with a real cache. |
There was a problem hiding this comment.
suggest filing an issue for this
| @@ -122,22 +123,30 @@ impl<'a> ScalerBuilder<'a> { | |||
| pub fn build(mut self, font: &impl TableProvider<'a>) -> Scaler<'a> { | |||
There was a problem hiding this comment.
I wonder if the scaler builder should let me set what outline tables I prefer in prioritized order, probably defaulting to [glyf, cff2, cff]?
There was a problem hiding this comment.
I'm going to agree with earlier Rod in saying that skrifa users probably shouldn't care about this level of detail. We should also likely be using sfntVersion to determine priority instead (that is, prefer CFF2->CFF->glyf when the version is OTTO and glyf->CFF2->CFF otherwise). We don't yet have enough information to do that here. See #485
- speculate on why we do the scaling factor fance - also why we filter some path elements - rename SimplifyingSink -> NopFilteringSink - notes and citations for dealing with FDSelect - rephrase confusing language for private dict range - links to spec for Version enum variants - suggest using skrifa as a higher level interface - better names for tests
Draft of the CFF hinting support. This will need to be broken into smaller PRs for actual review. Depends on #520
|
Per other discussions the scaler code has been moved from read-fonts to a private module in skrifa. We can likely move some of this code back to read-fonts to accommodate usage as requested in #541 but this PR has already been delayed long enough and I wanted to keep churn low. |
This is a unified scaler for charstrings in CFF/CFF2 tables. The API attempts to be similar in style to the new TrueType scaler. Infrastructure for hinting is here, but completing it will require a few more PRs. #519 added extracted glyph outlines for static and variable fonts. Those are tested here.
After this lands
hooking it up to skrifa will be trivial andwe'll magically support .otf fonts :)edit: diff for skrifa integration was small so I went ahead and pushed it here