1115916 - improve synthetic vertical metrics for fonts that don't have actual vhea/vmtx tables

Status: RESOLVED FIXED

Description

[Jonathan Kew (:jfkthame)]

The code in gfxFont::CreateVerticalMetrics is supposed to synthesize appropriate default metrics when the font does not have actual vertical-metrics tables. However, when an 'OS/2' table is present it does a poor job, which particularly affects non-Mac platforms (many of the standard OS X fonts do not have an OS/2 table, so we use the 'head' table as a fallback, and avoid the issues here).

To see the poor results on OS X, compare

(a) data:text/html,<div style="writing-mode:vertical-lr;text-orientation:upright;font:32px courier">ABC upright

(b) data:text/html,<div style="writing-mode:vertical-lr;text-orientation:upright;font:32px courier new">ABC upright

Result: (a) looks OK, as Courier has no OS/2 table; (b) is badly spaced (especially noticeable if you drag-select across some of the text) because it uses the OS/2 table and synthesizes bad metrics from it.

Non-Mac platforms will tend to hit cases like (b) with most fonts.

Comment 1

[Jonathan Kew (:jfkthame)]

Attachment: improve synthetic vertical metrics for fonts that don't have actual vhea/vmtx tables

Comment 2

[Jonathan Kew (:jfkthame)]

Attachment: Reftest for synthetic vertical metrics

Here's a reftest to go with this. Using the Ahem font, the background of the <span>s should not show at all as the glyphs completely fill the frame. But prior to the patch here, the middle case (vertical/upright) fails badly.

Comment 3

[Jonathan Kew (:jfkthame)]

Attachment: Reftest for synthetic vertical metrics

Reduced the reftest to include only the critical case using upright vertical glyphs; the horizontal-metrics cases (ironically) add too much 'fuzz' due to small subpixel discrepancies. (I think even this will need a small amount of fuzz, once tryserver returns its verdict.)

Comment 4

[Simon Montagu :smontagu]

Comment on attachment 8541955 [details] [diff] [review]
Reftest for synthetic vertical metrics

Review of attachment 8541955 [details] [diff] [review]:
-----------------------------------------------------------------

No need to rerequest review if you need to add a little fuzz

Comment 5

[Simon Montagu :smontagu]

Comment on attachment 8541877 [details] [diff] [review]
improve synthetic vertical metrics for fonts that don't have actual vhea/vmtx tables

Review of attachment 8541877 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +3395,5 @@
> +                (int16_t(os2->sTypoAscender) - int16_t(os2->sTypoDescender));
> +            metrics->aveCharWidth =
> +                std::max(metrics->emHeight, ascentDescent);
> +            gfxFloat halfCharWidth =
> +                int16_t(os2->xAvgCharWidth) * gfxFloat(mFUnitsConvFactor) / 2;

Is there a particular reason for using half the char-width rather than some other fraction, or is it just black magic? Should we experiment with a range of fonts on different platforms?

Comment 6

[Jonathan Kew (:jfkthame)]

Comment on attachment 8541877 [details] [diff] [review]
improve synthetic vertical metrics for fonts that don't have actual vhea/vmtx tables

Review of attachment 8541877 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +3395,5 @@
> +                (int16_t(os2->sTypoAscender) - int16_t(os2->sTypoDescender));
> +            metrics->aveCharWidth =
> +                std::max(metrics->emHeight, ascentDescent);
> +            gfxFloat halfCharWidth =
> +                int16_t(os2->xAvgCharWidth) * gfxFloat(mFUnitsConvFactor) / 2;

The idea is simply to give the font a (minimum) horizontal extent of xAvgCharWidth, with half of this as the ascent, and half as the descent -- as the vertical metrics are based on a default centered baseline.

(In many cases, this won't actually alter things from the default of 0.5em that was set at the start of this function. But it seems worth including for the benefit of fonts with unusual proportions.)

Comment 7

[Simon Montagu :smontagu]

Comment on attachment 8541877 [details] [diff] [review]
improve synthetic vertical metrics for fonts that don't have actual vhea/vmtx tables

Review of attachment 8541877 [details] [diff] [review]:
-----------------------------------------------------------------

Fair enough, but this code really needs more comments -- it's very hard to parse because it's not clear which of the terms like "width" "height" "ascent" "descent" etc. retain their physical meaning and which have it reversed.

Comment 8

[Jonathan Kew (:jfkthame)]

Fair point! I added some comments, hoping that'll help.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d260f281dfe6
https://hg.mozilla.org/integration/mozilla-inbound/rev/6536d9885d2e

Comment 9

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/d260f281dfe6
https://hg.mozilla.org/mozilla-central/rev/6536d9885d2e

Comment 10

[Gérard Talbot]

Hello,

I just visited this testpage

http://lxr.mozilla.org/mozilla-central/source/layout/reftests/writing-mode/1115916-1-vertical-metrics.html

and

http://lxr.mozilla.org/mozilla-central/source/layout/reftests/writing-mode/1115916-1-vertical-metrics-ref.html

and the Ahem font is not fetched and I believe it is because it should be "Ahem" and not "ahem" (see lines 8 and 12):

<style>
@font-face {
  font-family: ahem;
  src: url(../fonts/Ahem.ttf);
}

font: 16px/24px ahem;

The error console, CSS tab also reports:
"
downloadable font: rejected by sanitizer (font-family: "ahem" style:normal weight:normal stretch:normal src index:0) source: http://lxr.mozilla.org/mozilla-central/source/layout/reftests/fonts/Ahem.ttf
"

The Ahem.ttf font file is there and properly linked.

Should I create a separate bug report for this?

Gérard

Comment 11

[John Daggett (:jtd)]

(In reply to Gérard Talbot from comment #10)
> The error console, CSS tab also reports:
> "
> downloadable font: rejected by sanitizer (font-family: "ahem" style:normal
> weight:normal stretch:normal src index:0) source:
> http://lxr.mozilla.org/mozilla-central/source/layout/reftests/fonts/Ahem.ttf
> "
> 
> The Ahem.ttf font file is there and properly linked.
> 
> Should I create a separate bug report for this?
> 
> Gérard

If the sanitizer is rejecting a font it's generally a bug in your font. If you look closely at the error message above, you'll see the problem. The testcase your running on lxr is referencing the lxr-ified version of Ahem.ttf:

http://lxr.mozilla.org/mozilla-central/source/layout/reftests/fonts/Ahem.ttf

So running the test that way will pull down a *HTML* page, not binary font data, and the sanitizer will quickly reject it.