Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[read-fonts] Add scaler for CFF/CFF2 #520

Merged
merged 10 commits into from
Aug 8, 2023
Merged

[read-fonts] Add scaler for CFF/CFF2 #520

merged 10 commits into from
Aug 8, 2023

Conversation

dfrg
Copy link
Member

@dfrg dfrg commented Jun 29, 2023

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 and we'll magically support .otf fonts :)

edit: diff for skrifa integration was small so I went ahead and pushed it here

/// [`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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@rsheeter rsheeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest filing an issue for this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -122,22 +123,30 @@ impl<'a> ScalerBuilder<'a> {
pub fn build(mut self, font: &impl TableProvider<'a>) -> Scaler<'a> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the scaler builder should let me set what outline tables I prefer in prioritized order, probably defaulting to [glyf, cff2, cff]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
dfrg added a commit that referenced this pull request Jul 24, 2023
Draft of the CFF hinting support. This will need to be broken into smaller PRs for actual review. Depends on #520
@dfrg dfrg mentioned this pull request Jul 24, 2023
@dfrg
Copy link
Member Author

dfrg commented Aug 8, 2023

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.

@dfrg dfrg merged commit ed16038 into main Aug 8, 2023
7 checks passed
@dfrg dfrg deleted the cff-scaler branch August 8, 2023 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants