From 5b0976859f603d91c055ae51e9d36b533239cc66 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Wed, 24 Jun 2026 17:03:28 -0700 Subject: [PATCH] =?UTF-8?q?libfreemkv:=20rc.5.2=20SOTL=20video=20=E2=80=94?= =?UTF-8?q?=20full=20Windows=20fps,=20opening-GOP=20proof,=20self-sufficie?= =?UTF-8?q?nt=20log-level=203?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three Silence-of-the-Lambs (R2 PAL SD-DVD) follow-ups for rc.5.2. SUB-TASK 1 — Windows Explorer showed 12.5 fps (half) for the 576i25 track. Root cause: the DefaultDecodedFieldDuration (20 ms field) element rc.5.1 added to "fix" Windows fps did the opposite. With FlagInterlaced=1 + DefaultDuration=40 ms + DefaultDecodedFieldDuration=20 ms, Explorer halved to 12.5 fps and MediaInfo flipped to VFR. MakeMKV's correct rip omits the field-duration element, keeps FlagInterlaced=1 + FieldOrder=TFF + full-frame DefaultDuration (40 ms), and Explorer shows 25 fps / MediaInfo CFR. Fix: MkvTrack::video now passes field_duration_ns == 0 so the element is no longer written; the 1/DefaultDuration = 25 fps signal (the only one tools trust) is the full-frame value. Interlace signalling (FlagInterlaced, FieldOrder=TFF) is retained — MediaInfo reads scan type from the MPEG-2 ES picture coding extension, so it still reports Interlaced / Top Field First. Tests pin the new TrackEntry elements (element present/absent + values). SUB-TASK 2 — opening "menu"/still-frame video. Traced the MPEG-2 opening-GOP path; the wrong/last seq header and PTS-floor-to-0 hypotheses are RULED OUT with file:line evidence: codecPrivate is the FIRST sequence header (read once at headers-ready, mkvstream.rs:115 + pipelined_stream.rs:289), DVD VOBU structure guarantees each title opens on seq header + I-frame (no mid-GOP open), the parser back-anchors leading still-frames to the disc's real timeline (mpeg2.rs:296-303), and the muxer anchors base on the opening keyframe's real PTS so the t=0 floor (mkv.rs:963) never corrupts it. Regression tests pin all three (parser + muxer level). SUB-TASK 3 — make --log-level 3 self-sufficient (diag.rs + minimal hooks). (a) dump the ACTUAL MKV TrackEntry elements written per track (tag=mkv.track: FlagInterlaced, FieldOrder, DefaultDuration, field duration, Display dims, codecPrivate hex) so Windows-fps-class metadata is verifiable from a log alone. (b) capture the first ~100 coded frames per track (raw) to .opening.bin with a per-frame summary line (tag=mkv.opening.frame: track, key/delta, size, PTS) so opening-GOP/menu issues are diagnosable from a future log without the disc. Both gated to log-level 3; normal runs open no side file and record nothing. CI gate (Rust 1.86): fmt --check, clippy -D warnings, and test --tests all green. --- CHANGELOG.md | 45 ++++++++ src/diag.rs | 237 +++++++++++++++++++++++++++++++++++++++++ src/mux/codec/mpeg2.rs | 55 ++++++++++ src/mux/mkv.rs | 202 ++++++++++++++++++++++++++--------- src/mux/mkvstream.rs | 29 ++++- src/mux/resolve.rs | 2 +- 6 files changed, 519 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e7da885..8cdd1d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,50 @@ # Changelog +## [1.0.0-rc.5.2] — UNRELEASED + +### Fixed + +- **Windows Explorer now reports the full 25 fps for interlaced SD-DVD.** + rc.5.1 added a `DefaultDecodedFieldDuration` (20 ms field) element to the + 576i/480i track header on the theory that Windows derives fps from it. The + captured Silence-of-the-Lambs evidence proved the opposite: with + `FlagInterlaced=1` + `DefaultDuration=40 ms` + `DefaultDecodedFieldDuration=20 ms`, + Windows Explorer reported 12.5 fps (half) and MediaInfo flipped the track to + "Frame rate mode: Variable". MakeMKV's correct rip of the same disc OMITS + `DefaultDecodedFieldDuration`, keeps `FlagInterlaced=1` + `FieldOrder=TFF` + + full-frame `DefaultDuration` (40 ms), and Explorer shows the full 25 fps with + MediaInfo "Constant". The element is no longer written (`MkvTrack::video` now + passes `field_duration_ns == 0`); the only frame-rate signal tools trust, + `1/DefaultDuration` = 25 fps, is the full-frame value. Interlace signalling + (`FlagInterlaced=1`, `FieldOrder=TFF`) is retained, and MediaInfo still + reports "Interlaced / Top Field First" because it reads scan type from the + MPEG-2 elementary stream's picture coding extension, not the container flag. + +### Added + +- **`--log-level 3` is now self-sufficient for MKV/opening-frame diagnosis.** + The diagnostic pass now (a) dumps the ACTUAL MKV `TrackEntry` elements written + per track (`tag=mkv.track`: FlagInterlaced, FieldOrder, DefaultDuration, + DefaultDecodedFieldDuration via field-duration, Display dims, codecPrivate as + hex) so the Windows-fps-class metadata is verifiable from a log alone, and + (b) captures the first ~100 coded frames per track (raw bytes) to a + `.opening.bin` side file with a per-frame summary line + (`tag=mkv.opening.frame`: track, key/delta, size, PTS) so opening-GOP / menu + issues are diagnosable from a future log without the disc. Both are gated to + log-level 3; a normal run opens no side file and records nothing. + +### Verified + +- **DVD opening-GOP / still-frame open handling is correct (no change needed).** + The hypothesis that the opening pictures get the wrong (last-seen) sequence + header or have their PTS floored to t=0 was traced and ruled out: the + codecPrivate is the FIRST sequence header (read once at headers-ready, before + any later AU), DVD VOBU structure guarantees each title opens on a sequence + header + I-frame (no mid-GOP open), the parser back-anchors leading + still-frames to the disc's real timeline, and the muxer anchors its timestamp + base on the opening keyframe's real PTS so the t=0 floor never corrupts it. + Regression tests pin all three. + ## [1.0.0-rc.5.1] ### Fixed diff --git a/src/diag.rs b/src/diag.rs index a7ae712..0848e4a 100644 --- a/src/diag.rs +++ b/src/diag.rs @@ -272,6 +272,197 @@ pub fn dump_dvd_substream_probe(title_id: u16, probed: &std::collections::BTreeM } } +// ── MKV TrackEntry dump (the ACTUAL container elements written) ────────────── + +/// `true` when the `--log-level 3` diagnostic target is enabled. Hot-path +/// callers (the opening-frame capture) check this once and skip all work when +/// off, so a normal run pays nothing. +pub fn diag_enabled() -> bool { + tracing::enabled!(target: DIAG, tracing::Level::DEBUG) +} + +/// Cap on the number of codecPrivate bytes rendered to hex in a `tag=mkv.track` +/// line. The sequence header / avcC / hvcC prefix that matters for diagnosis +/// (resolution, frame rate, profile) is at the front; a multi-KB blob past this +/// is summarised as `..(+NB)` rather than flooding the log. +const CODEC_PRIVATE_HEX_CAP: usize = 64; + +/// Render a track's codecPrivate as an uppercase-hex string for the diagnostic +/// line, capped at [`CODEC_PRIVATE_HEX_CAP`] bytes (`..(+NB)` suffix beyond). +/// `None` / empty → `"none"`. Pure (no logging) so it is directly unit-testable. +fn codec_private_hex(cp: Option<&[u8]>) -> String { + match cp { + Some(b) if !b.is_empty() => { + use std::fmt::Write; + let shown = b.len().min(CODEC_PRIVATE_HEX_CAP); + let mut s = String::with_capacity(shown * 2 + 8); + for byte in &b[..shown] { + let _ = write!(s, "{byte:02X}"); + } + if b.len() > CODEC_PRIVATE_HEX_CAP { + let _ = write!(s, "..(+{}B)", b.len() - CODEC_PRIVATE_HEX_CAP); + } + s + } + _ => "none".to_string(), + } +} + +/// Frame the raw bytes of one captured opening frame for the `.opening.bin` side +/// file: `[track:u8][keyframe:u8][pts_ns:i64 LE][len:u32 LE][raw bytes]`. Pure +/// (no I/O) so the record layout is directly unit-testable; `record` appends the +/// returned bytes to the side file. +fn frame_record(track_idx: usize, pts_ns: i64, keyframe: bool, data: &[u8]) -> Vec { + let mut rec = Vec::with_capacity(14 + data.len()); + rec.push(track_idx as u8); + rec.push(keyframe as u8); + rec.extend_from_slice(&pts_ns.to_le_bytes()); + rec.extend_from_slice(&(data.len() as u32).to_le_bytes()); + rec.extend_from_slice(data); + rec +} + +/// Emit the MKV `TrackEntry` elements the muxer is about to WRITE for one +/// track — the Windows-fps-class metadata (FlagInterlaced, FieldOrder, +/// DefaultDuration, DefaultDecodedFieldDuration, Display dims) plus the +/// codecPrivate as hex. With this row a bug log alone is enough to verify why +/// Windows Explorer reports a given frame rate for an interlaced SD track: the +/// container values that drive its fps derivation are all present, no disc and +/// no MediaInfo needed. +/// +/// `track_number` is the 1-based MKV track number; `track` is the built +/// [`crate::mux::mkv::MkvTrack`] whose fields map one-to-one onto the emitted +/// elements (see `MkvMuxer::new`). No-op unless the diag target is on. +pub fn dump_mkv_track(track_number: u64, track: &crate::mux::mkv::MkvTrack) { + if !diag_enabled() { + return; + } + // codecPrivate as hex (capped so a multi-KB hvcC doesn't flood the log; the + // sequence header / avcC prefix that matters for diagnosis is at the front). + let cp = codec_private_hex(track.codec_private.as_deref()); + let field_order = match track.field_order { + crate::mux::ebml::FIELD_ORDER_TFF => "TFF", + crate::mux::ebml::FIELD_ORDER_BFF => "BFF", + _ => "—", + }; + // FlagInterlaced is only written for video tracks (1=interlaced/2=progressive); + // report what the muxer will emit, or "—" for non-video tracks where the + // element is omitted entirely. + let interlaced = if track.track_type == crate::mux::ebml::TRACK_TYPE_VIDEO { + if track.interlaced { + "1(interlaced)" + } else { + "2(progressive)" + } + } else { + "—" + }; + tracing::debug!( + target: DIAG, + "tag=mkv.track num={track_number} type={} codec={} flag_interlaced={interlaced} \ + field_order={field_order} default_duration_ns={} field_duration_ns={} \ + pixel={}x{} display={}x{} cp_len={} cp_hex={cp}", + track.track_type, + track.codec_id, + track.default_duration_ns, + track.field_duration_ns, + track.pixel_width, + track.pixel_height, + track.display_width, + track.display_height, + track.codec_private.as_ref().map_or(0, |b| b.len()), + ); +} + +// ── Opening-frame capture (first ~N coded frames per track → side file) ────── + +/// Number of coded frames captured PER TRACK before the capture goes dormant. +/// ~100 frames covers a DVD's first few seconds of every track (the +/// opening-GOP / still-frame / menu window where mid-GOP open or PTS-floor bugs +/// show up) while bounding the side file to a few MB even for HD I-frames. +const OPENING_FRAMES_PER_TRACK: usize = 100; + +/// Captures the first [`OPENING_FRAMES_PER_TRACK`] coded frames of EACH track to +/// a side file (`.opening.bin`) and logs a per-frame summary line, so an +/// opening-GOP / menu / mid-GOP-open issue is diagnosable from a future log + +/// side file WITHOUT the disc. Gated to `--log-level 3`: constructed only when +/// the diag target is on, so a normal run never opens the file or records a byte. +/// +/// Side-file record framing (so a reader can split it back into frames): +/// `[track:u8][keyframe:u8][pts_ns:i64 LE][len:u32 LE][raw frame bytes]`. +pub struct OpeningCapture { + file: std::fs::File, + /// Frames captured so far, per track index. Capture for a track stops once + /// its counter reaches [`OPENING_FRAMES_PER_TRACK`]. + counts: Vec, +} + +impl OpeningCapture { + /// Open `.opening.bin` next to the MKV output. Returns `None` (no + /// capture) when the diag target is off OR the side file can't be created — + /// a diagnostic must never fail the rip. `track_count` sizes the per-track + /// counters. + pub fn new(output_path: &std::path::Path, track_count: usize) -> Option { + if !diag_enabled() { + return None; + } + let mut name = output_path.as_os_str().to_os_string(); + name.push(".opening.bin"); + match std::fs::File::create(&name) { + Ok(file) => { + tracing::debug!( + target: DIAG, + "tag=mkv.opening.open path={:?} per_track_cap={OPENING_FRAMES_PER_TRACK}", + std::path::Path::new(&name), + ); + Some(Self { + file, + counts: vec![0; track_count], + }) + } + Err(e) => { + tracing::debug!( + target: DIAG, + "tag=mkv.opening.open path={:?} failed={e} (capture disabled, rip unaffected)", + std::path::Path::new(&name), + ); + None + } + } + } + + /// Record one coded frame for `track_idx` if that track is still under its + /// per-track cap. Writes the framed raw bytes to the side file and logs a + /// one-line summary. A write error disables further capture for the track + /// (counter pinned to the cap) but never propagates — the rip is unaffected. + pub fn record(&mut self, track_idx: usize, pts_ns: i64, keyframe: bool, data: &[u8]) { + let Some(count) = self.counts.get_mut(track_idx) else { + return; + }; + if *count >= OPENING_FRAMES_PER_TRACK { + return; + } + use std::io::Write; + let rec = frame_record(track_idx, pts_ns, keyframe, data); + if let Err(e) = self.file.write_all(&rec) { + // Stop trying on this track; a broken side file must not stall mux. + *count = OPENING_FRAMES_PER_TRACK; + tracing::debug!( + target: DIAG, + "tag=mkv.opening.frame track={track_idx} write_failed={e} (capture stopped for track)", + ); + return; + } + *count += 1; + tracing::debug!( + target: DIAG, + "tag=mkv.opening.frame track={track_idx} n={count} type={} size={} pts_ns={pts_ns}", + if keyframe { "key" } else { "delta" }, + data.len(), + ); + } +} + // ── Disc-level dump (post-lowering: titles, streams, decisions, AACS) ──────── /// Emit the full scan diagnostic block for a built [`Disc`]. Terse, one line @@ -476,6 +667,52 @@ mod tests { assert_eq!(sample_rate_hz(SampleRate::S96), 96000); } + #[test] + fn codec_private_hex_renders_caps_and_handles_empty() { + // None / empty → "none" (no hex). The Windows-fps diagnosis only needs + // the seq-header prefix, so render it but cap long blobs. + assert_eq!(codec_private_hex(None), "none"); + assert_eq!(codec_private_hex(Some(&[])), "none"); + // Short blob: full uppercase hex, no suffix. An MPEG-2 seq header starts + // 00 00 01 B3 — exactly what a reader greps for in a bug log. + assert_eq!( + codec_private_hex(Some(&[0x00, 0x00, 0x01, 0xB3])), + "000001B3" + ); + // Over the cap: first CODEC_PRIVATE_HEX_CAP bytes + a "..(+NB)" summary. + let big = vec![0xABu8; CODEC_PRIVATE_HEX_CAP + 5]; + let s = codec_private_hex(Some(&big)); + assert!(s.starts_with(&"AB".repeat(CODEC_PRIVATE_HEX_CAP)), "{s}"); + assert!(s.ends_with("..(+5B)"), "{s}"); + } + + #[test] + fn frame_record_layout_is_parseable() { + // The .opening.bin record framing must round-trip so a future tool can + // split the side file back into frames without the disc: + // [track:u8][keyframe:u8][pts_ns:i64 LE][len:u32 LE][raw bytes]. + let data = [0xDEu8, 0xAD, 0xBE, 0xEF]; + let rec = frame_record(2, -40_000_000, true, &data); + assert_eq!(rec.len(), 14 + data.len()); + assert_eq!(rec[0], 2, "track index"); + assert_eq!(rec[1], 1, "keyframe flag"); + assert_eq!( + i64::from_le_bytes(rec[2..10].try_into().unwrap()), + -40_000_000, + "pts_ns survives (signed — opening back-anchor can be negative)" + ); + assert_eq!( + u32::from_le_bytes(rec[10..14].try_into().unwrap()), + 4, + "len" + ); + assert_eq!(&rec[14..], &data, "raw frame bytes follow"); + // A non-keyframe records the flag as 0. + let delta = frame_record(0, 0, false, &[]); + assert_eq!(delta[1], 0); + assert_eq!(u32::from_le_bytes(delta[10..14].try_into().unwrap()), 0); + } + /// The cell row shows the raw category byte (0xNN) beside the decode, and /// the keep/drop verdict. A plain feature cell (0x00) is "keep"; a leading /// secondary-block cell flagged dropped reads "DROP". diff --git a/src/mux/codec/mpeg2.rs b/src/mux/codec/mpeg2.rs index d93f3c9..be74559 100644 --- a/src/mux/codec/mpeg2.rs +++ b/src/mux/codec/mpeg2.rs @@ -834,6 +834,61 @@ mod tests { assert!(f[0].keyframe); } + #[test] + fn opening_au_keeps_disc_pts_and_opening_seq_header_no_zero_floor() { + // SOTL SUB-TASK 2 regression (opening-GOP / still-frame open). A DVD + // title opens on a VOBU that begins with a sequence header + I-frame; the + // disc stamps that opening I-frame at its REAL (non-zero) timeline PTS, + // not 0. The parser must (a) emit the opening I-frame with that real PTS + // — never floored to 0 — and (b) capture THAT opening sequence header as + // codec_private (read at headers-ready, before any later AU). Proves the + // opening pictures are emitted with the correct seq header + PTS, ruling + // out the "wrong/last seq header" and "PTS floored to t=0" hypotheses. + let mut p = Mpeg2Parser::new(); + + // Opening AU: seq header (the codecPrivate) + GOP + I-frame TR0 carrying + // the disc's real opening PTS (2 s here, i.e. NOT zero). 25 fps PAL. + let mut a = make_seq_header(720, 576, 3, 3); // 16:9, 25 fps + a.extend_from_slice(&gop()); + a.extend_from_slice(&make_picture_header_tr(PICTURE_TYPE_I, 0)); + a.extend_from_slice(&[0xAA; 20]); + let mut frames = p.parse(&make_pes(a, Some(180_000))); // PTS = 2 s (90 kHz) + assert!(frames.is_empty(), "first AU waits for the next boundary"); + + // Second picture (no PTS) closes the opening AU: the I-frame emits and + // the opening sequence header is captured (headers-ready timing — the + // consumer reads codec_private once the first AU drains). + let mut b = make_picture_header_tr(3, 1); + b.extend_from_slice(&[0xBB; 20]); + frames.extend(p.parse(&make_pes(b, None))); + + // codec_private is the OPENING sequence header (read at headers-ready, + // before any later AU could replace it). + let cp = p + .codec_private() + .expect("opening seq header captured at headers-ready"); + assert_eq!( + &cp[..4], + &[0x00, 0x00, 0x01, SEQ_HEADER_CODE], + "codec_private is the opening sequence header" + ); + assert_eq!(p.resolution(), Some((720, 576)), "576i opening header"); + assert_eq!(p.frame_rate(), Some((25, 1)), "25 fps opening header"); + + frames.extend(p.flush()); + + assert_eq!(frames.len(), 2); + assert!(frames[0].keyframe, "opening picture is the I-frame"); + assert_eq!( + frames[0].pts_ns, 2_000_000_000, + "opening I-frame keeps the disc's real PTS (2 s), NOT floored to 0" + ); + assert_eq!( + frames[1].pts_ns, 2_040_000_000, + "next frame is one 40 ms interval later on the real timeline" + ); + } + // --- Sequence header → codec_private --- #[test] diff --git a/src/mux/mkv.rs b/src/mux/mkv.rs index 17af228..a339ce3 100644 --- a/src/mux/mkv.rs +++ b/src/mux/mkv.rs @@ -145,14 +145,32 @@ impl MkvTrack { } else { ebml::FIELD_ORDER_UNDETERMINED }, - // One field is half a frame. For 576i 25 fps (40 ms frame) this is - // 20 ms; for 480i 29.97 fps (~33.4 ms frame) ~16.68 ms. Only set on - // interlaced tracks with a known frame duration. - field_duration_ns: if v.resolution.is_interlaced() && default_duration_ns > 0 { - default_duration_ns / 2 - } else { - 0 - }, + // DefaultDecodedFieldDuration is DELIBERATELY NOT emitted (0 here + // suppresses the element; see the writer in `MkvMuxer::new`). + // + // rc.5.1 added it (= half the frame period, 20 ms for 576i25) to try + // to fix the Windows-fps report, on the theory that Windows derives + // fps from it. The captured SOTL evidence proves the opposite: with + // FlagInterlaced=1 + DefaultDuration=40 ms + DefaultDecodedFieldDuration=20 ms, + // Windows Explorer reports 12.5 fps (half), and MediaInfo flips the + // track to "Frame rate mode: Variable" with no clean rate. MakeMKV's + // correct rip of the same disc OMITS DefaultDecodedFieldDuration, + // keeps FlagInterlaced=1 + FieldOrder=TFF + DefaultDuration=40 ms, and + // Explorer reports the full 25 fps with MediaInfo "Constant". ffmpeg's + // matroskaenc.c does the same (full-frame DefaultDuration, no field + // duration). The lone frame-rate signal every tool actually trusts is + // `1 / DefaultDuration`; that full-frame value (40 ms → 25 fps) is kept + // below. Dropping the field-duration element removes the per-field + // signal that made Explorer halve the rate. + // + // Trade-off: the container no longer carries an explicit per-field + // decoded duration. Nothing is lost in practice — the interlace + // signaling that deinterlacers and MediaInfo rely on lives in the + // MPEG-2 elementary stream's picture_coding_extension (picture_structure / + // top_field_first), which MediaInfo reads directly (so it still reports + // "Interlaced / Top Field First"), and the container still flags + // FlagInterlaced=1 + FieldOrder=TFF so players keep deinterlacing. + field_duration_ns: 0, sample_rate: 0.0, channels: 0, bit_depth: 0, @@ -333,6 +351,12 @@ pub struct MkvMuxer { /// (to patch in place) and the IFO-claimed count (to warn on disagreement); /// `corrected` flips once patched so we only act on the first frame. ac3_channel_fixups: std::collections::HashMap, + /// `--log-level 3` opening-frame capture: the first ~100 coded frames per + /// track are written (raw) to a `.opening.bin` side file with a + /// per-frame summary logged, so an opening-GOP / menu / mid-GOP-open issue is + /// diagnosable from a future log without the disc. `None` on normal runs + /// (diag off) — the muxer pays nothing. + opening_capture: Option, } /// Deferred AC-3 channel-count correction: the track header's `Channels` byte @@ -727,12 +751,15 @@ impl MkvMuxer { )?; } - // DefaultDecodedFieldDuration (one FIELD = half a frame) on - // interlaced tracks. Per the Matroska schema it is a DIRECT child - // of TrackEntry (NOT inside Video). Without it an interlace-aware - // reader (Windows shell) assumes "block = one field" and reports - // half the frame rate (12.5 instead of 25 for 576i). DefaultDuration - // above stays the full-frame period (40 ms); this is 20 ms. + // DefaultDecodedFieldDuration (one FIELD = half a frame), a DIRECT + // child of TrackEntry. The production video path now ALWAYS passes + // `field_duration_ns == 0` (see `MkvTrack::video`) so this element is + // NOT written: emitting it (20 ms for 576i25) is exactly what made + // Windows Explorer report 12.5 fps and MediaInfo flip to VFR on the + // captured SOTL rip, while MakeMKV — which omits it — shows the full + // 25 fps. The guard below is retained so a non-zero value still emits + // a well-formed element for any future caller / round-trip test, but + // the muxer's own callers no longer trigger it. if track.track_type == ebml::TRACK_TYPE_VIDEO && track.interlaced && track.field_duration_ns > 0 @@ -885,9 +912,18 @@ impl MkvMuxer { track_uids, duration_secs, ac3_channel_fixups, + opening_capture: None, }) } + /// Attach an opening-frame capture (`--log-level 3`). The capture writes the + /// first ~100 coded frames per track to `.opening.bin` and logs a + /// per-frame summary, so opening-GOP / menu issues are diagnosable from a + /// log + side file without the disc. `None` is a no-op (normal runs). + pub fn set_opening_capture(&mut self, capture: Option) { + self.opening_capture = capture; + } + /// Write a single frame. /// /// When `duration_ns` is `Some`, the frame is emitted as a @@ -903,6 +939,15 @@ impl MkvMuxer { data: &[u8], duration_ns: Option, ) -> io::Result<()> { + // --log-level 3: capture the first ~100 coded frames per track to the + // side file BEFORE any timeline mangling, with the codec parser's own + // frame PTS — so an opening-GOP / mid-GOP-open / menu issue is + // reconstructable from the log + side file alone (no disc). No-op (and + // no allocation) on normal runs; the capture is `None`. + if let Some(cap) = self.opening_capture.as_mut() { + cap.record(track_idx, pts_ns, keyframe, data); + } + // Is this a video track? Used for the monotonic block-timestamp nudge // below, which must exempt EVERY video track (incl. a Dolby Vision EL). let is_video = self.track_is_video.get(track_idx).copied().unwrap_or(false); @@ -2516,6 +2561,50 @@ mod tests { } } + #[test] + fn opening_keyframe_with_nonzero_disc_pts_anchors_base_not_corrupted() { + // SOTL SUB-TASK 2 regression (opening-GOP PTS handling). A DVD title + // opens on an I-frame the disc stamps at its REAL timeline PTS (here + // ~10 s, a large non-zero value — NOT 0). The muxer must anchor `base` on + // that first kept keyframe so the first cluster's timestamp is 0 (the + // `.max(0)` floor must NOT corrupt it into a huge value or wrap), and the + // following frame must land exactly one frame interval (40 ms = 400 ticks + // at the 0.1 ms scale) later — proving the opening pictures keep their + // relative timeline and aren't garbled. + let tracks = [make_video_track()]; + const OPEN_PTS: i64 = 10_000_000_000; // 10 s opening anchor + let frames = vec![ + (0usize, OPEN_PTS, true, vec![0xAAu8; 8]), // opening I-frame + (0usize, OPEN_PTS + 40_000_000, false, vec![0xBBu8; 8]), // +40 ms + ]; + let (data, count) = mux_to_bytes(&tracks, &[], &frames); + assert_eq!( + count, 2, + "both opening frames written (none dropped/floored away)" + ); + + // Read the FIRST cluster's timestamp — must be 0 (base == opening PTS). + let (_, seg_start) = locate_segment(&data); + let cluster_abs = seg_start + + segment_children(&data) + .iter() + .find(|(id, _, _)| *id == ebml::CLUSTER) + .map(|(_, off, _)| *off - seg_start) + .expect("a cluster was written"); + let mut bc = Cursor::new(&data[cluster_abs..]); + let (tid, tsize, _) = ebml::read_element_header(&mut bc).unwrap(); + assert_eq!(tid, ebml::CLUSTER_TIMESTAMP); + let cluster_ts = ebml::read_uint_val(&mut bc, tsize as usize).unwrap(); + assert_eq!( + cluster_ts, 0, + "opening cluster timestamp must be 0 (base anchored on the opening keyframe's real PTS)" + ); + + // The first cue (opening keyframe) is at tick 0 — not the absolute disc PTS. + let cues = parse_cues(&data); + assert_eq!(cues[0].0, 0, "opening cue at t=0, disc PTS rebased to base"); + } + #[test] fn seekhead_is_first_child_of_segment() { let tracks = [make_video_track(), make_audio_track()]; @@ -3263,10 +3352,16 @@ mod tests { } #[test] - fn interlaced_576i_emits_default_decoded_field_duration() { - // 576i @ 25 fps: DefaultDuration = 40 ms (frame), and - // DefaultDecodedFieldDuration = 20 ms (field = half a frame). The field - // element stops interlace-aware readers (Windows) halving the frame rate. + fn interlaced_576i_omits_default_decoded_field_duration_keeps_full_frame_duration() { + // SOTL SUB-TASK 1 regression. The Windows-fps fix: a 576i25 track must + // carry the FULL-FRAME DefaultDuration (40 ms → `1/DefaultDuration` = 25 + // fps, the only rate every tool trusts) and must NOT emit + // DefaultDecodedFieldDuration. rc.5.1 emitted the 20 ms field duration to + // try to fix Windows; the captured SOTL evidence proved it did the + // opposite (Explorer 12.5 fps, MediaInfo VFR). MakeMKV's correct rip omits + // it (Explorer 25 fps, MediaInfo CFR). So: frame duration present = 40 ms, + // field duration ABSENT, interlace signalling (FlagInterlaced/FieldOrder) + // retained. let v = VideoStream { pid: 0xE0, codec: Codec::Mpeg2, @@ -3280,19 +3375,35 @@ mod tests { }; let t = MkvTrack::video(&v); assert_eq!(t.default_duration_ns, 40_000_000, "frame duration is 40 ms"); - assert_eq!(t.field_duration_ns, 20_000_000, "field duration is 20 ms"); + assert_eq!( + t.field_duration_ns, 0, + "field duration must be 0 so the element is suppressed" + ); let muxer = MkvMuxer::new(Cursor::new(Vec::new()), &[t], None, 0.0, &[]).unwrap(); let data = muxer.writer.into_inner(); - // DefaultDuration (frame) present and = 40 ms. + // DefaultDuration (frame) present and = 40 ms → drives the 25 fps report. let dd = find_id(&data, ebml::DEFAULT_DURATION).expect("DefaultDuration present"); - // [id 3B][size 0x84][4-byte value] — 40_000_000 needs 4 bytes. let frame_ns = u32::from_be_bytes([data[dd + 4], data[dd + 5], data[dd + 6], data[dd + 7]]); assert_eq!(frame_ns, 40_000_000, "DefaultDuration is the full frame"); - // DefaultDecodedFieldDuration present and = 20 ms. - let fd = - find_id(&data, ebml::DEFAULT_DECODED_FIELD_DURATION).expect("field duration present"); - let field_ns = u32::from_be_bytes([data[fd + 4], data[fd + 5], data[fd + 6], data[fd + 7]]); - assert_eq!(field_ns, 20_000_000, "field duration is half the frame"); + // DefaultDecodedFieldDuration must be ABSENT — this is the fix. + assert!( + find_id(&data, ebml::DEFAULT_DECODED_FIELD_DURATION).is_none(), + "DefaultDecodedFieldDuration must NOT be written (Windows halves the rate when it is)" + ); + // Interlace signalling is RETAINED so deinterlacers still engage and + // MediaInfo (which also reads scan type from the MPEG-2 ES) agrees. + let fi = find_id(&data, ebml::FLAG_INTERLACED).expect("FlagInterlaced present"); + assert_eq!( + data[fi + 2], + ebml::INTERLACED_INTERLACED as u8, + "FlagInterlaced=1 retained" + ); + let fo = find_id(&data, ebml::FIELD_ORDER).expect("FieldOrder present"); + assert_eq!( + data[fo + 2], + ebml::FIELD_ORDER_TFF, + "FieldOrder=TFF retained" + ); } #[test] @@ -3308,27 +3419,20 @@ mod tests { } #[test] - fn field_duration_is_direct_trackentry_child_not_in_video() { - // GUARDS THE REAL BUG (audit §3 #1): DefaultDecodedFieldDuration - // (0x234E7A) is, per the Matroska schema, a DIRECT child of TrackEntry — - // NOT nested inside the Video master (which only holds FlagInterlaced / - // FieldOrder). The pre-fix writer emitted it between start_master(VIDEO) - // and end_master(vid_pos), burying it inside Video. The old test used the - // flat `find_id` byte-scan, which passes regardless of nesting. This is a - // depth-aware check: the element MUST appear among TrackEntry's direct - // children and MUST NOT appear among Video's direct children. - let v = VideoStream { - pid: 0xE0, - codec: Codec::Mpeg2, - resolution: Resolution::R576i, - frame_rate: crate::disc::FrameRate::F25, - hdr: HdrFormat::Sdr, - color_space: ColorSpace::Bt470bg, - display_aspect: None, - secondary: false, - label: String::new(), - }; - let t = MkvTrack::video(&v); + fn field_duration_when_set_is_direct_trackentry_child_not_in_video() { + // GUARDS the element nesting for the retained (now non-default) writer + // path: if a caller DOES set field_duration_ns > 0, + // DefaultDecodedFieldDuration (0x234E7A) must be emitted as a DIRECT child + // of TrackEntry — NOT nested inside the Video master (which only holds + // FlagInterlaced / FieldOrder), per the Matroska schema. The production + // video path passes 0 (so the element is suppressed — see + // interlaced_576i_omits_default_decoded_field_duration_keeps_full_frame_duration); + // this test builds a track with a non-zero field duration to exercise the + // writer guard and pin its (correct) nesting depth. + let mut t = make_video_track(); + t.interlaced = true; + t.field_order = ebml::FIELD_ORDER_TFF; + t.field_duration_ns = 20_000_000; let muxer = MkvMuxer::new(Cursor::new(Vec::new()), &[t], None, 0.0, &[]).unwrap(); let data = muxer.writer.into_inner(); @@ -3544,8 +3648,8 @@ mod tests { "480i frame duration is ~33.37 ms (29.97 fps, not halved)" ); assert_eq!( - t.field_duration_ns, 16_683_333, - "480i field duration is half the frame (~16.68 ms)" + t.field_duration_ns, 0, + "field duration is suppressed (DefaultDecodedFieldDuration omitted — Windows-fps fix)" ); let muxer = MkvMuxer::new(Cursor::new(Vec::new()), &[t], None, 0.0, &[]).unwrap(); let data = muxer.writer.into_inner(); diff --git a/src/mux/mkvstream.rs b/src/mux/mkvstream.rs index f80b1f1..5a86001 100644 --- a/src/mux/mkvstream.rs +++ b/src/mux/mkvstream.rs @@ -95,6 +95,18 @@ impl MkvStream { /// Create for writing PES frames → MKV container. /// Codec privates come from title.codec_privates (populated by input stream). pub fn create(writer: Box, title: &DiscTitle) -> io::Result { + Self::create_at(writer, title, None) + } + + /// As [`create`](Self::create), but `output_path` (when known) enables the + /// `--log-level 3` opening-frame capture to `.opening.bin`. A `None` + /// path (e.g. an in-memory / stdio sink) silently skips the side-file + /// capture; the per-track TrackEntry dump still fires. + pub fn create_at( + writer: Box, + title: &DiscTitle, + output_path: Option<&std::path::Path>, + ) -> io::Result { let mut tracks = Vec::new(); let mut has_default_video = false; let mut has_default_audio = false; @@ -118,7 +130,15 @@ impl MkvStream { tracks.push(track); } - let muxer = MkvMuxer::new( + // --log-level 3: dump the ACTUAL TrackEntry elements about to be written + // (FlagInterlaced / FieldOrder / DefaultDuration / DefaultDecodedFieldDuration + // / Display dims / codecPrivate hex) so the Windows-fps-class metadata is + // verifiable from a log alone. No-op when diag is off. + for (i, track) in tracks.iter().enumerate() { + crate::diag::dump_mkv_track((i + 1) as u64, track); + } + + let mut muxer = MkvMuxer::new( writer, &tracks, Some(&title.playlist), @@ -126,6 +146,13 @@ impl MkvStream { &title.chapters, )?; + // --log-level 3: capture the first ~100 coded frames per track to + // `.opening.bin`. Only opens the side file when diag is on AND a + // real output path is known; otherwise it's a no-op the muxer never sees. + if let Some(path) = output_path { + muxer.set_opening_capture(crate::diag::OpeningCapture::new(path, tracks.len())); + } + Ok(Self { disc_title: title.clone(), mode: Mode::Write { diff --git a/src/mux/resolve.rs b/src/mux/resolve.rs index ef4482a..2d30b62 100644 --- a/src/mux/resolve.rs +++ b/src/mux/resolve.rs @@ -449,7 +449,7 @@ pub fn output( IO_BUF_SIZE, crate::io::WritebackFile::create_with_size_hint(path, title.size_bytes)?, )); - Ok(Box::new(MkvStream::create(writer, title)?)) + Ok(Box::new(MkvStream::create_at(writer, title, Some(path))?)) } StreamUrl::M2ts { ref path } => { validate_file_path(path, "m2ts")?;