From 8d54acb1efa434fc0d6a8e319be4f08b9afb34b7 Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Sat, 1 Aug 2026 23:29:49 +1000 Subject: [PATCH] fix(deps): fix fmtconv's non-x86 integer scaler, pin fmtconv to r31 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Build-side half of the QTGMC preset-brightening fix. The app-side pointer bump, tests and docs follow in a separate change, once deps-v1.7.0 is published — this one only affects how the deps bundles are built. fmtconv's Scaler::process_plane_int_cpp applied the same sign-conversion constants as process_plane_int_sse2/avx2. The vector kernels need them because they accumulate in signed 16-bit lanes and their proxy undoes the offset: ProxyRwSse2/Avx2 ::S16 XORs bit 15 on read and write, keyed on SB == 16 and DB == 16. The C++ kernel has no counterpart — it accumulates in a plain int and ProxyRwCpp is unsigned at both ends, read() returning the stored value and write_clip() clipping to [0, 2^DB-1]. So the constant only biased the sum and write_clip clipped the negative result to 0. The bug fires whenever that kernel runs with SB < 16, on either axis: source axis kernel unpatched 8-bit horizontal SB=16 DB=16 correct 8-bit vertical SB=8 DB=16 black 10-bit either SB=10 DB=16 black 12-bit either SB=12 DB=16 black 16-bit either SB=16 DB=16 correct 8-bit horizontal is the single case that escapes, because that route converts to 16-bit before the scaler and so lands on SB = DB = 16. On x86 none of it is reachable: the SSE2/AVX2 scalers replace the function in the Scaler constructor, making it dead code, which is why only macos-arm64 and linux-arm64 were affected. Downstream this destroyed havsfunc's Bob(), which bobs fields with fmtc.resample(scalev=2, ...). QTGMC's Placebo and Very Slow presets — the only ones defaulting NoiseProcess=2, whose noise pass calls Bob() — came out about +10/255 too bright, and Draft (EdiMode='bob') came out nearly black. Upstream r31 does not fix it: building r31 reproduces the failure, as do yuygfgg's prebuilt arm64 binary and a local -O0 build. Scripts/patches/fmtconv-r31-arm-int-scaler.patch is applied by download-deps-{macos,linux}.sh right after the clone, and hard-fails the build if it ever stops applying rather than silently shipping the bug back. Its header carries the full analysis and doubles as the write-up if it is ever sent upstream (not submitted — that needs a GitLab account). Windows needs nothing, since its prebuilt DLL takes the x86 SIMD path. Also adds havsfunc patch 5, making Bob() resample at 16-bit. Redundant now, and measured to produce the same output as the fmtconv fix, but it also covers the prebuilt Windows DLL and any build where the fmtconv patch is dropped. Applied on every platform so all platforms keep generating output from an identical havsfunc. Also pins fmtconv to r31 everywhere. Upstream moved to GitLab in Aug 2023 and the GitHub repo is now an abandoned mirror whose final commit is literally "Repository moved to Gitlab", so macOS and Linux were cloning an unpinned stale post-r30 snapshot while Windows took the r30 release zip. All three move together deliberately: r31 changed interlaced PAL-DV chroma placement (U/V vertical positions swapped, vertical subsampling > 2 unhandled), so a version skew would change chroma per platform. --- Scripts/download-deps-linux.sh | 51 +++++++- Scripts/download-deps-macos.sh | 51 +++++++- Scripts/download-deps-windows.ps1 | 33 ++++- .../patches/fmtconv-r31-arm-int-scaler.patch | 115 ++++++++++++++++++ 4 files changed, 244 insertions(+), 6 deletions(-) create mode 100644 Scripts/patches/fmtconv-r31-arm-int-scaler.patch diff --git a/Scripts/download-deps-linux.sh b/Scripts/download-deps-linux.sh index c90a08c..37e73ed 100755 --- a/Scripts/download-deps-linux.sh +++ b/Scripts/download-deps-linux.sh @@ -586,11 +586,31 @@ build_plugin "eedi3m" \ "sed -i '1i #include ' EEDI3/EEDI3.cpp && $PLUGIN_BUILD_ENV meson setup build --buildtype=release && ninja -C build" # fmtconv (format conversion) +# Upstream moved to GitLab in Aug 2023; the GitHub repo is an abandoned mirror +# whose final commit is literally "Repository moved to Gitlab", so cloning its +# master pinned us to a stale post-r30 snapshot. Use GitLab and pin the tag so +# the build is reproducible and every platform ships the same fmtconv version +# (r31 also fixes interlaced PAL-DV chroma placement — U/V vertical positions +# were swapped and vertical subsampling > 2 was unhandled). +# NOTE: r31 does NOT fix the aarch64 integer-scaler bug (vertical resampling of +# sub-16-bit sources returns black). We fix that ourselves with +# patches/fmtconv-r31-arm-int-scaler.patch, applied right after the clone below. +FMTCONV_TAG="r31" echo "" -echo "=== Building fmtconv ===" +echo "=== Building fmtconv ($FMTCONV_TAG) ===" if [ "$FORCE" = true ] || [ ! -f "$PLUGINS_DIR/libfmtconv.so" ]; then rm -rf fmtconv - git clone --depth 1 https://github.com/EleonoreMizo/fmtconv.git fmtconv + git clone --depth 1 --branch "$FMTCONV_TAG" https://gitlab.com/EleonoreMizo/fmtconv.git fmtconv + # Fix the non-x86 integer vertical scaler, which returns a black plane for + # any source below 16-bit — that is what broke havsfunc's Bob(), and with it + # QTGMC's Placebo/Very Slow noise pass and the Draft preset. The patch header + # has the full analysis. Hard-fail rather than silently shipping the bug back. + (cd fmtconv && git apply "$SCRIPT_DIR/patches/fmtconv-r31-arm-int-scaler.patch") || { + echo " ERROR: patches/fmtconv-r31-arm-int-scaler.patch did not apply." >&2 + echo " fmtconv $FMTCONV_TAG may have changed upstream — re-check the patch." >&2 + exit 1 + } + echo " Patched fmtconv integer scaler (non-x86 vertical resample)" cd fmtconv/build/unix if ./autogen.sh && \ PKG_CONFIG_PATH="$PLUGIN_PKG_CONFIG" \ @@ -899,6 +919,33 @@ if old_qtgmc in content: if patched_eedi3cl: patches.append('EEDI3CL fallback') +# Patch 5: Bob() bit depth — fmtconv's generic (non-x86-SIMD) VERTICAL resampler +# returns black for any input below 16-bit. havsfunc's Bob() bobs fields with +# fmtc.resample(scalev=2, ...), so on aarch64 it destroys the image outright. +# That silently broke two QTGMC paths: +# - Placebo / Very Slow are the only presets that default NoiseProcess=2, and +# that noise pass calls Bob() to expand fields before extracting noise. The +# near-black "denoised" clip made MakeDiff clip hard, so GrainRestore/ +# NoiseRestore merged in a large positive bias: ~+10/255 brighter output. +# - Draft sets EdiMode='bob', which interpolates via Bob() — near-black output. +# Horizontal resampling and >=16-bit input are fine, which is why Slower and +# below (whose only fmtconv use is the same-size Sbb gauss blur) look correct. +# x86 is unaffected: its SSE2/AVX2 scalers override the broken generic path, so +# this only ever hit linux-arm64 and macos-arm64. Still broken in upstream r31. +# Promote to 16-bit before the resample — Bob()'s existing tail already dithers +# back to the source depth, and fmtconv keeps doing its own interlaced chroma +# placement (which zimg would not). +old_bob = " clip = core.std.SeparateFields(clip, tff).fmtc.resample(scalev=2, kernel='bicubic', a1=b, a2=c, interlaced=1, interlacedd=0)\n" +if old_bob in content: + content = content.replace( + old_bob, + " _bob_fields = core.std.SeparateFields(clip, tff)\n" + " if bits < 16:\n" + " _bob_fields = core.fmtc.bitdepth(_bob_fields, bits=16)\n" + " clip = _bob_fields.fmtc.resample(scalev=2, kernel='bicubic', a1=b, a2=c, interlaced=1, interlacedd=0)\n" + ) + patches.append('Bob 16-bit resample') + if patches: with open(havsfunc_path, 'w') as f: f.write(content) diff --git a/Scripts/download-deps-macos.sh b/Scripts/download-deps-macos.sh index a2929f5..d149650 100755 --- a/Scripts/download-deps-macos.sh +++ b/Scripts/download-deps-macos.sh @@ -1010,11 +1010,31 @@ build_plugin "eedi3m" \ "meson setup build --buildtype=release && ninja -C build" # fmtconv (format conversion) +# Upstream moved to GitLab in Aug 2023; the GitHub repo is an abandoned mirror +# whose final commit is literally "Repository moved to Gitlab", so cloning its +# master pinned us to a stale post-r30 snapshot. Use GitLab and pin the tag so +# the build is reproducible and every platform ships the same fmtconv version +# (r31 also fixes interlaced PAL-DV chroma placement — U/V vertical positions +# were swapped and vertical subsampling > 2 was unhandled). +# NOTE: r31 does NOT fix the aarch64 integer-scaler bug (vertical resampling of +# sub-16-bit sources returns black). We fix that ourselves with +# patches/fmtconv-r31-arm-int-scaler.patch, applied right after the clone below. +FMTCONV_TAG="r31" echo "" -echo "=== Building fmtconv ===" +echo "=== Building fmtconv ($FMTCONV_TAG) ===" if [ "$FORCE" = true ] || [ ! -f "$PLUGINS_DIR/libfmtconv.dylib" ]; then rm -rf fmtconv - git clone --depth 1 https://github.com/EleonoreMizo/fmtconv.git fmtconv + git clone --depth 1 --branch "$FMTCONV_TAG" https://gitlab.com/EleonoreMizo/fmtconv.git fmtconv + # Fix the non-x86 integer vertical scaler, which returns a black plane for + # any source below 16-bit — that is what broke havsfunc's Bob(), and with it + # QTGMC's Placebo/Very Slow noise pass and the Draft preset. The patch header + # has the full analysis. Hard-fail rather than silently shipping the bug back. + (cd fmtconv && git apply "$SCRIPT_DIR/patches/fmtconv-r31-arm-int-scaler.patch") || { + echo " ERROR: patches/fmtconv-r31-arm-int-scaler.patch did not apply." >&2 + echo " fmtconv $FMTCONV_TAG may have changed upstream — re-check the patch." >&2 + exit 1 + } + echo " Patched fmtconv integer scaler (non-x86 vertical resample)" cd fmtconv/build/unix ./autogen.sh ./configure @@ -1393,6 +1413,33 @@ if old_qtgmc in content: if patched_eedi3cl: patches.append('EEDI3CL fallback') +# Patch 5: Bob() bit depth — fmtconv's generic (non-x86-SIMD) VERTICAL resampler +# returns black for any input below 16-bit. havsfunc's Bob() bobs fields with +# fmtc.resample(scalev=2, ...), so on aarch64 it destroys the image outright. +# That silently broke two QTGMC paths: +# - Placebo / Very Slow are the only presets that default NoiseProcess=2, and +# that noise pass calls Bob() to expand fields before extracting noise. The +# near-black "denoised" clip made MakeDiff clip hard, so GrainRestore/ +# NoiseRestore merged in a large positive bias: ~+10/255 brighter output. +# - Draft sets EdiMode='bob', which interpolates via Bob() — near-black output. +# Horizontal resampling and >=16-bit input are fine, which is why Slower and +# below (whose only fmtconv use is the same-size Sbb gauss blur) look correct. +# x86 is unaffected: its SSE2/AVX2 scalers override the broken generic path, so +# this only ever hit macos-arm64 and linux-arm64. Still broken in upstream r31. +# Promote to 16-bit before the resample — Bob()'s existing tail already dithers +# back to the source depth, and fmtconv keeps doing its own interlaced chroma +# placement (which zimg would not). +old_bob = " clip = core.std.SeparateFields(clip, tff).fmtc.resample(scalev=2, kernel='bicubic', a1=b, a2=c, interlaced=1, interlacedd=0)\n" +if old_bob in content: + content = content.replace( + old_bob, + " _bob_fields = core.std.SeparateFields(clip, tff)\n" + " if bits < 16:\n" + " _bob_fields = core.fmtc.bitdepth(_bob_fields, bits=16)\n" + " clip = _bob_fields.fmtc.resample(scalev=2, kernel='bicubic', a1=b, a2=c, interlaced=1, interlacedd=0)\n" + ) + patches.append('Bob 16-bit resample') + if patches: with open(havsfunc_path, 'w') as f: f.write(content) diff --git a/Scripts/download-deps-windows.ps1 b/Scripts/download-deps-windows.ps1 index bcfe3e1..d1de7cd 100644 --- a/Scripts/download-deps-windows.ps1 +++ b/Scripts/download-deps-windows.ps1 @@ -316,13 +316,21 @@ $Plugins7z = @( } ) -# Plugins with zip format. NOTE: fmtconv-r30.zip ships BOTH win32/fmtconv.dll and +# Plugins with zip format. NOTE: fmtconv-r31.zip ships BOTH win32/fmtconv.dll and # win64/fmtconv.dll under arch subfolders, so the copy loop below must pick win64 # (copying both would let win32 clobber the 64-bit DLL depending on file order). +# +# fmtconv upstream moved from GitHub to GitLab in Aug 2023 and no longer publishes +# GitHub release assets, so r31 comes from the author's site - that is the asset the +# official GitLab release for r31 links to. Keep this version in step with +# FMTCONV_TAG in download-deps-{macos,linux}.sh: r31 changed interlaced PAL-DV +# chroma placement, so a version skew between platforms would make the same job +# produce different chroma on Windows than on macOS/Linux. If this URL ever dies, +# deps-expected-plugins.json makes it a red build rather than a silent gap. $PluginsZip = @( @{ Name = "fmtconv" - Url = "https://github.com/EleonoreMizo/fmtconv/releases/download/r30/fmtconv-r30.zip" + Url = "https://ldesoras.fr/src/vs/fmtconv-r31.zip" Check = "fmtconv.dll" }, @{ @@ -670,6 +678,27 @@ def _fix_mv_args(args): $PatchesApplied += "EEDI3CL fallback" } + # Patch 5: Bob() bit depth - fmtconv's generic (non-x86-SIMD) VERTICAL + # resampler returns black for any input below 16-bit, and havsfunc's Bob() + # bobs fields with fmtc.resample(scalev=2, ...). Windows x64 is NOT affected + # (its SSE2/AVX2 scalers override the broken generic path) - this is applied + # here purely so every platform generates identical output from an identical + # havsfunc. On arm64 it is a real fix: it repaired QTGMC's Placebo/Very Slow + # presets (the only ones defaulting NoiseProcess=2, whose noise pass calls + # Bob) and the Draft preset (EdiMode='bob'). Promoting to 16-bit is a no-op + # for quality here because fmtc.resample already outputs 16-bit from 8-bit + # input, and Bob()'s existing tail dithers back to the source depth. + $OldBob = " clip = core.std.SeparateFields(clip, tff).fmtc.resample(scalev=2, kernel='bicubic', a1=b, a2=c, interlaced=1, interlacedd=0)`n" + if ($Content.Contains($OldBob)) { + Write-Host " Applying Bob 16-bit resample patch..." -ForegroundColor Gray + $NewBob = " _bob_fields = core.std.SeparateFields(clip, tff)`n" + + " if bits < 16:`n" + + " _bob_fields = core.fmtc.bitdepth(_bob_fields, bits=16)`n" + + " clip = _bob_fields.fmtc.resample(scalev=2, kernel='bicubic', a1=b, a2=c, interlaced=1, interlacedd=0)`n" + $Content = $Content.Replace($OldBob, $NewBob) + $PatchesApplied += "Bob 16-bit resample" + } + if ($PatchesApplied.Count -gt 0) { Set-Content $HavsfuncPath $Content -NoNewline Write-Host " havsfunc patched ($($PatchesApplied -join ', '))" -ForegroundColor Green diff --git a/Scripts/patches/fmtconv-r31-arm-int-scaler.patch b/Scripts/patches/fmtconv-r31-arm-int-scaler.patch new file mode 100644 index 0000000..5a11a52 --- /dev/null +++ b/Scripts/patches/fmtconv-r31-arm-int-scaler.patch @@ -0,0 +1,115 @@ +fmtconv r31: fix the non-x86 integer scaler returning a black plane +================================================================== + +Applies to: fmtconv r31 (gitlab.com/EleonoreMizo/fmtconv), src/fmtcl/Scaler.cpp +Affects: every build without the x86 SIMD path - in practice macos-arm64 and + linux-arm64. x86 is unaffected because its SSE2/AVX2 scalers replace + the patched function outright in the Scaler constructor, making it + dead code there. +Upstream: not reported yet (needs a GitLab account); this header doubles as the + write-up if it is ever submitted. + +Symptom +------- +core.fmtc.resample() returns an all-zero (black) plane whenever the integer +kernel runs with a source bitdepth below 16. Measured on macos-arm64, scaling a +flat plane by two: + + source axis kernel result + 8-bit horizontal SB=16 DB=16 correct + 8-bit vertical SB=8 DB=16 black + 10-bit either SB=10 DB=16 black + 12-bit either SB=12 DB=16 black + 16-bit either SB=16 DB=16 correct + +An 8-bit source scaled only horizontally is the one combination that escapes: it +reaches the kernel already converted to 16-bit, landing on SB = DB = 16. So this +is NOT a vertical-only bug - 10- and 12-bit sources break on both axes. Same-size +resampling and float sources are never affected. + +In VapourBox this broke havsfunc's Bob(), which bobs fields with +fmtc.resample(scalev=2, ...). QTGMC's Placebo and Very Slow presets - the only +ones that default NoiseProcess=2, whose noise pass calls Bob() - came out about ++10/255 too bright, because the near-black "denoised" clip made MakeDiff clip +hard and GrainRestore/NoiseRestore then merged a large positive bias back in. +The Draft preset (EdiMode='bob') came out nearly black. + +Cause +----- +Scaler::process_plane_int_cpp applies the same sign-conversion constants as its +SSE2/AVX2 siblings: + + s_in = (SB < 16) ? -(0x8000 << (SHIFT_INT + SB - DB)) : 0; + s_out = (DB < 16) ? 0x8000 << (SHIFT_INT + SB - DB) : 0; + +Those exist because the vector kernels accumulate in signed 16-bit lanes: they +shift whichever end is not 16-bit into the signed domain, and the vector proxy +undoes it via ProxyRwSse2::S16, which XORs +bit 15 on read/write with SIGN_FLAG = (SB == 16) / (DB == 16). + +The C++ path has no such counterpart. It accumulates in a plain int and its +proxies are unsigned at both ends - ProxyRwCpp::read() +returns the raw stored value, and write_clip() clamps to [0, 2^DB-1]. So +s_in biased the accumulator by -0x8000 << shift with nothing to undo it, and +write_clip clamped every resulting negative value to 0. + +Worked example, 8-bit source to 16-bit destination (SHIFT_INT = 12, so shift 4), +flat input 120, kernel summing to 4096: + + sum = 120*4096 + s_in + r_cst = 491520 - 524288 + 8 = -32760 + -32760 >> 4 = -2048 + write_clip<16>(-2048) -> limit(-2048, 0, 65535) = 0 <-- black + +SPAN_I instantiates SB = 16, 14, 12, 10, 9, 8 with DB always 16, so only +SB = DB = 16 was correct - there s_in and s_out are already 0. + +Fix +--- +Drop the sign constants from the C++ kernel, matching its unsigned proxies. +write_clip's existing [0, 2^DB-1] clamp is already correct for that domain, +so nothing else changes. With the same example the result becomes +491528 >> 4 = 30720, i.e. 120 * 2^8 - identical to what the horizontal path +produces for the same conversion. + +Verified on macos-arm64 by transposing the input, resampling along the other axis +and transposing back. After the patch that reference is bit-identical to the +direct result for luma in every format tested (8/10/12/16-bit, 4:2:0/4:2:2/4:4:4, +scaling up and down) and on every plane for 4:2:2 and 4:4:4. At 8 and 16 bits the +transposed path is genuinely unaffected by the bug; at 10 and 12 bits both axes +were broken beforehand, so there it shows mutual consistency rather than a check +against known-good output. 4:2:0 chroma cannot be compared that way at all, since +chroma siting is not symmetric under transposition; its residual difference is +identical for 16-bit input, which the bug never affected. + +diff --git a/src/fmtcl/Scaler.cpp b/src/fmtcl/Scaler.cpp +index b01f594..672e468 100644 +--- a/src/fmtcl/Scaler.cpp ++++ b/src/fmtcl/Scaler.cpp +@@ -399,14 +399,19 @@ void Scaler::process_plane_int_cpp (typename DST::Ptr::Type dst_ptr, typename SR + // Rounding constant for the final shift + const int r_cst = 1 << (SHIFT_INT + SB - DB - 1); + +- // Sign constants: when we have 16-bit data at one end only, +- // we need to make data signed at the oposite end. This sign +- // constant is reported on the summing constant. +- const int s_in = (SB < 16) ? -(0x8000 << (SHIFT_INT + SB - DB)) : 0; +- const int s_out = (DB < 16) ? 0x8000 << (SHIFT_INT + SB - DB) : 0; +- const int s_cst = s_in + s_out; +- +- const int add_cst = _add_cst_int + s_cst + r_cst; ++ // No sign constant here, unlike process_plane_int_sse2/avx2. ++ // Those compute on signed 16-bit vector lanes, so they convert whichever ++ // end is not 16-bit into the signed domain and let the proxy's S16 ++ // flip bit 15 back on read/write. ++ // This C++ path instead accumulates in a plain int and its proxies are ++ // unsigned at both ends: ProxyRwCpp ::read () returns the ++ // raw stored value, and write_clip () clamps to [0, 2^DB-1]. Applying a ++ // sign constant here therefore biased the accumulator by -0x8000 << shift ++ // with nothing to undo it, so every output clamped to 0 -- i.e. vertical ++ // resampling returned a black plane for any source bitdepth below 16 ++ // (SPAN_I covers SB = 14, 12, 10, 9 and 8; only SB = DB = 16 was correct, ++ // because there both s_in and s_out are already 0). ++ const int add_cst = _add_cst_int + r_cst; + + for (int y = y_dst_beg; y < y_dst_end; ++y) + {