From b2bd5f8b3edeae401f3b34d938289dc41738aa8e Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Sun, 19 Jul 2026 14:23:04 -0700 Subject: [PATCH] mux/codec/truehd: fix drop-forward poison + rate/resync validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three TrueHD state-machine fixes from an adversarial audit: - A corrupt access unit drops forward to the next major sync, but only the individually-verified corruption now feeds the whole-track poison verdict; the resync run is collateral. A couple of transient errors can no longer poison and discard an otherwise-good multi-hour track. The shared drop tally gains a verified/collateral split for this. - The per-AU PTS rate is refined only from a CRC-validated major sync, so a corrupt major sync whose rate nibble decodes to another rate family can no longer shift the resumed audio — a drop stays a silence gap. - The resync clears only on a CRC-validated major sync, never on a runt too short to hold and validate its header. --- src/mux/codec/dropgate.rs | 45 ++++++- src/mux/codec/truehd.rs | 253 ++++++++++++++++++++++++++++++-------- 2 files changed, 243 insertions(+), 55 deletions(-) diff --git a/src/mux/codec/dropgate.rs b/src/mux/codec/dropgate.rs index ba9e18d..121329c 100644 --- a/src/mux/codec/dropgate.rs +++ b/src/mux/codec/dropgate.rs @@ -34,6 +34,13 @@ pub(crate) struct DropTally { codec: &'static str, kept: u64, dropped: u64, + /// AUs dropped because they were INDIVIDUALLY verified undecodable (a failed + /// CRC/header/parity check). Only these feed the whole-track poison verdict. + /// Distinct from `dropped`, which also counts *collateral* drops — AUs + /// discarded as a consequence of one corruption (TrueHD's resync-forward run, + /// or a poisoned track), which must NOT amplify a few real errors into a + /// false whole-track loss. + verified_dropped: u64, dropped_dur_ns: u64, poisoned: bool, } @@ -44,6 +51,7 @@ impl DropTally { codec, kept: 0, dropped: 0, + verified_dropped: 0, dropped_dur_ns: 0, poisoned: false, } @@ -71,9 +79,32 @@ impl DropTally { self.kept += 1; } - /// Record a dropped (undecodable) access unit and log it. `reason` is a - /// short static label for the specific corruption check that failed. + /// Record a dropped access unit that was INDIVIDUALLY verified undecodable + /// (a failed CRC/header/parity check). Counts toward the whole-track poison + /// verdict. `reason` is a short static label for the check that failed. pub(crate) fn record_drop(&mut self, pts_ns: i64, dur_ns: i64, bytes: usize, reason: &str) { + self.verified_dropped += 1; + self.record_drop_common(pts_ns, dur_ns, bytes, reason); + self.maybe_poison(); + } + + /// Record a COLLATERAL drop — an AU discarded as a consequence of another + /// corruption rather than being individually undecodable (TrueHD's + /// resync-forward run to the next major sync, or an already-poisoned track). + /// Counted and logged for the drop report, but deliberately does NOT feed the + /// poison verdict, so one corruption event can't amplify into a false + /// whole-track loss. + pub(crate) fn record_collateral_drop( + &mut self, + pts_ns: i64, + dur_ns: i64, + bytes: usize, + reason: &str, + ) { + self.record_drop_common(pts_ns, dur_ns, bytes, reason); + } + + fn record_drop_common(&mut self, pts_ns: i64, dur_ns: i64, bytes: usize, reason: &str) { self.dropped += 1; self.dropped_dur_ns += dur_ns.max(0) as u64; tracing::debug!( @@ -86,7 +117,6 @@ impl DropTally { bytes, reason ); - self.maybe_poison(); } /// Whole-track fallback: after enough AUs to judge, if more than half were @@ -97,14 +127,17 @@ impl DropTally { if self.poisoned { return; } + // Judge on VERIFIED drops vs all AUs seen: a track is only poisoned when + // a majority of its access units are individually undecodable — not when + // a couple of corruption events forced long collateral resync runs. let total = self.kept + self.dropped; - if total >= TRACK_VERDICT_MIN_AUS && self.dropped * 2 > total { + if total >= TRACK_VERDICT_MIN_AUS && self.verified_dropped * 2 > total { self.poisoned = true; tracing::warn!( target: "mux", - "{}: track too damaged to mux — {}/{} AUs undecodable (>50%); dropping the whole track", + "{}: track too damaged to mux — {}/{} AUs individually undecodable (>50%); dropping the whole track", self.codec, - self.dropped, + self.verified_dropped, total ); } diff --git a/src/mux/codec/truehd.rs b/src/mux/codec/truehd.rs index 0a5ce09..a79f4b6 100644 --- a/src/mux/codec/truehd.rs +++ b/src/mux/codec/truehd.rs @@ -93,26 +93,43 @@ impl TrueHdParser { /// Returns `false` (not corrupt) when the AU is too short to judge or no /// major sync has established `num_substreams` yet — we never drop what we /// cannot verify. Verified against real ffmpeg TrueHD (3600/3600 AUs). - fn au_is_corrupt(&mut self, au: &[u8], is_major_sync: bool) -> bool { + fn au_check(&mut self, au: &[u8], is_major_sync: bool) -> AuCheck { let mut header_size = 4; + let mut format_info = None; if is_major_sync { let ms = &au[4..]; let Some(mshdr) = mlp_major_sync_header_size(ms) else { - return false; // too short to hold a major-sync header — can't judge + // A major sync too short to hold its header can't be CRC-validated + // — NOT a safe resync/re-init point. Treat as unverifiable, not a + // clean major sync. + return AuCheck::Unverifiable; }; if !mlp_major_sync_crc_ok(ms, mshdr) { - return true; // corrupt major-sync header + return AuCheck::Corrupt; // corrupt major-sync header } self.num_substreams = mlp_num_substreams(ms); header_size += mshdr; + // The rate nibble is only trustworthy once the major sync's CRC has + // validated (above), so capture format_info here and refine the PTS + // cadence from it ONLY on this validated path. + if au.len() >= 12 { + format_info = Some(u32::from_be_bytes([au[8], au[9], au[10], au[11]])); + } } let Some(nss) = self.num_substreams else { - return false; // no major sync seen yet — nothing to check against + return AuCheck::Unverifiable; // no major sync seen yet — can't check parity }; let Some(shs) = mlp_substr_header_size(au, header_size, nss) else { - return false; // directory runs off the AU — can't judge + return AuCheck::Unverifiable; // directory runs off the AU — can't judge }; - !mlp_parity_ok(au, header_size, shs) + if !mlp_parity_ok(au, header_size, shs) { + return AuCheck::Corrupt; + } + if is_major_sync { + AuCheck::ValidMajorSync { format_info } + } else { + AuCheck::Ok + } } /// Size (bytes) of the AC-3 frame at the buffer head. @@ -169,6 +186,23 @@ fn ac3_boundary_corroborated(buf: &[u8], frame_bytes: usize) -> bool { next_words != 0 && next_words * 2 <= 32768 } +/// Decodability verdict for one TrueHD/MLP access unit. +enum AuCheck { + /// Verified undecodable: a major-sync header whose CRC failed, or any AU + /// whose substream-directory parity failed. Feeds the poison verdict. + Corrupt, + /// A CRC-validated major sync — a safe re-init / resync point. `format_info` + /// (AU bytes 8..12, present when the AU is long enough) is trustworthy here, + /// so the caller refines the PTS cadence ONLY from this validated path. + ValidMajorSync { format_info: Option }, + /// A valid (parity-OK) non-major-sync access unit. + Ok, + /// Cannot be judged — a major sync too short to hold/CRC its header, or a + /// stream head before any major sync established `num_substreams`. Never + /// dropped on its own, and never treated as a clean resync point. + Unverifiable, +} + /// Outcome of sizing the AC-3 frame at the TrueHD buffer head. enum Ac3Size { /// fscod/frmsizecod don't map to a real frame size — resync, don't wait. @@ -385,62 +419,92 @@ impl CodecParser for TrueHdParser { & 0xFFFF_FFFE) == 0xF872_6FBA; - // On a major sync the 32-bit `format_info` word (immediately after - // the 4-byte sync, i.e. AU bytes 8..12) carries the rate nibble. - // Refine the per-AU PTS increment to the actual rate family. The - // 48 kHz family resolves to the unchanged 833_333 default, so the - // common case stays byte-identical; only the 44.1 kHz family shifts. - if is_major_sync && unit_bytes >= 12 { - let format_info = - u32::from_be_bytes([self.buf[8], self.buf[9], self.buf[10], self.buf[11]]); - self.au_duration_ns = truehd_au_duration_ns(format_info); - } - - // Decodability gate. MLP/TrueHD state persists across AUs, so a - // corrupt AU is dropped FORWARD to the next major sync (the clean - // re-init point) rather than excised in place; the PTS clock advances - // across every dropped AU so the drop is a silence gap, never a shift. + // Decodability gate. MLP/TrueHD decode state persists across access + // units, so a corrupt AU is dropped FORWARD to the next VALIDATED + // major sync (the clean re-init point) rather than excised in place. + // The PTS clock advances across every dropped AU so a drop is a + // silence gap, never a shift. let au = self.buf[..unit_bytes].to_vec(); let pts = self.next_pts_ns; - let corrupt = self.tally.is_poisoned() || self.au_is_corrupt(&au, is_major_sync); - let emit = if self.resync_pending { - // Only a valid major sync clears the resync and is emitted. - if is_major_sync && !corrupt { - self.resync_pending = false; - true - } else { - false - } - } else if corrupt { - self.resync_pending = true; - false - } else { - true - }; + let mut emit_keyframe: Option = None; // Some(is_keyframe) => emit + let mut drop_reason: Option<(&'static str, bool)> = None; // (reason, verified) - if emit { + if self.tally.is_poisoned() { + // Whole track already judged dead — collateral drop (does not + // re-feed the poison verdict). + drop_reason = Some(("track-poisoned", false)); + } else { + match self.au_check(&au, is_major_sync) { + AuCheck::ValidMajorSync { format_info } => { + // The rate nibble is trustworthy only now that the major + // sync's CRC has validated. Refine the per-AU PTS + // increment (48 kHz family stays the 833_333 default). + if let Some(fi) = format_info { + self.au_duration_ns = truehd_au_duration_ns(fi); + } + // A validated major sync is the ONLY clean resync point. + self.resync_pending = false; + emit_keyframe = Some(true); + } + AuCheck::Corrupt => { + if self.resync_pending { + // Part of the current drop-forward run — collateral. + drop_reason = Some(("resync", false)); + } else { + // The trigger: one verified corruption that starts the + // drop-forward. Only this counts toward poison. + let r = if is_major_sync { + "major-sync-crc" + } else { + "parity" + }; + drop_reason = Some((r, true)); + self.resync_pending = true; + } + } + AuCheck::Ok => { + if self.resync_pending { + // Decode state is invalid until the next validated + // major sync, so even a parity-OK AU is undecodable + // here — collateral drop. + drop_reason = Some(("resync", false)); + } else { + emit_keyframe = Some(false); + } + } + AuCheck::Unverifiable => { + if self.resync_pending { + // Not a validated major sync — do NOT clear the resync + // on it; keep dropping forward. + drop_reason = Some(("resync", false)); + } else { + // Head of stream / too-short AU: keep (never drop what + // we cannot verify). + emit_keyframe = Some(is_major_sync); + } + } + } + } + + if let Some(keyframe) = emit_keyframe { self.tally.record_kept(); frames.push(Frame { discontinuity: false, coding: None, source: None, pts_ns: pts, - keyframe: is_major_sync, + keyframe, data: au, duration_ns: None, }); - } else { - let reason = if self.tally.is_poisoned() { - "track-poisoned" - } else if is_major_sync && corrupt { - "major-sync-crc" - } else if corrupt { - "parity" + } else if let Some((reason, verified)) = drop_reason { + if verified { + self.tally + .record_drop(pts, self.au_duration_ns, au.len(), reason); } else { - "resync" - }; - self.tally - .record_drop(pts, self.au_duration_ns, au.len(), reason); + self.tally + .record_collateral_drop(pts, self.au_duration_ns, au.len(), reason); + } } self.buf.drain(..unit_bytes); self.next_pts_ns += self.au_duration_ns; @@ -747,6 +811,97 @@ mod tests { ); } + #[test] + fn transient_corruptions_do_not_poison_whole_track() { + // Regression (audit HIGH): TrueHD drop-forward must NOT amplify a couple + // of transient errors into a false whole-track poison. Two corruptions, + // each forcing a long collateral resync run past 200 total AUs, must + // leave the track un-poisoned and keep the good audio that follows. + let mut parser = TrueHdParser::new(); + let mut data = valid_major_sync(); + // Corruption #1 then a long run of normal AUs (all collateral-dropped + // while resyncing — no major sync to re-init on). + let mut bad1 = valid_normal_au(); + bad1[2] ^= 0x01; // single-nibble parity break + data.extend_from_slice(&bad1); + for _ in 0..210 { + data.extend_from_slice(&valid_normal_au()); + } + // A valid major sync resumes; the good AUs after it MUST be kept. + data.extend_from_slice(&valid_major_sync()); + for _ in 0..5 { + data.extend_from_slice(&valid_normal_au()); + } + let frames = parser.parse(&make_pes(data, Some(90000))); + assert!( + !parser.tally.is_poisoned(), + "two transient errors must not poison the track" + ); + // MS1 + resumed MS2 + the 5 good AUs after it survive. + assert_eq!( + frames.len(), + 7, + "post-resync good audio is kept, not poisoned away" + ); + assert!( + parser.dropped_frames() > 200, + "the resync run was still counted for reporting" + ); + } + + #[test] + fn corrupt_major_sync_rate_nibble_does_not_shift_pts() { + // Regression (audit MED): a corrupt major sync whose rate nibble decodes + // to the 44.1 kHz family must NOT refine au_duration_ns — the rate is only + // trustworthy after the CRC validates. Otherwise the resumed 48 kHz audio + // is shifted (not gapped). + let mut parser = TrueHdParser::new(); + let ms1 = valid_major_sync(); // 48 kHz + let mut ms_bad = valid_major_sync(); + // Set the rate nibble (top nibble of format_info = au[8]) to 0x8 (44.1k). + // au[8] is CRC-covered, so this also breaks the major-sync CRC → corrupt. + ms_bad[8] = (ms_bad[8] & 0x0F) | 0x80; + let ms2 = valid_major_sync(); // 48 kHz + let mut data = ms1; + data.extend_from_slice(&ms_bad); + data.extend_from_slice(&ms2); + let frames = parser.parse(&make_pes(data, Some(90000))); + assert_eq!(frames.len(), 2, "corrupt MS dropped; MS1 and MS2 survive"); + assert_eq!( + frames[1].pts_ns - frames[0].pts_ns, + 2 * AU_DURATION_NS, + "resumed audio keeps the 48 kHz cadence — the corrupt MS's 44.1k rate was ignored" + ); + } + + #[test] + fn too_short_major_sync_does_not_clear_resync() { + // Regression (audit LOW): while resyncing, a major sync too short to hold + // (and CRC-validate) its header must NOT be treated as a clean resync + // point — the runt is dropped and only a real validated major sync resumes. + let mut parser = TrueHdParser::new(); + let ms1 = valid_major_sync(); + let mut bad = valid_normal_au(); + bad[2] ^= 0x01; // parity break → triggers resync + // An 8-byte "major sync": length=4 words, sync at bytes 4..8, too short + // to hold the 28-byte major-sync header. + let runt = vec![0x00, 0x04, 0x00, 0x00, 0xF8, 0x72, 0x6F, 0xBA]; + let ms2 = valid_major_sync(); + let mut data = ms1; + data.extend_from_slice(&bad); + data.extend_from_slice(&runt); + data.extend_from_slice(&ms2); + let frames = parser.parse(&make_pes(data, Some(90000))); + assert_eq!(frames.len(), 2, "the runt major sync did not resume decode"); + for f in &frames { + assert_eq!( + f.data.len(), + 200, + "only the real 200-byte major syncs survive" + ); + } + } + #[test] fn clean_truehd_stream_drops_nothing() { // A run of valid AUs passes untouched — zero false positives (the CRC and