Apply a nested lookup only to a glyph the subtable it names covers - #106
Open
jakejackson1 wants to merge 3 commits into
Open
Apply a nested lookup only to a glyph the subtable it names covers#106jakejackson1 wants to merge 3 commits into
jakejackson1 wants to merge 3 commits into
Conversation
) A contextual or chaining lookup matches a sequence of glyphs on its own Coverages, then names other lookups to run at positions within that match. The glyph handed to one of those is whatever the context matched at that position; the subtables of the lookup it names each carry a Coverage of their own, and nothing compared the two. The three loops that apply a lookup from the top of the list do compare them - isset() on the subtable's Coverage before entering it. _applyGSUBlookupRecords() and _applyGPOSlookupRecords(), which apply what a matched context names, walked every subtable of the nested lookup unconditionally. Every handler then indexes that Coverage with no check of its own, because until now it could not miss: $GlyphPos = $LuCoverage[$currGID]; The index is not merely undefined. It is null, which is 0, so the handler reads whatever the subtable holds for the first glyph it covers and applies that to a glyph it was never meant to touch. It then reports having applied, so the loop breaks - and the subtable of the same lookup that does cover the glyph is never reached. The check goes in _applyGSUBsubtable() and _applyGPOSsubtable(), the one seam both callers pass through, rather than in the two *lookupRecords() methods, so that the invariant every handler below rests on is stated once where they all come past it. The three top-level loops keep theirs: a glyph no subtable covers then costs no seek, which is what #83 was about. ## What it was costing DejaVu Sans, which is shipped. It substitutes a shorter, higher form of a combining mark for the one written when it follows a capital, through a chained context naming a single substitution that covers the twelve marks it has such a form for. The context presents forty-four. The other thirty-two were all given Coverage Index 0 - the capital grave - so a macron, an overline, a hook above or a comma above written over a capital was drawn as a grave accent. Every one of them the same accent. Noto Sans Gurmukhi, the shape the issue was reported from. A `dist` chained context names a single adjustment at a Devanagari stress sign; that lookup's first subtable covers the bindi and its second covers the stress signs. The udatta and anudatta were positioned by the bindi's value record - on the baseline and 150 units to the left - instead of above and below the letter, and reading the Coverage raised a warning per glyph per run under E_ALL. Of the 104 fonts in packages/, tests/data/ttf and the current google/fonts releases of the families #104 names, 23 carry a nested lookup subtable a context can hand a glyph it does not cover: Padauk, Taamey David, sixteen faces of the DejaVu families, Free Serif, Noto Sans, and the Bengali, Devanagari and Gurmukhi Noto families. The issue reports none, because the check behind it was per lookup rather than per subtable - and a lookup can cover a glyph in its second subtable and still be entered at its first. Over the blocks those fonts shape - 8,067 runs across Gurmukhi, Devanagari, Bengali, Myanmar, Hebrew, Odia and the combining marks - 141 lines move, all of them in DejaVu Sans, DejaVu Serif and Noto Sans Gurmukhi, and all of them from a glyph or a position belonging to another character to the one the font asks for. Nothing else in the corpus moves, and the suite passes unchanged. ## The test font NotoSansGurmukhi-NestedCoverage-Subset, 5 glyphs of Noto Sans Gurmukhi 2.005 (OFL 1.1): the KA, the bindi, the two stress signs, and the conjunct that keeps the nested lookup's first subtable at the two entries which make it a format 2 - the format that indexes its Coverage, and so the one that reads a glyph it has not got rather than applying one record to everything. Nothing shipped reaches the positioning half; the substitution half is covered through DejaVu Sans, which is. Refs #102 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n a snapshot Both halves of this move a page, so the document is what a reader can check and the fixture is what stops either coming back. DejaVu Sans is the half that needs no font of its own. Ten combining marks are drawn over a capital A and again over a lowercase a, five the font has a capital form of and five it has not. The lowercase column is the mark as written and the capital column is what the font asks for, so a row shows both what should happen and what did: before the fix every mark in the second table was drawn as the capital grave of the first row above it, and the page was five À in a column labelled macron, overline, hook above, comma above and double overline. Noto Sans Gurmukhi is the half the issue was reported from, and the subset carries it. The udatta and anudatta are drawn on a KA, with the KA alone beside each so the mark can be seen against where the letter is, and the bindi under them - the glyph whose value record the two were being given. Two tests beside the snapshot read the glyphs off the page rather than naming them, since the capital forms have no codepoints of their own and are mapped into the Private Use Area as the subset is built. Each mark with no capital form has to be drawn as the mark that was written and not as the grave; each mark with one has to be drawn as something else, and as something different from the others, so that turning the rule off entirely would not pass. Both fail against the code before the fix, and so does the fixture. Refs #102 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…check is worth A review pass over the fix, none of which changes a page: 8,067 runs across the ten fonts that carry the shape and 4,096 across six Indic fonts are identical either way, and the suite passes unchanged. _applyGSUBsubtableSpecial() is the third public way into a subtable and was left indexing its Coverage unguarded, in exactly the caller-holds-the-invariant arrangement the fix rejected for the other two. It is indexed by the consonant after the Halant rather than by the glyph the cursor is on, so the guard is on $nextGID. Its one caller still checks, as the other loops do. _applyGPOSpairAdjustmentFormat1() was the one handler that defended itself. The isset() half of that is now dead, and leaving it in implies the invariant is not trusted - which is an invitation for the next handler to re-add its own. The two *lookupRecords() methods assigned $shift per record with no accumulation, so a record that applied nothing overwrote the shift of one that had. That was unreachable before: a nested subtable read Coverage Index 0 and reported having applied, whatever glyph it was handed. Returning 0 is the normal case now, so the path is live, and the docblock's "from the last nested lookup that shifted anything" describes what the code does rather than what it did. The docblock claimed the callers' check saves a seek. It does not any more - the guard stands ahead of the seek - so what it saves is the call. That is still worth keeping, and now measured: the callers turn away 88,700 of 90,000 glyphs on an Arabic run, leaning on the guard alone is 15% slower end to end, and keeping both costs 0.03 ms a page. _applyGSUBrulesIndic's copy could not have been removed anyway, its elseif being the Halant-Consonant case. The tests lose three assertions that restated their neighbours - the two-marks comparison is implied by the exact-array assertion above it, and the overline and the lowercase macron each repeat the line before - and the snapshot's glyphsOf() stops prepending the headings it then had to skip with end(). The breadth stays in the snapshot, which walks ten marks rather than three. Refs #102 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Closes #102.
A contextual or chaining lookup matches a sequence of glyphs on its own Coverages, then names other lookups to run at positions within that match. The glyph handed to one of those is whatever the context matched at that position; the subtables of the lookup it names each carry a Coverage of their own, and nothing compared the two.
Two commits:
Where the check goes
The three loops that apply a lookup from the top of the list test the glyph against each subtable's Coverage before entering it.
_applyGSUBlookupRecords()and_applyGPOSlookupRecords()walked every subtable of a nested lookup unconditionally, and every handler then indexes that Coverage with no check of its own, because until now it could not miss.The index is not merely undefined. It is
null, which is0, so the handler reads whatever the subtable holds for the first glyph it covers and applies that to a glyph it was never meant to touch. It then reports having applied, so the loop breaks — and the subtable of the same lookup that does cover the glyph is never reached.The issue asks where the check belongs. It is in
_applyGSUBsubtable()and_applyGPOSsubtable(), the one seam both callers pass through, so the invariant every handler below rests on is stated once where they all come past it. The three top-level loops keep theirs as a fast path: a glyph no subtable covers then costs noseek(), which is what #83 was about.What it was costing
DejaVu Sans, which is shipped. It substitutes a shorter, higher form of a combining mark for the one written when it follows a capital, through a chained context naming a single substitution that covers the twelve marks it has such a form for. The context presents forty-four. The other thirty-two were all given Coverage Index 0 — the capital grave — so a macron, an overline, a hook above or a comma above over a capital was drawn as a grave accent. Every one of them the same accent.
Noto Sans Gurmukhi, the shape the issue was reported from. A
distchained context names a single adjustment at a Devanagari stress sign; that lookup's first subtable covers the bindi and its second covers the stress signs. The udatta and anudatta were positioned by the bindi's record — on the baseline and 150 units to the left — instead of above and below the letter, and reading the Coverage raised a warning per glyph per run underE_ALL.How reachable
Of the 104 fonts in
packages/,tests/data/ttfand the currentgoogle/fontsreleases of the five families #104 names, 23 carry a nested lookup subtable a context can hand a glyph it does not cover: Padauk, Taamey David, sixteen faces of the DejaVu families, Free Serif, Noto Sans, and the Bengali, Devanagari and Gurmukhi Noto families.The issue reports none. The check behind that figure was per lookup; this one is per subtable, and a lookup can cover a glyph in its second subtable and still be entered at its first.
What moves
Every one of them is in DejaVu Sans, DejaVu Serif or Noto Sans Gurmukhi, and every one is a glyph or a position belonging to another character becoming the one the font asks for. Nothing else in the corpus moves, and the suite passes unchanged.
The test font
NotoSansGurmukhi-NestedCoverage-Subset— 5 glyphs of Noto Sans Gurmukhi 2.005 (OFL 1.1): the KA, the bindi, the two stress signs, and the conjunct that keeps the nested lookup's first subtable at the two entries which make it a format 2. Format 2 is the one that indexes its Coverage, so it is the one that reads a glyph it has not got rather than applying a single record to everything; a subsetter that drops the conjunct collapses it to format 1 and the diagnostic goes with it.Nothing shipped reaches the positioning half. The substitution half needs no font of its own — DejaVu Sans carries it.
🤖 Generated with Claude Code