Bug 102374

In void SimpleFontData::platformInit() float ascent = narrowPrecisionToFloat(font_extents.ascent); float descent = narrowPrecisionToFloat(font_extents.descent); float lineGap = narrowPrecisionToFloat(font_extents.height - font_extents.ascent - font_extents.descent); lineGap results in -1. That seems wrong, since the cairo documentation days: "double height; the recommended vertical distance between baselines when setting consecutive lines of text with the font. This is greater than ascent+descent by a quantity known as the line spacing or external leading. When space is at a premium, most fonts can be set with only a distance of ascent+descent between lines."

This is originally caused by freetype rounding when "GRID_FIT_METRICS" is on: #ifdef GRID_FIT_METRICS metrics->ascender = FT_PIX_CEIL( FT_MulFix( face->ascender, metrics->y_scale ) ); metrics->descender = FT_PIX_FLOOR( FT_MulFix( face->descender, metrics->y_scale ) ); metrics->height = FT_PIX_ROUND( FT_MulFix( face->height, metrics->y_scale ) ); metrics->max_advance = FT_PIX_ROUND( FT_MulFix( face->max_advance_width, metrics->x_scale ) ); #else /* !GRID_FIT_METRICS */ Here, the CEIL + FLOOR of ascender and descender results in a non-uniform scaling - making the sum of ascender and descender larger than the height :-/

Turns out, cairo is only using the Freetype clamping path if cairo_font_options_set_hint_metrics is set to CAIRO_HINT_METRICS_DEFAULT or CAIRO_HINT_METRICS_ON, see http://cgit.freedesktop.org/cairo/tree/src/cairo-ft-font.c?id=1.12.4#n1888 WebCore's Cairo backend does not modify the DEFAULT setting, which results in always receiving integer-clamped values SimpleFontData::platformInit(). As mentioned, the clamping in FreeType can lead to incorrect, negative line gap values which are exposed when enabling subpixel layout. Since WebCore maintains the font metrics in floats, there's no reason we should clamp early, allowing such bugs. I suggest to disable cairo's metrics hinting and receive correct, proportionally scaled values in SimpleFontDataFreetype.cpp.

Created attachment 175215 [details] Patch

If we wanted to keep the old letter-spacing behavior, we can round as follows: --- a/Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp +++ b/Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp @@ -146,7 +146,7 @@ float SimpleFontData::platformWidthForGlyph(Glyph glyph) const float w = (float)m_spaceWidth; if (cairo_scaled_font_status(m_platformData.scaledFont()) == CAIRO_STATUS_SUCCESS && extents.x_advance) - w = (float)extents.x_advance; + w = (float)lroundf(extents.x_advance); return w; }

CC'ing GTK+ people with the hope of supporting this change, agreeing on the big rebaseline that this would require. On EFL, we'd like to do this at the same time with enabling subpixel-layout. How do the GTK people feel about doing the same?

We did something like this once and then undid it, which was terribly labor intensive. If this is necessary perhaps we can only do it when subpixel layout is turned on.

(In reply to comment #7) > We did something like this once and then undid it, which was terribly labor intensive. If this is necessary perhaps we can only do it when subpixel layout is turned on. Yes, I would like to land this and activate subpixel on EFL hopefully this week - if you think the patch is okay? Ideally without a platform or subpixel specific ifdef. That's why i am asking whether you would be up for activating subpixel now as well. Mac has recently switched.

(In reply to comment #8) > Ideally without a platform or subpixel specific ifdef. That's why i am asking whether you would be up for activating subpixel now as well. Mac has recently switched. I think an #ifdef makes sense, as it will allow us to switch to subpixel positioning for the right reason (it's ready and we've tested it). We're in the middle of a development cycle and spending a lot of time updating baselines for a feature we may disable later is a bit risky. Since we have a responsibility to ship stable software on a schedule that we cannot control entirely, we need to be pretty cautious. Can you elaborate on the problems that you think an #ifdef poses. Perhaps there is some middle ground here.

(In reply to comment #9) > (In reply to comment #8) > > > Ideally without a platform or subpixel specific ifdef. That's why i am asking whether you would be up for activating subpixel now as well. Mac has recently switched. > Can you elaborate on the problems that you think an #ifdef poses. Perhaps there is some middle ground here. Well, as in this case there would not be a technical reason for the #ifdef since we're sharing the backends, I was hesitant to put one in the patch. Of course, it's possible, with a comment explaining why. > We did something like this once and then undid it, which was terribly labor intensive. Can you tell a bit more how similar this attempt was? I am positive that there is actually a bug here. I'd like to know a bit more why you reverted a similar change. Gyuyoung, what do you think about the letter spacing change implication?

(In reply to comment #10) > > Can you elaborate on the problems that you think an #ifdef poses. Perhaps there is some middle ground here. > Well, as in this case there would not be a technical reason for the #ifdef since we're sharing the backends, I was hesitant to put one in the patch. Of course, it's possible, with a comment explaining why. I think a comment and an #ifdef are a bit safer. We can always remove an #ifdef, but changing hundreds of baselines is a bigger task. > Can you tell a bit more how similar this attempt was? I am positive that there is actually a bug here. I'd like to know a bit more why you reverted a similar change. Wildfox made the original change, I believe. I tried searching for it but the relevant patches continue to elude me. If I recall correctly it had to do with enabling and disabling cairo_font_options_set_hint_metrics and the reason we reverted it is that it seemed to introduce flakiness into many tests.

Comment on attachment 175215 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175215&action=review > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:115 > + cairo_font_options_t* defaultFontOptions = cairo_font_options_create(); > + // Set to off in order to receive non-rounded floating point values for font metrics in SimpleFontDataFreeType instead > + // of integer-clamped values that may cause negative values for line-gap. > + cairo_font_options_set_hint_metrics(defaultFontOptions, CAIRO_HINT_METRICS_OFF); If we turn this off globally we probably want to force it off for the GDK font options as well above.

Martin, thanks for the details, I think I found the changes you refer to: > Wildfox made the original change, I believe. http://trac.webkit.org/changeset/101343 > [...] If I recall correctly it had to do with enabling and disabling cairo_font_options_set_hint_metrics and the reason we reverted it is that it seemed to introduce flakiness into many tests. And the revert came in two steps, http://trac.webkit.org/changeset/102748 + http://trac.webkit.org/changeset/114796 There's some magic there in the ChangeLog. "Re-enable Cairo metrics hinting, which seems to ensure consistent results in reference and pixel test results." From the other way 'round: Can you recall how the results were inconsistent? Were they inconsistent between runs on the same machine? Or were they inconsistent between different hosts? In the latter case, the reason might be in different fontconfig and gnome-settings-daemon configurations for hinting? I'd like to really resolve this issue and later hopefully land a patch that doesn't have the word "seems" in the Changelog :-).

(In reply to comment #13) > From the other way 'round: Can you recall how the results were inconsistent? Were they inconsistent between runs on the same machine? Or were they inconsistent between different hosts? In the latter case, the reason might be in different fontconfig and gnome-settings-daemon configurations for hinting? Tests started to flake after the change. If I recall correctly we had reftest failures that we didn't have before. > I'd like to really resolve this issue and later hopefully land a patch that doesn't have the word "seems" in the Changelog :-). Well, that's mostly just my poor style of writing, but I fully support this work. :)

I've been doing a little research [1] and it seems that Behdad suggests that metrics hinting should be on if subpixel layout is disabled. "If you don’t have subpixel text positioning (ie. your graphics system knows only how to rasterize one image for a glyph and blit that onto the screen), then you really want to enable metrics hinting." This is the first bullet point in the "Constraints" section. 1. https://docs.google.com/document/d/1wpzgGMqXgit6FBVaO76epnnFC_rQPdVKswrDQWyqO1M

CCing Behdad, who probably has some insightful input.

(In reply to comment #16) > 1. https://docs.google.com/document/d/1wpzgGMqXgit6FBVaO76epnnFC_rQPdVKswrDQWyqO1M Sorry. This link should be: https://docs.google.com/document/d/1wpzgGMqXgit6FBVaO76epnnFC_rQPdVKswrDQWyqO1M/edit

Hi, Martin is correct: cairo does not know how to render subpixel positioned text, and as such it will be a disaster IMO to turn metrics hinting off. There is an example in the document of the kind of wrong effects one would get doing that. What I suggest is to do something different only for getting the line metrics perhaps. For the actual measurement and layout of glyphs, don't disable metrics hinting with cairo.

Another reason why you can't disable metrics hinting is that you are not disabling hinting. Same document explains why that is a problem also. Second bullet in the "constraints" section.

(In reply to comment #18) > What I suggest is to do something different only for getting the line metrics perhaps. For the actual measurement and layout of glyphs, don't disable metrics hinting with cairo. Thanks, Behdad. I'll see what I can do to just get the right height info. Maybe I can temporarily disable metrics hinting just to retrieve these.

Created attachment 176736 [details] Patch

This new patch works for fixing the line height, line gap issue and leaves hinting (and metrics hinting) enabled. This way, we give non-rounded height information to WebCore for use in layout. Any other comments on this Behdad? What do you think, Martin? Thanks for your help.

Btw, this is also fixing bug 103740.

LGTM. I wonder whether there's something to be done in FreeType itself. Would help knowing what Windows does in this case. If you can get those numbers, I can do the upstreaming work in FreeType.

(In reply to comment #24) Behdad, thanks for your feedback, much appreciated. > LGTM. I wonder whether there's something to be done in FreeType itself. You mean, with regards to the issue I mentioned in comment 2? > Would help knowing what Windows does in this case. If you can get those numbers, I can do the upstreaming work in FreeType. What numbers would you need? Just the line height, ascent & descent of the same font? And would just retrieving the font details from a small test app be enough, or do you mean, dumping this from the same execution path in Safari/Win?

Thanks. I'm mostly interested to know what would Windows APIs returns for ascent / descent / linegap for that font at that size . Because I'm sure FreeType / others would insist that the lineheight should match what Windows returns...

Comment on attachment 176736 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176736&action=review > Source/WebCore/ChangeLog:12 > + than the line height. To work around this without globally disabling font metrics > + hinting, I am temporarily creating a new cairo scaled font with metrics hinting > + off and retrieving the height values from this one. isnt metrics hinting bad anyway for scaling? like it might affect layout at different scales.

(In reply to comment #27) > isnt metrics hinting bad anyway for scaling? like it might affect layout at different scales. See comment 15, metrics hinting is useful if you don't have subpixel positioning. When scaling, a new font with the right font size is instantiated.

(In reply to comment #26) > Thanks. I'm mostly interested to know what would Windows APIs returns for ascent / descent / linegap for that font at that size . Because I'm sure FreeType / others would insist that the lineheight should match what Windows returns... So, using a quick and dirty C# app [1], I got this: "Liberation Sans, 16 (fontSize: 16) - lineSpacingPixel: 18.39844, ascentPixel 14.48438, descent 3.390625, ascent+descent: 17.875" Very very close to what I get from Cairo/Freetype (after my temporary-hinting-off patch): ascent: 14.484375 descent: 3.390625 height: 18.3984375 [1] https://gist.github.com/4215546 That means, we're good to proceed with this patch, right?

(In reply to comment #29) > (In reply to comment #26) > > Thanks. I'm mostly interested to know what would Windows APIs returns for ascent / descent / linegap for that font at that size . Because I'm sure FreeType / others would insist that the lineheight should match what Windows returns... > > So, using a quick and dirty C# app [1], I got this: > "Liberation Sans, 16 (fontSize: 16) - lineSpacingPixel: 18.39844, ascentPixel 14.48438, descent 3.390625, ascent+descent: 17.875" > > Very very close to what I get from Cairo/Freetype (after my temporary-hinting-off patch): > ascent: 14.484375 > descent: 3.390625 > height: 18.3984375 > > [1] https://gist.github.com/4215546 > > That means, we're good to proceed with this patch, right? Thanks for testing. I think the patch is fine to go in. That said, for Windows. I was more interested in the hinted integer metrics, since that's where the current bug in FreeType is. If you can get those number I will followup with FreeType upstream to fix it properly. I don't have access to Windows myself. Thanks again!

(In reply to comment #30) > Thanks for testing. I think the patch is fine to go in. That said, for Windows. I was more interested in the hinted integer metrics, since that's where the current bug in FreeType is. If you can get those number I will followup with FreeType upstream to fix it properly. I don't have access to Windows myself. Thanks again! More lovely Win programming [1] using GDI GetTextMetrics on a device context handle optained from .net gives me (Liberation Sans, 16 pixel): ascent: 14 descent: 3 height: 17 whereas the freetype code here gives me: ascent: 15 descent: 4 height: 18 Good luck fixing this on Freetype. [1] https://gist.github.com/d4169b1068c203432846

Thanks Dominik. I'll see what I can do.

(In reply to comment #31) > (In reply to comment #30) > > Thanks for testing. I think the patch is fine to go in. That said, for Windows. I was more interested in the hinted integer metrics, since that's where the current bug in FreeType is. If you can get those number I will followup with FreeType upstream to fix it properly. I don't have access to Windows myself. Thanks again! > > More lovely Win programming [1] using GDI GetTextMetrics on a device context handle optained from .net gives me (Liberation Sans, 16 pixel): > ascent: 14 > descent: 3 > height: 17 > > whereas the freetype code here gives me: > ascent: 15 > descent: 4 > height: 18 > > Good luck fixing this on Freetype. > > [1] https://gist.github.com/d4169b1068c203432846 Win GDI uses simple truncate instead of rounding?

(In reply to comment #33) > Win GDI uses simple truncate instead of rounding? It looks as if it's flooring all values, yes. But let's say I haven't looked at the GDI code :-). As per comment 2, at least a combination of ceiling some values and rounding others is an unholy combination.

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

(In reply to comment #32) > Thanks Dominik. I'll see what I can do. Any update on the FreeType front here, Behdad? Thanks.

I wasn't sure what to suggest to FreeType, so I put it off. Went ahead and sent a mail right now bringing it to others' attention. We'll see what people have to offer. It doesn't show up in the list archives yet, but it will be here: http://lists.nongnu.org/archive/html/freetype-devel/2013-01/index.html

There is definitely a bug in FreeType. Thanks for the report. It seems that the rounded ascender and descender values must be added to get the height. Instead, we currently add the ascender and descender values, then do rounding. For verifying this assumption I ask you to run your program from comment #31 for a whole range of sizes, say, 8ppem to 30ppem (I don't use Windows :-). Please tell me exactly the used version (and size) of LiberationSans-Regular.otf to avoid any ambiguities. Maybe it also helps to repeat this with other MS core fonts, just to be sure. Additionally, I would like to know which FreeType function Webkit was calling to get the incorrect results (in comment #31). Theoretically, this shouldn't happen at all because there is already code in `sfobjs.c' which uses ROUND only (as requested by TrueType), overriding the CEIL and FLOOR results FreeType is using for other font formats.

(In reply to comment #38) Thanks for taking a look, Werner. I'll try to get you the data you requested.

I was relying on our jhbuild freetype version being somewhat up to date - turns out this was an incorrect assumption. I should have done the bisecting earlier. In any event, freetype commit b0962ac34e660 "[truetype] Fix metrics on size request for scalable fonts." by Steven Chu changes my initial failure example to the better looking: @@ -3,5 +3,5 @@ layer at (0,0) size 800x600 RenderBlock {HTML} at (0,0) size 800x600 RenderBody {BODY} at (8,8) size 784x584 - RenderText {#text} at (0,0) size 196x19 + RenderText {#text} at (0,0) size 196x17 text run at (0,0) width 196: "red squares background image." So, we're actually hitting the same problem that Steven has been describing in http://lists.gnu.org/archive/html/freetype-devel/2011-07/msg00029.html He's commenting there that FT_Request_Size - actually computes incorrect values for Ahem - similar for my case of Liberation Sans here. If for TrueType - the ascent and descent metrics are reset afterwards anyway - why are we calling it for scalable fonts? Werner, regarding your original question: How is the incorrect rounding code reached? It is reached from FT_Request_Size -> tt_size_request -> ft_recompute_scaled_metrics (for details, I attached the stacktrace with freetype 2.4.11). But then, after Steven's patch - as far as I understand his patch - these incorrectly rounded values are discarded and replaced. Would you still need the values from Windows after this finding?

Created attachment 182532 [details] Stacktrace leading to incorrect rounding

Yes, please, just to be sure. And sorry, I don't fully understand the rest of your last comment: Is there still a problem left?

(In reply to comment #42) > Yes, please, just to be sure. > > And sorry, I don't fully understand the rest of your last comment: Is there still a problem left? Well, it seems that the (incorrect) rounding code in question is still run, but then these preliminary values are replaced - do I understand that right? (Same as in Steven's analysis: http://lists.gnu.org/archive/html/freetype-devel/2011-07/msg00029.html ) [...] " FT_Request_Metrics( size->face, req ); /* Calculates new metrics, incorrect for Ahem */ if ( FT_IS_SCALABLE( size->face ) ) /* Ahem is scalable */ error = tt_size_reset( ttsize ); /* Calculates proper metrics stored in ttsize */ [...] Shouldn't this code be changed so that FT_Request_Metrics is not called for SCALABLE fonts in order to avoid unnecessary computation?

Comment on attachment 176736 [details] Patch Obsoleting patch - solved in Freetype >= 2.4.6, see bug 106774.

Werner informed me that there has been indeed a fix/change in FreeType after our discussion here: http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=e0469372be3870a5ad60b2c4586e9c281357bd28