Skip to content

feat: scene font bundles with pre-filled TMP and UI Toolkit atlases (0.20.0) - #126

Merged
dalkia merged 11 commits into
mainfrom
feat/font-bundles
Oct 5, 2026
Merged

dalkia merged 11 commits into
mainfrom
feat/font-bundles

Conversation

@dalkia

@dalkia dalkia commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

What

Scene .ttf files (the explorer's font_src, decentraland/unity-explorer#10317) now convert to a font bundle. Each bundle holds:

  • a Font carrying a font rebuilt from validated values, never the uploaded file (see Security);
  • a dynamic TMP_FontAsset, for TextShape;
  • a dynamic UI Toolkit FontAsset, for scene UI.

Each font asset has its own SDFAA atlas, pre-filled with:

  • ASCII;
  • smart quotes, dashes, ellipsis and bullet;
  • the euro sign;
  • the Latin-1 letters (es/pt/fr/de/nordic).

The assets use the explorer's runtime settings: 90pt, padding 9, 1024², SDFAA. Kerning between the pre-filled glyphs ships with them.

With the runtime path, every glyph is drawn by FreeType on the main thread the first time it's shown. With the bundle, common text renders on the first frame. The assets stay dynamic, so other characters are still added at runtime through the bundled font, into the same atlas's free space.

How

  • crate/src/fontgen copies what TextCore writes, fitted against atlases Unity 6000.5 baked itself:
    • FaceInfo from FreeType's face metrics;
    • glyph metrics from the 26.6 control box, with FreeType's FT_MulFix scaling;
    • TextCore's slot convention (padding plus a one-texel gap);
    • shelf packing, with free rects the runtime packer keeps filling;
    • the exact distance field, encoded the way SDFAA encodes it. Along glyph edges it matches Unity's field; further out it is smooth where TextCore's bitmap transform is jagged.
  • fontgen::rebuild rewrites the font from validated values. fontgen::kerning reads its kerning (see Security).
  • builder::font assembles the bundle from template/font-types.mac.bundle. That's a 44 KB type donor Unity built itself; the script that regenerates it is template/src/FontTypesTemplate.cs.
    • The TMP material is left out, because its shader lives in the explorer build and the client assigns its own.
    • Both assets carry the kerning as m_GlyphPairAdjustmentRecords, scaled the way TextCore stores what it reads from GPOS.
  • unity::serialized_file gains SerializedFile format 23 (Unity 6000.5) read and write support, needed to read the donor. Output bundles stay format 22, like everything else.
  • live: fonts convert for scenes only, and are digest-named under a new Font recipe at generation 0. A font the lane refuses (not TrueType, over a limit, not rebuildable) gets no bundle and counts as a tolerated failure: the manifest's exitCode goes non-zero, as with an undecodable image, without listing it as a failed bundle.
  • Examples:
    • sfdump dumps any bundle's objects (SFDUMP_NODES=<field> prints a type tree).
    • fontcal re-fits fontgen against a Unity-baked asset after a Unity upgrade.

Security

From @popuz's review: scene fonts are untrusted, and the client's FreeType reads the bundled font whenever text needs a glyph outside the pre-fill.

  1. No uploaded bytes reach the client. fontgen::rebuild writes a new TrueType file from what ttf-parser parsed:

    • glyf/loca, re-encoded without hinting instructions. Composites are flattened, glyph ids are kept, and fractional midpoints stay implied.
    • cmap formats 4 and 12.
    • head, hhea, hmtx, maxp, OS/2 and post, with the source's metrics.
    • A family/style name table.
    • A GPOS holding one kern lookup of the pairs fontgen::kerning read.

    Hinting programs, variation, colour/bitmap data and every other layout table are dropped. The bake runs on the rebuilt font, so the atlas and the font FreeType later reads agree. A test checks that every glyph of the donor font draws the same path, that coordinates move at most half a unit (scaled composites snap to whole units), and that mappings, advances and face metrics are unchanged.

  2. Bounded inputs, enforced before anything expands. A crafted font under 1 KB must not be able to exhaust the converter. The two tables that can be made expensive, glyf and cmap, are read raw by this crate rather than through ttf-parser, so the walk that is bounded is the only walk that runs:

    • the client's 16 MB file cap;
    • composites are flattened by fontgen::rebuild from the raw glyf, with ttf-parser 0.25's semantics where they are a choice (no grid rounding or scaled offsets, one-point and malformed glyphs empty, components outside the font skipped, depth 32) and the specification's layout for arguments, so anchored components are placed by their matched points; every glyph is sized first from the component graph with memoised, saturating counts of visits, points and height, under one glyph bound (min(loca entries − 1, numGlyphs)) shared by sizing and flattening, with the depth checked before each recursion: over 1,024 visits or 8,192 points per glyph, 1M visits or 4M points per font, 64 direct components, nesting to 32 or a cycle is refused before anything expands;
    • cmap is resolved per format from the raw subtables (0/4/6/10/12/13), never through Face::glyph_index: one unit of work per record read and per code point visited, under a budget of 2 × 0x110000 for the font, at most 32 encoding records; the pre-fill and is_supported use this map too;
    • GPOS kern lookup indices are counted into a fixed bit set (65,536 named, 256 lookups + subtables visited) and read by position, not iterated; a GPOS past a cap, or with an unresolvable lookup index, ships without kerning;
    • coordinates within ±16,383, checked as floats;
    • 1M flattened segments across the pre-fill;
    • a rebuilt file over 16 MB.

    A font past any of these is fontgen::Refused, a typed error. The conversion tolerates only that (no bundle, non-zero exitCode, cached per hash); a template, cache or fetch error fails the bundle so a retry builds the font. A .ttf path holding anything but a TrueType file is refused rather than handed to the texture builder. Fixtures cover each cap: a composite bomb (100 points × 2³⁰), a fan-out of empty leaves (64³⁰ visits, zero points), a composite cycle, a loca longer than maxp with a self-referencing glyph past it, a 65k-long chain (refused at depth 32, not followed), chains of exactly 32 and 31, the per-glyph point cap without the node cap, the per-font point and node caps, anchored components, three nested transforms against ttf-parser, a malformed glyph, a cmap group over all of Unicode, ranges past the budget, a 1.3M-group format 13 flood under 16 MB, 40 encoding records, a 2000-record kern feature overlay (which kerns when under the cap), an oversized rebuild, and a typed refusal through build_bundle.

  3. TrueType only: sfnt version 0x00010000 and .ttf paths, matching the protocol (feat: add font_src to TextShape, UiText, UiDropdown and UiInput protocol#489: "a ttf font file") and the client's header check. OTF/CFF, true, collections and web fonts get no bundle.

  4. Bounded conversion work: the distance field no longer evaluates every segment at every texel.

    • A texel a full gradient scale from the outline saturates, so each segment only visits the texels within that spread.
    • Winding comes from per-row crossings.
    • The bytes are unchanged; a test compares the result against the exhaustive field.
    • A font whose pre-fill would exceed a fixed work budget is refused rather than stalling the conversion. Measured: Azeret Mono and Inter use about 2% of it and bake, rebuild included, in 10–20 ms (fontcal prints the figure).

Verified

  • The font tests pass. They pin face and glyph metrics to Unity's output, and cover the rebuild round trip, kerning surviving the rebuild, the culled distance field, the embedded font being the rebuilt one, the hostile-font fixtures above, sfnt checksums (whole file and per table), and the format-23 SerializedFile writer round-tripping the donor.
  • Clippy (--workspace --all-targets -D warnings) and rustfmt are clean.
  • Before the rebuild landed:
    • bundles for Azeret Mono and Inter loaded and rendered in Unity 6000.5.9 the same as CreateFontAsset output;
    • characters outside the set (Ω, ж) were added at runtime;
    • an end-to-end Editor run against a local scene loaded TextShape fonts from their bundles (AB:tmp_v49_…).
  • Not yet checked in Unity: a bundle with the rebuilt font. That needs FreeType loading it, characters added at runtime, and kerned pairs rendering.

Not covered yet

  • Size: a bundle is about 700 KB, mostly the two 1 MB atlases (TMP and UI Toolkit) under LZ4.
  • Already-converted scenes get font bundles only when they are converted again, since a manifest without font in its recipes reads as current.
  • abgen-corpus batch builds skip fonts; the lambda and the JIT server include them.
  • Kerning for glyphs added at runtime: the rebuilt GPOS only holds pairs among the pre-filled glyphs.
  • No FreeType/OTS load in CI: the rebuilt file is checked by ttf-parser and by hand against the spec; a FreeType load would need a new CI dependency.
  • Composite fidelity: ttf-parser's flattening ignores USE_MY_METRICS and point-matched anchors. Advances come from hmtx, so metrics are unaffected; anchors are for the Unity check on an accent-heavy font.

🤖 Generated with Claude Code

dalkia and others added 2 commits September 30, 2026 15:38
Scene .ttf/.otf files (the explorer's font_src, unity-explorer#10118) now convert to a font
bundle: the source Font, a dynamic TMP_FontAsset and a dynamic UI Toolkit FontAsset, each
with an SDFAA atlas pre-filled with ASCII, smart punctuation, the euro sign and the common
Latin-1 letters. Common text renders on the first frame instead of waiting for FreeType on
the main thread; any other character is still added at runtime through the bundled font.

- fontgen: FreeType-exact face and glyph metrics, TextCore's packing convention, and the
  exact distance field, fitted against atlases Unity 6000.5 baked itself
- builder::font: assembles the bundle from template/font-types.mac.bundle, a Unity-built
  type donor (generator in template/src/FontTypesTemplate.cs)
- SerializedFile format 23 (Unity 6000.5) read and write support, needed to read the donor
- live: fonts convert for scenes only, digest-named under a new Font recipe; a font that
  does not parse gets no bundle and does not fail the entity
- sfdump and fontcal examples for inspecting bundles and re-fitting against Unity

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- fontgen::sdf used f64::hypot, which links glibc's hypot@GLIBC_2.35 past the 2.34 floor
  the node addon is held to; the curve deviation now takes the square root directly
- crate/abgen-wasm and crate/abgen-node keep their own lockfiles and did not know ttf-parser
- templates_missing_reports_every_absent_required_template lists the font template

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`pub mod fontgen;` had been inserted under the `#[cfg(not(target_arch = "wasm32"))]` that
gates glbscan, which dropped fontgen from the wasm build and compiled glbscan into it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@popuz popuz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Review by Claude Code, posted on behalf of Vitaly.

Security pass on the font lane (perf side looks great — measured on the client: pre-fill cuts Latin-text rasterization from ~40–50 ms to ~4 ms per burst).

As it stands the conversion is not a security boundary yet:

  1. Original bytes ship to the client — builder/font.rs (font.insert("m_FontData", Value::Bytes(bytes.to_vec()))). Any character outside the pre-filled set makes the client's FreeType parse the untrusted original file, and the scene controls its own text, so it can always trigger that. Suggestion: sanitize (OTS or fontTools re-serialization, drop hinting/unknown tables) and embed the sanitized bytes instead.
  2. Validation is minimal — fontgen/mod.rs is_font_file / is_supported: magic + Face::parse + upem + one mapped character. No file-size cap, no glyph/point/segment limits. Suggest matching the client's 16 MB cap (or lower) and capping points per glyph.
  3. Magic accepts OTTO and true, but the protocol and the client's raw path are TrueType-only (00010000). Suggest restricting to TrueType so both paths accept the same set.
  4. Converter DoS — fontgen/sdf.rs signed_distance is brute force O(pixels × segments) with no spatial culling, run on all cores, with every pre-filled outline held in memory before packing. A font with heavy outlines could burn a lot of CPU/RAM per conversion. Suggest bbox culling plus a timeout / memory cap on the font lane so a slow font is skipped rather than stalling the pipeline.

dalkia and others added 3 commits October 2, 2026 09:26
Review on #126 (security pass):

- Only sfnt version 0x00010000 and .ttf paths take the font lane, matching the protocol's
  font_src ("a ttf font file") and the explorer's header check; OTF/CFF, `true`, collections
  and web fonts get no bundle.
- Inputs are capped: the explorer's 16 MB file limit, 8192 outline points per glyph with
  composites expanded, 1M flattened segments across the pre-filled glyphs.
- The distance field no longer evaluates every segment at every texel. A texel a gradient
  scale from the outline saturates, so each segment visits only the texels within that spread,
  and winding comes from per-row crossings; the bytes are unchanged (pinned by a test against
  the exhaustive field). The bake refuses a font whose pre-fill would exceed a fixed work
  budget instead of stalling the conversion.
- The pre-fill gains åæøÅÆØýÝÿ, the Latin-1 letters it was missing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rning

Review on #126: the bundle embedded the uploaded file, which the client's FreeType parses
whenever text needs a glyph outside the pre-fill, and a scene controls its own text.

- fontgen::rebuild writes a new TrueType file from what ttf-parser parsed and the limits
  accepted: glyf/loca re-encoded without hinting instructions (composites flattened, glyph ids
  kept, fractional midpoints left implied), cmap formats 4 and 12, head/hhea/hmtx/maxp/OS/2/post
  with the source's metrics, a family/style name table, and a GPOS of one kern lookup. Hinting
  programs, variation, colour/bitmap and every other layout table are dropped. Capped at 8192
  points per glyph, 4M per font, coordinates within ±16383 and 16 MB of output.
- fontgen::kerning reads the pairs among the pre-fill glyphs from the GPOS kern feature (or the
  legacy kern table), visiting at most 256 subtables.
- The bake runs on the rebuilt font, so the atlas and the font FreeType reads agree, and both
  font assets carry the pairs as m_GlyphPairAdjustmentRecords, scaled the way TextCore stores
  what it reads from GPOS.
- A font the lane refuses is logged and skipped instead of failing the entity.
- sfdump: SFDUMP_NODES=<field> prints a field's type tree.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A font the lane turns away (not TrueType, over a limit, not rebuildable) still gets no
bundle and never reaches the failed list, but it now raises the manifest's exit code the way
an undecodable image does, so the refusal is visible to whoever reads the conversion result.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dalkia

dalkia commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

@popuz thanks for the security pass. All four points are addressed:

  1. Original bytes ship to the client (576a863). The bundle now carries a font rebuilt from validated values (fontgen::rebuild), never the uploaded file:

    • glyf/loca re-encoded without hinting instructions. Composites are flattened and glyph ids are kept.
    • cmap formats 4 and 12.
    • head, hhea, hmtx, maxp, OS/2 and post, with the source's metrics.
    • A family/style name table.
    • A GPOS with one kern lookup, holding the pairs read from the original (fontgen::kerning). TextCore kerns from GPOS at runtime, so dropping it would have cost proportional fonts their kerning.

    Hinting programs, variation, colour/bitmap data and every other layout table are dropped. The atlas is baked from the rebuilt file, and both font assets carry the kerning for the pre-filled glyphs as pair records.

  2. Validation is minimal (6019861, 576a863):

    • the client's 16 MB cap;
    • 8,192 outline points per glyph, with composites expanded;
    • 4M points per font;
    • coordinates within ±16,383;
    • 1M flattened segments across the pre-fill;
    • at most 256 kerning subtables read.
  3. Magic accepts OTTO and true (6019861). Now only sfnt 0x00010000 and .ttf paths are accepted, matching the protocol and the client's raw-path check.

  4. Converter DoS (6019861):

    • The distance field culls by spread: each segment only visits the texels within one gradient scale of it, since beyond that the byte saturates. Winding comes from per-row crossings. A test pins the result byte-for-byte to the exhaustive field.
    • A fixed work budget on the pre-fill refuses a font instead of stalling the pipeline.
    • Refused fonts get no bundle and count as tolerated failures, so the manifest's exitCode goes non-zero (98efcae).

Also from your client-side note: the pre-fill now includes åæøÅÆØýÝÿ.

Still open: a Unity check of a bundle carrying the rebuilt font. That means FreeType loading it, characters added at runtime, and kerned pairs rendering. The earlier Unity runs predate the rebuild.

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: follow-up on the security pass (HEAD 98efcae)

Thanks for the thorough turnaround. I checked each of the four points from @popuz's review against the code and against ttf-parser 0.25.1 (the version in Cargo.lock).

Point Status
1. Original bytes ship to the client ✅ Fixed. m_FontData is baked.font_data, the rebuilt font. No source table is copied whole. name is re-encoded, post is v3 and OS/2 is rebuilt field by field.
2. Validation is minimal ⚠️ Partly fixed. The 16 MB cap, the pre-fill segment cap and the subtable cap work as described. The per-glyph and per-font point caps are checked only after composites are expanded, and the cmap and GPOS-feature walks have no bounds at all (see below).
3. TrueType only ⚠️ Mostly fixed. The 0x00010000 magic is enforced. The .ttf path is not enforced in build_bundle (P2 below).
4. Converter DoS ⚠️ Fixed for the SDF stage. Culling is exact, and the budget is checked before render. But the budget does not cover rebuild or kerning, which run first and can exhaust memory or stall from a file under 1 KB.

regen::guard catches panics but not running out of memory, and bake has no timeout. So any of the issues below can take down the shared JIT server or lambda, and it then fails again on the same font when it is retried.

P0 — Blocker

  1. Unbounded cmap enumeration (fontgen/rebuild.rs:266). subtable.codepoints(|cp| codepoints.push(cp)) runs before any limit. ttf-parser's format 12 codepoints walks start..=end for each group without checking the range. A single 12-byte group 0..=0xFFFFFFFF pushes about 4.3e9 u32 values (about 17 GB) before char::from_u32 filters anything. Repeated encoding records and large format 4 segments make it worse.
    • Fix: walk the groups yourself, clamping each to 0..=0x10FFFF (or use a 0x110000-bit set), and dedup the subtables.

P1 — Major

  1. Point caps are checked after composite expansion (fontgen/rebuild.rs:233, also fontgen/mod.rs:376). outline_glyph runs to completion before c.points > MAX_GLYPH_POINTS is tested. ttf-parser limits composite depth to 32 (MAX_COMPONENTS) but not fan-out.
    • Example: a chain of about 30 composites, each referencing the next twice, expands to around 2^30 points (several GB). MAX_FONT_POINTS is also checked only between glyphs.
    • Fix: before outlining, do a memoized walk of the glyf component graph with saturating point counts. Alternatively, have Contours stop recording at the cap, though that bounds memory only and not CPU.
  2. The GPOS kern feature walk is unbounded (fontgen/kerning.rs:40-46). features.filter(kern).flat_map(lookup_indices).collect() runs before dedup and before the 256-subtable cap. Feature records are 6 bytes and can all point at one Feature table with 65535 lookup indices, giving about 4e9 u16 values from under 1 MB of input.
    • Fix: collect the indices into a 65536-bit set.
  3. Every font build error is treated as a tolerated refusal (live.rs:1431, live.rs:601-612).
    • Err(e) if it.is_font catches template, cache I/O and fetch errors along with real refusals.
    • font_supported returns false when fetch_mmap fails. image_decode_ok returns true in that case, so the build runs and the error surfaces.
    • Either way the manifest records font and the name never reaches the failed list. A passing network or infra error therefore drops the scene's font until it is reconverted by hand.
    • Fix: tolerate only a typed refusal from fontgen (an error enum or a marker), and send everything else to failed_m.

P2 — Minor

  • Font detection by content, not path (builder/mod.rs:657, :840). Any bytes starting with 00 01 00 00 take the font lane. A tex.png holding TTF bytes skips the font_supported gate, then either ships a font bundle under an image name or lands on the failed list. build() (live.rs:~723) also matches .ttf without the scene-only gate. Gate on is_font_path(file) as well.
  • The coordinate bound can be bypassed (fontgen/rebuild.rs:124-125). Nested near-2.0 F2Dot14 scales push a coordinate past i32::MIN. x.round() as i32 saturates, and .abs() then panics in a debug build or wraps in a release build, so the ±16,383 guarantee doesn't hold. Check the f32 value (!x.is_finite() || x.abs() > 16383.0) before casting.
  • The kerning loop counts subtables, not lookups (fontgen/kerning.rs:57-84). Lookups with no subtables never advance visited, so the pair loop can run about 40k × 65535 times.
  • The kerning fallback contradicts the PR text. Going over 256 subtables, or a single bad lookup index (? at kerning.rs:58), returns None and falls back to the legacy kern table. It does not "ship without kerning" as the description says.
  • The pairs in the assets can disagree with the GPOS. bake_face gets the original pairs, but gpos_table can trim them to fit the 64 KB offset limit (rebuild.rs:~447). Then m_GlyphPairAdjustmentRecords and the embedded GPOS differ.
  • The outline_glyph result is ignored (rebuild.rs:233). A malformed glyph can leave partial contours, which get re-encoded instead of becoming an empty glyph.
  • Bitmap-only fonts are accepted. A 0x00010000 font with no glyf passes is_supported and bakes a blank atlas. Refuse it when no pre-fill glyph has an outline.
  • Refusals aren't cached. Each platform and JIT request reruns rebuild and bake on the same hostile font.
  • Composite fidelity. ttf-parser flattening ignores USE_MY_METRICS, and possibly point-matched anchors. The round-trip test compares ttf-parser with ttf-parser, so it can't catch this.
  • Tests.
    • Add assertions that checksum(font) == 0xB1B0AFBA and that each table checksum is right.
    • Ideally, load the font through FreeType or OTS once in CI. A malicious-font fixture for each cap (cmap range, composite bomb, feature overlay) would also prevent regressions.
  • Nits.
    • sdf.rs:228 says "TrueType and CFF".
    • ".ttf" is defined three times (live.rs:17, fontgen/mod.rs:146, naming.rs:418).
    • The format-23 SerializedFile writer is unused and untested.
    • MAX_RENDER_WORK (400M evaluations) is probably seconds per platform, not "a few hundred ms". Worth measuring.

Rebuilt TrueType: spec check

I checked these by hand and they look correct:

  • The table directory: sorting, search fields, alignment, checksums and checkSumAdjustment.
  • head with long loca, hhea/hmtx consistency, and maxp v1.0 with the hinting fields zeroed.
  • glyf flag and delta encoding.
  • cmap 4 (terminator, idDelta wrap) and 12.
  • name (3/1/0x409).
  • GPOS offsets, and PairPos format 1 with sorted Coverage and PairSets.

For the open Unity check, worth confirming:

  • That glyphs are rendered unhinted. With no bytecode, a hinted load may go through the autohinter and differ from the pre-fill.
  • That TextCore's runtime GPOS reader accepts the minimal GPOS without duplicating the pre-filled pair records.
  • That x_advance * 90 / upem matches TextCore's 26.6 rounding.
  • A composite-heavy font, for accents.

Other checks

  • CI: all checks pass. Address sanitizer was skipped.
  • Consumers: no public API break. This is a new lane gated on is_font and the scene entity type, and format-22 output is unchanged.
  • Git conventions: the title, branch and commits follow ADR-6. The first commit body mentions .otf and #10118, so use the PR text in the squash message.
  • Rollout: scenes converted before this PR, and any later Font generation bump, won't pick up fonts without an explicit reconversion sweep. Worth planning.

Verdict: changes requested. Points 1 and 3 are resolved, and the SDF work is solid. But the bounds are still enforced after the expensive expansion, so a crafted font under 1 KB can still exhaust the converter's memory. Fix P0 #1 and P1 #2–#4 and this is close.


Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U03JSUQ5Z7U>) via Slack

Comment thread crate/src/fontgen/rebuild.rs Outdated
Comment thread crate/src/fontgen/rebuild.rs Outdated
Comment thread crate/src/fontgen/kerning.rs Outdated
Comment thread crate/src/live.rs Outdated
Comment thread crate/src/live.rs Outdated
Comment thread crate/src/builder/mod.rs Outdated
Follow-up review on #126: the caps were enforced after the expensive expansion, so a crafted
font under 1 KB could still exhaust the converter.

- cmap: each subtable is walked over its own declared ranges, read raw, clamped to Unicode and
  charged against a budget for the whole font; encoding records are capped at 32 (also gating
  is_supported, since ttf-parser tries each record per glyph_index). No more codepoints().
- Composites: the glyf component graph is sized first with memoised, saturating counts; a glyph
  over the point cap, a font over the total, a cycle, over 64 direct components or nesting past
  32 is refused before any outline is expanded. Contours stop recording past the cap as well,
  and a glyph ttf-parser cannot outline whole is written empty.
- GPOS: kern lookup indices go into a fixed bit set under a visit budget; lookups count toward
  the subtable cap; a GPOS past a cap or with an unresolvable index ships without kerning (the
  legacy kern fallback applies only to fonts without a kern feature).
- fontgen::Refused is a typed error: live.rs tolerates only that, so a template, cache or fetch
  error fails the bundle and a retry builds the font; font_supported treats unreadable content
  as "build and see", and a refusal is cached per hash.
- Also: the font lane needs a .ttf path as well as TrueType bytes, build() applies the
  scene-only gate, coordinates are range-checked as floats, the assets record exactly the pairs
  the GPOS kept, bitmap-only fonts are refused, ".ttf" is defined once.
- Tests: a TrueType fixture assembler; a composite bomb, a composite cycle, a cmap group over
  all code points, cmap ranges past the budget, 40 encoding records, a 2000-record kern feature
  overlay, sfnt checksums, and the format-23 SerializedFile writer round-tripping the donor.
- fontcal prints a font's render work; Azeret Mono and Inter use about 2% of the budget.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@dalkia

dalkia commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

Follow-up addressed in 619b300. Every P0/P1 held up against the code; thanks for checking ttf-parser 0.25.1 itself.

P0 — cmap. No more codepoints(). Each unicode subtable is walked over its own declared ranges, read raw (formats 0/4/6/10/12/13), clamped to 0..=0x10FFFF, under a budget of 2 × 0x110000 visits for the whole font. Records are capped at 32 and deduped by offset, and that cap also gates is_supported (ttf-parser tries every record per glyph_index). Fixtures: a 0x41..=0xFFFFFFFF group clamps and maps 'A'; three full-range groups are refused; 40 records are refused.

P1 — composites. expanded_points walks the glyf component graph with memoised, saturating counts before any outline is expanded: over 8,192 points per glyph, 4M per font, a cycle, over 64 direct components or nesting past 32 is refused. Contours::push also stops recording past the cap, and an outline_glyph that returns None now writes an empty glyph. Fixtures: the 2³⁰ bomb and a two-glyph cycle, both refused before expansion.

P1 — GPOS feature walk. Indices go into a fixed 65,536-bit set; the walk stops after 65,536 indices named, 256 lookups + subtables visited (lookups now count), or an unresolvable lookup index. Each of those means "no kerning", which the PR text had claimed; the legacy kern fallback now applies only to fonts with no kern feature. Fixture: 2,000 feature records over one 65,535-index feature yields no pairs.

P1 — tolerated errors. fontgen::Refused is a typed error. live.rs tolerates only that; template, cache and fetch errors go to the failed list so a retry builds the font. font_supported returns true on a fetch failure, like image_decode_ok, so the build runs and the real error surfaces. A refusal is cached per hash, so no platform or request bakes the same font twice.

P2s done: the .ttf path is required alongside the bytes in build_bundle; build() applies the scene-only gate; coordinates are checked as finite floats before the cast; rebuild returns the pairs its GPOS kept and the assets record exactly those; bitmap-only fonts are refused; ".ttf" is defined once; the "TrueType and CFF" doc nit; checksum tests (whole file = 0xB1B0AFBA, each table matches its directory entry); the format-23 writer now round-trips the donor in a test.

Measured: fontcal prints render_work. Azeret Mono: 1.8% of MAX_RENDER_WORK, 9 ms bake with the rebuild; Inter: 2.0%, 20 ms. The comment now says so.

Not done: a FreeType/OTS load in CI (new toolchain dependency); USE_MY_METRICS/anchor fidelity is listed for the Unity check. Both are in the PR body under "Not covered yet".

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: 619b300

Thanks, this is a solid round. Most of the previous findings are now closed, and I checked each one against the code and ttf-parser 0.25.1. Three things still block merge: Windows CI fails on a test this commit adds, and the new composite and cmap pre-walks can each still be pushed into unbounded work.

Previous findings

Finding Status
P0 cmap enumeration ✅ Fixed for formats 0/4/6/10/12. Ranges are clamped, records are capped and deduped, and each range is charged before it is walked. Format 13 has a new issue (#2 below).
P1 composite point caps ⚠️ Not fully fixed (#1 below).
P1 GPOS feature walk ✅ Fixed. Indices go into a bit set, and the named, lookup and subtable counts are capped. The pair loop is bounded.
P1 every build error tolerated ✅ Fixed. Only Refused is tolerated, the downcast works through anyhow context, refusals are cached by hash, and a fetch failure no longer counts as a refusal.
P2s ✅ All fixed except FreeType/OTS in CI and composite fidelity, which you listed as follow-ups: path gating, the scene-only gate in build(), the float coordinate check, lookups counted, kerning fallback, kept pairs plumbed through, None outlines, bitmap-only fonts refused, refusal cache, checksum tests, format-23 round trip, .ttf constant, nits.

P1 — Major

  1. The composite pre-walk doesn't bound ttf-parser's work (fontgen/rebuild.rs:156-185, :201-250). It has two holes:
    • It counts points, not visits. A leaf with numberOfContours = 0 scores 0 points. A chain where each glyph references the previous one 64 times passes expanded_points, and ttf-parser's outline_impl then visits about 64^31 nodes. Fan-out 2 is already about 2e9 visits. No point is pushed, so the Contours::push cap never trips. The converter hangs on a font of a few KB.
    • It reads composites differently from ttf-parser. components() always skips 2 or 4 bytes of arguments. ttf-parser's CompositeGlyphIter reads them only when ARGS_ARE_XY_VALUES (0x0002) is set. With that flag clear, ttf-parser reads those bytes as the next component's flags and glyph id, so a font can hide components from the pre-walk. That brings back the point bomb (CPU-bound now, because the push cap limits memory).
    • Fix: memoise a saturating count of nodes (1 + children), and cap both the per-glyph and per-font totals. Parse components exactly the way ttf-parser does, including the args quirk. Even better, flatten composites from raw glyf yourself rather than calling outline_glyph on the source font, so the walk you bound is the walk that runs.
  2. cmap format 13 lookups are linear (rebuild.rs:543, fontgen/mod.rs:193, :229). ttf-parser's Subtable13::glyph_index scans every group.
    • Attack: a format 13 subtable with about 1.3M groups that clamp to empty (they cost no budget), plus one group covering 0..=0x10FFFF (which costs 1.1M of it). That is about 1e12 comparisons per platform build, from a 16 MB file.
    • Separately, face.glyph_index in is_supported and the pre-fill tries every unicode record. 32 records pointing at the same large format 13 subtable add up to about 8e9 comparisons on the gating path.
    • Fix: compute the glyph straight from the raw group while walking it (start_glyph for 13, start_glyph + cp - start for 12). For the gate and the pre-fill, use the map you built (deduped by offset) rather than Face::glyph_index, or refuse format 13 above a small group count.
  3. Windows CI fails (windows tests + sanity). fontgen::kerning::tests::a_feature_overlay_ships_without_kerning_instead_of_being_walked panics at ttf-parser-0.25.1/src/parser.rs:415 with attempt to add with overflow.
    • Cause: LazyArrayIter16 keeps its index as a u16. Walking a full 65,535-entry lookup_indices to the end overflows it on the call that would return None. Release builds wrap and stop, but debug and overflow-checked builds panic.
    • Fix: stop before exhausting the iterator (iterate 0..len with .get(i), or break at the cap before the last next()).
    • The test is also weak: its lookup has no subtables, so pairs is empty with or without the cap. Give it a real PairPos subtable so it shows the cap is what drops the kerning.

P2 — Minor

  • An oversized rebuild fails with an untyped error (rebuild.rs:1073). This error uses anyhow!, not refuse!. A font under 100 KB can stay under 4M expanded points and still rebuild past 16 MB, since encode_glyph writes no flag repeats. It then lands on the failed list, isn't cached, and every retry rebuilds it. Make it refuse!.
  • A .ttf that isn't TrueType falls through to the texture builder (builder/mod.rs:639-642). This covers OTF or WOFF bytes, or a file over 16 MB. Usually font_supported catches it first, but it now returns true when the fetch fails, so a texture bundle can ship under the font's name. Refuse any .ttf path whose bytes are not a font file.
  • The memo bypasses the depth check (rebuild.rs:224). A memoised result returns before depth > MAX_COMPOSITE_DEPTH is checked, so chains walked from the leaf up pass at depth 1. ttf-parser still stops at 32, so it does no harm today, but the refusal doesn't happen. Store each glyph's subtree height in the memo and check depth + height.
  • The point sizing and Contours::push count different things. Implied midpoints and closing points mean a real glyph with about 4,100 mostly off-curve points passes the sizing, then hits the push cap and refuses the whole font. Rare in practice.
  • The cmap range pre-pass isn't charged (subtable_ranges). It reads every group's record before the budget applies. That is bounded (about 45M reads) but uncharged.
  • Missing fixtures: a zero-point fan-out composite, the xy-args desync, a large format 13 subtable, a typed refusal through build_bundle, and an oversized rebuild.

Other checks

  • Real fonts: no regressions from the new caps that I could find. Format 14 yields no ranges, symbol (3,0) behaves as before, and format 4 with idRangeOffset goes through ttf-parser. ttf-parser's own format 4 as i16 drops glyph ids of 32768 and above, but that predates this PR.
  • CI: windows tests + sanity fails as described in #3. arm/x86 nextest, the lambda, node addon and wasm jobs pass. arm lints and x86 build + sanity were still pending when I checked. Address sanitizer was skipped.

Verdict: changes requested. The design is right, and this is close. Fix #1–#3 and add fixtures for them.


Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U03JSUQ5Z7U>) via Slack

Comment thread crate/src/fontgen/rebuild.rs
Comment thread crate/src/fontgen/rebuild.rs Outdated
Comment thread crate/src/fontgen/rebuild.rs Outdated
Comment thread crate/src/fontgen/mod.rs Outdated
Comment thread crate/src/fontgen/kerning.rs Outdated
Comment thread crate/src/fontgen/rebuild.rs Outdated
Comment thread crate/src/builder/mod.rs Outdated
…that runs

Re-review on #126: the composite pre-walk counted points while ttf-parser's work is per node,
it parsed components differently from ttf-parser (arguments are only present with
ARGS_ARE_XY_VALUES), format 13 lookups were linear, and walking a 65535-entry lookup index
list to its end overflowed ttf-parser's u16 iterator on Windows CI.

- glyf: composites are flattened by this crate from the raw table, mirroring ttf-parser 0.25
  (argument quirk, Transform::combine, depth 32, one-point glyphs empty); ttf-parser no longer
  outlines the source. Every glyph is sized first from the component graph with memoised,
  saturating node, point and height counts: over 1,024 visits or 8,192 points per glyph, 1M
  visits or 4M points per font, 64 direct components, nesting past 32 or a cycle is refused
  before anything expands. The fidelity test still compares ttf-parser's outline of the
  original with ttf-parser's outline of the rebuild.
- cmap: resolved per format from the raw subtables (0/4/6/10/12/13), never through
  Face::glyph_index; one unit of work per record read and per code point visited, under one
  budget for the font. The pre-fill and is_supported use this map too.
- kerning: lookup indices are read by position, not iterated; an unresolvable index means no
  kerning; the overlay fixture now kerns a real pair when under the cap.
- A .ttf path holding anything but a TrueType file is refused, not handed to the texture
  builder; an oversized rebuild is a refusal, so it is cached.
- Fixtures: a fan-out of empty leaves, ttf-parser's argument reading, a scaled composite, a
  1.3M-group format 13 flood under 16 MB, an oversized rebuild, a typed refusal through
  build_bundle.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@dalkia

dalkia commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

Re-review addressed in 4ad1654. All three blockers were real; I took the stronger option on #1 and #2 and read glyf and cmap raw, so the walk that is bounded is the only walk that runs.

#1 Composite pre-walk. ttf-parser no longer outlines the source at all. fontgen::rebuild flattens composites itself from the raw glyf, mirroring ttf-parser 0.25 exactly where it matters: arguments are read only with ARGS_ARE_XY_VALUES, the same Transform::combine, depth 32, one-point glyphs empty. Before any glyph is expanded, size_glyphs walks the component graph with memoised, saturating counts of visits (1 + children), points and subtree height: over 1,024 visits or 8,192 points per glyph, 1M visits or 4M points per font, 64 direct components, height past 32 or a cycle is refused. The memo now stores height, so the depth check holds whichever way the chain is reached. The fidelity test still compares ttf-parser's outline of the original against ttf-parser's outline of the rebuild (so it now also checks the flattener), plus two new fixtures: the argument-reading quirk (a glyph a naive always-skip parser would read with a different second component) and a scaled, offset composite. The empty-leaf fan-out (64³⁰ visits, zero points) is a fixture and is refused by its visit count.

#2 cmap format 13. unicode_map resolves every format itself from the raw subtable (0/4/6/10/12/13: start_glyph for 13, start_glyph + cp − start for 12, idDelta/idRangeOffset for 4). No Subtable::glyph_index or Face::glyph_index anywhere on the source. One unit of work per group/segment record read and per code point visited, so the range pre-pass is charged too; the 1.3M-group format 13 flood under 16 MB is a fixture and is refused. The pre-fill and is_supported now use this map instead of Face::glyph_index, which removes the 32-records-at-one-subtable amplification on the gate.

#3 Windows CI. Lookup indices are read by position (get(i) over 0..len), never iterated to the end. The overlay fixture now has a real PairPos subtable: one feature naming lookup 0 65,535 times kerns A→B by −50; 2,000 such records ship without kerning.

P2s: oversized rebuild is a refuse!; a .ttf path whose bytes aren't TrueType is refused in build_bundle instead of reaching the texture builder (fixture: WOFF bytes under a .ttf path); the memo/depth hole is closed by storing height; the point-sizing-vs-push mismatch is gone with ttf-parser out of the loop (the flattener's own point count is what is sized); the cmap pre-pass is charged; fixtures added for the fan-out, the xy-args quirk, format 13, a typed refusal through build_bundle, and an oversized rebuild.

Font tests 35/35, clippy and rustfmt clean locally. Still deferred, as before: a FreeType/OTS load in CI, and USE_MY_METRICS/anchor fidelity for the Unity check.

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of 4ad1654: changes requested

This commit closes the cmap, kerning and Windows CI findings. Reading glyf and cmap raw was the right move. The composite sizing is now nearly sound, but it has two holes, and each one lets a crafted font of a few hundred bytes to ~1 MB abort the converter process with a stack overflow. That is worse than a hang: it kills the whole worker, and catch_unwind cannot catch it.

These findings come from reading the code against ttf-parser 0.25.1. I did not run any crafted fonts.

Previous findings

  • Composite pre-walk (was P1): partly fixed.
    • Visits are now counted, and composites are parsed the same way in the sizing and flattening passes.
    • The memoised, saturating sizing is sound for glyph ids below numGlyphs: about 1M visits × 65 components, plus 4M points, bounds the flattener's work.
    • Two new holes remain; see below.
  • cmap format 13 (was P1): fixed.
    • Every record, segment and code point is charged before it is read, and ranges are clamped.
    • Work is at most ~2.2M units per font, and every read is bounds-checked.
    • is_supported and bake now share unicode_map, so the gate and the rebuild cannot disagree.
  • Windows CI / u16 iterator (was P1): fixed.
    • Lookup indices are read by position.
    • The other iterators still walked on source data (RecordListIter, LookupSubtablesIter) check bounds before incrementing.
    • The overlay fixture now really kerns, so the 2000-record test fails if the cap is removed.
  • P2s from the last round: fixed.
    • An oversized rebuild is a typed refusal.
    • A .ttf path holding anything other than TrueType is refused in both build_bundle and build_bundle_multi.
    • The memo stores height.
    • The cmap pre-pass is charged.

Blocking

  • P0: sizing and flattening use different glyph counts (rebuild.rs:418-425 vs :508, :210-213).
    • size_glyphs treats any component id at or above maxp.numGlyphs as a one-node leaf and never visits it.
    • flatten recurses into any id below glyph_count(), which comes from the raw loca length. It has no memo, no depth limit and no cycle check.
    • ttf-parser cuts loca down to numGlyphs + 1 entries, but GlyfTables reads the raw table.
    • Repro: numGlyphs = 2, loca with 4 entries, glyph 1 → glyph 2, glyph 2 → glyph 2. Sizing passes, then flatten recurses until the stack overflows.
    • A fan-out placed above numGlyphs gives unbounded expansion instead (64^k).
  • P0: the depth check runs after the recursion (rebuild.rs:444-456).
    • visit follows the full chain before testing height > MAX_COMPOSITE_DEPTH.
    • 65,535 composites, each referencing the next one, take about 1.1 MB. Sizing then goes ~65k frames deep, which overflows a 2 MB worker stack before the depth-32 refusal is reached.

Minor (P2)

  • Depth off by one (:454): ttf-parser stops at depth >= 32 (glyf.rs:439). A chain exactly 32 deep is drawn here, but would be empty when ttf-parser outlines the original.
  • One bad glyph refuses the whole font (:328-391): truncated flags or coordinates, or non-increasing contour ends, refuse the font. ttf-parser drops only that glyph. A real font with one damaged glyph that is never pre-filled would get no bundle at all.
  • Point-matched composites (:240): when ARGS_ARE_XY_VALUES is clear, the spec still stores the two point-number arguments, but no bytes are consumed here. ttf-parser has the same bug. Copying it was needed while ttf-parser did the outlining. Now that one parser does both passes, being spec-correct no longer opens a gap between sizing and flattening. As written, anchored composites decode wrongly, and components_are_read_as_ttf_parser_reads_them locks the misparse in.
  • cmap details that differ from ttf-parser:
    • (3,10) records are accepted in any format; ttf-parser requires format 12/13.
    • Format 12 uses wrapping_add where ttf-parser uses checked_add.
    • Format 6 maps past 0xFFFF.
    • A linear first-match walk resolves overlapping segments differently from ttf-parser's binary search.
    • These only affect malformed fonts.
  • Kerning: one dangling lookup index drops all GPOS kerning and does not fall back to kern. This is documented and deliberate, so it is a policy point.
  • Tests:
    • The composite-bomb test is now refused by the node cap, so it would still pass if the per-glyph point check were removed.
    • No test reaches MAX_FONT_POINTS, MAX_FONT_NODES or MAX_COMPOSITE_DEPTH. The oversized-rebuild fixture is about 3.94M points and is refused by output size.
    • flattened_composites_match_ttf_parser nests only one level, so it would pass if the Transform::combine arguments were swapped.
    • TWO_BY_TWO, WE_HAVE_A_SCALE and negative i8 offsets are untested.
    • The fixture helper always writes a loca that matches maxp, which is why the first P0 has no coverage.

Suggested fix

  • Set GlyfTables::glyph_count = min(loca entries - 1, numGlyphs), and use that same bound in visit and flatten.
  • Pass depth into visit and refuse before recursing once depth >= 32 (this also fixes the off-by-one).
  • Add fixtures for a loca longer than maxp and for a 65k-long chain.

CI

Passing: x86 build and nextest, arm nextest, Lambda build, node addon, wasm and the windows gate. Still pending: windows tests + sanity and arm lints + sanity. Address sanitizer was skipped.


Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni (<@U03JSUQ5Z7U>) via Slack

Comment thread crate/src/fontgen/rebuild.rs Outdated
Comment thread crate/src/fontgen/rebuild.rs Outdated
Comment thread crate/src/fontgen/rebuild.rs Outdated
…d on the way down

Re-review on #126, two stack overflows a small font could cause:

- The sizing stopped at maxp's glyph count while the flattener followed loca's, so a glyph
  past maxp was flattened unsized: no memo, no depth limit, no cycle check. GlyfTables now
  holds the one bound, min(loca entries - 1, numGlyphs), and both passes use it.
- The depth was checked after the recursion, so a 65k chain of single-component composites
  was followed to the bottom first. visit takes a depth and refuses before recursing at 32,
  which is also where ttf-parser gives up; a memo hit checks depth + height the same way.

Also: composites are flattened in each glyph's own space, so anchored components (arguments
as point numbers) are placed by their matched points per the specification instead of
copying ttf-parser's misread; a malformed simple glyph empties that glyph and its parents as
ttf-parser does, instead of refusing the font; (3,10) cmap records count only for formats
12/13, format 12 uses checked_add, format 6 stops at the BMP.

Fixtures: loca longer than maxp with a self-referencing glyph past it, a 65k chain, a chain of
exactly 32 and of 31, the per-glyph point cap without the node cap, the per-font point and node
caps, anchored components, three nested transforms (2x2, x/y scale with a negative byte
offset, uniform scale with word offsets) against ttf-parser, and a malformed glyph.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@dalkia

dalkia commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

Both P0s addressed in 8668606, and they were exactly as described: the two passes used different glyph bounds, and the depth check sat below the recursion.

P0 — glyph bound. GlyfTables now carries the one bound, min(loca entries − 1, numGlyphs), and glyph(), size_glyphs and flatten all use it; a component at or past it is skipped in both passes, as ttf-parser skips it. Fixture: maxp declaring 2 glyphs over a 3-entry loca whose third glyph is a composite of itself, referenced from glyph 1. Rebuilds to two glyphs, glyph 1 empty, no recursion.

P0 — depth. visit takes depth and refuses at entry once depth >= 32, before recursing; a memo hit refuses when depth + height >= 32. That bounds the sizing's own stack to 32 frames and fixes the off-by-one (a chain of exactly 32 is refused, which is where ttf-parser draws it empty; 31 draws in both). Fixtures: the 65k chain (~1 MB, refused at once), and the 32/31 pair against ttf-parser.

P2s taken:

  • Anchored components: now that one parser does both passes, the layout follows the spec (two arguments always present) and anchors are resolved by matching points, since the flattener works in each glyph's own space. The test that locked in the misread is replaced by one that checks a point-matched placement and that the component after it is still read.
  • A malformed simple glyph empties that glyph and any parent using it, as in ttf-parser, instead of refusing the font (fixture).
  • cmap: (3,10) records count only with formats 12/13, format 12 uses checked_add, format 6 stops at the BMP. The linear first-match walk stays; it only differs on overlapping segments in malformed fonts.
  • Tests: the per-glyph point cap is now reached by a fixture the node cap does not catch (50 × 200 points), and MAX_FONT_POINTS and MAX_FONT_NODES each have one; the nested-transform test goes three levels deep with a 2x2, an x/y scale with a negative byte offset and a uniform scale with word offsets, against ttf-parser, so a swapped composition order fails it.

Kept as policy: a dangling GPOS lookup index yields no kerning rather than a kern fallback; a font whose GPOS does not resolve is not one to guess at.

CI on 4ad1654 finished green across all nine jobs, including windows tests + sanity. Font tests 42/42, clippy and rustfmt clean locally on 8668606.

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of 8668606: approved

Both P0s from the last round are fixed. A crafted font can no longer overflow the stack or recurse without a bound, and I found no new P0 or P1. Two things are left before merge: a CPU-cost P2, and the Unity check of the rebuilt font that you already listed as open.

These findings come from reading the code against ttf-parser 0.25.1. No crafted fonts were run.

Previous findings

  • P0: sizing and flattening used different glyph counts: fixed.
    • GlyfTables::glyphs = min(loca entries − 1, numGlyphs) is now the only bound. glyph(), size_glyphs and flatten all use it.
    • size_glyphs visits every glyph id below the bound. flatten only recurses into ids below it, so it never reaches a glyph that wasn't sized.
    • This matches ttf-parser: loca is cut down to numGlyphs + 1 entries, and glyph_range skips any component outside it.
  • P0: the depth check ran after the recursion: fixed.
    • visit now refuses on entry once depth >= 32, and refuses a memo hit once depth + height >= 32. So sizing goes at most 33 frames deep.
    • Every glyph that passes has a height of 31 or less, so flatten is bounded the same way.
    • The depth off-by-one is gone: a chain of exactly 32 is refused, which is where ttf-parser's outline_impl returns None.
  • P2s: all fixed.
    • Anchored components are read as the spec lays them out, and placed by their matched points after the component's own scale.
    • A malformed simple glyph now empties that glyph and its parents, matching ttf-parser's ? on the component outline.
    • The cmap fixes are in: (3,10) records only count with format 12/13, format 12 uses checked_add, and format 6 stops at the BMP.
    • The new tests are real regressions: the 65k chain and the over-long loca would both overflow the stack on 4ad1654. There is a per-glyph point-cap test that the node cap doesn't catch, plus MAX_FONT_POINTS/MAX_FONT_NODES tests and a three-level transform test.
  • Kerning policy (a dangling lookup index means no kerning): accepted as a policy decision.

Checks with no findings

  • Point counts: simple_points and simple_contours agree. A malformed glyph yields fewer points than were sized, never more.
  • Panics: none reachable in the changed code.
    • Indexing in simple_contours is guarded by end > total.
    • Coordinate sums fit in i32.
    • Composite scales stay finite in f32, and to_points rejects them.
    • The encode_glyph casts are safe under the 8,192-point cap.

Minor (P2)

  • Uncharged work in flatten (rebuild.rs:499-541).
    • flatten has no memo. Each nesting level copies its child's contours, and each anchored component finds its point with out.iter().flatten().nth(i), which costs O(contours).
    • Possible construction, from reading the code: a glyph with 8,192 one-point contours, about 15 levels of composites, each adding 63 anchored references to an empty glyph with parent point 0xFFFF, and about 470 loca aliases of the top glyph.
    • That stays under every cap, at about 3.6e9 iterator steps plus about 1e8 small allocations. My estimate is roughly 5–15 s of CPU from a font under 30 KB.
    • It is bounded, so this is not a hang. But the font is not refused, and this work happens before the render budget applies.
    • Possible fixes:
      • Flatten into one point buffer with contour end indices, so anchors are an index lookup and there is one allocation per level.
      • Memoise flatten per glyph id.
      • Charge contours against a font-wide budget.
  • Fidelity nit: ttf-parser empties a composite when a component's data is under 10 bytes but not empty, because the header read fails. Here, the other components are kept. This only affects malformed fonts.

Before merge

  • CI on 8668606 is still running: windows tests + sanity, arm lints + sanity and x86 build + sanity were pending when I checked. The other jobs pass, and address sanitizer was skipped. This approval assumes those three go green.
  • The Unity check of a bundle carrying the rebuilt font is still open. Please confirm the points from earlier rounds:
    • glyphs render without hinting;
    • TextCore reads the minimal GPOS without duplicating pairs;
    • the kerning scale matches Unity's rounding;
    • accented composites render correctly. Anchored ones are now placed by matched points, so they are worth a look.

Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni (<@U03JSUQ5Z7U>) via Slack

…own points

Review on #126 (P2): flatten copied every nesting level's contours and found anchors with
nth() over nested contours, so a contrived font under 30 KB could spend seconds of CPU inside
the caps, before the render budget applied. A glyph now flattens into one point buffer with
contour end indices: an anchor is an index lookup and each component costs one copy of its
own points, so the work is bounded by the point caps the sizing already enforces.

The review's other nit, that ttf-parser empties a composite whose component has under ten
bytes of data, did not reproduce against ttf-parser 0.25.1: it reads such a glyph as empty
and keeps the other components. The rebuild does the same, and a test pins it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@dalkia

dalkia commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks for the approval. The remaining P2 is taken in 221598e: flatten now builds one point buffer per glyph with contour end indices, so an anchor is an index lookup and each component costs exactly one copy of its own points. The work is then bounded by the point caps the sizing already enforces, with no separate budget needed.

The fidelity nit didn't reproduce: against ttf-parser 0.25.1, a composite whose component has six bytes of data is drawn with its other components, not emptied. I kept the rebuild matching that and added a test that compares the two outlines, so if ttf-parser changes its mind the test says so.

CI on 8668606 finished green on all nine jobs; 221598e needs its own run, which is only this change plus the test. 43/43 font tests, clippy and rustfmt clean locally. The Unity check of the rebuilt font is the last item and is being done now.

@decentraland-bot decentraland-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of 221598e: approved

The flattening P2 from the last round is fixed. This commit adds no new P0 or P1, and CI is green on 221598e.

These findings come from reading the code against ttf-parser 0.25.1. No crafted fonts were run.

Previous findings

  • Uncharged work in flatten (was P2): fixed.
    • Flat { points, ends } keeps one buffer per glyph.
    • An anchor is now looked up by index: out.points[..first] for the parent point and out.points[first..] for the child point, which is taken after the component's own transform and before its offset. That matches the old semantics exactly.
    • The quadratic .nth() scan and the per-contour allocations are gone.
    • Work is now bounded by expanded points × composite height, at most about 4M × 32 point copies across the font.
    • Small doc nit: each point is copied once per nesting level, not once per glyph as the doc comment says. It is still bounded.
  • Panics: none reachable.
    • first <= len always holds, so both slices are valid.
    • ends only increases and never passes the buffer length: simple glyphs check that contour ends increase, and composite ends are first + child_end. So the slicing in into_contours stays in bounds.

Fidelity nit (P2): partly right on both sides

My last note was imprecise. Your test is correct for the case it covers, but the gap is still there for some other short stubs.

  • Zero contour count (your all-zero 6-byte stub): ttf-parser draws it empty and keeps the composite's other components, and so does the rebuild.
  • Negative contour count on a 2–9 byte stub (e.g. FF FF 00 00 00 00):
    • In ttf-parser, outline_impl reads −1, advances to offset 10, and s.tail()? returns None. The parent's ? passes that up, so the whole composite comes out empty.
    • The rebuild's components() reads nothing and simple_contours returns Some(empty), so the parent still draws.
  • A 1-byte glyph: ttf-parser's read::<i16>()? fails and empties the parent, but the rebuild treats the glyph as empty and keeps the parent.
  • Positive count on a short stub: the two agree.
  • Suggested fix: in flatten, return Ok(None) for a glyph whose contour count is negative and that is shorter than 10 bytes, or that is 1 byte long. Add an FF FF stub test that expects an empty outline.
  • This only affects malformed fonts and has no security impact.

Before merge

  • CI: green on 221598e, all nine jobs. Address sanitizer was skipped.
  • Unity check of the rebuilt font: still open; you said it is in progress. The points to confirm are unchanged:
    • glyphs render without hinting;
    • TextCore reads the minimal GPOS without duplicating pairs;
    • the kerning scale matches Unity's rounding;
    • accented and anchored composites render correctly.

Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni (<@U03JSUQ5Z7U>) via Slack

Scene font bundles are a new asset lane with a new recipe and template, so a minor bump.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@dalkia dalkia changed the title feat: scene font bundles with pre-filled TMP and UI Toolkit atlases feat: scene font bundles with pre-filled TMP and UI Toolkit atlases (0.20.0) Oct 5, 2026
@dalkia
dalkia merged commit 2b5571e into main Oct 5, 2026
10 checks passed
@dalkia
dalkia deleted the feat/font-bundles branch October 5, 2026 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants