Skip to content

fix(deps): fix fmtconv's non-x86 integer scaler, pin fmtconv to r31 #52

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
StuartCameronCode merged 1 commit into from
Aug 1, 2026
Merged

fix(deps): fix fmtconv's non-x86 integer scaler, pin fmtconv to r31 #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 49 additions & 2 deletions Scripts/download-deps-linux.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -586,11 +586,31 @@ build_plugin "eedi3m" \
"sed -i '1i #include <cstddef>' 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" \
Expand Down Expand Up @@ -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)
Expand Down
51 changes: 49 additions & 2 deletions Scripts/download-deps-macos.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
33 changes: 31 additions & 2 deletions Scripts/download-deps-windows.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
@{
Expand Down Expand Up @@ -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
Expand Down
115 changes: 115 additions & 0 deletions Scripts/patches/fmtconv-r31-arm-int-scaler.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<SplFmt_INT16>::S16<CLIP_FLAG, SIGN_FLAG>, 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<SplFmt_INT8/INT16>::read()
returns the raw stored value, and write_clip<DB>() 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<DB>'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
+ // <CLIP_FLAG, SIGN_FLAG> 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 <SplFmt_INT8/INT16>::read () returns the
+ // raw stored value, and write_clip <DB> () 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)
{
Loading