Build Firefox 3 against a subpixel-patched cairo

Bug #164640 reported by Maia Everett on 2007-11-23
12
Affects Status Importance Assigned to Milestone
Fontconfig
Fix Released
Wishlist
libcairo
Unknown
Medium
fontconfig (Ubuntu)
Wishlist
Unassigned
libcairo (Ubuntu)
Wishlist
Unassigned
xulrunner-1.9 (Ubuntu)
Medium
Unassigned

Bug Description

Binary package hint: firefox-3.0

In Ubuntu 7.10 and newer, Firefox 3.0 looks out of place with subpixel rendering enabled, due to the fact that it uses an unpatched, bundled version of Cairo.

The patch should be ported to Cairo 1.5 and applied in the firefox-3.0 package.

Created an attachment (id=9148)
cairo-1.2.4-lcd-filter-1.patch

It seems, that following URL may contain updated version of attached patch.
http://lists.gnu.org/archive/html/freetype/2006-10/msg00004.html

No, checking mail dates, cairo-1.2.4-lcd-filter-1.patch seems to be the latest I can find: http://<email address hidden>/msg01028.html

The patch gives impressive results when LCD filtering is enabled in Freetype, but it won't apply to 1.4.10 cleanly.

Created an attachment (id=10583)
cairo-1.4.10-lcd-filter-1.patch

My attempt to update patch for cairo 1.4.10.

Removing _cairo_error (CAIRO_STATUS_NO_MEMORY); from the code doesn't sound right :-/

Binary package hint: firefox-3.0

In Ubuntu 7.10 and newer, Firefox 3.0 looks out of place with subpixel rendering enabled, due to the fact that it uses an unpatched, bundled version of Cairo.

The patch should be ported to Cairo 1.5 and applied in the firefox-3.0 package.

Behdad,

Is there any chance you are gonna apply this patch? This patch improves rendering a lot but its once again out of sync with cairo 1.5.2.

Please comment.

Anders Kaseorg (andersk) on 2007-11-29
Changed in firefox-3.0:
status: New → Confirmed

Looks good to me generally. Can go in 1.6 after testing.

Very good news, David Turner will like this news :-)

Well, he has a much larger chunk of code rewriting cairo-ft that is awaiting review...

(In reply to comment #10)
> Well, he has a much larger chunk of code rewriting cairo-ft that is awaiting
> review...

Must be private as I haven't seen it but his patches are always quality anyway, maybe its a good time to review it? :)

With this patch GTK+ apps looks really sharp.

It's been sent to cairo list multiple times (improved versions). Yes, his patches are very good, but it completely rewrites cairo-ft.c, so can use a review still.

I really like to get that in 1.6. Will see.

Please review carefully, porting of this patch was not trivial and I could make any mistake.

The Ubuntu developers slightly enhanced the patch:
"debian/patches/02-cairo-1.4.8-lcd-filter-2.dpatch:
    - Restore patch that uses the new FreeType LCD colour filtering features,
      with additional modification that the specific LCD Filter can be
      changed."
(you can get the source from http://packages.ubuntu.com/gutsy/libs/libcairo2)

I tried porting that patch on the trunk (as expected, it doesn't apply cleanly). However, glyph positioning is broken. I'll try to debug this when I have time.

I forgot to add:

I looked at the freetype rewrite patches from David Turner (http://david.freetype.org/cairo/fix-freetype-usage-4.patchset), but they do not seem to have the lcd-filtering enhancement proposed here (I saw no FT_Library_SetLcdFilter call). So these two changes should be independent (of course, the patch here might need large adaptations to by applied on top of the freetype rewrite).

I would like to see this go into 1.6 as well. It seems many packagers are already using it, so it wouldn't hurt, and it makes sub-pixel rendering much better looking.

Created an attachment (id=12955)
Incorrect glyph positioning on 1.5 branch

I've applied the patch to cairo 1.5.2 after some fuzzing, but it seems I've run into the same glyph layout issue Sylvain encountered.

I've attached a screenshot from my GNOME appearance pane. It was the only location I could find where the the labels are large enough to avoid completely clipping the text. It appears from this that the vertical positioning is significantly off, but the horizontal positioning is only wrong by a few pixels on some glyphs.

Created an attachment (id=12971)
patch for 1.5

The patch needed to be updated according to 31f5aafa36015ee6ea8ff769c2e1d5841f62642f.

This is now running quite better.

Tests will need to be updated / completed.

Maia Everett (linneris) wrote :

There is another way to achieve this: build Firefox with --enable-system-cairo, but apply a patch to the system libcairo 1.4 to export cairo_surface_show_page - the only reason why Firefox depends on 1.5, because this function was only exported in 1.5.

For a number of reasons which I hold to be obvious, using an external Cairo is preferable. Attached is a debdiff that adds the export. It is intended to be applied to version 1.4.10-1ubuntu4.1.

I built Firefox 3.0b2pre with a patched Cairo on my home machine, and it builds and runs correctly.

Maia Everett (linneris) wrote :

Just the cairo dpatch alone.

Maia Everett (linneris) wrote :

Patch for Firefox to build with system cairo.

Anders Kaseorg (andersk) wrote :

I completely support using the system cairo. Just make sure that LP bug #122735 got fixed, since that’s the reason system cairo was disabled in the first place.

Fabien Tassin (fta) wrote :

There are numerous problems with system cairo 1.4.10 compared to patched mozilla cairo 1.5.2-55.
1st, system cairo has tons of fatal asserts, making firefox crash in different locations with ugly messages. Mozilla turned asserts non-fatal as they deal with errors they think important and ignore the rest.
2nd, mozilla fixed a number of issues (performance, glyphs, bad repaints, printing issues, etc), some (most?) merged into system 1.5.*.

From a user perspective, system 1.4.10 will appear somewhat slower and will have more visual artifacts, not to mention that the associated bugs will not be supported by mozilla.
You are requesting improvements for subpixel, yet the patch decreases overall quality for everyone. That's a 6+ months regression of mozilla improvements/bug fixes.

It would be better to have that subpixel patch accepted upstream by cairo, or at least by mozilla.
Or hardy could ship a patched 1.5.* (I don't know if it's even possible).

Alexander Sack (asac) wrote :

Do I understand correctly that you want us to apply that patch to cairo 1.5? If you can come up with a patch against the cairo in xulrunner-1.9 source package, I would be happy to discuss the patch and see if its suitable for upstream inclusion .

Thanks,
 - Alexander

Behdad,

Do you prefer to wait for the freetype rewrite before dealing with this bug? As I said in comment 16, the freetype rewrite will not automatically solve this issue.

Otherwise, I can help with the test update or cleanup if you think you can make it for 1.6.

Maia Everett (linneris) wrote :

Well, since system Cairo is not the way to go, applying the subpixel patch to Firefox's bundled Cairo seems to be the only solution.

Sylvain, your ported patch requires definitions that come from Ubuntu's supplementary patch to fontconfig. You either need to get that patch into fontconfig upstream or remove the added options from the cairo patch.

As an added note, even after patching fontconfig and building from your patch, the fontconfig additions don't seem to have any effect. In fact, the original Ubuntu-modified changeset to Cairo has a significant error where they use FC_LCD_FILTER_* definitions where they should be using FT_LCD_FILTER_* to pass to FT_Library_SetLcdFilter. I checked the headers, and the definitions don't correspond. This leads me to believe that their changes never did what they were supposed to in the first place.

Created an attachment (id=12997)
Silvain's patch against 1.5, modified to use correct definitions.

Some definitions needed editing to make the filter configuration from Ubuntu work. This patch contains those modifications.

David Turner has modified FreeType to be able to render sub-pixel decimated glyphs using different methods of filtering. Fontconfig needs new configurables to support selecting these new filtering options. A patch follows that would correspond to one available for Cairo in bug 10301.

Created an attachment (id=12998)
Patch against fontconfig-2.5.0 adding options.

This patch adds a new option with 4 configuration constants. It is modified from one available at http://packages.ubuntu.com/gutsy/libs/libfontconfig1

Ubuntu developers originated this patch, albeit with the error of creating name constants that conflicted with existing ones. This patch modifies the original by prefixing the constant strings with "lcdfilter".

I've created bug 13566 with the modification patch to fontconfig that allows the cairo patch to apply.

(In reply to comment #23)
> I've created bug 13566 with the modification patch to fontconfig that allows
> the cairo patch to apply.

Tested with cairo 1.5.4 and fontconfig 2.5.0 with patches for each, works fine.

Thanks Brandon for making this Ubuntu independent. I didn't know Ubuntu's fontconfig was also patched.

By "subpixel-patched cairo", I'm guessing you mean the patch by David Turner to enable LCD-filtering (known as 02-cairo-1.4.8-lcd-filter-2.dpatch in Ubuntu cairo patches).

This patch has been ported for cairo 1.5, and is available at:

https://bugs.freedesktop.org/show_bug.cgi?id=10301

I hope that this will be included in upstream cairo. Otherwise, patching the bundled cairo version inside Xulrunner/Firefox would be the way to go.

I'm actually using this patch in my own Firefox build (no system cairo) and it works pretty well.

Maia Everett (linneris) wrote :

I can confirm that the patch on bugs.freedesktop.org works correctly with Firefox's bundled Cairo.

Here's the patch in a form suitable for inclusion in Firefox's debian/patches. The authors header has been retained.

Changed in libcairo:
status: New → Invalid
Alexander Sack (asac) on 2007-12-13
Changed in xulrunner-1.9:
importance: Undecided → Medium
Alexander Sack (asac) wrote :

thanks for the update/patch. Lets follow upstream discussion on this. The patch isn't minimal and updating/maintaining it on the unstable 1.5 branch might be quite cumbersome imho.

 - Alexander

Alexander Sack (asac) wrote :

milestoning for alpha 3 so I don't forget to follow up.

Changed in xulrunner-1.9:
milestone: none → hardy-alpha-3
status: Confirmed → Triaged
Fabien Tassin (fta) wrote :

Here are two debdiff.

fontconfig-2.5.0-2ubuntu1--2.5.0-2ubuntu2.debdiff
libcairo_1.4.10-1ubuntu6--1.5.4-0ubuntu1.debdiff

The 1st one (fontconfig) is a requirement for the 2nd.

I've tested that for a couple of days. No regression on my system.

I've used that cairo with xulrunner 1.9 and firefox 3.0 b2 rc1. It works like a charm.
Everything is available for review in my PPA.
Prism also benefited from this through xulrunner 1.9.
Same as experimental miro.

1st debdiff:

+fontconfig (2.5.0-2ubuntu2) hardy; urgency=low
+
+ * Replace Ubuntu's lcd patch by patch from Freedesktop bug #13566
+ which will allow native Cairo 1.6 lcd filtering (as per
+ Freedesktop bug #10301)
+ - drop debian/patches/03_ubuntu_lcd_filter.patch
+ - add debian/patches/03_lcd_filter_freedesktop_bug13566.patch
+ - update debian/patches/04_ubuntu_monospace_lcd_filter_conf.patch
+
+ -- Fabien Tassin <email address hidden> Fri, 14 Dec 2007 15:30:43 +0100

Fabien Tassin (fta) on 2007-12-16
Changed in libcairo:
status: Invalid → In Progress
Alexander Sack (asac) on 2007-12-18
Changed in xulrunner-1.9:
status: Triaged → Won't Fix
Alexander Sack (asac) on 2007-12-18
Changed in xulrunner-1.9:
status: Won't Fix → Triaged
Changed in fontconfig:
importance: Undecided → Wishlist
status: New → Incomplete
Changed in libcairo:
importance: Undecided → Wishlist
status: In Progress → Incomplete
Changed in fontconfig:
status: Unknown → Confirmed
Changed in libcairo:
status: Unknown → Confirmed
Changed in fontconfig:
status: Incomplete → Fix Released
Changed in libcairo:
status: Incomplete → Fix Released
Fabien Tassin (fta) on 2007-12-22
Changed in xulrunner-1.9:
status: Triaged → Fix Committed
Changed in xulrunner-1.9:
status: Fix Committed → Fix Released
Changed in fontconfig:
status: Confirmed → In Progress
Changed in fontconfig:
status: In Progress → Fix Released
Changed in libcairo:
status: Confirmed → Fix Released
51 comments hidden view all 131 comments

(In reply to comment #57)
> Hello Gentlemen,

Hi Nicolaus,

> With the recent release of 1.8.0, all of the lcd_filter checks back into
> fontconfig and Xft have been removed. For me, this is a (rather tremendous)
> regression in usability.

Yeah, sorry about that. I know that some people really wanted these new filtering options, and it had always been my intent that they be present in 1.8.0. It was just that I didn't notice until *very* late in the release cycle that I couldn't accept how this had been implemented.

> I would like this bug to be reopened so that the
> re-addition of the code can be followed.

Done. (Did the bugzilla interface not allow you to re-open it yourself? I'd generally prefer people just throw the switches they want in bugs, rather than asking for others to throw them.)

> Providing redundant (and inferior*) functionality in cairo again might once again
> jeopardize the stack.

No debates on patents, please. That's not cairo's concern.

> I would appreciate public updates on the debate around lcd_filtering ability,

It seems easy to me. Just use the freetype APIs if they are present *and* they are actually functional, (not the "you asked for subpixel, but I'll give you gray" thing). And if not, use the code that's already in cairo.

We should even be able to make a change like that for a minor release, (1.8.2 or 1.8.4).

> If you need anything from me, or would like to see some involvement from me,
> just ask.

Just need somebody to code that up.

> * I do apologize to whomever made cairo's filter in the first place, but in my
> opinion it _is_ inferior to freetype's filters

Well, it's really just a matter of what assumptions it was coded against.

> -- the current pixel-clipping
> code in cairo lcd filtering results in yellow-blue halos that are quite evident
> to me on any screen I've used.

The filters in cairo were written with the assumption of strong hinting. And with that hinting, I still haven't seen any better filters, (freetype provides a "LEGACY" filter that's supposed to be similar---it's not identical but I don't know if one or the other would be "better" in my opinion). I do agree that there are really bad color fringes if you use this filtered sub-pixel rendering without good hinting. I think "just don't do that" would be that reaction of the original filter's author. :-)

So that's not to disagree with you at all, just to explain how this code actually can function quite well.

-Carl

Hi Carl,

Sorry about asking about the bug switch. I don't like being pushy, and some people are very protective of their bug tracking systems. Glad to know you're not so overly-protective. ~ grin ~

As for the patent thing, you're probably right.

And again for cairo's built-in filtering -- I usually never use strong hinting. Slight hinting almost always produces better shapes (well for me at least -- I still find Windows users not using Cleartype, but then again this is a preference thing.) So that explains my usage case at least. Thanks for letting me know.

I'll get on coding up a piece of that switch, then.

Nicolaus

> (In reply to comment #57)

Hello Carl. I very much agree with Nicolaus. I also understood that you were not against the Turnerian FIR filters but against possible distributors’ choices affecting what Cairo can promise to render or not. Still I want to point something out regarding which filter is better for Cairo.

> The filters in cairo were written with the assumption of strong hinting. And
> with that hinting, I still haven't seen any better filters, (freetype provides
> a "LEGACY" filter that's supposed to be similar---it's not identical but I
> don't know if one or the other would be "better" in my opinion). I do agree
> that there are really bad color fringes if you use this filtered sub-pixel
> rendering without good hinting. I think "just don't do that" would be that
> reaction of the original filter's author. :-)

But strong hinting is the wrong assumption to begin with. Full hinting — or worse, byte-code superhinting — is so 1990’s!

Sure, a lot of coder/UNIX types still prefer high contrast on-screen text, but for Cairo an algorithm that is so robust that it can accomodate both full hinting and light autohinting is much a much better match.

Cairo is general purpose, so one thing it’s used for is on-screen text in UI and, e.g., webpages. This is a setting where full hinting makes some sense, because integer point sizes are used and the Cairo filter then has to deal with 1px stems only. No big deal, as only the curves and bowls and diagonals need attention.

But Cairo is also used for freely scalable canvases, for rich graphics using serif, script, or decorative typefaces. For animation. Strong hinting makes no sense here. On a general image surface nice autohinted text blends in a lot better. Autohinting /is/ »good hinting«. And the FIR filters are the best match for it. Even better, the FIR filters even give very good results for strongly hinted UI text. It’s just more versatile.

Please see the AB comparison page I made: http://www.alice-dsl.net/towolf/cairo/
(Weakness: It only presents one case, one text setting, unlike the huge comparison that Sylvain presented.)

Changed in libcairo:
status: Fix Released → Confirmed

(In reply to comment #60)
> Please see the AB comparison page I made:
> http://www.alice-dsl.net/towolf/cairo/
> (Weakness: It only presents one case, one text setting, unlike the huge
> comparison that Sylvain presented.)

I'm interested in replying to more of what you've written, but first I wanted to point out that I can't get your comparison page to work. (Nothing changes when I move the mouse on and off the images. I can right-click and do "open image" and then I will get a different image, but I can't even do that for both images for switching back and forth between two tabs.)

This is with gecko, (either Debian's iceweasel 3.0.1 or Debian's "GNOME Web Browser" 2.22.3 "powered by gecko-1.9).

Any ideas? I'm curious to see the images.

-Carl

PS. And I'm delighted to see so much commentary with the images. So often in the past I've seen people trying to advocate for these patches with things like "look at these two images! Obviously you see why I want this patch", and then I'm just lost as to which is supposed to look better or why, etc.

Created an attachment (id=19349)
First attempt to keep default cairo lcd rendering when freetype can't do it.

(From update of attachment 19349)
Carl,

I'm also interested in seeing these examples, but I too can't view the hoversrc
image -- Firefox 3.0.2 here.

I thought this shouldn't be too difficult to fix since " ... [the]
`FT_Library_SetLcdFilter' function returns the FT_Err_Unimplemented_Feature
error code."

Attached is a preliminary patch. I apologize for my newbieness with C, I've no
formal training in it. Doubtlessly I have a "paper bag issue" somewhere in my
code. Comments extremely welcome.

Nicolaus

(In reply to comment #61)
> (In reply to comment #60)
> I'm interested in replying to more of what you've written, but first I wanted
> to point out that I can't get your comparison page to work. (Nothing changes
> when I move the mouse on and off the images. I can right-click and do "open
> image" and then I will get a different image, but I can't even do that for both
> images for switching back and forth between two tabs.)

Ja, let me say ›oops‹. In a rush to get it out the door I neglected to test with Firefox. It seems I followed the wrong mouseover tutorial. Should work now.

But I’ve figured I must also add an example of large display type, so you get added value now.

> PS. And I'm delighted to see so much commentary with the images. So often in
> the past I've seen people trying to advocate for these patches with things like
> "look at these two images! Obviously you see why I want this patch", and then
> I'm just lost as to which is supposed to look better or why, etc.

There’s certainly an inherent danger in pointing to screenshots and shouting: »See? See? It’s so much better!« because that’s not necessarily true for everybody. Someone might just have gotten new, sharp spectacles, or she is myopic and sits 20cm from the screen — loupe effect!. Or someone’s screen is really low-res, rotated to portrait, or whatnot.

The point here is that configurability is a necessity due to taste differences. So with the patch we can have GRAY, LEGACY, FIR3 and FIR5. Importantly the FIR filters are the most versatile and robust. They work well across the board, LEGACY doesn’t. Thus they are the best candidates for the LCD_FILTER_DEFAULT designation.

A future font appearance configuration panel should offer more options to tweak this. But it should probably only make sensible combinations possible. That is, if native TrueType instructions are enabled then LEGACY or monochrome make sense. If slight hinting is enabled, then FIRx and GRAY make sense.

Created an attachment (id=19362)
Fallback to cairo lcd filtering if freetype cannot.

Gentlemen,

This is my second attempt, minus several "paper bag" sort of incidents.
I've compiled, tested, and am using this version now, switching between +/- lcd-filtering versions of freetype freely and having cairo lcd filter in all cases (although preferring freetype filtering when available).

Hope this helps!

Nicolaus

Hello again. I was curious, so I added a new section to my comparison page juxtaposing Vista and Cairo:
http://www.alice-dsl.net/towolf/cairo/#vista

(In reply to comment #66)
> Hello again. I was curious, so I added a new section to my comparison page
> juxtaposing Vista and Cairo:
> http://www.alice-dsl.net/towolf/cairo/#vista

Hello again,

While you're adding new images, I'd like to write up a couple quick thoughts based on your presentation so far.

First, you seem to be largely arguing for a change in the default filtering, beyond just adding code to allow for the freetype filtering to be used. That's certainly a useful discussion to have. But oddly, the images on the page don't actually show direct comparisons of the different choices we have here for a default. I'd like to see the results of cairo's current code compared to freetype's LEGACY and that compared to FIR3 and FIR3 compared to FIR5. Things like that.

You also limit the presentation by saying "This is not a test of the boring canonical high contrast scenario with bytecode hinted DejaVu Sans, 8pt text, and pixel-fitted stems.". As "boring" as that scenario might be, it's a terribly important one, so I would really like to see some examples of that added if you would. I'd like to ensure a "do no harm" situation here.

Meanwhile, another problem I had with the original patch series was that it simply exported all of the freetype filters as-is up to the user level through cairo's public API. One problem with this is that the filters are all obviously freetype-specific while cairo provides a cross-platform API. An argument for providing the API is for someone like Behdad to be able to write a font-preferences dialog that presents (unnamed) font-rendering samples to the user and lets them simply click on which one they prefer.

But for that kind of font-preferences dialog, we already have 3 different ANTIALIAS font options and 4 different HINT_STYLE options. So that's 12 different necessary samples now, and adding 4 different filters expands that to 48, which seems to go way beyond what any user could ever want to see.

Meanwhile, the descriptions on your page seem to argue for specific filters being best in specific situations. For example, there seems to be an implicit preference for FIR5, (no FIR3 examples, and a link to an Ubuntu poll favoring FIR5). So is there any reason to provide FIR3 at all? Also, you argue that LEGACY doesn't do well in anything but the strongly-hinted case, but does it (or cairo's code?) actually do best there?

I'm wondering if we shouldn't automatically choose a filter based on the HINT_STYLE option. I'm also wondering that if we do decide to expose these in the API whether we should just export two options absed on their general characteristics, (INTER_PIXEL and INTRA_PIXEL ?), rather than so much about their details implementation, (FIR3 and FIR5, etc.).

Well, this is all a little bit off-topic for the specific bug here, and it's getting into API discussion that really needs to take place on the cairo list.

Let's move the discussion there. Feel free to quote my stuff liberally or completely if you'd like to reply on the list.

Thanks,

-Carl

Carl, you really should try maintaining a distro's text stack for a couple years before you get it: There is no sensible defaults. People file bugs about their text rendering where it looks perfectly fine to me. Certainly much better than what they call the unbuggy version. Choosing filters based on hintstyle, or exposing some but not other filters is simply not cairo's business. Smple as that.

Nicolaus, what source tree is your patch based on? I can't get it to apply to either 1.8.0 or master.

(In reply to comment #69)
> Nicolaus, what source tree is your patch based on? I can't get it to apply to
> either 1.8.0 or master.
Ok, I see it applies well enough against master with commit 5d887ad5dca5af0f8216830d1b04d08a5aba9bee reverted.

Yea, it applies to a revert of the revert of the lcd_filtering functionality. There's still some stupid crap in that code, I realize, so I'm going to go over it again. But at least it works.

(In reply to comment #68)
> Carl, you really should try maintaining a distro's text stack for a couple
> years before you get it: There is no sensible defaults. People file bugs
> about their text rendering where it looks perfectly fine to me. Certainly much
> better than what they call the unbuggy version. Choosing filters based on
> hintstyle, or exposing some but not other filters is simply not cairo's
> business. Smple as that.

I don't buy it, Behdad.

Certainly, you have experience that I don't here. I don't content that. And users often do want text-rendering results that seem hard to believe---I've seen that.

But as for the details of which kind of LCD filtering---your distribution has never made that available as an option, right? So what evidence do you have that people won't be satisfied here? Just that "they're never satisfied"?

I'd really like to do something very much like "strong hinting"->"current filtering" and "slight/medium hinting"->"freetype's FIR5 filtering" and see if that satisfies people. And best would be to do that experiment by way of the new text-preferences dialog where people choose the version that they like the best. I really can't imagine many people saying "none of the above". (I think most hard-to-understand complaints come from people not being able to achieve a result that they are accustomed to---and that's precisely part of the reason that led to the revert of the freetype-filter using code during 1.7.x.)

-Carl

Sorry for bringing this bug up again, but I noticed cairo 1.8.4 is now out, and I was wondering what else needed to be done to put filtering back into the next point release (if it's still possible). What are everyone's opinions of Nicolaus's patch, and is something else required that his patch doesn't handle?

Created an attachment (id=21150)
Fallback to cairo's original LCD filtering if freetype lacks the capacity

Here's an updated patch for mainline, applies directly to HEAD.
I've compile-tested it, and a derivative of the patch on a VirtualBoxed Ubuntu Intrepid System. It works, but it's not HEAD.
If anyone finds any bugs, please let me know.
Now that classes are in remission and I'm no longer over-taxing myself, I'd really like to get this issue resolved.
There's been some discussion from Carl on what he'd like to see, but nothing really decided upon. If we could have a discussion and lay down what needs to be done to get these Freetype filtering patches upstream, that'd be a step in the right direction.

Thanks everyone!

Nicolaus

This is a gentle >>nudge<<.
Behdad or Carl, can either one or both of you suggest a plan of action?
I would like to get the ball moving on this one.
We currently have downstream distros differing on this issue greatly, the situation could easily be improved.

Nicolaus

Nicolaus, I tested your latest patch against HEAD, and it has a bug where grayscale or monocolor fonts are extra-wide. I've also tested HEAD with just reverting 5d887ad5dca5af0f8216830d1b04d08a5aba9bee and fixing the conflicts, and the bug isn't there. It looks like something in your patch is multiplying a number against the width/pitch when it shouldn't be, or not dividing it out when it should. Since both your original patch and HEAD with the revert work fine, I figure you'll know if you changed something along these lines that might be causing it.

Thanks Brandon, I'll take a look.
There's probably some thinko somewhere.

Ubuntu has included this patch, for some reason, and it causes a problem with drawing black-on-black text sometimes when VRGB subpixel smoothing is selected. The ubuntu bug is:

https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/145604

It appears there might be a byte-shift problem in this patch.

Brandon,

Sorry for taking so long to get back. I poured over my changes, and didn't notice anything out-of-ordinary, save for a single function call that was unnecessary, and probably wasn't the root of the problem. Could you tell me your use case, where you see this problem in the patch?

Lance

(In reply to comment #76)
> Nicolaus, I tested your latest patch against HEAD, and it has a bug where
> grayscale or monocolor fonts are extra-wide. I've also tested HEAD with just
> reverting 5d887ad5dca5af0f8216830d1b04d08a5aba9bee and fixing the conflicts,
> and the bug isn't there. It looks like something in your patch is multiplying a
> number against the width/pitch when it shouldn't be, or not dividing it out
> when it should. Since both your original patch and HEAD with the revert work
> fine, I figure you'll know if you changed something along these lines that
> might be causing it.
>

Bob,

Can you open up and look at the version of cairo used by ubuntu, and apply the 04_lcd_filter.dpatch?

In _fill_xrender_bitmap, look at the section near the end, where it converts vertical RGB into ARGB32. There is a weird #if 1 #else #endif block in this particular section, and I think this is where the problem is. I think the code in the #else block is the correct code, and changing the #if 1 to #if 0 in the 04_lcd_filter.dpatch might fix it. (you'll need either a copy of the libcairo2-2ubuntu1 source to compile from or you'll need to revert the patch. There's some other stipulations about compiling that you might already know, like you can't have the original gzipped or bzipped tars of the source in the parent directory, or the devscipts will use them to build from.)

If anyone else is enlightened as to the math this is trying to perform, and can tell me for sure that I'm wrong about this particular section screwing up, I will gladly listen (and learn).

Sylvain, can you look into this? I don't have a platform handy for looking up their most obvious use case in Compiz, and I can't get Firefox to do what they say on a couple handy examples.

It would be nice to have this fixed as well as getting a working version of the fallback patch finally upstream. I wish my time wasn't so spotty, but it would also be encouraging to hear from either Behdad or Carl. I know Cairo is a cross-platform tool, and these guys have to worry about other things cropping up elsewhere, but these patches really help usability for some of the linux guys. I know Behdad isn't fired up about this issue, but you, Brandon, and I at least see a point to getting these patches upstream in some acceptable form.

Nicolaus (Lance)

(In reply to comment #78)
> Ubuntu has included this patch, for some reason, and it causes a problem with
> drawing black-on-black text sometimes when VRGB subpixel smoothing is selected.
> The ubuntu bug is:
>
> https://bugs.launchpad.net/ubuntu/+source/cairo/+bug/145604
>
> It appears there might be a byte-shift problem in this patch.
>

You seem to be right, the code in "#if 1", sets the alpha channel to full opacity (0xff), so the background behind the glyph won't be visible.
The else sets the alpha to the value of the green channel (same thing as horizontal RGB).

I don't have time right now for looking at this, but could someone test with that change? That may be an easy fix.

(In reply to comment #80)
> It would be nice to have this fixed as well as getting a working version of the
> fallback patch finally upstream. I wish my time wasn't so spotty, but it would
> also be encouraging to hear from either Behdad or Carl. I know Cairo is a
> cross-platform tool, and these guys have to worry about other things cropping
> up elsewhere, but these patches really help usability for some of the linux
> guys. I know Behdad isn't fired up about this issue, but you, Brandon, and I at
> least see a point to getting these patches upstream in some acceptable form.

I still feel the some way about the API issues as I did when I wrote comment #67 above. Has anyone written a patch to address my concerns there?

Also, Behdad's latest review identified some bugs in the patch he saw as well. Have those been addressed?

-Carl

(In reply to comment #82)

I will initiate an API discussion later today on the cairo mailing list, then. I was hoping Behdad's short quip (comment #68) was enough to convince you, but a discussion is probably for the best.

Latest review? I went hunting around the mailing lists looking for said review, but could not find it. At least nothing after it was removed prior to 1.8.0. Are you talking about something before? I should hop onto the irc channel. I would relish the opportunity to dig in and learn more about these APIs.

Nicolaus

> (In reply to comment #80)
> > It would be nice to have this fixed as well as getting a working version of the
> > fallback patch finally upstream. I wish my time wasn't so spotty, but it would
> > also be encouraging to hear from either Behdad or Carl. I know Cairo is a
> > cross-platform tool, and these guys have to worry about other things cropping
> > up elsewhere, but these patches really help usability for some of the linux
> > guys. I know Behdad isn't fired up about this issue, but you, Brandon, and I at
> > least see a point to getting these patches upstream in some acceptable form.
>
>
> I still feel the some way about the API issues as I did when I wrote comment
> #67 above. Has anyone written a patch to address my concerns there?
>
> Also, Behdad's latest review identified some bugs in the patch he saw as well.
> Have those been addressed?
>
> -Carl
>

(In reply to comment #83)
> (In reply to comment #82)
> I will initiate an API discussion later today on the cairo mailing list, then.
> I was hoping Behdad's short quip (comment #68) was enough to convince you, but
> a discussion is probably for the best.

You can see my reply to that above. But I'd already asked for this discussion to happen on the list, not in this bug report. So, I think it's great you'll be taking it up there.

> Latest review? I went hunting around the mailing lists looking for said review,
> but could not find it.

I was talking about comment #76. But I misread that---it was from Brandon, not Behdad. But I still would like to know if those issued have been fixed.

> Are you talking about something before? I should hop onto the irc channel. I
> would relish the opportunity to dig in and learn more about these APIs.

I'll look forward to chatting with you there.

-Carl

Created an attachment (id=22299)
Screenshot showing error from latest patch

Nicolaus, here's a screenshot showing the problem I'm getting. When grayscale or no smoothing is used, all the font metrics seem to be scaled horizontally. Also, it seems that even when disabling antialiasing, it still ends up using grayscale smoothing.

Created an attachment (id=22302)
Revised version of "Fallback to cairo's original LCD filtering if freetype lacks the capacity"

Ok, I've discovered the problem for my issues. Referring to a patched version of master, On line 1431 of src/cairo-ft-font.c it tests against the variable ft_can_filter. However, this variable is only initialized on line 1367 if it's performing subpixel antialiasing, otherwise it's always false. It ends up using the internal glyph rasterizer, while it has everything set up for the external one. Moving the line 1431 testing block to the top level fixes the problem, with the addition of an extra line to put FT_SetLcdFilter back to normal.

Attached is a version of the patch with said changes made. While I was modifying it, I noticed that there are some tab/space formatting issues with the patch that might need to be cleared up.

Created an attachment (id=22336)
Revised Revised version of the Freetype Filtering with Fallback patch

Hi Brandon,

Thanks for catching that! I clearly had a thinko in there. But I don't think that bug was causing the behavior you saw. I'm also curious how your changes fixed it for you. I recreated the effect you saw, and your patch didn't fix it for me. Although it clearly fixes a bug that would have cropped up otherwise.

I did a more thorough review, and discovered a code block that had been lost. I've stuck it back in, and it fixes the problem for me. I've also tried to clean up as many of the spacing issues as I could see.

Nicolaus

(In reply to comment #87)
> Created an attachment (id=22336) [details]
> Revised Revised version of the Freetype Filtering with Fallback patch
>
> Hi Brandon,
>
> Thanks for catching that! I clearly had a thinko in there. But I don't think
> that bug was causing the behavior you saw. I'm also curious how your changes
> fixed it for you. I recreated the effect you saw, and your patch didn't fix it
> for me. Although it clearly fixes a bug that would have cropped up otherwise.
>
> I did a more thorough review, and discovered a code block that had been lost.
> I've stuck it back in, and it fixes the problem for me. I've also tried to
> clean up as many of the spacing issues as I could see.
>
> Nicolaus

It was hitting the correct code path everywhere but in that function, and the change I made ensured the FT_SetLcdFilter path was used properly there as well. Your missing code block caused another separate mixed code path error, and you had the right config to trigger it.

I tested your latest patch, and it works correctly.

I've applied the internal interface to pass the LCD filter option from fontconfig/screen resources to freetype. The interface is not ready to be made public yet, and I am uncertain about the removal of all fallback filtering for Cairo.

commit 7a023a62f7517ad0d54f4d59c99909fadcc05e82
Author: Nicolaus L Helper <email address hidden>
Date: Thu Jun 17 08:56:30 2010 +0100

    ft,fc,xlib: LCD filtering patch.

    This adds internal API to retrieve the LCD filtering parameters from
    fontconfig, or as set on the Screen, and feed them to FreeType when
    rendering the glyph.

    References:
      Bug 10301 - LCD filtering patch
      https://bugs.freedesktop.org/show_bug.cgi?id=10301

    Tested-by: Brandon Wright <email address hidden>
    Forward-ported-by: Robert Hooker <email address hidden>

    ickle: The API is clearly not ready for public consumption, the enum are
    poorly named, however this stands by itself as enabling system wide
    properties.

*** Bug 27721 has been marked as a duplicate of this bug. ***

I think the patch you applied should really be credited to whomever did the original patch (David Turner and Sylvain Pasche?), as all the fallback code I jimmied with is gone (also, it was mostly just playing with others' code).

Also, in _fill_xrender_bitmap in the FT_PIXEL_MODE_LCD_V part of the switch, there's an #if 1 ... 0xFF000000 ; #else ... #endif which makes it fully opaque. I would remove everything in the #if 1 ... #else section, and keep what's in the #else ... #endif.

N

> I've applied the internal interface to pass the LCD filter option from
> fontconfig/screen resources to freetype. The interface is not ready to be made
> public yet, and I am uncertain about the removal of all fallback filtering for
> Cairo.
>
> commit 7a023a62f7517ad0d54f4d59c99909fadcc05e82
> Author: Nicolaus L Helper <email address hidden>
> Date: Thu Jun 17 08:56:30 2010 +0100
>
> ft,fc,xlib: LCD filtering patch.
>
> This adds internal API to retrieve the LCD filtering parameters from
> fontconfig, or as set on the Screen, and feed them to FreeType when
> rendering the glyph.
>
> References:
> Bug 10301 - LCD filtering patch
> https://bugs.freedesktop.org/show_bug.cgi?id=10301
>
> Tested-by: Brandon Wright <email address hidden>
> Forward-ported-by: Robert Hooker <email address hidden>
>
> ickle: The API is clearly not ready for public consumption, the enum are
> poorly named, however this stands by itself as enabling system wide
> properties.

Created an attachment (id=36349)
fix alpha mapping in vertical RGB to ARGB32 conversion

Fixes the alpha mapping for vertical RGB in _fill_xrender_bitmap in cairo-ft-font.c

commit a150371a5d10e03d6c0d781c6fac950a9ac6be18
Author: Nicolaus L Hepler <email address hidden>
Date: Tue Aug 10 09:34:39 2010 +0200

    ft-font: Make alpha mapping consistent

    Vertical RGB mapping previously forced opaque pixels.
    To be consistent with horizontal RGB/BGR and vertical BGR it
    should use an alpha equal to the mid channel (green).

Changed in libcairo:
importance: Unknown → Medium
Changed in fontconfig:
importance: Unknown → Wishlist

Created attachment 40431
Allow changing of hintstyles by FC

http://weirdfellow.wordpress.com/2010/08/01/patching-cairo/

Changed in libcairo:
importance: Medium → Unknown
Changed in fontconfig:
importance: Wishlist → Unknown
Changed in libcairo:
importance: Unknown → Medium
Changed in fontconfig:
importance: Unknown → Wishlist

Like some others, I also object to keeping the "legacy" filter as the default. Freetype doesn't call it legacy without reason: it is optimized for one special rendering preference, full native hinting (IMO it's doing badly even in that case), and is crap in all others. Moreover, if the fonts do not have high-quality hinting instructions, it does a bad job as well. I think this isn't even subjective, the color fringing of the "legacy" filter is bad regardless of display and/or personal preferences.

If you still think it matters to not "surprise" users (I certainly would call improved text rendering a good surprise, though), the FIR3 filter might be a good choice for the default, but I'd surely go for the default (FIR5) by default. This is how it should be anyway.

I am trying to enable subpixel rendering on poppler and I found my rendering result is suboptimal. After gdbing into cairo, I am surprise that cairo is still using the legacy filter as the "default". To the the situation even worse, cairo removed the interfaces to set the lcdfiler (cairo_font_options_set_lcd_filter) because "they are too specific to FreeType".

Well, maybe that is true. But now how can I use the real Freetype default filter (FIR5) in the newest cairo without pathcing it? I think maybe manually set a FcPattern is OK but I really do not want to learn/invoke/rely Fontconfig just for setting a filter that should be default.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/cairo/cairo/issues/310.

Changed in libcairo:
status: Confirmed → Unknown
Displaying first 40 and last 40 comments. View all 131 comments or add a comment.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.