Bug 407059 - read OpenType MATH table for variants and parts of stretchy characters
 Summary: read OpenType MATH table for variants and parts of stretchy characters
 Status: RESOLVED FIXED dev-doc-complete Core Components MathML (show other bugs) Trunk x86 All P5 enhancement with 4 votes (vote) mozilla31 Frédéric Wang (:fredw) Anthony Jones (:kentuckyfriedtakahe, :k17e) 400938 663740 941019 945254 1000370 CVE-2014-1551 945183 963147 1001570 cambria-math asana-math neo-euler stix-1.1 LM-Math lucida-fonts 930504 947654 960115 961365 Show dependency tree / graph

Reported: 2007-12-05 14:58 PST by Karl Tomlinson (back Dec 13 :karlt)
Modified: 2014-06-27 09:51 PDT (History)
24 users (show)
ryanvm: in‑testsuite+
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
 relnote-firefox: 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

 Karl Tomlinson (back Dec 13 :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. Karl Tomlinson (back Dec 13 :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. Karl Tomlinson (back Dec 13 :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/ 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 Frédéric Wang (:fredw) 2012-03-15 03:43:29 PDT  http://www.gust.org.pl/projects/e-foundry/lm-math Frédéric Wang (:fredw) 2013-12-06 12:14:53 PST Created attachment 8343928 [details] [diff] [review] Patch V1 Frédéric Wang (:fredw) 2013-12-06 12:15:44 PST Created attachment 8343930 [details] [diff] [review] Patch to disable the scaling correction 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. 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... 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. Frédéric Wang (:fredw) 2013-12-07 16:13:46 PST Created attachment 8344251 [details] screenshot Gyre Pagella Frédéric Wang (:fredw) 2013-12-07 16:14:29 PST Created attachment 8344252 [details] screenshot Asana (XeTeX) Frédéric Wang (:fredw) 2013-12-07 16:14:51 PST Created attachment 8344253 [details] screenshot Gyre Pagella (XeTeX) 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). 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. 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. 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... Khaled Hosny 2013-12-07 17:29:26 PST The values are in EM units, if this helps. 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. Frédéric Wang (:fredw) 2013-12-08 13:56:29 PST Created attachment 8344397 [details] [diff] [review] Patch V4 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). 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? Robert O'Callahan (:roc) (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? Robert O'Callahan (:roc) (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. 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. 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()? 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) 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. 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. Behdad Esfahbod 2013-12-09 03:39:32 PST FWIW, I implemented MATH table in my fonttools/ttx at http://github.com/behdad/fonttools 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. 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. 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. 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. 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 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 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? 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... 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.) 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 mSVGGlyphs; > // list of gfxFonts that are using SVG glyphs > nsTArray mFontsUsingSVGGlyphs; > + nsAutoPtr mMathTable; > + nsTArray 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. 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. 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. 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. 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) 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/ 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? 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. 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. 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(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. Behdad Esfahbod 2013-12-12 10:48:54 PST I'll try to have something up early next week. 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 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 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... 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? Behdad Esfahbod 2014-01-07 06:28:46 PST Not yet. I should. Will reprioritize. 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. 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. Frédéric Wang (:fredw) 2014-01-13 08:49:00 PST Created attachment 8359284 [details] [diff] [review] Part 4 - tests 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? 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. 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... 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. 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. 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. Frédéric Wang (:fredw) 2014-01-15 06:49:04 PST Created attachment 8360426 [details] [diff] [review] Part 4 - tests 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. 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. 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. 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. 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. 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. 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). 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. 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. 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. 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 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 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. 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. 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) 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 , nsMathMLChar should expose these values for the selected variant or for the GlyphAssembly. Frédéric Wang (:fredw) 2014-02-04 02:35:02 PST Reported to STIX: https://sourceforge.net/p/stixfonts/tracking/49/ https://sourceforge.net/p/stixfonts/tracking/50/ 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 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(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(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). 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 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 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 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 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 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! 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. 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 :-) 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... 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) Frédéric Wang (:fredw) 2014-03-30 09:05:51 PDT @karl: any update on the review? Karl Tomlinson (back Dec 13 :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. Karl Tomlinson (back Dec 13 :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 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. Karl Tomlinson (back Dec 13 :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. Karl Tomlinson (back Dec 13 :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. 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. 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). 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 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/ 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. 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/ 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. Karl Tomlinson (back Dec 13 :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. Karl Tomlinson (back Dec 13 :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. Karl Tomlinson (back Dec 13 :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? Karl Tomlinson (back Dec 13 :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. 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. 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. 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). 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. 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. 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. Karl Tomlinson (back Dec 13 :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? Karl Tomlinson (back Dec 13 :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. Karl Tomlinson (back Dec 13 :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( Karl Tomlinson (back Dec 13 :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? 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. Frédéric Wang (:fredw) 2014-04-14 01:41:26 PDT Created attachment 8406012 [details] [diff] [review] 407059-4.diff 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. 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(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. 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 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. 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. 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 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(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(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. 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 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 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 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. 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 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. Karl Tomlinson (back Dec 13 :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? Karl Tomlinson (back Dec 13 :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. 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. 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. Karl Tomlinson (back Dec 13 :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 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 openTypeTable, set openTypeTable = nsOpenTypeTable::Create(fontGroup->GetFontAt(0)), and glyphTable = openTypeTable iff that succeeds, then the compiler will handle the deletion. 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... 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 Frédéric Wang (:fredw) 2014-04-17 00:45:05 PDT (In reply to Karl Tomlinson (:karlt) from comment #139) > >+ nsAutoPtr textRun; > >+ textRun = aFontGroup-> > > Initialize in the declaration, instead of assigning in a separate statement. I get a compilation error when doing so. Doing nsAutoPtr textRun(...) works, though. There are other places where nsAutoPtr 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? 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. Karl Tomlinson (back Dec 13 :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. Karl Tomlinson (back Dec 13 :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. Karl Tomlinson (back Dec 13 :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. 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) 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 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 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. 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). 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 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. 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(mHarfBuzzShaper.get()); -+ if (!shaper->Initialize()) { ++ if (!shaper->Initialize() || !SetupCairoFont(aCtx)) { + return 0; + } Frédéric Wang (:fredw) 2014-04-18 12:56:39 PDT https://tbpl.mozilla.org/?tree=Try&rev=c77847dbf21a 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.) 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. 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. 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 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 Frédéric Wang (:fredw) 2014-04-22 05:12:47 PDT Created attachment 8410218 [details] [diff] [review] Part 5: reftest Ryan VanderMeulen [:RyanVM] 2014-04-22 05:44:50 PDT https://hg.mozilla.org/integration/mozilla-inbound/rev/d8244f3ecdcb https://hg.mozilla.org/integration/mozilla-inbound/rev/cfcbc731d4db https://hg.mozilla.org/integration/mozilla-inbound/rev/77593dd1b044 https://hg.mozilla.org/integration/mozilla-inbound/rev/c940a2d399c4 https://hg.mozilla.org/integration/mozilla-inbound/rev/21fee1261f2e Ryan VanderMeulen [:RyanVM] 2014-04-22 08:00:42 PDT Backed out for Windows reftest asserts. https://hg.mozilla.org/integration/mozilla-inbound/rev/545be5328235 https://tbpl.mozilla.org/php/getParsedLog.php?id=38249702&tree=Mozilla-Inbound 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 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. Frédéric Wang (:fredw) 2014-04-22 14:58:02 PDT https://tbpl.mozilla.org/?tree=Try&rev=c2e160368b6e Karl Tomlinson (back Dec 13 :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. 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... 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 Frédéric Wang (:fredw) 2014-04-23 02:03:44 PDT https://tbpl.mozilla.org/?tree=Try&rev=99037fe55863 Ryan VanderMeulen [:RyanVM] 2014-04-23 06:59:58 PDT https://hg.mozilla.org/integration/mozilla-inbound/rev/cb4deaae3336 https://hg.mozilla.org/integration/mozilla-inbound/rev/de506f461074 https://hg.mozilla.org/integration/mozilla-inbound/rev/7d09c6ea9ae5 https://hg.mozilla.org/integration/mozilla-inbound/rev/b33c2654f667 https://hg.mozilla.org/integration/mozilla-inbound/rev/39bcb94e9b85 https://hg.mozilla.org/integration/mozilla-inbound/rev/427e3e62d7f3 Ryan VanderMeulen [:RyanVM] 2014-04-23 13:46:54 PDT https://hg.mozilla.org/mozilla-central/rev/cb4deaae3336 https://hg.mozilla.org/mozilla-central/rev/de506f461074 https://hg.mozilla.org/mozilla-central/rev/7d09c6ea9ae5 https://hg.mozilla.org/mozilla-central/rev/b33c2654f667 https://hg.mozilla.org/mozilla-central/rev/39bcb94e9b85 https://hg.mozilla.org/mozilla-central/rev/427e3e62d7f3 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. 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 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 documentation about mathematical fonts and the MathML Torture Test for details." 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.