Closed Bug 947650 Opened 6 years ago Closed 6 years ago

Some math fonts have excessive ascent/descent

Categories

(Core :: MathML, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: fredw, Assigned: jfkthame)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: testcase, Whiteboard: [parity-webkit])

Attachments

(4 files, 8 obsolete files)

1.80 KB, text/html
Details
14.97 KB, image/png
Details
4.98 KB, patch
RyanVM
: checkin+
Details | Diff | Splinter Review
3.72 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
Attached file testcase
As I mentioned in bug 415413 comment 74, some math fonts have excessive ascent/descent and can not be used in MathML.

I attach a testcase and a screenshot will follow.

Note: my understanding in that some fonts have a text and math versions the former is intended to be used in paragraph of text and the latter to be used in the equations.

I don't know if that's a bug in the fonts or if the math engine is supposed to use some different metrics (say ink bounds) or some parameters from the MATH table...
Attached image screenshot
Here is a screenshot.

At the moment, for problematic fonts I have not changed the MathML torture test to use the specified font-family.
I think these are font bugs, the issue in the all proplematic fonts is actually the same; The font has correct OS/2 Typo metrics (suitable for line spacing) and OS/2 Win metrics (big enough for clipping), but the hhea table has values equal to OS/2 Win metrics which is wrong, they should be equal to the typo metrics. It seems Gecko prefer hhea table over OS/2 Typo metrics.

I think the issue should reported to STIX and TeX Gyre Math projects. On Gecko side, if possible may be the OS/2 Typo metrics should be preferred either unconditionally or when such a problematic case is detected.
Keywords: testcase
Priority: -- → P5
I forgot to say that I have the same problem with Cambria Math on Windows.

(I don't have the problem with Cambria Math on Linux, but I used font forge to extract the font from the cambria.ttc file and save it as .odt - or otherwise my Gecko patches were not able to read the Open Type MATH table - so that's maybe not relevant)
My version of Cambria Math seems to have the correct metrics and looks fine (tested only on Linux, should be the smame unless on Windows OS/2 Win metrics are used for line spacing, but then that would be a Gecko bug IMO).
Blocks: 947654
@Jonathan: can you comment on this and confirm if it's a bug in the fonts or if Gecko should do something to avoid that?
Flags: needinfo?(jfkthame)
There's a known Gecko problem whereby Cambria Math, for example, causes excessive line-spacing on some platforms - at least Windows GDI, and probably also others in the case where 'hhea' metrics match the OS/2 win metrics rather than the typo ones. See bug 598900 comment 10.

Once upon a time, I started on a patch to rework this (see bug 598900 comment 18), but it's never been finished/tested/landed, sorry. (There are a number of related line-spacing bugs that ideally would all be fixed by reimplementing this stuff in a more consistent way.)

So I do think this is a Gecko bug that we should fix, but haven't had time to deal with it. I'd like to ensure more consistent behavior across platforms, but any patches in this area will need wide testing across a variety of platforms, fonts and sites.
Flags: needinfo?(jfkthame)
See Also: → 598900
Depends on: 598900
See Also: 598900
This bug is likely to become important for MathML if we want to move to Open Type MATH fonts. Would it be possible to fix that, without having to rewrite gfxGDIFont too much?
Attachment #8369467 - Attachment is obsolete: true
Depends on: 657864
Building on the patches in bug 657864; see also discussion in bug 598900. This fixes the line metrics for OpenType math fonts on Windows/GDI and on OS X by preferring the sTypo* metrics for these fonts.
Attachment #8369642 - Flags: review?(karlt)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
With the two patches above, the testcases at http://www.maths-informatique-jeux.com/ulule/mathml_torture_test/ascent-descent.html look good on all platforms I've tested so far (WinXP, Win8 using GDI, OS X, Linux) *except* for Windows with DirectWrite, where the excessive line spacing remains.

To fix that, we may need to switch the DWrite backend over to also use gfxFont::InitMetricsFromSfntTables, unless there are ways to get the metrics we need for these fonts via DWrite APIs.
Thanks the test renders correctly for me now!

Actually, this even fixes a long standing line-height bug with the STIX General fonts: try to compare the STIX General and STIX Word fonts in https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/MathML_Torture_Test e.g. tests 23 and 24.

How popular is the "Windows with DirectWrite"? And does the bug show up with Cambria Math too on that platform?
Attachment #8369482 - Attachment is obsolete: true
Attachment #8369644 - Attachment is obsolete: true
Attachment #8369644 - Flags: review?(karlt)
(In reply to Frédéric Wang (:fredw) from comment #13)

> How popular is the "Windows with DirectWrite"?

Very; that's what most Win7/Win8 users on modern hardware (anywhere we use D2D acceleration) will get. So we really do need to fix that case as well.

> And does the bug show up with
> Cambria Math too on that platform?

No, Cambria Math is OK there. I'm not sure offhand whether this is because it sets the USE_TYPO_METRICS flag (though I suspect this is the key), or because DirectWrite treats it as part of the larger Cambria family and uses consistent metrics across the whole family, or some other kind of magic.
OK, I was asking that because since Cambria Math is installed on Win7/Win8 by default we can arrange to make this math font the one used by default on these platforms and so the bug becomes less serious. Of course that does not help users who want to use custom math fonts...
Updated FT2 patch to avoid a b2g reftest failure that tryserver showed.
Attachment #8375041 - Flags: review?(karlt)
Attachment #8370008 - Attachment is obsolete: true
Attachment #8370008 - Flags: review?(karlt)
Attachment #8369642 - Flags: review?(karlt) → review+
Comment on attachment 8375041 [details] [diff] [review]
[FT2 fonts] ensure we prefer OS/2 sTypo* metrics to hhea ascent/descent for OpenType Math fonts.

>+        if (0 == FT_Load_Sfnt_Table(mFace, FT_MAKE_TAG('M','A','T','H'),
>+                                    0, NULL, &length)) {

nullptr, please.
Attachment #8375041 - Flags: review?(karlt) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #17)
> Created attachment 8375041 [details] [diff] [review]
> Updated FT2 patch to avoid a b2g reftest failure that tryserver showed.

Can you be more specific about the problem with USE_TYPO_METRICS, please?
There was a strange-looking failure in layout/reftests/border-image/border-image-element.html; see https://tbpl.mozilla.org/?tree=Try&rev=5d9be0d9fded for a fresh run. I have no idea what's going on there, or why this affected it, but making the patch as conservative as possible (targeting *only* math fonts) appeared to avoid it.

OTOH, that new run shows an unexpected PASS in layout/reftests/xul/tree-row-outline-1.xul, which might actually be a genuine improvement due to better metrics (I haven't captured the current failing version for comparison yet), so perhaps we should try to go that way.
It turns out the border-image-element.html test is already marked as "massively fuzzy" in the reftest manifest, and the USE_TYPO_METRICS patch doesn't substantially change things; it just happens that it perturbs the positioning a bit and so the exact number of fuzzy pixels changes. I think we can ignore that, and just adjust the fuzzy() annotation as needed.

(See https://tbpl.mozilla.org/?tree=Try&rev=88ab6ad82b23 for a try run -without- this patch, where that fuzzy() annotation was removed, so that we can see the existing fuzzy-failure captured in reftest analyzer.)
Comment on attachment 8370008 [details] [diff] [review]
[FT2 fonts] prefer OS/2 sTypo* metrics to hhea ascent/descent if USE_TYPO_METRICS flag is set, and for OpenType Math fonts.

This one looks good to me.

(In reply to Jonathan Kew (:jfkthame) from comment #21)
> It turns out the border-image-element.html test is already marked as
> "massively fuzzy" in the reftest manifest, and the USE_TYPO_METRICS patch
> doesn't substantially change things; it just happens that it perturbs the
> positioning a bit and so the exact number of fuzzy pixels changes. I think
> we can ignore that, and just adjust the fuzzy() annotation as needed.

Thank you for checking that out.  I'm guessing the biggest difference is in the sides of border2 where there seems to be a slightly different vertical alignment.  But there were already similar differences in the linux64 results.  The b2g results may just be amplifying those due to the apparent nearest neighbour interpolation.  The differences in these tests do not seem to be due to any problems in the font metrics, but expected from the nature of the test.  So, I agree, we can ignore that.
Attachment #8370008 - Attachment is obsolete: false
Attachment #8370008 - Flags: review+
(In reply to Karl Tomlinson (:karlt) from comment #22)
> Comment on attachment 8370008 [details] [diff] [review]
> [FT2 fonts] prefer OS/2 sTypo* metrics to hhea ascent/descent if
> USE_TYPO_METRICS flag is set, and for OpenType Math fonts.
> 
> This one looks good to me.

OK, let's go with this. I'll adjust the reftest manifest annotation as needed.
I didn't follow everything here. What is the status of the patches? Is it blocked by bug 657864?
Flags: needinfo?(jfkthame)
Yeah, these patches were written on top of bug 657864, and I haven't had time to get back to that lately. :( Maybe we can land these separately, with some rebasing, but I'm not sure offhand whether that's feasible.
Flags: needinfo?(jfkthame)
The main thing missing without the refactoring in bug 657864 I think would be a fix for GDI.
(In reply to Karl Tomlinson (:karlt) from comment #26)
> The main thing missing without the refactoring in bug 657864 I think would
> be a fix for GDI.

Yes, the FT2 patch seems to apply correctly (I'm not sure which of the two is correct, though).
Comment on attachment 8375041 [details] [diff] [review]
[FT2 fonts] ensure we prefer OS/2 sTypo* metrics to hhea ascent/descent for OpenType Math fonts.

Attachment 8370008 [details] [diff] is the preferred solution here.

It just needs adjustments to the reftest manifests to:

* remove fails-if(B2G) for tree-row-outline-1.xul, and

* handle "max difference: 151, number of differing pixels: 5809"
for border-image-element.html on B2G at least
Attachment #8375041 - Attachment is obsolete: true
Attachment #8370008 - Attachment is obsolete: true
(Untested patch)

Try to update 8369642 to apply on top of trunk.

IIUC, this won't work for GDI fonts until the refactoring of bug 657864.
Attachment #8406697 - Flags: review?(karlt)
Attachment #8406697 - Flags: review?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #25)
> Yeah, these patches were written on top of bug 657864, and I haven't had
> time to get back to that lately. :( Maybe we can land these separately, with
> some rebasing, but I'm not sure offhand whether that's feasible.

Do you have any suggestion to do that?

I tried to read the patches from bug 657864 to make this work with GDI fonts, but I don't really understand the code.

Also, I tried to check the Dwrite backend, but I only see IDWriteFontFace::GetMetrics and IDWriteFontFace::GetGdiCompatibleMetrics for which we don't have any control over the choice of metrics.
Flags: needinfo?(jfkthame)
Attachment #8406692 - Flags: review?(karlt) → review+
Comment on attachment 8375041 [details] [diff] [review]
[FT2 fonts] ensure we prefer OS/2 sTypo* metrics to hhea ascent/descent for OpenType Math fonts.

>+                                    0, NULL, &length)) {

nullptr instead of NULL, please.
Comment on attachment 8406697 [details] [diff] [review]
[GDI & Mac fonts] prefer OS/2 sTypo* metrics to hhea ascent/descent if USE_TYPO_METRICS flag is set, and for OpenType Math fonts.

jfkthame is a better reviewer than i for this.
Attachment #8406697 - Flags: review?(karlt)
(In reply to Khaled Hosny from comment #2)
> I think the issue should reported to STIX and TeX Gyre Math projects. On
> Gecko side, if possible may be the OS/2 Typo metrics should be preferred
> either unconditionally or when such a problematic case is detected.

I didn't come back to this, since we decided it was something to fix in Gecko, but should we still report something to them? I found similar issues while trying to implement the MATH table in WebKit MathML...
Whiteboard: [parity-webkit]
(In reply to Frédéric Wang (:fredw) from comment #36)
> (In reply to Khaled Hosny from comment #2)
> > I think the issue should reported to STIX and TeX Gyre Math projects. On
> > Gecko side, if possible may be the OS/2 Typo metrics should be preferred
> > either unconditionally or when such a problematic case is detected.
> 
> I didn't come back to this, since we decided it was something to fix in
> Gecko, but should we still report something to them? I found similar issues
> while trying to implement the MATH table in WebKit MathML...

I believe so (especially STIX Math which is being overhauled, so it is better to make sure this issue is addressed).
I've reported the issue to STIX (https://sourceforge.net/p/stixfonts/tracking/53/) and will write to the efoundry group, if I find how to contact them.
What about taking the patches for Linux/Mac, so that things work properly on these platforms for ESR and at least the Cambria Math for Windows?

Does anyone know the right way to contact the e-foundry team for bug report? I tried the *@gust.org.pl addresses but didn't get any reply so far...
Flags: needinfo?(khaledhosny)
Flags: needinfo?(karlt)
(In reply to Frédéric Wang (:fredw) from comment #39)
> Does anyone know the right way to contact the e-foundry team for bug report?
> I tried the *@gust.org.pl addresses but didn't get any reply so far...

No, I don’t know any other way (short of attending the next BachoTeX conference…).
Flags: needinfo?(khaledhosny)
(In reply to Frédéric Wang (:fredw) from comment #39)
> What about taking the patches for Linux/Mac, so that things work properly on
> these platforms for ESR and at least the Cambria Math for Windows?

I'm fine about landing the Linux patch (which includes Android).

Cambria Math should be OK with DWrite, but we need another patch to make it OK with GDI.
Flags: needinfo?(karlt)
I have not really been able to run the B2G tests because of bug 985864. Hopefully, the results have not changed since Jonathan's try

https://tbpl.mozilla.org/?tree=Try&rev=d907b59e0545
https://tbpl.mozilla.org/?tree=Try&rev=11f6a60fc76c
Keywords: checkin-needed
Whiteboard: [parity-webkit] → [parity-webkit][leave open][please commit attachment 8407371]
Attachment #8369642 - Attachment is obsolete: true
Attachment #8407371 - Flags: checkin+
https://hg.mozilla.org/integration/mozilla-inbound/rev/35ed12caf75f
Keywords: checkin-needed
Whiteboard: [parity-webkit][leave open][please commit attachment 8407371] → [parity-webkit][leave open]
Comment on attachment 8406697 [details] [diff] [review]
[GDI & Mac fonts] prefer OS/2 sTypo* metrics to hhea ascent/descent if USE_TYPO_METRICS flag is set, and for OpenType Math fonts.

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

Looks like there are a couple of issues with how this has been rebased...

::: gfx/thebes/gfxFont.cpp
@@ +4085,5 @@
> +        // (see http://www.microsoft.com/typography/otspec/os2.htm#fss)
> +        const uint16_t kUseTypoMetricsMask = 1 << 7;
> +        if (uint16_t(os2->fsSelection) & kUseTypoMetricsMask) {
> +            if ((uint16_t(os2->fsSelection) & kUseTypoMetricsMask) ||
> +                mFontEntry->HasFontTable(TRUETYPE_TAG('M','A','T','H'))) {

This looks broken: need to get rid of the outer one of those if()s.

Also, this needs to be within a block that checks that the length of the OS/2 table is sufficient for us to access the fsSelection field. Maybe that's what the outer if() should have been testing?
Attachment #8406697 - Flags: review?(jfkthame) → review-
Also, if we're going to land this patch now, we should update the commit message to refer to [sfnt metrics] instead of [GDI & Mac fonts], as GDI fonts don't yet use this code (but should do so in the future...).
Flags: needinfo?(jfkthame)
Comment on attachment 8411699 [details] [diff] [review]
[sfnt metrics] prefer OS/2 sTypo* metrics to hhea ascent/descent if USE_TYPO_METRICS flag is set, and for OpenType Math fonts.

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

LGTM, thanks!
Attachment #8411699 - Flags: review?(jfkthame) → review+
I'm keeping "leave open" to address the GDI case later.
Keywords: checkin-needed
Whiteboard: [parity-webkit][leave open] → [parity-webkit][leave open][please commit attachment 8411699]
Whiteboard: [parity-webkit][leave open][please commit attachment 8411699] → [parity-webkit][leave open]
Blocks: 1001169
Blocks: 1001570
(In reply to Frédéric Wang (:fredw) |away 27/04 to 06/05 from comment #48)
> I'm keeping "leave open" to address the GDI case later.

Other patches will be in a different version, so please clone this bug for futher changes.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [parity-webkit][leave open] → [parity-webkit]
Target Milestone: --- → mozilla31
Blocks: 1170782
You need to log in before you can comment on or make changes to this bug.