diff --git a/src/disc/dvd.rs b/src/disc/dvd.rs index 7dd6d4d..1b5af2a 100644 --- a/src/disc/dvd.rs +++ b/src/disc/dvd.rs @@ -179,7 +179,10 @@ impl Disc { // 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(); + // format_palette guards on (0, 0) and omits its `size:` line, + // so an unresolved resolution degrades to a palette-only .idx + // rather than one claiming a 0x0 frame. + let (vid_w, vid_h) = ts.video.resolution.pixels().unwrap_or((0, 0)); let codec_data = dvd_title .palette .as_ref() diff --git a/src/disc/mod.rs b/src/disc/mod.rs index 687249b..9f73662 100644 --- a/src/disc/mod.rs +++ b/src/disc/mod.rs @@ -945,15 +945,34 @@ impl Resolution { /// handles it: the Matroska sink omits the optional PixelWidth/PixelHeight, /// the VobSub `.idx` writer omits its `size:` line, and no caller divides by /// either dimension. - pub fn pixels(&self) -> (u32, u32) { + /// `None` when the resolution never resolved. + /// + /// Returns an `Option` rather than a sentinel because every caller has to + /// make a DIFFERENT decision and the compiler is the only thing that + /// reliably makes them: Matroska omits the optional PixelWidth/PixelHeight + /// elements, the metadata sinks report the dimensions as absent, and MP4 + /// cannot do either — ISO/IEC 14496-12 makes width and height MANDATORY in + /// both `tkhd` (8.3.2) and VisualSampleEntry (12.1.3), so it must refuse. + /// + /// This returned `(0, 0)` before, and that is the shape the sentinel + /// creates: `(0, 0)` looks like a usable pair, so the MP4 sink stored it + /// and serialised a 0x0 video track — a structurally complete file that no + /// player can render, written with no error. It returned a fabricated + /// 1920x1080 before that, which was wrong but at least playable, so the + /// sink had never needed a guard and the absence of one was invisible. + /// + /// A doc comment listing which callers are safe cannot hold this: one did + /// not check, and a third kept its own duplicate `Unknown` test long after + /// the accessor took the job over. + pub fn pixels(&self) -> Option<(u32, u32)> { match self { - Resolution::R480i | Resolution::R480p => (720, 480), - Resolution::R576i | Resolution::R576p => (720, 576), - Resolution::R720p => (1280, 720), - Resolution::R1080i | Resolution::R1080p => (1920, 1080), - Resolution::R2160p => (3840, 2160), - Resolution::R4320p => (7680, 4320), - Resolution::Unknown => (0, 0), + Resolution::R480i | Resolution::R480p => Some((720, 480)), + Resolution::R576i | Resolution::R576p => Some((720, 576)), + Resolution::R720p => Some((1280, 720)), + Resolution::R1080i | Resolution::R1080p => Some((1920, 1080)), + Resolution::R2160p => Some((3840, 2160)), + Resolution::R4320p => Some((7680, 4320)), + Resolution::Unknown => None, } } @@ -3273,18 +3292,18 @@ mod tests { fn unknown_resolution_reports_no_pixel_dimensions() { assert_eq!( Resolution::Unknown.pixels(), - (0, 0), + None, "an unknown resolution must report no dimensions, not a fabricated default" ); - assert_eq!(Resolution::R480i.pixels(), (720, 480)); - assert_eq!(Resolution::R480p.pixels(), (720, 480)); - assert_eq!(Resolution::R576i.pixels(), (720, 576)); - assert_eq!(Resolution::R576p.pixels(), (720, 576)); - assert_eq!(Resolution::R720p.pixels(), (1280, 720)); - assert_eq!(Resolution::R1080i.pixels(), (1920, 1080)); - assert_eq!(Resolution::R1080p.pixels(), (1920, 1080)); - assert_eq!(Resolution::R2160p.pixels(), (3840, 2160)); - assert_eq!(Resolution::R4320p.pixels(), (7680, 4320)); + assert_eq!(Resolution::R480i.pixels(), Some((720, 480))); + assert_eq!(Resolution::R480p.pixels(), Some((720, 480))); + assert_eq!(Resolution::R576i.pixels(), Some((720, 576))); + assert_eq!(Resolution::R576p.pixels(), Some((720, 576))); + assert_eq!(Resolution::R720p.pixels(), Some((1280, 720))); + assert_eq!(Resolution::R1080i.pixels(), Some((1920, 1080))); + assert_eq!(Resolution::R1080p.pixels(), Some((1920, 1080))); + assert_eq!(Resolution::R2160p.pixels(), Some((3840, 2160))); + assert_eq!(Resolution::R4320p.pixels(), Some((7680, 4320))); } /// Every `Unknown` variant that exposes a numeric accessor must report @@ -3293,7 +3312,11 @@ mod tests { /// than 0 fps, because callers divide by the numerator. #[test] fn no_unknown_variant_fabricates_a_numeric_value() { - assert_eq!(Resolution::Unknown.pixels(), (0, 0)); + assert_eq!( + Resolution::Unknown.pixels(), + None, + "the strongest form of this rule: not even a zero pair, which reads as a usable value and was serialised into an MP4 as a 0x0 track" + ); assert_eq!(FrameRate::Unknown.as_fraction(), (0, 1)); assert_eq!(AudioChannels::Unknown.count(), 0); assert_eq!(SampleRate::Unknown.hz(), 0.0); diff --git a/src/error.rs b/src/error.rs index 450400d..5642d74 100644 --- a/src/error.rs +++ b/src/error.rs @@ -207,6 +207,12 @@ pub const E_MP4_INVALID: u16 = 9049; /// `mp4://` video track is missing its codec-configuration record /// (`hvcC`/`avcC`), without which the sample entry can't be written. pub const E_MP4_MISSING_CODEC_PRIVATE: u16 = 9050; +/// `mp4://` video track has no resolved frame dimensions. ISO/IEC 14496-12 +/// makes width and height mandatory in both `tkhd` (8.3.2) and +/// VisualSampleEntry (12.1.3), so unlike Matroska there is no element to omit: +/// the sink would have to write 0x0, producing a structurally complete file no +/// player can render. Refuse instead. +pub const E_MP4_UNKNOWN_RESOLUTION: u16 = 9055; /// READ CAPACITY returned a short or overflowing transfer. pub const E_DISC_CAPACITY_MALFORMED: u16 = 9047; @@ -548,6 +554,9 @@ pub enum Error { Mp4Invalid, /// `mp4://` video track is missing its `hvcC`/`avcC` configuration record. Mp4MissingCodecPrivate, + /// `mp4://` video track has no resolved frame dimensions. See + /// [`E_MP4_UNKNOWN_RESOLUTION`]. + Mp4UnknownResolution, PesFrameTooLarge { size: usize, }, @@ -737,6 +746,7 @@ impl Error { Error::Mp4NoVideoTrack => E_MP4_NO_VIDEO_TRACK, Error::Mp4Invalid => E_MP4_INVALID, Error::Mp4MissingCodecPrivate => E_MP4_MISSING_CODEC_PRIVATE, + Error::Mp4UnknownResolution => E_MP4_UNKNOWN_RESOLUTION, Error::PesFrameTooLarge { .. } => E_PES_FRAME_TOO_LARGE, Error::PesInvalidMagic => E_PES_INVALID_MAGIC, Error::PesTrackTooLarge { .. } => E_PES_TRACK_TOO_LARGE, @@ -992,9 +1002,10 @@ impl From for std::io::Error { // mp4:// demux errors: a malformed/truncated source file // (E_MP4_INVALID), or a source whose tracks the mux can't use — no // video track / missing codec-private config. All are invalid data. - E_MP4_NO_VIDEO_TRACK | E_MP4_INVALID | E_MP4_MISSING_CODEC_PRIVATE => { - std::io::ErrorKind::InvalidData - } + E_MP4_NO_VIDEO_TRACK + | E_MP4_INVALID + | E_MP4_MISSING_CODEC_PRIVATE + | E_MP4_UNKNOWN_RESOLUTION => std::io::ErrorKind::InvalidData, // 9030 ExtentNotUnitAligned: a malformed/non-AACS-aligned // extent was handed to the prefetch producer. 9030 => std::io::ErrorKind::InvalidInput, @@ -1696,6 +1707,7 @@ mod tests { (Error::Mp4NoVideoTrack, E_MP4_NO_VIDEO_TRACK), (Error::Mp4Invalid, E_MP4_INVALID), (Error::Mp4MissingCodecPrivate, E_MP4_MISSING_CODEC_PRIVATE), + (Error::Mp4UnknownResolution, E_MP4_UNKNOWN_RESOLUTION), (Error::M2tsPacketMalformed, E_M2TS_PACKET_MALFORMED), (Error::ExtentNotUnitAligned, E_EXTENT_NOT_UNIT_ALIGNED), (Error::DiscCapacityMalformed, E_DISC_CAPACITY_MALFORMED), diff --git a/src/labels/mod.rs b/src/labels/mod.rs index 24823bb..0836aec 100644 --- a/src/labels/mod.rs +++ b/src/labels/mod.rs @@ -301,11 +301,7 @@ pub fn fill_defaults(titles: &mut [crate::disc::DiscTitle]) { // Unknown resolution: pass (0, 0) so the label omits the // resolution token rather than tagging it a fabricated // 1080p. - let px = if matches!(v.resolution, crate::disc::Resolution::Unknown) { - (0, 0) - } else { - v.resolution.pixels() - }; + let px = v.resolution.pixels().unwrap_or((0, 0)); v.label = generate_video_label( &v.codec, px, diff --git a/src/mux/meta_sink.rs b/src/mux/meta_sink.rs index 71f86b6..33f93cb 100644 --- a/src/mux/meta_sink.rs +++ b/src/mux/meta_sink.rs @@ -137,7 +137,9 @@ fn stream_json(s: &DiscStream) -> serde_json::Value { use serde_json::json; match s { DiscStream::Video(v) => { - let (w, h) = v.resolution.pixels(); + // Absent dimensions serialise as 0 here; the JSON consumer reads + // this as informational metadata, not as a mux input. + let (w, h) = v.resolution.pixels().unwrap_or((0, 0)); let (fps_num, fps_den) = v.frame_rate.as_fraction(); let mut o = json!({ "kind": "video", diff --git a/src/mux/mkv.rs b/src/mux/mkv.rs index d719175..5e0a938 100644 --- a/src/mux/mkv.rs +++ b/src/mux/mkv.rs @@ -385,7 +385,9 @@ impl MkvTrack { // that used to sit here existed only because `pixels()` fabricated a // 1920x1080 default; it does not any more, so the check belongs in the // one accessor rather than in each caller that remembered to write it. - let (w, h) = v.resolution.pixels(); + // None -> 0, and the writer already omits the optional + // PixelWidth/PixelHeight elements on 0 (RFC 9559 5.1.4.1.28-29). + let (w, h) = v.resolution.pixels().unwrap_or((0, 0)); let (num, den) = v.frame_rate.as_fraction(); let default_duration_ns = if num > 0 { (1_000_000_000u64 * den as u64) / num as u64 diff --git a/src/mux/mp4/mod.rs b/src/mux/mp4/mod.rs index 5632dc8..139d518 100644 --- a/src/mux/mp4/mod.rs +++ b/src/mux/mp4/mod.rs @@ -316,7 +316,15 @@ impl Mp4Sink { .get(i) .and_then(|c| c.clone()) .ok_or(crate::error::Error::Mp4MissingCodecPrivate)?; - let (w, h) = v.resolution.pixels(); + // ISO/IEC 14496-12 makes width/height mandatory in tkhd + // (8.3.2) and VisualSampleEntry (12.1.3) — unlike Matroska + // there is no element to omit. Writing 0x0 yields a + // structurally complete file no player can render, with no + // error anywhere; refuse instead. + let (w, h) = v + .resolution + .pixels() + .ok_or(crate::error::Error::Mp4UnknownResolution)?; tracks.push(Track { media: Media::Video, track_id, @@ -1155,6 +1163,46 @@ mod tests { assert_eq!(r.included, vec![1], "only the AC-3 audio is carried"); } + /// A video track whose resolution never resolved must FAIL the mux, not be + /// written as a 0x0 track. + /// + /// ISO/IEC 14496-12 makes width and height mandatory in both `tkhd` (8.3.2) + /// and VisualSampleEntry (12.1.3), so unlike Matroska — which simply omits + /// the optional PixelWidth/PixelHeight elements — MP4 has nothing to leave + /// out. Writing zeros produces a structurally complete file that passes + /// every container check and that no player can render, with no error + /// anywhere: a wrong answer that looks like a successful rip. + /// + /// `Resolution::pixels()` returns `Option` for this reason. It used to + /// return a fabricated 1920x1080, then `(0, 0)`; the zero pair reads as a + /// usable value, so this sink stored it and serialised it, and the guard + /// that two of the three sinks have was never needed here and so was never + /// written. + #[test] + fn a_video_track_with_no_resolved_resolution_is_an_error_not_a_zero_sized_track() { + let DiscStream::Video(mut v) = hevc_video() else { + unreachable!("hevc_video builds a video stream") + }; + v.resolution = Resolution::Unknown; + let t = title( + vec![DiscStream::Video(v), audio(Codec::Ac3, "eng")], + vec![Some(vec![0x01, 0x02, 0x03]), None], + ); + + let err = match Mp4Sink::create(std::io::Cursor::new(Vec::new()), &t) { + Ok(_) => panic!("an unrenderable 0x0 track must not be written silently"), + Err(e) => e, + }; + // `From for io::Error` stringifies as "E[: ...]", so the + // code round-trips in the message. + assert!( + err.to_string() + .starts_with(&format!("E{}", crate::error::E_MP4_UNKNOWN_RESOLUTION)), + "the failure must name the missing dimensions, not a generic mux \ + error; got {err}" + ); + } + #[test] fn no_video_track_is_an_error() { let t = title(vec![audio(Codec::Ac3, "eng")], vec![None]); diff --git a/src/mux/videomap.rs b/src/mux/videomap.rs index 79e5ee5..3e2f2c6 100644 --- a/src/mux/videomap.rs +++ b/src/mux/videomap.rs @@ -224,7 +224,8 @@ impl MapHeader { let stream = match video { Some(v) => { - let (width, height) = v.resolution.pixels(); + // Informational map; absent dimensions report as 0. + let (width, height) = v.resolution.pixels().unwrap_or((0, 0)); StreamInfo { codec: fvi_codec_id(v.codec), width,