pango 1.24 breaks pangofc-fontmap API and breaks mozilla build

[Alexander Sack]

See:
https://bugzilla.mozilla.org/show_bug.cgi?id=485597

PangoFcFontMapClass#context_substitute was renamed.

[M-kato (m-kato)]

Created attachment 362845
patch v0

1.23 series is unstable version.

Also, I don't test this patch on older version. After that, I will send a review.

[Kairo-kairo (kairo-kairo)]

It sounds reasonable for a development distribution to pick up an unstable version of that library, I guess. And that's why I'm using that stuff: Better to catch and fix our code for that change now than when it comes up in the stable version ;-)

[M-kato (m-kato)]

The latest snapshot of fedora rawhide (Fedora 11?) uses Pango-1.23. So, some distributions may use 1.23 series. And, the API Document says that these APIs are from 1.24 (stable) series.

So, we need fix this even if API is changed by later release. (I don't know release data of 1.24. 1.23 was this month.)

[Kairo-kairo (kairo-kairo)]

I can confirm that my builds (both 1.9.1-based and mozilla-central) compile and work with this patch and Pango 1.23.

[M-kato (m-kato)]

Created attachment 364007
patch v1

add comment

[Ginn-chen (ginn-chen)]

Since "// context_substitute and get_font are not likely to be used"
and from pango header both context_substitute and fontset_key_substitute may be NULL.
I think the right fix should be remove these functions from mozilla code. Use NULL.
Otherwise we may introduce crashes if Firefox is compiled and run with different versions of pango.

[M-kato (m-kato)]

Since stable version (1.24) is released, I analyse this.

I tested with pango 1.24, gfx callback function of fontset_key_subtitute and get_font are never called.

Becaues, the following reasons. So these call functions are never called.

- get_font and foreach in gfx_pango_fontset_class_init() is implemented.
- get_resolution in gfx_pango_font_map_class_init() is implemented.

So I will rewrite fix...

[M-kato (m-kato)]

Created attachment 369648
patch v2

[Alexander Sack (asac)]

See:
https://bugzilla.mozilla.org/show_bug.cgi?id=485597

PangoFcFontMapClass#context_substitute was renamed.

[Alexander Sack (asac)]

bzr log -c2378
------------------------------------------------------------
revno: 2378
committer: behdad
timestamp: Wed 2009-01-28 22:07:38 +0000
message:
  2009-01-09 Behdad Esfahbod <email address hidden>

          * docs/pango-sections.txt:
          * docs/tmpl/pangofc-fontmap.sgml:
          * docs/tmpl/text-attributes.sgml:
          * pango/pangocairo-fc.h:
          * pango/pangocairo-fcfont.c (get_font_size), (get_gravity_class),
          (get_gravity), (_pango_cairo_fc_font_new):
          * pango/pangocairo-fcfontmap.c
          (pango_cairo_fc_font_map_font_key_substitute),
          (pango_cairo_fc_font_map_create_font),
          (pango_cairo_fc_font_map_class_init):
          * pango/pangofc-font.c (_pango_fc_font_get_font_key),
          (_pango_fc_font_set_font_key):
          * pango/pangofc-fontmap.c (pango_fc_font_key_equal),
          (pango_fc_font_key_hash), (pango_fc_font_key_free),
          (pango_fc_font_key_copy), (get_context_matrix),
          (pango_fc_font_key_init), (pango_fc_font_key_get_pattern),
          (pango_fc_font_key_get_matrix),
          (pango_fc_font_key_get_context_key), (pango_fc_font_map_init),
          (pango_fc_font_map_class_init), (pango_fc_font_map_add),
          (_pango_fc_font_map_remove), (pango_fc_make_pattern),
          (pango_fc_font_map_new_font), (pango_fc_default_substitute),
          (pango_fc_font_map_get_patterns), (pango_fc_font_map_load_fontset):
          * pango/pangofc-fontmap.h:
          * pango/pangofc-private.h:
          Change PangoFc font loading API such that PangoContext is not passed
          down. We use a new opaque struct called PangoFcFontKey. This struct
          is in fact our font hash key. This avoids problems where previously
          we were using context members that were not necessarily considered
          by the pangofc layer when caching.
          This is in preparation for lazy loading of fonts in the pangofc fontmap.
asac@hector3:~/gnome/pango.trunk.bzr$ bzr diff -c2378 pango/pangofc-fontmap.h
=== modified file 'pango/pangofc-fontmap.h'
--- a/pango/pangofc-fontmap.h 2008-12-08 03:28:29 +0000
+++ b/pango/pangofc-fontmap.h 2009-01-28 22:07:38 +0000
@@ -24,16 +24,34 @@

 #include <fontconfig/fontconfig.h>
 #include <pango/pango-fontmap.h>
 #include <pango/pangofc-decoder.h>
 #include <pango/pangofc-font.h>

 G_BEGIN_DECLS

+/**
+ * PangoFcFontKey:
+ *
+ * An opaque structure containing all the information needed for
+ * loading a font #PangoFcFont.
+ *
+ * Since: 1.24
+ **/
+typedef struct _PangoFcFontKey PangoFcFontKey;
+
+const FcPattern *pango_fc_font_key_get_pattern (const PangoFcFontKey *key);
+const PangoMatrix *pango_fc_font_key_get_matrix (const PangoFcFontKey *key);
+gpointer pango_fc_font_key_get_context_key (const PangoFcFontKey *key);
+
+/*
+ * PangoFcFontMap
+ */
+
 #define PANGO_TYPE_FC_FONT_MAP (pango_fc_font_map_get_type ())
 #define PANGO_FC_FONT_MAP(object) (G_TYPE_CHECK_INSTANCE_CAST ((object), PANGO_TYPE_FC_FONT_MAP, PangoFcFontMap))
 #define PANGO_IS_FC_FONT_MAP(object) (G_TYPE_CHECK_INSTANCE_TYPE ((object), PANGO_TYPE_FC_FONT_MAP))

 typedef struct _P...

[Alexander Sack (asac)]

since the bzr log doesn't really speak about changing the context_substitute symbol I wonder if this might have been a commit mistake.

[Kurosawa Takeshi (takenspc)]

*** Bug 485597 has been marked as a duplicate of this bug. ***

[Kairo-kairo (kairo-kairo)]

Comment on attachment 369648
patch v2

>+ if (pango_version() < PANGO_VERSION_ENCODE(1,23,0)) {
>+#if PANGO_VERSION_CHECK(1,23,0)

When do we get inside here to that code? The outer if says we have pango < 1.23, right?

[Wade Menard (wade-ezri)]

*** Bug 481193 has been marked as a duplicate of this bug. ***

[Josh Aas (joshmoz)]

This bug also causes bustage on Ubuntu 9.04 Beta.

[Ginn-chen (ginn-chen)]

(In reply to comment #10)
> (From update of attachment 369648 [details])
> >+ if (pango_version() < PANGO_VERSION_ENCODE(1,23,0)) {
> >+#if PANGO_VERSION_CHECK(1,23,0)
>
> When do we get inside here to that code? The outer if says we have pango <
> 1.23, right?

The outer checking is runtime checking.
The inner checking is a compile-time checking.

I think it might be less confusion if we include a private structure declaration of PangoFcFontMapClass.

Also I'm wondering if context_substitute and create_font are really called in Firefox with Pango < 1.23.

[M-kato (m-kato)]

(In reply to comment #10)
> (From update of attachment 369648 [details])
> >+ if (pango_version() < PANGO_VERSION_ENCODE(1,23,0)) {
> >+#if PANGO_VERSION_CHECK(1,23,0)
>
> When do we get inside here to that code? The outer if says we have pango <
> 1.23, right?

see Ginn's comment. (comment #13)

(In reply to comment #10)
> Also I'm wondering if context_substitute and create_font are really called in
> Firefox with Pango < 1.23

I don't confirm it, yet. But I have checked code of version 1.23 and 1.24. Should we check all version of source code? (1.14.0 or greater??)

[Karlt (karlt)]

Comment on attachment 369648
patch v2

create_font and context_key_substitute are not currently used but are provided for a
complete PangoFcFontMap implementation. This is meant to protect from future
changes to Pango that may result in these functions being used, or, less
likely, shaper modules choosing to use these functions.

create_font can be NULL but in that case "new_font() is used", and should not
be NULL. Building two different create_font implementations and choosing at
runtime seems unappealing, so I suggest (unless Behdad suggests otherwise)
to change create_font() to new_font(). context and desc are not used anyway
and there is some history of maintaining backward compatibility for the
deprecated new_font().

context/fontset_key_substitute is probably even less likely to be used, but similarly a
default_substitute implementation would be the same for pre/post 1.23.
Without a context, defaults would need to be used for size and usePrinterFont.
usePrinterFont false is probably the better option, and pangocairo uses 18.0
for the default size, so let's do the same.

[Ori Avtalion (salty-horse)]

According to Behdad, it's not a commit mistake.

Fedora has this fix in place:
http://cvs.fedoraproject.org/viewvc/rpms/thunderbird/devel/thunderbird-pango.patch?view=markup

Mozilla are working on a patch which checks for the pango version:
https://bugzilla.mozilla.org/show_bug.cgi?id=478871

[Mozilla-behdad (mozilla-behdad)]

Karl's analysis is accurate. Although in some distant future when pango 1.24 can be assume, would be nice to switch to the new API. The change was necessary to get the PangoContext argument out of the callbacks to enable a range of optimizations. In general I don't like breaking backend API. Sorry that happened this time.

[Vladimir Vukicevic (vvuk)]

Karl, anything additional needed with your patch?

[Daniel Holbert (dholbert)]

(In reply to comment #0)
> Yesterday, I upgraded my system to the newest openSUSE Factory, which now
> includes pango 1.23.0, and I get that compile error

FWIW: this bug affects Ubuntu 9.04 ("Jaunty") as well, which is going to ship later this month and includes pango version 1.24.0. There's a bug filed (linked to this bug) in Ubuntu's bug-tracker at https://bugs.launchpad.net/bugs/349914

[Chris Double (doublec)]

It also affects Arch Linux, current version. When you do a pacman update you get pango version 1.24.0.

[John Vivirito (gnomefreak)]

This has been reported upstream a while ago and they know the reason this happens but havent fixed it yet from what i can tell from upstream bug.

[Zack Weinberg (zackw)]

pango 1.24.0 is now in Debian unstable as well. Makoto Kato's patch gets me compiling again.

[M-kato (m-kato)]

Created attachment 371367
patch v3

remove unnecessary callback functions
after testing this, send review.

[Karlt (karlt)]

Comment on attachment 371367
patch v3

Sorry, I missed some comments here.
(I thought I'd CC'd myself and Behdad, but I guess I didn't.)

This patch works around the compile error, but we should provide at least fcfontmap_class->new_font if we don't provide create_font.

That would simply involve changing gfx_pango_font_map_create_font to gfx_pango_font_map_new_font and modifying the parameters appropriately.

And we might as well change gfx_pango_font_map_context_substitute to gfx_pango_font_map_default_substitute and use default values from comment 15 where the information is not available for fcfontmap_class->default_subsitute.

[M-kato (m-kato)]

(In reply to comment #22)

Karl, do you have test case that context_substitute or create_font is called?

Even if pango is 1.14 (min version required gecko) in CentOS 5, these callbacks are never called on any case.
Although default_substitute (pango_fc_default_substitute) will call from pango_fc_font_map_get_patterns/pango_fc_font_map_get_resolution into pango, since gfx implementation has a lot of callback functions such as load_font and get_resolution, it is never called.

I would like to know the environment that context_substitute or create_font is really called.

[Karlt (karlt)]

(In reply to comment #23)
> Karl, do you have test case that context_substitute or create_font is called?

No. As far as I have seen no current Pango version will call these methods, given the way we use Pango and the PangoFontMap methods that we override (as you point out).

However, the PangoFcFontMap API docs say that a PangoFcFontMap should provide these functions. We create a PangoFcFontMap and pass it to Pango, so it would be reasonable for any future version of Pango or any shaper modules to expect these methods to be present.

For example, a call to pango_font_map_load_fontset with a fresh PangoContext that does not have a gfxPangoFontGroup on it will result in a call to these methods.
You can test this by replacing GetFontGroup(context) with NULL in gfx_pango_font_map_load_fontset.

[John Vivirito (gnomefreak)]

This bug doesn't only apply to Seamonkey does it?

[Zack Weinberg (zackw)]

No, it's in core gfx, so it applies to everything.

[John Vivirito (gnomefreak)]

Zack thanks

[Mozilla-behdad (mozilla-behdad)]

(In reply to comment #24)

> However, the PangoFcFontMap API docs say that a PangoFcFontMap should provide
> these functions. We create a PangoFcFontMap and pass it to Pango, so it would
> be reasonable for any future version of Pango or any shaper modules to expect
> these methods to be present.

If your concern future pango versions, then I suggest adding a compile-time version check on pango and setting the new methods instead of the deprecated new_font() and default_substitute().

[Karlt (karlt)]

(In reply to comment #28)
> If your concern future pango versions, then I suggest adding a compile-time
> version check on pango and setting the new methods instead of the deprecated
> new_font() and default_substitute().

We can use the new methods, but, if we do that, we need to have pre- and post-1.24 versions of the methods (regardless of the compile-time version), and do a run-time Pango version check.

[Mozilla-behdad (mozilla-behdad)]

(In reply to comment #29)
> (In reply to comment #28)
> > If your concern future pango versions, then I suggest adding a compile-time
> > version check on pango and setting the new methods instead of the deprecated
> > new_font() and default_substitute().
>
> We can use the new methods, but, if we do that, we need to have pre- and
> post-1.24 versions of the methods (regardless of the compile-time version), and
> do a run-time Pango version check.

Thinking about it again, you're right. If you don't need the context information anyway, just use the new_font() / default_substitute() callbacks.

[Vladimir Vukicevic (vvuk)]

We need a fix for this, but won't block the release for it; if this API changed, worst case the linux distros will need to ship a patch to fix it if we don't get some kind of patch in.

[Karlt (karlt)]

Created attachment 372739
use new_font
[Checkin: Comment 39 & 42]

Using new_font and default_substitute as discussed above.
This needs testing against Pango-1.24. (I've tested Pango-1.22.4.)

[FredBezies (fredbezies-deactivatedaccount)]

It works here with my ubuntu linux jaunty beta up-to-date.

fred@fredo-ubuntu:~$ apt-cache show libpango1.0-0
Package: libpango1.0-0
Priority: optional
Section: libs
Installed-Size: 980
Maintainer: Ubuntu Core Developers <email address hidden>
Original-Maintainer: Sebastien Bacher <email address hidden>
Architecture: amd64
Source: pango1.0
Version: 1.24.1-0ubuntu1

[FredBezies (fredbezies-deactivatedaccount)]

I think a fix is going to be released for this problem.

See https://bugzilla.mozilla.org/show_bug.cgi?id=478871#c32

[Zack Weinberg (zackw)]

WFM also with libpango 1.24.0-3 (debian unstable).

[Jfelps (jfelps)]

WFM with pango 1.24.1 on Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b4pre) Gecko/20090415 Firefox/3.5b4pre.

[Henning-henkel (henning-henkel)]

Building Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b4pre) Gecko/20090416 Lightning/1.0pre Shredder/3.0b3pre from Mercurial on ArchLinux with Pango 1.24.0-2 worked with the patch for me as well.

[John Vivirito (gnomefreak)]

I tried version 3 patch and it failed to apply in Sm 2 version 2 applies fine and fixes the FTB

[Daniel Holbert (dholbert)]

(In reply to comment #37)
> I tried version 3 patch and it failed to apply in Sm 2

Note that "patch v3" is marked as obsolete -- the "use new_font" patch is the one that's got r+sr & [needs landing]. Give that one a try -- it applies fine in mozilla-central and mozilla-1.9.1.

[Daniel Holbert (dholbert)]

"use new_font" patch landed in m-c:
http://hg.mozilla.org/mozilla-central/rev/4aed53dcf692

[Daniel Holbert (dholbert)]

Comment on attachment 372739
use new_font
[Checkin: Comment 39 & 42]

Requesting approval1.9.1 for this bug's patch (to fix a messy compile error on newer linux distros).

Note that comment 31 (in which this was set to blocking1.9.1- & wanted1.9.1+) indicates that this *needs* fixing on 1.9.1, but it just wouldn't hold the release. Now we've got a fix, so it should hopefully land on 1.9.1 after baking a bit on mozilla-central.

[Beltzner (beltzner)]

Comment on attachment 372739
use new_font
[Checkin: Comment 39 & 42]

a191=beltzner

[Sgautherie-bz (sgautherie-bz)]

Comment on attachment 372739
use new_font
[Checkin: Comment 39 & 42]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9de5cf733d70