Repository navigation
fix(producer): HDR sources are tone-mapped for formats that cannot carry HDR - #6597
Conversation
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · head de0b89ad
HDR clips are now tone-mapped, but in a mixed HDR+SDR timeline the extractor still lifts the SDR clips to BT.2020 for a render that comes out SDR. That's a second place deciding whether the output is HDR. Details inline.
Scope
Read all 3 files and the engine extractor's tone-map and SDR→HDR paths. Ran extractVideosStage.timelineBound.test.ts locally (17 pass), plus a probe extraction against real ffmpeg (see inline). CI was still running at review time. No prior reviews.
— Genesis
| outputDir: join(compiledDir, "__hyperframes_video_frames"), | ||
| format: job.config.videoFrameFormat ?? "auto", | ||
| toneMapHdrToSdr: job.config.hdrMode === "force-sdr", | ||
| toneMapHdrToSdr: rendersSdrOnly(job.config), |
There was a problem hiding this comment.
blocker: this flag reaches only the HDR clips. In extractAllVideoFrames, the SDR→HDR preflight (videoFrameExtractor.ts:2136-2179) checks only hdrInfo.hasHdr and ignores toneMapHdrToSdr. So in an auto + webm/mov/gif/hls render with one HDR clip and one SDR clip, the SDR clip still goes through SDR_TO_HDR_COLORSPACE_FILTER and skips sdrCanvasColourFilters (:1214). It ends up as PQ/HLG-encoded pixels in an SDR render, which is the washed-out symptom this PR fixes. Now the two clips also disagree within the same render.
I reproduced it against real ffmpeg: extractAllVideoFrames([sdr, hdr], …, { toneMapHdrToSdr: true }) gives hdrPreflightCount: 1, and the SDR clip's frame differs from the same clip extracted on its own. (--sdr has had the same gap all along, since it uses this path.)
Fix: gate the preflight with if (!options.toneMapHdrToSdr && hdrInfo.hasHdr && hdrInfo.dominantTransfer). Add a mixed-timeline case (auto + webm → hdrPreflightCount 0).
There was a problem hiding this comment.
Fixed in c22d33f: the SDR-to-HDR preflight is gated on !options.toneMapHdrToSdr, with a mixed SDR+HDR case in videoFrameExtractor.test.ts (hdrPreflightCount 0 when tone-mapping; 1 without the gate).
Edit accuracy: accurate 271 (base branch 271), smooth 135 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (2)
|
What
HDR video sources are now tone-mapped to SDR during frame extraction whenever the output format cannot carry HDR (anything but mp4), not only with
--sdr.Why
resolveEffectiveHdrModealready downgrades HDR to SDR for non-mp4 output (webm, mov, png-sequence, gif, hls). Frame extraction decided separately, and tone-mapped only whenhdrMode === "force-sdr". Soautoorforce-hdrinto a non-mp4 format kept the HDR-encoded (BT.2020 PQ/HLG) pixels in an SDR render, which comes out washed out.Related work
None known.
How
One predicate,
rendersSdrOnly(config)inhdrMode.ts(forced SDR or non-mp4 format), now owns "this output is SDR";extractVideosStageuses it fortoneMapHdrToSdr. The in-process and distributed-plan paths share that stage, so both are covered.Test plan
extractVideosStage.timelineBound.test.tsgains three cases (auto+mp4 false, auto+webm true, force-hdr+mov true), run for both the in-process and distributed paths. With the call site reverted tohdrMode === "force-sdr", four of them fail; with the fix, all 17 pass.--sdr.Typecheck, oxlint, oxfmt and the fallow audit were run on a Linux dev machine.
Size: under the 100-line floor because it is one self-contained fix with nothing else to fold it into.