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

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Blocks 1 bug)

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
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.
Attachment #8541901 - Flags: review?(smontagu)
(Assignee)

Comment 3

4 years ago
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.)
Attachment #8541955 - Flags: review?(smontagu)
(Assignee)

Updated

4 years ago
Attachment #8541901 - Attachment is obsolete: true
Attachment #8541901 - Flags: review?(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
Attachment #8541955 - Flags: review?(smontagu) → review+
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?
(Assignee)

Comment 6

4 years ago
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 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.
Attachment #8541877 - Flags: review?(smontagu) → review+
(Assignee)

Comment 8

4 years ago
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
Target Milestone: --- → mozilla37
https://hg.mozilla.org/mozilla-central/rev/d260f281dfe6
https://hg.mozilla.org/mozilla-central/rev/6536d9885d2e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Comment 10

4 years ago
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
(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.
You need to log in before you can comment on or make changes to this bug.