Use the same FT load flags in HB as in FT #230
Conversation
Avoids mismatches & broke-ass advances
|
That seems like the sane thing to do, and is what raqm does, FWIW. Hopefully leads to much more consistent results w/ HB + hinting. (I, err, did not test it :D). |
Been a while since I touched anything C++ ;D.
crengine/src/lvfntman.cpp
Outdated
| if (!_hb_font) | ||
| error = FT_Err_Invalid_Argument; | ||
| // Use the same load flags as we do when using FT directly, to avoid mismatching advances & raster |
There was a problem hiding this comment.
I guess the added part should be in a else {...} (if _hb_font ok) (not sure what happens next when _hb_font fails :)
There was a problem hiding this comment.
Indeed! I mistakenly thought the error path was a return, for, err, some reason? :D.
Thought that error was a return, for some reason >_<".
|
And, as the flags may change widths and lines layout, and previously cached calculations may not match how words will be drawn after that, you'd better increment this: crengine/crengine/src/lvtinydom.cpp Lines 66 to 67 in bb7b705 (although just toggling any setting and back will re-render the document correctly when one feels it looks ugly) |
| error = FT_Err_Invalid_Argument; | ||
| } else { | ||
| // Use the same load flags as we do when using FT directly, to avoid mismatching advances & raster |
There was a problem hiding this comment.
Ah, excellent!
So by default it uses the hinting as specified by the font or something? 'cause I only see auto and no hint.
There was a problem hiding this comment.
It defaults to FT_LOAD_DEFAULT | FT_LOAD_NO_HINTING (i.e., no hinting)... for apparently complex legacy reasons (the one release that flipped the defaults lasted all of 24h, because that massively broke firefox :D).
There was a problem hiding this comment.
I thought all relevant hinting patents had expired?
Could you link me to the Firefox thing? I must be out of the loop on that one. I guess the Mozilla dev thingy blog doesn't advertise their own failures. :-D
There was a problem hiding this comment.
https://github.com/harfbuzz/harfbuzz/issues/143
There was a problem hiding this comment.
Oh, I thought you meant within the past few weeks. I may have a vague recollection of that thing from a few years back. :-)
|
Mhhhh. Performance with harfbuzz went dramatically down with this.
If you I don't have the eyes and taste to catch any mismatches & broke-ass advances if there are any, so I can't say if this is really needed or not. |
|
I was expecting a hit, but admittedly not that severe... ;). |
|
But if you use |
|
Oups, no, you don't :( (forgot to uncomment the |
|
Well, it's superslow whatever the flags, except when you use the flags that are the defaults in harfbuzz: There's also somes notes marked TODO at https://github.com/harfbuzz/harfbuzz/blob/22defe0965adddaa09eebc13df7fa6c64e2abba3/src/hb-ft.cc#L42-L62 So, revert the flags parts? Or do you have some other ideas? Also, if we were to keep that, there's more to do: they are currently just set from the _hintingMode value at font load time - _hintingMode may be changed later when hinting is changed, and the flags would currently not be updated - it would need the same kind of thing done in crengine/crengine/src/lvfntman.cpp Lines 936 to 941 in c02f597 which is called for each loaded font when the global hinting mode is changed by: crengine/crengine/src/lvfntman.cpp Lines 2352 to 2364 in c02f597 |
|
I'll take a look at it later tonight (but, yeah, I smell a commenting out coming ;D). (And yeah, HB is aware that this is slow with hinting enabled, there's a couple issues/PRs on the subject, besides the recommendations of caching as much stuff as possible on the app's side). |
|
(btw, I haven't seen anything wrong with the current |
|
Okay, so, clearly, it's not viable as-is in terms of performance. But on the other hand, the other behavior seems somewhat broken, which explains the crazy spacing variations. (f.g., with the matched flags, I get a result that's basically identical to FT, but with proper kerning, the spacing of non-kerned pairs is untouched, which makes sense, and was pretty much the point of the whole thing ;)). |
|
Okay, a few comparison shots. FeeType, AutoHint FreeType linked against HarfBuzz, AutoHint Fairly identical results, only the hyphen is a tad more compressed on the second shot. |
|
FreeType, Native hinting FreeType linked against HarfBuzz, Native hinting Exact same observations, only the line w/ the hyphenation see a tiiiiny change. |
|
HarfBuzz, AutoHint Harfbuzz, Native hinting Spacing is weird/gone (but that actually looks fairly decent w/ Noto Serif, at this size, at least). |
|
HarfBuzz, matching flags, Native HarfBuzz, matching flags, AutoHint General spacing matches the FT result, but with kerning pairs apparently better obeyed. |
|
Now, shit starts getting weird. HarfBuzz, matching flags, autohinter, with a FT linked against HB. That's my preferred result, right there! And, again, general spacing matches the FT linked w/ HB results. And now, for the weird one:
This one's a mess, plain and simple. |
|
@poire-z : I don't quite get how we actually end up with the native hinter in CRE... Is that the default case, and hoping FT was built w/ native hinting enabled? Because that might need an explicit |
I like it, but it's just way too expensive given how things are implemented right now. re koreader#230
|
These tests were in the original code, and I assume they are allright when I read this in freetype.h: |
|
I really don't have an eye for catching what's wrong :) so take my comments for what they are :|
And in both your It mostly looks better on eInk than on our computer screens. The only bothering thing on eInk are the occassional fat and stuck |
|
@poire-z : Yeah, except FT can be built with no native hinting support at all, and has a mind of its own as to whether it actually always do what you tell it to (mostly, when dealing with tricky fonts). But, yeah, generally, nothing means native, but I tend to prefer being very very explicit when I want native hinting for sure ;). I'll see if that makes any difference in this case, though ;).
The autohint variant, on the other hand, looks pretty damn good (and, specifically, that from might not look all that great, but it is identical to FT's version ;)). |
|
Quick'n dirty visualisation w/ IM's compare tool (autohint in all cases). FT to HB w/ Flags FT to FT+HB FT to HB w/ flags w/ FT+HB |
|
^^ might need more proper arguments to the And it's unclear to me what |
|
Native: Auto: |
|
It's not honoring FT advances at all, which is basically what using the hb_ft approach with the wrong flags does, as is shown by the comparison shots (perfect match in both cases) ;). (i.e., it's using unhinted metrics). HB to PZ (Auto) HB To PZ (Native) |
|
Possibly a bit faster (I can still hear my CPU fan start to spin up ;p.) with the PR in, but it's apparently not perfect, so I'm not sure how viable that'd be. |
|
(OK :| about my hb_font_create try, thanks for testing :) |
|
@poire-z : I don't really have hard numbers since it was only spinning the CPU for a few seconds with only Noto Serif & Noto Sans being loaded ;). So, faster, yeah, but I can't really quantify it ;). Results were identical as without the PR, AFAICT, but I'll double-check. |
|
Ah. If you look at the page, stuff renders indentically, but there might be something wonky going on somewhere, because positioning/line-spacing is going bonkers:
|
|
Take that w/ a grain of salt, as I moved that build to my base head, so that's HB 1.8.8 vs. 1.8.7 (... plus whatever else might have changed, since that was a from-scratch build from a newly bootstrapped checkout). |
|
Yeah, the different spacing is the same with the other kerning methods, so, err, possibly because of something else that changed somewhere :D (Or I botched the CRe patch). |
|
Well, with latest crengine master (so, including this PR and not the one that revert it), with HB 1.8.7 and freetype NOT rebuilt against harfuzz, with their 1082.patch and this patch to our base, after cleaning the harfbuzz build tree: --- a/thirdparty/harfbuzz/CMakeLists.txt
+++ b/thirdparty/harfbuzz/CMakeLists.txt
@@ -15,10 +15,12 @@ assert_var_defined(FREETYPE_DIR)
ep_get_source_dir(SOURCE_DIR)
ep_get_binary_dir(BINARY_DIR)
+set(PATCH_CMD1 sh -c "patch -N -p1 < /tmp/1082.patch || true")
+
set(CFG_ENV_VAR "CC=\"${CC}\" CXX=\"${CXX}\" CFLAGS=\"${CFLAGS}\" CXXFLAGS=\"${CXXFLAGS}\" LDFLAGS=\"${LDFLAGS}\"")
set(CFG_ENV_VAR "${CFG_ENV_VAR} FREETYPE_CFLAGS=\"-I${FREETYPE_DIR}/include/freetype2\"")
set(CFG_ENV_VAR "${CFG_ENV_VAR} FREETYPE_LIBS=\"-L${FREETYPE_DIR} -lfreetype\"")
-set(CFG_OPTS "--prefix=${BINARY_DIR} --enable-shared --disable-static --with-freetype --with-ucdn --without-glib --without-gobject --without-cairo --without-fontconfig --without-icu --without-graphite2 --without-uniscribe --without-directwrite --without-coretext --host=\"${CHOST}\"")
+set(CFG_OPTS "--prefix=${BINARY_DIR} --enable-shared --disable-static --with-freetype --with-ucdn --without-glib --without-gobject --without-cairo --without-fontconfig --without-icu --without-graphite2 --without-uniscribe --without-directwrite --without-coretext --enable-h_advance_cache --host=\"${CHOST}\"")
set(CFG_CMD sh -c "${CFG_ENV_VAR} ${SOURCE_DIR}/configure ${CFG_OPTS}")
ko_write_gitclone_script(
@@ -33,7 +35,7 @@ ExternalProject_Add(
${PROJECT_NAME}
DOWNLOAD_COMMAND ${CMAKE_COMMAND} -P ${GIT_CLONE_SCRIPT_FILENAME}
BUILD_IN_SOURCE 1
- PATCH_COMMAND NOCONFIGURE=1 ./autogen.sh
+ PATCH_COMMAND COMMAND NOCONFIGURE=1 ./autogen.sh COMMAND ${PATCH_CMD1}
CONFIGURE_COMMAND ${CFG_CMD}
BUILD_COMMAND $(MAKE) -j${PARALLEL_JOBS} --silent
INSTALL_COMMAND $(MAKE) -j${PARALLEL_JOBS} install(not sure about the order of the patch commands...) I get back the 13s render time ! So, timing wise, harfbuzz PR 1082 helps a lot! Now, it's for you to confirm you get the kerning/hinting as you expect/liked it with this PR not reverted (= the same as in your screenshots from 2 days ago). |
|
Okay, I'm stupid, the spacing discrepancies were because... I had changed the spacing, and forgot to do that again on the new checkout :D. |
|
Okay, tried again with matching settings on master, similar results, with a caveat: Native hinting + harfbuzz shaping is borked when FT is NOT built against HB (for... some reason? That doesn't make sense, AFACT, FT+HB should only affect Auto). (It's okay with FT kerning). And, of course, an FT+HB build seems to slow things down some more with harfbuzz shaping... EDIT: Conversely, with FT+HB, it's Auto that's weird, and doesn't match my previous results... -_-". I'm also not feeling much speedup from the PR ;? |
|
TL;DR: Meh? I'll check again tomorrow without the PR, to see if I can match yesterday's results again. |
|
Also, forgot to answer that: yep, wrong patching order: patch first, autoreconf/autogen later, especially here, with a patch affecting configure.ac ;) |
OK. It works as well with the right ordering. |
|
Just a note, in case we use 1082.patch, and unrevert the |
|
@NiLuJe I just had a try with uncommenting the 2 So, you may want to revisit this again :) |
|
Yep, I noticed ;). Haven't had time to check again, though. Plus, IIRC, for best results, we'd have to do the whole "we need FT built against HB" thing. |
Re-enable c02f597 (koreader#230) that was reverted by 330d7b1 (koreader#231), as the performance hit as been fixed on Harfbuzz side by using caching. Also re-init HB fonts on hinting mode change to clear HB's internal caches (so we get reproducible renderings when changing hinting mode).
Re-enable c02f597 (koreader#230) that was reverted by 330d7b1 (koreader#231), as the performance hit has been fixed on Harfbuzz side by using caching. Also re-init HB fonts on hinting mode change to clear HB's internal caches (so we get reproducible renderings when changing hinting mode).
Avoids mismatches & broke-ass advances