Variable fonts #792

rcombs · 2024-06-20T20:32:57Z

Supersedes #789 and #790, and also adds variable font support, along with some other refactoring of font handling.

TODO:

Confirm GDI behavior on some edge cases around the STAT table (there are a couple cases where we believe the current behavior here is incorrect); cc @moi15moi
Consider cacheing a flag indicating that a given font has already been inserted; just requires a good key:
- On CoreText and Fontconfig, path+index+psname works, but on DirectWrite this gets more complicated; LOGFONT name works sometimes, but not in the IDWriteFontSet case… maybe add a cache like the fontconfig one?
Consider inserting all GDI-compatible font names into a cache at load-time (similar to the fontconfig one) and searching that in find_font instead of iterating the full global list (probably should be a separate PR after this one)
- Would require a downcase function but that should be fine

moi15moi

Here is the first check.

Also, if I request Bahnschrift Semibold, libass will actually select Bahnschrift Regular. I haven't checked exactly why it happen, but it may be because the provider extended_family is Bahnschrift Semibold and will be used for all the face of Bahnschrift which shouldn't be the case.

I used DirectWrite.

libass/ass_fontselect.c

moi15moi · 2024-06-24T23:07:45Z

libass/ass_fontselect.c

+
+        for (j = 0; j < variants->num_axis; j++) {
+            if (a->tag == variants->axis[j].tag) {
+                a->fvar_idx = j;


This is incorrect.
For example, Alegreya - Original.zip
has 2 DesignAxisRecord (wght and ital) in the STAT table, but the fvar table only has 1 axis (wght).

The sample font alone doesn't tell me much without knowing how it behaves in GDI. What's this supposed to do?

moi15moi · 2024-06-24T23:29:07Z

libass/ass_fontselect.c

+ * \return success
+ */
+static void
+compute_variant_data(struct VariantData *data, const FT_Var_Named_Style *variant,


You don't use the GDI score algorithm.
Here is the algorithm: https://github.com/moi15moi/FontCollector/blob/7d82835aab9de817a4953f50bf69c9e688acb7ce/font_collector/font/font_parser.py#L96-L141

I'm hoping we can document behavior in specific edge cases here (in particular, the documented behavior for format-4 tables differs from that code, and I'd like to confirm exactly what happens in those cases).

libass/ass_fontselect.c

We run a search once with just the original name and no extended matches, then run substitutions, then run again with extended matches allowed.

This no longer adds any marginal cost.

This keeps an internal cache of all known names for all fonts, and loads them in when requested. This means that font metadata can be loaded on-demand using freetype, as with all other providers.

This removes dead provider-driven name handling code, unifies paths for system and memory fonts, and paves the way for variable font support. Notable behavior change: all GDI-compatible names we find are now stored in the families array; no distinction is made between family, full, and postscript names. This will allow for some simplification of matching code, and is necessary for variable font support.

- Inserts a copy of a font with different metadata for each named instance - When searching for a font, tries to match on sub-names by iteratively truncating at spaces

MrSmile

I'm not an expert on font selection, so mostly comments about cosmetics.

MrSmile · 2024-06-26T19:43:51Z

libass/ass_font.c

@@ -458,8 +458,10 @@ static int add_face(ASS_FontSelector *fontsel, ASS_Font *font, uint32_t ch)
    ass_charmap_magic(font->library, face);
    set_font_metrics(face);

-    font->faces[font->n_faces] = face;
-    font->faces_uid[font->n_faces] = uid;
+    font->faces[font->n_faces] = (ASS_Face){


Style nit: I think it should be space here (ASS_Face) {.

MrSmile · 2024-06-26T19:54:11Z

libass/ass_cache.c

@@ -325,6 +325,54 @@ const CacheDesc glyph_metrics_cache_desc = {
 };


+#ifdef CONFIG_FONTCONFIG


Maybe we should think about separating individual cache implementations from generic cache code? Number of caches grows and now ass_cache.c consists of a ton of unrelated code.

MrSmile · 2024-06-26T19:55:04Z

libass/ass_cache.c

+{
+    FontconfigNameHashKey *k = key;
+    FontconfigNameHashValue *v = value;
+    free((char*)k->name.str);


Style nit: spaces (char *) k.

MrSmile · 2024-06-26T20:02:17Z

libass/ass_cache_template.h

+#ifdef CONFIG_FONTCONFIG
+// describes list of known fonts corresponding to a given fontconfig name
+START(fontconfig_name, fontconfig_name_hash_key)
+    STRING(name)


I think it would be more logically correct to use ASS_StringView as cache key directly, but in that case automatic function generation wouldn't work. Maybe we should make string functions compatible with cache interface?

MrSmile · 2024-06-26T20:04:35Z

libass/ass_fontconfig.c

+
+    if (value->size >= value->capacity) {
+        size_t new_cap = FFMAX(value->capacity, 1) * 2;
+        void *resized = ass_realloc_array(value->patterns, new_cap, sizeof(FcPattern*));


Wouldn't it be better to use ASS_REALLOC_ARRAY macro?
Also style: sizeof(FcPattern *).

MrSmile · 2024-06-26T20:52:23Z

libass/ass_fontselect.c

+    if (!stat->value_groups)
+        return false;
+
+    int i, j;


Better to use loop-local counters.

MrSmile · 2024-06-26T20:54:07Z

libass/ass_fontselect.c

+compute_variant_data(struct VariantData *data, const FT_Var_Named_Style *variant,
+                     const struct STAT *stat)
+{
+    int i, j, k;


Better to use loop-local counters.

MrSmile · 2024-06-26T20:55:04Z

libass/ass_fontselect.c

+                struct AppendedName *append = &data->appended_names[data->nb_appended_names++];
+                append->include_in_family = !elide_from_family;
+                append->id = g->name_id;
+                append->index = (FT_UInt)-1;


Style nit: space (FT_UInt) -1.

MrSmile · 2024-06-26T20:55:24Z

libass/ass_fontselect.c

+        struct AppendedName *append = &data->appended_names[data->nb_appended_names++];
+        append->include_in_family = false;
+        append->id = stat->elidedFallbackNameID;
+        append->index = (FT_UInt)-1;


Style nit: space (FT_UInt) -1.

MrSmile · 2024-06-26T20:57:53Z

libass/ass_fontselect.c

+            if (family_dup[len] == ' ')
+                family_dup[len] = 0;
+
+            if (!family_dup[len]) {


Excess {}.

rcombs requested review from astiob, TheOneric and MrSmile June 20, 2024 20:32

rcombs force-pushed the variable-fonts branch 4 times, most recently from 9fb6470 to 45288ac Compare June 20, 2024 22:06

This was referenced Jun 21, 2024

ass_fontselect: lazy-load GDI-compatible metadata for matching #789

Closed

ass_fontconfig: ignore named instances of variable fonts #790

Closed

moi15moi suggested changes Jun 24, 2024

View reviewed changes

rcombs added 16 commits June 24, 2024 22:31

ass_fontselect: fix whitespace error

caa7f3e

ass_fontselect: constify name param to match_fonts

219ec00

ass_utils: return out size in ass_utf16be_to_utf8

b499ea3

ass_font: merge per-face lists

54ef1fe

ass_fontselect: allow extended families in substitution matches

ce403c7

We run a search once with just the original name and no extended matches, then run substitutions, then run again with extended matches allowed.

ass_directwrite: eagerly load font index

db6e27d

This no longer adds any marginal cost.

ass_directwrite: remove obsolete check_postscript function

e828e0e

ass_fontconfig: use match_fonts

64c3689

This keeps an internal cache of all known names for all fonts, and loads them in when requested. This means that font metadata can be loaded on-demand using freetype, as with all other providers.

ass_fontconfig: remove obsolete check_postscript function

8c13ac6

ass_fontselect: remove obsolete fullname/postscript code

54f2f71

ass_fontselect: remove use of obsolete get_font_index function

9e94976

ass_font: get weight/flags from select meta

7f64cef

ass_fontselect: remove obsolete check_postscript/get_font_index pointers

d6a29b8

ass_fontselect: add support for variable fonts

1322a73

- Inserts a copy of a font with different metadata for each named instance - When searching for a font, tries to match on sub-names by iteratively truncating at spaces

ass_fontselect: remove obsolete fields

decb1ac

rcombs force-pushed the variable-fonts branch from 45288ac to decb1ac Compare June 25, 2024 07:55

MrSmile reviewed Jun 26, 2024

View reviewed changes

TheOneric added compatibility fonts labels Jun 30, 2024

astiob mentioned this pull request Feb 25, 2025

Request full font details in MatchFontsFunc #868

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Variable fonts #792

Variable fonts #792

		@@ -325,6 +325,54 @@ const CacheDesc glyph_metrics_cache_desc = {
		};


		#ifdef CONFIG_FONTCONFIG

Variable fonts #792

Are you sure you want to change the base?

Variable fonts #792

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment