Incorrect font metrics for bitmap font, results in excess vertical spacing

[ Bug originally reported to Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=524062 ]

When using Konsole with bitmapped fonts, particularly VGA fonts such as UNI-VGA [2] or the "console8x16" font distributed with KDE 3.5, I expect the font height to be reported as exactly 16 pixels so that certain (e.g., box drawing) glyphs are column contiguous. This is the behavior of KDE 3.5's Konsole (using Qt 3.3.8b) as well as xterm. [3] is an example of the proper font rendering.

Qt 4.4.3 reports the height of these fonts (via the QFontMetrics::height method) as 18 pixels. This results in rows with two pixels of excess vertical space, creating gaps in the column-contiguous glyphs, illustrated in [4]. Qt 4.5.1 reports the height of these fonts as 17 pixels, creating a similar but smaller one-pixel gap.

The Debian bug report contains a detailed explanation of the bug and my investigation, including patches against Qt 4.4.3 and 4.5.1 that appear to correct the bug. Here's the summary:

In Qt versions 3.3.8b, 4.4.3, and 4.5.1, the QFontMetrics::height method calculates font height using the formula "ascent + descent + 1" where the ascent and descent values are determined by the underlying font engines, and the "+1" is to account for the base line.

The Xft font engine in Qt 3.3 determines the font descent (QFontEngineXft::descent) as "qRound((font->descent-1)*_scale)" with "_font->descent" coming from the font itself. I'm not exactly sure what's intended by the "-1", but I believe that (at least for bitmapped fonts) the font's actual height is the sum of the font's ascent and descent values _including the base line. Qt defines descent as not including the base line, so the descent method must explicitly subtract it.

The combined effect of the QFontMetrics::height and QFontEngineXft::descent methods is to create a ceiling round function, where for unscaled bitmap fonts, QFontMetrics::height returns a value to identical to "ascent+descent", and for scaled (outline) fonts, it returns "ceil((ascent+descent)*scale)".

In Qt 4.4, QFontEngineFT::descent method calculates the descent as "QFixed::fromFixed(-metrics.descender + (1<<6))" where the "+ (1<<6)" factor is (I believe) intended to emulate the behavior of Qt 3.3 by subtracting the base line. Except there's a sign error here, and instead it actually doubly adds the base line, resulting in bitmapped fonts that are reported with descents and heights that are two pixels too large.

It appears that the incorrect "+ (1<<6)" factor was previously discovered, and an attempt was made to

correct it in Qt snapshot 20080903 by removing it entirely. Hence in Qt 4.5, bitmapped fonts are reported with descents and heights that are one pixel too large.

Attached is a patch against Qt 4.5.1 that corrects the descent calculation. It's the same at Qt 4.4.3's QFontEngineFT::descent method but with the sign error fixed. With this patch, Konsole renders fonts with the correct row spacing as seen in [3].

I should note that this patch does introduce a side effect in font rendering for (all?) other Qt applications. Although the patch does not alter font spacing, it does appear to alter font position. In particular, the patched Qt renders glyphs one pixel lower in their bounding box.

Here are examples of this side effect in Konqueror before [5] and after [6] applying the patch. The only difference is that every glyph is shifted down one pixel. As it turns out, I'm somewhat convinced that this side effect is actually the desired behavior. Notice that the locations of the underlines in the hyperlinks don't change. That is, before the patch there's two pixels of white space between the font base line and the underline, and after the patch there's only one. It turns out that Qt 3.3 renders underlines the same way, with Konqueror 3.5 using single pixel spaces for underlines too [7]. Plus, in these specific examples, the bottom curves of the 'g's in the underlined "holographic storage breakthrough" link in the patched version descends below the underline, whereas in the unpatched version they merely touch it. I find this behavior to be aesthetically more appealing, although I'm not sure if it's correct or intened.

There are alternative ways to correct the excess vertical spacing bug that don't modify the QFontEngineFT::descent method. For example, QFontMetrics::height could be modified to calculate height as "ascent + descent". Although I've not tested it, I suspect that doing so would avoid the position side effect at the cost of calculating font heights that might be one pixel too small (since the method would implement a floor round instead of a ceiling round).

In any event, the attached patch does correct the bug. However, because of its side effects and the ambiguous way in which the Qt 4 font descent methods was implemented and subsequently changed, I'm not sure if the patch corrects the bug properly, or if another fix is more appropriate. Thus, feedback on the issue is greatly appreciated.

Thanks!

[1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=524062
[2] http://www.inp.nsk.su/~bolkhov/files/fonts/univga/
[3] http://www.club.cc.cmu.edu/~mkasick/libqtgui4/libQtGui_good_metrics.png
[4] http://www.club.cc.cmu.edu/~mkasick/libqtgui4/libQtGui_bad_metrics.png
[5] http://www.club.cc.cmu.edu/~mkasick/libqtgui4/libQtGui_konq_45_old.png
[6] http://www.club.cc.cmu.edu/~mkasick/libqtgui4/libQtGui_konq_45_new.png
[7] http://www.club.cc.cmu.edu/~mkasick/libqtgui4/libQtGui_konq_35.png