1074223 - update OTS to pick up the fix from https://github.com/khaledhosny/ots/pull/35

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Comment 1

[Jonathan Kew [:jfkthame]]

If we also take the changes from https://github.com/khaledhosny/ots/pull/37, this will allow us to resolve bug 1074809.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Update OTS to pick up fixes for upstream issues 35, 37. Current rev: c24a839b1c66c4de09e58fabaacb82bf3bd692a4.

Comment 3

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ec8e8f037bb

Comment 4

[Jens]

Attachment: MisterKInformalWebPro_subset.ttf

I found another font that is erroneously rejected by OTS in the current Nightly 35.0a1 (2014-10-08) (GSUB table is discarded), but not by the upstream OTS version (https://github.com/khaledhosny/ots/commit/c24a839b1c66c4de09e58fabaacb82bf3bd692a4).

I believe that the font is valid. The GSUB table is valid when tested in MS Font Validator.

This is the console message for the equivalent WOFF file:

downloadable font: Layout: bad glyph count: 253 (font-family: "Web FontFont" style:normal weight:normal stretch:normal src index:0)
source: file:///Mister%20K/MisterKInformalWebPro_subset.woff MisterKInformalWebPro_subset.woff_proof.html:204
downloadable font: Layout: Failed to parse lookahead coverage table 0 in chain context format 3 (font-family: "Web FontFont" style:normal weight:normal stretch:normal src index:0)
source: file:///Mister%20K/MisterKInformalWebPro_subset.woff MisterKInformalWebPro_subset.woff_proof.html:204
downloadable font: Layout: Failed to parse chaining context format 3 subtable (font-family: "Web FontFont" style:normal weight:normal stretch:normal src index:0)
source: file:///Mister%20K/MisterKInformalWebPro_subset.woff MisterKInformalWebPro_subset.woff_proof.html:204
downloadable font: Layout: Failed to parse lookup subtable 5 (font-family: "Web FontFont" style:normal weight:normal stretch:normal src index:0)
source: file:///MisterKInformalWebPro_subset.woff MisterKInformalWebPro_subset.woff_proof.html:204
downloadable font: Layout: Failed to parse subtable 1 (font-family: "Web FontFont" style:normal weight:normal stretch:normal src index:0)
source: file:///Mister%20K/MisterKInformalWebPro_subset.woff MisterKInformalWebPro_subset.woff_proof.html:204
downloadable font: Layout: Failed to parse lookup 10 (font-family: "Web FontFont" style:normal weight:normal stretch:normal src index:0)
source: file:///Mister%20K/MisterKInformalWebPro_subset.woff MisterKInformalWebPro_subset.woff_proof.html:204
downloadable font: GSUB: Failed to parse lookup list table, table discarded (font-family: "Web FontFont" style:normal weight:normal stretch:normal src index:0)
source: file:///MisterKInformalWebPro_subset.woff

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Jens from comment #4)
> Created attachment 8502404 [details]
> MisterKInformalWebPro_subset.ttf
> 
> I found another font that is erroneously rejected by OTS in the current
> Nightly 35.0a1 (2014-10-08) (GSUB table is discarded), but not by the
> upstream OTS version
> (https://github.com/khaledhosny/ots/commit/
> c24a839b1c66c4de09e58fabaacb82bf3bd692a4).
> 
> I believe that the font is valid. The GSUB table is valid when tested in MS
> Font Validator.

I dumped the GSUB table from that font using TTX, and it looks to me like it is erroneous. An extract from Lookup index="10":

        <ChainContextSubst index="1" Format="3">
          <!-- BacktrackGlyphCount=0 -->
          <!-- InputGlyphCount=1 -->
          <InputCoverage index="0" Format="1">
            <Glyph value="d"/>
          </InputCoverage>
          <!-- LookAheadGlyphCount=1 -->
          <LookAheadCoverage index="0" Format="1">
            <Glyph value="H"/>
            <Glyph value="a"/>
            <Glyph value="a"/>
            <Glyph value="a"/>
            <Glyph value="d"/>
            <Glyph value="d"/>
            <Glyph value="d"/>
            <Glyph value="d"/>
            <Glyph value="e"/>
            <Glyph value="e"/>
            <Glyph value="e"/>
            <Glyph value="g"/>
            <Glyph value="g"/>
            <Glyph value="g"/>
            <Glyph value="l"/>
            <Glyph value="l"/>
            <Glyph value="l"/>
            <Glyph value="n"/>
            <Glyph value="n"/>
            <Glyph value="o"/>
            <Glyph value="o"/>
            <Glyph value="o"/>
            <Glyph value="s"/>
            <Glyph value="s"/>
            <Glyph value="s"/>
            <Glyph value="s"/>
            <Glyph value="v"/>
            <Glyph value="v"/>
            <Glyph value="v"/>
            <Glyph value="glyph00013"/>
            <Glyph value="glyph00013"/>
            <Glyph value="glyph00014"/>
            <Glyph value="glyph00015"/>

Note how there are lots of repeated glyphs in the list here. This should not be the case. Although the OpenType spec does not explicitly prohibit it (AFAICS), it makes a nonsense of the purpose of the Coverage table: "A Coverage table defines a unique index value (Coverage Index) for each covered glyph." If a glyph is listed repeatedly, it no longer has a unique index value!

So AFAICS the OTS messages quoted above are correct, and the font is faulty.

Comment 6

[Jens]

Whoa ... thanks for the tip. At first I thought that was an artifact of the subsetting that I did for this example font, but it turns out FontLab wrote the duplicate entries into the table.

I was able to fix the font with other tools. Thank you again.

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Jens from comment #6)
> Whoa ... thanks for the tip. At first I thought that was an artifact of the
> subsetting that I did for this example font, but it turns out FontLab wrote
> the duplicate entries into the table.

Oh, really? That sounds like a FontLab bug, then; I'd suggest you report it there.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ec8e8f037bb

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 8501939 [details] [diff] [review]
Update OTS to pick up fixes for upstream issues 35, 37. Current rev: c24a839b1c66c4de09e58fabaacb82bf3bd692a4.

Approval Request Comment
[Feature/regressing bug #]:
The OTS update in bug 1057488 led to an unplanned change in warning-logging behavior that can result in spamming the developer console with messages.

[User impact if declined]:
Potential multi-second hang on some pages that use webfonts, see bug 1074809.

[Describe test coverage new/current, TBPL]:
Existing webfont tests exercise this code.

[Risks and why]:
Low risk, updating to new version of upstream library.

[String/UUID change made/needed]:
none

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

I'd like to see this on m-c for a day or two before uplifting. If everything goes well on m-c, let's see if we can uplift on Sunday or, if not, before beta1.

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 8501939 [details] [diff] [review]
Update OTS to pick up fixes for upstream issues 35, 37. Current rev: c24a839b1c66c4de09e58fabaacb82bf3bd692a4.

Moving pending approval request from aurora to beta, as merge has happened in the meantime.

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8501939 [details] [diff] [review]
Update OTS to pick up fixes for upstream issues 35, 37. Current rev: c24a839b1c66c4de09e58fabaacb82bf3bd692a4.

This has been on m-c for a bit now. Although a big change, I'm going to approve this for beta2 to address the multi-second hangs. Beta+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6524ec11ce53