fix(deinterlace): interlaced chroma, field-order derivation, and QTGMC preset brightening (#49) - #53
Merged
Conversation
…on (#49) Reported as "QTGMC output has chroma artifacts on interlaced DVD sources". The suggested fix — mark the clip field-based before deinterlacing — was already in place since 0.9.0, so this addresses the underlying problems found while investigating instead. Deinterlace working format. Interlaced 4:2:0 stores chroma per field, so interpolating it at 4:2:0 mixes the two fields' chroma. The QTGMC pass now converts to 4:2:2 first (field-aware, because zimg honours _FieldBased) and restores the source format afterwards. A second option runs the pass at 16-bit and dithers back, avoiding rounding accumulated across QTGMC's many merge/expr steps. chromaUpsampleFix default on — no-op on 4:2:2/4:4:4 sources highPrecision default off — roughly doubles time and memory With both off the generated script is what it was before the block existed. ChromaEdi validation. havsfunc implements only '', 'nnedi3' and 'bob'. Any other non-empty value disables chroma EDI (planes=[0]) and then returns the luma-only interpolation without ever restoring chroma. The schema offered a "Blend" option that did exactly this — measured on a 4:2:0 fixture, paired output frames differed by up to 253 in chroma versus 16 on the correct path. "Blend" is removed from the schema and normalized_chroma_edi drops any unsupported value in the worker, so saved presets cannot reintroduce it. Field order. std.SeparateFields ignores its `tff` argument whenever _FieldBased is set — verified: with the property set, TFF=True and TFF=False produce bit-identical output. The property, not QTGMC's TFF parameter, decides the field order actually used. The encode path derived it from detected_field_order while the preview path derived it from qtgmc_parameters.tff, so the two could disagree and autodetection could silently override the user's choice. Both now share one derivation, ScriptGenerator::field_based_for, and PreviewParams.field_based is gone so there is no second value to drift. Tests: 5 script-generation tests in the push gate (Rust test_58-62 plus Flutter), 2 full-encode tests in nightly. The heavy tests use the 4:2:0 fixture — chromaUpsampleFix is a no-op on the shared 4:2:2 one — and assert the working format never reaches the encoder, and that a "Blend" run is frame-identical to one with no ChromaEdi set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deps are unchanged, so deps-version.json is untouched. Scripts/update-version.sh guards the macOS Info.plist update behind /usr/libexec/PlistBuddy, so running it on Windows silently skips the plist and leaves it on the previous version. Updated by hand here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Measured on a real source, the 4:2:0 -> 4:2:2 upconversion drops throughput from 35 to 24 fps — roughly 30% — for a quality gain that is subtle on most material. That is too much to impose on every deinterlace job, so it now defaults to off and joins 16-bit as an option the user opts into. The schema description states the cost so the trade-off is visible at the checkbox rather than buried in the docs. Consequence worth naming: with both working-format options off by default, the default deinterlace path is byte-for-byte what it was before #49, so the reported chroma striping is not fixed by default — it is a toggle. What does still apply by default from that work is the ChromaEdi "Blend" removal and the single field-order derivation. Tests split rather than re-baselined. "Chroma fix on by default" was a real assertion, it just is not about the default any more: test_58 default emits no working-format block at all test_58b chroma upsample emitted when opted into test_59 16-bit alone, and does not pull in the chroma upsample test_59b both together Same split on the Flutter side. The negative assertions are the point: the two options are independent, and with both off by default a change that coupled them would otherwise pass silently. test_55 (preview/encode parity) now opts in explicitly, since with the default off it had nothing to compare. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n arm64
App-side half of the fmtconv fix. The build-side change (the fmtconv patch, the
r31 pin and havsfunc patch 5) landed separately on main; this points the app at
the resulting bundle and adds the tests that can only pass against it.
Symptom was that QTGMC's Very Slow and Placebo presets brightened the picture by
roughly +10/255 on Apple Silicon while Windows was fine, and Draft came out very
nearly black — a much worse case that had gone unreported.
Root cause was in fmtconv, not QTGMC: Scaler::process_plane_int_cpp applied the
SSE2/AVX2 paths' sign-conversion constants while using unsigned C++ proxies, so
the accumulator went negative and write_clip clipped it to 0. Any resample whose
kernel ran with a source bitdepth below 16 returned a black plane, on builds
without the x86 SIMD path. That destroyed havsfunc's Bob(), which feeds QTGMC's
noise pass (Placebo and Very Slow are the only presets defaulting
NoiseProcess=2) and the Draft preset's interpolation. See the deps commit and
Scripts/patches/fmtconv-r31-arm-int-scaler.patch for the full analysis.
Measured on Tests/TestResources/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, already correct)
Deps bumped to 1.7.0 (deps-v1.7.0). Minor rather than patch because the bundle
also moves fmtconv r30 -> r31, which changes interlaced PAL-DV chroma placement.
Tests go in vapoursynth_integration_test.dart, which is in the per-push gate:
fmtc.resample scaling at 8/10/12/16-bit on both axes, Bob() preserving average
luma, and Very Slow plus Draft not shifting it. All three were confirmed to fail
before the fix (fmtc.resample 0.000 against an expected 119.53, Bob off by
120.000, QTGMC by +14.422) and pass after, so they are real guards rather than
assertions of current behaviour. They assert observable pixel behaviour rather
than patch text, so they stay valid if fmtconv ever fixes this upstream and the
patches are dropped.
No Rust test: despite its name, run_job only generates and inspects the .vpy and
never executes vspipe, so that suite cannot catch plugin behaviour. CLAUDE.md
claimed otherwise and is corrected here, along with the havsfunc patch list
(documented as three, actually four, now five) and a note that patch 3 misses
KNLMeansCL's vs.YCOCG check.
Unrelated but included: app/macos/Podfile.lock is regenerated because its PODFILE
CHECKSUM did not match the committed Podfile, which made a clean macOS debug build
fail with "the sandbox is not in sync with the Podfile.lock".
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
1 participant
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.
Closes #49, and fixes the QTGMC preset brightening found while testing it.
Requires deps-v1.7.0 —
app/assets/deps-version.jsonpoints at it, and the new tests only pass against that bundle. The build-side half is already on main (#52) and the five bundles are published.Interlaced chroma and field order (#49)
std.SeparateFieldsignores itstffargument whenever_FieldBasedis set, so the property decides the field order actually used._FieldBasedand QTGMC'sTFF=now come from one derivation (ScriptGenerator::field_based_for), used by both the encode and preview paths — otherwise autodetection silently overrides the user's choice, and the preview can deinterlace differently from the render.chromaUpsampleFix(4:2:0 → 4:2:2 before QTGMC, field-aware since zimg honours_FieldBased; ~30% throughput cost) andhighPrecision(16-bit, dithered back). Independent, both default off, so the default path is byte-for-byte what it was before.ChromaEdiis now validated rather than passed through: havsfunc implements only'','nnedi3'and'bob', and any other non-empty value disables chroma EDI then returns luma-only interpolation without restoring chroma.QTGMC preset brightening (found while testing the above)
Very Slow and Placebo brightened the picture by ~+10/255 on Apple Silicon while Windows was fine; Draft came out nearly black. Root cause was in fmtconv, not QTGMC —
Scaler::process_plane_int_cppapplied the SSE2/AVX2 paths' sign-conversion constants while using unsigned C++ proxies, so the accumulator went negative andwrite_clipclipped it to 0. Any resample whose kernel ran with a source bitdepth below 16 returned a black plane, on builds without the x86 SIMD path. That destroyed havsfunc'sBob(), which feeds QTGMC's noise pass (Placebo and Very Slow are the only presets defaultingNoiseProcess=2) and Draft's interpolation.Fixed in the bundle by
Scripts/patches/fmtconv-r31-arm-int-scaler.patch(#52). Measured oninterlaced_test.avi, source luma 130.896:Tests
Three new cases in
vapoursynth_integration_test.dart(in the push gate):fmtc.resamplescaling at 8/10/12/16-bit on both axes,Bob()preserving average luma, and Very Slow plus Draft not shifting it. All three were confirmed to fail before the fix (fmtc.resample0.000 against an expected 119.53,Boboff by 120.000, QTGMC by +14.422) and pass after, so they are real guards rather than assertions of current behaviour. They assert observable pixel behaviour rather than patch text, so they stay valid if fmtconv fixes this upstream and the patches are dropped.No Rust test for the fmtconv fix: despite its name,
run_jobonly generates and inspects the.vpyand never executes vspipe, so that suite cannot catch plugin behaviour. CLAUDE.md claimed otherwise and is corrected here.Notes
app/macos/Podfile.lockis regenerated — unrelated, but itsPODFILE CHECKSUMdid not match the committedPodfile, which made a clean macOS debug build fail with "the sandbox is not in sync with the Podfile.lock".KNLMeansCL'svs.YCOCGcheck, reachable only withDenoiser='knlmeanscl'andChromaNoise=True(both non-default), so it has never been hit. Not fixed here.