From 62450e19bdf01547ae70ae3486bb7d75b23cb7a4 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Wed, 29 Jul 2026 21:21:29 -0700 Subject: [PATCH] Make two public-API panics return errors, and drop two shipped citations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **The session panics are reachable, and my earlier triage of them was wrong.** `DiscSession::scan` and `resolve_keys` both did `self.drive.as_mut().expect(..)`. I previously downgraded these to LOW on the grounds that no shipped consumer calls them after the drive has been staged into the reader slot. That is the wrong test: `stage_drive_as_reader` is a PUBLIC method that empties the drive slot, so the public surface permits the sequence, and a library must not panic from public API regardless of what current callers happen to do. Both now return Error::DeviceNotReady. **A shipped doc comment cited a third-party source FILE** as the authority for the CLPI ProgramInfo layout ("Layout per the BD CLPI spec clpi_parse.c"). Now cites the Blu-ray Disc Read-Only Format Part 3 CLIPINF specification. **The CHANGELOG justified a muxer decision by naming a commercial competitor** ("MakeMKV's rip of the same disc omits it", "matching MakeMKV"). Reworded to stand on its own terms: the element is optional in RFC 9559, nothing requires it for interlaced SD, and the 40 ms DefaultDuration is the frame rate the source actually carries. The leak gate is extended for both new classes — third-party `*_parse.c` / `*_dec.c` / `*_demux.c` style filenames, and a competitor named as authority. Narrowing that rule took two attempts, which is worth recording. A bare `\.(c|cpp|cc)` pattern produced eight false positives: this repo has its own C shim (`macos_shim.c`) that build.rs and the docs legitimately reference, and the pattern also matched the Rust field access `p.cc`. It now matches only the suffixes typical of third-party media-library sources. This is the second false-positive round on this rule — the first flagged ffmpeg INVOCATIONS in the test harness — so the lesson is that a hygiene pattern needs testing in both directions before it lands, exactly like any other code. --- CHANGELOG.md | 13 +++++++------ src/clpi.rs | 4 ++-- src/session.rs | 21 +++++++++++++++------ 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d7041b..df67b66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -877,12 +877,13 @@ consumers are the in-tree toolchain crates. SD-DVD.** rc.5.1 added a 20 ms `DefaultDecodedFieldDuration` field element to the 576i/480i track header on the theory that Windows derives fps from it. Captured evidence showed that element made Windows Explorer report 12.5 fps - (half) and MediaInfo flip the track to "Frame rate mode: Variable", while - MakeMKV's rip of the same disc omits it. The element is therefore no longer - written (`MkvTrack::video` now passes `field_duration_ns == 0`); the track - keeps `FlagInterlaced=1` + `FieldOrder=TFF` and the full-frame 40 ms - `DefaultDuration` (`1/DefaultDuration` = 25 fps), matching MakeMKV. How a given - player or shell handler chooses to display interlaced fps is not guaranteed. + (half) and MediaInfo flip the track to "Frame rate mode: Variable". The + element is optional in RFC 9559 and nothing requires it for interlaced SD, so + it is no longer written (`MkvTrack::video` now passes `field_duration_ns == 0`); + the track keeps `FlagInterlaced=1` + `FieldOrder=TFF` and the full-frame 40 ms + `DefaultDuration` (`1/DefaultDuration` = 25 fps), which is the frame rate the + source actually carries. How a given player or shell handler chooses to display + interlaced fps is not guaranteed. - **Correct AC-3 audio track selected on DVDs with non-standard sub-stream ordering.** freemkv assigned each declared audio stream a physical sub-stream by ordinal (`0x80+n`), assuming the IFO's first stream lives at `0x80`. On diff --git a/src/clpi.rs b/src/clpi.rs index a5c3235..cc4630a 100644 --- a/src/clpi.rs +++ b/src/clpi.rs @@ -236,8 +236,8 @@ pub fn parse(data: &[u8]) -> Result { } /// Parse the ProgramInfo section: per-stream (pid, coding_type, -/// language, codec sub-fields). Layout per the BD CLPI spec -/// clpi_parse.c: +/// language, codec sub-fields). Layout per the Blu-ray Disc Read-Only Format +/// Part 3 CLIPINF (CLPI) specification: /// /// ```text /// ProgramInfo: diff --git a/src/session.rs b/src/session.rs index 0f86f44..94d0d06 100644 --- a/src/session.rs +++ b/src/session.rs @@ -244,7 +244,15 @@ impl DiscSession { /// set), runs [`Disc::scan`], stores the result, and returns a borrow. pub fn scan(&mut self, opts: ScanOptions) -> Result<&Disc> { let opts = forward_key_material(&mut self.spec, opts); - let disc = Disc::scan(self.drive.as_mut().expect("drive present for scan"), &opts)?; + // `stage_drive_as_reader` is PUBLIC and moves the drive into the reader + // slot, so this slot can legitimately be empty when a caller reaches + // here. A library must not panic from public API, and "no shipped + // consumer calls it in that order" is not the same as "cannot happen" — + // the public surface permits it, so it must be an error. + let drive = self.drive.as_mut().ok_or_else(|| Error::DeviceNotReady { + path: self.device.clone(), + })?; + let disc = Disc::scan(drive, &opts)?; self.disc = Some(disc); Ok(self.disc.as_ref().expect("disc just stored")) } @@ -272,12 +280,13 @@ impl DiscSession { let disc = self.disc.as_mut().expect("disc present (checked above)"); resolve_keys_for(reader.as_mut(), disc, sources) } else { + // Same reachability as `scan` above: the drive may have been staged + // into the reader slot by the public `stage_drive_as_reader`. + let drive = self.drive.as_mut().ok_or_else(|| Error::DeviceNotReady { + path: self.device.clone(), + })?; let disc = self.disc.as_mut().expect("disc present (checked above)"); - resolve_keys_for( - self.drive.as_mut().expect("drive present for key sampling"), - disc, - sources, - ) + resolve_keys_for(drive, disc, sources) }; self.key_fetch = resolved.key_fetch; Ok(resolved.trace)