bug#73730: 31.0.50; Support for color fonts on MS-Windows

[Eli Zaretskii <eliz@gnu.org>]

> Date: Wed, 16 Oct 2024 00:18:20 +0200
> Cc: 73730@debbugs.gnu.org
> From: Cecilio Pardo <cpardo@imayhem.com>
> 
> 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 <config.h>
> +#include <math.h>
> +#include <windows.h>
> +
> +#include "frame.h"
> +#include "w32font.h"
> +#include "w32common.h"
> +#include "w32term.h"
> +
> +#include "initguid.h"
> +#include <ole2.h>
> +#include <unknwn.h>

That should be <initguid.h>.  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);
	|             ^~~~~~~~~~~~~~~~~~