Skip to content

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

Merged
StuartCameronCode merged 1 commit into
mainfrom
deps/fmtconv-arm-int-scaler
Aug 1, 2026
Merged

fix(deps): fix fmtconv's non-x86 integer scaler, pin fmtconv to r31#52
StuartCameronCode merged 1 commit into
mainfrom
deps/fmtconv-arm-int-scaler

Conversation

@StuartCameronCode

Copy link
Copy Markdown
Owner

Build-side half of the QTGMC preset-brightening fix on Apple Silicon. Only affects how the deps bundles are built — the app-side pointer bump, tests and docs follow separately once deps-v1.7.0 is published.

Root cause

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 <SplFmt_INT16>::S16 <CLIP_FLAG, SIGN_FLAG> XORs bit 15 on read/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. 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 only case that escapes (that route converts to 16-bit before the scaler). On x86 none of it is reachable — the SSE2/AVX2 scalers replace the function in the Scaler constructor — so 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 (the only presets defaulting NoiseProcess=2, whose noise pass calls Bob()) came out ~+10/255 too bright; Draft (EdiMode='bob') came out nearly black.

Changes

  • Scripts/patches/fmtconv-r31-arm-int-scaler.patch — applied by download-deps-{macos,linux}.sh right after the clone. Hard-fails the build if it ever stops applying, rather than silently shipping the bug back. The header carries the full analysis. Not submitted upstream (needs a GitLab account); upstream r31 does not fix this — building r31 reproduces it, as do yuygfgg's prebuilt arm64 binary and a local -O0 build.
  • havsfunc patch 5 — makes Bob() resample at 16-bit. Redundant now and measured to give the same output, but it also covers the prebuilt Windows DLL and any build where the fmtconv patch is dropped. Applied on every platform so all platforms generate output from an identical havsfunc.
  • fmtconv pinned to r31 everywhere. Upstream moved to GitLab in Aug 2023; the GitHub repo is an abandoned mirror whose last commit is literally "Repository moved to Gitlab", so macOS/Linux were cloning an unpinned stale post-r30 snapshot while Windows took the r30 zip. All three move together — r31 changed interlaced PAL-DV chroma placement, so a version skew would change chroma per platform.

Verification

Measured on interlaced_test.avi (source luma 130.896): Very Slow 140.794 → 130.895, Placebo 140.825 → 130.910, Draft 36.752 → 131.032, Slower 130.930 → 130.994 (control).

Correctness rather than just non-black: transposing the input, resampling along the other axis and transposing back gives output 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, up and down) and on every plane for 4:2:2 and 4:4:4.

Nothing here is exercised by the push gate (it downloads the published bundle rather than building it), so CI passing on this PR does not validate the change — that happens when deps-v1.7.0 is built and the app-side PR runs against it.

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 <SplFmt_INT16>::S16 <CLIP_FLAG, SIGN_FLAG> 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<DB>() 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.
@StuartCameronCode
StuartCameronCode merged commit 9186a14 into main Aug 1, 2026
4 checks passed
@StuartCameronCode
StuartCameronCode deleted the deps/fmtconv-arm-int-scaler branch August 1, 2026 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant