Skip to content

Correct GDEF MarkGlyphSets offset and UseMarkFilteringSet semantics - #14

Merged
jakejackson1 merged 1 commit into
gravitypdffrom
fix/markglyphsets-coverage-offset
Sep 7, 2026
Merged

Correct GDEF MarkGlyphSets offset and UseMarkFilteringSet semantics#14
jakejackson1 merged 1 commit into
gravitypdffrom
fix/markglyphsets-coverage-offset

Conversation

@jakejackson1

@jakejackson1 jakejackson1 commented Sep 7, 2026

Copy link
Copy Markdown
Member

Summary

Two bugs here, and the first was hiding the second.

_getGDEFtables() reads the GDEF MarkGlyphSetsDef coverage offsets and then seeks to them as absolute file offsets. Per the OpenType spec they are ULONGs measured from the start of the MarkGlyphSetsDef table, 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 UseMarkFilteringSet with 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,099 Undefined array key notices on one short render. Both are Google's current static builds.

Correcting the offset then reveals that the UseMarkFilteringSet handling 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-shape says 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 subs key — 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 a TypeError.

Try it

$mpdf = new \Mpdf\Mpdf([
    'fontDir'      => ['/path/to/fonts'],
    'fontdata'     => ['montserrat' => ['R' => 'Montserrat-Regular.ttf', 'useOTL' => 0xFF]],
    'default_font' => 'montserrat',
]);
$mpdf->WriteHTML('<p>Waffle office AVATAR</p>');
$mpdf->Output();

With Google's current static Montserrat this exhausts memory in _getGDEFtables() on development and renders here. Open Sans gives the notice storm instead of the fatal.

Test plan

  • composer test — 1049 tests, 2531 assertions, OK
  • composer cs — clean
  • New unit test pins the parsed coverage; fails on development with [' ', ' ']
  • New snapshot covers the mark-filtered ligatures and distinguishes all three states: 1,654px against offset-fix-only, 107px against development
  • රි රී ර් match hb-shape (gid28/gid29/gid27) after the fix; they do not with the offset fix alone
  • Montserrat, Open Sans, Lora, Merriweather, Playfair Display, Roboto, Caveat, Dancing Script, Pacifico, Great Vibes, JetBrains Mono and Inconsolata all parse at useOTL => 0xFF with zero notices
Detail: the offsets, the inverted flag, and what the snapshot is for

The offset

MarkGlyphSetsDef sits at GDEF start + MarkGlyphSetsDef offset, and each coverage offset is relative to that table:

Font GDEF at MarkGlyphSetsDef Coverage offsets Correct absolute mPDF seeked to
Montserrat 183720 +1036 20, 96, 106, 146 184776, 184852, 184862, 184902 20, 96, 106, 146
Open Sans 87580 +360 16, 52, 104 87956, 87992, 88044 16, 52, 104
Noto Sans Sinhala 82440 +96 12, 22 82548, 82558 12, 22

Offsets like 12 and 20 land inside the sfnt header and table directory. A CoverageFormat read 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 a RangeCount and start/end that 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. For UseMarkFilteringSet that must be GlyphClassMarks minus the filtering set; a new marksOutsideFilteringSet() 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 are U+0DBB + each of those three, plus U+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 development is mark positioning on ා්. hb-shape maps 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.2 MarkGlyphSets table, 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.

@jakejackson1 jakejackson1 added bug Something isn't working create-upstream-pr labels Sep 7, 2026
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
jakejackson1 force-pushed the fix/markglyphsets-coverage-offset branch from 5f0dc5e to bbed232 Compare September 7, 2026 01:59
@jakejackson1 jakejackson1 changed the title Read GDEF MarkGlyphSets coverage at the right offset Correct GDEF MarkGlyphSets offset and UseMarkFilteringSet semantics Sep 7, 2026
@jakejackson1
jakejackson1 merged commit 4cbfaec into gravitypdf Sep 7, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working create-upstream-pr

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant