363861 - GDI ClearType rendered to RGBA surfaces sometimes looks bad

Status: RESOLVED FIXED

(Core :: Graphics, defect, P4)

Description

[Ryan Rubley]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061204 GranParadiso/3.0a1

The dropdown menus at the top of each page have the text sorta fuzzy looking with red/blue tints around the edges of some of the letters when using Gran Paradiso Alpha 1.

Reproducible: Always

Comment 1

[Ryan Rubley]

Attachment: What a menu looks like under 3.0a1

If you compare this to 2.0, you can tell that the right pixel is cut off of each line of text. It is most obvious on the lowercase 's' on the bottom two options shown in that picture. The left pixel is also cut off of each line, but it is harder to tell. All of the text is clearly "dimmer" looking and not properly aliased for cleartype display, causing it to not look as sharp as it should and does under 2.0 and other browsers. Another small change noticeable is the space between the T and F on the second and third lines is 1 pixel less in 3.0a1 for some reason.

Comment 2

[Ryan Rubley]

Attachment: What a menu looks like under 2.0

Comment 3

[Gábor Stefanik]

It seems that Cairo is unable to correctly detect the subpixel ordering of the screen, and does a BGR-ordered ClearType on the text. Or maybe a result of both Cairo and Windows attempting to antialias the text.

Comment 4

[Gábor Stefanik]

Confirming bug in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061208 Minefield/3.0a1.

Further investigation shown that Cairo selects the wrong contrast level when rendering the text. Setting contrast to 1.0 using ClearType fixes the issue (although text will likely to look weird in other places if that is set). Probably Cairo incorrectly reads the contrast values and takes them as if they were squared (e.g. a level of 1.4 is treated as 1.4^2, that's around 2.0, while 1.0 is processed as 1,0^2=1.0). Another thing I noticed is that if you hover over the menu items, the bug disappears, but opening a submenu causes it to re-appear. Testcase coming soon.

Comment 5

[Gábor Stefanik]

Attachment: Testcase

Testcase. Hover over the menu in it to trigger the bug.

Comment 6

[Gábor Stefanik]

It looks like the culprit is in cairo-win32-font.c, judging from these comments (line 1186):

"* Otherwise, we need to draw using software fallbacks. We create a mask
 * surface by drawing the the glyphs onto a DIB, black-on-white then
 * inverting. GDI outputs gamma-corrected images so inverted black-on-white
 * is very different from white-on-black. We favor the more common
 * case where the final output is dark-on-light."

After that (at line 1212):

"* For ClearType, we need a 4-channel mask. If we are compositing on
 * a surface with alpha, we need to compute the alpha channel of
 * the mask (we just copy the green channel). But for a destination
 * surface without alpha the alpha channel of the mask is ignored"
(Let's not forget that our surface is semi-transparent, so it does have an alpha!)

And a few lines later (line 1225):

"* XXX: Hacky, should expose this in cairo_image_surface"

Probably this is a bug in Cairo itself, but it seems to need quite a major rewrite.

Comment 9

[Emil Ivanov]

Attachment: screenshot from lineage2.com

On hover the bug is still visible. See the attachment. Why this bug is not blocker, when clear type technology is used mostly in LCD monitors, and its full of them now.

Comment 10

[Thom Stricklin]

Attachment: ClearType/Opacity bug in 3 Beta 5 - Inline HTML (No layers/DHTML)

Seems like this bug has gotten worse in 3 Beta 5...  I'm using opacity to fade out adjacent month dates in our in-house web calendars, and in Beta 5 the '1' digits are suffering badly.  This is all relatively positioned CSS, no floats, no z-indexing or layers.  Problem goes away with ClearType turned off.

Comment 11

[Matthew Holder]

This bug also manifests itself in the chrome as well if the element or it's parent uses opacity or has rounded borders (-moz-border-radius)  The Glasser extension screenshots show this problem alot:

http://www.neowin.net/forum/index.php?showtopic=632112

Comment 12

[Istari]

I absolutely consider the Glasser extension an essential now, but this bug is really bugging me quite a lot. I tried turning off ClearType, but the font looks absolutely hideous on a LCD monitor.

Comment 14

[aurum]

The Glasser Extension does not work properly because of this ...

Comment 15

[redsign]

That's nothing new but there also many websites that just look **** because of this. It's really sad that this doesn't get fixed for 3.0.  

Comment 17

[Jim]

Attachment: A sample HTML file showing css opacity usage with various color text and background

Comment 18

[Jim]

Attachment: opacity_test_p.html with cleartype enabled

Rendered in Firefox 3 RC1 on Windows Vista.

Comment 19

[Jim]

Attachment: opacity_test_p.html with cleartype disabled

Rendered in Firefox 3 RC1 on Windows Vista.

Comment 25

[Damon Sicore (:damons)]

Don't think this would block the 1.9.1 release.  Minusing.

Comment 27

[Kai Liu]

What I'm curious about is why painting ClearType text onto a surface with alpha forces cairo to take the software fallback branch in the first place.  Gecko 1.8 never had problems with drawing ClearType onto something with opacity, so what is it about cairo surfaces that prevents direct drawing?

Comment 28

[Thomas K. (:tom)]

Does this bug depend on bug 407531, or vice versa?

Comment 29

[Kai Liu]

(In reply to comment #28)
> Does this bug depend on bug 407531, or vice versa?
> 

Neither.

Comment 30

[Teal Bauer [:teal]]

The problem seems to be in ClearType since 1.0.4, see http://lists.cairographics.org/archives/cairo/2006-October/008081.html

There is also a 'very large hammer' fix in that thread (force surface format to CAIRO_FORMAT_RGB24).

Comment 31

[Mark Smith [:mcs]]

Attachment: XUL testcase using <menupopup>

Someone else I am working with encountered this bug when creating a theme that uses rounded corners and transparency with menus.  In case it helps someone debug the problem, I am attaching a XUL-based testcase.

Comment 32

[Mark Smith [:mcs]]

Attachment: screenshot of XUL testcase result (ClearType on, scaled to 200%)

Comment 33

[Mark Smith [:mcs]]

Attachment: screenshot of XUL testcase result (ClearType off, scaled to 200%)

Comment 34

[Mark Perris]

Attachment: BBC Homepage Snippet

This is now visible on the BBC Homepage. The link on the right of the attached image has been hovered once, and the color has changed. Once the cursor is moved out of the box, the color reverts to it's original incorrectly cleartyped style.

Comment 35

[Gábor Stefanik]

It appears that Microsoft is at least partially responsible for this bug - Firefox 3.0b1 renders the testcases significantly better on Windows 7 beta 1 than on Windows Vista SP1. Attachment 248910 [details] does not show any bug now, while attachment 321706 [details] only gets the white-on-white one wrong (and even that one is much better than on Vista). Attachment 339121 [details] now only shows the rightmost-pixels-cut-off bug, no contrast problem is visible.

Comment 36

[Gábor Stefanik]

Seems that the problem is the fact that we "just copy the green channel" to get the alpha channel. The following testcases clearly point to this:
data:text/html,<div style="background: white;"><div style="color: white; opacity: 0.999; font-size: 36px;">FAIL</div></div>
This one faintly displays the word "FAIL", even though it is white-on-white, and as such, should be invisible. What is visible appears to be a cyan line on the left (cyan=blue+green), and a yellow one on the right of the text (yellow=red+green); both colors contain the color green as a component.
These also show incorrect rendering:
data:text/html,<div style="background: red;"><div style="color: red; opacity: 0.999; font-size: 36px;">FAIL</div></div> (part of the red color is missing on the left side of the letters, black is visible)
data:text/html,<div style="background: blue;"><div style="color: blue; opacity: 0.999; font-size: 36px;">FAIL</div></div> (part of the blue is missing on the right of the letters)
data:text/html,<div style="background: cyan;"><div style="color: cyan; opacity: 0.999; font-size: 36px;">FAIL</div></div> (part of the blue color is missing, green shows through)
data:text/html,<div style="background: magenta;"><div style="color: magenta; opacity: 0.999; font-size: 36px;">FAIL</div></div> (red is missing on the left, while the right lacks blue)
data:text/html,<div style="background: yellow;"><div style="color: yellow; opacity: 0.999; font-size: 36px;">FAIL</div></div> (red is mising, green shows through)

These pass however:
data:text/html,<div style="background: black;"><div style="color: black; opacity: 0.999; font-size: 36px;">FAIL</div></div>
data:text/html,<div style="background: green;"><div style="color: green; opacity: 0.999; font-size: 36px;">FAIL</div></div>
data:text/html,<div style="background: lime;"><div style="color: lime; opacity: 0.999; font-size: 36px;">FAIL</div></div>

What I have noticed is that we only pass if R=0 and B=0, i.e. color=#00xx00, where xx is any hex number. This is because the only channel for which the alpha mask is correct is green, as we use the green channel for our alpha mask.
However, if any other channel is present, the alpha mask will show an off-by-one-subpixel problem, and the rendering of the non-green channels will be corrupted.

So, basically copying the green channel to the alpha channel is wrong, the actual alpha channel should take all color channels into account. This bug, probably coupled with a bug in ClearType in pre-7 Windowses, is the most likely cause of the misrendering.

I'll probably back with a proof-of-concept patch soon.

Comment 37

[Gábor Stefanik]

Attachment: WIP 0.000001: Move away from using the green channel as the alpha

Extremely-early preview of what I am working on right now.
With this patch, testcases involving extreme colors (white-on-white, black-on-black, #ff0000 on #ff0000, #00ffff on #00ffff) pass, but other tests still show errors, and real-life rendering is also still wrong. This is because the dummy maximum algorithm used in this patch is just a placeholder, and is definitely not correct. However, the basic structure of the _compute_argb32_mask_alpha function is in place, only an alpha-channel generation algorithm is needed. I think I need to convert the RGB colors to HSL and use the lightness value as the alpha, although I am not sure.

If you have an idea what the correct algorithm would be, please post it here!

Comment 39

[Phil Hartmann]

I just ran into this as well, any progress being made on this?

Comment 40

[Ilya]

I found another opacity and anti-aliasing related artifact. When I use 'Arial Black' font (about 18px size) together with opacity less than 1.0, the text appears a bit thinner (as if it had smaller font-weight). With larger font size the visual difference becomes less. Without anti-aliasing the text rendering doesn't change because of the opacity, but with 'standard' anti-aliasing the similar artifact still appears (so it may be not only ClearType-related problem). I didn't investigate if this happens to other fonts yet.

Comment 41

[Ilya]

Attachment: Demo of anti-aliased 'Arial Black' font with opacity issue

Comment 42

[Gingerbread Man]

Attachment: Buried story on Digg showing text with green halo

Burying stories on Digg.com displays the text with a sort of green halo around it.

Someone else has reported that this problem also occurs when deleting images on Flickr, and that it does not occur on Vista 32-bit (I am using Vista x64). The Mozillazine thread in question is http://forums.mozillazine.org/viewtopic.php?f=38&t=1414815

Comment 44

[Goran Baotic]

I'm not active in bugzilla community, so pardon me for dropping in.
I'd just like to point out the circumstances under which the bug occurs:

- *** 64-bit *** Windows Vista / 7 
- ClearType set to ON

I didn't succeed in replicating the bug on 32-bit versions of Vista/7, even
with ClearType set to on, so I think it's a 64-bit problem. I haven't tested teh bug on XP/64-bit.

Green artifacts appear on text elements when opacity is set to less than 1
(.999 or less). Clearly visible on jQuery or similar JS fade in/out effects
that utilize fast changes to element opacity.

I'm also having issues with PNG rendering in Firefox 3.5.* when color
management is turned on. Setting "gfx.color_management.mode" to 0 fixes the
issue. This is also a bug that only occurs on 64-bit versions of Windows OS.

From my layman point of view, I suggest that something is generally wrong with text and image rendering on Firefox 3.* when using a 64-bit Windows Vista/7.

Comment 45

[grant.abraham]

I can replicate this bug with the cut-off text (as per comment #1 from Ryan Rubley).  I am running Windows 7 RC 32-bit (build 7100).  I also was able to replicate it on Windows Vista 32-bit on the same machine.  Cleartype is on in both cases.

Comment 46

[Gábor Stefanik]

We already know the cause of this bug (the alpha-mask is incorrect for sub-pixel antialiased text), just don't know how to fix it (i.e., what the correct alpha mask would be).

Comment 48

[Wesley Johnston (:wesj)]

(In reply to comment #37)
Have you tried doing an rgb->hsl conversion and using one of those numbers? I don't really understand what's going on there (pardon the bug spam if I'm way off track), but just glancing at the patch, it seems like lightness might be a better match.

Comment 50

[Ken]

the new DirectWrite backend in Cairo, and demonstrated in this build (http://www.basschouten.com/media/blogs/blog/warning.html) fixes the issue for me (on Vista x64).

Comment 51

[Dão Gottwald [:dao]]

This bug is about Windows only... there's no ClearType on Linux. Whoever's adding the links to launchpad, please stop it.

Comment 52

[Micah Gersten]

(In reply to comment #51)
> This bug is about Windows only... there's no ClearType on Linux. Whoever's
> adding the links to launchpad, please stop it.

Apologies from Launchpad.  There's an open bug for this and I just asked for the priority to be increased:
https://bugs.launchpad.net/malone/+bug/499113

Comment 53

[Emil Ivanov]

I think this is a bot, adding those urls in the WRONG place. Something have to be done about that in bugzilla.

Comment 54

[Micah Gersten]

(In reply to comment #53)
> I think this is a bot, adding those urls in the WRONG place. Something have to
> be done about that in bugzilla.

As I said in comment #52, this is a Launchpad bug that's already open.  It's adding matches anytime a bug # is mentioned in text.

Comment 55

[Micah Gersten]

reed has temporarily disabled launchpad's account, so I'm removing the launchpad links, when syncing resumes, you shouldn't see any more links on this bug
Again, I'm sorry you had to be bothered with this.

Comment 56

[Dão Gottwald [:dao]]

This is going to affect another ~50% of our users, since bug 504698 forces cleartype on XP users. Requesting blocking.

Comment 57

[Joe Drew (not getting mail)]

We need to disable cleartype on non-opaque surfaces.

Comment 58

[Gábor Stefanik]

At least for XP and Vista. However, on Windows 7, font display is correct - minus the white-on-white issue.

Comment 59

[Glen A.]

(In reply to comment #57)
> We need to disable cleartype on non-opaque surfaces.

What will that do to a fade transition? If it means it will jump to and from anti-aliased text depending on the opacity (as it does in IE), then PLEASE don't do this. It ruins the effect.

Comment 60

[Gábor Stefanik]

(In reply to comment #59)
> (In reply to comment #57)
> > We need to disable cleartype on non-opaque surfaces.
> 
> What will that do to a fade transition? If it means it will jump to and from
> anti-aliased text depending on the opacity (as it does in IE), then PLEASE
> don't do this. It ruins the effect.

Well, we already jump between correctly antialiased and misrendered text depending on opacity, an equally bad (if not worse) effect.

Comment 61

[Glen A.]

(In reply to comment #60)
> Well, we already jump between correctly antialiased and misrendered text
> depending on opacity, an equally bad (if not worse) effect.
I'm using Windows 7 now, but I don't remember it being too noticeable on XP – at least not as noticeable as the issue in Internet Explorer.

Comment 62

[Ken]

Any chance of this getting fixed for FF 4.0? It makes quite a number of sites look like **** that look fine on other browsers, and the number is growing as more sites use jQuery alpha fades or similar, or simply use CSS opacity.

Comment 63

[Dan]

Attachment: Screen capture of the bug in the FF4.0b1 addons manager

This bug shows in the new addons manager for disabled addons which are alpha faded.  The blue developer link shows the mixed greenish coloring.

Comment 64

[John Daggett (:jtd)]

Opacity pinwheel test
http://people.mozilla.org/~jdaggett/tests/opacitytest.html

(Click to vary the background)

Comment 65

[John Daggett (:jtd)]

Attachment: screenshot, opacity pinwheel on XP with Cleartype enabled

Mozilla/5.0 (Windows; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100712 Minefield/4.0b2pre

Comment 66

[John Daggett (:jtd)]

Attachment: screenshot, opacity pinwheel on OSX 10.5

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1

Comment 67

[John Daggett (:jtd)]

(In reply to comment #58)
> At least for XP and Vista. However, on Windows 7, font display is correct -
> minus the white-on-white issue.

Gábor, could you attach a screenshot of the testpage in comment 64 with a recent nightly?  I want to be sure I'm seeing the same thing you are on your machine and if not, figure out what the reason is for the difference.

Comment 68

[Gábor Stefanik]

Attachment: The opacity wheel on Windows 7

It looks mostly the same, though perhaps font rendering is a bit smoother on 7. However, the green halos are still present.

I believe that th differences seen in Windows 7 are due to it correctly reporting the size of bounding box needed to contain the rendered text, including the extra pixels due to antialiasing, whereas Vista and below only took into account the "main" text (that is, even with Cleartype enabled, they would return the width of the un-antialiased text).

Comment 69

[John Daggett (:jtd)]

Yet another test, this time with text samples that clearly show divergent rendering when opacity is applied.

http://people.mozilla.org/~jdaggett/tests/problemopacities.html

Each line is repeated twice, the first opaque, the second with opacity set to 0.999.  Fonts used are Calibri, Arial, Constantia.

Comment 70

[John Daggett (:jtd)]

Attachment: screenshot, problem colors with and without opacity

Closeup of the testpage in the previous comment for just the Arial portion.  WinXP w/ SP3, latest trunk.  Note how the fringe colors almost completely flip between the opaque and opacity = 0.999 cases.

Comment 71

[Gábor Stefanik]

http://forums.macnn.com/89/macnn-lounge/406708/creating-subpixel-rendered-text-onto-transparent/ might hold the answer we are looking for:
"Anyhow, I figured out a way to do it from a screenshot, and the result is identical to what the Windows ClearType rasterizer produces on the same background:

   1. Take screenshot of ClearType (or Mac OS X's LCD-optimized) text on white background.
   2. In Photoshop, create RGB file.
   3. Paste in screenshot as a layer.
   4. Create 4 fill layers: white as the background, and one each with #00FFFF (cyan), #FF00FF (magenta), and #FFFF00 (yellow). Don't use Photoshop's CMYK fields, because it won't look right.
   5. Set the CMY layers' blend mode to Multiply.
   6. Open Channels view for the screenshot layer.
   7. Take the red channel of the screenshot and paste it as the layer mask for the cyan fill layer, then invert the layer mask. Repeat, taking the green channel for the magenta layer, and the blue channel for the yellow layer.
   8. Compare. The screenshot and the 3 multiplied channels should produce the identical result, except that now, you can change the background image to whatever you want."

I tried reproducing this in Paint Shop Pro, though, and didn't get good results; this may be due to the algorithm being incorrect or Paint Shop Pro using inaccurate math.

Comment 72

[John Daggett (:jtd)]

(In reply to comment #71)

Wow, that is some serious ki-ray-zi-ness!

The reason subpixel rendering to transparent layers doesn't "work" is
that there's basically a three-channel alpha generated by the font
rasterizer which gets thrown away once the text is blended into the
background.

Another thing to point out is that there's a subtle difference between
using opacity and using rgba colors.

div.blend {
  opacity: 0.5;
  color: red;
}

One would think that this is the same as:

div.blendrgba {
  color: rgba(255, 0, 0, 0.5);
}

But to the browser this code is *very* different, assigning opacity to
an element says "blend this element with all its children after it's
been rendered" while using just an rgba color simply implies "blend
rendered text with the background".  The latter will be more efficient
(no need to create an offscreen buffer in which to render the element
contents) *and* you'll get the correct subpixel antialiasing you're
looking for.  One could also use an rgba background color also and
still get subpixel antialiasing.

Why isn't an rgba color + rgba background-color the same as color +
background-color + opacity?  The blending semantics are such that it
will give slightly different results (i.e. blend - blend vs. opaque
render - blend, the pixels affected by the second blend will be
slightly different).  But my guess is authors can tweak colors to get
the effect they want in the rgba color case.

So the color + opacity case is not going to give ideal subpixel
antialiasing.  However the results here are much *worse* than that,
there are distinct color shifts that are hard to explain.  The
attachment in comment 70 shows that clearly, the edge colors appear to
flip between the opaque and opacity = 0.999 cases.  I suspect there's
some funky rendering code in cairo that's at the root of this.  The
edge should look darker but the hue should not change so radically
(compare with rendering behavior on OSX).  The hue change is what
makes the results appear so wrong/odd/jarring.

Comment 73

[Gábor Stefanik]

Note: Chrome has recently fixed the same bug, it may be worth looking into how the current Chrome handles this.

Comment 74

[Alex]

Attachment: Similar problem on OS X

This actually happens to a pretty slight extent on OS X as well, nowhere near as bad as on Windows though.

The v in over on the 6th line is a pretty good example, it pretty much destroys the sub-pixel components.

Comment 75

[John Daggett (:jtd)]

Related Chrome bug:
Issue 559: Cleartype text not rendered correctly with opacity applied
http://code.google.com/p/chromium/issues/detail?id=559

Fix in Chrome/Webkit:

http://src.chromium.org/viewvc/chrome?view=rev&revision=10637

Interesting code:

http://codereview.chromium.org/21201

Comment 76

[John Daggett (:jtd)]

I put together a quick hack that simply stubs out the ClearType
rendering paths for fallback rendering here:

  http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-win32-font.c#1440

Stubbing out the Cleartype case forces the code to the use an 8-bit
grayscale anti-aliasing; for opaque text, Cleartype is used on
semi-transparent layers text is rendered with grayscale anti-aliasing. 
This codepath seems *really* inefficient, I'm sure we could come up with
something more efficient but it at least demonstrates one possible
solution.

As roc notes in bug 582223, comment 13, the "home run" solution is to be
able to use Cleartype in as many situations as possible.  Chrome builds
show that this is possible.

Try server build:

  http://bit.ly/9IMySB

Be sure to run with DirectWrite disabled.  The testcases in comment 64
and comment 69 look fine but non-opaque opacities draw without Cleartype
subpixel anti-aliasing.

Comment 77

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The Chrome approach, or one of them, seems to be to copy the "current contents" of the lower buffers into the temporary buffer for transparent content before drawing into it, so most of the time the pixels in your buffers are always opaque.

That's a good approach in some ways, although it's no good if you're trying to cache the transparent content for faster rendering.

Comment 78

[John Daggett (:jtd)]

Attachment: screenshot, opacity pinwheel win7, ff trunk vs. ff w/ grayscale antialiasing on transparent layers

Comment 79

[John Daggett (:jtd)]

Attachment: ascii art version of rendering differences

Dump of the resulting renderings of the word 'quick' when displayed using two different style settings:

.colorA p {
  color: #9ac5cb;
}

.colorA p.opacity {
  color: #9ac5cb;
  opacity: 0.999;
}

The third result is the result of forcing grayscale AA on (this looks much closer to the opaque result).

Comment 80

[John Daggett (:jtd)]

Attachment: ascii art version of complete set of composite operations

Comment 81

[John Daggett (:jtd)]

Example of pixel calculations taken from the right edge of the 'q', using the data in the second ascii art dump.

Underlying text color (via CSS):     9ac5cb

text rendered (black on white): 00002b80 (argb value)

mask: d4ffd47f x color (9ac5cb) :
  a = d4
  r = ff * 9a / ff = 9a
  g = d4 * c5 / ff = a4
  b = 7f * cb / ff = 65

==> comp: d49aa465

This is effectively subpixel-antialiasing against a *black* background.

comp over white:
  r = 9a + (ff * ff) (ff - d4) / (ff * ff) = c5
  g = a4 + (ff * ff) (ff - d4) / (ff * ff) = cf
  b = 65 + (ff * ff) (ff - d4) / (ff * ff) = 90
  
==> comp over white: c5cf90

Compare this to the ideal case of using the text rendering as a three-channel
alpha mask to composite the color over a white background.

ideal comp, use mask channels as alpha:
  r = (9a * ff) / ff + (ff * ff) (ff - ff) / (ff * ff) = 9a
  g = (c5 * d4) / ff + (ff * ff) (ff - d4) / (ff * ff) = cf
  b = (cb * 7f) / ff + (ff * ff) (ff - 7f) / (ff * ff) = e5
  
==> ideal comp: 9acfe5 

The ideal calculation here matches precisely what is drawn when the layer is
opaque and ExtText is drawing directly into the white background:

direct rendering: 9acfe5

For any case where a given red or blue channel has an alpha value
significantly different from the green channel alpha and the given color
has a large red or blue component, a large error will result.

The two solutions would be to potentially figure out a way of passing
around layers represented by a color / three-channel mask combination
-or- to simply use grayscale antialiasing instead.  The first is
interesting but I propose we do the latter.

Comment 82

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I want D3D and GL to support component alpha in transparent layers with text. See bug 593604 and bug 593733. That would fix this problem for retained layers generated by CSS 'opacity'.

For BasicLayers (no hw accel), on trunk we are already falling back to essentially not retaining transparent layers that have text over transparent pixels, if the layer has 'opacity:1'. We can easily go further to fix this bug, doing what Chrome does for layers with fractional 'opacity': don't retain the layer, and copy the backbuffer contents into the temporary buffer created by PushGroup in BasicThebesLayer::Paint and BasicLayerManager::PaintLayer.

We do not want to just switch to grayscale antialiasing wholesale when drawing to an RGBA buffer. That would combine poorly with drawing text directly to the backbuffer to try to get subpixel AA, since the backbuffer is RGBA on Windows Vista/7.

We'll still have a problem drawing text onto the transparent parts of the Firefox chrome window, though. So I hope you can tweak cairo-win32-font.c to get good results for that. Will averaging the color channel alpha values to compute the alpha value for the destination pixel in an RGBA destination work well?

Comment 83

[John Daggett (:jtd)]

(In reply to comment #82)
> I want D3D and GL to support component alpha in transparent layers with text.

Sure, but it doesn't sound like the HW support is there, especially for D3D.

> We do not want to just switch to grayscale antialiasing wholesale when drawing
> to an RGBA buffer. That would combine poorly with drawing text directly to the
> backbuffer to try to get subpixel AA, since the backbuffer is RGBA on Windows
> Vista/7.

We sorta-kinda already have grayscale AA with the DWrite path, for transparent surfaces it renders text into a Cleartype alpha mask, then smears the green component to all channels and composites using that as a mask:

http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-dwrite-font.cpp#611

> We'll still have a problem drawing text onto the transparent parts of the
> Firefox chrome window, though. So I hope you can tweak cairo-win32-font.c to
> get good results for that.

I think the solution is higher up, if the result ends up in a
transparent ARGB surface you lose the channel separation.

> Will averaging the color channel alpha values to compute the alpha
> value for the destination pixel in an RGBA destination work well?

My guess is that it will be very close to the result of grayscale
antialiasing.

Comment 84

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #83)
> (In reply to comment #82)
> > I want D3D and GL to support component alpha in transparent layers with
> > text.
> 
> Sure, but it doesn't sound like the HW support is there, especially for D3D.

Bas says it is doable, with two-pass rendering.

> We sorta-kinda already have grayscale AA with the DWrite path, for transparent
> surfaces it renders text into a Cleartype alpha mask, then smears the green
> component to all channels and composites using that as a mask:
> 
> http://mxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-dwrite-font.cpp#611

Yes, and that's not good. Fortunately with D2D we should always have D3D9 layers on for almost all users.

> > We'll still have a problem drawing text onto the transparent parts of the
> > Firefox chrome window, though. So I hope you can tweak cairo-win32-font.c to
> > get good results for that.
> 
> I think the solution is higher up, if the result ends up in a
> transparent ARGB surface you lose the channel separation.

We do, but we have no choice when drawing into the transparent part of a toplevel window.

> > Will averaging the color channel alpha values to compute the alpha
> > value for the destination pixel in an RGBA destination work well?
> 
> My guess is that it will be very close to the result of grayscale
> antialiasing.

Great, let's do that!

Comment 85

[John Daggett (:jtd)]

Attachment: patch, disable cleartype quality when rendering to non-opaque surfaces

Turn off CLEARTYPE_QUALITY when rendering to non-opaque surfaces.  Within this cairo routine, this is the best way to fix the cases of poor rendering.  Where subpixel rendering is desired, it would be better to either assure that an opaque surface is used or to create a new type of surface that preserves multi-channel alpha masks (similar to the three-channel Cleartype mask that DirectWrite can generate).

For now though, this is a simple/safe fix.

Comment 86

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If we do this, we also need to change gfxWindowsSurface::GetTextQualityInTransparentSurfaces to return TEXT_QUALITY_BAD. That will regress performance for Windows XP users where we have transparent layers containing text where we know the text is all over an opaque background, because we'll stop retaining those layers in order to preserve subpixel AA.

I would much prefer a solution that fixes cairo to average the color channel alpha values to compute the alpha value for the destination pixel.

Comment 87

[Gábor Stefanik]

(In reply to comment #86)
> I would much prefer a solution that fixes cairo to average the color channel
> alpha values to compute the alpha value for the destination pixel.

The only problem: how do we get separate alpha values for each color channel? Windows is not giving us any alpha values, and using a=(r+g+b)/3 (that is, using the color values themselves) doesn't work well.

Comment 88

[John Daggett (:jtd)]

Attachment: patch, v2, disable cleartype quality when rendering to non-opaque surfaces

Revised so that the quality setting is only swizzled around when CLEARTYPE_QUALITY is set initially.

Comment 89

[John Daggett (:jtd)]

As discussed with roc in irc, getting the layer system to identify ARGB surfaces with opaque content would be a better basis for deciding between the two rendering pathes here.  The current code infers "opaque" from content == CAIRO_CONTENT_COLOR (i.e. no alpha) which doesn't cover the case of a transparent layer where all alpha values are set to opaque.

Comment 90

[John Daggett (:jtd)]

(In reply to comment #87)
> The only problem: how do we get separate alpha values for each color channel?
> Windows is not giving us any alpha values, and using a=(r+g+b)/3 (that is,
> using the color values themselves) doesn't work well.

The current code actually constructs a mask containing the separate alpha values but there's no way to save that away, the result is an ARGB surface. So from (Ar, Ag, Ab) we somehow must distill this to a single A value and that's where the quality loss occurs.

Comment 91

[John Daggett (:jtd)]

Jeff, I'm passing this over to you.

The conclusion of the discussion with roc about this is that we need a flag on the surface that declares the background under text to be opaque, such that the rendering code can render using subpixel AA directly to an ARGB surface.  The layer system will set this flag for areas it knows to be opaque.  In the GDI case, which ignores the alpha channel, we can just whack the alpha to 1, similar to what Chrome does.

The exact extent to define as the background of the text needs to be flushed out.  Roc wants to have a special rect set on the surface that controls this explicitly.  He also thought we should do this for all surfaces, not just Win32 surfaces.  I think it might make sense to do this first for Win32 surfaces and see the results before pushing it to other platforms also.

Comment 92

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Taking

Comment 93

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I know at the all-hands we discussed various approaches and decided against just using a context or surface flag to say "trust me, all my glyphs are over opaque pixels", on the grounds that it was very difficult to spec out the API. Instead we decided to add API to specify one or more rectangles where we say "we promise that the contents of these rectangles are currently all opaque".

But now that I come to implement it, I really want to go with the flag, at least for GDI.

Comment 94

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Although, if we can make compositing of component-alpha text over transparent pixels work nearly as well as grayscale-AA text here, we won't actually need a flag anyway.

It seems to me the problem is actually in the way pixman composites the component-alpha image. We shouldn't be trying to compute an "overall alpha" value for each mask pixel which is independent of the source value. Instead, I think pixman should completely ignore the alpha value of a component-alpha mask pixel. Once it sees the complete source pixel and the mask channel values when compositing, then and only then I think we have enough information to compute a good overall alpha value for the mask.

Comment 95

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Modified colorwheel

Modified version of John's testcase. I've added some small solid-color blocks behind the text so that you can compare the rendering of text over opaque pixels with text over transparent pixels in the same opacity group.

Comment 96

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Hacked-up prototype to show what's possible

I think it helps to think of this as a problem of compositing. The fundamental problem is the computation of the alpha value for the destination pixel when we blend a source with a component-alpha mask onto an ARGB surface. We can actually compute three destination alpha values --- one per color channel. If they're not all equal, then any given destination alpha we choose for the pixel will be wrong for one or more of the channels. So far, so obvious.

This patch addresses the problem in pixman by checking whether the destination pixel is already opaque. In that case, the final destination channel alphas are all guaranteed to be 1, and the destination pixel alpha will be 1, so everything's consistent and there is no problem. But if the destination pixel is not already opaque, we force things to be consistent by converting all the mask channel alphas to be equal! In fact we set each mask channel alpha to the average of them all. In practice, this means we get something close to grayscale antialiasing wherever text is over transparent pixels, and subpixel-AA where text is over opaque pixels --- exactly what we want.

There's a big question of how this blending behavior should be exposed to cairo users. Here I've made this the behavior you get when you composite an RGB24 component-alpha image into an ARGB32 destination with OPERATOR_OVER. Currently it seems to me that we actually use the (bogus) alpha value from the RGB24 image to produce the destination pixel alpha. I'm not really sure what cairo/pixman currently is supposed to do in this situation. Maybe my proposed behavior is as good as any.

The patch is a big hack. The (bogus) pixman-general path that gets used to composite RGB24 component-alpha masks onto ARGB32 with OVER calls out to SSE2 and MMX fast-paths, so I've disabled those temporarily. A better patch would detect RGB24-component-alpha-mask-over-ARGB32 specifically in pixman-general and do something reasonable for all the operators, including an SSE2 implementation for OVER.

Or maybe there's a better approach with a different API altogether.

Comment 97

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe in fact it would make sense to have all cases of component-alpha masks with ARGB32 destinations behave like this. The current behavior for ARGB32 component alpha masks doesn't make sense to me either.

Comment 98

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: screenshot with patch

Screenshot of "Modified colorwheel" with my patch.

Comment 99

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This patch is along the lines of comment #77. It's super-simple, works great. The API may be useful for other situations where we currently use PushGroup.

It doesn't solve everything. Sometimes we have to draw text to a transparent surface, e.g. when drawing to a <canvas>, or when using BasicLayers to draw to a transparent window. So I think we should take John's patch as well, to disable cleartype by default when drawing to non-opaque surfaces. And if we do that, we should also have another patch that adds a flag to force Cleartype to be enabled when we know the text will draw over opaque pixels. I'll write that patch next.

Comment 100

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The second paragraph assumes the approach in comment #96 isn't going to fly in the short term.

Comment 101

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 486839 [details] [diff] [review]
fix

Can you add a comment to the place where we call PopGroup where we might have used PushGroupAndCopyBackground, to add a warning for anyone modifying the code that the expectation is OPERATOR_OVER only?

Comment 102

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: reenable Cleartype on selected surfaces

Straightforward. This applies on top of John's patch.

Comment 103

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 486848 [details] [diff] [review]
Part 3: reenable Cleartype on selected surfaces

This patch has a bug. Will revise.

Comment 104

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: reenable Cleartype on selected surfaces

This works properly.

Comment 105

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: Track opaque rects in surfaces and use them to improve PushGroupAndCopyBackground

With the previous patches we still have some problems with BasicLayers when the window is transparent --- e.g. when someone has Aero Glass enabled but we've blacklisted the drivers. Basically, when we take the path in BasicThebesLayer::Paint that avoids retaining the layer by drawing directly into the backbuffer, we are likely to find that the backbuffer is a transparent surface, and Cleartype will be disabled. So, some non-retained layers mysteriously lose subpixel AA.

This patch fixes that problem by tracking in gfxASurface a device-space rectangle which is known to be fully opaque. In PushGroupAndCopyBackground, if the rounded clip extents are entirely inside such a rectangle, we can safely copy up the buffer contents to the pushed surface, even if the outer surface has an alpha channel.

We can set the opaque rect whenever we start painting inside a layer that is marked as opaque (meaning that every pixel in the visible region either paints something opaque or is covered by something opaque) --- if the layer's visible region maps to a device-space rectangle. It's enough to only set the rect based on the outermost opaque layer, since its visible region contains the visible regions of its descendants.

Firefox's Web content area is in a container layer that is always opaque, so this patch ensures that the content area is marked opaque while we draw Web content, so our PushGroupAndCopyBackgrounds will always work for Web content even if the window is transparent.

With all these patches together, Web content drawn to a window with a BasicLayerManager should always get correct subpixel-AA on Windows.

Comment 106

[Robert O'Callahan (:roc) (email my personal email if necessary)]

... when using GDI drawing.

Comment 107

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase

This tests most of the code paths. We can make a reftest out of it once we have it working everywhere.

Comment 108

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 487057 [details] [diff] [review]
Part 3: reenable Cleartype on selected surfaces

(don't forget patch file in gfx/cairo, entry in README, etc.)

Comment 109

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 487057 [details] [diff] [review]
Part 3: reenable Cleartype on selected surfaces

;
> 
> 	if (use_cleartype_mask)
> 	    mask.base.has_component_alpha = TRUE;
> 
> 	status = _cairo_surface_composite (op, pattern,
> 					   &mask.base,
>-					   &surface->base,
>+					   generic_surface,

What's this change for?

Comment 110

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 473948 [details] [diff] [review]
patch, v2, disable cleartype quality when rendering to non-opaque surfaces

> 
>+	/* Only use Cleartype rendering when drawing to an opaque surface.
>+	 * Transparent surfaces are blended with a background and because
>+	 * the background isn't known here, it's not possible to 
>+	 * do an accurate subpixel blend.  Without the background, subpixel
>+         * AA could result in radically different rendering than intended.
>+	 */
>+
>+	cleartype_quality = (scaled_font->quality == CLEARTYPE_QUALITY);
>+	use_cleartype_mask = (cleartype_quality && surface->base.content == CAIRO_CONTENT_COLOR);

I wonder if it would be better to check the surface format instead of the content type.

Comment 111

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #109)
> Comment on attachment 487057 [details] [diff] [review]
> Part 3: reenable Cleartype on selected surfaces
> 
> ;
> > 
> > 	if (use_cleartype_mask)
> > 	    mask.base.has_component_alpha = TRUE;
> > 
> > 	status = _cairo_surface_composite (op, pattern,
> > 					   &mask.base,
> >-					   &surface->base,
> >+					   generic_surface,
> 
> What's this change for?

To avoid using 'surface' (which is a cairo_win32_surface_t*) when the surface actually isn't actually a win32 surface. It's cosmetic.

(In reply to comment #110)
> Comment on attachment 473948 [details] [diff] [review]
> patch, v2, disable cleartype quality when rendering to non-opaque surfaces
> 
> > 
> >+	/* Only use Cleartype rendering when drawing to an opaque surface.
> >+	 * Transparent surfaces are blended with a background and because
> >+	 * the background isn't known here, it's not possible to 
> >+	 * do an accurate subpixel blend.  Without the background, subpixel
> >+         * AA could result in radically different rendering than intended.
> >+	 */
> >+
> >+	cleartype_quality = (scaled_font->quality == CLEARTYPE_QUALITY);
> >+	use_cleartype_mask = (cleartype_quality && surface->base.content == CAIRO_CONTENT_COLOR);
> 
> I wonder if it would be better to check the surface format instead of the
> content type.

Why would that be better?

Comment 112

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #111)
> > 
> > I wonder if it would be better to check the surface format instead of the
> > content type.
> 
> Why would that be better?

Because it seems like our ability to use cleartype has more to do with the alpha byte being ignored (RGB24) than what the content of the surface is.

Comment 113

[Robert O'Callahan (:roc) (email my personal email if necessary)]

CONTENT_COLOR says there is no alpha to worry about. Isn't that a more precise check == RGB24 or != ARGB32, both of which would become incorrect if we supported other formats with/without alpha?

Comment 114

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #113)
> CONTENT_COLOR says there is no alpha to worry about. Isn't that a more precise
> check == RGB24 or != ARGB32, both of which would become incorrect if we
> supported other formats with/without alpha?

Yep, that makes sense to me. I was a little bit confused about the meaning of 'use_cleartype_mask'; it makes more sense to me now.

Comment 115

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 487063 [details] [diff] [review]
Part 4: Track opaque rects in surfaces and use them to improve PushGroupAndCopyBackground


>
>+  PRBool pushedTargetOpaqueRect = PR_FALSE;
>+  const nsIntRegion& visibleRegion = aLayer->GetEffectiveVisibleRegion();
>+  nsRefPtr<gfxASurface> currentSurface = mTarget->CurrentSurface();
>+  const gfxRect& targetOpaqueRect = currentSurface->GetOpaqueRect();
>+  if (targetOpaqueRect.IsEmpty() && visibleRegion.GetNumRects() == 1 &&
>+      (aLayer->GetContentFlags() & Layer::CONTENT_OPAQUE) &&
>+      !transform.HasNonAxisAlignedTransform()) {
>+    const nsIntRect& bounds = visibleRegion.GetBounds();
>+    currentSurface->SetOpaqueRect(
>+        mTarget->UserToDevice(gfxRect(bounds.x, bounds.y, bounds.width, bounds.height)));
>+    pushedTargetOpaqueRect = PR_TRUE;
>+  }

When I saw this hunk, it felt very much like a wall of text.
Perhaps add a summary comment and some whitespace.


>+    void SetOpaqueRect(const gfxRect& aRect) {
>+        if (aRect.IsEmpty()) {
>+            mOpaqueRect = nsnull;
>+        } else if (mOpaqueRect) {
>+            *mOpaqueRect = aRect;
>+        } else {
>+            mOpaqueRect = new gfxRect(aRect);
>+        }
>+    }
>+    const gfxRect& GetOpaqueRect() {
>+        if (mOpaqueRect)
>+            return *mOpaqueRect;
>+        static const gfxRect empty(0, 0, 0, 0);
>+        return empty;
>+    }

Why not just store a gfxRect in the surface instead of a pointer to a rect? We don't use a NULL mOpaqueRect for anything.

Comment 116

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #115)
> When I saw this hunk, it felt very much like a wall of text.
> Perhaps add a summary comment and some whitespace.

OK.

> >+    void SetOpaqueRect(const gfxRect& aRect) {
> >+        if (aRect.IsEmpty()) {
> >+            mOpaqueRect = nsnull;
> >+        } else if (mOpaqueRect) {
> >+            *mOpaqueRect = aRect;
> >+        } else {
> >+            mOpaqueRect = new gfxRect(aRect);
> >+        }
> >+    }
> >+    const gfxRect& GetOpaqueRect() {
> >+        if (mOpaqueRect)
> >+            return *mOpaqueRect;
> >+        static const gfxRect empty(0, 0, 0, 0);
> >+        return empty;
> >+    }
> 
> Why not just store a gfxRect in the surface instead of a pointer to a rect? We
> don't use a NULL mOpaqueRect for anything.

To save 28 or 24 bytes per surface.

Comment 117

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4

I rebased all these patches on top of bug 612840. There were no significant changes, but I might as well attach the updated part 4 since it's still waiting for review

Comment 118

[Timothy Nikkel (:tnikkel)]

Comment on attachment 487057 [details] [diff] [review]
Part 3: reenable Cleartype on selected surfaces

>+SetAntialiasingFlags(Layer* aLayer, gfxContext* aTarget)
>+  if (surface->GetType() == gfxASurface::SurfaceTypeWin32) {
>+    gfxWindowsSurface* ws = static_cast<gfxWindowsSurface*>(surface.get());

Does this need to be wrapped in a Windows ifdef?

Comment 119

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3 v2

It actually is in my latest version of the patch.

Comment 120

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Introduce cairo_surface_get/set_subpixel_antialiasing

This replaces the previous cairo patches.

We introduce a new permit_subpixel_antialiasing flag for all surface types. By default it is true so there is no behavior change for cairo. There are implementations of !permit_subpixel_antialiasing for Quartz, xlib and GDI. (DirectWrite will have to be tackled later.) This differs from the previous GDI patch not just in breadth and API, but also in effect: the old patch would, for transparent surfaces, render with Cleartype and then use compute_mask_a8 to extract an A8 mask. This patch actually renders the text using a grayscale AA instead. This produces better-looking results.

Comment 121

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2.3: Disable subpixel AA in gfxASurfaces with alpha channels by default

This changes Thebes so that new RGBA surfaces disable subpixel AA by default.

Comment 122

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2.6: Wrap cairo API in gfxASurface::Get/SetSubpixelAntialiasingEnabled

Simple wrapper stuff.

Comment 123

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3 v3

This patch is now simpler and not win32-dependent. I'll carry forward the review if no-one objects.

Comment 124

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2 v2

While individual glyphs look better with ANTIALIASED_QUALITY than with CLEARTYPE_QUALITY+mask_a8, text can actually look pretty bad because the glyph spacing is for CLEARTYPE_QUALITY and ANTIALIASED_QUALITY is designed for very different glyph spacing. So this patch uses CLEARTYPE_QUALITY+mask_a8 when subpixelAA is disabled.

Comment 125

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #120)
> Created attachment 493160 [details] [diff] [review]
> Part 2: Introduce cairo_surface_get/set_subpixel_antialiasing
> 
> This replaces the previous cairo patches.
> 
> We introduce a new permit_subpixel_antialiasing flag for all surface types. By
> default it is true so there is no behavior change for cairo. There are
> implementations of !permit_subpixel_antialiasing for Quartz, xlib and GDI.
> (DirectWrite will have to be tackled later.) This differs from the previous GDI
> patch not just in breadth and API, but also in effect: the old patch would, for
> transparent surfaces, render with Cleartype and then use compute_mask_a8 to
> extract an A8 mask. This patch actually renders the text using a grayscale AA
> instead. This produces better-looking results.

Won't adding the api on cairo_surface is that it will break compilation with system cairo. It would also be good to propose this api on the cairo mailing list.

Comment 126

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #125)
> Won't adding the api on cairo_surface is that it will break compilation with
> system cairo.

I guess, but what choice do we have? Distros can stub out the calls if they want to use system cairo. I don't think it's worth jumping through hoops on our side to support system cairo.

> It would also be good to propose this api on the cairo mailing
> list.

I will, but I hope that progress in this bug doesn't depend on it.

Comment 127

[Joe Drew (not getting mail)]

I will cry one tear for breaking system Cairo, put that tear in a test tube, and seal that test tube with a rubber stopper. Then I'll fix more blockers.

Comment 128

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 493178 [details] [diff] [review]
Part 2 v2

API looks fine to me, one typo/bug:


>+/**
>+ * cairo_surface_get_subpixel_antialiasing:
>+ * @surface: a #cairo_surface_t
>+ *
>+ * Sets whether the surface supports subpixel antialiasing. By default,
>+ * CAIRO_CONTENT_COLOR surfaces support subpixel antialiasing but other
>+ * surfaces do not.

This function is the getter, not the setter -- just need updated doc text.

>+ *
>+ * Enabling subpixel antialiasing for CONTENT_COLOR_ALPHA surfaces generally
>+ * requires that the pixels in the areas under a subpixel antialiasing
>+ * operation be opaque.
>+ *
>+ * Since: 1.12
>+ **/

Comment 129

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 493178 [details] [diff] [review]
Part 2 v2



>+static cairo_scaled_font_t *
>+_cairo_xlib_get_grayscale_font (cairo_xlib_surface_t *dst,
>+                                cairo_scaled_font_t *scaled_font)
>+{

Might be worth adding a comment that this gets a grayscale version of scaled_font and will reuse it if possible.

Comment 130

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/c53f60831c43
http://hg.mozilla.org/mozilla-central/rev/8857392e37ae
http://hg.mozilla.org/mozilla-central/rev/a9a3f265895c
http://hg.mozilla.org/mozilla-central/rev/2e2eca6d5a6a
http://hg.mozilla.org/mozilla-central/rev/5c0c6d6f11e7
http://hg.mozilla.org/mozilla-central/rev/b7d1bb50e829

Comment 132

[Octoploid]

This breaks the build with --enable-system-cairo on Linux:

/var/tmp/mozilla-central/gfx/thebes/gfxASurface.cpp: In member function ‘void gfxASurface::Init(cairo_surface_t*, PRBool)’:
/var/tmp/mozilla-central/gfx/thebes/gfxASurface.cpp:225:62: error: ‘CAIRO_SUBPIXEL_ANTIALIASING_DISABLED’ was not declared in this scope
/var/tmp/mozilla-central/gfx/thebes/gfxASurface.cpp:225:98: error: ‘cairo_surface_set_subpixel_antialiasing’ was not declared in this scope
/var/tmp/mozilla-central/gfx/thebes/gfxASurface.cpp: In member function ‘void gfxASurface::SetSubpixelAntialiasingEnabled(PRBool)’:
/var/tmp/mozilla-central/gfx/thebes/gfxASurface.cpp:441:20: error: ‘CAIRO_SUBPIXEL_ANTIALIASING_ENABLED’ was not declared in this scope
/var/tmp/mozilla-central/gfx/thebes/gfxASurface.cpp:441:58: error: ‘CAIRO_SUBPIXEL_ANTIALIASING_DISABLED’ was not declared in this scope
/var/tmp/mozilla-central/gfx/thebes/gfxASurface.cpp:441:94: error: ‘cairo_surface_set_subpixel_antialiasing’ was not declared in this scope
/var/tmp/mozilla-central/gfx/thebes/gfxASurface.cpp: In member function ‘PRBool gfxASurface::GetSubpixelAntialiasingEnabled()’:
/var/tmp/mozilla-central/gfx/thebes/gfxASurface.cpp:449:60: error: ‘cairo_surface_get_subpixel_antialiasing’ was not declared in this scope
/var/tmp/mozilla-central/gfx/thebes/gfxASurface.cpp:449:65: error: ‘CAIRO_SUBPIXEL_ANTIALIASING_ENABLED’ was not declared in this scope
/var/tmp/mozilla-central/gfx/thebes/gfxASurface.cpp:450:1: error: control reaches end of non-void function

Comment 133

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yep.

Comment 134

[Octoploid]

Great. Now what?

Comment 135

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Whoever wants --enable-system-cairo to keep working will have to provide a patch to put the new functionality behind a configure check.

Comment 136

[Octoploid]

In other words: Linux isn't considered a worthy target for Firefox anymore.
Whenever an unrelated Windows patch breaks the Linux build, Mozilla will not
bother to fix it. This is left as an exercise to the distributions...

Comment 137

[Timothy Nikkel (:tnikkel)]

Compiling with the in tree cairo (the default) works fine.

Comment 138

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The patches here fix bugs on all platforms including Linux. This bug was originally about Windows but related issues happen on all platforms. If you look at "part 2" you will see a bunch of Xlib-specific code I wrote specifically to ensure that Linux stays at parity with the other platforms. Given that, I feel aggrieved by your accusations.

Linux builds continue to build and work fine as long as you use the default configure options, which --enable-system-cairo is not.

Comment 139

[Octoploid]

The reason why one would use "--enable-system-cairo" is that
without it Firefox does not honor the "lcdfilter" option in
.fonts.conf. This results in ugly color fringes on the glyphs,
when subpixel rendering is enabled.
So there are two possibilities: Either to use 
"--enable-system-cairo" or to update the in-tree cairo version,
so that it recognizes the "lcdfilter" option.

And BTW --enable-system-cairo always worked fine until your 
checkin.

Comment 140

[Ilja Sekler]

Attachment: Ubuntu lcd filter patch for in-tree cairo

Unfortunately, it is not just the fancy lcd filter which makes system cairo 1.10 so beneficial on Ubuntu 10.10. Just try to print <http://weblogs.mozillazine.org/asa/> to PDF in Firefox once with an in-tree-cairo build and once with a system-cairo build and "feel the difference" in form of blurry image fallbacks for semi-transparent parts of the page as a result of the in-tree cairo. Perfect visual quality of the PDF print with cairo 1.10.

Comment 141

[Oleg Romashin (:romaxa)]

Is http://hg.mozilla.org/mozilla-central/rev/8857392e37ae patch  coming to upstream cairo soon?
Can we just build with system cairo and enable system cairo version check or something like that?

Comment 142

[Oleg Romashin (:romaxa)]

Attachment: Quick workaround for missing cairo features

Comment 143

[Ilja Sekler]

(In reply to comment #142)
> Created attachment 501993 [details] [diff] [review]
> Quick workaround for missing cairo features

Thank you, maybe I got you wrong, but a build with "--enable-system-cairo" and this patch applied fails:

c++ -o gfxTextRunCache.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /home/ilja/moz/m-c/src/config/gcc_hidden.h -DIMPL_THEBES -DWOFF_MOZILLA_CLIENT -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux -DEXCLUDE_SKIA_DEPENDENCIES -DCHROMIUM_MOZILLA_BUILD -DOS_LINUX=1 -DOS_POSIX=1 -I/home/ilja/moz/m-c/src/ipc/chromium/src -I/home/ilja/moz/m-c/src/ipc/glue -I../../ipc/ipdl/_ipdlheaders  -I/home/ilja/moz/m-c/src/gfx/thebes -I. -I../../dist/include -I../../dist/include/nsprpub  -I/home/ilja/moz/m-c/fx-build/dist/include/nspr -I/home/ilja/moz/m-c/fx-build/dist/include/nss       -fPIC -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-variadic-macros -Werror=return-type -Wno-long-long -fno-strict-aliasing -fshort-wchar -pthread -pipe -DNDEBUG -DTRIMMED -Os -freorder-blocks -fomit-frame-pointer  -pthread -I/usr/include/cairo -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12   -pthread -I/usr/include/cairo -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12   -pthread -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/gtk-unix-print-2.0   -pthread -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/cairo -I/usr/include/pixman-1 -I/usr/include/libpng12     -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MF .deps/gfxTextRunCache.pp /home/ilja/moz/m-c/src/gfx/thebes/gfxTextRunCache.cpp
/home/ilja/moz/m-c/src/gfx/thebes/gfxTeeSurface.cpp: In member function ‘virtual const gfxIntSize gfxTeeSurface::GetSize() const’:
/home/ilja/moz/m-c/src/gfx/thebes/gfxTeeSurface.cpp:67: error: no return statement in function returning non-void
make[5]: *** [gfxTeeSurface.o] Fehler 1

Build with in-tree cairo succeeds.

hg identify
80dcb82a0cb7 tip

Comment 144

[Octoploid]

Yes, as the gcc error says, a non void function needs a return
statement:
Something like this should work:
in gfx/thebes/gfxTeeSurface.cpp

const gfxIntSize                                                                                                                                     
gfxTeeSurface::GetSize() const                                                                                                                       
{                                                                                                                                                    
#ifdef MOZ_TREE_CAIRO                                                                                                                                
    nsRefPtr<gfxASurface> master = Wrap(cairo_tee_surface_index(mSurface, 0));                                                                       
    return master->GetSize();                                                                                                                        
#else                                                                                                                                                
    gfxIntSize a;                                                                                                                                    
    return a;                                                                                                                                        
#endif                                                                                                                                               
}

Comment 145

[Ilja Sekler]

(In reply to comment #144)

Thanks, this fixed compilation, but with the resulting build I see a new, likely unrelated issue that semi-transparent parts of e.g. <http://weblogs.mozillazine.org/asa/archives/2010/12/visualizing_plugin_m.html> (.posted) are printed to PDF with image fallbacks like with the in-tree cairo, but these fallbacks are completely blank (white) if an ancestor element doesn't have opacity!=1. Only print output is affected, print preview is fine.

I'll investigate further and file a new bug if necessary.

Comment 146

[Ilja Sekler]

Filed bug 624152 on issue described in comment #145.

Comment 147

[Mgnn]

This makes me really disappointed. 

Many linux distributions uses custom patched system cairo installations. To
have consistent rendering it is good if dependent software respects those
settings and uses it!

I cannot understand the logic behind putting in the energy to fix a rendering
issue, and then completely disregard if it breaks rendering in other ways 
for many users.

Even though --enable-system-cairo is not the default option it is practically
required to get decent font rendering to be able to actually use firefox

Comment 148

[Ilja Sekler]

(In reply to comment #147)

> Many linux distributions uses custom patched system cairo installations. To
> have consistent rendering it is good if dependent software respects those
> settings and uses it!

The trunk required cairo 1.10 or later to build with --enable-system-cairo. AFAIK only Ubuntu 10.10 and derivatives shipped cairo 1.10 together with libfreetype6 where both bytecode interpreter and subpixel were enabled to get beautiful font rendering.

> Even though --enable-system-cairo is not the default option it is practically
> required to get decent font rendering to be able to actually use firefox

No, Ubuntu doesn't use --enable-system-cairo but patches in-tree cairo for its own builds. I've extracted this patch and attached to this bug for convenience.

https://bugzilla.mozilla.org/attachment.cgi?id=501837

My apologies for the bugspam.

Comment 149

[Siarhei Siamashka]

(In reply to comment #126)
> > It would also be good to propose this api on the cairo mailing
> > list.
> 
> I will, but I hope that progress in this bug doesn't depend on it.

Forward-porting the subpixel antialiasing patch to cairo 1.10.x code and submitting it to the cairo mailing list would be very nice. Do you have any rough estimate when this could be done (days/weeks/months/years)?

Comment 150

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know.

Comment 151

[Mike Hommey [:glandium]]

Other than the system cairo breakage, there is a lack of a corresponding .patch file in gfx/cairo.

Comment 152

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I have that in my patch queue, I'll land it ASAP.

Comment 153

[Mike Hommey [:glandium]]

(In reply to comment #142)
> Created attachment 501993 [details] [diff] [review]
> Quick workaround for missing cairo features

Did you file a separate bug for that?

Comment 154

[Mike Hommey [:glandium]]

(Sorry for the noise) I filed bug 624684 for gfxTeeSurface.cpp, which is a completely separate issue from what was broken here.

Comment 155

[Karl Tomlinson (:karlt)]

Comment on attachment 501993 [details] [diff] [review]
Quick workaround for missing cairo features

Obsoleted by bug 623797 and bug 624684.

Comment 156

[DonRumata]

Why it's marked as RESOLVED FIXED?
Compare this screenshots:
FF 3.6.13 http://habreffect.ru/files/747/c7046e0f8/2011-01-16_022016.png
FF 4.0b9  http://habreffect.ru/files/e1d/714b80720/2011-01-16_022229.png

Comment 157

[u310142]

(In reply to comment #156)
> Why it's marked as RESOLVED FIXED?

Because it is. Your problem is bug 622482.

Comment 158

[Hexadecima]

Hi. This one goes out more or less to Bobby Johnson, who seems to know everything about ClearType rendering bugs.

The new Slashdot design seems to be misbehaving in the manner typical of this bug: http://doom.iri5.net/show/slashdot-issues.png

I thought it was a fairly apt example of this kind of misbehaviour. Any ideas?

Build: Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110125 Firefox/4.0b10pre ID:20110125030335

Comment 159

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What configuration was used to generate that? I'm not sure that's related to this bug at all.

Comment 160

[Hexadecima]

Thanks for replying, and apologies to the 79 other people getting spammed with this. Here's what Nightly Tester Tools puts on pastebin when asked to dump about:support: http://pastebin.com/cEPYPC6p

Comment 161

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Please file a new bug for that.

Comment 162

[kinokijuf]

Is bug #923778 related to this one?