Bug 13566

Created attachment 12998 [details] [review]
Patch against fontconfig-2.5.0 adding options.

This patch adds a new option with 4 configuration constants. It is modified from one available at http://packages.ubuntu.com/gutsy/libs/libfontconfig1

Ubuntu developers originated this patch, albeit with the error of creating name constants that conflicted with existing ones. This patch modifies the original by prefixing the constant strings with "lcdfilter".

the enumerations use lcdfilter and the define lcd_filter is that normal?

Created attachment 13254 [details] [review]
Patch with correct define.

(In reply to comment #2)
> the enumerations use lcdfilter and the define lcd_filter is that normal?
> 
Nope, that was wrong. I've changed the patch.

I discussed this issue with Keith the other day:

17:48 < syp|> keithp: what do you think about https://bugs.freedesktop.org/show_bug.cgi?id=13566 ?
17:49 < keithp> syp|: fontconfig doesn't need these values, but cairo can add them itself if desired
17:50 < keithp> fontconfig allows applications to add their own variables to fontconfig
17:51 < keithp> you can do precisely the same operation from cairo code
17:51 < keithp> just add the values and constants using the existing public fontconfig API
17:51 < keithp> then fontconfig will happily parse those values appropriately
17:52 < syp|> keithp: is this way of doing already used for other configuration options?
17:53 < keithp> syp|: not in cairo, but I think Xft may use this
17:53 < keithp> yeah, FcNameRegisterConstants, FcNameRegisterObjectTypes
17:55 < syp|> ok thanks for the information. so this particular bug should be WONTFIXed?
17:57 < keithp> please see if you can make it work from cairo, then we can close the bug

So I tried to use the FcNameRegisterConstants and FcNameRegisterObjectTypes functions to register the lcdfilter object type and the lcdfilter* constants. However, I could see a time dependency issue when doing this:

FcNameRegisterConstants() has to be called before the fontconfig configuration file is read, otherwise the parser is not aware of the defined constants and do not fill the pattern with them.

This can work when using the toy API and registering the constants from _cairo_ft_scaled_font_create_toy(), because the fontconfig configuration file is parsed later when FcDefaultSubstitute() is called (so it knows about the constants at this time).

But it fails when using Pango, because it deals with FontConfig before calling into the cairo API, so that the fontconfig configuration is already parsed when we are in cairo-ft-font.c

I agree that this probably isn't the best place for these options. This bug is here because the fontconfig patch was part of the original Ubuntu patchset, and I was unaware it was possible to register configuration objects from outside fontconfig itself. 

Sylvain, the fontconfig configuration never really worked with the Ubuntu patchset. In fact, the cairo patch currently only uses the fontconfig options for name-to-constant mapping. This said, I'd say there are two courses of action we can take from this standpoint:

1. Discard the fontconfig configuration of this value, use the freetype constants, and load the configuration from the X resource database, handling the string constants in cairo. This is the only method working now anyway.

2. Figure out how to get the fontconfig object registration working even with pango, moving all constants and configuration to cairo.

With either path, I really agree that, given the availability of that config API, this patch would be unnecessary, and this bug can probably be closed.

(In reply to comment #5)
> 1. Discard the fontconfig configuration of this value, use the freetype
> constants, and load the configuration from the X resource database, handling
> the string constants in cairo. This is the only method working now anyway.
> 
> 2. Figure out how to get the fontconfig object registration working even with
> pango, moving all constants and configuration to cairo.

I'll let Keith pronounce if this is feasible. Possibilities I see:
* reparse the configuration file after the constants have been registered (not sure fontconfig allows this, not so nice for performance).
* introduce a cairo_init() method that cairo consumers have to call before touching fontconfig (adds burden/incompatibility to cairo users).

Otherwise, there is a third possibility: Keep this parameter in fontconfig and change cairo to take it into account, as this is done for antialiasing, subpixel order or hinting.

I'm questioning having such an option at all, see http://lists.cairographics.org/archives/cairo/2008-January/012685.html

(In reply to comment #6)
> I'll let Keith pronounce if this is feasible. Possibilities I see:
> * reparse the configuration file after the constants have been registered (not
> sure fontconfig allows this, not so nice for performance).
> * introduce a cairo_init() method that cairo consumers have to call before
> touching fontconfig (adds burden/incompatibility to cairo users).
> 
> Otherwise, there is a third possibility: Keep this parameter in fontconfig and
> change cairo to take it into account, as this is done for antialiasing,
> subpixel order or hinting.
> 
> I'm questioning having such an option at all, see
> http://lists.cairographics.org/archives/cairo/2008-January/012685.html

While I personally agree that the "Default" filter is adequate, we still need to have at least the "Legacy" filter available. The loss of this more-known, thinner filter would probably cause outrage among those who consider the new filters "too blurry." 
In that regard, I think that having "Default" as the default, but having only Xrdb configuration to set the other filters would be sufficient.

As been already pointed out, registering in a library is not possible.  However if we are going to do it in cairo anyway, in a cairo namespace, then we can forget about registering constants and types and just work with strings.  That's whay pango does for pangogravity element for example.

I still think this thing belongs into fontconfig because FreeType exposes this configuration.  It's not a cairo-only thing.

well, given that we can't make this work in any library, it seems like the whole plan of building application-specific fontconfig constants is a loss. Sigh.

I'll plan on adding this stuff for 2.6, please put together a patch that includes the necessary documentation additions and I'll apply it.

Created attachment 13713 [details] [review]
lcd filter type objects/constants and documentation update

As discussed on irc, I renamed the constants from lcdfilter* / FC_LCD_FILTER_* to lcd* / FC_LCD_*.

Applied in 53aec111074cf7b46d15eb84a55791d3c95bc15e