OpenType font variations and cairo

Submitted by Matthias Clasen on Sept. 21, 2017, 10:48 p.m.

Details

Message ID CAFwd_vBUDKmbtv4gzh+QBgWM83aTMj37mTTBziDXdQK+f9LMxA@mail.gmail.com
State New
Series "OpenType font variations and cairo"
Headers show

Commit Message

Matthias Clasen Sept. 21, 2017, 10:48 p.m.
I found one typo in your cairo fixes, Behdad:

        }
        else
            for (i = 0; i < ft_mm_var->num_axis; i++)

‚Äč
After fixing that, I _still_ get failures of my cairo tests. Here is what
happens: One of my tests happens to
set the axes to the coords of a named instance. freetype takes that as a
signal to turn the FT_Face into a
named instance, and then the next test that doesn't explicitly sets a
weight value gets the named instance
coordinate instead of the axis default. Not sure who to blame here, but the
end result is confusng and
suboptimal.

Patch hide | download patch | download mbox

diff --git a/src/cairo-ft-font.c b/src/cairo-ft-font.c
index 7d823c284..107746794 100644
--- a/src/cairo-ft-font.c
+++ b/src/cairo-ft-font.c
@@ -3664,7 +3664,7 @@  cairo_ft_apply_variations (FT_Face     face,
        if (instance_id && instance_id <= ft_mm_var->num_namedstyles)
        {
            FT_Var_Named_Style *instance =
&ft_mm_var->namedstyle[instance_id - 1];
-           memcpy (coords, instance->coords, ft_mm_var->num_axis & sizeof
(*coords));
+           memcpy (coords, instance->coords, ft_mm_var->num_axis * sizeof
(*coords));

Comments

Matthias Clasen Sept. 22, 2017, 11:10 a.m.
On Fri, Sep 22, 2017 at 1:10 AM, Werner LEMBERG <wl@gnu.org> wrote:

>
> > One of my tests happens to set the axes to the coords of a named
> > instance.  freetype takes that as a signal to turn the FT_Face into
> > a named instance,
>
> It sets the named instance index in `face->face_index', that's all.
>
> > and then the next test that doesn't explicitly sets a weight value
> > gets the named instance coordinate instead of the axis default.
>
> This part I don't understand.  Please elaborate.  What are you doing
> code-wise, what do you expect, and what does FreeType?
>
>
For each test, I create a new cairo_scaled_font instance, using the same
fontconfig pattern and different font options (this is where the test input
goes):

    pattern = FcPatternBuild (NULL,
                              FC_FAMILY, FcTypeString, (FcChar8*)"Adobe
Variable Font Prototype",
                              NULL);
    font_face = cairo_ft_font_face_create_for_pattern (pattern);
    cairo_font_options_set_variations (options, input);
    scaled_font = cairo_scaled_font_create (font_face, &matrix, &matrix,
options);

and then I pull out the FT_Face, and I find that I get the same one every
time, since
cairo caches them:

    ft_face = cairo_ft_scaled_font_lock_face (scaled_font);

But now, the nature of the cached object changes underneath us (sometimes
it represents a
named instance, and sometimes it doesn't). And the code applying the font
options to
changes its behavior depending on this, which leads to a mess. We need to
either

a) not cache a changing object
b) take the named instanceness into account for caching
c) ignore the named instanceness when applying font options

Behdad ?
Matthias Clasen Sept. 28, 2017, 8:36 p.m.
On Fri, Sep 22, 2017 at 7:10 AM, Matthias Clasen <matthias.clasen@gmail.com>
wrote:

>
> a) not cache a changing object
> b) take the named instanceness into account for caching
> c) ignore the named instanceness when applying font options
>
> Behdad ?
>
>
Behdad and I stuck our heads together for a while today, and came up with a
simple fix that I've pushed here:

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

With this, the cairo tests pass, and my gtk example works as expected as
well.
Adrian Johnson Sept. 28, 2017, 10:09 p.m.
On 29/09/17 06:06, Matthias Clasen wrote:
> On Fri, Sep 22, 2017 at 7:10 AM, Matthias Clasen
> <matthias.clasen@gmail.com <mailto:matthias.clasen@gmail.com>> wrote:
> 
> 
>     a) not cache a changing object
>     b) take the named instanceness into account for caching
>     c) ignore the named instanceness when applying font options
> 
>     Behdad ?
> 
> 
> Behdad and I stuck our heads together for a while today, and came up
> with a simple fix that I've pushed here:
> 
> https://github.com/matthiasclasen/cairo/tree/wip/matthiasc/font-variations
> 
> With this, the cairo tests pass, and my gtk example works as expected as
> well.

I pulled your latest changes and tested with various combinations of
freetype and fontconfig - see below. All good on the cairo side, except
for the build fail with FT 2.6.3 but I mentioned that needs fixing in my
review comments.

There appears to be a bug in fontconfig master when using freetype 2.8.

If you cleanup and rebase your commits to master with all review
comments addressed, I'll do a final check and then it should be good to
commit.


freetype 2.6.3: build failed - FT_Get_Var_Design_Coordinates not found

freetype 2.8, fontconfig 2.11.0 : pass

freetype 2.8, fontconfig 2.12.5 : pass

freetype 2.8, fontconfig master :

When running the fc-cache built from master I get:
  fc-cache: fcfreetype.c:2143: IA__FcFreeTypeQueryAll: Assertion
  `mm_var' failed.

fc-cache succeeds after removing AdobeVFPrototype.ttf from ~/.fonts

I tried again with AdobeVFPrototype.otf. fc-cache ran successfully and
the cairo test passed.

freetype 2.8.1, fontconfig 2.11.0 : pass

freetype 2.8.1, fontconfig 2.12.5 : pass

freetype 2.8.1, fontconfig master : pass

freetype master, fontconfig master : pass
Behdad Esfahbod Sept. 28, 2017, 11:01 p.m.
On Thu, Sep 28, 2017 at 6:09 PM, Adrian Johnson <ajohnson@redneon.com>
wrote:

> On 29/09/17 06:06, Matthias Clasen wrote:
> I pulled your latest changes and tested with various combinations of
> freetype and fontconfig - see below. All good on the cairo side, except
> for the build fail with FT 2.6.3 but I mentioned that needs fixing in my
> review comments.
>
> There appears to be a bug in fontconfig master when using freetype 2.8.
>
> If you cleanup and rebase your commits to master with all review
> comments addressed, I'll do a final check and then it should be good to
> commit.
>

Thanks Adrian for all the help.

freetype 2.6.3: build failed - FT_Get_Var_Design_Coordinates not found
>
> freetype 2.8, fontconfig 2.11.0 : pass
>
> freetype 2.8, fontconfig 2.12.5 : pass
>
> freetype 2.8, fontconfig master :
>
> When running the fc-cache built from master I get:
>   fc-cache: fcfreetype.c:2143: IA__FcFreeTypeQueryAll: Assertion
>   `mm_var' failed.
>
> fc-cache succeeds after removing AdobeVFPrototype.ttf from ~/.fonts
>
> I tried again with AdobeVFPrototype.otf. fc-cache ran successfully and
> the cairo test passed.
>

Humm.  This sounds very much like what might happen with old FreeType . If
not, can you elaborate what happens again?  Can you reproduce?  What
FreeType exactly?   Does `fc-query AdobeVFPrototype.ttf` work?

I can probably replace assert with a less harsh mode of failure.



> freetype 2.8.1, fontconfig 2.11.0 : pass
>
> freetype 2.8.1, fontconfig 2.12.5 : pass
>
> freetype 2.8.1, fontconfig master : pass
>
> freetype master, fontconfig master : pass
>
>
>
>
>
>
Behdad Esfahbod Sept. 28, 2017, 11:03 p.m.
+Khaled,Werner

Two things:

  * Werner, please please please remove code that change face->face_index.
It's bad bad bad.

  * With Matthias's branch, we break Khaled's code, ie. if user creates
FT_Face, sets variations on it, and creates cairo_face_t from, we reset /
ignore those variations.  The fix would require savings the variations that
were on the face when face was created, and use those instead of
instance-defaults in apply_variations().  Matthias, any chance you can do
this?

b

On Thu, Sep 28, 2017 at 7:01 PM, Behdad Esfahbod <behdad.esfahbod@gmail.com>
wrote:

> On Thu, Sep 28, 2017 at 6:09 PM, Adrian Johnson <ajohnson@redneon.com>
> wrote:
>
>> On 29/09/17 06:06, Matthias Clasen wrote:
>> I pulled your latest changes and tested with various combinations of
>> freetype and fontconfig - see below. All good on the cairo side, except
>> for the build fail with FT 2.6.3 but I mentioned that needs fixing in my
>> review comments.
>>
>> There appears to be a bug in fontconfig master when using freetype 2.8.
>>
>> If you cleanup and rebase your commits to master with all review
>> comments addressed, I'll do a final check and then it should be good to
>> commit.
>>
>
> Thanks Adrian for all the help.
>
> freetype 2.6.3: build failed - FT_Get_Var_Design_Coordinates not found
>>
>> freetype 2.8, fontconfig 2.11.0 : pass
>>
>> freetype 2.8, fontconfig 2.12.5 : pass
>>
>> freetype 2.8, fontconfig master :
>>
>> When running the fc-cache built from master I get:
>>   fc-cache: fcfreetype.c:2143: IA__FcFreeTypeQueryAll: Assertion
>>   `mm_var' failed.
>>
>> fc-cache succeeds after removing AdobeVFPrototype.ttf from ~/.fonts
>>
>> I tried again with AdobeVFPrototype.otf. fc-cache ran successfully and
>> the cairo test passed.
>>
>
> Humm.  This sounds very much like what might happen with old FreeType . If
> not, can you elaborate what happens again?  Can you reproduce?  What
> FreeType exactly?   Does `fc-query AdobeVFPrototype.ttf` work?
>
> I can probably replace assert with a less harsh mode of failure.
>
>
>
>> freetype 2.8.1, fontconfig 2.11.0 : pass
>>
>> freetype 2.8.1, fontconfig 2.12.5 : pass
>>
>> freetype 2.8.1, fontconfig master : pass
>>
>> freetype master, fontconfig master : pass
>>
>>
>>
>>
>>
>>
>
>
> --
> behdad
> http://behdad.org/
>
Behdad Esfahbod Sept. 28, 2017, 11:04 p.m.
Also, here's how you can test with pango-view:

$ pango-view --text Test --font 'voto serif gx 123 @wght=900'

Currently it's broken completely.  I'm investigating.

On Thu, Sep 28, 2017 at 7:03 PM, Behdad Esfahbod <behdad.esfahbod@gmail.com>
wrote:

> +Khaled,Werner
>
> Two things:
>
>   * Werner, please please please remove code that change
> face->face_index.  It's bad bad bad.
>
>   * With Matthias's branch, we break Khaled's code, ie. if user creates
> FT_Face, sets variations on it, and creates cairo_face_t from, we reset /
> ignore those variations.  The fix would require savings the variations that
> were on the face when face was created, and use those instead of
> instance-defaults in apply_variations().  Matthias, any chance you can do
> this?
>
> b
>
> On Thu, Sep 28, 2017 at 7:01 PM, Behdad Esfahbod <
> behdad.esfahbod@gmail.com> wrote:
>
>> On Thu, Sep 28, 2017 at 6:09 PM, Adrian Johnson <ajohnson@redneon.com>
>> wrote:
>>
>>> On 29/09/17 06:06, Matthias Clasen wrote:
>>> I pulled your latest changes and tested with various combinations of
>>> freetype and fontconfig - see below. All good on the cairo side, except
>>> for the build fail with FT 2.6.3 but I mentioned that needs fixing in my
>>> review comments.
>>>
>>> There appears to be a bug in fontconfig master when using freetype 2.8.
>>>
>>> If you cleanup and rebase your commits to master with all review
>>> comments addressed, I'll do a final check and then it should be good to
>>> commit.
>>>
>>
>> Thanks Adrian for all the help.
>>
>> freetype 2.6.3: build failed - FT_Get_Var_Design_Coordinates not found
>>>
>>> freetype 2.8, fontconfig 2.11.0 : pass
>>>
>>> freetype 2.8, fontconfig 2.12.5 : pass
>>>
>>> freetype 2.8, fontconfig master :
>>>
>>> When running the fc-cache built from master I get:
>>>   fc-cache: fcfreetype.c:2143: IA__FcFreeTypeQueryAll: Assertion
>>>   `mm_var' failed.
>>>
>>> fc-cache succeeds after removing AdobeVFPrototype.ttf from ~/.fonts
>>>
>>> I tried again with AdobeVFPrototype.otf. fc-cache ran successfully and
>>> the cairo test passed.
>>>
>>
>> Humm.  This sounds very much like what might happen with old FreeType .
>> If not, can you elaborate what happens again?  Can you reproduce?  What
>> FreeType exactly?   Does `fc-query AdobeVFPrototype.ttf` work?
>>
>> I can probably replace assert with a less harsh mode of failure.
>>
>>
>>
>>> freetype 2.8.1, fontconfig 2.11.0 : pass
>>>
>>> freetype 2.8.1, fontconfig 2.12.5 : pass
>>>
>>> freetype 2.8.1, fontconfig master : pass
>>>
>>> freetype master, fontconfig master : pass
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>> --
>> behdad
>> http://behdad.org/
>>
>
>
>
> --
> behdad
> http://behdad.org/
>
Matthias Clasen Sept. 28, 2017, 11:14 p.m.
On Thu, Sep 28, 2017 at 7:03 PM, Behdad Esfahbod <behdad.esfahbod@gmail.com>
wrote:

> +Khaled,Werner
>
> Two things:
>
>   * Werner, please please please remove code that change
> face->face_index.  It's bad bad bad.
>
>   * With Matthias's branch, we break Khaled's code, ie. if user creates
> FT_Face, sets variations on it, and creates cairo_face_t from, we reset /
> ignore those variations.  The fix would require savings the variations that
> were on the face when face was created, and use those instead of
> instance-defaults in apply_variations().  Matthias, any chance you can do
> this?
>

I'll have a look.
Behdad Esfahbod Sept. 28, 2017, 11:30 p.m.
On Thu, Sep 28, 2017 at 7:03 PM, Behdad Esfahbod <behdad.esfahbod@gmail.com>
wrote:

> +Khaled,Werner
>
> Two things:
>
>   * Werner, please please please remove code that change
> face->face_index.  It's bad bad bad.
>

Also, if you do that, then Set_Var_Design_Coords() with a too-short input
array should use the named-instance's values, not the axes' defaults.



>   * With Matthias's branch, we break Khaled's code, ie. if user creates
> FT_Face, sets variations on it, and creates cairo_face_t from, we reset /
> ignore those variations.  The fix would require savings the variations that
> were on the face when face was created, and use those instead of
> instance-defaults in apply_variations().  Matthias, any chance you can do
> this?
>
> b
>
> On Thu, Sep 28, 2017 at 7:01 PM, Behdad Esfahbod <
> behdad.esfahbod@gmail.com> wrote:
>
>> On Thu, Sep 28, 2017 at 6:09 PM, Adrian Johnson <ajohnson@redneon.com>
>> wrote:
>>
>>> On 29/09/17 06:06, Matthias Clasen wrote:
>>> I pulled your latest changes and tested with various combinations of
>>> freetype and fontconfig - see below. All good on the cairo side, except
>>> for the build fail with FT 2.6.3 but I mentioned that needs fixing in my
>>> review comments.
>>>
>>> There appears to be a bug in fontconfig master when using freetype 2.8.
>>>
>>> If you cleanup and rebase your commits to master with all review
>>> comments addressed, I'll do a final check and then it should be good to
>>> commit.
>>>
>>
>> Thanks Adrian for all the help.
>>
>> freetype 2.6.3: build failed - FT_Get_Var_Design_Coordinates not found
>>>
>>> freetype 2.8, fontconfig 2.11.0 : pass
>>>
>>> freetype 2.8, fontconfig 2.12.5 : pass
>>>
>>> freetype 2.8, fontconfig master :
>>>
>>> When running the fc-cache built from master I get:
>>>   fc-cache: fcfreetype.c:2143: IA__FcFreeTypeQueryAll: Assertion
>>>   `mm_var' failed.
>>>
>>> fc-cache succeeds after removing AdobeVFPrototype.ttf from ~/.fonts
>>>
>>> I tried again with AdobeVFPrototype.otf. fc-cache ran successfully and
>>> the cairo test passed.
>>>
>>
>> Humm.  This sounds very much like what might happen with old FreeType .
>> If not, can you elaborate what happens again?  Can you reproduce?  What
>> FreeType exactly?   Does `fc-query AdobeVFPrototype.ttf` work?
>>
>> I can probably replace assert with a less harsh mode of failure.
>>
>>
>>
>>> freetype 2.8.1, fontconfig 2.11.0 : pass
>>>
>>> freetype 2.8.1, fontconfig 2.12.5 : pass
>>>
>>> freetype 2.8.1, fontconfig master : pass
>>>
>>> freetype master, fontconfig master : pass
>>>
>>>
>>>
>>>
>>>
>>>
>>
>>
>> --
>> behdad
>> http://behdad.org/
>>
>
>
>
> --
> behdad
> http://behdad.org/
>
Behdad Esfahbod Oct. 11, 2017, 3:46 p.m.
Thanks Werner!

On Sat, Oct 7, 2017 at 7:47 AM, Werner LEMBERG <wl@gnu.org> wrote:

>
> > * FreeType should handle named instances and
> >   `FT_Set_Var_Design_Coords' (and friends) completely separate.
> >
> > * FreeType should provide a new function
> >
> >     FT_Set_Named_Instance( FT_Face  face,
> >                            FT_UInt  instance_index )
> >
> >   This adjusts the PS name also using the current FreeType algorithm
> >   for named instances.
> >
> > * `FT_Set_Var_Design_Coords' should construct a PS name as if there
> >   were no named instances.
> >
> > * `FT_Set_Var_Design_Coords(face, 0, NULL)' should reset to the
> >   current named instance.
>
> This is now implemented in git.  Please test!
>
>
>     Werner
>