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'); 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" + ); + } + } + } +}