OpenType font variations and cairo

Submitted by Adrian Johnson on Sept. 17, 2017, 5:12 a.m.

Details

Message ID e0137b21-cdd2-b108-428c-55093be27164@redneon.com
State New
Headers show
Series "OpenType font variations and cairo" ( rev: 2 ) in Cairo

Not browsing as part of any series.

Commit Message

Adrian Johnson Sept. 17, 2017, 5:12 a.m.
On 17/09/17 02:57, Matthias Clasen wrote:
> On Fri, Sep 15, 2017 at 6:51 AM, Adrian Johnson <ajohnson@redneon.com
> <mailto: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 :-)

I built the latest freetype, Behdad's fontconfig branch, and your branch.

First problem I had was the variation test ignores the status. See
attached patch.

After fixing that and ensuring I had the correct font installed I got a
crash in fontconfig.

  fcfreetype.c line 1313 divide by zero

1312: double default_value = master->axis[i].def / (double) (1 << 16);
1313: double mult = value / default_value;

The Adobe Variable Font Prototype font has a 'CNTR' axis with a default
value of 0.

Some more comments on your patch:

In cairo-font-options.c you are adding a new function
_intern_string_hash() which is almost identical to the
_intern_string_hash() in cairo-misc.c Any reason why the cairo-misc.c
version can't be used?

I've attached a patch that refactors the float parsing code from
cff-subset into a new function: _cairo_strtod(). So you can now use this
for a C locale strtod.

In cairo_ft_apply_variations(), strtod() returns a double so just make
'value' have type double instead of float.

+/**
+ * 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.

Should this be "It is valid until either the font options object is
destroyed or the font variations in this object is modified with
cairo_font_options_set_variations()"?

Patch hide | download patch | download mbox

From 315cf7b79b84a8b5763542a98e470727484112ff Mon Sep 17 00:00:00 2001
From: Adrian Johnson <ajohnson@redneon.com>
Date: Sun, 17 Sep 2017 14:15:25 +0930
Subject: [PATCH] factor out ascii to double code in cff-subset into
 _cairo_strtod

---
 src/cairo-cff-subset.c    | 27 +++------------------
 src/cairo-misc.c          | 60 +++++++++++++++++++++++++++++++++++++++++++++--
 src/cairo-output-stream.c |  2 +-
 src/cairo-type1-subset.c  |  2 +-
 src/cairoint.h            |  5 +++-
 5 files changed, 67 insertions(+), 29 deletions(-)

diff --git a/src/cairo-cff-subset.c b/src/cairo-cff-subset.c
index 49d981199..13fee5058 100644
--- a/src/cairo-cff-subset.c
+++ b/src/cairo-cff-subset.c
@@ -295,20 +295,11 @@  decode_nibble (int n, char *buf)
 static unsigned char *
 decode_real (unsigned char *p, double *real)
 {
-    const char *decimal_point;
-    int decimal_point_len;
-    int n;
     char buffer[100];
-    char buffer2[200];
-    char *q;
     char *buf = buffer;
     char *buf_end = buffer + sizeof (buffer);
-
-    decimal_point = cairo_get_locale_decimal_point ();
-    decimal_point_len = strlen (decimal_point);
-
-    assert (decimal_point_len != 0);
-    assert (sizeof(buffer) + decimal_point_len < sizeof(buffer2));
+    char *end;
+    int n;
 
     p++;
     while (buf + 2 < buf_end) {
@@ -324,19 +315,7 @@  decode_real (unsigned char *p, double *real)
     };
     *buf = 0;
 
-    buf = buffer;
-    if (strchr (buffer, '.')) {
-	 q = strchr (buffer, '.');
-	 strncpy (buffer2, buffer, q - buffer);
-	 buf = buffer2 + (q - buffer);
-	 strncpy (buf, decimal_point, decimal_point_len);
-	 buf += decimal_point_len;
-	 strcpy (buf, q + 1);
-	 buf = buffer2;
-    }
-
-    if (sscanf(buf, "%lf", real) != 1)
-        *real = 0.0;
+    *real = _cairo_strtod (buffer, &end);
 
     return p;
 }
diff --git a/src/cairo-misc.c b/src/cairo-misc.c
index e9b0ab6be..19629feca 100644
--- a/src/cairo-misc.c
+++ b/src/cairo-misc.c
@@ -771,7 +771,7 @@  _cairo_half_from_float (float f)
 # include <locale.h>
 
 const char *
-cairo_get_locale_decimal_point (void)
+_cairo_get_locale_decimal_point (void)
 {
     struct lconv *locale_data = localeconv ();
     return locale_data->decimal_point;
@@ -780,12 +780,68 @@  cairo_get_locale_decimal_point (void)
 #else
 /* Android's Bionic libc doesn't provide decimal_point */
 const char *
-cairo_get_locale_decimal_point (void)
+_cairo_get_locale_decimal_point (void)
 {
     return ".";
 }
 #endif
 
+/* strtod replacement that ignores locale and only accepts decimal points */
+double
+_cairo_strtod (const char *nptr, char **endptr)
+{
+    const char *decimal_point;
+    int decimal_point_len;
+    const char *p;
+    char buf[100];
+    char *bufptr;
+    char *bufend = buf + sizeof(buf) - 1;
+    double value;
+    char *end;
+    int delta;
+    cairo_bool_t have_dp;
+
+    decimal_point = _cairo_get_locale_decimal_point ();
+    decimal_point_len = strlen (decimal_point);
+    assert (decimal_point_len != 0);
+
+    p = nptr;
+    bufptr = buf;
+    delta = 0;
+    have_dp = FALSE;
+    while (*p && _cairo_isspace (*p)) {
+	p++;
+	delta++;
+    }
+
+    while (*p && (bufptr + decimal_point_len < bufend)) {
+	if (_cairo_isdigit (*p)) {
+	    *bufptr++ = *p;
+	} else if (*p == '.') {
+	    if (have_dp)
+		break;
+	    strncpy (bufptr, decimal_point, decimal_point_len);
+	    bufptr += decimal_point_len;
+	    delta -= decimal_point_len - 1;
+	    have_dp = TRUE;
+	} else {
+	    break;
+	}
+	p++;
+    }
+    *bufptr = 0;
+
+    value = strtod (buf, &end);
+    if (endptr) {
+	if (end == buf)
+	    *endptr = (char*)(nptr);
+	else
+	    *endptr = (char*)(nptr + (end - buf) + delta);
+    }
+
+    return value;
+}
+
 #ifdef _WIN32
 
 #define WIN32_LEAN_AND_MEAN
diff --git a/src/cairo-output-stream.c b/src/cairo-output-stream.c
index 07991703b..76d718aa7 100644
--- a/src/cairo-output-stream.c
+++ b/src/cairo-output-stream.c
@@ -312,7 +312,7 @@  _cairo_dtostr (char *buffer, size_t size, double d, cairo_bool_t limited_precisi
     if (d == 0.0)
 	d = 0.0;
 
-    decimal_point = cairo_get_locale_decimal_point ();
+    decimal_point = _cairo_get_locale_decimal_point ();
     decimal_point_len = strlen (decimal_point);
 
     assert (decimal_point_len != 0);
diff --git a/src/cairo-type1-subset.c b/src/cairo-type1-subset.c
index 810dc9f74..4dd7ac17e 100644
--- a/src/cairo-type1-subset.c
+++ b/src/cairo-type1-subset.c
@@ -311,7 +311,7 @@  cairo_type1_font_subset_get_matrix (cairo_type1_font_subset_t *font,
     const char *decimal_point;
     int decimal_point_len;
 
-    decimal_point = cairo_get_locale_decimal_point ();
+    decimal_point = _cairo_get_locale_decimal_point ();
     decimal_point_len = strlen (decimal_point);
 
     assert (decimal_point_len != 0);
diff --git a/src/cairoint.h b/src/cairoint.h
index f64355921..8573ae616 100644
--- a/src/cairoint.h
+++ b/src/cairoint.h
@@ -913,7 +913,10 @@  cairo_private void
 _cairo_intern_string_reset_static_data (void);
 
 cairo_private const char *
-cairo_get_locale_decimal_point (void);
+_cairo_get_locale_decimal_point (void);
+
+cairo_private double
+_cairo_strtod (const char *nptr, char **endptr);
 
 /* cairo-path-fixed.c */
 cairo_private cairo_path_fixed_t *
-- 
2.11.0


Comments

On Sun, Sep 17, 2017 at 1:12 AM, Adrian Johnson <ajohnson@redneon.com>
wrote:

> On 17/09/17 02:57, Matthias Clasen wrote:
> > On Fri, Sep 15, 2017 at 6:51 AM, Adrian Johnson <ajohnson@redneon.com
> > <mailto: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 :-)
>
> I built the latest freetype, Behdad's fontconfig branch, and your branch.
>
> First problem I had was the variation test ignores the status. See
> attached patch.
>
> After fixing that and ensuring I had the correct font installed I got a
> crash in fontconfig.
>
>   fcfreetype.c line 1313 divide by zero
>
> 1312: double default_value = master->axis[i].def / (double) (1 << 16);
> 1313: double mult = value / default_value;
>
> The Adobe Variable Font Prototype font has a 'CNTR' axis with a default
> value of 0.
>
> Some more comments on your patch:
>
> In cairo-font-options.c you are adding a new function
> _intern_string_hash() which is almost identical to the
> _intern_string_hash() in cairo-misc.c Any reason why the cairo-misc.c
> version can't be used?
>

Sure, thats possible. I just wasn't sure how things work in the cairo tree
wrt to factoring out internal utiltiies,
and wanted to keep this patch self-contained. But I can change things
around and use the same version,
after applying my fix there (the version in cairo-misc.c does not work for
empty strings).

>
> I've attached a patch that refactors the float parsing code from
> cff-subset into a new function: _cairo_strtod(). So you can now use this
> for a C locale strtod.
>

Yay, thanks.It might still be nice to apply the strtod_l patch on top, and
only use that code as fallback.


>
> In cairo_ft_apply_variations(), strtod() returns a double so just make
> 'value' have type double instead of float.
>

Sure.


> +/**
> + * 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.
>
> Should this be "It is valid until either the font options object is
> destroyed or the font variations in this object is modified with
> cairo_font_options_set_variations()"?
>

I guess I was too terse here, better to be explicit indeed.

I've reworked the branch to include all these suggestions, if you want to
have another look.
Fixed the div-by-zero.  Thanks.

On Sat, Sep 16, 2017 at 10:12 PM, Adrian Johnson <ajohnson@redneon.com>
wrote:

> On 17/09/17 02:57, Matthias Clasen wrote:
> > On Fri, Sep 15, 2017 at 6:51 AM, Adrian Johnson <ajohnson@redneon.com
> > <mailto: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 :-)
>
> I built the latest freetype, Behdad's fontconfig branch, and your branch.
>
> First problem I had was the variation test ignores the status. See
> attached patch.
>
> After fixing that and ensuring I had the correct font installed I got a
> crash in fontconfig.
>
>   fcfreetype.c line 1313 divide by zero
>
> 1312: double default_value = master->axis[i].def / (double) (1 << 16);
> 1313: double mult = value / default_value;
>
> The Adobe Variable Font Prototype font has a 'CNTR' axis with a default
> value of 0.
>
> Some more comments on your patch:
>
> In cairo-font-options.c you are adding a new function
> _intern_string_hash() which is almost identical to the
> _intern_string_hash() in cairo-misc.c Any reason why the cairo-misc.c
> version can't be used?
>
> I've attached a patch that refactors the float parsing code from
> cff-subset into a new function: _cairo_strtod(). So you can now use this
> for a C locale strtod.
>
> In cairo_ft_apply_variations(), strtod() returns a double so just make
> 'value' have type double instead of float.
>
> +/**
> + * 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.
>
> Should this be "It is valid until either the font options object is
> destroyed or the font variations in this object is modified with
> cairo_font_options_set_variations()"?
>
On 18/09/17 01:03, Matthias Clasen wrote:
> On Sun, Sep 17, 2017 at 1:12 AM, Adrian Johnson <ajohnson@redneon.com
> <mailto:ajohnson@redneon.com>> wrote:
>     I've attached a patch that refactors the float parsing code from
>     cff-subset into a new function: _cairo_strtod(). So you can now use this
>     for a C locale strtod.
> 
> 
> Yay, thanks.It might still be nice to apply the strtod_l patch on top,
> and only use that code as fallback.

-1. Just use the cairo_strtod all the time. We don't use non ANSI C
functions with #include guarded fallbacks in cairo. It adds to source
code bloat and one side of the #if won't get regularly tested.

> I've reworked the branch to include all these suggestions, if you want
> to have another look.

I pulled your latest changes (a827d7e5) and updated fontconfig to
Behdad's latest changes (ff5d57bd).

I'm getting a test failure running font-variations. The test log is:

axis Weight, value 19660800
axis CNTR, value 0
Axis Weight: not expected value (19660800 != 13107200)

I have tried both the otf and ttf version of the Adobe Variable Font
Prototype v1.003. Same result for both.

I'm not sure what I'm doing wrong. Could the test be sensitive to my
fontconfig configuration? I am using Debian Stretch.

In font-variations.c, FT_Get_Var_Design_Coordinates() will need an
#include guard for FreeType >= 2.8.
On Tue, Sep 19, 2017 at 7:04 AM, Adrian Johnson <ajohnson@redneon.com>
wrote:

>
> On 18/09/17 01:03, Matthias Clasen wrote:
> > On Sun, Sep 17, 2017 at 1:12 AM, Adrian Johnson <ajohnson@redneon.com
> > <mailto:ajohnson@redneon.com>> wrote:
> >     I've attached a patch that refactors the float parsing code from
> >     cff-subset into a new function: _cairo_strtod(). So you can now use
> this
> >     for a C locale strtod.
> >
> >
> > Yay, thanks.It might still be nice to apply the strtod_l patch on top,
> > and only use that code as fallback.
>
> -1. Just use the cairo_strtod all the time. We don't use non ANSI C
> functions with #include guarded fallbacks in cairo. It adds to source
> code bloat and one side of the #if won't get regularly tested.
>
>
I disagree, at least in this case, but your call.


>
> I'm getting a test failure running font-variations. The test log is:
>
> axis Weight, value 19660800
> axis CNTR, value 0
> Axis Weight: not expected value (19660800 != 13107200)
>
> I have tried both the otf and ttf version of the Adobe Variable Font
> Prototype v1.003. Same result for both.
>
> I'm not sure what I'm doing wrong. Could the test be sensitive to my
> fontconfig configuration? I am using Debian Stretch.


It sounds unlikely to me. Maybe Behdads fontconfig patches interfere here?
I've written the test on a system without them.


> In font-variations.c, FT_Get_Var_Design_Coordinates() will need an
> #include guard for FreeType >= 2.8.
>
On 19/09/17 21:02, Matthias Clasen wrote:
> On Tue, Sep 19, 2017 at 7:04 AM, Adrian Johnson <ajohnson@redneon.com
> <mailto:ajohnson@redneon.com>> wrote:
> 
> 
>     On 18/09/17 01:03, Matthias Clasen wrote:
>     > On Sun, Sep 17, 2017 at 1:12 AM, Adrian Johnson <ajohnson@redneon.com <mailto:ajohnson@redneon.com>
>     > <mailto:ajohnson@redneon.com <mailto:ajohnson@redneon.com>>> wrote:
>     >     I've attached a patch that refactors the float parsing code from
>     >     cff-subset into a new function: _cairo_strtod(). So you can now use this
>     >     for a C locale strtod.
>     >
>     >
>     > Yay, thanks.It might still be nice to apply the strtod_l patch on top,
>     > and only use that code as fallback.
> 
>     -1. Just use the cairo_strtod all the time. We don't use non ANSI C
>     functions with #include guarded fallbacks in cairo. It adds to source
>     code bloat and one side of the #if won't get regularly tested.
> 
> 
> I disagree, at least in this case, but your call.

We don't do it that way in cairo. If the fallback works just use it all
the time.

> 
>     I'm getting a test failure running font-variations. The test log is:
> 
>     axis Weight, value 19660800
>     axis CNTR, value 0
>     Axis Weight: not expected value (19660800 != 13107200)
> 
>     I have tried both the otf and ttf version of the Adobe Variable Font
>     Prototype v1.003. Same result for both.
> 
>     I'm not sure what I'm doing wrong. Could the test be sensitive to my
>     fontconfig configuration? I am using Debian Stretch.
> 
> 
> It sounds unlikely to me. Maybe Behdads fontconfig patches interfere here?
> I've written the test on a system without them.

I did some more testing. With my system fontconfig (2.11) the test
failed (same result) when using the ttf font. FC 2.11 did not recognize
the otf font.

I tested fontconfig 2.12.5. The test crashed with both the ttf and otf
fonts due to a divide by zero error.

What version of FC are you using and which font (otf or ttf)?

Is there some printfs you could add to help debug this?

> 
>     In font-variations.c, FT_Get_Var_Design_Coordinates() will need an
>     #include guard for FreeType >= 2.8.
> 
> 
> 
>
On Tue, Sep 19, 2017 at 8:41 AM, Adrian Johnson <ajohnson@redneon.com>
wrote:

> On 19/09/17 21:02, Matthias Clasen wrote:
> > On Tue, Sep 19, 2017 at 7:04 AM, Adrian Johnson <ajohnson@redneon.com
> > <mailto:ajohnson@redneon.com>> wrote:
> >
> >
> >     On 18/09/17 01:03, Matthias Clasen wrote:
> >     > On Sun, Sep 17, 2017 at 1:12 AM, Adrian Johnson <
> ajohnson@redneon.com <mailto:ajohnson@redneon.com>
> >     > <mailto:ajohnson@redneon.com <mailto:ajohnson@redneon.com>>>
> wrote:
> >     >     I've attached a patch that refactors the float parsing code
> from
> >     >     cff-subset into a new function: _cairo_strtod(). So you can
> now use this
> >     >     for a C locale strtod.
> >     >
> >     >
> >     > Yay, thanks.It might still be nice to apply the strtod_l patch on
> top,
> >     > and only use that code as fallback.
> >
> >     -1. Just use the cairo_strtod all the time. We don't use non ANSI C
> >     functions with #include guarded fallbacks in cairo. It adds to source
> >     code bloat and one side of the #if won't get regularly tested.
> >
> >
> > I disagree, at least in this case, but your call.
>
> We don't do it that way in cairo. If the fallback works just use it all
> the time.
>

Fine. Easy enough to drop that commit.


> I did some more testing. With my system fontconfig (2.11) the test
> failed (same result) when using the ttf font. FC 2.11 did not recognize
> the otf font.
>
> I tested fontconfig 2.12.5. The test crashed with both the ttf and otf
> fonts due to a divide by zero error.
>
> What version of FC are you using and which font (otf or ttf)?
>

I have fontconfig 2.12.5 and freetype 2.8 on my system, and I am using the
ttf version of the font.
Behdad, does my test work on your system ?

Is there some printfs you could add to help debug this?
>

 I'll think about it.
On 19/09/17 22:55, Matthias Clasen wrote:
> I have fontconfig 2.12.5 and freetype 2.8 on my system, and I am using
> the ttf version of the font.
> Behdad, does my test work on your system ?
> 
>     Is there some printfs you could add to help debug this?
> 
> 
>  I'll think about it.

I tested with freetype 2.8 (was previously using 2.8.1), fontconfig
2.12.5, and the ttf font. Still getting the same error:

axis Weight, value 19660800
axis CNTR, value 0
Axis Weight: not expected value (19660800 != 13107200)
Can someone summarize what's the status here?  I'm not sure what is being
asked of me.

On Tue, Sep 19, 2017 at 2:02 PM, Adrian Johnson <ajohnson@redneon.com>
wrote:

> On 19/09/17 22:55, Matthias Clasen wrote:
> > I have fontconfig 2.12.5 and freetype 2.8 on my system, and I am using
> > the ttf version of the font.
> > Behdad, does my test work on your system ?
> >
> >     Is there some printfs you could add to help debug this?
> >
> >
> >  I'll think about it.
>
> I tested with freetype 2.8 (was previously using 2.8.1), fontconfig
> 2.12.5, and the ttf font. Still getting the same error:
>
> axis Weight, value 19660800
> axis CNTR, value 0
> Axis Weight: not expected value (19660800 != 13107200)
>
Ok I've got to running the test.  It fails. How do I see the log?  The test
runner looks so foreign to me...

On Wed, Sep 20, 2017 at 12:37 PM, Behdad Esfahbod <behdad.esfahbod@gmail.com
> wrote:

> Can someone summarize what's the status here?  I'm not sure what is being
> asked of me.
>
> On Tue, Sep 19, 2017 at 2:02 PM, Adrian Johnson <ajohnson@redneon.com>
> wrote:
>
>> On 19/09/17 22:55, Matthias Clasen wrote:
>> > I have fontconfig 2.12.5 and freetype 2.8 on my system, and I am using
>> > the ttf version of the font.
>> > Behdad, does my test work on your system ?
>> >
>> >     Is there some printfs you could add to help debug this?
>> >
>> >
>> >  I'll think about it.
>>
>> I tested with freetype 2.8 (was previously using 2.8.1), fontconfig
>> 2.12.5, and the ttf font. Still getting the same error:
>>
>> axis Weight, value 19660800
>> axis CNTR, value 0
>> Axis Weight: not expected value (19660800 != 13107200)
>>
>
>
>
> --
> behdad
> http://behdad.org/
>
test/output/font-variations.log


On 21/09/17 05:55, Behdad Esfahbod wrote:
> Ok I've got to running the test.  It fails. How do I see the log?  The
> test runner looks so foreign to me...
> 
> On Wed, Sep 20, 2017 at 12:37 PM, Behdad Esfahbod
> <behdad.esfahbod@gmail.com <mailto:behdad.esfahbod@gmail.com>> wrote:
> 
>     Can someone summarize what's the status here?  I'm not sure what is
>     being asked of me.
> 
>     On Tue, Sep 19, 2017 at 2:02 PM, Adrian Johnson
>     <ajohnson@redneon.com <mailto:ajohnson@redneon.com>> wrote:
> 
>         On 19/09/17 22:55, Matthias Clasen wrote:
>         > I have fontconfig 2.12.5 and freetype 2.8 on my system, and I am using
>         > the ttf version of the font.
>         > Behdad, does my test work on your system ?
>         >
>         >     Is there some printfs you could add to help debug this?
>         >
>         >
>         >  I'll think about it.
> 
>         I tested with freetype 2.8 (was previously using 2.8.1), fontconfig
>         2.12.5, and the ttf font. Still getting the same error:
> 
>         axis Weight, value 19660800
>         axis CNTR, value 0
>         Axis Weight: not expected value (19660800 != 13107200)
> 
> 
> 
> 
>     -- 
>     behdad
>     http://behdad.org/
> 
> 
> 
> 
> -- 
> behdad
> http://behdad.org/
> 
>
Thanks.  Ok, I get the failure log now.

Matthias, what is the test trying to do anyway?!  It wasn't using the input
argument to test_variation() at all.  If I fix that, then I don't see any
failure text in the log, but the test is still failing.


https://github.com/behdad/cairo/commit/81e8abfaf263445d9ec7e8eb170b1e3ef70e7f98

I don't know how to proceed.

Matthias, your Get_MM thing failing sounds an awful lot like something we
fixed in FreeType last month.  Please check with master.

On Wed, Sep 20, 2017 at 1:43 PM, Adrian Johnson <ajohnson@redneon.com>
wrote:

> test/output/font-variations.log
>
>
> On 21/09/17 05:55, Behdad Esfahbod wrote:
> > Ok I've got to running the test.  It fails. How do I see the log?  The
> > test runner looks so foreign to me...
> >
> > On Wed, Sep 20, 2017 at 12:37 PM, Behdad Esfahbod
> > <behdad.esfahbod@gmail.com <mailto:behdad.esfahbod@gmail.com>> wrote:
> >
> >     Can someone summarize what's the status here?  I'm not sure what is
> >     being asked of me.
> >
> >     On Tue, Sep 19, 2017 at 2:02 PM, Adrian Johnson
> >     <ajohnson@redneon.com <mailto:ajohnson@redneon.com>> wrote:
> >
> >         On 19/09/17 22:55, Matthias Clasen wrote:
> >         > I have fontconfig 2.12.5 and freetype 2.8 on my system, and I
> am using
> >         > the ttf version of the font.
> >         > Behdad, does my test work on your system ?
> >         >
> >         >     Is there some printfs you could add to help debug this?
> >         >
> >         >
> >         >  I'll think about it.
> >
> >         I tested with freetype 2.8 (was previously using 2.8.1),
> fontconfig
> >         2.12.5, and the ttf font. Still getting the same error:
> >
> >         axis Weight, value 19660800
> >         axis CNTR, value 0
> >         Axis Weight: not expected value (19660800 != 13107200)
> >
> >
> >
> >
> >     --
> >     behdad
> >     http://behdad.org/
> >
> >
> >
> >
> > --
> > behdad
> > http://behdad.org/
> >
> >
>
>
Actually, my bad.  Let me debug this.

On Wed, Sep 20, 2017 at 2:55 PM, Behdad Esfahbod <behdad.esfahbod@gmail.com>
wrote:

> Thanks.  Ok, I get the failure log now.
>
> Matthias, what is the test trying to do anyway?!  It wasn't using the
> input argument to test_variation() at all.  If I fix that, then I don't see
> any failure text in the log, but the test is still failing.
>
>   https://github.com/behdad/cairo/commit/81e8abfaf263445d9ec7e8eb170b1e
> 3ef70e7f98
>
> I don't know how to proceed.
>
> Matthias, your Get_MM thing failing sounds an awful lot like something we
> fixed in FreeType last month.  Please check with master.
>
> On Wed, Sep 20, 2017 at 1:43 PM, Adrian Johnson <ajohnson@redneon.com>
> wrote:
>
>> test/output/font-variations.log
>>
>>
>> On 21/09/17 05:55, Behdad Esfahbod wrote:
>> > Ok I've got to running the test.  It fails. How do I see the log?  The
>> > test runner looks so foreign to me...
>> >
>> > On Wed, Sep 20, 2017 at 12:37 PM, Behdad Esfahbod
>> > <behdad.esfahbod@gmail.com <mailto:behdad.esfahbod@gmail.com>> wrote:
>> >
>> >     Can someone summarize what's the status here?  I'm not sure what is
>> >     being asked of me.
>> >
>> >     On Tue, Sep 19, 2017 at 2:02 PM, Adrian Johnson
>> >     <ajohnson@redneon.com <mailto:ajohnson@redneon.com>> wrote:
>> >
>> >         On 19/09/17 22:55, Matthias Clasen wrote:
>> >         > I have fontconfig 2.12.5 and freetype 2.8 on my system, and I
>> am using
>> >         > the ttf version of the font.
>> >         > Behdad, does my test work on your system ?
>> >         >
>> >         >     Is there some printfs you could add to help debug this?
>> >         >
>> >         >
>> >         >  I'll think about it.
>> >
>> >         I tested with freetype 2.8 (was previously using 2.8.1),
>> fontconfig
>> >         2.12.5, and the ttf font. Still getting the same error:
>> >
>> >         axis Weight, value 19660800
>> >         axis CNTR, value 0
>> >         Axis Weight: not expected value (19660800 != 13107200)
>> >
>> >
>> >
>> >
>> >     --
>> >     behdad
>> >     http://behdad.org/
>> >
>> >
>> >
>> >
>> > --
>> > behdad
>> > http://behdad.org/
>> >
>> >
>>
>>
>
>
> --
> behdad
> http://behdad.org/
>
The test was really broken...

Ok, I've pushed some things there that should help Matthias move foward:

  https://github.com/behdad/cairo/commits/font-variations

On Wed, Sep 20, 2017 at 2:56 PM, Behdad Esfahbod <behdad.esfahbod@gmail.com>
wrote:

> Actually, my bad.  Let me debug this.
>
> On Wed, Sep 20, 2017 at 2:55 PM, Behdad Esfahbod <
> behdad.esfahbod@gmail.com> wrote:
>
>> Thanks.  Ok, I get the failure log now.
>>
>> Matthias, what is the test trying to do anyway?!  It wasn't using the
>> input argument to test_variation() at all.  If I fix that, then I don't see
>> any failure text in the log, but the test is still failing.
>>
>>   https://github.com/behdad/cairo/commit/81e8abfaf263445d9ec7e
>> 8eb170b1e3ef70e7f98
>>
>> I don't know how to proceed.
>>
>> Matthias, your Get_MM thing failing sounds an awful lot like something we
>> fixed in FreeType last month.  Please check with master.
>>
>> On Wed, Sep 20, 2017 at 1:43 PM, Adrian Johnson <ajohnson@redneon.com>
>> wrote:
>>
>>> test/output/font-variations.log
>>>
>>>
>>> On 21/09/17 05:55, Behdad Esfahbod wrote:
>>> > Ok I've got to running the test.  It fails. How do I see the log?  The
>>> > test runner looks so foreign to me...
>>> >
>>> > On Wed, Sep 20, 2017 at 12:37 PM, Behdad Esfahbod
>>> > <behdad.esfahbod@gmail.com <mailto:behdad.esfahbod@gmail.com>> wrote:
>>> >
>>> >     Can someone summarize what's the status here?  I'm not sure what is
>>> >     being asked of me.
>>> >
>>> >     On Tue, Sep 19, 2017 at 2:02 PM, Adrian Johnson
>>> >     <ajohnson@redneon.com <mailto:ajohnson@redneon.com>> wrote:
>>> >
>>> >         On 19/09/17 22:55, Matthias Clasen wrote:
>>> >         > I have fontconfig 2.12.5 and freetype 2.8 on my system, and
>>> I am using
>>> >         > the ttf version of the font.
>>> >         > Behdad, does my test work on your system ?
>>> >         >
>>> >         >     Is there some printfs you could add to help debug this?
>>> >         >
>>> >         >
>>> >         >  I'll think about it.
>>> >
>>> >         I tested with freetype 2.8 (was previously using 2.8.1),
>>> fontconfig
>>> >         2.12.5, and the ttf font. Still getting the same error:
>>> >
>>> >         axis Weight, value 19660800
>>> >         axis CNTR, value 0
>>> >         Axis Weight: not expected value (19660800 != 13107200)
>>> >
>>> >
>>> >
>>> >
>>> >     --
>>> >     behdad
>>> >     http://behdad.org/
>>> >
>>> >
>>> >
>>> >
>>> > --
>>> > behdad
>>> > http://behdad.org/
>>> >
>>> >
>>>
>>>
>>
>>
>> --
>> behdad
>> http://behdad.org/
>>
>
>
>
> --
> behdad
> http://behdad.org/
>
On Wed, Sep 20, 2017 at 6:15 PM, Behdad Esfahbod <behdad.esfahbod@gmail.com>
wrote:

> The test was really broken...
>
> Ok, I've pushed some things there that should help Matthias move foward:
>
>   https://github.com/behdad/cairo/commits/font-variations
>
>
Thanks for those fixes, Behdad. Taking another look at the test, I'm not
sure what I was thinking - it was checking the width axis, but the font we
use only has weight and contrast as axes.
i've pushed all the fixes to

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

Does this work for you now, Adrian ?
On 21/09/17 12:03, Matthias Clasen wrote:
> On Wed, Sep 20, 2017 at 6:15 PM, Behdad Esfahbod
> <behdad.esfahbod@gmail.com <mailto:behdad.esfahbod@gmail.com>> wrote:
> 
>     The test was really broken...
> 
>     Ok, I've pushed some things there that should help Matthias move foward:
> 
>       https://github.com/behdad/cairo/commits/font-variations
>     <https://github.com/behdad/cairo/commits/font-variations>
> 
> 
> Thanks for those fixes, Behdad. Taking another look at the test, I'm not
> sure what I was thinking - it was checking the width axis, but the font
> we use only has weight and contrast as axes.
> i've pushed all the fixes to
> 
> https://github.com/matthiasclasen/cairo/tree/wip/matthiasc/font-variations
> 
> Does this work for you now, Adrian ?

It works with freetype 2.8.0, but not 2.8.1. Here are the results of
testing with different combinations of freetype and fontconfig.

freetype 2.8.0, fontconfig 2.11.0 : pass

freetype 2.8.0, fontconfig 2.12.5 : pass

freetype 2.8.0, Behdad's fontconfig (2d0063948a44) : pass

freetype 2.8.1, fontconfig 2.11.0 : fail

  axis Weight, value 300
  axis CNTR, value 0
  axis Weight, value 0
  axis CNTR, value 0
  Axis Weight: not expected value (0 != 300.5)

freetype 2.8.1, fontconfig 2.12.5 : fail

  axis Weight, value 300
  axis CNTR, value 0
  axis Weight, value 0
  axis CNTR, value 0
  Axis Weight: not expected value (0 != 300.5)

freetype 2.8.1, Behdad's fontconfig (2d0063948a44) : fail

  axis Weight, value 300
  axis CNTR, value 0
  axis Weight, value 0
  axis CNTR, value 0
  Axis Weight: not expected value (0 != 300.5)
On Thu, Sep 21, 2017 at 5:42 AM, Adrian Johnson <ajohnson@redneon.com>
wrote:

> On 21/09/17 12:03, Matthias Clasen wrote:
> > On Wed, Sep 20, 2017 at 6:15 PM, Behdad Esfahbod
> > <behdad.esfahbod@gmail.com <mailto:behdad.esfahbod@gmail.com>> wrote:
> >
> >     The test was really broken...
> >
> >     Ok, I've pushed some things there that should help Matthias move
> foward:
> >
> >       https://github.com/behdad/cairo/commits/font-variations
> >     <https://github.com/behdad/cairo/commits/font-variations>
> >
> >
> > Thanks for those fixes, Behdad. Taking another look at the test, I'm not
> > sure what I was thinking - it was checking the width axis, but the font
> > we use only has weight and contrast as axes.
> > i've pushed all the fixes to
> >
> > https://github.com/matthiasclasen/cairo/tree/wip/
> matthiasc/font-variations
> >
> > Does this work for you now, Adrian ?
>
> It works with freetype 2.8.0, but not 2.8.1. Here are the results of
> testing with different combinations of freetype and fontconfig.
>
> freetype 2.8.0, fontconfig 2.11.0 : pass
>
> freetype 2.8.0, fontconfig 2.12.5 : pass
>
> freetype 2.8.0, Behdad's fontconfig (2d0063948a44) : pass
>
> freetype 2.8.1, fontconfig 2.11.0 : fail
>
>   axis Weight, value 300
>   axis CNTR, value 0
>   axis Weight, value 0
>   axis CNTR, value 0
>   Axis Weight: not expected value (0 != 300.5)
>
> freetype 2.8.1, fontconfig 2.12.5 : fail
>
>   axis Weight, value 300
>   axis CNTR, value 0
>   axis Weight, value 0
>   axis CNTR, value 0
>   Axis Weight: not expected value (0 != 300.5)
>
> freetype 2.8.1, Behdad's fontconfig (2d0063948a44) : fail
>
>   axis Weight, value 300
>   axis CNTR, value 0
>   axis Weight, value 0
>   axis CNTR, value 0
>   Axis Weight: not expected value (0 != 300.5)
>
>

I'm seeing those failures with freetype 2.8.1 too. They are somewhat
mysterious - if I reorder the tests different ones fail. The fact that they
sometimes report values that are nowhere to be found in the input, and
sometime reports 0, which is not in the allowed range for the axis
makes me think that something goes wrong in freetype.
I'll try to take a look today as well.  Looping in Werner.

Let's aim to have this sorted out this week and commit.  I'll be travelling
again for multiple weeks and don't want to see what happened to color-fonts
patchset to happen here.

On Thu, Sep 21, 2017 at 4:22 AM, Matthias Clasen <matthias.clasen@gmail.com>
wrote:

>
>
> On Thu, Sep 21, 2017 at 5:42 AM, Adrian Johnson <ajohnson@redneon.com>
> wrote:
>
>> On 21/09/17 12:03, Matthias Clasen wrote:
>> > On Wed, Sep 20, 2017 at 6:15 PM, Behdad Esfahbod
>> > <behdad.esfahbod@gmail.com <mailto:behdad.esfahbod@gmail.com>> wrote:
>> >
>> >     The test was really broken...
>> >
>> >     Ok, I've pushed some things there that should help Matthias move
>> foward:
>> >
>> >       https://github.com/behdad/cairo/commits/font-variations
>> >     <https://github.com/behdad/cairo/commits/font-variations>
>> >
>> >
>> > Thanks for those fixes, Behdad. Taking another look at the test, I'm not
>> > sure what I was thinking - it was checking the width axis, but the font
>> > we use only has weight and contrast as axes.
>> > i've pushed all the fixes to
>> >
>> > https://github.com/matthiasclasen/cairo/tree/wip/matthiasc/
>> font-variations
>> >
>> > Does this work for you now, Adrian ?
>>
>> It works with freetype 2.8.0, but not 2.8.1. Here are the results of
>> testing with different combinations of freetype and fontconfig.
>>
>> freetype 2.8.0, fontconfig 2.11.0 : pass
>>
>> freetype 2.8.0, fontconfig 2.12.5 : pass
>>
>> freetype 2.8.0, Behdad's fontconfig (2d0063948a44) : pass
>>
>> freetype 2.8.1, fontconfig 2.11.0 : fail
>>
>>   axis Weight, value 300
>>   axis CNTR, value 0
>>   axis Weight, value 0
>>   axis CNTR, value 0
>>   Axis Weight: not expected value (0 != 300.5)
>>
>> freetype 2.8.1, fontconfig 2.12.5 : fail
>>
>>   axis Weight, value 300
>>   axis CNTR, value 0
>>   axis Weight, value 0
>>   axis CNTR, value 0
>>   Axis Weight: not expected value (0 != 300.5)
>>
>> freetype 2.8.1, Behdad's fontconfig (2d0063948a44) : fail
>>
>>   axis Weight, value 300
>>   axis CNTR, value 0
>>   axis Weight, value 0
>>   axis CNTR, value 0
>>   Axis Weight: not expected value (0 != 300.5)
>>
>>
>
> I'm seeing those failures with freetype 2.8.1 too. They are somewhat
> mysterious - if I reorder the tests different ones fail. The fact that they
> sometimes report values that are nowhere to be found in the input, and
> sometime reports 0, which is not in the allowed range for the axis
> makes me think that something goes wrong in freetype.
>