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

ErsatzTV.FFmpeg.Tests/Pipeline/QsvPipelineBuilderTests.cs

@@ -2,6 +2,7 @@ using System;
 using System.Collections.Generic;
 using System.Text.RegularExpressions;
 using ErsatzTV.FFmpeg.Capabilities;
+using ErsatzTV.FFmpeg.Filter;
 using ErsatzTV.FFmpeg.Format;
 using ErsatzTV.FFmpeg.OutputFormat;
 using ErsatzTV.FFmpeg.Pipeline;
@@ -20,6 +21,7 @@ namespace ErsatzTV.FFmpeg.Tests.Pipeline;
 public class QsvPipelineBuilderTests
 {
     private readonly ILogger _logger = Substitute.For<ILogger>();
+    private Option<SubtitleInputFile> _lastSubtitleInputFile;
 
     [Test]
     public void Qsv_PreferNativeDecoder_Should_Decode_Via_Vaapi_To_Software_Then_Qsv_Encode()
@@ -137,7 +139,11 @@ public class QsvPipelineBuilderTests
     {
         string command = BuildHdrAndPrint(preferNativeDecoder, deinterlace);
 
-        command.ShouldNotContain("tonemap=1");
+        // assert against the vpp_qsv OPTION, not the bare substring: "tonemap=1" alone could match
+        // an unrelated filter, and would miss an equivalent spelling
+        command.ShouldNotContain("vpp_qsv=tonemap");
+        Regex.IsMatch(command, @"vpp_qsv=[^,\s]*tonemap")
+            .ShouldBeFalse(command);
     }
 
     [Test]
@@ -169,6 +175,55 @@ public class QsvPipelineBuilderTests
         command.ShouldContain("all=bt709");
 
         command.ShouldContain("h264_qsv");
+
+        // pin the exact graph measured on the Intel host, in order — the assertions above would
+        // all still pass with setFormat off, hwdownload dropped, or the wrong tonemap output
+        // format, any of which breaks the validated command
+        command.ShouldContain(
+            "format=nv12|p010le|vaapi,hwupload=derive_device=vaapi," +
+            "scale_vaapi=1280:720:force_divisible_by=2:format=p010,setsar=1," +
+            "hwmap=derive_device=opencl,tonemap_opencl=tonemap=linear:format=nv12," +
+            "hwdownload,format=nv12");
+    }
+
+    [Test]
+    public void Qsv_Hdr_Should_Retag_Bt709_Even_When_Color_Normalization_Is_Disabled()
+    {
+        // a tonemap converts the PIXELS to SDR, so the stream must stop announcing bt2020 whether
+        // or not the profile asks for color normalization — otherwise the player converts twice
+        string command = BuildAndPrint(preferNativeDecoder: true, hdr: true, normalizeColors: false);
+
+        command.ShouldContain("tonemap_opencl");
+        command.ShouldContain("all=bt709");
+    }
+
+    [Test]
+    public void Qsv_Hdr_Anamorphic_Should_Fall_Back_To_Software_Tonemap()
+    {
+        // ScaleVaapiFilter multiplies by ffmpeg's runtime `sar` instead of the SAR VideoStream
+        // calculates, so anamorphic sources keep the software tonemap they already had
+        string command = BuildAndPrint(preferNativeDecoder: true, hdr: true, anamorphic: true);
+
+        command.ShouldContain("zscale");
+        command.ShouldNotContain("tonemap_opencl");
+    }
+
+    [Test]
+    public void Qsv_Hdr_With_Image_Subtitle_Should_Scale_The_Subtitle_To_Match_The_Video()
+    {
+        // the video is scaled by ScaleVaapiFilter on this path; if the subtitle-scaling predicate
+        // does not recognize it, the burned-in subtitle canvas stays at source resolution
+        string command = BuildAndPrint(preferNativeDecoder: true, hdr: true, imageSubtitle: true);
+
+        command.ShouldContain("scale_vaapi");
+
+        var subtitleSteps = new List<IPipelineFilterStep>();
+        foreach (SubtitleInputFile subtitle in _lastSubtitleInputFile)
+        {
+            subtitleSteps.AddRange(subtitle.FilterSteps);
+        }
+
+        subtitleSteps.ShouldContain(s => s is ScaleImageFilter, "subtitle canvas was never resized");
     }
 
     [TestCase(true, true, TestName = "Qsv_Hdr_Interlaced_Falls_Back_To_Software_Tonemap")]
@@ -218,13 +273,28 @@ public class QsvPipelineBuilderTests
         ScanKind scanKind = ScanKind.Progressive,
         bool deinterlace = false,
         bool hdr = false,
-        bool hasOpenClTonemap = true)
+        bool hasOpenClTonemap = true,
+        bool normalizeColors = true,
+        bool anamorphic = false,
+        bool imageSubtitle = false)
     {
         (VideoInputFile videoInputFile, AudioInputFile audioInputFile, FFmpegState ffmpegState, FrameState desiredState) =
-            BuildQsvH264Pipeline(preferNativeDecoder, scanKind, deinterlace, hdr);
+            BuildQsvH264Pipeline(preferNativeDecoder, scanKind, deinterlace, hdr, anamorphic);
 
         ffmpegState = ffmpegState with { MaybeQsvExtraHardwareFrames = maybeExtraHardwareFrames };
 
+        if (!normalizeColors)
+        {
+            desiredState = desiredState with { ColorsAreBt709 = false };
+        }
+
+        Option<SubtitleInputFile> subtitleInputFile = imageSubtitle
+            ? new SubtitleInputFile(
+                "/tmp/whatever.mkv",
+                new List<MediaStream> { new(2, "hdmv_pgs_subtitle", StreamKind.Subtitle) },
+                SubtitleMethod.Burn)
+            : Option<SubtitleInputFile>.None;
+
         var builder = new QsvPipelineBuilder(
             hasOpenClTonemap
                 ? new DefaultFFmpegCapabilities(FFmpegKnownFilter.TonemapOpenCL.Name)
@@ -234,7 +304,7 @@ public class QsvPipelineBuilderTests
             videoInputFile,
             audioInputFile,
             None,
-            None,
+            subtitleInputFile,
             None,
             Option<GraphicsEngineInput>.None,
             "",
@@ -243,6 +313,10 @@ public class QsvPipelineBuilderTests
 
         FFmpegPipeline result = builder.Build(ffmpegState, desiredState);
 
+        // the subtitle input's filter steps never reach CommandGenerator, so expose them for the
+        // subtitle-scaling assertion
+        _lastSubtitleInputFile = subtitleInputFile;
+
         return PrintCommand(videoInputFile, audioInputFile, None, None, None, result);
     }
 
@@ -250,7 +324,8 @@ public class QsvPipelineBuilderTests
         bool preferNativeDecoder,
         ScanKind scanKind,
         bool deinterlace,
-        bool hdr = false)
+        bool hdr = false,
+        bool anamorphic = false)
     {
         // the real trigger: HEVC Main10, BT.2020 primaries, smpte2084 (PQ) transfer — matching the
         // prod sources this was validated against on jazz
@@ -265,7 +340,7 @@ public class QsvPipelineBuilderTests
                     hdr ? new PixelFormatYuv420P10Le() : new PixelFormatYuv420P(),
                     hdr ? new ColorParams("tv", "bt2020nc", "smpte2084", "bt2020") : ColorParams.Default,
                     hdr ? new FrameSize(3840, 1608) : new FrameSize(1920, 1080),
-                    "1:1",
+                    anamorphic ? "4:3" : "1:1",
                     "16:9",
                     FrameRate.DefaultFrameRate,
                     false,

ErsatzTV.FFmpeg/Pipeline/QsvPipelineBuilder.cs

@@ -414,7 +414,15 @@ public class QsvPipelineBuilder : SoftwarePipelineBuilder
                 }
             }
 
-            if (desiredState.ColorsAreBt709 && (!videoStream.ColorParams.IsBt709 || usesVppQsv))
+            // A tonemap converted the PIXELS to SDR, so the stream must stop announcing HDR — that
+            // is a correctness requirement, not a normalization preference, and it holds even when
+            // the profile has NormalizeColors off. Without this an operator with NormalizeColors
+            // disabled gets tonemapped pixels still tagged bt2020 and the player converts them a
+            // second time. Deliberately NOT done by hoisting usesVppQsv out of the guard: a
+            // scale-only hardware chain on non-HDR content should still respect the preference.
+            bool tonemapped = videoInputFile.FilterSteps.Any(f => f is TonemapOpenClQsvFilter or TonemapFilter);
+
+            if (tonemapped || (desiredState.ColorsAreBt709 && (!videoStream.ColorParams.IsBt709 || usesVppQsv)))
             {
                 // _logger.LogDebug("Adding colorspace filter");
 
@@ -607,8 +615,12 @@ public class QsvPipelineBuilder : SoftwarePipelineBuilder
                         }
                     }
 
-                    // only scale if scaling or padding was used for main video stream
-                    if (videoInputFile.FilterSteps.Any(s => s is ScaleFilter or ScaleQsvFilter or PadFilter))
+                    // only scale if scaling or padding was used for main video stream.
+                    // ScaleVaapiFilter belongs here too: the HDR/OpenCL tonemap path (ersatztv#505)
+                    // scales the video with it, and leaving it out left the subtitle canvas at
+                    // source resolution while the video shrank. VaapiPipelineBuilder already lists it.
+                    if (videoInputFile.FilterSteps.Any(s =>
+                            s is ScaleFilter or ScaleQsvFilter or ScaleVaapiFilter or PadFilter))
                     {
                         var scaleFilter = new ScaleImageFilter(desiredState.PaddedSize);
                         subtitle.FilterSteps.Add(scaleFilter);
@@ -719,6 +731,16 @@ public class QsvPipelineBuilder : SoftwarePipelineBuilder
             return false;
         }
 
+        // ScaleQsvFilter is handed the SAR that VideoStream CALCULATES (it has a fallback for a
+        // missing or 0:0 SAR); ScaleVaapiFilter instead multiplies by ffmpeg's runtime `sar`, which
+        // is not the same value when the decoded frame leaves SAR unspecified. Rather than ship an
+        // anamorphic HDR graph nobody has run, keep anamorphic sources on the software tonemap —
+        // which is exactly what they got before this change, so it costs nothing they had.
+        if (videoStream.IsAnamorphic)
+        {
+            return false;
+        }
+
         return _ffmpegCapabilities.HasFilter(FFmpegKnownFilter.TonemapOpenCL);
     }
 

docs/decisions/records/ffmpeg/qsv-hdr-tonemap-opencl.md

@@ -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