From c8fafec3935aa03864cdc1e88eb8b0af1361a3d5 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Wed, 5 Aug 2026 17:31:43 -0700 Subject: [PATCH] Keep the seam plan to Blu-ray, and stop a missed crossing truncating a title MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the same escalation, both silent-wrong-output. The plan was built for every multi-clip title. Only a Blu-ray PlayItem's IN/OUT are positions in the clock the PES PTS runs on. HD-DVD fills the same fields from the XPL's title-relative times and a DVD's come from cell tables, so a plan built from them is an identity map with a drop filter: it suppresses the layer-break rebase inference performs, and drops whatever falls outside marks the PTS was never measured against. An earlier reading of this called HD-DVD safe because its marks are contiguous and every computed offset was zero — true, and irrelevant, because they were zero in the wrong clock. Gated on the content format, with a test using an HD-DVD-shaped table that the clock check alone accepts. The crossing test was also one-shot. A table whose clips restart their own bases could miss it, and a missed crossing STRANDS the track: every later frame falls outside the stranded clip's marks and is dropped for the rest of the title. Counting drops, which is all the previous round added, does not bound them. A table that is not one advancing clock is now refused outright and falls back to inference, which is the documented safe path for those titles. Also from the same round: read_sectors added an unchecked lba + i, where callers deliberately saturate their LBAs — a wrap folds the read back to a low sector and hands the muxer another file's bytes. classify added 1 to two numbers parsed verbatim out of a filename. read_head used a single read() where a short read on a network mount silently records no placement constraint at all. And the page-cache eviction added last round released only the read that crossed its threshold rather than everything accumulated, so seven eighths of what was read stayed pinned. --- src/dirimage/layout.rs | 22 +++++-- src/dirimage/mod.rs | 51 +++++++-------- src/mux/demux_sink.rs | 2 +- src/mux/mkv.rs | 8 ++- src/mux/mkvstream.rs | 2 +- src/mux/timeline.rs | 140 +++++++++++++++++++++++++++++++++++++++-- 6 files changed, 184 insertions(+), 41 deletions(-) diff --git a/src/dirimage/layout.rs b/src/dirimage/layout.rs index db91774..6e770b8 100644 --- a/src/dirimage/layout.rs +++ b/src/dirimage/layout.rs @@ -349,10 +349,14 @@ fn be_u32(buf: &[u8], off: usize) -> Option { fn read_head(path: &Path, n: usize) -> Result> { use std::io::Read; let mut buf = vec![0u8; n]; - let got = std::fs::File::open(path) - .and_then(|mut f| f.read(&mut buf)) - .map_err(Error::from)?; - buf.truncate(got); + // `read_exact`, not `read`: a single `read` may legally return fewer bytes + // than asked on a network or FUSE mount — and a NAS-hosted backup is the + // normal case for this feature. A short buffer sends every placement offset + // through `unwrap_or(0)`, recording NO constraint, so the planner and the + // reader disagree about where a VOB begins and the rip reads the wrong + // sectors for a whole title at exit 0. + let mut f = std::fs::File::open(path).map_err(Error::from)?; + f.read_exact(&mut buf).map_err(Error::from)?; Ok(buf) } @@ -496,12 +500,18 @@ fn classify(name: &str) -> Option { ("IFO", 0) => (0, Role::Ifo), ("VOB", 0) => (1, Role::MenuVob), ("VOB", 1) => (2, Role::TitleVob), - ("VOB", n) => (1 + n, Role::Sequential), + // Checked: `n` is parsed verbatim out of a filename, so a folder may + // legitimately contain `VTS_01_4294967295.VOB`. Wrapping would sort a + // stray VOB ahead of its own title set's IFO. + ("VOB", n) => (n.checked_add(1)?, Role::Sequential), ("BUP", 0) => (100, Role::Sequential), _ => return None, }; Some(Class { - group: set + 1, + // Checked for the same reason: a wrap here lands on group 0, which is + // the Video Manager's, so a stray file would contend for the VMG's + // placement slot. + group: set.checked_add(1)?, order, role, }) diff --git a/src/dirimage/mod.rs b/src/dirimage/mod.rs index b33add3..38705ac 100644 --- a/src/dirimage/mod.rs +++ b/src/dirimage/mod.rs @@ -86,12 +86,6 @@ struct FileRef { /// Owns everything it reads through (`PathBuf`s and its own file handles), so /// it is `Send + 'static` and can be moved into `build_iso_pipeline`, which /// hands it to `PrefetchedSectorSource`'s producer thread. -/// Bytes read between page-cache eviction calls (32 MiB). -/// -/// Large enough that the hint costs nothing measurable against a rip, small -/// enough that resident pages stay bounded well below any machine's RAM. -const DROP_CHUNK_BYTES: u64 = 32 * 1024 * 1024; - pub struct DirImage { meta: MetaSectors, /// Sorted by `start_lba`, non-overlapping. @@ -101,15 +95,6 @@ pub struct DirImage { total_sectors: u32, volume_id: String, data_bytes: u64, - /// Bytes read from host files since the last page-cache eviction. - /// - /// A rip streams every byte of the folder exactly once. Without eviction - /// the kernel keeps all of it resident, which starves the concurrent writer - /// — `io::file_sector_source` records the measured cost of exactly this - /// omission on the ISO path (2.7 MB/s mux against 70 MB/s isolated reads). - /// A folder source reads host files the same way and needs the same - /// treatment. - bytes_since_drop: u64, } impl std::fmt::Debug for DirImage { @@ -183,7 +168,6 @@ impl DirImage { total_sectors: plan.total_sectors, volume_id: plan.volume_id, data_bytes, - bytes_since_drop: 0, }) } @@ -251,16 +235,21 @@ impl DirImage { h.seek(SeekFrom::Start(at)).map_err(Error::from)?; let res = h.read_exact(&mut out[..want]); if res.is_ok() { - // Evict what we have consumed, per file handle. The window is the - // read just completed rather than a running offset, because reads - // here jump between files and a single monotonic cursor would name - // the wrong pages. - self.bytes_since_drop = self.bytes_since_drop.saturating_add(want as u64); - if self.bytes_since_drop >= DROP_CHUNK_BYTES { - if let Some((_, fh)) = self.open.iter().find(|(i, _)| *i == file) { - drop_window(fh, at, want as u64); - } - self.bytes_since_drop = 0; + // Release the window just read, every time. + // + // The ISO source accumulates and drops in chunks because it reads + // one file linearly, so a running start offset always names the + // bytes it has consumed. Reads here jump between files, so there is + // no single cursor to accumulate against — an accumulated byte + // count paired with one read's offset names 1/Nth of what was + // actually consumed and leaves the rest pinned, which is how the + // first version of this got it wrong. + // + // Dropping per read costs one advisory syscall per batch (4-16 MiB), + // which is nothing against the read itself, and it is correct + // regardless of how reads interleave across files. + if let Some((_, fh)) = self.open.iter().find(|(i, _)| *i == file) { + drop_window(fh, at, want as u64); } } match res { @@ -300,7 +289,15 @@ impl SectorSource for DirImage { // one 16 MiB sequential read. let mut i = 0u32; while i < count as u32 { - let at = lba + i; + // Checked: callers saturate their LBAs (`disc/dvd.rs` builds a cell + // start as `vob_start_sector.saturating_add(cell.first_sector)`, and + // the prefetcher adds an offset the same way), so a crafted IFO can + // present a request at the very top of the address space. Wrapping + // here would fold `at` back to a LOW sector and hand the muxer a + // different file's bytes with nothing reported. + let Some(at) = lba.checked_add(i) else { + break; + }; let off = i as usize * SECTOR; if let Some(s) = self.meta.get(&at) { buf[off..off + SECTOR].copy_from_slice(&s[..]); diff --git a/src/mux/demux_sink.rs b/src/mux/demux_sink.rs index 924bd87..739a949 100644 --- a/src/mux/demux_sink.rs +++ b/src/mux/demux_sink.rs @@ -742,7 +742,7 @@ impl DemuxSink { tracks, ref_video_track, ref_first_pts_ns: None, - timeline: TimelineContinuity::with_clips(&title.clips), + timeline: TimelineContinuity::with_clips(&title.clips, title.content_format), finished: false, }) } diff --git a/src/mux/mkv.rs b/src/mux/mkv.rs index b7eaaea..e47e5e7 100644 --- a/src/mux/mkv.rs +++ b/src/mux/mkv.rs @@ -1335,8 +1335,12 @@ impl MkvMuxer { /// /// No-op for a title with fewer than two clips or without usable marks — /// DVD, HD-DVD and file sources keep the inference path. - pub fn set_clips(&mut self, clips: &[crate::disc::Clip]) { - self.continuity = TimelineContinuity::with_clips(clips); + pub fn set_clips( + &mut self, + clips: &[crate::disc::Clip], + content_format: crate::disc::ContentFormat, + ) { + self.continuity = TimelineContinuity::with_clips(clips, content_format); } /// Write a single frame. diff --git a/src/mux/mkvstream.rs b/src/mux/mkvstream.rs index 3d8d7cd..06e4948 100644 --- a/src/mux/mkvstream.rs +++ b/src/mux/mkvstream.rs @@ -505,7 +505,7 @@ impl MkvStream { &self.disc_title.chapters, )?; // Seam correction from the playlist's marks where the title has them. - muxer.set_clips(&self.disc_title.clips); + muxer.set_clips(&self.disc_title.clips, self.disc_title.content_format); if let Some(path) = &pending.opening_capture_path { muxer.set_opening_capture(crate::diag::OpeningCapture::new(path, pending.tracks.len())); } diff --git a/src/mux/timeline.rs b/src/mux/timeline.rs index 3103e82..c3867d7 100644 --- a/src/mux/timeline.rs +++ b/src/mux/timeline.rs @@ -135,7 +135,7 @@ impl SeamPlan { if clips.len() < 2 { return None; } - let mut out = Vec::with_capacity(clips.len()); + let mut out: Vec = Vec::with_capacity(clips.len()); let mut cum: i64 = 0; for c in clips { let in_ns = mpls_ticks_to_ns(c.in_time); @@ -143,6 +143,25 @@ impl SeamPlan { if out_ns <= in_ns { return None; } + // Every placement rule below assumes the marks are points on ONE + // clock that advances across the whole title: a join is recognised + // by a frame landing at the next clip's IN, and a frame is assigned + // to a clip by falling inside its [in, out]. + // + // Not every title is like that — a playlist whose clips each restart + // their own STC has marks that all cover the same low values. Under + // such a table a crossing can be missed, and a missed crossing + // STRANDS the track on the clip it was on: every later frame then + // falls outside that clip's marks and is dropped, silently, for the + // rest of the title. That is precisely the truncation this type was + // written to fix, so a table we cannot place must not be placed at + // all — it falls back to inference, which is the documented safe + // path for exactly these titles. + if let Some(prev) = out.last() + && in_ns <= prev.in_ns + { + return None; + } out.push(SeamClip { in_ns, out_ns, @@ -357,12 +376,31 @@ impl TimelineContinuity { /// Falls back to [`Self::new`]'s inference when the title has fewer than two /// clips or its marks are unusable — so DVD, HD-DVD, `mkv://` and `m2ts://` /// sources behave exactly as before. - pub(crate) fn with_clips(clips: &[crate::disc::Clip]) -> Self { + pub(crate) fn with_clips( + clips: &[crate::disc::Clip], + content_format: crate::disc::ContentFormat, + ) -> Self { + // ONLY Blu-ray. A PlayItem's IN/OUT are positions in the same 45 kHz + // clock the PES PTS runs on, which is what makes placing frames by them + // meaningful. + // + // The other formats' clips carry marks from a different clock entirely: + // HD-DVD fills them from the XPL's title-relative `titleTimeBegin` / + // `titleTimeEnd`, and a DVD's come from cell tables. Those are elapsed + // times within a title, not PES positions — so a plan built from them + // is an identity map with a drop filter, it stops the layer-break + // rebase `adjust` exists to perform, and it drops whatever falls + // outside marks the PTS was never measured against. Those formats keep + // the inference path, which is what they have always used. + let seams = match content_format { + crate::disc::ContentFormat::BdTs => SeamPlan::from_clips(clips), + crate::disc::ContentFormat::MpegPs => None, + }; Self { offset_ns: 0, prev_offset_ns: 0, high_ns: None, - seams: SeamPlan::from_clips(clips), + seams, } } @@ -738,7 +776,7 @@ mod tests { #[test] fn map_under_a_seam_plan_tracks_offset_and_frontier() { let clips = seamless_branching_clips(); - let mut tc = TimelineContinuity::with_clips(&clips); + let mut tc = TimelineContinuity::with_clips(&clips, crate::disc::ContentFormat::BdTs); assert!(tc.seams.is_some(), "a multi-clip title must get a plan"); let c0_in = mpls_ticks_to_ns(clips[0].in_time); @@ -907,6 +945,100 @@ mod tests { ); } + /// Only Blu-ray gets a mark-driven plan. + /// + /// Audit finding, and a regression this nearly shipped: HD-DVD `Clip` marks + /// come from the XPL's title-relative times, and a DVD's from cell tables — + /// neither is a position in the PES clock. A plan built from them is an + /// identity map with a drop filter: it suppresses the layer-break rebase + /// `adjust` performs, and drops whatever falls outside marks the PTS was + /// never measured against. Both formats must stay on inference. + /// + /// An earlier reading of this concluded HD-DVD was safe because its marks + /// happen to be contiguous, so the computed offsets were all zero. That is + /// true and irrelevant: the offsets were zero in the WRONG CLOCK. + #[test] + fn only_blu_ray_gets_a_mark_driven_plan() { + let clips = seamless_branching_clips(); + let bd = TimelineContinuity::with_clips(&clips, crate::disc::ContentFormat::BdTs); + assert!(bd.seams.is_some(), "Blu-ray marks are PES-clock positions"); + + // The same table under the program-stream formats (DVD, HD-DVD). + let ps = TimelineContinuity::with_clips(&clips, crate::disc::ContentFormat::MpegPs); + assert!( + ps.seams.is_none(), + "DVD and HD-DVD must keep the inference path" + ); + + // And an HD-DVD-shaped table — contiguous, title-relative, strictly + // increasing, so the shared-clock check alone would have accepted it. + let hddvd: Vec = [(0u32, 132_690_000u32), (132_690_000, 288_489_000)] + .iter() + .enumerate() + .map(|(i, &(in_time, out_time))| crate::disc::Clip { + clip_id: format!("{i}"), + in_time, + out_time, + duration_secs: 0.0, + source_packets: 0, + }) + .collect(); + assert!( + SeamPlan::from_clips(&hddvd).is_some(), + "fixture: the shared-clock check does NOT reject this table" + ); + assert!( + TimelineContinuity::with_clips(&hddvd, crate::disc::ContentFormat::MpegPs) + .seams + .is_none(), + "so the format gate is what keeps HD-DVD off the plan" + ); + } + + /// A clip table that is not one advancing clock must fall back to + /// inference rather than be placed. + /// + /// Audit finding. Each clip is validated in isolation (span > 0) but the + /// placement rules assume the marks are points on a single clock. Under a + /// table whose clips each restart their own base, a crossing can be missed + /// — and a missed crossing STRANDS the track on its current clip, so every + /// later frame falls outside that clip's marks and is dropped for the rest + /// of the title. Silent truncation, which is what this type exists to + /// prevent. + #[test] + fn a_restarting_clock_falls_back_to_inference() { + let mk = |marks: &[(u32, u32)]| -> Vec { + marks + .iter() + .enumerate() + .map(|(i, &(in_time, out_time))| crate::disc::Clip { + clip_id: format!("{i}"), + in_time, + out_time, + duration_secs: 0.0, + source_packets: 0, + }) + .collect() + }; + // Clip 1 restarts near zero instead of continuing clip 0's clock. + let restarting = mk(&[(188_955_000, 271_486_824), (0, 12_462_450)]); + assert!( + SeamPlan::from_clips(&restarting).is_none(), + "a restarting clock must not be placed from marks" + ); + // Equal IN marks are just as unplaceable. + let repeated = mk(&[(1_000, 2_000), (1_000, 3_000)]); + assert!( + SeamPlan::from_clips(&repeated).is_none(), + "duplicate IN marks" + ); + // The real advancing table still gets a plan. + assert!( + SeamPlan::from_clips(&seamless_branching_clips()).is_some(), + "an advancing table must still be placed" + ); + } + /// Clips whose marks chain contiguously must come out byte-identical to the /// old behaviour: a constant offset, nothing moved, nothing dropped. ///