Variable fonts support #1672
Merged
Variable fonts support #1672
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ctrlcctrlv
reviewed
Dec 26, 2022
ctrlcctrlv
reviewed
Dec 26, 2022
ctrlcctrlv
reviewed
Dec 26, 2022
khaledhosny
requested review from
a team,
alerque and
simoncozens
as code owners
|
The test failures seem to be cause by outdated test expectations. How are these updated? |
khaledhosny
commented
Dec 26, 2022
|
I think I’m done here, this should be ready for review/testing for real now. |
|
Got it. Do you expect color fonts will work? If so I'll give the FRBAC specimen build a spin and add the Cantarell TTF as the font used to show the labels. |
|
Color fonts should work. |
|
I'm sorry to say that opsz variations don't seem to be working for me. Could it be a table check gone awry? This isn't a traditional variable font in that I only use FeatureVariations in it. |
|
Interesting, some of my fonts are causing crashes, but not all. |
|
Determined it's only the color ones that cause crashes. |
|
@khaledhosny I added a quick hack to my build script so I could see what happens when we don't crash. for f in `find /tmp/silespecimens -iname '*.pdf'`; do
PDFHEADERONLY='00000000: 2550 4446 2d31 2e35 0a25 e4f0 edf8 0a %PDF-1.5.%.....'
if [ "$(head -c 30 "$f" | xxd)" = "$PDFHEADERONLY" ]; then
rm "$f"
fi
doneIt's possible I'm doing it wrong, but it's not working for me at all for some reason.
I expected that Cantarell to be at its heaviest weight. Here's the .sil |
It works for me. Can you pull the last commit and run with |
Please send me and fonts and documents that are failing. |
|
Cantarell.zip is in #1182. Here's an example of a successful invocation. And here's a failed one: And here's the font that failed w/the document above: |
|
(Note that as mentioned I'm not seeing Cantarell at the weight I expected even when the invocation of
|
I was expecting to see the HarfBuzz version here, and some output about instanciation, but I see neither. You need harfbuzz-subset >= 6.0.0 for instanciation to happen.
I can reproduce this. Investigating, though since you don’t seem to have any instanciation going on, I’m suspecting it is an unrelated issue. |
I can’t run using this font with SILE v0.14.5 either. Looks like a bug in the color-fonts package (I would have replaced this code with the better maintained HarfBuzz API (which now supports COLRv1, too), but it when I suggested using HarfBuzz API to read MATH table others didn’t like it). |
|
If you add |
|
The infinite loop is caused by Lines 130 to 134 in 771d87f Also, conceptually, color support should be handled by the outputters not the shapers (I know, I wrote the initial |
|
An issue with looping after using color fonts sounds like the modified shaper object not getting relpaced with the original when done. I'll see if I can replicate that, but I ran into something like that recently with the fallback font stack so it sounds familiar. |
Creating HarfBuzz fonts was all over the place, done in a slightly different way each time and with potential memory leaks (e.g. often destroying the font, but not the face or the blob). It was also reading the whole font data in Lua, then passing it to HarfBuzz, which is inefficient—especially for large fonts. The code is now using hb_blob_create_from_file() instead which will mmap the file whenever it can. The created hb_font_t is now cached in the Lua font and retrieved from there when needed.
Each call hb_font_set_variations() resets all font variations, so we need to combine all variation settings and apply them at once.
FontConfig will use the upper bits of the face index for named instance index, and hb_font_create() handles this, but we were stripping these bits prematurely. Strip them before sending the index to libtextpdf instead. With named instances supported, saying \font[name=Foo, weight=700] of a variable font will set the correct variation coordinates for the named instance, instead of always using the default instance.
Extend the hack used for finding face index of TTC files to find named instance index of variable fonts. Switch the code to use HarfBuzz all the way to reduce duplication.
With the last commits, FreeType is no longer directly used. It is still needed by FontConfig making it a transitive dependency. This also paves the way to allow building on macOS without FontConfig as well, and maybe provide a DirectWrite-based Windows font manager and build without FontConfig there, too.
* Build HarfBuzz 6.0.0 and harfbuzz-subset library.
* Don’t build FreeType with HarfBuzz support.
FreeType uses HarfBuzz in the autohinter, but the autohinter is not
used since FreeType is only used by FontConfig to load the fonts and
it is not used for any rasterization.
* Don’t build HarfBuzz with ICU support, SILE does not use it.
* Build FreeType from upstream repository.
* Update to the latest release tag and use upstream repository.
This makes variable font support the default, but comes at the cost of requiring the supper-new HarfBuzz 6. Folks can explicitly disable this for older HB support but they should be aware they are limiting features, hence the --disable-font-variations flag.
Reported-by: Khaled Hosny <[email protected]>
If we can’t instanciate the glyph outlines, there is no point in applying variations during layout.
…anges the default
- Force `fontconfig` fontmanger as the test depends on `FONTCONFIG_FILE`. - Correctly call `getFace()` to actually load the Libertinus Serif font not the default one. - Check the name string not its length which might accidentally pass when loading a different font.
alerque
approved these changes
Jan 25, 2023
There was a problem hiding this comment.
Sorry for the delay getting this merged and released. The Nix stuff I was waiting for happened in a couple days but my kids have been sick. For my youngest daughter with TSC that means more seizures and I haven't gotten much work in the past few weeks.
I think we're good to go here. It is easy to only run supported tests but I couldn't come up with a mechanism for only running tests not supported by other runs that I liked, so I just hard coded the HB6 specific tests for now. Eventually we'll probably run the whole test suite there anyway, but for now I just wanted to get that one running.
|
Thanks @alerque for getting this across the finish line. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1182
This uses HarfBuzz subsetter to instanciate the font, saves it to a temporary file and passes it to libtextpdf for embedding into PDF file.
Fonts with CFF2 table are not yet supported and will result in an error.