diff --git a/CLAUDE.md b/CLAUDE.md index a69dbf8..a6cdd19 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -884,7 +884,7 @@ integration tests. Matrix: macOS **arm64** (`macos-15`), macOS **x64** All three `download-deps-*` scripts apply these automatically, and they must stay **identical across platforms** — a patch applied on only some platforms makes the -same job produce different output per OS. Five patches: +same job produce different output per OS. Six patches: 1. **mvtools API**: Renamed `_lambda`→`lambda`, `_global`→`global` parameters 2. **DFTTest API**: `sstring` parameter removed, replaced with `sigma=10.0` @@ -892,6 +892,7 @@ same job produce different output per OS. Five patches: 4. **EEDI3CL fallback**: modern `eedi3m` dropped `EEDI3CL`, so `opencl=True` fell over; fall back to CPU `EEDI3` (NNEDI3CL still uses the GPU) 5. **`Bob()` 16-bit resample** — see below +6. **ARM nnedi3 preference** — see "The ARM interpolator choice" below Each patch is a **literal string match** against havsfunc r31. If upstream ever changes one of those lines the patch silently does nothing, so behavioural tests @@ -902,6 +903,64 @@ matter more than usual (see `app/test/vapoursynth_integration_test.dart`). > would `AttributeError`. Only reachable with `Denoiser='knlmeanscl'` **and** > `ChromaNoise=True` (both non-default), so it has never been hit. +### The ARM interpolator choice: nnedi3, not znedi3 (patch 6) + +**Never name an nnedi3 implementation directly.** Both templates define a +`_nnedi3()` helper and havsfunc gets an `_nnedi3_impl()` via patch 6; every call +site goes through one of those. `test_92` fails the build if a template +reintroduces a direct `core.znedi3.nnedi3` / `core.nnedi3.nnedi3` call. + +The reason is a pure-performance trap that produces a **correct picture**, so +nothing but a benchmark or that assertion catches it: + +- znedi3's SIMD kernels are x86-only, so `download-deps-{macos,linux}.sh` build it + `make X86=0 X86_AVX512=0` and ARM gets the scalar `PredictorC`/`PrescreenerOldC` + path. The bundled dubhater **`nnedi3` has real NEON kernels** + (`computeNetwork0_neon`, `dotProd_neon`, …). +- Measured on an M1, QTGMC Slow, 400 frames of 720x576: **37.8s of CPU for znedi3 + vs 5.95s for nnedi3 — 6.3x**, and 30% of the entire arm64 QTGMC cost. End to end + the swap is worth **+10% (Faster) to +40% (Slow)**. +- havsfunc hardcoded `core.znedi3.nnedi3 if hasattr(core, 'znedi3')` at three call + sites (daa, santiag, QTGMC), so *every* ARM deinterlace paid it. + +The two plugins implement the same network from the same `nnedi3_weights.bin` and +their signatures are identical for every argument used, so this is a drop-in swap: +measured mean output difference **0.045/255** for the interpolator alone and +**0.072/255** end-to-end through QTGMC (tolerance is 2.0). Worst single pixel is +~48/255 on hard edges, where the two implementations' float rounding flips a +prescreener decision — expected for independent implementations of the same net. + +The choice is made **at runtime** (`platform.machine()`), not by the build, so the +patch text stays identical on every platform and **x86 keeps using znedi3 +unchanged**. nnedi3 is therefore required only on the **ARM** bundles — +`deps-expected-plugins.json` lists it for `macos-arm64` and `linux-arm64` and +deliberately **not** for the three x86 ones, which never call it. Requiring it on +x86 just fails the packaging guard on a plugin nothing would load (nnedi3's x86 +path also needs `yasm`, which the runners don't have). + +> **Building nnedi3 on aarch64 needs two source patches**, because dubhater's +> build system treats every ARM as 32-bit ARMv7. `-mfpu=neon` is an ARMv7 option +> that aarch64 gcc rejects outright, and `cpufeatures.cpp` reads `HWCAP_ARM_*` +> from `getauxval()` — constants that exist only for 32-bit ARM. macOS only ever +> hit the first (it takes the `__APPLE__` branch in `cpufeatures.cpp`), which is +> why Linux arm64 shipped without the plugin until 2026-08-07. Both edits, and a +> guard that hard-fails if either stops matching, are in the nnedi3 block of +> `download-deps-linux.sh`; keep the `-mfpu` expression identical to the macOS +> one. The second patch is the one to be careful with: `nnedi3.cpp` only does +> `if (!cpu.neon) d->opt = 0`, so a wrong answer there yields a **correct picture +> at scalar speed** — the same silent failure this whole section is about. + +> This is one instance of a much larger arm64 gap: native arm64 QTGMC runs +> **3–4.5x slower than the x64 bundle under Rosetta**. The dominant cause is not +> this but `std.Expr` — VapourSynth's `compile_jit()` +> (`src/core/expr/jitcompiler.cpp`) is wrapped in `#ifdef VS_TARGET_CPU_X86`, so on +> aarch64 it returns no compiler and `exprfilter.cpp` falls back to +> `ExprInterpreter::eval()`, a scalar switch-dispatch interpreter run **once per +> pixel**: 69.5s vs 3.3s of CPU on the same job, **21x**. Most of VS core is +> x86-SIMD-only the same way (genericfilters, mergefilters, averageframes, +> planestats). Fixing that needs a vectorized Expr on ARM and is tracked +> separately. Not a cause: mvtools is *faster* natively (it compiles its SSE2 +> paths through simde). ### Linux builds on ubuntu-24.04, and that sets the glibc floor The Linux **deps** builds and the runners that test against them diff --git a/Scripts/deps-expected-plugins.json b/Scripts/deps-expected-plugins.json index 4dbeded..60e5248 100644 --- a/Scripts/deps-expected-plugins.json +++ b/Scripts/deps-expected-plugins.json @@ -118,6 +118,7 @@ "libmiscfilters.so", "libmvtools.so", "libneo-f3kdb.so", + "libnnedi3.so", "libnnedi3cl.so", "libremovegrain.so", "libtcanny.so", diff --git a/Scripts/download-deps-linux.sh b/Scripts/download-deps-linux.sh index a381b85..6cec765 100755 --- a/Scripts/download-deps-linux.sh +++ b/Scripts/download-deps-linux.sh @@ -678,6 +678,30 @@ if [ "$FORCE" = true ] || [ ! -f "$PLUGINS_DIR/libnnedi3.so" ]; then rm -rf nnedi3 git clone --depth 1 https://github.com/dubhater/vapoursynth-nnedi3.git nnedi3 cd nnedi3 + # dubhater's build system treats every ARM as 32-bit ARMv7, so aarch64 fails + # in two unrelated places. Both are build-system bugs, not portability limits: + # the NEON intrinsics themselves compile fine on aarch64 (macOS arm64 builds + # this same simd_neon.c). + # + # 1. -mfpu=neon is an ARMv7 option. NEON is baseline on aarch64 and gcc + # rejects the flag outright. download-deps-macos.sh strips it with this + # same expression -- keep the two identical. + # 2. cpufeatures.cpp reads HWCAP_ARM_* out of getauxval(), and those + # constants exist only for 32-bit ARM. ARMv8-A mandates NEON, so take + # the constant-true path macOS already uses. + # + # (2) is the dangerous one, because getting it wrong is silent: nnedi3.cpp + # only does "if (!cpu.neon) d->opt = 0", so a false negative still produces a + # correct picture -- just at scalar speed, which is the entire thing this + # plugin is bundled to avoid. Both edits are literal string matches, so + # verify they applied rather than shipping an unpatched build. + sed -i 's/ -mfpu=neon//' Makefile.am + sed -i 's/#elif defined(__APPLE__) && defined(NNEDI3_ARM)/#elif (defined(__APPLE__) || defined(__aarch64__)) \&\& defined(NNEDI3_ARM)/' src/cpufeatures.cpp + if grep -q -- '-mfpu=neon' Makefile.am || ! grep -q '__aarch64__' src/cpufeatures.cpp; then + echo " ERROR: the nnedi3 aarch64 build patches no longer apply -- upstream changed." + echo " Refusing to build a silently-scalar nnedi3; fix the patches in this script." + exit 1 + fi if ./autogen.sh && \ PKG_CONFIG_PATH="$PLUGIN_PKG_CONFIG" \ CFLAGS="-I$VS_INCLUDE_DIR" \ @@ -1114,6 +1138,37 @@ if old_bob in content: ) patches.append('Bob 16-bit resample') +# Patch 6: prefer the NEON nnedi3 over the scalar znedi3 on ARM. +# znedi3's SIMD kernels are x86-only, so the ARM bundles build it with X86=0 and +# it runs fully scalar (PredictorC / PrescreenerOldC). The bundled dubhater +# nnedi3 ships real NEON kernels and is 6.3x faster for the same call — measured +# on an M1, QTGMC Slow, 400 frames of 720x576: 37.8s vs 5.95s of CPU, which is +# 30% of the whole arm64 QTGMC cost. havsfunc hardcodes znedi3 whenever it is +# present, so without this every ARM deinterlace pays that. +# Both plugins implement the same network from the same nnedi3_weights.bin and +# their signatures are identical for every argument havsfunc passes, so this is a +# drop-in swap: measured mean output difference 0.045/255 (worst pixel 27/255, on +# edges where the prescreener decision flips). +# The choice is made at runtime rather than by the build, so this patch text +# stays identical on every platform — x86 keeps using znedi3 exactly as before. +if '_nnedi3_impl' not in content: + old_edi = "myNNEDI3 = core.znedi3.nnedi3 if hasattr(core, 'znedi3') else core.nnedi3.nnedi3" + n_edi = content.count(old_edi) + if n_edi: + # Leading whitespace is untouched, so this covers all three call sites + # (daa, santiag, QTGMC) despite their differing indentation. + content = content.replace(old_edi, "myNNEDI3 = _nnedi3_impl()") + content = content.replace('import math\n', 'import math\n' + ''' + +# Prefer the NEON nnedi3 over the scalar znedi3 on ARM (see download-deps-*). +def _nnedi3_impl(): + import platform + if platform.machine().lower() in ('arm64', 'aarch64') and hasattr(core, 'nnedi3'): + return core.nnedi3.nnedi3 + return core.znedi3.nnedi3 if hasattr(core, 'znedi3') else core.nnedi3.nnedi3 +''') + patches.append(f'ARM nnedi3 preference ({n_edi} sites)') + 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 ab45c6a..17aa695 100755 --- a/Scripts/download-deps-macos.sh +++ b/Scripts/download-deps-macos.sh @@ -1577,6 +1577,37 @@ if old_bob in content: ) patches.append('Bob 16-bit resample') +# Patch 6: prefer the NEON nnedi3 over the scalar znedi3 on ARM. +# znedi3's SIMD kernels are x86-only, so the ARM bundles build it with X86=0 and +# it runs fully scalar (PredictorC / PrescreenerOldC). The bundled dubhater +# nnedi3 ships real NEON kernels and is 6.3x faster for the same call — measured +# on an M1, QTGMC Slow, 400 frames of 720x576: 37.8s vs 5.95s of CPU, which is +# 30% of the whole arm64 QTGMC cost. havsfunc hardcodes znedi3 whenever it is +# present, so without this every ARM deinterlace pays that. +# Both plugins implement the same network from the same nnedi3_weights.bin and +# their signatures are identical for every argument havsfunc passes, so this is a +# drop-in swap: measured mean output difference 0.045/255 (worst pixel 27/255, on +# edges where the prescreener decision flips). +# The choice is made at runtime rather than by the build, so this patch text +# stays identical on every platform — x86 keeps using znedi3 exactly as before. +if '_nnedi3_impl' not in content: + old_edi = "myNNEDI3 = core.znedi3.nnedi3 if hasattr(core, 'znedi3') else core.nnedi3.nnedi3" + n_edi = content.count(old_edi) + if n_edi: + # Leading whitespace is untouched, so this covers all three call sites + # (daa, santiag, QTGMC) despite their differing indentation. + content = content.replace(old_edi, "myNNEDI3 = _nnedi3_impl()") + content = content.replace('import math\n', 'import math\n' + ''' + +# Prefer the NEON nnedi3 over the scalar znedi3 on ARM (see download-deps-*). +def _nnedi3_impl(): + import platform + if platform.machine().lower() in ('arm64', 'aarch64') and hasattr(core, 'nnedi3'): + return core.nnedi3.nnedi3 + return core.znedi3.nnedi3 if hasattr(core, 'znedi3') else core.nnedi3.nnedi3 +''') + patches.append(f'ARM nnedi3 preference ({n_edi} sites)') + 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 458e05e..fd44412 100644 --- a/Scripts/download-deps-windows.ps1 +++ b/Scripts/download-deps-windows.ps1 @@ -692,6 +692,35 @@ def _fix_mv_args(args): $PatchesApplied += "Bob 16-bit resample" } + # Patch 6: prefer the NEON nnedi3 over the scalar znedi3 on ARM. + # znedi3's SIMD kernels are x86-only, so the ARM bundles build it with X86=0 + # and it runs fully scalar; the bundled dubhater nnedi3 ships real NEON + # kernels and is 6.3x faster for the same call (measured on an M1, QTGMC + # Slow: 37.8s vs 5.95s of CPU). havsfunc hardcodes znedi3 whenever it is + # present, so without this every ARM deinterlace pays that cost. + # Windows x64 is NOT affected - the runtime check below keeps it on znedi3 + # exactly as before. This is applied here purely so every platform generates + # identical output from an identical havsfunc, same as patch 5. + $OldEdi = "myNNEDI3 = core.znedi3.nnedi3 if hasattr(core, 'znedi3') else core.nnedi3.nnedi3" + if ($Content -notmatch "_nnedi3_impl" -and $Content.Contains($OldEdi)) { + Write-Host " Applying ARM nnedi3 preference patch..." -ForegroundColor Gray + # Leading whitespace is untouched, so this covers all three call sites + # (daa, santiag, QTGMC) despite their differing indentation. + $Content = $Content.Replace($OldEdi, "myNNEDI3 = _nnedi3_impl()") + $PatchFunction = @" + +# Prefer the NEON nnedi3 over the scalar znedi3 on ARM (see download-deps-*). +def _nnedi3_impl(): + import platform + if platform.machine().lower() in ('arm64', 'aarch64') and hasattr(core, 'nnedi3'): + return core.nnedi3.nnedi3 + return core.znedi3.nnedi3 if hasattr(core, 'znedi3') else core.nnedi3.nnedi3 + +"@ + $Content = $Content -replace "(import math\r?\n)", "`$1$PatchFunction" + $PatchesApplied += "ARM nnedi3 preference" + } + if ($PatchesApplied.Count -gt 0) { Set-Content $HavsfuncPath $Content -NoNewline Write-Host " havsfunc patched ($($PatchesApplied -join ', '))" -ForegroundColor Green diff --git a/app/test/integration_filter_parameters_test.dart b/app/test/integration_filter_parameters_test.dart index 61771da..ffd7e05 100644 --- a/app/test/integration_filter_parameters_test.dart +++ b/app/test/integration_filter_parameters_test.dart @@ -360,8 +360,10 @@ void main() { print(' Parsed ${actual.length} params'); expect(actual['alpha'], '0.4'); expect(actual['mdis'], '30'); - // The nnedi3 sclip that guides it carries the nnedi3 controls. - final sclip = parseFilterParams(script, 'core.znedi3.nnedi3('); + // The nnedi3 sclip that guides it carries the nnedi3 controls. Match the + // assignment, not a bare `_nnedi3(` — the helper's own `def _nnedi3(` line + // appears earlier in the script and would be found first. + final sclip = parseFilterParams(script, 'sclip = _nnedi3('); expect(sclip['nsize'], '4'); expect(sclip['nns'], '3'); // And the half-pixel dh shift is corrected per plane. diff --git a/app/test/vapoursynth_integration_test.dart b/app/test/vapoursynth_integration_test.dart index 5b78bda..3370c25 100644 --- a/app/test/vapoursynth_integration_test.dart +++ b/app/test/vapoursynth_integration_test.dart @@ -122,6 +122,18 @@ required = ['std', 'resize', 'mv', 'znedi3', 'eedi3m', 'fmtc', 'ctmf', 'warp', 'misc', 'grain', 'tcanny', 'zsmooth', 'descratch', 'vivtc', 'ttmpsm', 'tmedian', 'fft3dfilter'] + +# On ARM, `nnedi3` is load-bearing and `znedi3` is only the fallback: znedi3's +# SIMD is x86-only, so the ARM bundles build it scalar and both the templates' +# _nnedi3() helper and havsfunc's patched _nnedi3_impl() pick nnedi3 instead +# (6.3x faster, same network and weights). A bundle that dropped nnedi3 would +# still deinterlace correctly — it would just silently be slow again, which is +# exactly the kind of regression a "does it run" check never catches. +# x86 bundles deliberately do NOT ship plain nnedi3 (Windows and macOS-x64 have +# only znedi3 + nnedi3cl), so this requirement is arch-conditional, not global. +import platform +if platform.machine().lower() in ('arm64', 'aarch64'): + required.append('nnedi3') # nnedi3cl and knlm are deliberately NOT required: both are OpenCL and the app # degrades to a CPU path when the driver is absent. diff --git a/worker/templates/pipeline_template.vpy b/worker/templates/pipeline_template.vpy index 7cc0ed4..2920c02 100644 --- a/worker/templates/pipeline_template.vpy +++ b/worker/templates/pipeline_template.vpy @@ -49,6 +49,23 @@ print(f"INPUT_INFO:frames={total_frames},fps_num={input_fps_num},fps_den={input_ # Import havsfunc for various filters (QTGMC, SMDegrain, chroma fixes) import havsfunc as haf +# znedi3 carries x86-only SIMD, so on ARM it is built with X86=0 and runs fully +# scalar — measured 6.3x slower than the bundled nnedi3, which ships real NEON +# kernels. Both implement the same network from the same weights file and their +# signatures are identical for every argument used here, so preferring nnedi3 on +# ARM is a drop-in swap (measured mean difference 0.045/255). Route every +# interpolator call through this helper rather than naming a plugin directly. +# havsfunc's own three call sites get the same treatment via its patch 6. +import platform as _platform + +_IS_ARM = _platform.machine().lower() in ('arm64', 'aarch64') + + +def _nnedi3(clip, **kwargs): + if _IS_ARM and hasattr(core, 'nnedi3'): + return core.nnedi3.nnedi3(clip, **kwargs) + return core.znedi3.nnedi3(clip, **kwargs) + # ============================================================================ # PASS 1: PRE-CROP (before deinterlacing to reduce processing area) # ============================================================================ @@ -1047,7 +1064,7 @@ if clip.format.color_family == vs.YUV: {{#UPSCALE_EDGE_DIRECTED}} def _upscale_double_height(c): {{#UPSCALE_NNEDI3}} - return core.znedi3.nnedi3( + return _nnedi3( c, field=1, dh=True, @@ -1072,7 +1089,7 @@ def _upscale_double_height(c): # EEDI3 needs a second opinion to bound its cost search. nnedi3 is the # conventional choice and is bundled anyway, so the nnedi3 controls below # shape the sclip that guides EEDI3. - sclip = core.znedi3.nnedi3( + sclip = _nnedi3( c, field=1, dh=True, diff --git a/worker/templates/preview_template.vpy b/worker/templates/preview_template.vpy index 9b2c549..7fca623 100644 --- a/worker/templates/preview_template.vpy +++ b/worker/templates/preview_template.vpy @@ -41,6 +41,23 @@ print(f"INPUT_INFO:frames={total_frames},fps_num={{FPS_NUM}},fps_den={{FPS_DEN}} # Import havsfunc for various filters (QTGMC, SMDegrain, chroma fixes) import havsfunc as haf +# znedi3 carries x86-only SIMD, so on ARM it is built with X86=0 and runs fully +# scalar — measured 6.3x slower than the bundled nnedi3, which ships real NEON +# kernels. Both implement the same network from the same weights file and their +# signatures are identical for every argument used here, so preferring nnedi3 on +# ARM is a drop-in swap (measured mean difference 0.045/255). Route every +# interpolator call through this helper rather than naming a plugin directly. +# havsfunc's own three call sites get the same treatment via its patch 6. +import platform as _platform + +_IS_ARM = _platform.machine().lower() in ('arm64', 'aarch64') + + +def _nnedi3(clip, **kwargs): + if _IS_ARM and hasattr(core, 'nnedi3'): + return core.nnedi3.nnedi3(clip, **kwargs) + return core.znedi3.nnedi3(clip, **kwargs) + # ============================================================================ # PASS 1: PRE-CROP (before deinterlacing to reduce processing area) # ============================================================================ @@ -993,7 +1010,7 @@ if clip.format.color_family == vs.YUV: {{#UPSCALE_EDGE_DIRECTED}} def _upscale_double_height(c): {{#UPSCALE_NNEDI3}} - return core.znedi3.nnedi3( + return _nnedi3( c, field=1, dh=True, @@ -1018,7 +1035,7 @@ def _upscale_double_height(c): # EEDI3 needs a second opinion to bound its cost search. nnedi3 is the # conventional choice and is bundled anyway, so the nnedi3 controls below # shape the sclip that guides EEDI3. - sclip = core.znedi3.nnedi3( + sclip = _nnedi3( c, field=1, dh=True, diff --git a/worker/tests/filter_integration_test.rs b/worker/tests/filter_integration_test.rs index 75ae11d..e202ae5 100644 --- a/worker/tests/filter_integration_test.rs +++ b/worker/tests/filter_integration_test.rs @@ -2510,7 +2510,7 @@ fn test_70_upscale_nnedi3_granular_parameters() { }); run_job_and_verify(&job, "NNEDI3 upscale granular", &[ - "core.znedi3.nnedi3", + "return _nnedi3(", "nsize=4", "nns=4", "qual=2", @@ -3383,3 +3383,56 @@ fn test_91_chroma_subsampling_serde_names_match_the_app() { Some("vs.YUV422P10") ); } + +/// Test 92: neither template may name an nnedi3 implementation directly. +/// +/// znedi3's SIMD kernels are x86-only, so the ARM bundles build it with X86=0 +/// and it runs fully scalar — measured 6.3x slower than the bundled nnedi3, +/// which ships real NEON kernels, and 30% of the whole arm64 QTGMC cost. Both +/// implement the same network from the same weights file, so the templates pick +/// at runtime via the `_nnedi3()` helper (havsfunc does the same through its +/// patch 6). A direct `core.znedi3.nnedi3` call reintroduces the slow path on +/// ARM silently — it still produces a correct picture, just far slower, so only +/// an assertion like this one catches it. +#[test] +fn test_92_templates_do_not_hardcode_an_nnedi3_implementation() { + let templates_dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("templates"); + + for name in ["pipeline_template.vpy", "preview_template.vpy"] { + let path = templates_dir.join(name); + // Normalise line endings: git checks these templates out CRLF on Windows, + // and the blank-line search below is otherwise looking for "\n\n" in a + // file that only ever contains "\r\n\r\n". + let body = std::fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("read {}: {e}", path.display())) + .replace("\r\n", "\n"); + + // The helper itself is the one place either plugin may be named. + let helper_start = body + .find("def _nnedi3(") + .unwrap_or_else(|| panic!("{name} is missing the _nnedi3() helper")); + let helper_end = helper_start + + body[helper_start..] + .find("\n\n") + .expect("helper should be followed by a blank line"); + let (before, after) = (&body[..helper_start], &body[helper_end..]); + + for (plugin, call) in [ + ("znedi3", "core.znedi3.nnedi3("), + ("nnedi3", "core.nnedi3.nnedi3("), + ] { + assert!( + !before.contains(call) && !after.contains(call), + "{name} calls {plugin} directly; route it through _nnedi3() so ARM \ + gets the NEON implementation" + ); + } + + // And the helper is actually reached — the upscale path is the only + // caller in the template itself (QTGMC goes through havsfunc). + assert!( + body.contains("_nnedi3(\n"), + "{name} defines _nnedi3() but never calls it" + ); + } +}