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) + {