From d4021114cd805768089a70faea5ac58587dde0f9 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Mon, 13 Jul 2026 11:24:23 -0700 Subject: [PATCH] Harden 3D MVC mux: robustness + tests (audit round 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Triage of a 10-lens code audit of the 3D branch. Fixes for real defects; rejected three spec false-positives that matched the ISO/IEC 14496-15 §7.6.2 record verbatim. Robustness / correctness: - Never panic when a title's only video is the MVC dependent view: the base is now the first NON-dependent video, so a dependent-only title sets up no merge (muxed as an ordinary track) instead of hitting an `expect` on the skipped track slot. - Drop a per-frame BlockAdditional (BlockAddID=2) when the track declared no mvcC mapping (dependent params not captured before the header) — a plain block keeps the file conforming instead of an orphaned add. - A non-keyframe MVC base frame always carries a ReferenceBlock (fall back to a 0 offset in the pre-first-keyframe corner) so it is never mistaken for a seek point. - Reference the last keyframe on the PRIMARY video track only, so a secondary video track's keyframe can't become a cross-track reference. - dep_by_pts overflow: bound BEFORE inserting so the just-arrived dependent survives the drift-clear; count a displaced duplicate-PTS dependent as an orphan instead of losing it silently. API / docs: - Fold write_frame_with_additional into write_frame(..., Option<&[u8]>) per the "no foo_with_X" convention. - Fix mvc_params doc (StereoMode is intentionally not emitted); remove a stale PAT/PMT comment describing an approach that was never taken. Tests: MVCDecoderConfigurationRecord over-length guards; write_int minimal two's-complement widths; BlockGroup/BlockAdditions/BlockAdditional + ReferenceBlock emission; additional dropped without a mapping; h264 MVC passthrough keeps param sets in-band; extract_mvc_params no-panic on truncated/empty input; pairing window + dep-overflow edges; no-panic on a dependent-only title. --- src/disc/bluray.rs | 7 -- src/mux/codec/h264.rs | 40 ++++++++ src/mux/ebml.rs | 25 +++++ src/mux/mkv.rs | 216 +++++++++++++++++++++++++++++------------- src/mux/mkvstream.rs | 176 +++++++++++++++++++++++++++------- 5 files changed, 356 insertions(+), 108 deletions(-) diff --git a/src/disc/bluray.rs b/src/disc/bluray.rs index 7a23704..842ecc4 100644 --- a/src/disc/bluray.rs +++ b/src/disc/bluray.rs @@ -250,13 +250,6 @@ impl Disc { }) .collect(); - // 3D: the base STN table lists only the left-eye video. The right-eye - // MVC substream is a SECOND video PID (stream_type 0x20) in the SSIF. - // Read the SSIF head — the PAT/PMT sit in the clear, so no key is needed - // — and add any video PID the STN table didn't already list. This reuses - // the same PAT/PMT scanner the m2ts:// path uses; 0x20 now resolves to - // H.264 video (see `Codec::from_coding_type`), so the dependent view is - // enumerated instead of dropped. // 3D: add the MVC dependent (right-eye) video stream. The base STN table // lists only the left-eye video; the dependent view is a second video // PID (stream_type 0x20) carried in the SSIF. The on-disc PAT/PMT are diff --git a/src/mux/codec/h264.rs b/src/mux/codec/h264.rs index 1bfc6b2..2abc858 100644 --- a/src/mux/codec/h264.rs +++ b/src/mux/codec/h264.rs @@ -674,6 +674,46 @@ mod tests { v } + #[test] + fn mvc_passthrough_keeps_param_sets_inband() { + // A dependent-view access unit: subset SPS (NAL 15) + PPS (NAL 8) + + // coded-slice-extension (NAL 20). No IDR (type 5), so keyframe stays + // false and there is no keyframe re-assertion. + let au = || { + let mut d = Vec::new(); + d.extend_from_slice(&h264_nal(0x6F, &[0x80, 0x00, 0x33, 0xAA])); // subset SPS (15) + d.extend_from_slice(&h264_nal(0x68, &[0xCE, 0x01])); // PPS (8) + d.extend_from_slice(&h264_nal(0x74, &[0x11, 0x22])); // slice-ext (20) + d + }; + let nal_types = + |f: &Frame| -> Vec { h264_nals_in(&f.data).iter().map(|n| n[0] & 0x1F).collect() }; + + // Normal parser strips the PPS from a non-keyframe AU (it is captured for + // the avcC and, without an IDR, never re-asserted in-band). + let mut normal = H264Parser::new(); + let f = normal.parse(&make_pes(au(), Some(90000))); + assert_eq!(f.len(), 1); + assert!( + !nal_types(&f[0]).contains(&8), + "normal parser strips PPS from a non-keyframe AU: {:?}", + nal_types(&f[0]) + ); + + // Passthrough keeps EVERY parameter set in-band, so each dependent frame + // is a self-contained access unit for a BlockAdditional. + let mut pt = H264Parser::new().with_mvc_passthrough(true); + let f = pt.parse(&make_pes(au(), Some(90000))); + assert_eq!(f.len(), 1); + let types = nal_types(&f[0]); + assert!(types.contains(&15), "subset SPS kept in-band: {types:?}"); + assert!( + types.contains(&8), + "PPS kept in-band under passthrough: {types:?}" + ); + assert!(types.contains(&20), "slice kept: {types:?}"); + } + #[test] fn h264_populates_measured_coding_type_and_source() { use super::super::coding::CodingType; diff --git a/src/mux/ebml.rs b/src/mux/ebml.rs index f6de89b..c3a899b 100644 --- a/src/mux/ebml.rs +++ b/src/mux/ebml.rs @@ -1004,6 +1004,31 @@ mod tests { assert_eq!(buf, [0x42, 0x86, 0x81, 0x00]); } + #[test] + fn write_int_minimal_two_complement_width() { + // ReferenceBlock (0xFB) signed offsets, minimal two's-complement width. + let enc = |v: i64| { + let mut b = Vec::new(); + write_int(&mut b, REFERENCE_BLOCK, v).unwrap(); + b + }; + assert_eq!(enc(0), [0xFB, 0x81, 0x00], "0 -> 1 byte 0x00"); + assert_eq!(enc(-1), [0xFB, 0x81, 0xFF], "-1 -> 1 byte 0xFF"); + assert_eq!(enc(127), [0xFB, 0x81, 0x7F], "127 -> 1 byte"); + assert_eq!( + enc(128), + [0xFB, 0x82, 0x00, 0x80], + "128 needs 2 bytes (0x80 alone is -128)" + ); + assert_eq!(enc(-128), [0xFB, 0x81, 0x80], "-128 -> 1 byte 0x80"); + assert_eq!(enc(-129), [0xFB, 0x82, 0xFF, 0x7F], "-129 needs 2 bytes"); + // i64::MIN is the widest: 8 bytes, size 0x88. + let mn = enc(i64::MIN); + assert_eq!(mn[0], 0xFB); + assert_eq!(mn[1], 0x88); + assert_eq!(&mn[2..], &i64::MIN.to_be_bytes()); + } + // ============================================================ // write_float — EBML floats here are always 8-byte IEEE-754 doubles, // big-endian (Matroska SamplingFrequency/Duration). size byte = 0x88. diff --git a/src/mux/mkv.rs b/src/mux/mkv.rs index 5dbbd4c..ac954c2 100644 --- a/src/mux/mkv.rs +++ b/src/mux/mkv.rs @@ -232,10 +232,12 @@ pub struct MkvTrack { /// Blu-ray 3D (MVC): the dependent (right-eye) view's `(subset_sps, pps)` /// NAL units, from which the serializer builds the `mvcC` /// MVCDecoderConfigurationRecord (ISO/IEC 14496-15 §7.6.2) for the track's - /// BlockAdditionMapping, and emits `StereoMode`. `None` for non-3D tracks. - /// Set at muxer activation from the dependent stream's parameter sets — the - /// same deferred path `hdr10`/FieldOrder use — never at construction. When - /// `Some`, the per-frame dependent view rides as a `BlockAdditional`. + /// BlockAdditionMapping. `None` for non-3D tracks. Set at muxer activation + /// from the dependent stream's parameter sets — the same deferred path + /// `hdr10`/FieldOrder use — never at construction. When `Some`, the per-frame + /// dependent view rides as a `BlockAdditional`. (No `StereoMode` is written: + /// RFC 9559 assigns no StereoMode value to MVC-in-BlockAdditional; the mvcC + /// mapping is the 3D signal.) pub mvc_params: Option<(Vec, Vec)>, } @@ -594,6 +596,12 @@ pub struct MkvMuxer { /// the primary video. `None` when the title has no video track (no track /// drives epochs). primary_video_track: Option, + /// Per-track: whether the track declared an `mvcC` BlockAdditionMapping (its + /// `mvc_params` was set at activation). A `BlockAdditional` with BlockAddID=2 + /// is only conforming when the track carries the matching mapping, so + /// `write_frame`'s additional is dropped for a track without it (e.g. the + /// dependent view's parameter sets were never captured before activation). + track_has_mvc_mapping: Vec, /// Cross-clip timeline-continuity corrector (clip-boundary PTS rebasing). continuity: TimelineContinuity, cues: Vec, @@ -1109,6 +1117,7 @@ impl MkvMuxer { primary_video_track: tracks .iter() .position(|t| t.track_type == ebml::TRACK_TYPE_VIDEO), + track_has_mvc_mapping: tracks.iter().map(|t| t.mvc_params.is_some()).collect(), continuity: TimelineContinuity::new(), cues: Vec::new(), frame_count: 0, @@ -1155,6 +1164,13 @@ impl MkvMuxer { /// (`None`) leaves the written value untouched: an interlaced track keeps /// its guess rather than being cleared via a multi-element change. The byte /// width is fixed (FieldOrder is 0..=14), so the in-place rewrite is valid. + /// + /// `block_additional`, when `Some`, is attached to the frame as a Matroska + /// `BlockAdditional` (BlockAddID=2) — Blu-ray 3D (MVC): the base view is the + /// Block and the dependent (right-eye) access unit rides as the + /// BlockAdditional under the track's `mvcC` mapping. Such a frame is always a + /// `BlockGroup` (never a SimpleBlock), with a `ReferenceBlock` when it is not + /// a keyframe. `None` for every non-3D frame. pub fn write_frame( &mut self, track_idx: usize, @@ -1162,42 +1178,6 @@ impl MkvMuxer { keyframe: bool, data: &[u8], duration_ns: Option, - ) -> io::Result<()> { - self.write_frame_inner(track_idx, pts_ns, keyframe, data, duration_ns, None) - } - - /// As [`write_frame`](Self::write_frame), but attaches `block_additional` to - /// the frame as a Matroska `BlockAdditional` (BlockAddID=2). Used for Blu-ray - /// 3D (MVC): the base view is the Block, the dependent (right-eye) access - /// unit rides as the BlockAdditional under the track's `mvcC` mapping. A frame - /// carrying an additional is always written as a `BlockGroup` (never a - /// SimpleBlock), with a `ReferenceBlock` when it is not a keyframe. - pub fn write_frame_with_additional( - &mut self, - track_idx: usize, - pts_ns: i64, - keyframe: bool, - data: &[u8], - duration_ns: Option, - block_additional: Option<&[u8]>, - ) -> io::Result<()> { - self.write_frame_inner( - track_idx, - pts_ns, - keyframe, - data, - duration_ns, - block_additional, - ) - } - - fn write_frame_inner( - &mut self, - track_idx: usize, - pts_ns: i64, - keyframe: bool, - data: &[u8], - duration_ns: Option, block_additional: Option<&[u8]>, ) -> io::Result<()> { // --log-level 3: capture the first ~100 coded frames per track to the @@ -1362,6 +1342,24 @@ impl MkvMuxer { let relative_ts = (pts_ticks - self.cluster_ts_ticks) as i16; let duration_ticks = duration_ns.map(|dur_ns| (dur_ns as i64 / TIMESTAMP_SCALE_NS).max(1) as u64); + // A BlockAdditional with BlockAddID=2 is only conforming when the track + // declared the matching mvcC BlockAdditionMapping. If it did not (the + // dependent view's parameter sets were never captured before the header + // was written), drop the additional and emit a plain block rather than a + // non-conforming file with an orphaned BlockAddID. + let block_additional = match block_additional { + Some(a) + if self + .track_has_mvc_mapping + .get(track_idx) + .copied() + .unwrap_or(false) => + { + Some(a) + } + Some(_) => None, + None => None, + }; match block_additional { // MVC: base view Block + dependent-view BlockAdditional, always a // BlockGroup. Non-keyframe base frames get a ReferenceBlock to the @@ -1372,9 +1370,16 @@ impl MkvMuxer { None } else { // Offset (ticks) of the referenced keyframe relative to this - // block. `None` only if no keyframe preceded (unreachable for a - // real non-keyframe); then the ReferenceBlock is simply omitted. - self.last_video_keyframe_ticks.map(|kf| kf - pts_ticks) + // block. A non-keyframe MUST carry a ReferenceBlock or a reader + // treats it as a seek point; fall back to 0 (self-relative) in + // the pre-first-keyframe corner (unreachable in practice — such + // frames are dropped before a cluster opens) so the marker is + // never absent. + Some( + self.last_video_keyframe_ticks + .map(|kf| kf - pts_ticks) + .unwrap_or(0), + ) }; self.write_block_group_mvc( track_idx + 1, @@ -1395,9 +1400,11 @@ impl MkvMuxer { } }, } - // Remember the last video keyframe's tick so a later non-keyframe MVC - // base frame can reference it (see block_additional path above). - if is_video && keyframe { + // Remember the last PRIMARY-video keyframe's tick so a later non-keyframe + // MVC base frame references a keyframe on its OWN track (see the + // block_additional path above). Gating to the primary video track avoids a + // secondary video track's keyframe becoming a cross-track reference target. + if keyframe && Some(track_idx) == self.primary_video_track { self.last_video_keyframe_ticks = Some(pts_ticks); } self.frame_count += 1; @@ -2026,7 +2033,7 @@ mod tests { let tracks = [make_video_track()]; let mut muxer = MkvMuxer::new(buf, &tracks, None, 60.0, &[]).unwrap(); muxer - .write_frame(0, 0, true, &[0xDE, 0xAD, 0xBE, 0xEF], None) + .write_frame(0, 0, true, &[0xDE, 0xAD, 0xBE, 0xEF], None, None) .unwrap(); let data = muxer.writer.into_inner(); assert!( @@ -2046,7 +2053,7 @@ mod tests { let tracks = [make_video_track()]; let mut muxer = MkvMuxer::new(writer, &tracks, Some("Cue Test"), 60.0, &[]).unwrap(); muxer - .write_frame(0, 0, true, &[0x01, 0x02, 0x03], None) + .write_frame(0, 0, true, &[0x01, 0x02, 0x03], None, None) .unwrap(); muxer.finish().unwrap(); @@ -2072,7 +2079,7 @@ mod tests { let tracks = [make_video_track()]; let mut muxer = MkvMuxer::new(writer, &tracks, Some("NoCue"), 60.0, &[]).unwrap(); muxer - .write_frame(0, 0, true, &[0x01, 0x02, 0x03], None) + .write_frame(0, 0, true, &[0x01, 0x02, 0x03], None, None) .unwrap(); // Force the zero-cue branch: drop every cue before finalizing. let cues_entry_pos = muxer.cues_seek_entry_pos.expect("CUES seek entry recorded"); @@ -2323,16 +2330,16 @@ mod tests { let mut muxer = MkvMuxer::new(buf, &tracks, Some("Multi"), 120.0, &[]).unwrap(); // Write frames to both tracks muxer - .write_frame(0, 0, true, &[0x00, 0x00, 0x01], None) + .write_frame(0, 0, true, &[0x00, 0x00, 0x01], None, None) .unwrap(); muxer - .write_frame(1, 0, false, &[0x0B, 0x77, 0x00], None) + .write_frame(1, 0, false, &[0x0B, 0x77, 0x00], None, None) .unwrap(); muxer - .write_frame(0, 40_000_000, false, &[0x00, 0x00, 0x01], None) + .write_frame(0, 40_000_000, false, &[0x00, 0x00, 0x01], None, None) .unwrap(); muxer - .write_frame(1, 32_000_000, false, &[0x0B, 0x77, 0x01], None) + .write_frame(1, 32_000_000, false, &[0x0B, 0x77, 0x01], None, None) .unwrap(); // Should not panic let data = muxer.writer.into_inner(); @@ -2347,11 +2354,11 @@ mod tests { // Record position before first frame let pos_before_kf = muxer.writer.position(); - muxer.write_frame(0, 0, true, &[0xAA], None).unwrap(); + muxer.write_frame(0, 0, true, &[0xAA], None, None).unwrap(); let pos_after_kf = muxer.writer.position(); muxer - .write_frame(0, 1_000_000, false, &[0xBB], None) + .write_frame(0, 1_000_000, false, &[0xBB], None, None) .unwrap(); let pos_after_nkf = muxer.writer.position(); @@ -2597,7 +2604,7 @@ mod tests { let writer = SharedWriter(shared.clone()); let mut muxer = MkvMuxer::new(writer, tracks, None, 0.0, chapters).unwrap(); for (t, pts, kf, data) in frames { - muxer.write_frame(*t, *pts, *kf, data, None).unwrap(); + muxer.write_frame(*t, *pts, *kf, data, None, None).unwrap(); } let frame_count = muxer.frame_count; muxer.finish().unwrap(); @@ -2959,7 +2966,7 @@ mod tests { let mut muxer = MkvMuxer::new(writer, &tracks, None, 0.0, &[]).unwrap(); for f in &frames { muxer - .write_frame(0, f.pts_ns, f.keyframe, &f.data, f.duration_ns) + .write_frame(0, f.pts_ns, f.keyframe, &f.data, f.duration_ns, None) .unwrap(); } muxer.finish().unwrap(); @@ -3348,12 +3355,14 @@ mod tests { let writer = SharedWriter(shared.clone()); let mut muxer = MkvMuxer::new(writer, &tracks, None, 0.0, &[]).unwrap(); // Audio frames (track 1) and non-keyframe video — no track-0 keyframe. - muxer.write_frame(1, 0, true, &[0xAA; 8], None).unwrap(); muxer - .write_frame(0, 10_000_000, false, &[0xBB; 8], None) + .write_frame(1, 0, true, &[0xAA; 8], None, None) .unwrap(); muxer - .write_frame(1, 20_000_000, true, &[0xCC; 8], None) + .write_frame(0, 10_000_000, false, &[0xBB; 8], None, None) + .unwrap(); + muxer + .write_frame(1, 20_000_000, true, &[0xCC; 8], None, None) .unwrap(); let err = muxer.finish().unwrap_err(); assert_eq!(err.kind(), io::ErrorKind::InvalidData); @@ -3428,7 +3437,7 @@ mod tests { let writer = SharedWriter(shared.clone()); let mut muxer = MkvMuxer::new(writer, &tracks, None, 0.0, &[]).unwrap(); for (t, pts, kf, data) in &frames_in_order { - muxer.write_frame(*t, *pts, *kf, data, None).unwrap(); + muxer.write_frame(*t, *pts, *kf, data, None, None).unwrap(); } muxer.finish().unwrap(); let data = shared.lock().unwrap().clone().into_inner(); @@ -3498,7 +3507,7 @@ mod tests { let writer = SharedWriter(shared.clone()); let mut muxer = MkvMuxer::new(writer, tracks, None, 0.0, &[]).unwrap(); for (t, pts, kf, data, dur) in frames { - muxer.write_frame(*t, *pts, *kf, data, *dur).unwrap(); + muxer.write_frame(*t, *pts, *kf, data, *dur, None).unwrap(); } muxer.finish().unwrap(); shared.lock().unwrap().clone().into_inner() @@ -4347,10 +4356,10 @@ mod tests { let mut muxer = MkvMuxer::new(writer, &tracks, None, 10.0, &[]).unwrap(); // Video keyframe 1000 bytes; audio frame 500 bytes. muxer - .write_frame(0, 0, true, &vec![0xABu8; 1000], None) + .write_frame(0, 0, true, &vec![0xABu8; 1000], None, None) .unwrap(); muxer - .write_frame(1, 0, false, &vec![0xCDu8; 500], None) + .write_frame(1, 0, false, &vec![0xCDu8; 500], None, None) .unwrap(); muxer.finish().unwrap(); let data = shared.lock().unwrap().clone().into_inner(); @@ -4400,8 +4409,10 @@ mod tests { // lfeon(0) = 0b0100_0000 = 0x40. acmod_channels only needs >= 8 bytes. let ac3 = vec![0x0B, 0x77, 0x00, 0x00, 0x00, 8 << 3, 0x40, 0x00]; // Open a cluster with a video keyframe first (cluster invariant). - muxer.write_frame(0, 0, true, &[0x01, 0x02], None).unwrap(); - muxer.write_frame(1, 0, false, &ac3, None).unwrap(); + muxer + .write_frame(0, 0, true, &[0x01, 0x02], None, None) + .unwrap(); + muxer.write_frame(1, 0, false, &ac3, None, None).unwrap(); muxer.finish().unwrap(); let data = shared.lock().unwrap().clone().into_inner(); @@ -4477,6 +4488,79 @@ mod tests { // Guards: too-short SPS and empty PPS both refuse (no corrupt record). assert!(mvc_decoder_config_record(&[0x6F, 0x80, 0x00], &pps).is_none()); assert!(mvc_decoder_config_record(&subset_sps, &[]).is_none()); + // Over-length param sets (> 65535) would mis-frame the 16-bit length + // field; both must refuse rather than emit a truncated record. + let huge = vec![0u8; 0x1_0000]; + assert!(mvc_decoder_config_record(&huge, &pps).is_none()); + assert!(mvc_decoder_config_record(&subset_sps, &huge).is_none()); + } + + #[test] + fn mvc_frame_emits_blockgroup_additional_and_reference() { + // A track declaring an mvcC mapping: a keyframe base frame carrying a + // dependent AU emits BlockGroup > BlockAdditions > BlockMore + // {BlockAddID=2, BlockAdditional=dep}; a following non-keyframe adds a + // ReferenceBlock so it is not mistaken for a seek point. + let mut v = make_video_track(); + v.mvc_params = Some(( + vec![0x6F, 0x80, 0x00, 0x33, 0x11, 0x22], + vec![0x68, 0xEE, 0x3C], + )); + let mut muxer = MkvMuxer::new(Cursor::new(Vec::new()), &[v], None, 0.0, &[]).unwrap(); + let dep_kf = [0xDEu8, 0xAD, 0xBE, 0xEF]; + let dep_p = [0xCAu8, 0xFE]; + muxer + .write_frame(0, 0, true, &[0x65, 0x01, 0x02], None, Some(&dep_kf)) + .unwrap(); + muxer + .write_frame(0, 40_000_000, false, &[0x41, 0x03], None, Some(&dep_p)) + .unwrap(); + let data = muxer.writer.into_inner(); + + assert!( + find_id(&data, ebml::BLOCK_ADDITIONS).is_some(), + "BlockAdditions (0x75A1) present" + ); + // BlockAddID = 2 (uint): element 0xEE, size 0x81, value 0x02. + assert!( + data.windows(3).any(|w| w == [0xEE, 0x81, 0x02]), + "BlockAddID must be 2" + ); + assert!( + data.windows(4).any(|w| w == dep_kf), + "keyframe dependent AU present as BlockAdditional" + ); + assert!( + data.windows(2).any(|w| w == dep_p), + "non-keyframe dependent AU present" + ); + assert!( + find_id(&data, ebml::REFERENCE_BLOCK).is_some(), + "non-keyframe MVC base frame must carry a ReferenceBlock" + ); + } + + #[test] + fn additional_dropped_when_track_has_no_mvc_mapping() { + // If a track did NOT declare an mvcC mapping (mvc_params None), a stray + // block_additional must be dropped (no orphaned BlockAddID=2 / no + // BlockAdditions) so the file stays conforming. + let mut muxer = MkvMuxer::new( + Cursor::new(Vec::new()), + &[make_video_track()], + None, + 0.0, + &[], + ) + .unwrap(); + muxer + .write_frame(0, 0, true, &[0x65, 0x01], None, Some(&[0xDE, 0xAD])) + .unwrap(); + let data = muxer.writer.into_inner(); + assert!( + find_id(&data, ebml::BLOCK_ADDITIONS).is_none(), + "no BlockAdditions without a declared mvcC mapping" + ); } #[test] diff --git a/src/mux/mkvstream.rs b/src/mux/mkvstream.rs index 955dae0..fb02db9 100644 --- a/src/mux/mkvstream.rs +++ b/src/mux/mkvstream.rs @@ -198,13 +198,25 @@ impl MvcMerge { { pb.additional = Some(frame.data.clone()); } else { - self.dep_by_pts.insert(frame.pts, frame.data.clone()); - // Bound the orphan map: if dependents pile up unpaired (pairing - // badly drifted), drop the surplus rather than grow unbounded. - if self.dep_by_pts.len() > MVC_PAIR_WINDOW * 4 { + // Bound the orphan map BEFORE inserting: if dependents pile up + // unpaired (pairing badly drifted), drop the drifted buffer so it + // stays bounded — but keep THIS just-arrived dependent, whose base + // frame commonly arrives next. Clearing after the insert would + // discard it and overcount orphans by one. + if self.dep_by_pts.len() >= MVC_PAIR_WINDOW * 4 { self.orphan_deps += self.dep_by_pts.len() as u64; self.dep_by_pts.clear(); } + // A duplicate-PTS dependent (e.g. a stale repeat after a stream + // discontinuity) displaces the prior one — count it as an orphan + // rather than losing it silently. + if self + .dep_by_pts + .insert(frame.pts, frame.data.clone()) + .is_some() + { + self.orphan_deps += 1; + } } } else if frame.track == self.base_stream_idx { let additional = self.dep_by_pts.remove(&frame.pts); @@ -257,30 +269,22 @@ impl MvcMerge { } } -/// Hand a frame to the muxer: the plain path (`write_frame`) when there is no -/// MVC dependent view to attach, the BlockAdditional path otherwise. +/// Hand a frame to the muxer, attaching the MVC dependent view as a +/// `BlockAdditional` when `additional` is `Some` (a 3D base frame), else a +/// plain block. fn emit_to_muxer( m: &mut MkvMuxer>, frame: &crate::pes::PesFrame, additional: Option<&[u8]>, ) -> io::Result<()> { - match additional { - None => m.write_frame( - frame.track, - frame.pts, - frame.keyframe, - &frame.data, - frame.duration_ns, - ), - Some(_) => m.write_frame_with_additional( - frame.track, - frame.pts, - frame.keyframe, - &frame.data, - frame.duration_ns, - additional, - ), - } + m.write_frame( + frame.track, + frame.pts, + frame.keyframe, + &frame.data, + frame.duration_ns, + additional, + ) } /// Scan a length-prefixed (4-byte big-endian) H.264 NAL stream for the first @@ -336,10 +340,18 @@ impl MkvStream { .streams .iter() .position(|s| matches!(s, crate::disc::Stream::Video(v) if v.is_mvc_dependent())); + // The base is the first NON-dependent video. Excluding the dependent here + // means a (malformed / hand-built) title whose only video IS the dependent + // yields `base_stream_idx == None` → no merge (the dependent is muxed as an + // ordinary track) instead of `base == dep` and a panic on the skipped slot. let base_stream_idx = title .streams .iter() - .position(|s| matches!(s, crate::disc::Stream::Video(_))); + .position(|s| matches!(s, crate::disc::Stream::Video(v) if !v.is_mvc_dependent())); + // The merge is only active when BOTH a dependent and a distinct base exist; + // only then is the dependent's track skipped/folded. + let mvc_active = dep_stream_idx.is_some() && base_stream_idx.is_some(); + let skip_stream_idx = if mvc_active { dep_stream_idx } else { None }; let mut tracks = Vec::new(); let mut has_default_video = false; @@ -348,7 +360,7 @@ impl MkvStream { // which has no track). Streams after the dependent shift down by one. let mut stream_to_track: Vec> = Vec::with_capacity(title.streams.len()); for (idx, s) in title.streams.iter().enumerate() { - if Some(idx) == dep_stream_idx { + if Some(idx) == skip_stream_idx { stream_to_track.push(None); continue; } @@ -372,13 +384,15 @@ impl MkvStream { tracks.push(track); } - // Assemble the MVC merge when a dependent view is present and paired with - // a base video track. - let mvc = match (dep_stream_idx, base_stream_idx) { - (Some(dep_stream_idx), Some(base_stream_idx)) => { - let base_track_idx = - stream_to_track[base_stream_idx].expect("base video stream always has a track"); - Some(MvcMerge { + // Assemble the MVC merge only when active — i.e. a dependent AND a + // distinct base video both exist (established above). `base_stream_idx` + // then always has a built track, so its remap is `Some` (no panic path). + let mvc = match (mvc_active, dep_stream_idx, base_stream_idx) { + (true, Some(dep_stream_idx), Some(base_stream_idx)) => stream_to_track + .get(base_stream_idx) + .copied() + .flatten() + .map(|base_track_idx| MvcMerge { base_stream_idx, dep_stream_idx, base_track_idx, @@ -387,8 +401,7 @@ impl MkvStream { dep_by_pts: std::collections::HashMap::new(), captured_params: None, orphan_deps: 0, - }) - } + }), _ => None, }; @@ -466,7 +479,7 @@ impl MkvStream { muxer.set_opening_capture(crate::diag::OpeningCapture::new(path, pending.tracks.len())); } for (f, additional) in pending.buffered.drain(..) { - muxer.write_frame_with_additional( + muxer.write_frame( f.track, f.pts, f.keyframe, @@ -1294,6 +1307,99 @@ mod tests { assert!(tail[0].1.is_none(), "no BlockAdditional when unpaired"); } + #[test] + fn extract_mvc_params_no_panic_on_truncated_or_empty() { + // Empty, sub-header, zero-length NAL, and a length prefix claiming more + // than is present must all return None without panicking (untrusted AU). + assert!(extract_mvc_params(&[]).is_none()); + assert!(extract_mvc_params(&[0, 0, 0]).is_none()); + assert!( + extract_mvc_params(&[0, 0, 0, 0]).is_none(), + "zero-length NAL breaks" + ); + assert!( + extract_mvc_params(&[0, 0, 0, 10, 0x6F]).is_none(), + "length prefix past end breaks, no slice panic" + ); + } + + #[test] + fn mvc_merge_flushes_oldest_base_once_past_window() { + let mut m = empty_merge(); + // Push more unpaired base frames than the window; the excess flush as + // plain (unpaired) blocks in FIFO order once len exceeds MVC_PAIR_WINDOW. + let n = MVC_PAIR_WINDOW + 8; + let mut emitted = 0usize; + for pts in 0..n { + emitted += m + .ingest(&mvc_frame(0, pts as i64, false, vec![0, 0, 0, 1])) + .len(); + } + assert_eq!(emitted, 8, "the {n} bases beyond the window flush unpaired"); + assert_eq!(m.pending_base.len(), MVC_PAIR_WINDOW, "window still held"); + assert!(m.flush().iter().all(|(_, add)| add.is_none())); + } + + #[test] + fn mvc_merge_dep_overflow_drops_old_keeps_newest() { + let mut m = empty_merge(); + // Fill dep_by_pts to the bound with unpaired dependents (unique PTS). + for pts in 0..(MVC_PAIR_WINDOW * 4) { + assert!( + m.ingest(&mvc_frame(2, pts as i64, false, lp(&[&DEP_SLICE]))) + .is_empty() + ); + } + assert_eq!(m.dep_by_pts.len(), MVC_PAIR_WINDOW * 4); + // One more overflows: the drifted buffer is cleared BUT the newest survives + // so its (soon-to-arrive) base can still pair. + let dep_new = lp(&[&DEP_SLICE]); + m.ingest(&mvc_frame(2, 9_999, false, dep_new.clone())); + assert_eq!(m.dep_by_pts.len(), 1, "old cleared, newest kept"); + assert!(m.dep_by_pts.contains_key(&9_999)); + assert_eq!( + m.orphan_deps, + (MVC_PAIR_WINDOW * 4) as u64, + "old buffer counted once" + ); + // The surviving dependent pairs with its base. + let e = m.ingest(&mvc_frame(0, 9_999, false, vec![0x61, 1])); + assert_eq!(e.len(), 1); + assert_eq!(e[0].1.as_deref(), Some(dep_new.as_slice())); + } + + #[test] + fn create_does_not_panic_when_only_video_is_mvc_dependent() { + // A (malformed / hand-built) title whose single video IS the dependent + // must NOT panic: base_stream_idx is None, so no merge is set up and the + // dependent is muxed as an ordinary track. + use crate::disc::{ + Codec, ColorSpace, DiscTitle, FrameRate, HdrFormat, Resolution, Stream, VideoStream, + }; + let dep = VideoStream { + pid: 0x1012, + codec: Codec::H264, + resolution: Resolution::R1080p, + frame_rate: FrameRate::F24, + hdr: HdrFormat::Sdr, + color_space: ColorSpace::Bt709, + display_aspect: None, + secondary: true, + label: crate::disc::MVC_DEPENDENT_LABEL.to_string(), + measured_cicp: None, + }; + let title = DiscTitle { + streams: vec![Stream::Video(dep)], + ..DiscTitle::empty() + }; + let s = MkvStream::create(Box::new(Cursor::new(Vec::new())), &title) + .expect("create must succeed, not panic"); + assert!( + s.mvc.is_none(), + "no merge when there is no distinct base view" + ); + } + #[test] fn apply_coding_to_track_sets_measured_field_order_never_guesses() { use crate::disc::{Codec, ColorSpace, FrameRate, HdrFormat, Resolution, VideoStream};