Comment on attachment 42713
Revised version of respect-fontconfig patch against 1.10.2 version.

Review of attachment 42713:
-----------------------------------------------------------------

(In reply to comment #16)

Thanks for the helpful overview of the changes.

> * _cairo_ft_options_merge - changes here are small and focus on having the same
> algorithm for all font properties: if other structure define a property (it
> does not equal CAIRO_*_DEFAULT), this value will be used.

I'd like to agree in principle, but we need at least one exception to this
rule.

If FT_LOAD_NO_HINTING is not set, but hint_metrics is CAIRO_HINT_METRICS_OFF,
x_advance/y_advance metrics will not be hinted, but x/y_bearing/width/height
metrics and outlines will still be hinted.

https://bugzilla.mozilla.org/show_bug.cgi?id=490475#c5 says "cairo PDF/PS
always embeds (and subsets) the fonts used. If subsetting fails it will
fallback to embedding a font generated from the unhinted outlines of the
glyphs."
And https://bugzilla.mozilla.org/show_bug.cgi?id=490475#c19 says "Hintstyle
should also be set to none for printing although this only affects the use of
fallback fonts where cairo embeds a font created from the glyph outlines."

That means that a HINT_STYLE_NONE on the surface will need to override
fontconfig.  I guess we could argue that users shouldn't have fontconfig
settings that turn on hinting if it has been turned off, but history has shown
that people will use fontconfig to turn on hinting, overriding an explicitly
set value in the pattern.

There are also times when the font_options should be able to override the
cairo antialias option from fontconfig.  This would be beneficial if the
destination surface is monochrome or grayscale and so doesn't support the same
antialiasing options, or if the destination is a translucent part of a
CONTENT_COLOR_ALPHA surface and so component-alpha will be lost before the
glyphs are composited onto an opaque background (leading to excessive color
fringing).  I think what we want for cairo antialias is not a priority of
fontconfig over other interests, but the greatest common method among those
supported and desired.  The current code doesn't provide this exception,
however.

> * _get_pattern_ft_options - this routine loads FontConfig information into
> cairo_ft_options_t structure. 
> 
> ** The most significant change here covers the fact, that hinting information
> wasn't processed if antialiasing was switched off. 

Yes, processing hinting and hintstyle to determine whether to hint seems the
right thing to do (even if I don't know of a good reason, apart from testing
perhaps, why anyone would want to turn off hinting through fontconfig if
antialias has been set to false).

> ** Another change in behavior applies to antialiasing information - basically
> there are 7 different combination of FontConfig properties ('antialias' and
> 'rgba') values that define different behavior, but they cannot be naturally
> represented in cairo_ft_options_t structure (in theory it could be done, but
> with extremely nasty approach). As changing the contract and defining
> additional value for _cairo_subpixel_order type would be rather wrong idea, the
> routine will assume user wanted to switch on antialiasing when the 'rgba'
> property is defined.

There is an awkwardness in the mapping from fontconfig's rgba to cairo's
subpixel_order and antialias.  It looks like the current
_get_pattern_ft_options code is not achieving the desired result here because
there is no SUBPIXEL_ORDER_NONE and it doesn't change antialias from DEFAULT
to GRAY for FC_RGBA_NONE, and so it won't override a surface default
ANTIALIAS_SUBPIXEL.  A SUBPIXEL_ORDER_NONE would be worth considering
as there are device surfaces that are not linear arrays of RGB colors.

The patch also sets ANTIALIAS_SUBPIXEL if the pattern has antialias == true
but FC_RGBA_UNKNOWN or missing.  I'm cautious about the benefit of this change
as antialiasing would be the default in _cairo_ft_options_merge anyway.  I
guess this is so that someone can have antialias turned off on the screen
settings but enable it through fontconfig only for certain fonts.

However, this would turn on grayscale aa for surfaces with ANTIALIAS_NONE, and
perhaps even subpixel aa for surfaces with antialias GRAY or NONE, if the
subpixel_order has not been cleared out.  Those could both be less than ideal.
Clients would not expect to have to clear out the subpixel order because the
documentation says subpixel order is only used "when rendering with an
antialiasing mode of CAIRO_ANTIALIAS_SUBPIXEL."  These issues already can
happen when fontconfig has rgba set, but I fear this part of the patch would
not move things in the right direction.

The Fc prefix indicates a fontconfig symbol but FcUndefined is not a
fontconfig symbol.  Perhaps it would be better/clearer to just use -1, though
assigning a non-true/false value to FcBool doesn't feel quite right, even if
it really is just an int.  It looks like we may not need a special value at
all if the antialias=true/rgba=unknown case is removed.