Windows: support WinRT/UWP; unregress match_fonts using GDI’s EnumFontFamilies or IDWriteFontSet
#523
Conversation
|
Just some first remarks; a ""review"" (I don't know anything about DirectWritee or GDI, so it will be limited) coming later, possibly not today. I'm pretty sure the newly added
I guess the best bet is someone from VLC and there's probably no harm in asking. @ePirat: If you happen to have time and it doesn't bother you: Does VLC have a WInRT/UWP CI test for VLC with libass, or something similar which could be run on this patchset? (But not right now, since there still seem to be some other issues)
Not all versions work everywhere iirc, right? Do they always compile but not run? ART's basic crash tests (using the default font provider) failed on ea197b3, unfortunately without any error message; see https://github.com/TheOneric/libass-regression-tests-tmp/runs/2858753913?check_suite_focus=true#step:7:19 |
Fixed, thanks.
Fixed, thanks! And fixed a couple more bugs:
I can confirm I can now run libass’s test utility on a random testing ASS file successfully on Windows 7.
There are three paths in total:
Currently, this PR compiles either only the GDI path or both of the other two paths, but never all three. But we could compile all three on desktop, and we could run all three on Windows 10. |
There was a problem hiding this comment.
I don't know anything about DirectWrite, GDI or Core Text, so this is not a review of the actual Microsoft- and Apple-API usage. But fwiw, besides some questions, I didn't see anything problematic in the remaining parts.
Another general remark/question: With this there now are 3 ""DirectWrite"" font providers, but if I didn't miss something, it appears to be hard to tell from logs which one is actually used. Would it make sense to split them up as described in my previous post or otherwise add indications of which one's being used to the logs?
That’s what I wanted to do, too. The reason I didn’t is that they won’t be user-selectable until mpv and other clients update their own provider lists. But perhaps the improved logging is compelling enough reason to split the providers in its own right. On that note, we have
I don’t think we need to deprecate To be clear, are you suggesting that we end up with three possible choices (one per variant) or with four (one per variant plus one automagical)? We already have |
Currently at least mpv only makes fontconfig,
fair point.
I was originally thinking of 4 choices, to be most compatible with existing explicit |
Oh. Well, on the bright side, that makes it even less likely that anyone is currently explicitly requesting I imagine (without checking) that mpv may be the only consumer that exposes this setting at all.
Ignoring compatibility with existing users who explicitly select
|
| @@ -413,12 +448,17 @@ static void destroy_font(void *data) | |||
| { | |||
| FontPrivate *priv = (FontPrivate *) data; | |||
|
|
|||
| IDWriteFont_Release(priv->font); | |||
| if (priv->font != NULL) | |||
There was a problem hiding this comment.
I think simple if (priv->font) would be better.
There was a problem hiding this comment.
It would be, I agree, but only if the lines immediately below followed that style.
| #if DESKTOP_API | ||
| font_priv->shared_hdc = hdc_retain(shared_hdc); | ||
| #endif | ||
|
|
||
| ass_font_provider_add_font(provider, &meta, NULL, 0, font_priv); | ||
|
|
||
| cleanup: | ||
| if (meta.families) { |
There was a problem hiding this comment.
I've seen such cleanup code in several places. Maybe it should be factored out too?
There was a problem hiding this comment.
It’s slightly different in each place, which makes it annoying to unify. Also, this very PR actually removes it from ass_coretext.
I think I’ll try to make a separate PR that will remove redundant memory allocations in ass_font_provider_add_font and hopefully get rid of this duplicated cleanup code altogether.
astiob
force-pushed
the
fonts-windows
branch
2 times, most recently
from
6a351c3
to
384da5e
Compare
|
Pushed new version. Besides addressing review comments, I’ve also:
Comments not acted upon:
New worries:
|
|
it seems select_font / find_font will be rewrited, should I close #511? |
I’ve left a comment there: However, this PR here doesn’t actually change what #511 tries to change/fix, so you may also wish to keep it open for now. I do want to implement a better fix for #509 and related issues, but it’ll come later, in a separate PR. |
There was a problem hiding this comment.
As before, I'm not familiar with Apple- and MS-APIs, so I can't say anything about their usage.
Split into multiple font providers: do we want this in the end?
The main benefits I see of a split would be better logging (which could also be achieved in another way) and maybe debugging. But you know better than I about potential other drawbacks and if it's ultimately worth it, so I'll leave this decision to you.
I had configure.ac use
IF_PREPROC_IFELSEin an attempt to make configure faster, but this appears to have introduced an extra “checking how to run the C preprocessor…”
Without having tested it, I'm guessing the time difference for only 3 checks is very small either way. So unless measurements suggest otherwise, from a time-standpoint I'm indifferent to this.
Not sure if relevant for the cases at hand, but it might be worth mentioning that autoconf's default header check originally (long ago) used only the preprocessor, but switched to a compilation to be able to accurately recognise conflicting headers.
The “WinRT app” CI build includes dependencies such as Fontconfig that probably can’t exist on WinRT.
On UWP normal char-APIs seem to exist, not sure about RT. Is there any other reason why Fontconfig wouldn't work on UWP or RT?
Oh. Interesting: I’ve just noticed my Autoconf (which I’ve been building the recent releases with) has a preprocessor check regardless of this PR and uses the preprocessor to search for standard headers as a single action, and indeed it is older (2.69) than the docs in your link. I’ll make sure to update before the next libass release. And I’ll switch back to compile checks for DirectWrite, then.
Hmm, I was mainly thinking of file system access restrictions. But I suppose it technically can still work, just index only the app’s local files instead of system fonts. (Unless they use [other] forbidden APIs, but I don’t know about that.) |
Judging by autoconf 2.60 docs (release tagged 2006-06-23) the change to compile test should predate 2.60 (and thus 2.69). Checking |
|
I’m not sure where it comes from, but this is what I see in the This is way before our Compare this to the CI builds, which don’t have the preprocessor check and have multiple individual-header checks intead of one for “ANSI C header files”. Not that any of this is important. :-) |
Normally, we delay loading Dwrite.dll until runtime to allow building and running DirectWrite-enabled libass binaries on old Windows versions that lack DirectWrite. However, this is forbidden in WinRT/UWP. DirectWrite is present in all versions of Windows that support WinRT/UWP, so we lose nothing by requiring it. Older Windows SDKs (Microsoft and MinGW alike) lack <winapifamily.h>, so include it only if we really need it. Based on VLC patch for libass: videolan/vlc@eedb57a and on this autoconf code: lu-zero/mfx_dispatch@c51a54c Note: the VLC patch retained an unconditional call to FreeLibrary in destroy_provider. However, FreeLibrary does not actually expect NULL as argument, so this commit adds a guard to that call. Perhaps FreeLibrary(NULL) simply fails cleanly and that's why this has not caused VLC problems, but we should not rely on this.
This allowed writes to one past the end of `families` and `fullnames`.
...instead of passing them as arguments everywhere.
This will be needed for the next commit.
There's no point. We originally had extra code here that adjusted linker flags before the test, but that's long gone.
|
Pushed:
Will investigate after sleep:
|
|
Pushed CI change: assuming I transferred the code correctly to the CI config file, it no longer links to forbidden libraries and instead links to libraries for UWP apps*. Based on VLC build code. And this didn’t even break the build, yay! * Only for libass itself. The MSYS2-provided dependencies are, of course, not rebuilt, so they keep using Win32 DLLs. |
CreateFontFromLOGFONT does not actually use GDI's font lookup logic and fails to emulate it faithfully. In particular: it fails to find CFF-outline fonts by PostScript name; it fails to find TrueType fonts by full name on older versions of Windows; it fails to find at least some fonts installed on demand by font managers. When GDI is available, invoke GDI directly. This commit uses EnumFontFamilies, which is almost perfect, except that if the user has two different fonts such that one font's family name equals another's full/PostScript name, this will find only the family name match. To fix this case as well, we'd need to invoke CreateFontIndirect separately for each font request, complete with its weight and slant. This requires larger changes in our fontselect, which this commit does not attempt yet. GDI is not available in WinRT/UWP. On UWP (Windows 10), use the new IDWriteFontSet API to emulate GDI's font lookup as well as we can. In WinRT (Windows 8), we have no choice but to keep using CreateFontFromLOGFONT (unless we go back to loading all fonts eagerly, which we stopped doing for performance reasons). It is the builder's responsibility to avoid linking in Gdi32.lib in WinRT/UWP builds, just as it is to link in Dwrite.lib.
We use %zu format specifiers to log size_t values. However, __attribute__((format(printf, ...))) in modern GCC means "system's default" printf, which on MinGW means MSVCRT.DLL printf, which does not support the z length modifier (even on Windows 10), so GCC warns that our format strings are invalid. Older GCC is unable to check Microsoft's format strings at all and has only format(printf), which is equivalent to the newer gnu_printf. Clang also lacks gnu_printf, but it is covered by the same macro check because its __GNUC...__ macros always report version 4.2.1. On non-Microsoft platforms, printf and gnu_printf are currently aliases, so the __MINGW32__ check is redundant. However, with some luck, GCC may start to check other platforms' printf formats more carefully in the future; and we would like to receive warnings if our format strings don't work on some platform, although we intend to stick to standard C99 format strings. Indeed, if we use an extension by accident, this might help us catch it. And even if we make no mistake but there is another platform that fails to support C99 format strings, this might warn some poor soul building on that platform that their system printf doesn't understand our log format strings, so they will know they need to work around it in their log callback or to patch libass.
This avoids an extra trip to FreeType.
Reuse the desktop build for this, as UCRT32 doesn't exist.
|
Pushed a fix for the Windows 10/UWP path, where yet another interface had incorrect members declared in |
|
Gonna merge this in a few minutes unless someone jumps in with a last-minute objection. |
I swear I tried to make this as small as I could.
TL;DR at least review the small commits pls and let’s release this in 0.15.2
Many of the commits are tiny fixes, so it’s not as daunting as it seems. Please review commit by commit. The commits are ordered mostly by authoring time, but if necessary, I can reorder them to put all the tiny fixes up front.
This finally brings Windows font matching back in line with expectations after we stopped eagerly loading all fonts in 0.15.1. And this should finally support font managers with on-demand font activation, which should help fansubbers (cf. #502) (
as much as I still don’t really understand why or how such activation works or helps).This implements the approach outlined in #503 (comment) (but prioritizes GDI over
IDWriteFontSetto maximize font manager compatibility).Known remaining issues:
If, somehow, the user has two fonts A and B installed such that A’s family name equals B’s full (but not family) name, the GDI code finds only A when searching for this name.
I already have plans for fixing this, but they will require a more significant rework of font matching in all font providers (which will be useful in its own right, too, e. g. for xy-SubFilter finds non-attached regular font-variant, libass the attached bold variant #509). I thought it best to keep it out of scope of 0.15.2 and make this PR more contained, so for now I implemented an approach with
EnumFontFamiliesthat affects other providers’ code minimally and behaviour not at all.But this means that my planned future change will remove the
EnumFontFamiliescode and therefore also theSharedHDCcode, so this code is somewhat temporary. I’m not too happy about adding temporary code, but I think this is best in the current situation: I want to get 0.15.2 out as soon as possible withdirectwritefixed and little chance for new bugs.The GDI code can find a TrueType font by only one of its full names and only two of its family names, chosen depending on the user’s locale. This matches VSFilter, of course, but doesn’t match some of our other logic, notably the logic for embedded fonts.
If we think this is a problem, we could use both GDI and DirectWrite on desktop and add fonts received from both. But that’s as far as we can go. Alternatively, if it should turn out that
IDWriteFontSetworks perfectly with font managers nowadays, we could prioritize that path and only fall back on GDI when it’s unavailable.I’ve tested the involved primitives on up-to-date Windows 7 and Windows 10, but I’d be very curious to test
CreateFontFaceFromHdcon Windows Vista. I see it uses private APIs in Gdi32.dll on 7, but a random website suggests these APIs may not have existed in Vista. If true, that might mean it’s pretty bad on Vista.If the GDI code adds a font but FreeType cannot read it, an error will be logged saying just “Error opening memory font” without naming the font at all. This may not be very useful.
There’s quite a bit of ifdeffery in
ass_directwrite.c, and it has large chunks of code that are never compiled together. This one file now supports two distinct Windows-based platforms. We should modify our CI to do two Windows builds to cover all code.I don’t know how/by whom the WinRT/UWP code can be tested.
Toss-up choices I made:
I wanted to have all
ass_directwrite.ccode compiled on desktop and be presented as three distinct font providers, e. g.directwrite-gdi,directwriteanddirectwrite-basic. This would allow testing by switching them at runtime. Unfortunately, our font provider selector is not a string but an enum, so client code would need to be updated to allow this, making it less useful, so I didn’t take this route.I considered keeping
ass_get_font_infoseparate and having bothcoretextanddirectwritecall it beforeass_font_provider_add_font. But fordirectwrite, it needs to stream the font, and to stream the font, it needs itsFontPrivatepointer. Currently we neatly construct it and pass its ownership toass_font_provider_add_font, so I decided to keep it that way and embed theget_font_infocall insideass_font_provider_add_font. I think this also reduces code duplication. The cost is more complicated logic/API forass_font_provider_add_fontand the font-name-less error message fromass_face_stream.I considered using a separate DC for every font to keep them alive more granularly and simplify the code. But I’m concerned that the DC we did
EnumFontFamilieson likely already has the fonts in its cache or something and therefore provides both more reliable and more efficient results from the subsequentCreateFontIndirects, so I chose to share the one DC among all the discovered fonts and manage its life via ref counting.