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.

Sign up for GitHub

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

Jump to bottom

cre: use 'best' (Harfbuzz) as the default kerning method #5553

Merged
merged 5 commits into from Nov 1, 2019
Merged

cre: use 'best' (Harfbuzz) as the default kerning method #5553

merged 5 commits into from Nov 1, 2019

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Oct 31, 2019

It might be slower, but is needed to properly display books in arabic, indic...
See #5545 (comment) and follow ups.

One thing I'm not sure about, but I often felt when trying "best" that it looked nicer with font hinting set to "off". Our default for font hinting is "auto" (Freetype auto hinting).
@NiLuJe : any opinion/science about that?
(Lately, I've been using Font hinting "off" with everything, as the other options were sometimes triggering some font bugs or something.)

Not really needed, but I'll update https://github.com/koreader/koreader-base/blob/master/cre.cpp#L510 next time I tweak base/.

@poire-z
cre: use 'best' (Harfbuzz) as the default kerning method
3e46606 
It might be slower, but is needed to properly display
books in arabic, indic...
@poire-z poire-z mentioned this pull request Oct 31, 2019
Closed
@NiLuJe
Copy link
Member

NiLuJe commented Oct 31, 2019 via email

@Frenzie Frenzie added this to the 2019.11 milestone Oct 31, 2019
Frenzie
Frenzie approved these changes Oct 31, 2019
View reviewed changes
Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

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

👍

@poire-z
Copy link
Contributor Author

poire-z commented Oct 31, 2019

Just to be sure, if/when you're using kerning "best", can you check on some of your books what you have for font hinting, if you really have the default "auto" (because I'm not sure best is really best with auto :)

@Frenzie
Copy link
Member

Frenzie commented Oct 31, 2019

Not sure if you were asking me, but I think hinting off/kerning best gives the best results, mainly in the form of the best baseline alignment,[1] and the best kerning. Combining one of the other hinting settings with kerning best results in slightly worse kerning and sometimes a forced straight baseline.

[1] I.e., most true to the font's intended design. It allows them to peek slightly under or over as designed.

@poire-z
Copy link
Contributor Author

poire-z commented Oct 31, 2019

(I was indeed asking you and @NiLuJe :)
I too find it better with Hinting: off - it avoids some kerning and baseline bugs (at least with my favorite font Bitter). But it makes things more condensed (that Harfbuzz already does, hinting off make that even more noticable), which I like - but people used to fast+auto will be surprised.

@NiLuJe
Copy link
Member

NiLuJe commented Oct 31, 2019 via email

@Frenzie
Copy link
Member

Frenzie commented Oct 31, 2019

I might prefer native hinting in theory, at least on smaller font sizes where 300 DPI doesn't make one lick of a difference with 265 and is still fully pixel bound. But in combination with the effect on kerning in crengine/HarfBuzz I don't in practice.

@poire-z
Copy link
Contributor Author

poire-z commented Oct 31, 2019

Strangely, for FreeSans and FreeSerif, best + auto is totally ugly on my small emulator (x), while it's ok on my Kobo. It's fine on both for NotoSans and NotoSerif.
(x: although with FreeSans, Auto seems to correct some ugly s or c that went below the baseline... it makes it better when using "fast"...)

So, what should we do: let Font hinting to Auto for less drastic changes (and wait for any feedback) - or change it to Off?
(Or make that dependant on device DPI - but my above test with the Free fonts makes me think hinting is not even better on small DPI device.)

@Frenzie
Copy link
Member

Frenzie commented Oct 31, 2019

I've never cared for the auto default, nor for its name. Some old related comment: #1929 (comment) I think it should be relabeled something like FreeType or FT to indicate it's FreeType autohinting, because to me it implies using native when available.

I wouldn't quite call it ugly though? Just not as good. :-)

@poire-z
Copy link
Contributor Author

poire-z commented Oct 31, 2019 •
edited

I wouldn't quite call it ugly though?

I'm not that an aesthete regarding text, but I find it really ugly, all these glyphs merged into some black mess (that looks like super bad/off kerning), that we don't see on your screenshots with Off or Native:
image

it should be relabeled something like FreeType

I guess "auto" made it sound less technical.

We could go with "native" for a less drastic condensation than "off".
I remember reading "native" was sometimes buggy with fonts that did not provide native instructions (or provided buggy ones).
And once, I got a font that when switching to "native" made crengine go into some infinite loop somewhere (probably inside FreeType)- I didn't manage to track where things went wrong at the time, and I got rid of the font - I don't remember which one it was :|
edit: found it I think, it's called Isidore - all fine with Off or Auto, but with Native: ok with kerning off, blank pages with fast or best, and infinite loop with good (in crengine measureText(), so not inside Freetype).

@Frenzie
Copy link
Member

Frenzie commented Oct 31, 2019

Fine, it's ugly. ;-) But it's legible anyway.

@NiLuJe
Copy link
Member

NiLuJe commented Oct 31, 2019 via email

@poire-z
Copy link
Contributor Author

poire-z commented Oct 31, 2019

Light hinting should fix that

Is that about just replacing the 6 FT_LOAD_TARGET_NORMAL in crengine/src/lvfntman.cpp with FT_LOAD_TARGET_LIGHT (https://www.freetype.org/freetype2/docs/reference/ft2-base_interface.html#ft_load_target_xxx and https://www.freetype.org/freetype2/docs/text-rendering-general.html) ?

Just tested on the emulator, and it looks indeed cleaner! There's now hardly any difference between native and auto (to the point I wondered if it worked :) but there is some subtle ones: I notice only some small changes vertically (the tip of the t or f, or the dot on a i going higher with some fonts with Auto, with some other fonts with Native...). But both, like previously, make the text horizontally streched vs off (that makes it more condensed) - so I guess it works.

@NiLuJe
Copy link
Member

NiLuJe commented Oct 31, 2019

Yep ;). That is essentially the point: no more hinting on the x axis. Bonus points, that simplifies advances computation, as they're no longer modified by hinting ;).

The UI could probably also use it in freetype or font (I can't remember which does the load).

@poire-z
Copy link
Contributor Author

poire-z commented Oct 31, 2019

But both, like previously, make the text horizontally streched vs off (that makes it more condensed)

Well, that happens (now?) only with kerning "off" or "fast" - not with "good" or "best".
So, dunno, it might be we've had a bug with kerning vs hinting: should hinting on the x-asis change that much the spacing between letters, for a same kerning mode?
Anyway, it gets more stable with "best", so that's a plus to go with "best" as the default. And now that "auto" and "native" are not so much different, staying with "auto" is ok. (haven't read/understand everything in the 2nd url above, so I dunno if I understood right that auto may try to use the native hinting first with that load_target_light flag).
And I might not want to fix that bug, as that airy vs condensed feeling that this kerning/hinting bug may give is quite a nice bonus feature (now non-default with this PR).

Anyway, looks fine too on my Kobo. So I'll PR and bump crengine with that tomorrow.

Bonus points, that simplifies advances computation, as they're no longer modified by hinting ;).

For free? No other code change needed on the crengine side?

Using your FT cache submsystem , while sounding good, might need a lot of changes in crengine, which does its own caching - dunno how better or not that would be.
But using it on the UI side might be easier - but may be after my harfbuzz/fribidi - so we can see how to rework that freetype/font code to make it play fine in 2 different contexts.

@poire-z poire-z mentioned this pull request Nov 1, 2019
Merged
@NiLuJe
Copy link
Member

NiLuJe commented Nov 1, 2019 via email

@poire-z
Copy link
Contributor Author

poire-z commented Nov 1, 2019 •
edited

But from https://www.freetype.org/freetype2/docs/text-rendering-general.html:

Setting ‘slight’ hinting usually leads to FT_LOAD_TARGET_LIGHT. This mode implied the auto-hinter before and has now been changed to mean “Use native vertical-grid-only-snapping if driver and font supports it and vertical-grid-only auto-hinter otherwise”. Right now, only the OpenType/CFF driver is supported. In the future, this will hopefully include the TrueType engine once full support for ClearType arrives.

(That doc might be outdated, dunno).
The first sentence is a bit ... unclear :) Should I read it as Just use FT_LOAD_TARGET_LIGHT to enable 'slight' hinting?
The next sentences seem to suggest we could have a 4th hinting mode, a real "auto", that would use native if there are some instructions, and freetype hinting only if there are none, by just using some other combinations of FT_LOAD_NO_AUTOHINT and FT_LOAD_NO_HINTING ?
edit: or would that happen right now automagically?

I should have made pretty sure no means no, auto means auto, and native means native

I get that you mean your work on these above flags, right?

@NiLuJe
Copy link
Member

NiLuJe commented Nov 1, 2019 via email

@poire-z poire-z mentioned this pull request Nov 1, 2019
Merged
@poire-z poire-z merged commit 6baa2af into koreader:master Nov 1, 2019
@poire-z poire-z deleted the kerning_default_harfbuzz branch November 1, 2019 22:07
@poire-z poire-z mentioned this pull request Nov 21, 2019
Merged
mwoz123 pushed a commit to mwoz123/koreader that referenced this pull request Mar 29, 2020
@poire-z @mwoz123
cre: use 'best' (Harfbuzz) as the default kerning method (koreader#5553)
833deac 
It might be slower, but is needed to properly display
books in arabic, indic...
Also bump crengine: use FreeType "light" hinting algorithm,
which hints on the y-axis only (and so avoid messing with
advances and kerning on the x-axis).
@poire-z poire-z mentioned this pull request May 10, 2020
Merged
@poire-z poire-z mentioned this pull request Apr 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Frenzie Frenzie Frenzie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2019.11
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@poire-z @NiLuJe @Frenzie