You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
OtlDump extends TTFontFile so that it cannot disagree with it, and then overrides the four table readers wholesale. Its class docblock states the intent:
Extends the parser the renderer uses, rather than being a second copy of it. That is the whole point: a debugging tool that parses independently is free to disagree with the thing it is meant to explain, and is useless exactly when it is needed. What it overrides here is reporting - the four table readers emit HTML as they go - not reading.
The reporting is not all that is overridden. _getGSUBtables() and _getGPOStables() are replaced in full, and most of what they replace is reading:
method
TTFontFile
OtlDump
lines identical ignoring indentation
_getGSUBtables()
TTFontFile.php:1339-2388, 978 non-blank lines
OtlDump.php:907-1807, 856 non-blank lines
665, or 78% of the OtlDump copy
_getGPOStables()
TTFontFile.php:3309-3500, 173 non-blank lines
OtlDump.php:2718-2920, 191 non-blank lines
132, or 69%
Some of the difference is legitimate and should stay: the dumper emits HTML and reads deeper into subtables so it can print them, and it does not build GSLuCoverage or write the font cache, because it does not shape. The ~800 lines of identical offset arithmetic in between are the problem.
It has already cost us
Three commits in this area are the same change made twice, or a change made once that should have been made twice:
6d059a7 "Report the tables a font does not carry as empty, in the dump as well as the renderer" - the title is the duplication.
5fd2b4c "Make the OTL dump extend the parser it is meant to explain" set out to end this and stopped at the class declaration.
And #110 has just added one more. A Multiple Substitution to the empty sequence is how a font deletes a glyph. OtlDump.php:1460 now reads it and reports the deletion; TTFontFile.php:1836 still skips it, under a comment that says it is illegal:
if (!isset($Lookup[$i]['Subtable'][$c]['Sequences'][$g]['SubstituteGlyphID']) || count(...) == 0) {
continue;
} // Illegal for GlyphCount to be 0; either error in font, or something has gone wrong - lets carry on for now!
It is not illegal - the OpenType spec allows a Sequence of no glyphs, HarfBuzz honours it, and since #110 so does the shaper. So the dump now reports a rule the parser it is meant to explain does not record, which is the failure mode the class docblock is about.
The comment, which is worth fixing on its own
Nothing observable turns on the skip in TTFontFile, and this was measured rather than assumed. Removing it and regenerating every fixture moves nothing: no tests/data/fontcache/*.json changes and all 1,668 tests pass, and that is not a vacuous check any more, because NotoSans-GSUB53-NestedAdvance-Synthetic (added in #110) carries two Sequences of no glyphs and a GDEF table, so it reaches this code. The two NotoEmoji faces, which hold 62 each, are refused at the GDEF check on TTFontFile.php:1251 long before _getGSUBarray() runs.
The reason it is inert is worth writing down where the skip is. 'subs' feeds _getGSUBarray(), which builds the derived tables - rtlSUB and finals for Arabic and Syriac joining, rphf/half/pref/blwf/pstf for Indic, rtlPUAstr for magic_reverse - and their consumers filter on the feature tag and on the replacement being non-empty:
if ($key && strlen(trim($key)) == 5 && $sub) {
A deletion's replacement is the empty string, so it is dropped downstream whether or not the parser records it. The substituting itself happens in Otl from GSUBLookups, which #110 fixed.
So the skip should stay and the comment should change: say the entry is passed over because the derived tables have no way to spell "and nothing in its place" and their readers discard an empty replacement anyway, not that the font is malformed. Three lines, no behaviour change - small enough to ride along with whatever next touches that file rather than justify a pull request of its own.
Putting a deletion into $volt instead would push an empty replacement into a regex-driven Arabic and Indic path, and no font in the corpus can demonstrate whether the result is right. That is not worth doing without a reason.
What a fix looks like
Lift the reading out of both copies. The dumper's extra work is interleaved with the reading rather than wrapped around it, so this is not one extraction: the shape that fits is for the parser's reader to record what it read, and for the dumper to override small per-structure hooks that render it - which is close to what 222dd64 "Give every GSUB and GPOS subtable structure its own method" did for the shaper.
It is worth doing in pieces, and _getGPOStables() is the smaller of the two to start with.
The three golden masters make this safe to attempt: tests/data/fontcache/*.json captures everything TTFontFile hands the shaper, tests/data/otldump/*.txt captures what the dump prints, and both cover every font in tests/data/ttf. A refactor that leaves all of them byte-identical has not changed what either half does.
OtlDumpextendsTTFontFileso that it cannot disagree with it, and then overrides the four table readers wholesale. Its class docblock states the intent:The reporting is not all that is overridden.
_getGSUBtables()and_getGPOStables()are replaced in full, and most of what they replace is reading:TTFontFileOtlDump_getGSUBtables()TTFontFile.php:1339-2388, 978 non-blank linesOtlDump.php:907-1807, 856 non-blank linesOtlDumpcopy_getGPOStables()TTFontFile.php:3309-3500, 173 non-blank linesOtlDump.php:2718-2920, 191 non-blank linesSome of the difference is legitimate and should stay: the dumper emits HTML and reads deeper into subtables so it can print them, and it does not build
GSLuCoverageor write the font cache, because it does not shape. The ~800 lines of identical offset arithmetic in between are the problem.It has already cost us
Three commits in this area are the same change made twice, or a change made once that should have been made twice:
5cc2ed9"Read a GSUB subtable's record offsets unsigned, so a subtable over 32 KB still resolves (A font that shapes by expansion leaves its marker glyphs on the page #100)" patched the same offset read inOtl.phpandOtlDump.php.6d059a7"Report the tables a font does not carry as empty, in the dump as well as the renderer" - the title is the duplication.5fd2b4c"Make the OTL dump extend the parser it is meant to explain" set out to end this and stopped at the class declaration.And #110 has just added one more. A Multiple Substitution to the empty sequence is how a font deletes a glyph.
OtlDump.php:1460now reads it and reports the deletion;TTFontFile.php:1836still skips it, under a comment that says it is illegal:It is not illegal - the OpenType spec allows a Sequence of no glyphs, HarfBuzz honours it, and since #110 so does the shaper. So the dump now reports a rule the parser it is meant to explain does not record, which is the failure mode the class docblock is about.
The comment, which is worth fixing on its own
Nothing observable turns on the skip in
TTFontFile, and this was measured rather than assumed. Removing it and regenerating every fixture moves nothing: notests/data/fontcache/*.jsonchanges and all 1,668 tests pass, and that is not a vacuous check any more, becauseNotoSans-GSUB53-NestedAdvance-Synthetic(added in #110) carries two Sequences of no glyphs and a GDEF table, so it reaches this code. The two NotoEmoji faces, which hold 62 each, are refused at the GDEF check onTTFontFile.php:1251long before_getGSUBarray()runs.The reason it is inert is worth writing down where the skip is.
'subs'feeds_getGSUBarray(), which builds the derived tables -rtlSUBandfinalsfor Arabic and Syriac joining,rphf/half/pref/blwf/pstffor Indic,rtlPUAstrformagic_reverse- and their consumers filter on the feature tag and on the replacement being non-empty:A deletion's replacement is the empty string, so it is dropped downstream whether or not the parser records it. The substituting itself happens in
OtlfromGSUBLookups, which #110 fixed.So the skip should stay and the comment should change: say the entry is passed over because the derived tables have no way to spell "and nothing in its place" and their readers discard an empty replacement anyway, not that the font is malformed. Three lines, no behaviour change - small enough to ride along with whatever next touches that file rather than justify a pull request of its own.
Putting a deletion into
$voltinstead would push an empty replacement into a regex-driven Arabic and Indic path, and no font in the corpus can demonstrate whether the result is right. That is not worth doing without a reason.What a fix looks like
Lift the reading out of both copies. The dumper's extra work is interleaved with the reading rather than wrapped around it, so this is not one extraction: the shape that fits is for the parser's reader to record what it read, and for the dumper to override small per-structure hooks that render it - which is close to what
222dd64"Give every GSUB and GPOS subtable structure its own method" did for the shaper.It is worth doing in pieces, and
_getGPOStables()is the smaller of the two to start with.The three golden masters make this safe to attempt:
tests/data/fontcache/*.jsoncaptures everythingTTFontFilehands the shaper,tests/data/otldump/*.txtcaptures what the dump prints, and both cover every font intests/data/ttf. A refactor that leaves all of them byte-identical has not changed what either half does.