Correct GDEF MarkGlyphSets offset and UseMarkFilteringSet semantics - #14
Merged
Merged
Conversation
Two bugs, one of which had been hiding the other. _getGDEFtables() read the MarkGlyphSetsDef coverage offsets and seeked to them as absolute file offsets. Per the spec they are ULONGs measured from the start of the MarkGlyphSetsDef table, so the seek landed in the sfnt header and mPDF parsed the table directory as a Coverage table. Every GDEF 1.2 font therefore ended up with empty mark glyph sets; where the garbage decoded as a format 2 range it was not silent at all, and current Google builds of Montserrat (512 MB exhausted) and Open Sans (78,099 undefined-key notices) both fail on it. With real sets in hand, the UseMarkFilteringSet handling turns out to be inverted. The flag means "skip every mark except those in the set", but both the ignore-string builders and the per-glyph ignore checks treated the set itself as the glyphs to skip. That is the wrong way round, and with empty sets it never showed: correcting only the offset drops the mark-filtered ligatures in Noto Sans Sinhala back to base + floating mark, which hb-shape contradicts. Also guards two dereferences of a subtable whose entries were all filtered out by the Ignore flags, which then has no 'subs' key. Not reachable from the test corpus now that the semantics are right, but it is a legal state and the alternative is a TypeError. Adds a snapshot covering the mark-filtered ligature sequences, which no existing test touched: none of the 165 package fonts carries a GDEF 1.2 MarkGlyphSets table, so this code had no render-level coverage at all.
jakejackson1
force-pushed
the
fix/markglyphsets-coverage-offset
branch
from
September 7, 2026 01:59
5f0dc5e to
bbed232
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two bugs here, and the first was hiding the second.
_getGDEFtables()reads the GDEFMarkGlyphSetsDefcoverage offsets and then seeks to them as absolute file offsets. Per the OpenType spec they are ULONGs measured from the start of theMarkGlyphSetsDeftable, so the seek lands near the top of the file — usually in the table directory — and mPDF parses those header bytes as a Coverage table.For most fonts that is quiet: every font with a GDEF 1.2 table has silently ended up with empty mark glyph sets, disabling every lookup flagged
UseMarkFilteringSetwith no indication it happened. Where the garbage bytes decode as a format 2 range it is not quiet at all — Montserrat exhausts 512 MB and dies, and Open Sans emits 78,099Undefined array keynotices on one short render. Both are Google's current static builds.Correcting the offset then reveals that the
UseMarkFilteringSethandling is inverted. The flag means "skip every mark except those in the given set", but both ignore-string builders and both per-glyph ignore checks treated the set itself as the glyphs to skip. With empty sets that never surfaced. With real ones it does immediately: the mark-filtered ligature lookups in Noto Sans Sinhala have their own components skipped, soරිරීර්fall back to base + floating mark.hb-shapesays they should be single ligature glyphs (gid28,gid29,gid27), and with the semantics corrected mPDF produces exactly that again.This also revises what #1 landed: that change set
$str = $this->MarkGlyphSets[$MarkFilteringSet]in_getGCOMignoreString(), which is the inverted form. It produced correct output only because the sets it read were empty.Two dereferences of a subtable whose entries were all filtered out by the Ignore flags — which then never gets a
subskey — are guarded as well. With the semantics corrected nothing in the test corpus reaches that state, so the guards are defensive, but it is a legal condition and the alternative is aTypeError.Try it
With Google's current static Montserrat this exhausts memory in
_getGDEFtables()ondevelopmentand renders here. Open Sans gives the notice storm instead of the fatal.Test plan
composer test— 1049 tests, 2531 assertions, OKcomposer cs— cleandevelopmentwith[' ', ' ']developmentරිරීර්matchhb-shape(gid28/gid29/gid27) after the fix; they do not with the offset fix aloneuseOTL => 0xFFwith zero noticesDetail: the offsets, the inverted flag, and what the snapshot is for
The offset
MarkGlyphSetsDefsits atGDEF start + MarkGlyphSetsDef offset, and each coverage offset is relative to that table:MarkGlyphSetsDefOffsets like 12 and 20 land inside the sfnt header and table directory. A
CoverageFormatread from there is arbitrary: values other than 1 or 2 leave the set empty, 1 gives bogus glyph IDs ($glyphToChar[$glyphID][0]is null → the notices), and 2 gives aRangeCountandstart/endthat can span tens of thousands of glyphs → the OOM.The inverted flag
_getGSUBignoreString()and_getGCOMignoreString()build the set of glyphs to skip between matched glyphs. ForUseMarkFilteringSetthat must beGlyphClassMarksminus the filtering set; a newmarksOutsideFilteringSet()helper computes it in both classes._checkGSUBignore()and_checkGCOMignore()had the same inversion per glyph and now ignore a mark only when it is outside the set.The subset fixture's filtering set is
{U+0DCA, U+0DD2, U+0DD3}and its two mark-filtered ligature lookups areU+0DBB+ each of those three, plusU+0DCF+U+0DCA. Under the old reading the ignore pattern((?:(?: 00DCA| 00DD2| 00DD3))*)consumed the very component the ligature needed to match.The residual 107px against
developmentis mark positioning onා්.hb-shapemaps that pair to a single ligature (gid26) which mPDF does not apply in either version — a pre-existing gap this PR does not address.Why a snapshot
None of the 165 fonts in the
packages/tree carries a GDEF 1.2MarkGlyphSetstable, so the existing snapshot suite never reaches this code, and every unit test here asserts parsing or%PDF-rather than glyphs. Mark filtering going active-but-wrong was therefore invisible — which is exactly what happened when only the offset was corrected. The new snapshot is the first render-level coverage this code has.Note on the new unit test
It pins parsed content rather than "it didn't throw", because the old behaviour didn't throw either — it silently produced
[' ', ' ']. U+0DCA, U+0DD2 and U+0DD3 are the subset's Sinhala virama and vowel signs; the second set's single glyph has no cmap entry and is mapped into the Private Use Area.