fix(505): address cold-review findings — retag on tonemap, subtitle scale, anamorphic

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
parent 18c4f4e0b2
commit ee66cb7459
3 changed files with 136 additions and 9 deletions
@@ -59,6 +59,36 @@ mechanics: '`QsvPipelineBuilder.UseOpenClTonemap`; `QsvPipelineBuilder.SetScaleV
  reachable only from `ffprobe` on the real output; exit code 0 and a correct luma average both
  looked clean.

+- **Re-tagging follows the TONEMAP, not the normalization preference.** The colorspace filter was
+  originally reached only when `desiredState.ColorsAreBt709` (the profile's `NormalizeColors`) was
+  on, so a profile with normalization *off* got tonemapped pixels still tagged bt2020 — the same
+  double-conversion bug as above, just for a different operator setting. The guard is now
+  `tonemapped || (ColorsAreBt709 && …)`. Deliberately *not* fixed by hoisting `usesVppQsv` out of
+  the guard: a scale-only hardware chain on non-HDR content should still respect the preference.
+  Converting the pixels to SDR is what obliges the stream to stop announcing HDR; nothing else does.
+- **A new scale filter has to be declared to every consumer that asks "was the video scaled".**
+  Swapping `ScaleQsvFilter` for `ScaleVaapiFilter` silently broke image-subtitle burn-in: the
+  subtitle canvas is resized only when the video chain contains a recognized scale filter, and that
+  predicate listed the QSV ones only. A 4K HDR source with PGS subtitles scaled the video to 720p
+  and left the subtitle at source size. `ScaleVaapiFilter` is now in the predicate (as it already
+  was in `VaapiPipelineBuilder`). Generalizable: replacing a filter means grepping for every
+  `is <OldFilter>` type test, not just its construction site.
+- **Anamorphic sources stay on the software tonemap.** `ScaleQsvFilter` is handed the SAR that
+  `VideoStream` *calculates* (with a fallback for a missing or `0:0` SAR); `ScaleVaapiFilter`
+  instead multiplies by ffmpeg's runtime `sar`, which is a different value when the decoded frame
+  leaves SAR unspecified. Rather than ship an anamorphic HDR graph nobody has run, `IsAnamorphic`
+  is excluded from the gate — which leaves those sources exactly where they were before this
+  change, so it costs them nothing. Revisit only with a real anamorphic HDR sample on the Intel
+  host, asserting dimensions/SAR/DAR rather than just exit status.
+
+**Accepted residual:** `HardwareUploadVaapiFilter`'s `deriveDevice` is an optional bool defaulting
+to `false`, which is behavior-preserving for all four existing VA-API call sites but is a trap for a
+future QSV one: copying the familiar `new HardwareUploadVaapiFilter(true)` yields a bare `hwupload`,
+which lands on the QSV device (`-filter_hw_device hw`) and makes the OpenCL mapping unreachable. A
+named factory or an explicit device-target enum would be safer; it was not done here because it
+would push this diff into the VA-API pipeline for no behavior change. The XML comment on the
+parameter is the mitigation.
+
 **Accepted residual:** on Gen11+ hardware, where `vpp_qsv=tonemap=1` presumably does work, we now
 use OpenCL instead. That is deliberate — we have no capability probe that can tell the two apart
 (FFmpeg reports no error either way, which is the entire problem), and OpenCL is validated here and