OpenType font variations and cairo

Submitted by Matthias Clasen on Sept. 13, 2017, 9:41 p.m.

Details

Message ID CAFwd_vAQqDmW+v3JJ0c3PSrLXyqyYosdUt26VTLoAuKLzwE2XQ@mail.gmail.com
State New
Series "OpenType font variations and cairo"
Headers show

Commit Message

Matthias Clasen Sept. 13, 2017, 9:41 p.m.
Over the last few days, Behdad and I worked together on supporting OpenType
font variations in the Linux text rendering stack. This requires small
additions to pango, fontconfig, and ... cairo.

The cairo patch is simple and ready (i'm still debugging some issues on the
pango side).

It adds a new member to the font options struct, a string which contains
the font variation settings.

Patch hide | download patch | download mbox

From ac6223e62e4b6387b2df84a8cac3ae24802d9b65 Mon Sep 17 00:00:00 2001
From: Matthias Clasen <mclasen@redhat.com>
Date: Tue, 12 Sep 2017 22:19:07 -0400
Subject: [PATCH] Add support for OpenType font variations

Add a font option for OpenType font variations, specified
as a string in the form AXIS1=VALUE,AXIS2=VALUE...

We parse these variation settings when loading freetype
fonts, and pass them on to freetype.
---
 src/cairo-font-options.c  | 89 +++++++++++++++++++++++++++++++++++++++++++++--
 src/cairo-ft-font.c       | 64 +++++++++++++++++++++++++++++++++-
 src/cairo-types-private.h |  1 +
 src/cairo.h               |  7 ++++
 4 files changed, 157 insertions(+), 4 deletions(-)

diff --git a/src/cairo-font-options.c b/src/cairo-font-options.c
index ad28745e6..269a03e67 100644
--- a/src/cairo-font-options.c
+++ b/src/cairo-font-options.c
@@ -55,7 +55,8 @@  static const cairo_font_options_t _cairo_font_options_nil = {
     CAIRO_LCD_FILTER_DEFAULT,
     CAIRO_HINT_STYLE_DEFAULT,
     CAIRO_HINT_METRICS_DEFAULT,
-    CAIRO_ROUND_GLYPH_POS_DEFAULT
+    CAIRO_ROUND_GLYPH_POS_DEFAULT,
+    NULL
 };
 
 /**
@@ -73,6 +74,7 @@  _cairo_font_options_init_default (cairo_font_options_t *options)
     options->hint_style = CAIRO_HINT_STYLE_DEFAULT;
     options->hint_metrics = CAIRO_HINT_METRICS_DEFAULT;
     options->round_glyph_positions = CAIRO_ROUND_GLYPH_POS_DEFAULT;
+    options->variations = NULL;
 }
 
 void
@@ -85,6 +87,7 @@  _cairo_font_options_init_copy (cairo_font_options_t		*options,
     options->hint_style = other->hint_style;
     options->hint_metrics = other->hint_metrics;
     options->round_glyph_positions = other->round_glyph_positions;
+    options->variations = other->variations ? strdup (other->variations) : NULL;
 }
 
 /**
@@ -166,6 +169,7 @@  cairo_font_options_destroy (cairo_font_options_t *options)
     if (cairo_font_options_status (options))
 	return;
 
+    free (options->variations);
     free (options);
 }
 
@@ -226,6 +230,24 @@  cairo_font_options_merge (cairo_font_options_t       *options,
 	options->hint_metrics = other->hint_metrics;
     if (other->round_glyph_positions != CAIRO_ROUND_GLYPH_POS_DEFAULT)
 	options->round_glyph_positions = other->round_glyph_positions;
+
+    if (other->variations) {
+      if (options->variations) {
+        char *p;
+
+        /* 'merge' variations by concatenating - later entries win */
+        p = malloc (strlen (other->variations) + strlen (options->variations) + 2);
+        p[0] = 0;
+        strcat (p, options->variations);
+        strcat (p, ",");
+        strcat (p, other->variations);
+        free (options->variations);
+        options->variations = p;
+      }
+      else {
+        options->variations = strdup (other->variations);
+      }
+    }
 }
 slim_hidden_def (cairo_font_options_merge);
 
@@ -259,10 +281,25 @@  cairo_font_options_equal (const cairo_font_options_t *options,
 	    options->lcd_filter == other->lcd_filter &&
 	    options->hint_style == other->hint_style &&
 	    options->hint_metrics == other->hint_metrics &&
-	    options->round_glyph_positions == other->round_glyph_positions);
+	    options->round_glyph_positions == other->round_glyph_positions &&
+            ((options->variations == NULL && other->variations == NULL) ||
+             (options->variations != NULL && other->variations != NULL &&
+              strcmp (options->variations, other->variations) == 0)));
 }
 slim_hidden_def (cairo_font_options_equal);
 
+static unsigned long
+_intern_string_hash (const char *str, int len)
+{
+    const signed char *p = (const signed char *) str;
+    unsigned int h = *p;
+
+    for (p += 1; --len; p++)
+        h = (h << 5) - h + *p;
+
+    return h;
+}
+
 /**
  * cairo_font_options_hash:
  * @options: a #cairo_font_options_t
@@ -280,14 +317,19 @@  slim_hidden_def (cairo_font_options_equal);
 unsigned long
 cairo_font_options_hash (const cairo_font_options_t *options)
 {
+    unsigned long hash = 0;
+
     if (cairo_font_options_status ((cairo_font_options_t *) options))
 	options = &_cairo_font_options_nil; /* force default values */
 
+    if (options->variations)
+      hash = _intern_string_hash (options->variations, strlen (options->variations));
+
     return ((options->antialias) |
 	    (options->subpixel_order << 4) |
 	    (options->lcd_filter << 8) |
 	    (options->hint_style << 12) |
-	    (options->hint_metrics << 16));
+	    (options->hint_metrics << 16)) ^ hash;
 }
 slim_hidden_def (cairo_font_options_hash);
 
@@ -533,3 +575,44 @@  cairo_font_options_get_hint_metrics (const cairo_font_options_t *options)
 
     return options->hint_metrics;
 }
+
+/**
+ * cairo_font_options_set_variations:
+ * @options: a #cairo_font_options_t
+ * @variations: the new font variations, or %NULL
+ *
+ * Sets the OpenType font variations for the font options object.
+ * Font variations are specified as a string of the form
+ * AXIS1=VALUE1,AXIS2=VALUE2,... with each AXIS being a 4 character
+ * tag naming a font variation axis such as wdth or wght, and each
+ * VALUE being a floating point number.
+ *
+ * Since: 1.16
+ **/
+void
+cairo_font_options_set_variations (cairo_font_options_t *options,
+                                   const char           *variations)
+{
+  free (options->variations);
+  options->variations = variations ? strdup (variations) : NULL;
+}
+
+/**
+ * cairo_font_options_get_variations:
+ * @options: a #cairo_font_options_t
+ *
+ * Gets the OpenType font variations for the font options object.
+ * See cairo_font_options_set_variations() for details about the
+ * string format.
+ *
+ * Return value: the font variations for the font options object. The
+ *   returned string belongs to the @options and must not be modified.
+ *   It is valid until the @options struct is modified.
+ *
+ * Since: 1.16
+ **/
+const char *
+cairo_font_options_get_variations (cairo_font_options_t *options)
+{
+  return options->variations;
+}
diff --git a/src/cairo-ft-font.c b/src/cairo-ft-font.c
index 3b3087578..50442d600 100644
--- a/src/cairo-ft-font.c
+++ b/src/cairo-ft-font.c
@@ -1714,6 +1714,7 @@  _get_pattern_ft_options (FcPattern *pattern, cairo_ft_options_t *ret)
 #ifdef FC_HINT_STYLE
     int hintstyle;
 #endif
+    char *variations;
 
     _cairo_font_options_init_default (&ft_options.base);
     ft_options.load_flags = FT_LOAD_DEFAULT;
@@ -1855,6 +1856,13 @@  _get_pattern_ft_options (FcPattern *pattern, cairo_ft_options_t *ret)
     if (embolden)
 	ft_options.synth_flags |= CAIRO_FT_SYNTHESIZE_BOLD;
 
+#ifndef FC_FONT_VARIATIONS
+#define FC_FONT_VARIATIONS "fontvariations"
+#endif
+    if (FcPatternGetString (pattern, FC_FONT_VARIATIONS, 0, (FcChar8 **) &variations) == FcResultMatch) {
+      ft_options.base.variations = strdup (variations);
+    }
+
     *ret = ft_options;
 }
 #endif
@@ -3616,6 +3624,57 @@  cairo_ft_font_face_get_synthesize (cairo_font_face_t *font_face)
     return ft->ft_options.synth_flags;
 }
 
+static void
+cairo_ft_apply_variations (FT_Face     face,
+                           const char *variations)
+{
+    FT_MM_Var *ft_mm_var;
+    FT_Error ret;
+
+    if (!variations || !variations[0])
+        return;
+
+    ret = FT_Get_MM_Var (face, &ft_mm_var);
+    if (ret == 0) {
+        FT_Fixed *coords;
+        unsigned int i;
+        char *p;
+
+        coords = malloc (sizeof (FT_Fixed) * ft_mm_var->num_axis);
+
+        for (i = 0; i < ft_mm_var->num_axis; i++)
+            coords[i] = ft_mm_var->axis[i].def;
+
+        p = variations;
+        while (p && *p) {
+            char *end, *end2;
+            FT_ULong tag;
+            float value;
+
+            end = strchr (p, ',');
+            if (end && (end - p < 6) || p[4] != '=')
+                goto skip;
+
+            tag = (((uint8_t)p[0])<<24)|(((uint8_t)p[1])<<16)|(((uint8_t)p[2])<<8)|((uint8_t)p[3]);
+            value = strtod (p + 5, &end2);
+            if (end2 < end)
+                goto skip;
+
+            for (i = 0; i < ft_mm_var->num_axis; i++) {
+                if (ft_mm_var->axis[i].tag == tag) {
+                    coords[i] = (FT_Fixed)(value*65536);
+                    break;
+                }
+            }
+skip:
+            p = end ? end + 1 : NULL;
+        }
+
+        FT_Set_Var_Design_Coordinates (face, ft_mm_var->num_axis, coords);
+        free (coords);
+    }
+}
+
 /**
  * cairo_ft_scaled_font_lock_face:
  * @scaled_font: A #cairo_scaled_font_t from the FreeType font backend. Such an
@@ -3624,7 +3683,8 @@  cairo_ft_font_face_get_synthesize (cairo_font_face_t *font_face)
  *   cairo_ft_font_face_create_for_ft_face()).
  *
  * cairo_ft_scaled_font_lock_face() gets the #FT_Face object from a FreeType
- * backend font and scales it appropriately for the font. You must
+ * backend font and scales it appropriately for the font and applies OpenType
+ * font variations if applicable. You must
  * release the face with cairo_ft_scaled_font_unlock_face()
  * when you are done using it.  Since the #FT_Face object can be
  * shared between multiple #cairo_scaled_font_t objects, you must not
@@ -3677,6 +3737,8 @@  cairo_ft_scaled_font_lock_face (cairo_scaled_font_t *abstract_font)
 	return NULL;
     }
 
+    cairo_ft_apply_variations (face, scaled_font->base.options.variations);
+
     /* Note: We deliberately release the unscaled font's mutex here,
      * so that we are not holding a lock across two separate calls to
      * cairo function, (which would give the application some
diff --git a/src/cairo-types-private.h b/src/cairo-types-private.h
index 3d15d968e..da373030c 100644
--- a/src/cairo-types-private.h
+++ b/src/cairo-types-private.h
@@ -194,6 +194,7 @@  struct _cairo_font_options {
     cairo_hint_style_t hint_style;
     cairo_hint_metrics_t hint_metrics;
     cairo_round_glyph_positions_t round_glyph_positions;
+    char *variations;
 };
 
 struct _cairo_glyph_text_info {
diff --git a/src/cairo.h b/src/cairo.h
index 32fc88b17..f8ab1d306 100644
--- a/src/cairo.h
+++ b/src/cairo.h
@@ -1430,6 +1430,13 @@  cairo_font_options_set_hint_metrics (cairo_font_options_t *options,
 cairo_public cairo_hint_metrics_t
 cairo_font_options_get_hint_metrics (const cairo_font_options_t *options);
 
+cairo_public const char *
+cairo_font_options_get_variations (cairo_font_options_t *options);
+
+cairo_public void
+cairo_font_options_set_variations (cairo_font_options_t *options,
+                                   const char           *variations);
+
 /* This interface is for dealing with text as text, not caring about the
    font object inside the the cairo_t. */
 
-- 
2.14.1


Comments

Adrian Johnson Sept. 13, 2017, 10:04 p.m.
On 14/09/17 07:11, Matthias Clasen wrote:
> Over the last few days, Behdad and I worked together on supporting
> OpenType font variations in the Linux text rendering stack. This
> requires small additions to pango, fontconfig, and ... cairo.
> 
> The cairo patch is simple and ready (i'm still debugging some issues on
> the pango side).
> 
> It adds a new member to the font options struct, a string which contains
> the font variation settings.
> 
> 

> + * Sets the OpenType font variations for the font options object.
> + * Font variations are specified as a string of the form
> + * AXIS1=VALUE1,AXIS2=VALUE2,... with each AXIS being a 4 character
> + * tag naming a font variation axis such as wdth or wght, and each
> + * VALUE being a floating point number.

cairo_tag_begin() attributes uses space instead of comma to separate a
list of key=value. Maybe we could try to be consistent?
Matthias Clasen Sept. 14, 2017, 3:11 p.m.
On Wed, Sep 13, 2017 at 6:04 PM, Adrian Johnson <ajohnson@redneon.com>
wrote:

> On 14/09/17 07:11, Matthias Clasen wrote:
> > Over the last few days, Behdad and I worked together on supporting
> > OpenType font variations in the Linux text rendering stack. This
> > requires small additions to pango, fontconfig, and ... cairo.
> >
> > The cairo patch is simple and ready (i'm still debugging some issues on
> > the pango side).
> >
> > It adds a new member to the font options struct, a string which contains
> > the font variation settings.
> >
> >
>
> > + * Sets the OpenType font variations for the font options object.
> > + * Font variations are specified as a string of the form
> > + * AXIS1=VALUE1,AXIS2=VALUE2,... with each AXIS being a 4 character
> > + * tag naming a font variation axis such as wdth or wght, and each
> > + * VALUE being a floating point number.
>
> cairo_tag_begin() attributes uses space instead of comma to separate a
> list of key=value. Maybe we could try to be consistent?
>

I don't have a strong opinion on this. The current format is convenient,
since we can pass it through directly from PangoFontDescription. A
space-separated format would be harder to integrate there.

The pango patch I mentioned is working now, you can find it here:
https://bugzilla.gnome.org/show_bug.cgi?id=786016
Behdad Esfahbod Sept. 14, 2017, 3:23 p.m.
On HarfBuzz side we accept both space and comma.  CSS uses space.  I
suggest accepting both.

On Thu, Sep 14, 2017 at 8:11 AM, Matthias Clasen <matthias.clasen@gmail.com>
wrote:

>
>
> On Wed, Sep 13, 2017 at 6:04 PM, Adrian Johnson <ajohnson@redneon.com>
> wrote:
>
>> On 14/09/17 07:11, Matthias Clasen wrote:
>> > Over the last few days, Behdad and I worked together on supporting
>> > OpenType font variations in the Linux text rendering stack. This
>> > requires small additions to pango, fontconfig, and ... cairo.
>> >
>> > The cairo patch is simple and ready (i'm still debugging some issues on
>> > the pango side).
>> >
>> > It adds a new member to the font options struct, a string which contains
>> > the font variation settings.
>> >
>> >
>>
>> > + * Sets the OpenType font variations for the font options object.
>> > + * Font variations are specified as a string of the form
>> > + * AXIS1=VALUE1,AXIS2=VALUE2,... with each AXIS being a 4 character
>> > + * tag naming a font variation axis such as wdth or wght, and each
>> > + * VALUE being a floating point number.
>>
>> cairo_tag_begin() attributes uses space instead of comma to separate a
>> list of key=value. Maybe we could try to be consistent?
>>
>
> I don't have a strong opinion on this. The current format is convenient,
> since we can pass it through directly from PangoFontDescription. A
> space-separated format would be harder to integrate there.
>
> The pango patch I mentioned is working now, you can find it here:
> https://bugzilla.gnome.org/show_bug.cgi?id=786016
>
>
Behdad Esfahbod Sept. 14, 2017, 4:37 p.m.
Thanks Matthias.  I took a look at the patch.  Looks good!  Minor comments:

- Moving the format towards CSS by default (no comma, no equal sign; just
spaces!) while accepting both is what I do in HarfBuzz. Maybe advertise the
same here,  UPDATE: I was wrong.  CSS uses comma as well.  It's the equal
sign that they don't use, they use space.  We should accept both.

- Maybe make the variations member be "const char *" instead of "char *".

- In +cairo_font_options_set_variations(), typically in these kinds of
functions I like copying then freeing the old value, such that the input
can be the return value of get_variations() on the same object.

-
(((uint8_t)p[0])<<24)|(((uint8_t)p[1])<<16)|(((uint8_t)p[2])<<8)|((uint8_t)p[3]);
FreeType has a macro for that: FT_MAKE_TAG()

Looks really good otherwise.  Let's get this in quickly.

Cheers,
b

(Generally I find it easier pushing a tree on github and reviewing there;
also because I can just fix stuff and push my own tree.)

On Wed, Sep 13, 2017 at 2:41 PM, Matthias Clasen <matthias.clasen@gmail.com>
wrote:

> Over the last few days, Behdad and I worked together on supporting
> OpenType font variations in the Linux text rendering stack. This requires
> small additions to pango, fontconfig, and ... cairo.
>
> The cairo patch is simple and ready (i'm still debugging some issues on
> the pango side).
>
> It adds a new member to the font options struct, a string which contains
> the font variation settings.
>
Matthias Clasen Sept. 14, 2017, 8:22 p.m.
On Thu, Sep 14, 2017 at 12:37 PM, Behdad Esfahbod <behdad.esfahbod@gmail.com
> wrote:

> Thanks Matthias.  I took a look at the patch.  Looks good!  Minor comments:
>
> - Moving the format towards CSS by default (no comma, no equal sign; just
> spaces!) while accepting both is what I do in HarfBuzz. Maybe advertise the
> same here,  UPDATE: I was wrong.  CSS uses comma as well.  It's the equal
> sign that they don't use, they use space.  We should accept both.
>

I've made the parser a bit more flexible. It now accepts variations like

wdth=200,wght=300
wdth 200, wght=300
wdth 200 , wght 300


- Maybe make the variations member be "const char *" instead of "char *".
>

But I'm strdup'ing and freeing it. The const will get in the way there.


> - In +cairo_font_options_set_variations(), typically in these kinds of
> functions I like copying then freeing the old value, such that the input
> can be the return value of get_variations() on the same object.
>

Done


> - (((uint8_t)p[0])<<24)|(((uint8_t)p[1])<<16)|(((uint8_t)
> p[2])<<8)|((uint8_t)p[3]);
> FreeType has a macro for that: FT_MAKE_TAG()
>

Thanks for the reminder. Done.

(Generally I find it easier pushing a tree on github and reviewing there;
> also because I can just fix stuff and push my own tree.)
>
>
You can find the new version here:

https://github.com/matthiasclasen/cairo/commits/wip/matthiasc/font-variations
Adrian Johnson Sept. 14, 2017, 9:54 p.m.
On 15/09/17 05:52, Matthias Clasen wrote:
> On Thu, Sep 14, 2017 at 12:37 PM, Behdad Esfahbod
> <behdad.esfahbod@gmail.com <mailto:behdad.esfahbod@gmail.com>> wrote:
> 
>     Thanks Matthias.  I took a look at the patch.  Looks good!  Minor
>     comments:
> 
>     - Moving the format towards CSS by default (no comma, no equal sign;
>     just spaces!) while accepting both is what I do in HarfBuzz. Maybe
>     advertise the same here,  UPDATE: I was wrong.  CSS uses comma as
>     well.  It's the equal sign that they don't use, they use space.  We
>     should accept both.
> 
> 
> I've made the parser a bit more flexible. It now accepts variations like
> 
> wdth=200,wght=300
> wdth 200, wght=300
> wdth 200 , wght 300

Does it work with decimal commas?

> 
> 
>     - Maybe make the variations member be "const char *" instead of
>     "char *".
> 
> 
> But I'm strdup'ing and freeing it. The const will get in the way there.
>  
> 
>     - In +cairo_font_options_set_variations(), typically in these kinds
>     of functions I like copying then freeing the old value, such that
>     the input can be the return value of get_variations() on the same
>     object.
> 
> 
> Done
>  
> 
>     -
>     (((uint8_t)p[0])<<24)|(((uint8_t)p[1])<<16)|(((uint8_t)p[2])<<8)|((uint8_t)p[3]); 
>     FreeType has a macro for that: FT_MAKE_TAG()
> 
> 
> Thanks for the reminder. Done.
> 
>     (Generally I find it easier pushing a tree on github and reviewing
>     there; also because I can just fix stuff and push my own tree.)
> 
> 
> You can find the new version here:
> 
> https://github.com/matthiasclasen/cairo/commits/wip/matthiasc/font-variations
> 
> 
>
Behdad Esfahbod Sept. 14, 2017, 11:59 p.m.
On Thu, Sep 14, 2017 at 2:54 PM, Adrian Johnson <ajohnson@redneon.com>
wrote:

> On 15/09/17 05:52, Matthias Clasen wrote:
> > On Thu, Sep 14, 2017 at 12:37 PM, Behdad Esfahbod
> > <behdad.esfahbod@gmail.com <mailto:behdad.esfahbod@gmail.com>> wrote:
> >
> >     Thanks Matthias.  I took a look at the patch.  Looks good!  Minor
> >     comments:
> >
> >     - Moving the format towards CSS by default (no comma, no equal sign;
> >     just spaces!) while accepting both is what I do in HarfBuzz. Maybe
> >     advertise the same here,  UPDATE: I was wrong.  CSS uses comma as
> >     well.  It's the equal sign that they don't use, they use space.  We
> >     should accept both.
> >
> >
> > I've made the parser a bit more flexible. It now accepts variations like
> >
> > wdth=200,wght=300
> > wdth 200, wght=300
> > wdth 200 , wght 300
>
> Does it work with decimal commas?
>

Good point.  HarfBuzz has same problem.  glib has g_ascii_strtod() that
always uses '.' as decimal separator.  We need something like that in cairo
and HarfBuzz.  Such a pain :(.

  https://git.gnome.org/browse/glib/tree/glib/gstrfuncs.c#n687


>
> >
> >     - Maybe make the variations member be "const char *" instead of
> >     "char *".
> >
> >
> > But I'm strdup'ing and freeing it. The const will get in the way there.
> >
> >
> >     - In +cairo_font_options_set_variations(), typically in these kinds
> >     of functions I like copying then freeing the old value, such that
> >     the input can be the return value of get_variations() on the same
> >     object.
> >
> >
> > Done
> >
> >
> >     -
> >     (((uint8_t)p[0])<<24)|(((uint8_t)p[1])<<16)|(((uint8_t)
> p[2])<<8)|((uint8_t)p[3]);
> >     FreeType has a macro for that: FT_MAKE_TAG()
> >
> >
> > Thanks for the reminder. Done.
> >
> >     (Generally I find it easier pushing a tree on github and reviewing
> >     there; also because I can just fix stuff and push my own tree.)
> >
> >
> > You can find the new version here:
> >
> > https://github.com/matthiasclasen/cairo/commits/
> wip/matthiasc/font-variations
> >
> >
> >
>
>
Matthias Clasen Sept. 14, 2017, 11:59 p.m.
On Thu, Sep 14, 2017 at 5:54 PM, Adrian Johnson <ajohnson@redneon.com>
wrote:

> On 15/09/17 05:52, Matthias Clasen wrote:
> > On Thu, Sep 14, 2017 at 12:37 PM, Behdad Esfahbod
> > <behdad.esfahbod@gmail.com <mailto:behdad.esfahbod@gmail.com>> wrote:
> >
> >     Thanks Matthias.  I took a look at the patch.  Looks good!  Minor
> >     comments:
> >
> >     - Moving the format towards CSS by default (no comma, no equal sign;
> >     just spaces!) while accepting both is what I do in HarfBuzz. Maybe
> >     advertise the same here,  UPDATE: I was wrong.  CSS uses comma as
> >     well.  It's the equal sign that they don't use, they use space.  We
> >     should accept both.
> >
> >
> > I've made the parser a bit more flexible. It now accepts variations like
> >
> > wdth=200,wght=300
> > wdth 200, wght=300
> > wdth 200 , wght 300
>
> Does it work with decimal commas?
>

The patch currently uses strtod, so yes, it will parse floating point
numbers, but decimal separators spell trouble.
I could rewrite it to use strtod_l, if that is ok to use in cairo.
Behdad Esfahbod Sept. 15, 2017, midnight
Another issue:

+static void
+cairo_ft_apply_variations (FT_Face     face,
+                           const char *variations)
+{
+    FT_MM_Var *ft_mm_var;
+    FT_Error ret;
+
+    if (!variations || !variations[0])
+        return;
+
+    ret = FT_Get_MM_Var (face, &ft_mm_var);

This fails to reset the variations on the face, if variations is NULL or
empty.

On Wed, Sep 13, 2017 at 2:41 PM, Matthias Clasen <matthias.clasen@gmail.com>
wrote:

> Over the last few days, Behdad and I worked together on supporting
> OpenType font variations in the Linux text rendering stack. This requires
> small additions to pango, fontconfig, and ... cairo.
>
> The cairo patch is simple and ready (i'm still debugging some issues on
> the pango side).
>
> It adds a new member to the font options struct, a string which contains
> the font variation settings.
>
Behdad Esfahbod Sept. 15, 2017, 12:02 a.m.
Yeah, I think a configure.ac check and strtod_l() is best.  I'll do that in
HarfBuzz.

On Thu, Sep 14, 2017 at 4:59 PM, Matthias Clasen <matthias.clasen@gmail.com>
wrote:

> On Thu, Sep 14, 2017 at 5:54 PM, Adrian Johnson <ajohnson@redneon.com>
> wrote:
>
>> On 15/09/17 05:52, Matthias Clasen wrote:
>> > On Thu, Sep 14, 2017 at 12:37 PM, Behdad Esfahbod
>> > <behdad.esfahbod@gmail.com <mailto:behdad.esfahbod@gmail.com>> wrote:
>> >
>> >     Thanks Matthias.  I took a look at the patch.  Looks good!  Minor
>> >     comments:
>> >
>> >     - Moving the format towards CSS by default (no comma, no equal sign;
>> >     just spaces!) while accepting both is what I do in HarfBuzz. Maybe
>> >     advertise the same here,  UPDATE: I was wrong.  CSS uses comma as
>> >     well.  It's the equal sign that they don't use, they use space.  We
>> >     should accept both.
>> >
>> >
>> > I've made the parser a bit more flexible. It now accepts variations like
>> >
>> > wdth=200,wght=300
>> > wdth 200, wght=300
>> > wdth 200 , wght 300
>>
>> Does it work with decimal commas?
>>
>
> The patch currently uses strtod, so yes, it will parse floating point
> numbers, but decimal separators spell trouble.
> I could rewrite it to use strtod_l, if that is ok to use in cairo.
>
> --
> cairo mailing list
> cairo@cairographics.org
> https://lists.cairographics.org/mailman/listinfo/cairo
>
Matthias Clasen Sept. 15, 2017, 12:45 a.m.
On Thu, Sep 14, 2017 at 8:00 PM, Behdad Esfahbod <behdad@behdad.org> wrote:

> Another issue:
>
> +static void
> +cairo_ft_apply_variations (FT_Face     face,
> +                           const char *variations)
> +{
> +    FT_MM_Var *ft_mm_var;
> +    FT_Error ret;
> +
> +    if (!variations || !variations[0])
> +        return;
> +
> +    ret = FT_Get_MM_Var (face, &ft_mm_var);
>
> This fails to reset the variations on the face, if variations is NULL or
> empty.
>

Fixed this one.
Matthias Clasen Sept. 15, 2017, 2:08 a.m.
On Thu, Sep 14, 2017 at 8:02 PM, Behdad Esfahbod <behdad.esfahbod@gmail.com>
wrote:

> Yeah, I think a configure.ac check and strtod_l() is best.  I'll do that
> in HarfBuzz.
>
>
I've put the equivalent code in my cairo branch as well now.
Adrian Johnson Sept. 15, 2017, 10:51 a.m.
On 15/09/17 09:29, Matthias Clasen wrote:
> On Thu, Sep 14, 2017 at 5:54 PM, Adrian Johnson <ajohnson@redneon.com
> <mailto:ajohnson@redneon.com>> wrote:
> 
>     On 15/09/17 05:52, Matthias Clasen wrote:
>     > On Thu, Sep 14, 2017 at 12:37 PM, Behdad Esfahbod
>     > <behdad.esfahbod@gmail.com <mailto:behdad.esfahbod@gmail.com>
>     <mailto:behdad.esfahbod@gmail.com
>     <mailto:behdad.esfahbod@gmail.com>>> wrote:
>     >
>     >     Thanks Matthias.  I took a look at the patch.  Looks good!  Minor
>     >     comments:
>     >
>     >     - Moving the format towards CSS by default (no comma, no equal sign;
>     >     just spaces!) while accepting both is what I do in HarfBuzz. Maybe
>     >     advertise the same here,  UPDATE: I was wrong.  CSS uses comma as
>     >     well.  It's the equal sign that they don't use, they use space.  We
>     >     should accept both.
>     >
>     >
>     > I've made the parser a bit more flexible. It now accepts variations like
>     >
>     > wdth=200,wght=300
>     > wdth 200, wght=300
>     > wdth 200 , wght 300
> 
>     Does it work with decimal commas?
> 
> 
> The patch currently uses strtod, so yes, it will parse floating point
> numbers, but decimal separators spell trouble.
> I could rewrite it to use strtod_l, if that is ok to use in cairo.

So what happens on platforms where strtod_l is not available?

The function decode_real() in cairo-cff-subset.c has code for locale
independent parsing of floating-point numbers. Maybe that could be
factored out into separate function and reused.

Where is the variations text string normally expected to come from? I
can see the code reading the FC_FONT_VARIATIONS property but I can't
find this in the current fontconfig git. Where is this documented?

Is the text string supplied to cairo_font_options_set_variations()
likely to be generated by sprintf or is it always read from an external
source in C locale format?

We really need a test case for this. It is hard to review code I can't test.
Matthias Clasen Sept. 15, 2017, 11:35 a.m.
On Fri, Sep 15, 2017 at 6:51 AM, Adrian Johnson


> Is the text string supplied to cairo_font_options_set_variations()
> likely to be generated by sprintf or is it always read from an external
> source in C locale format?
>

 You have a good point here. The string is expected to be generated
programmatically, as in this example here:

https://git.gnome.org/browse/gtk+/tree/demos/gtk-demo/font_features.c?h=wip/matthiasc/font-variations#n454

I need to fix that code up to use C locale as well.


> We really need a test case for this. It is hard to review code I can't
> test.
>

I don't disagree. As always with testing font-related things, the problem
is to find a suitable
font that can be shipped with the tests...
Adrian Johnson Sept. 15, 2017, 11:55 a.m.
On 15/09/17 21:05, Matthias Clasen wrote:
> On Fri, Sep 15, 2017 at 6:51 AM, Adrian Johnson
> 
> 
>     Is the text string supplied to cairo_font_options_set_variations()
>     likely to be generated by sprintf or is it always read from an external
>     source in C locale format?
> 
> 
>  You have a good point here. The string is expected to be generated
> programmatically, as in this example here:
> 
> https://git.gnome.org/browse/gtk+/tree/demos/gtk-demo/font_features.c?h=wip/matthiasc/font-variations#n454

In cairo_begin_tag() where the attributes are expected to be generated
programmatically, I ignore the locale since the strings as parsed in the
same locale as the printf. That is also why I avoided commas between the
key=value so it will work with decimal commas.

> 
> I need to fix that code up to use C locale as well.
> 
> 
>     We really need a test case for this. It is hard to review code I
>     can't test.
> 
> 
> I don't disagree. As always with testing font-related things, the
> problem is to find a suitable
> font that can be shipped with the tests...
> 

If you don't have a redistributable font, just provide a test that can
be run manually similar to what I did in 23fd706b. Include a comment
explaining where a suitable font could be downloaded from. I used this
font for testing 0fd0fd0a.

https://github.com/adobe-fonts/adobe-variable-font-prototype/releases
Behdad Esfahbod Sept. 15, 2017, 4:09 p.m.
On Fri, Sep 15, 2017 at 4:35 AM, Matthias Clasen <matthias.clasen@gmail.com>
wrote:

> On Fri, Sep 15, 2017 at 6:51 AM, Adrian Johnson
>
> Is the text string supplied to cairo_font_options_set_variations()
>> likely to be generated by sprintf or is it always read from an external
>> source in C locale format?
>>
>
>  You have a good point here. The string is expected to be generated
> programmatically, as in this example here:
>
> https://git.gnome.org/browse/gtk+/tree/demos/gtk-demo/font_
> features.c?h=wip/matthiasc/font-variations#n454
>
> I need to fix that code up to use C locale as well.
>
>
>> We really need a test case for this. It is hard to review code I can't
>> test.
>>
>
> I don't disagree. As always with testing font-related things, the problem
> is to find a suitable
> font that can be shipped with the tests...
>

We have many many fonts:


https://github.com/googlei18n/noto-fonts-alpha/tree/master/from-pipeline/unhinted/variable-ttf

Voto is nice because it has all main three registered axes.  Our FontTools
subsetter can subset varfonts.  Here:

behdad:fonttools 0 (master)$ ./fonttools subset VotoSerifGX.ttf --text=cairo
behdad:fonttools 0 (master)$ ll VotoSerifGX.*
-rw-r--r-- 1 behdad eng   30848 Sep 15 12:08 VotoSerifGX.subset.ttf
-rw-r--r-- 1 behdad eng 2182012 Sep 15 12:07 VotoSerifGX.ttf

Attached.

b
Behdad Esfahbod Sept. 15, 2017, 4:15 p.m.
On Fri, Sep 15, 2017 at 3:51 AM, Adrian Johnson <ajohnson@redneon.com>
wrote:

> On 15/09/17 09:29, Matthias Clasen wrote:
> > On Thu, Sep 14, 2017 at 5:54 PM, Adrian Johnson <ajohnson@redneon.com
> > <mailto:ajohnson@redneon.com>> wrote:
> >
> >     On 15/09/17 05:52, Matthias Clasen wrote:
> >     > On Thu, Sep 14, 2017 at 12:37 PM, Behdad Esfahbod
> >     > <behdad.esfahbod@gmail.com <mailto:behdad.esfahbod@gmail.com>
> >     <mailto:behdad.esfahbod@gmail.com
> >     <mailto:behdad.esfahbod@gmail.com>>> wrote:
> >     >
> >     >     Thanks Matthias.  I took a look at the patch.  Looks good!
> Minor
> >     >     comments:
> >     >
> >     >     - Moving the format towards CSS by default (no comma, no equal
> sign;
> >     >     just spaces!) while accepting both is what I do in HarfBuzz.
> Maybe
> >     >     advertise the same here,  UPDATE: I was wrong.  CSS uses comma
> as
> >     >     well.  It's the equal sign that they don't use, they use
> space.  We
> >     >     should accept both.
> >     >
> >     >
> >     > I've made the parser a bit more flexible. It now accepts
> variations like
> >     >
> >     > wdth=200,wght=300
> >     > wdth 200, wght=300
> >     > wdth 200 , wght 300
> >
> >     Does it work with decimal commas?
> >
> >
> > The patch currently uses strtod, so yes, it will parse floating point
> > numbers, but decimal separators spell trouble.
> > I could rewrite it to use strtod_l, if that is ok to use in cairo.
>
> So what happens on platforms where strtod_l is not available?
>

Then parsing fails.  But then again, this is only hooked up in the FreeType
backend right now...


> The function decode_real() in cairo-cff-subset.c has code for locale
> independent parsing of floating-point numbers. Maybe that could be
> factored out into separate function and reused.
>

Sounds good.

Where is the variations text string normally expected to come from? I
> can see the code reading the FC_FONT_VARIATIONS property but I can't
> find this in the current fontconfig git. Where is this documented?
>

I have a branch that adds that.  I'll finish it today or tomorrow and
post.  You can wait till then.

Is the text string supplied to cairo_font_options_set_variations()
> likely to be generated by sprintf or is it always read from an external
> source in C locale format?
>
> We really need a test case for this. It is hard to review code I can't
> test.
>

In a day or two I'll have cairo piece hooked-up in hb-view, and then cairo
and fontconfig bits in pango-view.  That should simplify testing.
Matthias Clasen Sept. 16, 2017, 1:16 a.m.
On Fri, Sep 15, 2017 at 7:55 AM, Adrian Johnson <ajohnson@redneon.com>
wrote:

> On 15/09/17 21:05, Matthias Clasen wrote:
> > On Fri, Sep 15, 2017 at 6:51 AM, Adrian Johnson
> >
> >
> >     Is the text string supplied to cairo_font_options_set_variations()
> >     likely to be generated by sprintf or is it always read from an
> external
> >     source in C locale format?
> >
> >
> >  You have a good point here. The string is expected to be generated
> > programmatically, as in this example here:
> >
> > https://git.gnome.org/browse/gtk+/tree/demos/gtk-demo/font_f
> eatures.c?h=wip/matthiasc/font-variations#n454
>
> In cairo_begin_tag() where the attributes are expected to be generated
> programmatically, I ignore the locale since the strings as parsed in the
> same locale as the printf. That is also why I avoided commas between the
> key=value so it will work with decimal commas.
>

I don't think thats a great idea here. While the strings are generated at
runtime most of the time, you might as well
specify them in a .ui file, or in css, for that matter. Best to fix the
locale.
Matthias Clasen Sept. 16, 2017, 5:27 p.m.
On Fri, Sep 15, 2017 at 6:51 AM, Adrian Johnson <ajohnson@redneon.com>
wrote:

>
>
> We really need a test case for this. It is hard to review code I can't
> test.
>

I've added a minimal testcase here:

https://github.com/matthiasclasen/cairo/tree/wip/matthiasc/font-variations

It promptly found a freetype bug :-)