andodeki
ad3fe19b90
Some checks failed
repo hygiene / hygiene (push) Has been cancelled
Ten PDF feature commits landed without an ADR, covering three whole phases.
Every feature gets one; these are the four that were owed. Written against
the code as it stands and re-verified by running it, not transcribed from
the commit messages.
0020 CCITT, JBIG2 and JPEG 2000 — the codecs ADR 0015 refused by name
0021 Phase 4 completion — stamping, reconciliation, CFF, cmap, and the
audit that corrected a false "complete" in ADR 0019
0022 Editing — content_edit, page_ops, flatten, catalog_edit
0023 Redaction and compaction, and the three reader defects they found
Verified rather than assumed, on the tree at f75c1cc:
ccitt 30, jbig2 29, jpx 44 unit tests; jpx_roundtrip's 7 compare against
OpenJPEG-generated pixels, exactly, because a JPEG 2000 bug produces a
plausible image rather than an error.
Redaction re-checked with a test written from outside the module. The
middle line is the one worth keeping:
after redact, secret present = true <- earlier revision
after compact, secret present = false
public text retained = true
Redaction alone leaves the secret in the bytes. That is why the two
shipped together, and why the report carries
earlier_revisions_retain_content.
Determinism re-checked by generating the same document from four separate
processes: byte-identical. The HashMap key-order defect really is fixed.
This ADR contains a correction to its own first draft. 0022 initially
listed text-run rewriting and annotation sub-types as deferred, on the
strength of a grep for "rewrite" finding nothing. Both are implemented —
the rewriter is ContentEditor::set_text, which preserves the operator kind
so a ' keeps its line advance and a TJ keeps its kerning; and "callout" and
"comment" are dart-pdf's names for a FreeText with a /CL and a Text
annotation, not distinct /Subtype values. Checking the plan's claim against
the code was right; concluding from a failed grep was not.
One criterion across the four is left unticked, and it is real: the ui.rs
interaction tests, blocked on the Makepad headless backend since Phase 1.
0021 also records a process note. ADR 0019's merge criteria were derived
from what had been built, so all of them ticked while three unbuilt items
stayed invisible. Criteria should come from the plan text first, then be
marked done or explicitly deferred — a deferral stated is fine, a deferral
unstated is a false claim.
pdf: 1187 passed. No code changed.
137 lines
6.2 KiB
Markdown
137 lines
6.2 KiB
Markdown
# ADR 0021: finishing Phase 4 — stamping, reconciliation, CFF, and an audit that corrected a false claim
|
|
|
|
- **Status:** Accepted
|
|
- **Date:** 2026-08-17
|
|
- **Review item:** `NIGIG_PDF_FEATURE_PARITY_PLAN.md` §1 Phase 4, the items
|
|
ADR 0019 did not implement
|
|
- **Supersedes:** nothing
|
|
- **Related:** ADR 0019 (document creation — this closes what it left and
|
|
corrects what it overclaimed), ADR 0017 (declared-versus-delivered)
|
|
- **Written retrospectively.** The work landed in `8701f5d`, `89ca518` and
|
|
`1220f89` without an ADR.
|
|
|
|
## Context
|
|
|
|
ADR 0019 declared Phase 4 complete. **It was not, and the ADR said so over
|
|
three unimplemented items.** A later audit (`89ca518`) checked the tree
|
|
rather than the commit message and found:
|
|
|
|
- **Field-value reconciliation** — setting a value wrote `/V`, marked the
|
|
field dirty and regenerated `/AP`, which is right for a field *we*
|
|
changed. The missing case is a file that arrives with `/V` and `/AP`
|
|
already disagreeing. Nothing decided that.
|
|
- **Type1/CFF embedding** — `sfnt.rs` detected CFF outlines and `font.rs`
|
|
read an existing `/FontFile3`; nothing wrote one. Creation was
|
|
TrueType-only. The plan hedges this with "if feasible", so deferring was
|
|
legitimate — claiming it was done was not.
|
|
- **`repair-cmap`** — no equivalent existed.
|
|
|
|
Also missing from ADR 0019: image embedding and header/footer stamping,
|
|
both named in the Phase 4 spec.
|
|
|
|
This is worth recording plainly because it is the same failure this
|
|
project keeps finding in its own code, one level up. ADR 0019's merge
|
|
criteria were all ticked; the box that was not on the list was the one that
|
|
mattered. A checklist derived from what you built cannot tell you what you
|
|
did not build — only the spec can, item by item.
|
|
|
|
## Decision
|
|
|
|
### Image embedding and stamping (`stamp.rs`, 835 lines, 27 tests)
|
|
|
|
`embed_image` takes a JPEG or raw RGB/grey source and produces an image
|
|
XObject; `jpeg_dimensions` sniffs geometry from the JPEG header rather than
|
|
trusting a caller. `attach_image_to_page` wires it into page resources.
|
|
`Banner`, `header_content`, `footer_content` and `image_stamp_content`
|
|
generate the content streams, with `Align` for placement and
|
|
`height_for_width`/`width_for_height` so a caller can scale without
|
|
distorting.
|
|
|
|
`combine_content` concatenates content fragments. Stamping is deliberately
|
|
*additive content*, not a page rewrite: a header that corrupted the page
|
|
beneath it would be a poor trade for convenience.
|
|
|
|
### Reconciliation (`reconcile.rs`, 825 lines, 20 tests)
|
|
|
|
The important decision here is a refusal. A field's value lives in `/V` and
|
|
its rendered appearance in `/AP`, and nothing in the format keeps them in
|
|
step. PDF 32000-1 §12.7.3.3 settles exactly **one** case —
|
|
`/NeedAppearances true` means `/V` wins — and is silent on the other, where
|
|
a conforming viewer renders `/AP` and never consults `/V`.
|
|
|
|
So the module **does not pick a winner**. It classifies the disagreement and
|
|
resolves it against a caller-declared `Intent`, because the correct answer
|
|
genuinely differs by caller: a viewer must show `/AP` to match other
|
|
viewers, an extractor must read `/V`, an editor must regenerate so the saved
|
|
file agrees with itself.
|
|
|
|
Choosing silently would be ADR 0017's failure in a new place — every answer
|
|
plausible, none checkable, and the caller unaware a decision was made on its
|
|
behalf.
|
|
|
|
Two cases are not judgement calls and are handled outright: a missing or
|
|
dangling appearance renders **blank**, and blank is never what a producer
|
|
meant, so even `Display` regenerates; and an unselected radio member showing
|
|
`/Off` while the group's `/V` names another member is correct, not a
|
|
disagreement.
|
|
|
|
### CFF and cmap
|
|
|
|
CFF embedding and `repair-cmap` landed in `create.rs` and `subset.rs`, with
|
|
a checked-in `cff_sample.otf` fixture and its generator.
|
|
|
|
### External verification became a CI gate
|
|
|
|
The audit did the thing ADR 0019 could not: ran generated output through
|
|
implementations sharing no code with ours — `qpdf --check`, `pdfinfo`,
|
|
`pdftotext`, `qpdf --list-attachments` — and all passed.
|
|
`tools/check-pdf-external-readers.sh` makes it repeatable and `pdf.yml` runs
|
|
it. It treats a qpdf **warning** as failure, not just an error, because qpdf
|
|
warns where it had to *reconstruct* — and needing reconstruction is exactly
|
|
the defect a generator must not ship.
|
|
|
|
That closes the half of the Phase 4 exit criterion that ADR 0019 recorded as
|
|
unverifiable. It is a better answer than the one I gave.
|
|
|
|
## Verification
|
|
|
|
Re-run against the tree at `f75c1cc`:
|
|
|
|
```
|
|
reconcile:: 20 passed
|
|
stamp.rs 27 #[test] (within pdf-graphics: 289 passed)
|
|
tools/check-pdf-external-readers.sh present, executable, wired into pdf.yml
|
|
```
|
|
|
|
## Merge criteria
|
|
|
|
- [x] Image embedding for JPEG and raw samples
|
|
- [x] JPEG geometry read from the codestream, not from the caller
|
|
- [x] Header, footer and image stamping with alignment and margins
|
|
- [x] Stamping is additive; page content beneath is unmodified
|
|
- [x] Reconciliation classifies `/V` vs `/AP` disagreement
|
|
- [x] Resolution is driven by a caller-declared intent, not chosen silently
|
|
- [x] `/NeedAppearances` honoured where the spec settles the case
|
|
- [x] A blank or dangling appearance is always regenerated
|
|
- [x] An unselected radio member is not treated as a disagreement
|
|
- [x] CFF embedding, with a checked-in fixture and generator
|
|
- [x] `repair-cmap` equivalent
|
|
- [x] External readers verified and gated in CI
|
|
- [x] The plan's status line corrected to match the tree
|
|
|
|
## Consequences
|
|
|
|
**Positive.** Phase 4's exit criterion is now enforced by CI against three
|
|
external tools rather than asserted. Reconciliation gives the viewer, the
|
|
extractor and the editor each the answer their job requires.
|
|
|
|
**Negative.** `Intent` is a concept a caller must understand to use the API
|
|
correctly. The alternative — choosing for them — is worse, but this is a
|
|
real cost and the documentation carries it.
|
|
|
|
**Process consequence, and the reason this ADR exists.** ADR 0019's merge
|
|
criteria were self-derived: they listed what had been built, so everything
|
|
was ticked and three unbuilt items were invisible. Future phase ADRs should
|
|
enumerate criteria **from the plan text first**, then mark each done or
|
|
explicitly deferred — a deferral stated is fine, a deferral unstated is a
|
|
false claim. ADR 0020 and 0022 follow that shape.
|