Repository navigation
Fix the VARC review findings of #298 - #300
Merged
Merged
Conversation
Face::glyph_outline_at_coords, GlyphOutlines and the glyph extents
shaping reads all asked Face::varc first and returned its error, so a
font whose VARC table this parser rejects failed every glyph: plain
glyphs and .notdef included. Shaping then failed with Malformed on text
whose marks fall back to positioning by glyph extents, and vertical
origins fell back to the ascender.
Such fonts exist: after 14.5.0, HarfBuzz and the VARC spec moved the
MultiItemVariationStore to a new layout (a u32 region count, and an
Offset32 from each MultiVarData to its delta-set INDEX), and HarfBuzz's
own test fonts moved with it. HarfBuzz 14.5.0 drops a VARC its
sanitizer rejects and draws and shapes the font from glyf or CFF.
Those paths now read the table through Face::drawable_varc, which
counts a VARC that does not parse as absent. Face::varc still reports
the parse error. tests/varc_unparsable.rs splices a VARC with a
new-layout store into a CFF font and a glyf font: every glyph draws
through every outline path as without the table, and "\u{e1}b" (fallback
marks) and a vertical "abc" (origins from extents) shape the same.
Before, both tests failed on the first glyph with "CFF2 INDEX offSize
out of range".
sincos_pi evaluated sine and cosine with a Taylor series that stopped at y^7 and y^8 over [-π, π], so a rotation past about 90 degrees came out wrong: at 180 degrees the sine was 0.075 off, and a box rotated about a center 500 units away landed 42 units from HarfBuzz's. HarfBuzz's own varc-6868 test font (the 14.5.0 version) drew glyph00002, with its 145-degree component, 2.1 units off. The decomposed transform is now built op for op as HarfBuzz's hb_transform_decomposed_t::to_transform builds it, in f32: translate, rotate by the angle in radians (half-turns times f32 π, as HarfBuzz multiplies by HB_PI), scale, skew by tanf of the angles, translate back, each skipping what HarfBuzz skips. Sine, cosine and tangent are worked out in f64 (the nearest quarter turn removed in two parts, a Taylor polynomial over [-π/4, π/4] good to about 1e-14) and rounded to f32, as libm's sinf, cosf and tanf return them. An angle that is not finite gives NaN, and one past 2^30 radians is taken modulo 2π first, so no loop can spin. Against HarfBuzz 14.5.0, the largest difference over the eight rotations of the reviewer's rot.ttf (45 to 180 and -162 to -90 degrees, center 500 units away) drops from 42.1 to 0.00003 units, varc-6868's glyph00002 from 2.10 to 0.00003, and varc_face.ttf gains rotations of 90, 135, 180, 270 and -135 degrees, one varied past a half turn, and a 72-degree skew, all within 0.00005 (49.5 units off before at 180).
A glyph whose record held a million three-byte components was read to the end (each record costs one unit of the 2^20 work budget, so all of them fit) and every resolved component kept, before the walk failed at its 2,048-component cap: 70 ms and 86 MB of heap for a 3 MB glyph. The walk now passes Varc::resolve the most components it can still draw plus one, and reading stops there. A longer list fails the walk anyway, so the composite cut short is only kept for a walk that is about to fail; Varc::composite_with_font_coords reads every record as before. The 3 MB glyph now fails in 1.5 ms with under 1 MB of heap (the reviewer's flood_1000k fonts; flood_250k from 21.5 ms and 21 MB).
Only a leaf's ops counted toward the walk's caps; the gvar and charstring work behind them did not, and each leaf drew with fresh per-glyph limits. A glyph of 2,047 components naming a 480-point glyph with 4,095 gvar tuples took 2.4 to 3.5 s (HarfBuzz 2.4 ms: it charges its 2^24 glyph budget inside glyf and stops after 8 leaves), and the time grew with the components. The walk's leaves now: - keep each outline they draw by glyph and coords (f32 bits), so a leaf reached again at the same coords is drawn once and placed from that outline (at most 2^18 ops kept; a leaf past that is drawn again when reached again, and charged again); - share one budget of 2^24 units, HarfBuzz's HB_BUDGET_GLYPH: gvar tuple work as a glyf walk charges it (Glyf gains outline_at_coords_with_work, which takes its tuple work from the caller), 64 per glyph record and 1 per point a glyf leaf lays down, 4 per charstring operation (Cff::draw_limited and Cff2::outline_limited take their operation limit from the caller; the Interp limit can be lowered), and regions times axes of the CFF2 store for each coord vector, whose Cff2Shared is now kept per coords for the walk. Past the budget a leaf draws nothing, as HarfBuzz's leaves stop, instead of failing the glyph. The reviewer's leaf_gvar.ttf now takes 13 to 18 ms. Hostile fonts built to defeat the memo, every component at its own coords: the same leaf 2.5 s to 14 ms (it stops after 8 leaves, as HarfBuzz does); a glyf composite of 255 x 255 visits 5.5 s to 15 ms; a CFF2 leaf running 39,000 operations 416 ms to 25 ms. Glyph drawing outside VARC keeps its per-glyph limits and output.
The glyph_outline_at_coords docs (and the PR) said cycles no longer reach the depth cap. HarfBuzz's decycler, which the walk follows exactly, cuts a cycle of n glyphs 2n - 1 levels down, so a cycle of more than 32 glyphs still reaches the 64-level cap first and fails the glyph, where HarfBuzz stops expanding at that depth (the reviewer's cycle_33 and cycle_40 fonts; cycle_4 through cycle_32 match HarfBuzz). The docs now say so, and that leaves share one glyph budget and are drawn once per coords.
The leaves' memo and the CFF2 state kept per coord vector were BTreeMaps keyed by the coords themselves, so a lookup compared whole coord vectors at every step of the search. Components with 4,096-axis coords that differ only in their last axis made each lookup cost tens of thousands of comparisons, and the hostile wide_coords font went from 2.3 ms to 16 to 20 ms with the memo. Both maps now key by the glyph (for leaves) and an FNV-1a hash of the coords' bits, with the coords compared once inside the bucket. That font now takes 4.6 to 5.2 ms; leaf_gvar.ttf and the other leaf fonts are unchanged.
The reviewer-style CFF2 hostile font (2,047 components at distinct coords naming a leaf that runs 39,000 charstring operations) still took 23 to 28 ms per glyph at 4 units an operation, and 13 s in the face fuzz target under ASan, where an operation costs about eight times one point of one gvar tuple. At 8 units the leaves stop after 53 such leaves instead of 106, and the glyph takes 13 to 15 ms. A real CFF2 leaf runs a few hundred operations, far inside the 2^24-unit budget at either weight; reviewer-built CFF2 VARC fonts still match HarfBuzz at every location.
The subset failed on a font whose VARC the core rejects, such as one with the MultiItemVariationStore layout HarfBuzz adopted after 14.5.0. Such a table now counts as absent, as the face's outlines read it and as HarfBuzz drops it: the subset leaves it out with a warning, and its glyphs keep their glyf or CFF outlines. The output equals the subset of the font without VARC.
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.
Follow-up to #298 (#296), which merged at
e0ae353before these review fixes were pushed. Fixes the five findings of the independent review of #298: an unparsableVARCtable failed every glyph, rotations past about 90 degrees were inaccurate, leaf work was not charged, a flood of component records was read past the component cap, and the docs overstated which cycles the decycler cuts. No change for fonts withoutVARC.Commits
Face::glyph_outline_at_coords,GlyphOutlinesand the glyph extents shaping reads (fallback mark positioning, vertical origins) readVARCthrough a newFace::drawable_varc, which counts a table that does not parse as absent, as HarfBuzz 14.5.0's sanitizer drops it.Face::varcstill reports the error.sigilbuzz-subset:subsetleaves out aVARCthat does not parse, with a warning, and the glyphs keep theirglyfor CFF outlines, as the face reads them (Fix the VARC review findings of #298 #300). The whole subset failed before. The output equals the subset of the font withoutVARC(tests/varc_unparsable.rsin the subset crate). || Rotate VARC components as HarfBuzz's to_transform does | 2 (must fix) | The decomposed transform is built op for op as
hb_transform_decomposed_t::to_transformbuilds it, inf32, with sine, cosine and tangent worked out inf64(quarter-turn reduction in two parts, Taylor polynomial over [-π/4, π/4]) and rounded tof32as libm'ssinf/cosf/tanfreturn them. Replaces a Taylor series that stopped at y^7/y^8 over [-π, π]. || Read no more VARC components than the walk can draw | 4 (nit) | The walk asks
Varc::resolvefor at most its remaining component cap plus one; reading stops there. || Charge VARC leaves to one budget and draw each leaf once | 3 (should fix) | Leaves share HarfBuzz's 2^24 glyph budget (gvar tuple work, glyph visits and points, charstring operations, CFF2 region scalars per coord vector) and draw nothing past it, as HarfBuzz's do; each leaf outline is kept by glyph and coords and drawn once.
Glyf,Cff,Cff2and the charstring interpreter gain crate-internal entry points that take their work or operation limit from the caller; their own paths keep their per-glyph limits. || Say which VARC cycles the decycler cuts | 5 (nit) | Docs: the decycler cuts a cycle of n glyphs 2n - 1 levels down, so one of more than 32 glyphs still reaches the 64-level cap. |
| Bucket the VARC leaf memo by a hash of the coords | 3, follow-up | The leaf memo and per-coords CFF2 state key by an FNV-1a hash of the coords with one compare in the bucket, so 4,096-axis coords do not make each lookup compare whole vectors at every step. |
| Weigh VARC leaf charstring operations at 8 units | 3, follow-up | A charstring operation costs about eight times one point of one gvar tuple; 4 units let a CFF2 hostile glyph take 23 to 28 ms. |
Added by the coordinator after the agent's report:
ab18e67Leave a VARC that does not parse out of a subset (crates/sigilbuzz-subset/src/lib.rs, newcrates/sigilbuzz-subset/tests/varc_unparsable.rs). The subset crate's tests, fmt and clippy pass locally.Evidence, before (#298 at
e0ae353) and afterHarfBuzz 14.5.0 is uharfbuzz 0.56.2. Outlines match when the op sequence is the same and every coordinate is within 0.01 units.
1. A
VARCtable that does not parse. After 14.5.0, HarfBuzz main (bd079077d2, boring-expansion-spec #175) moved the MultiItemVariationStore to a new layout (a u32 region count, an Offset32 from each MultiVarData to its delta-set INDEX) and migrated its test fonts; sigilbuzz's parser, like HarfBuzz 14.5.0's sanitizer, rejects it. Fonts with a new-layoutVARC(from the review):scp_newvarc.otf(Source Code Pro subset, CFF) outlinesCFF2 INDEX offSize out of rangess3_newvarc.otf(Source Sans 3, CFF2) outlinesparity_newvarc.ttf(glyf) outlinesscp_newvarc.otf"abc" top to bottom, y offsetsss3_newvarc.otf"abc" top to bottom, y offsetsNew
tests/varc_unparsable.rssplices a hand-built new-layoutVARCinto a CFF font and a glyf font: every glyph draws through every outline path as without the table, and "\u{e1}b" (fallback marks) and a vertical "abc" (origins from extents) shape the same. Both tests fail before the fix on the first glyph. Correction to #298: its Not done said HarfBuzz'svarc-6868.ttfandvarc-ac0*use an older encoding. They are in the new layout; the copies at tag 14.5.0 parse, and match HarfBuzz after the rotation fix (below).2. Rotation. Largest difference from HarfBuzz:
rot.ttf: a box rotated about a center 500 units away by 45, 90, 120, 144, 162, 180, -162, -90 degreesvarc-6868.ttfat tag 14.5.0,glyph00002(a 145-degree component), default andwght700varc_face.ttf, new glyphs: 90, 135, 180, 270, -135 degrees about a far center, one varied past a half turn, a 72-degree skewtests/fixtures/varc_face.ttfgains those seven glyphs (48 VARC glyphs now);tests/varc_parity.rschecks them through Face and bothGlyphOutlinespaths at the eight locations.3 and 4. Leaf work and record floods (release build, slowest glyph at the default and at AAAA=100, two runs, the face and
GlyphOutlinespaths within the same range):leaf_gvar.ttf: 2,047 components naming a 480-point glyph with 4,095 gvar tuples, one coordsleaf_gvar_distinct.ttf: the same, each component at its own coordsglyf_visits.ttf: 2,047 components at distinct coords naming a glyf composite of 255 x 255 visitscff2_ops.otf: 2,047 components at distinct coords naming a CFF2 leaf that runs 39,000 operationsflood_1000k_nofvar.ttf: a 3 MB glyph of a million three-byte recordsflood_1000k_fvar.ttfflood_250k_nofvar.ttfThe earlier hostile set stays under 24 ms per glyph (
distinct_regions13 to 16 ms,gated_flood17 to 21,deep_chain7 to 10,fanout_cond6,wide_coords5, up from 2 for the leaf keys it now builds, with 6.6 MB of heap).leaf_gvar.ttfstill holds 43 MB: its outline is a million ops, which the 2^20-op cap allows.New tests:
a_leaf_reached_again_at_the_same_coords_is_drawn_once(500 boxes at one coords cost one leaf's 80 units) andleaves_past_their_work_draw_nothinginsrc/face/varc_draw.rs;a_walk_reads_no_more_components_than_it_can_draw(100,000 records, 3 read, 4 units) insrc/tables/varc/tests.rs; sine and cosine against exact values in every quadrant, libm's rounding ofsinf(f32 π), and NaN for angles that are not finite.5. Cycles. The review's
cycle_4throughcycle_32match HarfBuzz;cycle_33andcycle_40still fail with the depth-cap error (HarfBuzz stops expanding at that depth), now documented.chain_63/chain_64match;chain_65/chain_66fail at the cap as documented.Parity overall (all glyphs, eight locations, Face,
GlyphOutlines::outline,GlyphOutlines::draw):varc_parity.ttfvarc_face.ttf(now 51 glyphs)extra.ttf(nested resets, transforms, conditions, cycles)extra_cff2.otf(the same over CFF2 leaves)face_cff2.otfFonts without
VARC: byte-identical to main throughglyph_outline,glyph_outline_at_coords,GlyphOutlines::outline,GlyphOutlines::drawandglyph_bounds_at_coords, floats printed exactly: 26 of 26 dumps (Fraunces and Fraunces Italic, Hahmlet, Nabla, Noto Sans KR VF CFF2 and glyf, Recursive, Roboto Flex, Source Serif 4 VF, Arial, Segoe UI, Segoe UI Emoji, Calibri, Georgia, Consolas, Bahnschrift, Sitka VF, Gabriola, Ebrima, Malgun Gothic, and the Source Code Pro, Source Sans 3 VF, CID CFF, Noto Sans KR CFF2, Hahmlet and Rubik fixtures).Verification
cargo fmt --all -- --check: clean.cargo +1.99.0 clippy --workspace --all-targets --all-features -- -D warnings, the same with--no-default-features,cargo +1.99.0 clippy -p sigilbuzz --no-default-features -- -D warnings: clean.cargo test --workspace --all-features: 3,825 passed, 0 failed, 9 ignored, 201 test binaries.cargo build --no-default-features: ok.CARGO_RESOLVER_INCOMPATIBLE_RUST_VERSIONS=fallback cargo +stable generate-lockfile, thencargo +1.81 check --workspace --lib --no-default-featuresand--lib --bins --all-features): ok.cargo doc -p sigilbuzz --no-deps: no new warnings.rustlang/rust:nightly, cargo-fuzz,-O -a,-max_len=65536 -rss_limit_mb=2048 -timeout=10, no--privileged),faceandshape, 600 s each, seeds adding the review's fonts under 64 KB,rot.ttf, the new-layout fonts and the leaf hostile fonts. On 434e153:face11,565 runs andshape34,737, no crashes, timeouts or OOMs; one slow-unit report (13 s under ASan) forcff2_ops.otf, which led to 737e787. On the head 737e787:face13,307 runs andshape19,654, peak RSS 906 / 494 MB, no crashes, timeouts, OOMs or slow units. Runs are few because the seeds now include the hostile fonts, whose glyphs each take milliseconds in release and far longer under ASan.Not done
Malformed, where HarfBuzz stops expanding at 64 levels and 16,384 edges and draws what it has (the review'scycle_33,cycle_40,chain_65,chain_66,fan_50x50,leaves_2049).VARCnow counts as absent, as in HarfBuzz 14.5.0. To be filed as its own issue.glyfbox for aglyffont and the control box of the VARC outline for a CFF font.vsindexvalues pays for the extra region scalars only through its operations.CHANGELOG lines
Changed:
glyfor CFF) share HarfBuzz's 2^24-unit glyph budget for theirgvartuple work, glyph visits, points and charstring operations, and draw nothing past it, as HarfBuzz's do; a leaf reached again at the same coords is drawn once (Finish VARC through Face: nested reset, self-reference, a shared work budget, short delta sets #296, Finish VARC through Face: nested reset, self-reference, one work budget, short delta sets #298, Fix the VARC review findings of #298 #300). A glyph of 2,047 components over a leaf with 4,095gvartuples took up to 4 s and now takes about 20 ms.Fixed:
VARCtable does not parse, such as one in the MultiItemVariationStore layout HarfBuzz adopted after 14.5.0, draws and shapes as if it had none, as HarfBuzz 14.5.0 does (Finish VARC through Face: nested reset, self-reference, a shared work budget, short delta sets #296, Fix the VARC review findings of #298 #300). Every glyph failed to draw, shaping failed on fallback-positioned marks, and vertical origins fell back to the ascender.Face::varcstill reports the error.sigilbuzz-subset:subsetleaves out aVARCthat does not parse, with a warning, and the glyphs keep theirglyfor CFF outlines, as the face reads them (Fix the VARC review findings of #298 #300). The whole subset failed before. The output equals the subset of the font withoutVARC(tests/varc_unparsable.rsin the subset crate).to_transformcomposes them, withsinf,cosfandtanfprecision (Finish VARC through Face: nested reset, self-reference, a shared work budget, short delta sets #296, Fix the VARC review findings of #298 #300). A rotation past about 90 degrees was off: a box rotated 180 degrees about a center 500 units away landed 42 units from HarfBuzz's, and HarfBuzz'svarc-6868test font drew 2 units off.Corrections to #298's lines: "longer cycles stop where HarfBuzz's decycler stops them" holds for cycles of up to 32 glyphs; longer ones still fail at the 64-level cap.