Bug 102374 – [Cairo, FreeType] Incorrect baseline positioning & line spacing due to rounding

• Component: WebKit EFL
• Status: RESOLVED INVALID

[Dominik Röttsches (drott)]

When enabling subpixel layout on EFL, many fast/css/* tests have enclosed elements positioned 1 pixel too high.

Example:

--- /fast/dominik/dev/WebKitGit_EFL/lt_sub/fast/css/background-image-with-baseurl-expected.txt
+++ /fast/dominik/dev/WebKitGit_EFL/lt_sub/fast/css/background-image-with-baseurl-actual.txt
@@ -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
-        text run at (0,0) width 196: "red squares background image."
+      RenderText {#text} at (0,-1) size 196x19
+        text run at (0,-1) width 196: "red squares background image."

Mac & Chromium subpixel-enabled expectations have:

layer at (0,0) size 800x600
  RenderView at (0,0) size 800x600
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 196x18
        text run at (0,0) width 196: "red squares background image."

Levi, Emil, any suggestions what kind of platform-specific metrics input to the line layout calculations could lead to such an offset?

[Dominik Röttsches (drott)]

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."

[Dominik Röttsches (drott)]

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 :-/

[Dominik Röttsches (drott)]

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.

[Dominik Röttsches (drott)]

Created attachment 175215 [details]
Patch

[Dominik Röttsches (drott)]

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;    
 }

[Dominik Röttsches (drott)]

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?

[Martin Robinson]

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.

[Dominik Röttsches (drott)]

(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.

[Martin Robinson]

(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.

[Dominik Röttsches (drott)]

(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?

[Martin Robinson]

(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.

[Martin Robinson]

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.

[Dominik Röttsches (drott)]

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 :-).

[Martin Robinson]

(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. :)

[Martin Robinson]

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

[Martin Robinson]

CCing Behdad, who probably has some insightful input.

[Martin Robinson]

(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

[Behdad Esfahbod]

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.

[Behdad Esfahbod]

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.

[Dominik Röttsches (drott)]

(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.

[Dominik Röttsches (drott)]

Created attachment 176736 [details]
Patch

[Dominik Röttsches (drott)]

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.

[Dominik Röttsches (drott)]

Btw, this is also fixing bug 103740.

[Behdad Esfahbod]

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.

[Dominik Röttsches (drott)]

(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?

[Behdad Esfahbod]

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...

[Kenneth Rohde Christiansen]

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.

[Dominik Röttsches (drott)]

(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.

[Dominik Röttsches (drott)]

(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?

[Behdad Esfahbod]

(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!

[Dominik Röttsches (drott)]

(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

[Behdad Esfahbod]

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

[Viatcheslav Ostapenko]

(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?

[Dominik Röttsches (drott)]

(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.

[Viatcheslav Ostapenko]

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

[Dominik Röttsches (drott)]

(In reply to comment #32)
> Thanks Dominik.  I'll see what I can do.

Any update on the FreeType front here, Behdad? Thanks.

[Behdad Esfahbod]

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

[Werner Lemberg]

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.

[Dominik Röttsches (drott)]

(In reply to comment #38)

Thanks for taking a look, Werner. I'll try to get you the data you requested.

[Dominik Röttsches (drott)]

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?

[Dominik Röttsches (drott)]

Created attachment 182532 [details]
Stacktrace leading to incorrect rounding

[Werner Lemberg]

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?

[Dominik Röttsches (drott)]

(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?

[Dominik Röttsches (drott)]

Comment on attachment 176736 [details]
Patch

Obsoleting patch - solved in Freetype >= 2.4.6, see bug 106774.

[Dominik Röttsches (drott)]

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