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

Font loading from directory keep system memory full #548

Closed
CastagnaIT opened this issue Sep 13, 2021 · 18 comments
Closed

Font loading from directory keep system memory full #548

CastagnaIT opened this issue Sep 13, 2021 · 18 comments

Comments

@CastagnaIT
Copy link

CastagnaIT commented Sep 13, 2021

A Kodi user has reported a problem on libass that causes system crashes on devices with limited memory 1/2 gb.

if you have a folder with many fonts 100/500/1000 etc...
or also few fonts but with large size (in mb) like often happen with asian languages
libass load and keep all these fonts in memory without release it

I have also tested on Windows with your provided libass script
i have set ass_set_fonts_dir to a directory with 2000 fonts and
with the results in a ~800mb of ram occupied and never released

i think that if the load of all fonts is needed to get some type of metadata,
suggest to load the metadata and after release the font without kept it in memory
then if an ASS file need e.g. two fonts only then keep in memory only these two fonts
not whole 2000 fonts of the directory...

i would like to know if there is a possible alternative or are you already planning changes

Sidenote:
loading so many font files is slow also on fast systems should be improved somehow

@rcombs
Copy link
Member

rcombs commented Sep 13, 2021

ass_set_fonts_dir is not intended for use with large font dirs; as you mentioned, in addition to occupying large amounts of RAM, it's also slow. Use font providers instead (DirectWrite on Windows, Fontconfig on Linux, CoreText on Mac).

That said, ass_set_fonts_dir really should switch over to opening fonts on-disk rather than loading them into memory in their entirety (that won't completely solve this, though, and it'd still likely be pretty slow on large dirs).

@CastagnaIT
Copy link
Author

Use font providers instead (DirectWrite on Windows, Fontconfig on Linux, CoreText on Mac).

this gives me a little more light

i have recently fixed the kodi libass sources which where we had added a custom implementation to load fonts from directory with DirectWrite (by using same ass_set_fonts_dir) and it is very fast

then i think we should do something similar also with Fontconfig and CoreText

@TheOneric
Copy link
Member

This was already discussed a while back: #510 (incidentally also related to and communicated to Kodi, but I don't expect anyone to know about all Kodi's tenthousands of issues).
The conclusion then was, that we don't want to make ass_set_fonts_dir any more complex than loading a bunch of memory fonts, as that would just be a reimplementation of our Fontconfig fontprovider (or we'd need to start to require Fontconfig). The recommended way to deal with large font collections is to either install them such that the default system fontprovider can pick them up, or if it is required to keep some fonts out of the regular system/user fonts, you can request Fontconfig specifically (it's cross-platform) and pass a custom Fontconfig config to ass_set_fonts. (read linked issues and Fontconfig's documentation for more info)

From your comment I infer that it is possible to add additional directories or additional single files to DirectWrite and Kodi is currently doing this via custom libass patches? If so and this isn't too complex and possible for all our DirectWrite-variants and also CoreText, then maayybe we could extend the currently Fontconfig specific argument of ass_set_fonts to allows to add additional font dirs to DirectWrite and CoreText.

It would probably be good to make it more clear in our documentation that ass_set_fonts_dir loads everything as memory fonts.

@CastagnaIT
Copy link
Author

thanks for the meaning
i have found the mentioned PR's too late to understand better the situation

however, at least with DirectWrite it is possible to load fonts from a custom folder
on Kodi it was already implemented but had some bugs that caused crashes...
that i have recently fixed (not yet added in kodi), you can see the patches here (see all latest commits):
https://github.com/CastagnaIT/libass/commits/font_load

I have test with more than 2000 fonts in a kodi user fonts folder,
does not seem to affect system memory when load the fonts
the first time is a bit slow to load 2000 fonts kodi player stuck for ~6/10 seconds
after the first time, it seems the fonts are loaded in a more fast way ~2/3 secs

this seems like a improvement over the original loading of "ass_set_fonts_dir"
at least without the devil of memory allocation

perhaps other providers could provide similar functionality to DirectWrite to use custom font collections
but i'm not very well documented on this...

sidenote,
i have found a problem on libass
in ass_face_open method, on Windows case only (but not sure), FT_New_Face fails every time when the fonts have unicode chars in paths
(like "華康仿宋體-W2.ttf"), i do not know if this affect also other OS.
I have found this solution, see changes here:
https://github.com/CastagnaIT/libass/blob/a94ce5c2b8a4e7732282c2cecd722d2b25c2e576/libass/ass_font.c#L135-L185
if you would like to take a look maybe could be fixed on your libass

@TheOneric
Copy link
Member

@CastagnaIT : ass_face_open is never supposed to be called with DirectWrite as font provider in libass, this is likely due to Kodi-patches. (And Fontconfig afaik won't index non-fopen-able files; if on recent enough WIndows 10, set encoding in the app manifest to UTF-8 to circumvent this).
Your patches are intermingled with a lot of space and indent changes so it's hard to tell where the "adding a directory to DirectWrtie"-bit lives, similarly the entirety of Kodi-patches has both a modified ass_directwrite.c and a custom ass_directwrite.cpp and I can't tell where this bit is. Can you point out how and where specifically a directory get's added to DirectWrite for font-lookup?

I do not intend to fully review your patches, but I noticed that this bit: CastagnaIT@146f851, is likely wrong: it treats the font-dir differntly based on whether the build suppots DIrectWrite, but one may at runtime choose to use Fontconfig.

@CastagnaIT
Copy link
Author

CastagnaIT commented Sep 15, 2021

you are looking in paxxi repo
his repo is a bit a mess imo need to be cleaned/rebased

some comment above i have linked my repo where i have squashed
all kodi commits in one (commit name "Kodi changes")
in addition in my branch there are some commits with fixes that i have done to improve the curren broken situation

the ass_directwrite.cpp is not used anymore, and not exists in my branch (on kodi branch is also not used)

I do not intend to fully review your patches, but I noticed that this bit: CastagnaIT/libass@146f851, is likely wrong: it treats the font-dir differntly based on whether the build suppots DIrectWrite, but one may at runtime choose to use Fontconfig.

you have asked to provide some information about our directwrite implementations
but i am not the author of these changes
I studied a bit to try fix and improve the current situation stopped on kodi libass for windows

yes i will have to change something here, exclude it when a font provider is used, this to fix also Kodi OS's like LibreElec that currenlty they also have this problem. (I'm realizing these inherent bugs in these last days)

I was not knowledgeable enough to know the name of the other providers (discovered yesterday) and how works all whole things in libass, i am beginning to understand in these days a little more...

hard to go into details, I'm not knowledgeable enough about how direct write works,
but i can say this:

ref: https://github.com/CastagnaIT/libass/blob/font_load/libass/ass_directwrite.c#L1477-L1501

when we set ass_set_fonts_dir the path is also set to priv->dirPath line 1477
init_LocalFontLoader is an interface used to browse the dirPath
IDWriteFactory_GetSystemFontCollection is to get a references to the system fonts collection
IDWriteFactory_CreateCustomFontCollection is to create references to fonts in a custom directory, this method call automatically in the LocalFontLoader (initialized by init_LocalFontLoader) the LocalFontLoader_CreateEnumeratorFromKey method where pass the custom directory path to watch by using init_LocalFontEnumerator interface, this interface can browse files and create references to the fonts,
all these interface should be executed from "scan_fonts" method calls (line 1500)

@CastagnaIT
Copy link
Author

intermingled with a lot of space and indent changes

would be simpler modify libass sources if you add the support to clang-format like done on kodi sources
this can automatically format the source code while edit it
it is very useful

@CastagnaIT
Copy link
Author

i have reworked paxxi branch, you can see commits here:
libass:master...Paxxi:kodi-15.2

there is not much difference but a little more clean

until further developments are made on this issue,
can you at least limit the amount of files that load_fonts_from_dir can load into memory?
https://github.com/libass/libass/blob/master/libass/ass_fontselect.c#L169-L204
because this is the main problem that cause system crashes, because users place many fonts in folder and this method has no limits fill whole RAM by killing the system
i can propose a PR if this can be considered

@rcombs
Copy link
Member

rcombs commented Sep 21, 2021

As discussed, there's a clear solution to your problem: don't use ass_set_fonts_dir for directories with large numbers of fonts. This is an issue of the API being used incorrectly, and there are already better solutions to the problem you're using ass_set_fonts_dir to solve.

@CastagnaIT
Copy link
Author

there are already better solutions to the problem

i am not able to find a good solution for each type of OS, platforms and Kodi OS like LibreElec etc..

we need to say to libass to reads fonts from a specific user folder,
from what i understand there is only two ways the font provider or the load_fonts_from_dir when call ass_set_fonts_dir

we can provide a patched libass only for Windows OS where we exclude load_fonts_from_dir and we load fonts from the user folder directly with DirectWrite.

For all other OS(linux distro, mac, raspbian, etc) and Kodi OS(LibreElec etc..) there are different situations,
how would you tell libass to read fonts from a user folder?

@rcombs
Copy link
Member

rcombs commented Sep 21, 2021

On any platform, you can ship fontconfig with a config file pointing at the directory of your choice.

In actual usage, the usual solution is to just tell users to install fonts normally (so in ~/Library/Fonts on macOS, etc).

@CastagnaIT
Copy link
Author

the installation of fonts directly into the system is known,
but what Kodi authors would like to keep Fonts in a user folder to avoid installing fonts in the operating system

Can you link me a page with instructions for how to do add a folder in fontconfig e.g. for Ubuntu?
Do you think that a non-expert user can do fontconfig changes?

@CastagnaIT
Copy link
Author

another question with android case and IOS how can works?

@TheOneric
Copy link
Member

TheOneric commented Sep 22, 2021 via email

@CastagnaIT
Copy link
Author

i have tried MPV as suggested
i have tried to put this file
fonts.conf.txt
in AppData\Roaming\mpv\ and also in mpv/portable_config/
as specified in mpv docs
but not load any font from the specified folder
there is no others info on mpv documentation, do you have tried? am i missing something?

@CastagnaIT
Copy link
Author

i have discuss with kodi dev,
due to also the additional problem of injecting fonts when they are embedded in video files like MKV,
we will continue to maintain the use of ass_set_fonts_dir although its limits, we will warn users of the possible solutions also proposed from you here in the wiki
thanks for the help given

@TheOneric
Copy link
Member

TheOneric commented Sep 22, 2021 via email

@astiob
Copy link
Member

astiob commented Sep 22, 2021

due to also the additional problem of injecting fonts when they are embedded in video files like MKV,

MKV attachment fonts are normally added to libass using ass_add_font, which is unrelated to system fonts.

I can understand not wanting Fontconfig on macOS: after all, libass introduced a dedicated Core Text font provider not for nothing. But on Linux, for huge font directories, you should certainly prefer a custom fonts.conf that adds the directory to Fontconfig over ass_set_fonts_dir.

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

No branches or pull requests

4 participants