222920 - [Xft] fix the conversion error in SUB/SUP/Underline position calculation

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Zack Weinberg (:zwol)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20031010 Galeon/1.3.9
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20031010 Galeon/1.3.9

<code>&lt;sup&gt;</code> is intended to be suitable for mathematical
exponent notation, and <code>&lt;sub&gt;</code> is intended to be suitable
for index notation.  But Mozilla does not raise superscripts high enough,
or lower subscripts low enough, for these purposes.

I will attach an HTML file demonstrating the problem.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Zack Weinberg (:zwol)]

Attachment: test case

Comment 2

[Bill Mason]

WFM 20031019 PC/WinXP

Comment 3

[Boris Zbarsky [:bzbarsky]]

WFM as well, with a current Linux build (non-xft mozilla.org nightly).  Zack,
are you using an XFT build, by any chance?  about:buildconfig is the way to check.

Comment 4

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

Mozilla should be getting the subscript and superscript offsets from the font's
metrics, at least when they're available.  What is your default font?  Could you
paste the "Configure Arguments" section of about:buildconfig into the bug?

Comment 5

[Zack Weinberg (:zwol)]

I went and looked up the rules for sub- and superscript positioning in TeX.  As
you might guess, they're bloody complicated.  However, the take-home message is:
 Draw an invisible line at 4/5 the x-height of the parent font.  All of a
superscript should be above that line; all of a subscript should be below that
line.  Further, there should be sufficient vertical whitespace that if a
superscript were stacked directly above a subscript, the glyphs would not collide.

By comparison, Mozilla appears to shift SUB and SUP by only one or two pixels.

(The added complexities of TeX's algorithm are to allow precision tuning by the
font designer; they make use of "math font parameters" that I suspect do not
exist in plain old boring PostScript or TrueType fonts.)

(Please ignore the suggested algorithm in the test case, which I wrote before I
looked up the Right Thing.)

It should also be noted that neither SUP nor SUB appears to reduce the font size
enough.  TeX reduces each successive level of script to 70% of the parent font
size (this is actually hardwired in plain.tex, but never mind).  Mozilla appears
to use only a single point reduction, which is way too big.

Comment 6

[Zack Weinberg (:zwol)]

now to answer questions raised while i was digging through the TeXbook...

about:buildconfig says:

Configure arguments
--prefix=/usr --sysconfdir=/etc --mandir=/usr/share/man
--infodir=/usr/share/info --localstatedir=/var --disable-pedantic
--with-default-mozilla-five-home=/usr/lib/mozilla --disable-debug
--disable-tests --disable-short-wchar --enable-xprint --enable-strip-libs
--enable-crypto --enable-mathml --enable-oji --enable-extensions=all
--enable-ldap --with-system-zlib --enable-freetype2
--enable-default-toolkit=gtk2 --enable-toolkit=gtk2 --enable-svg
--without-system-mng --without-system-png --disable-xft

HOWEVER, I have the mozilla-xft package installed, which overrides that
--disable-xft at runtime.  I will retest the given test case with Xft turned
back off again.

Default serif font is Bitstream Vera Serif (12 px).

Comment 7

[Zack Weinberg (:zwol)]

If I swap the non-Xft libgfx_gtk.so back in, and re-select the same font
(bitstream vera serif) then superscripts are raised far enough, but subscripts
are still not lowered far enough.  This appears to be an issue with the font --
other font choices give wildly different (mostly moved too far!) script
positions.  Sloppy font metrics are certainly not Mozilla's fault.  However,
there does seem to be a genuine bug when Xft is in use, and I would also suggest
that Mozilla increase the superscript and subscript offsets when necessary to
duplicate the "invisible line at 4/5 the parent x-height" rule that TeX uses.

Comment 8

[Zack Weinberg (:zwol)]

Further experimentation reveals that some font choices cause good script
positioning even with Xft in use.  For instance, "Nimbus Roman No9 L" and "URW
Bookman L" look good.  Most of the TrueType fonts available through the font
selector, however, produce bad script positioning.

Comment 9

[Boris Zbarsky [:bzbarsky]]

.

Comment 10

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

Do the fonts that are bad provide superscript and subscript metrics?  If so,
then this is probably a bug in the font rather than Mozilla, since we use (as we
should) the metrics provided by the font when they're available.  If not, it
could be a bug in the code that guesses what to use when the font doesn't
provide the metrics.

Comment 11

[Zack Weinberg (:zwol)]

How do I tell? I don't know anything about the guts of a TT font.

Comment 12

[Jo Hermans]

The code (for Xft) can be found at :
http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsFontMetricsXft.cpp#1001 .
From what I can see, there's a PR_MAX function that ensures that the
mSuperscriptOffset is never larger than mXHeight (shouldn't that be 0.8 *
mXHeight ?), but there's no PR_MIN function. Similar code can be found for Gtk,
PS, OS2, Windows, ... too.

Comment 13

[Zack Weinberg (:zwol)]

Note that 0.8 * x-height is TeX's minimum, but (as far as I can tell) there is
_no_ maximum.  (I may have misunderstood.  Appendix G of the TeXbook is not
exactly easy reading.)

Maybe someone just got it backward?

Comment 14

[Jungshik Shin]

I'm afraid the current code reading off superscript/subscript positions from
TTFs in nsFontMetricsXft.cpp  is broken 

http://lxr.mozilla.org/seamonkey/source/gfx/src/gtk/nsFontMetricsXft.cpp#1001

1001     // mSuperscriptOffset
1002     if (os2 && os2->ySuperscriptYOffset) {
1003         val = os2->ySuperscriptYOffset >> 16;
1004         mSuperscriptOffset = nscoord(PR_MAX(f, NSToIntRound(val * f)));
1005     }
1006     else {
1007         mSuperscriptOffset = mXHeight;
1008     }

Right-shifting os2->ySuperScritpYOffset by 16bits always results in 0 so that
the superscript offset is always set to 1 pixel as mentioned in comment #5. The
apparent font dependency mentioned in commment #8 is misleading (Bitstream Vera
fonts were made by a professional font designer at Bitstream and it's highly
unlikely that he made such a blunder). It is due to that PS type1 fonts(such as
Nimbus* and URW*) don't have os2 table (only truetype fonts have) and the
fallback of using Xheight for superscript offset is used for them. I found this
problem in bug 218032 (fixing the underline position), but forgot to file a
separate bug. I'll upload a patch soon. 

Comment 15

[Jungshik Shin]

I haven't been able to test my patch because of a very weired problem with 
freetype2 library. Somehow, FT_Get_Sfnt_Table() (FT2 API) used to get OS2 
table  fails for all truetype fonts. I wrote to the freetype mailing list to ask for 
help and am still waiting for the answer.  With FT_Get_Sfnt_Table failing,  the 
default fallback values are used for SUP and SUB position. For SUP, it's the 
height of 'x'.  
 

Comment 16

[Jungshik Shin]

For the last couple of months(?), there has been a bug in freetype2 library that
broke OS/2 table reading. The bug was recently fixed and I'm now back to business...

Anyway, my suspicion was right. The current Xft code is broken and
super/subscript offset is always set to 1pixel for truetype fonts with OS/2 table.

To my surprise, Bitstream Vera Serif is also buggy in that its ySubscriptOffset
in OS/2 table is negative. According to truetype/opentype spec, it's positive
downward from the baseline.

With f=15 (the conversion factor from pixel to mozilla's internal unit),
I got the following for Bitstream Vera Serif (with my patch I'm gonna upload in
a moment)

os2->sup=595, xheight=135, emHeight=240, sup>>16 = 0,    
 val=5.000000,mSuper=75

os2->sub=-446, xheight=135, emHeight=240, sub>>16 = -1,  
   val=-3.000000,mSub=15

As you can see, superscript offset specified in the font's OS2 table is barely
larger than half of xheight.  Should we make it at least 4/5 of Xheight? I don't
know. Let's fix the bug (mentioned above) first.

Due to the bug in Bitstream Vera, subscript offset(mSub) is just 1 pixel (==>15
in mozilla's unit). If os2->sub were +446 instead of -446, it would be 3 pixel
(==> 45). We have to report this bug to Bitstream. 

BTW, Zack, what's Knuth's recommendation for the subscript offset? I'm being
lazy here with The TeX book in front of me :-). We're currently using xheight if
it's NOT specified in the font, but we may have to change it. 

Comment 17

[Jungshik Shin]

Attachment: patch

patch as promised.

Comment 18

[Zack Weinberg (:zwol)]

If I understand it right, TeX's basic rule for subscripts is "lower as far as
necessary so that no part of the glyph extends above 4/5 the parent font's
x-height."  This is then modified by font parameters.  I do not completely grok
the algorithm in appendix G of the TeXbook, though.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Zack is pretty much correct... The TeXBook algorithm is far more sophisticated
than anything we are likely to do (suffice it to say that it allows for
different subscript offsets based on whether a superscript is also present and
will position superscripts directly above subscripts, something Mozilla can only
dream of at present).  But it always enforces the constraint that the top of the
subscript is at at most 4/5*(x-height) and the bottom of the superscript is at
at least that point.

Comment 20

[Jungshik Shin]

Now the question is whether we want to override font designer's specification to
guarantee that the bottom of a superscript letter is at least 4/5 of xheight. 

How about the subscript position?  It's only more complicated because we also
have to determine 'subscript' size to guarantee that the top of any subscript is
at most 4/5 of xheight.

How about this? In this bug, let's just fix the Xft bug when reading 
ySuperscript/ySubscript values off the OS/2 table of truetype fonts. Then, let's
deal with the superscript/subscript positioning issue in a separate bug because
that's not only for Xft build but has to be done across platforms. 
 

Comment 21

[Jungshik Shin]

Attachment: patch

identical to attachment 135005 [details] [diff] [review] except that I fixed the same problem ( '>>16'
always yielding zero) in the code to obtain the underline position/thickness.

Comment 22

[Jungshik Shin]

I filed bug 225109 for sub/superscript positioning on all platforms so that I'm
chagning the summary line

Comment 23

[Jungshik Shin]

Comment on attachment 135100 [details] [diff] [review]
patch

asking for r/sr.

Comment 24

[Zack Weinberg (:zwol)]

A full implementation of the TeX math layout algorithm (the whole thing, not
just the sub/superscript positioning bit) would be very nice in the context of
MathML, but needn't happen to resolve this bug.

Comment 25

[Zack Weinberg (:zwol)]

Note I'm going to be unreachable by email for the next two weeks, but will
respond to queries when I get back.

Wikipedia (http://www.wikipedia.org/) math articles are a good source of test
cases for this stuff.  That wiki has support for embedded TeX math markup, which
it tries to translate to HTML if it's "simple enough" (otherwise it generates PNGs).

Comment 26

[Jungshik Shin]

Zack, please note that this bug kinda metamorphosed itself into something
different(OK. it didn't itself, but I did :-)). For sub/super positioning (that
should be dealt with across platforms), your comment would be welcome in bug
225109. BTW, also note that Mozilla fully supports MathML (for mathematical
typesetting on the web : see <http://www.mozilla.org/projects/mathml). Sub/Sup
tag in 'plain'(?) html is kinda 'poor man's math', isn't it?

While implementing MathML  in Mozilla, I'm sure rbs extensively consulted the
TeX book (as can be seen in comments in the actual code). 

Comment 27

[Jungshik Shin]

Comment on attachment 135100 [details] [diff] [review]
patch


>+// ceiling and truncation functions for Freetype floating point numbers
>+// represented with 26bit integer part and 6bit fractional part.
>+#define MOZ_FT_CEIL(x) (((x) + 63) & -64)
>+#define MOZ_FT_TRUNC(x) ((x) >> 6)
>+#define CONVERT_DESIGN_UNITS_TO_PIXELS(v, s) \
>+        MOZ_FT_TRUNC(MOZ_FT_CEIL(FT_MulFix((v) , (s))))

I'll use the following instead of the above. I found that using '-64' is not
64bit-safe. Alternatively, I can use PRInt32(-64) in place of 0xFFFFFFC0u

// ceiling and truncation functions for a Freetype floating point number
// stored in a 32bit integer with high 26 bits for the integer part and low
// 6 bits for the fractional part. 
#define MOZ_FT_CEIL(x) (((x) + 63) & 0xFFFFFFC0u) // 63 = 2^6 - 1
#define MOZ_FT_TRUNC(x) ((x) >> 6)
#define CONVERT_DESIGN_UNITS_TO_PIXELS(v, s) \
	MOZ_FT_TRUNC(MOZ_FT_CEIL(FT_MulFix((v) , (s))))

Comment 28

[Brendan Eich [:brendan]]

Nit: Using -64 instead of ~63 seems a little less clear to me.

Why is -64 not 64-bit safe?  It's an int, so if sizeof(int) == 4 and
sizeof(long) (FT_Long) == 8, it should be sign-extended when used in an
expression with longs. I don't see how 0xFFFFFFC0 is better, but using
0xFFFFFFc0u seems worse, because it is unsigned int.  It may not matter for the
true domain of these macros, which may be small-ish integers, but shouldn't they
treat their arguments as FT_Long?

Sorry for jumping in here, I'm just a fan of integer strength reduction, trying
to keep up with the Joneses.  If you trusted the compiler to do reduction in
strength, you could use PR_ROUNDUP.

/be

Comment 29

[Jungshik Shin]

Thanks for the tip. I meant to check what FT_Long (and FT_F26Dot6) is(are) 
type-defined to be, but was lazy and incorrectly assumed that it's guaranteed to 
be 32bit unsigned integer like PRUint32. Of course, that doesn't justify my 
totally wrong statement about 64bit cleanness(I don't know what I was thinking. 
perhaps, I was thinking it was OR'd instead of AND'ed with the rest.). I should 
have checked the fttypes.h. Both FT_Long and FT_F26Dot6 are defined to be signed 
long. So, I should use either ~63 (or -64 as it is. or PR_ROUNDUP(x, 64)). as 
you wrote.

Comment 30

[Jungshik Shin]

Comment on attachment 135100 [details] [diff] [review]
patch

asking again for r. 

I'll just go with this patch (except for replacing '-64' with '~63' and the
comment change mentioned in the prev. comment)

It's strange that my request was cleared although I haen't received an email to
that effect. Did I clear it inadvertently?

Comment 31

[Christopher Blizzard (:blizzard)]

Comment on attachment 135100 [details] [diff] [review]
patch

Please remove the DEBUG_junshik bits.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135100 [details] [diff] [review]
patch

sr=bzbarsky with the rounding to 64 done right.

Comment 33

[Jungshik Shin]

Attachment: patch w/o DEBUG

.

Comment 34

[Jungshik Shin]

Comment on attachment 135487 [details] [diff] [review]
patch w/o DEBUG

asking for r.
the same as before except that there's no DEBUG and '~63' is used instead of
'-64'.

Comment 35

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

This patch seems to have made, in my build, all underlines be two pixels thick,
when they were previously one pixel thick and are one pixel thick in other apps
that use the same font via Xft.

Comment 36

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

Looking through the freetype code itself, whenever it wants to round the result
of FT_MulFix to a pixel, it uses (FT_MulFix( , ) + 32) & -64.  In other words,
it's doing rounding, not ceiling.

Comment 37

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

Attachment: round rather than ceil

This fixes the extra-thick underlines.

Comment 38

[Brendan Eich [:brendan]]

May as well get this for 1.6b.  I'm guilty for not seeing the forest for the
trees here, distracted by reduction-in-strength minutiae.

/be

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135952 [details] [diff] [review]
round rather than ceil

Don't we wamt to ensure that we always end up with at least a pixel? 
sr=bzbarsky either way.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Then again, on some devices maybe we don't.  Ignore that part.

Comment 41

[Jungshik Shin]

Comment on attachment 135952 [details] [diff] [review]
round rather than ceil


> // ceiling and truncation functions for a Freetype floating point number 
> // (FT26Dot6) stored in a 32bit integer with high 26 bits for the integer

Thanks for catching it.
r=jshin with 'ceiling replaced by 'rounding' in the comment above.

bz, we guaranteed that underlines are at least one pixel using PR_MAX(1,
value_obtained_with_this_macro) later in the code.

Comment 42

[Brendan Eich [:brendan]]

Comment on attachment 135952 [details] [diff] [review]
round rather than ceil

a=[e-mail] for drivers, for 1.6b.

/be

Comment 43

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

Comment on attachment 135952 [details] [diff] [review]
round rather than ceil

Checked in, 2003-11-20 12:05 -0800.

Comment 44

[Jeremy M. Dolan]

OT Anecdote: I looked at my own web page the other day with a recent build, and
thought "Wow, something's different. This looks really cool."  Couldn't put my
finger on it--thought something about the font rendering was changed.

Saw this check-in today, and I'm sad to see my beefy underlines go. There isn't
any CSS property that affects the line thickness for links or otherwise, is there?

Comment 45

[Jungshik Shin]

fixed a while ago.