From c3afb994f945f389820f72d40c8400065dd947f6 Mon Sep 17 00:00:00 2001 From: SteveYu000 <111431635+SteveYu000@users.noreply.github.com> Date: Mon, 2 Nov 2026 12:36:41 +0800 Subject: [PATCH 1/2] Preserve source clocks and VFR range-end coverage Refine only progressive non-TS clocks that need it, preserving ordinary CFR grids and timescales. Distinguish an internal segment seam from the end of a kept range so a normal 23.976 picture at 37.996 s is not reduced to a 4 ms flash by OUT at 38 s. Retain existing unbounded-hold semantics. Classify repeated holds against typical display gaps as well as the average, and let auto indexing scan ambiguous progressive B-frame sources whose public container table only supplies decode timestamps. Invalidate old classifications with seek-cache version 19. Keep the latest displayed picture writable at the final refined-clock tail: decoder reorder depth alone does not bound how many B pictures can follow that picture. Reuse the existing seam-buffer limit. Add an 80-case real-media suite and document its scope. Locally compile both v0.8.39 and this CLI with Rust 1.98/MSVC/FFmpeg 7.1.1: 45/80 to 80/80; review reproducer 41.708333 ms in MP4 and 42 ms in MKV; existing VFR 8/8; 574 workspace unit tests passed, four known Windows failures filtered; clippy exits zero with existing warnings. Historical matrix 140/235 to 165/235 without new failures. Twenty ordinary CFR MP4 files and all 70 TS/M2TS files remain byte-identical to the baseline. This is the independent source-clock/VFR split of #10. SAR #11, AV1/MP4 #12 and M2TS arrival-time changes are separate. A forced --index scan on the reviewer's duplicate-PTS synthetic input retains its pre-existing unbounded-hold behavior and is documented as a limitation. --- docs/technical/building.md | 1 + docs/technical/source-clock.md | 95 ++++++++++ rust/crates/core/src/cut.rs | 185 ++++++++++++++++-- rust/crates/core/src/index.rs | 37 ++++ rust/crates/core/src/seek_index.rs | 4 +- tests/ci.sh | 2 +- tests/run_source_clock_tests.sh | 4 + tests/source_clock_tests.py | 295 +++++++++++++++++++++++++++++ 8 files changed, 602 insertions(+), 21 deletions(-) create mode 100644 docs/technical/source-clock.md create mode 100755 tests/run_source_clock_tests.sh create mode 100644 tests/source_clock_tests.py diff --git a/docs/technical/building.md b/docs/technical/building.md index 2543cf6..3349ffc 100644 --- a/docs/technical/building.md +++ b/docs/technical/building.md @@ -86,6 +86,7 @@ bash tests/run_bd_audio_tests.sh # the sound a disc carries, written out bash tests/run_vc1_tests.sh # the VC-1 encoder, put through a decoder 4 bash tests/run_vp9_av1_tests.sh # VP9 and AV1: cut, and still the source's bytes 5 bash tests/run_vfr_tests.sh # variable rate, both ways round 8 +bash tests/run_source_clock_tests.sh # source clocks, range ends and VFR copying 80 bash tests/run_demux_tests.sh # what a reader loses by reading what it does not 9 bash tests/run_transrate_tests.sh # fitting a disc: the identity, the size, the cost 15 bash tests/run_join_tests.sh # several recordings into one file, and the seams 17 diff --git a/docs/technical/source-clock.md b/docs/technical/source-clock.md new file mode 100644 index 0000000..03e7347 --- /dev/null +++ b/docs/technical/source-clock.md @@ -0,0 +1,95 @@ +# Source clocks and variable-rate range ends + +The timestamp clock and the nominal picture rate serve different purposes. +The boundary encoder still uses the existing rate and quality settings. The +output timeline must also represent the source's display timestamps. + +## When the clock changes + +`Grid::of` refines a progressive, non-TS source only when it is variable-rate, +or its nominal frame period is not an integer number of source ticks (for +example, 23.976 fps stored in milliseconds). An ordinary 25 fps MP4, or a +23.976 fps MP4 on a 1/24000 clock, retains its existing grid and timescale. +An already suitable grid is returned unchanged, without fraction reduction. + +`on_clock` chooses a subdivision that represents both the field grid and +the source tick. Only a newly aligned grid reduces its output time base. +Checked arithmetic and the existing 32-bit MP4 step/timescale limits bound +the result; if no such refinement fits, the existing bounded grid remains. +This does not promise exact ticks for every possible clock and file length. + +The walk measures repeated holds against the typical positive display gap +as well as the average. A 40/40/80 ms pattern must not be called constant +merely because its average frame lasts about 53 ms. Millisecond-rounded +NTSC and a 2:3 cadence still do not count as variable by that test. + +An unclassified progressive stream with B pictures and an average over 1% +below its declared rate needs its display times read: the public container +index contains decode timestamps. `ContainerIndex` declines that case, +so the default `auto` mode falls back to the packet scan. This can add a +sequential read for those sources; ordinary CFR keeps the fast table path. +An explicitly required container index reports the reason it cannot answer. +Seek-cache version 19 invalidates the former classification. + +## A seam is not the end of a keep-range + +At a seam inside one kept range, the following segment resumes on a measured +picture. Extending the preceding segment to a nominal frame length would +shift every picture after the seam. `SegmentCtx::last` distinguishes this +from the end of a keep-range. + +At a range end, a normal picture retains the coded-length floor used before +this change. The review reproducer has a picture at 37.996 s and OUT at +38 s. Refining the clock must not turn that picture into a 4 ms flash; its +normal 23.976 frame lasts about 41.708 ms. The existing +`coded_length_unbounded` cases retain their time-based hold behavior, +including a range entirely inside a long held picture. + +For newly refined CFR, the decoder's next measured picture compensates for +the planner's coarse-clock selection margin. A copy has that coverage from +the planner's terminating access point, including its leading pictures. + +On a refined clock, the final displayed picture also stays in the write +queue until a later displayed picture arrives or the final hold is known. +Decoder reorder depth alone is insufficient: a copied H.264 P picture can +precede three B pictures while `has_b_frames` is two. Releasing that P +picture too early left a 40/40/80 ms selection about 27 ms short. The queue +uses the existing 256 MiB seam-buffer limit and reports an error if that +limit is exceeded. Other clocks retain the existing queue behavior. + +## Reproducing the checks + +Build the actual CLI with the project's FFmpeg 7.1 SDK, then run: + +```sh +bash tests/run_source_clock_tests.sh +# Or select the executable and tools explicitly: +python3 tests/source_clock_tests.py --cut rust/target/release/smartcut \ + --ffmpeg ffmpeg --ffprobe ffprobe --work-dir /tmp +bash tests/run_vfr_tests.sh +``` + +The 80-case suite generates AVC, HEVC and AV1 inputs, includes the reviewer's +unchanged 90-second 720p command, and checks single/multiple selections, +copy-only and single-picture cuts, long holds and CFR controls in MP4/MKV. +It checks decoded counts, all display PTS, decode ordering, movie and video +track ends, compressed copied pictures and their decoded pixels. AVC/HEVC +VCL bytes are checked separately from repeated parameter-set NAL units. +Matroska stores no DTS; equal inferred DTS on its millisecond clock are +recorded, while backward DTS are rejected. MP4 DTS must strictly increase. + +Each run writes complete commands, hashes and results to a new directory. +`--fixtures /fixtures` reuses the exact input bytes. FFmpeg +7.1's synthetic B-frame VFR MP4 edit list can hide its last generated +picture: the reference pixel decoder alone uses `ignore_editlist=1` to +read all generated packets. Outputs always undergo normal demuxing and +strict decoding, with their end times checked independently. + +This suite tests timing and copying, not AV1 sample-description compliance, +SAR propagation, M2TS arrival clocks, GUI playback or hardware players. + +The review assertions use the original default indexing. Forcing `--index scan` +on that duplicate-PTS synthetic file retains a pre-existing unbounded-hold +case (796 pictures rather than 794, with an approximately 4 ms range bound) +in both v0.8.39 and this change. That separate selection/hold behavior is not +claimed fixed here. diff --git a/rust/crates/core/src/cut.rs b/rust/crates/core/src/cut.rs index cf8f6e7..4fc269d 100644 --- a/rust/crates/core/src/cut.rs +++ b/rust/crates/core/src/cut.rs @@ -635,17 +635,37 @@ struct Span { /// `seg.end` is the answer, held to the last picture's place and no /// further. A recording of fields of a declared rate (2:3 pulldown in an /// MP4) is on that grid too, and is answered the same way. -fn held_to_the_end(span: Span, anchor: Option, seg: &Segment, src: &Source, field: f64) -> Span { +fn held_to_the_end(span: Span, anchor: Option, seg: &Segment, src: &Source, + ctx: &SegmentCtx, next_picture: Option) -> Span { + let field = ctx.grid.unit(); let Some(a) = anchor else { return span }; - let until = if src.duration > 0.0 { + let mut until = if src.duration > 0.0 { seg.end.min(src.duration) } else { seg.end }; + let own = Grid::of(src); + let clock_cfr = own.divisor.is_some() && !src.video.variable_rate; + if clock_cfr { + // The CFR planner moves coarse-clock bounds just before a picture + // to settle selection. That margin is not display time to remove. + if let Some(next) = next_picture.filter(|t| *t >= until && *t - until < src.video.frame_duration()) { + until = next; + } else if src.duration > until && src.duration - until <= 3.5 * src.video.time_base { + until = src.duration; + } + } // Held to [`FAR`] for the reason the copy's own placing is. let held = ((until - a).min(FAR) / field).round() as i64; - let own = Grid::of(src); - if coded_length_unbounded(&src.video) && own.unit() == field { + // Refining the clock must not remove a normal picture's coded floor: + // a varying 23.976 MP4 can end a range only 4 ms into its last picture. + // Only the original unbounded-length case and newly refined CFR grids + // may replace that floor by measured coverage. Genuine VFR holds keep + // their existing semantics, including cuts entirely inside one hold. + // Inside a kept range the next segment resumes at a measured picture, + // so preserving a nominal floor there would shift every later picture. + let continuing = own.divisor.is_some() && !ctx.last; + if (coded_length_unbounded(&src.video) || clock_cfr || continuing) && own.unit() == field { // The last picture starts at most a frame of this grid before the // end its coded length gives (a field before, for the second field // of a pair), and it is up for at least a unit. @@ -745,6 +765,9 @@ struct SegmentCtx<'a> { /// Whether this is the opening segment of its keep-range, which is where /// the range's audio boundary decision is made. first: bool, + /// The end of a keep-range, rather than a seam inside it. A normal + /// picture retains its coded floor here even on a finer source clock. + last: bool, /// What the recording says about its own colour, read once for the /// whole cut. See [`signalling_of`]. signalling: &'a Signalling, @@ -832,6 +855,10 @@ struct Writer { /// Only over the cut's last range, and only where that is of use; see /// there. keep_last: bool, + /// On a refined clock the last displayed picture must remain writable + /// until its exact hold is known, even if several B pictures follow it + /// in decode order. Decoder reorder depth alone does not bound that run. + hold_latest: bool, /// Where the output's pictures end, in units, for the last of them to /// be up until. /// @@ -1464,6 +1491,19 @@ impl Writer { // pulldown does not move the boundary either. let keep = (self.depth * 2).max(0) + if self.keep_last { 2 } else { 0 }; while self.held_halves() > keep { + // A copied P picture can be followed by three B pictures while + // has_b_frames says two. Writing it now loses the chance to set + // its final hold: a 40/40/80 ms cut ended 27 ms early. Let it go + // when a later displayed picture arrives, or lengthen it in + // flush. Apply only to the clocks this change refines. + if self.hold_latest && self.pending.front().is_some_and(|front| + self.pending.iter().all(|p| p.display <= front.display)) + { + if self.pending.iter().map(|p| p.packet.size()).sum::() > HELD_AT_MOST { + bail!("last-picture reorder buffer exceeds the seam buffer limit"); + } + break; + } self.emit_one()?; } Ok(()) @@ -4664,7 +4704,8 @@ fn copy_segment( } bail!("could not find the entry point at {:.3}s", seg.start); } - Ok(held_to_the_end(span, anchor, seg, src, field)) + let next_picture = seg.copy_until.map(|_| seg.end); + Ok(held_to_the_end(span, anchor, seg, src, ctx, next_picture)) } /// How the pictures of a re-encoded segment are produced. @@ -5404,6 +5445,14 @@ fn open_encoder_as( /// [`Grid`]. const FINE: i64 = 32; +fn needs_source_clock(variable: bool, frame: f64, tick: f64) -> bool { + if !(frame.is_finite() && frame > 0.0 && tick.is_finite() && tick > 0.0) { + return false; + } + let ticks = frame / tick; + variable || (ticks - ticks.round()).abs() > 1e-6 +} + /// The units the output timeline is counted in. /// /// **A field of the rate the recording is built on**, which for everything @@ -5433,12 +5482,60 @@ struct Grid { den: i64, /// Units per field. sub: i64, + /// Reduce only a newly aligned clock. Existing grids retain their exact + /// timescale and sample tables, even where the fraction could be reduced. + divisor: Option, } impl Grid { fn of(src: &Source) -> Grid { let v = &src.video; - Grid::from_rates(v.frame_rate, v.base_rate, v.variable_rate).held_within(src.duration) + let grid = Grid::from_rates(v.frame_rate, v.base_rate, v.variable_rate).held_within(src.duration); + // A fine container clock alone is not a reason to change a CFR + // output. Only varying pictures, or a frame period the source clock + // cannot state exactly (e.g. 23.976 on milliseconds), need it. + if !src.on_a_ts && !v.interlaced() + && needs_source_clock(v.variable_rate, v.frame_duration(), v.time_base) + { + grid.on_clock(v.time_base, src.duration) + } else { + grid + } + } + + /// Keep the container's timestamp ticks representable, including on a + /// nominally constant recording. A rounded average is not its clock. + fn on_clock(self, seconds_per_tick: f64, duration: f64) -> Grid { + if !(seconds_per_tick.is_finite() && seconds_per_tick > 0.0) { + return self; + } + let clock = ff::Rational::from(seconds_per_tick).reduce(); + let (n, d) = (i64::from(clock.numerator()), i64::from(clock.denominator())); + if n <= 0 || d <= 0 { return self; } + let Some(needed) = d.checked_mul(self.den) else { return self }; + let Some(available) = n.checked_mul(2 * self.num) else { return self }; + let step = needed / unsafe { ff::ffi::av_gcd(needed, available) }; + if self.sub % step == 0 { + return self; + } + // Any multiple of `step` represents both the field and source clock. + // Keep at least the existing precision, without taking the LCM with + // FINE: that can needlessly multiply the clock and exhaust the bound + // on a long recording whose GOP-sized timestamps fit comfortably. + let Some(groups) = self.sub.checked_add(step - 1).map(|s| s / step) else { return self }; + let Some(sub) = groups.checked_mul(step) else { return self }; + if self.num.checked_mul(sub).and_then(|v| v.checked_mul(2)).is_none() { + return self; + } + let divisor = unsafe { ff::ffi::av_gcd(2 * self.num * sub, self.den) }; + let aligned = Grid { sub, divisor: Some(divisor), ..self }; + let scale = aligned.timescale(); + if scale > i64::from(i32::MAX) + || (duration.is_finite() && duration > 0.0 && scale as f64 * duration > f64::from(i32::MAX)) + { + return self; + } + aligned } /// The grid, with ticks coarse enough that the longest gap the recording @@ -5477,6 +5574,7 @@ impl Grid { num: i64::from(n).max(1), den: i64::from(d).max(1), sub: self.sub, + divisor: None, } } @@ -5528,7 +5626,7 @@ impl Grid { fn of_rate(num: i64, den: i64, sub: i64) -> Grid { let most = i64::from(i32::MAX) / (2 * sub); if num <= most { - return Grid { num, den, sub }; + return Grid { num, den, sub, divisor: None }; } let (mut n, mut d) = (0i32, 0i32); // Bounds both terms, which is what is wanted: a frame rate is over @@ -5538,12 +5636,17 @@ impl Grid { num: i64::from(n).max(1), den: i64::from(d).max(1), sub, + divisor: None, } } /// Ticks per second in the output's own time base. fn timescale(self) -> i64 { - 2 * self.num * self.sub + 2 * self.num * self.sub / self.divisor.unwrap_or(1) + } + + fn ticks_per_unit(self) -> i64 { + self.den / self.divisor.unwrap_or(1) } fn time_base(self) -> ff::Rational { @@ -5552,7 +5655,7 @@ impl Grid { /// How long one unit lasts, in seconds. fn unit(self) -> f64 { - self.den as f64 / self.timescale() as f64 + self.ticks_per_unit() as f64 / self.timescale() as f64 } /// What a picture shown for `fields` fields occupies. @@ -6014,6 +6117,7 @@ fn reencode_segment( // the gap to the previous picture is a frame by construction and the // range is answered by the picture it already had. let mut still_on_screen: Option = None; + let mut next_picture: Option = None; let least = src.video.least_gap(); macro_rules! place { @@ -6081,6 +6185,7 @@ fn reencode_segment( let Some(pts) = frame.pts() else { continue }; let t = pts as f64 * in_tb - src.start_time; if t >= seg.end - tol { + next_picture = Some(next_picture.map_or(t, |old| old.min(t))); past_end = true; continue; } @@ -6303,7 +6408,7 @@ fn reencode_segment( seg.start, seg.end, span.pictures, ); } - Ok(held_to_the_end(span, anchor, seg, src, field)) + Ok(held_to_the_end(span, anchor, seg, src, ctx, next_picture)) } /// Where the streams go on the way out. @@ -9407,14 +9512,12 @@ fn cut_into( } }; let grid = Grid::of(src); - let den = grid.den; - - // Output timebase: one tick per 1/(2*num*sub) second, so a *unit* is - // exactly `den` ticks, a field is `sub` of them and a normal frame twice - // that. 30000/1001 lands on 1/60000 with 1001 ticks per field -- integer - // arithmetic throughout, no rounding anywhere on the timeline, and - // 3-field pictures are representable. See [`Grid`] for the recording - // that needs the units smaller than a field. + let den = grid.ticks_per_unit(); + + // A unit is `den` output ticks, a field is `sub` units. A newly aligned + // source clock reduces both together; untouched grids retain exactly + // their old time base and sample tables. 30000/1001 remains 1001/60000 s + // per field. See [`Grid`] for the finer units varying pictures need. let mut muxer_opts = ff::Dictionary::new(); let timescale = grid.timescale().to_string(); let to_ts = writing_ts(output); @@ -10897,6 +11000,7 @@ fn cut_into( held: None, opening: false, keep_last: false, + hold_latest: false, ends: None, top_written: None, }; @@ -11409,8 +11513,11 @@ fn cut_into( // [`Writer::ends`] -- on its own grid only, as [`held_to_the_end`]. writer.keep_last = reel_no + 1 == reels.len() && plan_no + 1 == reel_plans[reel_no].len() - && last_held_to_the_end(&rsrc.video) + && (last_held_to_the_end(&rsrc.video) + || (!rsrc.on_a_ts && !rsrc.video.interlaced() + && !rsrc.video.variable_rate && Grid::of(rsrc).divisor.is_some())) && Grid::of(rsrc).unit() == grid.unit(); + writer.hold_latest = writer.keep_last && Grid::of(rsrc).divisor.is_some(); for (n, seg) in plan.segments.iter().enumerate() { let first_segment = n == 0; // A stretch written in the master's shape -- a reel that does not @@ -11436,6 +11543,7 @@ fn cut_into( offset: target_start - plan.t_in, subpictures: thread.graphics, first: first_segment, + last: n + 1 == plan.segments.len(), signalling: &signalling, }; // Each segment reports the span it actually occupied, so the @@ -12037,6 +12145,45 @@ fn cut_into( mod tests { use super::*; + #[test] + fn container_ticks_survive_a_rate_derived_grid() { + for (rate, base, variable, tick) in [ + (6000.0 / 319.0, 25.0, true, 1.0 / 12800.0), + (30000.0 / 1001.0, 30000.0 / 1001.0, false, 0.001), + ] { + let before = Grid::from_rates(rate, base, variable); + let grid = before.on_clock(tick, 6000.0); + assert!(grid.unit() <= before.unit()); + for i in 0..10000 { + let t = i as f64 * tick; + let restored = (t / grid.unit()).round() * grid.unit(); + assert!((restored - t).abs() < 1e-10); + } + assert!(grid.timescale() <= i64::from(i32::MAX)); + } + } + + #[test] + fn ordinary_cfr_does_not_acquire_a_new_clock() { + for (rate, tick) in [(25.0, 1.0 / 12800.0), (25.0, 0.001), + (24000.0 / 1001.0, 1.0 / 24000.0), + (30000.0 / 1001.0, 1.0 / 30000.0)] { + assert!(!needs_source_clock(false, 1.0 / rate, tick)); + } + assert!(needs_source_clock(false, 1001.0 / 24000.0, 0.001)); + assert!(needs_source_clock(true, 1.0 / 25.0, 1.0 / 12800.0)); + let original = Grid::from_rates(25.0, 25.0, false); + let already = original.on_clock(1.0 / 25.0, 60.0); + assert_eq!(already.timescale(), original.timescale()); + assert_eq!(already.sub, original.sub); + for bad in [0.0, -1.0, f64::NAN, f64::INFINITY] { + assert!(!needs_source_clock(true, 0.04, bad)); + assert_eq!(original.on_clock(bad, 60.0).timescale(), original.timescale()); + } + } + + + /// Every `.sup` of one cut has a name of its own. A transport stream's /// are named as they always were, by language or else by PID; a Matroska /// file's, whose streams all sit on 0, by their stream where the language diff --git a/rust/crates/core/src/index.rs b/rust/crates/core/src/index.rs index 0538fce..beb0cd8 100644 --- a/rust/crates/core/src/index.rs +++ b/rust/crates/core/src/index.rs @@ -417,6 +417,13 @@ fn varies(shown: &[f64], fd: f64) -> bool { if fd <= 0.0 || shown.len() < ENOUGH { return false; } + // The average can hide repeated holds: 40/40/80 ms averages to + // 53 ms, below the old 1.75-frame threshold. Use the typical measured + // gap as well; a 2:3 cadence and millisecond rounding still fit. + let mut gaps: Vec = shown.windows(2) + .map(|w| w[1] - w[0]).filter(|g| g.is_finite() && *g > 0.0).collect(); + gaps.sort_by(f64::total_cmp); + let fd = gaps.get(gaps.len().saturating_sub(1) / 2).copied().map_or(fd, |g| fd.min(g)); let (mut held, mut dense, mut counted) = (0usize, 0usize, 0usize); for w in shown.windows(2) { let gap = w[1] - w[0]; @@ -612,6 +619,16 @@ impl IndexSource for ContainerIndex { { bail!("an MPEG program or transport stream keeps no seek table"); } + // An MP4's public table contains decode times. With B pictures, + // outline_of cannot use it to classify display gaps. A progressive + // stream averaging materially below its declared rate may contain + // repeated holds (40/40/80 ms), so let the caller's packet scan ask + // its PTS instead. Constant-rate sources retain the fast table path. + if !video.variable_rate && !video.interlaced() && video.has_b_frames > 0 + && video.base_rate > video.frame_rate * 1.01 && video.base_rate < 1000.0 + { + bail!("reordered pictures below the declared rate need their display times scanned"); + } // How long the recording is, by the container's own reckoning. Only // used to ask whether the table below covers it, so a container that // will not say how long it is simply asks nothing. @@ -2064,6 +2081,26 @@ pub fn sample_bit_rate( mod tests { use super::*; + #[test] + fn repeated_holds_are_not_hidden_by_the_average() { + let mut shown = vec![0.0]; + for i in 0..239 { + shown.push(shown.last().unwrap() + if i % 3 == 2 { 0.08 } else { 0.04 }); + } + assert!(varies(&shown, 319.0 / 6000.0)); + } + + #[test] + fn rounded_ntsc_and_two_three_cadence_are_not_variable() { + let ntsc: Vec<_> = (0..240).map(|i| (i as f64 * 1001.0 / 30.0).round() / 1000.0).collect(); + assert!(!varies(&ntsc, 1001.0 / 30000.0)); + let mut film = vec![0.0]; + for i in 0..239 { + film.push(film.last().unwrap() + if i % 2 == 0 { 0.04 } else { 0.06 }); + } + assert!(!varies(&film, 0.04)); + } + /// Entry points every `every` seconds from `from` to `to`. fn every(from: f64, to: f64, every: f64) -> Vec { let mut out = Vec::new(); diff --git a/rust/crates/core/src/seek_index.rs b/rust/crates/core/src/seek_index.rs index 27972bf..2784e5a 100644 --- a/rust/crates/core/src/seek_index.rs +++ b/rust/crates/core/src/seek_index.rs @@ -144,7 +144,9 @@ static SERIAL: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new( /// the walk sees that its pictures come three times as close: an index /// written before has no such figure, and a cut opened from it would be /// planned on the average where a fresh open plans on the pictures. -pub const VERSION: u32 = 18; +/// 19: repeated holds are measured against the typical gap as well as the +/// average. Older caches may have classified 40/40/80 ms as constant. +pub const VERSION: u32 = 19; const MAGIC: &[u8; 4] = b"SCIX"; diff --git a/tests/ci.sh b/tests/ci.sh index 55166ad..20451e3 100755 --- a/tests/ci.sh +++ b/tests/ci.sh @@ -28,7 +28,7 @@ SUITES=( audio_codec audio_smart audio_format audio_head bilingual index proxy disc bdav udf dvd bd_audio - vp9_av1 vfr demux transrate join + vp9_av1 vfr source_clock demux transrate join ) [ $# -gt 0 ] && SUITES=("$@") diff --git a/tests/run_source_clock_tests.sh b/tests/run_source_clock_tests.sh new file mode 100755 index 0000000..c610837 --- /dev/null +++ b/tests/run_source_clock_tests.sh @@ -0,0 +1,4 @@ +#!/usr/bin/env bash +set -euo pipefail +cd "$(dirname "$0")/.." +exec python3 tests/source_clock_tests.py "$@" diff --git a/tests/source_clock_tests.py b/tests/source_clock_tests.py new file mode 100644 index 0000000..2313b3b --- /dev/null +++ b/tests/source_clock_tests.py @@ -0,0 +1,295 @@ +"""Source clocks and range-end coverage, using real files and the built CLI. + +Python standard library only. All inputs, commands, hashes and results are +kept in a fresh directory; --fixtures reuses exactly the same input bytes. +The 720p reproducer deliberately keeps the reviewer's command unchanged. +""" +import argparse +from fractions import Fraction +import hashlib +import json +import os +from pathlib import Path +import re +import subprocess +import tempfile +import time + + +def sha(path): + with Path(path).open('rb') as f: + return hashlib.file_digest(f, 'sha256').hexdigest() + + +def main(): + ap = argparse.ArgumentParser() + ap.add_argument('--cut', type=Path, default=Path('rust/target/release/smartcut' + ('.exe' if os.name == 'nt' else ''))) + ap.add_argument('--ffmpeg', default='ffmpeg') + ap.add_argument('--ffprobe', default='ffprobe') + ap.add_argument('--work-dir', type=Path, default=Path(tempfile.gettempdir())) + ap.add_argument('--fixtures', type=Path) + ap.add_argument('--review-source', type=Path) + args = ap.parse_args() + args.work_dir.mkdir(parents=True, exist_ok=True) + root = Path(tempfile.mkdtemp(prefix='smartcut-source-clock-', dir=args.work_dir)).resolve() + fx = args.fixtures.resolve() if args.fixtures else root / 'fixtures' + fx.mkdir(parents=True, exist_ok=True) + result = {'test_sha256': sha(__file__), 'cut': str(args.cut.resolve()), + 'cut_sha256': sha(args.cut), 'commands': [], 'fixtures': {}, 'cases': [], + 'source_pixel_oracle': 'Generated MP4s: ignore_editlist=1 decodes all generated packets; outputs use normal demux/decoding.'} + + def run(cmd, key, check=True): + cmd = list(map(str, cmd)) + start = time.monotonic() + p = subprocess.run(cmd, capture_output=True) + result['commands'].append({'command': cmd, 'exit_code': p.returncode, + 'seconds': time.monotonic() - start}) + (root / (key + '.log')).write_bytes(p.stdout + b'\nSTDERR\n' + p.stderr) + if check and p.returncode: + raise RuntimeError(f'{key}: {p.stderr.decode(errors="replace")[-1800:]}') + return p + + def inspect(path, key): + p = run([args.ffprobe, '-v', 'error', '-select_streams', 'v:0', + '-show_streams', '-show_format', '-show_packets', '-show_data_hash', 'sha256', + '-of', 'json', path], key) + data = json.loads(p.stdout) + data['sorted'] = sorted(data['packets'], key=lambda x: int(x['pts'])) + tb = Fraction(data['streams'][0]['time_base']) + data['times'] = [float(int(p['pts']) * tb) for p in data['sorted']] + codec = data['streams'][0]['codec_name'] + if codec in ('h264', 'hevc'): + # SmartCut may repeat parameter sets at an entry point. Compare + # the compressed pictures (VCL NALs) separately from those headers. + types = '1-5' if codec == 'h264' else '0-31' + hashes = run([args.ffmpeg, '-v', 'error', '-i', path, '-map', '0:v:0', + '-c:v', 'copy', '-bsf:v', 'filter_units=pass_types=' + types, + '-f', 'framehash', '-hash', 'sha256', '-'], key + '-vcl') + hashes = [line.rsplit(',', 1)[1].strip() for line in hashes.stdout.decode().splitlines() + if line and not line.startswith('#')] + assert len(hashes) == len(data['packets']) + data['picture_hashes'] = [h for _, h in sorted(zip(data['packets'], hashes), + key=lambda item: int(item[0]['pts']))] + else: + data['picture_hashes'] = [p['data_hash'] for p in data['sorted']] + return data + + def pixels(path, key, source=False): + # FFmpeg 7.1's muxer can end the edit list at the final PTS of these + # reordered VFR generators, hiding their last encoded picture from + # normal decoding. All 240 generated packets are the reference here. + # Never bypass an output's edit list: it must decode normally. + options = ['-ignore_editlist', '1'] if source and path.suffix == '.mp4' else [] + p = run([args.ffmpeg, '-v', 'error', *options, '-i', path, '-map', '0:v:0', + '-fps_mode', 'passthrough', '-f', 'framemd5', '-'], key) + return [line.rsplit(',', 1)[1].strip() for line in p.stdout.decode().splitlines() + if line and not line.startswith('#')] + + def generate(name, cmd): + path = fx / name + if not args.fixtures: + run([args.ffmpeg, '-hide_banner', '-v', 'error', '-y', *cmd, path], + 'generate-' + name) + result['fixtures'][name] = {'path': str(path), 'sha256': sha(path)} + return path + + base = ['-f', 'lavfi', '-i', 'testsrc2=size=192x112:rate=25', '-frames:v', '240'] + codec_options = { + 'h264': ['-c:v', 'libx264', '-preset', 'veryfast', '-crf', '20', + '-x264-params', 'keyint=24:min-keyint=24:scenecut=0:bframes=3'], + 'hevc': ['-c:v', 'libx265', '-preset', 'ultrafast', '-crf', '24', + '-x265-params', 'keyint=24:min-keyint=24:scenecut=0:bframes=3:log-level=error'], + 'av1': ['-c:v', 'libaom-av1', '-cpu-used', '8', '-crf', '32', '-b:v', '0', '-g', '24'], + } + specs = [] + for codec, options in codec_options.items(): + mp4 = generate(f'{codec}-vfr.mp4', base + ['-vf', + 'setpts=N+floor(N/3)', '-fps_mode', 'passthrough', + *options, '-video_track_timescale', '12800']) + mkv = generate(f'{codec}-vfr.mkv', ['-i', mp4, '-map', '0:v:0', '-c', 'copy']) + specs.extend([(mp4, 'vfr'), (mkv, 'vfr')]) + for codec in ('h264', 'hevc'): + for label, rate, timescale in [('25', '25', '12800'), ('ntsc', '24000/1001', '24000')]: + mp4 = generate(f'{codec}-{label}.mp4', ['-f', 'lavfi', '-i', + f'testsrc2=size=192x112:rate={rate}', '-frames:v', '240', + *codec_options[codec], '-video_track_timescale', timescale]) + mkv = generate(f'{codec}-{label}.mkv', ['-i', mp4, '-map', '0:v:0', '-c', 'copy']) + specs.extend([(mp4, 'cfr'), (mkv, 'coarse' if label == 'ntsc' else 'cfr')]) + hold = generate('holds.mp4', ['-f', 'lavfi', '-i', 'testsrc2=size=192x112:rate=30', + '-frames:v', '180', '-vf', r'setpts=N+gte(N\,60)*60+gte(N\,120)*60', + '-fps_mode', 'passthrough', *codec_options['h264'], + '-video_track_timescale', '30000']) + hold_mkv = generate('holds.mkv', ['-i', hold, '-map', '0:v:0', '-c', 'copy']) + if args.fixtures and not (fx / 'review.mp4').exists(): + saved = json.loads((fx / 'fixtures.json').read_text(encoding='utf-8')) + review = Path(saved['review.mp4']['path']) + result['fixtures']['review.mp4'] = {'path': str(review), 'sha256': sha(review)} + elif args.review_source and not args.fixtures: + review = args.review_source.resolve() + result['fixtures']['review.mp4'] = {'path': str(review), 'sha256': sha(review)} + else: + review = generate('review.mp4', ['-f', 'lavfi', '-i', + 'testsrc2=rate=24000/1001:size=1280x720', '-t', '90', '-vf', + r'setpts=(N*1001/24000-if(eq(mod(N\,300)\,299)\,0.025\,0))/TB', + '-fps_mode', 'passthrough', '-c:v', 'libx264', '-g', '120', + '-video_track_timescale', '1000']) + # A reference to an existing exact reproducer also survives --fixtures. + manifest = fx / 'fixtures.json' + if args.fixtures: + saved = json.loads(manifest.read_text(encoding='utf-8')) + if not (fx / 'review.mp4').exists(): + review = Path(saved['review.mp4']['path']) + result['fixtures']['review.mp4'] = {'path': str(review), 'sha256': sha(review)} + assert saved == result['fixtures'], 'fixture bytes changed' + else: + manifest.write_text(json.dumps(result['fixtures'], indent=2), encoding='utf-8') + + sources = {} + for path in [p for p, _ in specs] + [hold, hold_mkv, review]: + sources[path] = inspect(path, 'source-' + path.name) + sources[path]['pixels'] = pixels(path, 'pixels-' + path.name, source=True) + assert len(sources[path]['pixels']) == len(sources[path]['times']), path + + def save(): + result['passed'] = sum(c['passed'] for c in result['cases']) + result['total'] = len(result['cases']) + (root / 'results.json').write_text(json.dumps(result, indent=2), encoding='utf-8') + + def case(path, name, ranges, expected, end, suffix=None, kind='', extra=()): + src = sources[path] + out = root / (name + (suffix or path.suffix)) + command = [args.cut.resolve(), path, '-o', out] + for a, b in ranges: + command += ['--keep', f'{a:.12f}-{b:.12f}'] + command += list(extra) + row = {'name': name, 'kind': kind, 'input': str(path), 'input_sha256': sha(path), + 'ranges': ranges, 'output': str(out), 'issues': []} + try: + proc = run(command, name, check=False) + row['exit_code'] = proc.returncode + if proc.returncode: + raise RuntimeError('export failed') + got = inspect(out, name + '-probe') + pts = got['times'] + count = len(expected) + row['frames'] = len(pts) + row['expected_frames'] = count + if len(pts) != count: + row['issues'].append('frame_count') + tolerance = max(1e-6, float(Fraction(got['streams'][0]['time_base']))) + 1e-7 + error = max((abs(t - at) for t, (_, at) in zip(pts, expected)), default=0.0) + row['max_display_error_seconds'] = error + if error > tolerance: + row['issues'].append('display_pts') + packets = got['packets'] + dts = [int(p['dts']) for p in packets if 'dts' in p] + # Matroska stores PTS, not DTS. On the reviewer's duplicate-PTS + # source its millisecond clock can give equal inferred DTS. + row['equal_dts'] = sum(b == a for a, b in zip(dts, dts[1:])) + row['dts_monotonic'] = all((b >= a if out.suffix == '.mkv' else b > a) + for a, b in zip(dts, dts[1:])) + if not row['dts_monotonic']: + row['issues'].append('dts') + strict = run([args.ffmpeg, '-v', 'error', '-xerror', '-err_detect', 'explode', + '-i', out, '-map', '0:v:0', '-f', 'null', '-'], name + '-strict', check=False) + row['strict_decode'] = strict.returncode == 0 + if not row['strict_decode']: + row['issues'].append('strict_decode') + # These outputs contain video only. Check the movie duration as + # well as every display PTS: decoding alone cannot detect a + # final hold shortened by the muxer's sample-table durations. + duration = float(got['format']['duration']) + row['presentation_duration'] = duration + row['stream_duration'] = got['streams'][0].get('duration') + row['expected_duration'] = end + if abs(duration - end) > max(tolerance, .001001): + row['issues'].append('last_picture_duration') + if out.suffix == '.mp4' and abs(float(row['stream_duration']) - end) > max(tolerance, .001001): + row['issues'].append('video_track_duration') + log = proc.stdout + proc.stderr + intervals = [(float(a), float(b)) for a, b in + re.findall(rb'^\s*copy\s+([\d.]+)\s+->\s*([\d.]+)', log, re.M)] + decoded = pixels(out, name + '-pixels') + row['decoded_frames'] = len(decoded) + if len(decoded) != count: + row['issues'].append('decoded_count') + copied = [i for i, (source_index, _) in enumerate(expected) + if any(a + .001 < src['times'][source_index] < b - .001 for a, b in intervals)] + row['copy_intervals'] = intervals + row['copy_interior_frames'] = len(copied) + row['copied_packet_changes'] = [i for i in copied if i >= len(got['sorted']) or + got['sorted'][i]['data_hash'] != src['sorted'][expected[i][0]]['data_hash']] + row['copied_picture_errors'] = [i for i in copied if i >= len(got['picture_hashes']) or + got['picture_hashes'][i] != src['picture_hashes'][expected[i][0]]] + row['copied_pixel_errors'] = [i for i in copied if i >= len(decoded) or + decoded[i] != src['pixels'][expected[i][0]]] + if row['copied_picture_errors'] or row['copied_pixel_errors']: + row['issues'].append('copied_picture') + row['output_sha256'] = sha(out) + row['time_base'] = got['streams'][0]['time_base'] + if kind == 'review' and len(ranges) == 2: + seam = sum(20 <= t < 38 for t in src['times']) + row['seam_gap_seconds'] = pts[seam] - pts[seam-1] + if not .040 <= row['seam_gap_seconds'] <= .043: + row['issues'].append('four_ms_regression') + except Exception as exc: + row['issues'].append('exception') + row['error'] = str(exc) + row['passed'] = not row['issues'] + result['cases'].append(row) + save() + print(('PASS' if row['passed'] else 'FAIL'), name, row['issues'], flush=True) + + cuts = {'partial': [(13, 227)], 'multi': [(13, 81), (100, 227)], + 'aligned': [(24, 216)], 'one': [(23, 24)], 'full': [(0, 240)]} + for path, kind in specs: + src = sources[path] + stream = src['streams'][0] + # The generating cadence gives the final picture 40 ms. A B-frame + # MP4's stream.duration is a decode duration, not its displayed end. + final = ((240 * 1001 / 24000 if 'ntsc' in path.name else 9.6) if kind != 'vfr' + else 12.76) + times = src['times'] + [final] + for label, indices in cuts.items(): + expected, ranges, offset = [], [], 0.0 + for a, b in indices: + ranges.append((times[a], times[b])) + expected.extend((i, offset + times[i] - times[a]) for i in range(a, b)) + offset += times[b] - times[a] + case(path, path.stem + '-' + path.suffix[1:] + '-' + label, + ranges, expected, offset, kind=kind) + + for path in (hold, hold_mkv): + times = sources[path]['times'] + for label, ranges in [('ends-in-hold', [(0.6, 2.7)]), + ('inside-hold', [(2.2, 2.7)]), + ('joins-held-ranges', [(0.6, 2.7), (4.4, 6.8)])]: + expected, offset = [], 0.0 + for a, b in ranges: + indices = [i for i, t in enumerate(times) if a - 1e-8 <= t < b - 1e-8] + if not indices: + indices = [max(i for i, t in enumerate(times) if t < a)] + expected.extend((i, offset + max(times[i], a) - a) for i in indices) + offset += b - a + case(path, label + '-' + path.suffix[1:], ranges, expected, offset, kind='hold') + + for label, ranges in [('multi', [(20, 38), (60, 75)]), ('single', [(20, 38)])]: + times = sources[review]['times'] + expected, offset = [], 0.0 + for a, b in ranges: + indices = [i for i, t in enumerate(times) if a <= t < b] + first = times[indices[0]] + expected.extend((i, offset + times[i] - first) for i in indices) + # This normal 23.976 picture keeps its coded floor, including + # the reviewer's picture at 37.996 s before a bound of 38 s. + offset += max(b - first, times[indices[-1]] - first + 1001/24000) + for suffix in ('.mp4', '.mkv'): + case(review, 'review-' + label + suffix, ranges, expected, offset, + suffix=suffix, kind='review') + save() + print(f"{result['passed']}/{result['total']} passed; {root}", flush=True) + return 0 if result['passed'] == result['total'] else 1 + + +if __name__ == '__main__': + raise SystemExit(main()) From d9313dc11900e874de9bb29caf9d98ccfbff25a8 Mon Sep 17 00:00:00 2001 From: SteveYu000 <111431635+SteveYu000@users.noreply.github.com> Date: Fri, 6 Nov 2026 12:41:08 +0800 Subject: [PATCH 2/2] Keep CFR on its nominal grid and release oversized holds Limit source-clock refinement to inputs classified as variable-rate. A millisecond-clock CFR source, including 29.97 VP9 in Matroska, keeps its nominal output cadence. Remove the refined-CFR coverage special case. Release the latest picture through emit_one when its queue exceeds the existing budget instead of refusing the cut. Document the full packet scan and its opening-time cost for suspect B-frame sources. Add 15 CFR-to-MP4 checks and a real Writer buffer-limit regression test. The same fixed inputs pass 60/95 on v0.8.39, 80/95 on the previous PR, and 95/95 here. Rust/MSVC/FFmpeg 7.1.1 build, VFR 8/8, workspace units 594 passed (four known Windows exclusions), and clippy checked locally. --- docs/technical/building.md | 2 +- docs/technical/source-clock.md | 30 ++++---- rust/crates/core/src/cut.rs | 123 ++++++++++++++++++++++----------- tests/source_clock_tests.py | 43 ++++++++++-- 4 files changed, 139 insertions(+), 59 deletions(-) diff --git a/docs/technical/building.md b/docs/technical/building.md index 3233d1a..5415b9c 100644 --- a/docs/technical/building.md +++ b/docs/technical/building.md @@ -88,7 +88,7 @@ bash tests/run_vp9_av1_tests.sh # VP9 and AV1: cut, and still the source's bash tests/run_sar_tests.sh # SAR: 44 exports + 16 outlines 60 bash tests/run_av1_mp4_tests.sh # AV1 MP4 seam levels and Matroska controls 21 bash tests/run_vfr_tests.sh # variable rate, both ways round 8 -bash tests/run_source_clock_tests.sh # source clocks, range ends and VFR copying 80 +bash tests/run_source_clock_tests.sh # source clocks, range ends and VFR copying 95 bash tests/run_demux_tests.sh # what a reader loses by reading what it does not 9 bash tests/run_transrate_tests.sh # fitting a disc: the identity, the size, the cost 15 bash tests/run_join_tests.sh # several recordings into one file, and the seams 17 diff --git a/docs/technical/source-clock.md b/docs/technical/source-clock.md index 03e7347..08b1a18 100644 --- a/docs/technical/source-clock.md +++ b/docs/technical/source-clock.md @@ -6,10 +6,12 @@ output timeline must also represent the source's display timestamps. ## When the clock changes -`Grid::of` refines a progressive, non-TS source only when it is variable-rate, -or its nominal frame period is not an integer number of source ticks (for -example, 23.976 fps stored in milliseconds). An ordinary 25 fps MP4, or a -23.976 fps MP4 on a 1/24000 clock, retains its existing grid and timescale. +`Grid::of` refines a progressive, non-TS source only when it is classified +as variable-rate. Constant-rate sources retain their nominal grid and +timescale even when a frame period is not a whole number of source ticks. +For example, 29.97 fps in Matroska has 33/34 ms timestamp steps because its +clock rounds to milliseconds. Written to MP4, that CFR source still has +exact 1001/30000 second steps, not the millisecond rounding of the input. An already suitable grid is returned unchanged, without fraction reduction. `on_clock` chooses a subdivision that represents both the field grid and @@ -26,8 +28,10 @@ NTSC and a 2:3 cadence still do not count as variable by that test. An unclassified progressive stream with B pictures and an average over 1% below its declared rate needs its display times read: the public container index contains decode timestamps. `ContainerIndex` declines that case, -so the default `auto` mode falls back to the packet scan. This can add a -sequential read for those sources; ordinary CFR keeps the fast table path. +so the default `auto` mode falls back to a full packet scan before cutting. +Opening a large file can therefore take noticeably longer, since the whole +file must be read instead of just its seek table. This is a packet scan, +not a full-video decode. Ordinary CFR keeps the fast table path. An explicitly required container index reports the reason it cannot answer. Seek-cache version 19 invalidates the former classification. @@ -45,17 +49,15 @@ normal 23.976 frame lasts about 41.708 ms. The existing `coded_length_unbounded` cases retain their time-based hold behavior, including a range entirely inside a long held picture. -For newly refined CFR, the decoder's next measured picture compensates for -the planner's coarse-clock selection margin. A copy has that coverage from -the planner's terminating access point, including its leading pictures. - On a refined clock, the final displayed picture also stays in the write queue until a later displayed picture arrives or the final hold is known. Decoder reorder depth alone is insufficient: a copied H.264 P picture can precede three B pictures while `has_b_frames` is two. Releasing that P picture too early left a 40/40/80 ms selection about 27 ms short. The queue -uses the existing 256 MiB seam-buffer limit and reports an error if that -limit is exceeded. Other clocks retain the existing queue behavior. +uses the existing 256 MiB seam-buffer limit. If that limit is exceeded, +`emit_one()` releases the picture as before so the cut can continue; its +final hold can then be shorter because that packet is no longer writable. +Other clocks retain the existing queue behavior. ## Reproducing the checks @@ -69,9 +71,11 @@ python3 tests/source_clock_tests.py --cut rust/target/release/smartcut \ bash tests/run_vfr_tests.sh ``` -The 80-case suite generates AVC, HEVC and AV1 inputs, includes the reviewer's +The 95-case suite generates AVC, HEVC, AV1 and VP9 inputs, includes the reviewer's unchanged 90-second 720p command, and checks single/multiple selections, copy-only and single-picture cuts, long holds and CFR controls in MP4/MKV. +The millisecond CFR controls also export to MP4 and require exact nominal +frame steps and average rates, including 29.97 fps VP9 from Matroska. It checks decoded counts, all display PTS, decode ordering, movie and video track ends, compressed copied pictures and their decoded pixels. AVC/HEVC VCL bytes are checked separately from repeated parameter-set NAL units. diff --git a/rust/crates/core/src/cut.rs b/rust/crates/core/src/cut.rs index 8936d1d..c035e1d 100644 --- a/rust/crates/core/src/cut.rs +++ b/rust/crates/core/src/cut.rs @@ -648,36 +648,26 @@ struct Span { /// further. A recording of fields of a declared rate (2:3 pulldown in an /// MP4) is on that grid too, and is answered the same way. fn held_to_the_end(span: Span, anchor: Option, seg: &Segment, src: &Source, - ctx: &SegmentCtx, next_picture: Option) -> Span { + ctx: &SegmentCtx) -> Span { let field = ctx.grid.unit(); let Some(a) = anchor else { return span }; - let mut until = if src.duration > 0.0 { + let until = if src.duration > 0.0 { seg.end.min(src.duration) } else { seg.end }; let own = Grid::of(src); - let clock_cfr = own.divisor.is_some() && !src.video.variable_rate; - if clock_cfr { - // The CFR planner moves coarse-clock bounds just before a picture - // to settle selection. That margin is not display time to remove. - if let Some(next) = next_picture.filter(|t| *t >= until && *t - until < src.video.frame_duration()) { - until = next; - } else if src.duration > until && src.duration - until <= 3.5 * src.video.time_base { - until = src.duration; - } - } // Held to [`FAR`] for the reason the copy's own placing is. let held = ((until - a).min(FAR) / field).round() as i64; // Refining the clock must not remove a normal picture's coded floor: // a varying 23.976 MP4 can end a range only 4 ms into its last picture. - // Only the original unbounded-length case and newly refined CFR grids - // may replace that floor by measured coverage. Genuine VFR holds keep + // Only the original unbounded-length case may replace that floor by + // measured coverage at a range end. Genuine VFR holds keep // their existing semantics, including cuts entirely inside one hold. // Inside a kept range the next segment resumes at a measured picture, // so preserving a nominal floor there would shift every later picture. let continuing = own.divisor.is_some() && !ctx.last; - if (coded_length_unbounded(&src.video) || clock_cfr || continuing) && own.unit() == field { + if (coded_length_unbounded(&src.video) || continuing) && own.unit() == field { // The last picture starts at most a frame of this grid before the // end its coded length gives (a field before, for the second field // of a pair), and it is up for at least a unit. @@ -1511,13 +1501,13 @@ impl Writer { // has_b_frames says two. Writing it now loses the chance to set // its final hold: a 40/40/80 ms cut ended 27 ms early. Let it go // when a later displayed picture arrives, or lengthen it in - // flush. Apply only to the clocks this change refines. + // flush. Apply only to the clocks this change refines. A bounded + // hold is best effort: over the limit, emit the picture as before + // rather than refuse the cut for a longer final hold. if self.hold_latest && self.pending.front().is_some_and(|front| self.pending.iter().all(|p| p.display <= front.display)) + && self.pending.iter().map(|p| p.packet.size()).sum::() <= HELD_AT_MOST { - if self.pending.iter().map(|p| p.packet.size()).sum::() > HELD_AT_MOST { - bail!("last-picture reorder buffer exceeds the seam buffer limit"); - } break; } self.emit_one()?; @@ -4757,8 +4747,7 @@ fn copy_segment( } bail!("could not find the entry point at {:.3}s", seg.start); } - let next_picture = seg.copy_until.map(|_| seg.end); - Ok(held_to_the_end(span, anchor, seg, src, ctx, next_picture)) + Ok(held_to_the_end(span, anchor, seg, src, ctx)) } /// How the pictures of a re-encoded segment are produced. @@ -5783,11 +5772,7 @@ fn open_encoder_as( const FINE: i64 = 32; fn needs_source_clock(variable: bool, frame: f64, tick: f64) -> bool { - if !(frame.is_finite() && frame > 0.0 && tick.is_finite() && tick > 0.0) { - return false; - } - let ticks = frame / tick; - variable || (ticks - ticks.round()).abs() > 1e-6 + variable && frame.is_finite() && frame > 0.0 && tick.is_finite() && tick > 0.0 } /// The units the output timeline is counted in. @@ -5828,9 +5813,9 @@ impl Grid { fn of(src: &Source) -> Grid { let v = &src.video; let grid = Grid::from_rates(v.frame_rate, v.base_rate, v.variable_rate).held_within(src.duration); - // A fine container clock alone is not a reason to change a CFR - // output. Only varying pictures, or a frame period the source clock - // cannot state exactly (e.g. 23.976 on milliseconds), need it. + // CFR keeps the nominal grid even when the source tick rounds each + // timestamp (e.g. 29.97 fps written as 33/34 ms in Matroska). + // Only pictures classified as varying need the source clock. if !src.on_a_ts && !v.interlaced() && needs_source_clock(v.variable_rate, v.frame_duration(), v.time_base) { @@ -5840,8 +5825,8 @@ impl Grid { } } - /// Keep the container's timestamp ticks representable, including on a - /// nominally constant recording. A rounded average is not its clock. + /// Keep a variable recording's container ticks representable. + /// A rounded average is not its clock. fn on_clock(self, seconds_per_tick: f64, duration: f64) -> Grid { if !(seconds_per_tick.is_finite() && seconds_per_tick > 0.0) { return self; @@ -6454,7 +6439,6 @@ fn reencode_segment( // the gap to the previous picture is a frame by construction and the // range is answered by the picture it already had. let mut still_on_screen: Option = None; - let mut next_picture: Option = None; let least = src.video.least_gap(); macro_rules! place { @@ -6522,7 +6506,6 @@ fn reencode_segment( let Some(pts) = frame.pts() else { continue }; let t = pts as f64 * in_tb - src.start_time; if t >= seg.end - tol { - next_picture = Some(next_picture.map_or(t, |old| old.min(t))); past_end = true; continue; } @@ -6745,7 +6728,7 @@ fn reencode_segment( seg.start, seg.end, span.pictures, ); } - Ok(held_to_the_end(span, anchor, seg, src, ctx, next_picture)) + Ok(held_to_the_end(span, anchor, seg, src, ctx)) } /// Where the streams go on the way out. @@ -11967,9 +11950,7 @@ fn cut_into( // [`Writer::ends`] -- on its own grid only, as [`held_to_the_end`]. writer.keep_last = reel_no + 1 == reels.len() && plan_no + 1 == reel_plans[reel_no].len() - && (last_held_to_the_end(&rsrc.video) - || (!rsrc.on_a_ts && !rsrc.video.interlaced() - && !rsrc.video.variable_rate && Grid::of(rsrc).divisor.is_some())) + && last_held_to_the_end(&rsrc.video) && Grid::of(rsrc).unit() == grid.unit(); writer.hold_latest = writer.keep_last && Grid::of(rsrc).divisor.is_some(); for (n, seg) in plan.segments.iter().enumerate() { @@ -12603,7 +12584,7 @@ mod tests { fn container_ticks_survive_a_rate_derived_grid() { for (rate, base, variable, tick) in [ (6000.0 / 319.0, 25.0, true, 1.0 / 12800.0), - (30000.0 / 1001.0, 30000.0 / 1001.0, false, 0.001), + (30000.0 / 1001.0, 30000.0 / 1001.0, true, 0.001), ] { let before = Grid::from_rates(rate, base, variable); let grid = before.on_clock(tick, 6000.0); @@ -12621,10 +12602,12 @@ mod tests { fn ordinary_cfr_does_not_acquire_a_new_clock() { for (rate, tick) in [(25.0, 1.0 / 12800.0), (25.0, 0.001), (24000.0 / 1001.0, 1.0 / 24000.0), - (30000.0 / 1001.0, 1.0 / 30000.0)] { + (30000.0 / 1001.0, 1.0 / 30000.0), + (24000.0 / 1001.0, 0.001), + (30000.0 / 1001.0, 0.001)] { assert!(!needs_source_clock(false, 1.0 / rate, tick)); } - assert!(needs_source_clock(false, 1001.0 / 24000.0, 0.001)); + assert!(needs_source_clock(true, 1001.0 / 24000.0, 0.001)); assert!(needs_source_clock(true, 1.0 / 25.0, 1.0 / 12800.0)); let original = Grid::from_rates(25.0, 25.0, false); let already = original.on_clock(1.0 / 25.0, 60.0); @@ -12638,6 +12621,66 @@ mod tests { + #[test] + fn a_last_picture_over_the_buffer_limit_is_written_not_refused() { + use ff::codec::packet::{Mut, Ref}; + + ff::init().unwrap(); + let sink = if cfg!(windows) { "NUL" } else { "/dev/null" }; + let mut output = ff::format::output_as(sink, "null").unwrap(); + { + let mut stream = output.add_stream(ff::encoder::find(ff::codec::Id::RAWVIDEO)).unwrap(); + stream.set_time_base((1, 50)); + let mut parameters = stream.parameters(); + unsafe { + let p = parameters.as_mut_ptr(); + (*p).codec_type = ff::ffi::AVMediaType::AVMEDIA_TYPE_VIDEO; + (*p).codec_id = ff::ffi::AVCodecID::AV_CODEC_ID_RAWVIDEO; + (*p).width = 1; + (*p).height = 1; + (*p).format = ff::ffi::AVPixelFormat::AV_PIX_FMT_GRAY8 as i32; + } + } + output.write_header().unwrap(); + let out_tb = output.stream(0).unwrap().time_base(); + let mut writer = Writer { + octx: output, av1_mp4: None, on_a_ts: false, into_ts: false, + field_ticks: 1, sub: 1, our_tb: (1, 50).into(), out_tb, depth: 2, + pending: Default::default(), seen: Default::default(), shown: Default::default(), + first_shown: None, last_dts: None, written: 0, skipped: 0, halves: 0, + audio: Vec::new(), lengths: Default::default(), captions: Vec::new(), + graphics: Vec::new(), subpictures: None, converted: Vec::new(), + progress: None, expected: 0, shrink: None, held: None, opening: false, + keep_last: true, hold_latest: true, ends: None, top_written: None, + }; + // Exercise the real 256 MiB threshold without allocating that much. + // These packets share one refcounted payload; queue counts their + // declared sizes, and the null muxer needs no encoded picture data. + let payload = ff::Packet::new(1 << 20); + let packet = || { + let mut p = ff::Packet::empty(); + unsafe { assert_eq!(ff::ffi::av_packet_ref(p.as_mut_ptr(), payload.as_ptr()), 0); } + p + }; + let count = HELD_AT_MOST / payload.size(); + for i in 0..count { + writer.queue(Emitted { + packet: packet(), display: if i == 0 { 1000 } else { (i as i64 - 1) * 2 }, fields: 2, + }).unwrap(); + } + assert_eq!(writer.written, 0); + assert_eq!(writer.pending.len(), count); + writer.queue(Emitted { + packet: packet(), display: (count as i64 - 1) * 2, fields: 2, + }).unwrap(); + assert!(writer.written > 0); + assert!(writer.pending.iter().map(|p| p.packet.size()).sum::() <= HELD_AT_MOST); + writer.flush().unwrap(); + assert_eq!(writer.written, count as i64 + 1); + assert_eq!(writer.skipped, 0); + writer.octx.write_trailer().unwrap(); + } + /// Every `.sup` of one cut has a name of its own. A transport stream's /// are named as they always were, by language or else by PID; a Matroska /// file's, whose streams all sit on 0, by their stream where the language diff --git a/tests/source_clock_tests.py b/tests/source_clock_tests.py index 2313b3b..5748d10 100644 --- a/tests/source_clock_tests.py +++ b/tests/source_clock_tests.py @@ -115,6 +115,11 @@ def generate(name, cmd): *codec_options[codec], '-video_track_timescale', timescale]) mkv = generate(f'{codec}-{label}.mkv', ['-i', mp4, '-map', '0:v:0', '-c', 'copy']) specs.extend([(mp4, 'cfr'), (mkv, 'coarse' if label == 'ntsc' else 'cfr')]) + vp9 = generate('vp9-ntsc.mkv', ['-f', 'lavfi', '-i', + 'testsrc2=size=192x112:rate=30000/1001', '-frames:v', '240', + '-c:v', 'libvpx-vp9', '-deadline', 'good', '-cpu-used', '6', + '-b:v', '0', '-crf', '30', '-g', '24', '-lag-in-frames', '0', '-auto-alt-ref', '0']) + specs.append((vp9, 'coarse-vp9')) hold = generate('holds.mp4', ['-f', 'lavfi', '-i', 'testsrc2=size=192x112:rate=30', '-frames:v', '180', '-vf', r'setpts=N+gte(N\,60)*60+gte(N\,120)*60', '-fps_mode', 'passthrough', *codec_options['h264'], @@ -155,7 +160,7 @@ def save(): result['total'] = len(result['cases']) (root / 'results.json').write_text(json.dumps(result, indent=2), encoding='utf-8') - def case(path, name, ranges, expected, end, suffix=None, kind='', extra=()): + def case(path, name, ranges, expected, end, suffix=None, kind='', extra=(), nominal_rate=None): src = sources[path] out = root / (name + (suffix or path.suffix)) command = [args.cut.resolve(), path, '-o', out] @@ -227,6 +232,22 @@ def case(path, name, ranges, expected, end, suffix=None, kind='', extra=()): row['issues'].append('copied_picture') row['output_sha256'] = sha(out) row['time_base'] = got['streams'][0]['time_base'] + row['avg_frame_rate'] = got['streams'][0].get('avg_frame_rate') + row['variable_rate'] = b'(the pictures vary)' in log + row['minimum_display_step'] = min((b-a for a, b in zip(pts, pts[1:])), default=None) + if nominal_rate is not None: + row['expected_nominal_rate'] = str(nominal_rate) + if row['variable_rate']: + row['issues'].append('cfr_classified_variable') + if out.suffix == '.mp4': + tb = Fraction(row['time_base']) + exact = [int(p['pts']) * tb for p in got['sorted']] + row['exact_nominal_steps'] = all(b-a == 1/nominal_rate + for a, b in zip(exact, exact[1:])) + if not row['exact_nominal_steps']: + row['issues'].append('nominal_cfr_steps') + if Fraction(row['avg_frame_rate']) != nominal_rate: + row['issues'].append('nominal_avg_frame_rate') if kind == 'review' and len(ranges) == 2: seam = sum(20 <= t < 38 for t in src['times']) row['seam_gap_seconds'] = pts[seam] - pts[seam-1] @@ -247,17 +268,29 @@ def case(path, name, ranges, expected, end, suffix=None, kind='', extra=()): stream = src['streams'][0] # The generating cadence gives the final picture 40 ms. A B-frame # MP4's stream.duration is a decode duration, not its displayed end. - final = ((240 * 1001 / 24000 if 'ntsc' in path.name else 9.6) if kind != 'vfr' + rate = Fraction(30000 if kind == 'coarse-vp9' else 24000, 1001) if 'ntsc' in path.name else Fraction(25) + final = ((float(240 / rate)) if kind != 'vfr' else 12.76) times = src['times'] + [final] for label, indices in cuts.items(): expected, ranges, offset = [], [], 0.0 for a, b in indices: ranges.append((times[a], times[b])) - expected.extend((i, offset + times[i] - times[a]) for i in range(a, b)) - offset += times[b] - times[a] + if kind.startswith('coarse'): + # CFR millisecond ticks are rounded timestamps, not + # actual variation. Keep the nominal frame grid. + expected.extend((i, offset + float((i-a) / rate)) for i in range(a, b)) + offset += float((b-a) / rate) + else: + expected.extend((i, offset + times[i] - times[a]) for i in range(a, b)) + offset += times[b] - times[a] case(path, path.stem + '-' + path.suffix[1:] + '-' + label, - ranges, expected, offset, kind=kind) + ranges, expected, offset, kind=kind, + suffix='.mp4' if kind == 'coarse-vp9' else None, + nominal_rate=rate if kind != 'vfr' else None) + if kind == 'coarse': + case(path, path.stem + '-mkv-to-mp4-' + label, + ranges, expected, offset, suffix='.mp4', kind=kind, nominal_rate=rate) for path in (hold, hold_mkv): times = sources[path]['times']