OpenType font variations and cairo

Submitted by Adrian Johnson on Sept. 28, 2017, 11:29 p.m.

Details

Message ID 896e37bd-3204-3fc8-b3da-a6c076b0df2a@redneon.com
State New
Headers show
Series "OpenType font variations and cairo" ( rev: 4 ) in Cairo

Not browsing as part of any series.

Commit Message

Adrian Johnson Sept. 28, 2017, 11:29 p.m.
On 29/09/17 08:31, Behdad Esfahbod wrote:
> On Thu, Sep 28, 2017 at 6:09 PM, Adrian Johnson <ajohnson@redneon.com
> <mailto: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?

FreeType 2.8.0. I can reproduce. fc-query AdobeVFPrototype.ttf also
produces the same error.

I looked into it and it seems to be the same problem Matthias fixed in
a827d7e515c7c. I applied a similar patch to fontconfig (see attached)
and fc-cache and fc-query now 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/
> 
>

Patch hide | download patch | download mbox

diff --git a/src/fcfreetype.c b/src/fcfreetype.c
index cddd3a1..f8c81ee 100644
--- a/src/fcfreetype.c
+++ b/src/fcfreetype.c
@@ -2139,7 +2139,10 @@  FcFreeTypeQueryAll(const FcChar8	*file,
     num_instances = face->style_flags >> 16;
     if (num_instances && (!index_set || instance_num))
     {
-	FT_Get_MM_Var (face, &mm_var);
+	FT_Error ret;
+	ret = FT_Get_MM_Var (face, &mm_var);
+	if (ret != 0)
+	    FT_Get_MM_Var (face, &mm_var);
 	assert (mm_var);
     }
 

Comments

On Thu, Sep 28, 2017 at 7:29 PM, Adrian Johnson <ajohnson@redneon.com>
wrote:

> On 29/09/17 08:31, Behdad Esfahbod wrote:
> > On Thu, Sep 28, 2017 at 6:09 PM, Adrian Johnson <ajohnson@redneon.com
> > <mailto: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?
>
> FreeType 2.8.0. I can reproduce. fc-query AdobeVFPrototype.ttf also
> produces the same error.
>
> I looked into it and it seems to be the same problem Matthias fixed in
> a827d7e515c7c. I applied a similar patch to fontconfig (see attached)
> and fc-cache and fc-query now work.
>

Yeah, bad bad FreeType. Humm.  Not sure if I like to take that patch.  I
prefer to bump required FreeType version to 2.8.1.  Suggest we do the same
to cairo and clean up all old #ifdef's.


>
> >
> > 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/
> >
> >
>
>
On 29/09/17 09:02, Behdad Esfahbod wrote:
> On Thu, Sep 28, 2017 at 7:29 PM, Adrian Johnson <ajohnson@redneon.com
>     I looked into it and it seems to be the same problem Matthias fixed in
>     a827d7e515c7c. I applied a similar patch to fontconfig (see attached)
>     and fc-cache and fc-query now work.
> 
> 
> Yeah, bad bad FreeType. Humm.  Not sure if I like to take that patch.  I
> prefer to bump required FreeType version to 2.8.1.  Suggest we do the
> same to cairo and clean up all old #ifdef's.

I don't think we should bump the FT requirement to 2.8.1 until that
version or later is widely available in distros. Otherwise it creates an
extra burden for anyone who wants to build cairo and doesn't need to use
variable fonts.

I suggest we disable variations internally in cairo for FT < 2.8.1 and
print a warning during configure that font variations needs FT >= 2.8.1

I do agree we could move the FT minimum to something more recent and
drop all the old FT #ifdefs.
On Thu, Sep 28, 2017 at 7:53 PM, Adrian Johnson <ajohnson@redneon.com>
wrote:

> On 29/09/17 09:02, Behdad Esfahbod wrote:
> > On Thu, Sep 28, 2017 at 7:29 PM, Adrian Johnson <ajohnson@redneon.com
> >     I looked into it and it seems to be the same problem Matthias fixed
> in
> >     a827d7e515c7c. I applied a similar patch to fontconfig (see attached)
> >     and fc-cache and fc-query now work.
> >
> > Yeah, bad bad FreeType. Humm.  Not sure if I like to take that patch.  I
> > prefer to bump required FreeType version to 2.8.1.  Suggest we do the
> > same to cairo and clean up all old #ifdef's.
>
> I don't think we should bump the FT requirement to 2.8.1 until that
> version or later is widely available in distros. Otherwise it creates an
> extra burden for anyone who wants to build cairo and doesn't need to use
> variable fonts.
>

Yeah, but who builds cairo manually anyway...


> I suggest we disable variations internally in cairo for FT < 2.8.1 and
> print a warning during configure that font variations needs FT >= 2.8.1
>
> I do agree we could move the FT minimum to something more recent and
> drop all the old FT #ifdefs.
>

Sure, if that's what you prefer.  Give me a minimum version and I'll do the
patch.  That said, I just bumped fontconfig requirement to 2.8.1.  So
distros, etc, will have to carry forward (which is good).
On 29/09/17 09:28, Behdad Esfahbod wrote:
> On Thu, Sep 28, 2017 at 7:53 PM, Adrian Johnson <ajohnson@redneon.com
> <mailto:ajohnson@redneon.com>> wrote:
> 
>     On 29/09/17 09:02, Behdad Esfahbod wrote:
>     > On Thu, Sep 28, 2017 at 7:29 PM, Adrian Johnson <ajohnson@redneon.com <mailto:ajohnson@redneon.com>
>     >     I looked into it and it seems to be the same problem Matthias fixed in
>     >     a827d7e515c7c. I applied a similar patch to fontconfig (see attached)
>     >     and fc-cache and fc-query now work.
>     >
>     > Yeah, bad bad FreeType. Humm.  Not sure if I like to take that patch.  I
>     > prefer to bump required FreeType version to 2.8.1.  Suggest we do the
>     > same to cairo and clean up all old #ifdef's.
> 
>     I don't think we should bump the FT requirement to 2.8.1 until that
>     version or later is widely available in distros. Otherwise it creates an
>     extra burden for anyone who wants to build cairo and doesn't need to use
>     variable fonts.
> 
> 
> Yeah, but who builds cairo manually anyway...
>  
> 
>     I suggest we disable variations internally in cairo for FT < 2.8.1 and
>     print a warning during configure that font variations needs FT >= 2.8.1
> 
>     I do agree we could move the FT minimum to something more recent and
>     drop all the old FT #ifdefs.
> 
> 
> Sure, if that's what you prefer.  Give me a minimum version and I'll do
> the patch.  That said, I just bumped fontconfig requirement to 2.8.1. 
> So distros, etc, will have to carry forward (which is good).

I'd go with 2.6.0 released in Jun 2015. From the FT 2.6.0 CHANGELOG entry:

    - Function `FT_Bitmap_New'  has been renamed  to `FT_Bitmap_Init',
      since  this name  better reflects  its  function.   For backward
      compatibility, the old function name is still available.

    - Function   `FT_Get_X11_Font_Format'   has    been   renamed   to
      `FT_Get_Font_Format',  since  this   name  better  reflects  its
      function.  For backward compatibility,  the old function name is
      still available.

      Additionally, the header  file macro for this  function has been
      renamed to  `FT_FONT_FORMATS_H' (the old name  `FT_XFREE86_H' is
      retained for backward compatibility).

Then you can update all of these as well as dropping the old #ifdefs.

> 
> 
> -- 
> behdad
> http://behdad.org/