Created attachment 183971 [details]
gnome-shell panel vs application's top bar

As you can see on the image - hinting is correctly applied to the application's top bar, but not to the gnome-shell's panel.

What exactly did you do? The shell font currently cannot be changed (lest editing the stylesheet), but as of bug 599713 hinting and antialiasing should be picked up correctly (a quick test doesn't suggest any regressions).

I did change font rendering settings using dconf-editor... and took a screenshot.

Note, my bug report was created in March, while the fix has been pushed to live at the beginning of May :)

I'll test it and post my results here.

(In reply to comment #3)
> Note, my bug report was created in March, while the fix has been pushed to live
> at the beginning of May :)

Nope, this bug report is from March *2011*, the fix was pushed in May *2010* - so that's not it ...

I am writing this post using Fedora 15 Live CD, running gnome-shell and Gnome 3.0.

I've installed dconf-editor and gnome-tweak-tool, I've changed font rendering settings from greyscale to rgba, hinting from medium to slight... although I can see font rendenring has changed - it's more fussy now, but not rgba, still greyscalled. see screenshot attached.

Created attachment 189053 [details]
screenshot of not respected font settings

just to confirm - I did restart gnome shell to make sure, changes were applied.

FYI, your latest screen shot doesn't have rgb hinting for the applications either.

I am seeing the same issue here though. Everything respects the hinting settings except for gnome-shell. It is consistently ignored though, not just some elements.

This is on Fedora 16, so gnome-shell 3.2.

Should bug 599713 be reopened?

Looking at the patch for bug 599713, it seems like that patch was simply bad. I only considers no antialias and gray, not rgb. So more work is needed.

Created attachment 201169 [details] [review]
Support subpixel font rendering

Attached patch works like a charm here. The comment about mutter not supporting it no longer applies I guess.

Please review and apply. :)

Could we get this patch applied? Or comments on what's preventing it from getting applied?

Review of attachment 201169 [details] [review]:

As far as I can tell, the limitations in Clutter still exist, so I don't think you're actually seeing subpixel rendering here...

Created attachment 208424 [details]
screenshot of g-s 3.2.2.1 with the subpixel rendering patch

This is how the calendar of a patched g-s looks on Fedora 16 with subpixel-enabled freetype, slight hinting, lcddefault lcd filter setting and DejaVuSans font instead of Cantarell.

Created attachment 208425 [details]
screenshot of eog showing the screenshot above 4x magnified

That should settle it then. :)

As Ilja alluded to, Fedora's freetype doesn't support subpixel rendering. So you need to test this on a system with a fully featured one. There is a replacement one for Fedora available in rpmfusion. No idea what the state of the other major distributions is.

(In reply to comment #14)

> As Ilja alluded to, Fedora's freetype doesn't support subpixel rendering. 
> So you need to test this on a system with a fully featured one. [...]
> No idea what the state of the other major distributions is.

AFAIK all except of openSUSE ship subpixel-enabled freetype packages, though only Ubuntu and its derivatives ship with subpixel rendering + slight hinting + lcdfilter=default as default setting.

How subpixel rendering can be enabled in freetype on Fedora is mentioned on line 2 of <http://pkgs.fedoraproject.org/gitweb/?p=freetype.git;a=blob;f=freetype.spec>

(In reply to comment #10)
> Could we get this patch applied? Or comments on what's preventing it from
> getting applied?

The code to do sub-pixel AA in clutter is incorrect - the math is simply wrong. Beyond that, at a theoretical level, there's a second problem which is that you cannot do sub-pixel AA correctly when a temporary buffer is involved - that would require a 6 channel temporary buffer. Un some cases clutter automatically will use such a temporary buffer. That problem may possibly be ignorable.

Do you have any idea when this will go wrong? And what the result will be? I haven't checked every portion of the shell, but what I've seen has rendered correctly. So it seems to work well in practice.

The patch works incredibly well on my system and makes a huge difference. Thanks Pierre. I hope that problems preventing its inclusion may be sorted out, since the Shell looks much better after applying it.

I just want to point out that Clutter already has similar code. This will be fixed if the patches from bug #672807 land.

(In reply to comment #19)
> I just want to point out that Clutter already has similar code. This will be
> fixed if the patches from bug #672807 land.

well, if by fixed, you mean, "rendering an incorrect result that looks mostly similar to what the user requested"

(In reply to comment #20)
> (In reply to comment #19)
> > I just want to point out that Clutter already has similar code. This will be
> > fixed if the patches from bug #672807 land.
> 
> well, if by fixed, you mean, "rendering an incorrect result that looks mostly
> similar to what the user requested"

What else can we give them?

There are two errors involved:

 A) The incorrect compositing math in Cogl. This is fixable as long as Cogl is rendering directly to the screen.

 B) The inherent error from temporary surfaces. Let me explain this in detail - consider drawing white glyphs onto a  temporary surface with a transparent background and then compositing that down against a solid background. If the solid background is black, then we should have blue fringes on the right and red fringes on the left for a vertical stem.

 rgb|rgb|rgb
    xxxxxxx

If the background is white, we should have no fringes at all. If we choose early - when we still have a transparent background, we don't know, so we risk having white glyphs outlined with ghostly fringes.

So, what would be useful is making mockups of the correct result and the result we're getting now for, say, the scrolled application list in the overview. What I think we should make sure that we aren't giving people fringes, any fringes, but giving them fringes that make things look better.

Review of attachment 201169 [details] [review]:

We now respect ClutterSettings, instead of rolling our own font options. This patch is obsolete. Cogl still has to be fixed.

Owen, do you know of a bug for CoglPango/Cogl/Clutter re: wrong math?

I'm not sure my case is the same as described on this bug:

I've connected my PC on the TV, but with 1920x1080 resolution (needed to watch HD movies) text are too small to read. I've changed font size in gnome-tweak-tool which makes all apps much easier to use, but gnome-shell does not respect that setting. The panel, password request dialog, etc, are all still too small to read.

Which version of Gnome Shell is required for this? I upgraded to 3.4 (would have been 3.6 if Fedora could get F18 in order) and that seems to be insufficient.

Created attachment 227889 [details] [review]
Support subpixel font rendering (3.4)

Checked the code, and 3.4 is apparently too old. Attached an updated patch for those also running 3.4.

It was landed in 3.6.

http://git.gnome.org/browse/gnome-shell/commit/src/shell-global.c?h=gnome-3-6&id=dc3d3acb3b6ce16303ee1cd08ff075817f923243

If anything, we should just cherry-pick that for gnome-3-4. I think the Clutter there is new enough.

Confirmed working (modulo Owen's gotchas) on 3.6.

Let's move this to cogl to let them fix it.

a new bug would have been better.

Owen: can you clarify comment 22?

(In reply to comment #31)
> a new bug would have been better.
> 
> Owen: can you clarify comment 22?

Can you be more specific about what aspects of comment 22 are unclear?

point A:

> A) The incorrect compositing math in Cogl. This is fixable as long as Cogl is
> rendering directly to the screen.

what would be the correct compositing math? is it a bug in the Cogl Pango renderer when submitting geometry, or is it inside the glyph cache when generating the glyph masks?

The basic idea is that compositing subpixel glyphs requires specific math, if I remember correctly:

 dest_a = source_a * mask_a + (1 - source_a * mask_a) * dest_a
 dest_r = source_r * mask_r + (1 - source_a * mask_r) * dest_r
 dest_g = source_g * mask_g + (1 - source_a * mask_g) * dest_g
 dest_b = source_b * mask_b + (1 - source_a * mask_b) * dest_b

Instead of this, the way that blending is done in the cogl-pango is:

 dest_a = source_a * mask_a + (1 - source_a * mask_a) * dest_a
 dest_r = source_r * mask_r + (1 - source_a * mask_a) * dest_r
 dest_g = source_g * mask_g + (1 - source_a * mask_a) * dest_g
 dest_b = source_b * mask_b + (1 - source_a * mask_a) * dest_b

(The comment in cogl-pango-pipeline-cache.c:get_base_texture_alpha_pipeline() mentions what the default combine mode is. get_base_texture_rgba_pipeline() just uses that, which gives us a fragment of source_a * mask_a, source_r * mask_r, source_g * mask_g, source_b * mask_b. Then we just leave the default blending mode in place, giving the above equations.)

So the result is correct if the background is black, but not otherwise. In some cases you can do it in a single pass with a blend factor of 1 - SRC_COLOR - I think this is the case where source_a == source_r == source_g == source_b - white, black, or gray - but check my math. In other cases, two passes are needed.

See also the comment for exaTryMagicTwoPassCompositeHelper() in http://cgit.freedesktop.org/xorg/xserver/tree/exa/exa_render.c.

If you have an intermediate texture - if you, are, say, applying an effect to a Cogl actor - then this doesn't work out at all, because we no longer have a separate source color and 4-channel mask, we just have a combined argb value.

This is still an open issue for Wayland sessions:
  https://bugs.launchpad.net/ubuntu/+source/gnome-shell/+bug/1714459

Should we start a new bug?

Well, if it's a problem in GNOME Shell then this issue should be re-assigned to Mutter, otherwise the Shell developers won't ever be aware of it.

I'm trying to learn the mutter code and fix this right now so you could assign it to me. Still wouldn't mind if someone else fixed it in the mean time.

Created attachment 361177 [details] [review]
Subpixel font rendering and more for Wayland shells

A patch to try and close this bug, finally.

Created attachment 361212 [details] [review]
0001-eglnative-Use-gnome-settings-daemon-font-settings.patch

Patch version 2.

Created attachment 361215 [details] [review]
fix-645433-v3.patch

Version 3: Fix typo and other documentation.

Review of attachment 361215 [details] [review]:

This is a good improvement, I'm not in love of the usage of this gsettings key as you know, but... If we don't move it now (I would), this is fine.

Unfortunately the RGB subpixel isn't available for wayland anyway, but it's in a different level.

Some thoughts on code:

::: clutter/clutter/egl/clutter-backend-eglnative.c
@@ +64,3 @@
 
+  g_signal_handlers_disconnect_by_data (backend_egl_native->xsettings,
+                                        backend_egl_native);

This is pointless here, since you'll clear the object anyway, so no risk that it will emit any signal.

Being a backend it's probably not needed either if you clear, while you can instead use connect_object. Just the clear is fine for me.

@@ +145,3 @@
+
+  hinting = g_settings_get_enum (xsettings, "hinting");
+  if (hinting >= 0 && hinting < G_N_ELEMENTS (hintings))

G_N_ELEMENTS is defined using sizeof, so an unsigned, you might need to cast this.

@@ +152,3 @@
+  else
+    {
+      output->cairo_hint_style = CAIRO_HINT_STYLE_NONE;

use CAIRO_HINT_STYLE_DEFAULT

@@ +153,3 @@
+    {
+      output->cairo_hint_style = CAIRO_HINT_STYLE_NONE;
+      output->clutter_font_hint_style = "hintnone";

Then in this case there's no need to set any clutter hint style I think

@@ +164,3 @@
+  else
+    {
+      output->cairo_antialias = CAIRO_ANTIALIAS_GRAY;

Use CAIRO_ANTIALIAS_DEFAULT instead

@@ +176,3 @@
+  else
+    {
+      output->cairo_subpixel_order = CAIRO_SUBPIXEL_ORDER_RGB;

And here too CAIRO_SUBPIXEL_ORDER_DEFAULT

@@ +197,3 @@
+  options = cairo_font_options_create ();
+  if (!options)
+    return;

g_return_if_fail probably here is wanted as I guess it's something we expect to have, no?

@@ +203,3 @@
+  cairo_font_options_set_antialias (options, current.cairo_antialias);
+  cairo_font_options_set_subpixel_order (options, current.cairo_subpixel_order);
+  clutter_backend_set_font_options (backend, options);

I didn't check, but this is also initializing the ClutterSettings properties, right?

@@ +225,3 @@
+  ClutterSettings *csettings = clutter_settings_get_default ();
+  GValue value = G_VALUE_INIT;
+  FontSettings current;

maybe just fs? Current isn't really explicative here.

@@ +247,3 @@
+  g_value_set_string (&value, current.clutter_font_subpixel_order);
+  clutter_settings_set_property_internal (csettings, "font-subpixel-order", &value);
+  g_value_unset (&value);

I would remove the hassle of gvalue here and just use g_object_set:

  g_object_set (csettings,
                "font-hinting", 1,
                "font-antialias", current.clutter_font_antialias,
                "font-hint-style", current.clutter_font_hint_style,
                "font-subpixel-order", current.clutter_font_subpixel_order,
                NULL);

Using g_intern_string in case, but I don't see the point.

@@ +257,3 @@
   backend_egl_native->event_timer = g_timer_new ();
+
+  backend_egl_native->xsettings = g_settings_new ("org.gnome.settings-daemon.plugins.xsettings");

I'd put this in a define

@@ +259,3 @@
+  backend_egl_native->xsettings = g_settings_new ("org.gnome.settings-daemon.plugins.xsettings");
+
+  if (backend_egl_native->xsettings)

This won't actually ensure that the settings exist, but if they don't it will assert badly.

So, it's probably saner to use g_settings_schema_source_lookup with this source, and if if found you can create the gsettings using g_settings_new_full.

Maybe a gwarning stating that we miss this could be useful too.

@@ +262,3 @@
+    {
+      clutter_backend_egl_native_init_font_options (backend_egl_native);
+      g_signal_connect (backend_egl_native->xsettings, "change-event",

Why not "changed"?

And in the callback you could even ignore changes to the keys we don't need, or instead add 3 connections with changed::key-name (probably not the nicest anyway).

Created attachment 362243 [details] [review]
fix-645433-v4.patch

Patch version 4.

Review of attachment 362243 [details] [review]:

::: clutter/clutter/egl/clutter-backend-eglnative.c
@@ +138,3 @@
+  };
+
+  guint hinting, antialiasing, rgba_order;

I find these a bit hard to read... as values[value].. 

Since you don't need to preserve the vars, you can just initialize a guint value and use it for every enum.

@@ +152,3 @@
+       */
+      output->cairo_hint_style = CAIRO_HINT_STYLE_NONE;
+      output->clutter_font_hint_style = "hintnone";

What I was suggesting was to not sending such info to clutter at all in case, but in any case I think you can just add support for such values in settings_update_font_options too, as it will reconvert these in cairo settings again, so there's no point of changing it since we've all the codepath in control.

@@ +193,3 @@
+  FontSettings fs;
+
+  g_return_if_fail (xsettings = backend_egl_native->xsettings);

I preferred just an "if (!xsettings) here", since this will cause a warning which we already have triggered on _init.

@@ +194,3 @@
+
+  g_return_if_fail (xsettings = backend_egl_native->xsettings);
+  g_return_if_fail (options = cairo_font_options_create ());

I don't think this could fail either, but if you want to add the g_return, do the initialization first, as doing it in there is quite confusing.

@@ +259,3 @@
+      if (!backend_egl_native->xsettings)
+        {
+          g_warning ("Failed to open settings for %s", xsettings_path);

Honestly I don't think this can happen, if we've the schema, well get something here.

No need to over-check imho.

Created attachment 362324 [details] [review]
fix-645433-v5.patch

Patch version 5.

Still pretty much unchanged logic for the last few versions. Only formatting changes for Marco.

Review of attachment 362324 [details] [review]:

Ok, looks good now

::: clutter/clutter/egl/clutter-backend-eglnative.c
@@ +137,3 @@
+    /* vbgr=4 */ {CAIRO_SUBPIXEL_ORDER_VBGR, "vbgr"},
+  };
+  guint i;

Ehehe, you went minimal for real now :).

Review of attachment 362324 [details] [review]:

Functionality wise it looks good. My comments are regarding readability and coding style.

::: clutter/clutter/egl/clutter-backend-eglnative.c
@@ +94,3 @@
+
+static void
+get_font_gsettings (GSettings *xsettings, FontSettings *output)

Function arguments goes on their own lines, with the argument variable names aligned

@@ +183,3 @@
+{
+  GSettings *xsettings = backend_egl_native->xsettings;
+  cairo_font_options_t *options = NULL;

There is no need to initialize this.

@@ +187,3 @@
+
+  if (!xsettings)
+    return FALSE;

You only ever call this after already having checked this, so why check it again?

@@ +189,3 @@
+    return FALSE;
+
+  options = cairo_font_options_create ();

This only happens on OOM, so no need to handle it. It's not handled in other places in clutter, so lets keep things consistent.

@@ +204,3 @@
+  cairo_font_options_destroy (options);
+
+  return TRUE;

Given the two above comments, you can stop making this function look like it might fail when it will never.

Review of attachment 362324 [details] [review]:

::: clutter/clutter/egl/clutter-backend-eglnative.c
@@ +183,3 @@
+{
+  GSettings *xsettings = backend_egl_native->xsettings;
+  cairo_font_options_t *options = NULL;

Agree.

@@ +187,3 @@
+
+  if (!xsettings)
+    return FALSE;

I was about to say the same, but then I noticed it could be still the case where you have initialized but there's no such settings schema, thus here it's null, but not an error.

(In reply to Marco Trevisan (Treviño) from comment #47)
> Review of attachment 362324 [details] [review] [review]:
>
> @@ +187,3 @@
> +
> +  if (!xsettings)
> +    return FALSE;
> 
> I was about to say the same, but then I noticed it could be still the case
> where you have initialized but there's no such settings schema, thus here
> it's null, but not an error.

The function init_font_options() is only called if the backend_egl_native->xsettings is non-NULL (in clutter_backend_egl_native_init()).

(In reply to Jonas Ådahl from comment #48)
> The function init_font_options() is only called if the
> backend_egl_native->xsettings is non-NULL (in
> clutter_backend_egl_native_init()).

Ah you're right, I think it wasn't like this in the previous version, thus I didn't pay attention

Created attachment 362385 [details] [review]
fix-645433-v6.patch

Patch version 6.

Review of attachment 362385 [details] [review]:

lgtm

Pushed as https://git.gnome.org/browse/mutter/commit/?id=a37956c

I don't think we need this for gnome-3-26, right?

Well I developed it against the gnome-3-26 branch, so it definitely works there. But it's not an important fix to backport to 3.26.

We definitely want the fix in Ubuntu 18.04 at least.

If there's a chance 18.04 will still be on Gnome 3.26 then yes this fix should be applied to gnome-3-26. That would be a bit nicer than having to distro patch it in 18.04.

(In reply to Daniel van Vugt from comment #54)
> If there's a chance 18.04 will still be on Gnome 3.26 then yes this fix
> should be applied to gnome-3-26.

No. If 18.04 will still be on Gnome 3.26, then it would be more convenient for you if the fix was on the gnome-3-26 branch.

To be clear: In this particular case, pushing the patch to the gnome-3-26 branch is fine with me, and if the patch makes it in today I'll include it in the 3.26.2 release.

But please don't imply that upstream has any obligation to comply with any downstream decision Ubuntu (or any other distribution) makes. Not too long ago, this would have meant freezing private API between gnome-shell, gnome-settings-daemon and gnome-control-center, because Ubuntu was mixing core components from three stable releases ...

Whoa, peace. I didn't mean to imply that "should" is a strong word.

- should
+ ideally would

I don't really mind which way it goes. I would just like to know so that a decision doesn't get deferred till April and we're left with an LTS without the fix.

I am happy to distro-patch this just for Ubuntu but at the moment there are no Ubuntu-specific patches on mutter. I thought it would be nice to suggest as a first preference that this not become the singular reason we deviate from the Debian debs. And now this discussion should really move back into Launchpad...

(In reply to Florian Müllner from comment #55)
> To be clear: In this particular case, pushing the patch to the gnome-3-26
> branch is fine with me, and if the patch makes it in today I'll include it
> in the 3.26.2 release.

Since there were no objections, I've pushed this to gnome-3-26 as https://git.gnome.org/browse/mutter/commit/?h=gnome-3-26&id=d358e, so in in any case it will hit the users using 3.26.3 (including ubuntu for sure).