diff --git a/CLAUDE.md b/CLAUDE.md index ce06904..e174ff7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -296,7 +296,7 @@ Adding a filter touches many files. Missing any step causes silent failures (fil **8. VapourSynth Compatibility Guards** - If the plugin only supports 8-bit, add bit-depth conversion guard in templates -- If the plugin fails on field-based clips, note this — the preview template conditionally sets field-based via `{{#SET_FIELD_BASED}}` (only when deinterlacing is enabled) +- If the plugin fails on field-based clips, note this — both templates set field-based via `{{#SET_FIELD_BASED}}`, emitted whenever `ScriptGenerator::field_based_for` returns a value (see "Field Order" below) **9. Integration Tests** — required, BOTH suites (see "Integration Tests for Filter Changes" below): - Add a numbered Rust test in `worker/tests/filter_integration_test.rs`. @@ -446,6 +446,49 @@ The most important parameters: - **SourceMatch**: Higher fidelity mode (0=off, 1-3=increasingly accurate) - **FPSDivisor**: 1=double-rate (50i→50p), 2=single-rate (50i→25p) +### Field Order (issue #49) — read before touching `_FieldBased` or `TFF` + +`std.SeparateFields` **ignores its `tff` argument whenever the `_FieldBased` +frame property is set** — the property wins, so QTGMC's `TFF=` parameter is +inert on a marked clip. Both must therefore come from one value: +`ScriptGenerator::field_based_for` is that single derivation, used by the encode +path *and* the preview path. Deriving them separately lets autodetection +silently override the user's field-order choice and lets the preview deinterlace +differently from the final render. + +- Deinterlacing enabled → `_FieldBased` comes from `pipeline.deinterlace.tff`. +- Deinterlacing disabled → falls back to `job.detected_field_order`, so + field-aware chroma resampling in a later `resize` is still correct + (zimg **does** honour `_FieldBased`). + +### Deinterlace Working Format (issue #49) + +Two QTGMC options wrap the deinterlace pass in a format conversion, restoring +the source format immediately afterwards (`{{#DEINT_WORKING_FORMAT}}` / +`{{#DEINT_RESTORE_FORMAT}}` in both templates): + +Both are **opt-in** — they trade throughput for quality, so the choice is the +user's: + +- **`chromaUpsampleFix`** (default **off**): interlaced 4:2:0 stores chroma per + field, so convert to 4:2:2 before deinterlacing — field-aware, because zimg + honours `_FieldBased`. Costs roughly 30% throughput (measured 35 → 24 fps). + No-op on 4:2:2/4:4:4 sources. +- **`highPrecision`** (default **off**): run the pass at 16-bit and dither back, + avoiding accumulated 8-bit rounding across QTGMC's many merge/expr steps. + Roughly doubles time and memory. + +The two are independent — enabling one must not pull in the other. With both +off (the default) the generated script is byte-for-byte what it was before the +block existed, asserted by `test_58_deinterlace_working_format_default` and +`test_60_deinterlace_working_format_disabled`. + +**`ChromaEdi` is validated, not passed through.** 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, badly corrupting it. `QTGMCParameters::normalized_chroma_edi` +drops unsupported values — don't bypass it. + ## Testing VapourBox has **three distinct test suites**. Know which is which before adding @@ -459,8 +502,11 @@ cd worker && cargo test --test subtitle_integration_test -- --nocapture ``` - `filter_integration_test.rs` — **every filter/parameter change must add a - numbered test here** (see "Adding a New Filter"). It generates `.vpy` scripts - and, via `run_job`, runs the `vspipe | ffmpeg` pipeline, so it needs `deps/`. + numbered test here** (see "Adding a New Filter"). Note that despite the name, + `run_job` **only generates and inspects the `.vpy` script** — it does not run + `vspipe | ffmpeg` (see its body: "For now, just verify script generation + works"). So this suite validates *script generation*, and cannot catch runtime + or plugin-behaviour bugs; those need the Dart shell-out/integration tests. - `subtitle_integration_test.rs` — runs `whisper-cli` on `Tests/TestResources/small_clip.mp4`; needs the whisper add-on + ffmpeg. - In **debug** builds `DependencyLocator` finds repo-root `deps/`/`addons/` by @@ -534,10 +580,114 @@ integration tests. Matrix: macOS **arm64** (`macos-15`), macOS **x64** ## havsfunc Compatibility Patches -The `download-deps-windows.ps1` and `download-deps-macos.sh` scripts apply these automatically. Three patches are needed for modern VapourSynth: +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: + 1. **mvtools API**: Renamed `_lambda`→`lambda`, `_global`→`global` parameters 2. **DFTTest API**: `sstring` parameter removed, replaced with `sigma=10.0` 3. **VapourSynth YCOCG removal**: `vs.YCOCG` removed, simplify to `!= vs.YUV` +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 + +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 +matter more than usual (see `app/test/vapoursynth_integration_test.dart`). + +> Known gap: patch 3 only rewrites the `input.format...` YCOCG check, so +> `KNLMeansCL`'s `clip.format.color_family not in [vs.YUV, vs.YCOCG]` survives and +> would `AttributeError`. Only reachable with `Denoiser='knlmeanscl'` **and** +> `ChromaNoise=True` (both non-default), so it has never been hit. + +### fmtconv's aarch64 integer scaler bug (fixed by our own patch) + +**Symptom.** `fmtc.resample` returned **black** whenever fmtconv's integer kernel +ran with a source bitdepth below 16. Only **macos-arm64** and **linux-arm64** were +affected: on x86 the SSE2/AVX2 scalers replace the offending function in the +`Scaler` constructor, so there it is dead code. Measured by 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 | + +It is **not** vertical-only: 10- and 12-bit sources broke on both axes. 8-bit +horizontal is the single case that escaped, because that route converts to 16-bit +*before* the scaler and so lands on `SB = DB = 16`. Same-size resampling and float +sources were never affected. (Deinterlacing hit it via the vertical axis, which is +why it first looked like a vertical-only bug.) + +**Root cause** (`src/fmtcl/Scaler.cpp`, `process_plane_int_cpp`). That kernel +applied the same sign-conversion constants as its SSE2/AVX2 siblings: + +```c +s_in = (SB < 16) ? -(0x8000 << (SHIFT_INT + SB - DB)) : 0; +``` + +The vector kernels need those because they accumulate in **signed** 16-bit lanes +and their proxy (`ProxyRwSse2::S16`) XORs bit +15 back on read/write. The C++ kernel has no such counterpart — it accumulates in +a plain `int` and `ProxyRwCpp` is **unsigned** at both ends (`read()` returns the +raw value, `write_clip()` clamps to `[0, 2^DB-1]`). So the bias was applied +with nothing to undo it and every output clamped to 0. For 8→16-bit, +flat input 120: `120*4096 - 524288 + 8 = -32760`, `>>4 = -2048`, clamped to `0`. +`SPAN_I` covers SB = 16, 14, 12, 10, 9, 8 with DB always 16, so only SB = DB = 16 +was correct — there the constants are already 0. + +**Fix.** `Scripts/patches/fmtconv-r31-arm-int-scaler.patch` drops the sign +constants from the C++ kernel. Applied by `download-deps-{macos,linux}.sh` right +after the clone, and it **hard-fails the build** if it stops applying rather than +silently shipping the bug back. Windows needs nothing (prebuilt x86 DLL). + +Not submitted upstream — that needs a GitLab account. If anyone wants to send it +later, the patch is the whole change and its header carries the full analysis. + +**How it surfaced.** havsfunc's `Bob()` bobs fields via +`fmtc.resample(scalev=2, ...)`, so on Apple Silicon it destroyed the image: + +- **Placebo / Very Slow** came out **~+10/255 brighter**. They 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 a large positive + bias back in. +- **Draft** came out nearly black (`EdiMode='bob'` interpolates via `Bob()`). +- **Slower and below** were fine: their only fmtconv use is the same-size `Sbb` + gauss blur, which doesn't scale vertically. + +havsfunc patch 5 (`Bob()` at 16-bit) is kept as **defence in depth** — it is +redundant now that fmtconv is fixed, and both routes were measured to produce the +same output, but it also covers the prebuilt Windows DLL and any future build +where the fmtconv patch is dropped. Removing it would be safe; removing the +fmtconv patch would not, since `Bob()` is far from the only sub-16-bit +resample in havsfunc. + +**Upstream status.** Not fixed as of r31. Its changelog has a promising "program +path without x86 SIMD: fixed wrong conversions (noticed on ARM/Apple)" entry, but +r31 reproduces the failure exactly — as do yuygfgg's prebuilt arm64 binary and a +local `-O0` build. Worth re-checking on the next release so the patch can be +dropped; the tests assert pixel behaviour, not patch text, so they stay valid +either way. + +### fmtconv version is pinned, and upstream moved to GitLab + +fmtconv development moved to **`gitlab.com/EleonoreMizo/fmtconv`** in Aug 2023. +The GitHub repo is an abandoned mirror whose last commit is literally +*"Repository moved to Gitlab"*, so cloning its `master` silently pinned us to a +stale post-r30 snapshot. All platforms now use **r31**, pinned: + +- macOS / Linux: `FMTCONV_TAG` in `download-deps-{macos,linux}.sh` (GitLab tag) +- Windows: the r31 zip in `download-deps-windows.ps1` (from the author's site — + GitHub release assets stopped at r30, and that URL is what the official GitLab + r31 release links to) + +**Keep these in step.** r31 changed interlaced PAL-DV chroma placement (U/V +vertical positions were swapped, vertical subsampling > 2 unhandled), so a +version skew between platforms would change chroma per-OS. ## Debugging Tips @@ -750,3 +900,5 @@ Create the app-specific password at appleid.apple.com → Sign-In and Security | Deps Version | Date | Changes | |--------------|------|---------| | 1.0.0 | 2025-01-15 | Initial release | +| … | | (1.1.0–1.6.0 went unrecorded) | +| 1.7.0 | 2026-08-01 | Fixes QTGMC Placebo/Very Slow brightening and near-black Draft on arm64, via `Scripts/patches/fmtconv-r31-arm-int-scaler.patch` (root cause: sign constants in fmtconv's non-SIMD integer scaler) plus havsfunc patch 5 as defence in depth; fmtconv r30 → **r31**, now pinned and sourced from GitLab on every platform | diff --git a/app/assets/deps-version.json b/app/assets/deps-version.json index daac979..3540106 100644 --- a/app/assets/deps-version.json +++ b/app/assets/deps-version.json @@ -1,6 +1,6 @@ { - "version": "1.6.0", - "releaseTag": "deps-v1.6.0", - "releaseDate": "2026-06-17", + "version": "1.7.0", + "releaseTag": "deps-v1.7.0", + "releaseDate": "2026-08-01", "githubRepo": "StuartCameronCode/VapourBox" } diff --git a/app/assets/filters/core/deinterlace.json b/app/assets/filters/core/deinterlace.json index 1918f8b..1a768f4 100644 --- a/app/assets/filters/core/deinterlace.json +++ b/app/assets/filters/core/deinterlace.json @@ -32,6 +32,8 @@ "tff", "inputType", "fpsDivisor", + "chromaUpsampleFix", + "highPrecision", "tr0", "tr1", "tr2", @@ -239,6 +241,34 @@ } } }, + "chromaUpsampleFix": { + "type": "boolean", + "default": false, + "ui": { + "label": "Interlaced chroma fix", + "description": "Interlaced 4:2:0 stores chroma per field. Convert to 4:2:2 with field-aware resampling before deinterlacing and back afterwards. Reduces faint chroma striping in flat colour areas, at roughly 30% slower processing. No effect on 4:2:2 or 4:4:4 sources.", + "widget": "checkbox", + "visibleWhen": { + "method": [ + "qtgmc" + ] + } + } + }, + "highPrecision": { + "type": "boolean", + "default": false, + "ui": { + "label": "16-bit processing", + "description": "Run the deinterlace pass at 16-bit and dither back at the end, avoiding accumulated 8-bit rounding. Roughly doubles processing time.", + "widget": "checkbox", + "visibleWhen": { + "method": [ + "qtgmc" + ] + } + } + }, "tr0": { "type": "integer", "default": 2, @@ -487,8 +517,7 @@ "options": [ "", "NNEDI3", - "Bob", - "Blend" + "Bob" ], "vapoursynth": { "name": "ChromaEdi" @@ -2057,7 +2086,9 @@ { "title": "Input", "parameters": [ - "inputType" + "inputType", + "chromaUpsampleFix", + "highPrecision" ], "expanded": false, "advancedOnly": true diff --git a/app/lib/models/parameter_converter.dart b/app/lib/models/parameter_converter.dart index b711ca6..9ae6ef1 100644 --- a/app/lib/models/parameter_converter.dart +++ b/app/lib/models/parameter_converter.dart @@ -38,6 +38,10 @@ class ParameterConverter { 'preset': params.preset.displayName, 'tff': params.tff, 'fpsDivisor': params.fpsDivisor, + // Null means "not set" in the model; surface the effective default so + // the checkbox always reflects what the worker will actually do. + 'chromaUpsampleFix': params.chromaUpsampleFix ?? false, + 'highPrecision': params.highPrecision ?? false, 'inputType': params.inputType, 'tr0': params.tr0, 'tr1': params.tr1, @@ -439,6 +443,8 @@ class ParameterConverter { ), tff: v['tff'] as bool?, fpsDivisor: v['fpsDivisor'] as int?, + chromaUpsampleFix: v['chromaUpsampleFix'] as bool?, + highPrecision: v['highPrecision'] as bool?, inputType: v['inputType'] as int?, tr0: v['tr0'] as int?, tr1: v['tr1'] as int?, diff --git a/app/lib/models/qtgmc_parameters.dart b/app/lib/models/qtgmc_parameters.dart index 438dd14..6ea7712 100644 --- a/app/lib/models/qtgmc_parameters.dart +++ b/app/lib/models/qtgmc_parameters.dart @@ -133,6 +133,15 @@ class QTGMCParameters { final bool? tff; final int? fpsDivisor; + // === Working Format (issue #49) === + /// Upsample 4:2:0 chroma to 4:2:2 (field-aware) before deinterlacing and + /// restore the source format afterwards. Costs roughly 30% throughput, so + /// null = disabled. + final bool? chromaUpsampleFix; + + /// Run the deinterlace pass at 16-bit and dither back. Null = disabled. + final bool? highPrecision; + // === Quality (Temporal Radius) === final int? tr0; final int? tr1; @@ -260,6 +269,8 @@ class QTGMCParameters { this.inputType, this.tff, this.fpsDivisor, + this.chromaUpsampleFix, + this.highPrecision, this.tr0, this.tr1, this.tr2, @@ -355,6 +366,8 @@ class QTGMCParameters { int? inputType, bool? tff, int? fpsDivisor, + bool? chromaUpsampleFix, + bool? highPrecision, int? tr0, int? tr1, int? tr2, @@ -445,6 +458,8 @@ class QTGMCParameters { inputType: inputType ?? this.inputType, tff: tff ?? this.tff, fpsDivisor: fpsDivisor ?? this.fpsDivisor, + chromaUpsampleFix: chromaUpsampleFix ?? this.chromaUpsampleFix, + highPrecision: highPrecision ?? this.highPrecision, tr0: tr0 ?? this.tr0, tr1: tr1 ?? this.tr1, tr2: tr2 ?? this.tr2, diff --git a/app/macos/Podfile.lock b/app/macos/Podfile.lock index 5f8b640..2d820e5 100644 --- a/app/macos/Podfile.lock +++ b/app/macos/Podfile.lock @@ -60,6 +60,6 @@ SPEC CHECKSUMS: url_launcher_macos: f87a979182d112f911de6820aefddaf56ee9fbfd window_manager: 1d01fa7ac65a6e6f83b965471b1a7fdd3f06166c -PODFILE CHECKSUM: fb89ba838adf1640583998715865bc112303ec7e +PODFILE CHECKSUM: 44b9a8eeddbb64e41bceb717e91b78eaf05a72c5 COCOAPODS: 1.16.2 diff --git a/app/macos/Runner/Info.plist b/app/macos/Runner/Info.plist index 876e032..556aa33 100644 --- a/app/macos/Runner/Info.plist +++ b/app/macos/Runner/Info.plist @@ -17,9 +17,9 @@ CFBundlePackageType APPL CFBundleShortVersionString - 0.9.10 + 0.9.11 CFBundleVersion - 0.9.10 + 0.9.11 LSMinimumSystemVersion $(MACOSX_DEPLOYMENT_TARGET) NSHumanReadableCopyright diff --git a/app/pubspec.yaml b/app/pubspec.yaml index 748d10a..4a55c9b 100644 --- a/app/pubspec.yaml +++ b/app/pubspec.yaml @@ -1,7 +1,7 @@ name: vapourbox description: "Video processing and cleanup powered by VapourSynth" publish_to: 'none' -version: 0.9.10+25 +version: 0.9.11+26 environment: sdk: ^3.6.2 diff --git a/app/test/integration_new_passes_test.dart b/app/test/integration_new_passes_test.dart index b0efb73..1dfdcb8 100644 --- a/app/test/integration_new_passes_test.dart +++ b/app/test/integration_new_passes_test.dart @@ -15,6 +15,7 @@ library; import 'dart:io'; import 'package:flutter_test/flutter_test.dart'; +import 'package:path/path.dart' as p; import 'package:uuid/uuid.dart'; import 'package:vapourbox/models/descratch_parameters.dart'; @@ -52,7 +53,13 @@ VideoJob _baseJob( /// Assert the worker produced a playable video with a video stream. Future _expectValidVideo(JobResult result) async { - expect(result.success, isTrue, reason: result.error); + // Include the tail of the worker log — "ffmpeg exited with -22" on its own + // says nothing about which arguments were rejected. + final tail = result.logs.length > 25 + ? result.logs.sublist(result.logs.length - 25) + : result.logs; + expect(result.success, isTrue, + reason: '${result.error}\n--- worker log (tail) ---\n${tail.join('\n')}'); expect(File(result.outputPath!).existsSync(), isTrue, reason: 'output file should exist'); final v = await WorkerHarness.firstStream(result.outputPath!, @@ -138,4 +145,122 @@ void main() { await _expectValidVideo(result); }, timeout: const Timeout(Duration(minutes: 5))); }); + + // =========================================================================== + // Issue #49: deinterlace working format, end-to-end. + // + // The script-generation assertions live in integration_qtgmc_parameters_test; + // these confirm the converted pipeline actually runs through vspipe | ffmpeg + // and — critically — that the working format never leaks into the encode. + // + // These use the 4:2:0 fixture rather than the shared 4:2:2 interlaced_test.avi: + // the chroma upsample is a deliberate no-op on 4:2:2 sources, so 4:2:2 would + // not exercise the path at all. + // =========================================================================== + group('Deinterlace working format (issue #49, full encode)', () { + final yuv420Input = p.join( + WorkerHarness.repoRoot, 'Tests', 'TestResources', 'pal-dvbt-fieldcoded-25i.ts'); + + VideoJob job420( + String name, { + bool? chromaUpsampleFix, + bool? highPrecision, + String? chromaEdi, + }) => + VideoJob( + id: const Uuid().v4(), + inputPath: yuv420Input, + outputPath: '$_outDir/$name.mkv', + // Supply the probe results the app always sends. ffprobe reports + // nb_frames=N/A for MPEG-TS, and the worker falls back to + // total_frames=1 / 720x480 / 29.97 when these are absent — which + // silently produces a one-frame output. + totalFrames: 126, + inputFrameRate: 25.0, + inputWidth: 720, + inputHeight: 576, + inputPixelFormat: 'yuv420p', + processingPipeline: ProcessingPipeline( + deinterlace: QTGMCParameters( + preset: QTGMCPreset.fast, + tff: true, + fpsDivisor: 2, + chromaUpsampleFix: chromaUpsampleFix, + highPrecision: highPrecision, + chromaEdi: chromaEdi, + ), + ), + encodingSettings: const EncodingSettings( + codec: VideoCodec.h264, + container: ContainerFormat.mkv, + audioMode: AudioMode.none, + ), + // Keep the encodes short — this group runs five of them. + startFrame: 0, + endFrame: 40, + ); + + Future runAndGetPixFmt(String name, JobResult result) async { + await _expectValidVideo(result); + final v = await WorkerHarness.firstStream(result.outputPath!, + selector: 'v:0', entries: ['pix_fmt']); + print(' $name pix_fmt: ${v?['pix_fmt']}'); + return v?['pix_fmt'] as String; + } + + setUpAll(() { + expect(File(yuv420Input).existsSync(), isTrue, + reason: '4:2:0 interlaced fixture missing: $yuv420Input'); + }); + + // The working format (4:2:2 and/or 16-bit) must be restored before the + // encode, so all three variants must produce the same output pixel format. + test('working format never leaks into the encoded output', () async { + final off = await runAndGetPixFmt( + 'deint_wf_off', + await WorkerHarness.runJob( + job420('deint_wf_off', chromaUpsampleFix: false, highPrecision: false) + .toJson(), + label: 'deint_wf_off')); + + // Both options are opt-in, so the chroma variant must ask for it. + final chroma = await runAndGetPixFmt( + 'deint_wf_chroma', + await WorkerHarness.runJob( + job420('deint_wf_chroma', chromaUpsampleFix: true).toJson(), + label: 'deint_wf_chroma')); + + final bits = await runAndGetPixFmt( + 'deint_wf_16bit', + await WorkerHarness.runJob( + job420('deint_wf_16bit', highPrecision: true).toJson(), + label: 'deint_wf_16bit')); + + expect(chroma, off, + reason: 'the 4:2:2 working format must be restored before encode'); + expect(bits, off, + reason: 'the 16-bit working format must be dithered back before encode'); + }, timeout: const Timeout(Duration(minutes: 15))); + + // havsfunc implements only ChromaEdi '' / 'nnedi3' / 'bob'. 'Blend' used to + // reach QTGMC and corrupt chroma; it is now dropped, so the run must be + // identical to one with no ChromaEdi set at all. + test('unsupported ChromaEdi value is dropped, not passed to QTGMC', () async { + final blend = await WorkerHarness.runJob( + job420('deint_chromaedi_blend', chromaEdi: 'Blend').toJson(), + label: 'chromaedi_blend'); + await _expectValidVideo(blend); + + final none = await WorkerHarness.runJob( + job420('deint_chromaedi_none').toJson(), + label: 'chromaedi_none'); + await _expectValidVideo(none); + + final blendHash = await WorkerHarness.frameHash(blend.outputPath!); + final noneHash = await WorkerHarness.frameHash(none.outputPath!); + expect(blendHash, noneHash, + reason: 'ChromaEdi="Blend" must be dropped, producing the default pipeline'); + print(' frame hash matches: $blendHash'); + }, timeout: const Timeout(Duration(minutes: 10))); + }); } diff --git a/app/test/integration_qtgmc_parameters_test.dart b/app/test/integration_qtgmc_parameters_test.dart index db3d550..93a2917 100644 --- a/app/test/integration_qtgmc_parameters_test.dart +++ b/app/test/integration_qtgmc_parameters_test.dart @@ -615,4 +615,146 @@ void main() { 'no advanced params leaked'); }, timeout: const Timeout(Duration(minutes: 2))); }); + + // =========================================================================== + // Issue #49: deinterlace working format + field-order derivation + // =========================================================================== + group('Deinterlace working format (issue #49)', () { + /// Build a QTGMC job with the given working-format options. + VideoJob buildJob(String name, + {bool? chromaUpsampleFix, bool? highPrecision, bool tff = true}) { + final params = QTGMCParameters( + enabled: true, + method: DeinterlaceMethod.qtgmc, + preset: QTGMCPreset.fast, + tff: tff, + fpsDivisor: 2, + chromaUpsampleFix: chromaUpsampleFix, + highPrecision: highPrecision, + ); + return VideoJob( + id: const Uuid().v4(), + inputPath: TestConfig.inputFile, + outputPath: '${TestConfig.outputDir}/$name.mkv', + qtgmcParameters: params, + processingPipeline: ProcessingPipeline(deinterlace: params), + encodingSettings: const EncodingSettings( + codec: VideoCodec.h264, + container: ContainerFormat.mkv, + audioMode: AudioMode.none, + ), + startFrame: 10, + endFrame: 30, + ); + } + + test('both working-format options are opt-in by default', () async { + // The 4:2:2 upsample costs ~30% throughput and 16-bit roughly doubles it, + // so the default script must be what it was before the block existed. + final script = await generateScriptViaWorker(buildJob('deint_wf_default')); + + expect(script, contains('haf.QTGMC(')); + for (final marker in ['_deint_src_format', '_deint_ss_h', '_deint_bits']) { + expect(script, isNot(contains(marker)), + reason: 'working-format conversion should be opt-in'); + } + + print(' PASS: no working-format conversion by default'); + }, timeout: const Timeout(Duration(minutes: 2))); + + test('chroma upsample is emitted when enabled, without 16-bit', () async { + // Interlaced 4:2:0 stores chroma per field, so the pass converts to + // 4:2:2 before QTGMC and restores the source format afterwards. + final script = await generateScriptViaWorker( + buildJob('deint_wf_chroma', chromaUpsampleFix: true)); + + expect(script, contains('_deint_src_format = clip.format')); + expect(script, contains('_deint_ss_h = 0')); + expect(script, contains('subsampling_h=_deint_ss_h')); + expect(script, contains('dither_type="error_diffusion"')); + + // The two options are independent. + expect(script, isNot(contains('_deint_bits = max(_deint_bits, 16)')), + reason: 'the chroma fix must not enable 16-bit as a side effect'); + + print(' PASS: chroma upsample emitted on its own'); + }, timeout: const Timeout(Duration(minutes: 2))); + + test('16-bit processing is emitted when enabled, without chroma upsample', + () async { + final script = await generateScriptViaWorker( + buildJob('deint_wf_16bit', highPrecision: true)); + + expect(script, contains('_deint_bits = max(_deint_bits, 16)')); + expect(script, contains('dither_type="error_diffusion"')); + expect(script, isNot(contains('_deint_ss_h = 0')), + reason: '16-bit must not enable the chroma upsample as a side effect'); + + print(' PASS: 16-bit working format emitted on its own'); + }, timeout: const Timeout(Duration(minutes: 2))); + + test('both options off generates no working-format conversion', () async { + final script = await generateScriptViaWorker(buildJob('deint_wf_off', + chromaUpsampleFix: false, highPrecision: false)); + + expect(script, contains('haf.QTGMC(')); + for (final marker in ['_deint_src_format', '_deint_ss_h', '_deint_bits']) { + expect(script, isNot(contains(marker)), + reason: 'no working-format conversion expected when both are off'); + } + + print(' PASS: no conversion when both options are off'); + }, timeout: const Timeout(Duration(minutes: 2))); + + test('_FieldBased follows the requested field order, not detection', + () async { + // std.SeparateFields ignores its tff argument whenever _FieldBased is + // set, so the property must agree with QTGMC's TFF or the user's choice + // is silently overridden. + final bff = await generateScriptViaWorker( + buildJob('deint_fieldorder_bff', tff: false)); + expect(bff, contains('core.std.SetFieldBased(clip, 1)')); + expect(bff, contains('TFF=False')); + + final tff = await generateScriptViaWorker( + buildJob('deint_fieldorder_tff', tff: true)); + expect(tff, contains('core.std.SetFieldBased(clip, 2)')); + expect(tff, contains('TFF=True')); + + print(' PASS: _FieldBased matches the requested field order'); + }, timeout: const Timeout(Duration(minutes: 2))); + + test('unsupported ChromaEdi values are dropped', () async { + // havsfunc implements only '' / 'nnedi3' / 'bob'. Anything else disables + // chroma EDI and never restores chroma, badly corrupting it. + const params = QTGMCParameters( + enabled: true, + method: DeinterlaceMethod.qtgmc, + preset: QTGMCPreset.fast, + tff: true, + fpsDivisor: 2, + chromaEdi: 'Blend', + ); + final job = VideoJob( + id: const Uuid().v4(), + inputPath: TestConfig.inputFile, + outputPath: '${TestConfig.outputDir}/deint_chromaedi.mkv', + qtgmcParameters: params, + processingPipeline: ProcessingPipeline(deinterlace: params), + encodingSettings: const EncodingSettings( + codec: VideoCodec.h264, + container: ContainerFormat.mkv, + audioMode: AudioMode.none, + ), + startFrame: 10, + endFrame: 30, + ); + + final script = await generateScriptViaWorker(job); + expect(script, isNot(contains('ChromaEdi=')), + reason: 'unsupported ChromaEdi values must be dropped'); + + print(' PASS: ChromaEdi="Blend" dropped'); + }, timeout: const Timeout(Duration(minutes: 2))); + }); } diff --git a/app/test/vapoursynth_integration_test.dart b/app/test/vapoursynth_integration_test.dart index 4a0ebd3..31c7006 100644 --- a/app/test/vapoursynth_integration_test.dart +++ b/app/test/vapoursynth_integration_test.dart @@ -324,6 +324,127 @@ clip.set_output() expect(result.exitCode, 0, reason: 'stderr: ${result.stderr}'); }); }); + + // Regression guards for the QTGMC "Very Slow / Placebo brighten the picture on + // Apple Silicon" bug (and its worse sibling, a near-black Draft preset). + // + // Root cause was in fmtconv: 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 clamped 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 (macos-arm64, linux-arm64): 8-bit on the + // vertical axis, and 10/12-bit on BOTH axes — 8-bit horizontal escaped only + // because that route converts to 16-bit before the scaler. It broke havsfunc's + // Bob(), which feeds QTGMC's noise pass (Placebo/Very Slow default + // NoiseProcess=2) and the Draft preset's interpolation. + // + // Two layers now guard it, and these tests cover both: + // 1. Scripts/patches/fmtconv-r31-arm-int-scaler.patch, applied when the deps + // scripts build fmtconv from source (macOS/Linux) — the actual fix. + // 2. havsfunc "Patch 5" in download-deps-*, which makes Bob() resample at + // 16-bit — belt and braces, and it also covers the prebuilt Windows DLL. + // + // All three assert observable pixel behaviour rather than patch text, so they + // stay valid however the fix is delivered — including if fmtconv fixes this + // upstream and both patches are eventually dropped. + group('fmtconv vertical resample / havsfunc Bob', () { + test('fmtc.resample scales vertically at sub-16-bit depths', () async { + final script = ''' +import vapoursynth as vs +core = vs.core + +# Flat mid-grey must survive a 2x vertical resample. Before the fix this came +# back as a pure black plane for every depth below 16. Note fmtconv converts +# depth by a power-of-two shift, so 8-bit 120 becomes 120 * 256 = 30720, which +# reads back as 119.53 in 8-bit terms rather than 120.0. +for bits, fmt in ((8, vs.GRAY8), (10, vs.GRAY10), (12, vs.GRAY12), (16, vs.GRAY16)): + peak = (1 << bits) - 1 + val = round(120 / 255 * peak) + src = core.std.BlankClip(format=fmt, width=160, height=120, length=2, color=[val]) + expected = val / peak * 255 + for label, kw in (("scalev=2", dict(scalev=2)), ("scalev=0.5", dict(scalev=0.5))): + out = core.fmtc.resample(src, **kw) + avg = core.std.PlaneStats(out).get_frame(0).props['PlaneStatsAverage'] * 255 + print(f"CHECK bits={bits} {label} avg={avg:.3f} expected={expected:.3f}") + if abs(avg - expected) > 1.0: + raise Exception(f"fmtc.resample {label} broken at {bits}-bit: " + f"got {avg:.3f}, expected ~{expected:.3f}") +print("fmtc vertical resample OK") +'''; + + final result = + await _runVspipeScript(vspipePath, script, depsDir: depsDir); + expect(result.exitCode, 0, reason: 'stderr: ${result.stderr}'); + expect(result.stdout.toString(), contains('fmtc vertical resample OK')); + }); + + test('havsfunc Bob preserves average luma', () async { + final script = ''' +import vapoursynth as vs +import havsfunc as haf +core = vs.core + +# Bob() doubles the frame rate by interpolating each field to a full frame, so +# average luma must be preserved. Before the fix it collapsed towards black. +src = core.std.BlankClip(format=vs.YUV420P8, width=160, height=120, + length=4, color=[120, 128, 128]) +src = core.std.SetFieldBased(src, 2) # TFF +bobbed = haf.Bob(src, 0, 1, True) + +s = core.std.PlaneStats(bobbed, plane=0) +avgs = [s.get_frame(i).props['PlaneStatsAverage'] * 255 for i in range(len(bobbed))] +worst = max(abs(a - 120) for a in avgs) +print(f"CHECK frames={len(bobbed)} worst_deviation={worst:.3f}") +if len(bobbed) != 2 * len(src): + raise Exception(f"Bob should double the frame count, got {len(bobbed)}") +if worst > 1.0: + raise Exception(f"Bob() shifted luma by {worst:.3f}; expected ~0 " + f"(fmtconv vertical resample regression?)") +print("havsfunc Bob OK") +'''; + + final result = + await _runVspipeScript(vspipePath, script, depsDir: depsDir); + expect(result.exitCode, 0, reason: 'stderr: ${result.stderr}'); + expect(result.stdout.toString(), contains('havsfunc Bob OK')); + }); + + test('QTGMC Very Slow and Draft do not shift luma', () async { + // Very Slow is the cheapest preset that defaults NoiseProcess=2 (Placebo + // is the same code path but far slower); Draft exercises EdiMode='bob'. + final script = ''' +import vapoursynth as vs +import havsfunc as haf +core = vs.core + +src = core.std.BlankClip(format=vs.YUV420P8, width=160, height=120, + length=6, color=[120, 128, 128]) +# Give the denoiser some detail to chew on, otherwise a flat clip can mask a +# bias that only shows up once MakeDiff has something to clip. +src = core.grain.Add(src, var=8, seed=1) +src = core.std.SetFieldBased(src, 2) # TFF + +base = sum(core.std.PlaneStats(src, plane=0).get_frame(i) + .props['PlaneStatsAverage'] for i in range(len(src))) / len(src) * 255 + +for preset in ('Fast', 'Very Slow', 'Draft'): + out = haf.QTGMC(src, Preset=preset, TFF=True, FPSDivisor=1) + st = core.std.PlaneStats(out, plane=0) + avg = sum(st.get_frame(i).props['PlaneStatsAverage'] + for i in range(len(out))) / len(out) * 255 + print(f"CHECK preset={preset} luma={avg:.3f} base={base:.3f} delta={avg - base:+.3f}") + if abs(avg - base) > 2.0: + raise Exception(f"QTGMC Preset={preset} shifted luma by {avg - base:+.3f} " + f"(expected ~0)") +print("QTGMC preset luma OK") +'''; + + final result = + await _runVspipeScript(vspipePath, script, depsDir: depsDir); + expect(result.exitCode, 0, reason: 'stderr: ${result.stderr}'); + expect(result.stdout.toString(), contains('QTGMC preset luma OK')); + }, timeout: const Timeout(Duration(minutes: 5))); + }); } /// Builds a VapourSynth script that uses pipe_source to read raw video from stdin. diff --git a/app/windows/runner/Runner.rc b/app/windows/runner/Runner.rc index 5b64293..0b317e2 100644 --- a/app/windows/runner/Runner.rc +++ b/app/windows/runner/Runner.rc @@ -73,8 +73,8 @@ IDI_APP_ICON ICON "resources\\app_icon.ico" #endif VS_VERSION_INFO VERSIONINFO - FILEVERSION 0,9,10,0 - PRODUCTVERSION 0,9,10,0 + FILEVERSION 0,9,11,0 + PRODUCTVERSION 0,9,11,0 FILEFLAGSMASK VS_FFI_FILEFLAGSMASK #ifdef _DEBUG FILEFLAGS VS_FF_DEBUG diff --git a/worker/Cargo.toml b/worker/Cargo.toml index c779f5e..106e563 100644 --- a/worker/Cargo.toml +++ b/worker/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "vapourbox-worker" -version = "0.9.10" +version = "0.9.11" edition = "2021" description = "Video processing worker using VapourSynth" authors = ["Stuart Cameron"] diff --git a/worker/src/models/qtgmc_parameters.rs b/worker/src/models/qtgmc_parameters.rs index 72f031a..4423b2a 100644 --- a/worker/src/models/qtgmc_parameters.rs +++ b/worker/src/models/qtgmc_parameters.rs @@ -48,6 +48,21 @@ pub struct QTGMCParameters { #[serde(default, skip_serializing_if = "Option::is_none")] pub fps_divisor: Option, + // === Working Format (issue #49) === + /// Upsample 4:2:0 chroma to 4:2:2 with field-aware resampling before + /// deinterlacing, and restore the source format afterwards. Interlaced + /// 4:2:0 stores chroma per field, so interpolating it at 4:2:0 mixes the + /// two fields' chroma. Costs roughly 30% throughput (measured 35 -> 24 fps), + /// so `None` = disabled and the quality/speed trade-off is the user's call. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub chroma_upsample_fix: Option, + + /// Run the deinterlace pass at 16-bit and dither back afterwards. QTGMC + /// performs many merge/expr steps that round at the working depth. Costs + /// roughly 2x time and memory, so `None` = disabled. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub high_precision: Option, + // === Quality (Temporal Radius) === /// Temporal radius for pre-filtering (0-2) #[serde(skip_serializing_if = "Option::is_none")] @@ -391,6 +406,44 @@ pub struct QTGMCParameters { // Default value functions fn default_true() -> bool { true } +/// ChromaEdi values havsfunc's QTGMC actually implements. Any other non-empty +/// string disables chroma EDI (`planes=[0]`) and then returns the luma-only +/// interpolation without ever restoring chroma, producing badly broken chroma +/// (issue #49) — so unsupported values are dropped rather than passed through. +const SUPPORTED_CHROMA_EDI: [&str; 2] = ["nnedi3", "bob"]; + +impl QTGMCParameters { + /// Whether to upsample 4:2:0 chroma to 4:2:2 around the deinterlace pass. + /// Defaults to disabled: it is the more correct way to handle interlaced + /// 4:2:0 chroma, but it costs roughly 30% throughput, so it is offered as + /// an option rather than imposed. + pub fn chroma_upsample_fix_enabled(&self) -> bool { + self.chroma_upsample_fix.unwrap_or(false) + } + + /// Whether to run the deinterlace pass at 16-bit. Defaults to disabled + /// because it roughly doubles processing time. + pub fn high_precision_enabled(&self) -> bool { + self.high_precision.unwrap_or(false) + } + + /// `chroma_edi` restricted to the values havsfunc implements, lowercased + /// the way QTGMC itself does. + /// + /// The empty string is passed through — it is QTGMC's own default and means + /// "interpolate chroma with the main EDI". Unsupported non-empty values + /// return `None` so the parameter is omitted entirely rather than sent + /// through to the broken code path. + pub fn normalized_chroma_edi(&self) -> Option { + let value = self.chroma_edi.as_deref()?.trim().to_lowercase(); + if value.is_empty() || SUPPORTED_CHROMA_EDI.contains(&value.as_str()) { + Some(value) + } else { + None + } + } +} + impl Default for QTGMCParameters { fn default() -> Self { Self { @@ -400,6 +453,8 @@ impl Default for QTGMCParameters { input_type: None, tff: None, fps_divisor: None, + chroma_upsample_fix: None, + high_precision: None, tr0: None, tr1: None, tr2: None, diff --git a/worker/src/pipeline_executor.rs b/worker/src/pipeline_executor.rs index fb40605..7c60a51 100644 --- a/worker/src/pipeline_executor.rs +++ b/worker/src/pipeline_executor.rs @@ -936,12 +936,9 @@ impl PipelineExecutor { target, first, target + radius, local, output_index, seek_time, width, height, pix_fmt ); - // Determine field order for interlaced content - let field_based = if job.qtgmc_parameters.tff == Some(true) { - 2 // TFF - } else { - 1 // BFF - }; + // Field order is derived inside the script generator (see + // ScriptGenerator::field_based_for) so the preview and the encode can + // never disagree about it — issue #49. // FPS as rational let script_generator = ScriptGenerator::new()? @@ -956,7 +953,6 @@ impl PipelineExecutor { num_frames, fps_num, fps_den, - field_based, output_index, }; diff --git a/worker/src/script_generator.rs b/worker/src/script_generator.rs index 9a6bbdf..20bce0f 100644 --- a/worker/src/script_generator.rs +++ b/worker/src/script_generator.rs @@ -11,6 +11,7 @@ use anyhow::{Context, Result}; use crate::models::{ VideoJob, ProcessingPipeline, NoiseReductionMethod, ResizeKernel, UpscaleMethod, DehaloMethod, DeblockMethod, SharpenMethod, ChromaSubsampling, DeinterlaceMethod, + FieldOrder, }; /// Generates VapourSynth scripts from templates. @@ -44,8 +45,6 @@ pub struct PreviewParams { pub fps_num: i32, /// FPS denominator pub fps_den: i32, - /// Field order: 1 = BFF, 2 = TFF - pub field_based: i32, /// Output frame index to emit as the preview (after the pipeline runs on /// the decoded window). Accounts for any frame-rate change in the pipeline. pub output_index: i32, @@ -111,11 +110,12 @@ impl ScriptGenerator { script = script.replace("{{FPS_NUM}}", &preview_params.fps_num.to_string()); script = script.replace("{{FPS_DEN}}", &preview_params.fps_den.to_string()); script = script.replace("{{PREVIEW_OUTPUT_INDEX}}", &preview_params.output_index.to_string()); - // Field-based: only set when deinterlacing is enabled - if pipeline.deinterlace.enabled { + // Field-based: same derivation as the encode path, so the preview can't + // deinterlace at a different field order than the final render. + if let Some(fb) = Self::field_based_for(job, &pipeline) { script = script.replace("{{#SET_FIELD_BASED}}", ""); script = script.replace("{{/SET_FIELD_BASED}}", ""); - script = script.replace("{{FIELD_BASED}}", &preview_params.field_based.to_string()); + script = script.replace("{{FIELD_BASED}}", &fb.to_string()); } else { script = remove_block("{{#SET_FIELD_BASED}}", "{{/SET_FIELD_BASED}}", script); } @@ -182,6 +182,30 @@ impl ScriptGenerator { ) } + /// The `_FieldBased` value to mark the source clip with, or `None` to leave + /// it unmarked. + /// + /// This is the single derivation used by both the encode and the preview + /// path. It must stay that way: `std.SeparateFields` ignores its `tff` + /// argument whenever `_FieldBased` is set, so the property — not QTGMC's + /// `TFF=` parameter — decides the field order that is actually used. Two + /// derivations that can disagree means the user's field-order choice can be + /// silently overridden, and preview can disagree with the encode (issue #49). + pub fn field_based_for(job: &VideoJob, pipeline: &ProcessingPipeline) -> Option { + if pipeline.deinterlace.enabled { + // Deinterlacing: the pass's own field order is authoritative. + Some(if pipeline.deinterlace.tff.unwrap_or(true) { 2 } else { 1 }) + } else { + // Not deinterlacing: still mark what was detected, so field-aware + // chroma resampling in any later resize stays correct. + match job.detected_field_order { + Some(FieldOrder::TopFieldFirst) => Some(2), + Some(FieldOrder::BottomFieldFirst) => Some(1), + _ => None, + } + } + } + /// Substitute parameters in a script string. fn substitute_parameters(&self, template: &str, job: &VideoJob, pipeline: &ProcessingPipeline, _input_path: &str) -> String { let mut script = template.to_string(); @@ -206,19 +230,15 @@ impl ScriptGenerator { script = script.replace("{{INPUT_FPS_NUM}}", &fps_num.to_string()); script = script.replace("{{INPUT_FPS_DEN}}", &fps_den.to_string()); - // Field order — set from detected field order or TFF parameter - let field_based = match job.detected_field_order { - Some(crate::models::FieldOrder::TopFieldFirst) => Some(2), - Some(crate::models::FieldOrder::BottomFieldFirst) => Some(1), - _ => { - // Fall back to QTGMC TFF parameter - if pipeline.deinterlace.enabled { - Some(if pipeline.deinterlace.tff.unwrap_or(true) { 2 } else { 1 }) - } else { - None - } - } - }; + // Field order. + // + // When deinterlacing, this MUST come from the same value as the + // deinterlacer's TFF argument: std.SeparateFields ignores its `tff` + // argument whenever _FieldBased is set, so deriving the two from + // different sources lets the property silently override the user's + // field-order choice (issue #49). `field_based_for` is the single + // derivation shared with the preview path. + let field_based = ScriptGenerator::field_based_for(job, pipeline); if let Some(fb) = field_based { script = script.replace("{{#SET_FIELD_BASED}}", ""); script = script.replace("{{/SET_FIELD_BASED}}", ""); @@ -323,6 +343,34 @@ impl ScriptGenerator { script = remove_block("{{#DEINT_IVTC}}", "{{/DEINT_IVTC}}", script); script = remove_block("{{#DEINT_SOFT_TELECINE}}", "{{/DEINT_SOFT_TELECINE}}", script); + // Working format around the QTGMC call (issue #49): 4:2:2 + // chroma and/or 16-bit, restored to the source format after. + // Only emitted when at least one of them is on, so the + // common case generates exactly the script it did before. + let chroma_fix = params.chroma_upsample_fix_enabled(); + let high_precision = params.high_precision_enabled(); + if chroma_fix || high_precision { + script = script.replace("{{#DEINT_WORKING_FORMAT}}", ""); + script = script.replace("{{/DEINT_WORKING_FORMAT}}", ""); + script = script.replace("{{#DEINT_RESTORE_FORMAT}}", ""); + script = script.replace("{{/DEINT_RESTORE_FORMAT}}", ""); + if chroma_fix { + script = script.replace("{{#DEINT_WF_CHROMA}}", ""); + script = script.replace("{{/DEINT_WF_CHROMA}}", ""); + } else { + script = remove_block("{{#DEINT_WF_CHROMA}}", "{{/DEINT_WF_CHROMA}}", script); + } + if high_precision { + script = script.replace("{{#DEINT_WF_BITS}}", ""); + script = script.replace("{{/DEINT_WF_BITS}}", ""); + } else { + script = remove_block("{{#DEINT_WF_BITS}}", "{{/DEINT_WF_BITS}}", script); + } + } else { + script = remove_block("{{#DEINT_WORKING_FORMAT}}", "{{/DEINT_WORKING_FORMAT}}", script); + script = remove_block("{{#DEINT_RESTORE_FORMAT}}", "{{/DEINT_RESTORE_FORMAT}}", script); + } + // Preset (required) script = script.replace("{{PRESET}}", params.preset.as_str()); @@ -346,7 +394,9 @@ impl ScriptGenerator { script = process_optional_int("NN_NEURONS", params.nn_neurons, script); script = process_optional_int("EDI_QUAL", params.edi_qual, script); script = process_optional_int("EDI_MAX_D", params.edi_max_d, script); - script = process_optional_string("CHROMA_EDI", params.chroma_edi.as_deref(), script); + // Only values havsfunc implements — anything else silently + // breaks chroma (issue #49). See normalized_chroma_edi. + script = process_optional_string("CHROMA_EDI", params.normalized_chroma_edi().as_deref(), script); // Motion analysis script = process_optional_int("BLOCK_SIZE", params.block_size, script); diff --git a/worker/templates/pipeline_template.vpy b/worker/templates/pipeline_template.vpy index 3a76a81..480ed06 100644 --- a/worker/templates/pipeline_template.vpy +++ b/worker/templates/pipeline_template.vpy @@ -61,6 +61,29 @@ clip = core.std.Crop(clip, left={{CROP_LEFT}}, right={{CROP_RIGHT}}, top={{CROP_ # ============================================================================ {{#DEINTERLACE}} {{#DEINT_QTGMC}} +{{#DEINT_WORKING_FORMAT}} +# Working format for the deinterlace pass (issue #49). +# - Interlaced 4:2:0 stores chroma per field, so interpolating it at 4:2:0 +# mixes the two fields' chroma. Upsampling to 4:2:2 first is field-aware +# because zimg honours the _FieldBased prop set above. +# - QTGMC runs many merge/expr steps that round at the working depth, so 16-bit +# avoids accumulating 8-bit rounding error. +# The source format is restored immediately after the pass, so nothing +# downstream sees a different format than it did before. +_deint_src_format = clip.format +if clip.format.color_family == vs.YUV: + _deint_ss_h = clip.format.subsampling_h + _deint_bits = clip.format.bits_per_sample +{{#DEINT_WF_CHROMA}} + _deint_ss_h = 0 +{{/DEINT_WF_CHROMA}} +{{#DEINT_WF_BITS}} + _deint_bits = max(_deint_bits, 16) +{{/DEINT_WF_BITS}} + if _deint_ss_h != clip.format.subsampling_h or _deint_bits != clip.format.bits_per_sample: + clip = core.resize.Bicubic(clip, format=clip.format.replace( + subsampling_h=_deint_ss_h, bits_per_sample=_deint_bits).id) +{{/DEINT_WORKING_FORMAT}} clip = haf.QTGMC( clip, Preset="{{PRESET}}", @@ -293,6 +316,13 @@ clip = haf.QTGMC( device={{DEVICE}}, {{/DEVICE}} ) +{{#DEINT_RESTORE_FORMAT}} +# Back to the source format. QTGMC has already reset _FieldBased to 0, so this +# downsample is progressive — which is correct for the now-deinterlaced frames. +if clip.format.id != _deint_src_format.id: + clip = core.resize.Bicubic(clip, format=_deint_src_format.id, + dither_type="error_diffusion") +{{/DEINT_RESTORE_FORMAT}} {{/DEINT_QTGMC}} {{#DEINT_IVTC}} # IVTC: Inverse Telecine (VFM field matching + VDecimate) diff --git a/worker/templates/preview_template.vpy b/worker/templates/preview_template.vpy index 23f9dc2..3a7394a 100644 --- a/worker/templates/preview_template.vpy +++ b/worker/templates/preview_template.vpy @@ -53,6 +53,23 @@ clip = core.std.Crop(clip, left={{CROP_LEFT}}, right={{CROP_RIGHT}}, top={{CROP_ # ============================================================================ {{#DEINTERLACE}} {{#DEINT_QTGMC}} +{{#DEINT_WORKING_FORMAT}} +# Working format for the deinterlace pass (issue #49) — see pipeline_template.vpy. +# The preview must apply the same conversion as the encode or it won't match. +_deint_src_format = clip.format +if clip.format.color_family == vs.YUV: + _deint_ss_h = clip.format.subsampling_h + _deint_bits = clip.format.bits_per_sample +{{#DEINT_WF_CHROMA}} + _deint_ss_h = 0 +{{/DEINT_WF_CHROMA}} +{{#DEINT_WF_BITS}} + _deint_bits = max(_deint_bits, 16) +{{/DEINT_WF_BITS}} + if _deint_ss_h != clip.format.subsampling_h or _deint_bits != clip.format.bits_per_sample: + clip = core.resize.Bicubic(clip, format=clip.format.replace( + subsampling_h=_deint_ss_h, bits_per_sample=_deint_bits).id) +{{/DEINT_WORKING_FORMAT}} clip = haf.QTGMC( clip, Preset="{{PRESET}}", @@ -270,6 +287,11 @@ clip = haf.QTGMC( device={{DEVICE}}, {{/DEVICE}} ) +{{#DEINT_RESTORE_FORMAT}} +if clip.format.id != _deint_src_format.id: + clip = core.resize.Bicubic(clip, format=_deint_src_format.id, + dither_type="error_diffusion") +{{/DEINT_RESTORE_FORMAT}} {{/DEINT_QTGMC}} {{#DEINT_IVTC}} # IVTC Preview: VFM field matching only (no VDecimate for single-frame preview) diff --git a/worker/tests/filter_integration_test.rs b/worker/tests/filter_integration_test.rs index 63026d2..bde9414 100644 --- a/worker/tests/filter_integration_test.rs +++ b/worker/tests/filter_integration_test.rs @@ -1912,6 +1912,9 @@ fn test_55_preview_selects_exact_frame() { let mut job = create_base_job("test_55_preview_frame"); job.qtgmc_parameters.enabled = true; job.qtgmc_parameters.tff = Some(true); + // Opt into the chroma upsample so the preview/encode parity assertions + // below have something to compare. + job.qtgmc_parameters.chroma_upsample_fix = Some(true); job.processing_pipeline = Some(ProcessingPipeline { deinterlace: job.qtgmc_parameters.clone(), ..ProcessingPipeline::default() @@ -1925,7 +1928,6 @@ fn test_55_preview_selects_exact_frame() { num_frames: 11, fps_num: 25, fps_den: 1, - field_based: 2, output_index: 7, }; let script_path = generator @@ -1942,6 +1944,17 @@ fn test_55_preview_selects_exact_frame() { !script.contains("num_frames // 2"), "preview must not fall back to the old middle-frame heuristic" ); + // The preview must apply the same field marking and working-format + // conversion as the encode, or it won't represent the final render (#49). + assert!( + script.contains("core.std.SetFieldBased(clip, 2)"), + "preview must mark field order the same way the encode does" + ); + assert!( + script.contains("_deint_src_format = clip.format") + && script.contains("_deint_ss_h = 0"), + "preview must apply the same deinterlace working format as the encode" + ); let _ = std::fs::remove_file(&script_path); } @@ -2000,3 +2013,248 @@ fn test_57_ivtc_high_bit_depth_guard() { "core.vivtc.VDecimate(clip", ]).unwrap(); } + +#[test] +fn test_58_deinterlace_working_format_default() { + // Both working-format options are opt-in: the 4:2:2 chroma upsample costs + // roughly 30% throughput and 16-bit roughly doubles it, so neither is + // imposed. The default script must therefore be exactly what it was before + // the working-format block existed. + create_output_dir(); + + let mut job = create_base_job("test_58_deint_working_format_default"); + job.qtgmc_parameters = QTGMCParameters { + enabled: true, + preset: QTGMCPreset::Fast, + tff: Some(true), + ..QTGMCParameters::default() + }; + job.processing_pipeline = Some(ProcessingPipeline { + deinterlace: job.qtgmc_parameters.clone(), + ..ProcessingPipeline::default() + }); + + let generator = ScriptGenerator::new().unwrap(); + let script = std::fs::read_to_string(generator.generate(&job).unwrap()).unwrap(); + for marker in [ + "_deint_src_format", + "_deint_ss_h", + "_deint_bits", + "DEINT_WORKING_FORMAT", + ] { + assert!(!script.contains(marker), "expected no '{}' by default", marker); + } + assert!(script.contains("haf.QTGMC("), "QTGMC call should still be generated"); +} + +#[test] +fn test_58b_deinterlace_chroma_upsample_opt_in() { + // Issue #49: interlaced 4:2:0 stores chroma per field, so interpolating it + // at 4:2:0 mixes the two fields' chroma. When the option is enabled the + // pass converts to 4:2:2 (field-aware, since zimg honours _FieldBased) and + // restores the source format afterwards, without touching the bit depth. + create_output_dir(); + + let mut job = create_base_job("test_58b_deint_chroma_upsample"); + job.qtgmc_parameters = QTGMCParameters { + enabled: true, + preset: QTGMCPreset::Fast, + tff: Some(true), + chroma_upsample_fix: Some(true), + ..QTGMCParameters::default() + }; + job.processing_pipeline = Some(ProcessingPipeline { + deinterlace: job.qtgmc_parameters.clone(), + ..ProcessingPipeline::default() + }); + + run_job_and_verify(&job, "Deinterlace Chroma Upsample (opt-in)", &[ + "_deint_src_format = clip.format", + "_deint_ss_h = 0", + "subsampling_h=_deint_ss_h", + "dither_type=\"error_diffusion\"", + ]).unwrap(); + + // Enabling the chroma fix must not drag 16-bit along with it. + let generator = ScriptGenerator::new().unwrap(); + let script = std::fs::read_to_string(generator.generate(&job).unwrap()).unwrap(); + assert!( + !script.contains("_deint_bits = max(_deint_bits, 16)"), + "16-bit processing should stay off unless separately enabled" + ); +} + +#[test] +fn test_59_deinterlace_high_precision() { + // 16-bit on its own: the bit depth is raised and dithered back, and the + // chroma subsampling is left alone because the two options are independent. + create_output_dir(); + + let mut job = create_base_job("test_59_deint_high_precision"); + job.qtgmc_parameters = QTGMCParameters { + enabled: true, + preset: QTGMCPreset::Fast, + tff: Some(true), + high_precision: Some(true), + ..QTGMCParameters::default() + }; + job.processing_pipeline = Some(ProcessingPipeline { + deinterlace: job.qtgmc_parameters.clone(), + ..ProcessingPipeline::default() + }); + + run_job_and_verify(&job, "Deinterlace Working Format (16-bit)", &[ + "_deint_src_format = clip.format", + "_deint_bits = max(_deint_bits, 16)", + "dither_type=\"error_diffusion\"", + ]).unwrap(); + + let generator = ScriptGenerator::new().unwrap(); + let script = std::fs::read_to_string(generator.generate(&job).unwrap()).unwrap(); + assert!( + !script.contains("_deint_ss_h = 0"), + "16-bit must not enable the chroma upsample as a side effect" + ); +} + +#[test] +fn test_59b_deinterlace_both_working_format_options() { + // Both on: 4:2:2 and 16-bit, restored to the source format afterwards. + create_output_dir(); + + let mut job = create_base_job("test_59b_deint_both_options"); + job.qtgmc_parameters = QTGMCParameters { + enabled: true, + preset: QTGMCPreset::Fast, + tff: Some(true), + chroma_upsample_fix: Some(true), + high_precision: Some(true), + ..QTGMCParameters::default() + }; + job.processing_pipeline = Some(ProcessingPipeline { + deinterlace: job.qtgmc_parameters.clone(), + ..ProcessingPipeline::default() + }); + + run_job_and_verify(&job, "Deinterlace Working Format (4:2:2 + 16-bit)", &[ + "_deint_ss_h = 0", + "_deint_bits = max(_deint_bits, 16)", + "dither_type=\"error_diffusion\"", + ]).unwrap(); +} + +#[test] +fn test_60_deinterlace_working_format_disabled() { + // Both off: the script must be exactly what it was before the working-format + // block existed, so the conversion can't cost anything when it's not wanted. + create_output_dir(); + + let mut job = create_base_job("test_60_deint_working_format_off"); + job.qtgmc_parameters = QTGMCParameters { + enabled: true, + preset: QTGMCPreset::Fast, + tff: Some(true), + chroma_upsample_fix: Some(false), + high_precision: Some(false), + ..QTGMCParameters::default() + }; + job.processing_pipeline = Some(ProcessingPipeline { + deinterlace: job.qtgmc_parameters.clone(), + ..ProcessingPipeline::default() + }); + + let generator = ScriptGenerator::new().unwrap(); + let script = std::fs::read_to_string(generator.generate(&job).unwrap()).unwrap(); + for marker in ["_deint_src_format", "_deint_ss_h", "_deint_bits", "DEINT_WORKING_FORMAT"] { + assert!(!script.contains(marker), "expected no '{}' when both options are off", marker); + } + assert!(script.contains("haf.QTGMC("), "QTGMC call should still be generated"); +} + +#[test] +fn test_61_chroma_edi_rejects_unsupported_value() { + // Issue #49: havsfunc implements only ChromaEdi '' / 'nnedi3' / 'bob'. Any + // other non-empty value disables chroma EDI and then returns the luma-only + // interpolation without restoring chroma, which badly corrupts chroma. Such + // values must be dropped so QTGMC falls back to its default. + create_output_dir(); + + let mut base = create_base_job("test_61_chroma_edi_guard"); + base.qtgmc_parameters = QTGMCParameters { + enabled: true, + preset: QTGMCPreset::Fast, + tff: Some(true), + ..QTGMCParameters::default() + }; + + let generator = ScriptGenerator::new().unwrap(); + let render = |chroma_edi: Option<&str>| { + let mut job = base.clone(); + job.qtgmc_parameters.chroma_edi = chroma_edi.map(str::to_string); + job.processing_pipeline = Some(ProcessingPipeline { + deinterlace: job.qtgmc_parameters.clone(), + ..ProcessingPipeline::default() + }); + std::fs::read_to_string(generator.generate(&job).unwrap()).unwrap() + }; + + // Supported values pass through, lowercased the way QTGMC lowercases them. + assert!(render(Some("NNEDI3")).contains("ChromaEdi=\"nnedi3\"")); + assert!(render(Some("Bob")).contains("ChromaEdi=\"bob\"")); + + // The empty string is QTGMC's own default and stays. + for value in ["", " "] { + assert!( + render(Some(value)).contains("ChromaEdi=\"\""), + "ChromaEdi={:?} should pass through as the default", + value + ); + } + + // Unsupported values are omitted entirely. + for value in ["Blend", "nonsense"] { + assert!( + !render(Some(value)).contains("ChromaEdi="), + "ChromaEdi={:?} should have been dropped", + value + ); + } +} + +#[test] +fn test_62_field_based_follows_deinterlace_tff() { + // Issue #49: std.SeparateFields ignores its `tff` argument whenever + // _FieldBased is set, so the property decides the field order that is + // actually used. It must therefore be derived from the same value as + // QTGMC's TFF argument — otherwise autodetection silently overrides the + // user's field-order choice. + create_output_dir(); + + let generator = ScriptGenerator::new().unwrap(); + let render = |tff: bool, detected: Option| { + let mut job = create_base_job("test_62_field_based"); + job.detected_field_order = detected; + job.qtgmc_parameters = QTGMCParameters { + enabled: true, + preset: QTGMCPreset::Fast, + tff: Some(tff), + ..QTGMCParameters::default() + }; + job.processing_pipeline = Some(ProcessingPipeline { + deinterlace: job.qtgmc_parameters.clone(), + ..ProcessingPipeline::default() + }); + std::fs::read_to_string(generator.generate(&job).unwrap()).unwrap() + }; + + // BFF requested while detection says TFF: the property must say BFF (1), + // matching TFF=False, not follow the detected order. + let script = render(false, Some(FieldOrder::TopFieldFirst)); + assert!(script.contains("core.std.SetFieldBased(clip, 1)"), "expected BFF marking"); + assert!(script.contains("TFF=False"), "expected TFF=False"); + + // And the reverse. + let script = render(true, Some(FieldOrder::BottomFieldFirst)); + assert!(script.contains("core.std.SetFieldBased(clip, 2)"), "expected TFF marking"); + assert!(script.contains("TFF=True"), "expected TFF=True"); +}