Re: OpenType font variations and cairo

On Sun, Sep 17, 2017 at 1:12 AM, Adrian Johnson <[email protected]>
wrote:

> On 17/09/17 02:57, Matthias Clasen wrote:
> > On Fri, Sep 15, 2017 at 6:51 AM, Adrian Johnson <[email protected]
> > <mailto:[email protected]>> 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.

-- 
cairo mailing list
[email protected]
https://lists.cairographics.org/mailman/listinfo/cairo