Make two public-API panics return errors, and drop two shipped citations

**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.
This commit is contained in:
Matthew Jackson
2026-07-29 21:21:29 -07:00
parent 013881ac06
commit 62450e19bd
3 changed files with 24 additions and 14 deletions
+7 -6
View File
@@ -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
+2 -2
View File
@@ -236,8 +236,8 @@ pub fn parse(data: &[u8]) -> Result<ClipInfo> {
}
/// 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:
+15 -6
View File
@@ -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)