From 41e287011322491fe7524d80b07685970f382281 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 20:55:05 +0200 Subject: [PATCH 1/4] 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 --- .../Pipeline/QsvPipelineBuilderTests.cs | 114 ++++++++++++-- .../Filter/Qsv/TonemapOpenClQsvFilter.cs | 30 ++++ .../Filter/Qsv/TonemapQsvFilter.cs | 12 -- .../Filter/Vaapi/HardwareUploadVaapiFilter.cs | 26 +++- .../Pipeline/QsvPipelineBuilder.cs | 144 +++++++++++++++--- docs/decisions/README.md | 1 + .../records/ffmpeg/qsv-hdr-tonemap-opencl.md | 68 +++++++++ 7 files changed, 346 insertions(+), 49 deletions(-) create mode 100644 ErsatzTV.FFmpeg/Filter/Qsv/TonemapOpenClQsvFilter.cs delete mode 100644 ErsatzTV.FFmpeg/Filter/Qsv/TonemapQsvFilter.cs create mode 100644 docs/decisions/records/ffmpeg/qsv-hdr-tonemap-opencl.md diff --git a/ErsatzTV.FFmpeg.Tests/Pipeline/QsvPipelineBuilderTests.cs b/ErsatzTV.FFmpeg.Tests/Pipeline/QsvPipelineBuilderTests.cs index d3af9ee99..2f50e67d6 100644 --- a/ErsatzTV.FFmpeg.Tests/Pipeline/QsvPipelineBuilderTests.cs +++ b/ErsatzTV.FFmpeg.Tests/Pipeline/QsvPipelineBuilderTests.cs @@ -122,6 +122,89 @@ public class QsvPipelineBuilderTests command.ShouldContain("hwupload=extra_hw_frames=128"); } + // ersatztv#505. Measured on the deployed FFmpeg 8.1.2 / iHD 25.1.4 / UHD 630: a graph ending in + // "vpp_qsv=tonemap=1" returns a frame that is BYTE-IDENTICAL (same md5) to the same graph with + // no tonemap step at all — QSV VPP tonemapping needs Gen11+, and pre-Gen11 iHD ignores it with + // no warning. So the assertion that matters is not "GPU tonemap is used" but "the silent no-op + // is never emitted", which is why every case below asserts its absence. + [TestCase(true, true)] + [TestCase(true, false)] + [TestCase(false, true)] + [TestCase(false, false)] + public void Qsv_Hdr_Should_Never_Emit_The_Silently_No_Op_Vpp_Qsv_Tonemap( + bool preferNativeDecoder, + bool deinterlace) + { + string command = BuildHdrAndPrint(preferNativeDecoder, deinterlace); + + command.ShouldNotContain("tonemap=1"); + } + + [Test] + public void Qsv_Hdr_NativeDecode_Should_Tonemap_On_The_Gpu_Via_OpenCL() + { + string command = BuildHdrAndPrint(preferNativeDecoder: true); + + // upload to VA-API explicitly: "-filter_hw_device hw" points at the QSV device, so a bare + // hwupload here would land on a QSV surface, which cannot be mapped to OpenCL + command.ShouldContain("hwupload=derive_device=vaapi"); + + // scale BEFORE tonemap, on the VA-API device (tonemapping full-size costs ~50% more wall + // clock than the software tonemap this replaces) + int scaleAt = command.IndexOf("scale_vaapi", StringComparison.Ordinal); + int tonemapAt = command.IndexOf("tonemap_opencl", StringComparison.Ordinal); + scaleAt.ShouldBeGreaterThan(-1, command); + tonemapAt.ShouldBeGreaterThan(-1, command); + scaleAt.ShouldBeLessThan(tonemapAt, command); + + // no vpp_qsv scale on this path — a QSV surface could not reach OpenCL afterwards + command.ShouldNotContain("vpp_qsv"); + + // and no CPU tonemap, which is the cost ersatztv#505 was filed about + command.ShouldNotContain("zscale"); + + // the hardware filters strip color info, so the output has to be re-tagged bt709 — without + // this the picture is tonemapped but still ANNOUNCES bt2020 primaries, and the player + // converts it a second time (verified against ffprobe on the Intel host) + command.ShouldContain("all=bt709"); + + command.ShouldContain("h264_qsv"); + } + + [TestCase(true, true, TestName = "Qsv_Hdr_Interlaced_Falls_Back_To_Software_Tonemap")] + [TestCase(false, false, TestName = "Qsv_Hdr_QsvDecode_Falls_Back_To_Software_Tonemap")] + public void Qsv_Hdr_Should_Fall_Back_To_Software_Tonemap_When_Frames_Cannot_Reach_OpenCL( + bool preferNativeDecoder, + bool deinterlace) + { + // both cases put frames on a QSV surface before the tonemap would run (deinterlace_qsv, or + // the QSV decoder itself), and a QSV surface maps to neither OpenCL nor VA-API. Slower on + // the CPU, but correct — unlike the vpp_qsv no-op this replaces. + string command = BuildHdrAndPrint(preferNativeDecoder, deinterlace); + + command.ShouldContain("zscale"); + command.ShouldNotContain("tonemap_opencl"); + } + + [Test] + public void Qsv_Hdr_Should_Fall_Back_To_Software_Tonemap_Without_The_OpenCL_Filter() + { + // an ffmpeg build with no tonemap_opencl must not silently skip tonemapping + string command = BuildAndPrint(preferNativeDecoder: true, hdr: true, hasOpenClTonemap: false); + + command.ShouldContain("zscale"); + command.ShouldNotContain("tonemap_opencl"); + command.ShouldNotContain("tonemap=1"); + } + + private string BuildHdrAndPrint(bool preferNativeDecoder, bool deinterlace = false) => + BuildAndPrint( + preferNativeDecoder, + maybeExtraHardwareFrames: default, + deinterlace ? ScanKind.Interlaced : ScanKind.Progressive, + deinterlace, + hdr: true); + private string BuildInterlacedAndPrint(Option maybeExtraHardwareFrames = default) => BuildAndPrint( preferNativeDecoder: true, @@ -133,15 +216,19 @@ public class QsvPipelineBuilderTests bool preferNativeDecoder, Option maybeExtraHardwareFrames = default, ScanKind scanKind = ScanKind.Progressive, - bool deinterlace = false) + bool deinterlace = false, + bool hdr = false, + bool hasOpenClTonemap = true) { (VideoInputFile videoInputFile, AudioInputFile audioInputFile, FFmpegState ffmpegState, FrameState desiredState) = - BuildQsvH264Pipeline(preferNativeDecoder, scanKind, deinterlace); + BuildQsvH264Pipeline(preferNativeDecoder, scanKind, deinterlace, hdr); ffmpegState = ffmpegState with { MaybeQsvExtraHardwareFrames = maybeExtraHardwareFrames }; var builder = new QsvPipelineBuilder( - new DefaultFFmpegCapabilities(), + hasOpenClTonemap + ? new DefaultFFmpegCapabilities(FFmpegKnownFilter.TonemapOpenCL.Name) + : new DefaultFFmpegCapabilities(), new DefaultHardwareCapabilities(), HardwareAccelerationMode.Qsv, videoInputFile, @@ -162,19 +249,22 @@ public class QsvPipelineBuilderTests private static (VideoInputFile, AudioInputFile, FFmpegState, FrameState) BuildQsvH264Pipeline( bool preferNativeDecoder, ScanKind scanKind, - bool deinterlace) + bool deinterlace, + bool hdr = false) { + // the real trigger: HEVC Main10, BT.2020 primaries, smpte2084 (PQ) transfer — matching the + // prod sources this was validated against on jazz var videoInputFile = new VideoInputFile( "/tmp/whatever.mkv", new List { new( 0, - VideoFormat.H264, + hdr ? VideoFormat.Hevc : VideoFormat.H264, VideoProfile.Main, - new PixelFormatYuv420P(), - ColorParams.Default, - new FrameSize(1920, 1080), + 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", "16:9", FrameRate.DefaultFrameRate, @@ -212,7 +302,9 @@ public class QsvPipelineBuilderTests 2000, 4000, 90_000, - false, + // HDR output is normalized to bt709, which is what makes the colorspace filter + // reachable at all; leaving this false would hide the output-tagging assertions + hdr, deinterlace); var ffmpegState = new FFmpegState( @@ -270,11 +362,11 @@ public class QsvPipelineBuilderTests return command; } - public class DefaultFFmpegCapabilities() : FFmpegCapabilities( + public class DefaultFFmpegCapabilities(params string[] filters) : FFmpegCapabilities( string.Empty, new System.Collections.Generic.HashSet(), new System.Collections.Generic.HashSet(), - new System.Collections.Generic.HashSet(), + new System.Collections.Generic.HashSet(filters), new System.Collections.Generic.HashSet(), new System.Collections.Generic.HashSet(), new System.Collections.Generic.HashSet()); diff --git a/ErsatzTV.FFmpeg/Filter/Qsv/TonemapOpenClQsvFilter.cs b/ErsatzTV.FFmpeg/Filter/Qsv/TonemapOpenClQsvFilter.cs new file mode 100644 index 000000000..a968db8ee --- /dev/null +++ b/ErsatzTV.FFmpeg/Filter/Qsv/TonemapOpenClQsvFilter.cs @@ -0,0 +1,30 @@ +using ErsatzTV.FFmpeg.Format; + +namespace ErsatzTV.FFmpeg.Filter.Qsv; + +// vpp_qsv=tonemap=1 is a SILENT no-op on pre-Gen11 Intel graphics (ersatztv#505): the frame comes +// back untouched, byte for byte, with no warning and no error, so HDR content ships untonemapped. +// The QSV pipeline therefore tonemaps through OpenCL, the same route VaapiPipelineBuilder takes. +// +// This filter always runs on VA-API frames and hands SOFTWARE frames back: QSV surfaces cannot be +// mapped to OpenCL ("Media sharing must be enabled on context creation") and cannot be mapped to +// VA-API either (hwmap returns -38, function not implemented), so the only route from a QSV-encode +// profile into tonemap_opencl is to stay on the VA-API device the QSV device was derived from. +public class TonemapOpenClQsvFilter(FFmpegState ffmpegState, IPixelFormat desiredPixelFormat) : BaseFilter +{ + public override string Filter => + $"hwmap=derive_device=opencl,tonemap_opencl=tonemap={ffmpegState.TonemapAlgorithm}:format={OutputFormat}," + + $"hwdownload,format={OutputFormat}"; + + private string OutputFormat => + desiredPixelFormat.BitDepth == 10 ? FFmpegFormat.P010LE : FFmpegFormat.NV12; + + public override FrameState NextState(FrameState currentState) => + currentState with + { + FrameDataLocation = FrameDataLocation.Software, + PixelFormat = desiredPixelFormat.BitDepth == 10 + ? new PixelFormatP010() + : new PixelFormatNv12(desiredPixelFormat.Name) + }; +} diff --git a/ErsatzTV.FFmpeg/Filter/Qsv/TonemapQsvFilter.cs b/ErsatzTV.FFmpeg/Filter/Qsv/TonemapQsvFilter.cs deleted file mode 100644 index 9fc05dca9..000000000 --- a/ErsatzTV.FFmpeg/Filter/Qsv/TonemapQsvFilter.cs +++ /dev/null @@ -1,12 +0,0 @@ -namespace ErsatzTV.FFmpeg.Filter.Qsv; - -public class TonemapQsvFilter : BaseFilter -{ - public override string Filter => "vpp_qsv=tonemap=1"; - - public override FrameState NextState(FrameState currentState) => - currentState with - { - FrameDataLocation = FrameDataLocation.Hardware - }; -} diff --git a/ErsatzTV.FFmpeg/Filter/Vaapi/HardwareUploadVaapiFilter.cs b/ErsatzTV.FFmpeg/Filter/Vaapi/HardwareUploadVaapiFilter.cs index f2dc03686..82c7a5897 100644 --- a/ErsatzTV.FFmpeg/Filter/Vaapi/HardwareUploadVaapiFilter.cs +++ b/ErsatzTV.FFmpeg/Filter/Vaapi/HardwareUploadVaapiFilter.cs @@ -1,16 +1,28 @@ -namespace ErsatzTV.FFmpeg.Filter.Vaapi; +namespace ErsatzTV.FFmpeg.Filter.Vaapi; public class HardwareUploadVaapiFilter : BaseFilter { + private readonly bool _deriveDevice; private readonly bool _setFormat; - public HardwareUploadVaapiFilter(bool setFormat) => _setFormat = setFormat; - - public override string Filter => _setFormat switch + // deriveDevice matters only where the graph's default filter device is NOT the VA-API one: the + // QSV pipeline sets "-filter_hw_device hw" (the QSV device), so a bare hwupload there would + // upload to QSV instead of VA-API. It defaults to false so the VA-API pipeline, whose default + // filter device already is VA-API, keeps emitting exactly what it emitted before. + public HardwareUploadVaapiFilter(bool setFormat, bool deriveDevice = false) { - false => "hwupload", - true => "format=nv12|p010le|vaapi,hwupload" - }; + _setFormat = setFormat; + _deriveDevice = deriveDevice; + } + + public override string Filter + { + get + { + string hwupload = _deriveDevice ? "hwupload=derive_device=vaapi" : "hwupload"; + return _setFormat ? $"format=nv12|p010le|vaapi,{hwupload}" : hwupload; + } + } public override FrameState NextState(FrameState currentState) => currentState with { FrameDataLocation = FrameDataLocation.Hardware }; diff --git a/ErsatzTV.FFmpeg/Pipeline/QsvPipelineBuilder.cs b/ErsatzTV.FFmpeg/Pipeline/QsvPipelineBuilder.cs index 1b295e798..05bc1a913 100644 --- a/ErsatzTV.FFmpeg/Pipeline/QsvPipelineBuilder.cs +++ b/ErsatzTV.FFmpeg/Pipeline/QsvPipelineBuilder.cs @@ -6,6 +6,7 @@ using ErsatzTV.FFmpeg.Encoder.Qsv; using ErsatzTV.FFmpeg.Environment; using ErsatzTV.FFmpeg.Filter; using ErsatzTV.FFmpeg.Filter.Qsv; +using ErsatzTV.FFmpeg.Filter.Vaapi; using ErsatzTV.FFmpeg.Format; using ErsatzTV.FFmpeg.GlobalOption.HardwareAcceleration; using ErsatzTV.FFmpeg.InputOption; @@ -18,6 +19,7 @@ namespace ErsatzTV.FFmpeg.Pipeline; public class QsvPipelineBuilder : SoftwarePipelineBuilder { + private readonly IFFmpegCapabilities _ffmpegCapabilities; private readonly IHardwareCapabilities _hardwareCapabilities; private readonly ILogger _logger; @@ -46,6 +48,7 @@ public class QsvPipelineBuilder : SoftwarePipelineBuilder fontsFolder, logger) { + _ffmpegCapabilities = ffmpegCapabilities; _hardwareCapabilities = hardwareCapabilities; _logger = logger; } @@ -215,12 +218,31 @@ public class QsvPipelineBuilder : SoftwarePipelineBuilder }; } + // HDR has to be tonemapped through OpenCL on the VA-API device (ersatztv#505); when that is + // the plan the downscale has to happen in scale_vaapi rather than vpp_qsv, because a QSV + // surface can be mapped neither to OpenCL nor back to VA-API. Decided once, up front, so + // the scale and tonemap steps cannot disagree about which device the frames are on. + bool useOpenClTonemap = UseOpenClTonemap(videoStream, context, ffmpegState); + // _logger.LogDebug("After decode: {PixelFormat}", currentState.PixelFormat); currentState = SetDeinterlace(videoInputFile, context, ffmpegState, currentState); // _logger.LogDebug("After deinterlace: {PixelFormat}", currentState.PixelFormat); - currentState = SetScale(videoInputFile, videoStream, context, ffmpegState, desiredState, currentState); + currentState = SetScale( + videoInputFile, + videoStream, + context, + ffmpegState, + desiredState, + currentState, + useOpenClTonemap); // _logger.LogDebug("After scale: {PixelFormat}", currentState.PixelFormat); - currentState = SetTonemap(videoInputFile, videoStream, ffmpegState, desiredState, currentState); + currentState = SetTonemap( + videoInputFile, + videoStream, + ffmpegState, + desiredState, + currentState, + useOpenClTonemap); currentState = SetPad(videoInputFile, videoStream, desiredState, currentState); // _logger.LogDebug("After pad: {PixelFormat}", currentState.PixelFormat); currentState = SetCrop(videoInputFile, desiredState, currentState); @@ -335,9 +357,15 @@ public class QsvPipelineBuilder : SoftwarePipelineBuilder IPixelFormat formatForDownload = pixelFormat; + // "did a hardware filter run", not "was it a QSV one": these all strip or rewrite the + // frame's color info, so the colorspace filter below has to re-assert it explicitly. + // The VA-API/OpenCL tonemap route (ersatztv#505) belongs here too — leaving it out + // shipped a correctly-tonemapped picture still TAGGED bt2020 primaries, which invites + // the player to convert it a second time. bool usesVppQsv = videoInputFile.FilterSteps.Any(f => - f is QsvFormatFilter or ScaleQsvFilter or DeinterlaceQsvFilter or TonemapQsvFilter); + f is QsvFormatFilter or ScaleQsvFilter or DeinterlaceQsvFilter + or ScaleVaapiFilter or TonemapOpenClQsvFilter); // if we have no filters, check whether we need to convert pixel format // since qsv doesn't seem to like doing that at the encoder @@ -640,14 +668,60 @@ public class QsvPipelineBuilder : SoftwarePipelineBuilder return currentState; } + // The QSV pipeline can only reach tonemap_opencl through the VA-API device that its own QSV + // device is derived from, so every condition here is about that device existing and being + // reachable with software frames in hand. + private bool UseOpenClTonemap(VideoStream videoStream, PipelineContext context, FFmpegState ffmpegState) + { + if (!videoStream.ColorParams.IsHdr) + { + return false; + } + + // ffmpeg has no vaapi on Windows, so there is no device to derive OpenCL from + if (OperatingSystem.IsWindows()) + { + return false; + } + + // with no configured device QsvHardwareAccelerationOption emits a bare "-init_hw_device + // qsv=hw" and never initializes a VA-API device at all + if (ffmpegState.VaapiDevice.Filter(d => !string.IsNullOrWhiteSpace(d)).IsNone) + { + return false; + } + + // frames from the QSV decoder are ALREADY on a QSV surface (-hwaccel_output_format qsv), + // and a QSV surface maps to neither OpenCL nor VA-API, so there is no route to the GPU + // tonemap from here. Software tonemap is slower but it is the only one that is correct. + if (ffmpegState.DecoderHardwareAccelerationMode == HardwareAccelerationMode.Qsv) + { + return false; + } + + // deinterlace_qsv runs before the scale and leaves frames on a QSV surface, same problem + if (context.ShouldDeinterlace) + { + return false; + } + + return _ffmpegCapabilities.HasFilter(FFmpegKnownFilter.TonemapOpenCL); + } + private static FrameState SetScale( VideoInputFile videoInputFile, VideoStream videoStream, PipelineContext context, FFmpegState ffmpegState, FrameState desiredState, - FrameState currentState) + FrameState currentState, + bool useOpenClTonemap) { + if (useOpenClTonemap) + { + return SetScaleVaapiForTonemap(videoInputFile, desiredState, currentState); + } + IPipelineFilterStep scaleStep; bool useSoftwareFilter = ffmpegState is @@ -701,6 +775,40 @@ public class QsvPipelineBuilder : SoftwarePipelineBuilder return currentState; } + // HDR frames arrive from the VA-API decoder in system memory (the QSV pipeline deliberately + // omits -hwaccel_output_format), so upload them to the VA-API device and scale THERE. Scaling + // first matters: tonemapping the full-size frame instead costs ~50% more wall clock than the + // software tonemap it replaces, which is the difference between above and below realtime. + private static FrameState SetScaleVaapiForTonemap( + VideoInputFile videoInputFile, + FrameState desiredState, + FrameState currentState) + { + // the decoder's yuv420p10le is not a VA-API surface format; p010/nv12 are + IPixelFormat uploadFormat = currentState.PixelFormat.Map(pf => pf.BitDepth).IfNone(10) == 10 + ? new PixelFormatP010() + : new PixelFormatNv12(currentState.PixelFormat.Map(pf => pf.Name).IfNone(FFmpegFormat.NV12)); + + var upload = new HardwareUploadVaapiFilter(setFormat: true, deriveDevice: true); + currentState = upload.NextState(currentState) with { PixelFormat = Some(uploadFormat) }; + videoInputFile.FilterSteps.Add(upload); + + var scaleStep = new ScaleVaapiFilter( + currentState, + desiredState.ScaledSize, + desiredState.PaddedSize, + desiredState.CroppedSize, + VideoStream.IsAnamorphicEdgeCase); + + if (!string.IsNullOrWhiteSpace(scaleStep.Filter)) + { + currentState = scaleStep.NextState(currentState); + videoInputFile.FilterSteps.Add(scaleStep); + } + + return currentState; + } + private static FrameState SetDeinterlace( VideoInputFile videoInputFile, PipelineContext context, @@ -722,26 +830,24 @@ public class QsvPipelineBuilder : SoftwarePipelineBuilder VideoStream videoStream, FFmpegState ffmpegState, FrameState desiredState, - FrameState currentState) + FrameState currentState, + bool useOpenClTonemap) { if (videoStream.ColorParams.IsHdr) { foreach (IPixelFormat pixelFormat in desiredState.PixelFormat) { - if (ffmpegState.DecoderHardwareAccelerationMode == HardwareAccelerationMode.Qsv) - { - var filter = new TonemapQsvFilter(); - currentState = filter.NextState(currentState); - videoStream.ResetColorParams(ColorParams.Default); - videoInputFile.FilterSteps.Add(filter); - } - else - { - var filter = new TonemapFilter(ffmpegState, currentState, pixelFormat); - currentState = filter.NextState(currentState); - videoStream.ResetColorParams(ColorParams.Default); - videoInputFile.FilterSteps.Add(filter); - } + // NOTE: vpp_qsv=tonemap=1 is deliberately NOT an option here. On pre-Gen11 Intel + // graphics it returns the frame untouched with no warning, so it does not tonemap, + // it only LOOKS like it did (ersatztv#505). Either OpenCL tonemaps on the GPU or + // the software filter does it on the CPU; there is no silently-wrong third branch. + IPipelineFilterStep filter = useOpenClTonemap + ? new TonemapOpenClQsvFilter(ffmpegState, pixelFormat) + : new TonemapFilter(ffmpegState, currentState, pixelFormat); + + currentState = filter.NextState(currentState); + videoStream.ResetColorParams(ColorParams.Default); + videoInputFile.FilterSteps.Add(filter); } } diff --git a/docs/decisions/README.md b/docs/decisions/README.md index e7cb319a6..bd8d1147a 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -74,6 +74,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `ffmpeg.hls-cold-start-burst` | HLS cold-start latency is fixed with a bounded `-readrate_initial_burst` (gated on FFmpeg ≥6.1 capability detection), not by raising `work_ahead_limit`, which would remove the concurrency guarantee it exists for. | 2026-07-20 | [link](records/ffmpeg/hls-cold-start-burst.md) | | `ffmpeg.qsv-decode-encode-split` | QSV decode is decoupled from QSV encode via a single `FFmpegProfile.QsvPreferNativeDecoder` bool (default ON, Linux-only), so a QSV encode profile can decode with the more tolerant native VA-API decoder instead of the QSV decoder, mirroring Jellyfin's hybrid decode/encode toggle instead of a general decode-family enum. | 2026-07-20 | [link](records/ffmpeg/qsv-decode-encode-split.md) | | `ffmpeg.qsv-extra-hw-frames-floor` | a QSV upload never emits `extra_hw_frames` below `FFmpegState.MinimumQsvExtraHardwareFrames` (64); a stored `0` or negative value is treated as "no pool configured" rather than honored literally, because with no headroom any unthrottled read exhausts the pool and the transcode writes nothing at all. | 2026-07-21 | [link](records/ffmpeg/qsv-extra-hw-frames-floor.md) | +| `ffmpeg.qsv-hdr-tonemap-opencl` | the QSV pipeline never emits `vpp_qsv=tonemap=1`, which is a SILENT no-op on pre-Gen11 Intel graphics; HDR is tonemapped on the GPU via `hwupload=derive_device=vaapi` → `scale_vaapi` → `hwmap=derive_device=opencl` → `tonemap_opencl` when a VA-API device exists, the frames are still in software, and `tonemap_opencl` is available, and by the software `TonemapFilter` otherwise. The scale runs BEFORE the tonemap, and any hardware filter on the path forces the output to be re-tagged bt709. | 2026-07-26 | [link](records/ffmpeg/qsv-hdr-tonemap-opencl.md) | | `ffmpeg.remote-image-fetcher-bounded` | remote graphics-engine images are fetched through `IRemoteImageFetcher` with a pooled `HttpClientFactory` client, a body-covering deadline, a wire-transfer size cap, and a decoder-enforced `DecoderOptions.MaxFrames` bound re-verified post-decode — never cached, re-fetched per element init. | 2026-07-20 | [link](records/ffmpeg/remote-image-fetcher-bounded.md) | | `ffmpeg.work-ahead-slot-atomic` | `workAheadSegmenterLimit` is enforced by a single compare-exchange claim on a shared `WorkAheadSlots` pool taken by the *caller* of `Transcode`, which then passes ownership in and gets the release in `Transcode`'s `finally` — never a `Volatile.Read` compare in one place and an `Interlocked.Increment` in another. | 2026-07-21 | [link](records/ffmpeg/work-ahead-slot-atomic.md) | | `ffmpeg.work-ahead-slot-release-never-negative` | `Release()` reads the count and compare-exchanges `current - 1` only when `current > 0`; a release against an empty pool records an unbalanced release and returns `false` **without ever writing a negative value**. It never decrements first and clamps afterward. The single caller (`HlsSessionWorker.Transcode`'s `finally`) logs a warning on the `false` return. | 2026-07-21 | [link](records/ffmpeg/work-ahead-slot-release-never-negative.md) | diff --git a/docs/decisions/records/ffmpeg/qsv-hdr-tonemap-opencl.md b/docs/decisions/records/ffmpeg/qsv-hdr-tonemap-opencl.md new file mode 100644 index 000000000..2e65a194a --- /dev/null +++ b/docs/decisions/records/ffmpeg/qsv-hdr-tonemap-opencl.md @@ -0,0 +1,68 @@ +--- +key: ffmpeg.qsv-hdr-tonemap-opencl +title: 2026-07-26 — the QSV pipeline tonemaps HDR through OpenCL, never vpp_qsv (#505) +status: active +since: '2026-07-26' +supersedes: none +superseded-by: none +rule: the QSV pipeline never emits `vpp_qsv=tonemap=1`, which is a SILENT no-op on pre-Gen11 Intel graphics; HDR is tonemapped on the GPU via `hwupload=derive_device=vaapi` → `scale_vaapi` → `hwmap=derive_device=opencl` → `tonemap_opencl` when a VA-API device exists, the frames are still in software, and `tonemap_opencl` is available, and by the software `TonemapFilter` otherwise. The scale runs BEFORE the tonemap, and any hardware filter on the path forces the output to be re-tagged bt709. +signals: 'QSV, HDR, tonemap, vpp_qsv, tonemap_opencl, hwmap, derive_device, smpte2084, bt2020, washed out HDR, silent no-op, Gen9.5/Gen11, iHD · paths: `QsvPipelineBuilder`, `TonemapOpenClQsvFilter`, `HardwareUploadVaapiFilter`, `ScaleVaapiFilter`, `TonemapFilter` · issues: #505, #498, #523, #529' +mechanics: '`QsvPipelineBuilder.UseOpenClTonemap`; `QsvPipelineBuilder.SetScaleVaapiForTonemap`; `TonemapOpenClQsvFilter`; `HardwareUploadVaapiFilter(setFormat, deriveDevice)`; the `usesVppQsv` predicate in `SetPixelFormat`' +--- + +- **`vpp_qsv=tonemap=1` does not tonemap — it returns the frame untouched, with no warning.** + Measured on the deployed FFmpeg 8.1.2 / iHD 25.1.4 / UHD 630 (i7-10700K, Gen9.5) against real + HDR HEVC Main10 (`bt2020nc`/`bt2020`/`smpte2084`): a graph ending in `vpp_qsv=tonemap=1` produced + a frame **byte-identical (same md5)** to the same graph with no tonemap step at all. True with the + explicit `hevc_qsv` decoder, on genuine QSV video-memory surfaces, with the filter before the + scale, after the scale, and with `format=nv12`. The output still carried `bt2020`/`smpte2084` + tags. QSV VPP tonemapping requires Gen11+; pre-Gen11 iHD ignores the option silently. Reference + luma for the same frame: software tonemap `YAVG=26.6`, no tonemap `YAVG=44.3`, `vpp_qsv` `44.3`. +- **So #505's premise was inverted, and the branch it wanted to extend was already broken.** The + issue asked to route the #498 native-decode path through `TonemapQsvFilter` to save CPU; doing so + would have shipped *untonemapped* HDR. Worse, the pre-existing `DecoderHardwareAccelerationMode + == Qsv` branch already did exactly that — so any operator who flipped `QsvPreferNativeDecoder` + off, which is precisely the escape hatch #498 and #523 recommend, got washed-out HDR. Prod was + not affected (native-decode is the default, and it took the working software branch). + `TonemapQsvFilter` is deleted rather than left in place: a filter that silently does nothing is + worse than no filter, because it looks like coverage. +- **OpenCL is the working GPU route, and it is genuinely faster — but only if the scale runs + first.** 300 frames, 3840x1608 HDR HEVC → 1280x720 `h264_qsv`, two reproducible rounds: + + | arm | user CPU | total CPU | wall (12.5s of content) | tonemapped | + |---|---|---|---|---| + | software `zscale`/`tonemap` (what #505 wanted to replace) | 33.2s | 35.6s | 10.25s | yes | + | `vpp_qsv=tonemap=1` | — | — | — | **no (no-op)** | + | OpenCL, tonemap at full size then scale | 13.4s | 21.3s | **15.5s** | yes | + | **OpenCL, `scale_vaapi` first then tonemap** | 9.9s | **14.1s** | **9.25s** | yes | + | no tonemap at all (floor) | 13.1s | 15.6s | 9.2s | no | + + Scale-first cuts total CPU ~60% versus the software tonemap and lands at the no-tonemap wall-clock + floor. Tonemapping at full size instead is *slower than the software path it replaces* (15.5s for + 12.5s of content — below realtime), which is why the ordering is a correctness-adjacent + requirement and not a micro-optimization. This is what forces the scale decision and the tonemap + decision to be made together, up front, in `UseOpenClTonemap`. +- **A QSV surface is a dead end: it maps to neither OpenCL nor VA-API.** `hwmap=derive_device=opencl` + from QSV fails ("Media sharing must be enabled on context creation"), and `hwmap=derive_device=vaapi` + from QSV fails with `-38` (function not implemented). So the OpenCL route is only reachable while + frames are still in **software**, which is why the gate excludes the QSV decoder + (`-hwaccel_output_format qsv`) and `ShouldDeinterlace` (`deinterlace_qsv` uploads first). Both fall + back to the software tonemap — slower, but correct, which is the whole point. The upload must also + say `derive_device=vaapi` explicitly: the QSV pipeline sets `-filter_hw_device hw` (the QSV + device), so a bare `hwupload` would land on a QSV surface and strand the frames. +- **Tonemapping the pixels is only half the job; the stream has to stop claiming it is HDR.** The + first end-to-end run on the Intel host was correctly tonemapped (`YAVG=26.39`, transfer `bt709`) + and *still* tagged `color_primaries=bt2020`/`color_space=bt2020nc`, inviting the player to convert + it a second time. Cause: `SetPixelFormat`'s `usesVppQsv` predicate — which is really "did a + hardware filter strip the color info" — listed only the QSV filters, and this path replaces them + with `ScaleVaapiFilter`/`TonemapOpenClQsvFilter`. Both are now in the predicate. The tell was + reachable only from `ffprobe` on the real output; exit code 0 and a correct luma average both + looked clean. + +**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 +is the route Jellyfin uses. The gate is therefore a *reachability* test (VA-API device present, +frames in software, `tonemap_opencl` compiled in), never a hardware-generation guess. If a Gen11+ +box is ever available to measure, compare the two there before adding a generation check — do not +add one on inference. -- 2.47.3 From 7c075ffa70c4276508f91f6374d829332d51ab6e Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 20:59:54 +0200 Subject: [PATCH 2/4] fix(505): gate the OpenCL tonemap on the ACTUAL frame location, not an enumeration UseOpenClTonemap excluded the two known ways frames reach a hardware surface before the tonemap (the QSV decoder, deinterlace_qsv). That is only correct for as long as the enumeration stays complete -- and the failure mode if it ever stops being complete is a second hwupload stacked on frames that are already on a surface. Check currentState.FrameDataLocation directly instead. The route begins with hwupload, so "frames are in software" is the actual precondition; the specific exclusions stay as documentation of the known cases, but the state check is what makes it safe. A future filter landing ahead of the tonemap now degrades to the software tonemap rather than emitting a broken graph. --- ErsatzTV.FFmpeg/Pipeline/QsvPipelineBuilder.cs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/ErsatzTV.FFmpeg/Pipeline/QsvPipelineBuilder.cs b/ErsatzTV.FFmpeg/Pipeline/QsvPipelineBuilder.cs index 05bc1a913..845eb7e90 100644 --- a/ErsatzTV.FFmpeg/Pipeline/QsvPipelineBuilder.cs +++ b/ErsatzTV.FFmpeg/Pipeline/QsvPipelineBuilder.cs @@ -222,7 +222,7 @@ public class QsvPipelineBuilder : SoftwarePipelineBuilder // the plan the downscale has to happen in scale_vaapi rather than vpp_qsv, because a QSV // surface can be mapped neither to OpenCL nor back to VA-API. Decided once, up front, so // the scale and tonemap steps cannot disagree about which device the frames are on. - bool useOpenClTonemap = UseOpenClTonemap(videoStream, context, ffmpegState); + bool useOpenClTonemap = UseOpenClTonemap(videoStream, context, ffmpegState, currentState); // _logger.LogDebug("After decode: {PixelFormat}", currentState.PixelFormat); currentState = SetDeinterlace(videoInputFile, context, ffmpegState, currentState); @@ -671,13 +671,27 @@ public class QsvPipelineBuilder : SoftwarePipelineBuilder // The QSV pipeline can only reach tonemap_opencl through the VA-API device that its own QSV // device is derived from, so every condition here is about that device existing and being // reachable with software frames in hand. - private bool UseOpenClTonemap(VideoStream videoStream, PipelineContext context, FFmpegState ffmpegState) + private bool UseOpenClTonemap( + VideoStream videoStream, + PipelineContext context, + FFmpegState ffmpegState, + FrameState currentState) { if (!videoStream.ColorParams.IsHdr) { return false; } + // The backstop for every case below, and for any future filter that lands ahead of the + // tonemap: the route starts with hwupload, so the frames have to actually be in software. + // Checked against the state rather than inferred from the enumeration, so a later change + // that puts frames on a surface earlier degrades to the software tonemap instead of + // emitting a second upload on top of an existing one. + if (currentState.FrameDataLocation == FrameDataLocation.Hardware) + { + return false; + } + // ffmpeg has no vaapi on Windows, so there is no device to derive OpenCL from if (OperatingSystem.IsWindows()) { -- 2.47.3 From 18c4f4e0b2318cab80bc157bcc923e3065a36f02 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 21:00:49 +0200 Subject: [PATCH 3/4] docs(505): correct the #498 record's escape-hatch claim, which was wrong the dangerous way The qsv-decode-encode-split record told HDR-on-QSV users to set QsvPreferNativeDecoder OFF "to keep GPU tonemap". Measured on the Intel host, vpp_qsv=tonemap=1 is a silent no-op on pre-Gen11 graphics, so following that advice did not preserve GPU tonemapping -- it disabled tonemapping entirely and shipped untonemapped HDR. Prose correction only; the decision itself (the QsvPreferNativeDecoder toggle) is unchanged and stays active. The software-tonemap half of the bullet was always accurate and stands. Decisions-Edit: yes --- .../records/ffmpeg/qsv-decode-encode-split.md | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/docs/decisions/records/ffmpeg/qsv-decode-encode-split.md b/docs/decisions/records/ffmpeg/qsv-decode-encode-split.md index 066b37137..a8d881fec 100644 --- a/docs/decisions/records/ffmpeg/qsv-decode-encode-split.md +++ b/docs/decisions/records/ffmpeg/qsv-decode-encode-split.md @@ -42,8 +42,15 @@ ErsatzTV. Full design: `docs/superpowers/specs/2026-07-20-qsv-native-decode-desi `DecoderHardwareAccelerationMode == Qsv`) falls to the software `zscale`/`tonemap` chain for HDR content. Output is correct but costs CPU on the realtime path. Accepted for now because software tonemap is correct-but-slower while an unvalidated GPU-tonemap graph could be worse (needs the Intel host to - verify), and the escape hatch covers it: HDR-on-QSV users who don't need the tolerant decoder set the - flag OFF to keep GPU tonemap. Optimizing the native path to `tonemap_qsv` is tracked in **#505**. + verify). Optimizing the native path is tracked in **#505**. + + **Corrected 2026-07-26 (#505):** the escape-hatch half of this bullet was wrong, and wrong in the + dangerous direction. It claimed HDR-on-QSV users could set the flag **OFF to keep GPU tonemap** — but + `vpp_qsv=tonemap=1` is a *silent no-op* on pre-Gen11 Intel graphics, so turning the flag off did not + preserve GPU tonemapping, it disabled tonemapping altogether and shipped untonemapped HDR. The + software-tonemap half stands (that path was always correct). Do not offer the flag as an HDR + work-around. See `ffmpeg.qsv-hdr-tonemap-opencl` for the measurements and the OpenCL route that + replaced it; `TonemapQsvFilter` no longer exists. - **Native decode is Linux-only.** Guarded with `!OperatingSystem.IsWindows()` in the QSV builder — FFmpeg has no `vaapi` hwaccel on Windows (and Windows QSV capabilities are over-reported), so on Windows a QSV profile keeps QSV decode regardless of the flag. -- 2.47.3 From ee66cb7459a41853d035e23e7de802b16b1e48c3 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 21:16:05 +0200 Subject: [PATCH 4/4] =?UTF-8?q?fix(505):=20address=20cold-review=20finding?= =?UTF-8?q?s=20=E2=80=94=20retag=20on=20tonemap,=20subtitle=20scale,=20ana?= =?UTF-8?q?morphic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../Pipeline/QsvPipelineBuilderTests.cs | 87 +++++++++++++++++-- .../Pipeline/QsvPipelineBuilder.cs | 28 +++++- .../records/ffmpeg/qsv-hdr-tonemap-opencl.md | 30 +++++++ 3 files changed, 136 insertions(+), 9 deletions(-) diff --git a/ErsatzTV.FFmpeg.Tests/Pipeline/QsvPipelineBuilderTests.cs b/ErsatzTV.FFmpeg.Tests/Pipeline/QsvPipelineBuilderTests.cs index 2f50e67d6..38bae13a0 100644 --- a/ErsatzTV.FFmpeg.Tests/Pipeline/QsvPipelineBuilderTests.cs +++ b/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(); + private Option _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(); + 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 = imageSubtitle + ? new SubtitleInputFile( + "/tmp/whatever.mkv", + new List { new(2, "hdmv_pgs_subtitle", StreamKind.Subtitle) }, + SubtitleMethod.Burn) + : Option.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.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, diff --git a/ErsatzTV.FFmpeg/Pipeline/QsvPipelineBuilder.cs b/ErsatzTV.FFmpeg/Pipeline/QsvPipelineBuilder.cs index 845eb7e90..108469303 100644 --- a/ErsatzTV.FFmpeg/Pipeline/QsvPipelineBuilder.cs +++ b/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); } diff --git a/docs/decisions/records/ffmpeg/qsv-hdr-tonemap-opencl.md b/docs/decisions/records/ffmpeg/qsv-hdr-tonemap-opencl.md index 2e65a194a..373052a28 100644 --- a/docs/decisions/records/ffmpeg/qsv-hdr-tonemap-opencl.md +++ b/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 ` 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 -- 2.47.3