Re: harfbuzz 2f72162: Fix crash in the Cairo build

classic Classic list List threaded Threaded
36 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Robert Pluim
[hidden email] (Eli Zaretskii) writes:

> diff --git a/src/ftcrfont.c b/src/ftcrfont.c
> index e2f84d4..6d74d93 100644
> --- a/src/ftcrfont.c
> +++ b/src/ftcrfont.c
> @@ -41,6 +41,9 @@ struct ftcrfont_info
>    bool maybe_otf;  /* Flag to tell if this may be OTF or not.  */
>    OTF *otf;
>  #endif /* HAVE_LIBOTF */
> +#ifdef HAVE_HARFBUZZ
> +  hb_font_t *hb_font;
> +#endif  /* HAVE_HARFBUZZ */
>    FT_Size ft_size;
>    int index;
>    FT_Matrix matrix;

Eli, what would be your preferred type of solution to avoid this in
future? A #define of the common portion of the structs in ftfont.h? An
embedded 'struct ft_font_fixed' member inside struct ft{,cr}font_info?
Something else? At the very least I propose syncing the comments:

diff --git i/src/ftcrfont.c w/src/ftcrfont.c
index e2f84d44fc..7d63a344ec 100644
--- i/src/ftcrfont.c
+++ w/src/ftcrfont.c
@@ -35,12 +35,16 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 struct ftcrfont_info
 {
   struct font font;
-  /* The following six members must be here in this order to be
-     compatible with struct ftfont_info (in ftfont.c).  */
+  /* The following members up to and including 'matrix' must be here
+     in this order to be compatible with struct ftfont_info (in
+     ftfont.c).  */
 #ifdef HAVE_LIBOTF
   bool maybe_otf;  /* Flag to tell if this may be OTF or not.  */
   OTF *otf;
 #endif /* HAVE_LIBOTF */
+#ifdef HAVE_HARFBUZZ
+  hb_font_t *hb_font;
+#endif  /* HAVE_HARFBUZZ */
   FT_Size ft_size;
   int index;
   FT_Matrix matrix;
diff --git i/src/ftfont.c w/src/ftfont.c
index a645bbf029..ef4ccab3ec 100644
--- i/src/ftfont.c
+++ w/src/ftfont.c
@@ -56,8 +56,9 @@ struct ftfont_info
 {
   struct font font;
 #ifdef HAVE_LIBOTF
-  /* The following four members must be here in this order to be
-     compatible with struct xftfont_info (in xftfont.c).  */
+  /* The following members up to and including 'matrix' must be here in
+     this order to be compatible with struct xftfont_info (in
+     xftfont.c).  */
   bool maybe_otf; /* Flag to tell if this may be OTF or not.  */
   OTF *otf;
 #endif /* HAVE_LIBOTF */

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Eli Zaretskii
> From: Robert Pluim <[hidden email]>
> Date: Fri, 14 Dec 2018 13:18:41 +0100
> Cc: Ari Roponen <[hidden email]>
>
> Eli, what would be your preferred type of solution to avoid this in
> future?

If at all possible, I prefer a single definition of each struct, with
as few #ifdef's as possible.  If some member is not used in some
configuration, I prefer to have it anyway, unless it cannot be
compiled in the configuration(s) that don't use it (e.g., because it
uses a data type that is only visible in some configurations).

> At the very least I propose syncing the comments:

Yes, please push.

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Robert Pluim
Eli Zaretskii <[hidden email]> writes:

>> From: Robert Pluim <[hidden email]>
>> Date: Fri, 14 Dec 2018 13:18:41 +0100
>> Cc: Ari Roponen <[hidden email]>
>>
>> Eli, what would be your preferred type of solution to avoid this in
>> future?
>
> If at all possible, I prefer a single definition of each struct, with
> as few #ifdef's as possible.  If some member is not used in some
> configuration, I prefer to have it anyway, unless it cannot be
> compiled in the configuration(s) that don't use it (e.g., because it
> uses a data type that is only visible in some configurations).
>

This I will do in master.

>> At the very least I propose syncing the comments:
>
> Yes, please push.

This I pushed to emacs-26 as f14d5742db

Robert

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Robert Pluim
In reply to this post by Eli Zaretskii
Eli Zaretskii <[hidden email]> writes:

>> From: Robert Pluim <[hidden email]>
>> Date: Fri, 14 Dec 2018 13:18:41 +0100
>> Cc: Ari Roponen <[hidden email]>
>>
>> Eli, what would be your preferred type of solution to avoid this in
>> future?
>
> If at all possible, I prefer a single definition of each struct, with
> as few #ifdef's as possible.  If some member is not used in some
> configuration, I prefer to have it anyway, unless it cannot be
> compiled in the configuration(s) that don't use it (e.g., because it
> uses a data type that is only visible in some configurations).
>

Aligning ftfont.c and ftcrfont.c is pretty easy. Did you want me to
try to unify xftfont.c in there as well? Iʼm tempted to get rid of
ftxfont.c as well, unless someone is actually still using that. [1]

Robert

Footnotes:
[1] How many different font backends can we come up with using only 3
     letters?


Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Eli Zaretskii
> From: Robert Pluim <[hidden email]>
> Cc: [hidden email],  [hidden email]
> Date: Fri, 14 Dec 2018 17:16:46 +0100
>
> Aligning ftfont.c and ftcrfont.c is pretty easy. Did you want me to
> try to unify xftfont.c in there as well?

If that makes sense, i.e. if the commonality seems to justify that,
then yes.

> Iʼm tempted to get rid of ftxfont.c as well, unless someone is
> actually still using that. [1]

I think this is a decision for another time.  In any case, we cannot
just remove it, we need to deprecate it first.  We may declare it
deprecated now, and perhaps cause its inclusion to trigger some
deprecation message during building Emacs.

> [1] How many different font backends can we come up with using only 3
>      letters?

That calls for a Lisp program, no?

Thanks.

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Stefan Monnier
In reply to this post by Robert Pluim
> [1] How many different font backends can we come up with using only 3
>      letters?

Are we limited to ASCII letters?


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Robert Pluim
In reply to this post by Eli Zaretskii
Eli Zaretskii <[hidden email]> writes:

>> From: Robert Pluim <[hidden email]>
>> Cc: [hidden email],  [hidden email]
>> Date: Fri, 14 Dec 2018 17:16:46 +0100
>>
>> Aligning ftfont.c and ftcrfont.c is pretty easy. Did you want me to
>> try to unify xftfont.c in there as well?
>
> If that makes sense, i.e. if the commonality seems to justify that,
> then yes.
>

I had a quick go at this. Iʼve not measured the memory difference, but
I can do if people thinks this refactor is worth it. Apart from the
unification of the three struct definitions, most of the patch is
mechanical changes.

 src/ftcrfont.c | 41 ++++++-----------------------------------
 src/ftfont.c   | 58 +++++++++++++++++++++++++---------------------------------
 src/ftfont.h   | 28 +++++++++++++++++++++++++++-
 src/xftfont.c  | 43 +++++++++++--------------------------------
 4 files changed, 69 insertions(+), 101 deletions(-)

diff --git i/src/ftcrfont.c w/src/ftcrfont.c
index dc1a389c60..3c963e0e71 100644
--- i/src/ftcrfont.c
+++ w/src/ftcrfont.c
@@ -27,32 +27,6 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include "font.h"
 #include "ftfont.h"
 
-/* FTCR font driver.  */
-
-/* The actual structure for FTCR font.  A pointer to this structure
-   can be cast to struct font *.  */
-
-struct ftcrfont_info
-{
-  struct font font;
-  /* The following six members must be here in this order to be
-     compatible with struct ftfont_info (in ftfont.c).  */
-#ifdef HAVE_LIBOTF
-  bool maybe_otf;  /* Flag to tell if this may be OTF or not.  */
-  OTF *otf;
-#endif /* HAVE_LIBOTF */
-  FT_Size ft_size;
-  int index;
-  FT_Matrix matrix;
-
-  cairo_font_face_t *cr_font_face;
-  /* To prevent cairo from cluttering the activated FT_Size maintained
-     in ftfont.c, we activate this special FT_Size before drawing.  */
-  FT_Size ft_size_draw;
-  /* Font metrics cache.  */
-  struct font_metrics **metrics;
-  short metrics_nrows;
-};
 
 #define METRICS_NCOLS_PER_ROW (128)
 
@@ -70,7 +44,7 @@ ftcrfont_glyph_extents (struct font *font,
                         unsigned glyph,
                         struct font_metrics *metrics)
 {
-  struct ftcrfont_info *ftcrfont_info = (struct ftcrfont_info *) font;
+  struct font_info *ftcrfont_info = (struct font_info *) font;
   int row, col;
   struct font_metrics *cache;
 
@@ -132,7 +106,7 @@ ftcrfont_open (struct frame *f, Lisp_Object entity, int pixel_size)
 {
   Lisp_Object font_object;
   struct font *font;
-  struct ftcrfont_info *ftcrfont_info;
+  struct font_info *ftcrfont_info;
   FT_Face ft_face;
   FT_UInt size;
 
@@ -140,14 +114,14 @@ ftcrfont_open (struct frame *f, Lisp_Object entity, int pixel_size)
   size = XFIXNUM (AREF (entity, FONT_SIZE_INDEX));
   if (size == 0)
     size = pixel_size;
-  font_object = font_build_object (VECSIZE (struct ftcrfont_info),
+  font_object = font_build_object (VECSIZE (struct font_info),
    Qftcr, entity, size);
   font_object = ftfont_open2 (f, entity, pixel_size, font_object);
   if (NILP (font_object)) return Qnil;
 
   font = XFONT_OBJECT (font_object);
   font->driver = &ftcrfont_driver;
-  ftcrfont_info = (struct ftcrfont_info *) font;
+  ftcrfont_info = (struct font_info *) font;
   ft_face = ftcrfont_info->ft_size->face;
   FT_New_Size (ft_face, &ftcrfont_info->ft_size_draw);
   FT_Activate_Size (ftcrfont_info->ft_size_draw);
@@ -167,7 +141,7 @@ ftcrfont_close (struct font *font)
   if (font_data_structures_may_be_ill_formed ())
     return;
 
-  struct ftcrfont_info *ftcrfont_info = (struct ftcrfont_info *) font;
+  struct font_info *ftcrfont_info = (struct font_info *) font;
   int i;
 
   block_input ();
@@ -223,7 +197,7 @@ ftcrfont_draw (struct glyph_string *s,
 {
   struct frame *f = s->f;
   struct face *face = s->face;
-  struct ftcrfont_info *ftcrfont_info = (struct ftcrfont_info *) s->font;
+  struct font_info *ftcrfont_info = (struct font_info *) s->font;
   cairo_t *cr;
   cairo_glyph_t *glyphs;
   cairo_surface_t *surface;
@@ -312,9 +286,6 @@ struct font_driver const ftcrfont_driver =
 void
 syms_of_ftcrfont (void)
 {
-  if (ftfont_info_size != offsetof (struct ftcrfont_info, cr_font_face))
-    abort ();
-
   DEFSYM (Qftcr, "ftcr");
   register_font_driver (&ftcrfont_driver, NULL);
 }
diff --git i/src/ftfont.c w/src/ftfont.c
index e83eff3ad0..5d4fcc4a45 100644
--- i/src/ftfont.c
+++ w/src/ftfont.c
@@ -24,6 +24,17 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 #include <fontconfig/fontconfig.h>
 #include <fontconfig/fcfreetype.h>
 
+/* These two blocks are here because this file is built when using XFT
+   and when using Cairo, so struct font_info in ftfont.h needs access
+   to the appropriate types.  */
+#ifdef HAVE_XFT
+#include <X11/Xlib.h>
+#include <X11/Xft/Xft.h>
+#endif
+#ifdef USE_CAIRO
+#include <cairo-ft.h>
+#endif
+
 #include <c-strcase.h>
 
 #include "lisp.h"
@@ -49,25 +60,6 @@ static Lisp_Object freetype_font_cache;
 /* Cache for FT_Face and FcCharSet. */
 static Lisp_Object ft_face_cache;
 
-/* The actual structure for FreeType font that can be cast to struct
-   font.  */
-
-struct ftfont_info
-{
-  struct font font;
-#ifdef HAVE_LIBOTF
-  /* The following four members must be here in this order to be
-     compatible with struct xftfont_info (in xftfont.c).  */
-  bool maybe_otf; /* Flag to tell if this may be OTF or not.  */
-  OTF *otf;
-#endif /* HAVE_LIBOTF */
-  FT_Size ft_size;
-  int index;
-  FT_Matrix matrix;
-};
-
-size_t ftfont_info_size = sizeof (struct ftfont_info);
-
 enum ftfont_cache_for
   {
     FTFONT_CACHE_FOR_FACE,
@@ -452,7 +444,7 @@ ftfont_get_fc_charset (Lisp_Object entity)
 
 #ifdef HAVE_LIBOTF
 static OTF *
-ftfont_get_otf (struct ftfont_info *ftfont_info)
+ftfont_get_otf (struct font_info *ftfont_info)
 {
   OTF *otf;
 
@@ -1095,7 +1087,7 @@ ftfont_open2 (struct frame *f,
               int pixel_size,
               Lisp_Object font_object)
 {
-  struct ftfont_info *ftfont_info;
+  struct font_info *ftfont_info;
   struct font *font;
   struct ftfont_cache_data *cache_data;
   FT_Face ft_face;
@@ -1146,7 +1138,7 @@ ftfont_open2 (struct frame *f,
 
   ASET (font_object, FONT_FILE_INDEX, filename);
   font = XFONT_OBJECT (font_object);
-  ftfont_info = (struct ftfont_info *) font;
+  ftfont_info = (struct font_info *) font;
   ftfont_info->ft_size = ft_face->size;
   ftfont_info->index = XFIXNUM (idx);
 #ifdef HAVE_LIBOTF
@@ -1236,7 +1228,7 @@ ftfont_open (struct frame *f, Lisp_Object entity, int pixel_size)
   size = XFIXNUM (AREF (entity, FONT_SIZE_INDEX));
   if (size == 0)
     size = pixel_size;
-  font_object = font_build_object (VECSIZE (struct ftfont_info),
+  font_object = font_build_object (VECSIZE (struct font_info),
    Qfreetype, entity, size);
   return ftfont_open2 (f, entity, pixel_size, font_object);
 }
@@ -1247,7 +1239,7 @@ ftfont_close (struct font *font)
   if (font_data_structures_may_be_ill_formed ())
     return;
 
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   Lisp_Object val, cache;
 
   val = Fcons (font->props[FONT_FILE_INDEX], make_fixnum (ftfont_info->index));
@@ -1291,9 +1283,9 @@ ftfont_has_char (Lisp_Object font, int c)
     }
   else
     {
-      struct ftfont_info *ftfont_info;
+      struct font_info *ftfont_info;
 
-      ftfont_info = (struct ftfont_info *) XFONT_OBJECT (font);
+      ftfont_info = (struct font_info *) XFONT_OBJECT (font);
       return (FT_Get_Char_Index (ftfont_info->ft_size->face, (FT_ULong) c)
       != 0);
     }
@@ -1302,7 +1294,7 @@ ftfont_has_char (Lisp_Object font, int c)
 unsigned
 ftfont_encode_char (struct font *font, int c)
 {
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   FT_Face ft_face = ftfont_info->ft_size->face;
   FT_ULong charcode = c;
   FT_UInt code = FT_Get_Char_Index (ft_face, charcode);
@@ -1314,7 +1306,7 @@ void
 ftfont_text_extents (struct font *font, unsigned int *code,
      int nglyphs, struct font_metrics *metrics)
 {
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   FT_Face ft_face = ftfont_info->ft_size->face;
   int i, width = 0;
   bool first;
@@ -1357,7 +1349,7 @@ ftfont_text_extents (struct font *font, unsigned int *code,
 int
 ftfont_get_bitmap (struct font *font, unsigned int code, struct font_bitmap *bitmap, int bits_per_pixel)
 {
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   FT_Face ft_face = ftfont_info->ft_size->face;
   FT_Int32 load_flags = FT_LOAD_RENDER;
 
@@ -1401,7 +1393,7 @@ int
 ftfont_anchor_point (struct font *font, unsigned int code, int idx,
      int *x, int *y)
 {
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   FT_Face ft_face = ftfont_info->ft_size->face;
 
   if (ftfont_info->ft_size != ft_face->size)
@@ -1466,7 +1458,7 @@ ftfont_otf_features (OTF_GSUB_GPOS *gsub_gpos)
 Lisp_Object
 ftfont_otf_capability (struct font *font)
 {
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   OTF *otf = ftfont_get_otf (ftfont_info);
   Lisp_Object gsub_gpos;
 
@@ -2616,7 +2608,7 @@ Lisp_Object
 ftfont_shape (Lisp_Object lgstring)
 {
   struct font *font = CHECK_FONT_GET_OBJECT (LGSTRING_FONT (lgstring));
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   OTF *otf = ftfont_get_otf (ftfont_info);
 
   return ftfont_shape_by_flt (lgstring, font, ftfont_info->ft_size->face, otf,
@@ -2630,7 +2622,7 @@ ftfont_shape (Lisp_Object lgstring)
 int
 ftfont_variation_glyphs (struct font *font, int c, unsigned variations[256])
 {
-  struct ftfont_info *ftfont_info = (struct ftfont_info *) font;
+  struct font_info *ftfont_info = (struct font_info *) font;
   OTF *otf = ftfont_get_otf (ftfont_info);
 
   if (! otf)
diff --git i/src/ftfont.h w/src/ftfont.h
index 4201b2c2d6..1f3416d2d2 100644
--- i/src/ftfont.h
+++ w/src/ftfont.h
@@ -41,6 +41,32 @@ extern Lisp_Object ftfont_open2 (struct frame *f,
                                  Lisp_Object entity,
                                  int pixel_size,
                                  Lisp_Object font_object);
-extern size_t ftfont_info_size;
+
+struct font_info
+{
+  struct font font;
+#ifdef HAVE_LIBOTF
+  bool maybe_otf; /* Flag to tell if this may be OTF or not.  */
+  OTF *otf;
+#endif /* HAVE_LIBOTF */
+  FT_Size ft_size;
+  int index;
+  FT_Matrix matrix;
+
+#ifdef USE_CAIRO
+  cairo_font_face_t *cr_font_face;
+  /* To prevent cairo from cluttering the activated FT_Size maintained
+     in ftfont.c, we activate this special FT_Size before drawing.  */
+  FT_Size ft_size_draw;
+  /* Font metrics cache.  */
+  struct font_metrics **metrics;
+  short metrics_nrows;
+#else
+  /* These are used by the XFT backend.  */
+  Display *display;
+  XftFont *xftfont;
+  unsigned x_display_id;
+#endif
+};
 
 #endif /* EMACS_FTFONT_H */
diff --git i/src/xftfont.c w/src/xftfont.c
index 85df0d857a..764862aa3a 100644
--- i/src/xftfont.c
+++ w/src/xftfont.c
@@ -35,27 +35,6 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 
 /* Xft font driver.  */
 
-
-/* The actual structure for Xft font that can be cast to struct
-   font.  */
-
-struct xftfont_info
-{
-  struct font font;
-  /* The following five members must be here in this order to be
-     compatible with struct ftfont_info (in ftfont.c).  */
-#ifdef HAVE_LIBOTF
-  bool maybe_otf;  /* Flag to tell if this may be OTF or not.  */
-  OTF *otf;
-#endif /* HAVE_LIBOTF */
-  FT_Size ft_size;
-  int index;
-  FT_Matrix matrix;
-  Display *display;
-  XftFont *xftfont;
-  unsigned x_display_id;
-};
-
 /* Structure pointed by (struct face *)->extra  */
 
 struct xftface_info
@@ -255,7 +234,7 @@ xftfont_open (struct frame *f, Lisp_Object entity, int pixel_size)
   Display *display = FRAME_X_DISPLAY (f);
   Lisp_Object val, filename, idx, font_object;
   FcPattern *pat = NULL, *match;
-  struct xftfont_info *xftfont_info = NULL;
+  struct font_info *xftfont_info = NULL;
   struct font *font;
   double size = 0;
   XftFont *xftfont = NULL;
@@ -330,7 +309,7 @@ xftfont_open (struct frame *f, Lisp_Object entity, int pixel_size)
 
   /* We should not destroy PAT here because it is kept in XFTFONT and
      destroyed automatically when XFTFONT is closed.  */
-  font_object = font_build_object (VECSIZE (struct xftfont_info),
+  font_object = font_build_object (VECSIZE (struct font_info),
    Qxft, entity, size);
   ASET (font_object, FONT_FILE_INDEX, filename);
   font = XFONT_OBJECT (font_object);
@@ -338,7 +317,7 @@ xftfont_open (struct frame *f, Lisp_Object entity, int pixel_size)
   font->driver = &xftfont_driver;
   font->encoding_charset = font->repertory_charset = -1;
 
-  xftfont_info = (struct xftfont_info *) font;
+  xftfont_info = (struct font_info *) font;
   xftfont_info->display = display;
   xftfont_info->xftfont = xftfont;
   xftfont_info->x_display_id = FRAME_DISPLAY_INFO (f)->x_id;
@@ -460,7 +439,7 @@ static void
 xftfont_close (struct font *font)
 {
   struct x_display_info *xdi;
-  struct xftfont_info *xftfont_info = (struct xftfont_info *) font;
+  struct font_info *xftfont_info = (struct font_info *) font;
 
 #ifdef HAVE_LIBOTF
   if (xftfont_info->otf)
@@ -526,7 +505,7 @@ xftfont_done_face (struct frame *f, struct face *face)
 static int
 xftfont_has_char (Lisp_Object font, int c)
 {
-  struct xftfont_info *xftfont_info;
+  struct font_info *xftfont_info;
   struct charset *cs = NULL;
 
   if (EQ (AREF (font, FONT_ADSTYLE_INDEX), Qja)
@@ -540,7 +519,7 @@ xftfont_has_char (Lisp_Object font, int c)
 
   if (FONT_ENTITY_P (font))
     return ftfont_has_char (font, c);
-  xftfont_info = (struct xftfont_info *) XFONT_OBJECT (font);
+  xftfont_info = (struct font_info *) XFONT_OBJECT (font);
   return (XftCharExists (xftfont_info->display, xftfont_info->xftfont,
  (FcChar32) c) == FcTrue);
 }
@@ -548,7 +527,7 @@ xftfont_has_char (Lisp_Object font, int c)
 static unsigned
 xftfont_encode_char (struct font *font, int c)
 {
-  struct xftfont_info *xftfont_info = (struct xftfont_info *) font;
+  struct font_info *xftfont_info = (struct font_info *) font;
   unsigned code = XftCharIndex (xftfont_info->display, xftfont_info->xftfont,
  (FcChar32) c);
 
@@ -559,7 +538,7 @@ static void
 xftfont_text_extents (struct font *font, unsigned int *code,
       int nglyphs, struct font_metrics *metrics)
 {
-  struct xftfont_info *xftfont_info = (struct xftfont_info *) font;
+  struct font_info *xftfont_info = (struct font_info *) font;
   XGlyphInfo extents;
 
   block_input ();
@@ -601,7 +580,7 @@ xftfont_draw (struct glyph_string *s, int from, int to, int x, int y,
 
   struct frame *f = s->f;
   struct face *face = s->face;
-  struct xftfont_info *xftfont_info = (struct xftfont_info *) s->font;
+  struct font_info *xftfont_info = (struct font_info *) s->font;
   struct xftface_info *xftface_info = NULL;
   XftDraw *xft_draw = xftfont_get_xft_draw (f);
   FT_UInt *code;
@@ -664,7 +643,7 @@ static Lisp_Object
 xftfont_shape (Lisp_Object lgstring)
 {
   struct font *font = CHECK_FONT_GET_OBJECT (LGSTRING_FONT (lgstring));
-  struct xftfont_info *xftfont_info = (struct xftfont_info *) font;
+  struct font_info *xftfont_info = (struct font_info *) font;
   FT_Face ft_face = XftLockFace (xftfont_info->xftfont);
   xftfont_info->ft_size = ft_face->size;
   Lisp_Object val = ftfont_shape (lgstring);
@@ -708,7 +687,7 @@ static bool
 xftfont_cached_font_ok (struct frame *f, Lisp_Object font_object,
                         Lisp_Object entity)
 {
-  struct xftfont_info *info = (struct xftfont_info *) XFONT_OBJECT (font_object);
+  struct font_info *info = (struct font_info *) XFONT_OBJECT (font_object);
   FcPattern *oldpat = info->xftfont->pattern;
   Display *display = FRAME_X_DISPLAY (f);
   FcPattern *pat = FcPatternCreate ();

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Eli Zaretskii
> From: Robert Pluim <[hidden email]>
> Cc: [hidden email],  [hidden email]
> Date: Mon, 17 Dec 2018 13:41:11 +0100
>
> >> Aligning ftfont.c and ftcrfont.c is pretty easy. Did you want me to
> >> try to unify xftfont.c in there as well?
> >
> > If that makes sense, i.e. if the commonality seems to justify that,
> > then yes.
> >
>
> I had a quick go at this. Iʼve not measured the memory difference, but
> I can do if people thinks this refactor is worth it. Apart from the
> unification of the three struct definitions, most of the patch is
> mechanical changes.

Thanks, this LGTM, and is a significant improvement, IMO.  If no one
objects in a few days, please push.

P.S.  Will this have any problems with the harfbuzz branch?

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Paul Eggert
In reply to this post by Robert Pluim
Thanks, looks good to me too. One minor point:

On 12/17/18 4:41 AM, Robert Pluim wrote:
> +#ifdef HAVE_XFT
> +#include <X11/Xlib.h>
> +#include <X11/Xft/Xft.h>
> +#endif
> +#ifdef USE_CAIRO
> +#include <cairo-ft.h>
> +#endif

Please indent the include directives by using "# include" (with a space
after "#").


Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Robert Pluim
Paul Eggert <[hidden email]> writes:

> Thanks, looks good to me too. One minor point:
>
> On 12/17/18 4:41 AM, Robert Pluim wrote:
>> +#ifdef HAVE_XFT
>> +#include <X11/Xlib.h>
>> +#include <X11/Xft/Xft.h>
>> +#endif
>> +#ifdef USE_CAIRO
>> +#include <cairo-ft.h>
>> +#endif
>
> Please indent the include directives by using "# include" (with a
> space after "#").

Fixed.

Robert

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Robert Pluim
In reply to this post by Eli Zaretskii
Eli Zaretskii <[hidden email]> writes:

>> From: Robert Pluim <[hidden email]>
>> Cc: [hidden email],  [hidden email]
>> Date: Mon, 17 Dec 2018 13:41:11 +0100
>>
>> >> Aligning ftfont.c and ftcrfont.c is pretty easy. Did you want me to
>> >> try to unify xftfont.c in there as well?
>> >
>> > If that makes sense, i.e. if the commonality seems to justify that,
>> > then yes.
>> >
>>
>> I had a quick go at this. Iʼve not measured the memory difference, but
>> I can do if people thinks this refactor is worth it. Apart from the
>> unification of the three struct definitions, most of the patch is
>> mechanical changes.
>
> Thanks, this LGTM, and is a significant improvement, IMO.  If no one
> objects in a few days, please push.
>
> P.S.  Will this have any problems with the harfbuzz branch?

Apart from having to adjust the addition to the struct in question
done on the harfbuzz branch, I donʼt think so. Probably there will be
a merge conflict, I can help out with that if needed.

Robert

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Robert Pluim
Robert Pluim <[hidden email]> writes:

> Eli Zaretskii <[hidden email]> writes:
>>> I had a quick go at this. Iʼve not measured the memory difference, but
>>> I can do if people thinks this refactor is worth it. Apart from the
>>> unification of the three struct definitions, most of the patch is
>>> mechanical changes.
>>

Given that my cairo build of emacs opens only 4 fonts on startup, and
the freetype one only 2, I think the memory issue is nonexistent.

>> Thanks, this LGTM, and is a significant improvement, IMO.  If no one
>> objects in a few days, please push.
>>
>> P.S.  Will this have any problems with the harfbuzz branch?
>
> Apart from having to adjust the addition to the struct in question
> done on the harfbuzz branch, I donʼt think so. Probably there will be
> a merge conflict, I can help out with that if needed.

Having looked closer, there might be a few more conflicts, but nothing
too complicated to handle. Mainly a few instances of struct
ftfont_info -> struct font_info

Robert

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Robert Pluim
In reply to this post by Robert Pluim
Robert Pluim <[hidden email]> writes:

> Paul Eggert <[hidden email]> writes:
>
>> Thanks, looks good to me too. One minor point:
>>
>> On 12/17/18 4:41 AM, Robert Pluim wrote:
>>> +#ifdef HAVE_XFT
>>> +#include <X11/Xlib.h>
>>> +#include <X11/Xft/Xft.h>
>>> +#endif
>>> +#ifdef USE_CAIRO
>>> +#include <cairo-ft.h>
>>> +#endif
>>
>> Please indent the include directives by using "# include" (with a
>> space after "#").
>
> Fixed.

BTW, Emacs has ~500 instances of this rule not being followed. Iʼm
assuming that a mass change (similarly to mass whitespace fixes) would
be frowned upon.

Robert

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Eli Zaretskii
> From: Robert Pluim <[hidden email]>
> Date: Wed, 19 Dec 2018 09:32:52 +0100
>
> >> On 12/17/18 4:41 AM, Robert Pluim wrote:
> >>> +#ifdef HAVE_XFT
> >>> +#include <X11/Xlib.h>
> >>> +#include <X11/Xft/Xft.h>
> >>> +#endif
> >>> +#ifdef USE_CAIRO
> >>> +#include <cairo-ft.h>
> >>> +#endif
> >>
> >> Please indent the include directives by using "# include" (with a
> >> space after "#").
> >
> > Fixed.
>
> BTW, Emacs has ~500 instances of this rule not being followed. Iʼm
> assuming that a mass change (similarly to mass whitespace fixes) would
> be frowned upon.

We usually fix those as part of other changes, not as a changeset in
itself.

Thanks.

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Robert Pluim
In reply to this post by Eli Zaretskii
Eli Zaretskii <[hidden email]> writes:
>
> Thanks, this LGTM, and is a significant improvement, IMO.  If no one
> objects in a few days, please push.

I built this on GNU/Linux with the following configs:

./configure                # This builds xfont.o ftfont.o xftfont.o ftxfont.o
./configure --with-cairo   # This builds ftfont.o ftcrfont.o
./configure --without-xft  # This builds xfont.o only

Pushed to emacs-26 as

Just kidding. Pushed to master as

I can cherry-pick this to the harfbuzz branch if you want.

Robert

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Robert Pluim
Robert Pluim <[hidden email]> writes:

> Eli Zaretskii <[hidden email]> writes:
>>
>> Thanks, this LGTM, and is a significant improvement, IMO.  If no one
>> objects in a few days, please push.
>
> I built this on GNU/Linux with the following configs:
>
> ./configure                # This builds xfont.o ftfont.o xftfont.o ftxfont.o
> ./configure --with-cairo   # This builds ftfont.o ftcrfont.o
> ./configure --without-xft  # This builds xfont.o only
>
> Pushed to emacs-26 as
>
> Just kidding. Pushed to master as
>
> I can cherry-pick this to the harfbuzz branch if you want.

ENOCOFFEE.

Pushed to master as b0088103bc

Robert

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Robert Pluim
Robert Pluim <[hidden email]> writes:

> Robert Pluim <[hidden email]> writes:
>
>> Eli Zaretskii <[hidden email]> writes:
>>>
>>> Thanks, this LGTM, and is a significant improvement, IMO.  If no one
>>> objects in a few days, please push.
>>
>> I built this on GNU/Linux with the following configs:
>>
>> ./configure                # This builds xfont.o ftfont.o xftfont.o ftxfont.o
>> ./configure --with-cairo   # This builds ftfont.o ftcrfont.o
>> ./configure --without-xft  # This builds xfont.o only
>>
>> Pushed to emacs-26 as
>>
>> Just kidding. Pushed to master as
>>
>> I can cherry-pick this to the harfbuzz branch if you want.
>
> ENOCOFFEE.
>
> Pushed to master as b0088103bc

Uhm 9e0d69b5a1 , I mean.

Robert

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Eli Zaretskii
In reply to this post by Robert Pluim
> From: Robert Pluim <[hidden email]>
> Cc: [hidden email],  [hidden email]
> Date: Fri, 08 Feb 2019 09:01:22 +0100
>
> Eli Zaretskii <[hidden email]> writes:
> >
> > Thanks, this LGTM, and is a significant improvement, IMO.  If no one
> > objects in a few days, please push.
>
> I built this on GNU/Linux with the following configs:
>
> ./configure                # This builds xfont.o ftfont.o xftfont.o ftxfont.o

I wonder why we build both xftfont.o and ftxfont.o, when we only use
one of them, if my reading of the code is correct.

> I can cherry-pick this to the harfbuzz branch if you want.

Thanks, please do.

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Robert Pluim
Eli Zaretskii <[hidden email]> writes:

>> From: Robert Pluim <[hidden email]>
>> Cc: [hidden email],  [hidden email]
>> Date: Fri, 08 Feb 2019 09:01:22 +0100
>>
>> Eli Zaretskii <[hidden email]> writes:
>> >
>> > Thanks, this LGTM, and is a significant improvement, IMO.  If no one
>> > objects in a few days, please push.
>>
>> I built this on GNU/Linux with the following configs:
>>
>> ./configure                # This builds xfont.o ftfont.o xftfont.o ftxfont.o
>
> I wonder why we build both xftfont.o and ftxfont.o, when we only use
> one of them, if my reading of the code is correct.
>

I did suggest removing ftxfont.o, but you said we had to deprecate it
first. Itʼs easy enough to not build in the HAVE_XFT config.

>> I can cherry-pick this to the harfbuzz branch if you want.
>
> Thanks, please do.

Will do.

Robert

Reply | Threaded
Open this post in threaded view
|

Re: harfbuzz 2f72162: Fix crash in the Cairo build

Eli Zaretskii
> From: Robert Pluim <[hidden email]>
> Cc: [hidden email]
> Date: Fri, 08 Feb 2019 12:46:32 +0100
>
> >> ./configure                # This builds xfont.o ftfont.o xftfont.o ftxfont.o
> >
> > I wonder why we build both xftfont.o and ftxfont.o, when we only use
> > one of them, if my reading of the code is correct.
> >
>
> I did suggest removing ftxfont.o, but you said we had to deprecate it
> first. Itʼs easy enough to not build in the HAVE_XFT config.

That's orthogonal.  Regardless of whether we want to drop ftxfont.c, I
don't understand why build with both xftfont.c and ftxfont.c.  They
seem to be mutually exclusive, from the end-user POV.

> >> I can cherry-pick this to the harfbuzz branch if you want.
> >
> > Thanks, please do.
>
> Will do.

TIA

12