fix(fonts): deterministic subset choice in extract_embedded_fonts

[ajbufort]

Description

When several font subsets share a base name, get_font_set() yields them in HashMap order, so the or_insert in extract_embedded_fonts (and extract_embedded_fonts_with_unicode_maps) kept a nondeterministic subset - the bytes returned for a given PDF changed run to run. A consumer that re-runs extraction (or diffs/caches output) sees different font programs each time.

Both functions now keep the largest subset (most glyph coverage), tie-broken by a byte comparison for a total-order-stable choice. The Unicode/width maps in the _with_unicode_maps variant still accumulate across all subsets, so coverage is never reduced.

Found while debugging why a downstream PDF->HTML converter emitted byte-different output for the same input on every run; the embedded @font-face bytes were the source.

Type of Change

• Bug fix

Testing

cargo test --features rendering green on this branch (full suite). The change is localized to the two extraction functions; existing font tests pass unchanged.

Checklist

• Tests pass
• Code formatted
• Documentation updated (inline comments explaining the deterministic choice)

[yfedoseev]

🔴 Changes requested — needs a determinism test

@ajbufort the fix logic is sound: replacing or_insert with a max over the total-order key (bytes.len(), bytes.as_slice()) genuinely removes the HashMap-iteration-order nondeterminism, and the final out.sort_by keeps output ordering stable. O(n), clean.

Blocker: the PR whose entire purpose is determinism ships zero tests (gh pr diff 853 → no #[test]; and I confirmed no sibling PR or branch of yours adds one). Please add a test that:

1. builds a doc embedding two subsets of the same base font (ABCDEF+Helvetica, GHIJKL+Helvetica) of different sizes, and
2. asserts the larger subset is returned regardless of insertion order, plus repeated extraction yields identical bytes.

Also (non-blocking): the "largest bytes = most glyph coverage" comment overstates it (byte size also grows with hinting/aux tables) — soften it, or tiebreak on glyph count first. And flag as a follow-up: the cp→gid / gid→width maps still accumulate across all subsets while the emitted program is now a single chosen subset (potential GID/program mismatch).

[ajbufort]

Thanks - the blocker was fair. A determinism fix that ships no test is asking to be taken on trust, and that is exactly the thing that should not be.

1. Determinism test (blocker) - added.

build_pdf_with_two_font_subsets() builds a one-page PDF embedding ABCDEF+Helvetica and GHIJKL+Helvetica with font programs of different sizes, and the test asserts:

• the two subsets dedup to one entry, prefix stripped;
• the larger program wins regardless of which resource slot holds it - the helper takes a flag that swaps them, so the choice is shown to follow the total order rather than encounter order;
• repeated extraction of one unchanged PDF is byte-identical.

Extraction runs 64 rounds, because each call builds a fresh HashMap whose iteration order is independently seeded - a single round would pass on the old code by luck.

I checked the test actually earns its place rather than just going green: reverting either site to or_insert fails it at round 2, on both the plain and the _with_unicode_maps_and_widths variant. The latter carries its own copy of the subset choice, so it gets its own guard.

2. "largest = most glyph coverage" - softened.

You are right that it overstates it: byte count also grows with hinting and auxiliary tables, so a larger subset is not necessarily a superset of a smaller one. The comment now says what the total order actually buys - stability, not coverage - and stops claiming otherwise. I left the key as (len, bytes) rather than tiebreaking on glyph count: parsing numGlyphs out of each program would still need a byte tiebreak to be a total order, so it would add font-parsing failure modes to a PR whose only job is determinism. Happy to do it if you would rather have coverage as the primary key.

3. The map/program mismatch - flagged in code, and it is worse than it looks.

Noted in a comment at the site. Worth being precise about why it deserves its own PR: the maps do not merely risk a stale GID, they carry the very nondeterminism this PR removes from the program. When two subsets of one base font disagree about a codepoint's GID - which they routinely do, since each subset numbers its own glyphs - entry.1.entry(cp).or_insert(gid) keeps whichever HashMap order surfaced first. So extract_embedded_fonts_with_unicode_maps_and_widths can still return run-varying maps even with a stable program, and a GID in those maps need not exist in the program handed back.

The fix is to bind the maps to the chosen subset instead of merging across all of them. That is a behaviour change - coverage shrinks where subsets are genuinely disjoint - so I have deliberately kept it out of this PR. Say the word and I will open it as a follow-up.

cargo test, cargo clippy -- -D warnings and cargo fmt are green; both commits are DCO signed off.

[ajbufort]

The determinism test the review asked for is in f300b0f7 (on src/document.rs): build_pdf_with_two_font_subsets() builds a one-page PDF embedding ABCDEF+Helvetica and GHIJKL+Helvetica at different sizes, and the two tests assert (1) the larger subset wins regardless of which resource slot holds it (the helper swaps them), and (2) repeated extraction is byte-identical. Verified they earn their place: reverting either site to or_insert fails them. Also softened the "largest = most coverage" comment (byte size grows with hinting/aux tables; the guarantee is stability, not coverage). Ready for re-review.

[yfedoseev]

Approved — reviewed and regression-passed; in the merge queue. (Formal approval to supersede the earlier review state and record sign-off.)