bug#54564: 29.0.50; [PATCH] Use gsettings font rendering entries for pgtk builds

[Pieter van Prooijen <pieter.van.prooijen@teloden.nl>]

[-- Attachment #1: Type: text/plain, Size: 2547 bytes --]

Hi All, 

It took a fair bit of digging, but sub-pixel anti-aliasing on pgtk now
seems to work correctly.

The problem lies with the "source" operator set by pgtk when drawing a
font, this will take a particular execution path in the cairo glyph
compositor (using a mask image) which converts the colors created by
freetype for the sub pixel effect to greys-cale. Using the default
"over" operator takes a different path which preserves the colors. Note
that on small screens the difference between sub-pixel and grey-scale
rendering is hard to see, it's more pronounced on larger displays)

I've amended my gsettings patch with this change, only replacing the
operator for anti-aliased fonts in ftcrfont_draw, because I'm not sure
about its impact for non-anti-aliased fonts.    

I've also fixed the indent error, but couldn't find the non-breaking
space in the patch, it's perhaps caused by the mail program.

Regarding the copyright assignment, I've had no response after sending
in the questionnaire at
https://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future
to assign@gnu.org

Do you get sent a paper form of some kind which has to be filled in?
(I'm not at my home address atm so I will have missed that)

Kind Regards,

Pieter


On Fri, 2022-04-01 at 10:00 +0800, Po Lu wrote:
> Pieter van Prooijen <pieter.van.prooijen@teloden.nl> writes:
> 
> > Hi All,
> > 
> > Thanks for the review, I've updated the commit message and other
> > changes you mentioned in the attached patch (against
> > c5af19cba5924de89a38e7a177c07f42fd3cd543)
> > 
> > I've requested the form for the copyright assignment, but have not
> > received it yet, will send it in as soon as it arrives.
> > 
> > No progress yet on the problem with subpixel antialiasing, but it
> > doesn't look like a fontconfig issue, as a non-pgtk build from the
> > same
> > source works correctly, will have to dig deeper to find out what is
> > happening. 
> 
> Thanks.
> 
> > > > +   on the result.  */
> 
> Hmm... Is there an actual non-breaking space character here, or is your
> MUA munging the messages?
> 
> > +  if (dpyinfo_valid (first_dpyinfo))
> > +      store_config_changed_event (Qfont_render,
> > +                                  XCAR (first_dpyinfo-
> > >name_list_element));
> 
> You got the indentation wrong here.  The "s" in
> "store_config_changed_event" should be two spaces behind where it is
> now.
> 
> Otherwise, LGTM.  Hopefully the copyright paperwork can be finished
> soon.


[-- Attachment #2: 0001-Use-gsettings-font-rendering-entries-for-pgtk-builds.patch --]
[-- Type: text/x-patch, Size: 10626 bytes --]

From b9d96a47c4b301e0445a6d7b80cfb8876c921c05 Mon Sep 17 00:00:00 2001
From: Pieter van Prooijen <pieter.van.prooijen@teloden.nl>
Date: Thu, 7 Apr 2022 18:52:14 +0200
Subject: [PATCH] Use gsettings font rendering entries for pgtk builds

If present, apply the gsettings font hinting and antialiasing
entries when creating a font in cairo.  Do this at
initialization and when the entries change, re-rendering the
frames.

* src/ftcrfont.c (ftcrfont_open): Use the font_options derived
from gsettings when opening a font.
(ftcrfont_cached_font_ok): Report a cached font as invalid if
its font options differ from the current options inside
gsettings.
(ftcrfont_draw): Use a different cairo operator when
drawing subpixel antialiased fonts.
* src/xsettings.c (apply_gsettings_font_hinting)
(apply_gsettings_font_alias, apply_gsettings_font_rgba_order):
Convert the settings from GSettings to the cairo_font_options_t
object.
(init_gsettings, something_changed_gsettingsCB): Invoke the
apply functions if the relevant settings changed.
(store_font_options_changed): Store an event to re-render the
fonts.
(xsetting_get_font_options)
* src/xsettings.h (xsettings_get_font_options): New function.
---
 src/ftcrfont.c  |  43 ++++++++++++++
 src/xsettings.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/xsettings.h |   5 ++
 3 files changed, 195 insertions(+)

diff --git a/src/ftcrfont.c b/src/ftcrfont.c
index 98a28af5f2..e6067d25c7 100644
--- a/src/ftcrfont.c
+++ b/src/ftcrfont.c
@@ -37,6 +37,9 @@
 #include "font.h"
 #include "ftfont.h"
 #include "pdumper.h"
+#ifdef HAVE_PGTK
+#include "xsettings.h"
+#endif
 
 #ifdef USE_BE_CAIRO
 #define RED_FROM_ULONG(color)	(((color) >> 16) & 0xff)
@@ -168,7 +171,12 @@ ftcrfont_open (struct frame *f, Lisp_Object entity, int pixel_size)
   cairo_matrix_t font_matrix, ctm;
   cairo_matrix_init_scale (&font_matrix, pixel_size, pixel_size);
   cairo_matrix_init_identity (&ctm);
+
+#ifdef HAVE_PGTK
+  cairo_font_options_t *options = xsettings_get_font_options ();
+#else
   cairo_font_options_t *options = cairo_font_options_create ();
+#endif
 #ifdef USE_BE_CAIRO
   if (be_use_subpixel_antialiasing ())
     cairo_font_options_set_antialias (options, CAIRO_ANTIALIAS_SUBPIXEL);
@@ -599,6 +607,16 @@ ftcrfont_draw (struct glyph_string *s,
   x_set_cr_source_with_gc_foreground (f, s->gc, false);
 #else
   pgtk_set_cr_source_with_color (f, s->xgcv.foreground, false);
+
+  /* The CAIRO_OPERATOR_SOURCE operator set by pgtk_set_cr_source_with_color
+     above renders sub-pixel anti-aliased fonts in grey-scale, use the
+     default CAIRO_OPERATOR_OVER instead.  */
+  cairo_font_options_t *options = cairo_font_options_create ();
+  cairo_scaled_font_get_font_options (ftcrfont_info->cr_scaled_font,
+				      options);
+  if (cairo_font_options_get_antialias (options) == CAIRO_ANTIALIAS_SUBPIXEL)
+    cairo_set_operator (cr, CAIRO_OPERATOR_OVER);
+  cairo_font_options_destroy (options);
 #endif
 #else
   uint32_t col = be_foreground;
@@ -624,6 +642,28 @@ ftcrfont_draw (struct glyph_string *s,
   return len;
 }
 
+#ifdef HAVE_PGTK
+/* Determine if FONT_OBJECT is a valid cached font for ENTITY by
+   comparing the options used to open it with the user's current
+   preferences specified via GSettings.  */
+static bool
+ftcrfont_cached_font_ok (struct frame *f, Lisp_Object font_object,
+			 Lisp_Object entity)
+{
+  struct font_info *info = (struct font_info *) XFONT_OBJECT (font_object);
+
+  cairo_font_options_t *options = cairo_font_options_create ();
+  cairo_scaled_font_get_font_options (info->cr_scaled_font, options);
+  cairo_font_options_t *gsettings_options = xsettings_get_font_options ();
+
+  bool equal = cairo_font_options_equal (options, gsettings_options);
+  cairo_font_options_destroy (options);
+  cairo_font_options_destroy (gsettings_options);
+
+  return equal;
+}
+#endif
+
 #ifdef HAVE_HARFBUZZ
 
 static Lisp_Object
@@ -694,6 +734,9 @@ ftcrhbfont_end_hb_font (struct font *font, hb_font_t *hb_font)
 #endif
   .filter_properties = ftfont_filter_properties,
   .combining_capability = ftfont_combining_capability,
+#ifdef HAVE_PGTK
+  .cached_font_ok = ftcrfont_cached_font_ok
+#endif
   };
 #ifdef HAVE_HARFBUZZ
 struct font_driver ftcrhbfont_driver;
diff --git a/src/xsettings.c b/src/xsettings.c
index 71d02e6152..e71887e03d 100644
--- a/src/xsettings.c
+++ b/src/xsettings.c
@@ -215,11 +215,116 @@ #define GSETTINGS_MONO_FONT  "monospace-font-name"
 #define GSETTINGS_FONT_NAME  "font-name"
 #endif
 
+#ifdef HAVE_PGTK
+#define GSETTINGS_FONT_ANTIALIASING  "font-antialiasing"
+#define GSETTINGS_FONT_RGBA_ORDER    "font-rgba-order"
+#define GSETTINGS_FONT_HINTING       "font-hinting"
+#endif
 
 /* The single GSettings instance, or NULL if not connected to GSettings.  */
 
 static GSettings *gsettings_client;
 
+#ifdef HAVE_PGTK
+
+/* The cairo font_options as obtained using gsettings.  */
+static cairo_font_options_t *font_options;
+
+/* Store an event for re-rendering of the fonts.  */
+static void
+store_font_options_changed (void)
+{
+  if (dpyinfo_valid (first_dpyinfo))
+    store_config_changed_event (Qfont_render,
+				XCAR (first_dpyinfo->name_list_element));
+}
+
+/* Apply changes in the hinting system setting.  */
+static void
+apply_gsettings_font_hinting (GSettings *settings)
+{
+  GVariant *val = g_settings_get_value (settings, GSETTINGS_FONT_HINTING);
+  if (val)
+    {
+      g_variant_ref_sink (val);
+      if (g_variant_is_of_type (val, G_VARIANT_TYPE_STRING))
+	{
+	  const char *hinting = g_variant_get_string (val, NULL);
+
+	  if (!strcmp (hinting, "full"))
+	    cairo_font_options_set_hint_style (font_options,
+					       CAIRO_HINT_STYLE_FULL);
+	  else if (!strcmp (hinting, "medium"))
+	    cairo_font_options_set_hint_style (font_options,
+					       CAIRO_HINT_STYLE_MEDIUM);
+	  else if (!strcmp (hinting, "slight"))
+	    cairo_font_options_set_hint_style (font_options,
+					       CAIRO_HINT_STYLE_SLIGHT);
+	  else if (!strcmp (hinting, "none"))
+	    cairo_font_options_set_hint_style (font_options,
+					       CAIRO_HINT_STYLE_NONE);
+	}
+      g_variant_unref (val);
+    }
+}
+
+/* Apply changes in the antialiasing system setting.  */
+static void
+apply_gsettings_font_antialias (GSettings *settings)
+{
+  GVariant *val = g_settings_get_value (settings, GSETTINGS_FONT_ANTIALIASING);
+  if (val)
+    {
+      g_variant_ref_sink (val);
+      if (g_variant_is_of_type (val, G_VARIANT_TYPE_STRING))
+	{
+	  const char *antialias = g_variant_get_string (val, NULL);
+
+	  if (!strcmp (antialias, "none"))
+	    cairo_font_options_set_antialias (font_options,
+					      CAIRO_ANTIALIAS_NONE);
+	  else if (!strcmp (antialias, "grayscale"))
+	    cairo_font_options_set_antialias (font_options,
+					      CAIRO_ANTIALIAS_GRAY);
+	  else if (!strcmp (antialias, "rgba"))
+	    cairo_font_options_set_antialias (font_options,
+					      CAIRO_ANTIALIAS_SUBPIXEL);
+	}
+      g_variant_unref (val);
+    }
+}
+
+/* Apply the settings for the rgb element ordering.  */
+static void
+apply_gsettings_font_rgba_order (GSettings *settings)
+{
+  GVariant *val = g_settings_get_value (settings,
+					GSETTINGS_FONT_RGBA_ORDER);
+  if (val)
+    {
+      g_variant_ref_sink (val);
+      if (g_variant_is_of_type (val, G_VARIANT_TYPE_STRING))
+	{
+	  const char *rgba_order = g_variant_get_string (val, NULL);
+
+	  if (!strcmp (rgba_order, "rgb"))
+	    cairo_font_options_set_subpixel_order (font_options,
+						   CAIRO_SUBPIXEL_ORDER_RGB);
+	  else if (!strcmp (rgba_order, "bgr"))
+	    cairo_font_options_set_subpixel_order (font_options,
+						   CAIRO_SUBPIXEL_ORDER_BGR);
+	  else if (!strcmp (rgba_order, "vrgb"))
+	    cairo_font_options_set_subpixel_order (font_options,
+						   CAIRO_SUBPIXEL_ORDER_VRGB);
+	  else if (!strcmp (rgba_order, "vbgr"))
+	    cairo_font_options_set_subpixel_order (font_options,
+						   CAIRO_SUBPIXEL_ORDER_VBGR);
+	}
+      g_variant_unref (val);
+    }
+}
+#endif /* HAVE_PGTK */
+
 /* Callback called when something changed in GSettings.  */
 
 static void
@@ -273,6 +378,23 @@ something_changed_gsettingsCB (GSettings *settings,
         }
     }
 #endif /* USE_CAIRO || HAVE_XFT */
+#ifdef HAVE_PGTK
+  else if (!strcmp (key, GSETTINGS_FONT_ANTIALIASING))
+    {
+      apply_gsettings_font_antialias (settings);
+      store_font_options_changed ();
+    }
+  else if (!strcmp (key, GSETTINGS_FONT_HINTING))
+    {
+      apply_gsettings_font_hinting (settings);
+      store_font_options_changed ();
+    }
+  else if (!strcmp (key, GSETTINGS_FONT_RGBA_ORDER))
+    {
+      apply_gsettings_font_rgba_order (settings);
+      store_font_options_changed ();
+    }
+#endif /* HAVE_PGTK */
 }
 
 #endif /* HAVE_GSETTINGS */
@@ -900,6 +1022,16 @@ init_gsettings (void)
         dupstring (&current_font, g_variant_get_string (val, NULL));
       g_variant_unref (val);
     }
+
+  /* Only use the gsettings font entries for the Cairo backend
+     running on PGTK.  */
+#ifdef HAVE_PGTK
+  font_options = cairo_font_options_create ();
+  apply_gsettings_font_antialias (gsettings_client);
+  apply_gsettings_font_hinting (gsettings_client);
+  apply_gsettings_font_rgba_order (gsettings_client);
+#endif /* HAVE_PGTK */
+
 #endif /* USE_CAIRO || HAVE_XFT */
 
 #endif /* HAVE_GSETTINGS */
@@ -1021,6 +1153,17 @@ xsettings_get_system_normal_font (void)
 }
 #endif
 
+#ifdef HAVE_PGTK
+/* Return the cairo font options, updated from the gsettings font
+   config entries.  The caller should call cairo_font_options_destroy
+   on the result.  */
+cairo_font_options_t *
+xsettings_get_font_options (void)
+{
+  return cairo_font_options_copy (font_options);
+}
+#endif
+
 DEFUN ("font-get-system-normal-font", Ffont_get_system_normal_font,
        Sfont_get_system_normal_font,
        0, 0, 0,
@@ -1073,6 +1216,10 @@ syms_of_xsettings (void)
   gconf_client = NULL;
   PDUMPER_IGNORE (gconf_client);
 #endif
+#ifdef HAVE_PGTK
+  font_options = NULL;
+  PDUMPER_IGNORE (font_options);
+#endif
 
   DEFSYM (Qmonospace_font_name, "monospace-font-name");
   DEFSYM (Qfont_name, "font-name");
diff --git a/src/xsettings.h b/src/xsettings.h
index ccaa36489d..5e5df37062 100644
--- a/src/xsettings.h
+++ b/src/xsettings.h
@@ -23,6 +23,8 @@ #define XSETTINGS_H
 #ifndef HAVE_PGTK
 #include "dispextern.h"
 #include <X11/Xlib.h>
+#else
+#include <cairo.h>
 #endif
 
 struct x_display_info;
@@ -41,5 +43,8 @@ #define XSETTINGS_H
 extern const char *xsettings_get_system_normal_font (void);
 #endif
 
+#ifdef HAVE_PGTK
+extern cairo_font_options_t *xsettings_get_font_options (void);
+#endif
 
 #endif /* XSETTINGS_H */
-- 
2.32.0