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

Selectable kerning methods: disabled / freetype / harfbuzz #228

Merged
merged 6 commits into from
Aug 17, 2018

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Aug 17, 2018

See #224.
Includes from upstream:

Kerning methods made selectable: Off / Freetype / Harfbuzz as Harfbuzz shaping it a bit slower than pure-freetype, but it is perfectly usable on small books, and on big books once a cache has been built. It's just the rendering when toggling an other option that will take a bit longer to re-render the document.

Anyway, it looks like harfbuzz does more than kerning by default, and does some Right-To-Left shaping of words in arabic (I can't read, but it looks like they are reversed :) , but only on the word level: in the sentence/line, they are in a Left-To-Right order (as they are in the HTML). I guess that would be lvtextfm.cpp work to do it on the line level.

Another thing I forgot to add as a comment, but did not investigate much: the lines like:
hb_feature_from_string("+kern", -1, &_hb_kern_feature);
do not seem to change harfbuzz behaviour. Even when not requesting +kern, we get ligatures (so, may be these are different features, and ligatures are enabled by default).
For reference: https://www.preusstype.com/techdata/features.php (I tried to enable a few, got no change in my sample text).

Closes #224.

Picked the only relevant changes (crengine/*) from upstream PRs:
    buggins/coolreader#24
    buggins/coolreader#29
Fixed a single rejection due to our soft-hyphens support changes.
Make the kerning method selectable:
- disabled
- freetype (same as always)
- harfbuzz (new method)

Text shaping/kerning with HarfBuzz may be 2 times slower than
previously with freetype only, so let's keep the fast method.
Also, the auto-hinting with freetype is ugly with harfbuzz, but
it still provides a pleasant airy rendering with freetype kerning.
It was wrong since we increased the number of gamma values
from 30 to 56 (so the value 15 has uneeded processing, and the
value 28 was ignored.)
Mostly comments, as HarfBuzz already deals correctly with
soft-hyphens, and original PRs also did that correctly.
@poire-z
Copy link
Contributor Author

poire-z commented Aug 17, 2018

Just adding some notes I found for reference:

https://lists.freedesktop.org/archives/harfbuzz/2015-December/005381.html

The OpenType model positions glyphs from left to right always, HarfBuzz
should give you the glyphs and positions in their visual order, so after
shaping you don’t need to care whether the text was LTR or RTL (until
you start doing line breaking, because it need to be done in logical
order).

https://lists.freedesktop.org/archives/harfbuzz/2015-December/005387.html
about hb_buffer_guess_segment_properties, that we use, that guesses some words are RTL:

HarfBuzz does not do an such detection by default (there is the guess
segment properties function, but it does very simplistic detection and
is meant only for quick testing, not real world use).

https://lists.freedesktop.org/archives/harfbuzz/2015-August/005036.html
links to various harfbuzz incomplete documentations

@Frenzie
Copy link
Member

Frenzie commented Aug 17, 2018

Well, I guess I can just read a book whenever I was planning to look into this. Thanks! :-D

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.

Nothing strikes me as suspicious looking, but I didn't look in close detail because I assume it was already reviewed in the other repo as well and I imagine you also had a look at it. :-)

@poire-z
Copy link
Contributor Author

poire-z commented Aug 17, 2018

It's been disabled today in the other repo.
But as it's a toggable for us, and the default is still freetype-only, and I tried to make sure no harfbuzz code was used (except some data structure initialisation, that have no noticable impact on performances), it should be fine to merge, so people can taste a bit of HarfBuzz.
Just curious if you'll find that with it, E Ink can look finally as good as cheap newspaper :)

@poire-z poire-z merged commit 95dc60b into koreader:master Aug 17, 2018
@poire-z poire-z deleted the harfbuzz branch August 17, 2018 16:46
@Frenzie
Copy link
Member

Frenzie commented Aug 17, 2018

MuPDF (in EPUB as well as PDF) already does that. Including literally. The NRC subscription includes downloadable PDFs of the articles. (Not that I stick them on the ereader… but I could.)

@virxkane
Copy link
Contributor

virxkane commented Aug 19, 2018

This line

hb_feature_from_string("+kern", -1, &_hb_kern_feature);

enable kerning feature. If you don't look difference maybe you font don't have any kerning table.

@poire-z
Copy link
Contributor Author

poire-z commented Aug 19, 2018

Thanks @virxkane , and for your work on this HarfBuzz integration :)
OK, I didn't check that hb_feature_from_string("+kern"...) much. I just wondered if there were other features that would be interesting to enable too, tried some other in place of +kern and didn't see much difference.
Anyone know about a latin font super-loaded with OpenType features, so we can see what harfbuzz would do with more enabled?
For reference again: https://en.wikipedia.org/wiki/List_of_typographic_features

@virxkane
Copy link
Contributor

@poire-z
Copy link
Contributor Author

poire-z commented Aug 21, 2018

Just mentionning that (no intent to investigate):
Standalone paragraph with arabic or hebrew text is badly rendered with harfbuzz, like one glyph per line, which make the number of pages rise like rabbits :) (it's still ok for inline arabic or hebrew in english text).
With freetype, it is rendered on a line, ok, still LTR badly, but it doesn't have any impact on the rest of the book.
So, another reason to stick with freetype as the default.
(can be tested with the EPUB available at https://www.mobileread.com/forums/showthread.php?t=211123)

@poire-z
Copy link
Contributor Author

poire-z commented Apr 7, 2019

Mentionning this Mobiread post, with some sample EPUB mixing english and persian words, and screenshots of how it should look: https://www.mobileread.com/forums/showthread.php?p=3828770#post3828770
(It renders badly for us with "best", just as explained in my previous post above.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants