imazen / imazen/zenpipe

hdr-s4-tonemap (34f7ef84) still builds and passes, but needs 3 fixes before landing

Open
#80 0 comments 0 reactions 1 assignee View on GitHub

@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

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.