diff --git a/src/disc/extract.rs b/src/disc/extract.rs index ff75a26..c7e84e3 100644 --- a/src/disc/extract.rs +++ b/src/disc/extract.rs @@ -314,8 +314,24 @@ impl Disc { if extents.is_empty() { return base_keys.clone(); } - // Largest extent first (movie body), matching the scan heuristic. - extents.sort_by_key(|e| std::cmp::Reverse(e.sector_count)); + // PLAYBACK ORDER — do NOT sort. This is the 1.5.1 garbage bug, and it + // grew back here in a new code path: the comment this replaced said it + // "matched the scan heuristic", but that heuristic WAS the bug and was + // already fixed in `Disc::decrypt_keys_for_title`, which documents the + // rule ("PLAYBACK ORDER, never largest-cell-first") and pins it with + // `decrypt_keys_for_title_scans_playback_order_not_largest_first`. + // + // Why order decides correctness: a CSS DVD's biggest cell opens with a + // long CLEAR run, and `crack_key`'s sector budget is shared across the + // whole extent list. Starting at the largest cell can exhaust the budget + // without ever MEETING a scrambled sector — and CSS recovers the title + // key from scrambled data itself, so the scan has to reach some. The + // crack then returns None, this function falls back to `base_keys`, and + // every VOB in the VTS is descrambled with the wrong key: corrupt PES + // behind an intact header, written out as a complete extract at exit 0. + // + // `planned` is already in playback order (VTS_xx_1.VOB..\_9.VOB, extents + // in file order), so the correct action is to leave the vector alone. // Crack against the raw (still-scrambled) inner reader, NOT the // decrypting view — `crack_key` runs the descrambler itself. match crate::css::crack_key(dec.inner_mut(), &extents, 64) { diff --git a/src/disc/mod.rs b/src/disc/mod.rs index c85d121..5dcb526 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -1961,6 +1961,21 @@ impl Disc { Self::read_aacs_inputs_from_reader(&mut reader, &udf_fs) } + /// Same as [`Disc::read_aacs_inputs`] but over an extracted disc FOLDER. + /// + /// `dir://` is an image-level source: `dirimage` synthesizes a real UDF + /// volume over the folder, so the AACS inputs are read by exactly the same + /// reader path an ISO uses. Without this the online key fetch was silently + /// unavailable for folders — the CLI's fetch helper matched only `Iso` and + /// returned None for a folder, so the same disc that fetched its key fine + /// as an ISO failed as an extracted directory. A sink is a sink: any input + /// has to work with any output, and that includes the key path. + pub fn read_aacs_inputs_from_dir(dir: &std::path::Path) -> Result<(Vec, Vec, u8)> { + let mut reader = crate::dirimage::DirImage::open(dir)?; + let udf_fs = udf::read_filesystem(&mut reader)?; + Self::read_aacs_inputs_from_reader(&mut reader, &udf_fs) + } + /// Same as [`Disc::read_aacs_inputs`] but reads from a live drive. The /// out-of-band Unit Key path fetches the disc's key files from the drive, /// resolves a key from them however it likes, then applies it via diff --git a/src/ifo.rs b/src/ifo.rs index a31ffec..722fc96 100644 --- a/src/ifo.rs +++ b/src/ifo.rs @@ -825,16 +825,36 @@ fn parse_pgcit( let entries_start = pgcit_offset + 8; let mut titles = Vec::new(); + // Every `continue` below drops a title the user will never see. None of + // them may be silent: `parse_vmg` already counts and warns per skipped + // title SET, and this function was the remaining place where a disc could + // quietly report fewer titles than it has. + let mut skipped = 0usize; for &(chapter_count, vts_title_num) in titles_info { // VTS title numbers are 1-based; map to PGC index (typically 1:1) let pgc_index = vts_title_num.saturating_sub(1) as usize; if pgc_index >= num_pgcs as usize { + skipped += 1; + tracing::warn!( + target: "freemkv::scan", + pgc_index, + num_pgcs, + "title points past the end of the PGC table; its title is omitted" + ); continue; } let entry_offset = entries_start + pgc_index * 8; if entry_offset + 8 > data.len() { + skipped += 1; + tracing::warn!( + target: "freemkv::scan", + pgc_index, + entry_offset, + len = data.len(), + "PGC entry table is truncated; this title is omitted" + ); continue; } @@ -846,13 +866,38 @@ fn parse_pgcit( match parse_pgc(data, pgc_abs, chapter_count) { Ok(title) => titles.push(title), - // By design: a single unparseable PGC (truncated/corrupt entry, - // authoring-tool quirk) must not lose the whole title list. - // Skip it and keep collecting the titles that do parse. - Err(_) => continue, + Err(e) => { + // By design a single unparseable PGC (truncated/corrupt entry, + // authoring-tool quirk) must not lose the whole title list. But + // "not fatal" is not the same as "not worth saying": every PGC + // skipped here is a title the user will never see, and this was + // the only remaining silent one — `parse_vmg` above already + // counts and warns per skipped title SET for exactly this + // reason. A disc quietly reporting fewer titles than it has is + // the failure this release exists to stop. + skipped += 1; + tracing::warn!( + target: "freemkv::scan", + pgc = pgc_index + 1, + of = num_pgcs, + error = %e, + "PGC could not be parsed; its title is omitted" + ); + continue; + } } } + if skipped > 0 { + tracing::warn!( + target: "freemkv::scan", + skipped, + kept = titles.len(), + declared = titles_info.len(), + "some titles were omitted from this title set" + ); + } + Ok(titles) } diff --git a/src/io/image_writer.rs b/src/io/image_writer.rs index 0533e95..26afc0e 100644 --- a/src/io/image_writer.rs +++ b/src/io/image_writer.rs @@ -103,7 +103,22 @@ pub fn write_image( on_progress(written); } - out.flush().map_err(|source| Error::IoError { source })?; + // flush() only pushes the BufWriter's bytes into the kernel via write(2). + // It makes no durability promise at all, so returning Ok here would report + // a finished image while up to several gigabytes of it still sit in the + // page cache. A crash, a power loss, or yanking the removable/network + // volume the image was written to then leaves a truncated or empty file + // that the caller was told was complete. + // + // For a 6-90 GB image that is exactly the failure this crate treats as + // worst: success reported over wrong output. `into_inner` is used rather + // than `flush` so a buffered-write error is surfaced instead of being + // dropped on the floor by BufWriter's Drop. + let file = out.into_inner().map_err(|e| Error::IoError { + source: e.into_error(), + })?; + file.sync_all() + .map_err(|source| Error::IoError { source })?; Ok(written) } diff --git a/src/io/pipeline.rs b/src/io/pipeline.rs index c28060b..d158f0c 100644 --- a/src/io/pipeline.rs +++ b/src/io/pipeline.rs @@ -334,8 +334,11 @@ impl Pipeline { /// Sweep uses [`Pipeline::spawn_named`] directly so the consumer /// thread shows up as `freemkv-sweep-consumer`; mux uses /// `freemkv-mux-consumer`. `Pipeline::spawn` (this function, with - /// the default name) is used by `disc::patch` and by the unit - /// tests in this module. + /// the default name) is used only by the unit tests in this module. + /// It used to name `disc::patch` as a caller; that module does not exist + /// here any more — the sweep/patch recovery passes moved to freemkv-engine + /// in 1.6.0, so the comment sent readers hunting a caller in this crate + /// that had already left it. pub fn spawn>(depth: usize, sink: S) -> Result { Self::spawn_named("freemkv-pipeline-consumer", depth, sink) } diff --git a/src/mux/demux_sink.rs b/src/mux/demux_sink.rs index 8293a56..59fb35c 100644 --- a/src/mux/demux_sink.rs +++ b/src/mux/demux_sink.rs @@ -190,7 +190,13 @@ pub(crate) fn codec_label(codec: Codec) -> &'static str { /// to `.sup`, VobSub records `.idx` entries and emits the sidecar at finish. trait EsWriter: Send { /// Write one frame's payload to `w`. Returns the number of bytes written to - /// the main file (used by the VobSub writer for `.idx` filepos tracking). + /// the main file. + /// + /// Nothing in production reads this count: the sole caller discards it with + /// `?`, and `VobSubWriter` tracks `.idx` fileposes from its own `pos` field. + /// This previously claimed the VobSub writer depended on the return value, + /// which would have a maintainer believe a wrong count shows up as broken + /// `.idx` output. It does not. fn write_frame(&mut self, w: &mut dyn Write, f: &PesFrame, pts_ns: i64) -> io::Result; /// Finalize. Default: no-op. The VobSub writer serializes its `.idx` here. diff --git a/src/mux/mkv.rs b/src/mux/mkv.rs index 4ed3605..67526a2 100644 --- a/src/mux/mkv.rs +++ b/src/mux/mkv.rs @@ -680,8 +680,9 @@ pub struct MkvMuxer { cues: Vec, frame_count: u64, /// Frames handed to `write_frame` that were dropped because no cluster was - /// open yet (a cluster only opens on a track-0 video keyframe). See - /// `write_frame` for the track-0 invariant. + /// open yet (a cluster only opens on a keyframe from the primary video + /// track, whatever index that is — not necessarily track 0). See + /// `write_frame` for the cluster-driver invariant. /// /// The ALL-dropped case is surfaced as an error by `finish()`, but via /// `frame_count == 0`, not via this counter. A PARTIAL drop — leading audio / @@ -1683,11 +1684,17 @@ impl MkvMuxer { /// Finish the MKV file: write Cues element. /// - /// # Track-0 invariant + /// # Cluster-driver invariant /// - /// A cluster only opens on a track-0 video keyframe, so the caller must - /// supply track 0 as the video track and deliver a keyframe on it before - /// (or alongside) other-track data. If no track-0 keyframe ever arrives, + /// A cluster only opens on a keyframe from the PRIMARY VIDEO TRACK — the + /// first track whose type is video, at whatever index it occupies (see + /// `cluster_driver`, which is `primary_video_track.unwrap_or(0)`; index 0 + /// is only the fallback for a file with no video track at all). The caller + /// must deliver a keyframe on that track before (or alongside) other-track + /// data. This said "track-0" and required the caller to place video at + /// index 0, which the code has never actually required — so a reader + /// debugging dropped frames would suspect track ordering, which is not it. + /// If no such keyframe ever arrives, /// every `write_frame` is silently dropped; rather than emit a structurally /// valid but empty MKV (zero clusters, zero frames), `finish` returns /// `Error::MkvInvalid` when frames were submitted but none were written. diff --git a/src/mux/timeline.rs b/src/mux/timeline.rs index 74a67b1..d6dd67b 100644 --- a/src/mux/timeline.rs +++ b/src/mux/timeline.rs @@ -264,7 +264,13 @@ impl SeamPlan { // the end of the list. let stepped_back = pos.last_raw_ns.is_some_and(|last| raw_ns < last) && if has_reorder { - (raw_ns.saturating_sub(next_in)).abs() <= CLIP_START_TOLERANCE_NS + // saturating_abs, not abs: every other comparison in this + // module is saturating because these timestamps come off a + // disc and are not trusted. `abs()` is the one exception + // and it panics on i64::MIN — which saturating_sub can + // produce exactly — taking down the mux thread on one bad + // frame instead of comparing false like its neighbours. + (raw_ns.saturating_sub(next_in)).saturating_abs() <= CLIP_START_TOLERANCE_NS } else { raw_ns >= next_in.saturating_sub(CLIP_START_TOLERANCE_NS) && raw_ns <= self.clips[clip + 1].out_ns