Use HarfBuzz ot-math API to parse the OpenType MATH table

RESOLVED FIXED in Firefox 52

Status

()

Core
MathML
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: fredw, Assigned: fredw)

Tracking

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

Trunk
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(URL)

Attachments

(1 attachment, 10 obsolete attachments)

80.99 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

8 months ago
The next version of HarfBuzz will provide a new API to parse the OpenType MATH table:

https://github.com/behdad/harfbuzz/blob/master/src/hb-ot-math.h 

We should use that API instead of our own parsing code in gfx/thebes/gfxMathTable.cpp
(Assignee)

Comment 1

8 months ago
Created attachment 8796056 [details] [diff] [review]
Part 1 - Upgrade HarfBuzz to fd7a245d3525905ffbce57472b52900fcb0e330b
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
(Assignee)

Comment 2

8 months ago
Created attachment 8796176 [details] [diff] [review]
Part 2 - Use HarfBuzz ot-math API to parse the OpenType MATH table (WIP)
(Assignee)

Comment 3

8 months ago
Created attachment 8796498 [details] [diff] [review]
Part 2 - Use HarfBuzz ot-math API to parse the OpenType MATH table (WIP)

This is still work-in-progress, but I believe it's the minimal change needed to switch to HarfBuzz ot-math API. I guess we probably want to improve the caching for the MathVariant table (as that used to be done) and we can maybe simplify things a bit more.
Attachment #8796176 - Attachment is obsolete: true
Attachment #8796498 - Flags: feedback?(jfkthame)
(Assignee)

Updated

8 months ago
Attachment #8796056 - Flags: feedback?(jfkthame)
Comment on attachment 8796498 [details] [diff] [review]
Part 2 - Use HarfBuzz ot-math API to parse the OpenType MATH table (WIP)

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

I may be mis-reading this, but at first look it seems like there's a problem here: a gfxFontEntry is potentially shared by many different-sized gfxFont instances, but you're giving it a single size-specific hb_font_t. Doesn't that mean that the gfxMathTable will be correct for the first gfxFont that requests it, but subsequent requests by fonts with a different size will (incorrectly) get that same table using the same-sized hb_font_t?
(Assignee)

Comment 5

8 months ago
(In reply to Jonathan Kew (:jfkthame) from comment #4)
> I may be mis-reading this, but at first look it seems like there's a problem
> here: a gfxFontEntry is potentially shared by many different-sized gfxFont
> instances, but you're giving it a single size-specific hb_font_t. Doesn't
> that mean that the gfxMathTable will be correct for the first gfxFont that
> requests it, but subsequent requests by fonts with a different size will
> (incorrectly) get that same table using the same-sized hb_font_t?

Mmmh, I suspect you are right. So I guess we should now probably create a gfxMathTable per gfxFont, right?
Flags: needinfo?(jfkthame)
Yes, that sounds right. In this new world, gfxMathTable will be a pretty lightweight wrapper around the hb_font, providing Gecko-friendly interfaces to the hb_ot_math_... accessors for the values we need. The sharing of the actual table data (previously held by gfxMathTable, for all instances of gfxFont using the same gfxFontEntry) will now happen in the underlying hb_face, and the gfxMathTable will be instance-specific.
Flags: needinfo?(jfkthame)
(Assignee)

Comment 7

8 months ago
Created attachment 8799218 [details] [diff] [review]
Part 2 - Use HarfBuzz ot-math API to parse the OpenType MATH table
Attachment #8796498 - Attachment is obsolete: true
Attachment #8796498 - Flags: feedback?(jfkthame)
Attachment #8799218 - Flags: feedback?(jfkthame)
(Assignee)

Comment 8

8 months ago
Created attachment 8799220 [details] [diff] [review]
Part 3 - gfxMathTable: Cache MathVariants data.
Attachment #8799220 - Flags: feedback?(jfkthame)
Comment on attachment 8799218 [details] [diff] [review]
Part 2 - Use HarfBuzz ot-math API to parse the OpenType MATH table

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

This looks basically fine, I think. (It actually makes gfxMathTable virtually redundant, to the extent that at this stage it'd make sense to remove it and store the hb_font directly in gfxFont itself, but I see that the next patch introduces enough additional content into the gfxMathTable wrapper that it's worth keeping after all.)

::: gfx/thebes/gfxFont.h
@@ +1845,5 @@
>          delete sDefaultFeatures;
>      }
>  
> +
> +    enum MathConstant {

Could we move this over to be defined in gfxMathTable instead, or would that make things significantly more awkward in places where they're used? It would mean an extra #include in a few places, I expect, but it seems like a more fitting home for the constants, and avoids making the already over-large gfxFont.h even longer. (And it'll allow you to drop the gfxFont:: prefix where they're used in gfxMathTable.cpp itself.)

@@ +2193,5 @@
>      mozilla::UniquePtr<const Metrics> mVerticalMetrics;
>  
> +    // OpenType MATH table, used for MathML layout.
> +    mozilla::UniquePtr<gfxMathTable> mMathTable;
> +    bool                             mMathInitialized : 1;

Please group the bool together with other bool fields in the class (for potentially better packing). Also, it doesn't look like gfxFont currently uses single-bit bool fields (like gfxFontEntry did), so it's probably better not to declare this one as a bit-field either.

::: gfx/thebes/gfxMathTable.h
@@ +10,1 @@
>  struct MathConstants;

This struct isn't needed any longer either, is it?
Attachment #8799218 - Flags: feedback?(jfkthame) → feedback+
(Assignee)

Comment 10

8 months ago
(In reply to Jonathan Kew (:jfkthame) from comment #9)
> This looks basically fine, I think. (It actually makes gfxMathTable
> virtually redundant, to the extent that at this stage it'd make sense to
> remove it and store the hb_font directly in gfxFont itself, but I see that
> the next patch introduces enough additional content into the gfxMathTable
> wrapper that it's worth keeping after all.)

Yes, that's what I meant by "we can maybe simplify things a bit more.". I believe it makes sense to keep math-specific stuff in its own class, though. Given your suggestion about the "enum MathConstant" I'm even tempted to move almost all the GetMath* functions from gfxFont to gfxMathTable and just keep a gfxMathTable* gfxFont::GetMathTable() that will return a non-null pointer iff a MATH table is available. Then instead of the current gfxFont::GetFontEntry()->GetMath*() calls we would do gfxFont::GetMathTable()->GetMath*(). What do you think?

> ::: gfx/thebes/gfxMathTable.h
> @@ +10,1 @@
> >  struct MathConstants;
> 
> This struct isn't needed any longer either, is it?

Indeed.
(Assignee)

Updated

8 months ago
Flags: needinfo?(jfkthame)
(In reply to Frédéric Wang (:fredw) from comment #10)
> I'm even tempted to move
> almost all the GetMath* functions from gfxFont to gfxMathTable and just keep
> a gfxMathTable* gfxFont::GetMathTable() that will return a non-null pointer
> iff a MATH table is available. Then instead of the current
> gfxFont::GetFontEntry()->GetMath*() calls we would do
> gfxFont::GetMathTable()->GetMath*(). What do you think?

Yes, that sounds reasonable. Actually, I think gfxFont::GetMathTable() should be an infallible method that can never return null. It should simply crash if there's no table, because that's a programming error; we should never call it unless TryGetMathTable() has returned true. (Use MOZ_RELEASE_ASSERT?)

Maybe consider shortening some of these names, too? Instead of things like

  font->GetMathTable()->GetMathItalicsCorrection(glyphID)

which seems like it has a bunch of redundancy, it might be nice to be able to write

  font->MathTable()->ItalicsCorrection(glyphID)

which I think is equally clear, and a bit more concise. WDYT?
Flags: needinfo?(jfkthame)
(Assignee)

Comment 12

8 months ago
Comment on attachment 8796056 [details] [diff] [review]
Part 1 - Upgrade HarfBuzz to fd7a245d3525905ffbce57472b52900fcb0e330b

Asking review for that one, although we probably want to use the latest version (maybe wait that Bedhad makes a new release)
Attachment #8796056 - Flags: feedback?(jfkthame) → review?(jfkthame)
(Assignee)

Comment 13

8 months ago
Created attachment 8799364 [details] [diff] [review]
Part 2 - Use HarfBuzz ot-math API to parse the OpenType MATH table
Attachment #8799364 - Flags: review?(jfkthame)
(Assignee)

Updated

8 months ago
Attachment #8799218 - Attachment is obsolete: true
Comment on attachment 8799364 [details] [diff] [review]
Part 2 - Use HarfBuzz ot-math API to parse the OpenType MATH table

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

Looks good; r=me, just a couple of minor adjustments suggested below.

::: gfx/thebes/gfxFont.h
@@ +1845,5 @@
>          delete sDefaultFeatures;
>      }
>  
> +    // Call TryGetMathTable() to try and load the Open Type MATH table. You can
> +    // then access the gfxMathTable data via the MathTable() function.

Please make it really explicit that MathTable() may *only* be called if TryGetMathTable() has returned true. Something like

  // If (and ONLY if) TryGetMathTable() has returned true, the MathTable() method
  // may be called to access the gfxMathTable data.

@@ +2087,5 @@
>      bool                       mKerningSet;     // kerning explicitly set?
>      bool                       mKerningEnabled; // if set, on or off?
>  
> +    bool                       mMathInitialized; // TryGetMathTable() called?
> +    

nit: whitespace

::: gfx/thebes/gfxMathTable.cpp
@@ +37,2 @@
>    }
> +  return FixedToFloat(value);

I'm not sure FixedToFloat is available everywhere... there are several definitions of it scattered around the codebase, but none of them necessarily included here. And I believe OS X provides it in a system header, but that's not standard elsewhere AFAIK.

So probably safer to define a local version of it, unless you can confirm that it's more universal than I thought.

::: gfx/thebes/gfxMathTable.h
@@ +19,2 @@
>       */
> +    explicit gfxMathTable(hb_face_t *aFace, gfxFont *aFont);

No need for the 'explicit' any longer, now that the constructor takes two params.

I'd prefer to simply pass a size parameter, rather than a gfxFont, given that the only thing it's used for is to look up its size.

@@ +88,4 @@
>      /**
>       * Returns the value of the specified constant from the MATH table.
>       */
> +    gfxFloat Constant(MathConstant aConstant);

Can we declare this and the following accessors as const methods, or will that create problems?
Attachment #8799364 - Flags: review?(jfkthame) → review+
Comment on attachment 8796056 [details] [diff] [review]
Part 1 - Upgrade HarfBuzz to fd7a245d3525905ffbce57472b52900fcb0e330b

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

r=me, in principle, but let's see if we can get an official release sometime soon.
Attachment #8796056 - Flags: review?(jfkthame) → review+
Comment on attachment 8799220 [details] [diff] [review]
Part 3 - gfxMathTable: Cache MathVariants data.

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

This seems fine, changing feedback? to review+ on it. :)

::: gfx/thebes/gfxMathTable.cpp
@@ +52,5 @@
>                                    uint16_t aSize)
>  {
> +  UpdateMathVariantCache(aGlyphID, aVertical);
> +  if (aSize < kMaxCachedSizeCount)
> +    return mMathVariantCache.sizes[aSize];

braces, please

::: gfx/thebes/gfxMathTable.h
@@ +68,5 @@
>  
>  private:
>      // size-specific font object, owned by the gfxMathTable
>      hb_font_t *mHBFont;
> +      

stray whitespace
Attachment #8799220 - Flags: feedback?(jfkthame) → review+
(Assignee)

Comment 17

8 months ago
(In reply to Jonathan Kew (:jfkthame) from comment #14)
> Can we declare this and the following accessors as const methods, or will
> that create problems?

Thanks for the review, I guess all the feedback can be easily addressed. Regarding making const accessors, that won't probably work after caching is implemented in part 3. But maybe I can just make mMathVariantCache a mutable member.
Flags: needinfo?(jfkthame)
(In reply to Frédéric Wang (:fredw) from comment #17)
> (In reply to Jonathan Kew (:jfkthame) from comment #14)
> > Can we declare this and the following accessors as const methods, or will
> > that create problems?
> 
> Thanks for the review, I guess all the feedback can be easily addressed.
> Regarding making const accessors, that won't probably work after caching is
> implemented in part 3. But maybe I can just make mMathVariantCache a mutable
> member.

Yeah, that's what I'd be inclined to do. The API is logically const, and the (mutable) cache is purely an internal implementation detail. So if it works to do that, and doesn't run into other complications, I'd say go for it.
Flags: needinfo?(jfkthame)
(Assignee)

Comment 19

8 months ago
Created attachment 8799405 [details] [diff] [review]
Part 1 - Use HarfBuzz ot-math API to parse the OpenType MATH table
Attachment #8799220 - Attachment is obsolete: true
Attachment #8799364 - Attachment is obsolete: true
(Assignee)

Comment 20

8 months ago
Created attachment 8799407 [details] [diff] [review]
Part 2 - gfxMathTable: Cache MathVariants data
(Assignee)

Updated

8 months ago
Depends on: 1306543
(Assignee)

Comment 21

8 months ago
(In reply to Jonathan Kew (:jfkthame) from comment #15)
> Comment on attachment 8796056 [details] [diff] [review]
> Part 1 - Upgrade HarfBuzz to fd7a245d3525905ffbce57472b52900fcb0e330b
> 
> Review of attachment 8796056 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, in principle, but let's see if we can get an official release sometime
> soon.

This no longer applies on ToT, so let's wait a bit to see if a new version of HarfBuzz is released before updating the source again.
(Assignee)

Comment 22

8 months ago
Created attachment 8799543 [details] [diff] [review]
Part 1 - Upgrade HarfBuzz to 7201fdd0a8e26d49b13e289b53de375d5b1c9fcb
Attachment #8796056 - Attachment is obsolete: true
(Assignee)

Comment 23

8 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c787ae62c49c43e02ef07839ea79cdc9dd102ee9
(Assignee)

Comment 24

8 months ago
Some test failures to check:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7d024358ab5
(Assignee)

Updated

7 months ago
Depends on: 1313097
(Assignee)

Updated

7 months ago
Attachment #8799405 - Attachment description: Part 2 - Use HarfBuzz ot-math API to parse the OpenType MATH table → Part 1 - Use HarfBuzz ot-math API to parse the OpenType MATH table
Attachment #8799405 - Attachment filename: bug1305977-2.patch → bug-1305977-1.patch
(Assignee)

Updated

7 months ago
Attachment #8799407 - Attachment description: Part 3 - gfxMathTable: Cache MathVariants data → Part 2 - gfxMathTable: Cache MathVariants data
Attachment #8799407 - Attachment filename: bug1305977-3.patch → bug-1305977-2.patch
(Assignee)

Updated

7 months ago
Attachment #8799543 - Attachment is obsolete: true
(Assignee)

Comment 25

7 months ago
Comment on attachment 8799405 [details] [diff] [review]
Part 1 - Use HarfBuzz ot-math API to parse the OpenType MATH table

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

I submitted a try build again now that HarfBuzz has been updated: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18032aeea46fd3bcff42c8a4fc975672a5e1e7d6
However, there will probably still be failures again (see below).

::: gfx/thebes/gfxFont.cpp
@@ +4020,5 @@
> +        if (!face) {
> +            return false;
> +        }
> +
> +        mMathTable = MakeUnique<gfxMathTable>(face, GetAdjustedSize());

There is a subtle change here: TryGetMathTable used to return false when the font does not contain any MATH table. Now, it succeeds whenever the hb_face_t can be obtained. I believe we should add a check for hb_ot_math_has_data here.
(Assignee)

Comment 26

7 months ago
Created attachment 8805778 [details] [diff] [review]
Part 1bis - Make gfxFont::TryGetMathTable check the present of a MATH table

https://treeherder.mozilla.org/#/jobs?repo=try&revision=adcc21b52376b42599f58793159c051ca3127baf
Attachment #8805778 - Flags: review?(jfkthame)
Comment on attachment 8805778 [details] [diff] [review]
Part 1bis - Make gfxFont::TryGetMathTable check the present of a MATH table

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

::: gfx/thebes/gfxFont.cpp
@@ +4016,5 @@
>      if (!mMathInitialized) {
>          mMathInitialized = true;
>  
>          hb_face_t *face = GetFontEntry()->GetHBFace();
> +        if (!face || !hb_ot_math_has_data(face)) {

Ah, right - good catch, that could mislead us into using the wrong font. However, it looks like this will leak the face if it has no math table. I'd suggest rearranging as:

        if (face) {
            if (hb_ot_math_has_data(face)) {
                mMathTable = ...
            }
            hb_face_destroy(face);
        }

to ensure it's always released properly. r=me with an adjustment along these lines.
Attachment #8805778 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 28

7 months ago
(In reply to Jonathan Kew (:jfkthame) from comment #27)
> Ah, right - good catch, that could mislead us into using the wrong font.

Yes, and the reftests result confirmed that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=18032aeea46fd3bcff42c8a4fc975672a5e1e7d6 VS
https://treeherder.mozilla.org/#/jobs?repo=try&revision=adcc21b52376b42599f58793159c051ca3127baf

> However, it looks like this will leak the face if it has no math table. I'd
> suggest rearranging as:

OK, I'll do that and squash the patches before landing.
(Assignee)

Comment 29

7 months ago
Created attachment 8805910 [details] [diff] [review]
Patch

Squashing the patches into a single patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b60c9cd1db2add23dadec74ec55d43f442ae3ea
Attachment #8799405 - Attachment is obsolete: true
Attachment #8799407 - Attachment is obsolete: true
Attachment #8805778 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 30

7 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/961a84574836
Use HarfBuzz ot-math API to parse the OpenType MATH table. r=jfkthame
Keywords: checkin-needed

Comment 31

7 months ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/374fb57de7e2
Use HarfBuzz ot-math API to parse the OpenType MATH table: disable test multiscripts-1.html on Windows. r=developer-request on a CLOSED TREE
Depends on: 1314684

Comment 32

7 months ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d53a00dfd353
Use HarfBuzz ot-math API to parse the OpenType MATH table: really disable test multiscripts-1.html on Windows. r=developer-request
I had to back this out for windows reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=38550058&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/773aed2f801b27cbad3c031fc7aa747ea9b2c1ed
Flags: needinfo?(fred.wang)

Comment 34

7 months ago
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3898797a6267
Use HarfBuzz ot-math API to parse the OpenType MATH table. r=jfkthame
Ugh, sorry, didn't catch that this had a followup land.

Relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/3898797a6267e6d0f84049258e261e207fafd4e5
Flags: needinfo?(fred.wang)

Comment 36

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/374fb57de7e2
https://hg.mozilla.org/mozilla-central/rev/d53a00dfd353
https://hg.mozilla.org/mozilla-central/rev/3898797a6267
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.