diff --git a/ErsatzTV.FFmpeg.Tests/Pipeline/QsvPipelineBuilderTests.cs b/ErsatzTV.FFmpeg.Tests/Pipeline/QsvPipelineBuilderTests.cs index d3af9ee99..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() @@ -122,6 +124,142 @@ 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); + + // 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] + 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"); + + // 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")] + [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,21 +271,40 @@ public class QsvPipelineBuilderTests bool preferNativeDecoder, Option maybeExtraHardwareFrames = default, ScanKind scanKind = ScanKind.Progressive, - bool deinterlace = false) + bool deinterlace = false, + bool hdr = false, + bool hasOpenClTonemap = true, + bool normalizeColors = true, + bool anamorphic = false, + bool imageSubtitle = false) { (VideoInputFile videoInputFile, AudioInputFile audioInputFile, FFmpegState ffmpegState, FrameState desiredState) = - BuildQsvH264Pipeline(preferNativeDecoder, scanKind, deinterlace); + 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( - new DefaultFFmpegCapabilities(), + hasOpenClTonemap + ? new DefaultFFmpegCapabilities(FFmpegKnownFilter.TonemapOpenCL.Name) + : new DefaultFFmpegCapabilities(), new DefaultHardwareCapabilities(), HardwareAccelerationMode.Qsv, videoInputFile, audioInputFile, None, - None, + subtitleInputFile, None, Option.None, "", @@ -156,26 +313,34 @@ 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); } private static (VideoInputFile, AudioInputFile, FFmpegState, FrameState) BuildQsvH264Pipeline( bool preferNativeDecoder, ScanKind scanKind, - bool deinterlace) + bool deinterlace, + 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 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), - "1:1", + hdr ? new PixelFormatYuv420P10Le() : new PixelFormatYuv420P(), + hdr ? new ColorParams("tv", "bt2020nc", "smpte2084", "bt2020") : ColorParams.Default, + hdr ? new FrameSize(3840, 1608) : new FrameSize(1920, 1080), + anamorphic ? "4:3" : "1:1", "16:9", FrameRate.DefaultFrameRate, false, @@ -212,7 +377,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 +437,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..108469303 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, currentState); + // _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 @@ -386,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"); @@ -579,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); @@ -640,14 +680,84 @@ 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, + 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()) + { + 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; + } + + // 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); + } + 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 +811,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 +866,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-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. 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..373052a28 --- /dev/null +++ b/docs/decisions/records/ffmpeg/qsv-hdr-tonemap-opencl.md @@ -0,0 +1,98 @@ +--- +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. + +- **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 +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.