hdr-s4-tonemap (34f7ef84) still builds and passes, but needs 3 fixes before landing
@lilith is already working on this.
Since Aug 29, 2026.
- Dominant language
- Rust
- Stars
- 2
- Forks
- 0
- PR merge metrics
- No merged PRs in 30d
Description
`hdr-s4-tonemap` (`34f7ef84`) has been sitting off `main` since 2026-06-14. It
adds `zencodecs/src/tonemap.rs` (+337 lines), a `tonemap` cargo feature pulling
`zentone 0.1.0`, and a hook at the transcode seam so an HDR (PQ/HLG) f32 source
targeting an SDR-only format (JPEG/WebP/GIF) is tone-mapped through zentone's
BT.2408 EETF at the 203-nit anchor instead of being handed to an SDR encoder
that would reinterpret the samples as sRGB.
Audited today. It is **worth landing, but not as-is** — three things first.
## It still builds, and its tests pass
Duplicated onto current `main` and evaluated (then abandoned; the branch is
untouched). Only three trivial textual conflicts — `lib.rs` module ordering
against `pub mod speed`, the `all` feature list against `heic-decode`, and
`Cargo.lock`. After resolving:
- `cargo check -p zencodecs --features tonemap` — clean.
- `cargo test -p zencodecs --features tonemap --lib tonemap` — **4 passed**
(`pq_midgray_tonemaps_to_plausible_sdr`, `measures_pq_content_peak_from_pixels`,
`hdr_capable_target_passes_through`, `sdr_source_passes_through`).
Two and a half months of `zencodec 0.1.26` taxonomy churn, `zenpixels 0.2.16`
and the ultrahdr 0.5/0.6 split did not break it.
## The MaxCLL is a whole-frame maximum — *not* a per-strip bug
Worth stating explicitly since it is the obvious thing to suspect.
`measure_pq_content_peak` iterates `for y in 0..h` over the entire
`PixelBuffer` and reduces once with a single `ContentLightLevel::measure` over
the whole linearized buffer; `tonemap_to_srgb8` computes `content_max` **once**
before the row loop and drives every row from the same `Bt2408Tonemapper`. The
transcode seam is whole-frame anyway (`decode_full_frame`), so there are no
strips there. No correctness bug on that axis.
## Blocker 1 — it fails `-D warnings`
```
error: use of deprecated associated function `zencodec::ContentLightLevel::measure`:
use zenpixels_convert::hdr::measure::CllMeasure::measure_max instead
--> zencodecs/src/tonemap.rs:116:34
error: could not compile `zencodecs` (lib) due to 1 previous error
```
`cargo clippy -p zencodecs --features tonemap -- -D warnings` fails. (Note the
feature is in `all` but in no CI feature set, so CI would not currently catch
this — which is its own gap: a landed `tonemap` should be in one of the
`zencodecs` extra runs.)
## Blocker 2 — it allocates a whole extra f32 frame to compute one scalar
`measure_pq_content_peak` builds a full `PixelBuffer::new(w, h, RGBF32_LINEAR)`,
PQ-decodes every row into it, reduces it to a single MaxCLL, and throws it away.
That is 12 bytes/px on top of the 16 bytes/px source — for a 24 MP HDR RGBA-f32
image, ~288 MB of transient peak on top of ~384 MB, at a seam whose whole job is
transcoding large images.
**This is the exact defect `zenpixels#69` already fixed upstream**, and the fix
is proven there. From `zenpixels-convert/src/ext.rs` (`convert_to_sdr`):
> The linearized image is only needed to find the brightest pixel, so it is
> never materialized: each row is decoded into one reused F32 scratch row and
> measured immediately (imazen/zenpixels#69 — this used to allocate a full F32
> copy, 4× an RGBA8 source, and throw it away). MaxCLL is a max, so the per-row
> maximum of `measure_max` composes exactly with the whole-image reading
> (`nits_to_u16` is monotone).
So the per-row rewrite **preserves the whole-frame maximum exactly** — it is not
a retreat to per-strip peaks. It also clears blocker 1, since
`CllMeasure::measure_max` is the non-deprecated entry point. The cost is a new
`zenpixels-convert/hdr-experimental` feature edge on a trait whose own docs say
it "may move or rename ahead of 0.3.0" — that is the call to make.
## Blocker 3 (design, needs a human) — measure-first contradicts its own doc
The module doc says:
> Content peak comes from the source `MaxCLL`; absent that, the HDR10 baseline
> of 1000 nits is assumed.
The code does the opposite: it prefers the *measured* peak and falls back to the
forwarded `MaxCLL` only if measurement fails. That makes the EETF
content-dependent — two frames of the same scene tone-map differently, and a dim
HDR image whose brightest pixel is ~250 nits gets its top mapped to SDR white,
which is auto-exposure rather than tone mapping. BT.2408's EETF takes a
*mastering display* peak, and CTA-861.3 MaxCLL is a measured property of the
mastering pass, not of one decoded frame.
The commit's own test was loosened to accommodate this — it asserts only
`v > 8` with the comment *"Measure-first reads the content peak as the 203-nit
diffuse white, so it maps bright; no tight upper bound."*
This may well be deliberate (the title says "measured content peak" and the
docs cite maintainer confirmation of the operator + anchor). Flagging it rather
than deciding it: either the doc should be corrected to describe measure-first,
or the preference order should be flipped back — and either way the test wants a
real expected value rather than a floor.
## Recommendation
Land it after: (1) per-row `CllMeasure::measure_max` replacing the full-frame
temp — clears blockers 1 and 2 together and keeps the whole-frame maximum;
(2) `tonemap` added to a `zencodecs` CI feature set so it is actually built;
(3) a decision on blocker 3 plus a tightened test. Until then the branch stays
where it is — it was **not** pushed to `main`, and `hdr-s4-tonemap@origin` is
unchanged.
---
*By Claude (Anthropic) on the repo owner's instruction, 2026-08-28.*
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Assessment
This issue has not been assessed yet.