feat(render): auto-detect HDR from media probes, add --sdr flag#526
feat(render): auto-detect HDR from media probes, add --sdr flag#526vanceingalls merged 2 commits intomainfrom
Conversation
Replace the --hdr opt-in model with automatic detection. When no flags are passed, the renderer probes all video/image sources and enables HDR output if any HDR color space is detected. Existing --hdr flag becomes a force override. New --sdr flag forces SDR output. Behavior matrix: (no flags) + HDR content → HDR output (no flags) + SDR content → SDR output --hdr → force HDR (defaults to HLG if no HDR sources) --sdr → force SDR (skips probing) --hdr --sdr → error Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Looks good — auto-detect as the default is the right call, and the docker-arg test coverage carries the original --hdr flag-drop regression forward cleanly. Approving with a few suggestions inline.
One ask before merge: please run the three unchecked manual test items from the PR description. The existing hdr-regression / hdr-hlg-regression fixtures only exercise force-hdr (via the harness mapping), so the auto-detect happy path isn't covered by the suite.
| videoBitrate?: string; | ||
| /** Enable HDR color space probing on video/image sources. */ | ||
| hdr?: boolean; | ||
| /** HDR rendering mode. `auto` probes sources and enables HDR if any HDR content is found. */ |
There was a problem hiding this comment.
The JSDoc only describes auto semantics — worth a one-liner per mode so readers don't have to read the orchestrator to find out:
/** HDR rendering mode.
* - `auto` (default): probe sources; enable HDR if any HDR content is found.
* - `force-hdr`: enable HDR even on SDR-only compositions (falls back to HLG transfer).
* - `force-sdr`: skip probing entirely; always render SDR.
*/Also: the format doc comment at line ~216 still references hdr: true (H.264 (or H.265 + HDR10 when 'hdr: true')). Please update that to reference hdrMode: "force-hdr" / auto-detect while you're here.
There was a problem hiding this comment.
Updated. I expanded the hdrMode JSDoc to document all three modes (auto, force-hdr, force-sdr), and I also fixed the stale format comment that still referenced hdr: true so it now points to hdrMode/auto-detect semantics instead.
| @@ -1831,7 +1831,7 @@ export async function executeRenderJob( | |||
| // overhead on SDR-only compositions. | |||
There was a problem hiding this comment.
This comment is now stale — the --hdr flag no longer gates probing; auto mode also probes (which is what makes auto-detect work). Suggested rewrite:
Skipped only in
force-sdrmode to avoid ffprobe overhead when the user has explicitly opted out.
Same applies to the mirrored comment block above the image probe (Probe images for HDR color spaces…).
There was a problem hiding this comment.
Updated. Both probe comments were stale after the auto-detect change. They now say probing is skipped only in force-sdr mode, which matches the current behavior for both video and image probing.
| if (info?.hasHdr && info.dominantTransfer) { | ||
| effectiveHdr = { transfer: info.dominantTransfer }; | ||
| } else { | ||
| effectiveHdr = { transfer: "hlg" }; |
There was a problem hiding this comment.
Forcing HDR on an SDR-only composition produces an HLG container wrapping SDR-range pixels — that can look perceptually wrong on real HDR displays. Two small asks:
- Log this branch as
log.warn(...)— the user explicitly asked for HDR but got the fallback, and they probably want to know. - The info log below at L1962 just says
forced by --hdr flag, which doesn't distinguish 'forced over real HDR sources' from 'forced with no HDR sources'. Worth differentiating, e.g.HDR forced (no HDR sources detected — defaulting to HLG).
There was a problem hiding this comment.
Fixed. The SDR-only force-hdr fallback now emits a warning, and the logging distinguishes between “forced HDR with real HDR sources present” vs “forced HDR with no HDR sources detected, defaulting to HLG.”
| useGpu: false, | ||
| debug: false, | ||
| hdr: suite.meta.renderConfig.hdr ?? false, | ||
| hdrMode: suite.meta.renderConfig.hdr ? "force-hdr" : "auto", |
There was a problem hiding this comment.
Behavior change for existing fixtures worth flagging: any meta.json that omits hdr or sets hdr: false used to skip probing entirely, but now lands in auto and runs ffprobe across every video + image. SDR-only fixtures should still produce identical golden frames, but if any style-*-prod fixture happens to reference an HDR-tagged source asset, auto could flip output to HDR and break the baseline.
For strict parity with the previous semantics:
hdrMode: suite.meta.renderConfig.hdr ? "force-hdr" : "force-sdr",If you'd rather have fixtures opt into auto-detect, that's fine too — but worth a one-shot run of the full producer suite before merging to confirm nothing flipped.
There was a problem hiding this comment.
can probably ignore this
There was a problem hiding this comment.
I took the strict-parity route for the regression harness and mapped legacy fixture metadata back to forced modes: hdr=true -> force-hdr, otherwise force-sdr. That preserves the previous fixture semantics and avoids silently routing omitted/false hdr suites into auto-detect (and therefore ffprobe/HDR baseline drift).
2 participants
Summary
--hdrflag needed when HDR sources are present--hdrbecomes a force override (enables HDR even without HDR sources)--sdrflag forces SDR output regardless of source content--hdr --sdrerrors with a clear messageBehavior matrix
--hdr--sdr--hdr --sdrTest plan
tsc --noEmitpasses for CLI and producer--sdr— should force SDR--hdron SDR-only composition — should force HDR🤖 Generated with Claude Code