Skip to content

[skera] Preserve instanced conditions, wide deltas, and MATH values - #2255

Closed
behdad wants to merge 12 commits into
skera-cff-review-charstringsfrom
skera-cff-review-fixes
Closed

behdad wants to merge 12 commits into
skera-cff-review-charstringsfrom
skera-cff-review-fixes

Conversation

@behdad

@behdad behdad commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Full and partial CFF2 instances could overflow layout/metric arithmetic, lose the sign threshold of large variable feature conditions, leave MATH values referencing a removed GDEF store, or retain stale average widths. Subsequent subsetting also silently discarded condition formats 2–5, making those substitutions unconditional.

Use saturating arithmetic for bounded fields and wide signed deltas for conditions. Encode rebased LONG_WORD sums through repeated region indexes and preserve oversized folded condition defaults in added constant store rows. Preserve recursive conditions and their variation-index remapping during subsetting; apply GDEF deltas to MATH values and recompute full-font average widths.

Validate borrowed recursive paint/condition trees before owned conversion, reject invalid source axis bounds, and return a clear error for unsupported VARC component instancing. Add ten regressions covering long-word extremes, malformed fonts, MATH composition, and partial-instance → subset → full-instance feature selection. Average-width expectations use HarfBuzz instances with the same glyph set.

Follows the charstring review fixes in native GitHub stack #2335. Coupled avar2 pins still use hidden axes until full instancing; restricted maps retain the documented F2Dot14 rounding limitation. VARC instancing remains unsupported.

Validation: cargo +1.89 test -p skera --all-features (185 unit, seven CFF semantic, 92 existing integration tests; eight existing ignored tests), Rust 1.89 Clippy with all features/targets and warnings denied, and OTS on 28 generated outputs (16 subsets, four full instances, four partial instances, four CFF1 downgrades). HarfBuzz comparisons cover outlines, metrics, and average widths.

Overnight review additions: validate direct API range bounds before clamping (NaN must return an error), use effective clamped pins for style metadata (out-of-range weight classes match HarfBuzz), and keep COLR delta sums wide until combining them with signed Fixed bases. Use floating CFF2 region scalars and round complete full-instance operands to preserve contours at half-unit boundaries. Four additional unit regressions and two HarfBuzz-backed contour cases pass. All 168 full-instance corpus cases now match HarfBuzz outlines and metrics; the subset corpus covers 2,256 cases and 174,904 glyph comparisons (the 24 source VARC-stub cases cannot be drawn by the independent oracle). The full suite passes 189 unit tests, seven CFF semantic tests, and 92 integration tests; the added eighth CFF test also passes. Rust 1.89 Clippy remains clean. Review continues.

Further corpus fixes: use HarfBuzz's centered CFF vertical-origin fallback when VORG is absent, including typo metric selection and MVAR. Preserve zero-byte PairPos class matrices during owned conversion (including extension lookups), with a bounded allocation check. The regressions and all 176 full-instance / 222 partial-instance corpus cases pass; partial comparisons cover 67,767 glyphs and subsequent full instancing. The full suite now passes 191 unit, eight CFF semantic, and 92 integration tests (291 total, eight existing ignored).

Malformed-input review: reject missing metric variation data and out-of-range HVAR/VVAR indices instead of indexing or unwrapping them; preserve explicit no-variation sentinels. Partial CFF2 stores emit zero itemCount/wordDeltaCount, with a format assertion in the semantic comparisons. The triggering mutation now returns an error, all 292 tests pass (192 unit, eight CFF, 92 integration; eight existing ignored), and Clippy is clean. A restarted deterministic mutation run has passed 419,722 cases without another panic so far.

Readers use read-fonts directly. write-fonts handles font assembly and owned table rebuilding; Skera retains its direct serializers and repacker. There is no separate generated Skera table tree.

Use saturating arithmetic for metric/layout fields and preserve wide signed
condition deltas. Encode large rebased sums with repeated region indexes;
add constant store rows when folded condition defaults cannot fit i16.

Preserve and remap recursive FeatureVariation conditions during subsequent
subsetting. Fold MATH values with the GDEF store and recompute the instance's
average width. Validate source axis bounds and borrowed recursive trees
before owned conversion. Reject unsupported VARC instancing.

Add semantic, malformed-input, long-word delta, and partial/subset/full
composition regressions. Compare full-font average widths against HarfBuzz
instances retaining the same glyphs.
Reject nonfinite/reversed ranges before deriving their default, so direct
AxisLimits API calls cannot panic on NaN bounds. Keep effective clamped pin
values in the axis plan so OS/2 weight and width classes match the instance.

Two targeted regressions and all 19 instancing tests pass. Out-of-range
weight classes agree with HarfBuzz (200 and 900 for AdobeVFPrototype).
A sum of LONG_WORD deltas can exceed i32 while still yielding a valid
Fixed field after adding an opposing signed base. Keep the sum in i64
and clamp only the final field value.

Add positive and negative cancellation regressions. All three color
instancing tests and Rust 1.89 Clippy pass.
Use HarfBuzz-compatible floating region scalars for charstring and Private
DICT folds, and round full operands after combining defaults and deltas.
Remove the unused generic transform fold. Add scalar and two real-font
half-unit contour regressions backed by HarfBuzz references.

The 168-case full-instance corpus now matches HarfBuzz outlines and metrics.
Center CFF glyphs without VORG in the horizontal line advance, selecting
OS/2 typo metrics and applying MVAR as HarfBuzz does. Recover zero-byte
PairPos matrix dimensions before owned layout instancing, including
extension lookups, and bound the inferred allocation.

Add HarfBuzz metric expectations and ordinary/extension GPOS regressions.
The 176 full-instance and 222 partial-instance corpus cases now pass.
Return errors for missing or out-of-range HVAR/VVAR data instead of indexing
empty vectors or unwrapping absent remappings. Preserve no-variation index
sentinels. Emit zero item/word-delta counts in partial CFF2 stores, whose
deltas live in charstrings and Private DICTs.

Add malformed/mixed-sentinel coverage and assert CFF2 store fields in the
HarfBuzz partial-instance comparisons. The triggering fuzz case is bounded.
@behdad
behdad force-pushed the skera-cff-review-fixes branch from 4762c66 to 44e4263 Compare October 14, 2026 23:11
@behdad

behdad commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Self-review of this slice, before applying corrections:

  • The new scalar/rounding and axis-validation changes overlap the fixes already moved into [skera] fully instance CFF2 outlines, metrics, and layout #2249/[skera] partially instance CFF2 fonts and shared variation stores #2250. I will retain the shared read-fonts float scalar calculation, continuous source bounds, and the distinct full/partial operand-rounding rules while integrating the reviewed parent.
  • Found a valid-input failure at both CFF2 VariationStore serialization sites: converting the byte length with u16::try_from rejects stores above 65,535 bytes. A source with 5,000 global regions and only one referenced region also hits this at the intermediate rebuild, before compaction; Skera fails and HarfBuzz succeeds.
  • As Behdad pointed out, OFF 2026 §5.3.3.11.2 explicitly permits ItemVariationStore data of 65,535 bytes or more and requires length=65535. The length is not needed to parse its internal offsets/counts. Both Skera write sites must saturate the field and preserve all bytes. I will cover a genuinely large final output as well as the intermediate-store case.
  • HarfBuzz's partial-instancing serializer has an analogous explicit 65,535-byte cap; I will reproduce and fix that in a separate HarfBuzz PR.

The remaining review covers the metric-index validation, shared variation rows, recursive layout conditions, and COLR changes. Findings here are incremental; they do not yet mark that review complete.

@behdad

behdad commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Two additional findings before fixing them:

  • Ordinary CFF2 subsetting copies the VariationStore using only its 16-bit length. For the permitted sentinel value, this truncates a larger store. The new large-store fixture fails the existing variation-location subsetting test. I will calculate the referenced extent from the borrowed store header, region list, and VarData byte ranges, preserving the original bytes without an owned-table conversion.
  • Format 2 feature conditions currently round their interpolated delta before comparing default + delta > 0. A positive fractional result such as 0.25 becomes zero and selects the wrong feature. A new focused regression fails before the fix. HarfBuzz evaluates the condition in double without integer rounding. I will preserve the fractional delta for full condition evaluation while retaining integer rounding for serialized positioning values and partial folded defaults.

The large-store fixture uses 200 glyphs, each with its own one-region VarData, and 60 axes. This keeps every region referenced and obeys the one-vsindex-per-charstring rule; its final store remains larger than the sentinel length. The original scratch fixture with repeated vsindex in one glyph was invalid and has been replaced before validation.

@behdad

behdad commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

One more review finding: the zero-byte class-pair matrix recovery handles PairPos format 2 only. The new format 4 has the same zero-byte record representation and loses its matrix dimensions during owned conversion too. Extending the existing regression to format 4 reproduces the gap. I will apply the same recovery and size guard to both formats, including extension lookups.

Integrate the reviewed parent while retaining shared float region scalars,
continuous bounds and separate metric normalization. Remove the MATH guard
now that this slice supplies its handler.

Use length=65535 for large CFF2 VariationStores as specified by OFF 2026
5.3.3.11.2, at intermediate and final serialization. Ordinary subsetting
copies through the actual referenced extent instead of truncating at the
sentinel. Add reproducible large-final and unused-region fixtures.

Evaluate feature-condition signs before rounding fractional deltas, using
float region scalars and double accumulation. Recover empty PairPos4
matrix dimensions as for PairPos2, including extension lookups.

Validation: 458 Skera tests, fractional-region reader regression, Clippy,
Rust1.85, formatting and typos.90full and38partial comparisons preserve
the checked outlines and metrics. Large outputs match HB at147932bytes
withlength65535; HB analogous serializer fix is PR6432.

Assisted-by: OpenAI Codex
@behdad

behdad commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Reviewed, fixed, validated and pushed as 5bb4605.

  • CFF2 VariationStores now use length=65535 when larger, at both intermediate and final serialization. Ordinary subsetting determines their actual referenced extent, preserving the bytes beyond the sentinel. Tests cover a genuinely large final store and an intermediate expansion with unused regions. Both Skera and patched HarfBuzz emit a 147,932-byte store with length=65535; HarfBuzz's analogous cap is fixed separately in [subset] Allow CFF2 VariationStores larger than 65535 bytes harfbuzz/harfbuzz#6432 (272 Meson tests pass, one skipped).
  • Format 2 feature conditions now compare the fractional interpolated value against zero before rounding, with float scalars and double accumulation. The positive-quarter-unit regression fails before and passes after.
  • Empty PairPos matrices preserve their dimensions in both formats 2 and 4, including extension lookups. The extended-format regression fails before and passes after.
  • Parent integration retains the shared reader scalar helper, continuous source bounds and separate metric coordinates. The temporary MATH guard is removed because this slice implements that consumer; the JSTF guard remains until its handler lands.

Validation: 458 Skera tests, reader scalar regression, Clippy with warnings denied, Rust 1.85, formatting and typos pass. All 90 full-instance comparisons preserve the checked outlines, metrics and Private DICT values; all 38 partial comparisons preserve axis records and sampled outlines within 0.01. Header differences against the older HB comparison binary include the separately fixed Private DICT reader issue (#6431), so this is not a claim of byte-for-byte table identity. Continuing with #2263.

Merge the repaired #2249 ancestry after the #2248 squash merge.
The tree is identical to the previously validated PR head.

Assisted-by: OpenAI Codex
@behdad

behdad commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by the rebuilt, consolidated instancing stack in #2416–#2423. MATH and name-table overrides remain separately covered by #2415 and #2411.

@behdad behdad closed this Oct 28, 2026
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.

1 participant