Skip to content

feat(avif): reserve and write a C2PA manifest store, and locate one on read - #506

Open
justin13888 wants to merge 14 commits into
feat/443-isobmff-top-level-box-listfrom
feat/444-avif-c2pa-reserve
Open

justin13888 wants to merge 14 commits into
feat/443-isobmff-top-level-box-listfrom
feat/444-avif-c2pa-reserve

Conversation

@justin13888

@justin13888 justin13888 commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #498 (feat/443-isobmff-top-level-box-list): this PR is opened against that branch and merges only after it. It contains no gamut-isobmff or gamut-heic changes of its own.

This is an unattended backlog run. No human approved the plan; the decision record below is what a human reads afterwards.

Summary

Lets AvifEncoder reserve and write a C2PA manifest store, and lets the AVIF read path locate one and report its byte range — the first write side of epic #239, on the top-level box slice #498 added to gamut-isobmff.

  • Reserve. AvifEncoder::with_c2pa_reserved(len) writes the C2PA 2.4 §A.5.1 ContentProvenanceBox — a top-level uuid box with user type D8FEC3D6-1B0E-483C-9297-5828877EC481, zero FullBox version/flags, box_purpose manifest, a zero 8-byte merkle offset — after ftyp and before meta (§A.5.3, via IsoBmffImage::push_top_level_box) around a slot of len zero bytes for an external signer to fill.
  • Write. AvifEncoder::with_c2pa(bytes) writes a store the caller has already computed over this exact output; its doc says so and says the metadata facade never hands one over.
  • Report. AvifEncoder::encode_with_report(image) -> (Vec<u8>, AvifEncodeReport { c2pa: Option<Range<usize>> }) returns the same bytes encode_to_vec would plus the slot's file range, found by the crate's own locator over the finished file. EncodeImage is untouched.
  • Locate. AvifContainer::c2pa() / c2pa_manifest_stores() report every top-level C2PA uuid box as a C2paSlot { slot_bytes, range, purpose: C2paBoxPurpose }, in file order. The type is named for the box-bounded slot it reports — the store plus any §A.5.3 padding, or zeros for an unfilled reservation — because gamut_heic::C2paManifestStore trims to the JUMBF LBox and both are re-exported from the umbrella. update boxes are probed over the same [8, 0] prefixes gamut-heic uses. New public items: C2PA_UUID, C2paBoxPurpose, C2paSlot, AvifEncodeReport.
  • Range semantics. Every doc states the range is for patching and byte accounting, and that BMFF binds by box path (§18.6/§A.5.6); no type is named "exclusion".

Files: new crates/gamut-avif/src/c2pa.rs, crates/gamut-avif/src/encoder.rs, crates/gamut-avif/src/lib.rs, new crates/gamut-avif/tests/c2pa.rs, crates/gamut-avif/STATUS.md, crates/gamut-avif/README.md.

Tests (each names the function whose mutation it kills):

  • exact-byte (tests/c2pa.rs): two equal-length stores give files differing in exactly the reported span and nowhere else; patching a reserved file at the reported range reproduces the file written with the store outright; encode_with_report bytes equal encode_to_vec and the range is None when unconfigured.
  • read-back: the crate locates the slot it reserved at the reported range; a hand-transcribed §A.5.1.2 original + update pair is reported in file order.
  • oracle: libavif (decode_avif + introspect) and dav1d (through AvifContainer::parse + decode_primary_rgba8) decode a file carrying the reserved box to the same pixels as one without.
  • inline (src/c2pa.rs, src/encoder.rs): the §A.5.1.2 payload layout exact-byte; the parser's user-type / version-flags / purpose / truncation rules and offset arithmetic; the box is emitted AfterFtyp with no item or property added; last call wins across both knobs; Debug prints the slot by kind and length.

Validation

All commands run from the worktree on feat/444-avif-c2pa-reserve; classification per command.

Command Outcome Class
cargo check -p gamut-avif --lib pass
cargo clippy -p gamut-avif --all-targets --all-features -- -D warnings pass (exit 0)
cargo test -p gamut-avif --all-features pass: every test binary green, incl. 9 new inline tests and the 7 tests of tests/c2pa.rs (libavif + dav1d oracles built from submodules)
RUSTDOCFLAGS="-D warnings" cargo doc -p gamut-avif --no-deps --all-features pass (exit 0)
cargo test -p gamut-avif --all-features --doc pass (3 doctests)
__CARGO_TEST_ROOT=<worktree> mise run fmt then __CARGO_TEST_ROOT=<worktree> mise run fmt-check pass (exit 0). The prefix is the recorded workaround for the nested-worktree tooling/* manifest resolution
mise run check-tests pass: "module docs, pinned proptest seeds and oracle filenames all conform"
mise run check-commits / convco check origin/feat/443-isobmff-top-level-box-list..HEAD pass (no errors in 14 / 2 commits)
mise run lint (systemd scope, MemoryMax=16G, CARGO_BUILD_JOBS=2, ulimit -v 12000000) pass (exit 0) — run twice: before the PR opened and again on the final head after the repair below
mise run test (same scope) pass (exit 0, 204 green test binaries, no failures) on the pre-repair head; re-run on the final head 38c7c3e: pass (exit 0, 204 green test binaries, no failures)
GAMUT_MUTANTS_BASE=origin/feat/443-isobmff-top-level-box-list mise run mutants-crate gamut-avif --diff --shard 0/1 --budget 16 (the runner's own capped scope; the --diff selection is what CI's incremental job blocks on) first run: 41 mutants, 35 caught, 5 unviable, 1 missedc2pa.rs:211 replace match guard &ty == b"uuid" with true in manifest_stores. Repaired in test(avif): key the C2PA locator fixture on the box type, not the body (the fixture's free box now carries a C2PA-shaped body, which must not be reported per §A.5.1.1; the exact mutant expression was applied by hand and confirmed to fail that test). Second run on the final head: 41 tested, 36 caught, 5 unviable, 0 missed (exit 0) caused → repaired
post-repair: cargo test -p gamut-avif -p gamut-isobmff --all-features (after merging #498's advanced head c8b87cf by merge commit), mise run fmt-check, mise run check-tests, convco check origin/feat/443-isobmff-top-level-box-list..HEAD all pass

Review round (five repairs R1–R5 from the read-only review of 38c7c3e, applied as five commits; every gate re-run on 32cdc93):

Command Outcome Class
cargo test -p gamut-avif --all-features pass (exit 0, 15 green test binaries; 11 inline c2pa tests, 7 in tests/c2pa.rs)
cargo clippy -p gamut-avif --all-targets --all-features -- -D warnings pass (exit 0)
RUSTDOCFLAGS="-D warnings" cargo doc -p gamut-avif --no-deps --all-features first run failed: public documentation for C2paBoxPurpose links to private item Self::store_prefix_candidates — caused by R2's new doc; link removed, re-run passes. cargo test --doc 3 pass caused → repaired
__CARGO_TEST_ROOT=<worktree> mise run fmt-check, mise run check-tests, convco check origin/feat/443-isobmff-top-level-box-list..HEAD (8 commits) all pass
mise run lint (capped scope) pass (exit 0)
mise run test (capped scope) pass (exit 0, 204 green test binaries)
GAMUT_MUTANTS_BASE=origin/feat/443-isobmff-top-level-box-list mise run mutants-crate gamut-avif --diff --shard 0/1 --budget 16 52 mutants: 44 caught, 8 unviable, 0 missed (exit 0) — the repairs added mutants and no survivors

No Cargo.toml was touched, so check-release-deps / check-ffi-features / check-ffi-header are not triggered; no C-surface type changed.

CI status: red for an infrastructure outage, not for this change (#517). Every required check on 38c7c3e was green on 2026-09-06. Since 2026-09-09 every job in this repository fails in its first step, Install system build deps: sudo apt-get update exits 100 with E: Failed to fetch …/chrome-stable/… Hash Sum mismatch from the runner image's preinstalled Google Chrome apt repo, so pkg-config/mise/sccache never install and every later step dies with sccache: command not found (127). No gamut code is compiled before the failure, so the runs triggered by the repair push are red for that reason alone — classified unavailable / infrastructure, tracked as #517, and deliberately not "repaired" here. The local gates above, re-run in full on 32cdc93, are this change's evidence.

Correction (round 3): the paragraph above is superseded. It is false at 32cdc93. #517 was fixed before that head ran, and the required checks did run: Format & Metadata pass, Clippy & Doctests pass, and all four Incremental (PR diff) shards pass (Mutants run 34384870602, conclusion success). Coverage (test gate) ran the whole suite to completion — TOTAL 133659 regions, 4641 missed, 96.53%, with crates/gamut-avif/src/c2pa.rs at 436 regions, 5 missed, 98.85% region and 26 functions, 0 missed, 100.00% function, both comfortably over the 80 % floor — and then failed in the actions/upload-artifact@v6 step alone:

2026-09-09T19:00:48.5089881Z ##[error]Failed to CreateArtifact: Unable to make request: ETIMEDOUT

That is an artifact-upload timeout after the gate itself had passed, so the correct classification for Coverage at 32cdc93 is unavailable (infrastructure, artifact upload), not caused and not the #517 apt outage. The check is re-run at the round-3 head below.

Round 3 (head 55bd77d6)

Every command below completed in this round; the workspace-wide ones ran under a 16 GiB memory-capped scope with CARGO_BUILD_JOBS=2.

Command Result Classification
CARGO_BUILD_JOBS=2 cargo test -p gamut-avif --all-features 226 tests over 14 test binaries plus 3 doctests, 229 in all, 0 failed pass
CARGO_BUILD_JOBS=2 cargo clippy -p gamut-avif --all-targets --all-features -- -D warnings clean pass
__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel) mise run fmt-check clean (the prefix works around cargo walking past a nested worktree root when loading the tooling/* manifests; no manifest was changed) pass
mise run check-tests module docs, pinned proptest seeds and oracle filenames all conform pass
convco check origin/feat/443-isobmff-top-level-box-list..HEAD no errors in 10 commits pass
mise run lint clean over the whole workspace pass
mise run test 204 test result: ok blocks, 0 failed pass
mise run mutants-diff first run: 87 mutants, 69 caught, 17 unviable, 1 missedc2pa.rs:348 replace + with * in content_provenance_payload, an unobservable Vec capacity hint. Repaired by removing the arithmetic (reserve_exact) rather than excluding the mutant. Second run on the final head: 85 mutants, 68 caught, 17 unviable, 0 missed (exit 0) caused → repaired

The F1 repair, executed in both profiles. A standalone consumer crate outside the workspace, depending on gamut-avif by path, calling AvifEncoder::new().with_c2pa_reserved(len).encode_to_vec(img):

len Before, debug Before, release After, both profiles
usize::MAX panic: attempt to add with overflow at c2pa.rs:287 Ok, 345 bytes, c2pa_manifest_stores() finds 0 Err(InvalidInput): "the reserved C2PA slot exceeds the largest ContentProvenanceBox that fits in memory"
usize::MAX - 21 panic: capacity overflow in RawVec (found while repairing; the sum is representable but larger than a Vec can hold) Err(InvalidInput), same message
0, 7 wrote an unusable slot wrote an unusable slot Err(InvalidInput): "a reserved C2PA slot is at least 8 bytes, the size of a JUMBF box header"
8 Ok, 354 bytes, c2pa_slots() finds 1

No Cargo.toml was touched in round 3 either, so check-release-deps / check-ffi-features / check-ffi-header remain untriggered; no C-surface type changed. mise run coverage was not re-run locally — no new module was added and CI's Coverage gate runs the full suite; the round-2 head measured this file at 98.85 % region / 100 % function.

CI at 55bd77d6, all required checks terminal and green: Format & Metadata pass (2m06s), Clippy & Doctests pass (21m02s), Incremental (PR diff) shards 0-3 pass (4m51s / 5m49s / 5m42s / 5m54s), Coverage (test gate) pass (33m27s). Coverage passing on re-run confirms the round-2 failure was the transient artifact upload and not this change — unavailable, as classified above.

Round 4 (closing) — validation

Commits ec2412a, 05f9bbd, c38e64c. The round changes documentation and adds one assertion;
no emitted byte and no returned range moves.

Command Result
__CARGO_TEST_ROOT=$(git rev-parse --show-toplevel) mise run fmt-check pass
mise run check-tests pass — "module docs, pinned proptest seeds and oracle filenames all conform"
convco check origin/master..HEAD pass — "no errors in 27 commits"
mise run mutants-diff (capped scope, MemoryMax=16G) 85 mutants tested in 6m: 68 caught, 17 unviable, 0 missed
cargo test -p gamut-avif --all-features (capped scope, oracles built from third_party/) pass — 15 targets, 229 tests, 0 failed
cargo clippy -p gamut-avif --all-targets --all-features -- -D warnings pass
RUSTDOCFLAGS="-D warnings" cargo doc -p gamut-avif --all-features --no-deps pass (every new intra-doc link resolves)

Workspace gates deliberately not re-run this round, with the reason. mise run lint and
mise run test were skipped: the only compiled files that changed are one crate's doc comments
and one inline test, so the crate-scoped Clippy, rustdoc-with--D warnings and full
--all-features suite above exercise the same lints over exactly the changed files, and the
workspace runs are what CI's Clippy & Doctests and Coverage jobs perform on the pushed head
anyway. mise run coverage was skipped: no module was added, and the added assertion only
strengthens an already-covered function. check-release-deps / check-ffi-features /
check-ffi-header stay untriggered — no Cargo.toml and no C-surface type changed. The AV1/AVIF
oracle submodules were already present in the worktree, so mise run fetch-av1-oracles was not
needed.

Evidence for the new assertion. With MIN_SLOT_LEN set to 9, cargo test -p gamut-avif --lib reports 44 passed; 1 failed, the single failure being
c2pa::tests::a_reservation_below_a_jumbf_box_header_is_refused at the is_ok() on length 8.
With it set to 7, the same test fails at the expect_err on length 7. At 8 all 45 pass. Before
this round both 7 and 9 left all 45 green.

Evidence for the two findings, executed at 55bd77d6.

  • Third refusal: with_c2pa_reserved(4_294_967_251) returns unsupported: ISOBMFF: top-level box at or beyond 4 GiB [origin: gamut-isobmff]; 4_294_967_250 clears that check and then aborts
    with memory allocation of 8589934654 bytes failed inside the writer's copy.
  • Supplied store vs. reservation: with_c2pa(&[0xAB; 3]) encodes Ok, and
    AvifContainer::c2pa_slot returns C2paSlot { slot_bytes: [171, 171, 171], range: 77..80, purpose: Manifest }; with_c2pa_reserved(3) returns invalid input: AVIF: a reserved C2PA slot is at least 8 bytes, the size of a JUMBF box header [origin: gamut-avif].

Correction to a round-3 commit message. The body of 55bd77d6 ("refactor(avif): ask for the
store's room instead of adding it in") says the buffer ends up exactly the size needed.
Vec::reserve_exact guarantees only at least the requested capacity; it may over-allocate. The
pushed commit is not amended — this entry is the correction. Nothing in the code or its behaviour
depends on the difference: a capacity is a hint, which is why the mutant on that path was
unkillable and the arithmetic was removed rather than excluded.

CI at c38e64c7, all required checks terminal and green: Format & Metadata pass (41s),
Clippy & Doctests pass (17m35s), Incremental (PR diff) shards 0–3 pass (4m49s / 5m50s /
5m45s / 5m27s), Coverage (test gate) pass (23m15s). "Full workspace" is skipping, as it is
on every PR head.

Risks and rollout

  • Additive, semver-minor for gamut-avif: new builder methods on the non-Copy AvifEncoder, one new inherent method, four new public items (all #[non_exhaustive] where they are structs/enums). An unconfigured encoder writes byte-identical output (pinned: no item, no property, no top-level box is added).
  • The read-side slot is box-bounded, not LBox-trimmed as gamut-heic's is, so the two crates report different bounds for the same file today. Recorded in STATUS.md; the shared lens is gamut-isobmff: share the C2PA uuid lens between gamut-heic and gamut-avif #505.
  • update framing is assumed to carry the 8-byte merkle offset (§A.5.3 is silent). Recorded.
  • The c2pa-rs reserve → sign → validate direction is tooling/c2pa-oracle: differential oracle against c2pa-rs, both directions #447's; nothing here claims validation.
  • Rollback: revert the two commits; no on-disk format changes outside the optional box.

Issue

Closes #444. Follow-up filed: #505 "gamut-isobmff: share the C2PA uuid lens between gamut-heic and gamut-avif".

Decisions taken

Issue 444 - gamut-avif: reserve and write a C2PA manifest store, and locate one on read
Plan:     v1
Branch:   feat/444-avif-c2pa-reserve
Base:     head of PR #498 (origin/feat/443-isobmff-top-level-box-list, 2e7f2ef) - stacked; PR opened against that branch
Cause:    -
Touches:  gamut-avif encoder.rs (with_c2pa_reserved / with_c2pa), image.rs (locate), new c2pa.rs, lib.rs, tests, STATUS/README
Will not: change EncodeImage or any codec-abi surface; move gamut-heic's typed lens; parse the JUMBF interior; offer the located range as a hash exclusion range; touch gamut-isobmff
Lane:     serialised behind E-443 (stacked); #447's direction 1 branches from this head
Settled:  S2 semver (AvifEncoder/AvifImage additions are minor: check #[non_exhaustive]/builder shape before assuming); S3 docs/testing.md; #498's decision 4a (push_top_level_box; interleaving refused)

Decisions taken.
1. Deliverable boundary
   Taken:    the whole issue - reserve+write on AvifEncoder, locate on read, range reported for observability; no split
   Filed:    -
2. Read lens
   Taken:    a minimal AVIF locator over IsoBmffImage::top_level_boxes: first top-level uuid box with the C2PA user type -> `AvifImage::c2pa() -> Option<C2paManifestStore>` giving the store bytes (payload after version/flags/box_purpose NUL-terminated string/merkle offset per §A.5.1 - transcribe the layout from the vendored clause, do not copy gamut-heic's code), its byte range in the file, and the purpose string; `c2pa_manifest_stores()` iterator for files mid-update carrying two
   Rejected: moving gamut-heic's typed lens down into gamut-isobmff - a second issue's scope; filed
   Reverses: replace the local parse with a call into a shared lens once one exists
   Filed:    "gamut-isobmff: share the C2PA uuid lens between gamut-heic and gamut-avif" (search first; file if absent)
3. Reserve API
   Taken:    `AvifEncoder::with_c2pa_reserved(len: usize)` writes a uuid box whose store region is `len` zero bytes with `box_purpose = "manifest"` and no merkle offset, placed AfterFtyp via push_top_level_box; `AvifEncoder::with_c2pa(bytes)` writes a caller-computed store and its doc says the facade never hands one over (C2paPolicy); the encoder reports the reserved region's file range through a new `encode_with_report(...) -> (bytes, AvifEncodeReport { c2pa: Option<Range<usize>> })` alongside the unchanged `EncodeImage` path
   Rejected: a bytes-only setter - cannot satisfy the epic's reserve criterion; returning the range from EncodeImage - changes the object-safe seam
   Reverses: drop encode_with_report and return the range via a getter after encode
4. Range semantics
   Taken:    the reported range is the uuid box's payload span used for patching/observability; the doc states BMFF binds by box path (§18.6/§A.5.6) and the type is not named "exclusion"; exact-byte test: two equal-length payloads written into the reserved region give two files differing exactly in that span and nowhere else
   Reverses: none needed
5. Oracle
   Taken:    libavif and dav1d must decode a file carrying the reserved box unchanged (pixel-identical to the same encode without it); the c2pa-rs direction-1 test is #447's

Appended by the lane (forks the record did not cover), in the record's shape:

6. Locator home
   Taken:    `AvifContainer::c2pa()` / `c2pa_manifest_stores()` - the byte-accounting layer, which owns the file bytes and the segment ranges - not `AvifImage::c2pa()`: the model (`IsoBmffImage::top_level_boxes`) carries no file offsets, so the range decision 2 requires cannot be reported from `AvifImage`. The boxes examined are the top-level `uuid` segments of the primary stream, the same set `read` fills `top_level_boxes` from. `image.rs` is therefore untouched.
   Rejected: `AvifImage::c2pa()` returning bytes + purpose without a range - fails the record's own requirement; deriving offsets by re-serialising the model - a normalised remux is not the file
   Reverses: none needed; #505 may move the function again
7. Slot bound
   Taken:    `C2paManifestStore::bytes`/`range` run from just after the 8-byte merkle offset to the end of the uuid box (the store plus any §A.5.3 padding); no JUMBF `LBox` trim; a reserved, unfilled slot reads back as zeros, and the write-side range equals the read-side range for the same file (pinned)
   Rejected: trimming to `LBox` as gamut-heic does - an all-zero reserved slot would read back as absent, so `encode_with_report` could not locate its own reservation; copying heic's parse - the record forbids it
   Reverses: #505 decides the bound once for both crates
8. Merkle offset
   Taken:    decision 3's "no merkle offset" read as "no merkle *box*": the 8 bytes §A.5.3 makes a `shall` for `box_purpose = manifest` are written, as zero
   Rejected: omitting the 8 bytes - puts the box out of spec and mislocates the store for every §A.5.3 reader
   Reverses: none needed
9. `update` framing
   Taken:    the same 8-byte merkle-offset prefix is applied to all three purposes, documented as an assumption (§A.5.3 states it only for manifest/original; the reference implementation writes it for update too)
   Rejected: gamut-heic's LBox-validity probe - a box-bounded slot has no in-band length to probe with
   Reverses: #505
10. Range computation
   Taken:    `encode_with_report` runs the crate's own locator (`gamut_isobmff::walk_segments` + `manifest_stores`) over the finished bytes, so gamut-avif never re-derives the writer's layout; a configured slot that cannot be located is `Error::InvalidInput`, never a silent `None`
   Rejected: computing the offset from the `ftyp` length and model order - couples gamut-avif to gamut-isobmff's serialisation
   Reverses: if gamut-isobmff's `write` ever reports top-level box offsets, this becomes a lookup
11. Purpose type
   Taken:    `C2paBoxPurpose` is a fieldless `#[repr(u8)]` `#[non_exhaustive]` enum with `as_str()`, not a `String` - the C-portability rule for plain-data payloads
   Rejected: a `String` purpose
   Reverses: none needed
12. dav1d oracle bridge
   Taken:    a local `Dav1dDecoder` in `tests/c2pa.rs`; `tests/conformance.rs` has its own and does not use `tests/common`
   Rejected: refactoring conformance.rs to share one - churn outside the feature
   Reverses: none needed

Appended after the read-only review of 38c7c3e (every item below decided by the orchestrator on review, not by the author):

13. Review repairs R1-R5 (orchestrator, on review of 38c7c3e)
   R1 Slot naming - amends decision 7
   Taken:    keep the box bound and RENAME: `C2paManifestStore` -> `C2paSlot`, `bytes` -> `slot_bytes`,
             documented as the box-bounded slot (store plus any §A.5.3 padding, or zeros for an unfilled
             reservation) and pointing at #505 for the LBox unification. The private encoder enum takes
             the name `SlotSource`.
   Rejected: trimming to the JUMBF `LBox` on read (an all-zero reservation has no `LBox`, so
             `encode_with_report` could not locate its own reservation); leaving the name (one name
             returning 100 vs 4100 bytes for one file across two crates re-exported from the umbrella)
   Reverses: rename back and trim on read
   R2 `update` prefix probe - amends decision 9
   Taken:    probe the same `[8, 0]` candidates `gamut-heic` uses, for `update` only; pinned by a test
             where a prefix-less `update` store shorter than 8 bytes is located rather than dropped, and
             the stated purposes do not fall back
   Rejected: leaving the two crates divergent about identical bytes
   Reverses: return to a single stated offset
   R3 Reserved-slot allocation
   Taken:    split the payload builder into framing + slot and `resize` the zeros into the payload buffer,
             so reserving n bytes peaks at n rather than 2n (CLAUDE.md's allocation-conscious rule);
             equivalence with the zero-slot payload pinned at three lengths x three purposes
   Rejected: keeping `vec![0; len]` + copy
   Reverses: none needed
   R4 dav1d bridge placement - amends decision 12
   Taken:    move the bridge into `tests/common/mod.rs`, whose `allow(dead_code)` exists for exactly this;
             `tests/c2pa.rs` already declared `mod common`. `conformance.rs` / `high_bitdepth_roundtrip.rs`
             keep their own copies - moving those is churn outside this change
   Rejected: a third local copy (the dead-code objection is falsified at zero churn)
   Reverses: re-inline the bridge
   R5 Write-side purpose limit
   Taken:    state on BOTH `with_c2pa_reserved` and `with_c2pa` that the encoder writes `box_purpose =
             manifest` only, while the reader reports all three, and why (§A.5.3 re-labels the earlier
             store `original` once an `update` box exists); recorded as a fourth limit in STATUS.md
   Rejected: silence
   Reverses: none needed
14. Review questions decided without a code change (orchestrator, on review of 38c7c3e)
   Q3 write-side `manifest`-only
   Taken:    it is the intended scope of #444; documented by R5
   Rejected: filing an issue for an `original`/`update` writer - no demand, and it is a file-update
             operation rather than an encode
   Reverses: file it when a caller needs to update an existing manifest
   Q4 two knobs, last-wins
   Taken:    KEPT and documented - it matches every other payload builder on this encoder
             (`with_exif`/`with_xmp`/`with_icc_profile`)
   Rejected: erroring on a second call - would make this one builder behave unlike its siblings
   Reverses: replace with a typed error if the pair ever grows a third member
   Q5 a newtype for "not a hash exclusion range"
   Taken:    deferred to #505, where it can land in both crates at once; carried below as a residual
   Rejected: introducing an AVIF-only newtype now - it would need renaming when heic adopts it
   Reverses: land the newtype in #505
   Q6 mutation evidence
   Taken:    the lane's 44/8/0 diff-mutants result stands; no re-run - the reviewer could not build the
             suite (the `dav1d` submodule is `update = none` in their worktree), and CI's four
             `Incremental (PR diff)` shards ran the same in-diff gate independently and were green at
             38c7c3e
   Rejected: re-running to satisfy an unreproduced result
   Reverses: none needed

Round 3 — decisions carried out

The seven below were taken before this round began and were executed, not reopened. No human approved them: this is an unattended run, and this record is what a human reads afterwards.

R3-1 F1: with_c2pa_reserved near usize::MAX
   Taken:    return a typed error, do not merely document a bound. Checked arithmetic,
             refused at encode as Error::InvalidInput, and — for consistency with MIN_STORE_LEN
             on #508 and min_store_len on #520 — a reservation shorter than the 8-byte JUMBF
             box header is refused too
   Evidence: in release, with_c2pa_reserved(usize::MAX).encode_to_vec(img) returned Ok with a
             uuid box whose 21-byte framing had been resized to 20, and c2pa_manifest_stores()
             then found zero. A silent wrong answer on the range a signer binds is the one
             outcome this epic cannot ship
   Rejected: documenting the bound and leaving the arithmetic unchecked
   Reverses: none needed

R3-2 the write side refuses a degenerate reservation, the read side keeps reporting one
   Taken:    strict in what it writes, honest about what it reads. The asymmetry is documented
             deliberately on both sides — on with_c2pa_reserved and on c2pa_slots — rather than
             left to be discovered
   Rejected: making the locator hide a zero-length slot that is genuinely in the file
   Reverses: none needed

R3-3 F2: the rename stopped at the type
   Taken:    AvifContainer::c2pa -> c2pa_slot, c2pa_manifest_stores -> c2pa_slots, and the
             private locator manifest_stores -> slots. The doc sentence still opening "Every
             C2PA manifest store among the top-level boxes" now says slot
   Evidence: both old names were byte-identical to gamut-heic's, whose bounds differ (box-bounded
             slot here, JUMBF-LBox-trimmed store there), with both crates re-exported from the
             umbrella. Cheaper now than after #505. Every remaining use of the old vocabulary is
             inside crates/gamut-avif; gamut-heic's identically-named methods are untouched
   Rejected: renaming gamut-heic's methods as well
   Reverses: none needed

R3-4 F3: the PR body's CI claim
   Taken:    corrected in place, with the Coverage failure classified `unavailable` on the
             artifact-timeout evidence rather than `caused`, and the check re-run
   Evidence: at 32cdc93 Format & Metadata, Clippy & Doctests and all four Incremental shards
             passed; Coverage ran the full suite to 96.53 % total and failed only at
             actions/upload-artifact@v6 with "Failed to CreateArtifact: ... ETIMEDOUT"
   Rejected: leaving a sixth PR body in this run claiming a gate state no run produced
   Reverses: the "CI status" paragraph in Validation and the last Unresolved bullet

R3-5 F4: the allocation claim
   Taken:    narrowed to the payload builder it is true of. gamut_isobmff::writer copies the
             payload into the output buffer, so the encode as a whole still peaks near 2n
   Reverses: none needed

R3-6 the range-newtype deferral
   Taken:    moved out of the PR body into crates/gamut-avif/STATUS.md as recorded limit (5),
             since a residual recorded only in a body does not survive the merge. The body's
             bullet is annotated rather than deleted, so nothing is lost
   Rejected: editing issue #505 to name the newtype — this run never edits an existing issue,
             and STATUS.md notes that #505's text does not yet mention it
   Reverses: none needed

R3-7 AvifEncodeReport derived Default while documenting "Construct nothing here"
   Taken:    dropped the derive. Established first that nothing needs it: the only construction
             in the workspace is the struct literal in encode_with_report, and #[non_exhaustive]
             does not restrict that within the defining crate
   Reverses: none needed

Two forks the record did not cover were decided in this round and are appended in the same shape:

R3-8 whether the rename is a BREAKING CHANGE commit
   Taken:    no. Committed as fix(avif) / refactor(avif) without `!` or a BREAKING CHANGE
             footer, with the rename named in full in the commit body instead
   Evidence: the premise that a renamed public method is breaking does not hold here — the
             whole c2pa module is new on this branch. `git cat-file -e origin/master:
             crates/gamut-avif/src/c2pa.rs` fails, and so does the same probe against the
             released tag gamut-avif-v1.1.0. No published version ever exposed these names, so
             a BREAKING CHANGE footer would make release-plz major-bump gamut-avif 1.1.0 -> 2.0.0
             over an API no consumer can be holding. The branch's own feat commit already
             carries this surface, and 1.2.0 is the correct SemVer outcome
   Rejected: emitting a `!` that is accurate about the diff but false about the released API
   Reverses: the publishing instruction's assumption, not its intent — what moved is still
             named explicitly, in the commit body

R3-9 the one missed diff-mutant
   Taken:    removed the arithmetic instead of excluding the mutant. The payload path pre-sized
             its buffer with `framing_len(purpose) + slot.len()`, and a Vec capacity is a hint,
             so replacing + with * changes no output byte and no test could kill it.
             reserve_exact asks for the slot's room directly, leaving the single addition of
             the two lengths at the one caller that checks it
   Evidence: mutants-diff went from 1 missed to 0 missed; the cost is one reallocation copying
             the 21 bytes of framing
   Rejected: a mutants.toml exclusion for an equivalent mutant
   Reverses: none needed

Round 4 (closing) decisions, appended in the record's shape. No entry above is edited.

R4-1 F1 - the contract text promised a refusal set the crate does not have
   Taken:    fixed the text at every site that states it (the with_c2pa_reserved rustdoc, the
             MAX_PAYLOAD_LEN constant's doc, STATUS.md, README.md): there is a third refusal,
             it comes from gamut-isobmff's 32-bit top-level box size field, and it arrives as
             Error::Unsupported rather than the Error::InvalidInput the text promised
   Evidence: executed - len 4_294_967_251 -> "unsupported: ISOBMFF: top-level box at or beyond
             4 GiB [origin: gamut-isobmff]"; len 4_294_967_250 clears the box check and aborts
             in the allocator at 8589934654 bytes
   Rejected: moving MAX_PAYLOAD_LEN down to the effective limit in this round - that changes
             behaviour and which crate owns the error, and this review loop has converged.
             Filed as #576 with both sides of the argument
   Reverses: none needed

R4-2 F2 - the "strict in what it writes" claim is false
   Taken:    fixed the claim, not the code. with_c2pa's rustdoc now states that it trusts a
             caller-supplied slice and applies no minimum, matching how this codebase carries
             every other supplied metadata payload; the sentence "a file this crate would
             decline to produce is still one it will faithfully describe" is deleted from the
             locator docs, and the same claim is corrected in STATUS.md and README.md
   Evidence: executed - with_c2pa(&[0xAB; 3]) encodes Ok and c2pa_slot() reports range 77..80,
             while with_c2pa_reserved(3) is refused
   Rejected: applying MIN_SLOT_LEN to with_c2pa - a behavioural change that would make a length
             check the only content judgement this crate's C2PA surface makes. Filed as #577
   Reverses: none needed

R4-3 F3 - the minimum's value was pinned only against itself
   Taken:    added two literal-valued assertions to the existing refusal test - 7 refused with
             the message that names 8, and 8 accepted - so the constant cannot move without a
             failure, in either direction
   Evidence: at MIN_SLOT_LEN = 9 this is the only failing test of 45; at 7 it fails from the
             other side; at 8 all 45 pass. Before the round, 7 and 9 both left all 45 green
   Rejected: a separate test - the reach and the failure reason are identical to the existing
             one, so a second test would be redundant under docs/testing.md
   Reverses: none needed

R4-4 F6 - a commit message overstates reserve_exact's guarantee
   Taken:    corrected in this body rather than by amending 55bd77d6, which is pushed and which
             PR #535 has already merged. See the correction in the Validation section
   Reverses: none needed

R4-5 the range-newtype deferral
   Taken:    stands, and stays recorded as limit (5) in crates/gamut-avif/STATUS.md
   Reverses: none needed

R4-6 whether workspace-wide lint/test/coverage were warranted this round
   Taken:    no; ran the crate-scoped equivalents plus the four capped gates instead, and
             recorded which were skipped and why in the Validation section
   Evidence: the compiled diff is one crate's doc comments plus one inline assertion; CI's
             Clippy & Doctests and Coverage jobs run the workspace versions on the pushed head
   Reverses: none needed

Unresolved review notes

Round 4 (closing).

  • Two behavioural questions this round declined to decide, both filed rather than taken:
    gamut-avif: should the C2PA reservation ceiling move to the container writer's 4 GiB bound? #576 "gamut-avif: should the C2PA reservation ceiling move to the container writer's 4 GiB
    bound?" and gamut-avif: should AvifEncoder::with_c2pa apply the reserved slot's 8-byte minimum? #577 "gamut-avif: should AvifEncoder::with_c2pa apply the reservation
    minimum?". Each names the executed evidence and both sides of the argument.
  • The round-3 review converged. Its six new findings were all Low or informational, and none
    changed an emitted byte or a returned range; the four that were actionable are R4-1 to R4-4
    above. No finding is left open.
  • This is an unattended run. No human approved the plan or any decision in the record above;
    the record is what a human reads before deciding whether this stack merges. That judgement,
    including whether to merge at all, is the maintainer's.

A C2PA manifest store binds to the finished file, so the encoder reserves
rather than receives: AvifEncoder::with_c2pa_reserved(len) writes the
C2PA 2.4 A.5.1 ContentProvenanceBox - a top-level uuid box with the
D8FEC3D6-...-C481 user type, zero FullBox version/flags, box_purpose
manifest and a zero 8-byte merkle offset - after ftyp and before meta
(A.5.3, via IsoBmffImage::push_top_level_box) around a slot of len zero
bytes; with_c2pa(bytes) writes a store the caller computed over this
exact output; encode_with_report returns the same bytes encode_to_vec
would plus the slot's file range, so a signer knows where to patch
before it runs and nothing after the slot moves. The object-safe
EncodeImage entry point is unchanged.

On read, AvifContainer::c2pa / c2pa_manifest_stores locate every
top-level C2PA uuid box and report its box_purpose, slot bytes and file
range, in file order. The slot is box-bounded (store plus any A.5.3
padding; no JUMBF LBox trim, unlike gamut-heic - sharing one lens is
and the range is documented as observability, not a hash exclusion
range (BMFF binds by box path, 18.6 / A.5.6).

Tests: exact-byte - two equal-length stores give files differing in
exactly the reported span, and patching a reserved file at that range
reproduces the directly written file; the crate locates the slot it
reserved at the reported range; a mid-update original/update pair is
reported in file order from a hand-transcribed A.5.1.2 payload; libavif
and dav1d (through the crate's own parse) decode a file carrying the
box to the same pixels as one without. Inline: the A.5.1.2 payload
layout, the parser's user-type / version-flags / purpose / truncation
rules and its offset arithmetic.

Refs #444
Adds the ContentProvenanceBox row to section A and the locator row to
section L of STATUS.md, a scope paragraph for the reserve/write/report
surface, and a note recording the locator's three limits (box-bounded
slot rather than an LBox trim, the assumed update framing, and that the
range is not an exclusion range) with #505 as the shared-lens follow-up.
README gains a content-credentials paragraph.

Refs #444
The manifest_stores fixture's only non-uuid box had an empty body, so
the uuid type guard could be replaced with true and nothing in the suite
noticed (a MISSED mutant in the diff run). Give the free box a body that
is byte-for-byte a C2PA payload: C2PA 2.4 A.5.1.1 fixes the box type to
uuid, so that box must not be reported, and the guard is now observable.

Refs #444
C2paManifestStore was byte-identical in name to gamut-heic's type while
promising something different: heic trims to the store's own JUMBF LBox,
this crate reports the box-bounded slot, so for one file the two could
report different lengths under one name - and both are re-exported from
the gamut umbrella.

Rename the type to C2paSlot and its field to slot_bytes, and say in the
docs what the bound is: the store plus any A.5.3 padding, or all zeros
for an unfilled reservation. The bound itself is kept, because an LBox
trim cannot locate a reservation - an all-zero slot has no LBox, so the
reserve/report/patch flow would have nothing to patch. Unifying the two
lenses is #505. The encoder's private slot-source enum takes the name
SlotSource, freeing C2paSlot for the public type.

Refs #444
C2PA 2.4 A.5.3 states the 8-byte merkle offset for box_purpose manifest
and original, and says nothing about the bytes ahead of an update store.
Assuming the prefix unconditionally dropped an in-spec prefix-less
update store shorter than 8 bytes as absent, and made this crate
disagree with gamut-heic about identical bytes.

Probe the same [8, 0] candidate list gamut-heic's locator uses, first
match winning, for update only. A box-bounded slot has no JUMBF LBox to
discriminate on, so the fallback fires exactly when data is too short
for the prefix; a longer prefix-less update store is still reported 8
bytes short, which needs the LBox check #505 unifies. Documented on the
purpose type.

Refs #444
with_c2pa_reserved(len) built a vec![0; len] and copied it into the
payload buffer, so reserving n bytes peaked at 2n - reserving 64 MiB
touched 128 MiB. Split the payload builder into the A.5.1.2 framing plus
a slot, and give the reserved path a resize into the buffer the payload
already owns. The equivalence with handing the same zeros to the payload
builder is pinned at three lengths and all three purposes.

Refs #444
The Av1StillDecoder-over-dav1d bridge was a third copy in the crate.
tests/common/mod.rs already carries allow(dead_code) precisely so a
binary can use a subset of the fixtures, and tests/c2pa.rs already
declares mod common, so the dead-code objection to sharing it does not
apply. conformance.rs and high_bitdepth_roundtrip.rs keep their own -
moving those is churn beyond this change.

Refs #444
The read side reports all three A.5.3 purposes; the encoder writes one.
Once a file carries an update box, A.5.3 requires the earlier store to
be re-labelled original, so re-encoding such a file through these
builders would label it wrongly - producing that pair is a
manifest-update operation on an existing file, outside #444. State it on
both with_c2pa_reserved and with_c2pa, and record it as a fourth limit
in STATUS.md alongside the probe and rename this review settled.

Refs #444
`with_c2pa_reserved` is an infallible builder, so an unusable `len`
reached the payload builder unchecked and added to the framing length
without a guard. Two failures sat next to each other at the top of the
range. Nearest `usize::MAX` the addition wrapped, and because a wrapped
total is *smaller* than the framing already written, `resize` shrank the
21-byte framing to 20 instead of growing it: in debug that panicked with
"attempt to add with overflow", but in release — the profile every
downstream consumer builds with — `encode_to_vec` returned `Ok` with a
well-formed AVIF whose `uuid` box the crate's own locator then reported
as absent. Just below the wrap the sum was representable but larger than
a `Vec` can hold, so `with_capacity` panicked. A silent wrong answer
about the range a signer binds is the outcome this surface must never
produce.

Both are now refused at encode as `Error::InvalidInput`, on every entry
point including the object-safe `EncodeImage` path, together with a
reservation below the 8-byte JUMBF `LBox`/`TBox` header that could not
hold a manifest store at all. The read side is deliberately left
permissive: a degenerate slot that is genuinely present is still
reported with its true range. Strict in what it writes, honest about
what it reads, and the asymmetry is documented on both sides.

Also in this round, all interlocking with the above:

- The C2PA read accessors are renamed for what they report:
  `AvifContainer::c2pa` -> `c2pa_slot`, `c2pa_manifest_stores` ->
  `c2pa_slots`, and the private locator `manifest_stores` -> `slots`.
  `C2paSlot` was already renamed for its box bound, but the accessors
  kept the old vocabulary and stayed byte-identical in name to
  `gamut-heic`'s, whose bounds differ, with both re-exported from the
  umbrella. The doc sentence still opening "Every C2PA manifest store
  among the top-level boxes" is corrected to say slot. Nothing released
  carries these names: the whole `c2pa` module is new on this branch and
  absent from `gamut-avif-v1.1.0`, so this is not a breaking change to
  any published API and must not bump the major.
- `AvifEncodeReport` no longer derives `Default`, which had made a type
  documented "Construct nothing here" externally constructible. Nothing
  builds one that way; the encoder uses a struct literal.
- The allocation claim on `content_provenance_reserved` is narrowed to
  the payload builder it is true of: `gamut_isobmff::writer` copies the
  payload into the output, so the encode as a whole still peaks near 2n.
- STATUS.md records the deferral of a range newtype to #505, which until
  now lived only in the pull request body and would not have survived
  the merge.
The payload path pre-sized its buffer with `framing_len(purpose) +
slot.len()`, but a `Vec` capacity is a hint: replacing that `+` with `*`
changes no byte of the output, so the mutation gate reported it missed
and no test could have killed it. The arithmetic was also the only place
outside the checked reservation where a framing length and a slot length
were added.

`reserve_exact` asks for the slot's room directly, so the payload path
computes no total at all and the single addition of the two lengths is
the one `content_provenance_reserved` already checks. The buffer still
ends up exactly the size it needs, at the cost of one reallocation that
copies the 21 bytes of framing.
The documented refusal set for `with_c2pa_reserved` named two lengths, both
`Error::InvalidInput` from this crate, and promised that every `len` between
them "is written as asked", with only the allocator's limit beyond.

A third refusal sits inside that promised range and comes from another crate. A
top-level ISOBMFF box carries a 32-bit size field, so `gamut_isobmff::write`
rejects a `ContentProvenanceBox` at or beyond 4 GiB as `Error::Unsupported`.
Measured on the `manifest` framing, `len = 4_294_967_251` is refused that way
and `4_294_967_250` clears the check, then aborts in the allocator while the
writer copies the payload into the output buffer.

Say so at every site that states the contract, and name the error type it
arrives as. Moving this crate's own ceiling to that bound is a behavioural
change, filed as #576.

Refs #444
The locator's docs claimed the asymmetry between a strict writer and a
permissive reader made "a file this crate would decline to produce still one it
will faithfully describe". It does produce it: `with_c2pa(&[0xAB; 3])` encodes,
and `AvifContainer::c2pa_slot` finds a 3-byte slot at 77..80, while
`with_c2pa_reserved(3)` refuses.

The 8-byte minimum bounds the reservation path alone, because a reservation is a
bare integer with no other evidence behind it. A supplied slice is carried
verbatim, as every other caller-supplied metadata payload in this workspace is.
Document that where `with_c2pa` is defined, and delete the false sentence rather
than tightening the builder — applying the minimum there would change behaviour
and make this the only content judgement the C2PA surface makes, so it is filed
as #577 instead.

Refs #444
`a_reservation_below_a_jumbf_box_header_is_refused` sweeps `0..MIN_SLOT_LEN` and
then accepts `MIN_SLOT_LEN`, so it holds for whatever value the constant carries.
Setting it to 7 or 9 leaves all 45 inline tests green, and at 9 the crate would
ship a refusal message reading "at least 8 bytes" while refusing 8 — the test
asserts the message against a literal but the guard compares against the
constant. The mutation gate cannot see this, because it does not mutate
constants.

Add the two literal-valued assertions the sweep cannot make: 7 is refused with
the message that names 8, and 8 is accepted. At `MIN_SLOT_LEN = 9` this is now
the only failing test of the 45; at 7 it fails too, from the other side.

Refs #444
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