From 462821dc7390cdde9220d8c931dd7c54a30856ad Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Mon, 5 Oct 2026 19:05:50 +1100 Subject: [PATCH 1/2] fix(deinterlace): Soft Telecine left Bwdif's template block in the script (#108) Each deinterlace method's arm removed the other methods' template blocks by hand. When Bwdif was added, the QTGMC and IVTC arms gained the new remove_block line and the Soft Telecine arm did not, so its script kept the whole Bwdif block with {{BWDIF_FIELD}} unsubstituted. vspipe reported that as a Python SyntaxError, failing both preview and encode for every source with Soft Telecine selected. Block selection now happens once, from a DEINT_METHOD_BLOCKS table, before the per-method match: the selected method's block is kept and every other is dropped. A unit test with a catch-all-free match keeps the table complete when a method is added. Tests: test_161 generates the encode and preview scripts for every method and asserts each emits only its own call with no placeholder left; a script-generation case in integration_filter_parameters_test; and a heavy preview + encode of soft_telecine_test.mkv. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_014GLXdGLfPwgYjW1AkonGqN --- .../integration_filter_parameters_test.dart | 21 +++++ app/test/integration_new_passes_test.dart | 32 ++++++++ worker/src/script_generator.rs | 78 ++++++++++++------- worker/tests/filter_integration_test.rs | 61 +++++++++++++++ 4 files changed, 165 insertions(+), 27 deletions(-) diff --git a/app/test/integration_filter_parameters_test.dart b/app/test/integration_filter_parameters_test.dart index daacc70..d314981 100644 --- a/app/test/integration_filter_parameters_test.dart +++ b/app/test/integration_filter_parameters_test.dart @@ -1432,5 +1432,26 @@ void main() { expect(script, contains('core.vivtc.VDecimate(clip')); print(' PASS'); }, timeout: const Timeout(Duration(minutes: 2))); + + // --- Soft Telecine (issue #108) --- + // The method left Bwdif's block in the script with `{{BWDIF_FIELD}}` raw, + // which vspipe reports as a Python SyntaxError on every source. + test('soft telecine: VDecimate only, no other method\'s block', () async { + final job = buildJob( + testName: 'soft_telecine', + deinterlace: const QTGMCParameters( + enabled: true, method: DeinterlaceMethod.softTelecine, + ), + ); + print(' Generating soft telecine script...'); + final script = await generateScriptViaWorker(job); + expect(script, contains('core.vivtc.VDecimate(clip)')); + expect(script, isNot(contains('core.bwdif.Bwdif('))); + expect(script, isNot(contains('core.vivtc.VFM('))); + expect(script, isNot(contains('haf.QTGMC('))); + expect(script, isNot(contains('{{BWDIF_'))); + expect(script, isNot(contains('{{#DEINT'))); + print(' PASS'); + }, timeout: const Timeout(Duration(minutes: 2))); }); } diff --git a/app/test/integration_new_passes_test.dart b/app/test/integration_new_passes_test.dart index 68fe98f..ace9d2f 100644 --- a/app/test/integration_new_passes_test.dart +++ b/app/test/integration_new_passes_test.dart @@ -1000,4 +1000,36 @@ void main() { } }, timeout: const Timeout(Duration(minutes: 10))); }); + + // Issue #108: Soft Telecine kept Bwdif's template block, placeholders and + // all, so both the preview and the encode died on a Python SyntaxError. The + // script assertions cover the text; this proves vspipe accepts it. + group('soft telecine (preview + full encode)', () { + test('previews and encodes the soft-telecined fixture', () async { + final input = p.join(WorkerHarness.repoRoot, 'Tests', 'TestResources', + 'soft_telecine_test.mkv'); + final job = VideoJob( + id: const Uuid().v4(), + inputPath: input, + outputPath: '$_outDir/soft_telecine.mkv', + processingPipeline: const ProcessingPipeline( + deinterlace: + QTGMCParameters(method: DeinterlaceMethod.softTelecine), + ), + encodingSettings: const EncodingSettings( + codec: VideoCodec.h264, + container: ContainerFormat.mkv, + audioMode: AudioMode.passthrough, + ), + ); + + final preview = await WorkerHarness.runPreview(job.toJson(), + frame: 0, label: 'soft_telecine'); + expect(preview.png, isNotNull, + reason: '${preview.error}\n${preview.logs}'); + + await _expectValidVideo( + await WorkerHarness.runJob(job.toJson(), label: 'soft_telecine')); + }, timeout: const Timeout(Duration(minutes: 8))); + }); } diff --git a/worker/src/script_generator.rs b/worker/src/script_generator.rs index 4b5cd07..411107b 100644 --- a/worker/src/script_generator.rs +++ b/worker/src/script_generator.rs @@ -384,14 +384,23 @@ impl ScriptGenerator { script = script.replace("{{#DEINTERLACE}}", ""); script = script.replace("{{/DEINTERLACE}}", ""); + // Keep the selected method's block and drop every other one, in one + // place. Each arm used to remove the others by hand, and the Soft + // Telecine arm missed Bwdif's — leaving its raw placeholders in the + // script as a Python SyntaxError (issue #108). + for (method, name) in DEINT_METHOD_BLOCKS { + let open = format!("{{{{#{name}}}}}"); + let close = format!("{{{{/{name}}}}}"); + if method == params.method { + script = script.replace(&open, ""); + script = script.replace(&close, ""); + } else { + script = remove_block(&open, &close, script); + } + } + match params.method { DeinterlaceMethod::Bwdif => { - script = remove_block("{{#DEINT_QTGMC}}", "{{/DEINT_QTGMC}}", script); - script = remove_block("{{#DEINT_IVTC}}", "{{/DEINT_IVTC}}", script); - script = remove_block("{{#DEINT_SOFT_TELECINE}}", "{{/DEINT_SOFT_TELECINE}}", script); - script = script.replace("{{#DEINT_BWDIF}}", ""); - script = script.replace("{{/DEINT_BWDIF}}", ""); - // field: 0/1 keep one field per input frame (single rate), // 2/3 emit one per field (double rate). The parity half is // the same TFF value QTGMC uses, so the two methods cannot @@ -417,13 +426,6 @@ impl ScriptGenerator { } } DeinterlaceMethod::Qtgmc => { - // Enable QTGMC block, remove IVTC and Soft Telecine blocks - script = script.replace("{{#DEINT_QTGMC}}", ""); - script = script.replace("{{/DEINT_QTGMC}}", ""); - script = remove_block("{{#DEINT_IVTC}}", "{{/DEINT_IVTC}}", script); - script = remove_block("{{#DEINT_SOFT_TELECINE}}", "{{/DEINT_SOFT_TELECINE}}", script); - script = remove_block("{{#DEINT_BWDIF}}", "{{/DEINT_BWDIF}}", 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 @@ -624,13 +626,6 @@ impl ScriptGenerator { script = process_optional_int("DEVICE", params.device, script); } DeinterlaceMethod::Ivtc => { - // Enable IVTC block, remove QTGMC and Soft Telecine blocks - script = remove_block("{{#DEINT_QTGMC}}", "{{/DEINT_QTGMC}}", script); - script = script.replace("{{#DEINT_IVTC}}", ""); - script = script.replace("{{/DEINT_IVTC}}", ""); - script = remove_block("{{#DEINT_SOFT_TELECINE}}", "{{/DEINT_SOFT_TELECINE}}", script); - script = remove_block("{{#DEINT_BWDIF}}", "{{/DEINT_BWDIF}}", script); - // Derive IVTC_ORDER from tff field (TFF→1, BFF→0), falling back to ivtc_order let order = match params.tff { Some(true) => 1, @@ -651,13 +646,8 @@ impl ScriptGenerator { script = process_optional_double("IVTC_DUPTHRESH", params.ivtc_dupthresh, script); script = process_optional_double("IVTC_SCTHRESH", params.ivtc_scthresh, script); } - DeinterlaceMethod::SoftTelecine => { - // Enable Soft Telecine block, remove QTGMC and IVTC blocks - script = remove_block("{{#DEINT_QTGMC}}", "{{/DEINT_QTGMC}}", script); - script = remove_block("{{#DEINT_IVTC}}", "{{/DEINT_IVTC}}", script); - script = script.replace("{{#DEINT_SOFT_TELECINE}}", ""); - script = script.replace("{{/DEINT_SOFT_TELECINE}}", ""); - } + // No parameters: the block is a bare VDecimate. + DeinterlaceMethod::SoftTelecine => {} } } else { script = remove_block("{{#DEINTERLACE}}", "{{/DEINTERLACE}}", script); @@ -2192,6 +2182,15 @@ fn process_optional_string(name: &str, value: Option<&str>, mut script: String) script } +/// The template block each deinterlace method owns. Exactly one survives into +/// a script; `every_deinterlace_method_owns_a_block` keeps this complete. +const DEINT_METHOD_BLOCKS: [(DeinterlaceMethod, &str); 4] = [ + (DeinterlaceMethod::Qtgmc, "DEINT_QTGMC"), + (DeinterlaceMethod::Ivtc, "DEINT_IVTC"), + (DeinterlaceMethod::SoftTelecine, "DEINT_SOFT_TELECINE"), + (DeinterlaceMethod::Bwdif, "DEINT_BWDIF"), +]; + /// Remove a block from start tag to end tag (including the line). fn remove_block(start_tag: &str, end_tag: &str, mut script: String) -> String { while let Some(start_pos) = script.find(start_tag) { @@ -2215,6 +2214,31 @@ fn remove_block(start_tag: &str, end_tag: &str, mut script: String) -> String { mod tests { use super::*; + /// A method missing from DEINT_METHOD_BLOCKS keeps every other method's + /// block in its script (issue #108). The match has no catch-all, so a new + /// variant fails to compile here until it is listed. + #[test] + fn every_deinterlace_method_owns_a_block() { + for method in [ + DeinterlaceMethod::Qtgmc, + DeinterlaceMethod::Ivtc, + DeinterlaceMethod::SoftTelecine, + DeinterlaceMethod::Bwdif, + ] { + match method { + DeinterlaceMethod::Qtgmc + | DeinterlaceMethod::Ivtc + | DeinterlaceMethod::SoftTelecine + | DeinterlaceMethod::Bwdif => {} + } + assert_eq!( + DEINT_METHOD_BLOCKS.iter().filter(|(m, _)| *m == method).count(), + 1, + "{method:?} must own exactly one block" + ); + } + } + #[test] fn templates_load_with_lf_endings_only() { // Guards the CRLF normalization in load_template_by_name. Without it, diff --git a/worker/tests/filter_integration_test.rs b/worker/tests/filter_integration_test.rs index 46df540..f6808da 100644 --- a/worker/tests/filter_integration_test.rs +++ b/worker/tests/filter_integration_test.rs @@ -5885,3 +5885,64 @@ fn test_160_pad_to_fill_uses_the_border_step_and_its_colour() { assert_no_border_tags("either", script); } } + +/// Issue #108: Soft Telecine left the whole Bwdif block in the script, raw +/// `{{BWDIF_FIELD}}` included, so preview and encode both died on a Python +/// SyntaxError. Every method must emit its own call and no other method's, in +/// both scripts. +#[test] +fn test_161_each_deinterlace_method_emits_only_its_own_block() { + create_output_dir(); + + // (method, its call in the encode script, its call in the preview script). + // Soft Telecine is a frame-count change only, so the preview has no block. + let cases = [ + (DeinterlaceMethod::Qtgmc, "haf.QTGMC(", Some("haf.QTGMC(")), + (DeinterlaceMethod::Ivtc, "core.vivtc.VFM(", Some("core.vivtc.VFM(")), + (DeinterlaceMethod::SoftTelecine, "core.vivtc.VDecimate(clip)", None), + (DeinterlaceMethod::Bwdif, "core.bwdif.Bwdif(", Some("core.bwdif.Bwdif(")), + ]; + let all_calls = ["haf.QTGMC(", "core.vivtc.VFM(", "core.vivtc.VDecimate(", "core.bwdif.Bwdif("]; + // Prefix-scoped, not a bare "{{": the templates' docstrings show the syntax. + let prefixes = ["{{#DEINT", "{{/DEINT", "{{BWDIF_", "{{IVTC_", "{{#IVTC_", "{{PRESET"]; + + for (method, encode_call, preview_call) in cases { + let mut job = create_base_job(&format!("test_161_{method:?}")); + let deinterlace = QTGMCParameters { + enabled: true, + method, + tff: Some(true), + ..Default::default() + }; + job.qtgmc_parameters = deinterlace.clone(); + job.processing_pipeline = Some(ProcessingPipeline { + deinterlace, + ..ProcessingPipeline::default() + }); + + let (encode, preview) = generate_both_scripts(&job); + for (name, script, own) in [ + ("encode", &encode, Some(encode_call)), + ("preview", &preview, preview_call), + ] { + if let Some(own) = own { + assert!(script.contains(own), "{method:?} {name} must emit {own}"); + } + for call in all_calls { + // IVTC is VFM + VDecimate; everything else owns one call. + let allowed = own.is_some_and(|o| o.starts_with(call)) + || (method == DeinterlaceMethod::Ivtc && call == "core.vivtc.VDecimate("); + assert!( + allowed || !script.contains(call), + "{method:?} {name} must not emit {call}" + ); + } + for prefix in prefixes { + assert!( + !script.contains(prefix), + "{method:?} {name} left an unsubstituted {prefix} placeholder" + ); + } + } + } +} From 7df289ce947f88394554e547d9edac3ecd51dd7a Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Mon, 5 Oct 2026 19:34:35 +1100 Subject: [PATCH 2/2] test: hash the colour-tag source reference from the stored frame The FFV1 losslessness control built its source reference with `-s 720x576`, mirroring the worker's old decoder. #106 changed the worker to decode the full stored frame with `-apply_cropping codec` and never rescale, so the reference became a 702->720 rescale of the clean-aperture fixture while the worker's output was the true stored frame. The nightly has failed on this one test on every platform since both landed on main. Decode the reference with PreviewGenerator.sourceDecodeOptions, the same option the worker uses. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_014GLXdGLfPwgYjW1AkonGqN --- app/test/integration_colour_tag_pixels_test.dart | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/app/test/integration_colour_tag_pixels_test.dart b/app/test/integration_colour_tag_pixels_test.dart index d394e24..b5e388a 100644 --- a/app/test/integration_colour_tag_pixels_test.dart +++ b/app/test/integration_colour_tag_pixels_test.dart @@ -29,6 +29,7 @@ import 'package:vapourbox/models/encoding_settings.dart'; import 'package:vapourbox/models/processing_pipeline.dart'; import 'package:vapourbox/models/qtgmc_parameters.dart'; import 'package:vapourbox/models/video_job.dart'; +import 'package:vapourbox/services/preview_generator.dart'; import 'support/worker_harness.dart'; @@ -45,18 +46,19 @@ const _frames = 10; /// Decoding to rawvideo in the stream's native format involves no scaler, so /// this hashes exactly what the file stores rather than a conversion of it. /// -/// [size] reproduces the worker's own decoder for the source reference: this -/// fixture has a clean aperture (720x576 coded, 702x576 decoded) and the -/// worker's decoder forces the probed 720x576 back with `-s`. -Future _sampleMd5(String path, {int? frames, String? size, String? pixFmt}) async { +/// The decode uses the worker's own source options (`-apply_cropping codec`): +/// this fixture has a clean aperture (720x576 stored, 702x576 once ffmpeg's +/// default container cropping is applied) and the worker reads the full +/// stored frame. Rescaling 702 back to 720 here is not the same samples. +Future _sampleMd5(String path, {int? frames, String? pixFmt}) async { final r = await Process.run( WorkerHarness.ffmpegPath, [ '-v', 'error', + ...PreviewGenerator.sourceDecodeOptions, '-i', path, '-map', '0:v:0', if (frames != null) ...['-frames:v', '$frames'], - if (size != null) ...['-s', size], if (pixFmt != null) ...['-pix_fmt', pixFmt], '-f', 'md5', '-', ], @@ -139,7 +141,6 @@ void main() { // The picture: identical to the untagged encode and to the source itself. final sourceMd5 = await _sampleMd5(_fixture, frames: _frames, - size: '${src['width']}x${src['height']}', pixFmt: src['pix_fmt'] as String); expect(await _sampleMd5(untagged), sourceMd5, reason: 'control: a no-op pipeline into FFV1 must be lossless');