diff --git a/src/dirimage/layout.rs b/src/dirimage/layout.rs index 8f10fcc..bb7aec8 100644 --- a/src/dirimage/layout.rs +++ b/src/dirimage/layout.rs @@ -403,6 +403,11 @@ fn place_video_ts(vts: &mut DirNode, start: u32) -> Result { // Required start blocks, resolved as each IFO is placed. let mut menu_req: std::collections::HashMap = std::collections::HashMap::new(); let mut title_req: std::collections::HashMap = std::collections::HashMap::new(); + // Groups whose IFO has already been placed. Kept separately from the + // constraint maps because an IFO that declares no offsets inserts into + // neither, so keying the duplicate check on those maps would miss a second + // IFO for the same set — which is the collision the check exists for. + let mut seen_ifo: std::collections::HashSet = std::collections::HashSet::new(); let mut cursor = start; for &i in &order { @@ -438,7 +443,7 @@ fn place_video_ts(vts: &mut DirNode, start: u32) -> Result { // parse to the same group, and the second insert would overwrite // the first's constraint, placing a VOB at an address the IFO the // reader uses does not point to. Refuse instead of picking one. - if menu_req.contains_key(&c.group) || title_req.contains_key(&c.group) { + if !seen_ifo.insert(c.group) { return Err(Error::DirNameCollision { host: vts.files[i].disc_path.clone(), }); @@ -639,6 +644,16 @@ pub(super) fn plan(root: &Path) -> Result { .position(|d| d.name.eq_ignore_ascii_case("VIDEO_TS")) { cursor = place_video_ts(&mut tree.dirs[idx], cursor)?; + // `place_video_ts` places only the FILES directly inside VIDEO_TS, + // because only those carry the IFO-relative constraints. Anything in a + // subdirectory of VIDEO_TS still needs data placed: without this its + // File Entry is written with the file's real size but no extents, so it + // appears in the tree at full length and reads back as nothing, at + // exit 0. The same folder under BDMV/ was always placed correctly, + // which is what made the gap easy to miss. + for sub in tree.dirs[idx].dirs.iter_mut() { + place_generic(sub, &mut cursor)?; + } for f in &mut tree.files { cursor = place_file(f, cursor)?; } diff --git a/src/dirimage/tests.rs b/src/dirimage/tests.rs index bfe5288..33ba17b 100644 --- a/src/dirimage/tests.rs +++ b/src/dirimage/tests.rs @@ -447,6 +447,35 @@ fn an_oversized_vob_offset_leaves_a_gap_rather_than_failing() { assert!(buf.iter().all(|&b| b == 0)); } +/// A file inside a SUBDIRECTORY of `VIDEO_TS` must still have its data placed. +/// +/// Audit finding. The DVD branch places only the files directly inside +/// `VIDEO_TS`, because only those carry the IFO-relative constraints — and the +/// follow-up loop skipped that directory entirely, so anything one level deeper +/// got a File Entry declaring the file's real size with no extents behind it. +/// It appeared in the tree at full length and read back as nothing, at exit 0. +/// The identical folder under `BDMV/` was always placed correctly, which is +/// what made the gap easy to miss. +#[test] +fn a_file_below_video_ts_is_placed_not_just_declared() { + let s = Scratch::new("dvdsubdir"); + s.file("VIDEO_TS/VIDEO_TS.IFO", &vec![0u8; SECTOR]); + s.file("VIDEO_TS/VTS_01_0.IFO", &vts_ifo(SECTOR, 0, 2)); + s.file("VIDEO_TS/VTS_01_1.VOB", &pattern(1, SECTOR)); + let payload = pattern(7, SECTOR); + s.file("VIDEO_TS/EXTRA/notes.bin", &payload); + + let mut img = DirImage::open(s.path()).unwrap(); + let fs = udf::read_filesystem(&mut img).unwrap(); + let got = fs + .read_file(&mut img, "/VIDEO_TS/EXTRA/notes.bin") + .expect("the file must be readable"); + assert_eq!( + got, payload, + "a file below VIDEO_TS must read back as its real contents, not zeros" + ); +} + /// A VOBS offset far past the content must be REFUSED, not honoured. /// /// Audit finding. The planner honours a title set's declared VOBS offset diff --git a/src/error.rs b/src/error.rs index 3bfe734..5c3096e 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1053,6 +1053,12 @@ impl std::fmt::Display for Error { }, Error::Halted => write!(f, "E{}", self.code()), Error::UdfNotFound { path } => write!(f, "E{}: {}", self.code(), path), + Error::SeamPlanDroppedMost { dropped, written } => { + write!(f, "E{} {dropped}/{written}", self.code()) + } + Error::ShortImageRead { lba, expected, got } => { + write!(f, "E{} {lba} {expected} {got}", self.code()) + } Error::DirImagePlacement { path } | Error::DirImageFileChanged { path } | Error::DirNameTooLong { path } @@ -1572,6 +1578,19 @@ mod tests { Error::DirImageTooLarge.code(), Error::DirNameTooLong { path: "x".into() }.code(), Error::DirImageFanout { path: "x".into() }.code(), + Error::SeamPlanDroppedMost { + dropped: 1, + written: 0, + } + .code(), + Error::SinkWroteNothing.code(), + Error::ShortImageRead { + lba: 0, + expected: 1, + got: 0, + } + .code(), + Error::EmptyImage.code(), ]; let mut sorted = codes.to_vec(); sorted.sort(); diff --git a/src/mux/demux_sink.rs b/src/mux/demux_sink.rs index 4051914..8293a56 100644 --- a/src/mux/demux_sink.rs +++ b/src/mux/demux_sink.rs @@ -660,6 +660,12 @@ pub struct DemuxSink { /// Index = track id; `None` for unselected tracks. tracks: Vec>, ref_video_track: Option, + /// Every VIDEO track index, recorded before the kind filter. + /// + /// The filtered `tracks` slots are `None` for a class this export drops, so + /// they cannot answer "is this video" for a frame that still flows through + /// `write()`. + video_tracks: std::collections::HashSet, /// First PTS observed on `ref_video_track`, recorded in `write()` REGARDLESS /// of whether that track has a `TrackOut`. The DELAY reference cannot live in /// `TrackOut::first_pts_ns`: `audio://` / `sub://` filter the video track's @@ -671,11 +677,15 @@ pub struct DemuxSink { ref_first_pts_ns: Option, timeline: TimelineContinuity, finished: bool, - /// Frames actually placed on the timeline and written. + /// Frames the timeline PLACED (not necessarily persisted). /// - /// A sink that wrote nothing must not report success, and a sink that - /// dropped more than it kept is not looking at a real join. - frames_written: u64, + /// A frame for a track this export filters out still flows through + /// `write()` and still counts here, so this is "the timeline placed + /// something" rather than "a file received bytes". That is the right + /// denominator for the drop gate below — it asks whether placement worked + /// at all — but it is deliberately NOT a per-track written count, so a + /// single lost track among many is not visible to it. + frames_mapped: u64, } impl DemuxSink { @@ -684,6 +694,7 @@ impl DemuxSink { std::fs::create_dir_all(dir)?; let mut tracks: Vec> = Vec::with_capacity(title.streams.len()); let mut ref_video_track = None; + let mut video_tracks: std::collections::HashSet = std::collections::HashSet::new(); for (idx, stream) in title.streams.iter().enumerate() { let selected = opts @@ -710,6 +721,15 @@ impl DemuxSink { if kind == TrackKind::Video && ref_video_track.is_none() { ref_video_track = Some(idx); } + // Also BEFORE the kind filter, and for the same reason: the seam + // crossing rule differs for a track that carries B-frame reorder, + // and a video track still flows through write() on an `audio://` + // or `sub://` export even though nothing persists it. Reading the + // kind back out of the filtered slot would call it non-video and + // hand it the permissive rule. + if kind == TrackKind::Video { + video_tracks.insert(idx); + } // Kind filter: `audio://` / `sub://` keep only their class. if opts.kind_filter.is_some_and(|k| k != kind) { tracks.push(None); @@ -746,10 +766,11 @@ impl DemuxSink { opts: opts.clone(), tracks, ref_video_track, + video_tracks, ref_first_pts_ns: None, timeline: TimelineContinuity::with_clips(&title.clips, title.content_format), finished: false, - frames_written: 0, + frames_mapped: 0, }) } @@ -876,17 +897,13 @@ impl Stream for DemuxSink { // (a Dolby Vision enhancement layer) does not drive epochs but does // carry B-frame reorder, and the seam-crossing rule differs on exactly // that property. - let is_video = self - .tracks - .get(frame.track) - .and_then(|s| s.as_ref()) - .is_some_and(|t| t.kind == TrackKind::Video); + let is_video = self.video_tracks.contains(&frame.track); // See `MkvMuxer::write_frame`: `None` is material outside the // playlist's clip marks and is dropped rather than emitted. let Some(pts) = self.timeline.map(frame.pts, drives, frame.track, is_video) else { return Ok(()); }; - self.frames_written = self.frames_written.saturating_add(1); + self.frames_mapped = self.frames_mapped.saturating_add(1); if drives { // Delay reference: recorded here, not in the track's `TrackOut`, so // it survives the `audio://` / `sub://` kind filter dropping the @@ -916,13 +933,13 @@ impl Stream for DemuxSink { // 0. Keyed on frames having been offered, because a sink that is never // given any — a chapters-only export, or a track class the title does // not carry — legitimately writes none. - if self.frames_written == 0 && seam_dropped > 0 { + if self.frames_mapped == 0 && seam_dropped > 0 { return Err(crate::error::Error::SinkWroteNothing.into()); } - if seam_dropped > self.frames_written { + if seam_dropped > self.frames_mapped { return Err(crate::error::Error::SeamPlanDroppedMost { dropped: seam_dropped, - written: self.frames_written, + written: self.frames_mapped, } .into()); } diff --git a/src/mux/mkv.rs b/src/mux/mkv.rs index c79539c..4ed3605 100644 --- a/src/mux/mkv.rs +++ b/src/mux/mkv.rs @@ -1697,6 +1697,15 @@ impl MkvMuxer { // would otherwise yield a structurally-empty MKV with no clusters or // cues. Surface that as an error rather than writing valid-but-empty // output. + // Order matters. A title the seam plan dropped ENTIRELY also has a zero + // frame count, and `MkvInvalid` is classified by + // `is_skippable_title_stub` as an empty nav/menu stub — so reporting it + // that way would make an all-titles rip drop a real feature and finish + // the rest at exit 0. Decide the seam case FIRST, with a code that is + // not skippable. + if self.frame_count == 0 && self.continuity.dropped_total() > 0 { + return Err(crate::error::Error::SinkWroteNothing.into()); + } if self.frame_count == 0 { return Err(crate::error::Error::MkvInvalid.into()); } diff --git a/src/mux/timeline.rs b/src/mux/timeline.rs index d94f95a..74a67b1 100644 --- a/src/mux/timeline.rs +++ b/src/mux/timeline.rs @@ -1022,7 +1022,6 @@ mod tests { fn a_second_video_track_keeps_the_reorder_safe_window() { let clips = seamless_branching_clips(); let mut plan = SeamPlan::from_clips(&clips).expect("plan"); - let c0_in = mpls_ticks_to_ns(clips[0].in_time); let c0_out = mpls_ticks_to_ns(clips[0].out_time); let c1_in = mpls_ticks_to_ns(clips[1].in_time); @@ -1040,7 +1039,6 @@ mod tests { base - 42_000_000, "an enhancement layer's reorder dip must not be read as a join" ); - let _ = c0_in; } /// A clip table that is not one advancing clock must fall back to