Bug 3307

this hasn't happened yet as far as I can tell

Here is a proof-of-concept kludge.  It’s a ~10 line patch that forces subpixel rendering in the cairo backend:
  <http://anders.kaseorg.com/pub/patches/poppler-subpixel.patch>

The major issue is that I disabled the clipping region.  My debugging revealed that when the dimensions of the clipping rectangle translate to non-integral numbers of pixels, cairo fails to translate this rectangle into a pixman region in _cairo_traps_extract_region, and instead uses some fallback code that doesn’t support subpixel rendering.  BTW, I wonder if this fallback code is also a performance problem...

I'm not familiar enough with Cairo to say what should be done about this, but maybe this will give someone else ideas.

Created attachment 8715 [details]
patched example 1

(In reply to comment #2)
> Here is a proof-of-concept kludge.

I gave it a shot and it works quite well (at least superficially).
I cannot add much for the implementation but I can try to document the change.

I’ll attach some examples with mixed copy in pairs; with (SR) and without the patch (noSR).

Switching back and forth helps seeing what differences there are.
For me small type has better contrast with subpixel rendering, but quite obvious kerning problems are apparent with SR. Those also appear elsewhere (e.g., GTK windows)
Ignore the graphics in the Nature example, they are not antilaliased because Ubuntu builds poppler with splash only.

TBH I expected a more salient effect, one that was immediately apparent patching Xft for browsing for instance. Perhaps this is due to the fact that in PDFs the point sizes vary so much.

Created attachment 8716 [details]
unpatched example 1

Created attachment 8717 [details]
patched example 2

Created attachment 8718 [details]
unpatched example 2

Created attachment 8719 [details]
patched example 3

Created attachment 8720 [details]
unpatched example 3

Created attachment 8727 [details]
acroread 7 example 3

Now comparing the rendering with Adobe’s so-called "CoolType" the similarity is striking. The letters are extremely similar. Reader has different kerning problems and worse yellow collision artifacts, while we have some redness problems in vertical stems (on my screen of course).

I surely prefer patched Poppler over Acroread.

You’re basically comparing Splash with Cairo, and so the fonts are likely to be rendered in very different ways (like different hinting methods).  If you compare Cairo with subpixel Cairo, you should find that the letter spacing is identical; all that will change is that the letters will be sharper on LCD screens.  (It is theoretically possible to take advantage of subpixel rendering to position letters more accurately in 1/3-pixel increments, but I don’t think current Cairo does that yet.)

Any color artifacts you see are Cairo’s fault.  I recommend applying this patch, which makes Cairo use Freetype 2.3’s (much better) subpixel filtering: <http://www.freetype.org/freetype2/patches/cairo-1.2.4-lcd-filter-1.patch>  Anyway, any improvements in these areas are Cairo’s job and not Poppler’s.

On the Poppler side, all we need to do is figure out the clipping problem I mentioned above, and make Poppler respect the desktop-wide font rendering settings.

(In reply to comment #10)
> You’re basically comparing Splash with Cairo, and so the fonts are likely to
> be rendered in very different ways (like different hinting methods).  If you
> compare Cairo with subpixel Cairo, you should find that the letter spacing is
> identical; all that will change is that the letters will be sharper on LCD
> screens.  (It is theoretically possible to take advantage of subpixel rendering
> to position letters more accurately in 1/3-pixel increments, but I don’t
> think current Cairo does that yet.)

I was aware that the comparison is flawed but I was also a bit lazy. Still the images document a quite significant improvement. And yes subpixel kerning is my next big wish. I’m afraid it will not be easy though. Conceptually the vert. stems that you can more or less still fit to the grid now will then have to be shifted, with a risk of more artifacting (e.g., consider « ill » with SR kerning).

> Any color artifacts you see are Cairo’s fault.  I recommend applying this
> patch, which makes Cairo use Freetype 2.3’s (much better) subpixel filtering:
> <http://www.freetype.org/freetype2/patches/cairo-1.2.4-lcd-filter-1.patch> 
> Anyway, any improvements in these areas are Cairo’s job and not Poppler’s.

The color artifacts are not that bad actually, they are worse for Adobe. I actually already have the Freetype 2.3.1 flavor and Poppler was a missing piece of the puzzle, that you thankfully created.

> On the Poppler side, all we need to do is figure out the clipping problem I
> mentioned above, and make Poppler respect the desktop-wide font rendering
> settings.

Hope you can work it out. 

Hmm, somehow re-patching a new Ubuntu package (0.5.9-9ubuntu1 / gutsy) doesn’t give subpixel rendering anymore, even though the patching should’ve worked.

This patch was pretty awesome, couldn’t we get someone to review it?

From the cairo point-of-view, a patch to poppler like this should
not be needed and should not be applied. If poppler is rendering
to a cairo-xlib surface and the user has requested sub-pixel rendered
fonts, (by configuring font preferences in GNOME or KDE dialogs or
whatever), then cairo should be doing subpixel rendering.

If it's not, then masking that bug by forcing the rendering on, (and
hard-coding a particular sub-pixel order), doesn't help anything and
will hurt any users that don't want this.

Please find the real problem, and we'll definitely want to fix it,
but please don't apply this patch.

-Carl

PS. Reading back over the original description of the patch, it sounds
like Anders is describing a real problem, (a non-integer clipping
rectangle being passed to cairo). For cairo, I'm still curious as to
why that should disable sub-pixel rendering, (that sounds like a bug).
But meanwhile, the right thing to do, and likely the easiest, is to
simply fix poppler to correctly round the crop box values to integers
before passing them to cairo. And yes, this should also improve
performance.

Looking at the poppler core and glib parts, it seems that the surface used is not a cairo-xlib, but a cairo-image surface. To me that sounds like a reasonable explanation for cairo not using the xlib options! 

Maybe a solution would be to create temporarily a small xlib surface, copy its font options using cairo_surface_get_font_options() and apply them using cairo_set_font_options()?

Ok, I've looked at the bug a bit deeper. To me it seems there are two different problems:
* poppler_page_render()
This actually works a bit: if I pass a surface which has font options set to subpixel antialiasing, some fonts are correctly antialiased. For instance, I've modified poppler-glib-demo, and either forcing the font options of an cairo_image surface or asking to render directly on the xlib surface leads to some font being antialiased with subpixel. (I'll come back to this later on.)

* poppler_page_render_selection_to_pixbuf()
This is the place which comment #2 "fixed". A cairo image surface has default font options set to simple antialiasing. That's perfectly fine, after all no one knows what is going to be done with this pixbuf. The problem is that some applications uses this function and later display the pixbuf on the screen. So I see three possible solutions, depending on what is the purpose of the function (I don't know enough poppler to decide):
 - it's supposed to be a generic function, for rendering anywhere approximately correctly and easily used: then don't modify the function, update the API description to let know the developers that the antialiasing of the fonts is fixed to grey levels.
 - it's supposed to be a generic function, but can be complexified: add a second function which has another parameter for font_options. Therefore the developers can decide which antialiasing to use (for instance by copying the  values of X).
 - it's supposed to be specific to glib on a screen: Add inside the function a read to the default values in glib and setup the font options accordingly.


Problem with some fonts:
I've noted that in the PDFs some fonts, do get correctly displayed following the subpixel antialiasing, while some don't. At first look, it seems that type1 embedded are correctly displayed, but TrueType embedded are not (always grey). This is a major problem.

Also, I've noticed that in poppler/CairoFontEngine.cc, the load of fonts is forced with FT_LOAD_NO_HINTING, I wonder if it's a good idea. There is no comment explaining why this is so...

> Also, I've noticed that in poppler/CairoFontEngine.cc, the load of fonts is
> forced with FT_LOAD_NO_HINTING, I wonder if it's a good idea. There is no
> comment explaining why this is so...

I would guess this is because it is important for a PDF viewer to give an accurate reproduction of a printed page, and the purpose of hinting is to sacrifice accurate metrics and color for increased contrast.

Since the legacy LCD filter (FC_LCD_LEGACY / FT_LCD_FILTER_LEGACY / CAIRO_LCD_FILTER_DEFAULT(!)) adds bad color fringing to unhinted text, it might be a good idea to also ensure that the LCD filter is FC_LCD_DEFAULT or FC_LCD_LIGHT.

Created attachment 19607 [details]
Comparing HINT_NONE to HINT_SLIGHT

> I would guess this is because it is important for a PDF viewer to give an
> accurate reproduction of a printed page, and the purpose of hinting is to
> sacrifice accurate metrics and color for increased contrast.
> 
> Since the legacy LCD filter (FC_LCD_LEGACY / FT_LCD_FILTER_LEGACY /
> CAIRO_LCD_FILTER_DEFAULT(!)) adds bad color fringing to unhinted text, it might
> be a good idea to also ensure that the LCD filter is FC_LCD_DEFAULT or
> FC_LCD_LIGHT.
> 

This is interesting. I didn’t realize that Poppler disables hinting. It’s true that full hinting would alter the positioning due to the pixel snapping, but fortunately HINT_SLIGHT does not, because it only hints in the vertical axis to reduce smear on horizontal strokes.

Combined with the so called DEFAULT LCD filter HINT_SLIGHT gives really nice results -- without affecting the positioning. See the attachment.

Ooohhh, I really wish all my PDFs could be displayed with HINT_SLIGHT & FIR5! Just out of curiosity, what did you modify to force it?

BTW, I think that for precise positioning of small glyphs, it could be possible to do sub-pixel positioning, as defined here: http://www.freetype.org/freetype2/docs/glyphs/glyphs-5.html . For the small glyphs, instead of generating one glyph per character, it would generate 4 glyphs (or whatever number) corresponding to the possible displacement compared to the base pixel. Then depending on the exact position of the character, the best fitting of the 4 glyphs would be used.

Created attachment 19620 [details]
Layered Gimp file comparing small text

(In reply to comment #18)
> Ooohhh, I really wish all my PDFs could be displayed with HINT_SLIGHT & FIR5!
> Just out of curiosity, what did you modify to force it?

Well, I simply removed  lines with FT_LOAD_NO_HINTING and then changed CAIRO_HINT_STYLE_NONE to CAIRO_HINT_STYLE_SLIGHT. I realized last night that sometimes vertical hinting leads to roundish letters drooping slightly below the baseline at certain zoom levels. This is the only drawback I found.

> BTW, I think that for precise positioning of small glyphs, it could be possible
> to do sub-pixel positioning, as defined here:
> http://www.freetype.org/freetype2/docs/glyphs/glyphs-5.html . For the small
> glyphs, instead of generating one glyph per character, it would generate 4
> glyphs (or whatever number) corresponding to the possible displacement compared
> to the base pixel. Then depending on the exact position of the character, the
> best fitting of the 4 glyphs would be used.

Yes, this is one thing that should be looked at some time in the future. Actually David Turner implemented an algorithm that adjusts kerning by caching side-bearing deltas before and after hinting, .i.e., no actual subpixel positioning. But the results are already striking. The ftdiff tool is a good demo for this.
If you want to see how it looks when positioning is done well, download the attachment, open it in Gimp, and compare the layers. The top layer is so much more regular and readable than what Poppler and Cairo produce.

Presently, and as noted in comment #15, poppler-glib appears to use font options from the surface it was given by the caller.  It is rather convenient, as it allows to implement management of subpixel rendering from the application with minimal effort.  As an example, here's a patch for evince pdf backend that hard-codes horizontal rgb with slight hinting:

diff -pur evince-2.24.2-r1/work/evince-2.24.2/backend/pdf/ev-poppler.cc evince-2.24.2-r2/work/evince-2.24.2/backend/pdf/ev-poppler.cc
--- evince-2.24.2-r1/work/evince-2.24.2/backend/pdf/ev-poppler.cc       2008-11-24 22:09:30.000000000 +0300
+++ evince-2.24.2-r2/work/evince-2.24.2/backend/pdf/ev-poppler.cc       2009-01-19 01:15:15.048150478 +0300
@@ -451,14 +451,22 @@ pdf_page_render (PopplerPage     *page,

 #ifdef HAVE_POPPLER_PAGE_RENDER
        cairo_t *cr;
+       cairo_font_options_t *options;

        surface = cairo_image_surface_create (CAIRO_FORMAT_RGB24,
                                              width, height);
        memset (cairo_image_surface_get_data (surface), 0xff,
                cairo_image_surface_get_height (surface) *
                cairo_image_surface_get_stride (surface));
+       options = cairo_font_options_create ();
+       cairo_surface_get_font_options (surface, options);
+       cairo_font_options_set_antialias (options, CAIRO_ANTIALIAS_SUBPIXEL);
+       cairo_font_options_set_subpixel_order (options, CAIRO_SUBPIXEL_ORDER_RGB);
+       cairo_font_options_set_hint_style (options, CAIRO_HINT_STYLE_SLIGHT);

        cr = cairo_create (surface);
+       cairo_set_font_options (cr, options);
+       cairo_font_options_destroy (options);
        switch (rc->rotation) {
                case 90:
                        cairo_translate (cr, width, 0);

So I think poppler is already doing its job with regards to subpixel rendering.

However, I'd like to second the point from comment #17: I believe it is better to use slight hinting instead of disabling hinting altogether. What about something along these lines:

diff -pur poppler-0.10.0/poppler/CairoFontEngine.cc poppler-0.10.0-r1/poppler/CairoFontEngine.cc
--- poppler-0.10.0/poppler/CairoFontEngine.cc   2008-09-22 22:25:45.000000000 +0400
+++ poppler-0.10.0-r1/poppler/CairoFontEngine.cc        2008-10-31 14:10:27.000000000 +0300
@@ -80,7 +80,7 @@ _ft_new_face_uncached (FT_Library lib,
     return gFalse;

   font_face = cairo_ft_font_face_create_for_ft_face (face,
-                                                         FT_LOAD_NO_HINTING |
+                                                         FT_LOAD_TARGET_LIGHT |
                                                          FT_LOAD_NO_BITMAP);
   if (cairo_font_face_set_user_data (font_face,
                                     &_ft_cairo_key,
@@ -214,7 +214,7 @@ _ft_new_face (FT_Library lib,
   _ft_open_faces = l;

   l->font_face = cairo_ft_font_face_create_for_ft_face (tmpl.face,
-                                                         FT_LOAD_NO_HINTING |
+                                                         FT_LOAD_TARGET_LIGHT |
                                                          FT_LOAD_NO_BITMAP);
   if (cairo_font_face_set_user_data (l->font_face,
                                     &_ft_cairo_key,
@@ -481,7 +481,7 @@ CairoFont::getSubstitutionCorrection(Gfx
        cairo_matrix_t m;
        cairo_matrix_init_identity(&m);
        cairo_font_options_t *options = cairo_font_options_create();
-       cairo_font_options_set_hint_style(options, CAIRO_HINT_STYLE_NONE);
+       cairo_font_options_set_hint_style(options, CAIRO_HINT_STYLE_SLIGHT);
        cairo_font_options_set_hint_metrics(options, CAIRO_HINT_METRICS_OFF);
        cairo_scaled_font_t *scaled_font = cairo_scaled_font_create(cairo_font_face, &m, &m, options);

Created attachment 22153 [details]
comparison of Alexander’s patch vs. one that sets CAIRO_LCD_FILTER_FIR3 explicitly

Alexander, your patch is interesting. Which is the Cairo version you are using? If it’s greater than 1.7.4, then I think the Cairo surface will use the internal intra-pixel method for LCD filtering. This is clearly worse than what I have shown above because one cannot really mate the intra-pixel method with light hinting due to excessive color bleeding. See the image

> Alexander, your patch is interesting. Which is the Cairo version you are using?
> If it’s greater than 1.7.4, then I think the Cairo surface will use the
> internal intra-pixel method for LCD filtering. This is clearly worse than what
> I have shown above because one cannot really mate the intra-pixel method with
> light hinting due to excessive color bleeding. See the image
> 

Right, I should have mentioned I'm assuming patched cairo. Right now I'm using 1.8.4 with, IIUC, ubuntu's lcd filtering patches.  On the content I read there's no color bleeding.

I'm also seeing the problem mentioned in comment #15: some embedded truetype fonts are not subpixel-antialiased.

Is there any chance that somebody tackles that one day? Subpixel smoothing is now used everywhere but when viewing PDF and PS files, which is when you read the most text. Thanks! ;-)

Created attachment 36194 [details] [review]
Set ft load flags depending on current font settings

This patch sets the ft load flags depending on current font settings. You can try with poppler-glib-demo.

Enabling sub-pixel hinted text turns out to be far tricker than one naively expects. This is due to the number of intermediate layers through which PDF is rendered, all supporting just a single alpha channel. At the moment, the only way I can think of supporting sub-pixel hinted geometry (why just limit it to text ;-) is through the use of auxiliary alpha maps, i.e. we need to preserve per-component alpha through the entire render pipeline in order to composite correctly onto the destination framebuffer. Tricky, but not impossible.

I've noticed that in Poppler 0.14, there are a lot more fonts that are not subpixel-rendered with this patch.  In the 0.12 series, pdfTeX documents for instance were subpixel-rendered fine, but not in 0.14.  Is this something that could be addressed, or is there currently no way?

By the way, this is the exact patch I'm using to force subpixel-rendering:

http://aur.archlinux.org/packages/poppler-qt-lcd/poppler-qt-lcd/0003-Forcing-subpixel-rendering-in-Cairo-backend.patch

Created attachment 37991 [details]
poppler-python demo left, popper-glib demo right

Chris, if what you’re saying is that there is a fundamental obstacle, why is it that python-poppler render with proper LCD filtering by default?

This is evident running its test script demo-poppler.py. Compiled against latest Poppler.

(In reply to comment #27)
> Created an attachment (id=37991) [details]
> poppler-python demo left, popper-glib demo right
> 
> Chris, if what you’re saying is that there is a fundamental obstacle, why is it
> that python-poppler render with proper LCD filtering by default?

because python poppler demo fills in white before rendering, while we do it after rendering using DEST_OVER operator. We need to do that for blend modes to work.

> This is evident running its test script demo-poppler.py. Compiled against
> latest Poppler.

Sadly, it would appear that this patch no longer works with Cairo 1.10.0:

/home/dannpg/Projects/pkgbuilds/poppler-qt-lcd/src/poppler-0.14.0/poppler/CairoOutputDev.cc:191:51: error: ‘CAIRO_LCD_FILTER_FIR5’ was not declared in this scope
/home/dannpg/Projects/pkgbuilds/poppler-qt-lcd/src/poppler-0.14.0/poppler/CairoOutputDev.cc:191:72: error: ‘cairo_font_options_set_lcd_filter’ was not declared in this scope

Any suggestions?

Hello guys!

(In reply to comment #29)
> Sadly, it would appear that this patch no longer works with Cairo 1.10.0:
> Any suggestions?

Have any plans for fixing this bug?

Created attachment 84143 [details] [review]
poppler-0.22.5 Set ft load flags depending on current font settings

The old patch doesn't work anymore with actual versions of Poppler. The updated patch was created for Poppler-0.22.5. Haven't been tested with Poppler-0.24.

-- 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/poppler/poppler/issues/61.