Fix audit findings: DTS AMODE bound, key-fetch negative memoization, PGS probe coverage

- dts: accept all 16 legal AMODE channel-arrangement codes (0-15), not just
  0-9. Per ETSI TS 102 114 the 6-bit AMODE field has 16 defined arrangements;
  only 16-63 are reserved. ffmpeg's ff_dca_channels[16] confirms 10-15 are
  decodable 6/7/8-channel layouts. The old bound of 10 dropped spec-legal
  multichannel core frames as undecodable, silencing recoverable audio. Add a
  regression test (literal 0..16 range) that fails if the bound reverts to 10.

- keysource: only memoize a NEGATIVE (empty) key-fetch result when every source
  genuinely ran and none held the key — never when a source Err'd (network down,
  unreachable). A transient outage was being cached as a permanent "no key" for
  the fingerprint, permanently dropping a unit that could be recovered once the
  source came back. Thread an `errored` flag out of the drivers and gate the
  cache insert on it. Tests cover both the recover-after-outage case and that a
  genuine absence is still memoized.

- pgs_forced_probe: add happy-path coverage feeding real synthetic BD-TS PGS
  display sets through the full demux -> parse -> observe -> apply path, both a
  forced verdict landing and a non-forced verdict clearing a vendor flag.

- mp4: correct fit_report doc (audio carried is AC-3/E-AC-3 AND DTS/DTS-HD).

- scan_iso test: add independent fixture expectations (volume id) so the parity
  test is no longer purely tautological against a re-run of the same composition.
This commit is contained in:
Matthew Jackson
2026-07-24 08:32:37 -07:00
parent c00384d4df
commit 661ab138c6
5 changed files with 360 additions and 27 deletions
+130
View File
@@ -182,6 +182,136 @@ mod tests {
assert!(s.forced, "no content observed → vendor forced preserved");
}
/// A reader that serves a fixed BD-TS byte stream once (across sequential
/// `read_sectors` calls), then EOF — so the probe's demux→parse→observe→apply
/// path runs on real synthetic PGS content.
struct TsReader {
data: Vec<u8>,
pos: usize,
}
impl SectorSource for TsReader {
fn read_sectors(
&mut self,
_lba: u32,
_count: u16,
buf: &mut [u8],
_recovery: bool,
) -> crate::error::Result<usize> {
if self.pos >= self.data.len() {
return Ok(0);
}
let n = buf.len().min(self.data.len() - self.pos);
buf[..n].copy_from_slice(&self.data[self.pos..self.pos + n]);
self.pos += n;
Ok(n)
}
fn capacity_sectors(&self) -> u32 {
self.data.len().div_ceil(SECTOR_BYTES) as u32
}
}
// PGS PCS layout (matches the private constants in mux::codec::pgs): a
// display-set frame begins with a PCS (segment type 0x16); byte 13 is
// number_of_composition_objects; byte 17 is the first object's flags, whose
// 0x40 bit is forced_on_flag.
const PCS_SEG: u8 = 0x16;
const PCS_NUM_OBJECTS_OFF: usize = 13;
const PCS_FLAGS_OFF: usize = 17;
const PCS_FORCED_FLAG: u8 = 0x40;
/// One PGS display-set elementary payload with a single composition object;
/// `forced` sets forced_on_flag.
fn pcs_display(forced: bool) -> Vec<u8> {
let mut d = vec![0u8; 18];
d[0] = PCS_SEG;
d[PCS_NUM_OBJECTS_OFF] = 1;
d[PCS_FLAGS_OFF] = if forced { PCS_FORCED_FLAG } else { 0 };
d
}
/// Wrap an elementary payload in one 192-byte BD-TS PES packet (PUSI, PTS
/// present) on `pid`. `cc` is the 4-bit continuity counter.
fn bd_pes_packet(pid: u16, cc: u8, es: &[u8]) -> Vec<u8> {
let mut pkt = vec![0u8; 192];
// pkt[0..4] = TP_extra_header (zeros). TS packet starts at pkt[4].
pkt[4] = 0x47; // sync
pkt[5] = 0x40 | ((pid >> 8) & 0x1F) as u8; // PUSI + PID high 5 bits
pkt[6] = (pid & 0xFF) as u8; // PID low 8 bits
pkt[7] = 0x10 | (cc & 0x0F); // adaptation=payload-only + continuity counter
// PES header (at ts payload = pkt[8..]): 00 00 01 stream_id len flags.
let p = 8;
pkt[p] = 0x00;
pkt[p + 1] = 0x00;
pkt[p + 2] = 0x01;
pkt[p + 3] = 0xBD; // private_stream_1 (carries the standard PES extension)
pkt[p + 4] = 0x00; // PES packet length hi (0 = unbounded; ignored by demux)
pkt[p + 5] = 0x00; // PES packet length lo
pkt[p + 6] = 0x80; // flags1 ('10' marker)
pkt[p + 7] = 0x80; // flags2 → PTS present
pkt[p + 8] = 0x05; // PES_header_data_length = 5 (one PTS)
// 5-byte PTS with the mandatory marker bits (bytes 0,2,4 low bit = 1).
pkt[p + 9] = 0x21;
pkt[p + 10] = 0x00;
pkt[p + 11] = 0x01;
pkt[p + 12] = 0x00;
pkt[p + 13] = 0x01;
let es_off = p + 14; // ES data follows the 14-byte PES header
let n = es.len().min(192 - es_off);
pkt[es_off..es_off + n].copy_from_slice(&es[..n]);
pkt
}
/// Two BD-TS PES on `pid`: the FIRST carries `es` (the observed display set);
/// the second (a fresh PUSI) exists only to flush the first PES out of the
/// demuxer — the probe never calls `TsDemuxer::flush`, so an open PES stays
/// buffered until the next PES start arrives.
fn ts_stream(pid: u16, es: &[u8]) -> Vec<u8> {
let mut s = bd_pes_packet(pid, 0, es);
s.extend_from_slice(&bd_pes_packet(pid, 1, &pcs_display(false)));
s
}
#[test]
fn forced_display_sets_apply_forced_verdict() {
// Feed REAL synthetic PGS bytes through the full demux→parse→observe→apply
// path: a forced display set must flip a vendor-not-forced PGS track to
// forced. Mutation guard: inverting ForcedTracker::is_forced flips this.
let pid = 0x1200u16;
let mut reader = TsReader {
data: ts_stream(pid, &pcs_display(true)),
pos: 0,
};
let mut title = pgs_title(pid, false); // vendor label says NOT forced
probe_and_set_forced(&mut reader, &mut title);
let Stream::Subtitle(s) = &title.streams[0] else {
panic!()
};
assert!(
s.forced,
"an all-forced PGS track → forced verdict applied onto the stream"
);
}
#[test]
fn nonforced_display_sets_clear_forced_verdict() {
// A non-forced display set observed on the wire overrides a vendor-forced
// label → the track settles as not-forced.
let pid = 0x1200u16;
let mut reader = TsReader {
data: ts_stream(pid, &pcs_display(false)),
pos: 0,
};
let mut title = pgs_title(pid, true); // vendor label says forced
probe_and_set_forced(&mut reader, &mut title);
let Stream::Subtitle(s) = &title.streams[0] else {
panic!()
};
assert!(
!s.forced,
"a non-forced display set observed → forced verdict cleared"
);
}
#[test]
fn no_pgs_streams_is_noop() {
// A title with no PGS subtitle streams is a no-op (the reader is never
+166 -21
View File
@@ -375,14 +375,42 @@ pub fn resolve_and_apply_traced(
/// [`resolve_and_apply`] this does not validate/commit to a disc — the read's
/// decorator re-decrypts with the returned keys, which is the validation.
pub fn fetch_unit_keys(sources: &[Box<dyn KeySource>], ctx: &dyn ResolveCtx) -> Vec<UnitKey> {
drive_unit_keys(sources, ctx).keys
}
/// Whether a driver run resolved keys, and — when it did NOT — whether the miss
/// was a genuine "no source holds this key" (`errored == false`) or at least one
/// source FAILED (`errored == true`, e.g. a network source was unreachable). The
/// distinction gates negative-result memoization: an empty-because-absent result
/// is safe to cache, an empty-because-a-source-was-down result is transient and
/// must NOT be cached (the key may resolve once the source recovers).
struct FetchOutcome {
keys: Vec<UnitKey>,
errored: bool,
}
/// [`fetch_unit_keys`] plus the error signal: drive `sources` in order, return the
/// first source's non-empty Unit Keys, and flag whether any source that failed to
/// answer did so with an `Err` (a source failure) rather than an empty `Ok`
/// (genuine absence — see [`KeySource::get_unit_keys`]).
fn drive_unit_keys(sources: &[Box<dyn KeySource>], ctx: &dyn ResolveCtx) -> FetchOutcome {
let mut errored = false;
for source in sources {
if let Ok(uks) = source.get_unit_keys(ctx) {
if !uks.is_empty() {
return uks;
match source.get_unit_keys(ctx) {
Ok(uks) if !uks.is_empty() => {
return FetchOutcome {
keys: uks,
errored: false,
};
}
Ok(_) => {}
Err(_) => errored = true,
}
}
Vec::new()
FetchOutcome {
keys: Vec::new(),
errored,
}
}
/// The forensic counterpart to [`fetch_unit_keys`]: drive `sources` in order and
@@ -392,14 +420,29 @@ pub fn fetch_unit_keys(sources: &[Box<dyn KeySource>], ctx: &dyn ResolveCtx) ->
/// keying on `disc_hash`) ignores them. Whatever the winning source returns —
/// ≥ 1 key — is trusted as the COMPLETE ordered set; no fixed count is assumed.
pub fn fetch_fmts_indexes(sources: &[Box<dyn KeySource>], ctx: &dyn ResolveCtx) -> Vec<UnitKey> {
drive_fmts_indexes(sources, ctx).keys
}
/// [`fetch_fmts_indexes`] plus the error signal (see [`drive_unit_keys`]): the
/// forensic counterpart that flags whether any source `Err`ed during the miss.
fn drive_fmts_indexes(sources: &[Box<dyn KeySource>], ctx: &dyn ResolveCtx) -> FetchOutcome {
let mut errored = false;
for source in sources {
if let Ok(uks) = source.get_fmts_indexes(ctx) {
if !uks.is_empty() {
return uks;
match source.get_fmts_indexes(ctx) {
Ok(uks) if !uks.is_empty() => {
return FetchOutcome {
keys: uks,
errored: false,
};
}
Ok(_) => {}
Err(_) => errored = true,
}
}
Vec::new()
FetchOutcome {
keys: Vec::new(),
errored,
}
}
/// Build the read-time [`crate::sector::KeyFetch`] from the disc's public AACS
@@ -426,12 +469,17 @@ pub fn key_fetch(
// batch: the resolved keys are disc-level (a clip's index / CPS keys are
// identical for every title that references it), so the first batch resolves
// over the network and every repeat is answered from the cache with no
// request. Empty replies are cached too — a key the service lacks for a batch
// won't appear on a re-ask, so re-hitting the network buys nothing. Each
// operation gets its OWN cache: a base batch and a forensic anchor never
// collide, and the same bytes could legitimately resolve differently per op.
// The per-kind driver: `fetch_unit_keys` or `fetch_fmts_indexes`.
type FetchDriver = fn(&[Box<dyn KeySource>], &dyn ResolveCtx) -> Vec<UnitKey>;
// request. A GENUINELY-empty reply (every source ran and none held the key)
// is cached too — the key the service lacks for a batch won't appear on a
// re-ask, so re-hitting the network buys nothing. But an empty reply caused
// by a source FAILURE (network down, source unreachable) is NOT cached: that
// is a transient miss, and caching it would permanently drop a unit that
// could be recovered once the source recovers — the `errored` flag on
// `FetchOutcome` draws exactly that line. Each operation gets its OWN cache:
// a base batch and a forensic anchor never collide, and the same bytes could
// legitimately resolve differently per op.
// The per-kind driver: `drive_unit_keys` or `drive_fmts_indexes`.
type FetchDriver = fn(&[Box<dyn KeySource>], &dyn ResolveCtx) -> FetchOutcome;
fn make_op(
inputs: DiscInputs,
make_sources: std::sync::Arc<dyn Fn() -> Vec<Box<dyn KeySource>> + Send + Sync>,
@@ -460,16 +508,22 @@ pub fn key_fetch(
// derives unit keys from `enc_title_keys`, which a V10 disc parses at
// the 48-byte stride — hardcoding the V20 stride here corrupted them.
let ctx = DiscInputsCtx::new(&di);
let keys: Vec<[u8; 16]> = drive(&sources, &ctx).into_iter().map(|u| u.key).collect();
cache
.lock()
.unwrap_or_else(|e| e.into_inner())
.insert(fp, keys.clone());
let outcome = drive(&sources, &ctx);
let keys: Vec<[u8; 16]> = outcome.keys.into_iter().map(|u| u.key).collect();
// Memoize a positive result always; memoize a NEGATIVE (empty) result
// only when it is a genuine absence, never when a source errored — a
// transient outage must not permanently poison this fingerprint.
if !keys.is_empty() || !outcome.errored {
cache
.lock()
.unwrap_or_else(|e| e.into_inner())
.insert(fp, keys.clone());
}
keys
})
}
let unit = make_op(inputs.clone(), make_sources.clone(), fetch_unit_keys);
let fmts = make_op(inputs, make_sources, fetch_fmts_indexes);
let unit = make_op(inputs.clone(), make_sources.clone(), drive_unit_keys);
let fmts = make_op(inputs, make_sources, drive_fmts_indexes);
crate::sector::KeyFetch::new(unit, fmts)
}
@@ -867,6 +921,97 @@ mod tests {
);
}
/// A transient source outage must NOT be memoized as a permanent "no key":
/// a fingerprint whose first fetch failed because the source errored must be
/// re-asked, and once the source recovers the key resolves. Regression guard
/// for the negative-result memoization fix — caching the errored empty would
/// permanently drop a recoverable unit for the rest of the op.
#[test]
fn errored_empty_is_not_cached_and_retries_when_source_recovers() {
use std::sync::atomic::{AtomicUsize, Ordering};
let key = [0x77u8; 16];
// Shared across every `make_sources()` rebuild: call 0 errors (source
// down), every later call succeeds (source recovered).
let calls = Arc::new(AtomicUsize::new(0));
struct Flaky {
calls: Arc<AtomicUsize>,
key: [u8; 16],
}
impl KeySource for Flaky {
fn get_unit_keys(&self, _ctx: &dyn ResolveCtx) -> Result<Vec<UnitKey>, Error> {
if self.calls.fetch_add(1, Ordering::SeqCst) == 0 {
Err(Error::AacsNoKeys) // first attempt: source unreachable
} else {
Ok(vec![UnitKey::new(0, self.key)])
}
}
}
let calls_c = Arc::clone(&calls);
let make: Arc<dyn Fn() -> Vec<Box<dyn KeySource>> + Send + Sync> = Arc::new(move || {
vec![Box::new(Flaky {
calls: Arc::clone(&calls_c),
key,
}) as Box<dyn KeySource>]
});
let cb = key_fetch(empty_inputs(), make);
let samples = vec![vec![0xCDu8; 8]];
// First fetch: the source errors → empty, but the miss must NOT be cached.
assert!(
cb.unit_keys(&samples).is_empty(),
"source down → empty this time"
);
// Second fetch, SAME samples: not blocked by a cached empty → the now-
// recovered source resolves the key.
assert_eq!(
cb.unit_keys(&samples),
vec![key],
"recovered source resolves — errored empty was not memoized"
);
}
/// A GENUINE absence (a source that runs and returns an empty `Ok`) is still
/// memoized — the benefit the fix preserves. A source counting its calls must
/// be asked exactly once for a fingerprint whose first (clean) reply was empty.
#[test]
fn genuine_empty_is_still_memoized() {
use std::sync::atomic::{AtomicUsize, Ordering};
let calls = Arc::new(AtomicUsize::new(0));
struct AlwaysEmpty {
calls: Arc<AtomicUsize>,
}
impl KeySource for AlwaysEmpty {
fn get_unit_keys(&self, _ctx: &dyn ResolveCtx) -> Result<Vec<UnitKey>, Error> {
self.calls.fetch_add(1, Ordering::SeqCst);
Ok(Vec::new()) // ran fine, genuinely holds no key
}
}
let calls_c = Arc::clone(&calls);
let make: Arc<dyn Fn() -> Vec<Box<dyn KeySource>> + Send + Sync> = Arc::new(move || {
vec![Box::new(AlwaysEmpty {
calls: Arc::clone(&calls_c),
}) as Box<dyn KeySource>]
});
let cb = key_fetch(empty_inputs(), make);
let samples = vec![vec![0xEFu8; 8]];
assert!(cb.unit_keys(&samples).is_empty());
assert!(cb.unit_keys(&samples).is_empty());
assert_eq!(
calls.load(Ordering::SeqCst),
1,
"a clean empty reply is cached — the source is asked only once"
);
}
/// The two `KeyFetch` operations route to the two DISTINCT trait methods:
/// `unit_keys` drives `get_unit_keys`, `fmts_indexes` drives
/// `get_fmts_indexes`. A source that returns different keys per method proves
+50 -3
View File
@@ -688,7 +688,14 @@ fn dts_core_duration_ns(data: &[u8]) -> u64 {
/// `DTS_AMODE_COUNT`; `lfe_present == DTS_LFE_FLAG_INVALID` is rejected.
const DTS_PCMBLOCK_SAMPLES: u32 = 32;
const DTS_SUBBAND_SAMPLES: u32 = 8;
const DTS_AMODE_COUNT: u32 = 10;
/// Number of LEGAL `AMODE` (channel-arrangement) codes. The 6-bit AMODE field
/// (ETSI TS 102 114 §5.3.1) has 16 defined channel arrangements, codes 0-15;
/// only 16-63 are reserved/user-defined and undecodable. ffmpeg's
/// `ff_dca_channels[16] = {1,2,2,2,2,3,3,4,4,5,6,6,6,7,8,8}` confirms all 16 are
/// decodable — codes 10-15 are the 6/7/8-channel layouts. A frame is dropped
/// only when `audio_mode >= DTS_AMODE_COUNT` (i.e. a truly reserved 16-63 code);
/// dropping a legal 10-15 multichannel core would silence recoverable audio.
const DTS_AMODE_COUNT: u32 = 16;
const DTS_LFE_FLAG_INVALID: u32 = 3;
/// Sample rate (Hz) per core `SFREQ` code (ETSI TS 102 114 Table 6-4); a `0`
@@ -1946,8 +1953,11 @@ mod tests {
d[5] = (d[5] & 0x03) | (14u8 << 2);
assert_eq!(core_header_drop_reason(&d), Some(DropReason::PcmBlocks));
// audio_mode >= 10: AMODE = byte7 bits3-0 (high 4) + byte8 bits7-6. Set
// AMODE high nibble to 0xF → audio_mode >= 60.
// audio_mode reserved (>= 16): AMODE = byte7 bits3-0 (high 4) + byte8
// bits7-6. Set AMODE high nibble to 0xF → audio_mode = 60, a genuinely
// RESERVED code (16-63) a decoder rejects. (Codes 10-15 are LEGAL
// multichannel layouts and must NOT be dropped — see
// legal_multichannel_amode_is_not_dropped.)
let mut d = good.clone();
d[7] |= 0x0F;
assert_eq!(core_header_drop_reason(&d), Some(DropReason::Amode));
@@ -1975,6 +1985,43 @@ mod tests {
assert_eq!(core_header_drop_reason(&d), Some(DropReason::PcmRes));
}
#[test]
fn legal_multichannel_amode_is_not_dropped() {
// ETSI TS 102 114 §5.3.1: AMODE is a 6-bit field with 16 LEGAL
// channel-arrangement codes (0-15); only 16-63 are reserved. ffmpeg's
// ff_dca_channels[16] = {1,2,2,2,2,3,3,4,4,5,6,6,6,7,8,8} confirms codes
// 10-15 are decodable 6/7/8-channel layouts. The decodability gate must
// KEEP them — dropping a spec-legal multichannel core silences audio the
// recover-100% goal must preserve.
fn set_amode(core: &mut [u8], amode: u32) {
// audio_mode = (byte7 & 0x0F) << 2 | (byte8 >> 6).
core[7] = (core[7] & 0xF0) | ((amode >> 2) & 0x0F) as u8;
core[8] = (core[8] & 0x3F) | (((amode & 0x03) << 6) as u8);
}
// Every legal code 0-15 is kept — the range is a literal (NOT
// DTS_AMODE_COUNT) so reverting the bound to 10 makes 10-15 fail here.
for amode in 0u32..16 {
let mut core = make_dts_core(512);
set_amode(&mut core, amode);
assert_eq!(
core_header_drop_reason(&core),
None,
"legal AMODE {amode} must not be dropped"
);
}
// The first reserved code (16) and above are still rejected.
for amode in [16u32, 40, 63] {
let mut core = make_dts_core(512);
set_amode(&mut core, amode);
assert_eq!(
core_header_drop_reason(&core),
Some(DropReason::Amode),
"reserved AMODE {amode} must be dropped"
);
}
}
/// Real-data fixture (ignored). Re-parses a raw `.dts` elementary stream
/// through `DtsParser` and writes the emitted access units back out, so the
/// garbage-extension → core-only drop can be validated against an actual
+3 -2
View File
@@ -158,8 +158,9 @@ pub struct Mp4FitReport {
}
/// Compute the fit plan without opening a file. Video: the first primary
/// HEVC/H.264 track. Audio: every AC-3 / E-AC-3 track. Everything else is
/// skipped with a reason.
/// HEVC/H.264 track. Audio: every track `audio::audio_fits` carries — the Dolby
/// family (AC-3 / E-AC-3) and DTS (core / DTS-HD HRA / DTS-HD MA). Everything
/// else is skipped with a reason.
pub fn fit_report(title: &DiscTitle) -> Mp4FitReport {
let mut included = Vec::new();
let mut skipped = Vec::new();
+11 -1
View File
@@ -141,10 +141,20 @@ fn scan_iso_matches_manual_scan_image_path() {
.expect("manual scan_image succeeds");
assert_eq!(disc.capacity_sectors, manual.capacity_sectors, "capacity");
assert_eq!(disc.capacity_sectors, expected_capacity, "capacity value");
assert_eq!(disc.titles.len(), manual.titles.len(), "title count");
assert_eq!(disc.encrypted, manual.encrypted, "encrypted flag");
assert_eq!(disc.format, manual.format, "disc format");
// Independent expectations (not parity against a re-run of the same
// composition): the scanned Disc must match KNOWN properties of the fixture
// itself — its capacity (max LBA + 1), its PVD volume id ("TEST_DISC"), and
// that a UDF with no /AACS directory is unencrypted. These would fail even if
// scan_iso and the manual path drifted together.
assert_eq!(disc.capacity_sectors, expected_capacity, "capacity value");
assert_eq!(
disc.volume_id, "TEST_DISC",
"PVD volume id from the fixture"
);
assert!(!disc.encrypted, "minimal UDF (no /AACS) is not encrypted");
// The returned reader is usable: correct capacity and a real read of sector