fix(deps): fix fmtconv's non-x86 integer scaler, pin fmtconv to r31 - #52
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.0is published.Root cause
fmtconv's
Scaler::process_plane_int_cppapplied the same sign-conversion constants asprocess_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 onSB == 16andDB == 16. The C++ kernel has no counterpart: it accumulates in a plainintandProxyRwCppis unsigned at both ends. So the constant only biased the sum, andwrite_clipclipped the negative result to 0.The bug fires whenever that kernel runs with
SB < 16, on either axis:SB=16 DB=16SB=8 DB=16SB=10 DB=16SB=12 DB=16SB=16 DB=168-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
Scalerconstructor — so only macos-arm64 and linux-arm64 were affected.Downstream this destroyed havsfunc's
Bob(), which bobs fields withfmtc.resample(scalev=2, ...). QTGMC's Placebo and Very Slow (the only presets defaultingNoiseProcess=2, whose noise pass callsBob()) came out ~+10/255 too bright; Draft (EdiMode='bob') came out nearly black.Changes
Scripts/patches/fmtconv-r31-arm-int-scaler.patch— applied bydownload-deps-{macos,linux}.shright 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-O0build.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.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.0is built and the app-side PR runs against it.