From c9e97ead5ec76513d0473a21a215ed6ecd2223ec Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Fri, 31 Jul 2026 16:47:10 +1000 Subject: [PATCH 1/4] fix(deinterlace): interlaced chroma handling and field-order derivation (#49) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CLAUDE.md | 39 +++- app/assets/filters/core/deinterlace.json | 37 +++- app/lib/models/parameter_converter.dart | 6 + app/lib/models/qtgmc_parameters.dart | 14 ++ app/test/integration_new_passes_test.dart | 126 +++++++++++- .../integration_qtgmc_parameters_test.dart | 126 ++++++++++++ worker/src/models/qtgmc_parameters.rs | 52 +++++ worker/src/pipeline_executor.rs | 10 +- worker/src/script_generator.rs | 88 ++++++-- worker/templates/pipeline_template.vpy | 30 +++ worker/templates/preview_template.vpy | 22 ++ worker/tests/filter_integration_test.rs | 191 +++++++++++++++++- 12 files changed, 709 insertions(+), 32 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ce06904..1152984 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,43 @@ 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): + +- **`chromaUpsampleFix`** (default **on**): interlaced 4:2:0 stores chroma per + field, so convert to 4:2:2 before deinterlacing — field-aware, because zimg + honours `_FieldBased`. 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. + +With both off, the generated script is byte-for-byte what it was before the +block existed (asserted by `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 diff --git a/app/assets/filters/core/deinterlace.json b/app/assets/filters/core/deinterlace.json index 1918f8b..d0d8c15 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": true, + "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. 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..96ff614 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 ?? true, + '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..c05b7a5 100644 --- a/app/lib/models/qtgmc_parameters.dart +++ b/app/lib/models/qtgmc_parameters.dart @@ -133,6 +133,14 @@ 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. Null = enabled. + 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 +268,8 @@ class QTGMCParameters { this.inputType, this.tff, this.fpsDivisor, + this.chromaUpsampleFix, + this.highPrecision, this.tr0, this.tr1, this.tr2, @@ -355,6 +365,8 @@ class QTGMCParameters { int? inputType, bool? tff, int? fpsDivisor, + bool? chromaUpsampleFix, + bool? highPrecision, int? tr0, int? tr1, int? tr2, @@ -445,6 +457,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/test/integration_new_passes_test.dart b/app/test/integration_new_passes_test.dart index b0efb73..51c650c 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,121 @@ 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')); + + final chroma = await runAndGetPixFmt( + 'deint_wf_default', + await WorkerHarness.runJob( + job420('deint_wf_default').toJson(), + label: 'deint_wf_default')); + + 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..628f839 100644 --- a/app/test/integration_qtgmc_parameters_test.dart +++ b/app/test/integration_qtgmc_parameters_test.dart @@ -615,4 +615,130 @@ 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('4:2:0 chroma is upsampled field-aware by default, 16-bit is opt-in', + () async { + final script = await generateScriptViaWorker(buildJob('deint_wf_default')); + + // Interlaced 4:2:0 stores chroma per field, so the pass converts to + // 4:2:2 before QTGMC and restores the source format afterwards. + 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"')); + + // 16-bit costs roughly 2x, so it must stay opt-in. + expect(script, isNot(contains('_deint_bits = max(_deint_bits, 16)')), + reason: '16-bit processing should be off unless requested'); + + print(' PASS: chroma upsample on by default, 16-bit off'); + }, timeout: const Timeout(Duration(minutes: 2))); + + test('16-bit processing is emitted when enabled', () async { + final script = await generateScriptViaWorker( + buildJob('deint_wf_16bit', highPrecision: true)); + + expect(script, contains('_deint_ss_h = 0')); + expect(script, contains('_deint_bits = max(_deint_bits, 16)')); + expect(script, contains('dither_type="error_diffusion"')); + + print(' PASS: 16-bit working format emitted'); + }, 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/worker/src/models/qtgmc_parameters.rs b/worker/src/models/qtgmc_parameters.rs index 72f031a..0dc486e 100644 --- a/worker/src/models/qtgmc_parameters.rs +++ b/worker/src/models/qtgmc_parameters.rs @@ -48,6 +48,20 @@ 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. `None` = enabled. + #[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 +405,42 @@ 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 enabled — it is a no-op for sources that are not 4:2:0. + pub fn chroma_upsample_fix_enabled(&self) -> bool { + self.chroma_upsample_fix.unwrap_or(true) + } + + /// 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 +450,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..77a0c66 100644 --- a/worker/tests/filter_integration_test.rs +++ b/worker/tests/filter_integration_test.rs @@ -1925,7 +1925,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 +1941,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 +2010,182 @@ fn test_57_ivtc_high_bit_depth_guard() { "core.vivtc.VDecimate(clip", ]).unwrap(); } + +#[test] +fn test_58_deinterlace_working_format_default() { + // Issue #49: interlaced 4:2:0 stores chroma per field, so QTGMC must not + // interpolate it at 4:2:0. By default the pass converts to 4:2:2 with + // field-aware resampling (zimg honours _FieldBased) and restores the source + // format afterwards. 16-bit is off by default, so the bit depth must be + // left alone. + 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() + }); + + run_job_and_verify(&job, "Deinterlace Working Format (default)", &[ + "_deint_src_format = clip.format", + "_deint_ss_h = 0", + "subsampling_h=_deint_ss_h", + "dither_type=\"error_diffusion\"", + ]).unwrap(); + + // 16-bit is opt-in, so the bit-depth line must not be emitted. + 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 be off by default" + ); +} + +#[test] +fn test_59_deinterlace_high_precision() { + // With both working-format options on, the pass converts to 4:2:2/16-bit + // and dithers back to the source format. + 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_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"); +} From 75c7ea5bd60df7f17d7f3de9422c37402536652b Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Fri, 31 Jul 2026 16:48:24 +1000 Subject: [PATCH 2/4] chore: bump app version to 0.9.11 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) --- app/macos/Runner/Info.plist | 4 ++-- app/pubspec.yaml | 2 +- app/windows/runner/Runner.rc | 4 ++-- worker/Cargo.toml | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) 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/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"] From 831ae1eb99f3aedf15691aab36dae6b1a6afffaf Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Fri, 31 Jul 2026 23:52:32 +1000 Subject: [PATCH 3/4] fix(deinterlace): make the interlaced chroma fix opt-in (#49) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CLAUDE.md | 14 ++- app/assets/filters/core/deinterlace.json | 4 +- app/lib/models/parameter_converter.dart | 2 +- app/lib/models/qtgmc_parameters.dart | 3 +- app/test/integration_new_passes_test.dart | 7 +- .../integration_qtgmc_parameters_test.dart | 32 +++++-- worker/src/models/qtgmc_parameters.rs | 9 +- worker/tests/filter_integration_test.rs | 89 ++++++++++++++++--- 8 files changed, 128 insertions(+), 32 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1152984..635e04c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -467,15 +467,21 @@ 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): -- **`chromaUpsampleFix`** (default **on**): interlaced 4:2:0 stores chroma per +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`. No-op on 4:2:2/4:4:4 sources. + 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. -With both off, the generated script is byte-for-byte what it was before the -block existed (asserted by `test_60_deinterlace_working_format_disabled`). +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 diff --git a/app/assets/filters/core/deinterlace.json b/app/assets/filters/core/deinterlace.json index d0d8c15..1a768f4 100644 --- a/app/assets/filters/core/deinterlace.json +++ b/app/assets/filters/core/deinterlace.json @@ -243,10 +243,10 @@ }, "chromaUpsampleFix": { "type": "boolean", - "default": true, + "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. No effect on 4:2:2 or 4:4:4 sources.", + "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": [ diff --git a/app/lib/models/parameter_converter.dart b/app/lib/models/parameter_converter.dart index 96ff614..9ae6ef1 100644 --- a/app/lib/models/parameter_converter.dart +++ b/app/lib/models/parameter_converter.dart @@ -40,7 +40,7 @@ class ParameterConverter { '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 ?? true, + 'chromaUpsampleFix': params.chromaUpsampleFix ?? false, 'highPrecision': params.highPrecision ?? false, 'inputType': params.inputType, 'tr0': params.tr0, diff --git a/app/lib/models/qtgmc_parameters.dart b/app/lib/models/qtgmc_parameters.dart index c05b7a5..6ea7712 100644 --- a/app/lib/models/qtgmc_parameters.dart +++ b/app/lib/models/qtgmc_parameters.dart @@ -135,7 +135,8 @@ class QTGMCParameters { // === Working Format (issue #49) === /// Upsample 4:2:0 chroma to 4:2:2 (field-aware) before deinterlacing and - /// restore the source format afterwards. Null = enabled. + /// 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. diff --git a/app/test/integration_new_passes_test.dart b/app/test/integration_new_passes_test.dart index 51c650c..1dfdcb8 100644 --- a/app/test/integration_new_passes_test.dart +++ b/app/test/integration_new_passes_test.dart @@ -223,11 +223,12 @@ void main() { .toJson(), label: 'deint_wf_off')); + // Both options are opt-in, so the chroma variant must ask for it. final chroma = await runAndGetPixFmt( - 'deint_wf_default', + 'deint_wf_chroma', await WorkerHarness.runJob( - job420('deint_wf_default').toJson(), - label: 'deint_wf_default')); + job420('deint_wf_chroma', chromaUpsampleFix: true).toJson(), + label: 'deint_wf_chroma')); final bits = await runAndGetPixFmt( 'deint_wf_16bit', diff --git a/app/test/integration_qtgmc_parameters_test.dart b/app/test/integration_qtgmc_parameters_test.dart index 628f839..93a2917 100644 --- a/app/test/integration_qtgmc_parameters_test.dart +++ b/app/test/integration_qtgmc_parameters_test.dart @@ -648,33 +648,49 @@ void main() { ); } - test('4:2:0 chroma is upsampled field-aware by default, 16-bit is opt-in', - () async { + 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"')); - // 16-bit costs roughly 2x, so it must stay opt-in. + // The two options are independent. expect(script, isNot(contains('_deint_bits = max(_deint_bits, 16)')), - reason: '16-bit processing should be off unless requested'); + reason: 'the chroma fix must not enable 16-bit as a side effect'); - print(' PASS: chroma upsample on by default, 16-bit off'); + print(' PASS: chroma upsample emitted on its own'); }, timeout: const Timeout(Duration(minutes: 2))); - test('16-bit processing is emitted when enabled', () async { + 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_ss_h = 0')); 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'); + 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 { diff --git a/worker/src/models/qtgmc_parameters.rs b/worker/src/models/qtgmc_parameters.rs index 0dc486e..4423b2a 100644 --- a/worker/src/models/qtgmc_parameters.rs +++ b/worker/src/models/qtgmc_parameters.rs @@ -52,7 +52,8 @@ pub struct QTGMCParameters { /// 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. `None` = enabled. + /// 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, @@ -413,9 +414,11 @@ 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 enabled — it is a no-op for sources that are not 4:2:0. + /// 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(true) + self.chroma_upsample_fix.unwrap_or(false) } /// Whether to run the deinterlace pass at 16-bit. Defaults to disabled diff --git a/worker/tests/filter_integration_test.rs b/worker/tests/filter_integration_test.rs index 77a0c66..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() @@ -2013,11 +2016,10 @@ fn test_57_ivtc_high_bit_depth_guard() { #[test] fn test_58_deinterlace_working_format_default() { - // Issue #49: interlaced 4:2:0 stores chroma per field, so QTGMC must not - // interpolate it at 4:2:0. By default the pass converts to 4:2:2 with - // field-aware resampling (zimg honours _FieldBased) and restores the source - // format afterwards. 16-bit is off by default, so the bit depth must be - // left alone. + // 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"); @@ -2032,26 +2034,60 @@ fn test_58_deinterlace_working_format_default() { ..ProcessingPipeline::default() }); - run_job_and_verify(&job, "Deinterlace Working Format (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(); - // 16-bit is opt-in, so the bit-depth line must not be emitted. + // 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 be off by default" + "16-bit processing should stay off unless separately enabled" ); } #[test] fn test_59_deinterlace_high_precision() { - // With both working-format options on, the pass converts to 4:2:2/16-bit - // and dithers back to the source format. + // 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"); @@ -2068,6 +2104,39 @@ fn test_59_deinterlace_high_precision() { }); 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\"", From de4dc98e5924f747a1289550a64e8f0ade531553 Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Sat, 1 Aug 2026 23:31:42 +1000 Subject: [PATCH 4/4] fix(deinterlace): adopt deps 1.7.0, fixing QTGMC preset brightening on arm64 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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". --- CLAUDE.md | 115 +++++++++++++++++++- app/assets/deps-version.json | 6 +- app/macos/Podfile.lock | 2 +- app/test/vapoursynth_integration_test.dart | 121 +++++++++++++++++++++ 4 files changed, 237 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 635e04c..e174ff7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -502,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 @@ -577,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 @@ -793,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/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/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.