DVD: correct PAL/NTSC, anamorphic aspect, and SD colour

Fix three DVD video-attribute bugs surfaced by a PAL disc detected as
NTSC:

- PAL/NTSC: parse video_format from VTS_V_ATR bits 5-4, not bits 1-0
  (the old mask read permitted_df, so PAL 576i/25fps was mis-detected
  as NTSC 480i/29.97). Named consts replace the magic bit positions.
- Anamorphic aspect: write MKV DisplayWidth/Height from the disc's
  display_aspect (16:9 720x576 -> 1024x576) instead of square pixels,
  so 16:9 DVDs no longer render as 4:3.
- Colour: stamp SD colorimetry (PAL=BT.470BG, NTSC=SMPTE-170M) instead
  of BT.709 (HD).

Adds VideoStream.display_aspect (threaded through every muxer) plus
TvSystem/DvdAspect/ColorSpace plumbing, with regression tests. Removes
the deprecated Disc mux set_halt bridge (use with_halt).
This commit is contained in:
Matthew Jackson
2026-06-23 15:38:49 -07:00
parent 705857f117
commit e96528ad5b
17 changed files with 279 additions and 78 deletions
+14 -31
View File
@@ -11,7 +11,9 @@ use crate::event::{BatchSizeReason, Event, EventKind};
use crate::halt::Halt;
use crate::sector::{DecryptingSectorSource, SectorSource};
use std::io;
#[cfg(test)]
use std::sync::Arc;
#[cfg(test)]
use std::sync::atomic::AtomicBool;
/// Ramp back up to the preferred batch size after this many sectors
@@ -149,9 +151,8 @@ pub struct DiscStream {
/// When set and the token is cancelled, fill_extents returns Err(Halted)
/// at the next retry boundary. Unlike skip_errors, this propagates the
/// error up so the rip terminates cleanly. Construct with
/// [`DiscStream::with_halt`] (preferred) or set post-hoc via the
/// deprecated [`DiscStream::set_halt`] bridge — both populate this same
/// field and either entry point yields one source of truth.
/// [`DiscStream::with_halt`], passing the same `Halt` clone handed to
/// sweep / patch / mux so every phase observes one Stop signal.
halt: Option<Halt>,
event_fn: Option<Box<dyn Fn(Event) + Send>>,
eof: bool,
@@ -296,29 +297,13 @@ impl DiscStream {
/// during dense bad-sector regions (where the outer PES read() loop
/// can spend minutes inside fill_extents before emitting a frame).
///
/// Preferred over the post-hoc [`DiscStream::set_halt`] bridge —
/// pass the same `Halt` clone you hand to sweep / patch / mux so
/// every phase observes a single Stop signal.
/// Pass the same `Halt` clone you hand to sweep / patch / mux so every
/// phase observes a single Stop signal.
pub fn with_halt(mut self, halt: Halt) -> Self {
self.halt = Some(halt);
self
}
/// Bridge for callers that haven't migrated to the
/// [`DiscStream::with_halt`] constructor-time path yet. Wraps the
/// supplied `Arc<AtomicBool>` as a [`Halt`] (`Halt::from_arc`) and
/// stores it in the same internal slot, so a halt installed via
/// either entry point goes through one halt-check inside
/// `fill_extents`. Calling `set_halt` after `with_halt` (or vice
/// versa) replaces the previous token with the new one.
#[deprecated(
since = "1.0.0",
note = "use `DiscStream::with_halt(Halt)` at construction instead"
)]
pub fn set_halt(&mut self, flag: Arc<AtomicBool>) {
self.halt = Some(Halt::from_arc(flag));
}
fn is_halted(&self) -> bool {
self.halt
.as_ref()
@@ -918,11 +903,9 @@ mod tests {
assert_eq!(frames, 0);
}
/// `is_halted()` must observe a cancellation signal regardless of
/// which entry point installed the token. The deprecated
/// `set_halt(Arc<AtomicBool>)` and the new `with_halt(Halt)` are
/// two views over one slot — flipping either bit must cause the
/// next `fill_extents` retry boundary to bail.
/// `is_halted()` must observe a cancellation signal installed via
/// `with_halt(Halt)` — flipping the token must cause the next
/// `fill_extents` retry boundary to bail.
#[test]
fn halt_via_with_halt_observed_by_is_halted() {
let halt = Halt::new();
@@ -1212,21 +1195,21 @@ mod tests {
}
#[test]
fn halt_via_set_halt_bridge_observed_by_is_halted() {
fn halt_via_with_halt_from_arc_observed_by_is_halted() {
let arc = Arc::new(AtomicBool::new(false));
let mut stream = DiscStream::new(
let stream = DiscStream::new(
Box::new(ZeroReader { capacity: 8 }),
synthetic_title(8),
crate::decrypt::DecryptKeys::None,
8,
crate::disc::ContentFormat::BdTs,
);
stream.set_halt(arc.clone());
)
.with_halt(Halt::from_arc(arc.clone()));
assert!(!stream.is_halted());
arc.store(true, std::sync::atomic::Ordering::Relaxed);
assert!(
stream.is_halted(),
"set_halt(Arc<AtomicBool>) bridge must observe Arc-side flips"
"with_halt(Halt::from_arc) must observe Arc-side flips"
);
}
}
+1
View File
@@ -113,6 +113,7 @@ mod tests {
frame_rate: FrameRate::F24,
hdr: HdrFormat::Sdr,
color_space: ColorSpace::Bt709,
display_aspect: None,
secondary: false,
label: String::new(),
})],
+3
View File
@@ -209,6 +209,7 @@ impl M2tsMeta {
.unwrap_or(crate::disc::FrameRate::Unknown),
hdr: hdr_fmt,
color_space: cs,
display_aspect: None,
secondary: *secondary,
label: label.clone(),
})
@@ -403,6 +404,7 @@ mod tests {
frame_rate: FrameRate::F23_976,
hdr,
color_space: cs,
display_aspect: None,
secondary: false,
label: String::new(),
}));
@@ -651,6 +653,7 @@ mod tests {
frame_rate: FrameRate::F23_976,
hdr: HdrFormat::Hdr10,
color_space: ColorSpace::Bt2020,
display_aspect: None,
secondary: false,
label: "x".into(),
}));
+53 -2
View File
@@ -83,9 +83,12 @@ 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
@@ -94,6 +97,17 @@ impl MkvTrack {
HdrFormat::Hlg => 18,
_ => transfer,
};
// Display dimensions. For square-pixel video (HD/UHD/BD) the display
// aspect equals the pixel grid, so display == pixel. For anamorphic
// content (DVD: 720x480/576 pixels shown as 16:9 or 4:3) the coded
// pixels are NOT square — keep the coded height and derive the width so
// DisplayWidth:DisplayHeight carries the intended DAR (e.g. 720x576
// 16:9 → 1024x576). Without this, players use the square-pixel ratio
// and show the disc as 5:4 / 3:2 instead of 16:9.
let (display_width, display_height) = match v.display_aspect {
Some((an, ad)) if an > 0 && ad > 0 && h > 0 => ((h * an + ad / 2) / ad, h),
_ => (w, h),
};
Self {
track_type: ebml::TRACK_TYPE_VIDEO,
codec_id,
@@ -105,8 +119,8 @@ impl MkvTrack {
pixel_width: w,
pixel_height: h,
default_duration_ns,
display_width: w,
display_height: h,
display_width,
display_height,
colour_matrix: matrix,
colour_transfer: transfer,
colour_primaries: primaries,
@@ -1075,6 +1089,43 @@ mod tests {
use super::*;
use std::io::Cursor;
/// Anamorphic DVD: a 720x576 (R576i) PAL stream flagged 16:9 must write a
/// DisplayWidth/Height carrying the 16:9 DAR (1024x576), NOT the square-pixel
/// 720x576 (which players show as ~5:4). Square-pixel video
/// (`display_aspect == None`) keeps display == pixel.
#[test]
fn video_track_anamorphic_display_aspect() {
let base = VideoStream {
pid: 0xE0,
codec: Codec::Mpeg2,
resolution: Resolution::R576i,
frame_rate: crate::disc::FrameRate::F25,
hdr: HdrFormat::Sdr,
color_space: ColorSpace::Bt709,
display_aspect: Some((16, 9)),
secondary: false,
label: String::new(),
};
let t = MkvTrack::video(&base);
assert_eq!((t.pixel_width, t.pixel_height), (720, 576));
assert_eq!(
(t.display_width, t.display_height),
(1024, 576),
"16:9 anamorphic must emit a 16:9 DAR, not square-pixel 720x576"
);
let square = VideoStream {
display_aspect: None,
..base
};
let t2 = MkvTrack::video(&square);
assert_eq!(
(t2.display_width, t2.display_height),
(720, 576),
"square pixels: display == pixel"
);
}
/// Helper: search for a 4-byte big-endian EBML ID in a byte slice.
fn find_id(data: &[u8], id: u32) -> Option<usize> {
let bytes = id.to_be_bytes();
+3
View File
@@ -582,6 +582,9 @@ fn parse_track(
frame_rate: FrameRate::Unknown,
hdr: HdrFormat::Sdr,
color_space: ColorSpace::Bt709,
// Remux input: the source MKV's DisplayWidth/Height is preserved
// by the writer separately; nothing anamorphic to reconstruct here.
display_aspect: None,
secondary: is_secondary,
label: name,
}))
+1
View File
@@ -352,6 +352,7 @@ mod tests {
frame_rate: FrameRate::F23_976,
hdr: HdrFormat::Hdr10,
color_space: ColorSpace::Bt2020,
display_aspect: None,
secondary: false,
label: "Main".into(),
}),
+1
View File
@@ -618,6 +618,7 @@ mod tests {
frame_rate: FrameRate::F23_976,
hdr: HdrFormat::Hdr10,
color_space: ColorSpace::Bt2020,
display_aspect: None,
secondary,
label: String::new(),
}));
+1
View File
@@ -156,6 +156,7 @@ mod tests {
frame_rate: crate::disc::FrameRate::F23_976,
hdr: crate::disc::HdrFormat::Hdr10,
color_space: crate::disc::ColorSpace::Bt2020,
display_aspect: None,
secondary: false,
label: String::new(),
}));
+2
View File
@@ -690,6 +690,8 @@ pub fn scan_streams(data: &[u8]) -> Option<Vec<crate::disc::Stream>> {
frame_rate: FrameRate::Unknown,
hdr: HdrFormat::Sdr,
color_space: ColorSpace::Bt709,
// TS is a passthrough container — aspect stays in the ES.
display_aspect: None,
secondary: false,
label: String::new(),
}))