Bug 130324 - Operator stretching: read the Open Type MATH table
Summary: Operator stretching: read the Open Type MATH table
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: MathML (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Frédéric Wang (:fredw)
URL: http://www.ntg.nl/maps/38/03.pdf
Keywords:
Depends on: 130572
Blocks: 122297 130322 130325
  Show dependency treegraph
 
Reported: 2014-03-17 00:11 PDT by Frédéric Wang (:fredw)
Modified: 2014-04-02 06:02 PDT (History)
9 users (show)

See Also:


Attachments
WIP Patch (33.36 KB, patch)
2014-03-17 06:13 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch + bug 130572 for testing (42.95 KB, patch)
2014-03-21 04:11 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch + 130572 for testing (43.90 KB, patch)
2014-03-21 04:34 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch + 130572 for testing (43.40 KB, patch)
2014-03-21 04:52 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (25.62 KB, patch)
2014-03-21 05:49 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (26.17 KB, patch)
2014-03-24 09:31 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (25.78 KB, patch)
2014-03-27 01:00 PDT, Frédéric Wang (:fredw)
cfleizach: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2014-03-17 00:11:57 PDT
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.
Comment 1 Frédéric Wang (:fredw) 2014-03-17 02:07:13 PDT
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.
Comment 2 Frédéric Wang (:fredw) 2014-03-17 06:13:14 PDT
Created attachment 226905 [details]
WIP Patch

This applies on top of bug 130321.
Comment 3 Frédéric Wang (:fredw) 2014-03-21 04:11:53 PDT
Created attachment 227410 [details]
Patch + bug 130572 for testing
Comment 4 Frédéric Wang (:fredw) 2014-03-21 04:34:50 PDT
Created attachment 227414 [details]
Patch + 130572 for testing
Comment 5 Frédéric Wang (:fredw) 2014-03-21 04:52:20 PDT
Created attachment 227416 [details]
Patch + 130572 for testing
Comment 6 Frédéric Wang (:fredw) 2014-03-21 05:49:22 PDT
Created attachment 227420 [details]
Patch
Comment 7 Frédéric Wang (:fredw) 2014-03-24 09:31:20 PDT
Created attachment 227657 [details]
Patch
Comment 8 chris fleizach 2014-03-26 09:03:47 PDT
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
Comment 9 Frédéric Wang (:fredw) 2014-03-26 09:11:10 PDT
(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.
Comment 10 Frédéric Wang (:fredw) 2014-03-27 00:48:45 PDT
(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?
Comment 11 Frédéric Wang (:fredw) 2014-03-27 01:00:31 PDT
Created attachment 227932 [details]
Patch
Comment 12 chris fleizach 2014-03-27 09:04:51 PDT
(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
Comment 13 Frédéric Wang (:fredw) 2014-04-02 06:02:07 PDT
Committed r166640: <http://trac.webkit.org/changeset/166640>