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. ///