Skip to content
This repository has been archived by the owner on Mar 1, 2019. It is now read-only.

speed-up and freetype integration #120

Open
mviikki16 opened this issue Nov 20, 2015 · 58 comments
Open

speed-up and freetype integration #120

mviikki16 opened this issue Nov 20, 2015 · 58 comments

Comments

@mviikki16
Copy link

I have a couple of patches, the minimal set to replace getenv/sscanf type of settings reading and also speed-up some hot areas of the code which I attached (they also include a reversal of commit 75722f89290eb1cb316cd240763545d7277e0481 from 'master' which you might want to ignore).

After wrongly raising the speedup issue on fretype-devel mailing list I found that there is a possibility to finally integrate the cleaned up Infinality patchset upstream.

(Nov. 20th thread from here
http://lists.nongnu.org/archive/html/freetype-devel/2015-11/index.html)

Werner would need a maintainer and a proper integration with the settings API (FT_Property_{Set,Get}) for this to happen. If you are willing to continue with your work upstream please get in touch e.g. on freetype-devel list.

Regards,
/Mikko

inf_env.txt

@goddesse
Copy link
Contributor

@mviikki16 It might be easier for @bohoomil to integrate if you did a pull request through Github. I can help you with that if you want. If I understand your patch correctly, there's no longer a (user-friendly) way to completely customize the infinality parameters anymore and you just pick a preset?

Perhaps bohoomil could just apply http://lists.nongnu.org/archive/html/freetype-devel/2015-11/txtPJqSQCqZZz.txt and then decide how he wants to do a custom configuration file.

@mviikki16
Copy link
Author

The patch removes the slow parameter settings from within the hot code
areas and places them in a few custom presets. At the moment there's not
even the possibility to pick among these (the PUSH preset is always
taken). However, I uploaded it only as an idea and it's quite trivial
to extend with similar getenv type of setup or a more freetype agreeable
way as Werner suggests. The settings will then be read only once at
initialization and avoid the slow, repeated readings when rendering.

For me it does all I need already now while also providing a substantial
rendering speed boost (anybody compiling himself the library can "burn
in" its preferred settings). However, I can finalize the patch and add
the missing parts for a properly working solution once a suitable setup
is agreed also with freetype maintainers. I raised the issue here and
sent the early version to given an idea about it and get feedback from
Infinality patch-set maintainers as well.

Werner looks open to the idea of integrating the Infinality
modifications upstream once "cleaned-up". Would be nice to finally have
all the code in one place...

On 11/21/2015 01:07 PM, goddesse wrote:

@mviikki16 https://github.com/mviikki16 It might be easier for
@bohoomil https://github.com/bohoomil to integrate if you did a pull
request through Github. I can help you with that if you want. If I
understand your patch correctly, there's no longer a (user-friendly)
way to completely customize the infinality parameters anymore and you
just pick a preset?

Perhaps bohoomil could just apply
http://lists.nongnu.org/archive/html/freetype-devel/2015-11/txtPJqSQCqZZz.txt
and then decide how he wants to do a custom configuration file.


Reply to this email directly or view it on GitHub
#120 (comment).

@goddesse
Copy link
Contributor

Ah, this was mostly a request for comments.

I agree the environment variable reading should go, but even with that change, the main part of the infinality code (stem alignment) wouldn't be suitable to integrate with upstream as it still causes strange crashes upon occasion and has some miscellaneous poor code. I think it might be worth seeing how the experimental warping feature compares, maybe it would produce an acceptable substitute for people.

@mviikki16
Copy link
Author

Not as much an RFC but an attempt to make such changes available to whomever finds them useful.

I have used the Infinality patches since ~2013 from http://www.infinality.net originally. While I once had a crash issue with them, since early 2014 I had no more problems while propagating these updates through quite a few freetype releases (at least with the set of fonts I'm using). The code does need some extra cleanup but as it's under defines and can be enabled/disabled at will would still be good to get a bigger exposure if placed upstream, it will only accelerate its improvement. In my experience the visual quality is considerably improved when enabling Infinality although I haven't really done a comparison with recent "vanilla" freetype versions, only near versions 2.4.-2.5. (having an 100dpi monitor also makes these more visible, perhaps not longer the case when moving to higher resolutions and dpi's like QHD etc...)

@mviikki16
Copy link
Author

Just did a quick comparison also with latest freetype master (ver 2.6.1 d8fc009dbbab054164166b7dd643d5d72c9269c3) and I still see the similar extra clarity and reduction of red fringes I originally saw. Infinality @infinality.net did a really nice job even if his coding skills leave room to improvements.

@goddesse
Copy link
Contributor

Absolutely. He's the one responsible for the subpixel rendering code upstream, afterall. I'm just trying to give an honest assessment of the current code. Everyone upstream seems really helpful so if they'll accept the code in their repo once the need for envvars or a filesystem is removed, that's up to them.

@mviikki16
Copy link
Author

bohomil looks to be off-line these days...

I included more cleanup and speed gains for reading built-in rules. I have now a bit closer view of the code. Indeed, unfortunately it's quite far from the quality required to be integrated upstream... the multitude of options make it a rather difficult to understand mess.

I attached diffs to apply on top of the original Infinality code (clnup) as well as one to apply directly on freetype master (patch). Quite a bit of speed is gained but still lots to do for "clean" code... That would require properly understanding the algorithm for which I currently lack the time. Someday, perhaps... (I did actually test the warping feature when evaluating 2.6.1 but there's a clear gain with Infinality)

inf_clnup.txt
inf_patch.txt

@beniked
Copy link

beniked commented Nov 24, 2015

updated openSUSE repos with above patch included, thank you.

http://download.opensuse.org/repositories/home:/nick31:/INFINALITY-ULTIMATE/

@mviikki16
Copy link
Author

You're welcome. There's still work to do to allow the user settings without having to recompile the code but even as such most users might be pleased with the updated font rendering.

@bohoomil
Copy link
Owner

Hello and thank you for the feedback. Unfortunately, the new patch has a rather devastating effect on font rendering as it removes important bits of the original code. Please, refer to issue #121 for details.

Of course, it'd be really great if the upstream found a way to integrate more pieces with the official code base.

@bohoomil bohoomil self-assigned this Nov 25, 2015
@mviikki16
Copy link
Author

Indeed, the patch removes all environment settings and built-in rules are redesigned. However, it shouldn't modify rendering if same settings are applied in the "burned in" database (my system renders the fonts looking same as before but does it much faster).

I wasn't expecting such quick integration into a public release, would be happy to update the code if a clear error is seen although for issue #121 I don't use the programs mentioned while for chrome I'm quite happy with the bold font rendering (attached image).

chrome

@bohoomil
Copy link
Owner

Actually, the original built-in rules are crucial for fonts to look as they IMO should...

@mviikki16
Copy link
Author

Built-in rules were redesigned in implementation while keeping their content same as before (as far as no other coding error is there...) However, environment variables are no longer read and the original "PUSH" Infinality preset is used (unless other changes were done to the openSUSE integration). If you take a look over the changes you can see what I mean...

@bohoomil
Copy link
Owner

Unfortunately, I'm not a programmer so please excuse my rather lame remarks: I truly appreciate all efforts put into development of freetype2.

I'm now studying the structure of the new patch and once feel comfortable with it, I'll try and recreate the original ultimate setting in it. If I manage to built a testing package successfully, I'll drop my code here.

@mviikki16
Copy link
Author

I do understand, if you have a source package I'll check it up, anything I can do to help... (I am a programmer but I also do mistakes and this patch can definitely have some).

To add customized settings please check the file src/base/ftinf.h. All the previous environment settings are now elements in structure ftinf_t (the names are a prefix removed lower case version of the original environment variables). ftinf_style is an enum with the few presets I included. To include the new preset would mean adding a INF_ULTIMATE name for it in the enum and a new element (at the proper position) in the vector of settings in src/base/ftinf.c
static const ftinf_t _inf[INF_LAST]=...

as the other examples included. To activate it a change must be done here

const ftinf_t *ftinf=_inf+INF_PUSH;

replaced with

const ftinf_t *ftinf=_inf+INF_ULTIMATE;

as the original line simply selects the PUSH preset at all times.

(an update to the patch would be needed to allow a flexible selection from all the available variants)

@bohoomil
Copy link
Owner

Thanks a lot, this is what I've figured out so far. BTW, deprecation of run-time settings in its current form is actually a good step. Still, it'd be nice if we had a similar functionality for those who use their own rendering styles. It's just a suggestion to think about in future releases though.

@mviikki16
Copy link
Author

I agree, the subject of flexible settings was discussed on freetype-devel forum and one reason I didn't rush into providing one was to avoid coding into such early patch something that does not fit well within upstream's view of the configuration API. However, it's not a difficult job reading the original env variables in just one go at library startup. I can add that code and it's only a matter of one function although would be nice to have a better way than uploading text files when managing such changes.

@goddesse
Copy link
Contributor

You can use git to clone freetype2's repo, create a separate branch based on master, apply bohoomil's infinality patch to that, then apply your own patches on top of that. You can use a combination of git diff branch1 branch2 or git format-patch to then get a suitable patch to make a pull request from in github.

@goddesse
Copy link
Contributor

Nicholas Waxweiler maintains his own git repo with his darkening patches. You might follow some of his threads on the mailing list to see if his way of using integration branches is helpful to you.

@mviikki16
Copy link
Author

Thanks, I'm already doing that (git clone and all...). What I meant is if I'll keep updating the patches I'll have to always upload a full version of the changes compared with master (or base Infinality) which is doable but requires more work from everybody to keep in sync... on 2nd thought, perhaps no easier way really exists...

@goddesse
Copy link
Contributor

What I mean is there should be nothing stopping you or bohoomil from making a separate repo that's the full freetype2 tree with your changes representing the master branch and people can push their code changes to that instead. Easy patches meant for package providers can still be made from that and uploaded to this repo with no change in process for people that just want to use the patches. Hopefully I've explained better.

@mviikki16
Copy link
Author

Yeah, maybe adding these as a new branch here eg under freetype/freetype2 directory with only HEAD of freetype2 master for a start would allow for simpler sync until the code settles to a stable form. After that the branch can be deleted and only the patch itself kept further...

@mviikki16
Copy link
Author

Here's an update against freetype master with environment variables reading and custom setups. There's also an empty place for the 'ultimate' setting addition. Parameter verifications were also moved into a one-time called function for reading the user settings. Hopefully, this will make your testing easier...

inf_patch.txt

(update, also added what I assume to be the 'ultimate' style (nr. 3).
inf_ultimate.txt

@bohoomil
Copy link
Owner

Thank you very much. I've just built and installed a testing package and for the very first time I'm running freetype2-infinality with built-in ultimate settings only. So far it feels like you've done an impressive job. Wow. :-)

Let me ask you about two things:

  1. How does reading environment variables is supposed to be done properly now? Should I put the old style infinality-settings.sh in /etc/X11/xinit/xinitrc.d and once it's there, the library will drop internal settings and process the external ones?
  2. Originally, infinality-settings.sh also provided variables for Xft. Should we still use a stripped-down version of the file for use with xrdb only? I believe the answer is positive but just want to confirm whether my reasoning is correct.

@mviikki16
Copy link
Author

You're welcome. All I've done is change the way settings are processed, the real magic is the original freetype and Infinality updates.

As always, the code itself is the best documentation, please check the function ftinf_env in src/base/ftinf.c

If you update the internal data you can pick a setting by name using the evironment variable "INFINALITY_FT", e.g. if you set it to "ultimate" then style nr. 3 is activated without any further modifications. This should keep environment "pollution" to a minimum. It's very simple to add all 5 of the ultimate styles and name them as "ultimate1", "ultimate2", ... etc. Another option to get another style without further modifications to the code is setting "INFINALITY_FT" to ultimate and then "INFINALITY_FT_FILTER_PARAMS" to the corresponding style case as the "INFINALITY_FT" has the same effect as setting defaults which are then updated with the other specified parameters. It's also possible to set everything just as before without any global "INFINALITY_FT" setting. When nothing is defined the "push" presets are taken.

I hope you now have answers to your questions but to address them directly:

  1. You can use the old settings unchanged or you can also add them with named styles inside the code to have less environment variables defined. Please note that the value for the variables which are not defined are taken from the "push" internal scheme or the one defined with "INFINALITY_FT" if valid.
  2. Would be nice not mixing xrdb with environment settings but I don't know of a way to safely handle these on all platforms. In my case I do use dedicated xrdb files (not only for Xft but other X programs as well) and these are manually loaded when the X server starts ( ~/.xinitrc does it). Speaking of this, I do have Xft.dpi set to 101 to match the actual monitor, perhaps other users should be aware of this, especially if having high dpi hardware.

@bohoomil
Copy link
Owner

Thank you very much. I generated a new infinality patch set with all ultimate settings and style 3 hard coded as the default one. Switching styles via environmental variables works like a charm, too, so now I'm going to prepare a public release.

@mviikki16
Copy link
Author

Great! Still lots to do to have suitable code for upstream integration but one step at a time...

@mviikki16
Copy link
Author

If you used my version you might have rushed a bit with the update since bohoomil keeps the official patch, the one above is missing 4 of the 5 ultimate styles...

I have now attached one which is updated with bohoomil's styles and fixes the default to be ULTIMATE3 when no environment variables are set (if you compare the patches against each other you'll quickly see the changes). This can be directly used by bohoomil as the official release but I leave it up to him to double check and update.

inf_patch2.txt

@bohoomil
Copy link
Owner

@mviikki16 Thank you very much. Now I can see what I misunderstood about the newer version of the patch. The update is on the way.

@mviikki16
Copy link
Author

No misunderstanding, your version was also working fine without any evironment variables (to me it looked more clear having all settings at the same place this is why the slight change for the _env update).

@bohoomil
Copy link
Owner

For some reason now building fails with the following error:

/package_x86_64/freetype-2.6.2/src/base/ftbase.c: In function ‘ftinf_env’:
/package_x86_64/freetype-2.6.2/src/base/ftbase.c:38:0: error: expected declaration or statement at end of input
 #endif
 ^
/package_x86_64/freetype-2.6.2/src/base/rules.mk:97: recipe for target '/package_x86_64/freetype-2.6.2/objs/ftbase.lo' failed
make: *** [/package_x86_64/freetype-2.6.2/objs/ftbase.lo] Error 1

What can it be?

@mviikki16
Copy link
Author

It builds fine for me... might be the reference used, the patch is against current HEAD of master
f8c20577897d17fb8a62e64fa3bf3ebec0691608

@bohoomil
Copy link
Owner

Yes, that was exactly it. Sorry for the mess.

@mviikki16
Copy link
Author

No problem, I should have mentioned the exact reference before, good to have this cleared.

@bohoomil
Copy link
Owner

There's one more little thing I don't really understand what to do with: when I select 'osx' in the 'infinality-settings.sh' the rendering style used is actually 'windowsxp' and vice versa: selecting 'windowsxp' activates visually 'osx'. What can I do to fix it? (The problem was also reported by other users.)

@goddesse
Copy link
Contributor

It's the order they're defined in the large array ftinf_t _inf[INF_LAST]. They're swapped from what the enum says they are.

@bohoomil
Copy link
Owner

OK, thank you for the hint. I'll edit the patch and test it again.

@mviikki16
Copy link
Author

Indeed, that's an error with enum values ordering

inf_fix.txt

@bohoomil
Copy link
Owner

Yes, now it seems that everything is working as it should. Some users reported that they see the difference between the previous and the new rendering in a couple of styles. Can it be the upstream's changes to the LCD filtering? I cannot confirm it myself though: the 'ultimate' styles look exactly the same IMO.

@mviikki16
Copy link
Author

I have only looked at the PUSH style that I'm using and that was also identical (to my eyes) to how it looked before. There were also upstream changes (e.g. "Switch off stem darkening by default.") which complicate the picture...

If a user can provide a clear case (i.e. a font and size and settings used) would help to spot the difference in implementation. Otherwise, the only way to do a proper comparison is rendering various fonts to bitmaps and doing binary diffs of these when old-patch and new version are used on the same code base with as many of the presets and fonts and sizes as possible... Unfortunately, I lack the time and interest for such work but perhaps I will provide a patch to add internally also the older styles from

fontconfig-ultimate/freetype/generic_settings/infinality-settings.sh

as it might be easier for users to tweak an ideal setting starting from a relatively close looking, built-in one.

@bohoomil
Copy link
Owner

Thank you again for your assistance. It's been extremely helpful. Hats off to you!

@goddesse
Copy link
Contributor

If people are complaining about the darkness or lightness of the fonts and they relied on the default value infinality set, that definitely would've changed recently. Please see: http://www.freetype.org/freetype2/docs/reference/ft2-lcd_filtering.html

The first filter is what's internally set by default (for filter lcddefault) in the older infinality patches and that might be what some users who weren't using the ultimate presets are expecting.

@bohoomil
Copy link
Owner

Thanks a lot. I reposted your response on the Arch's Forums in case people won't read it here.

@ismail
Copy link

ismail commented Nov 30, 2015

Thanks for working on this! Greatly appreciated!

@mviikki16
Copy link
Author

@bohoomil Thanks, you're too kind, I'm also benefiting from the updates :)

I have a new version, unfortunately lots of changes to properly include all the former Infinality styles. There was also, I suspect, a memory leak fixed in src/smooth/ftsmooth.c.

All settings and rules are now in one place, no enums or indexing required hence no potential errors with misplaced array elements.

The patch is attached (against current HEAD of master f8c20577897d17fb8a62e64fa3bf3ebec0691608) but to regenerate in the future the gperf hashes gperf has to be patched as well (base version 3.0.4) and a patch for it is also included.

inf_patch_all.txt
gperf.txt

@bohoomil
Copy link
Owner

bohoomil commented Dec 1, 2015

Thank you. I'll jump into testing as soon as possible. (Is the patched gperf needed for building only or is it required by the package to work properly as well?)

@mviikki16
Copy link
Author

The patch for gperf is only needed if rules or settings are to be added/removed/renamed in the future and it is not needed for this build as the generated files are included and definitely not needed for the package to work as well (freetype+infinality usage has no need of gperf).

@madig
Copy link

madig commented Dec 8, 2015

Hi guys! I'm interested in integrating Infinality's autohinter improvements and improving the v38 TT interpreter and was directed here. I skimmed the rest of the FT patch in fontconfig-ultimate and found mostly post-processing of Bitmaps which I'm less interested in, I think a few things like gamma correction are misguided hacks. Real rendering quality will only be achieved through linear alpha blending and gamma correction by the rendering system, which sadly requires quite a bit more work on the FT side and more importantly on the rendering system (cairo) side.

I applied the autohinting changes and removed the #ifdefs but saw no change in rendering.. will have a closer look in the next days hopefully. Anybody have a screenshot comparing vanilla and autohinter-improvement-only rendering?

@madig
Copy link

madig commented Dec 8, 2015

By the way mviikki16, I personally used a "dirty" development branch of freetype master, constantly rebasing against master and rewriting history (my patches) to publish a patch series. Maybe there are easier ways of maintaining patch series. For local development it's good. If you want to publish one big patch, you can simply do git diff master my-dirty-branch

http://code.tutsplus.com/tutorials/rewriting-history-with-git-rebase--cms-23191

@mviikki16
Copy link
Author

@madig Thanks for the info, that's the the easiest way and what I'm also doing. Earlier I mostly wanted to find a better way to maintain the changes and not have to upload all of the diffs with master but maybe just keep updating a dedicated github branch...

Anyway, now that the changes are fully integrated I can send any further updates against the infinality branch itself which are way smaller and make the actual change stand out a lot better.

@madig
Copy link

madig commented Dec 10, 2015

So I played around with the autohinter changes (03-infinality-2.6.2-2015.12.05.patch -> changes to aflatin.*) some more and see no difference at all to vanilla, neither in Y-only nor in X-and-Y mode. What am I missing?

Oh, and https://github.com/bohoomil/fontconfig-ultimate/blob/master/freetype/03-infinality-2.6.2-2015.12.05.patch#L351 and the following lines are a no-op?

@madig
Copy link

madig commented Dec 11, 2015

I figured it out, accidentally set something zero. I find the results somewhat underwhelming, I only noticed significant differences in a few fonts at specific sizes (Ubuntu-R > 14 ppem or something, some smaller sizes of Liberation Sans, Roboto Thin something). Roboto Thin had vanishing horizontal stems with strength set to 25. Not sure I understand the exact purpose of this part of Infinality, but my impression is that stem darkening is the better way to make stuff readable.

@goddesse
Copy link
Contributor

I was under the impression that the highest impact changes Infinality had made to the true type interpreter and autohinter for Cleartype are already merged upstream as part of the subpixel patch around 2013. The things that are left are (by his own comments) alpha quality stuff he was still working on making work universally well. Most of the visual changes are done by the bitmap post-processing code you aren't interested in.

Judging from the images posted, stem darkening + alpha blending and proper gamma do look great, but I don't believe Freetype has any info about the surrounding glyph's background colors so that would have to be done in something like Cairo or Skia?

@madig
Copy link

madig commented Dec 11, 2015

Ah, I made an oops. Strength should be set to 100. Okay, it does make a difference, but results depend heavily of the font.

@goddesse Hm, okay, that could be. I was told that he lost interest in integrating his work further, so I was looking for things to salvage. What autohinter for ClearType do you mean? I only know of his work on the TrueType interpreter. Yes, LAB+GC must be done outside FreeType by cairo and friends, only stem darkening can be done within. My interest there is to provide an API and change all font drivers to support stem darkening.

@goddesse
Copy link
Contributor

There's not a special, separate autohinter, but I mean I think he was working on custom rules for how Cleartype fonts should be hinted to work around not having diagonal hints or something to that effect. I think changing a glyph's x-height is already a library property upstream, but I'm not sure what other changes are of interest. There's not emboldening in the x-direction that might be a useful property to control.

@madig
Copy link

madig commented Jul 7, 2016

Werner Lemberg is thinking about adding a mechanism to configure driver-level settings like the TrueType interpreter version through an environment variable. You may want to chime in on the discussion.

http://lists.nongnu.org/archive/html/freetype-devel/2016-07/msg00025.html

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

No branches or pull requests

6 participants