From bf2d15f39cacc6365d042f8ffb6fc00c1a887a8c Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Wed, 29 Jul 2026 17:54:27 -0700 Subject: [PATCH] Cover the non-NAL video path, including the wiring that selects it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit set_nal_video had exactly one production caller and zero test callers. The branch it gates decides whether a video track's ES goes through length_prefixed_to_annex_b, and MPEG-2 and VC-1 must not: they are already start-code ES. Getting it wrong is silent — frame_count still increments, so the mux reports success while emitting a video-less file. Five tests, each verified against a real mutant: * non-NAL ES passes through byte-for-byte, and the default path still converts. Both use a deliberately length-prefix-SHAPED payload so a wrongly-applied conversion rewrites the leading four bytes into a start code — a payload the converter happened to leave alone would let a mutant pass. * a non-NAL keyframe arms params_written, so following non-keyframes are not dropped by the pre-keyframe guard. * set_nal_video rejects an out-of-range track instead of panicking. * M2tsStream::create wires a VC-1 track to the non-NAL path. That last one matters more than it looks. The four TsMuxer-level tests set the flag themselves, so deleting the set_nal_video loop from M2tsStream::create left all 2366 tests passing — the exact mutant the finding named went undetected until a test drove the real wiring. It now also catches the subtler mutant of widening the matches! arm to include Vc1. --- src/mux/m2ts.rs | 46 ++++++++++++++++++++++ src/mux/tsmux.rs | 99 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+) diff --git a/src/mux/m2ts.rs b/src/mux/m2ts.rs index a72042c..fc45b39 100644 --- a/src/mux/m2ts.rs +++ b/src/mux/m2ts.rs @@ -170,6 +170,52 @@ mod tests { } } + /// `M2tsStream::create` must opt a VC-1 video track OUT of Annex-B conversion. + /// + /// This pins the WIRING in `create`, not just `TsMuxer`'s flag: deleting the + /// `set_nal_video` loop leaves every TsMuxer-level test passing, because those + /// drive the muxer directly and set the flag themselves. Only a test that goes + /// through `create` catches it — and mangling MPEG-2/VC-1 video is silent, since + /// frame_count still increments and the mux reports success. + /// + /// Mutation: remove the `set_nal_video` loop from `create`, or widen its + /// `matches!` to include Vc1 -> the ES gains a start code and this fails. + #[test] + fn vc1_video_is_wired_to_the_non_nal_path() { + let mut title = make_title(); + if let DiscStream::Video(v) = &mut title.streams[0] { + v.codec = Codec::Vc1; + } + title.codec_privates = vec![None]; + + // Length-prefix SHAPED ES: if the conversion is wrongly applied it rewrites + // these leading four bytes into a 00 00 00 01 start code. + let es: Vec = vec![0x00, 0x00, 0x00, 0x06, 0x0F, 0x12, 0x34, 0x56, 0x78, 0x9A]; + + let shared = std::sync::Arc::new(std::sync::Mutex::new(Vec::::new())); + let sink = SharedSink(shared.clone()); + let mut stream = M2tsStream::create(sink, &title).unwrap(); + stream + .write(&PesFrame { + coding: None, + source: None, + track: 0, + pts: 0, + keyframe: true, + data: es.clone(), + duration_ns: None, + }) + .unwrap(); + stream.finish().unwrap(); + drop(stream); + + let buf = shared.lock().unwrap().clone(); + assert!( + buf.windows(es.len()).any(|w| w == &es[..]), + "VC-1 ES must reach the output verbatim, not converted to Annex-B" + ); + } + #[test] fn m2ts_stream_forwards_keyframe_to_rai() { let title = make_title(); diff --git a/src/mux/tsmux.rs b/src/mux/tsmux.rs index 7163c89..ab2c2fc 100644 --- a/src/mux/tsmux.rs +++ b/src/mux/tsmux.rs @@ -786,6 +786,105 @@ mod tests { out } + /// `set_nal_video(_, false)` must pass the ES through byte-for-byte: MPEG-2 and + /// VC-1 are not NAL-based, so their ES already IS the wire format and + /// `length_prefixed_to_annex_b` would mangle it. + /// + /// The payload is deliberately length-prefix SHAPED (a big-endian length + /// followed by that many bytes) so the conversion, if wrongly applied, rewrites + /// the leading four bytes into a `00 00 00 01` start code. That makes the two + /// paths produce visibly different bytes; a payload the converter happened to + /// leave alone would let a mutant pass. + /// + /// Mutation: delete the `set_nal_video` call, or flip the `nal_video` default, + /// and the emitted ES gains a start code -> this fails. + #[test] + fn non_nal_video_es_passes_through_unconverted() { + // 4-byte BE length (6) + 6 payload bytes: exactly what the Annex-B + // converter looks for, so a wrongly-applied conversion is unmissable. + let es: Vec = vec![0x00, 0x00, 0x00, 0x06, 0xB3, 0x12, 0x34, 0x56, 0x78, 0x9A]; + + let mut sink: Vec = Vec::new(); + { + let mut mux = TsMuxer::new(&mut sink, &[VIDEO_PID]); + mux.set_nal_video(0, false).unwrap(); + mux.write_frame(0, 0, true, &es).unwrap(); + mux.finish().unwrap(); + } + let packets = parse_bd_ts(&sink); + let out = reassemble_es(&packets, VIDEO_PID); + assert_eq!( + &out[..es.len()], + &es[..], + "non-NAL video ES must be emitted verbatim, start-code-free" + ); + } + + /// The default (`nal_video` = true) still converts, so the test above is + /// pinning the flag rather than a no-op. Same input, opposite expectation. + #[test] + fn nal_video_es_is_converted_to_annex_b_by_default() { + let es: Vec = vec![0x00, 0x00, 0x00, 0x06, 0xB3, 0x12, 0x34, 0x56, 0x78, 0x9A]; + + let mut sink: Vec = Vec::new(); + { + let mut mux = TsMuxer::new(&mut sink, &[VIDEO_PID]); + // No set_nal_video call — the default must be the converting path. + mux.write_frame(0, 0, true, &es).unwrap(); + mux.finish().unwrap(); + } + let packets = parse_bd_ts(&sink); + let out = reassemble_es(&packets, VIDEO_PID); + assert_eq!( + &out[..4], + &[0x00, 0x00, 0x00, 0x01], + "the default path replaces the length prefix with an Annex-B start code" + ); + assert_eq!( + &out[4..10], + &es[4..10], + "the NAL body itself is carried unchanged" + ); + } + + /// A non-NAL video track must still arm `params_written`, or every later + /// non-keyframe would fail the drop guard and silently vanish — the same class + /// of bug `empty_data_keyframe_arms_params_so_later_frames_survive` guards on + /// the NAL path. + #[test] + fn non_nal_video_keyframe_arms_params_so_later_frames_survive() { + let key: Vec = vec![0x00, 0x00, 0x01, 0xB3, 0xAA, 0xBB]; + let non_key: Vec = vec![0x00, 0x00, 0x01, 0xB6, 0xCC, 0xDD]; + + let mut sink: Vec = Vec::new(); + { + let mut mux = TsMuxer::new(&mut sink, &[VIDEO_PID]); + mux.set_nal_video(0, false).unwrap(); + mux.write_frame(0, 0, true, &key).unwrap(); + mux.write_frame(0, 41_000_000, false, &non_key).unwrap(); + mux.finish().unwrap(); + } + let packets = parse_bd_ts(&sink); + let out = reassemble_es(&packets, VIDEO_PID); + assert!( + out.windows(non_key.len()).any(|w| w == &non_key[..]), + "the non-keyframe following a non-NAL keyframe must not be dropped" + ); + } + + /// `set_nal_video` rejects an out-of-range track rather than panicking on the + /// index — this is library API and the crate must not panic from it. + #[test] + fn set_nal_video_out_of_range_track_errors() { + let mut sink: Vec = Vec::new(); + let mut mux = TsMuxer::new(&mut sink, &[VIDEO_PID]); + assert!( + mux.set_nal_video(1, false).is_err(), + "track 1 does not exist on a one-track muxer" + ); + assert!(mux.set_nal_video(0, false).is_ok(), "track 0 does exist"); + } + #[test] fn every_packet_is_exactly_192_bytes() { // BD-TS packets are 192 bytes (4 TP_extra + 188 TS). The muxer must