After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 630426 - Metacity 2.30.2 breaks many themes
Metacity 2.30.2 breaks many themes
Status: RESOLVED OBSOLETE
Product: metacity
Classification: Other
Component: general
2.30.x
Other All
: Normal critical
: ---
Assigned To: Metacity maintainers list
Metacity maintainers list
Depends on:
Blocks:
 
 
Reported: 2010-09-23 15:16 UTC by Brandon Wright
Modified: 2015-04-15 14:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to resolve theme issues (2.66 KB, patch)
2010-09-23 15:17 UTC, Brandon Wright
none Details | Review
An image for comparison purposes. (28.11 KB, image/png)
2010-09-25 17:22 UTC, Brandon Wright
  Details
Patch to fix bugs, but not diagonal antialiasing. (1.65 KB, patch)
2010-09-25 18:04 UTC, Brandon Wright
committed Details | Review
Additional: turn off antialiasing for diagonals (497 bytes, patch)
2010-09-30 11:02 UTC, Brandon Wright
none Details | Review
Clean up antialiasing fixes (2.95 KB, patch)
2010-10-06 17:09 UTC, Owen Taylor
committed Details | Review

Description Brandon Wright 2010-09-23 15:16:23 UTC
The change which removed GDK drawing (see bug #627245) in favor of cairo broke the drawing of any and all themes that use:
* Diagonal lines
* Lines with X2 < X1 or Y2 > Y1
* The <tint> function

The result on this multitude of themes are window border appearances that are not as the theme authors intended or even glitchy.

Attached is a patch which corrects these issues.
Comment 1 Brandon Wright 2010-09-23 15:17:38 UTC
Created attachment 170925 [details] [review]
Patch to resolve theme issues
Comment 2 Brandon Wright 2010-09-24 20:47:27 UTC
Upgrade to BLOCKER. This is a very user-visible problem with an extremely simple fix that needs to get pushed before GNOME 2.32.
Comment 3 Owen Taylor 2010-09-25 14:24:24 UTC
Same as bug 628401

See https://bugzilla.gnome.org/show_bug.cgi?id=626583#c7 for why antialiased rendering is not turned off.

I see no reason why drawing diagonal lines with antialiasing would inherently produce broken rendering; yes diagonal lines will be slightly *different*.

The cairo conversion was come done with specific examples and plentiful use of drawing things out on graph paper, somebody is going to have to identify the specifics and do the same for remaining issues.
Comment 4 Brandon Wright 2010-09-25 15:19:17 UTC
Antialiasing produces broken rendering because existing themes were designed to work around aliased lines and make assumptions based on them. I don't think that replacing an existing function with an expected behavior with something that produces a different result is a good practice. If antialiased lines are needed, they should be added as a separate function.

Individually, the problems here are:
* Antialiasing reduces the contrast of diagonal lines. As such, it isn't desirable under all circumstances and shouldn't completely replace aliased lines.

* A massive theme base exists that assume aliased lines, and also assume pixel-precision regarding how they behave. These themes will therefore have unintended rendering artifacts from changing the behavior.

* The whole "offset" code block makes little sense:
- Lines don't necessarily have X1 < X2 and Y1 < Y2. Adding an +1 offset _reduces_ the width of these lines.
- The offset isn't even applied to diagonal lines, which theoretically also need it.

* The <tint> instruction adds 0.5, 0.5 to the rectangle origin, which puts it in the middle of a pixel. I think we can agree that this is wrong.

I don't know what examples you used to test this code, but I noticed at least half of my installed ~100 themes exhibited at least pixel anomalies with it. After applying the included patch, none of them differed from Metacity 2.30.1. 

I'm not at all against antialiased lines, I just don't believe you should replace functionality in favor of them, and not without a theme format version change and especially not in a point-release--so as to not alienate the users of all the existing themes.
Comment 5 Owen Taylor 2010-09-25 16:00:17 UTC
(In reply to comment #4)
> Antialiasing produces broken rendering because existing themes were designed to
> work around aliased lines and make assumptions based on them. I don't think
> that replacing an existing function with an expected behavior with something
> that produces a different result is a good practice. If antialiased lines are
> needed, they should be added as a separate function.
> 
> Individually, the problems here are:
> * Antialiasing reduces the contrast of diagonal lines. As such, it isn't
> desirable under all circumstances and shouldn't completely replace aliased
> lines.

On the other hand, the contrast of the text in the screen, and the text in the titlebar is always antialiased under all circumstances. And for good appearance what you want is *consistent* contrast.  (Yes, the user can turn font AA off, but we don't have acceptable non-aa font renderers, and nobody is going to publish a "window manager theme for people with non-AA fonts")
 
> * A massive theme base exists that assume aliased lines, and also assume
> pixel-precision regarding how they behave. These themes will therefore have
> unintended rendering artifacts from changing the behavior.

We spent a lot of effort trying to get the best pixel-per-fidelity we could. There are likely bugs in some cases (in fact there are - you've listed some below.) If we fix all bugs, and there are still problems inherent to the transition that make a lot of themes look really bad, then maybe we have to reconsider. If it's just a few pixels wrong ... well I feel bad for the theme authors who have spent all that effort getting every pixel perfect, but we're also giving theme authors a considerable ability to make their themes look *better*.

Non-antialiasing with Cairo will hit some really bad code paths from the fact that modern GPU's don't handle 1-bit-per-pixel masks. (It could be worked around in the X server drivers, but people aren't very interested in spending a lot of effort to make stair-stepped diagonal lines faster.)
 
> * The whole "offset" code block makes little sense:

There's quite a bit of documentation in the code and in the bug I pointed to above about why it is the way it is.

> - Lines don't necessarily have X1 < X2 and Y1 < Y2. Adding an +1 offset
> _reduces_ the width of these lines.

Yep, you are right, the adjustment to include the end point for 0-width horizontal/vertical lines is wrong if these are drawn backwards. A bug.

> - The offset isn't even applied to diagonal lines, which theoretically also
> need it.

You are probably right - I'd have to check on graph paper to be sure. Your approach of using square caps for 0 width lines is likely a good one.

(the "block" isn't mostly about the end cap adjustment - it's about getting horizontal and vertical lines pixel-crisp)

> * The <tint> instruction adds 0.5, 0.5 to the rectangle origin, which puts it
> in the middle of a pixel. I think we can agree that this is wrong.

Yep (stroked rectangles need that adjustment, filled rectangles don't)

> I don't know what examples you used to test this code, but I noticed at least
> half of my installed ~100 themes exhibited at least pixel anomalies with it.
> After applying the included patch, none of them differed from Metacity 2.30.1. 
> 
> I'm not at all against antialiased lines, I just don't believe you should
> replace functionality in favor of them, and not without a theme format version
> change and especially not in a point-release--so as to not alienate the users
> of all the existing themes.

I probably wouldn't have landed this change in the 2.30 branch. But it seems to have had the good effect of getting you to test it and identify problems :-)
Comment 6 Brandon Wright 2010-09-25 17:22:02 UTC
Created attachment 171098 [details]
An image for comparison purposes.

(In reply to comment #5)
> On the other hand, the contrast of the text in the screen, and the text in the
> titlebar is always antialiased under all circumstances. And for good appearance
> what you want is *consistent* contrast.  (Yes, the user can turn font AA off,
> but we don't have acceptable non-aa font renderers, and nobody is going to
> publish a "window manager theme for people with non-AA fonts")
This is actually what I was referring to, except I was comparing the close glyphs (that typically use diagonal lines) to the other glyphs. In that case, the minimize and maximize glyphs will be pixel-aligned and the close glyph will not.

> We spent a lot of effort trying to get the best pixel-per-fidelity we could.
> There are likely bugs in some cases (in fact there are - you've listed some
> below.) If we fix all bugs, and there are still problems inherent to the
> transition that make a lot of themes look really bad, then maybe we have to
> reconsider. If it's just a few pixels wrong ... well I feel bad for the theme
> authors who have spent all that effort getting every pixel perfect, but we're
> also giving theme authors a considerable ability to make their themes look
> *better*.
If this were a major version change (2.30->2.32) and if even just the GNOME base themes were updated for it, I wouldn't be as concerned. But it seems the default themes are going to have some errors, and it doesn't really speak well for quality.

> You are probably right - I'd have to check on graph paper to be sure. Your
> approach of using square caps for 0 width lines is likely a good one.
> 
> (the "block" isn't mostly about the end cap adjustment - it's about getting
> horizontal and vertical lines pixel-crisp)
Except the patch I submitted actually does this fine. If you take a look at the attached image and look at the close glyphs in the master and patch+antialiasing pictures. Even if antialiasing is turned back on, you can see that axial lines are drawn just as sharp. Also look at rows 2 and 4 in the image: with antialiasing, it appears that my square-endcap solution comes closer to matching the original aliased glyph and achieves a higher contrast, particularly on the black glyph border. So even if you decide to leave antialiasing on, at least use this method instead.

You can also see an assumption that this theme (ClearlooksClassic) makes regarding aliased lines. It draws a couple of width-2 lines, and strokes through those using a couple of width-1 lines to fill the corners of the X and make it "pointy." This obviously doesn't work with antialiased lines. :-(
Comment 7 Brandon Wright 2010-09-25 18:04:40 UTC
Created attachment 171100 [details] [review]
Patch to fix bugs, but not diagonal antialiasing.

Ok, I found a case that the offset block was designed to fix, so I understand it why it's there. 
I've attached a patch that implements the fixes mentioned and leaves the anti-aliased diagonals intact. This should at least be good for mutter/metacity 3.0 if metacity has the GDK->cairo change reverted for GNOME 2.32.
Comment 8 Brandon Wright 2010-09-30 10:25:42 UTC
Thomas, I noticed that you committed that last patch of mine. That certainly fixes the bugs involved, but not the intent. Antialiased lines simply draw differently, and the themes need to be adjusted for this. Even with that last patch, none of the current default themes draw correctly. The X button glyph lines are actually hyperextended on all of them, and they need fixes to look correctly. What the Clearlooks themes do just plain won't work with antialiased lines. 

This late in the game, I think the safest thing to do is to revert the GDK->Cairo change for GNOME 2.32, but to also get the themes updated or maybe add a separate antialiased primitive for GNOME 3.0.
Comment 9 Brandon Wright 2010-09-30 11:02:25 UTC
Created attachment 171419 [details] [review]
Additional: turn off antialiasing for diagonals

Oops. I guess it's too late for that now :-). As a note, in case you want to do a bug-fix for it, just turning off the antialiasing for the diagonals now (attached patch against master/2.30.3) gets behavior like previous versions. 

As someone who's made metacity themes, I'm still pretty sure that changing the behavior of the existing primitive was a bad idea. I personally would have added an optional attribute "antialias" to the line tag and included it in metacity theme format 3 in case someone wanted to use them for some reason. As it is, no theme I've seen intentionally uses diagonals that aren't at 45 degree angles, so antialiasing existing lines really has no significant benefit.
Comment 10 Owen Taylor 2010-10-06 17:09:35 UTC
Created attachment 171832 [details] [review]
Clean up antialiasing fixes

Sorry for not getting to a review of your patch earlier; it looks
correct to me, I'd mainly comment on some stylistic issues:

 * xor-swapping is a neat historical technique, but is neither
   readable nor efficient, so should be avoided. (Some discussion
   in Wikipedia)

 * The swapping, and so forth can actually be avoided entirely if
   you note that the use-square-line-caps-and-offset-by-0.5 approach
   gives the right pixel-aligned rectangle for horizontal and
   vertical lines. A lot of code falls out and things get simpler.

 * No braces on single line statements for the Metacity style.

Here's a cleanup patch that implements that. I'm pretty sure it's
pixel-per-pixel the same, but haven't actually tested with themes
and xmag'ed the result.
Comment 11 Owen Taylor 2010-10-06 19:13:07 UTC
I filed bug 631554 with a patch to ClearLooks that fixes the one-pixel-too-extended rendering of the close button. (The approach in the patch
works with both AA and non-AA/Xlib rendering.)
Comment 12 Brandon Wright 2010-10-08 01:35:48 UTC
I can confirm that the cleanup patch retains the effect of the original. (despite still using the butt-style line caps for the horizontal and vertical lines?) 

The Clearlooks patch does as you suggest. But there are also four more default themes (Glider, Glossy, Inverted, and Mist). Three have the same problem as Clearlooks, but the Glider theme is extended and also a bit different. The close button looks blurry compared to the other button glyphs on that one, too. 

What do you think of my suggestion to use a theme line tag attribute (antialias="bool") to opt-in to antialiasing here? I understand what you're saying about the 1-bit alpha speed problem, but this is being drawn at a low size and resolution, not in significant frequency like text glyphs, and at this time the window border pixmap might not even be migrated yet. If it was transferring the entire border back and forth from the server to draw a line each time I might be reluctant, but--to my knowledge--Cairo isn't that inefficient.

Looking through a bunch of themes for compatibility, it seems like almost half of them have either copy-paste jobs of Clearlooks's close button or are fuzzy-looking, and most of the rest just use premade images (which don't scale and are therefore inferior :-) ).
Comment 13 Owen Taylor 2010-10-11 21:21:19 UTC
(In reply to comment #12)
> I can confirm that the cleanup patch retains the effect of the original.
> (despite still using the butt-style line caps for the horizontal and vertical
> lines?) 

For wide horizontal and vertical lines - thin/0-width lines go through the other code path. Thanks for the testing.
 
> The Clearlooks patch does as you suggest. But there are also four more default
> themes (Glider, Glossy, Inverted, and Mist). Three have the same problem as
> Clearlooks, but the Glider theme is extended and also a bit different. The
> close button looks blurry compared to the other button glyphs on that one, too. 

Yeah, I just fixed ClearLooks because it was the default and then copied the fix over to ClearlooksClassic since it obviously had the same code.

> What do you think of my suggestion to use a theme line tag attribute
> (antialias="bool") to opt-in to antialiasing here?

Couple of problems here:

 * The metacity theme format is inherently not extensible. If an attribute isn't supported in metacity-theme-1.xml or metacity-theme-2.xml, then it can't be added there. metacity-theme-3.xml adds a more flexible mechanism, but I don't think everybody wants to create an entire new copy of their theme just to add a few antialiased lines, and also, it's not currently supported in Metacity, just in Mutter. (see bug 592503)

 * Presumably, in the future, we want all lines/arcs/etc. to be antialiased. I don't think metacity themes need more unreadability in the form of antialias="true" all over the place.

In general, my expectation was that antialising was just going to cause some fuzziness,. I also wasn't expecting it to land in Metacity without more testing. In consideration of obvious artifacts like the close button ends, turning on antialias all over the place probably wasn't a good idea for us to do. However, now it's on, I'd rather not waste a lot of energy trying to restore compatibility - the horse is already out out of the barn. It's better, I think to fix up themes (my ClearLooks patch demonstrates you can do that without breaking compatibility with older versions of Metacity) and move forward to what we can do in the future to make GNOME 3 themes look better - real rounded corners, etc.

> I understand what you're
> saying about the 1-bit alpha speed problem, but this is being drawn at a low
> size and resolution, not in significant frequency like text glyphs, and at this
> time the window border pixmap might not even be migrated yet. 

Unlikely - pixmaps are going start off in video ran.

> If it was  transferring the entire border back and forth from the server 
> to draw a line each time I might be reluctant, but--to my knowledge--Cairo 
> isn't that inefficient.

The transfer here is inside the X server between video ram and system ram. (Copying bits out of video ram is actually a lot slower than moving them between processes, as it turns out.) What gets moved is going to depend on complicated heuristics inside EXA and UXA; it's a little hard for me to predict the details.
Comment 14 Brandon Wright 2010-10-11 22:01:53 UTC
(In reply to comment #13)
 
> The transfer here is inside the X server between video ram and system ram.
> (Copying bits out of video ram is actually a lot slower than moving them
> between processes, as it turns out.) What gets moved is going to depend on
> complicated heuristics inside EXA and UXA; it's a little hard for me to predict
> the details.

I was making the (false) association server:video ram::client:system ram, but suggesting cairo ideally would only need to read back from video ram once instead of for every line. In the metacity-theme-viewer benchmark, I'm personally seeing no performance difference.

> In general, my expectation was that antialising was just going to cause some
> fuzziness,. I also wasn't expecting it to land in Metacity without more
> testing. In consideration of obvious artifacts like the close button ends,
> turning on antialias all over the place probably wasn't a good idea for us to
> do. However, now it's on, I'd rather not waste a lot of energy trying to
> restore compatibility - the horse is already out out of the barn. It's better,
> I think to fix up themes (my ClearLooks patch demonstrates you can do that
> without breaking compatibility with older versions of Metacity) and move
> forward to what we can do in the future to make GNOME 3 themes look better -
> real rounded corners, etc.

Ok, this is what I wanted to hear. If you're intentionally going with to stick with this approach, then everything's fine. Once the cleanup patch goes into Metacity, too, then this bug is resolved. Bug 631554 can handle the default theme errors.
Comment 15 André Klapper 2010-12-04 10:14:00 UTC
(Not a blocker.)
Comment 16 Alberts Muktupāvels 2015-04-15 14:13:04 UTC
Comment on attachment 171832 [details] [review]
Clean up antialiasing fixes

https://git.gnome.org/browse/metacity/commit/?id=ebbea1b46d74d4feb9e9262c270e333a6598d041