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

UI font rendering: use available bold fonts for bold #5675

Merged
merged 1 commit into from Dec 8, 2019

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Dec 7, 2019

Follow up to #5598 "we might need to work on our bold font use and handling..."

A few fixes and enhancement related to bold text:

  • When using bold=true with a regular font, use its bold variant if one exists.
  • When using a bold font without bold=true, promote bold to true, so fallback fonts are drawn bold too.
  • Whether using a bold font, or using bold=true, ensure fallback fonts are drawn bold, with their available bold variant if one exists, or with synthetized bold.
  • When using a bold variant of a fallback font, keep using the regular variant as another fallback (as bold fonts may contain less glyphs than their regular counterpart).
  • Allow providing bold=Font.FORCE_SYNTHETIZED_BOLD to get fake bold even when a bold font exists (might be interesting to get text in bold the same width as the same text non-bold).
  • Use the font realname in the key when caching glyphs, instead of our aliases (cfont, infont...) to avoid duplication and wasting memory.

Some of these issues are there since the switch to XText (#5598), but some where there before. I think CJK users may have seen our titles non-bold (I hope using synthetized bold with NotoSansCJK is ok and doesn't make the CJK glyphs too busy).

Before:
image

After:
image

Before:
image

After:
image


This change is Reviewable

@poire-z
Copy link
Contributor Author

poire-z commented Dec 8, 2019

Just added a hidden setting to avoid using bold fonts when bold=true with a non-bold font - to allow for easier comparison (and user tweaking): "use_bold_font_for_bold" = false
Real bold fallback fonts would still be used when the main font is real bold.

May be I'm too used after a few weeks to the "thin" bold made by FreeType Outline emboldening, but I find the real bold provided by NotoSansBold a bit less elegant than the fake bold :) Mainly in ButtonTable/ButtonDialog (the highlight dialog, or the button at the bottom of the dict result) - it's also wider, so more prone to have buttons' long translated text truncated.

Before XText, we had for these the regular font with Freetype Glyph emboldening, which made the glyphs wider.
With XText, we currently have the regular font with Freetype Outline emboldening, which keeps the regular glyphs widths (and look a little less bold than Freetype Glyph emboldening).
With this PR, we have the bold font with no tweaking - and it looks a little more like "Before XText" (but possibly even wider, not sure) so I guess it will not strike users who dont use nightlies.

Anyway, please tell me how you like it or not when this is merged.

If we'd just want to have narrower bold for buttons, we could just go with:

--- a/frontend/ui/widget/button.lua
+++ b/frontend/ui/widget/button.lua
@@ -49,5 +49,5 @@ local Button = InputContainer:new{
     text_font_face = "cfont",
     text_font_size = 20,
-    text_font_bold = true,
+    text_font_bold = Font.FORCE_SYNTHETIZED_BOLD,
 }

A few fixes and enhancement related to bold text:
- When using bold=true with a regular font, use its bold
  variant if one exists (can be prevented by manually
  adding a setting: "use_bold_font_for_bold" = false).
- When using a bold font without bold=true, promote bold
  to true, so fallback fonts are drawn bold too.
- Whether using a bold font, or using bold=true, ensure
  fallback fonts are drawn bold, with their available bold
  variant if one exists, or with synthetized bold.
- When using a bold variant of a fallback font, keep using
  the regular variant as another fallback (as bold fonts
  may contain less glyphs than their regular counterpart).
- Allow providing bold=Font.FORCE_SYNTHETIZED_BOLD to
  get synth bold even when a bold font exists (might be
  interesting to get text in bold the same width as the
  same text non-bold).
- Use the font realname in the key when caching glyphs,
  instead of our aliases (cfont, infont...) to avoid
  duplication and wasting memory.

allow avoiding font promotion to bold font
@robert00s robert00s added this to the 2019.12 milestone Dec 8, 2019
@poire-z poire-z merged commit 55f3575 into koreader:master Dec 8, 2019
@poire-z poire-z deleted the better_bold_font_handling branch December 8, 2019 19:31
@poire-z
Copy link
Contributor Author

poire-z commented Dec 11, 2019

Adding this note in this issue that is the most recent Freetype/Font related PR.

From #5525 (comment):

Also on the list of things to consider: FreeType's caching subsystem?

Regarding that, I had a look. It's quite hard to get into :| and understand the interaction of face, size, and the FTC_ImageCache_Lookup that don't accept any face or size as parameters :) Hopefully, there's another quite discreet function FTC_ImageCache_LookupScaler that looks better.
https://freetype.org/freetype2/docs/reference/ft2-cache_subsystem.html doc from C headers
http://www.fifi.org/doc/libfreetype6/cache.html only other documenation found

Anyway, I didn't find much code that uses it, and the only one I found that uses it in a way similar to how we could use it is:
https://github.com/xerpi/sftdlib/blob/master/libsftd/source/sftd.c

So, the idea is that instead of instantiating a FT face and keep it as ftface in our face_obj object, we'd just store an ID, and never instantiante a FT_Face directly, and just provide that ID to ffi wrappers that would use these FTC_ functions, that would hiddenly manage these FT_Face instances in some kind of a pool.
Unfortunatly, with XText (#5598), we pass that ftface instantiated object to XText, that passes it to Harfbuzz, and Harfbuzz instantiates/re-uses that FT_Face to instantiate its own HB_Font, that we then store (as a Lua userdata that keeps it alive) into that face_obj - so in effect killing the benefit of that pool (if there's any benefit at all, not sure).

We could still use it to cache glyphs, and drop our Lua glyph Cache - but there would still be that alternative pool of FT_Face used by that cache, so we'll have these duplicated anyway.
It could also be complicated by the fact that FTC would not help caching both the regular and the synthetized bold glyphs, so we may need some tricks or convolutions.
I dunno how much FT own glyph caching would compare to our Lua glyphs BB caching regarding performance, memory usage and memory fragmentation.
I don't notice any performance problem with our current Lua cache code.
Memory fragmentation might be what interests me the most - it might be all our little bb glyphs that are created all along the adress space that prevent memory from shrinking once you close a Wikipedia result.
And I'm not sure FTC would help with that: it does not seem to do any malloc stuff, it just cache faces and glyphs created by the FT code and increase reference counting, so they would still be and stay anywhere where there was some room at creation time.
What I'd like, I guess, is for a cache system to allocate at startup 200K (FTC default) or 512K (our Lua cache default) of memory, and keep putting glyphs in that static area.
Which is not what FTC provides.

So, dropping any more thinking about switching to it :)

@NiLuJe
Copy link
Member

NiLuJe commented Dec 11, 2019

Yeah, even in the best of worlds, mixed with plain HB, that would have made things more complex, that's perfectly understandable ;).

@poire-z
Copy link
Contributor Author

poire-z commented Dec 13, 2019

May be I'm too used after a few weeks to the "thin" bold made by FreeType Outline emboldening, but I find the real bold provided by NotoSansBold a bit less elegant than the fake bold :) Mainly in ButtonTable/ButtonDialog (the highlight dialog, or the button at the bottom of the dict result) - and for the book titles in CoverBrowser list mode

Any feedback on this? @NiLuJe (you're the customer for real bold :), you're fine with NotoSansBold being more often used?

@NiLuJe
Copy link
Member

NiLuJe commented Dec 13, 2019 via email

mwoz123 pushed a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020
A few fixes and enhancement related to bold text:
- When using bold=true with a regular font, use its bold
  variant if one exists (can be prevented by manually
  adding a setting: "use_bold_font_for_bold" = false).
- When using a bold font without bold=true, promote bold
  to true, so fallback fonts are drawn bold too.
- Whether using a bold font, or using bold=true, ensure
  fallback fonts are drawn bold, with their available bold
  variant if one exists, or with synthetized bold.
- When using a bold variant of a fallback font, keep using
  the regular variant as another fallback (as bold fonts
  may contain less glyphs than their regular counterpart).
- Allow providing bold=Font.FORCE_SYNTHETIZED_BOLD to
  get synth bold even when a bold font exists (might be
  interesting to get text in bold the same width as the
  same text non-bold).
- Use the font realname in the key when caching glyphs,
  instead of our aliases (cfont, infont...) to avoid
  duplication and wasting memory.
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.

None yet

3 participants