1279785 - font rendering regression with xrender disabled

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

See https://bugzilla.redhat.com/show_bug.cgi?id=1343947

Comment 1

[Sylvain Pasche]

Looks like Firefox with xrender disabled ignores the lcdfilter setting that is set on Xrdb ("Xft.lcdfilter" set by gnome-setting-daemon).

It's taken into account when set through fontconfig:
mkdir -p ~/.config/fontconfig/conf.d/
cat > ~/.config/fontconfig/conf.d/10-lcdfilter-default.conf << EOF
<?xml version="1.0"?>

<!-- https://bugzilla.mozilla.org/show_bug.cgi?id=1279785 -->

<!DOCTYPE fontconfig SYSTEM "fonts.dtd">
<fontconfig>
  <match target="font">
    <edit name="lcdfilter" mode="assign"><const>lcddefault</const></edit>
  </match>
</fontconfig>
EOF

Chrome has a similar behavior, it ignores the hinting style set from xrdb and I have another fontconfig rule to set it to hintslight.

Comment 2

[Lee Salzman [:lsalzman]]

Can you test the following experimental build to see if it fixes your issue? http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-e95afc09e5fe040d0ec4d96be07e64951602d3f4/try-linux64/firefox-50.0a1.en-US.linux-x86_64.tar.bz2

Try removing the extra fontconfig stuff you added and check if it picks up the right lcdfilter still. If so, then I can rework this into a better fix.

Comment 3

[Lee Salzman [:lsalzman]]

(In reply to Lee Salzman [:lsalzman] from comment #2)
> Can you test the following experimental build to see if it fixes your issue?
> http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> e95afc09e5fe040d0ec4d96be07e64951602d3f4/try-linux64/firefox-50.0a1.en-US.
> linux-x86_64.tar.bz2
> 
> Try removing the extra fontconfig stuff you added and check if it picks up
> the right lcdfilter still. If so, then I can rework this into a better fix.

Comment 4

[Sylvain Pasche]

Hi Lee,
I tried that build, but it doesn't fix the issue unfortunately: It doesn't pick the lcddefault lcdfilter set on Xrdb if I remove my fontconfig file.

xrdb -query|grep lcd
Xft.lcdfilter:	lcddefault

Comment 5

[Karl Tomlinson (:karlt)]

I don't think the pattern is used after
https://hg.mozilla.org/try/rev/b161b7ffba59#l1.193

https://hg.mozilla.org/mozilla-central/annotate/3c5025f98e56/gfx/thebes/gfxFcPlatformFontList.cpp#l1729 would be the place to apply screen font options.

Comment 6

[Lee Salzman [:lsalzman]]

(In reply to Sylvain Pasche from comment #4)
> Hi Lee,
> I tried that build, but it doesn't fix the issue unfortunately: It doesn't
> pick the lcddefault lcdfilter set on Xrdb if I remove my fontconfig file.
> 
> xrdb -query|grep lcd
> Xft.lcdfilter:	lcddefault

Yet another build for you to try: http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-e4567b04181fb84e687bd581969dfac4ecfba6c7/try-linux64/firefox-50.0a1.en-US.linux-x86_64.tar.bz2

See if this one fixes it.

Comment 7

[Sylvain Pasche]

This one fixes the issue. Thanks!

Comment 8

[Lee Salzman [:lsalzman]]

Attachment: check for Xft resources when creating fontconfig fonts

This duplicates the nastiness that the Cairo xlib backend was doing with font options. It only tries to use the Xft resources if the fontconfig resources are not available. It also tries to be compatible with Cairo's Xft parsing code.

Mainly of interest for this bug is the lcdfilter setting, which Cairo does not expose any generic means of setting through its API on font options. So we have to supply it on the FcPattern, making sure to only set it there after the FcPattern has been fully constructed so we don't accidentally overwrite the existing value, and then let Cairo detect it from that. Other settings beside lcdfilter can thankfully go through normal font options settings.

... It ain't pretty, but it works, at least.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 8765521 [details] [diff] [review]
check for Xft resources when creating fontconfig fonts

(In reply to Lee Salzman [:lsalzman] from comment #8)
> This duplicates the nastiness that the Cairo xlib backend was doing with
> font options. It only tries to use the Xft resources if the fontconfig
> resources are not available. It also tries to be compatible with Cairo's Xft
> parsing code.

>     FcBool hinting = FcFalse;
>     if (FcPatternGetBool(aPattern, FC_HINTING, 0, &hinting) != FcResultMatch) {
>+#ifdef MOZ_X11
>+      if (!GetXftBool(display, "hinting", hinting))
>+#endif
>         hinting = FcTrue;

The font options process is complex.  A large part of that is due to combining
screen font options and letting fontconfig rules adjust those settings.  cairo
has a number of bugs.  libXft is the reference, but there are some times when
rules should be overridden: e.g. no hinting when rendering to surface
without pixels etc.  I guess this is what cairo is trying to handle but
struggles.

However the options set in PrepareFontOptions() will be merged into the
surface font options, overriding the surface font options, so cairo was never
applying surface font options for hinting at that point.

Screen font options for hinting were already applied in
ApplyGdkScreenFontOptions().

In PrepareFontOptions(), if that value for |hinting| has been changed, it is
because a fontconfig rule has decided to change it.  These rules can consider
the provided surface (screen or printer) options and override for specific
fonts or situations.  The rules are more flexible than Xft defaults and take
precedence over Xft defaults.

This part of the patch is fixing something that isn't broken, and unfixing it.

> Mainly of interest for this bug is the lcdfilter setting, which Cairo does
> not expose any generic means of setting through its API on font options.

>+    FcValue fc_value;
>+    int lcdfilter;
>+    if (FcPatternGet(aRenderPattern, FC_LCD_FILTER, 0, &fc_value) == FcResultNoMatch &&
>+        GetXftInt(DefaultXDisplay(), "lcdfilter", lcdfilter)) {
>+      FcPatternAddInteger(aRenderPattern, FC_LCD_FILTER, lcdfilter);
>+    }

Yes, this is the Xft default not already considered.

Please wrap to keep lines within 80 columns.

> So
> we have to supply it on the FcPattern, making sure to only set it there
> after the FcPattern has been fully constructed so we don't accidentally
> overwrite the existing value, and then let Cairo detect it from that.

https://dxr.mozilla.org/mozilla-central/rev/b69a5bbb5e40bd426e35222baa600b481e50d265/gfx/thebes/gfxFcPlatformFontList.cpp#745
is the place to apply this screen option.

It applies only for rendering to the screen, and permits fontconfig rules to 
fine tune in PrepareFontOptions.

>+{
>+  if (!aDisplay) {

Please follow the indentation level of the file.

>+GetXftInt(Display* aDisplay, const char* aName, int& aResult)

"Use pointers instead of references for function out parameters, even for
primitive types."

>+  if (FcNameConstant(reinterpret_cast<FcChar8*>(value), &aResult)) {

ToFcChar8Ptr(value)

>+  char* end = nullptr;
>+  aResult = strtol(value, &end, 0);
>+  if (end && end != value) {

|end| will not be null, so no need to check.
(Even if it could be null, end != value because value != null.)
Initialization of |end| is unnecessary.

Comment 10

[Lee Salzman [:lsalzman]]

So I am confused then. Aside from lcdfilter, are you saying all the other resources shouldn't be checked at all? And you want lcdfilter checked in ApplyGdkScreenOptions?

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to Lee Salzman [:lsalzman] from comment #10)
> Aside from lcdfilter, are you saying all the other
> resources shouldn't be checked at all?

Yes, thanks.

> And you want lcdfilter checked in ApplyGdkScreenOptions?

Either ApplyGdkScreenOptions() or the place where PreparePattern() calls ApplyGdkScreenOptions(), please.

lcdfilter is not a GDK option so please rename to ApplyScreenOptions() if doing that there.

Comment 12

[Lee Salzman [:lsalzman]]

Attachment: check for Xft lcdfilter resource when creating fontconfig fonts

Fixed issues brought up with earlier patch. Just checks lcdfilter in PreparePattern and fixes other style issues.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e749fcac8cb3
check for Xft lcdfilter resource when creating fontconfig fonts. r=karlt

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/903eba9d0ef1 for bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=32244922&repo=mozilla-inbound

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdefc01b91df
check for Xft lcdfilter resource when creating fontconfig fonts. r=karlt

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fdefc01b91df