604836 - at some font sizes, max extents of Ahem come out 1px larger than font size

Status: NEW

(Core :: Graphics, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looking at the CSS 2.1 test suite on Linux, I'm seeing a lot of tests that are affected by what seems likely to be a rounding bug in our font code.  There are a significant number of font sizes at which the max extents of the Ahem font (i.e., max ascent + max descent, which we use for the size of the background on an inline element) come out 1px larger than the actual size.  This isn't supposed to happen for Ahem, but it does.  See, for example:

http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20HTML%3E%0A%3Cstyle%3E%0Aspan%20%7B%20background%3A%20red%3B%20color%3A%20green%3B%20font%3A%2032px%2F1%20Ahem%20%7D%0A%3C%2Fstyle%3E%0A%3Cspan%3EHello%3C%2Fspan%3E

There, it happens with a 32px font size, but it also happens at other sizes.

I might poke into this in a bit.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, I've traced this down to gfxFT2LockedFace::GetMetrics, where the numbers we get out of freetype already appear to be rounded-up:  for 32px Ahem freetype gives us ascender of exactly 26 and descender of exactly 7.  (Really they should be 25.6 and 6.4.)

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: possible patch

This fixes it, although I really don't know if it's something we'd want to do.

I'd note there is a bunch of rounding at the bottom of the function, though, which might make this a little less scary.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd note that I tried using height/ascender and height/descender instead of ascender/descender; that didn't work because of the code checking that maxDescent wasn't smaller than emDescent, etc.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This patch causes underlines to be too close to the text some portion of the time.

I think I can make my other idea work, though.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: less problematic patch

This patch seems less problematic.

Thoughts?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, I think this is still wrong since we end up drawing the text 1px higher than we should in the cases where this reduces the size by 1px.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmmm, I think the observed behavior suggests we're positioning the glyphs relative to their baseline, so both ways of allocating the reduction break some of the time.  (In any case, both ways break some of the time.)

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 490164 [details] [diff] [review]
less problematic patch

AFAIK ftMetrics.height is a line-height here and that's not what is wanted for
maxAscent and maxDescent (because it "height" includes leading).

http://freetype.sourceforge.net/freetype2/docs/reference/ft2-base_interface.html#FT_Size_Metrics
http://freetype.sourceforge.net/freetype2/docs/reference/ft2-base_interface.html#FT_FaceRec

Comment 9

[Karl Tomlinson (:karlt)]

In a pixellated world it's not generally possible to achieve all the desirable
properties and it's not clear which are the preferred properties to maintain.

And, as you mention, the baseline alignment makes choosing ascent and descent
difficult, and in reality the number of affected pixels will be the ceiling of
(absolute value of) each in each direction.

It might be worth experimenting with unrounded maxAscent and maxDescent, which
could be obtained from the FT_FaceRec mFace (as underline_position is used).
It may be best not to round ascent in GetMetrics if rounding to nearest as
glyphs will also be snapped to pixels based on non-pixel layout offsets.
Rounding absolute positions and snapping can add the errors (sometimes, and
cancel others).

A risk with not using the ceiling of ascent and descent comes from the fact
that we don't measure glyph bounds for normal text sizes and instead rely on
maxAscent and maxDescent.  Perhaps we should use a font bbox ceiling for
maximum glyph extents, and probably not call the layout metrics *max*,
because they are not.

I /think/ the underline offset really should be rounded because it is the
distance *between* two snapped items, though I don't see that happening.  When
the distance between them is integer, both snapped items get snapped in the
same direction (assuming consistent pixel snapping).

Maybe that was part of the problem with underlines mentioned in comment 4?

Though there is also the problem that glyphs get hinted and at small sizes and
those order-1-pixel changes are significant.  Rounding metrics up tends to get
lucky and compensate for this.

I wonder whether it is unfair for a test with non-pixel-aligned font metrics
to expect floating point accuracy.  The glyphs will cover an integer number of
pixels.

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 490164 [details] [diff] [review]
less problematic patch

As per the preceding few comments, I don't think it's clear exactly how we should proceed here at this point.

What I'd like to do would be to re-examine how we obtain and use font metrics across all platforms/backends, with a view to achieving more consistent behavior, as we currently have a number of unfortunate discrepancies. (See, for example, bug 598900 and bug 657864.)

I think we should try reading metrics directly from the 'sfnt' tables for TrueType/OpenType fonts, in order to give us well-understood, consistent results, and fall back to other metrics APIs such as FT2, GDI, DWrite, Core Text, etc only for the (increasingly rare) case where we're using some other kind of font. However, this will involve a more extensive patch cutting across all backends, and will obviously need to be carefully tested.

Currently, there is code used on OS X to read metrics from sfnt tables, and we could reuse much of this across other platforms; however, I'm not sure that it is entirely correct at this point (as illustrated by bug 657864 comment #0, where the Mac metrics appear to be significantly off).

Comment 11

[Gérard Talbot]

Test:
http://test.csswg.org/source/approved/css2.1/src/css1/c526-font-sz-001.xht

Its reftest:
http://test.csswg.org/source/approved/css2.1/src/css1/c526-font-sz-001-ref.xht

OS -> Linux

Comment 12

[Jet Villegas (inactive)]

Jonathan: I'd like to reach out to the Linux OS text engine developers if we need their help to fix this. Can you describe the desired API for the OS bug? If we start reading metrics directly from the 'sfnt' tables on Linux, is that preferable to using the OS API?

Comment 13

[Gérard Talbot]

Hello all,

I believe this bug is very important to fix. It annoys me tremendously and every single day. One consequence of this bug is that it makes Firefox's DOM inspector NOT reliable: what I can see rendered on a page is not what DOM inspector will report.

I understand that Firefox developers can not fix this bug and why.

I would like to know if this bug has been filed UPSTREAM to Linux font/text rendering engine developers. If so, then what is the bug report URL.

Thank you, Gérard

Comment 14

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #10)

> What I'd like to do would be to re-examine how we obtain and use font
> metrics across all platforms/backends, with a view to achieving more
> consistent behavior, as we currently have a number of unfortunate
> discrepancies. (See, for example, bug 598900 and bug 657864.)
> 
> I think we should try reading metrics directly from the 'sfnt' tables for
> TrueType/OpenType fonts, in order to give us well-understood, consistent
> results, and fall back to other metrics APIs such as FT2, GDI, DWrite, Core
> Text, etc only for the (increasingly rare) case where we're using some other
> kind of font. However, this will involve a more extensive patch cutting
> across all backends, and will obviously need to be carefully tested.

This makes a lot of sense.  I think how to test this needs to be thought out carefully, so that we know where differences vs. platform API's exist and why.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

ought to look into whether this was fixed by https://hg.mozilla.org/mozilla-central/rev/1bd6b04a535f  (bug 883997, despite the commit message).

Comment 16

[Jonathan Kew [:jfkthame]]

I don't believe so. That patch just avoids the case where we sometimes use an unrounded dimension for one or other of ascent/descent, but it won't prevent them -both- being rounded up, and giving a sum that is greater than the (rounded) em-height.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that Xidorn wrote basically the same patch for DWrite in bug 1476151.