Commit Graph
2 Commits
Author SHA1 Message Date
timothy ee66cb7459 fix(505): address cold-review findings — retag on tonemap, subtitle scale, anamorphic
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 20s
PR Gates / Docs update reminder (pull_request) Successful in 22s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 24s
PR Gates / decisions lifecycle (pull_request) Successful in 30s
Review verdict / Set review-verdict status (pull_request) Successful in 9s
PR Gates / Script tests (pytest) (pull_request) Successful in 42s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 1m29s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 17m53s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 21m3s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 22m48s
review-verdict/h10 Review-verdict: MERGEABLE @ ee66cb7
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Independent cold review (Codex, no implementation role) found no Blocker and
three real defects, all fixed here:

HIGH — HDR was re-tagged bt709 only when the profile had NormalizeColors on.
The colorspace filter sat behind desiredState.ColorsAreBt709, so an operator
with normalization disabled got tonemapped SDR pixels still tagged bt2020 and
the player converted them a second time. The guard is now
"tonemapped || (ColorsAreBt709 && ...)". Deliberately NOT fixed by hoisting
usesVppQsv out of the guard, which would force bt709 on scale-only non-HDR
chains that legitimately opted out.

MEDIUM — image subtitles stopped being resized. The subtitle canvas is scaled
only when the video chain contains a recognized scale filter, and that predicate
listed the QSV filters only; swapping ScaleQsvFilter for ScaleVaapiFilter left a
4K HDR + PGS source with a 720p video and a source-size subtitle overlay.
VaapiPipelineBuilder already listed ScaleVaapiFilter; QsvPipelineBuilder does now.

MEDIUM — anamorphic HDR now falls back to the software tonemap. ScaleQsvFilter
is handed the SAR VideoStream calculates (it has a 0:0 fallback); ScaleVaapiFilter
multiplies by ffmpeg's runtime `sar`, which differs when the decoded frame leaves
SAR unspecified. Rather than ship a graph nobody has run, exclude anamorphic --
which leaves those sources exactly where they were before this change.

LOW — tests now pin the exact validated graph as an ordered substring (the prior
assertions would have passed with setFormat off, hwdownload dropped, or the wrong
tonemap output format), assert against the vpp_qsv OPTION rather than a bare
"tonemap=1" substring, and cover NormalizeColors=false, anamorphic and image
subtitles. Each of the three fixes was negative-controlled: reverting it fails
exactly one test, and no others.

The remaining LOW (deriveDevice's defaulted bool is a future-call-site trap) is
recorded as an accepted residual rather than fixed, since a named factory would
push this diff into the VA-API pipeline for no behavior change.

The record is 86 prose lines, over the 60-line ceiling. Declining to cut: every
bullet is a distinct measured finding, which docs.corpus-size-signal names as a
legitimate decline.

Decisions-Edit: yes
2026-07-26 21:16:05 +02:00
timothy 41e2870113 fix(505): tonemap QSV HDR through OpenCL; vpp_qsv=tonemap is a silent no-op
#505 asked to route the #498 native-decode path through TonemapQsvFilter to
move HDR tonemapping off the CPU. Measured on the Intel host (jazz: FFmpeg
8.1.2 / iHD 25.1.4 / UHD 630) against real HDR HEVC Main10, that filter is a
SILENT no-op: a graph ending in vpp_qsv=tonemap=1 returns a frame byte-identical
(same md5) to the same graph with no tonemap at all, with no warning and no
error. QSV VPP tonemapping needs Gen11+; pre-Gen11 iHD ignores it.

So the issue's premise was inverted, and the branch it wanted to extend was
already broken: the existing DecoderHardwareAccelerationMode == Qsv path shipped
untonemapped HDR whenever QsvPreferNativeDecoder was off -- which is exactly the
escape hatch #498/#523 recommend. Prod was unaffected (native-decode is the
default and took the working software branch).

Tonemap on the GPU via OpenCL instead, the route VaapiPipelineBuilder already
uses and the one Jellyfin uses. The scale has to run first, in scale_vaapi:
tonemapping full-size is slower than the software path it replaces (15.5s wall
for 12.5s of content, below realtime), while scale-first cuts total CPU ~60%
(35.6s -> 14.1s) and lands at the no-tonemap wall-clock floor. A QSV surface
maps to neither OpenCL nor VA-API, so the gate requires software frames: the
QSV decoder and deinterlace_qsv both fall back to the software tonemap, slower
but correct. TonemapQsvFilter is deleted -- a filter that silently does nothing
is worse than no filter.

Also fixes output tagging: the first end-to-end run was correctly tonemapped yet
still announced bt2020 primaries, because SetPixelFormat's usesVppQsv predicate
("did a hardware filter strip color info") listed only the QSV filters. Both new
filters are now in it.

Validated end to end on jazz with the exact generated command: exit 0,
YAVG 26.39 (software reference 26.6, untonemapped 44.3), and ffprobe reports
bt709 space/transfer/primaries.

fixes #505
2026-07-26 20:55:05 +02:00