[cairo] Problem with text rendering on Win32+ClearType, and possible fix

[Owen Taylor]

On Wed, 2006-10-04 at 03:28 +0300, Tor Lillqvist wrote:
> I have for a long time been noticing a problem you can see in the
> screenshots http://www.saunalahti.fi/tlillqvi/ctbug.png and
> http://www.saunalahti.fi/tlillqvi/ctbug2.png (note how the "1" digits
> that occur as the final character of a string have been partially cut
> off). This problem happens only when ClearType is active, and it
> started happening between cairo 1.0.2 and 1.0.4 (yes, that long
> ago...).  Today I finally did some searching for the root cause.
> 
> At first, the problem seemed stem from the fact that starting with
> 1.0.4, cairo_win32_surface_create() sets the surface format to
> CAIRO_FORMAT_ARGB32 if GetDeviceCaps(hdc,BITSPIXEL) returns 32. (In
> 1.0.2, the format was always set to CAIRO_FORMAT_RGB24.) If the
> surface format is ARGB32, _cairo_win32_scaled_font_show_glyphs() uses
> its software fallback branch, and I thought, maybe that code is buggy.
> 
> If I changed cairo_win32_surface_create() to use RGB24 also when
> BITSPIXEL is 32, the problem went away, as
> _cairo_win32_scaled_font_show_glyphs() then uses the "direct"
> branch. But this seemed like using a awfully large hammer to fix the
> problem.

But it may be the right hammer none-the-less. If the second branch
of _cairo_win32_scaled_font_show_glyphs is always getting called,
well, that's basically a disaster:

 - The performance is much, much worse
 - The quality is much, much worse (it's not very obvious for 
   black-text on white-background, but if you try white-text
   on a black-background, the result is horrid)

While the documentation of BITSPIXEL "Number of adjacent color
bits for each pixel" makes it sound like like the X concept of
"depth" - number of *used* bits per pixel, if you are seeing a value of
32 in normal cases, then it's pretty clearly the X concept of
"bits per pixel" - the number of *total* bits per pixel.

I'm not really sure how to extract the diff and commit message
for the 1.0.2 => 1.0.4 change.

The only reason I could see for the change is if someone had a 
ARGB surface stored in a win32 DIB/DDB and wanted to use it as
a *source* surface. But that can only properly be handled with
a separate constructor. AFAIK Win32 does not know how to use a ARGB
surface as a drawing *destination* so marking all Win32 surfaces
created from a HDC as ARGB32 means one of two things:

 1) We are never using the GDI-accelerated fast paths
 2) We are using the GDI-accelerated fast paths when we shouldn't,
    and the result wouldn't be correct if the surface was
    truly ARGB32

[ Not sure what is up with the || dst->format == CAIRO_FORMAT_ARGB32
in _cairo_win32_surface_composite's ... does AlphaBlend really 
handle that ARGB destinations correctly? Or was that just hacked
in when someone noticed that cairo was never using AlphaBlend? ]

It's just a bad idea all around to treat all xRGB surfaces as ARGB
surfaces.

> I then tried if I could fix the problem by some clever hacking on the
> code in the software fallback branch in
> _cairo_win32_scaled_font_show_glyphs(). What worked was to use width+1
> and height+1 instead of width and height there in all places. But that
> didn't really seem proper either. Anyway, clearly the problem had
> something to do with the metrics of glyph strings to be rendered.
> 
> I then looked for a way to make cairo use slightly wider and higher
> metrics for a string to be rendered. It turns out that adding one to
> the GLYPHMETRICS::gmBlackBoxX and GLYPHMETRICS::gmBlackBoxY that are
> filled in by the GetGlyphOutlineW() calls in
> _cairo_win32_scaled_font_init_glyph_metrics() had the desired effect.
> 
> Could this be the proper fix? Is there a fencepost error in the
> interpretation of the GLYPHMETRICS::gmBlackBoxX and gmBlackBoxY
> values?

This isn't *the* proper fix, but it could possibly be *a* proper
fix. I'm skeptical, however... the docs for GLPYHMETRICS are
pretty unambiguous. I would rather suspect a rounding
problem elsewhere that is causing a pixel to be dropped.

> I.e., my potential patch is:
> 
> --- 1.2.4/src/cairo-win32-font.c	Fri Aug 18 17:20:16 2006
> +++ 1.2.4-tml/src/cairo-win32-font.c	Wed Oct  4 02:42:27 2006
> @@ -773,15 +773,15 @@ _cairo_win32_scaled_font_init_glyph_metr
>  	if (scaled_font->swap_axes) {
>  	    extents.x_bearing = - metrics.gmptGlyphOrigin.y / scaled_font->y_scale;
>  	    extents.y_bearing = metrics.gmptGlyphOrigin.x / scaled_font->x_scale;
> -	    extents.width = metrics.gmBlackBoxY / scaled_font->y_scale;
> -	    extents.height = metrics.gmBlackBoxX / scaled_font->x_scale;
> +	    extents.width = (metrics.gmBlackBoxY+1) / scaled_font->y_scale;
> +	    extents.height = (metrics.gmBlackBoxX+1) / scaled_font->x_scale;
>  	    extents.x_advance = metrics.gmCellIncY / scaled_font->x_scale;
>  	    extents.y_advance = metrics.gmCellIncX / scaled_font->y_scale;
>  	} else {
>  	    extents.x_bearing = metrics.gmptGlyphOrigin.x / scaled_font->x_scale;
>  	    extents.y_bearing = - metrics.gmptGlyphOrigin.y / scaled_font->y_scale;
> -	    extents.width = metrics.gmBlackBoxX / scaled_font->x_scale;
> -	    extents.height = metrics.gmBlackBoxY / scaled_font->y_scale;
> +	    extents.width = (metrics.gmBlackBoxX+1) / scaled_font->x_scale;
> +	    extents.height = (metrics.gmBlackBoxY+1) / scaled_font->y_scale;
>  	    extents.x_advance = metrics.gmCellIncX / scaled_font->x_scale;
>  	    extents.y_advance = metrics.gmCellIncY / scaled_font->y_scale;
>  	}
> @@ -811,8 +811,8 @@ _cairo_win32_scaled_font_init_glyph_metr
>  
>  	extents.x_bearing = (double)metrics.gmptGlyphOrigin.x / scaled_font->em_square;
>  	extents.y_bearing = - (double)metrics.gmptGlyphOrigin.y / scaled_font->em_square;
> -	extents.width = (double)metrics.gmBlackBoxX / scaled_font->em_square;
> -	extents.height = (double)metrics.gmBlackBoxY / scaled_font->em_square;
> +	extents.width = (double)(metrics.gmBlackBoxX+1) / scaled_font->em_square;
> +	extents.height = (double)(metrics.gmBlackBoxY+1) / scaled_font->em_square;
>  	extents.x_advance = (double)metrics.gmCellIncX / scaled_font->em_square;
>  	extents.y_advance = (double)metrics.gmCellIncY / scaled_font->em_square;
>      }