From bbed232b57e615fd32b9cf1a9d5a8663dc8b7395 Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Mon, 7 Sep 2026 11:17:12 +1000 Subject: [PATCH] fix: correct GDEF MarkGlyphSets offset and UseMarkFilteringSet semantics 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. --- src/Otl.php | 35 ++++++++++- src/TTFontFile.php | 59 +++++++++++++++--- tests/Mpdf/TTFontFileTest.php | 13 ++++ tests/Snapshots/MarkGlyphSetsSnapshotTest.php | 51 +++++++++++++++ tests/data/snapshots/markglyphsets.pdf | Bin 0 -> 6036 bytes 5 files changed, 147 insertions(+), 11 deletions(-) create mode 100644 tests/Snapshots/MarkGlyphSetsSnapshotTest.php create mode 100644 tests/data/snapshots/markglyphsets.pdf diff --git a/src/Otl.php b/src/Otl.php index 3a458afac..21e8ef9d7 100644 --- a/src/Otl.php +++ b/src/Otl.php @@ -4474,6 +4474,33 @@ private function _getMarkRecord($offset, $MarkPos) return $MarkRecord; } + /** + * UseMarkFilteringSet means "skip every mark except those in the given mark glyph set", so the glyphs to + * ignore are GlyphClassMarks minus that set - not the set itself. + * + * @param string $marks Space-prefixed, "|"-separated glyph list, e.g. " 00DCA| 00DD2" + * @param string $set The mark glyph set, in the same format + * + * @return string + */ + private function marksOutsideFilteringSet($marks, $set) + { + $keep = []; + $inSet = []; + foreach (explode('|', $set) as $glyph) { + $inSet[trim($glyph)] = true; + } + + foreach (explode('|', $marks) as $glyph) { + $glyph = trim($glyph); + if ($glyph !== '' && !isset($inSet[$glyph])) { + $keep[] = $glyph; + } + } + + return $keep ? ' ' . implode('| ', $keep) : ''; + } + private function _getGCOMignoreString($flag, $MarkFilteringSet) { // If ignoreFlag set, combine all ignore glyphs into -> "(?:( 0FBA1| 0FBA2| 0FBA3)*)" @@ -4496,7 +4523,8 @@ private function _getGCOMignoreString($flag, $MarkFilteringSet) if ($MarkFilteringSet === '' || !isset($this->MarkGlyphSets[$MarkFilteringSet])) { throw new \Mpdf\MpdfException("This font [" . $this->fontkey . "] contains MarkGlyphSets - but MarkFilteringSet not set"); } - $str = $this->MarkGlyphSets[$MarkFilteringSet]; + $ignoreflag = $flag; + $str = $this->marksOutsideFilteringSet($this->GlyphClassMarks, $this->MarkGlyphSets[$MarkFilteringSet]); } // If Ignore Marks set, supercedes any above @@ -4550,8 +4578,9 @@ private function _checkGCOMignore($flag, $glyph, $MarkFilteringSet) $ignore = true; } } - // Flag & 0x0010 = UseMarkFilteringSet - if (($flag & 0x0010) && strpos($this->MarkGlyphSets[$MarkFilteringSet], $glyph)) { + // Flag & 0x0010 = UseMarkFilteringSet: skip every mark *except* those in the set + if (($flag & 0x0010) && strpos($this->GlyphClassMarks, $glyph) + && !strpos($this->MarkGlyphSets[$MarkFilteringSet], $glyph)) { $ignore = true; } return $ignore; diff --git a/src/TTFontFile.php b/src/TTFontFile.php index 3da167fb8..0c64db88a 100644 --- a/src/TTFontFile.php +++ b/src/TTFontFile.php @@ -1344,7 +1344,8 @@ function _getGDEFtables() $MarkSetOffset[] = $this->read_ulong(); } for ($i = 0; $i < $MarkSetCount; $i++) { - $this->seek($MarkSetOffset[$i]); + // Coverage offsets are relative to the MarkGlyphSetsDef table, not the file + $this->seek($gdef_offset + $MarkGlyphSetsDef_offset + $MarkSetOffset[$i]); $glyphs = $this->_getCoverage(); $this->MarkGlyphSets[$i] = ' ' . implode('| ', $glyphs); } @@ -2441,6 +2442,10 @@ function _getGSUBarray(&$Lookup, &$lul, $scripttag) } } // LookupType 2: Multiple Substitution Subtable elseif ($Lookup[$i]['Type'] == 2) { + if (!isset($Lookup[$i]['Subtable'][$c]['subs'])) { + continue; // every entry was filtered out by the Ignore flags + } + for ($s = 0; $s < count($Lookup[$i]['Subtable'][$c]['subs']); $s++) { $inputGlyphs = $Lookup[$i]['Subtable'][$c]['subs'][$s]['Replace']; $substitute = implode(" ", $Lookup[$i]['Subtable'][$c]['subs'][$s]['substitute']); @@ -2451,6 +2456,10 @@ function _getGSUBarray(&$Lookup, &$lul, $scripttag) } } // LookupType 3: Alternate Forms elseif ($Lookup[$i]['Type'] == 3) { + if (!isset($Lookup[$i]['Subtable'][$c]['subs'])) { + continue; // every entry was filtered out by the Ignore flags + } + for ($s = 0; $s < count($Lookup[$i]['Subtable'][$c]['subs']); $s++) { $inputGlyphs = $Lookup[$i]['Subtable'][$c]['subs'][$s]['Replace']; $substitute = $Lookup[$i]['Subtable'][$c]['subs'][$s]['substitute'][0]; @@ -2461,6 +2470,10 @@ function _getGSUBarray(&$Lookup, &$lul, $scripttag) } } // LookupType 4: Ligature Substitution Subtable elseif ($Lookup[$i]['Type'] == 4) { + if (!isset($Lookup[$i]['Subtable'][$c]['subs'])) { + continue; // every entry was filtered out by the Ignore flags + } + for ($s = 0; $s < count($Lookup[$i]['Subtable'][$c]['subs']); $s++) { $inputGlyphs = $Lookup[$i]['Subtable'][$c]['subs'][$s]['Replace']; $substitute = $Lookup[$i]['Subtable'][$c]['subs'][$s]['substitute'][0]; @@ -2497,7 +2510,7 @@ function _getGSUBarray(&$Lookup, &$lul, $scripttag) // $Lookup[$lup] = secondary Lookup for ($lus = 0; $lus < $Lookup[$lup]['SubtableCount']; $lus++) { - if (count($Lookup[$lup]['Subtable'][$lus]['subs'])) { + if (!empty($Lookup[$lup]['Subtable'][$lus]['subs'])) { foreach ($Lookup[$lup]['Subtable'][$lus]['subs'] as $luss) { $lookupGlyphs = $luss['Replace']; $mLen = count($lookupGlyphs); @@ -2628,7 +2641,7 @@ function _getGSUBarray(&$Lookup, &$lul, $scripttag) $lup = $Lookup[$i]['Subtable'][$c]['SubstLookupRecord'][$b]['LookupListIndex']; $seqIndex = $Lookup[$i]['Subtable'][$c]['SubstLookupRecord'][$b]['SequenceIndex']; for ($lus = 0; $lus < $Lookup[$lup]['SubtableCount']; $lus++) { - if (count($Lookup[$lup]['Subtable'][$lus]['subs'])) { + if (!empty($Lookup[$lup]['Subtable'][$lus]['subs'])) { foreach ($Lookup[$lup]['Subtable'][$lus]['subs'] as $luss) { $lookupGlyphs = $luss['Replace']; $mLen = count($lookupGlyphs); @@ -2707,7 +2720,7 @@ function _getGSUBarray(&$Lookup, &$lul, $scripttag) // $Lookup[$lup] = secondary Lookup for ($lus = 0; $lus < $Lookup[$lup]['SubtableCount']; $lus++) { - if (count($Lookup[$lup]['Subtable'][$lus]['subs'])) { + if (!empty($Lookup[$lup]['Subtable'][$lus]['subs'])) { foreach ($Lookup[$lup]['Subtable'][$lus]['subs'] as $luss) { $lookupGlyphs = $luss['Replace']; $mLen = count($lookupGlyphs); @@ -2823,7 +2836,7 @@ function _getGSUBarray(&$Lookup, &$lul, $scripttag) // $Lookup[$lup] = secondary Lookup for ($lus = 0; $lus < $Lookup[$lup]['SubtableCount']; $lus++) { - if (count($Lookup[$lup]['Subtable'][$lus]['subs'])) { + if (!empty($Lookup[$lup]['Subtable'][$lus]['subs'])) { foreach ($Lookup[$lup]['Subtable'][$lus]['subs'] as $luss) { $lookupGlyphs = $luss['Replace']; $mLen = count($lookupGlyphs); @@ -2946,14 +2959,43 @@ function _checkGSUBignore($flag, $glyph, $MarkFilteringSet) $ignore = true; } } - // Flag & 0x0010 = UseMarkFilteringSet - if (($flag & 0x0010) && strpos($this->MarkGlyphSets[$MarkFilteringSet], $glyph)) { + // Flag & 0x0010 = UseMarkFilteringSet: skip every mark *except* those in the set + if (($flag & 0x0010) && strpos($this->GlyphClassMarks, $glyph) + && !strpos($this->MarkGlyphSets[$MarkFilteringSet], $glyph)) { $ignore = true; } return $ignore; } + + /** + * UseMarkFilteringSet means "skip every mark except those in the given mark glyph set", so the glyphs to + * ignore are GlyphClassMarks minus that set - not the set itself. + * + * @param string $marks Space-prefixed, "|"-separated glyph list, e.g. " 00DCA| 00DD2" + * @param string $set The mark glyph set, in the same format + * + * @return string + */ + private function marksOutsideFilteringSet($marks, $set) + { + $keep = []; + $inSet = []; + foreach (explode('|', $set) as $glyph) { + $inSet[trim($glyph)] = true; + } + + foreach (explode('|', $marks) as $glyph) { + $glyph = trim($glyph); + if ($glyph !== '' && !isset($inSet[$glyph])) { + $keep[] = $glyph; + } + } + + return $keep ? ' ' . implode('| ', $keep) : ''; + } + function _getGSUBignoreString($flag, $MarkFilteringSet) { // If ignoreFlag set, combine all ignore glyphs into -> "((?:(?: FBA1| FBA2| FBA3))*)" @@ -2976,7 +3018,8 @@ function _getGSUBignoreString($flag, $MarkFilteringSet) if (!isset($this->MarkGlyphSets[$MarkFilteringSet])) { throw new \Mpdf\Exception\FontException(sprintf('Font "%s" uses mark filtering set %s, which GDEF does not define', $this->fontkey, $MarkFilteringSet)); } - $str = $this->MarkGlyphSets[$MarkFilteringSet]; + $ignoreflag = $flag; + $str = $this->marksOutsideFilteringSet($this->GlyphClassMarks, $this->MarkGlyphSets[$MarkFilteringSet]); } // If Ignore Marks set, supercedes any above diff --git a/tests/Mpdf/TTFontFileTest.php b/tests/Mpdf/TTFontFileTest.php index 66a90d509..9e4320240 100644 --- a/tests/Mpdf/TTFontFileTest.php +++ b/tests/Mpdf/TTFontFileTest.php @@ -47,4 +47,17 @@ public function testGetMetricsWithMarkGlyphSets() $this->assertSame('NotoSansSinhala-Regular', $this->ttf->fullName); } + /** + * MarkGlyphSetsDef coverage offsets are relative to that table, not to the file. Seeking to them as + * absolute offsets lands in the table directory and yields empty sets, which silently disables every + * UseMarkFilteringSet lookup. U+0DCA/U+0DD2/U+0DD3 are the subset's Sinhala marks; the second set's + * glyph has no cmap entry, so it is mapped into the Private Use Area. + */ + public function testGetMetricsReadsMarkGlyphSetsCoverage() + { + $this->ttf->getMetrics(__DIR__ . '/../data/ttf/NotoSansSinhala-Subset.ttf', (string) time(), 0, false, false, 0xFF); + + $this->assertSame([' 00DCA| 00DD2| 00DD3', ' 0E00A'], $this->ttf->MarkGlyphSets); + } + } diff --git a/tests/Snapshots/MarkGlyphSetsSnapshotTest.php b/tests/Snapshots/MarkGlyphSetsSnapshotTest.php new file mode 100644 index 000000000..1df905c6e --- /dev/null +++ b/tests/Snapshots/MarkGlyphSetsSnapshotTest.php @@ -0,0 +1,51 @@ +mpdf and + * loading it with content + * + * @return void + * @internal Don't call any $this->mpdf->Output*() method + */ + public function generatePdf() + { + $this->mpdf = new \Mpdf\Mpdf([ + 'fontDir' => [__DIR__ . '/../data/ttf'], + 'fontdata' => [ + 'sinhalasubset' => [ + 'R' => 'NotoSansSinhala-Subset.ttf', + 'useOTL' => 0xFF, + ], + ], + 'default_font' => 'sinhalasubset', + 'default_font_size' => 30, + ]); + + $this->mpdf->WriteHTML( + '
රි රී ර් ා්
' + . '
ශ්‍රී ලංකා
' + . '
සිංහල අකුරු
' + ); + } +} diff --git a/tests/data/snapshots/markglyphsets.pdf b/tests/data/snapshots/markglyphsets.pdf new file mode 100644 index 0000000000000000000000000000000000000000..b364a2c93913c4588cc0271e9f1fc6e571b7f254 GIT binary patch literal 6036 zcmcIoc|6qJ_a7-V5pA}TPePWMeGJ*jQueWC$(R{qiW$uekw}pxsh+Q-#}<#Wq%0|- zMT^pcER{$hib!NhMf~n(=;T?F*J}4q3kFOIucM0oyCJNpw$71vZm9R6k~P>c2FRss|(G~Jy%DO zL7hWkaRXr`sG$(b1aM60!AvUM!Q2?q*N2cO6E=$nG~_~fSd_j#lFp*R>wv5C>oH^c z^XME1W#&)e(M{=8HVrJ#iq2y2{2(k=3yk6NICM$?G9*2*9>ZFMkyv7P#8DG6|8maL z`@V1MYuR|HDf{YON23d-vE)YcM@pPECl$7-$tu5dE(o-V?(R z%Zkb6+4%P2aw|^jyOPzTL(6AkwRidrCTw|8G5UNg-1%-1r7QMa-HnRpO1XlqVQ<## z)u)uf=9ypZyrJN&U9j<7Bx*a8#)aHrw{U>K)|#+`U_ba*ANaWggx>{k4dI!Sd0-Ea zC}V2~Wy9tKQ2dc7Dg_p6Vg%^V!NlJOn}K~sqR2r$Jid3rK8gl%C|o++0->ydI~y6P z+pu|TGH@9(ljTS8r)W6P8A1LO4idGM1@@iEVn8TM8t@k;FI2+<37m;b1$LmYc(4?( zUM+aNuqU&fSWLdBkYLU~G=qEmVz1vcGqE&NucY)j?9G3snMngC}fiy?(T5yd7T3~`fqqQI$0YjK0 zM7S#f!)IwhBm(F~z+&czHop@V2jr5pNsu;>`h@}V=BEF=aC(U?@;e2vee22cX|GygO>ocVya zFhO*LLMlceiO^M%C|eGV&Vk)V#mR&W>g1rnK!17woD_ef+CYP{C6D6Iq#Cgp{&cYW z5Q+?<)fpmbfuc2&%LQ)Y!lZ#i1&&R)H5BxN4>daqi_HZ^U_dlD@_ZlypddKFi-k-2 z;3%70IXJ55AYkW?#h+2Y4~t*J0E;GqAq!77I8*(7oDDU!A*0cvOB$~p9qg~0J#cZW z>HcKop-e%h=4Hji2adhV5&S1IA!t))O555($B9!01*v}G72h*sCf1cr3e2_tzf;}L z$rE`OpL|M~6+7_ToRKMRh!|%?{6dH1o9Q>JkhDP3{cmQOKkzXSUJFn9#Yh>_yRF$-c zCTW~p!O6SCm9-oaH9oYvWcabbU!(MyVejuNE~Ie^51080FU{)OIdbIUg?is}QoTJQ zPQv})pM7WxdTQzsIf1_r^!39)L7#I3N9K8CPej$tayLYPhlr(fe|Sx{P+Xpb)P_+E zXAxS}SNYgRsitMF0x7+L+KLY=MJ%EZC`N3GiCt9_W|6nb;mpwViVXv}zt)uE&;BQ! zdi8Fa&rRc9lFoP2ZsCOWqWX5+a@?3P@}H3<-A|$oq-Ko2Z11-2W=G3+c6St1k*{6b zKBm?dUQKhkeC^s)>yY}(%Aww1pDpEbuRgwB&f43r#yiQf?U|YuaTK_3mnAIqNkZPq z`03)Tc!_vBF~57#Ve3Q;HnA7U$wQ@HbC4c#P@>l3URKJJ+ODr9J?{ioX(x{Fdv&%ssh}|N^5E3Eb=TBS9D8q>aI&DK*uLR5DPcWac(a;$ir09(dc!k%7_Wc2=T!Dy6FCi=_jS9@XCB#|mABwI(j-ok^3cKKX$#A?mKVlQrT- zU^GPc9Xil2Q(ALjBTo6ooo7&!nxbzI=S1z7#*(m<>t=);9on6U?8M2t21{IXl$aZr z-p>kGkFRSf{^D#Vbb$2ksKLvm%5>R?eSJfpJNo~)J~=8la8j>vbI8mt9fK^Uw!GM4 z#xv6|+3%_1lPh8}+YI|WCP&|^C5Ik6wx^{imSbdf$n{eE#Fim9BHpa{W`jl8$>4n9 z+BnbK&D3DQf;dH%?x&uO++k0>$^9IrHa5SXdzVbK>CGQn$Snzb8>#Jw)~7d1~{nuF6P3!duujZN>Wn`}>DM!UKAUfvtP4?7MVhFR^P^c=`hgg~EDo`I6fD(uj!{wRb;x zUt*(WEj1aN#tZBeOhhG?C$%Xkh&H@;-0kJf-dz@^Ecj~GkvKJP%r4S8_gG>`-P;)( z?_Rq@wQHrN-)}!>+z@}uFI_=n`F6u|f%40rNS+?65h%YQR4%9akf4?1u)i^=Q)NYH z^@xm^@2r>V$vniKH@!LEv(LqqKk{R-l)lB6hN$FfpI=_J_D1E?(GL+L_bU$lamOxP z@3xCbj|_RRyU6FoU8%(xm42Op4Ddk2xgL zFq!-OPS^^@pv=U|VF~%Ws%?=co{~8+2cON{AjyOXwenQ(Z68ySvAOM2VO3!YO@a9- z`2(j%_vPv66g4=iz4)U)@9U!nvUdz`Eb3dl?zO`Q1pCv`nkBo7lA4z;TI9`* z&TD?*x&NRgb@bcxn;o$iZkkD-Ncub*=lCYyS+j8O<+ks=v8HBul4K1B)noQ=KYHFV zYWG`;d|*(WuFcysU^MjQwAIQ#%-=c}lz0_?zO0h0vHpsqr^bac%T(bz%H$BkTYY9~ zHOH**se9ro(?!nYi54dwFFu^?8gmqXB4S{-h2S!e$d6T{IkCdQO_~aqD^&|0wFii# zXL^x_U1DG9kWUR*eAS33R@(L1X7A$M@OJ$?aS7G?lh|g9nEV8b;~#FxsXtB^`?8O+ zs!+vd1Jz>*-Ao$$_2W0;oQ!%-Jo%;eQ?+C*XZI@64907=W0>PWjM;z>nNj>X`$T=a zOKtb_w179hX-^sPr(YhAJ>Xd!|HzZDO_9j`u~8mGWMu&b>zA_txG) zeQxYVCCnUjk7~CS8hEUE!qhm|T4iy)m~w^i%Clx3`!aO%PCXR~zqC{Cx}%N1Tbzy? z`GJ~!*K}x9t$H;zUpLKm^0n$U|0BU3(N|yEwPwnVAd|inp`1L;`Uq>wP)-pXg`sUv z$~R4YMc&J7KGQDxy5;sZM&Sk-tlXCTZ_QE8VbywagxV^_=9P&LUB;dN@XfzcU&tJO z@uzyVLiVicz{SJq$n5O!`r*89UI)iQ_oHiusea)zQ!h9qs@9dnw>oCER~4EvG0IWf znqJVlo$Z_5o%PJ6w1{qtli$sh=Ln0rhcv1UF>^Hr_c$%vM!n5$%ukk8H^O^1UqLR7 zw|08l{8;o#^2h#$hfy~W9}Rap6t>$w<-OXvdSa=kLq!yOCNgtYx#eo`>Ma#tpV;w= z>#1u!r{!9DMh!abJGi%JHVNxkV#e!g#6Xe{H}>y6Kf z)(L*kb31+9r-r)O}HSnq7?^xy4A*2-MlFRNHTY`M`BgJi<_%R-}D(b*XaEt z@+VYwm<;x>Yd1`2)a$bouhZ)+kx0eJdc# z+SMI>e9=gUX-2+S=8`TevVXQrZ)EZFMm6uT&9$M@p^-#^ilrw*v+Dj@BmI130n@b) zGs}~ym+mh`q%%VVD-Tw$GBE0YAJc7#aO`#HwQtCtKpdY%k@t2b>~Rh7e#P9*Q`)iL z>0yGannnTQ%jQ(UDkrNlQBpfYr^Fvj`Q_9xTZ55|C6%#gqWHQ?hFcb~?~fH4wg^An zeM>!|!F#{J_AEl$T_SzMd8@O2(;KcQ9Y(d5h9@XpR_!XQ>`TOd=%b_yrHXx?(tW;b zecvOsch2WsM1!^V+-&0EYh@nXigMKXppj~@Fm^g_9^O3s~rdH+kboZC^bHL!n-~yin_35KwM4SXxn<%pjhucF*M8=5Et0g1Sn^!- z9B$8~9t@%-0h{%Z*U0QjZ` z?n$74fneN+kG7!xMvM4O=41@GFU`08<6`s^wggxuhfNCtmuX04W)=~EutS(4%n%TQ zgwRCbz>5WC4;2If;fp{cXb37O(Gec1;8Je_&_5oN%`yep6r^IRgGFPBXl=9>8bcrw zNa|?xCh!COtl6}G>GBWFegcpHP2yu#CKPagWHVq;l>xBNc}&$dkj^rq!qfji+fc4< zJ0Mn&zyBP;029F@tSF)EAYeyZ7TtvH9~8j)2PhT70jL5R!Vr)!hA{U)L;?)c_(DH; z1T46J!Vkpy!2>Ly#;-gySoQ)Q7GOfZ_X9Rs$ODMc@BK(vV2fYpz+eCj^gEA0S}-q$ zNLVm0Mw_@m9u|uS?)U2)1RTJ)e&^w}{;^&jhXU@~9Dw`)V28Pn4%-jqz-9w10Ny