collective
Browse / github.com/storytold/filmcraft / patch
matroska: preserve PTS semantics in the lacing oracle
matroska: preserve PTS semantics in the lacing oracle `filmcraft-matroska`'s `laced_remux` fails on unchanged `main` at `bfc23840` with FFmpeg/ffprobe 9.0.2: `vorbis.mkv Xiph: nothing was laced`. The generated Vorbis fixture declares a 1,333,333 ns `DefaultDuration` while its stored packet timestamps advance by 21 or 22 ms. The pinned writer correctly preserves those timestamps by declining to lace packets whose timing that duration cannot describe; the oracle incorrectly requires a lace in this case. This is an oracle repair, with no muxer or dependency changes. The positive matrix still requires actual Xiph/EBML lacing on AAC and FLAC, and Fix
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- KeroZelvin (imported from pull request #3225, open)
- Base
70375a3ee8894343080e1f77d80139d0b1510738- Size
- +125 −42 in 2 files
- Review
- pending
- Shared
- 2026-10-28
- Id
d7f3c7aff08a56e23976593c
How it went
No other operator has reported using it yet.
Diff
raw · crates/matroska/README.md crates/matroska/tests/oracle.rs
@@ -1,3 +1,19 @@ # filmcraft-matroska A re-export of `craft_media::matroska`. Documentation: https://github.com/storytold/craft-libs/blob/main/crates/media/README.md++## Oracle coverage++`tests/oracle.rs` requires actual Xiph/EBML lacing on AAC and FLAC fixtures, and+FixedOrEbml lacing on PCM. The Vorbis remux regression supplies a deliberately+incompatible declared duration and requires the writer to keep every packet's+original timestamp, payload, keyframe flag and codec headers without lacing.+This avoids assuming that every encoder's `DefaultDuration` permits lacing.+All remux cases are checked independently with ffprobe; assertions are not skipped+when requested lacing cannot preserve timestamps.+The encoded Block flags must match the requested mode: Xiph, EBML, or either+fixed-size or EBML for FixedOrEbml; unlaced fallback blocks remain valid.++Laced frames share a block timestamp and must be contiguous; see+[RFC 9559 section 10.3.5](https://www.rfc-editor.org/rfc/rfc9559.html#section-10.3.5).+The muxer implementation and media dependencies are unchanged by this oracle repair.@@ -266,22 +266,104 @@ fn mjpeg_ac3() { assert!(f.tags.iter().flat_map(|t| &t.simple).any(|s| s.name == "ENCODER")); } +/// Compare AV1 packets and track metadata with ffprobe, including the av1C configuration record. #[test] fn av1() { let Some((_, f, _)) = run("av1.mkv") else { return }; assert!(matches!(&f.tracks[0].codec, Codec::Av1 { av1c } if av1c.first() == Some(&0x81))); } -/// Re-mux a fixture with our writer using each lacing mode; ffprobe must read the result identically-/// to our demuxer (and to the source packets).+/// Read the owning Block's lacing bits, not the sample's index within its lace (RFC 9559 §10.3).+fn block_lacing_flags(bytes: &[u8], sample: &Sample) -> u8 {+ use element_ids::{BLOCK, BLOCK_GROUP, SIMPLE_BLOCK};++ let offset = usize::try_from(sample.block_offset).unwrap();+ let element = ebml::Children::new(&bytes[offset..]).next().expect("sample's block element");+ assert!(!element.truncated);+ let data = match element.id {+ SIMPLE_BLOCK => element.data,+ BLOCK_GROUP => ebml::Children::new(element.data).find(|e| e.id == BLOCK).expect("Block in BlockGroup").data,+ id => panic!("unexpected sample element {id:#x}"),+ };+ let (_, track_number_len, _) = ebml::read_vint(data).expect("block's track number");+ // Track-number VINT, signed 16-bit relative timestamp, then the flags byte.+ data[track_number_len + 2] & 0x06+}++/// Remux without changing packet timestamps or payloads, then check the output independently+/// with ffprobe. Requesting a lacing mode does not guarantee a lace: the supplied duration must+/// also reproduce the packet timestamps (RFC 9559 section 10.3.5).+fn remux_and_compare(ffprobe: &Path, file: &MkvFile, packets: &[Packet], mode: LacingMode, label: &str, expect_laced: bool) {+ let specs: Vec<TrackSpec> = file+ .tracks+ .iter()+ .enumerate()+ .map(|(ti, t)| {+ let mut s = TrackSpec::new(t.kind, t.codec_id.clone());+ s.codec_private = t.codec_private.clone();+ // Preserve a declared duration; only supply one when the input has none.+ s.default_duration_ns = t.default_duration_ns.or_else(|| {+ let pts: Vec<i64> = packets.iter().filter(|p| p.track == ti).map(|p| p.pts_ns).collect();+ (t.kind == TrackKind::Audio && pts.len() > 1).then(|| ((pts[pts.len() - 1] - pts[0]) / (pts.len() as i64 - 1)) as u64)+ });+ s.video_size = t.video.as_ref().map(|v| (v.pixel_width, v.pixel_height));+ s.audio = t.audio.as_ref().map(|a| (a.sampling_frequency, a.channels, a.bit_depth));+ s.codec_delay_ns = t.codec_delay_ns;+ s+ })+ .collect();+ let opts = MuxOptions { lacing: mode, ..Default::default() };+ let mut w = MkvWriter::new(std::io::Cursor::new(Vec::new()), specs, opts).unwrap();+ for p in packets {+ w.write_frame(p.track, p.pts_ns, p.keyframe, &p.data, None).unwrap();+ }+ let out = w.finish().unwrap().into_inner();+ let laced = {+ let f = open(&out[..]).unwrap();+ // The first sample visits each block once, including blocks that did not lace.+ for sample in f.tracks.iter().flat_map(|t| &t.samples).filter(|s| s.lace == 0) {+ let flags = block_lacing_flags(&out, sample);+ if flags != 0 {+ assert!(expect_laced, "{label} {mode:?}: unexpected laced block at {}", sample.block_offset);+ let matches_mode = match mode {+ LacingMode::None => false,+ LacingMode::Xiph => flags == 0x02,+ LacingMode::Ebml => flags == 0x06,+ LacingMode::FixedOrEbml => matches!(flags, 0x04 | 0x06),+ };+ assert!(matches_mode, "{label} {mode:?}: wrong block lacing flags {flags:#04x} at {}", sample.block_offset);+ }+ }+ f.tracks.iter().any(|t| t.samples.iter().any(|s| s.lace > 0))+ };+ assert_eq!(laced, expect_laced, "{label} {mode:?}: unexpected lacing");+ let outp = fixture_dir().join(format!("remux_{mode:?}_{label}"));+ std::fs::write(&outp, &out).unwrap();+ let (got_file, got) = compare(ffprobe, &outp);+ assert_eq!(got_file.tracks.len(), file.tracks.len());+ for (a, b) in got_file.tracks.iter().zip(&file.tracks) {+ assert_eq!((&a.codec_id, &a.codec_private, a.codec_delay_ns), (&b.codec_id, &b.codec_private, b.codec_delay_ns), "{label} {mode:?}: codec headers");+ }+ assert_eq!(got.len(), packets.len());+ for (a, b) in got.iter().zip(packets) {+ assert_eq!(a.data, b.data, "{label} {mode:?}: payload");+ assert_eq!((a.track, a.pts, a.keyframe), (b.track, b.pts, b.keyframe), "{label} {mode:?}: timestamp/keyframe");+ if !expect_laced {+ assert_eq!(a.pts_ns, b.pts_ns, "{label} {mode:?}: explicit timestamp precision");+ }+ }+ std::fs::remove_file(&outp).unwrap();+}++/// Exercise each lacing mode on packets that can actually share a lace. Vorbis's encoder-+/// supplied DefaultDuration need not match its packet spacing; that fallback is checked below. #[test] fn laced_remux() { let Some((ffmpeg, ffprobe)) = tools() else { return }; for (src, modes) in [- ("vorbis.mkv", &[LacingMode::Xiph, LacingMode::Ebml][..]), ("h264_aac.mkv", &[LacingMode::Ebml, LacingMode::Xiph][..]), ("prores_pcm.mkv", &[LacingMode::FixedOrEbml][..]),- ("flac.mkv", &[LacingMode::Ebml][..]),+ ("flac.mkv", &[LacingMode::Ebml, LacingMode::Xiph][..]), ] { let Some(path) = fixture(&ffmpeg, src) else { continue }; let bytes = std::fs::read(&path).unwrap();@@ -289,48 +371,33 @@ fn laced_remux() { let file = d.file().clone(); let packets: Vec<Packet> = d.collect::<Result<_>>().unwrap(); for &mode in modes {- let specs: Vec<TrackSpec> = file- .tracks- .iter()- .enumerate()- .map(|(ti, t)| {- let mut s = TrackSpec::new(t.kind, t.codec_id.clone());- s.codec_private = t.codec_private.clone();- // audio without a DefaultDuration gets its mean packet spacing, so it can lace- s.default_duration_ns = t.default_duration_ns.or_else(|| {- let pts: Vec<i64> = packets.iter().filter(|p| p.track == ti).map(|p| p.pts_ns).collect();- (t.kind == TrackKind::Audio && pts.len() > 1).then(|| ((pts[pts.len() - 1] - pts[0]) / (pts.len() as i64 - 1)) as u64)- });- s.video_size = t.video.as_ref().map(|v| (v.pixel_width, v.pixel_height));- s.audio = t.audio.as_ref().map(|a| (a.sampling_frequency, a.channels, a.bit_depth));- s.codec_delay_ns = t.codec_delay_ns;- s- })- .collect();- let opts = MuxOptions { lacing: mode, ..Default::default() };- let mut w = MkvWriter::new(std::io::Cursor::new(Vec::new()), specs, opts).unwrap();- for p in &packets {- w.write_frame(p.track, p.pts_ns, p.keyframe, &p.data, None).unwrap();- }- let out = w.finish().unwrap().into_inner();- let laced = {- let f = open(&out[..]).unwrap();- f.tracks.iter().any(|t| t.samples.iter().any(|s| s.lace > 0))- };- assert!(laced, "{src} {mode:?}: nothing was laced");- let outp = fixture_dir().join(format!("remux_{mode:?}_{src}"));- std::fs::write(&outp, &out).unwrap();- let (_, got) = compare(&ffprobe, &outp);- assert_eq!(got.len(), packets.len());- for (a, b) in got.iter().zip(&packets) {- assert_eq!(a.data, b.data, "{src} {mode:?}: payload");- assert_eq!((a.track, a.pts), (b.track, b.pts), "{src} {mode:?}: timestamp");- }- std::fs::remove_file(&outp).unwrap();+ remux_and_compare(&ffprobe, &file, &packets, mode, src, true); } } } +/// A mode request must not force lacing when DefaultDuration cannot describe the existing PTS.+/// In particular, some FFmpeg Vorbis fixtures declare a much shorter duration than their packet+/// spacing. Make that mismatch deterministic without changing any source packet's timestamp.+#[test]+fn vorbis_lacing_preserves_pts_with_mismatched_duration() {+ let Some((ffmpeg, ffprobe)) = tools() else { return };+ let Some(path) = fixture(&ffmpeg, "vorbis.mkv") else { return };+ let bytes = std::fs::read(path).unwrap();+ let d = Demuxer::from_slice(&bytes).unwrap();+ let mut file = d.file().clone();+ let packets: Vec<Packet> = d.collect::<Result<_>>().unwrap();+ assert_eq!(file.tracks.len(), 1);+ assert_eq!(file.tracks[0].kind, TrackKind::Audio);+ assert!(packets.len() > 1 && packets.windows(2).all(|w| w[1].pts > w[0].pts), "fixture must have distinct increasing timestamps");+ // One nanosecond cannot advance a frame by even one of this file's timestamp ticks.+ // This remains a mismatch regardless of the encoder's own DefaultDuration choice.+ file.tracks[0].default_duration_ns = Some(1);+ for mode in [LacingMode::Xiph, LacingMode::Ebml] {+ remux_and_compare(&ffprobe, &file, &packets, mode, "mismatched_vorbis.mkv", false);+ }+}+ /// Damaged file: overwrite bytes in the middle of the clusters; the demuxer must not fail and must /// resync on a later cluster. #[test]