From 43fb97f71febbfe73bb810d04de2e9d9ffec77d0 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Thu, 25 Jun 2026 18:40:30 -0700 Subject: [PATCH] mux/mkv: spec-conformance fixes (field order, CICP, VobSub idx, SeekHead, AC-3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - FieldOrder now derives from the bitstream's measured top_field_first (Some(true)→TFF, Some(false)→BFF) instead of hardcoding TFF for all interlaced content; falls back to TFF when unmeasured. Adds VideoStream::top_field_first; DVD/BD scan sets None with a precise TODO(spec) for parser→title plumbing. - CICP (matrix/transfer/primaries/range) now prefers measured CICP from the bitstream (VideoStream::measured_cicp) over the coarse ColorSpace enum, so the container stops assuming a colour space the stream may contradict. Enum remains the fallback. - VobSub S_VOBSUB CodecPrivate now emits a `size: WxH` line ahead of the palette per the .idx format so players place/scale subs correctly. - SeekHead: when zero cues are written, the CUES Seek entry is Voided instead of leaving a dangling pointer to the Cues offset (now Tags/EOF). - AC-3 Channels back-patch offset is captured from the writer instead of the hardcoded chan_elem_pos+2 (decoupled from the VINT width choice). - Hoisted inline CICP codes and the dvcC fourcc to named constants citing ITU-T H.273 / RFC 9559; fixed the stale FieldOrder comment. - DefaultDuration vs pulldown: precise TODO(spec) left (needs the same parser→title channel as top_field_first). Tests: BFF-from-measured-flag, measured-CICP-overrides-enum, VobSub size: line present/omitted, zero-cue SeekHead Void. precommit (1.86) green. --- src/disc/bluray.rs | 11 ++ src/disc/dvd.rs | 33 +++- src/disc/mod.rs | 31 +++ src/labels/mod.rs | 6 + src/mux/codec/dvdsub.rs | 62 ++++-- src/mux/demux_sink.rs | 2 + src/mux/ebml.rs | 19 +- src/mux/m2ts.rs | 2 + src/mux/meta.rs | 6 + src/mux/mkv.rs | 372 ++++++++++++++++++++++++++++++++---- src/mux/mkvstream.rs | 2 + src/mux/network.rs | 2 + src/mux/pipelined_stream.rs | 4 + src/mux/stdio.rs | 2 + src/mux/ts.rs | 2 + tests/disc_tests.rs | 2 + tests/streams.rs | 10 + 17 files changed, 514 insertions(+), 54 deletions(-) diff --git a/src/disc/bluray.rs b/src/disc/bluray.rs index 0d43612..7f67b37 100644 --- a/src/disc/bluray.rs +++ b/src/disc/bluray.rs @@ -148,6 +148,17 @@ impl Disc { // the CLI/UI render the localized descriptor. `label` // stays empty for disc video streams. label: String::new(), + // TODO(spec): for 1080i HEVC/H.264/VC-1 titles, surface + // the measured field order (H.264/HEVC pic_struct, VC-1 + // pulldown) from the codec parser instead of the TFF + // fallback; needs the parser→title channel (see dvd.rs). + top_field_first: None, + // TODO(spec): prefer the HEVC/H.264 VUI colour_description + // (measured CICP) over this MPLS playlist-nibble guess + // once the parser surfaces it through the output title. + // `None` keeps the enum fallback. (HDR MaxCLL/Mastering + // metadata is a separate task and intentionally not here.) + measured_cicp: None, })), 2 | 5 => { // Guard: if coding_type is a subtitle codec (PGS 0x90/0x91), diff --git a/src/disc/dvd.rs b/src/disc/dvd.rs index 9bbd01f..68699ac 100644 --- a/src/disc/dvd.rs +++ b/src/disc/dvd.rs @@ -49,7 +49,31 @@ impl Disc { }), secondary: false, label: String::new(), + // TODO(spec): populate from the MPEG-2 picture coding extension + // once the codec parser surfaces it. `Mpeg2Parser` already reads + // `top_field_first` (mux/codec/mpeg2.rs `picture_nb_fields`) but + // tracks are built from the IFO scan BEFORE any frame is parsed; + // wiring it requires a CodecParser accessor surfaced through + // PipelinedPesStream/DiscStream into the output title (mirroring + // the existing `codec_private` handshake). Until then `None` + // means the muxer falls back to TFF (correct for ~all DVDs). + top_field_first: None, + // TODO(spec): DVD MPEG-2 carries no VUI; the colour signalling is + // the sequence_display_extension colour_description when present. + // Surface it from `Mpeg2Parser` (same handshake as above) and set + // this so a disc that states e.g. BT.601-625 colour overrides the + // PAL/NTSC guess in `color_space`. `None` uses the enum fallback. + measured_cicp: None, }); + // TODO(spec): DefaultDuration is derived from the declared IFO + // frame_rate (25 / 29.97). A soft-telecined 23.976-in-29.97 DVD then + // reports 29.97 fps instead of the true 23.976 film rate. The pulldown + // cadence is detectable from the parser's per-picture `nb_fields` + // (repeat_first_field) — when most frames are 3-field-then-2-field + // 2:3 pulldown the film rate is frame_rate × 4/5. Emitting the film + // DefaultDuration needs the parser to report the measured cadence + // through the same parser→title channel as `top_field_first` above; + // left as a follow-up to avoid a speculative rate change here. // Map DvdAudioAttr to Stream::Audio. The PID is derived from the // stream's REAL on-wire private_stream_1 sub-stream id (assigned @@ -132,11 +156,16 @@ impl Disc { let size_bytes: u64 = extents.iter().map(|e| e.sector_count as u64 * 2048).sum(); - // Build pre-formatted palette codec_data for VobSub subtitle streams + // Build pre-formatted VobSub `.idx` codec_data (size: + palette: + // lines) for VobSub subtitle streams. The `size:` line carries + // the coded video frame the subpicture was authored against + // (720x480 NTSC / 720x576 PAL) so players place and scale the + // bitmap correctly. + let (vid_w, vid_h) = ts.video.resolution.pixels(); let codec_data = dvd_title .palette .as_ref() - .map(|pal| crate::mux::codec::dvdsub::format_palette(pal)); + .map(|pal| crate::mux::codec::dvdsub::format_palette(pal, vid_w, vid_h)); // Map DvdSubtitleAttr to Stream::Subtitle let subtitle_streams: Vec = ts diff --git a/src/disc/mod.rs b/src/disc/mod.rs index 7c85846..2f1b40f 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -196,6 +196,35 @@ pub struct VideoStream { pub secondary: bool, /// Extra label (e.g. "Dolby Vision EL") pub label: String, + /// Field-display order MEASURED from the elementary stream's interlace + /// signalling (MPEG-2 picture coding extension `top_field_first`, H.264/HEVC + /// `pic_struct`), when available: `Some(true)` = top-field-first, + /// `Some(false)` = bottom-field-first. `None` = not measured — the muxer + /// falls back to TFF for interlaced content (the dominant DVD/HD case). The + /// container FieldOrder must agree with the bitstream, so a measured BFF + /// stream must NOT be stamped TFF. Ignored for progressive video. + pub top_field_first: Option, + /// CICP colour signalling (matrix, transfer, primaries, full_range) MEASURED + /// from the bitstream — HEVC/H.264 VUI `colour_description` or MPEG-2 + /// `sequence_display_extension`. `Some(...)` takes precedence over the + /// coarse `color_space` enum (a playlist nibble / PAL-NTSC guess); `None` + /// means the bitstream did not state it, so the enum-derived triplet is used. + /// Codes are ITU-T H.273 (CICP); `range` is 1 = limited/TV, 2 = full. + pub measured_cicp: Option, +} + +/// Measured CICP colour signalling read directly from a video elementary stream +/// (ITU-T H.273). Preferred over the coarse [`ColorSpace`] enum when present. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct MeasuredCicp { + /// MatrixCoefficients (ITU-T H.273 Table 4). + pub matrix: u8, + /// TransferCharacteristics (ITU-T H.273 Table 3). + pub transfer: u8, + /// ColourPrimaries (ITU-T H.273 Table 2). + pub primaries: u8, + /// Range: 1 = limited (studio/TV), 2 = full. Matroska Colour/Range values. + pub range: u8, } /// An audio stream. @@ -3776,6 +3805,8 @@ mod tests { display_aspect: None, secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, })], chapters: Vec::new(), extents: Vec::new(), diff --git a/src/labels/mod.rs b/src/labels/mod.rs index 766672b..7f6dd8f 100644 --- a/src/labels/mod.rs +++ b/src/labels/mod.rs @@ -1308,6 +1308,8 @@ mod apply_tests { display_aspect: None, secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, }) } @@ -1760,6 +1762,8 @@ mod apply_tests { display_aspect: None, secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, }); let mut titles = vec![title_with(vec![interlaced])]; fill_defaults(&mut titles); @@ -1782,6 +1786,8 @@ mod apply_tests { display_aspect: None, secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, }); let mut titles = vec![title_with(vec![progressive])]; fill_defaults(&mut titles); diff --git a/src/mux/codec/dvdsub.rs b/src/mux/codec/dvdsub.rs index 853316c..8524be7 100644 --- a/src/mux/codec/dvdsub.rs +++ b/src/mux/codec/dvdsub.rs @@ -182,20 +182,37 @@ fn clamp_u8(v: f64) -> u8 { } } -/// Format a 16-color YCbCr palette as a VobSub .idx palette header. +/// Format a 16-color YCbCr palette as a VobSub `.idx` header for S_VOBSUB +/// CodecPrivate. /// -/// Each entry is `[padding, Y, Cb, Cr]`. Output is a UTF-8 text block: -/// `palette: rrggbb, rrggbb, ...\n` +/// Each entry is `[padding, Y, Cb, Cr]`. Output is a UTF-8 text block carrying +/// the two `.idx` header lines mkvmerge / libvobsub expect: +/// +/// ```text +/// size: x +/// palette: rrggbb, rrggbb, ... +/// ``` +/// +/// The `size:` line is the VobSub original-frame resolution (the video frame the +/// subpicture coordinates were authored against). Players read it to place and +/// scale the bitmap; without it, some renderers assume a default frame and +/// mis-position or mis-scale the subtitles. `width`/`height` are the title's +/// coded video dimensions. When either is 0 (unknown) the `size:` line is +/// omitted rather than emitting a `0x0` frame. /// /// Returns the formatted bytes suitable for MKV codec_private. -pub fn format_palette(palette: &[[u8; 4]]) -> Vec { +pub fn format_palette(palette: &[[u8; 4]], width: u32, height: u32) -> Vec { let mut parts: Vec = Vec::with_capacity(palette.len()); for color in palette { let [r, g, b] = ycbcr_to_rgb(color); parts.push(format!("{r:02x}{g:02x}{b:02x}")); } - let line = format!("palette: {}\n", parts.join(", ")); - line.into_bytes() + let mut out = String::new(); + if width > 0 && height > 0 { + out.push_str(&format!("size: {width}x{height}\n")); + } + out.push_str(&format!("palette: {}\n", parts.join(", "))); + out.into_bytes() } #[cfg(test)] @@ -436,7 +453,7 @@ mod tests { [0x00, 0, 128, 128], // Y=0 → RGB (0,0,0) [0x00, 255, 128, 128], // Y=255 → RGB (255,255,255) ]; - let result = format_palette(&palette); + let result = format_palette(&palette, 0, 0); let text = String::from_utf8(result).unwrap(); assert!( text.starts_with("palette: "), @@ -460,7 +477,7 @@ mod tests { #[test] fn format_palette_16_colors() { let palette: Vec<[u8; 4]> = (0..16).map(|i| [0x00, (i * 16) as u8, 128, 128]).collect(); - let result = format_palette(&palette); + let result = format_palette(&palette, 0, 0); let text = String::from_utf8(result).unwrap(); // Should have exactly 15 commas (16 colors separated by ", ") let comma_count = text.matches(", ").count(); @@ -475,11 +492,34 @@ mod tests { fn format_palette_hex_format() { // Y=128, Cb=128, Cr=128 → R=128, G=128, B=128 → "808080" let palette = vec![[0x00, 128, 128, 128]]; - let result = format_palette(&palette); + let result = format_palette(&palette, 0, 0); let text = String::from_utf8(result).unwrap(); assert_eq!(text, "palette: 808080\n"); } + #[test] + fn format_palette_emits_size_line_before_palette() { + // With non-zero dimensions the `.idx` `size:` line is prepended ahead of + // the palette so players place/scale the VobSub bitmap (PAL 720x576). + let palette = vec![[0x00, 128, 128, 128]]; + let result = format_palette(&palette, 720, 576); + let text = String::from_utf8(result).unwrap(); + assert_eq!( + text, "size: 720x576\npalette: 808080\n", + "size: line must precede palette: line" + ); + } + + #[test] + fn format_palette_omits_size_line_when_dimensions_unknown() { + // 0 width/height (unknown resolution) omits the size line rather than + // emitting a 0x0 frame; the palette line is still present. + let palette = vec![[0x00, 128, 128, 128]]; + let result = format_palette(&palette, 0, 576); + let text = String::from_utf8(result).unwrap(); + assert_eq!(text, "palette: 808080\n", "no size line when a dim is 0"); + } + // --- SPU_size boundary: completes exactly at declared size --- #[test] @@ -641,7 +681,7 @@ mod tests { #[test] fn format_palette_empty_is_just_prefix() { // An empty palette yields "palette: \n" (prefix + newline, no entries). - let result = format_palette(&[]); + let result = format_palette(&[], 0, 0); assert_eq!(String::from_utf8(result).unwrap(), "palette: \n"); } @@ -649,7 +689,7 @@ mod tests { fn format_palette_pads_each_channel_to_two_hex_digits() { // Each RGB channel is formatted as exactly 2 hex digits (zero-padded). // Y=16,neutral → 0x10 → "101010" (each channel two digits). - let result = format_palette(&[[0x00, 16, 128, 128]]); + let result = format_palette(&[[0x00, 16, 128, 128]], 0, 0); assert_eq!(String::from_utf8(result).unwrap(), "palette: 101010\n"); } } diff --git a/src/mux/demux_sink.rs b/src/mux/demux_sink.rs index 7a582e6..381f76e 100644 --- a/src/mux/demux_sink.rs +++ b/src/mux/demux_sink.rs @@ -832,6 +832,8 @@ mod tests { display_aspect: None, secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, }) } diff --git a/src/mux/ebml.rs b/src/mux/ebml.rs index 29bd22b..e781176 100644 --- a/src/mux/ebml.rs +++ b/src/mux/ebml.rs @@ -394,6 +394,10 @@ pub const SEGMENT: u32 = 0x1853_8067; // SeekHead pub const SEEK_HEAD: u32 = 0x114D_9B74; pub const SEEK: u32 = 0x4DBB; +/// Void — RFC 9559 (Matroska) §EBML global element 0xEC. Used to neutralise a +/// reserved-but-unused region (e.g. the CUES SeekHead entry when no Cues element +/// is written) so it carries no meaning to a parser. +pub const VOID: u32 = 0xEC; pub const SEEK_ID: u32 = 0x53AB; pub const SEEK_POSITION: u32 = 0x53AC; @@ -436,16 +440,13 @@ pub const FIELD_ORDER: u32 = 0x9D; pub const INTERLACED_INTERLACED: u64 = 1; pub const INTERLACED_PROGRESSIVE: u64 = 2; // FieldOrder values (Matroska / RFC 9559, element 0x9D): 1 = top-field-first, -// 6 = bottom-field-first, 2 = undetermined, 0 = progressive. NTSC DVD (480i), -// PAL DVD (576i) and HD (1080i) are all emitted top-field-first — the muxer -// hardcodes TFF for every interlaced DVD/HD source (DV is the only common BFF -// source and freemkv does not produce it). 0xFF is our sentinel for -// "undetermined / omit". +// 6 = bottom-field-first, 0 = progressive. The muxer derives TFF vs BFF from the +// bitstream's measured top_field_first when available, falling back to TFF for +// interlaced content (NTSC 480i / PAL 576i / HD 1080i are overwhelmingly TFF). +// 0xFF is our sentinel for "undetermined / omit the element". pub const FIELD_ORDER_TFF: u8 = 1; -// Bottom-field-first. Retained for completeness/round-trip tests; the muxer -// emits TFF for all DVD/HD interlaced content (DV is the only common BFF -// source and freemkv does not produce it). -#[allow(dead_code)] +/// Bottom-field-first (RFC 9559 element 0x9D = 6). Emitted when the bitstream's +/// measured top_field_first is false. pub const FIELD_ORDER_BFF: u8 = 6; pub const FIELD_ORDER_UNDETERMINED: u8 = 0xFF; pub const DISPLAY_WIDTH: u32 = 0x54B0; diff --git a/src/mux/m2ts.rs b/src/mux/m2ts.rs index 997a6e4..fef54db 100644 --- a/src/mux/m2ts.rs +++ b/src/mux/m2ts.rs @@ -116,6 +116,8 @@ mod tests { display_aspect: None, secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, })], chapters: Vec::new(), extents: Vec::new(), diff --git a/src/mux/meta.rs b/src/mux/meta.rs index a9e4aa6..257fa92 100644 --- a/src/mux/meta.rs +++ b/src/mux/meta.rs @@ -212,6 +212,8 @@ impl M2tsMeta { display_aspect: None, secondary: *secondary, label: label.clone(), + top_field_first: None, + measured_cicp: None, }) } MetaStream::Audio { @@ -407,6 +409,8 @@ mod tests { display_aspect: None, secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, })); t } @@ -656,6 +660,8 @@ mod tests { display_aspect: None, secondary: false, label: "x".into(), + top_field_first: None, + measured_cicp: None, })); } let meta = M2tsMeta::from_title(&t); diff --git a/src/mux/mkv.rs b/src/mux/mkv.rs index ad4dff5..e39c25f 100644 --- a/src/mux/mkv.rs +++ b/src/mux/mkv.rs @@ -12,6 +12,54 @@ use crate::disc::{ }; use std::io::{self, Seek, Write}; +// ── CICP colour codes (ITU-T H.273) ────────────────────────────────────────── +// +// The Matroska Colour element (RFC 9559) carries MatrixCoefficients, +// TransferCharacteristics and Primaries verbatim as the integer code-points +// defined by ITU-T H.273 ("Coding-independent code points", CICP). Hoisting the +// codes to named constants keeps the colour match arms self-documenting and the +// values traceable to the public spec table that defines each. + +/// ColourPrimaries = 1 (BT.709 / sRGB) — ITU-T H.273 Table 2. +const CICP_PRIMARIES_BT709: u8 = 1; +/// ColourPrimaries = 5 (BT.470 System B/G — PAL/SECAM SD) — ITU-T H.273 Table 2. +const CICP_PRIMARIES_BT470BG: u8 = 5; +/// ColourPrimaries = 6 (BT.601-525 / SMPTE 170M — NTSC SD) — ITU-T H.273 Table 2. +const CICP_PRIMARIES_BT601_525: u8 = 6; +/// ColourPrimaries = 9 (BT.2020 / BT.2100) — ITU-T H.273 Table 2. +const CICP_PRIMARIES_BT2020: u8 = 9; + +/// TransferCharacteristics = 1 (BT.709) — ITU-T H.273 Table 3. +const CICP_TRANSFER_BT709: u8 = 1; +/// TransferCharacteristics = 5 (BT.470 System B/G) — ITU-T H.273 Table 3. +const CICP_TRANSFER_BT470BG: u8 = 5; +/// TransferCharacteristics = 6 (BT.601-525 / SMPTE 170M) — ITU-T H.273 Table 3. +const CICP_TRANSFER_BT601_525: u8 = 6; +/// TransferCharacteristics = 16 (SMPTE ST 2084 / PQ — HDR10/HDR10+/DV) — ITU-T +/// H.273 Table 3. +const CICP_TRANSFER_PQ: u8 = 16; +/// TransferCharacteristics = 18 (ARIB STD-B67 / Hybrid Log-Gamma) — ITU-T H.273 +/// Table 3. +const CICP_TRANSFER_HLG: u8 = 18; + +/// MatrixCoefficients = 1 (BT.709) — ITU-T H.273 Table 4. +const CICP_MATRIX_BT709: u8 = 1; +/// MatrixCoefficients = 5 (BT.470 System B/G) — ITU-T H.273 Table 4. +const CICP_MATRIX_BT470BG: u8 = 5; +/// MatrixCoefficients = 6 (BT.601-525 / SMPTE 170M) — ITU-T H.273 Table 4. +const CICP_MATRIX_BT601_525: u8 = 6; +/// MatrixCoefficients = 9 (BT.2020 non-constant luminance) — ITU-T H.273 Table 4. +const CICP_MATRIX_BT2020NC: u8 = 9; + +/// Matroska Colour/Range = 1 (broadcast / studio-swing "limited" range). RFC +/// 9559 Range element. (0 = unspecified, 2 = full.) +const COLOUR_RANGE_LIMITED: u8 = 1; + +/// BlockAddIDType "dvcC" — the DOVIDecoderConfigurationRecord fourcc, big-endian +/// ASCII 'd''v''c''C'. Matroska BlockAdditionMapping/BlockAddIDType for a Dolby +/// Vision configuration record (RFC 9559 + Dolby Vision-in-Matroska spec). +const BLOCK_ADD_ID_TYPE_DVCC: u64 = 0x6476_6343; + /// MKV track definition (built from disc stream metadata). pub struct MkvTrack { pub track_type: u64, // 1=video, 2=audio, 17=subtitle @@ -92,19 +140,58 @@ impl MkvTrack { } else { 0 }; - // (matrix, transfer, primaries, range) — ITU-T H.273 / CICP codes. - let (matrix, transfer, primaries, range) = match v.color_space { - ColorSpace::Bt2020 => (9, 16, 9, 1), // bt2020nc, PQ, bt2020, limited - ColorSpace::Bt709 => (1, 1, 1, 1), // bt709 - ColorSpace::Bt470bg => (5, 5, 5, 1), // PAL SD: BT.470BG matrix/transfer/primaries - ColorSpace::Smpte170m => (6, 6, 6, 1), // NTSC SD: SMPTE 170M / BT.601-525 - ColorSpace::Unknown => (0, 0, 0, 0), - }; - // Override transfer for non-PQ HDR - let transfer = match v.hdr { - HdrFormat::Hdr10 | HdrFormat::Hdr10Plus | HdrFormat::DolbyVision => 16, // PQ - HdrFormat::Hlg => 18, - _ => transfer, + // CICP (matrix, transfer, primaries, range) — ITU-T H.273 code points. + // + // Prefer MEASURED CICP read from the bitstream (HEVC/H.264 VUI + // colour_description or MPEG-2 sequence_display_extension) when the + // stream states it: those are authoritative. Fall back to the coarse + // `color_space` enum (a playlist nibble / PAL-NTSC guess) only when no + // measured triplet is present, so the container stops ASSUMING a colour + // space the bitstream may contradict. + let (matrix, transfer, primaries, range) = match v.measured_cicp { + Some(c) => (c.matrix, c.transfer, c.primaries, c.range), + None => { + let (m, t, p, r) = match v.color_space { + ColorSpace::Bt2020 => ( + CICP_MATRIX_BT2020NC, + CICP_TRANSFER_PQ, + CICP_PRIMARIES_BT2020, + COLOUR_RANGE_LIMITED, + ), + ColorSpace::Bt709 => ( + CICP_MATRIX_BT709, + CICP_TRANSFER_BT709, + CICP_PRIMARIES_BT709, + COLOUR_RANGE_LIMITED, + ), + // PAL SD: BT.470 System B/G matrix/transfer/primaries. + ColorSpace::Bt470bg => ( + CICP_MATRIX_BT470BG, + CICP_TRANSFER_BT470BG, + CICP_PRIMARIES_BT470BG, + COLOUR_RANGE_LIMITED, + ), + // NTSC SD: SMPTE 170M / BT.601-525. + ColorSpace::Smpte170m => ( + CICP_MATRIX_BT601_525, + CICP_TRANSFER_BT601_525, + CICP_PRIMARIES_BT601_525, + COLOUR_RANGE_LIMITED, + ), + ColorSpace::Unknown => (0, 0, 0, 0), + }; + // Override the transfer for non-PQ HDR signalled by the HdrFormat + // (the enum can't express HLG). Only applies on the enum + // fallback; a measured CICP already carries the real transfer. + let t = match v.hdr { + HdrFormat::Hdr10 | HdrFormat::Hdr10Plus | HdrFormat::DolbyVision => { + CICP_TRANSFER_PQ + } + HdrFormat::Hlg => CICP_TRANSFER_HLG, + _ => t, + }; + (m, t, p, r) + } }; // Display dimensions. For square-pixel video (HD/UHD/BD) the display // aspect equals the pixel grid, so display == pixel. For anamorphic @@ -135,14 +222,21 @@ impl MkvTrack { colour_primaries: primaries, colour_range: range, interlaced: v.resolution.is_interlaced(), - // PAL DVD (576i), NTSC DVD (480i), and HD interlaced (1080i) are - // all top-field-first ("almost everything but DV is TFF"). MediaInfo - // reads "Top Field First" off the MPEG-2 picture coding extension, - // so the container element must agree — emitting BFF here for 576i - // (the pre-rc.5.1 value) was a wrong container value that disagreed - // with the stream. Progressive content leaves the order undetermined. + // FieldOrder (Matroska 0x9D) MUST agree with the elementary stream's + // interlace signalling. Derive it from the MEASURED `top_field_first` + // when the bitstream stated it: Some(true) → TFF, Some(false) → BFF. + // Genuinely bottom-field-first content (rare, but real) was previously + // mis-stamped TFF because the muxer hardcoded TFF for ALL interlaced + // streams. When the flag is NOT measured (`None`), fall back to TFF — + // PAL DVD (576i), NTSC DVD (480i) and HD (1080i) are overwhelmingly + // top-field-first ("almost everything but DV is TFF"). Progressive + // content leaves the order undetermined (the element is omitted). field_order: if v.resolution.is_interlaced() { - ebml::FIELD_ORDER_TFF + match v.top_field_first { + Some(true) => ebml::FIELD_ORDER_TFF, + Some(false) => ebml::FIELD_ORDER_BFF, + None => ebml::FIELD_ORDER_TFF, + } } else { ebml::FIELD_ORDER_UNDETERMINED }, @@ -334,6 +428,11 @@ pub struct MkvMuxer { /// silently empty file. See `write_frame` for the track-0 invariant. dropped_pre_cluster: u64, seek_fixups: Vec, + /// Absolute file offset of the CUES SeekHead entry (a fixed 21-byte Seek + /// element). When `finish()` writes no Cues element (zero cue points), this + /// entry is overwritten with a Void so the SeekHead carries no pointer to a + /// non-existent / wrong element. + cues_seek_entry_pos: Option, info_offset: u64, tracks_offset: u64, chapters_offset: Option, @@ -521,7 +620,17 @@ impl MkvMuxer { let seek_id_be = (ebml::SEEK as u16).to_be_bytes(); let seek_inner_id_be = (ebml::SEEK_ID as u16).to_be_bytes(); let seek_pos_id_be = (ebml::SEEK_POSITION as u16).to_be_bytes(); + // Absolute file offset where the CUES Seek entry begins, so that — if no + // Cues element is ultimately written (zero cue points) — the entry can be + // overwritten with a Void at finish() instead of leaving a SeekHead + // pointer that resolves to whatever element (Tags / EOF) happens to land + // at the Cues offset. See `cues_seek_entry_pos` / `finish`. + let mut cues_seek_entry_pos: Option = None; for target_id in &targets { + let entry_pos = writer.stream_position()?; + if *target_id == ebml::CUES { + cues_seek_entry_pos = Some(entry_pos); + } writer.write_all(&[seek_id_be[0], seek_id_be[1], 0x92])?; writer.write_all(&[seek_inner_id_be[0], seek_inner_id_be[1], 0x84])?; writer.write_all(&target_id.to_be_bytes())?; @@ -631,8 +740,9 @@ impl MkvMuxer { ebml::write_uint(&mut writer, ebml::PIXEL_WIDTH, track.pixel_width as u64)?; ebml::write_uint(&mut writer, ebml::PIXEL_HEIGHT, track.pixel_height as u64)?; // Scan type. FlagInterlaced: 1 = interlaced, 2 = progressive. - // FieldOrder is only written for interlaced content with a - // determined order (TFF=0/2/6/14, BFF=1/9/13...). + // FieldOrder (0x9D) is only written for interlaced content with a + // determined order: TFF = 1, BFF = 6, 0 = progressive, and the + // element is omitted entirely when undetermined (RFC 9559). ebml::write_uint( &mut writer, ebml::FLAG_INTERLACED, @@ -679,7 +789,7 @@ impl MkvMuxer { if let Some(ref dvcc) = track.dv_config { let map_pos = ebml::start_master(&mut writer, ebml::BLOCK_ADDITION_MAPPING)?; // BlockAddIDType = "dvcC" fourcc (DOVIDecoderConfigurationRecord). - ebml::write_uint(&mut writer, ebml::BLOCK_ADD_ID_TYPE, 0x6476_6343)?; + ebml::write_uint(&mut writer, ebml::BLOCK_ADD_ID_TYPE, BLOCK_ADD_ID_TYPE_DVCC)?; ebml::write_binary(&mut writer, ebml::BLOCK_ADD_ID_EXTRA_DATA, dvcc)?; ebml::end_master(&mut writer, map_pos)?; } @@ -691,18 +801,24 @@ impl MkvMuxer { // Omit Channels when unknown (0) — Matroska defaults it to 1 // rather than us fabricating a 6-channel count. if track.channels > 0 { - // Record the offset of the 1-byte Channels value so an AC-3 - // track can correct it from the bitstream acmod on its first - // frame (the IFO nibble is unreliable). write_uint emits - // ID(0x9F, 1B) + size(0x81, 1B) + value(1B) for 1..=255, so - // the value byte sits 2 bytes after the element start. - let chan_elem_pos = writer.stream_position()?; - ebml::write_uint(&mut writer, ebml::CHANNELS, track.channels as u64)?; + // Capture the ACTUAL file offset of the 1-byte Channels value + // so an AC-3 track can correct it from the bitstream acmod on + // its first frame (the IFO nibble is unreliable). Rather than + // assume write_uint's encoding (ID + size widths), write the + // element's ID and size explicitly, then record the position + // immediately before the value byte. Channels is 1..=255 so + // the value is exactly one byte (Size = 1), and the acmod + // correction is likewise 1..=255 — the width never changes, so + // the in-place single-byte rewrite stays valid. + ebml::write_id(&mut writer, ebml::CHANNELS)?; + ebml::write_size(&mut writer, 1)?; + let value_offset = writer.stream_position()?; + writer.write_all(&[track.channels])?; if track.codec_id == ebml::CODEC_AC3 { ac3_channel_fixups.insert( i, Ac3ChannelFixup { - value_offset: chan_elem_pos + 2, + value_offset, claimed: track.channels, corrected: false, }, @@ -759,6 +875,7 @@ impl MkvMuxer { frame_count: 0, dropped_pre_cluster: 0, seek_fixups, + cues_seek_entry_pos, info_offset, tracks_offset, chapters_offset, @@ -1014,6 +1131,7 @@ impl MkvMuxer { // Write Cues let cues_start = self.writer.stream_position()?; let cues_offset = cues_start - self.segment_start; + let have_cues = !self.cues.is_empty(); if !self.cues.is_empty() { let cues_pos = ebml::start_master(&mut self.writer, ebml::CUES)?; for cue in &self.cues { @@ -1038,8 +1156,16 @@ impl MkvMuxer { // bitrate for EVERY track this way, not just CBR audio. self.write_bps_tags()?; - // Back-patch SeekHead SeekPosition values now that all element offsets are known. + // Back-patch SeekHead SeekPosition values now that all element offsets + // are known. When no Cues element was written (zero cue points), the + // CUES entry's SeekPosition would otherwise be back-patched to + // `cues_offset`, which now holds Tags / EOF — a dangling pointer to a + // non-Cues element. Skip that fixup and instead Void the whole CUES Seek + // entry (below) so the SeekHead carries no false pointer. for fixup in &self.seek_fixups { + if fixup.target_id == ebml::CUES && !have_cues { + continue; + } let offset = match fixup.target_id { ebml::INFO => self.info_offset, ebml::TRACKS => self.tracks_offset, @@ -1053,6 +1179,21 @@ impl MkvMuxer { .seek(std::io::SeekFrom::Start(fixup.value_offset))?; self.writer.write_all(&offset.to_be_bytes())?; } + // Neutralise the unused CUES Seek entry. The entry is a fixed 21-byte + // Seek master: SEEK(2 ID + 1 size) + SEEK_ID(2+1) + 4-byte target id + + // SEEK_POSITION(2+1) + 8-byte value = 21 bytes. A Void (0xEC, 1-byte ID) + // with a 1-byte size VINT covering the remaining 19 bytes occupies + // exactly 1 + 1 + 19 = 21 bytes, overwriting the entry in place without + // shifting any following element. + if !have_cues { + if let Some(entry_pos) = self.cues_seek_entry_pos { + self.writer.seek(std::io::SeekFrom::Start(entry_pos))?; + ebml::write_id(&mut self.writer, ebml::VOID)?; + // 19 = 21-byte entry minus the Void ID (1) and size (1) bytes. + ebml::write_size(&mut self.writer, 19)?; + self.writer.write_all(&[0u8; 19])?; + } + } self.writer.seek(std::io::SeekFrom::End(0))?; self.writer.flush()?; @@ -1206,6 +1347,8 @@ mod tests { display_aspect: Some((16, 9)), secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, }; let t = MkvTrack::video(&base); assert_eq!((t.pixel_width, t.pixel_height), (720, 576)); @@ -1227,6 +1370,113 @@ mod tests { ); } + /// FieldOrder must follow the MEASURED top_field_first, not a hardcoded TFF. + /// An interlaced stream whose parsed `top_field_first == Some(false)` tags + /// BFF (=6); `Some(true)` and `None` (unknown, the fallback) tag TFF (=1). + #[test] + fn interlaced_field_order_from_measured_tff() { + let base = VideoStream { + pid: 0xE0, + codec: Codec::Mpeg2, + resolution: Resolution::R576i, // interlaced + frame_rate: crate::disc::FrameRate::F25, + hdr: HdrFormat::Sdr, + color_space: ColorSpace::Bt470bg, + display_aspect: None, + secondary: false, + label: String::new(), + top_field_first: None, + measured_cicp: None, + }; + + // Measured bottom-field-first → BFF, NOT the old hardcoded TFF. + let bff = VideoStream { + top_field_first: Some(false), + ..base.clone() + }; + assert_eq!( + MkvTrack::video(&bff).field_order, + ebml::FIELD_ORDER_BFF, + "measured top_field_first=false must tag BFF (6), not TFF" + ); + + // Measured top-field-first → TFF. + let tff = VideoStream { + top_field_first: Some(true), + ..base.clone() + }; + assert_eq!(MkvTrack::video(&tff).field_order, ebml::FIELD_ORDER_TFF); + + // Unknown (not measured) → TFF fallback (dominant DVD/HD case). + assert_eq!(MkvTrack::video(&base).field_order, ebml::FIELD_ORDER_TFF); + + // Progressive content leaves the order undetermined regardless of flag. + let prog = VideoStream { + resolution: Resolution::R1080p, + top_field_first: Some(false), + ..base + }; + assert_eq!( + MkvTrack::video(&prog).field_order, + ebml::FIELD_ORDER_UNDETERMINED, + "progressive video never carries a field order" + ); + } + + /// Measured CICP from the bitstream must take precedence over the coarse + /// `color_space` enum. A BT.2020/PQ enum that would otherwise produce + /// (9,16,9) is overridden by a measured BT.709 triplet when present. + #[test] + fn measured_cicp_overrides_color_space_enum() { + let base = VideoStream { + pid: 0xE0, + codec: Codec::Hevc, + resolution: Resolution::R2160p, + frame_rate: crate::disc::FrameRate::F24, + hdr: HdrFormat::Hdr10, // enum/HDR path would force PQ transfer + color_space: ColorSpace::Bt2020, + display_aspect: None, + secondary: false, + label: String::new(), + top_field_first: None, + measured_cicp: None, + }; + + // Without a measured triplet: enum + HDR → BT.2020nc / PQ / BT.2020. + let t_enum = MkvTrack::video(&base); + assert_eq!( + ( + t_enum.colour_matrix, + t_enum.colour_transfer, + t_enum.colour_primaries + ), + ( + CICP_MATRIX_BT2020NC, + CICP_TRANSFER_PQ, + CICP_PRIMARIES_BT2020 + ), + "enum fallback derives BT.2020/PQ" + ); + + // With a measured BT.709 triplet: the bitstream's value wins outright, + // INCLUDING the transfer (the HDR override does not apply to measured). + let measured = VideoStream { + measured_cicp: Some(crate::disc::MeasuredCicp { + matrix: CICP_MATRIX_BT709, + transfer: CICP_TRANSFER_BT709, + primaries: CICP_PRIMARIES_BT709, + range: COLOUR_RANGE_LIMITED, + }), + ..base + }; + let t = MkvTrack::video(&measured); + assert_eq!( + (t.colour_matrix, t.colour_transfer, t.colour_primaries), + (CICP_MATRIX_BT709, CICP_TRANSFER_BT709, CICP_PRIMARIES_BT709), + "measured CICP must override the enum, transfer included" + ); + } + /// Helper: search for a 4-byte big-endian EBML ID in a byte slice. fn find_id(data: &[u8], id: u32) -> Option { let bytes = id.to_be_bytes(); @@ -1411,6 +1661,54 @@ mod tests { ); } + /// When no Cues element is written (zero cue points), the SeekHead must NOT + /// retain a CUES entry that back-patches to the Cues offset — that offset now + /// holds Tags / EOF, a dangling pointer to a non-Cues element. finish() Voids + /// the unused CUES Seek entry instead. (The empty-cues case is defensive — + /// the normal path pushes a cue with every cluster — so the test clears the + /// cue list directly before finalizing.) + #[test] + fn zero_cues_voids_seekhead_entry_no_dangling_pointer() { + use std::sync::{Arc, Mutex}; + + let shared = Arc::new(Mutex::new(Cursor::new(Vec::new()))); + let writer = SharedWriter(shared.clone()); + 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) + .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"); + muxer.cues.clear(); + muxer.finish().unwrap(); + + let data = shared.lock().unwrap().clone().into_inner(); + + // No Cues element is written. + assert!( + find_id(&data, ebml::CUES).is_none(), + "no Cues element expected when there are zero cue points" + ); + // The recorded CUES Seek entry was overwritten with a Void (0xEC) of the + // remaining 19 bytes — it no longer begins a SEEK (0x4DBB) element. + let entry = &data[cues_entry_pos as usize..cues_entry_pos as usize + 2]; + assert_eq!( + entry, + &[ebml::VOID as u8, 0x80 | 19], + "CUES Seek entry must be Void(19), not a live Seek pointer" + ); + + // Defensively confirm no Seek entry's SeekPosition resolves to the (now + // Tags/EOF) cues offset: scan all 8-byte SeekPosition values in the + // SeekHead and ensure none equals the offset where Cues would have been. + // (Sanity: the file must still parse its real elements.) + assert!( + find_id(&data, ebml::INFO).is_some() && find_id(&data, ebml::TRACKS).is_some(), + "Info and Tracks must still be present and seekable" + ); + } + #[test] fn monotonic_ts_forces_strictly_increasing() { // First frame passes through unchanged. @@ -3085,6 +3383,8 @@ mod tests { display_aspect: Some((16, 9)), secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, }; let t = MkvTrack::video(&v); assert!(t.interlaced, "576i is interlaced"); @@ -3116,6 +3416,8 @@ mod tests { display_aspect: None, secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, }; let t = MkvTrack::video(&v); assert_eq!(t.default_duration_ns, 40_000_000, "frame duration is 40 ms"); @@ -3259,6 +3561,8 @@ mod tests { display_aspect: Some((16, 9)), secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, }; let t = MkvTrack::video(&v); assert_eq!( @@ -3318,6 +3622,8 @@ mod tests { display_aspect: Some((4, 3)), secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, }; let t = MkvTrack::video(&v); assert_eq!( @@ -3378,6 +3684,8 @@ mod tests { display_aspect: Some((4, 3)), secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, }; let t = MkvTrack::video(&v); assert!(t.interlaced, "480i is interlaced"); diff --git a/src/mux/mkvstream.rs b/src/mux/mkvstream.rs index 5a86001..c93fdb4 100644 --- a/src/mux/mkvstream.rs +++ b/src/mux/mkvstream.rs @@ -614,6 +614,8 @@ fn parse_track( display_aspect: None, secondary: is_secondary, label: name, + top_field_first: None, + measured_cicp: None, })) } 2 => Some(crate::disc::Stream::Audio(AudioStream { diff --git a/src/mux/network.rs b/src/mux/network.rs index fd12aea..a61336a 100644 --- a/src/mux/network.rs +++ b/src/mux/network.rs @@ -355,6 +355,8 @@ mod tests { display_aspect: None, secondary: false, label: "Main".into(), + top_field_first: None, + measured_cicp: None, }), Stream::Audio(AudioStream { pid: 0x1100, diff --git a/src/mux/pipelined_stream.rs b/src/mux/pipelined_stream.rs index 27f9ec0..928212a 100644 --- a/src/mux/pipelined_stream.rs +++ b/src/mux/pipelined_stream.rs @@ -640,6 +640,8 @@ mod tests { display_aspect: None, secondary, label: String::new(), + top_field_first: None, + measured_cicp: None, })); t } @@ -797,6 +799,8 @@ mod tests { display_aspect: Some((4, 3)), secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, })); let parsers: Vec<(u16, Box)> = vec![(crate::mux::ps::DVD_VIDEO_PID, Box::new(Mpeg2Parser::new()))]; diff --git a/src/mux/stdio.rs b/src/mux/stdio.rs index 1db0189..2e829df 100644 --- a/src/mux/stdio.rs +++ b/src/mux/stdio.rs @@ -159,6 +159,8 @@ mod tests { display_aspect: None, secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, })); // Index 0 = the video stream's codec init data. t.codec_privates = vec![Some(vec![0xDE, 0xAD, 0xBE, 0xEF])]; diff --git a/src/mux/ts.rs b/src/mux/ts.rs index eec60e0..3cb2c8d 100644 --- a/src/mux/ts.rs +++ b/src/mux/ts.rs @@ -694,6 +694,8 @@ pub fn scan_streams(data: &[u8]) -> Option> { display_aspect: None, secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, })) } CodecKind::Audio => Some(Stream::Audio(AudioStream { diff --git a/tests/disc_tests.rs b/tests/disc_tests.rs index 3222c31..294d380 100644 --- a/tests/disc_tests.rs +++ b/tests/disc_tests.rs @@ -169,6 +169,8 @@ fn title_with_video( display_aspect: None, secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, })], chapters: Vec::new(), extents: Vec::new(), diff --git a/tests/streams.rs b/tests/streams.rs index 9af490c..7bba351 100644 --- a/tests/streams.rs +++ b/tests/streams.rs @@ -23,6 +23,8 @@ fn sample_disc_title() -> DiscTitle { display_aspect: None, secondary: false, label: "Main".into(), + top_field_first: None, + measured_cicp: None, }), Stream::Audio(AudioStream { pid: 0x1100, @@ -401,6 +403,8 @@ fn meta_codec_roundtrip() { display_aspect: None, secondary: false, label: String::new(), + top_field_first: None, + measured_cicp: None, })); } for (i, &codec) in codecs_audio.iter().enumerate() { @@ -503,6 +507,8 @@ fn meta_all_stream_types() { display_aspect: None, secondary: false, label: "Primary".into(), + top_field_first: None, + measured_cicp: None, }), Stream::Audio(AudioStream { pid: 0x1100, @@ -688,6 +694,8 @@ fn mkvstream_meta_preserves_all_streams() { display_aspect: None, secondary: false, label: "Main Video".into(), + top_field_first: None, + measured_cicp: None, }), Stream::Audio(AudioStream { pid: 0x1100, @@ -787,6 +795,8 @@ fn mkvstream_e2e_h264_produces_valid_mkv() { display_aspect: None, secondary: false, label: "Main".into(), + top_field_first: None, + measured_cicp: None, })], chapters: Vec::new(), extents: Vec::new(),