Rework fontconfig fallback to use fallback list computed from font_sort #3163

kchibisov · 2020-01-07T22:33:37Z

Attempt to improve fontconfig fallback mechanism to be faster and more predictive for users. The idea is to use a fallback list from font_sort.

Probably should fix:
#3134
#2657
#1560
#2935
#965
Some fallback problems there #153
Concerns there #2706
Improve Linux/BSD parts of #511?

cc @nixpulvis

P.s. I've just published some initial attempt to solve mentioned issues, so feedback and testing is welcome.

TODO:

Check fallbacks for styled glyphs.
Bitmap fonts?
~Hash set self.keys(&path) could be ~~2micro seconds, should be in nano!~~ not true, and using a different thing doesn't change anything...

Out of scope:

Apply fixup factor for metrics
Gray, etc downsampling?

chrisduerr

Just some ramblings that jumped to my mind when having a quick glance at things.

Generally most of this makes sense to me, so we just need to nail down the exact implementation, but the approach seems good.

font/src/ft/fc/mod.rs

font/src/ft/fc/pattern.rs

font/src/ft/mod.rs

chrisduerr · 2020-01-10T02:46:28Z

font/src/ft/mod.rs

+            let fallback_list = self.fallback_lists.remove(&font_key).unwrap_or_default();
+
+            for font in fallback_list {
+                let font_path = font.file(0).unwrap();


I don't think any of the old code unwraps on font.file(0). Is this safe to do? Also mind explaining to me what the importance of the 0 is? Are there potentially multiple files available?

If this isn't just because this is a WIP, but it's still safe, we should probably expect.

This unwrap could be if let Some(), but I just I'd like to keep it like that for now, since I don't get it, how your pattern couldn't have a path after you query the thing. I'll come back to it later, since I don't really like to unwrap things.

Makes sense, yeah. I could see how file(0).unwrap() might be safe, but file(1).unwrap() might not be or something like that.

font/src/ft/mod.rs

chrisduerr · 2020-01-10T02:55:14Z

font/src/ft/mod.rs

+            // Reuse the original fontkey if you're reloading the font
+            let key = if let Some(key) = key {
+                key
+            } else {
+                FontKey::next()
+            };


Why do we bother with reusing the fontkey if possible here? Is that related to possible duplications between different fonts (like bold/italic/normal/...).

Imagine you're a downstream crate, and you request 2 fonts, A, B, where A == B. So I'm loading font A and getting FontKey 1, then I'm trying to load font B which is the same as A, so if I don't reuse FontKey, you'll invalidate a FontKey for A and generate a new one for B, so next time your downstream will query the font by FontKey,which it got for A it wont get the right one (I was fixing this issue during implementation). So you don't want your internal caches affect your public API. Technically this should also applies to fallback fonts, but since alacritty is the only one downstream right now, and wiping the cache I thought that it's fine for now.

font/src/ft/mod.rs

chrisduerr · 2020-01-10T03:06:18Z

font/src/ft/mod.rs

+                        // This code will be hit, only if you don't have glyph at all or didn't
+                        // loaded font, so not that hot for a normal scenario and all costly parts
+                        // are inside the if


If I understand this correctly, this code will load the font once, if it hasn't been loaded yet?

Why don't we do this when the fallback list is loaded initially for all fonts in that list? Is it to minimize the number of fonts we have to load, so on a system with 150 fallback fonts we don't load 150 fonts but instead just load the first few (assuming those cover most characters)?

Yes, it'll load once when you're facing a font that hasn't been loaded in your fallback list and the char you're requesting is presented in this font. This code is fast and there's no desire to build all faces for all fonts during load_font. I don't want to load 150 faces for fonts that we wont use. Like I can work only with ASCII most of time, why should I load all the things, it's just pointless.

Yeah, makes a lot of sense. Though it is a bit messy. Might be good to at least put that in another function with a descriptive name, so the initialization of a font isn't just sitting in some random match statement somewhere.

There are still some things I want to try on this branch "path", so I'll leave it here as it is for now, and come back later.

kchibisov · 2020-01-10T14:40:31Z

The main problem I see with the current FontKey + cache updates is that I didn't have much time to look into how we should approach our internal structure. Right now you can't have multiple fonts, but with different font sizes for e.g. So you can't have Fira Sans:size=12 and Fira Sans:size=14 at the same time. For Alacritty it's not that critical, but this crate lives on its own, so I think It makes sense to support such use cases. I also think that we shouldn't invalidate FontKeys for fallbacks, but I didn't have time to think of how to do it properly.

But there's nothing wrong in invalidating FontKeys for fallbacks tbh(without solving fist issue), because you can't load the same font twice, but with different sizes, so your fallbacks should also update their sizes -> we can safely invalidate them for now.

chrisduerr · 2020-01-10T14:43:27Z

Right now you can't have multiple fonts, but with different font sizes for e.g. So you can't have Fira Sans:size=12 and Fira Sans:size=14 at the same time. For Alacritty it's not that critical, but this crate lives on its own, so I think It makes sense to support such use cases.

That makes sense, yeah.

Maybe it makes sense to think about our API a bit and how we would want both Alacritty and others to use it.

chrisduerr · 2020-01-26T14:42:09Z

font/src/ft/fc/pattern.rs

+    pub fn get_charset(&self) -> &CharSetRef {
+        unsafe {
+            let mut charset: *mut _ = ptr::null_mut();
+            // We should likely check the result


If we should, we should probably do it now?

I can't find any docs on what it can return or fail with. Like I can check, but I don't know what to check...

These are convenience functions that call FcPatternGet and verify that the returned data is of the expected type. They return FcResultTypeMismatch if this is not the case.

font/src/ft/mod.rs

chrisduerr · 2020-01-26T14:46:37Z

font/src/ft/mod.rs

+            let fallback_list = self.fallback_lists.remove(&font_key).unwrap_or_default();
+
+            for font_pattern in &fallback_list.list {
+


Suggested change

chrisduerr · 2020-01-26T14:48:33Z

font/src/ft/mod.rs

-    fn face_from_pattern(&mut self, pattern: &fc::Pattern) -> Result<Option<FontKey>, Error> {
+    fn face_from_pattern(
+        &mut self,
+        pattern: &fc::PatternRef,


Suggested change

pattern: &fc::PatternRef,

pattern: &PatternRef,

I think PatternRef can probably safely be imported directly, right?

I think we can.

chrisduerr · 2020-01-26T14:59:27Z

font/src/ft/mod.rs

+                        // Recreate a pattern
+                        let mut pattern = fc::Pattern::new();
+                        pattern.add_pixelsize(self.pixel_size as f64);
+                        // Unwrap should be safe in general, however let's play safe here


I don't think there's much value in this comment. If it's 100% safe and we decide to go for it, we should just unwrap. If we have doubts, then we should fallback. But stating that it's 100% safe while doing the opposite will just lead to confusion.

I guess I've forgot to remove this comment or something like that.

chrisduerr · 2020-01-26T15:02:43Z

font/src/ft/mod.rs

+                        let config = fc::Config::get_current();
+
+                        // Recreate a pattern
+                        let mut pattern = fc::Pattern::new();


Why do we have to create a new pattern here, instead of using the pattern we cloned from font_pattern and then just setting the pixelsize on that?

This is a specific thing to docs... You should use your "original pattern" (the one you've used to font_sort) and use a thing from font_sort(the font_pattern) + render_pattern to get a valid thing.

chrisduerr · 2020-01-26T15:03:05Z

font/src/ft/mod.rs

+                        pattern.add_family(
+                            font_pattern.family().next().unwrap_or_else(|| "monospace"),
+                        );
+                        // We should render pattern, otherwise most of its properties wont work


Suggested change

// We should render pattern, otherwise most of its properties wont work

// Render pattern, otherwise most of its properties wont work

chrisduerr · 2020-01-26T15:04:31Z

font/src/ft/mod.rs

+                        // We should render pattern, otherwise most of its properties wont work
+                        let pattern = pattern.render_prepare(config, font_pattern);
+
+                        let key = self.face_from_pattern(&pattern, None)?.unwrap();


Unwrapping on this seems strange? How do we know it will always be Some?

You can get None if your pattern doesn't have path, but you're already matching by path in this loop, so you know that pattern has path, that means that check can't fail.

chrisduerr · 2020-01-26T15:06:28Z

font/src/ft/mod.rs

+        // Theoretically you shouldn't reach this point, but let's just keep it and don't try to be
+        // smart and assert here
+        Ok(glyph.font_key)


Similar to the unwrap comment, I don't think this comment is great. It also indicates that maybe our code structure isn't ideal. Having this here at the end 'just to be safe' with a half-hearted comment does not give me much confidence in this function.

I don't want to wrap everything in a giant if-else or something like that, suggestion are welcome here.

Fixes alacritty#3134. Fixes alacritty#2657. Fixes alacritty#1560. Fixes alacritty#965. Fixes Linux/BSD parts of alacritty#511.

font/src/ft/mod.rs

chrisduerr

I think this is good to go as far as I'm concerned.

chrisduerr self-requested a review January 7, 2020 23:17

kchibisov force-pushed the rework-fc-fallbacks branch from 9cc6e9c to b3f653d Compare January 8, 2020 15:07

chrisduerr reviewed Jan 10, 2020

View reviewed changes

kchibisov force-pushed the rework-fc-fallbacks branch from b3f653d to b586616 Compare January 10, 2020 13:47

kchibisov mentioned this pull request Jan 11, 2020

Match font family & style for fontconfig fallback #2935

Closed

kchibisov force-pushed the rework-fc-fallbacks branch from 51b3aa4 to f77f502 Compare January 13, 2020 10:36

This was referenced Jan 13, 2020

Some emoji are rendered black and white while others are rendered colored #3199

Closed

Colored emojis on Linux/BSD with freetype. #3011

Merged

kchibisov added the C - waiting on author label Jan 15, 2020

kchibisov added this to the Version 0.4.2 milestone Jan 15, 2020

kchibisov force-pushed the rework-fc-fallbacks branch 4 times, most recently from a9f608d to 0499f4f Compare January 22, 2020 22:47

kchibisov removed the C - waiting on author label Jan 25, 2020

kchibisov force-pushed the rework-fc-fallbacks branch from 0499f4f to 7cb289c Compare January 25, 2020 01:00

kchibisov requested a review from chrisduerr January 25, 2020 01:01

kchibisov mentioned this pull request Jan 26, 2020

RFC: Font Configuration #957

Open

chrisduerr requested changes Jan 26, 2020

View reviewed changes

kchibisov added 2 commits January 26, 2020 18:39

Make fontconfig fallback to use fallback lists computed from font_sort

89f5315

Fixes alacritty#3134. Fixes alacritty#2657. Fixes alacritty#1560. Fixes alacritty#965. Fixes Linux/BSD parts of alacritty#511.

Update style + comments

35faa8d

kchibisov force-pushed the rework-fc-fallbacks branch from 7cb289c to 35faa8d Compare January 26, 2020 15:40

kchibisov added 5 commits January 26, 2020 18:41

Remove comment

25f0254

Use result

2b67a3a

Format a bit more

00cdadf

Fix check

c52f6c1

Remove unwrap_or_else

b76b5e3

kchibisov requested a review from chrisduerr January 26, 2020 17:31

chrisduerr requested changes Jan 26, 2020

View reviewed changes

Update style

43e7d47

chrisduerr force-pushed the rework-fc-fallbacks branch from c780141 to 43e7d47 Compare January 26, 2020 22:09

chrisduerr approved these changes Jan 26, 2020

View reviewed changes

Merge branch 'master' into rework-fc-fallbacks

eb178d6

kchibisov merged commit 6b327b6 into alacritty:master Jan 27, 2020

This was referenced Jan 27, 2020

Invistigate macOS performance on unicode #3254

Open

Can't render Chinese characters in bold face #3271

Closed

kchibisov deleted the rework-fc-fallbacks branch February 13, 2020 06:01

abelsromero mentioned this pull request Apr 8, 2020

Alacritty doesn't display emoji #153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework fontconfig fallback to use fallback list computed from font_sort #3163

Rework fontconfig fallback to use fallback list computed from font_sort #3163

kchibisov commented Jan 7, 2020 •

edited

chrisduerr left a comment

chrisduerr Jan 10, 2020

kchibisov Jan 10, 2020

chrisduerr Jan 10, 2020

chrisduerr Jan 10, 2020

kchibisov Jan 10, 2020 •

edited

chrisduerr Jan 10, 2020

kchibisov Jan 10, 2020

chrisduerr Jan 10, 2020

kchibisov Jan 10, 2020

kchibisov commented Jan 10, 2020 •

edited

chrisduerr commented Jan 10, 2020

chrisduerr Jan 26, 2020

kchibisov Jan 26, 2020

chrisduerr Jan 26, 2020

chrisduerr Jan 26, 2020

chrisduerr Jan 26, 2020

kchibisov Jan 26, 2020

chrisduerr Jan 26, 2020

kchibisov Jan 26, 2020

chrisduerr Jan 26, 2020

kchibisov Jan 26, 2020

chrisduerr Jan 26, 2020

chrisduerr Jan 26, 2020

kchibisov Jan 26, 2020

chrisduerr Jan 26, 2020

kchibisov Jan 26, 2020

chrisduerr left a comment

		let fallback_list = self.fallback_lists.remove(&font_key).unwrap_or_default();

		for font_pattern in &fallback_list.list {

	// We should render pattern, otherwise most of its properties wont work
	// Render pattern, otherwise most of its properties wont work

Rework fontconfig fallback to use fallback list computed from font_sort #3163

Rework fontconfig fallback to use fallback list computed from font_sort #3163

Conversation

kchibisov commented Jan 7, 2020 • edited

chrisduerr left a 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

kchibisov Jan 10, 2020 • edited

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

kchibisov commented Jan 10, 2020 • edited

chrisduerr commented Jan 10, 2020

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

chrisduerr left a comment

Choose a reason for hiding this comment

kchibisov commented Jan 7, 2020 •

edited

kchibisov Jan 10, 2020 •

edited

kchibisov commented Jan 10, 2020 •

edited