GDI ClearType rendered to RGBA surfaces sometimes looks bad

RESOLVED FIXED

Status

()

defect
P4
major
RESOLVED FIXED
13 years ago
3 years ago

People

(Reporter: bugzilla, Assigned: roc)

Tracking

({regression, testcase})

Trunk
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +
blocking1.9 -
wanted1.9 +
in-testsuite ?

Firefox Tracking Flags

(blocking2.0 beta9+)

Details

(URL)

Attachments

(35 attachments, 8 obsolete attachments)

1.95 KB, image/png
Details
1.96 KB, image/png
Details
4.24 KB, text/html
Details
36.18 KB, image/png
Details
3.63 KB, image/jpeg
Details
6.11 KB, text/html
Details
86.18 KB, image/gif
Details
80.50 KB, image/gif
Details
852 bytes, application/vnd.mozilla.xul+xml
Details
4.89 KB, image/png
Details
1.79 KB, image/png
Details
11.88 KB, image/png
Details
1.96 KB, patch
Details | Diff | Splinter Review
7.41 KB, text/html
Details
11.59 KB, image/png
Details
9.20 KB, image/png
Details
41.84 KB, image/png
Details
76.88 KB, image/png
Details
33.83 KB, image/png
Details
25.52 KB, image/png
Details
37.74 KB, image/png
Details
78.76 KB, image/png
Details
12.05 KB, text/plain
Details
25.65 KB, text/plain
Details
2.44 KB, text/html
Details
5.52 KB, patch
Details | Diff | Splinter Review
138.28 KB, image/png
Details
fix
4.25 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
965 bytes, text/html
Details
5.77 KB, patch
Details | Diff | Splinter Review
826 bytes, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.08 KB, patch
vlad
: review+
Details | Diff | Splinter Review
2.78 KB, patch
Details | Diff | Splinter Review
18.78 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
30.88 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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
(Reporter)

Comment 1

13 years ago
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.
(Reporter)

Comment 2

13 years ago
Component: General → GFX: Thebes
Product: Firefox → Core
QA Contact: general → thebes
Version: unspecified → Trunk

Comment 3

13 years ago
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.
Keywords: regression, testcase

Comment 4

13 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase

Comment 5

13 years ago
Posted file Testcase
Testcase. Hover over the menu in it to trigger the bug.

Updated

13 years ago
Keywords: testcase

Updated

13 years ago
Summary: ClearType + Opacity text rendering → [cairo]ClearType + Opacity text rendering

Comment 6

12 years ago
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.
Flags: blocking1.9?
Duplicate of this bug: 357456
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Flags: wanted1.9+
Whiteboard: [wanted-1.9]

Comment 9

11 years ago
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

11 years ago
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

11 years ago
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

11 years ago
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.

Updated

11 years ago
Duplicate of this bug: 431904

Comment 14

11 years ago
The Glasser Extension does not work properly because of this ...

Comment 15

11 years ago
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.  

Updated

11 years ago
Duplicate of this bug: 434666

Comment 18

11 years ago
Rendered in Firefox 3 RC1 on Windows Vista.

Comment 19

11 years ago
Rendered in Firefox 3 RC1 on Windows Vista.

Updated

11 years ago
Attachment #321706 - Attachment mime type: text/plain → text/html

Updated

11 years ago
Duplicate of this bug: 370429
Blocks: 433825

Updated

11 years ago
Flags: wanted1.9.1?
Flags: blocking1.9.1?

Updated

11 years ago
Duplicate of this bug: 439804

Updated

11 years ago
Duplicate of this bug: 439850

Updated

11 years ago
Severity: normal → major

Updated

11 years ago
Duplicate of this bug: 439872

Updated

11 years ago
Duplicate of this bug: 440791
Don't think this would block the 1.9.1 release.  Minusing.
Flags: blocking1.9.1? → blocking1.9.1-

Updated

11 years ago
Duplicate of this bug: 433825

Comment 27

11 years ago
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?
Blocks: 436304
Blocks: 395980

Comment 28

11 years ago
Does this bug depend on bug 407531, or vice versa?

Updated

11 years ago
No longer blocks: 433825

Comment 29

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

Neither.
Flags: wanted1.9.1? → wanted1.9.1+
Priority: -- → P4

Comment 30

11 years ago
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

11 years ago
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 34

11 years ago
Posted image 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.

Updated

10 years ago
Assignee: nobody → jmathies
Blocks: 413059

Comment 35

10 years ago
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

10 years ago
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

10 years ago
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!

Updated

10 years ago
Assignee: jmathies → nobody

Updated

10 years ago
Duplicate of this bug: 457538

Updated

10 years ago
Assignee: nobody → jdaggett

Comment 39

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

Comment 40

10 years ago
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 42

10 years ago
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

Updated

10 years ago
Duplicate of this bug: 457538

Comment 44

10 years ago
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

10 years ago
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

10 years ago
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).
Duplicate of this bug: 520400
(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.

Updated

10 years ago
Duplicate of this bug: 512136

Comment 50

10 years ago
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).
Depends on: 527707

Updated

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

Comment 52

9 years ago
(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

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

Comment 54

9 years ago
(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

9 years ago
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.
This is going to affect another ~50% of our users, since bug 504698 forces cleartype on XP users. Requesting blocking.
Blocks: 504698
blocking2.0: --- → ?
We need to disable cleartype on non-opaque surfaces.
blocking2.0: ? → final+

Comment 58

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

Comment 59

9 years ago
(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

9 years ago
(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

9 years ago
(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

9 years ago
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

9 years ago
This bug shows in the new addons manager for disabled addons which are alpha faded.  The blue developer link shows the mixed greenish coloring.
Opacity pinwheel test
http://people.mozilla.org/~jdaggett/tests/opacitytest.html

(Click to vary the background)
Mozilla/5.0 (Windows; Windows NT 5.1; en-US; rv:2.0b2pre) Gecko/20100712 Minefield/4.0b2pre
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:2.0b1) Gecko/20100630 Firefox/4.0b1
(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

9 years ago
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).
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.
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

9 years ago
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.
(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

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

Comment 74

9 years ago
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.
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
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.
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.

Updated

9 years ago
Attachment #461415 - Attachment description: screenshot, opacity pinwheel, ff trunk vs. ff w/ grayscale antialiasing on transparent layers → screenshot, opacity pinwheel win7, ff trunk vs. ff w/ grayscale antialiasing on transparent layers
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).
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.
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?
(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.
(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!
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.
Attachment #473940 - Flags: review?(jmuizelaar)
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

9 years ago
(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.
Revised so that the quality setting is only swizzled around when CLEARTYPE_QUALITY is set initially.
Attachment #473940 - Attachment is obsolete: true
Attachment #473948 - Flags: review?(jmuizelaar)
Attachment #473940 - Flags: review?(jmuizelaar)
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.
(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.
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.
Assignee: jdaggett → jmuizelaar
Taking
Assignee: jmuizelaar → roc
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.
Summary: [cairo]ClearType + Opacity text rendering → GDI ClearType rendered to RGBA surfaces sometimes looks bad
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.
Posted file 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.
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.
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.
Screenshot of "Modified colorwheel" with my patch.
Posted patch fixSplinter Review
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.
Attachment #486839 - Flags: superreview?(vladimir)
Attachment #486839 - Flags: review?(jmuizelaar)
The second paragraph assumes the approach in comment #96 isn't going to fly in the short term.
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?
Attachment #486839 - Flags: superreview?(vladimir) → superreview+
Straightforward. This applies on top of John's patch.
Attachment #486848 - Flags: superreview?(vladimir)
Attachment #486848 - Flags: review?(jmuizelaar)
Comment on attachment 486848 [details] [diff] [review]
Part 3: reenable Cleartype on selected surfaces

This patch has a bug. Will revise.
Attachment #486848 - Attachment is obsolete: true
Attachment #486848 - Flags: superreview?(vladimir)
Attachment #486848 - Flags: review?(jmuizelaar)
This works properly.
Attachment #487057 - Flags: superreview?(vladimir)
Attachment #487057 - Flags: review?(jmuizelaar)
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.
Attachment #487063 - Flags: superreview?(vladimir)
Attachment #487063 - Flags: review?(jmuizelaar)
... when using GDI drawing.
Posted file testcase
This tests most of the code paths. We can make a reftest out of it once we have it working everywhere.
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.)
Attachment #487057 - Flags: superreview?(vladimir) → superreview+
Attachment #487065 - Attachment is patch: false
Attachment #487065 - Attachment mime type: text/plain → text/html
Attachment #486839 - Flags: review?(jmuizelaar) → review+
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?
Attachment #487057 - Flags: review?(jmuizelaar) → review+
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.
Attachment #473948 - Flags: review?(jmuizelaar) → review+
(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?
(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.
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?
(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 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.
Attachment #487063 - Flags: review?(jmuizelaar) → review+
(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.
Posted patch Part 4Splinter Review
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
Attachment #487063 - Attachment is obsolete: true
Attachment #491136 - Flags: superreview?(vladimir)
Attachment #487063 - Flags: superreview?(vladimir)
Attachment #491136 - Flags: superreview?(vladimir) → superreview+
Whiteboard: [needs landing]
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?
Posted patch Part 3 v2 (obsolete) — Splinter Review
It actually is in my latest version of the patch.
Attachment #487057 - Attachment is obsolete: true
Whiteboard: [needs landing] → [needs review]
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.
Attachment #493160 - Flags: superreview?(vladimir)
Attachment #493160 - Flags: review?(jmuizelaar)
This changes Thebes so that new RGBA surfaces disable subpixel AA by default.
Attachment #493161 - Flags: review?(jmuizelaar)
Posted patch Part 3 v3Splinter Review
This patch is now simpler and not win32-dependent. I'll carry forward the review if no-one objects.
Attachment #473948 - Attachment is obsolete: true
Attachment #491484 - Attachment is obsolete: true
Posted patch Part 2 v2Splinter Review
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.
Attachment #493160 - Attachment is obsolete: true
Attachment #493178 - Flags: superreview?(vladimir)
Attachment #493178 - Flags: review?(jmuizelaar)
Attachment #493160 - Flags: superreview?(vladimir)
Attachment #493160 - Flags: review?(jmuizelaar)
(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.
(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.
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.
blocking2.0: final+ → beta9+
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
>+ **/
Attachment #493178 - Flags: superreview?(vladimir) → superreview+
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.
Attachment #493178 - Flags: review?(jmuizelaar) → review+
Attachment #493161 - Flags: review?(jmuizelaar) → review+
Whiteboard: [needs review] → [needs landing]

Updated

8 years ago
Duplicate of this bug: 605002

Comment 132

8 years ago
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 134

8 years ago
Great. Now what?
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

8 years ago
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...
Compiling with the in tree cairo (the default) works fine.
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

8 years ago
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

8 years ago
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.

Updated

8 years ago
Depends on: 623797
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?
OS: Windows XP → All

Comment 143

8 years ago
(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

8 years ago
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

8 years ago
(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

8 years ago
Filed bug 624152 on issue described in comment #145.

Comment 147

8 years ago
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

8 years ago
(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.
Depends on: 623852
(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)?
Other than the system cairo breakage, there is a lack of a corresponding .patch file in gfx/cairo.
I have that in my patch queue, I'll land it ASAP.
(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?
(Sorry for the noise) I filed bug 624684 for gfxTeeSurface.cpp, which is a completely separate issue from what was broken here.
Comment on attachment 501993 [details] [diff] [review]
Quick workaround for missing cairo features

Obsoleted by bug 623797 and bug 624684.
Attachment #501993 - Attachment is obsolete: true

Comment 156

8 years ago
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

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

Because it is. Your problem is bug 622482.

Comment 158

8 years ago
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
What configuration was used to generate that? I'm not sure that's related to this bug at all.

Comment 160

8 years ago
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
Please file a new bug for that.

Updated

8 years ago
Depends on: 647560

Updated

8 years ago
No longer depends on: 647560

Comment 162

6 years ago
Is bug #923778 related to this one?
See Also: → 1281993
You need to log in before you can comment on or make changes to this bug.