Last Comment Bug 407059 - read OpenType MATH table for variants and parts of stretchy characters
: read OpenType MATH table for variants and parts of stretchy characters
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: x86 All
: P5 enhancement with 4 votes (vote)
: mozilla31
Assigned To: Frédéric Wang (:fredw)
:
Mentors:
http://lists.w3.org/Archives/Public/w...
Depends on: 400938 663740 941019 945254 1000370 CVE-2014-1551
Blocks: 945183 963147 1001570 cambria-math asana-math neo-euler stix-1.1 LM-Math lucida-fonts 930504 947654 960115 961365
  Show dependency treegraph
 
Reported: 2007-12-05 14:58 PST by Karl Tomlinson (:karlt)
Modified: 2014-06-27 09:51 PDT (History)
24 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
31+


Attachments
Patch V1 (54.89 KB, patch)
2013-12-06 12:14 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch to disable the scaling correction (2.20 KB, patch)
2013-12-06 12:15 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V2 (54.91 KB, patch)
2013-12-06 14:14 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Patch V3 (82.17 KB, patch)
2013-12-07 15:48 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
screenshot Asana (3.84 KB, image/png)
2013-12-07 15:55 PST, Frédéric Wang (:fredw)
no flags Details
screenshot Gyre Pagella (2.88 KB, image/png)
2013-12-07 16:13 PST, Frédéric Wang (:fredw)
no flags Details
screenshot Asana (XeTeX) (3.74 KB, image/png)
2013-12-07 16:14 PST, Frédéric Wang (:fredw)
no flags Details
screenshot Gyre Pagella (XeTeX) (3.55 KB, image/png)
2013-12-07 16:14 PST, Frédéric Wang (:fredw)
no flags Details
Patch V4 (90.95 KB, patch)
2013-12-08 13:56 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
subscript-placement.png (11.84 KB, image/png)
2013-12-09 01:59 PST, Frédéric Wang (:fredw)
no flags Details
Part 1: add a gfxMathTable class to read the MATH table. (29.76 KB, patch)
2013-12-09 05:38 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2: add a gfxFont interface to the MATH table. (13.04 KB, patch)
2013-12-09 05:39 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 3: make nsMathMLChar use the MATH table (37.44 KB, patch)
2013-12-09 05:39 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 4: remove mathfontAsanaMath.properties and STIX 1.0 beta ; add Latin Modern to the default fonts of nsMathMLChar (23.14 KB, patch)
2013-12-09 05:40 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 1: add a gfxMathTable class to read the MATH table. (30.08 KB, patch)
2013-12-09 09:05 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2: make nsMathMLChar use the MATH table (37.44 KB, patch)
2013-12-09 22:25 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 3: remove mathfontAsanaMath.properties and STIX 1.0 beta ; add Latin Modern to the default fonts of nsMathMLChar (23.16 KB, patch)
2013-12-09 22:27 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 3: remove mathfontAsanaMath.properties and STIX 1.0 beta ; add Latin Modern to the default fonts of nsMathMLChar (23.20 KB, patch)
2013-12-12 11:04 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 1: add a gfxMathTable class to read the MATH table (30.59 KB, patch)
2013-12-12 11:05 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2: make nsMathMLChar use the MATH table (38.13 KB, patch)
2014-01-13 08:22 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 4 - tests (12.19 KB, patch)
2014-01-13 08:32 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 4 - tests (10.27 KB, patch)
2014-01-13 08:49 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 4 - tests (12.58 KB, patch)
2014-01-15 06:49 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 5 - OTS support (24.99 KB, patch)
2014-01-15 06:56 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 1: add a gfxMathTable class to read the MATH table (33.89 KB, patch)
2014-01-16 05:48 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2: make nsMathMLChar use the MATH table (38.72 KB, patch)
2014-01-16 05:50 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2: make nsMathMLChar use the MATH table (41.44 KB, patch)
2014-01-17 00:25 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 5 - OTS support (24.93 KB, patch)
2014-01-17 04:55 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2: make nsMathMLChar use the MATH table (42.21 KB, patch)
2014-01-18 05:02 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 1: add a gfxMathTable class to read the MATH table. r=jfkthame, b=407059 (34.38 KB, patch)
2014-02-12 01:51 PST, Frédéric Wang (:fredw)
jfkthame: review+
Details | Diff | Splinter Review
Part 1: add a gfxMathTable class to read the MATH table. r=jfkthame, b=407059 (35.03 KB, patch)
2014-02-12 10:55 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2: make nsMathMLChar use the MATH table. r=karl, b=407059 (40.19 KB, patch)
2014-02-12 10:56 PST, Frédéric Wang (:fredw)
karlt: review-
Details | Diff | Splinter Review
Part 3: remove mathfontAsanaMath.properties and STIX 1.0 beta ; add Latin Modern to the default fonts of nsMathMLChar. r=karl, b=407059 (22.43 KB, patch)
2014-02-12 10:57 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 4: reftest. r=karl, b=407059 (15.59 KB, patch)
2014-02-12 10:57 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 1: add a gfxMathTable class to read the MATH table. r=jfkthame, b=407059 (35.10 KB, patch)
2014-02-13 08:07 PST, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 5: reftest. r=karl, b=407059 (15.62 KB, patch)
2014-02-13 08:08 PST, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Part 1: add a gfxMathTable class to read the MATH table. (35.09 KB, patch)
2014-04-10 09:28 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2: make nsMathMLChar use the MATH table. (39.11 KB, patch)
2014-04-10 09:30 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2: make nsMathMLChar use the MATH table. (38.69 KB, patch)
2014-04-11 08:07 PDT, Frédéric Wang (:fredw)
karlt: review-
Details | Diff | Splinter Review
407059-4.diff (22.26 KB, patch)
2014-04-13 11:10 PDT, Frédéric Wang (:fredw)
karlt: review-
Details | Diff | Splinter Review
407059-4.diff (22.44 KB, patch)
2014-04-14 01:41 PDT, Frédéric Wang (:fredw)
jfkthame: review-
Details | Diff | Splinter Review
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators (21.46 KB, patch)
2014-04-14 06:01 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators (24.00 KB, patch)
2014-04-14 11:31 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators (27.13 KB, patch)
2014-04-15 00:46 PDT, Frédéric Wang (:fredw)
jfkthame: review+
Details | Diff | Splinter Review
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators (27.12 KB, patch)
2014-04-15 02:12 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 3: remove mathfontAsanaMath.properties and STIX 1.0 beta. r=karl, b=407059 (22.42 KB, patch)
2014-04-15 02:20 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Part 3: remove mathfontAsanaMath.properties and STIX 1.0 beta. (22.39 KB, patch)
2014-04-16 23:08 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Part 2: make nsMathMLChar use the MATH table. (37.93 KB, patch)
2014-04-17 00:49 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
minHeight (29.51 KB, image/png)
2014-04-17 03:06 PDT, Frédéric Wang (:fredw)
no flags Details
Part 5: reftest (15.91 KB, patch)
2014-04-17 08:31 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 5: reftest (15.94 KB, patch)
2014-04-18 08:13 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators (27.15 KB, patch)
2014-04-18 09:16 PDT, Frédéric Wang (:fredw)
jfkthame: review+
Details | Diff | Splinter Review
Part 1: add a gfxMathTable class to read the MATH table. (35.09 KB, patch)
2014-04-22 05:10 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 2: make nsMathMLChar use the MATH table. (37.93 KB, patch)
2014-04-22 05:10 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 3: remove mathfontAsanaMath.properties and STIX 1.0 beta (22.39 KB, patch)
2014-04-22 05:11 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators (27.15 KB, patch)
2014-04-22 05:12 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 5: reftest (15.94 KB, patch)
2014-04-22 05:12 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 6: Refactor the minHeight verification to prevent an ASSERT on Windows. (1.62 KB, patch)
2014-04-22 12:26 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
Part 2: make nsMathMLChar use the MATH table. (37.61 KB, patch)
2014-04-22 22:34 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Part 0: Remove the Truncate() calls from nsMathMLChar::StretchEnumContext (1.77 KB, patch)
2014-04-22 22:35 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review

Description Karl Tomlinson (:karlt) 2007-12-05 14:58:40 PST
Microsoft have used a MATH table as an extension to OpenType in their Cambria Math font, that contains much information about typesetting mathematics.

Notably it contains the information that we have stored in mathfontFONT.properties files for variants and parts of stretchy characters, but it provides glyph ids instead of character code points.

Being able to read the information from the font would be much better than maintaining our own tables.

The only other currently available font that I can find having variant and construction (from part) information in these tables is Asana Math.

STIX seem to have no intention at this stage of including this information in their fonts.  (See the response to the comment dated 19 November at http://www.stixfonts.org/feedback-glyphs.html)

In the case of Cambria Math, variant and part glyphs that do not have a corresponding Unicode code point have no entries in the cmaps, so we cannot use a character code point table in its current form to access these glyphs.

The best way to get detailed information on these tables is by emailing Sergey Malkin.  Implementations for accessing these tables exist in fontforge (http://fontforge.sourceforge.net/math.html) and XeTeX.
Comment 1 Karl Tomlinson (:karlt) 2011-06-12 21:54:06 PDT
The OpenType sanitizer library in gfx/ots currently strips unrecognized tables from downloaded/web fonts (but not system fonts).  This would include MATH table.

This could be resolved by adding MATH support to OTS, or perhaps a bypass could be added for MATH because system libraries are unlikely to use the MATH table and so a corrupt MATH table is unlikely to cause trouble.
Comment 2 Karl Tomlinson (:karlt) 2011-06-12 22:33:06 PDT
It seems that STIX do plan to add MATH table support to their fonts, so maybe one day we'll be able to remove our mathfontSTIX*.properties files:

"Version 1.1, which will include fonts packaged for use with Microsoft Office applications, is scheduled for release by the end of 2010."

A derivative already exists with MATH table support:
https://github.com/khaledhosny/xits-math/
Comment 3 sid.kshatriya 2011-07-05 22:34:01 PDT
This might be a useful resource for people wanting to implement OpenType MATH support:

http://www.ntg.nl/maps/38/03.pdf
Comment 4 Frédéric Wang (:fredw) 2012-03-15 03:43:29 PDT

http://www.gust.org.pl/projects/e-foundry/lm-math
Comment 5 Frédéric Wang (:fredw) 2013-12-06 12:14:53 PST
Created attachment 8343928 [details] [diff] [review]
Patch V1
Comment 6 Frédéric Wang (:fredw) 2013-12-06 12:15:44 PST
Created attachment 8343930 [details] [diff] [review]
Patch to disable the scaling correction
Comment 7 Frédéric Wang (:fredw) 2013-12-06 14:14:17 PST
Created attachment 8344010 [details] [diff] [review]
Patch V2

The previous patch had some errors with stretching by parts (I incorrectly copied the Python script that I wrote for MathJax).

This should now support size variants and stretching by parts. I've added options to the torture test: https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/MathML_Torture_Test

There are remaining issues, especially the advance width computation as I discussed on IRC with Karl.
Comment 8 Frédéric Wang (:fredw) 2013-12-07 15:48:12 PST
Created attachment 8344248 [details] [diff] [review]
Patch V3

So here is an updated patch. I think this is essentially ready for what we want in this bug. Some issues:

- bug 947650: excessive ascent/descent with some math fonts (not related to that patch, though)

- I can not get the advance width from the glyph index. I can create a gfxTextRun from the glyph index and then measure the bounding metrics as Karl suggested, but I must specify the advance width when creating the gfxTextRun and then this is the one returned by the MeasureText function! I need to study that more carefully and see if there are some APIs in gfx to do that (like those in the gfxHarfbuzzShaper class). At the moment I just use right bearing - max(0, left bearing) but this does not alway give good result (especially when positioning scripts on slanted integrals).

- For large op in display mode, I use the DisplayOperatorMinHeight parameter from the MATH Table to pick the right size variant. This is to avoid small integrals in the Gyre TeX fonts. However, with Asana and Cambria, this gives the largest size variant. When checking the values of Asana and Cambria with font forge, that seems to be the expected behavior so either I didn't understand the DisplayOperatorMinHeight parameter or this is the designers' choice for Asana and Cambria.

- This does not exactly do the layout indicated in the MATH table yet. In particular Karl suggested to create one buffer with all the glyphs rather than doing separate positioning, drawing & clipping for each glyph. I guess this would give more accurate positioning (especially bad alignment of connectors in some cases), will probably be faster and perhaps help to fix bug 827039. Also, I think we may want to remove the scale-stretchy fallback in some cases where the MATH table is used (for example to fix bug 403958).

- I need to do more code cleanup, verification and testing, write documentation and to split the patches so that it be easier to review...
Comment 9 Frédéric Wang (:fredw) 2013-12-07 15:55:02 PST
Created attachment 8344250 [details]
screenshot Asana

So here is a screenshot with the Asana fonts showing the two issues with large op in display mode and incorrect advance width.
Comment 10 Frédéric Wang (:fredw) 2013-12-07 16:13:46 PST
Created attachment 8344251 [details]
screenshot Gyre Pagella
Comment 11 Frédéric Wang (:fredw) 2013-12-07 16:14:29 PST
Created attachment 8344252 [details]
screenshot Asana (XeTeX)
Comment 12 Frédéric Wang (:fredw) 2013-12-07 16:14:51 PST
Created attachment 8344253 [details]
screenshot Gyre Pagella (XeTeX)
Comment 13 Khaled Hosny 2013-12-07 16:18:26 PST
(In reply to Frédéric Wang (:fredw) from comment #8)
> - For large op in display mode, I use the DisplayOperatorMinHeight parameter
> from the MATH Table to pick the right size variant. This is to avoid small
> integrals in the Gyre TeX fonts. However, with Asana and Cambria, this gives
> the largest size variant. When checking the values of Asana and Cambria with
> font forge, that seems to be the expected behavior so either I didn't
> understand the DisplayOperatorMinHeight parameter or this is the designers'
> choice for Asana and Cambria.

That is a known issue in both fonts, but MS implementation does not seem to use DisplayOperatorMinHeight so the issue does not show there, either that or we are not correctly understanding what this constant is for. XeTeX implementation has the following workaround:

h1:=get_ot_math_constant(cur_f,displayOperatorMinHeight);
if h1<(height(p)+depth(p))*5/4 then h1:=(height(p)+depth(p))*5/4;

(but may be you should ask Murray Sargent for an explanation).
Comment 14 Frédéric Wang (:fredw) 2013-12-07 16:38:29 PST
> h1:=get_ot_math_constant(cur_f,displayOperatorMinHeight);
> if h1<(height(p)+depth(p))*5/4 then h1:=(height(p)+depth(p))*5/4;

I'm not sure I understand this out of context, but does it mean taking only 4/5 of displayOperatorMinHeight?

> (but may be you should ask Murray Sargent for an explanation).

I'll do that.
Comment 15 Khaled Hosny 2013-12-07 17:01:23 PST
My bad. ‘p’ is the base glyph (the starting glyph) of the operator, so if DisplayOperatorMinHeight < 5/4 its height and depth, set it to that value.

Now I see that your issue is the reverse of what XeTeX code is trying to solve (one gets a small than desired integral by respecting DisplayOperatorMinHeight in Cambria Math, with that workaround the value gets upped from 2500 to 2771 so one gets the desired integral). So I suspect you are not getting the correct constant value from the font.
Comment 16 Frédéric Wang (:fredw) 2013-12-07 17:13:30 PST
Breakpoint 1, nsOpenTypeTable::GetDisplayOperatorMinHeight (this=0x7fffc04c3918, aAppUnitsPerGfxUnit=60) at /home/fred/src-obj/mozilla/src/mozilla/layout/mathml/nsMathMLChar.cpp:413
(gdb) p mFontEntry->GetDisplayOperatorMinHeight()
[Thread 0x7fffc03ff700 (LWP 12510) exited]
$1 = 2500

That seems to give the right value but then I multiply it by aAppUnitsPerGfxUnit to compare with the glyph's bounding metrics. That's probably where I do something wrong, I'll have to check that...
Comment 17 Khaled Hosny 2013-12-07 17:29:26 PST
The values are in EM units, if this helps.
Comment 18 Frédéric Wang (:fredw) 2013-12-08 01:57:17 PST
(In reply to Khaled Hosny from comment #17)
> The values are in EM units, if this helps.

OK, I was wondering why the code for the SVG glyphs checked the availability of the UnitsPerEm parameter in the Open Type head... Now I know why :-) So for Cambria I now get 2500/2048 = 1.220703125. Then I'm not yet 100% sure how to convert to something independent of the zoom level / font size, but I used the EmHeight() function mentioned in http://dxr.mozilla.org/mozilla-central/source/layout/mathml/nsMathMLFrame.h#290 (the current MathML code seems to use the font size, though...). Now I think I get more reasonable size for Cambria and Asana, although as you said the integrals are sometimes a bit small.
Comment 19 Frédéric Wang (:fredw) 2013-12-08 13:56:29 PST
Created attachment 8344397 [details] [diff] [review]
Patch V4
Comment 20 Frédéric Wang (:fredw) 2013-12-08 14:18:14 PST
Comment on attachment 8344397 [details] [diff] [review]
Patch V4

The remaining problems I'm trying to solve (see nsMathMLChar.cpp):

1) In nsOpenTypeTable::UpdateCache, I have a character and I want to get its glyph index. In order to do that, I create a gfxTextRun with the character and then read the glyph data. I'm wondering if I could just use gfxFont::GetGlyph (and thus avoid this aThebesContext parameter and extra work to handle the text run). However, I guess this method is ok for now.

2) In nsMathMLChar::MakeTextRun, I create a text run from a glyph index. Since I don't know its advance width, I set gfxTextRun::DetailedGlyph::mAdvance to zero. Now nsMathMLChar::MeasureGlyph uses that text run to determine the bounding metrics of the glyph. But the metrics.mAdvanceWidth returned is just the one I specified in the gfxTextRun::DetailedGlyph (here zero)! I'm wondering if I could just use gfxFont::GetGlyphWidth?

In both cases, this is for Open Type fonts (that have a MATH table). So while gfxFont::GetGlyph and gfxFont::GetGlyphWidth do not seem to be implemented on all fonts/platforms, perhaps they work for Open Type fonts? Or should I keep 1) like this and do some low-level operation for 2) in order to retrieve the advance width from the glyph index? Karl also said that the CMAP could just be used for 1).
Comment 21 Khaled Hosny 2013-12-08 14:22:04 PST
Shouldn’t GetDisplayOperatorMinHeight() be replaced with a more genera GetOpenTypeMathConstant() (or so) that that it usable in the (hopefully near) future when we start using other constants?
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-12-08 15:07:38 PST
(In reply to Frédéric Wang (:fredw) from comment #20)
> 1) In nsOpenTypeTable::UpdateCache, I have a character and I want to get its
> glyph index. In order to do that, I create a gfxTextRun with the character
> and then read the glyph data. I'm wondering if I could just use
> gfxFont::GetGlyph (and thus avoid this aThebesContext parameter and extra
> work to handle the text run). However, I guess this method is ok for now.

gfxFont::GetGlyph is only implemented for gfxFT2Fonts so we may as well ignore it.

> 2) In nsMathMLChar::MakeTextRun, I create a text run from a glyph index.
> Since I don't know its advance width, I set
> gfxTextRun::DetailedGlyph::mAdvance to zero. Now nsMathMLChar::MeasureGlyph
> uses that text run to determine the bounding metrics of the glyph. But the
> metrics.mAdvanceWidth returned is just the one I specified in the
> gfxTextRun::DetailedGlyph (here zero)! I'm wondering if I could just use
> gfxFont::GetGlyphWidth?

I think the clean way to do this is to expose an API on gfxFont that creates a gfxTextRun from a list of glyph IDs with x and y positions. This API would bypass shaping completely and just fill in the gfxTextRun manually. It would use cairo_glyph_extents internally to get the glyph advance widths. You would probably use it to create a gfxTextRun for a single glyph with position 0,0. Does that make sense?
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-12-08 18:33:03 PST
It would also be fine to have the API just take a single glyph ID and no position, since we don't have any uses for the list-of-positioned-glyphs API yet.
Comment 24 Jonathan Kew (:jfkthame) 2013-12-09 00:59:41 PST
(In reply to Frédéric Wang (:fredw) from comment #20)

> In both cases, this is for Open Type fonts (that have a MATH table). So
> while gfxFont::GetGlyph and gfxFont::GetGlyphWidth do not seem to be
> implemented on all fonts/platforms, perhaps they work for Open Type fonts?

Not on all platforms, no. As Roc said, GetGlyph() is only in the FT2 backend; and GetGlyphWidth() is not implemented on OS X, at least.

We could implement them more widely if necessary, but at present they're not intended for general use; they exist only to support the harfbuzz shaper, and are only implemented in cases where accessing the font tables directly is not adequate for some reason.
Comment 25 Jonathan Kew (:jfkthame) 2013-12-09 01:18:18 PST
(In reply to Frédéric Wang (:fredw) from comment #20)
> Comment on attachment 8344397 [details] [diff] [review]
> Patch V4
> 
> The remaining problems I'm trying to solve (see nsMathMLChar.cpp):
> 
> 1) In nsOpenTypeTable::UpdateCache, I have a character and I want to get its
> glyph index. In order to do that, I create a gfxTextRun with the character
> and then read the glyph data. I'm wondering if I could just use
> gfxFont::GetGlyph (and thus avoid this aThebesContext parameter and extra
> work to handle the text run). However, I guess this method is ok for now.
> 
> 2) In nsMathMLChar::MakeTextRun, I create a text run from a glyph index.
> Since I don't know its advance width, I set
> gfxTextRun::DetailedGlyph::mAdvance to zero. Now nsMathMLChar::MeasureGlyph
> uses that text run to determine the bounding metrics of the glyph. But the
> metrics.mAdvanceWidth returned is just the one I specified in the
> gfxTextRun::DetailedGlyph (here zero)!

Well, yes; if you told it the only glyph in the run has an advance of zero, then that's the advance it'll report.

Could you get the advance you actually want at the same time as you're getting the glyph id, in nsOpenTypeTable::UpdateCache()?
Comment 26 Frédéric Wang (:fredw) 2013-12-09 01:59:39 PST
Created attachment 8344535 [details]
subscript-placement.png

(In reply to Jonathan Kew (:jfkthame) from comment #25)
> (In reply to Frédéric Wang (:fredw) from comment #20)
> Could you get the advance you actually want at the same time as you're
> getting the glyph id, in nsOpenTypeTable::UpdateCache()?

If you mean from the MATH table, I think the advance is only in the direction of the stretching.

I think what roc proposed using cairo_glyph_extents will allow to get that advance. However using that advance did not really seem to help so far and actually made the rendering worse...

I know see that the placement of subscript is no longer correct with STIX and MathJax on Nightly so I suspect the regression of bug 945254 is involved here. So perhaps I can just use the bearings for now and we can fix that later with bug 945254 and better use of the MATH table parameters.

From left to right: MathJax, STIX General, Asana (Release) ; MathJax, STIX General, Asana (Nightly)
Comment 27 Jonathan Kew (:jfkthame) 2013-12-09 02:52:13 PST
(In reply to Frédéric Wang (:fredw) from comment #26)
> Created attachment 8344535 [details]
> subscript-placement.png
> 
> (In reply to Jonathan Kew (:jfkthame) from comment #25)
> > (In reply to Frédéric Wang (:fredw) from comment #20)
> > Could you get the advance you actually want at the same time as you're
> > getting the glyph id, in nsOpenTypeTable::UpdateCache()?
> 
> If you mean from the MATH table, I think the advance is only in the
> direction of the stretching.

No, not from the MATH table; I meant the normal advance of the glyph - the same thing GetGlyphWidth() would gives you in the case when it's implemented. I thought that's what you needed for putting the glyph directly into a textrun later. You could read that advance from the textrun in UpdateCache() at the same time as reading the glyph ID, and save them both together for later use.
Comment 28 Frédéric Wang (:fredw) 2013-12-09 03:31:52 PST
(In reply to Jonathan Kew (:jfkthame) from comment #27)
> No, not from the MATH table; I meant the normal advance of the glyph - the
> same thing GetGlyphWidth() would gives you in the case when it's
> implemented. I thought that's what you needed for putting the glyph directly
> into a textrun later. You could read that advance from the textrun in
> UpdateCache() at the same time as reading the glyph ID, and save them both
> together for later use.

OK, I misunderstood your question I thought you suggested it would help to get the advance width. I can probably do that, but I'm not sure caching is really useful with the current code, which does several calls to get each piece in each font tried. nsOpenTypeTable::UpdateCache only does the caching of the glyph id for the base character in the current font, which is the value passed to MATH table. I normally don't need the advance for this glyph. The glyph ids and metrics for the selected size variant or parts are stored on nsMathMLChar::mGlyphs and nsMathMLChar::mBmData once we decided which font will be used to draw the stretchy op. My problem is how to get the advance width of these glyphs once I know their ids.
Comment 29 Behdad Esfahbod 2013-12-09 03:39:32 PST
FWIW, I implemented MATH table in my fonttools/ttx at http://github.com/behdad/fonttools
Comment 30 Jonathan Kew (:jfkthame) 2013-12-09 04:04:52 PST
(In reply to Frédéric Wang (:fredw) from comment #28)
> (In reply to Jonathan Kew (:jfkthame) from comment #27)
> > No, not from the MATH table; I meant the normal advance of the glyph - the
> > same thing GetGlyphWidth() would gives you in the case when it's
> > implemented. I thought that's what you needed for putting the glyph directly
> > into a textrun later. You could read that advance from the textrun in
> > UpdateCache() at the same time as reading the glyph ID, and save them both
> > together for later use.
> 
> OK, I misunderstood your question I thought you suggested it would help to
> get the advance width. I can probably do that, but I'm not sure caching is
> really useful with the current code, which does several calls to get each
> piece in each font tried. nsOpenTypeTable::UpdateCache only does the caching
> of the glyph id for the base character in the current font, which is the
> value passed to MATH table. I normally don't need the advance for this
> glyph. The glyph ids and metrics for the selected size variant or parts are
> stored on nsMathMLChar::mGlyphs and nsMathMLChar::mBmData once we decided
> which font will be used to draw the stretchy op. My problem is how to get
> the advance width of these glyphs once I know their ids.

Ah, OK. Sorry, it wasn't clear to me that you needed advances for other glyphs than the one you're getting there - I haven't tried to understand all this code properly yet.

I wonder if perhaps the simplest thing would be for us to provide gfxFont::GetGlyph and GetGlyphWidth methods that are available across all platforms. This would basically mean moving code from gfxHarfBuzzShaper into gfxFont instead. We'd just have to be careful not to regress performance at all by introducing extra virtual calls or anything like that, but it should be fine.
Comment 31 Frédéric Wang (:fredw) 2013-12-09 04:44:35 PST
(In reply to Jonathan Kew (:jfkthame) from comment #30)
> Ah, OK. Sorry, it wasn't clear to me that you needed advances for other
> glyphs than the one you're getting there - I haven't tried to understand all
> this code properly yet.
> 
> I wonder if perhaps the simplest thing would be for us to provide
> gfxFont::GetGlyph and GetGlyphWidth methods that are available across all
> platforms. This would basically mean moving code from gfxHarfBuzzShaper into
> gfxFont instead. We'd just have to be careful not to regress performance at
> all by introducing extra virtual calls or anything like that, but it should
> be fine.

I think for now, I'll use the left/right bearing. At the moment I tried to keep the same measuring/positioning/drawing code for the old fonts with .properties file and the new fonts with MATH table. I expect in the future we could deprecate the old fonts and progressively move to the MATH table and improve, simplify the code and improve the rendering.

I have used XeTeX's idea to correct the display op min heigth, except that I replaced the 5/4 factor by sqrt(2) for large operators and by 2 for integrals, since that was what our scale workaround does.

I also followed the Khaled's suggestion for the math constants.

I'll try to split the patches and prepare them for a first review.
Comment 32 Frédéric Wang (:fredw) 2013-12-09 05:38:37 PST
Created attachment 8344593 [details] [diff] [review]
Part 1: add a gfxMathTable class to read the MATH table.
Comment 33 Frédéric Wang (:fredw) 2013-12-09 05:39:16 PST
Created attachment 8344594 [details] [diff] [review]
Part 2: add a gfxFont interface to the MATH table.
Comment 34 Frédéric Wang (:fredw) 2013-12-09 05:39:57 PST
Created attachment 8344595 [details] [diff] [review]
Part 3: make nsMathMLChar use the MATH table
Comment 35 Frédéric Wang (:fredw) 2013-12-09 05:40:57 PST
Created attachment 8344598 [details] [diff] [review]
Part 4: remove mathfontAsanaMath.properties and STIX 1.0 beta ; add Latin Modern to the default fonts of nsMathMLChar
Comment 36 Khaled Hosny 2013-12-09 06:01:28 PST
Comment on attachment 8344598 [details] [diff] [review]
Part 4: remove mathfontAsanaMath.properties and STIX 1.0 beta ; add Latin Modern to the default fonts of nsMathMLChar

>-pref("font.mathfont-family", "MathJax_Main, STIXNonUnicode, STIXSizeOneSym, STIXSize1, STIXGeneral, Asana Math, DejaVu Sans");
>+pref("font.mathfont-family", "Latin Modern, MathJax_Main, STIX Math, STIXNonUnicode, STIXSizeOneSym, STIXGeneral, Asana Math, DejaVu Sans");

Shouldn’t that be Latin Modern Math?
Comment 37 Frédéric Wang (:fredw) 2013-12-09 06:06:49 PST
(In reply to Khaled Hosny from comment #36)
> Shouldn’t that be Latin Modern Math?

oops, yes indeed...
Comment 38 Jonathan Kew (:jfkthame) 2013-12-09 06:39:44 PST
Comment on attachment 8344593 [details] [diff] [review]
Part 1: add a gfxMathTable class to read the MATH table.

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

::: gfx/thebes/MathTableStructures.h
@@ +9,5 @@
> +// will read the header of the structure first, verify that there is enough
> +// space for the specified arrays and then use a pointer to browse these arrays.
> +
> +typedef AutoSwap_PRUint16 Offset;
> +typedef AutoSwap_PRUint16 GlyphID;

I'd prefer to explicitly use mozilla::AutoSwap here in the header, so that this isn't dependent on being #included after a "using namespace mozilla;"

(And maybe you could use additional typedefs such as Percentage, FUnitsValue, and Count16 for many of the fields that are currently typed as simple AutoSwap_PRUint16, to make it more explicit what they represent.)

@@ +38,5 @@
> +  AutoSwap_PRUint16 mRangeCount;
> +  // RangeRecord    mRangeArray[mRangeCount];
> +};
> +
> +struct Header {

Could we call this MATHHeader or MATHTableHeader, please? The name "Header" seems a little too generic.

@@ +102,5 @@
> +  MathValueRecord   mRadicalExtraAscender;
> +  MathValueRecord   mRadicalKernBeforeDegree;
> +  MathValueRecord   mRadicalKernAfterDegree;
> +  AutoSwap_PRUint16 mRadicalDegreeBottomRaisePercent;
> +};

How about defining this as something like

  struct MathConstants {
    mozilla::AutoSwap_PRUint16 mSingleValues[gfxFontEntry::DisplayOperatorMinHeight -
                                             gfxFontEntry::ScriptPercentScaleDown + 1];
    MathValueRecord            mMathValues[gfxFontEntry::RadicalKernAfterDegree -
                                           gfxFontEntry::MathLeading + 1];
    mozilla::AutoSwap_PRUint16 mRadicalDegreeBottomRaisePercent;
  };

Then most of that huge switch() in gfxMathTable::GetMathConstant() could be replaced by a couple of array lookups. Just need to verify that the constants match up, but I assume you've kept the list in the same order!

::: gfx/thebes/gfxMathTable.cpp
@@ +24,5 @@
> +}
> +
> +gfxMathTable::~gfxMathTable()
> +{
> +    hb_blob_destroy(mMathTable);

Nit: the rest of this file uses 2-space indent.

::: gfx/thebes/gfxMathTable.h
@@ +78,5 @@
> +    // HarfBuzz blob where the MATH table is stored.
> +    hb_blob_t*    mMathTable;
> +
> +    // gfxFontEntry that holds this MATH table.
> +    gfxFontEntry* mFontEntry;

Why is this here? I don't think the MATH table cares what font entry it belongs to.

@@ +82,5 @@
> +    gfxFontEntry* mFontEntry;
> +
> +    // Pointer to the raw data and its size.
> +    const char*  mMathData;
> +    unsigned int mMathDataLength;

No need to store these either, just get them from the blob into local variables in the methods that need them. (Accessing them from the blob is very cheap.)
Comment 39 Jonathan Kew (:jfkthame) 2013-12-09 07:07:28 PST
Comment on attachment 8344594 [details] [diff] [review]
Part 2: add a gfxFont interface to the MATH table.

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

::: gfx/thebes/gfxFont.h
@@ +528,5 @@
>      nsAutoPtr<gfxSVGGlyphs> mSVGGlyphs;
>      // list of gfxFonts that are using SVG glyphs
>      nsTArray<gfxFont*> mFontsUsingSVGGlyphs;
> +    nsAutoPtr<gfxMathTable> mMathTable;
> +    nsTArray<gfxFont*> mFontsUsingMathTable;

I don't think you need to keep track of this, or have the associated Disconnect stuff, etc. We needed to do that for SVG glyphs in order to release all the SVG document stuff that hangs off them before other services are torn down, but for the MATH table there isn't anything of that nature.

The automatic deletion of the gfxMathTable when the gfxFontEntry goes away should be sufficient, I believe.
Comment 40 Frédéric Wang (:fredw) 2013-12-09 09:05:42 PST
Created attachment 8344709 [details] [diff] [review]
Part 1: add a gfxMathTable class to read the MATH table.

Merging parts 1 and 2.
Comment 41 Behdad Esfahbod 2013-12-09 16:15:14 PST
I can add the MATH table to harfbuzz itself, which simplifies the "Valid" checks.  I won't export any API, but you can include the hb-ot-math-table.hh directly and use it.
Comment 42 Frédéric Wang (:fredw) 2013-12-09 22:25:38 PST
Created attachment 8345139 [details] [diff] [review]
Part 2:  make nsMathMLChar use the MATH table

refresh after changes to bug 663740.
Comment 43 Frédéric Wang (:fredw) 2013-12-09 22:27:20 PST
Created attachment 8345140 [details] [diff] [review]
Part 3: remove mathfontAsanaMath.properties and STIX 1.0 beta ; add Latin Modern to the default fonts of nsMathMLChar

Update the patch to fix the "Latin Modern" mistake.
(Now, I'm wondering if we should wait further improvements to the new fonts before switching to them)
Comment 44 Frédéric Wang (:fredw) 2013-12-09 22:33:11 PST
(In reply to Behdad Esfahbod from comment #41)
> I can add the MATH table to harfbuzz itself, which simplifies the "Valid"
> checks.  I won't export any API, but you can include the hb-ot-math-table.hh
> directly and use it.

Yes, I guess the code could be simplified once the MATH table is added to harfbuzz itself. What is exactly the plan/schedule for that? It seems that Gecko includes its own copy of Harfbuzz in gfx/harfbuzz/
Comment 45 Behdad Esfahbod 2013-12-11 17:18:12 PST
(In reply to Frédéric Wang (:fredw) from comment #44)
> (In reply to Behdad Esfahbod from comment #41)
> > I can add the MATH table to harfbuzz itself, which simplifies the "Valid"
> > checks.  I won't export any API, but you can include the hb-ot-math-table.hh
> > directly and use it.
> 
> Yes, I guess the code could be simplified once the MATH table is added to
> harfbuzz itself. What is exactly the plan/schedule for that?

It's mostly mechanical.  So I can get that done fairly quick.

> It seems that
> Gecko includes its own copy of Harfbuzz in gfx/harfbuzz/

That part is fine, and a plus.  Though, it does raise an interesting question: what about platforms (Fedora, etc?) who build Firefox with system HarfBuzz?  I don't want to export MATH table API just yet as it's very low-level and of limited use.  Maybe it's not worth doing.  Jonathan, any input?
Comment 46 Jonathan Kew (:jfkthame) 2013-12-12 01:39:26 PST
(In reply to Behdad Esfahbod from comment #45)
> (In reply to Frédéric Wang (:fredw) from comment #44)
> > It seems that
> > Gecko includes its own copy of Harfbuzz in gfx/harfbuzz/
> 
> That part is fine, and a plus.  Though, it does raise an interesting
> question: what about platforms (Fedora, etc?) who build Firefox with system
> HarfBuzz?  I don't want to export MATH table API just yet as it's very
> low-level and of limited use.  Maybe it's not worth doing.  Jonathan, any
> input?

Are there platforms that do this? We don't have an --enable-system-harfbuzz option, AFAIK, so if anyone's doing that it must be an added hack of their own.
Comment 47 Behdad Esfahbod 2013-12-12 01:41:59 PST
(In reply to Jonathan Kew (:jfkthame) from comment #46)
> (In reply to Behdad Esfahbod from comment #45)
> > (In reply to Frédéric Wang (:fredw) from comment #44)
> > > It seems that
> > > Gecko includes its own copy of Harfbuzz in gfx/harfbuzz/
> > 
> > That part is fine, and a plus.  Though, it does raise an interesting
> > question: what about platforms (Fedora, etc?) who build Firefox with system
> > HarfBuzz?  I don't want to export MATH table API just yet as it's very
> > low-level and of limited use.  Maybe it's not worth doing.  Jonathan, any
> > input?
> 
> Are there platforms that do this? We don't have an --enable-system-harfbuzz
> option, AFAIK, so if anyone's doing that it must be an added hack of their
> own.

Ok, good to know.  I'll ask Fedora.  If anyone does it, it should be them.
Comment 48 Jonathan Kew (:jfkthame) 2013-12-12 05:15:53 PST
Comment on attachment 8344709 [details] [diff] [review]
Part 1: add a gfxMathTable class to read the MATH table.

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

::: gfx/thebes/gfxFont.cpp
@@ +391,5 @@
> +        // We don't use AutoTable here because we'll pass ownership of this
> +        // blob to the gfxMathTable, once we've confirmed the table exists
> +        hb_blob_t *mathTable = GetFontTable(TRUETYPE_TAG('M','A','T','H'));
> +        if (!mathTable) {
> +            return nullptr;

This should return false (not nullptr).

@@ +399,5 @@
> +        // with it.
> +        mMathTable = new gfxMathTable(mathTable, this);
> +        if (!mMathTable->HasValidHeaders()) {
> +            mMathTable = nullptr;
> +            return nullptr;

Ditto here.

::: gfx/thebes/gfxFont.h
@@ +312,5 @@
>      // (e.g. animated SVG glyphs)
>      void NotifyGlyphsChanged();
>  
> +    // Call TryGetMathTable to try to load the Open Type MATH table. The other
> +    // functions forward the call to the gfxMathTable class.

Add to the comment something like "The GetMath...() functions MUST NOT be called unless TryGetMathTable() has returned true."

@@ +314,5 @@
>  
> +    // Call TryGetMathTable to try to load the Open Type MATH table. The other
> +    // functions forward the call to the gfxMathTable class.
> +    bool     TryGetMathTable(gfxFont* aFont);
> +    enum MathConstant {

Add a comment noting that the order of the constants here MUST match the order of the fields defined in the MATH table (as we use the value as an array index when accessing them).

Oh, and this file uses 4-space indent (the older gfx style), so it'd be better to match that.

::: gfx/thebes/gfxMathTable.cpp
@@ +243,5 @@
> +const MATHTableHeader*
> +gfxMathTable::GetMATHTableHeader()
> +{
> +  unsigned int mathDataLength;
> +  const char* mathData = hb_blob_get_data(mMathTable, &mathDataLength);

You don't need the mathDataLength variable here; just pass nullptr as second param to hb_blob_get_data and it won't try to return the length.

@@ +251,5 @@
> +const MathConstants*
> +gfxMathTable::GetMathConstants()
> +{
> +  unsigned int mathDataLength;
> +  const char* mathData = hb_blob_get_data(mMathTable, &mathDataLength);

Ditto here.

@@ +261,5 @@
> +const MathVariants*
> +gfxMathTable::GetMathVariants()
> +{
> +  unsigned int mathDataLength;
> +  const char* mathData = hb_blob_get_data(mMathTable, &mathDataLength);

And here.

@@ +306,5 @@
> +      const GlyphID* glyphArray =
> +        reinterpret_cast<const GlyphID*>(start);
> +      for (uint16_t i = 0; i < count; i++) {
> +        if (uint16_t(glyphArray[i]) == aGlyph) {
> +          return i;

We should do a binary search here, not a linear scan of all the glyph IDs.

@@ +323,5 @@
> +      for (uint16_t i = 0; i < count; i++) {
> +        uint16_t rStart = rangeArray[i].mStart;
> +        uint16_t rEnd = rangeArray[i].mEnd;
> +        if (rStart <= aGlyph && aGlyph <= rEnd) {
> +          return uint16_t(rangeArray[i].mStartCoverageIndex) + aGlyph - rStart;

And here, do a binary search of the ranges.

(However, if Behdad provides MATH table support within harfbuzz shortly, we may be able to drop this altogether and just use the HB code.)

::: gfx/thebes/gfxMathTable.h
@@ +28,5 @@
> +     * The gfxMathTable object takes over ownership of the blob references
> +     * that are passed in, and will hb_blob_destroy() them when finished;
> +     * the caller should -not- destroy these references.
> +     */
> +    gfxMathTable(hb_blob_t* aMathTable, gfxFontEntry* aFontEntry);

The aFontEntry parameter here is unnecessary, afaics; let's get rid of it.
Comment 49 Behdad Esfahbod 2013-12-12 10:48:54 PST
I'll try to have something up early next week.
Comment 50 Frédéric Wang (:fredw) 2013-12-12 11:04:48 PST
Created attachment 8346697 [details] [diff] [review]
Part 3: remove mathfontAsanaMath.properties and STIX 1.0 beta ; add Latin Modern to the default fonts of nsMathMLChar
Comment 51 Frédéric Wang (:fredw) 2013-12-12 11:05:18 PST
Created attachment 8346699 [details] [diff] [review]
Part 1: add a gfxMathTable class to read the MATH table
Comment 52 Behdad Esfahbod 2013-12-13 14:26:33 PST
On second thought, Iven it's end of year and all, I probably won't get to do this in the next three weeks...
Comment 53 Frédéric Wang (:fredw) 2014-01-07 06:22:47 PST
(In reply to Behdad Esfahbod from comment #52)
> On second thought, Iven it's end of year and all, I probably won't get to do
> this in the next three weeks...

@Behdad: did you find time to work on this?
Comment 54 Behdad Esfahbod 2014-01-07 06:28:46 PST
Not yet.  I should.  Will reprioritize.
Comment 55 Frédéric Wang (:fredw) 2014-01-13 08:22:00 PST
Created attachment 8359269 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table

Refreshing patch.
Comment 56 Frédéric Wang (:fredw) 2014-01-13 08:32:22 PST
Created attachment 8359276 [details] [diff] [review]
Part 4 - tests

This adds a reftest together with a script to generate a custom font with a MATH table.

This seems to work if I install the custom font on my system but not when I use downloadable fonts. My understanding is that the parameters in gfxFontEntry::TryGetMathTable are cached so we only try to access the MATH table once and if the font is not available yet the first time, we never try it again later. I'll debug this further.
Comment 57 Frédéric Wang (:fredw) 2014-01-13 08:49:00 PST
Created attachment 8359284 [details] [diff] [review]
Part 4 - tests
Comment 58 Khaled Hosny 2014-01-13 15:16:41 PST
(In reply to Frédéric Wang (:fredw) from comment #56)
> Created attachment 8359276 [details] [diff] [review]
> Part 4 - tests
> 
> This adds a reftest together with a script to generate a custom font with a
> MATH table.
> 
> This seems to work if I install the custom font on my system but not when I
> use downloadable fonts.

I guess because OTS (the OpenType sanitizer used for downloadable fonts) strips out the MATH table as it does not understand it, or has it been updated to keep the MATH table?
Comment 59 Jonathan Kew (:jfkthame) 2014-01-13 16:49:06 PST
(In reply to Khaled Hosny from comment #58)
> (In reply to Frédéric Wang (:fredw) from comment #56)
> > Created attachment 8359276 [details] [diff] [review]
> > Part 4 - tests
> > 
> > This adds a reftest together with a script to generate a custom font with a
> > MATH table.
> > 
> > This seems to work if I install the custom font on my system but not when I
> > use downloadable fonts.
> 
> I guess because OTS (the OpenType sanitizer used for downloadable fonts)
> strips out the MATH table as it does not understand it, or has it been
> updated to keep the MATH table?

Correct: OTS drops all tables it doesn't specifically support, and 'MATH' (which is not currently part of any standard, although MS is now proposing its addition) is not supported.

See https://code.google.com/p/ots/wiki/IssueTracking for how to file an issue against OTS. (Better still if someone could submit a patch, of course!)

Meanwhile, if we're sufficiently confident that our code for handling the MATH table is robust against corrupt or malicious data, we could consider working around this by adding an "OTS bypass" code path for that table, so that it is preserved even if not actually sanitized.
Comment 60 Frédéric Wang (:fredw) 2014-01-14 02:30:00 PST
Thank you for the information about OTS. I already have the Chromium development environment installed on my system so I can try to write a patch and submit it to their review system this week.

Besides that, I was wondering what happens if the font is not downloaded yet (IIUC, when mProxy is true). gfxFontEntry::GetFontTable, gfxFontEntry::TryGetSVGData and the new gfxFontEntry::TryGetMathTable do not seem to handle that case and just cache the null pointer for the table and m*Initialized = true boolean, so it looks like trying to access the table again will fail when called after the font is downloaded. So I was thinking that the functions should return immediately without caching the data when mProxy is true...
Comment 61 Jonathan Kew (:jfkthame) 2014-01-14 07:55:24 PST
(In reply to Frédéric Wang (:fredw) from comment #60)

> Besides that, I was wondering what happens if the font is not downloaded yet
> (IIUC, when mProxy is true). gfxFontEntry::GetFontTable,
> gfxFontEntry::TryGetSVGData and the new gfxFontEntry::TryGetMathTable do not
> seem to handle that case and just cache the null pointer for the table and
> m*Initialized = true boolean, so it looks like trying to access the table
> again will fail when called after the font is downloaded. So I was thinking
> that the functions should return immediately without caching the data when
> mProxy is true...

When mProxy is true, we don't even attempt to instantiate a gfxFont object and actually use the font, but instead fall back to whatever's next in the font list. So I don't think this should be a concern, unless the math code is somehow bypassing the use of a gfxFont instance altogether.
Comment 62 Frédéric Wang (:fredw) 2014-01-14 10:06:50 PST
(In reply to Jonathan Kew (:jfkthame) from comment #59)
> See https://code.google.com/p/ots/wiki/IssueTracking for how to file an
> issue against OTS. (Better still if someone could submit a patch, of course!)

OK, I've opened an issue http://code.google.com/p/chromium/issues/detail?id=334318 and have an (untested) patch to sanitize the MATH table.
Comment 63 Frédéric Wang (:fredw) 2014-01-14 13:15:01 PST
(In reply to Jonathan Kew (:jfkthame) from comment #61)
> When mProxy is true, we don't even attempt to instantiate a gfxFont object
> and actually use the font, but instead fall back to whatever's next in the
> font list. So I don't think this should be a concern, unless the math code
> is somehow bypassing the use of a gfxFont instance altogether.

Indeed, the reftest seems to work if I make OTS accept the MATH table. I'll come back to this tomorrow.
Comment 64 Frédéric Wang (:fredw) 2014-01-15 06:49:04 PST
Created attachment 8360426 [details] [diff] [review]
Part 4 - tests
Comment 65 Frédéric Wang (:fredw) 2014-01-15 06:56:20 PST
Created attachment 8360429 [details] [diff] [review]
Part 5 - OTS support

Here is a patch to add the MATH support to OTS. I'll submit a version upstream and ask review there.

The version of the MATH spec I have does not say whether the offset MathKernInfo, MathItalicsCorrectionInfo and MathTopAccentAttachment may be NULL while it is explicitly say so for ExtendedShapeCoverage. However some fonts like Neo Euler, TeX Gyre *, Latin Modern and STIX don't have MathKernInfo. STIX also does not have MathItalicsCorrectionInfo. Hence I have modified the code to accept NULL offsets. With this version, all the math fonts mentioned on https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/MathML_Torture_Test (except Lucida that I don't have) are accepted by OTS.
Comment 66 Frédéric Wang (:fredw) 2014-01-15 11:06:32 PST
Comment on attachment 8359269 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table

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

::: layout/mathml/nsMathMLChar.cpp
@@ +1208,5 @@
>    bm.ascent = NSToCoordCeil(-metrics.mBoundingBox.Y());
>    bm.descent = NSToCoordCeil(metrics.mBoundingBox.YMost());
>    bm.width = NSToCoordRound(metrics.mAdvanceWidth);
> +  if (aGlyph.IsGlyphID()) {
> +    bm.width = bm.rightBearing - bm.leftBearing;

So I think one solution to avoid the script placement problem (in addition to fixing the regression 945254) would be to set  bm.width = bm.rightBearing - max(0, italicCorrection) here so that GetItalicCorrection() in nsMathMLFrame.h will return the correct value. The italic correction can be obtained from the MATH table. I'll try that tomorrow.
Comment 67 Khaled Hosny 2014-01-15 12:16:44 PST
(In reply to Frédéric Wang (:fredw) from comment #65)
> Created attachment 8360429 [details] [diff] [review]
> Part 5 - OTS support
> 
> Here is a patch to add the MATH support to OTS. I'll submit a version
> upstream and ask review there.
> 
> The version of the MATH spec I have does not say whether the offset
> MathKernInfo, MathItalicsCorrectionInfo and MathTopAccentAttachment may be
> NULL while it is explicitly say so for ExtendedShapeCoverage. However some
> fonts like Neo Euler, TeX Gyre *, Latin Modern and STIX don't have
> MathKernInfo. STIX also does not have MathItalicsCorrectionInfo. Hence I
> have modified the code to accept NULL offsets. With this version, all the
> math fonts mentioned on
> https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/
> MathML_Torture_Test (except Lucida that I don't have) are accepted by OTS.


I think we should just whitelist MATH table since we are not passing it to any OS libraries. On the other hand, FreeType has a MATH table validator, you may want to check it for inspirations.
Comment 68 Frédéric Wang (:fredw) 2014-01-15 23:14:52 PST
(In reply to Khaled Hosny from comment #67)
> I think we should just whitelist MATH table since we are not passing it to
> any OS libraries. On the other hand, FreeType has a MATH table validator,
> you may want to check it for inspirations.

Kunihiko Sakamoto said he will take a look at my code this week, but otherwise we could just whitelist the MATH table. I just had a look at the FreeType validator and it also accepts NULL offsets for MathItalicsCorrectionInfo, MathTopAccentAttachment and MathKernInfo.
Comment 69 Behdad Esfahbod 2014-01-15 23:17:19 PST
(In reply to Frédéric Wang (:fredw) from comment #68)
> (In reply to Khaled Hosny from comment #67)
> > I think we should just whitelist MATH table since we are not passing it to
> > any OS libraries. On the other hand, FreeType has a MATH table validator,
> > you may want to check it for inspirations.
> 
> Kunihiko Sakamoto said he will take a look at my code this week, but
> otherwise we could just whitelist the MATH table. I just had a look at the
> FreeType validator and it also accepts NULL offsets for
> MathItalicsCorrectionInfo, MathTopAccentAttachment and MathKernInfo.

My take on this:

1. Any offset in GSUB/GPOS/GDEF/MATH/BASE can be null,
2. FreeType otvalid code has got zero use in real world, so what you've got is already better tested than that.
Comment 70 Frédéric Wang (:fredw) 2014-01-16 02:57:03 PST
(In reply to Frédéric Wang (:fredw) from comment #66)
> So I think one solution to avoid the script placement problem (in addition
> to fixing the regression 945254) would be to set  bm.width = bm.rightBearing
> - max(0, italicCorrection) here so that GetItalicCorrection() in
> nsMathMLFrame.h will return the correct value. The italic correction can be
> obtained from the MATH table. I'll try that tomorrow.

I tried that this morning and that seems to work. The subscript is sometimes placed very close to the operator, but let's improve that later (probably the right thing would be to follow the MATH spec: get the advance width and shift the sub/sup script by minus/plus half the italic correction). Unfortunately, this also does not work with the STIX-Word fonts because the italic correction table is not available as I previously said (verified with version 1.1.1).

Let's take bug 945254 before, because it's a small patch that fixes a regression with the MathJax / STIX-General fonts and could be ready for Gecko 29, while I'm not sure this work will have been reviewed.
Comment 71 Khaled Hosny 2014-01-16 03:13:27 PST
I have not been following closely, but please note that in MATH fonts the application of italic correction is reversed for big operators, i.e. the superscript is placed immediately after the operator, while the subscript is placed by *subtracting* italic correction from the operator advance width (I never understood the rationale behind this, but this how Word works and how Cambria is designed, and I later confirmed it with Murray Sargent).
Comment 72 Frédéric Wang (:fredw) 2014-01-16 03:25:22 PST
From the MATH spec:

"When positioning limits on an N-ary operator (e.g., integral sign),
the horizontal position of the upper limit is moved to the right by
1⁄2 of the italics correction, while the position of the lower limit is
moved to the left by the same distance."

Although I'm not sure from which position we move by plus/minus the half of the italic correction...

At the moment, the code has the left/right bearing of the glyph but not the advance width. So I determine it with

advanceWidth = max(0, rightBearing - italicCorrection)

so that GetItalicCorrection in nsMathMLFrame will return the right italic correction (when advanceWidth >= 0) and mmultiscripts with use that value to place the scripts with its current method (+ your patch).

Alternatively, I could perhaps get the advance width by the methods previously discussed in this bug and then we could use the italic correction directly in the mmultiscripts code.
Comment 73 Khaled Hosny 2014-01-16 04:05:43 PST
(In reply to Frédéric Wang (:fredw) from comment #72)
> From the MATH spec:
> 
> "When positioning limits on an N-ary operator (e.g., integral sign),
> the horizontal position of the upper limit is moved to the right by
> 1⁄2 of the italics correction, while the position of the lower limit is
> moved to the left by the same distance."
> 
> Although I'm not sure from which position we move by plus/minus the half of
> the italic correction...

This applies to placing the limits above an below the operator; you first enter the limits in the operator advance width (without the italic correction), then moved to right or left as described, just like TeX does it.

What I’m talking about is when the limits are placed as super/subscripts.

> At the moment, the code has the left/right bearing of the glyph but not the
> advance width. So I determine it with
> 
> advanceWidth = max(0, rightBearing - italicCorrection)
> 
> so that GetItalicCorrection in nsMathMLFrame will return the right italic
> correction (when advanceWidth >= 0) and mmultiscripts with use that value to
> place the scripts with its current method (+ your patch).
> 
> Alternatively, I could perhaps get the advance width by the methods
> previously discussed in this bug and then we could use the italic correction
> directly in the mmultiscripts code.

I think this would be cleaner approach.
Comment 74 Khaled Hosny 2014-01-16 04:07:48 PST
BTW, STIX Math font is just broken here, it renders wrong with MS and other MATH table implementations as well.
Comment 75 Frédéric Wang (:fredw) 2014-01-16 05:48:19 PST
Created attachment 8361060 [details] [diff] [review]
Part 1: add a gfxMathTable class to read the MATH table
Comment 76 Frédéric Wang (:fredw) 2014-01-16 05:50:46 PST
Created attachment 8361061 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table
Comment 77 Frédéric Wang (:fredw) 2014-01-17 00:25:19 PST
Created attachment 8361550 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table

Another version where I use GetGlyphWidth to determine the advance width of the glyph and I still substract the italic correction given in the MATH table for large operators.

For STIX-Word fonts, since the advance width for integrals is at the level of the subscript position and not of the superscript position, and since the italic correction is not provided, this ends up giving the expected result (as with STIX-General).

This also still works for other MATH fonts.
Comment 78 Frédéric Wang (:fredw) 2014-01-17 04:55:17 PST
Created attachment 8361636 [details] [diff] [review]
Part 5 - OTS support

I got upstream approval with only minor review comments (mostly formatting). Here is the latest version of the patch, which addresses these comments. Also this should fix a build failure on B2G and Android.
Comment 79 Frédéric Wang (:fredw) 2014-01-18 05:02:03 PST
Created attachment 8362094 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table

Just a small change To prepare for bug 960115 and disable the scaling correction when a variant or construction is found. The scale is still use for mirroring (bug 945183)
Comment 80 Frédéric Wang (:fredw) 2014-01-23 08:32:18 PST
(In reply to Khaled Hosny from comment #73)
> > Alternatively, I could perhaps get the advance width by the methods
> > previously discussed in this bug and then we could use the italic correction
> > directly in the mmultiscripts code.
> 
> I think this would be cleaner approach.

I'll postpone that to bug 961482 where we will need a general mechanism to extract the italic correction and kerning of MathML elements. In particular for stretchy <mo>, nsMathMLChar should expose these values for the selected variant or for the GlyphAssembly.
Comment 82 Frédéric Wang (:fredw) 2014-02-12 01:51:01 PST
Created attachment 8374710 [details] [diff] [review]
Part 1: add a gfxMathTable class to read the MATH table. r=jfkthame, b=407059
Comment 83 Jonathan Kew (:jfkthame) 2014-02-12 05:24:05 PST
Comment on attachment 8374710 [details] [diff] [review]
Part 1: add a gfxMathTable class to read the MATH table. r=jfkthame, b=407059

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

This looks fine; some minor cosmetic suggestions below. r=me with these cleaned up.

Also note that if/when Behdad gives us equivalent API within harfbuzz, we'll probably want to switch to using that instead. (E.g. harfbuzz already has code to look up glyphs in Coverage tables, which is duplicated here; but the HB code isn't public at this point, so for now this is OK.)

::: gfx/thebes/MathTableStructures.h
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Add an #include guard around this file.

Please add an #include "gfxFontUtils.h" here, as that's where the AutoSwap types are defined. I realize it's currently redundant, as gfxMathTable ends up including it indirectly anyhow, but it seems the correct thing to do IMO.

::: gfx/thebes/gfxFont.h
@@ +316,5 @@
> +    // Call TryGetMathTable to try to load the Open Type MATH table. The other
> +    // functions forward the call to the gfxMathTable class. The GetMath...()
> +    // functions MUST NOT be called unless TryGetMathTable() has returned true.
> +    bool     TryGetMathTable(gfxFont* aFont);
> +    enum MathConstant {

This is purely a cosmetic nit, but I'd prefer the big enum to be moved above TryGetMathTable and its comment, with a blank line separating; then all the actual functions will be nicely together. At the moment, TryGetMathTable itself gets rather lost in the clutter.

::: gfx/thebes/gfxMathTable.cpp
@@ +4,5 @@
> +
> +#include "gfxMathTable.h"
> +
> +#include "MathTableStructures.h"
> +#include "gfxFont.h"

I don't think you need gfxFont.h here, as gfxMathTable.h already includes it.

@@ +13,5 @@
> +
> +gfxMathTable::gfxMathTable(hb_blob_t* aMathTable)
> +  : mMathTable(aMathTable)
> +  , mGlyphConstruction(nullptr)
> +  , mGlyphID(0)

Might be better to initialize mGlyphID to uint32_t(-1), which is more clearly -never- a valid glyph ID. (Although we shouldn't be trying to look up info for glyph 0, so in practice I don't expect it to matter.)

@@ +104,5 @@
> +    return false;
> +  }
> +  start += offset;
> +
> +  // Verify the validy of the MathItalicsCorrectionInfo and retrieve it.

s/validy/validity/

@@ +110,5 @@
> +    return false;
> +  }
> +  const MathItalicsCorrectionInfo* italicsCorrectionInfo =
> +    reinterpret_cast<const MathItalicsCorrectionInfo*>(start);
> +  

nit: trailing whitespace

@@ +180,5 @@
> +
> +  // XXXfredw The structure of the Open Type Math table is a bit more general
> +  // than the one currently used by the nsMathMLChar code, so we try to fallback
> +  // in reasonable way. We use the approach of the copyComponents function in
> +  // github.com/mathjax/MathJax-dev/blob/master/fonts/OpenTypeMath/fontUtil.py 

nit: trailing space

@@ +187,5 @@
> +  // aGlyphs[1] and aGlyphs[2]) and the extenders between these pieces should
> +  // all be the same (aGlyphs[4]). Also, the parts of vertical assembly are
> +  // stored from bottom to top in the Open Type MATH table while they are
> +  // stored from top to bottom in nsMathMLChar.
> +  //

delete the spare // line

@@ +215,5 @@
> +  // First extender char found.
> +  uint32_t extenderChar = 0;
> +
> +  // Clear the aGlyphs table.
> +  aGlyphs[0] = aGlyphs[1] = aGlyphs[2] = aGlyphs[3] = 0;

memset()?

@@ +347,5 @@
> +    return nullptr;
> +  }
> +  start += offset;
> +
> +  // Verify the validy of the GlyphAssembly and return it.

s/validy/validity/

@@ +452,5 @@
> +  // Make mGlyphConstruction point to the desired glyph construction.
> +  start = reinterpret_cast<const char*>(mathvariants);
> +  if (!ValidOffset(start, offset)) {
> +    return;
> +  }

I think you should check the validity of the MathGlyphConstruction record here - ensure it will be safe for the caller to at least read the mVariantCount and mGlyphAssembly fields. Or is that checked elsewhere, and I overlooked it?

::: gfx/thebes/gfxMathTable.h
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef GFX_MATH_TABLE_WRAPPER_H
> +#define GFX_MATH_TABLE_WRAPPER_H

The convention (at least in gfx/thebes) would be to name the guard GFX_MATH_TABLE_H; drop the WRAPPER.

@@ +4,5 @@
> +
> +#ifndef GFX_MATH_TABLE_WRAPPER_H
> +#define GFX_MATH_TABLE_WRAPPER_H
> +
> +#include "gfxFontUtils.h"

I think this is redundant - I don't think there's anything below that requires it (AFAICS).

@@ +27,5 @@
> +     * @param aMathTable The MATH table from the OpenType font
> +     *
> +     * The gfxMathTable object takes over ownership of the blob references
> +     * that are passed in, and will hb_blob_destroy() them when finished;
> +     * the caller should -not- destroy these references.

comment nit: there's only one reference passed in, not plural!

@@ +32,5 @@
> +     */
> +    gfxMathTable(hb_blob_t* aMathTable);
> +
> +    /**
> +     * Releases our references to the Math table and cleans up everything else.

s/references/reference/
s/Math/MATH/

@@ +74,5 @@
> +     * the format is not supported by the nsMathMLChar code.
> +     *
> +     */
> +    bool GetMathVariantsParts(uint32_t aGlyphID, bool aVertical,
> +                              uint32_t* aGlyphs);

Can we use "uint32_t aGlyphs[4]" here, to force the caller to pass a correctly-sized buffer? (This would need corresponding changes up through callers as well, of course.)

If that's problematic for the higher-level code, though, I won't worry too much about it.

@@ +88,5 @@
> +    hb_blob_t*    mMathTable;
> +
> +    // Cached values for the latest (mGlyphID, mVertical) pair that has been
> +    // accessed and the corresponding glyph construction. These are verified
> +    // by SelectGlyphConstruction and updated if necessary.

I realize all this is internal, but more documentation wouldn't hurt:
mGlyphConstruction will be set to nullptr if no construction is defined for the glyph. If non-null, its mGlyphAssembly and mVariantCount fields may be safely read, but no further validation will have been done.

@@ +94,5 @@
> +    uint32_t mGlyphID;
> +    bool     mVertical;
> +    void     SelectGlyphConstruction(uint32_t aGlyphID, bool aVertical);
> +
> +    // Access to some structures of the MATH table.

Add a note to remind us that these accessors just return a pointer, but do NOT themselves check the validity of anything. Until we've checked that HasValidHeaders (which does validate them) returns true, they might return pointers that cannot even safely be dereferenced.

@@ +99,5 @@
> +    const MATHTableHeader* GetMATHTableHeader();
> +    const MathConstants*   GetMathConstants();
> +    const MathGlyphInfo*   GetMathGlyphInfo();
> +    const MathVariants*    GetMathVariants();
> +    const GlyphAssembly*   GetGlyphAssembly(uint32_t aGlyphID, bool aVertical);

Add note that GetGlyphAssembly may return nullptr (if the given glyph has no assembly defined).
Comment 84 Frédéric Wang (:fredw) 2014-02-12 10:55:47 PST
Created attachment 8374988 [details] [diff] [review]
Part 1: add a gfxMathTable class to read the MATH table. r=jfkthame, b=407059
Comment 85 Frédéric Wang (:fredw) 2014-02-12 10:56:16 PST
Created attachment 8374990 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table. r=karl, b=407059
Comment 86 Frédéric Wang (:fredw) 2014-02-12 10:57:05 PST
Created attachment 8374991 [details] [diff] [review]
Part 3: remove mathfontAsanaMath.properties and STIX 1.0 beta ; add Latin Modern to the default fonts of nsMathMLChar. r=karl, b=407059
Comment 87 Frédéric Wang (:fredw) 2014-02-12 10:57:48 PST
Created attachment 8374993 [details] [diff] [review]
Part 4: reftest. r=karl, b=407059
Comment 88 Frédéric Wang (:fredw) 2014-02-13 03:23:50 PST
Comment on attachment 8374988 [details] [diff] [review]
Part 1: add a gfxMathTable class to read the MATH table. r=jfkthame, b=407059

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

::: gfx/thebes/gfxMathTable.cpp
@@ +78,5 @@
> +    return int16_t(mathconstants->mInt16[aConstant]);
> +  }
> +
> +  if (aConstant <= gfxFontEntry::DisplayOperatorMinHeight) {
> +    return uint16_t(mathconstants->mUint16[aConstant]);

This should be aConstant - DelimitedSubFormulaMinHeight
Comment 89 Jonathan Kew (:jfkthame) 2014-02-13 06:12:25 PST
(In reply to Frédéric Wang (:fredw) from comment #88)
> Comment on attachment 8374988 [details] [diff] [review]
> Part 1: add a gfxMathTable class to read the MATH table. r=jfkthame, b=407059
> 
> Review of attachment 8374988 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxMathTable.cpp
> @@ +78,5 @@
> > +    return int16_t(mathconstants->mInt16[aConstant]);
> > +  }
> > +
> > +  if (aConstant <= gfxFontEntry::DisplayOperatorMinHeight) {
> > +    return uint16_t(mathconstants->mUint16[aConstant]);
> 
> This should be aConstant - DelimitedSubFormulaMinHeight

Indeed it should - good catch. I imagine you can get some odd effects otherwise!
Comment 90 Frédéric Wang (:fredw) 2014-02-13 06:24:38 PST
(In reply to Jonathan Kew (:jfkthame) from comment #89)
> Indeed it should - good catch. I imagine you can get some odd effects
> otherwise!

I think only DisplayOperatorMinHeight is affected (it's not clear how DelimitedSubFormulaMinHeight  should be used). The test for DisplayOperatorMinHeight is currently a bit lax ; I thought I couldn't do better because of rounding approximation, but that might just be that error.
Comment 91 Frédéric Wang (:fredw) 2014-02-13 08:07:27 PST
Created attachment 8375554 [details] [diff] [review]
Part 1: add a gfxMathTable class to read the MATH table. r=jfkthame, b=407059

OK, that works better with the correct value :-)
Comment 92 Frédéric Wang (:fredw) 2014-02-13 08:08:46 PST
Created attachment 8375556 [details] [diff] [review]
Part 5: reftest. r=karl, b=407059

Just making the reftest a bit more strict...
Comment 93 Frédéric Wang (:fredw) 2014-02-13 11:30:25 PST
There are some test failures on Windows:
https://tbpl.mozilla.org/?tree=Try&rev=7a89fce35336

- small shifts in script placements (math-display.html)
- crash (opentype-stretchy.html)
Comment 94 Frédéric Wang (:fredw) 2014-03-30 09:05:51 PDT
@karl: any update on the review?
Comment 95 Karl Tomlinson (:karlt) 2014-04-09 23:29:19 PDT
Comment on attachment 8375554 [details] [diff] [review]
Part 1: add a gfxMathTable class to read the MATH table. r=jfkthame, b=407059

>+gfxFontEntry::GetMathItalicCorrection(uint32_t aGlyphID,
>+                                      gfxFloat* aItalicCorrection)
>+{
>+    NS_ASSERTION(mMathTable, "Math data has not yet been loaded. TryGetMathData() first.");
>+    int16_t italicCorrection;
>+    if (!mMathTable->GetMathItalicCorrection(aGlyphID, &italicCorrection)) {

Can these functions be named GetMathItalicsCorrection for consistency with the table names please?
That makes it easier to cross-search for the relevant parts of spec or interface.
Comment 96 Karl Tomlinson (:karlt) 2014-04-09 23:29:52 PDT
Comment on attachment 8374990 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table. r=karl, b=407059

>+  explicit nsGlyphTable()
>+    : mCharCache(0) {
>+    MOZ_COUNT_CTOR(nsGlyphTable);
>+  }
>+  virtual ~nsGlyphTable()
>+  {
>+    MOZ_COUNT_DTOR(nsGlyphTable);
>+  }
>+
>+  virtual const nsAString&
>+  FontNameFor(const nsGlyphCode& aGlyphCode) const = 0;

No need for MOZ_COUNT_* on this abstract class because it can't be
instantiated.

Making the constructor protected instead of public might make this more
explicit.

The "explicit" keyword is usually only used for conversion constructors -
those with one argument.

I think the explicit can be dropped here...

>+  explicit nsPropertiesTable(const nsString& aPrimaryFontName)
>+    : nsGlyphTable()

>+  explicit nsOpenTypeTable(gfxFontEntry* aFontEntry)
>+    : nsGlyphTable(),

And then the "nsGlyphTable()," is not required in the derived classes.

>+gfxTextRun*
>+nsGlyphTable::MakeTextRun(gfxContext*        aThebesContext,
>+                          int32_t            aAppUnitsPerDevPixel,
>+                          gfxFontGroup*      aFontGroup,
>+                          const nsGlyphCode& aGlyph)
>+{
>+  if (!aGlyph.IsGlyphID()) {
>+    return aFontGroup->
>+      MakeTextRun(aGlyph.code, aGlyph.Length(), aThebesContext,
>+                  aAppUnitsPerDevPixel, 0);
>+  }
>+
>+  gfxTextRunFactory::Parameters params = {

The concrete derived classes already implement many virtual functions.
Implementing this method on the base class and essentially switching according
to the derived class type is a different approach.

It would be more consistent to implement this as a virtual function on each of
the derived classes.  It think that would help clarify what is expected to
happen.

Perhaps there may be a small advantage in testing a condition over calling a
virtual function, but that won't make much difference here.

>+// An instance of nsPropertiesTable is associated to one primary font. Extra

>+  // mGlyphCache is a buffer containing the glyph data associated to mCharCache.

>+// An instance of nsOpenTypeTable is associated to one gfxFontEntry that

English uses "associated *with*".

>+  // allocate a table
>+  glyphTable = mOpenTypeTableList.AppendElement(aFont->GetFontEntry());

The opentype tables are different from the properties tables because the list
of tables can continue to grow indefinitely because each web font will have a
different font entry.  The font entry references in these tables will cause
the web fonts to leak.

An opentype table doesn't do much caching (most of that is on the font entry,
I assume), so a new opentype table can be created each time.

>+    // If the font contains an Open Type MATH table, use it.
>+    glyphTable = gGlyphTableList->GetGlyphTableFor(fontGroup->GetFontAt(0));

A static method such as OpenTypeTable::Create(gfxFont*) could be used here.

The difficult part then is managing the lifetimes of properties and opentype
tables differently.  OpenTypeTables need to be released after use.
mTablesTried adds to the difficulties there.

Possible solutions are:

1) Tracking existing opentype tables separately.

2) Changing mTablesTried to a list of families or some special name (blank)
   for generic.

3) Making nsGlyphTable reference counted.

4) Making GetGlyphTableFor(const nsAString& aFamily) return a wrapper that can
   be deleted and has a pointer into the properties table list.

>+  gfxTextRun::DetailedGlyph detailedGlyph;
>+  detailedGlyph.mGlyphID = aGlyph.glyphID;
>+  if (aFontGroup->GetFontAt(0)->ProvidesGlyphWidths()) {
>+    detailedGlyph.mAdvance = aAppUnitsPerDevPixel *
>+      aFontGroup->GetFontAt(0)->GetGlyphWidth(aThebesContext, aGlyph.glyphID) /
>+      65536.0;
>+  } else {
>+    // Set the advance width to zero. This will be fixed in MeasureTextRun.
>+    // XXXfredw: Is it possible that an Open Type MATH font does not provide
>+    // glyph widths? If not, then this and the hack in MeasureTextRun can be
>+    // removed.
>+    detailedGlyph.mAdvance = 0;
>+  }
>+  detailedGlyph.mXOffset = detailedGlyph.mYOffset = 0;

If the second path is good enough for some platforms, then we should use it on
all platforms.  I'm concerned that this is being developed and tested on one
kind of platform but might be behaving differently on another.

I think we can use gfxHarfbuzzShaper::GetGlyphHAdvance() if we do something to
ensure the shaper is suitably initialized.  That should be another patch and
can be done later if the second path is good enough here.

>+  if (bm.width == 0) {
>+    // We assume that this happens because ProvidesGlyphWidths() was not
>+    // available in nsGlyphTable::MakeTextRun and we had to set the advance
>+    // width to zero. Try to fix it here.
>+    bm.width = bm.rightBearing - bm.leftBearing;
>+  }

width and rightBearing are both measured from the origin not from leftBearing,
so leftBearing should not be involved here.

>+  nsGlyphCode ch;
>+  nscoord displayOperatorMinHeight = 0;
>+  if (largeopOnly) {
>+    ch = aGlyphTable->BigOf(mThebesContext, oneDevPixel, *aFontGroup, uchar,
>+                            isVertical, 0);
>+    if (!mChar->SetFontFamily(mPresContext, aGlyphTable, ch, aFamily, font,
>+                              aFontGroup)) {
>+      return false;
>+    }

STIXSizeOneSym has some missing entries for size0.
Isn't that a problem here?

>+    gfxFont* mathFont = aFontGroup->get()->GetFontAt(0);
>+    if (mathFont->GetFontEntry()->TryGetMathTable(mathFont)) {
>+      displayOperatorMinHeight =
>+        NSToCoordRound(mathFont->GetFontEntry()->
>+                       GetMathConstant(gfxFontEntry::DisplayOperatorMinHeight) *
>+                       mathFont->GetAdjustedSize() * oneDevPixel);
>+    }
>+    nsAutoPtr<gfxTextRun> textRun;
>+    textRun = aGlyphTable->MakeTextRun(mThebesContext, oneDevPixel, *aFontGroup,
>+                                       ch);
>+    nsBoundingMetrics bm = MeasureTextRun(mThebesContext, textRun);
>+    float largeopFactor = kLargeOpFactor;
>+    if (NS_STRETCH_INTEGRAL & mStretchHint) {
>+      // integrals are drawn taller
>+      largeopFactor = kIntegralFactor;
>+    }
>+    nscoord minHeight = largeopFactor * (bm.ascent + bm.descent);
>+    if (displayOperatorMinHeight < minHeight) {
>+      displayOperatorMinHeight = minHeight;
>+    }
>+  }

An assert that this is a vertical stretch would be helpful, I think.

If the font has a displayOperatorMinHeight, then do we need to still measure
the base glyph and use largeopFactor?

This is saying that a MATH table can only use a large op *larger* than our
fallback.

Asking for an operator strictly larger than the fallback may also select extra
large variants for STIX.  What have you checked to see the effect this has
there?

>+  if (glyphFound) {
>+    return NS_OK;
>+  }

>-  if (!glyphFound && largeop) {
>+  if (largeop) {

These changes need to be in a different patch, please, perhaps even a
different bug, so that we can track what is changing and the reasons.
Comment 97 Karl Tomlinson (:karlt) 2014-04-10 00:22:30 PDT
Comment on attachment 8374990 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table. r=karl, b=407059

(In reply to Khaled Hosny from comment #71)
> I have not been following closely, but please note that in MATH fonts the
> application of italic correction is reversed for big operators, i.e. the
> superscript is placed immediately after the operator, while the subscript is
> placed by *subtracting* italic correction from the operator advance width (I
> never understood the rationale behind this, but this how Word works and how
> Cambria is designed, and I later confirmed it with Murray Sargent).

Glyphs don't know whether they are large operator variants or stretchy
operator variants or base size operators.

I assume you are referring to all large size variants, here, but in any sane
world ItalicsCorrection would be applied similarly in all situations,
including at the base size, which I assume is as described as:

"• Set the default horizontal position for the subscript immediately after
the base glyph.
• Set the default horizontal position for the superscript as shifted relative
to the position of the subscript by the italics correction of the base
glyph."

This is consistent with http://fontforge.org/math.html.

Are you sure that Murray didn't mean that the ItalicsCorrection from the
GlyphAssembly Table is different?

Would you mind emailing to me exactly what Murray said, please?

(In reply to Frédéric Wang (:fredw) from comment #72)
> From the MATH spec:
> 
> "When positioning limits on an N-ary operator (e.g., integral sign),
> the horizontal position of the upper limit is moved to the right by
> 1⁄2 of the italics correction, while the position of the lower limit is
> moved to the left by the same distance."
> 
> Although I'm not sure from which position we move by plus/minus the half of
> the italic correction...

Yes, I can't reconcile this with the subscript/superscript use of
ItalicsCorrection.  Carefully picking the position before adjusting might
help.

To make me more confused, the "Technical Issues: Glyph Metrics" slides in
http://meeting.contextgarden.net/2011/talks/day3_05_ulrik_opentype/ot-math-fonts.pdf
say that OpenType math usually ignores italic correction.
I wonder what that means.

>+    if (largeop && ch.IsGlyphID()) {
>+      gfxFont* mathFont = aFontGroup->get()->GetFontAt(0);
>+      if (mathFont->GetFontEntry()->TryGetMathTable(mathFont)) {
>+        // For large operator, the application of italic correction is reversed
>+        // in the Open Type MATH spec. Hence we modify the advance width here so
>+        // that nsMathMLmmultiscripts will place the scripts correctly. Note
>+        // that STIX-Word does not provide italic correction for large
>+        // operators but does not give the expected advance width either, so we
>+        // actually don't need to correct the value.
>+        // See https://bugzilla.mozilla.org/show_bug.cgi?id=407059#c71
>+        // XXXfredw Move this to nsMathMLmmultiscripts (bug 961482).
>+        gfxFloat italicCorrection;
>+        if (mathFont->GetFontEntry()->
>+            GetMathItalicCorrection(ch.glyphID, &italicCorrection)) {
>+          bm.width -=
>+            NSToCoordRound(italicCorrection *
>+                           mathFont->GetAdjustedSize() * oneDevPixel);
>+          if (bm.width < 0) {
>+            bm.width = 0;

This makes sense, whether largeop or not even, when width comes from
rightBearing, so please modify the explanation based on that, and apply
regardless of largeop.

I think we'll need some data from fonts if we want to continue to do this once
width is an advance width.
Comment 98 Karl Tomlinson (:karlt) 2014-04-10 00:37:22 PDT
(In reply to Karl Tomlinson (:karlt) from comment #97)
> Are you sure that Murray didn't mean that the ItalicsCorrection from the
> GlyphAssembly Table is different?

I don't see a global advance in the GlyphAssembly table so subtracting ItalicCorrection from the maximum of the glyph bounds would give a rough estimate there.  I don't know of a better way.

I wonder why MathGlyphVariantRecord has AdvanceMeasurement.
Comment 99 Frédéric Wang (:fredw) 2014-04-10 00:53:18 PDT
> If the font has a displayOperatorMinHeight, then do we need to still measure
> the base glyph and use largeopFactor?

The displayOperatorMinHeight is sometimes incorrect, so the selected variant is too small (comment 13). So that's why I take the maximum of our default fallback.

There is also one weird thing in the MATH spec is that they don't distinguish between integrals and sums (the former being generally taller), so I'm not sure they can use a single displayOperatorMinHeight for all operators.
Comment 100 Frédéric Wang (:fredw) 2014-04-10 00:57:59 PDT
(In reply to Karl Tomlinson (:karlt) from comment #97)
> (In reply to Frédéric Wang (:fredw) from comment #72)
> > From the MATH spec:
> > 
> > "When positioning limits on an N-ary operator (e.g., integral sign),
> > the horizontal position of the upper limit is moved to the right by
> > 1⁄2 of the italics correction, while the position of the lower limit is
> > moved to the left by the same distance."
> > 
> > Although I'm not sure from which position we move by plus/minus the half of
> > the italic correction...
> 
> Yes, I can't reconcile this with the subscript/superscript use of
> ItalicsCorrection.  Carefully picking the position before adjusting might
> help.

As indicated in comment #73, this is for munderover and we already do that with our TeX-like algorithm. So I think modifying the italic correction to make it positive should work (although I have not checked).
Comment 101 Frédéric Wang (:fredw) 2014-04-10 01:04:56 PDT
(In reply to Karl Tomlinson (:karlt) from comment #98)
> I wonder why MathGlyphVariantRecord has AdvanceMeasurement.

I'm not sure either why variants/parts have advance in the direction of the stretching, since I assume it is already accessible from the glyph metrics. I guess this as well as overlap rules are to let authors give more accurate positioning of glyphs. At least for parts, I see that naively gluing the parts may produce gaps. See the cubic/quartic roots in this screenshot of WebKit: https://bug-122297-attachments.webkit.org/attachment.cgi?id=224379
Comment 102 Frédéric Wang (:fredw) 2014-04-10 04:31:01 PDT
(In reply to Karl Tomlinson (:karlt) from comment #96)
> If the second path is good enough for some platforms, then we should use it
> on
> all platforms.  I'm concerned that this is being developed and tested on one
> kind of platform but might be behaving differently on another.
> 
> I think we can use gfxHarfbuzzShaper::GetGlyphHAdvance() if we do something
> to
> ensure the shaper is suitably initialized.  That should be another patch and
> can be done later if the second path is good enough here.

Just taking the right bearing and subtracting the italic correction seems good enough for most fonts for now. However, that does not work on STIX Word because of

http://sourceforge.net/p/stixfonts/tracking/49/
http://sourceforge.net/p/stixfonts/tracking/50/
Comment 103 Frédéric Wang (:fredw) 2014-04-10 09:00:54 PDT
> STIXSizeOneSym has some missing entries for size0.
> Isn't that a problem here?

> Asking for an operator strictly larger than the fallback may also select extra large variants for STIX.  What have you checked to see the effect this has there?

As I understand, the 0 size will return a null glyph so the base height is zero and the fallback factor will be ignored. I'm willing to drop/merge tables in bug 947654 (http://www.wg9s.com/mozilla/firefox/mathml/947654-3.diff) so I'm not really willing to do a change for now. Actually, I don't want to worry too much about the old fonts because I want to suggest people to move to MATH fonts.
Comment 104 Frédéric Wang (:fredw) 2014-04-10 09:28:54 PDT
Created attachment 8404795 [details] [diff] [review]
Part 1: add a gfxMathTable class to read the MATH table.

s/Italic/Italics/
Comment 105 Frédéric Wang (:fredw) 2014-04-10 09:30:11 PDT
Created attachment 8404799 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table.

This addresses most of the comment. The main issue is the management of lifetime for OpenTypeMath tables.
Comment 106 Karl Tomlinson (:karlt) 2014-04-10 17:00:46 PDT
(In reply to Frédéric Wang (:fredw) from comment #103)
> > STIXSizeOneSym has some missing entries for size0.
> > Isn't that a problem here?
> 
> > Asking for an operator strictly larger than the fallback may also select extra large variants for STIX.  What have you checked to see the effect this has there?
> 
> As I understand, the 0 size will return a null glyph so the base height is
> zero and the fallback factor will be ignored.

I expect a null glyph to be rendered as a hex box.

Isn't it simplest to just explicitly use a zero min height for properties
tables, and then their behavior won't change?

For properties tables, the search begins at size 1 and so will find a larger
variant even with a zero min-height.

> I'm willing to drop/merge
> tables in bug 947654
> (http://www.wg9s.com/mozilla/firefox/mathml/947654-3.diff) so I'm not really
> willing to do a change for now. Actually, I don't want to worry too much
> about the old fonts because I want to suggest people to move to MATH fonts.

Unless a switch to replacement fonts can be made at the same time as changes
here, we need to keep the old fonts working.

Sounds like calculating the advance correctly is required before switching to
STIX-Word.

(In reply to Frédéric Wang (:fredw) from comment #99)
> > If the font has a displayOperatorMinHeight, then do we need to still measure
> > the base glyph and use largeopFactor?
> 
> The displayOperatorMinHeight is sometimes incorrect, so the selected variant
> is too small (comment 13). So that's why I take the maximum of our default
> fallback.

I see now that nsOpenTypeTable::BigOf() maps size == 1 to the 0th variant,
which is usually the base size.  If the opentype variants were treated
similarly to the properties tables, then I assume there would be no problem
here.

Or the search could start at the 1th variant only for largeop operators.

> There is also one weird thing in the MATH spec is that they don't
> distinguish between integrals and sums (the former being generally taller),
> so I'm not sure they can use a single displayOperatorMinHeight for all
> operators.

Yes, that is odd.  I wonder whether min height is being misinterpreted here,
particularly if Microsoft don't use it this way.
Comment 107 Karl Tomlinson (:karlt) 2014-04-10 17:06:10 PDT
Comment on attachment 8404799 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table.

>-        // Continue to check other sizes unless largeopOnly
>-        haveBetter = largeopOnly;

>-    // if largeopOnly is set, break now
>-    if (largeopOnly) break;
>+    // If this a largeop only operator, we stop if the glyph is large enough.
>+    if (largeopOnly && (bm.ascent + bm.descent) >= displayOperatorMinHeight) {
>+      break;
>+    }

>   return haveBetter &&
>     (largeopOnly ||
>      IsSizeOK(mPresContext, bestSize, mTargetSize, mStretchHint));

Looks like this will continue to check other fonts when maxWidth, even after
the correct variant has already been found.  That's not necessary.
Comment 108 Karl Tomlinson (:karlt) 2014-04-10 19:43:04 PDT
Looking at even the base size integral in Cambria Math version 5.93,
ItalicsCorrection seems to be set appropriately for the limits above and below.
It does not look appropriate to add ItalicsCorrection for a superscript (with
movablelimits), though subtracting for the subscript position might be
reasonable approximation.

Least unreasonable conclusions seem to be:

1) ItalicsCorrection needs to be assumed to mean different things for n-ary
operators than for identifiers.

2) Cambria Math was designed with advances such that ItalicCorrection or ItalicCorrection/2 is subtracted for subscript positions on all characters.

In both cases, ItalicsCorrection provides an estimate of the slope of the
glyph and so rightBearing - ItalicsCorrection would provide a reasonable
estimate of the ideal advance.

It seems unclear how glyph advances should be interpreted.

The advances and italics corrections for U+212F SCRIPT SMALL E and U+212C
SCRIPT CAPITAL B seem to even suggest an expectation that italics
correction is always subtracted for subscripts.  The advances are such that
the subscripts don't need any correction.

(In reply to Karl Tomlinson (:karlt) from comment #106)
> Sounds like calculating the advance correctly is required before switching to
> STIX-Word.

Actually, perhaps movablelimits are not used frequently enough that this needs to block anything?
Comment 109 Karl Tomlinson (:karlt) 2014-04-10 20:01:45 PDT
(In reply to Karl Tomlinson (:karlt) from comment #108)
> 1) ItalicsCorrection needs to be assumed to mean different things for n-ary
> operators than for identifiers.
> 
> 2) Cambria Math was designed with advances such that ItalicCorrection or
> ItalicCorrection/2 is subtracted for subscript positions on all characters.

I meant either 1 or 2.  Hopefully, we don't need to assume both.
Comment 110 Frédéric Wang (:fredw) 2014-04-11 00:11:36 PDT
(In reply to Karl Tomlinson (:karlt) from comment #106)
> I see now that nsOpenTypeTable::BigOf() maps size == 1 to the 0th variant,
> which is usually the base size.  If the opentype variants were treated
> similarly to the properties tables, then I assume there would be no problem
> here.

I think the design of fonts is inconsistent here, for example the Oth variant of "integral" is the base size for Latin Modern Math but it is the first larger variant for Asana Math. So always mapping size == 0 ensures consistent behavior (even if that means using the base size for size=0,1 in some cases) and using displayminheight or the target size ensures that we always pick the desired size. displayminheight is actually necessary for fonts such that TeX Gyre * families, where the desired display operators are at size >> 1.

For the property files, Asana uses the base size for the 0th size and the display size for the 1th size. MathJax does the same, except that the base size is from MathJax_Size1 and the display size from MathJax_Size2. STIX also uses the display size at the 1th size but the 0th size is empty.

So it seems that the simplest behavior will be to either fix the STIX property to explicitly add the base size for the 0th variant (but then I'll have to redo the merge in http://www.wg9s.com/mozilla/firefox/mathml/947654-3.diff, which was a bit tedious) or just keep minheight=0 for the property files.
Comment 111 Frédéric Wang (:fredw) 2014-04-11 00:16:21 PDT
(In reply to Karl Tomlinson (:karlt) from comment #108)
> Actually, perhaps movablelimits are not used frequently enough that this
> needs to block anything?

I think integrals are often used with msubsup in display mode, so we need make it work (unless we don't want to recommend STIX but suggests XITS instead). However, I believe your proposal to use gfxHarfbuzzShaper::GetGlyphHAdvance() and then subtracting the italic correction from the MATH table will work: it will be the same as rightbearing - italic for most MATH fonts while it will be advanceWidth - 0 for STIX Math.
Comment 112 Frédéric Wang (:fredw) 2014-04-11 08:07:07 PDT
Created attachment 8405426 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table.

This addresses the comment about TryVariants not stopping and leave minHeight=0 for properties file.

The nsOpenTypeTable's are now just created during the font enumeration phase and destroyed when we try another font (after bug 663740 no longer use the glyphtables during the painting).
Comment 113 Frédéric Wang (:fredw) 2014-04-13 11:10:31 PDT
Created attachment 8405895 [details] [diff] [review]
407059-4.diff

> If the second path is good enough for some platforms, then we should use it on
> all platforms.  I'm concerned that this is being developed and tested on one
> kind of platform but might be behaving differently on another.
>
> I think we can use gfxHarfbuzzShaper::GetGlyphHAdvance() if we do something to
> ensure the shaper is suitably initialized.  That should be another patch and
> can be done later if the second path is good enough here.

OK, I just tried again the gfxHarfbuzzShaper::GetGlyphHAdvance this evening. That seems to fix the problem with STIX Word fonts on Linux. I'll try on Windows later when Bill has updated his builds.
Comment 114 Bill Gianopoulos [:WG9s] 2014-04-13 14:31:06 PDT
(In reply to Frédéric Wang (:fredw) from comment #113)
> OK, I just tried again the gfxHarfbuzzShaper::GetGlyphHAdvance this evening.
> That seems to fix the problem with STIX Word fonts on Linux. I'll try on
> Windows later when Bill has updated his builds.

Update builds have completed.
Comment 115 Frédéric Wang (:fredw) 2014-04-13 14:48:34 PDT
(In reply to Bill Gianopoulos [:WG9s] from comment #114)
> (In reply to Frédéric Wang (:fredw) from comment #113)
> > OK, I just tried again the gfxHarfbuzzShaper::GetGlyphHAdvance this evening.
> > That seems to fix the problem with STIX Word fonts on Linux. I'll try on
> > Windows later when Bill has updated his builds.
> 
> Update builds have completed.

Thanks. The placement of scripts with STIX Word seems to work on Windows too. However, Jonathan's patches do not fix bug 947650 on all Windows system yet (bug 947650 comment 12) so STIX Word won't be usable atm anyway.
Comment 116 Karl Tomlinson (:karlt) 2014-04-14 00:33:34 PDT
Comment on attachment 8405895 [details] [diff] [review]
407059-4.diff

Jonathan, can you review the gfx changes here, please, or indicate whether you are happy with the general idea, and wait for the issues below to be addressed?

I haven't looked at the gfxHarfBuzzShaper changes in detail.

GetGlyphHAdvance() needs some font data that is managed in gfxHarfBuzzShaper,
so I assumed it would easier to use gfxHarfBuzzShaper than to move this to
gfxFont.

IIUC most platform implementations create the gfxHarfBuzzShaper even when it
won't be used, so initializing from the GetHarfBuzzShaper() method seemed the
easiest way to ensure this happened without adding unnecessary overhead
elsewhere.

>+    gfxFT2LockedFace face(this);
>+    mFUnitsConvFactor = face.XScale();
>+
>     if (MOZ_UNLIKELY(GetStyle()->size <= 0.0)) {
>         new(&mMetrics) gfxFont::Metrics(); // zero initialize
>         mSpaceGlyph = 0;
>     } else {
>-        gfxFT2LockedFace(this).GetMetrics(&mMetrics, &mSpaceGlyph);
>+        face.GetMetrics(&mMetrics, &mSpaceGlyph);

I don't know whether or not it is safe to lock a face of zero size but there
is no need to be mFUnitsConvFactor is initialized to zero.

Please move the new lines into the else block here.

>         if (!mHarfBuzzShaper) {
>             gfxFT2LockedFace face(this);
>-            mFUnitsConvFactor = face.XScale();
>-
>             mHarfBuzzShaper = new gfxHarfBuzzShaper(this);
>         }

Please also remove "face" here.
Similarly in gfxPangoFonts.cpp.

>+const gfxHarfBuzzShaper* gfxFont::GetHarfBuzzShaper()
>+{
>+    if (mFUnitsConvFactor == 0.0f)
>+        GetMetrics();
>+    mHarfBuzzShaper = new gfxHarfBuzzShaper(this);

Please first check whether mHarfBuzzShaper is already set, and if so, reuse
that.

>+  detailedGlyph.mAdvance = aAppUnitsPerDevPixel *
>+    aFontGroup->GetFontAt(0)->GetHarfBuzzShaper()->
>+    GetGlyphHAdvance(aThebesContext, aGlyph.glyphID) / 65536.0;

I assume mFUnitsConvFactor should be involved here.
Where does 65536.0 come from?
Comment 117 Karl Tomlinson (:karlt) 2014-04-14 00:42:04 PDT
(In reply to Karl Tomlinson (:karlt) from comment #116)
> I assume mFUnitsConvFactor should be involved here.
> Where does 65536.0 come from?

Oh, I see mFUnitsConvFactor is already involved, and 65536.0 is because this was fixed point.  I wonder whether we have any methods to do that conversion.  The gfxHarfBuzzShaper methods are internal.

>>+  detailedGlyph.mAdvance = aAppUnitsPerDevPixel *
>>+    aFontGroup->GetFontAt(0)->GetHarfBuzzShaper()->
>>+    GetGlyphHAdvance(aThebesContext, aGlyph.glyphID) / 65536.0;

mAdvance is an integer in app units.  If there are no methods available to perform the conversion to app units, then NSToCoordRound() should be used.
Comment 118 Karl Tomlinson (:karlt) 2014-04-14 00:52:16 PDT
(In reply to Frédéric Wang (:fredw) from comment #110)
> (In reply to Karl Tomlinson (:karlt) from comment #106)
> > I see now that nsOpenTypeTable::BigOf() maps size == 1 to the 0th variant,
> > which is usually the base size.  If the opentype variants were treated
> > similarly to the properties tables, then I assume there would be no problem
> > here.
> 
> I think the design of fonts is inconsistent here, for example the Oth
> variant of "integral" is the base size for Latin Modern Math but it is the
> first larger variant for Asana Math.

Fontforge says that the variants of integral in Asana Math 000.951 are

integral(∫) integralbig1(
Comment 119 Karl Tomlinson (:karlt) 2014-04-14 01:04:23 PDT
(In reply to Karl Tomlinson (:karlt) from comment #118)
> integral(∫) integralbig1(

I wonder whether it was Firefox or bugzilla that chopped my comment off at U+10FF99.

The first two seem similar to Latin Modern Math, apparently with the zeroth variant being the base size.

> So always mapping size == 0 ensures
> consistent behavior (even if that means using the base size for size=0,1 in
> some cases) and using displayminheight or the target size ensures that we
> always pick the desired size. displayminheight is actually necessary for
> fonts such that TeX Gyre * families, where the desired display operators are
> at size >> 1.

Do you know how they achieve an integral that is taller than a summation?
Comment 120 Frédéric Wang (:fredw) 2014-04-14 01:15:10 PDT
(In reply to Karl Tomlinson (:karlt) from comment #119)
> I wonder whether it was Firefox or bugzilla that chopped my comment off at
> U+10FF99.
> 
> The first two seem similar to Latin Modern Math, apparently with the zeroth
> variant being the base size.

OK, I checked again, and you're right. I'm not sure why I remember why I changed force zero to be the base size.

> 
> > So always mapping size == 0 ensures
> > consistent behavior (even if that means using the base size for size=0,1 in
> > some cases) and using displayminheight or the target size ensures that we
> > always pick the desired size. displayminheight is actually necessary for
> > fonts such that TeX Gyre * families, where the desired display operators are
> > at size >> 1.
> 
> Do you know how they achieve an integral that is taller than a summation?

What do you mean? My guess is that if the sizes are for example

sum => 1 3
integral => 1 2 4 8

then they just take minheight=3 so that the sum of size 3 and the integral of size 4 is taken in display mode, so the integral is indeed larger than the sum. I assume the other sizes for integrals are taken when stretchy=true.
Comment 121 Frédéric Wang (:fredw) 2014-04-14 01:41:26 PDT
Created attachment 8406012 [details] [diff] [review]
407059-4.diff
Comment 122 Jonathan Kew (:jfkthame) 2014-04-14 01:50:17 PDT
(In reply to Karl Tomlinson (:karlt) from comment #119)
> (In reply to Karl Tomlinson (:karlt) from comment #118)
> > integral(∫) integralbig1(
> 
> I wonder whether it was Firefox or bugzilla that chopped my comment off at
> U+10FF99.

That sounds like bug 405011, which was fixed recently in bugzilla trunk, but presumably not deployed on b.m.o yet.
Comment 123 Jonathan Kew (:jfkthame) 2014-04-14 02:55:05 PDT
Comment on attachment 8406012 [details] [diff] [review]
407059-4.diff

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

In general, I'm OK with the approach here, but there are a couple of adjustments I'd like to see to the current patch before we go forward, so marking r- for now.

First, I'd prefer that we don't expose gfxHarfBuzzShaper more widely. Instead, add a GetGlyphHAdvance method to gfxFont, and let the use of gfxHarfBuzzShaper remain an internal implementation detail.

(Indeed, I'm wondering whether it might make sense to move the actual work into the gfxFont method, and let gfxHarfBuzzShaper call that rather than vice versa. But that would be a refactoring we could consider independently of this bug; I haven't thought through exactly what benefits or drawbacks would be involved.)

Secondly, it's a bit unclear here whether the boolean return value from gfxHarfBuzzShaper::Initialize() merely indicates whether Initialize has been called, or whether the font is actually fit for the rest of the gfxHarfBuzzShaper code to use. The checks from http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxHarfBuzzShaper.cpp#937 could be moved into Initialize(), and only create mHBFont if the font is usable; Initialize() would then return true if mInitialized == true && mHBFont != nullptr.

::: gfx/thebes/gfxFont.cpp
@@ +2030,5 @@
>  
> +const gfxHarfBuzzShaper* gfxFont::GetHarfBuzzShaper()
> +{
> +    if (mFUnitsConvFactor == 0.0f)
> +        GetMetrics();

style nit: braces

(although I don't think this method will be needed in quite this form with the requested refactoring)

@@ +2036,5 @@
> +        mHarfBuzzShaper = new gfxHarfBuzzShaper(this);
> +    }
> +    gfxHarfBuzzShaper* shaper =
> +        static_cast<gfxHarfBuzzShaper*>(mHarfBuzzShaper.get());
> +    shaper->Initialize();

If Initialize() returns false here, the caller could be headed for trouble when it tries to use shaper methods... maybe return nullptr in that case, and check for that in the caller?

::: gfx/thebes/gfxFont.h
@@ +1489,5 @@
>          kAntialiasGrayscale,
>          kAntialiasSubpixel
>      } AntialiasOption;
>  
> +    const gfxHarfBuzzShaper* GetHarfBuzzShaper();

Let's avoid the need to make this public.

::: gfx/thebes/gfxHarfBuzzShaper.cpp
@@ +820,5 @@
>  bool
> +gfxHarfBuzzShaper::Initialize()
> +{
> +    if (mInitialized) {
> +        return true;

If the first call to Initialize() returned false (e.g. no cmap table found), returning true on subsequent calls will be surprising.

@@ +881,5 @@
> +        bool symbol;
> +        mCmapFormat = gfxFontUtils::
> +            FindPreferredSubtable(data, len,
> +                                  &mSubtableOffset, &mUVSTableOffset,
> +                                  &symbol);

We probably want
    if (mCmapFormat <= 0) {
        return false;
    }
here.

@@ +913,5 @@
> +                        mHmtxTable = nullptr;
> +                    }
> +                }
> +            }
> +        }

And
    if (!mHmtxTable) {
        return false;
    }
here.

::: gfx/thebes/moz.build
@@ +23,5 @@
>      'gfxFontInfoLoader.h',
>      'gfxFontTest.h',
>      'gfxFontUtils.h',
>      'gfxGradientCache.h',
> +    'gfxHarfBuzzShaper.h',

And let's avoid the need to make this public, too.
Comment 124 Frédéric Wang (:fredw) 2014-04-14 06:01:32 PDT
Created attachment 8406114 [details] [diff] [review]
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators
Comment 125 Frédéric Wang (:fredw) 2014-04-14 06:13:13 PDT
(In reply to Frédéric Wang (:fredw) from comment #92)
> Created attachment 8375556 [details] [diff] [review]
> Part 5: reftest. r=karl, b=407059
> 
> Just making the reftest a bit more strict...

@Karl: I mentioned that on IRC, but IIRC this tests assumes that the scale correction does not happen, so attachment 8404896 [details] [diff] [review] might be necessary.
Comment 126 Jonathan Kew (:jfkthame) 2014-04-14 10:09:24 PDT
Comment on attachment 8406114 [details] [diff] [review]
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators

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

A couple of further comments below; I think this is about ready to go, but I'd like to see how it looks with these suggestions addressed.

::: gfx/thebes/gfxFont.cpp
@@ +2032,5 @@
> +gfxFont::GetGlyphHAdvance(gfxContext *aCtx, uint16_t aGID)
> +{
> +    if (mFUnitsConvFactor == 0.0f) {
> +        GetMetrics();
> +    }

It's not guaranteed that GetMetrics() will cause mFUnitsConvFactor to be set; if the font is not an 'sfnt', at least under GDI, it will remain 0.0 (as there is no concept of "font units" in this case).

I don't think that will lead to any actual problems - if we're trying to use such a font, I'd expect ProvidesGlyphWidths() to be true, and we'll call the backend-specific GetGlyphWidth() instead of reading font tables, so we won't actually need the conversion factor. But it might be worth adding a comment here.

In fact, you could even add

   NS_ASSERTION(mFUnitsConvFactor > 0.0f || ProvidesGlyphWidths(),
                "missing font unit conversion factor");

here, as we *should* always have a valid conversion factor for fonts where we're using the harfbuzz table-based code.

::: gfx/thebes/gfxFont.h
@@ +1599,5 @@
>      }
>  
> +    // The return value is interpreted as a horizontal advance in 16.16 fixed
> +    // point format.
> +    int32_t GetGlyphHAdvance(gfxContext *aCtx, uint16_t aGID);

Hmm - it's not obvious why we have this in addition to the existing GetGlyphWidth(). We'd better have some extra comments here!

AIUI, GetGlyphWidth() is an *optional* platform-specific method for getting hinted glyph widths from the platform font backend; it is used by the shaper if (and only if) ProvidesGlyphWidths() returns true. If ProvidesGlyphWidths() returns false, the result of GetGlyphWidth() may not be meaningful.

The new GetGlyphHAdvance() is a public method for getting the advance (width) of a glyph and will always be implemented.

In view of this, I think we should take GetGlyphWidth out of the public gfxFont API, and make it a protected method; we'll need to declare gfxHarfBuzzShaper as a friend class in order to still access it, but that will help to clarify which of these methods should be used by external code that wants glyph widths.

Also, what about making the return value here floating-point, rather than a fixed-point value? The use of 16.16 fixed-point is an internal detail of how we use the harfbuzz shaper, and I'd prefer not to leak it out more widely.
Comment 127 Frédéric Wang (:fredw) 2014-04-14 11:31:23 PDT
Created attachment 8406298 [details] [diff] [review]
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators
Comment 128 Jonathan Kew (:jfkthame) 2014-04-14 12:29:16 PDT
Comment on attachment 8406298 [details] [diff] [review]
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators

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

Yeah, that was what I had in mind - but it's actually wrong for legacy (non-sfnt) fonts on Windows, at least; see below.

So we'd better have another round on this one. Sorry about the churn!

::: gfx/thebes/gfxFont.cpp
@@ +2031,5 @@
> +gfxFloat
> +gfxFont::GetGlyphHAdvance(gfxContext *aCtx, uint16_t aGID)
> +{
> +    NS_ASSERTION(mFUnitsConvFactor > 0.0f || ProvidesGlyphWidths(),
> +                 "missing font unit conversion factor");

This assertion should go *after* the check of mFUnitsConvFactor == 0.0 below.

@@ +2043,5 @@
> +        static_cast<gfxHarfBuzzShaper*>(mHarfBuzzShaper.get());
> +    if (!shaper->Initialize()) {
> +        return 0;
> +    }
> +    return shaper->GetGlyphHAdvance(aCtx, aGID) / 65536.0;

Actually, on re-reading this I've realized it's not quite right: it will fail for non-sfnt fonts, because shaper->Initialize() will return false, so we won't ever call shaper->GetGlyphHAdvance() and thus reach the backend-specific GetGlyphWidth().

A simple fix would be to add

    if (ProvidesGlyphWidths()) {
        return GetGlyphWidth(aCtx, aGID) / 65536.0;
    }

at the beginning of this method. (And then drop ProvidesGlyphWidths() from the assertion.)

This is still less than ideal, as we'd then check ProvidesGlyphWidths() again from within shaper->GetGlyphHAdvance(), but I guess we could live with that for now.

Or to fix that as well, perhaps we should refactor things in gfxHarfBuzzShaper a bit more, so that we have something like

static hb_position_t
HBGetGlyphHAdvance(hb_font_t *font, void *font_data,
                   hb_codepoint_t glyph, void *user_data)
{
    const gfxHarfBuzzShaper::FontCallbackData *fcd =
        static_cast<const gfxHarfBuzzShaper::FontCallbackData*>(font_data);
    gfxFont *font = fcd->mShaper->GetFont();
    if (font->ProvidesGlyphWidths()) {
        return font->GetGlyphWidth(fcd->mContext, glyph);
    } else {
        return fcd->mShaper->GetGlyphHAdvance(fcd->mContext, glyph);
    }
}

and then gfxHarfBuzzShaper::GetGlyphHAdvance() would not need to check ProvidesGlyphWidths() but would always read the font tables.

Ah, one more thing - that would require access to ProvidesGlyphWidths and GetGlyphWidth from the static callback function. So we'd have to either keep those public, or (better, I think) make the callback a static method of gfxHarfBuzzShaper, which is a friend of gfxFont. Let's do it that way.

::: gfx/thebes/gfxFont.h
@@ +1460,5 @@
>  /* a SPECIFIC single font family */
>  class gfxFont {
> +
> +    friend gfxHarfBuzzShaper;
> +    friend gfxGraphiteShaper;

We normally write "friend class ..." for these, I think.
Comment 129 Frédéric Wang (:fredw) 2014-04-15 00:46:39 PDT
Created attachment 8406683 [details] [diff] [review]
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators
Comment 130 Jonathan Kew (:jfkthame) 2014-04-15 01:58:00 PDT
Comment on attachment 8406683 [details] [diff] [review]
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators

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

Looks good, thanks.

::: gfx/thebes/gfxHarfBuzzShaper.h
@@ +46,5 @@
> +    // get harfbuzz horizontal advance in 16.16 fixed point format.
> +    static hb_position_t
> +    HBGetGlyphHAdvance(hb_font_t *font, void *font_data,
> +                       hb_codepoint_t glyph, void *user_data);
> +        

oops, stray whitespace
Comment 131 Frédéric Wang (:fredw) 2014-04-15 02:12:02 PDT
Created attachment 8406718 [details] [diff] [review]
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators
Comment 132 Frédéric Wang (:fredw) 2014-04-15 02:20:11 PDT
Created attachment 8406723 [details] [diff] [review]
Part 3: remove mathfontAsanaMath.properties and STIX 1.0 beta. r=karl, b=407059

I update the patch to just remove STIX1.0Beta and the font data for Asana Math. Latin Modern Math is not usable until bug 947650 is fixed.
Comment 133 Frédéric Wang (:fredw) 2014-04-15 08:06:25 PDT
There are still some failures with the new test on some Windows platform. I wonder if that's related to the problem with font metrics or with the way I generated the test font...

https://tbpl.mozilla.org/?tree=Try&rev=4e16f181323a
Comment 134 Khaled Hosny 2014-04-15 09:35:49 PDT
(In reply to Karl Tomlinson (:karlt) from comment #97)
> Comment on attachment 8374990 [details] [diff] [review]
> Part 2: make nsMathMLChar use the MATH table. r=karl, b=407059
> 
> (In reply to Khaled Hosny from comment #71)
> > I have not been following closely, but please note that in MATH fonts the
> > application of italic correction is reversed for big operators, i.e. the
> > superscript is placed immediately after the operator, while the subscript is
> > placed by *subtracting* italic correction from the operator advance width (I
> > never understood the rationale behind this, but this how Word works and how
> > Cambria is designed, and I later confirmed it with Murray Sargent).
> 
> Glyphs don't know whether they are large operator variants or stretchy
> operator variants or base size operators.

But the code that places the scripts knows (or should know).

> I assume you are referring to all large size variants, here,

No, all glyphs for a big operator (i.e. an integral) including the base size should be handled as descriped above. This how Cambria Math is designed and how existing implementations behave.
Comment 135 Karl Tomlinson (:karlt) 2014-04-16 19:56:31 PDT
Comment on attachment 8406723 [details] [diff] [review]
Part 3: remove mathfontAsanaMath.properties and STIX 1.0 beta. r=karl, b=407059

>-pref("font.mathfont-family", "MathJax_Main, STIXNonUnicode, STIXSizeOneSym, STIXSize1, STIXGeneral, Asana Math, Standard Symbols L, DejaVu Sans, Cambria Math");
>+pref("font.mathfont-family", "MathJax_Main, STIX Math, STIXNonUnicode, STIXSizeOneSym, STIXGeneral, Asana Math, Standard Symbols L, DejaVu Sans, Cambria Math");

Can you mention in the comment, please, that STIX Math is now used?
Comment 136 Karl Tomlinson (:karlt) 2014-04-16 20:14:00 PDT
Comment on attachment 8375556 [details] [diff] [review]
Part 5: reftest. r=karl, b=407059

Nice test.

(In reply to Frédéric Wang (:fredw) from comment #125)
> (In reply to Frédéric Wang (:fredw) from comment #92)
> > Created attachment 8375556 [details] [diff] [review]
> > Part 5: reftest. r=karl, b=407059
> > 
> > Just making the reftest a bit more strict...
> 
> @Karl: I mentioned that on IRC, but IIRC this tests assumes that the scale
> correction does not happen, so attachment 8404896 [details] [diff] [review]
> might be necessary.

IIUC the scaling would apply the same way for test and reference, so I wouldn't expect it to need attachment 8404896 [details] [diff] [review].

(In reply to Frédéric Wang (:fredw) from comment #133)
> There are still some failures with the new test on some Windows platform. I
> wonder if that's related to the problem with font metrics or with the way I
> generated the test font...
> 
> https://tbpl.mozilla.org/?tree=Try&rev=4e16f181323a

Perhaps view the fonts with fontforge to see whether you get what you expect.
Check glyph advances and right bearings.
Comment 137 Frédéric Wang (:fredw) 2014-04-16 23:04:55 PDT
(In reply to Karl Tomlinson (:karlt) from comment #135)
> Comment on attachment 8406723 [details] [diff] [review]
> Part 3: remove mathfontAsanaMath.properties and STIX 1.0 beta. r=karl,
> b=407059
> 
> >-pref("font.mathfont-family", "MathJax_Main, STIXNonUnicode, STIXSizeOneSym, STIXSize1, STIXGeneral, Asana Math, Standard Symbols L, DejaVu Sans, Cambria Math");
> >+pref("font.mathfont-family", "MathJax_Main, STIX Math, STIXNonUnicode, STIXSizeOneSym, STIXGeneral, Asana Math, Standard Symbols L, DejaVu Sans, Cambria Math");
> 
> Can you mention in the comment, please, that STIX Math is now used?

That's a mistake. I don't want to include STIX Math until bug 947650 is fixed.
Comment 138 Frédéric Wang (:fredw) 2014-04-16 23:08:50 PDT
Created attachment 8408004 [details] [diff] [review]
Part 3: remove mathfontAsanaMath.properties and STIX 1.0 beta.
Comment 139 Karl Tomlinson (:karlt) 2014-04-16 23:17:23 PDT
Comment on attachment 8405426 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table.

>+class nsPropertiesTable MOZ_FINAL : public nsGlyphTable {
>+ public:
>+  explicit nsPropertiesTable(const nsString& aPrimaryFontName)
>+    : mFontName(1) // ensure space for primary font name.
>+    , mState(NS_TABLE_STATE_EMPTY) {
>+    MOZ_COUNT_CTOR(nsPropertiesTable);
>+     mFontName.AppendElement(aPrimaryFontName);
>+   }
> 
>-class nsGlyphTable {
>-public:
>-  explicit nsGlyphTable(const nsString& aPrimaryFontName)
>-    : mFontName(1), // ensure space for primary font name.
>-      mState(NS_TABLE_STATE_EMPTY),
>-      mCharCache(0)
>-  {

Something is wrong with the indentation here.

I think Gecko style is usually to have the '{' on a new line for functions.
I don't mind for short functions, but at least with a constructor list, or a
long list of arguments, that would be easier to read.

>+  ~nsPropertiesTable() {
>+    MOZ_COUNT_DTOR(nsPropertiesTable);
>+   }
>+
>+  const nsAString& PrimaryFontName() const {
>+     return mFontName[0];
>   }

Indentation.

>+public:
>+  explicit nsOpenTypeTable(gfxFontEntry* aFontEntry)
>+    : mFontEntry(aFontEntry) {
>+    MOZ_COUNT_CTOR(nsOpenTypeTable);
>+  }

This can be private, now that there is a Create() method.

>+  const gfxFontEntry* GetFontEntry() const {
>+    return mFontEntry;
>+  }

I don't think this is used.

>+  // This returns a new OpenTypeTable instance to give access to OpenType MATH
>+  // table or nullptr if the font does not have such table. The instance must
>+  // be released after use.
>+  static nsGlyphTable* Create(gfxFont* aFont) {

// s/released/deleted/ or "ownership is passed to the caller".

>+    nsAutoPtr<gfxTextRun> textRun;
>+    textRun = aFontGroup->

Initialize in the declaration, instead of assigning in a separate statement.

>+    const gfxTextRun::CompressedGlyph data = textRun->GetCharacterGlyphs()[0];

Make this a reference.  There's no need to copy the data.
const gfxTextRun::CompressedGlyph&

>+  uint32_t glyphID =
>+    (aSize == 0 ? mGlyphID :
>+     mFontEntry->GetMathVariantsSize(mGlyphID, aVertical, aSize - 1));

This would be easier to follow if it returned the variant corresponding to
aSize.  That would be consistent with the corresponding method for properties
tables.  The selection algorithm can be changed for both types of tables in
the future to start from the base size, if desired and not a large op.

>+  // XXXfredw: gfxFontEntry could expose a function to verify whether the
>+  // character has parts (i.e. the GlyphAssembly offset is not NULL).

I don't think that's necessary.  The return value of GetMathVariantsParts()
already indicates what is necessary here and it has to do all the processing
to determine whether the glyphs are suitable.

>+  nsGlyphCode ch;
>+  nscoord displayOperatorMinHeight = 0;
>+  if (largeopOnly) {
>+    NS_ASSERTION(isVertical, "Stretching should be in the vertical direction");
>+    ch = aGlyphTable->BigOf(mThebesContext, oneDevPixel, *aFontGroup, uchar,
>+                            isVertical, 0);
>+    if (!mChar->SetFontFamily(mPresContext, aGlyphTable, ch, aFamily, font,
>+                              aFontGroup)) {
>+      return false;
>+    }
>+    gfxFont* mathFont = aFontGroup->get()->GetFontAt(0);
>+    if (mathFont->GetFontEntry()->TryGetMathTable(mathFont)) {

'ch' is only used here if mathFont has a MATH table, so move the BigOf() to
after the TryGetMathTable() test.

The only time that SetFontFamily() would be required is for a generic family,
using the Unicode properties table, but then there wouldn't be any need for
displayOperatorMinHeight.  I suspect it would be simpler to test whether this
is an opentype table, perhaps through the nsGlyphCode, than to call
SetFontFamily().

>+      displayOperatorMinHeight =
>+        NSToCoordRound(mathFont->GetFontEntry()->
>+                       GetMathConstant(gfxFontEntry::DisplayOperatorMinHeight) *
>+                       mathFont->GetAdjustedSize() * oneDevPixel);

GetAdjustedSize() isn't quite the right thing here, as that is the requested
size, but hinting and bitmap fonts can mean that the real em is different.

FUnitsToDevUnitsFactor() would have been the right value but the division by
mUnitsPerEm in GetMathConstant() is making things harder.  If you don't want
to remove that and make the method return design units, then
GetMetrics().emHeight should be better than GetAdjustedSize().

>+      nscoord minHeight = largeopFactor * (bm.ascent + bm.descent);
>+      if (displayOperatorMinHeight < minHeight) {
>+        displayOperatorMinHeight = minHeight;
>+      }

I think we would have made faster progress here if the initial implementation
had more closely matched the existing properties table algorithm, and then
added the variations.

I don't think this is the best thing to do.  These values are reasonably large
and overriding what the font designer has put in place.  I don't know why you
prefer this over starting the search at the first variant (as with the
properties table), but in the end this is your decision.

>+    if (ch.IsGlyphID()) {
>+      gfxFont* mathFont = aFontGroup->get()->GetFontAt(0);
>+      if (mathFont->GetFontEntry()->TryGetMathTable(mathFont)) {
>+        // MeasureTextRun has set the advance width to the right bearing. We now
>+        // subtract the italic correction, so that nsMathMLmmultiscripts will
>+        // place the scripts correctly.

It might make sense to skip the IsGlyphID() check here for MATH fonts found
from the Unicode properties table, but I guess that is unlikely to matter.

>+            NSToCoordRound(italicCorrection *
>+                           mathFont->GetAdjustedSize() * oneDevPixel);

Similar issue with GetAdjustedSize() here.

>+  bool isOpenTypeTable = false;
>+  nsGlyphTable* glyphTable;

If you replace bool isOpenTypeTable with nsAutoPtr<nsOpenTypeTable>
openTypeTable, set openTypeTable =
nsOpenTypeTable::Create(fontGroup->GetFontAt(0)), and glyphTable =
openTypeTable iff that succeeds, then the compiler will handle the deletion.
Comment 140 Frédéric Wang (:fredw) 2014-04-16 23:43:45 PDT
(In reply to Karl Tomlinson (:karlt) from comment #139)
> I think we would have made faster progress here if the initial implementation
> had more closely matched the existing properties table algorithm, and then
> added the variations.
> 
> I don't think this is the best thing to do.  These values are reasonably
> large
> and overriding what the font designer has put in place.  I don't know why you
> prefer this over starting the search at the first variant (as with the
> properties table), but in the end this is your decision.

Just starting trying the first variant is not enough for some fonts like TeX Gyre * Math where the first variant is almost the same as the base size (in that case one won't see a difference between normal and displaystyle operators). So a MinHeight is necessary and that seems what DisplayOperatorMinHeight is about. However, this value is sometimes too small for some fonts (e.g. Cambria): XeTeX uses a heuristics of 5/4 * the base size (comment 13) and I use the factors of our fallback scale transform. I wrote to Murray Sargent about this MinHeight but his reply was not useful at all...
Comment 141 Frédéric Wang (:fredw) 2014-04-16 23:50:23 PDT
(In reply to Karl Tomlinson (:karlt) from comment #139)
> GetAdjustedSize() isn't quite the right thing here, as that is the requested
> size, but hinting and bitmap fonts can mean that the real em is different.
> 
> FUnitsToDevUnitsFactor() would have been the right value but the division by
> mUnitsPerEm in GetMathConstant() is making things harder.  If you don't want
> to remove that and make the method return design units, then
> GetMetrics().emHeight should be better than GetAdjustedSize().
> 

Mmm, I think I did various tries and GetMetrics().emHeight only seemed to be a rounded version of GetAdjustedSize(). I asked Jonathan about that on IRC (04/02/2014):

18:21:07 - fredw : jfkthame: Hi. As indicated on bug 961365, I'm trying to find the best way to convert from the MATH table em values to nscoord. Does MATH_table_value * gfxFontStyle::size * AppUnitsPerDevPixel / gfxFontEntry::mUnitsPerEm make sense?
18:22:28 - jfkthame : fredw, what are the units of gfxFontStyle.size? i don't remember offhand - is that device pixels or CSS px?
18:22:56 - fredw : jfkthame: the comment says "The logical size of the font, in pixels"
18:22:59 - jfkthame : i think it's dev pix, actually, in which case yes, that sounds about right
18:26:39 - fredw : jfkthame: the MathML codes currently uses with gfxFont::Metrics::emHeight which is rounded and seems to call gfxFont::GetAdjustedSize.
18:28:03 - jfkthame : fredw, hmm, actually you probably want gfxFont::GetAdjustedSize rather than using gfxFontStyle::size, otherwise it won't be right if font-size-adjust is in effect
Comment 142 Frédéric Wang (:fredw) 2014-04-17 00:45:05 PDT
(In reply to Karl Tomlinson (:karlt) from comment #139)
> >+    nsAutoPtr<gfxTextRun> textRun;
> >+    textRun = aFontGroup->
> 
> Initialize in the declaration, instead of assigning in a separate statement.

I get a compilation error when doing so. Doing nsAutoPtr<gfxTextRun> textRun(...) works, though. There are other places where nsAutoPtr<gfxTextRun> is separated from the initialization.

> 'ch' is only used here if mathFont has a MATH table, so move the BigOf() to
> after the TryGetMathTable() test.
> 
> The only time that SetFontFamily() would be required is for a generic family,
> using the Unicode properties table, but then there wouldn't be any need for
> displayOperatorMinHeight.  I suspect it would be simpler to test whether this
> is an opentype table, perhaps through the nsGlyphCode, than to call
> SetFontFamily().

I'm not sure what is your suggestion here. Could you elaborate?
Comment 143 Frédéric Wang (:fredw) 2014-04-17 00:49:34 PDT
Created attachment 8408050 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table.

This addresses the review comment, except the points mentioned in the last comments.
Comment 144 Karl Tomlinson (:karlt) 2014-04-17 01:01:15 PDT
(In reply to Frédéric Wang (:fredw) from comment #141)
> Mmm, I think I did various tries and GetMetrics().emHeight only seemed to be
> a rounded version of GetAdjustedSize().
> 

OK, yes, emHeight includes rounding.  FUnitsToDevUnitsFactor() would be best but it is tricky because mFUnitsConvFactor is not set until metrics.  GetAdjustedSize() is probably close enough.
Comment 145 Karl Tomlinson (:karlt) 2014-04-17 02:25:24 PDT
(In reply to Frédéric Wang (:fredw) from comment #142)
> I'm not sure what is your suggestion here. Could you elaborate?

Sorry, my comment didn't actually make much sense.
I think less code is needed here, but my suggestion didn't work, and the
SetFontFamily complexity is making things hard.  I'll explain but this can all
wait for a clean up after this lands.

>+    ch = aGlyphTable->BigOf(mThebesContext, oneDevPixel, *aFontGroup, uchar,
>+                            isVertical, 0);
>+    if (!mChar->SetFontFamily(mPresContext, aGlyphTable, ch, aFamily, font,
>+                              aFontGroup)) {
>+      return false;
>+    }
>+    gfxFont* mathFont = aFontGroup->get()->GetFontAt(0);
>+    if (mathFont->GetFontEntry()->TryGetMathTable(mathFont)) {

We only need to take this path when aGlyphTable is an OpenType table.
For OpenType tables, aFontGroup is already set and the first font has a math
table.

If BigOf() is left where it is, then the TryGetMathTable() call can be
replaced with ch.IsGlyphID() and SetFontFamily() is not required.

>+    if (ch.IsGlyphID()) {
>+      gfxFont* mathFont = aFontGroup->get()->GetFontAt(0);
>+      if (mathFont->GetFontEntry()->TryGetMathTable(mathFont)) {

Similarly here either IsGlyphID() or TryGetMathTable(mathFont) should be
sufficient.  It should not be necessary to test both.

However, we need to keep the TryGetMathTable() calls because of these lines:

>   // Ensure SetFontFamily will set the font
>   font.name.Truncate();

That prevents SetFontFamily() from being a no-op.  It could fetch a new font
group which could theoretically have a different first font with the same
family name that doesn't have a MATH table.

I think that Truncate() call is no longer necessary since the !*aFontGroup check
was added to SetFontFamily(), but enough has changed here already, and we
don't need to fix this now.
Comment 146 Karl Tomlinson (:karlt) 2014-04-17 02:47:21 PDT
Comment on attachment 8408050 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table.

(In reply to Frédéric Wang (:fredw) from comment #140)
> So a MinHeight is necessary and that seems what
> DisplayOperatorMinHeight is about. However, this value is sometimes too
> small for some fonts (e.g. Cambria): XeTeX uses a heuristics of 5/4 * the
> base size (comment 13) and I use the factors of our fallback scale
> transform. I wrote to Murray Sargent about this MinHeight but his reply was
> not useful at all...

I understand the need for a DisplayOperatorMinHeight.

I'm not so sure about a problem with Cambria Math.

I suggest trying removing/disabling the largeopFactor minHeight code (keeping DisplayOperatorMinHeight) and looking at Cambria Math.  If it looks OK without, then I'd go with that.

r=me whether you decide to use this as is, or with that code removed.
Thank you for your patience here.
Comment 147 Frédéric Wang (:fredw) 2014-04-17 03:06:27 PDT
Created attachment 8408120 [details]
minHeight

So here are screenshots for Cambria Math and TeX Gyre Termes Math (we get similar things for Pagella and Bonum).

From top to bottom:

- minHeight = 0
- minHeight = the one indicated in the MATH table
- minHeight = max(MATH table, our factors)
Comment 148 Frédéric Wang (:fredw) 2014-04-17 08:31:31 PDT
Created attachment 8408310 [details] [diff] [review]
Part 5: reftest

The change with the base size broke the test. I've updated the test to explicitly include the base size in the size variants. For some reason, the only way I found to do that from the Python script is to use the "uniXXXX" names for the glyph.

I've also set em = 1000 and make all the contour to be clockwise to fix some fontforge warning. But that didn't seem to help the Windows failure.

The tests are currently running here:
https://tbpl.mozilla.org/?tree=Try&rev=f708cc72713e
Comment 149 Frédéric Wang (:fredw) 2014-04-17 08:33:00 PDT
Comment on attachment 8406718 [details] [diff] [review]
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators

I now get a crash for Windows 7 in the cairo code. Any idea why that could happen? https://tbpl.mozilla.org/php/getParsedLog.php?id=38013929&tree=Try#error1
Comment 150 Frédéric Wang (:fredw) 2014-04-17 22:06:12 PDT
(In reply to Frédéric Wang (:fredw) from comment #149)
> Comment on attachment 8406718 [details] [diff] [review]
> Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators
> 
> I now get a crash for Windows 7 in the cairo code. Any idea why that could
> happen?
> https://tbpl.mozilla.org/php/getParsedLog.php?id=38013929&tree=Try#error1

The crash does not happen without attachment 8406718 [details] [diff] [review] (but perhaps it is random):

https://tbpl.mozilla.org/?tree=Try&rev=0fe0990b7fca

However without that attachment, we get a failure due to bad computation of underbar width.
Comment 151 Jonathan Kew (:jfkthame) 2014-04-18 08:12:04 PDT
(In reply to Frédéric Wang (:fredw) from comment #149)
> Comment on attachment 8406718 [details] [diff] [review]
> Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators
> 
> I now get a crash for Windows 7 in the cairo code. Any idea why that could
> happen?
> https://tbpl.mozilla.org/php/getParsedLog.php?id=38013929&tree=Try#error1

I see the crash is only in the unaccelerated case. I suspect you may need to ensure that SetupCairoFont is called before trying to use GetGlyphHAdvance. Otherwise there's a possibility that the font has not been initialized yet, and then cairo ends up creating a "toy" DirectWrite face internally; but we think we're using GDI fonts, and everything goes wrong...

Compare http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxGDIFont.cpp#133, where the GDI version of ShapeText handles this.

Note that we don't want to add a SetupCairoFont call *within* GetGlyphHAdvance, as that would have a perf impact on every glyph during text shaping; we should do it at a higher level somewhere (assuming my guess here is correct).
Comment 152 Frédéric Wang (:fredw) 2014-04-18 08:13:20 PDT
Created attachment 8408983 [details] [diff] [review]
Part 5: reftest

So only the reftest actually only fails on Windows unaccelerated builds.

I executed the reftests again three times today and was not able to reproduce the crash of comment 149:

https://tbpl.mozilla.org/?tree=Try&rev=8a03e1712ddd
https://tbpl.mozilla.org/?tree=Try&rev=7f585ddc6883
https://tbpl.mozilla.org/?tree=Try&rev=790d6ceebdd9
Comment 153 Frédéric Wang (:fredw) 2014-04-18 08:47:48 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #151)
> (In reply to Frédéric Wang (:fredw) from comment #149)
> > Comment on attachment 8406718 [details] [diff] [review]
> > Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators
> > 
> > I now get a crash for Windows 7 in the cairo code. Any idea why that could
> > happen?
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=38013929&tree=Try#error1
> 
> I see the crash is only in the unaccelerated case. I suspect you may need to
> ensure that SetupCairoFont is called before trying to use GetGlyphHAdvance.
> Otherwise there's a possibility that the font has not been initialized yet,
> and then cairo ends up creating a "toy" DirectWrite face internally; but we
> think we're using GDI fonts, and everything goes wrong...
> 
> Compare
> http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxGDIFont.cpp#133,
> where the GDI version of ShapeText handles this.
> 
> Note that we don't want to add a SetupCairoFont call *within*
> GetGlyphHAdvance, as that would have a perf impact on every glyph during
> text shaping; we should do it at a higher level somewhere (assuming my guess
> here is correct).

OK, that makes sense. I see that gfxHarfBuzzShaper::ShapeText also does that and that part was not shared in the Initialize() function.

What about checking that in gfxFont::GetGlyphHAdvance (the one of gfxFont)? I see that other functions like gfxGlyphExtents::GetTightGlyphExtentsAppUnits or gfxTextRun::FetchGlyphExtents also do that.
Comment 154 Frédéric Wang (:fredw) 2014-04-18 09:16:41 PDT
Created attachment 8409004 [details] [diff] [review]
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators

The only change with respect to the previous patch is

 +    gfxHarfBuzzShaper* shaper =
 +        static_cast<gfxHarfBuzzShaper*>(mHarfBuzzShaper.get());
-+    if (!shaper->Initialize()) {
++    if (!shaper->Initialize() || !SetupCairoFont(aCtx)) {
 +        return 0;
 +    }
Comment 155 Frédéric Wang (:fredw) 2014-04-18 12:56:39 PDT
https://tbpl.mozilla.org/?tree=Try&rev=c77847dbf21a
Comment 156 Jonathan Kew (:jfkthame) 2014-04-22 05:01:01 PDT
Comment on attachment 8409004 [details] [diff] [review]
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators

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

Looks fine for now. (I have some thoughts about possibly refactoring a bunch of this, but that's for another time, another bug...if ever.)
Comment 157 Frédéric Wang (:fredw) 2014-04-22 05:10:19 PDT
Created attachment 8410212 [details] [diff] [review]
Part 1: add a gfxMathTable class to read the MATH table.
Comment 158 Frédéric Wang (:fredw) 2014-04-22 05:10:41 PDT
Created attachment 8410215 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table.
Comment 159 Frédéric Wang (:fredw) 2014-04-22 05:11:56 PDT
Created attachment 8410216 [details] [diff] [review]
Part 3: remove mathfontAsanaMath.properties and STIX 1.0 beta
Comment 160 Frédéric Wang (:fredw) 2014-04-22 05:12:18 PDT
Created attachment 8410217 [details] [diff] [review]
Part 4: Use gfxHarfbuzzShaper::GetGlyphHAdvance() for math operators
Comment 161 Frédéric Wang (:fredw) 2014-04-22 05:12:47 PDT
Created attachment 8410218 [details] [diff] [review]
Part 5: reftest
Comment 164 Frédéric Wang (:fredw) 2014-04-22 08:28:01 PDT
(In reply to Karl Tomlinson (:karlt) from comment #145)
> >+    ch = aGlyphTable->BigOf(mThebesContext, oneDevPixel, *aFontGroup, uchar,
> >+                            isVertical, 0);
> >+    if (!mChar->SetFontFamily(mPresContext, aGlyphTable, ch, aFamily, font,
> >+                              aFontGroup)) {
> >+      return false;
> >+    }
> >+    gfxFont* mathFont = aFontGroup->get()->GetFontAt(0);
> >+    if (mathFont->GetFontEntry()->TryGetMathTable(mathFont)) {
> 
> We only need to take this path when aGlyphTable is an OpenType table.
> For OpenType tables, aFontGroup is already set and the first font has a math
> table.
> 
> If BigOf() is left where it is, then the TryGetMathTable() call can be
> replaced with ch.IsGlyphID() and SetFontFamily() is not required.
> 

I'm not sure how the assert can happen and why it didn't appear in the try server tests. I think it's when the Open Type MATH is used but the bigof returns a kNullGlyph. Doing the ch.IsGlyphID() check as you suggested should avoid that.

https://tbpl.mozilla.org/?tree=Try&rev=41e4fef6da3d
Comment 165 Frédéric Wang (:fredw) 2014-04-22 12:26:15 PDT
Created attachment 8410465 [details] [diff] [review]
Part 6: Refactor the minHeight verification to prevent an ASSERT on Windows.
Comment 166 Frédéric Wang (:fredw) 2014-04-22 14:58:02 PDT
https://tbpl.mozilla.org/?tree=Try&rev=c2e160368b6e
Comment 167 Karl Tomlinson (:karlt) 2014-04-22 18:28:56 PDT
Comment on attachment 8410465 [details] [diff] [review]
Part 6: Refactor the minHeight verification to prevent an ASSERT on Windows.

>Part 6 - Refactor the minHeight verification to prevent an ASSERT on Windows. r=karlt

Commit messages need to be more specific than "an ASSERT on Windows".  "to
avoid calling MakeTextRun() with a null glyph" would be better, but this is
new code in part 2 so let's fix this in part 2 by folding these changes in
there.  I can review them separately, as long as they are folded into part 2.

The other part of comment 145 is not yet addressed. 

(In reply to Karl Tomlinson (:karlt) from comment #145)
> However, we need to keep the TryGetMathTable() calls because of these lines:
> 
> >   // Ensure SetFontFamily will set the font
> >   font.name.Truncate();
> 
> That prevents SetFontFamily() from being a no-op.  It could fetch a new font
> group which could theoretically have a different first font with the same
> family name that doesn't have a MATH table.
> 
> I think that Truncate() call is no longer necessary since the !*aFontGroup
> check was added to SetFontFamily(),

Removing the Truncate() calls is modifying existing code, so this should be a
separate patch.  I suggest a part 0.1 patch removing both of these
Truncate() calls.

r=me on this patch if it is folded into part 2 before landing and there is a
separate patch to remove the font.name.Truncate() calls.
Comment 168 Frédéric Wang (:fredw) 2014-04-22 22:34:57 PDT
Created attachment 8410770 [details] [diff] [review]
Part 2: make nsMathMLChar use the MATH table.

Folding part 6 into part 2...
Comment 169 Frédéric Wang (:fredw) 2014-04-22 22:35:58 PDT
Created attachment 8410771 [details] [diff] [review]
Part 0: Remove the Truncate() calls from nsMathMLChar::StretchEnumContext
Comment 170 Frédéric Wang (:fredw) 2014-04-23 02:03:44 PDT
https://tbpl.mozilla.org/?tree=Try&rev=99037fe55863
Comment 173 Frédéric Wang (:fredw) 2014-05-01 09:17:22 PDT
I've updated

https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/MathML_Torture_Test#MathML_Torture_Test
https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/Fonts

I leave the dev-doc-needed flag, so that partial support for OpenType MATH font can be documented elsewhere.
Comment 174 Frédéric Wang (:fredw) 2014-05-07 09:01:46 PDT
So I also added this to Firefox 31 release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/31#MathML
Comment 175 Frédéric Wang (:fredw) 2014-05-23 08:43:42 PDT
There are many related bugs for OpenType MATH support, but I'm setting relnote? on the main one. Here is a short relnote:

"New - Partial implementation of the <a href="http://mpeg.chiariglione.org/standards/mpeg-4/open-font-format/text-isoiec-cd-14496-22-3rd-edition>OpenType MATH table</a> (section 6.3.6). See the <a href="https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/Fonts">documentation about mathematical fonts</a> and the <a href="https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/MathML_Torture_Test">MathML Torture Test</a> for details."
Comment 176 Sylvestre Ledru [:sylvestre] 2014-05-26 09:21:09 PDT
Added in the release notes for 31.

Note You need to log in before you can comment on or make changes to this bug.