From 9ace2497a92e343350ff0ba0cc9ff75e1d2a05f9 Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Mon, 26 Oct 2026 11:19:26 +1100 Subject: [PATCH] fix(worker): count retimed frames in the progress total (#134) The frame rate pass needs the source rate to know its retime ratio, but the app never sends sourceFpsNum/Den, so the pass mapped as the identity. The progress total stayed at the source count while RIFE/FlowFPS emitted more frames: progress pinned at 100% with "ETA: --" for the rest of the encode, and frame-accurate preview of a retimed job seeked the wrong frame. effective_pipeline() now fills the rate from the job's input rate (the same rational the script declares), and the ratio is taken against the rate at the pass, so double-rate deinterlacing followed by a frame rate conversion no longer counts double. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 6 +- app/test/integration_frame_mapping_test.dart | 69 ++++++++++- worker/src/models/frame_rate_parameters.rs | 14 +-- worker/src/models/processing_pipeline.rs | 46 ++++++- worker/src/models/video_job.rs | 121 ++++++++++++++++++- worker/src/script_generator.rs | 22 +--- 6 files changed, 236 insertions(+), 42 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e3bd5d3d..125088a1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1103,7 +1103,11 @@ than fail cleanly. frame it would merely copy (an exact source position, the held tail) is taken from the source by `FrameEval`, so photographed frames are bit-exact. The count is `floor(n * num / den)` of the reduced ratio — exactly - `FrameMap::Retime`. `sc=True` with `misc.SCDetect` holds the frame across a + `FrameMap::Retime`. That ratio needs the source rate, which the app does + **not** send: `VideoJob::effective_pipeline` fills `sourceFpsNum/Den` from + the job's input rate, and the ratio is taken against the rate *at the pass* + (×2 after double-rate deinterlacing). Without that the progress total stays + at the source count and pins at 100% with no ETA (issue #134). `sc=True` with `misc.SCDetect` holds the frame across a cut rather than blending two shots. - **Upscaling: models, factors, tiles.** Five of vs-mlrt's ONNX conversions ship (`gpu_plugin::UPSCALE_MODELS`): Real-ESRGAN animevideo 2x/4x and diff --git a/app/test/integration_frame_mapping_test.dart b/app/test/integration_frame_mapping_test.dart index 8880ba80..8d1f32ed 100644 --- a/app/test/integration_frame_mapping_test.dart +++ b/app/test/integration_frame_mapping_test.dart @@ -22,6 +22,8 @@ /// Decimate{cycle:5,keep:4} 90 -> 72 /// Retime 25 -> 24000/1001 ratio reduces to 960/1001, so 75 -> 71 /// (75*960/1001 = 71.93, floored) +/// Fanout then Retime to 25 31 -> 62 -> 31 (the retime converts from the +/// doubled 50p, not the source's 25) /// /// Heavy (full encode + preview) — nightly, not the push gate. @Tags(['heavy']) @@ -29,6 +31,7 @@ library; // ignore_for_file: avoid_print — these tests print diagnostics to the test log. +import 'dart:convert'; import 'dart:io'; import 'dart:typed_data'; @@ -115,6 +118,26 @@ Future _countFrames(String path) async { return n!; } +/// Every progress total the worker reported — what the app shows as +/// "frame / total" and derives the percentage and ETA from. The last one is +/// always the frames written (the worker sends a final 100% with the real +/// count), so only the ones before it say whether the total was right while +/// the encode ran. A clip this short may finish before the first poll, leaving +/// none; `video_job.rs`'s frame_rate_progress_tests are the deterministic +/// guard, this one checks the real pipeline agrees whenever it can. +List _progressTotals(JobResult r) { + final totals = []; + for (final line in r.logs) { + if (!line.startsWith('{')) continue; + try { + final json = jsonDecode(line) as Map; + final total = json['totalFrames']; + if (json['type'] == 'progress' && total is int) totals.add(total); + } catch (_) {/* not a worker message */} + } + return totals; +} + /// One frame of a video as packed rgb24, by absolute frame index. Future _frameRgb(String path, int index) async { final out = File(p.join(Directory.systemTemp.path, @@ -260,11 +283,11 @@ void main() { input: _interlaced, pipeline: const ProcessingPipeline( deinterlace: QTGMCParameters(enabled: false), + // No sourceFpsNum/Den: the app never sends them (issue #134), so + // the worker must work the ratio out from the job's input rate. frameRate: FrameRateParameters( enabled: true, target: FrameRateTarget.film23976, - sourceFpsNum: 25, - sourceFpsDen: 1, ), ), width: 720, @@ -284,6 +307,46 @@ void main() { 'floor((n-1)*r)+1. A count of ${expected + 1} means the ' 'BlockFPS arithmetic is in play and every retimed job reports a ' 'progress total one frame out.'); + print(' progress totals: ${_progressTotals(r).toSet()}'); + expect(_progressTotals(r), everyElement(n), + reason: 'issue #134: a total left at the source count pins ' + 'progress at 100% with no ETA for the rest of the encode'); + }); + + test('a frame rate after double-rate QTGMC converts from the doubled rate', + () async { + // 25i doubled to 50p, then to 25: 50 -> 25 halves, so 31 -> 62 -> 31. + // Taking the ratio against the source's 25 would claim 62. + const start = 10, end = 40; + const sourceFrames = end - start + 1; // 31 + final job = _job( + 'double_rate_retime', + input: _interlaced, + pipeline: const ProcessingPipeline( + deinterlace: QTGMCParameters( + preset: QTGMCPreset.superFast, tff: true, fpsDivisor: 1), + frameRate: FrameRateParameters( + enabled: true, + target: FrameRateTarget.pal25, + ), + ), + width: 720, + height: 576, + pixFmt: 'yuv422p', + fps: _interlacedFps, + totalFrames: sourceFrames, + startFrame: start, + endFrame: end, + ); + + final r = await WorkerHarness.runJob(job.toJson(), + label: 'double_rate_retime'); + expect(r.success, isTrue, reason: r.error); + final n = await _countFrames(r.outputPath!); + print(' $sourceFrames source -> $n output (expected $sourceFrames)'); + expect(n, sourceFrames); + print(' progress totals: ${_progressTotals(r).toSet()}'); + expect(_progressTotals(r), everyElement(n)); }); }); @@ -396,8 +459,6 @@ void main() { enabled: true, target: FrameRateTarget.pal50, method: FrameRateMethod.rife, - sourceFpsNum: 25, - sourceFpsDen: 1, ), ); diff --git a/worker/src/models/frame_rate_parameters.rs b/worker/src/models/frame_rate_parameters.rs index 1243b140..21e1211d 100644 --- a/worker/src/models/frame_rate_parameters.rs +++ b/worker/src/models/frame_rate_parameters.rs @@ -105,9 +105,10 @@ pub struct FrameRateParameters { #[serde(default = "default_overlap")] pub overlap: i32, - /// The source rate, supplied by the app from ffprobe. The pipeline needs it - /// to report a correct `FrameMap::Retime`, and cannot see the job — the - /// same reason `inputSar` is carried rather than looked up. + /// The source rate. The pipeline needs it to report a correct + /// `FrameMap::Retime`, and cannot see the job, so + /// `VideoJob::effective_pipeline` fills it from the job's input rate when + /// absent — the app never sends it (issue #134). #[serde(default, skip_serializing_if = "Option::is_none")] pub source_fps_num: Option, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -160,14 +161,9 @@ impl FrameRateParameters { fn as_fraction(&self) -> (u32, u32) { self.target.as_fraction() } - - /// The retime ratio using the carried source rate, if the app supplied one. - pub fn ratio(&self) -> Option<(u32, u32)> { - self.ratio_against(self.source_fps_num?, self.source_fps_den?) - } } -fn gcd(a: u64, b: u64) -> u64 { +pub(super) fn gcd(a: u64, b: u64) -> u64 { if b == 0 { a.max(1) } else { gcd(b, a % b) } } diff --git a/worker/src/models/processing_pipeline.rs b/worker/src/models/processing_pipeline.rs index 7357cd3c..58afb12e 100644 --- a/worker/src/models/processing_pipeline.rs +++ b/worker/src/models/processing_pipeline.rs @@ -2,6 +2,8 @@ use serde::{Deserialize, Serialize}; +use super::frame_rate_parameters::gcd; + use super::{ AntiAliasParameters, GeometryParameters, GrainParameters, StabilizeParameters, ChromaDenoiseParameters, ChromaFixParameters, ColorCorrectionParameters, CropResizeParameters, @@ -67,6 +69,17 @@ impl FrameMap { } } + /// How the map scales the frame rate, as num/den: a pass that emits more + /// frames over the same duration raises the rate by the same factor. + pub fn rate_factor(&self) -> (u64, u64) { + match *self { + FrameMap::Identity { .. } => (1, 1), + FrameMap::Fanout { factor, .. } => (factor as u64, 1), + FrameMap::Decimate { cycle, keep } => (keep as u64, cycle as u64), + FrameMap::Retime { num, den, .. } => (num as u64, den as u64), + } + } + /// Map an output frame index back to the input-side source frames that /// produce it. (Preview / seek.) pub fn inverse(&self, m: u64) -> SourceSpan { @@ -542,15 +555,15 @@ impl ProcessingPipeline { /// Frame mapping for the frame rate pass. /// - /// The source rate is not known to the pipeline, so this reports the - /// identity and the executor substitutes the real ratio once vspipe has - /// reported the input rate. Reporting a wrong fixed ratio here would make - /// the progress total and the preview seek disagree with the output. + /// Needs the source rate, which `VideoJob::effective_pipeline` fills in; + /// a pipeline built without one reports the identity. Reporting a wrong + /// fixed ratio here would make the progress total and the preview seek + /// disagree with the output. fn frame_rate_frame_map(&self) -> FrameMap { // Without a source rate we cannot know the ratio, and a wrong fixed // ratio is worse than none: it would make the progress total and the // preview index disagree with what the encoder actually receives. - match self.frame_rate.ratio() { + match self.frame_rate_input_fps().and_then(|(n, d)| self.frame_rate.ratio_against(n, d)) { Some((num, den)) => FrameMap::Retime { num, den, @@ -563,6 +576,29 @@ impl ProcessingPipeline { } } + /// The rate the frame rate pass converts *from*: the source rate as + /// changed by every pass ahead of it. Double-rate deinterlacing hands it + /// 50p from a 25i source, and its target is absolute, so the ratio has to + /// be taken against that, not against the source. + fn frame_rate_input_fps(&self) -> Option<(u32, u32)> { + let (mut num, mut den) = ( + self.frame_rate.source_fps_num? as u64, + self.frame_rate.source_fps_den? as u64, + ); + for pass in self.enabled_passes() { + if pass == PassType::FrameRate { + break; + } + let (f_num, f_den) = self.frame_map_for(pass).rate_factor(); + num *= f_num; + den *= f_den; + let g = gcd(num, den); + num /= g; + den /= g; + } + Some((u32::try_from(num).ok()?, u32::try_from(den).ok()?)) + } + /// Frame mapping for the deinterlace pass. QTGMC double-rate (FPSDivisor=1, /// the default when unset) doubles the frame count; IVTC/soft-telecine /// decimate on a cycle. Mirrors the count logic the progress reporter and diff --git a/worker/src/models/video_job.rs b/worker/src/models/video_job.rs index c4f4b420..93f00a18 100644 --- a/worker/src/models/video_job.rs +++ b/worker/src/models/video_job.rs @@ -127,11 +127,58 @@ impl VideoJob { /// Get the effective processing pipeline. /// Uses processing_pipeline if set, otherwise creates one from legacy qtgmc_parameters. + /// + /// The frame rate pass needs the source rate to know its retime ratio, and + /// the pipeline cannot see the job, so it is filled in here from the same + /// rate the script declares for the clip. Without it the pass maps as the + /// identity: the progress total stays at the source count while RIFE or + /// FlowFPS emit more frames, so progress pins at 100% with no ETA for the + /// rest of the encode (issue #134), and the preview seeks the wrong frame. pub fn effective_pipeline(&self) -> ProcessingPipeline { - self.processing_pipeline + let mut pipeline = self + .processing_pipeline .clone() - .unwrap_or_else(|| ProcessingPipeline::from_legacy(&self.qtgmc_parameters)) + .unwrap_or_else(|| ProcessingPipeline::from_legacy(&self.qtgmc_parameters)); + let fr = &mut pipeline.frame_rate; + if fr.source_fps_num.is_none() || fr.source_fps_den.is_none() { + let (num, den) = self.input_fps_rational(); + fr.source_fps_num = Some(num); + fr.source_fps_den = Some(den); + } + pipeline + } + + /// The source rate as the script declares it to VapourSynth, 29.97 when + /// the job does not say. + pub fn input_fps_rational(&self) -> (u32, u32) { + let (num, den) = fps_to_rational(self.input_frame_rate.unwrap_or(29.97)); + (num.max(1) as u32, den.max(1) as u32) + } +} + +/// Convert a floating-point frame rate to a rational number (num/den). +pub fn fps_to_rational(fps: f64) -> (i32, i32) { + // Common frame rates + let common = [ + (23.976, 24000, 1001), + (24.0, 24, 1), + (25.0, 25, 1), + (29.97, 30000, 1001), + (30.0, 30, 1), + (50.0, 50, 1), + (59.94, 60000, 1001), + (60.0, 60, 1), + ]; + + for (rate, num, den) in common { + if (fps - rate).abs() < 0.01 { + return (num, den); + } } + + // Fallback: multiply by 1000 and simplify + let num = (fps * 1000.0).round() as i32; + (num, 1000) } /// Subtitle generation settings using Whisper AI. @@ -1079,6 +1126,76 @@ impl FieldOrder { } } +#[cfg(test)] +mod frame_rate_progress_tests { + use super::*; + use crate::models::{DeinterlaceMethod, FrameRateMethod, FrameRateParameters, FrameRateTarget}; + + fn job(fps: f64, deinterlace: bool, frame_rate: FrameRateParameters) -> VideoJob { + let mut job: VideoJob = serde_json::from_value(serde_json::json!({ + "id": "00000000-0000-0000-0000-000000000000", + "inputPath": "in.mkv", + "outputPath": "out.mkv", + "qtgmcParameters": {}, + "encodingSettings": {}, + })) + .unwrap(); + job.input_frame_rate = Some(fps); + let mut pipeline = ProcessingPipeline::default(); + pipeline.deinterlace.enabled = deinterlace; + pipeline.deinterlace.method = DeinterlaceMethod::Qtgmc; + pipeline.frame_rate = frame_rate; + job.processing_pipeline = Some(pipeline); + job + } + + fn rife_to(target: FrameRateTarget) -> FrameRateParameters { + FrameRateParameters { + enabled: true, + target, + method: FrameRateMethod::Rife, + ..Default::default() + } + } + + /// Issue #134: the app never sends the frame rate pass its source rate, so + /// the progress total used to stay at the source count while RIFE doubled + /// it — 100% at the halfway point and no ETA for the rest. + #[test] + fn the_progress_total_counts_the_retimed_frames_without_an_app_supplied_rate() { + let j = job(23.976, false, rife_to(FrameRateTarget::Ntsc5994)); + // 24000/1001 -> 60000/1001 is exactly 5/2. + assert_eq!(j.effective_pipeline().output_count(1775), 1775 * 5 / 2); + } + + #[test] + fn a_rate_the_app_did_supply_is_kept() { + let mut fr = rife_to(FrameRateTarget::Pal50); + fr.source_fps_num = Some(25); + fr.source_fps_den = Some(1); + // The job claims 29.97; the carried rate wins. + assert_eq!(job(29.97, false, fr).effective_pipeline().output_count(100), 200); + } + + /// The target is absolute, so it converts from the rate deinterlacing + /// hands it, not the source's: 25i double-rate is 50p, and 50p -> 25 halves. + #[test] + fn the_ratio_is_taken_after_double_rate_deinterlacing() { + let j = job(25.0, true, rife_to(FrameRateTarget::Pal25)); + assert_eq!(j.effective_pipeline().output_count(100), 100); + let j = job(25.0, true, rife_to(FrameRateTarget::Ntsc5994)); + // 50 -> 59.94 is 1200/1001. + assert_eq!(j.effective_pipeline().output_count(100), 200 * 1200 / 1001); + } + + #[test] + fn the_preview_inverse_uses_the_same_ratio() { + let j = job(25.0, false, rife_to(FrameRateTarget::Pal50)); + let span = j.effective_pipeline().invert(10); + assert_eq!(span.start, 5); + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/worker/src/script_generator.rs b/worker/src/script_generator.rs index f902693a..73956291 100644 --- a/worker/src/script_generator.rs +++ b/worker/src/script_generator.rs @@ -411,27 +411,7 @@ impl ScriptGenerator { /// Convert a floating-point frame rate to a rational number (num/den). pub fn frame_rate_to_rational(&self, fps: f64) -> (i32, i32) { - // Common frame rates - let common = [ - (23.976, 24000, 1001), - (24.0, 24, 1), - (25.0, 25, 1), - (29.97, 30000, 1001), - (30.0, 30, 1), - (50.0, 50, 1), - (59.94, 60000, 1001), - (60.0, 60, 1), - ]; - - for (rate, num, den) in common { - if (fps - rate).abs() < 0.01 { - return (num, den); - } - } - - // Fallback: multiply by 1000 and simplify - let num = (fps * 1000.0).round() as i32; - (num, 1000) + crate::models::fps_to_rational(fps) } /// Substitute pipeline parameters on an already-prepared script.