Bug 130324 – Operator stretching: read the Open Type MATH table

• Component: MathML
• Status: RESOLVED FIXED

[Frédéric Wang (:fredw)]

Third step for attachment 226844 [details].

On some platforms (Linux/Windows), we can directly read the MATH data from the font and thus get a generic support. On other platforms (Mac), we will still use the hardcoded data introduced in bug 130321.

I'll add an ENABLE_OPENTYPE_MATH compilation flag for that purpose.

[Frédéric Wang (:fredw)]

For the MATH data, see http://mpeg.chiariglione.org/standards/mpeg-4/open-font-format/call-proposals-isoiec-14496-22-open-font-format-color-font

I'm not sure if April's Committee Draft will be made public, but anyone interested in the MATH table can ask me a copy of Microsoft's document.

[Frédéric Wang (:fredw)]

Created attachment 226905 [details]
WIP Patch

This applies on top of bug 130321.

[Frédéric Wang (:fredw)]

Created attachment 227410 [details]
Patch + bug 130572 for testing

[Frédéric Wang (:fredw)]

Created attachment 227414 [details]
Patch + 130572 for testing

[Frédéric Wang (:fredw)]

Created attachment 227416 [details]
Patch + 130572 for testing

[Frédéric Wang (:fredw)]

Created attachment 227420 [details]
Patch

[Frédéric Wang (:fredw)]

Created attachment 227657 [details]
Patch

[chris fleizach]

Comment on attachment 227657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227657&action=review

> Source/WTF/wtf/Platform.h:1053
> +#if OS(DARWIN) || USE(FREETYPE) || (PLATFORM(WIN) && (USE(CG) || USE(CAIRO)))

i think the OS(DARWIN) also needs the USE(CG) macro to go along with it right

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:44
> +#pragma pack(1)

are you trying to save space here or is the required for reading math data? 
how much extra memory will be used by reading in this data?

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:78
> +        if (!getCoverageIndex(buffer, coverage, glyph, i) || i>= count)

i >=

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:162
> +    OpenType::Offset vertGlyphCoverageOffset;

my thought is we should just use "vertical" and horizontal instead of these abbreviations. The names are already really long anyway

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:242
> +    const OpenType::MathConstants* mathconstants = math->mathConstants(*m_mathBuffer);

mathConstants for the name

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:248
> +    const OpenType::MathVariants* mathvariants = math->mathVariants(*m_mathBuffer);

mathVariants

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:259
> +    int32_t value = 0;

seems like you could declare this as int16_t and avoid the casts when assigning the value

> Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:266
> +    if (constant <= ScriptScriptPercentScaleDown)

for these if/else statements it seems we should also check that
   constants >= 0 in this case (or rather ScriptPercentScaleDown I believe)
and likewise is >= X in the other else if cases

> Source/WebCore/platform/graphics/opentype/OpenTypeTypes.h:127
> +    getCoverageIndex(const SharedBuffer& buffer, const CoverageTable* coverage, Glyph glyph, uint32_t& coverageIndex) const

this should be on the same line as above

[Frédéric Wang (:fredw)]

(In reply to comment #8)
> > Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:44
> > +#pragma pack(1)
> 
> are you trying to save space here or is the required for reading math data? 
> how much extra memory will be used by reading in this data?
> 

No ideas, I just copied that from OpenTypeVerticalData. I'm assuming it was necessary to prevent unexpected data rearrangement but didn't really think about it.

[Frédéric Wang (:fredw)]

(In reply to comment #8)
> > Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:259
> > +    int32_t value = 0;
> 
> seems like you could declare this as int16_t and avoid the casts when assigning the value

Some values are int16_t and others are uint16_t, so that would mean declaring two variables and duplicating the conversion to % / em?

[Frédéric Wang (:fredw)]

Created attachment 227932 [details]
Patch

[chris fleizach]

(In reply to comment #10)
> (In reply to comment #8)
> > > Source/WebCore/platform/graphics/opentype/OpenTypeMathData.cpp:259
> > > +    int32_t value = 0;
> > 
> > seems like you could declare this as int16_t and avoid the casts when assigning the value
> 
> Some values are int16_t and others are uint16_t, so that would mean declaring two variables and duplicating the conversion to % / em?

as missed the unit v. int

[Frédéric Wang (:fredw)]

Committed r166640: <http://trac.webkit.org/changeset/166640>