From 967d0ac77e5814c4b988800ca5e7499bbb27759a Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Fri, 24 Jul 2026 10:28:21 -0700 Subject: [PATCH] mux: fix DTS core-header false-drops + close TrueHD/mux gate coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DTS core decodability gate (core_header_drop_reason) — full ETSI TS 102 114 spec-conformance sweep against ffmpeg ff_dca_parse_core_frame_header and dcadec parse_frame_header: - deficit_samples: only require ==32 for NORMAL frames (FTYPE==1). A TERMINATION frame (FTYPE==0, the last frame of a stream) legitimately carries fewer and is fully decodable; the old unconditional check dropped it on every stream that ends on one — a guaranteed per-track silence gap. Matches ffmpeg (normal_frame && deficit != DCA_PCMBLOCK_SAMPLES) and dcadec (branches on normal_frame). - reserved bit (after RATE): both reference decoders SKIP it (ffmpeg skip_bits1, dcadec bits_skip1 "Reserved field") and never reject on it. Rejecting was a false-drop that silenced any real stream whose encoder set the bit. Relaxed to read-and-discard; DropReason::ReservedBit removed. Swept and confirmed spec-correct as-is (no change): npcmblocks multiple-of-8, frame_size>=96, audio_mode>=16 (ffmpeg-permissive), sample-rate validity table (matches avpriv_dca_sample_rates incl 96k/192k at 14/15), LFE flag==3 invalid, PCMR bits table (matches dcadec sample_res {16,16,20,20,0,24,24,0}). Bit-read order verified field-by-field against dcadec. bit_rate is left unvalidated (lenient, never-false-drop direction) as before. Tests: termination frame with small deficit is kept; normal frame with bad deficit is dropped; reserved-bit-set frame is kept. make_bad_dts_core now uses an invalid LFE flag (duration-neutral) instead of the relaxed reserved bit. TrueHD: add coverage for the EXTENDED major-sync header CRC path (ms[25]&1, mshdr=28+2+2n) — previously zero-tested, the exact path a shipped endianness bug once used to silently drop whole 7.1/Atmos tracks. Trailer is an independently-computed oracle (separate CRC-16/0x2D, anchored to the 0x4FF7 catalogue value, NOT crc16_mlp), stored little-endian; test asserts accept, body-corruption reject, and big-endian-trailer reject. mux driver: extract the finish completion mapping into pure mux_run_completed so the finalize_failed -> completed=false branch (reachable only via real write-thread wedge timing) is unit-tested; add an out-of-range MuxInput::Session title_index test asserting a clean Error::MuxTrackRange (E9011) instead of a panic. --- src/mux/codec/dts.rs | 128 +++++++++++++++++++++++++++++++--------- src/mux/codec/truehd.rs | 97 ++++++++++++++++++++++++++++++ src/mux/driver.rs | 99 ++++++++++++++++++++++++++++++- 3 files changed, 295 insertions(+), 29 deletions(-) diff --git a/src/mux/codec/dts.rs b/src/mux/codec/dts.rs index 938e3df..1d36714 100644 --- a/src/mux/codec/dts.rs +++ b/src/mux/codec/dts.rs @@ -683,9 +683,10 @@ fn dts_core_duration_ns(data: &[u8]) -> u64 { } /// DTS core-header validity constants (ETSI TS 102 114). -/// `deficit_samples` must equal this (`DTS_PCMBLOCK_SAMPLES`); `npcmblocks` -/// must be a multiple of `DTS_SUBBAND_SAMPLES`; `audio_mode` must be below -/// `DTS_AMODE_COUNT`; `lfe_present == DTS_LFE_FLAG_INVALID` is rejected. +/// For a NORMAL frame `deficit_samples` must equal this (`DTS_PCMBLOCK_SAMPLES`) +/// — a termination frame may carry fewer; `npcmblocks` must be a multiple of +/// `DTS_SUBBAND_SAMPLES`; `audio_mode` must be below `DTS_AMODE_COUNT`; +/// `lfe_present == DTS_LFE_FLAG_INVALID` is rejected. const DTS_PCMBLOCK_SAMPLES: u32 = 32; const DTS_SUBBAND_SAMPLES: u32 = 8; /// Number of LEGAL `AMODE` (channel-arrangement) codes. The 6-bit AMODE field @@ -723,7 +724,6 @@ enum DropReason { FrameSize, Amode, SampleRate, - ReservedBit, LfeFlag, PcmRes, TrackPoisoned, @@ -738,7 +738,6 @@ impl DropReason { DropReason::FrameSize => "frame-size", DropReason::Amode => "audio-mode", DropReason::SampleRate => "sample-rate", - DropReason::ReservedBit => "reserved-bit", DropReason::LfeFlag => "lfe-flag", DropReason::PcmRes => "pcm-resolution", DropReason::TrackPoisoned => "track-poisoned", @@ -762,9 +761,17 @@ impl DropReason { fn core_header_drop_reason(au: &[u8]) -> Option { let mut r = BitReader::new(au.get(SYNCWORD_BYTES..)?); - let _ftype = r.read_bit()?; + // FTYPE: 1 = NORMAL frame, 0 = TERMINATION frame (the last frame of the + // stream). Per ETSI TS 102 114 and both reference decoders — ffmpeg's + // `ff_dca_parse_core_frame_header` (`normal_frame && deficit_samples != + // DCA_PCMBLOCK_SAMPLES`) and dcadec's `parse_frame_header` (which branches + // on `normal_frame`) — the deficit-sample field must equal 32 ONLY for a + // normal frame. A termination frame legitimately carries fewer samples and + // is fully decodable; dropping it would silence the last frame of every + // stream that ends on one (a guaranteed per-track loss on real discs). + let normal_frame = r.read_bit()? == 1; let deficit_samples = r.read_bits(5)? + 1; - if deficit_samples != DTS_PCMBLOCK_SAMPLES { + if normal_frame && deficit_samples != DTS_PCMBLOCK_SAMPLES { return Some(DropReason::DeficitSamples); } let crc_present = r.read_bit()? == 1; @@ -785,9 +792,12 @@ fn core_header_drop_reason(au: &[u8]) -> Option { return Some(DropReason::SampleRate); } let _br_code = r.read_bits(5)?; - if r.read_bit()? != 0 { - return Some(DropReason::ReservedBit); - } + // Reserved bit. Both reference decoders SKIP this field rather than reject + // on it — ffmpeg (`skip_bits1`) and dcadec (`bits_skip1`, comment "Reserved + // field"). A frame that sets it is still fully decodable, so rejecting it + // was a false-drop that silenced any real stream whose encoder set the bit. + // Read past it without gating (never reject a decodable frame). + let _reserved = r.read_bit()?; // drc, ts, aux, hdcd (1 each) → ext_audio_type (3) → ext_present, aspf (1 each). r.skip_bits(4)?; r.skip_bits(3)?; @@ -831,11 +841,12 @@ mod tests { let fsize = size - 1; let mut data = vec![0u8; size]; data[0..4].copy_from_slice(&DTS_CORE_SYNC); - // byte4: FTYPE(0) SHORT(5) CPF(0) NBLKS-high(0). SHORT = 31 makes - // deficit_samples = 32 = DTS_PCMBLOCK_SAMPLES, which the decodability - // gate (per ETSI TS 102 114) requires of a real core frame. NBLKS high - // bit (byte4 bit0) stays 0 for NBLKS = 15. - data[4] = 31u8 << 2; + // byte4: FTYPE(1) SHORT(5) CPF(0) NBLKS-high(0). FTYPE = 1 = a NORMAL + // frame (the common real-stream case); SHORT = 31 makes deficit_samples + // = 32 = DTS_PCMBLOCK_SAMPLES, which the decodability gate (per ETSI TS + // 102 114) requires of a normal frame. NBLKS high bit (byte4 bit0) stays + // 0 for NBLKS = 15. (0x80 | (31 << 2) = 0xFC.) + data[4] = 0x80 | (31u8 << 2); // NBLKS = 15 → (15+1)*32 = 512 samples/frame (the DVD/UHD DTS-core norm). // NBLKS is byte4 bit0 + byte5 bits7-2; here byte4 bit0 = 0, byte5 = 15<<2. data[5] = (15u8 << 2) | ((fsize >> 12) & 0x03) as u8; @@ -1791,22 +1802,24 @@ mod tests { ); } - /// A structurally-framed but UNDECODABLE core: a valid `make_dts_core` - /// whose reserved header bit is set. It still sizes and syncs correctly (so - /// the framer delimits it normally), but the core-frame header validity - /// check rejects it as a set reserved bit (ETSI TS 102 114). The reserved - /// bit is byte9 bit4 in the core header (after SYNC..RATE). + /// A structurally-framed but UNDECODABLE core: a valid `make_dts_core` whose + /// LFE flag is set to the reserved value 3 (`DTS_LFE_FLAG_INVALID`). It still + /// sizes and syncs correctly (so the framer delimits it normally), but the + /// core-frame header validity check rejects it as an invalid LFE flag (ETSI + /// TS 102 114; dcadec `LFE_FLAG_INVALID`). LFE is byte10 bits2-1, and does + /// NOT feed the frame duration (NBLKS + SFREQ only), so a dropped bad core + /// still carries the same `DTS_CORE_DUR_NS` as its good peers. fn make_bad_dts_core(size: usize) -> Vec { let mut d = make_dts_core(size); assert!( core_header_drop_reason(&d).is_none(), "base core is decodable" ); - d[9] |= 0x10; // set the reserved bit + d[10] |= 0x06; // LFE flag = 3 (invalid) assert_eq!( core_header_drop_reason(&d), - Some(DropReason::ReservedBit), - "reserved-bit core must be judged undecodable" + Some(DropReason::LfeFlag), + "invalid-LFE core must be judged undecodable" ); d } @@ -1967,11 +1980,6 @@ mod tests { d[8] &= !0x3C; assert_eq!(core_header_drop_reason(&d), Some(DropReason::SampleRate)); - // reserved bit set: byte9 bit4. - let mut d = good.clone(); - d[9] |= 0x10; - assert_eq!(core_header_drop_reason(&d), Some(DropReason::ReservedBit)); - // lfe_present == 3: LFE is byte10 bits2-1. let mut d = good.clone(); d[10] |= 0x06; @@ -2022,6 +2030,70 @@ mod tests { } } + /// Set FTYPE (byte4 bit7: 1 = normal, 0 = termination) and the 5-bit SHORT + /// field (byte4 bits6-2), leaving CPF and the NBLKS high bit (bits1-0) intact. + /// `deficit_samples = short_field + 1`. + fn set_ftype_short(core: &mut [u8], normal: bool, short_field: u8) { + core[4] = (core[4] & 0x03) | ((normal as u8) << 7) | ((short_field & 0x1F) << 2); + } + + #[test] + fn termination_frame_with_small_deficit_is_kept() { + // ETSI TS 102 114 / ffmpeg (`normal_frame && deficit != 32`) / dcadec: + // a TERMINATION frame (FTYPE=0) may legally carry fewer than 32 deficit + // samples and is fully decodable. It must NOT be dropped — dropping the + // last frame of a stream silences real audio (recover-100% violation). + let mut core = make_dts_core(512); + set_ftype_short(&mut core, false, 10); // termination, deficit = 11 (< 32) + assert_eq!( + core_header_drop_reason(&core), + None, + "a termination frame with a small deficit is decodable and must be kept" + ); + // End-to-end: a termination frame closed by a following core survives. + let mut term = make_dts_core(512); + set_ftype_short(&mut term, false, 5); // deficit = 6 + let mut stream = term; + stream.extend_from_slice(&make_dts_core(640)); + let mut parser = DtsParser::new(); + let mut frames = parser.parse(&make_pes(stream, Some(90000))); + frames.extend(parser.flush()); + assert_eq!(frames.len(), 2, "termination frame is emitted, not dropped"); + assert_eq!(frames[0].data.len(), 512); + assert_eq!(parser.dropped_frames(), 0); + } + + #[test] + fn normal_frame_with_wrong_deficit_is_dropped() { + // The other side of the FTYPE gate: a NORMAL frame (FTYPE=1) whose + // deficit-sample field is not 32 is genuinely undecodable and must be + // dropped. Guards against the fix over-relaxing into "never check deficit". + let mut core = make_dts_core(512); + set_ftype_short(&mut core, true, 10); // normal, deficit = 11 (!= 32) + assert_eq!( + core_header_drop_reason(&core), + Some(DropReason::DeficitSamples), + "a normal frame with deficit != 32 is undecodable and must be dropped" + ); + } + + #[test] + fn reserved_bit_set_is_not_dropped() { + // The bit after RATE is a RESERVED field that both reference decoders + // SKIP (ffmpeg `skip_bits1`, dcadec `bits_skip1` "Reserved field") — they + // never reject a frame that sets it. Rejecting was a false-drop that + // silenced any real stream whose encoder set the bit. Setting it (byte9 + // bit4) on an otherwise-valid core must leave it KEPT. + let mut core = make_dts_core(512); + assert_eq!(core_header_drop_reason(&core), None, "baseline decodable"); + core[9] |= 0x10; // set the reserved bit + assert_eq!( + core_header_drop_reason(&core), + None, + "a set reserved bit must NOT drop a decodable frame" + ); + } + /// 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 diff --git a/src/mux/codec/truehd.rs b/src/mux/codec/truehd.rs index ddb8629..10cba86 100644 --- a/src/mux/codec/truehd.rs +++ b/src/mux/codec/truehd.rs @@ -835,6 +835,103 @@ mod tests { // `corrupt_major_sync_drops_forward_to_next_valid`). } + /// Independent bitwise CRC-16 (poly 0x002D, init 0, MSB-first) — a SEPARATE + /// oracle from `crc16_mlp`, so a fixture built with it is not tautological + /// with the validator under test. Anchored to the catalogue check value + /// (0x4FF7 for "123456789") so the oracle itself is proven correct without + /// reference to the code under test. + fn ref_crc16_2d(data: &[u8]) -> u16 { + let mut crc: u16 = 0; + for &b in data { + crc ^= (b as u16) << 8; + for _ in 0..8 { + crc = if crc & 0x8000 != 0 { + (crc << 1) ^ 0x002D + } else { + crc << 1 + }; + } + } + crc + } + + #[test] + fn extended_major_sync_crc_validates_and_rejects() { + // COVERAGE GAP (the endianness bug that once "silently dropped the whole + // track" on real 7.1/Atmos): the EXTENDED major-sync header path + // (ms[25]&1 set, mshdr = 28 + 2 + 2*n) had ZERO test coverage — every + // other fixture builds only the basic 28-byte header. Build an extended + // header whose trailer is an INDEPENDENTLY-computed oracle (ref_crc16_2d, + // NOT crc16_mlp) stored LITTLE-ENDIAN, and assert the validator accepts + // it, rejects a body corruption, and rejects the same trailer stored + // big-endian (which is exactly the endianness-mix regression). + assert_eq!( + ref_crc16_2d(b"123456789"), + 0x4FF7, + "oracle anchored to catalogue" + ); + + // n = 3 extension words → mshdr = 28 + 2 + 2*3 = 36. + let n = 3usize; + let mshdr = 28 + 2 + 2 * n; + assert_eq!(mshdr, 36); + let mut ms = vec![0u8; 40]; // slack past the 36-byte header + // Non-trivial, varied body so the CRC is a meaningful function of it. + for (i, b) in ms.iter_mut().enumerate().take(mshdr - 4) { + *b = (0x37u8).wrapping_add((i as u8).wrapping_mul(0x53)); + } + ms[25] |= 1; // extension flag → selects the extended header size + ms[26] = (ms[26] & 0x0F) | ((n as u8) << 4); // extension word count in high nibble + + // The 2-byte "penultimate" word (between the CRC-covered body and the + // trailer). Chosen non-zero and non-palindromic so the LE/BE distinction + // is observable. + ms[mshdr - 4] = 0x12; + ms[mshdr - 3] = 0x34; + + // Oracle: checksum16 = crc16_2D(body).swap_bytes() ^ le16(penultimate), + // computed with the INDEPENDENT ref CRC, then stored LITTLE-ENDIAN. + let le_word = u16::from_le_bytes([ms[mshdr - 4], ms[mshdr - 3]]); + let trailer = ref_crc16_2d(&ms[..mshdr - 4]).swap_bytes() ^ le_word; + ms[mshdr - 2] = (trailer & 0xFF) as u8; + ms[mshdr - 1] = (trailer >> 8) as u8; + assert_ne!( + ms[mshdr - 2], + ms[mshdr - 1], + "trailer bytes must differ so the LE/BE swap below is a real distinction" + ); + + // The extended header size is computed from ms[25]/ms[26]. + assert_eq!( + mlp_major_sync_header_size(&ms), + Some(mshdr), + "extended header size = 28 + 2 + 2*n" + ); + // The validator accepts the independently-built extended major sync. + assert!( + mlp_major_sync_crc_ok(&ms, mshdr), + "valid extended major-sync checksum must validate" + ); + + // A single corrupted body byte must be rejected. + let mut corrupt = ms.clone(); + corrupt[10] ^= 0xFF; + assert!( + !mlp_major_sync_crc_ok(&corrupt, mshdr), + "a corrupted extended major sync must be rejected" + ); + + // The endianness regression: the SAME checksum stored big-endian must be + // rejected. A validator that reads the trailer big-endian (the shipped + // bug) would instead accept this and reject the correct LE form above. + let mut swapped = ms.clone(); + swapped.swap(mshdr - 2, mshdr - 1); + assert!( + !mlp_major_sync_crc_ok(&swapped, mshdr), + "a big-endian-stored trailer must be rejected (little-endian is load-bearing)" + ); + } + #[test] fn parity_failure_is_dropped() { // A normal AU whose header parity is broken (after a major sync sets diff --git a/src/mux/driver.rs b/src/mux/driver.rs index eb418b1..e8d0239 100644 --- a/src/mux/driver.rs +++ b/src/mux/driver.rs @@ -540,6 +540,20 @@ fn reader_event_fn(events: Arc) -> crate::sector::prefetched::Eve /// The reader-agnostic driver body: headers → gate → sink → pump → finish. /// Split out so it can be unit-tested against a synthetic [`Stream`] (the /// injection seam), independent of which constructor built `stream`. +/// Whether a finished mux counts as COMPLETED. A clean operator stop +/// (`interrupted`), a wedged/halted finalize (`finalize_failed` — the write +/// [`Pipeline`] returned `Halted`/`PipelineJoinTimeout` from `finish`), or a +/// halt cancellation each force `completed = false`, so the consumer runs its +/// stop-preserves-staging path instead of reporting a truncated file as done. +/// +/// Extracted as a pure fn because the `finalize_failed` branch is otherwise +/// reachable only through real write-thread wedge timing (the internally-built +/// `WriteSink` offers no seam to force a `finish` timeout deterministically), so +/// the mapping is unit-tested here directly. +fn mux_run_completed(interrupted: bool, finalize_failed: bool, halt_cancelled: bool) -> bool { + !(interrupted || finalize_failed || halt_cancelled) +} + fn drive_mux( mut stream: Box, dest_url: &str, @@ -738,7 +752,7 @@ fn drive_mux( Err(e) => return Err(e.into()), }; - if interrupted || finalize_failed || halt.is_cancelled() { + if !mux_run_completed(interrupted, finalize_failed, halt.is_cancelled()) { return Ok(MuxOutcome { completed: false, output_opened: true, @@ -1692,6 +1706,89 @@ mod tests { ); } + /// FIX 3: a mux whose read side drained cleanly (`interrupted = false`, halt + /// not cancelled) but whose write pipeline WEDGED on finish (`finish_with_halt` + /// → `Err(Halted | PipelineJoinTimeout)` → `finalize_failed = true`) must fall + /// through to `completed = false` — never surface a truncated file as a + /// finished rip. The wedge is reachable only via real write-thread timing, so + /// the completion mapping is tested via the extracted pure fn. + /// + /// Mutation: dropping `finalize_failed` from `mux_run_completed`'s condition + /// makes `mux_run_completed(false, true, false)` return `true` → this fails. + #[test] + fn finalize_failed_forces_incomplete_outcome() { + // The load-bearing case: clean drain, wedged finalize → NOT completed. + assert!( + !mux_run_completed(false, true, false), + "a wedged/halted finalize must force completed = false" + ); + // A fully clean finish is the only path to completed = true. + assert!( + mux_run_completed(false, false, false), + "a clean drain + clean finalize completes" + ); + // The other two forcers likewise yield incomplete. + assert!( + !mux_run_completed(true, false, false), + "operator stop → incomplete" + ); + assert!( + !mux_run_completed(false, false, true), + "halt cancel → incomplete" + ); + } + + /// FIX 4: `MuxInput::Session` with a `title_index` past the disc's title count + /// must surface a clean `Error::MuxTrackRange` (code E9011), NOT panic on the + /// out-of-range `titles.get(idx)`. Everything else is valid (disc scanned, + /// reader staged) so the range guard is the sole failure. + /// + /// Mutation: replacing the `.ok_or(MuxTrackRange…)?` guard with `.unwrap()` + /// panics on the out-of-range index → this test fails. + #[test] + fn mux_input_session_out_of_range_title_is_clean_error_not_panic() { + use crate::disc::Extent; + use crate::session::DiscSession; + + let unit_key = [0x5Au8; 16]; + let reader = Box::new(AacsUnitReader { + unit: encrypted_audio_unit(&unit_key), + capacity: 2048, + }); + let mut title = aac_audio_title(0x1100); + title.extents = vec![Extent { + start_lba: 0, + sector_count: 3, + }]; + let disc = aacs_session_disc(title, unit_key); + let num_titles = disc.titles.len(); + let mut session = DiscSession::from_parts_for_test(Some(disc), Some(reader), None); + + let opts = MuxOptions { + skip_errors: false, + batch_sectors: 3, + raw: false, + send_deadline: Some(Duration::from_secs(60)), + }; + let halt = Halt::new(); + let err = mux_stream( + MuxInput::Session { + session: &mut session, + title_index: num_titles + 5, // out of range + }, + "null://", + &opts, + &halt, + Arc::new(NoopEvents), + ) + .expect_err("an out-of-range title index must be a clean error, not a panic"); + // MuxTrackRange renders as "E9011: track/tracks". + assert!( + err.to_string().contains("E9011"), + "expected MuxTrackRange (E9011), got: {err}" + ); + } + /// The shared `resolve_inline_base_map` helper's gating: an AACS key set /// yields a map (Some); CSS/clear/None and `raw` yield None (CSS self-cracks /// in `DiscStream::new`; raw is ciphertext passthrough). Guards the Session