From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#73730: 31.0.50; Support for color fonts on MS-Windows Date: Wed, 16 Oct 2024 14:01:36 +0300 Message-ID: <861q0gqpkf.fsf@gnu.org> References: <36a6b4d5-c719-44d6-957d-bcd7db5a854b@imayhem.com> <86o73s14x5.fsf@gnu.org> <0a0c622c-5b69-4f30-94b1-67a238e124b4@imayhem.com> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="27251"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 73730@debbugs.gnu.org To: Cecilio Pardo , Ken Brown Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Wed Oct 16 13:03:00 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1t11nw-0006wA-Dj for geb-bug-gnu-emacs@m.gmane-mx.org; Wed, 16 Oct 2024 13:03:00 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1t11ng-0005XW-4G; Wed, 16 Oct 2024 07:02:44 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1t11ne-0005X7-Ud for bug-gnu-emacs@gnu.org; Wed, 16 Oct 2024 07:02:43 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1t11ne-0008ED-F0 for bug-gnu-emacs@gnu.org; Wed, 16 Oct 2024 07:02:42 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=debbugs.gnu.org; s=debbugs-gnu-org; h=References:In-Reply-To:From:Date:To:Subject; bh=iEvw1YG9B4pjFZiF+UTMK5io09AbC8WM7AXzYH5CaRs=; b=tJbolRspTRQoAf2T5RUAMKSycvrJt8xorGTHiq9GFi0xjNN+Hfl0Tit5viIpdBD23DiOiPl1l+avQIGi2OXRErA2OTEq6UnfWg7F4gqBM6qnHj5UqJg6dpwmdEgGyUPCrL3xYFlol7DA1M4c83F2d8HVUksDyWTURjXe2Fvx2TPbRIrScaQHSe+vlIDMeIZvaecJvsl5lDkHWarcjxge6r74sVTkNf8101GiMTeevHdz/js5ERB+Puizn/WYNz5j1LR6fzB7ZAlZldESaLgKTQUmTKl3OPCa+lbxWB0YKEQGxRu/M/ZmbM8KWyiv89yDJzcBq5F6biw4BMLEDYSlZA==; Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1t11nx-0002fr-Jg for bug-gnu-emacs@gnu.org; Wed, 16 Oct 2024 07:03:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Wed, 16 Oct 2024 11:03:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 73730 X-GNU-PR-Package: emacs Original-Received: via spool by 73730-submit@debbugs.gnu.org id=B73730.172907653110204 (code B ref 73730); Wed, 16 Oct 2024 11:03:01 +0000 Original-Received: (at 73730) by debbugs.gnu.org; 16 Oct 2024 11:02:11 +0000 Original-Received: from localhost ([127.0.0.1]:58730 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1t11n8-0002eV-Ua for submit@debbugs.gnu.org; Wed, 16 Oct 2024 07:02:11 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:60176) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1t11n6-0002e4-1A for 73730@debbugs.gnu.org; Wed, 16 Oct 2024 07:02:09 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1t11mf-0008AX-Mn; Wed, 16 Oct 2024 07:01:41 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=iEvw1YG9B4pjFZiF+UTMK5io09AbC8WM7AXzYH5CaRs=; b=B93XL711OvbM Qq6IgaXnCYZszwlOZLhP8yxIg+8vb4b+vSmh43NjudV6KD8CQNag1s6SuNkjWZHphqd6iH+fWsg2n UtV8xydaCOtTo5/ZgGhDtHCL+uVychfCgpII/VjesO0blgZOb+c1yClEMgFTtACzBqNt5mKhkwxsu FgZ9cIEOuqYpx3VPZjyEBxRVNV05FyxYDXd7twokCQgFcY4goJ4cDILCDe2gwLNqGDdAM7FHFvHoP 5tl2qRBj08CgkhmEuwc7qvmGIATr4ulZeyY0hdrE0jEJeQ0asUd+qfqvADEz6dlguYCMASnfmdGsa h7/oXHGTtvkyqMeZ/EGDmQ==; In-Reply-To: <0a0c622c-5b69-4f30-94b1-67a238e124b4@imayhem.com> (message from Cecilio Pardo on Wed, 16 Oct 2024 00:18:20 +0200) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:293670 Archived-At: > Date: Wed, 16 Oct 2024 00:18:20 +0200 > Cc: 73730@debbugs.gnu.org > From: Cecilio Pardo > > The attached patch works without a new font driver. Thanks a lot for working on this! I'n CC'ing Ken, who maintains the Cygwin ports of Emacs, because I think the cygw32 build would like to use this feature as well. Ken, could you please give the patch some testing and report any problems? > This requires the harfbuzz font backend to be available. > > It works by modifying the harfbuzz backend to use DirectWrite at > some points, if it is available: > > - When encoding characters: w32hb_encode_char > - When measuring text: w32font_text_extents > - When drawing text: w32font_draw > > DirectWrite is setup by calling w32_initialize_direct_write. From > that point, the function w32_use_direct_write will return true if > DirectWrite is to be used. > > The lisp variable w32-inhibit-direct-write can switches it on and off. This design SGTM, thanks. Eventually, we will need a NEWS entry for this change, and w32-inhibit-direct-write (which I'd rename to w32-inhibit-dwrite for better mnemonic value) should be mentioned there. > It builds with Mingw64 and Mingw. As mingw doesn't have the DirectWrite > includes, I had to declare a lot of things at the start of the file. > This line marks the end of that: > > /* End of dwrite_3.h definitions. */ I wonder if this is sufficiently future-proof. AFAIU, dwrite_3.h is a generated file, so it could change in the future. So maybe we should only define these on our own for mingw.org, and in MinGW64 builds use the header file they provide? Maybe even on a separate header file? This way, at least most of the users will be immune to future changes in the header file. WDYT? > * configure.ac: Add src/w32drite to W32_OBJ ^ Period missing there. > * src/w32dwrite.c: This is a new file. > (w32-initialize-direct-write): Initializes the DirectWrite library if ^^^^^^^^^^^ A nit: our style is to say "Initialize". > (w32_use_direct_write): Allows to check if DirectWrite is available ^^^^^^^^^^^^^^^ And here, just "Check" will do. > (w32_dwrite_encode_char): Replacement for harfbuzz's encode_char. > (w32_dwrite_text_extents): Replacement for w32font text_extents. > (w32_dwrite_draw): Replacement for w32font draw. > (w32_dwrite_free_cached_face): Used on the font deletion process to > also delete DirectWrite data. > (verify_hr): Verify COM method results. These are new functions, so please say so. Like this: (w32_dwrite_encode_char): New function, implements encode_char for DirectWrite. > * src/w32font.c: > (w32font_text_extents): If DirectWrite is enabled, call This and similar entries should be formatted like so: * src/w32font.c (w32font_text_extents): If DirectWrite is enabled, ... We only break the line between the file name and the function name if the latter is too long to be placed on the same line. > * src/w32uniscribe.c: > (w32hb_encode_char): If DirectWrite is enabled, call > w32_dwrite_encode_char. Same here. > --- /dev/null > +++ b/src/w32dwrite.c > @@ -0,0 +1,930 @@ > +/* Support for using DirectWrite on MS-Windows to draw text. This allows > + for color fonts. ^^ Two spaces between sentences, per our conventions. > +/* > + This requires the harfbuzz font backend to be available. ^^^^^^^^ HarfBuzz, here and elsewhere (except in code). > +#include > +#include > +#include > + > +#include "frame.h" > +#include "w32font.h" > +#include "w32common.h" > +#include "w32term.h" > + > +#include "initguid.h" > +#include > +#include That should be . And I think the last 3 includes should be before our Emacs-specific header files. > +/* The following definitions would be included from dwrite_3.h, but it > + is not available when building with MinGW. Methods that we don't use ^^^^^ Please say "mingw.org's MinGW", to make this more clear. > + are declared with the UNUSED macro, to avoid bringing in more types > + that would need to be declared. > +*/ > + > +#define UNUSED(name) void (STDMETHODCALLTYPE *name)(void) Can we use something more specific than UNUSED here? Like W32_UNUSED or maybe EMACS_DWRITE_UNUSED? > +/* Function prototypes. */ > +void w32_initialize_direct_write (void); > +bool w32_use_direct_write (struct w32font_info *w32font); > +void w32_dwrite_draw (HDC hdc, int x, int y, unsigned *glyphs, int len, > + COLORREF color, struct font *font ); > +void w32_dwrite_text_extents (struct font *font, const unsigned *code, > + int nglyphs, struct font_metrics *metrics); > +unsigned w32_dwrite_encode_char (struct font *font, int c); > +void w32_dwrite_free_cached_face(void *cache); > + > +/* This two functions are on w32uninscribe.c */ > +void *w32_font_get_dwrite_cache (struct font *font, float *font_size); > +void w32_font_set_dwrite_cache (struct font *font, void *cache, > + float font_size); These prototypes should go to w32font.h. Then you won't need to repeat them in other source files. > +static void > +verify_hr (HRESULT hr, const char *msg) > +{ > + if (FAILED (hr)) > + emacs_abort (); > +} I think aborting is too harsh here. We could use eassert instead (in which case it will abort, but only in a build with --enable-checking). We could also augment that with DebPrint in important places, which will show a message when a production build of Emacs is run under a debugger. In a production build (i.e., when eassert compiles to nothing), it is better to disable the use of DirectWrite when one of these problems happen, if you detect that early enough, and fall back to HarfBuzz instead. Is that reasonable here? > + return abs(logfont.lfHeight); ^ SPC missing there. > +void > +w32_dwrite_free_cached_face(void *cache) ^ And here (and in a few places elsewhere in the patch). > +static float > +convert_metrics_sz( int sz, float font_size, int units_per_em ) > +{ > + return (float)sz * font_size / units_per_em; ^^ Here also. > + dwrite_font_face->lpVtbl-> > + GetMetrics (dwrite_font_face, &dwrite_font_metrics); It is better not to break at the dereference operator: dwrite_font_face->lpVtbl->GetMetrics (dwrite_font_face, &dwrite_font_metrics); Same in other places in the patch. > + CreateFactory create_factory > + = (CreateFactory) get_proc_addr (direct_write, "DWriteCreateFactory"); It is better to call the procedure pointers by names that are closer to their names in the library. In this case, I'd use dw_create_factory instead of a more general create_factory. > + /* We never Release this, get_bitmap_render_target reuses it. */ ^^^^^^^ "release", in lower case. > + DWRITE_GLYPH_RUN glyph_run; > + glyph_run.fontFace = dwrite_font_face; > + glyph_run.fontEmSize = font_size; > + glyph_run.glyphIndices = indices; > + glyph_run.glyphCount = len; > + glyph_run.isSideways = false; > + glyph_run.bidiLevel = 0; ^^^^^^^^^^^^^ Is this because otherwise DirectWrite will possibly reorder the glyphs or something? Did you test this with R2L text, e.g. by displaying TUTORIAL.he? I did try, and it looks OK, but are there any color fonts that cover the Hebrew block, so I could be sure everything works in that case? More generally, what color fonts can be used with this build on Windows for characters other than Emoji, in addition to Segoe UI Emoji? > + /* No color. Just draw the GlyphRun. */ ^^ Two spaces. > + /* If there were color glyphs, layers contains a containts a list ^^^^^^^^^^ ^^^^^^^^^^^ One of these is redundant. > --- a/src/w32font.c > +++ b/src/w32font.c > @@ -55,6 +55,13 @@ #define VIETNAMESE_CHARSET 163 > #define JOHAB_CHARSET 130 > #endif > > +/* Function prototypes from w32write.c */ ^^^^^^^^^^ w32dwrite.c > +bool w32_use_direct_write (struct w32font_info *w32font); > +void w32_dwrite_draw (HDC hdc, int x, int y, unsigned *glyphs, int len, > + COLORREF color, struct font *font ); > +void w32_dwrite_text_extents (struct font *font, const unsigned *code, > + int nglyphs, struct font_metrics *metrics); > + If you put them on w32font.h, this won't be needed. > --- a/src/w32uniscribe.c > +++ b/src/w32uniscribe.c > @@ -44,6 +44,15 @@ #define _WIN32_WINNT 0x0600 > #include "pdumper.h" > #include "w32common.h" > > +/* Function prototypes from w32dwrite.c */ > +bool w32_use_direct_write (struct w32font_info *w32font); > +unsigned w32_dwrite_encode_char (struct font *font, int c); > +void w32_dwrite_free_cached_face (void *cache); Likewise: if these are in w32font.h, you won't need this. > +/* Function prototypes from this file */ > +void *w32_font_get_dwrite_cache (struct font *font, float *font_size); > +void w32_font_set_dwrite_cache (struct font *font, void *cache, float font_size); Why aren't these functions defined in w32dwrite.c? > +void *w32_font_get_dwrite_cache (struct font *font, float *font_size) We break the line between the return type and the function name: void * w32_font_get_dwrite_cache (struct font *font, float *font_size) > + if (w32_use_direct_write(&uniscribe_font->w32_font)) > + { > + return w32_dwrite_encode_char(font, c); > + } No need for braces if you have just one statement. Compiling this with mingw.org's MinGW GCC 9.2.0, I get the following warnings: In file included from w32dwrite.c:44: w32dwrite.c:123:13: warning: 'IID_IDWriteBitmapRenderTarget1' initialized and declared 'extern' 123 | DEFINE_GUID( CC w32inevt.oIID_IDWriteBitmapRenderTarget1 , 0x791e8298, 0x3ef3, 0x4230, 0x98,0x80, 0xc9,0xbd,0xec,0xc4,0x20,0x64); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ w32dwrite.c:124:13: warning: 'IID_IDWriteFactory2' initialized and declared 'extern' 124 | DEFINE_GUID(IID_IDWriteFactory2, 0x0439fc60, 0xca44, 0x4994, 0x8d,0xee, 0x3a,0x9a,0xf7,0xb7,0x32,0xec); | ^~~~~~~~~~~~~~~~~~~ w32dwrite.c:125:13: warning: 'IID_IDWriteFactory' initialized and declared 'extern' 125 | DEFINE_GUID(IID_IDWriteFactory, 0xb859ee5a, 0xd838, 0x4b5b, 0xa2,0xe8, 0x1a,0xdc,0x7d,0x93,0xdb,0x48); | ^~~~~~~~~~~~~~~~~~