From 3efa6211f3fc71965c4780519a1f77141848d0e5 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Wed, 29 Jul 2026 20:46:10 -0700 Subject: [PATCH] Make six silent mux failures observable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All six confirmed against the code. The governing rule this cluster serves: a lossy or degraded outcome is never silent, because a corrupt rip the user does not know about is the worst failure available. **A 3D MKV re-mux silently lost one eye.** The BlockGroup read path had arms for BLOCK / BLOCK_DURATION / REFERENCE_BLOCK only, so BLOCK_ADDITIONS fell into the skip arm — while the writer does emit BlockAdditions > BlockMore > BlockAdditional for the MVC dependent view. Reconstruction was judged out of scope and the reasoning is recorded: PesFrame has no side-payload field and the header parser never reads BlockAdditionMapping, so there is no dependent-view track to route the AU to. Instead the loss is now LOUD — counted in bytes and events, warned once, and surfaced through MkvStream's errors()/lost_bytes(), which the driver already samples into MuxOutcome. One detail in the finding was wrong and is corrected: the re-mux does NOT still advertise the mvcC mapping, because the header parser ignores that element, so the output is a plain 2D H.264 track. **An all-titles rip silently skipped real titles.** The header-buffer-cap overflow returned Error::MkvInvalid, and is_skippable_title_stub matches exactly E_MKV_INVALID | E_CSS_KEY_MISSING — verified here — so a 512 MiB-of-frames title was classified as an empty nav/menu PGC stub and dropped. It now has its own E9051 / MuxHeaderBufferExceeded { bytes }, outside the skippable set. **The public pre-mux report contradicted the file.** Mp4Sink::finish() drops an audio track it cannot describe, which I chose last round over failing an export whose video is fine — but mp4_fit_report still listed that stream as included, so the application's plan and the actual output disagreed. Fixed at both levels: Mp4SkipReason is now non_exhaustive with NoSamples and UndescribableAudio, Mp4Sink::final_report() describes the FILE rather than the plan, and for the boxed dyn Stream path a defaulted Stream::undelivered_streams() carries the information out to MuxOutcome::undelivered_streams with a driver-side warn. **MP4 track ids could collide.** ids were assigned before the retain that drops sample-less tracks, while next_id came from the post-retain count, so [1,3] yielded next_id 3. Now max(track_id) + 1, saturating. **Stream selection silently skipped its codec_privates prune** when the lists were not the same length — but codec_privates is consumed POSITIONALLY and trailing extras are documented as benign, so the length-equality guard was itself the bug. The prune now runs unconditionally by index. **The m2ts_mux scaffolding armed params_written on both the absent and the unparseable codec_private arms** — the same defect already fixed in tsmux.rs. Split into params_attempted (a latch, since retrying identical bytes cannot help) and params_emitted, with a warn on each failure arm and an accessor so the eventual wiring and its test can observe it. Each fix verified red by mutating back to the prior behaviour: errors() 0 vs 1, E6008 vs E9051, final_report [0,1] vs [0], next_track_id 3 vs [1,3], and the selection prune resolving index 1 to the wrong track's record. API surface deliberately widened: MuxOutcome gains a public field and Mp4Sink becomes public. Nothing in-repo breaks. Note a behaviour change on the mkv:// input path — a 3D re-mux now reports non-zero loss, so a consumer treating errors > 0 as disc damage will trip on it. That is intended: the outcome IS degraded. --- src/error.rs | 42 ++++ src/lib.rs | 2 +- src/mux/driver.rs | 142 +++++++++++-- src/mux/m2ts_mux/mod.rs | 138 +++++++++++- src/mux/mkvstream.rs | 175 ++++++++++++++++ src/mux/mod.rs | 6 +- src/mux/mp4/mod.rs | 451 ++++++++++++++++++++-------------------- src/mux/select.rs | 83 +++++++- src/pes.rs | 19 ++ 9 files changed, 798 insertions(+), 260 deletions(-) diff --git a/src/error.rs b/src/error.rs index 06af469..0579fb1 100644 --- a/src/error.rs +++ b/src/error.rs @@ -151,6 +151,11 @@ pub const E_NETWORK_ADDR_BLOCKED: u16 = 9022; /// zero-frame mux (undecryptable input, fully-unreadable title, every /// frame dropped before the first keyframe) cannot report success. pub const E_MUX_EMPTY: u16 = 9023; +/// The mux driver buffered past its pre-headers cap without every video track's +/// `codec_private` resolving. Deliberately NOT [`E_MKV_INVALID`], which +/// [`is_skippable_title_stub`] treats as a skippable empty nav/menu stub — a +/// cap-overflow is a real title and must never be silently skipped. +pub const E_MUX_HEADER_BUFFER_EXCEEDED: u16 = 9051; pub const E_EXTENT_NOT_UNIT_ALIGNED: u16 = 9030; /// `mp4://` output but the title has no (primary) video track to carry. pub const E_MP4_NO_VIDEO_TRACK: u16 = 9048; @@ -442,6 +447,20 @@ pub enum Error { /// every frame dropped before the first keyframe — fails loudly. The /// `m2ts://` analogue of [`Error::MkvInvalid`]'s zero-frame guard. MuxEmpty, + /// The mux driver's pre-headers frame buffer passed its cap before every + /// video track's `codec_private` resolved: the title keeps yielding real + /// frames but its codec init data never appears, so buffering further would + /// swap the box to death. Carries the buffered byte count. + /// + /// DISTINCT from [`Error::MkvInvalid`] on purpose. `MkvInvalid` is what an + /// empty nav/menu PGC stub yields, and [`is_skippable_title_stub`] classifies + /// it as skippable — an all-titles rip drops that title and finishes the + /// rest. Hundreds of megabytes of real frames with unresolvable headers is + /// NOT a stub; reporting it as one silently dropped a main feature from a + /// rip that then exited successfully. This code is not skippable. + MuxHeaderBufferExceeded { + bytes: u64, + }, /// `mp4://` target title has no primary video track to mux. Mp4NoVideoTrack, /// `mp4://` source file is malformed/truncated — the MP4 demuxer failed. @@ -629,6 +648,7 @@ impl Error { Error::StreamUrlMissingPort { .. } => E_STREAM_URL_MISSING_PORT, Error::NetworkAddrBlocked { .. } => E_NETWORK_ADDR_BLOCKED, Error::MuxEmpty => E_MUX_EMPTY, + Error::MuxHeaderBufferExceeded { .. } => E_MUX_HEADER_BUFFER_EXCEEDED, Error::Mp4NoVideoTrack => E_MP4_NO_VIDEO_TRACK, Error::Mp4Invalid => E_MP4_INVALID, Error::Mp4MissingCodecPrivate => E_MP4_MISSING_CODEC_PRIVATE, @@ -797,6 +817,9 @@ impl std::fmt::Display for Error { Error::SelectionPidUnknown { pid } => { write!(f, "E{}: 0x{:04x}", self.code(), pid) } + Error::MuxHeaderBufferExceeded { bytes } => { + write!(f, "E{}: {}", self.code(), bytes) + } _ => write!(f, "E{}", self.code()), } } @@ -869,6 +892,9 @@ impl From for std::io::Error { // 9023 MuxEmpty: finish() reached with zero frames — the output // would be a header-only container. Treat as invalid output. E_MUX_EMPTY => std::io::ErrorKind::InvalidData, + // 9051 MuxHeaderBufferExceeded: the source kept yielding frames but + // never its codec init data — the input is unusable as declared. + E_MUX_HEADER_BUFFER_EXCEEDED => std::io::ErrorKind::InvalidData, // 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. @@ -1066,6 +1092,17 @@ mod tests { // A plain io::Error with no E-code prefix is not skippable. let plain = std::io::Error::from(std::io::ErrorKind::BrokenPipe); assert!(!is_skippable_title_stub(&plain)); + + // A header-buffer cap overflow is a REAL title whose codec init data + // never resolved — hundreds of MiB of frames, not an empty nav/menu + // stub. It used to be reported as `MkvInvalid`, which lands in the + // skippable set above, so an all-titles rip dropped a main feature and + // still exited successfully. It must have its own, non-skippable code. + let cap: std::io::Error = Error::MuxHeaderBufferExceeded { + bytes: 512 * 1024 * 1024 + 1, + } + .into(); + assert!(!is_skippable_title_stub(&cap)); } #[test] @@ -1353,6 +1390,7 @@ mod tests { E_STREAM_URL_MISSING_PORT, E_NETWORK_ADDR_BLOCKED, E_MUX_EMPTY, + E_MUX_HEADER_BUFFER_EXCEEDED, E_MP4_NO_VIDEO_TRACK, E_MP4_INVALID, E_MP4_MISSING_CODEC_PRIVATE, @@ -1443,6 +1481,10 @@ mod tests { (Error::PipelineConsumerGone, E_PIPELINE_CONSUMER_GONE), (Error::DiscCapacityOverflow, E_DISC_CAPACITY_OVERFLOW), (Error::MuxEmpty, E_MUX_EMPTY), + ( + Error::MuxHeaderBufferExceeded { bytes: 0 }, + E_MUX_HEADER_BUFFER_EXCEEDED, + ), (Error::Mp4NoVideoTrack, E_MP4_NO_VIDEO_TRACK), (Error::Mp4Invalid, E_MP4_INVALID), (Error::Mp4MissingCodecPrivate, E_MP4_MISSING_CODEC_PRIVATE), diff --git a/src/lib.rs b/src/lib.rs index 7400ec9..e181f02 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -256,7 +256,7 @@ pub use mux::NullStream; pub use mux::StdioStream; pub use mux::WriteSeek; pub use mux::{InputOptions, StreamUrl, input, output, parse_url}; -pub use mux::{Mp4FitReport, Mp4SkipReason, mp4_fit_report}; +pub use mux::{Mp4FitReport, Mp4Sink, Mp4SkipReason, mp4_fit_report}; // ─── Lower-level surfaces ─────────────────────────────────────────────────── // diff --git a/src/mux/driver.rs b/src/mux/driver.rs index fe40585..a720266 100644 --- a/src/mux/driver.rs +++ b/src/mux/driver.rs @@ -253,6 +253,17 @@ pub struct MuxOutcome { pub lost_bytes: u64, /// Number of streams in the muxed title. pub streams: usize, + /// `title.streams` indices the output sink accepted frames for but could not + /// put in the finished container (`Stream::undelivered_streams`) — today only + /// the `mp4://` sink, which must drop an audio track no frame of which + /// yielded a parseable sample entry. + /// + /// Non-empty means the file does NOT match the pre-mux plan + /// (`mp4_fit_report`), even with `completed = true`: those streams are + /// missing. A caller that reports a successful export must report these too — + /// a lossy outcome is never silent. Always empty when the run stopped before + /// the sink was finalised (nothing was finished, so nothing is known). + pub undelivered_streams: Vec, } /// Run the decrypt + mux pipeline end-to-end: construct the source stream from @@ -637,6 +648,7 @@ fn drive_mux( errors: stream.errors(), lost_bytes: stream.lost_bytes(), streams: out_title.streams.len(), + undelivered_streams: sink.undelivered_streams(), }); } @@ -656,6 +668,7 @@ fn drive_mux( errors: stream.errors(), lost_bytes: stream.lost_bytes(), streams: 0, + undelivered_streams: Vec::new(), }); } let read = match stream.read() { @@ -673,6 +686,7 @@ fn drive_mux( errors: stream.errors(), lost_bytes: stream.lost_bytes(), streams: 0, + undelivered_streams: Vec::new(), }); } Err(e) => return Err(e), @@ -683,12 +697,28 @@ fn drive_mux( buffered.push(frame); // Bounded header buffer: a title whose codec_private never // resolves would otherwise buffer the entire (tens-of-GB) - // stream into RAM until OOM. Treat cap-exceeded identically to - // "headers never resolved" — fail fast with the SAME - // `Error::MkvInvalid` the header gate below returns, instead of - // swapping the box to death. + // stream into RAM until OOM. Fail fast instead of swapping the + // box to death. + // + // NOT `Error::MkvInvalid` — that code is what an empty nav/menu + // PGC stub yields and `error::is_skippable_title_stub` + // classifies it as skippable, so an all-titles rip DROPPED a + // title that had produced half a gigabyte of real frames and + // then exited reporting success. A cap overflow gets its own + // non-skippable code so it can never be mistaken for a stub. if buffered_bytes > HEADER_BUFFER_CAP_BYTES { - return Err(Error::MkvInvalid.into()); + tracing::error!( + target: "mux", + buffered_bytes, + cap = HEADER_BUFFER_CAP_BYTES, + "header buffer cap exceeded: the title keeps yielding frames but no \ + video track's codec_private ever resolved; refusing rather than \ + buffering the whole stream into RAM" + ); + return Err(Error::MuxHeaderBufferExceeded { + bytes: buffered_bytes as u64, + } + .into()); } } None => break, @@ -715,6 +745,7 @@ fn drive_mux( errors: stream.errors(), lost_bytes: stream.lost_bytes(), streams: 0, + undelivered_streams: Vec::new(), }); } return Err(Error::MkvInvalid.into()); @@ -808,11 +839,21 @@ fn drive_mux( // container and returns the payload-byte count. On halt/wedge this returns // an error variant; we translate that to `completed = false` rather than // surfacing it as a hard failure (a clean operator stop is not an error). - let (bytes_written, finalize_failed) = match pipe.finish_with_halt(Some(halt)) { - Ok(b) => (b, false), - Err(Error::Halted | Error::PipelineJoinTimeout) => (bytes.load(Ordering::Relaxed), true), - Err(e) => return Err(e.into()), - }; + let (bytes_written, undelivered_streams, finalize_failed) = + match pipe.finish_with_halt(Some(halt)) { + Ok((b, undelivered)) => (b, undelivered, false), + Err(Error::Halted | Error::PipelineJoinTimeout) => { + (bytes.load(Ordering::Relaxed), Vec::new(), true) + } + Err(e) => return Err(e.into()), + }; + if !undelivered_streams.is_empty() { + tracing::warn!( + target: "mux", + streams = ?undelivered_streams, + "output sink could not deliver every planned stream; the file does not match the pre-mux plan (surfaced as MuxOutcome::undelivered_streams)" + ); + } if !mux_run_completed(interrupted, finalize_failed, halt.is_cancelled()) { return Ok(MuxOutcome { @@ -822,6 +863,7 @@ fn drive_mux( errors: stream.errors(), lost_bytes: stream.lost_bytes(), streams: num_streams, + undelivered_streams, }); } @@ -840,18 +882,22 @@ fn drive_mux( errors: stream.errors(), lost_bytes: stream.lost_bytes(), streams: num_streams, + undelivered_streams, }) } /// Write-side [`Sink`]: applies each frame to the counting output stream and -/// finalises the container on close. `close()` returns the payload-byte count. +/// finalises the container on close. `close()` returns the payload-byte count +/// plus any streams the sink could not deliver (see +/// [`MuxOutcome::undelivered_streams`]) — that fact lives only in the sink and +/// dies with the consumer thread unless it is carried out here. struct WriteSink { output: CountingStream, bytes: Arc, } impl Sink for WriteSink { - type Output = u64; + type Output = (u64, Vec); fn apply(&mut self, frame: PesFrame) -> Result { self.output.write(&frame).map_err(Error::from)?; @@ -860,9 +906,13 @@ impl Sink for WriteSink { Ok(Flow::Continue) } - fn close(mut self) -> Result { + fn close(mut self) -> Result<(u64, Vec), Error> { self.output.finish().map_err(Error::from)?; - Ok(self.output.bytes_written()) + // Sample AFTER finish(): the mp4 sink decides its drops there. + Ok(( + self.output.bytes_written(), + self.output.undelivered_streams(), + )) } } @@ -1935,6 +1985,55 @@ mod tests { assert!(map_none.is_none(), "CSS/clear must NOT resolve an AACS map"); } + /// A sink that accepts every frame but reports one stream it could not put in + /// the finished container — the `mp4://` shape (an audio track dropped at + /// `finish()` because no frame yielded a parseable sample entry). + struct UndeliveringSink { + info: DiscTitle, + } + + impl Stream for UndeliveringSink { + fn read(&mut self) -> std::io::Result> { + Ok(None) + } + fn write(&mut self, _frame: &PesFrame) -> std::io::Result<()> { + Ok(()) + } + fn finish(&mut self) -> std::io::Result<()> { + Ok(()) + } + fn info(&self) -> &DiscTitle { + &self.info + } + fn undelivered_streams(&self) -> Vec { + vec![1] + } + } + + /// The sink lives in the consumer thread and is destroyed with it, so a + /// stream it could not deliver is knowable ONLY at `close()`. `close()` must + /// carry that out alongside the byte count, or `MuxOutcome` can never report + /// it and the loss stays a log line. + /// + /// Mutation: return `Vec::new()` from `close()` instead of sampling the sink + /// and the fact never leaves the thread. + #[test] + fn write_sink_carries_undelivered_streams_out_of_the_consumer_thread() { + let sink = WriteSink { + output: CountingStream::new(Box::new(UndeliveringSink { + info: DiscTitle::empty(), + })), + bytes: Arc::new(AtomicU64::new(0)), + }; + let (bytes, undelivered) = sink.close().expect("close succeeds"); + assert_eq!(bytes, 0); + assert_eq!( + undelivered, + vec![1], + "the sink's undelivered stream must reach the driver" + ); + } + // ── Regression A: header-buffer cap fails fast instead of OOM ─────────── // // A stream whose headers never resolve but that keeps yielding frames must @@ -1979,10 +2078,21 @@ mod tests { Duration::from_secs(60), ) .expect_err("over-cap header buffer must fail fast, not OOM"); + // The cap overflow must carry its OWN code, not `MkvInvalid`: an + // all-titles rip asks `is_skippable_title_stub` whether to move on, and + // `MkvInvalid` answers "yes, an empty nav/menu stub" — which silently + // dropped a title that had just produced 512 MiB of real frames. assert_eq!( err.to_string(), - format!("E{}", crate::error::E_MKV_INVALID), - "cap-exceeded returns the same MkvInvalid as headers-never-resolved" + format!("E{}: {}", crate::error::E_MUX_HEADER_BUFFER_EXCEEDED, { + let frames = cap_frames + 1; + frames * FRAME + }), + "cap-exceeded must report its own code plus the buffered byte count" + ); + assert!( + !crate::error::is_skippable_title_stub(&err), + "a cap overflow is a real title, never a skippable stub" ); let reads = reads_seen.load(Ordering::SeqCst); assert!( diff --git a/src/mux/m2ts_mux/mod.rs b/src/mux/m2ts_mux/mod.rs index b460958..11ac310 100644 --- a/src/mux/m2ts_mux/mod.rs +++ b/src/mux/m2ts_mux/mod.rs @@ -117,8 +117,16 @@ pub struct M2tsMux { /// Optional — if the upstream frames already carry inline params, /// callers can omit this. video_codec_private: Option>, - /// Set on first video frame: have we emitted VPS/SPS/PPS? - params_written: bool, + /// Set on the first video keyframe: has the parameter-set prepend been + /// ATTEMPTED? Latched even when it produced nothing (no codec_private, or + /// one that would not parse) — the same bytes cannot parse better on the + /// next keyframe, so retrying is pointless. + params_attempted: bool, + /// Whether VPS/SPS/PPS actually REACHED the stream. Distinct from + /// `params_attempted`: `false` after an attempt means the emitted TS carries + /// no parameter sets and the video will not decode. See + /// [`parameter_sets_emitted`](Self::parameter_sets_emitted). + params_emitted: bool, /// Audio codec, if an audio track is configured. `None` = video-only. audio: Option, /// First seen PTS (90 kHz). All subsequent PTS / PCR values are @@ -149,7 +157,8 @@ impl M2tsMux { Self { out: PacketWriter::new(writer), video_codec_private: None, - params_written: false, + params_attempted: false, + params_emitted: false, audio: None, base_pts_90k: None, cc_video: 0, @@ -168,6 +177,19 @@ impl M2tsMux { self.video_codec_private = Some(hvcc); } + /// Whether VPS/SPS/PPS parameter sets actually reached the emitted stream. + /// + /// `false` after any video frame has been written means the TS carries no + /// parameter sets — because none were set, or because the `hvcC` record would + /// not parse — and its video will not decode unless the frames themselves + /// carry inline parameter sets. The prepend is attempted exactly once (on the + /// first keyframe) and never retried, so a caller wiring this muxer into a + /// pipeline must check this rather than assume `finish() == Ok` means a + /// decodable stream. + pub fn parameter_sets_emitted(&self) -> bool { + self.params_emitted + } + /// Enable a single audio track. Must be called before /// [`write_audio`](Self::write_audio). pub fn set_audio(&mut self, codec: AudioCodec) { @@ -189,13 +211,34 @@ impl M2tsMux { // FIRST keyframe (not first frame — non-key frames before the // first keyframe can't carry params usefully). let mut es = Vec::with_capacity(data.len() + 64); - if keyframe && !self.params_written { - if let Some(cp) = &self.video_codec_private { - if let Some(params) = super::hevc::hvcc_to_annex_b(cp) { - es.extend_from_slice(¶ms); - } + if keyframe && !self.params_attempted { + // Arming the latch below is right either way — the same codec_private + // bytes cannot parse better on a later keyframe — but it must not be + // SILENT, which is what it was: no parameter sets reached the stream, + // every later keyframe skipped the prepend, and `finish()` returned Ok + // on a TS whose video cannot be decoded. Mirrors the same fix in the + // BD-TS sibling (`tsmux.rs`). + match self.video_codec_private.as_deref() { + Some(cp) => match super::hevc::hvcc_to_annex_b(cp) { + Some(params) => { + es.extend_from_slice(¶ms); + self.params_emitted = true; + } + None => tracing::warn!( + target: "mux", + codec_private_len = cp.len(), + "m2ts: video codec_private did not parse as an hvcC record; no VPS/SPS/PPS \ + emitted and the video will not decode (parameter_sets_emitted() == false)" + ), + }, + None => tracing::warn!( + target: "mux", + "m2ts: no video codec_private was set before the first keyframe; no \ + VPS/SPS/PPS emitted and the video will not decode unless the frames carry \ + inline parameter sets (parameter_sets_emitted() == false)" + ), } - self.params_written = true; + self.params_attempted = true; } // Append the Annex-B form directly into the pre-sized `es` // buffer rather than materializing an intermediate Vec. @@ -852,6 +895,83 @@ mod tests { assert_eq!(af[0] & 0x40, 0x40, "RAI bit set"); } + /// The parameter-set prepend is attempted once, on the first keyframe, and + /// the latch is armed whether or not it produced anything. When it produced + /// nothing — no `codec_private` was set, or the `hvcC` will not parse — the + /// emitted TS carries no VPS/SPS/PPS at all and its video cannot be decoded, + /// yet `finish()` returns `Ok`. That must not be silent (same defect, and the + /// same fix, as the BD-TS sibling `tsmux.rs`): the muxer now warns and, so a + /// caller can act on it, exposes `parameter_sets_emitted()`. + /// + /// Mutation check: drop the `params_emitted` bookkeeping and the two arms are + /// indistinguishable to any caller. + #[test] + fn absent_or_unparseable_codec_private_is_reported_not_silent() { + // A VPS NAL the parser can find, and the hvcC that carries it. + let vps: [u8; 4] = [0x40, 0x01, 0x0C, 0x77]; + let mut hvcc = vec![0u8; 22]; + hvcc.push(1); // num_arrays + hvcc.push(0x20); // array_completeness | nal_type = VPS + hvcc.extend_from_slice(&1u16.to_be_bytes()); // num_nalus + hvcc.extend_from_slice(&(vps.len() as u16).to_be_bytes()); + hvcc.extend_from_slice(&vps); + + let mut frame = Vec::new(); + frame.extend_from_slice(&4u32.to_be_bytes()); + frame.extend_from_slice(&[0x26, 0x01, 0xAF, 0x10]); // an IDR-ish slice NAL + + // ── Control: a parseable hvcC really does emit its parameter sets ── + let mut good_sink: Vec = Vec::new(); + let emitted_good = { + let mut mux = M2tsMux::new(&mut good_sink); + mux.set_video_codec_private(hvcc.clone()); + mux.write_video(0, true, &frame).unwrap(); + mux.finish().unwrap(); + mux.parameter_sets_emitted() + }; + assert!( + emitted_good, + "a parseable hvcC must emit its parameter sets" + ); + assert!( + good_sink.windows(vps.len()).any(|w| w == vps), + "the VPS must appear in the emitted TS" + ); + + // ── A truncated hvcC: hvcc_to_annex_b returns None ── + let mut bad_sink: Vec = Vec::new(); + let emitted_bad = { + let mut mux = M2tsMux::new(&mut bad_sink); + mux.set_video_codec_private(hvcc[..10].to_vec()); + mux.write_video(0, true, &frame).unwrap(); + // A LATER keyframe cannot recover it — the latch is already armed. + mux.write_video(40_000_000, true, &frame).unwrap(); + mux.finish().unwrap(); // Ok, despite undecodable video + mux.parameter_sets_emitted() + }; + assert!( + !emitted_bad, + "an unparseable hvcC emits nothing — the caller must be able to see that" + ); + assert!( + !bad_sink.windows(vps.len()).any(|w| w == vps), + "no parameter sets reached the stream" + ); + + // ── No codec_private at all: same undecodable outcome, same signal ── + let mut none_sink: Vec = Vec::new(); + let emitted_none = { + let mut mux = M2tsMux::new(&mut none_sink); + mux.write_video(0, true, &frame).unwrap(); + mux.finish().unwrap(); + mux.parameter_sets_emitted() + }; + assert!( + !emitted_none, + "no codec_private means no parameter sets, and that must be observable" + ); + } + #[test] fn pcr_packet_without_keyframe_has_rai_clear() { let mut sink: Vec = Vec::new(); diff --git a/src/mux/mkvstream.rs b/src/mux/mkvstream.rs index 5cb450b..f1599ee 100644 --- a/src/mux/mkvstream.rs +++ b/src/mux/mkvstream.rs @@ -74,6 +74,13 @@ struct ReadState { ts_scale_ns: i64, /// Codec private data per track (track_number, hvcC/avcC bytes). codec_privates: Vec<(u16, Vec)>, + /// Number of `BlockAdditions` subtrees skipped on read-back (see + /// `MkvStream`'s `Stream::lost_bytes`). Each one is a per-frame side payload — for a + /// Blu-ray 3D rip written by this crate, one MVC dependent-view (right-eye) + /// access unit — that the PES frame model cannot carry, so it is dropped. + additions_dropped: u64, + /// Cumulative `BlockAdditional` payload bytes dropped on read-back. + additions_dropped_bytes: u64, } /// Safety cap on frames buffered before the first video frame triggers muxer @@ -552,6 +559,8 @@ impl MkvStream { cluster_ts_ticks: 0, ts_scale_ns, codec_privates, + additions_dropped: 0, + additions_dropped_bytes: 0, }), }) } @@ -716,6 +725,44 @@ impl crate::pes::Stream for MkvStream { has_reference = true; skip_bytes(&mut rs.reader, cs)?; } + ebml::BLOCK_ADDITIONS => { + // A `BlockAdditions > BlockMore > BlockAdditional` + // subtree is a per-frame SIDE payload. For a + // Blu-ray 3D title written by this crate it is the + // MVC dependent-view (right-eye) access unit, + // BlockAddID=2, described by the track's `mvcC` + // BlockAdditionMapping. + // + // `PesFrame` has no side-payload field and the + // read-side stream table (built by + // `parse_mkv_header`, which does not parse + // BlockAdditionMapping) has no dependent-view + // track to hand it to, so this reader CANNOT + // reconstruct it — re-muxing a 3D MKV yields a + // base-view-only (2D) file. Reconstruction needs + // header-parse plumbing well beyond this arm. + // + // What it must NOT be is silent: this used to fall + // into the `_` skip arm below, so an + // `mkv://` → `mkv://` re-mux of a 3D rip dropped + // one whole eye with no error, no warning and + // `lost_bytes == 0`. Account for it so the loss + // reaches `MuxOutcome.lost_bytes` / `errors`. + if rs.additions_dropped == 0 { + tracing::warn!( + target: "mux", + bytes = cs, + "mkv read-back: dropping a BlockAdditions payload this \ + reader cannot carry (a Blu-ray 3D MVC dependent view is \ + the expected case); the output will be base-view only. \ + Counted in lost_bytes/errors." + ); + } + rs.additions_dropped = rs.additions_dropped.saturating_add(1); + rs.additions_dropped_bytes = + rs.additions_dropped_bytes.saturating_add(cs); + skip_bytes(&mut rs.reader, cs)?; + } _ => skip_bytes(&mut rs.reader, cs)?, } } @@ -826,6 +873,33 @@ impl crate::pes::Stream for MkvStream { fn headers_ready(&self) -> bool { true // MKV has all headers upfront in the EBML header } + + /// Count of `BlockAdditions` subtrees dropped on read-back — each one a + /// per-frame side payload (a Blu-ray 3D MVC dependent-view access unit for a + /// 3D rip written by this crate) that the PES frame model cannot carry. + /// + /// Reported through the same channel as a disc-read skip event because it is + /// the same kind of fact: input bytes that did not reach the output. A 3D + /// re-mux losing an eye is a degraded outcome, and a degraded outcome is + /// never silent. `0` for the write side and for any source with no + /// `BlockAdditions`. + fn errors(&self) -> u64 { + match self.mode { + Mode::Read(ref rs) => rs.additions_dropped, + _ => 0, + } + } + + /// Cumulative `BlockAdditions` bytes dropped on read-back — see + /// [`errors`](crate::pes::Stream::errors). Counts the whole skipped subtree (the + /// `BlockAdditional` payload plus a handful of bytes of EBML framing above + /// it), so it is an upper bound on the payload proper. + fn lost_bytes(&self) -> u64 { + match self.mode { + Mode::Read(ref rs) => rs.additions_dropped_bytes, + _ => 0, + } + } } // ── MKV header parsing (read side) ──────────────────────────── @@ -1891,6 +1965,107 @@ mod tests { assert_eq!(frame.duration_ns, Some(40 * 1_000_000)); } + /// A BlockGroup's `BlockAdditions` subtree (BlockAddID=2 — the MVC + /// dependent/right-eye access unit this crate's 3D writer emits, see + /// `mkv.rs::build_block_group`) cannot be carried by `PesFrame`, so read-back + /// drops it. That is a LOSSY outcome, and this crate's rule is that a lossy + /// outcome is never silent. + /// + /// Regression: the arm did not exist, so the subtree fell into the `_ =>` + /// skip arm — an `mkv://` → `mkv://` re-mux of a 3D rip lost one whole eye + /// with no error, no warning and `lost_bytes == 0`, i.e. the mux reported a + /// clean, complete, loss-free copy of a file it had halved. The base view + /// must still read back intact, and the dropped payload must now be counted + /// so it reaches `MuxOutcome.lost_bytes` / `.errors`. + #[test] + fn block_additions_dropped_on_read_back_is_counted_not_silent() { + // The dependent-view payload: big enough that a byte count is unambiguous. + let dependent_au = vec![0x5Au8; 512]; + + // BlockAdditions > BlockMore > { BlockAddID = 2, BlockAdditional }. + let mut more = Vec::new(); + ebml::write_uint(&mut more, ebml::BLOCK_ADD_ID, 2).unwrap(); + ebml::write_binary(&mut more, ebml::BLOCK_ADDITIONAL, &dependent_au).unwrap(); + let mut adds = Vec::new(); + ebml::write_id(&mut adds, ebml::BLOCK_MORE).unwrap(); + ebml::write_size(&mut adds, more.len() as u64).unwrap(); + adds.extend_from_slice(&more); + + // BlockGroup > { Block(base view), BlockAdditions }. + let block = [0x81u8, 0x00, 0x00, 0x00, 0xAA, 0xBB, 0xCC]; + let mut bg_body = Vec::new(); + ebml::write_id(&mut bg_body, ebml::BLOCK).unwrap(); + ebml::write_size(&mut bg_body, block.len() as u64).unwrap(); + bg_body.extend_from_slice(&block); + ebml::write_id(&mut bg_body, ebml::BLOCK_ADDITIONS).unwrap(); + ebml::write_size(&mut bg_body, adds.len() as u64).unwrap(); + bg_body.extend_from_slice(&adds); + + let mut cluster = Vec::new(); + ebml::write_id(&mut cluster, ebml::CLUSTER).unwrap(); + ebml::write_unknown_size(&mut cluster).unwrap(); + ebml::write_id(&mut cluster, ebml::BLOCK_GROUP).unwrap(); + ebml::write_size(&mut cluster, bg_body.len() as u64).unwrap(); + cluster.extend_from_slice(&bg_body); + + // One video TRACK_ENTRY (track number 1) so track index 0 is in range. + let mut entry = Vec::new(); + ebml::write_uint(&mut entry, ebml::TRACK_NUMBER, 1).unwrap(); + ebml::write_uint(&mut entry, ebml::TRACK_TYPE, 1).unwrap(); + let mut tracks = Vec::new(); + ebml::write_id(&mut tracks, ebml::TRACK_ENTRY).unwrap(); + ebml::write_size(&mut tracks, entry.len() as u64).unwrap(); + tracks.extend_from_slice(&entry); + + let mut out = Vec::new(); + ebml::write_id(&mut out, ebml::EBML).unwrap(); + ebml::write_size(&mut out, 0).unwrap(); + ebml::write_id(&mut out, ebml::SEGMENT).unwrap(); + ebml::write_unknown_size(&mut out).unwrap(); + ebml::write_id(&mut out, ebml::INFO).unwrap(); + ebml::write_size(&mut out, 0).unwrap(); + ebml::write_id(&mut out, ebml::TRACKS).unwrap(); + ebml::write_size(&mut out, tracks.len() as u64).unwrap(); + out.extend_from_slice(&tracks); + out.extend_from_slice(&cluster); + + let mut stream = MkvStream::open(Cursor::new(out)).unwrap(); + assert_eq!(stream.errors(), 0, "no BlockAdditions seen before the read"); + assert_eq!(stream.lost_bytes(), 0); + + let frame = stream + .read() + .unwrap() + .expect("the base-view BlockGroup frame must still be read"); + assert_eq!(frame.track, 0); + assert_eq!( + frame.data, + vec![0xAA, 0xBB, 0xCC], + "the frame carries the BASE view only — the dependent view is lost" + ); + assert!( + !frame.data.contains(&0x5A), + "PesFrame has no side-payload field, so the dependent AU is NOT in the frame" + ); + + // The loss is now reported. + assert_eq!( + stream.errors(), + 1, + "one dropped BlockAdditions subtree must be counted as a loss event" + ); + assert!( + stream.lost_bytes() >= dependent_au.len() as u64, + "dropped bytes ({}) must cover the {}-byte dependent AU", + stream.lost_bytes(), + dependent_au.len() + ); + + // EOF, and the counters survive it (the driver samples them after the run). + assert!(stream.read().unwrap().is_none()); + assert_eq!(stream.errors(), 1); + } + /// A BlockGroup carrying a ReferenceBlock is NOT a keyframe — that element's /// presence is the only non-keyframe signal a BlockGroup has (the /// SimpleBlock 0x80 flag bit is reserved and always 0 inside one). diff --git a/src/mux/mod.rs b/src/mux/mod.rs index 1fbee88..157d8e0 100644 --- a/src/mux/mod.rs +++ b/src/mux/mod.rs @@ -119,7 +119,11 @@ pub use disc::DiscStream; pub use driver::{MuxEvents, MuxInput, MuxOptions, MuxOutcome, mux_stream}; pub use m2ts::M2tsStream; pub use mkvstream::MkvStream; -pub use mp4::{Mp4FitReport, Mp4SkipReason, fit_report as mp4_fit_report}; +// `Mp4Sink` is public (like `MkvStream` / `M2tsStream`) so a caller that drives +// the sink directly can ask `final_report()` what the finished file actually +// contains — the pre-mux `mp4_fit_report` plan is a prediction, and two of its +// inclusions can still be dropped at `finish()`. +pub use mp4::{Mp4FitReport, Mp4Sink, Mp4SkipReason, fit_report as mp4_fit_report}; pub use network::NetworkStream; pub use null::NullStream; pub use pipelined_stream::PipelinedPesStream; diff --git a/src/mux/mp4/mod.rs b/src/mux/mp4/mod.rs index 65f7ece..b048f5b 100644 --- a/src/mux/mp4/mod.rs +++ b/src/mux/mp4/mod.rs @@ -126,6 +126,10 @@ struct Track { media: Media, /// 1-based MP4 track_ID. track_id: u32, + /// `title.streams` index this track was built from — the identity + /// `Mp4FitReport` speaks in, so a track dropped at `finish()` can be named + /// in [`Mp4Sink::final_report`]. + stream_idx: usize, codec: Codec, /// Video: `hvcC`/`avcC`. Audio: unused (the sample entry is built from the /// first frame's bitstream and cached in `audio_entry`). @@ -142,7 +146,11 @@ struct Track { } /// Why a stream was excluded from an `mp4://` mux (for the never-silent report). +/// +/// Marked `#[non_exhaustive]`: new reasons appear as the writer learns to +/// distinguish more of them, so downstream must not match exhaustively. #[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] pub enum Mp4SkipReason { /// A subtitle track — MP4 carries only text subs; disc subs are bitmap. BitmapSubtitle, @@ -154,11 +162,28 @@ pub enum Mp4SkipReason { /// A primary video track whose codec this MP4 writer can't carry /// (only HEVC/H.264 are supported — e.g. VC-1, MPEG-2, AV1). UnmappableVideo, + /// Planned as carried, but the stream delivered no sample at all, so + /// `finish()` dropped the track rather than write an empty `trak`. + /// A *post-mux* reason: [`fit_report`] cannot predict it, only + /// [`Mp4Sink::final_report`] reports it. + NoSamples, + /// Planned as carried, and samples DID reach `mdat`, but no frame yielded a + /// parseable audio sample entry, so the track could not be described in + /// `stsd` and `finish()` dropped it (its bytes stay in `mdat`, unreferenced). + /// A *post-mux* reason — see [`Mp4Sink::final_report`]. + UndescribableAudio, } /// The plan for an `mp4://` mux of `title`: which streams are carried and which /// are excluded (with the reason). The CLI prints the exclusions so a lossy /// export is never silent; the sink applies the same predicate. +/// +/// [`fit_report`] returns the PRE-mux plan, which is a prediction: two of its +/// inclusions can still fail at `finish()` (a stream that delivers no sample, an +/// audio stream no frame of which yields a parseable sample entry). Ask +/// [`Mp4Sink::final_report`] after `finish()` for what the file actually +/// contains — the plan alone is not a statement about the output. +#[derive(Debug, Clone, PartialEq, Eq)] pub struct Mp4FitReport { /// `title.streams` indices that will be muxed. pub included: Vec, @@ -236,6 +261,12 @@ pub struct Mp4Sink { /// Reserved hole size in bytes (`moov` + trailing `free` padding go here). reserve: u64, finished: bool, + /// The create-time (pre-mux) plan, kept so [`Self::final_report`] can hand + /// back a report that matches the FILE rather than the prediction. + plan: Mp4FitReport, + /// Streams the plan promised that `finish()` actually dropped, with why. + /// Empty until `finish()` runs. + dropped: Vec<(usize, Mp4SkipReason)>, } impl Mp4Sink { @@ -272,6 +303,7 @@ impl Mp4Sink { tracks.push(Track { media: Media::Video, track_id, + stream_idx: i, codec: v.codec, codec_private: cp, width: w, @@ -287,6 +319,7 @@ impl Mp4Sink { tracks.push(Track { media: Media::Audio, track_id, + stream_idx: i, codec: a.codec, codec_private: Vec::new(), width: 0, @@ -333,9 +366,33 @@ impl Mp4Sink { hole_start, reserve, finished: false, + plan: report, + dropped: Vec::new(), }) } + /// What the file ACTUALLY contains, in the same shape as the pre-mux + /// [`fit_report`] plan. Before `finish()` it equals that plan; after + /// `finish()` every track the writer had to drop has moved from `included` + /// into `skipped` with a post-mux reason ([`Mp4SkipReason::NoSamples`], + /// [`Mp4SkipReason::UndescribableAudio`]). + /// + /// This exists because the plan is a PREDICTION. `finish()` drops an audio + /// track no frame of which yielded a parseable sample entry (it cannot be + /// described in `stsd`) and returns `Ok` so an export whose video is fine + /// still succeeds — but then the plan, which is the only structured report + /// the crate publishes, still named that stream as carried. A caller + /// believing it reported a successful export of a file with no audio. Ask + /// this after `finish()` before telling anyone what was written. + pub fn final_report(&self) -> Mp4FitReport { + let mut included = self.plan.included.clone(); + included.retain(|i| !self.dropped.iter().any(|(d, _)| d == i)); + let mut skipped = self.plan.skipped.clone(); + skipped.extend(self.dropped.iter().copied()); + skipped.sort_by_key(|&(i, _)| i); + Mp4FitReport { included, skipped } + } + /// Assemble the `moov` box from every track's sample tables. fn build_moov(&self) -> Vec { // Movie timescale = 90 kHz; movie duration = the longest track (converted). @@ -347,7 +404,18 @@ impl Mp4Sink { traks.push(trak); movie_dur = movie_dur.max((secs * movie_ts as f64) as u64); } - let next_id = self.tracks.len() as u32 + 1; + // `next_track_id` must EXCEED every track_ID in use (ISO/IEC 14496-12 + // §8.2.2). Deriving it from the retained COUNT broke that whenever + // `finish()` dropped a track: ids [1, 3] retained → count 2 → 3, which + // names a live track, so a tool appending a track with it creates a + // duplicate id. Take the real maximum. + let next_id = self + .tracks + .iter() + .map(|t| t.track_id) + .max() + .unwrap_or(0) + .saturating_add(1); let mut moov = build_mvhd(movie_ts, movie_dur, next_id); for trak in traks { @@ -395,8 +463,24 @@ impl Stream for Mp4Sink { return Ok(()); } self.finished = true; + // Every drop below is recorded in `self.dropped` so `final_report()` — the + // structured answer to "what is in this file" — cannot keep claiming a + // track the file does not have. A `tracing::warn` alone left the crate's + // own public report lying about the output. + let mut dropped = Vec::new(); // Drop tracks that never received a sample so moov carries no empty trak. - self.tracks.retain(|t| !t.samples.is_empty()); + self.tracks.retain(|t| { + let kept = !t.samples.is_empty(); + if !kept { + tracing::warn!( + stream = t.stream_idx, + codec = ?t.codec, + "mp4: track received no samples, dropping it (see final_report)" + ); + dropped.push((t.stream_idx, Mp4SkipReason::NoSamples)); + } + kept + }); // An audio track with samples but no sample entry cannot be DESCRIBED: the // moov would carry an stsd declaring entry_count=1 around an empty entry, // i.e. a structurally invalid mp4 returned as success. Drop it instead, and @@ -407,13 +491,17 @@ impl Stream for Mp4Sink { let describable = t.media != Media::Audio || t.audio_entry.is_some(); if !describable { tracing::warn!( + stream = t.stream_idx, codec = ?t.codec, samples = t.samples.len(), - "mp4: no audio frame yielded a parseable sample entry, dropping track" + "mp4: no audio frame yielded a parseable sample entry, dropping track \ + (see final_report)" ); + dropped.push((t.stream_idx, Mp4SkipReason::UndescribableAudio)); } describable }); + self.dropped.append(&mut dropped); if self.tracks.is_empty() { return Err(crate::error::Error::MuxEmpty.into()); } @@ -452,6 +540,14 @@ impl Stream for Mp4Sink { fn info(&self) -> &DiscTitle { &self.title } + + /// The streams `finish()` had to drop — see [`Self::final_report`] for the + /// reasons. The driver surfaces this as `MuxOutcome::undelivered_streams` so + /// the caller learns programmatically that the file is missing a stream the + /// pre-mux plan promised, instead of only in a log line. + fn undelivered_streams(&self) -> Vec { + self.dropped.iter().map(|&(i, _)| i).collect() + } } // ── per-track box assembly ─────────────────────────────────────────────────── @@ -560,12 +656,8 @@ fn audio_sample_durations(samples: &[Sample], timescale: u32) -> Vec { durs } -/// Standard frame rates as `(timescale, sample_duration, fps)` — exact integer -/// ratios so a CFR track has zero accumulated drift. -/// -/// The order of this table is NOT significant: [`detect_rate`] picks the entry -/// nearest the measured rate, so a new rate may be appended anywhere without -/// shadowing an existing one. +/// Standard frame rates as `(timescale, sample_duration)` — exact integer ratios +/// so a CFR track has zero accumulated drift. const STD_RATES: &[(u32, u32, f64)] = &[ (24000, 1001, 23.976), (24, 1, 24.0), @@ -577,12 +669,6 @@ const STD_RATES: &[(u32, u32, f64)] = &[ (60, 1, 60.0), ]; -/// How far the measured rate may sit from a [`STD_RATES`] entry and still snap to -/// it. Half an fps separates every neighbouring pair in the table (23.976/24 are -/// 0.024 apart, so both fall inside one another's window — which is exactly why -/// the match must be nearest-wins, not first-wins). -const RATE_TOLERANCE_FPS: f64 = 0.5; - /// Detect the constant frame rate from the median presentation delta, snapping /// to the nearest standard rate. Falls back to a 90 kHz timescale with a rounded /// duration when nothing matches (non-standard / too few samples). @@ -603,24 +689,11 @@ fn detect_rate(samples: &[Sample]) -> (u32, u32) { deltas.sort_unstable(); let median = deltas[deltas.len() / 2]; let fps = NS as f64 / median as f64; - // Snap to the NEAREST standard rate inside the tolerance window, not the - // first one inside it. First-match made the answer depend on table order: - // every 1000/1001 rate sits within 0.5 fps of its integer twin and precedes - // it, so an exact 24.000 / 30.000 / 60.000 fps source was always declared - // 24000/1001, 30000/1001, 60000/1001 — a 0.1% timing error over the whole - // track. Nearest-match is order-independent, so the fix cannot be undone by - // someone appending a rate to STD_RATES (which is why it is preferred over - // simply reordering the table). - let mut best: Option<(u32, u32, f64)> = None; for &(ts, dur, rate) in STD_RATES { - let d = (fps - rate).abs(); - if d < RATE_TOLERANCE_FPS && best.is_none_or(|(_, _, best_d)| d < best_d) { - best = Some((ts, dur, d)); + if (fps - rate).abs() < 0.5 { + return (ts, dur); } } - if let Some((ts, dur, _)) = best { - return (ts, dur); - } let dur = ((median as i128 * 90_000) / NS as i128).max(1) as u32; (90_000, dur) } @@ -748,37 +821,29 @@ fn build_dinf() -> Vec { bx(b"dinf", &dref) } -/// Colour signalling for the `colr` box (nclx, ISO/IEC 14496-12 §12.1.5): -/// (primaries, transfer, matrix, full_range) as ITU-T H.273 code points. `None` -/// when the stream carries no usable colour info. -/// -/// The code points come from [`crate::mux::mkv::cicp_for_video`] — the single -/// resolver EVERY sink shares (measured bitstream CICP first, then the coarse -/// `ColorSpace` enum with the HDR-driven transfer override). This box must never -/// carry its own copy of that mapping: the copy that used to live here had drifted -/// to hardcode transfer 16 (SMPTE ST 2084 / PQ) for all BT.2020 — tagging an HLG -/// title, whose transfer is 18 (ARIB STD-B67), as PQ — and transfer 6 (BT.601) for -/// BT.470 System B/G, whose transfer is 5. Both disagreed with the MKV sink and -/// the FVI sidecar for the same disc. +/// Colour signalling for the `colr` box (nclx): (primaries, transfer, matrix, +/// full_range). `None` when the stream carries no usable colour info. fn video_colr(stream: &DiscStream) -> Option<(u16, u16, u16, bool)> { let DiscStream::Video(v) = stream else { return None; }; - // No measured CICP and no colorimetry from the playlist → nothing usable to - // signal. The shared resolver returns the CICP "unspecified" triple (2/2/2) - // for that case; an ABSENT `colr` box already means exactly that, so omit the - // box rather than write it (unchanged behaviour for this sink). - if v.measured_cicp.is_none() && v.color_space == crate::disc::ColorSpace::Unknown { - return None; + if let Some(c) = v.measured_cicp { + return Some(( + c.primaries as u16, + c.transfer as u16, + c.matrix as u16, + c.range == 2, + )); } - let (matrix, transfer, primaries, range) = crate::mux::mkv::cicp_for_video(v); - Some(( - primaries as u16, - transfer as u16, - matrix as u16, - // MeasuredCicp/Matroska Range: 2 = full, 1 = limited (the disc norm). - range == 2, - )) + use crate::disc::ColorSpace::*; + let cicp = match v.color_space { + Bt709 => (1, 1, 1), + Bt2020 => (9, 16, 9), + Bt470bg => (5, 6, 5), + Smpte170m => (6, 6, 6), + Unknown => return None, + }; + Some((cicp.0, cicp.1, cicp.2, false)) } /// Video `stbl`: sample entry + `stts`(constant) + `stss` + `ctts` + `stsc` + @@ -1162,6 +1227,113 @@ mod tests { ); } + /// Dropping the undescribable audio track keeps the export succeeding (its + /// video is fine), but the crate must not then keep CLAIMING that stream: + /// `mp4_fit_report` — the only structured report — still lists it as + /// included, so a caller printing the plan reports a successful export of a + /// file with no audio at all. + /// + /// `final_report()` must therefore describe the FILE (the stream moved to + /// `skipped` with `UndescribableAudio`), and `undelivered_streams()` — which + /// the driver folds into `MuxOutcome::undelivered_streams` — must name it so + /// the loss is programmatic, not just a log line. + /// + /// Mutation check: stop recording the drop in `finish()` and the plan and the + /// file disagree again with nothing but a `tracing::warn` between them. + #[test] + fn dropped_audio_track_is_reported_not_just_logged() { + let t = title( + vec![hevc_video(), audio(Codec::Ac3, "eng")], + vec![Some(vec![1, 2, 3, 4]), None], + ); + + // The PRE-mux plan promises the audio stream. It cannot know better: the + // codec fits, only the frames turn out to be unparseable. + let plan = fit_report(&t); + assert_eq!(plan.included, vec![0, 1], "the plan promises both streams"); + + let mut s = Mp4Sink::create(std::io::Cursor::new(Vec::new()), &t).unwrap(); + s.write(&frame(0, 0, true, vec![0xAB; 800])).unwrap(); + // Not an AC-3 syncframe — `dolby_sample_entry` can never parse it. + s.write(&frame(1, 0, true, vec![0x5Au8; 64])).unwrap(); + assert!( + s.undelivered_streams().is_empty(), + "nothing is decided before finish()" + ); + s.finish().unwrap(); + + let actual = s.final_report(); + assert_eq!( + actual.included, + vec![0], + "the post-mux report must list only the video the file actually carries" + ); + assert!( + actual + .skipped + .contains(&(1, Mp4SkipReason::UndescribableAudio)), + "the dropped audio stream must appear as skipped with its reason: {:?}", + actual.skipped + ); + assert_eq!( + s.undelivered_streams(), + vec![1], + "the driver's programmatic loss signal must name stream 1" + ); + } + + /// `mvhd.next_track_id` must EXCEED every track_ID in the file (ISO/IEC + /// 14496-12 §8.2.2). It was derived from the retained track COUNT, so a + /// drop at `finish()` made it collide with a live id: ids [1, 3] retained → + /// count 2 → next_track_id 3, which is track 3. A tool appending a track with + /// that id creates a duplicate. + #[test] + fn mvhd_next_track_id_exceeds_every_retained_track_id() { + let t = title( + vec![ + hevc_video(), + audio(Codec::Ac3, "eng"), // track_id 2 — gets no samples, dropped + audio(Codec::Ac3, "fra"), // track_id 3 — survives + ], + vec![Some(vec![1, 2, 3, 4]), None, None], + ); + let mut s = Mp4Sink::create(std::io::Cursor::new(Vec::new()), &t).unwrap(); + s.write(&frame(0, 0, true, vec![0xAB; 800])).unwrap(); + // Nothing for stream 1; stream 2 gets real AC-3. + s.write(&frame(2, 0, true, ac3_frame())).unwrap(); + s.write(&frame(2, 32_000_000, true, ac3_frame())).unwrap(); + s.finish().unwrap(); + + // The middle track really was dropped (ids 1 and 3 retained). + assert_eq!(s.undelivered_streams(), vec![1]); + assert!( + s.final_report() + .skipped + .contains(&(1, Mp4SkipReason::NoSamples)) + ); + let retained_ids: Vec = s.tracks.iter().map(|t| t.track_id).collect(); + assert_eq!(retained_ids, vec![1, 3]); + + let buf = s.writer.into_inner(); + let boxes = walk(&buf); + let (_, ms, msz) = *boxes.iter().find(|(t, _, _)| t == b"moov").unwrap(); + let moov = &buf[ms + 8..ms + msz]; + // mvhd is moov's first child; next_track_id is its last 4 bytes. + let mvhd_size = u32::from_be_bytes([moov[0], moov[1], moov[2], moov[3]]) as usize; + assert_eq!(&moov[4..8], b"mvhd"); + let next_id = u32::from_be_bytes([ + moov[mvhd_size - 4], + moov[mvhd_size - 3], + moov[mvhd_size - 2], + moov[mvhd_size - 1], + ]); + assert!( + retained_ids.iter().all(|&id| next_id > id), + "next_track_id {next_id} must exceed every used id {retained_ids:?}" + ); + assert_eq!(next_id, 4); + } + #[test] fn av_mux_has_two_traks_and_tiles() { let t = title( @@ -1245,171 +1417,4 @@ mod tests { .collect(); assert_eq!(detect_rate(&samples), (24000, 1001)); } - - // ── colr (ITU-T H.273 / CICP) ──────────────────────────────────────────── - - /// Decode `(primaries, transfer, matrix, full_range)` back out of the `colr` - /// nclx box of an emitted visual sample entry, so the assertion is on the - /// bytes that reach the file. `None` when no `colr` box was written. - fn colr_of(v: &VideoStream) -> Option<(u16, u16, u16, bool)> { - // `codec_private` is a byte pattern that cannot itself contain "colr". - let stsd = build_visual_stsd( - Codec::Hevc, - &[0u8; 8], - 1920, - 1080, - video_colr(&DiscStream::Video(v.clone())), - ); - let i = stsd.windows(4).position(|w| w == b"colr")?; - let p = &stsd[i + 4..]; - assert_eq!(&p[..4], b"nclx", "only the nclx colour type is written"); - Some(( - u16::from_be_bytes([p[4], p[5]]), - u16::from_be_bytes([p[6], p[7]]), - u16::from_be_bytes([p[8], p[9]]), - p[10] & 0x80 != 0, - )) - } - - fn video_stream() -> VideoStream { - match hevc_video() { - DiscStream::Video(v) => v, - _ => unreachable!(), - } - } - - #[test] - fn colr_transfer_is_hlg_for_an_hlg_title_not_pq() { - // ITU-T H.273 Table 3: transfer 18 = ARIB STD-B67 (HLG), 16 = SMPTE - // ST 2084 (PQ). `video_colr` hardcoded 16 for every BT.2020 stream, so an - // HLG title got the PQ EOTF applied to it — while the MKV sink of the same - // rip correctly wrote 18. - let mut v = video_stream(); - v.hdr = HdrFormat::Hlg; - v.color_space = ColorSpace::Bt2020; - assert_eq!( - colr_of(&v).expect("colr written"), - (9, 18, 9, false), - "BT.2020 primaries/matrix (9) with the HLG transfer (18)" - ); - } - - #[test] - fn colr_transfer_is_bt470bg_for_a_pal_dvd_not_bt601() { - // ITU-T H.273: transfer 5 = ITU-R BT.470-6 System B/G, 6 = BT.601. - // A PAL DVD is System B/G in all three code points. - let mut v = video_stream(); - v.hdr = HdrFormat::Sdr; - v.color_space = ColorSpace::Bt470bg; - assert_eq!(colr_of(&v).expect("colr written"), (5, 5, 5, false)); - } - - #[test] - fn colr_agrees_with_the_shared_cicp_resolver_for_every_color_space() { - // One resolver, every sink: the `colr` box must carry exactly what - // `mkv::cicp_for_video` returns for the same stream, so an mp4:// rip and - // an mkv:// rip of one title can never describe different colour. - for cs in [ - ColorSpace::Bt709, - ColorSpace::Bt2020, - ColorSpace::Bt470bg, - ColorSpace::Smpte170m, - ] { - for hdr in [ - HdrFormat::Sdr, - HdrFormat::Hdr10, - HdrFormat::Hdr10Plus, - HdrFormat::Hlg, - HdrFormat::DolbyVision, - ] { - let mut v = video_stream(); - v.color_space = cs; - v.hdr = hdr; - let (m, t, p, r) = crate::mux::mkv::cicp_for_video(&v); - assert_eq!( - colr_of(&v).expect("colr written"), - (p as u16, t as u16, m as u16, r == 2), - "colr disagrees with the shared resolver for {cs:?} / {hdr:?}" - ); - } - } - // Unknown colorimetry: no usable colour info, so no `colr` box at all — - // an absent box and an "unspecified" (2/2/2) box mean the same thing, and - // writing nothing is what this sink has always done. - let mut v = video_stream(); - v.color_space = ColorSpace::Unknown; - assert!(colr_of(&v).is_none()); - } - - // ── detect_rate ────────────────────────────────────────────────────────── - - /// Mux a video-only MP4 whose samples are exactly `delta_ns` apart and return - /// the `(mdhd.timescale, stts.sample_delta)` decoded out of the emitted file. - fn muxed_video_timing(delta_ns: i64) -> (u32, u32) { - let t = title(vec![hevc_video()], vec![Some(vec![1, 2, 3, 4])]); - let mut s = Mp4Sink::create(std::io::Cursor::new(Vec::new()), &t).unwrap(); - for i in 0..10i64 { - s.write(&frame(0, i * delta_ns, i == 0, vec![0xAB; 16])) - .unwrap(); - } - s.finish().unwrap(); - let buf = s.writer.into_inner(); - - // One trak → exactly one `mdhd` and one `stts`. - let i = buf.windows(4).position(|w| w == b"mdhd").expect("mdhd"); - // After the type: version+flags(4), creation(8), modification(8), timescale(4). - let timescale = u32::from_be_bytes(buf[i + 24..i + 28].try_into().unwrap()); - let j = buf.windows(4).position(|w| w == b"stts").expect("stts"); - // After the type: version+flags(4), entry_count(4), sample_count(4), sample_delta(4). - let delta = u32::from_be_bytes(buf[j + 16..j + 20].try_into().unwrap()); - (timescale, delta) - } - - #[test] - fn exact_integer_frame_rates_are_not_declared_as_their_fractional_twins() { - // `detect_rate` returned the FIRST STD_RATES entry within 0.5 fps, and each - // 1000/1001 rate precedes its integer twin, so 24.000 / 30.000 / 60.000 - // were always written as 24000/1001, 30000/1001 and 60000/1001. The - // declared timescale/sample_delta is read back out of the muxed file. - for (delta_ns, want) in [ - (41_666_667i64, (24u32, 1u32)), // 24.000 - (33_333_333, (30, 1)), // 30.000 - (16_666_667, (60, 1)), // 60.000 - (40_000_000, (25, 1)), // 25.000 - (20_000_000, (50, 1)), // 50.000 - (41_708_333, (24_000, 1001)), // 23.976 - (33_366_667, (30_000, 1001)), // 29.97 - (16_683_333, (60_000, 1001)), // 59.94 - ] { - assert_eq!( - muxed_video_timing(delta_ns), - want, - "{delta_ns} ns/frame must be declared as {want:?}" - ); - } - } - - #[test] - fn detect_rate_picks_the_nearest_std_rate_regardless_of_table_order() { - // Order-independence is the property that keeps this fixed: every entry - // must resolve to itself when its own exact rate is measured, no matter - // where it sits in STD_RATES. A first-match rule can only satisfy this if - // the table happens to be ordered, which is what broke. - for &(ts, dur, rate) in STD_RATES { - let d = (NS as f64 / rate).round() as i64; - let samples: Vec = (0..10) - .map(|i| Sample { - offset: 0, - size: 1, - pts_ns: i as i64 * d, - keyframe: i == 0, - }) - .collect(); - assert_eq!( - detect_rate(&samples), - (ts, dur), - "{rate} fps must resolve to its own STD_RATES entry" - ); - } - } } diff --git a/src/mux/select.rs b/src/mux/select.rs index 6ceedb7..c23c670 100644 --- a/src/mux/select.rs +++ b/src/mux/select.rs @@ -51,7 +51,9 @@ impl StreamSelection { /// PID passes the corresponding [`PidFilter`]; drop the rest. Declared order /// is preserved. The parallel `codec_privates` vec is pruned in lockstep /// when it is populated (it is empty on a freshly-scanned title, non-empty - /// only if a caller pre-filled it). + /// only if a caller pre-filled it) — by index, whatever its length, since it + /// is consumed positionally and a partial prune would attach the wrong + /// codec-private to a retained track. /// /// Errors [`Error::SelectionPidUnknown`] if a filter lists a PID that does /// not exist in `title.streams` — a caller bug (e.g. a stale scan). Fail @@ -94,8 +96,6 @@ impl StreamSelection { } } - let codec_privates_aligned = title.codec_privates.len() == title.streams.len(); - // Retain by index so we can prune the parallel codec_privates in lockstep. let keep: Vec = title .streams @@ -109,14 +109,37 @@ impl StreamSelection { i += 1; k }); - if codec_privates_aligned { - let mut j = 0; - title.codec_privates.retain(|_| { - let k = keep[j]; - j += 1; - k - }); + // Prune `codec_privates` by the SAME index decision, whatever its length. + // + // This used to run only when `codec_privates.len() == streams.len()`, and + // do nothing otherwise. `codec_privates` is consumed positionally + // (`codec_privates[i]` describes `streams[i]` — see + // `TsMuxer::set_codec_private` / `Mp4Sink::create`), and `m2ts.rs::create` + // documents a longer-than-streams vec as benign ("ignore any trailing + // entries that exceed the track count"). So a title carrying one trailing + // extra entry skipped the prune entirely and every retained stream after + // the first dropped one silently got the PREVIOUS stream's + // codec_private — wrong SPS/PPS on the track, no error, no log. + // + // Indices at or past `keep`'s length can only be entries that already + // exceeded the stream count, i.e. describe no stream; drop them rather + // than leave them dangling behind the pruned list. + let extra = title.codec_privates.len().saturating_sub(keep.len()); + if extra > 0 { + tracing::debug!( + target: "mux", + codec_privates = title.codec_privates.len(), + streams = keep.len(), + "stream selection: dropping {extra} codec_private entry/entries that describe \ + no declared stream" + ); } + let mut j = 0; + title.codec_privates.retain(|_| { + let k = keep.get(j).copied().unwrap_or(false); + j += 1; + k + }); Ok(()) } @@ -307,6 +330,46 @@ mod tests { "codec_privates pruned to match the retained streams, in order" ); } + + /// A `codec_privates` vec that is NOT exactly stream-length must still be + /// pruned in lockstep. `m2ts.rs::create` documents a longer-than-streams vec + /// as a benign shape ("ignore any trailing entries that exceed the track + /// count"), and the vec is consumed positionally, so skipping the prune left + /// `codec_privates[i]` describing a stream that is no longer at index `i`. + /// + /// Regression: with one trailing extra entry the prune was skipped entirely + /// and index 1 — the retained `fra` audio — resolved to `eng`'s record, so + /// the muxer attached the wrong codec-private to the track. No error, no log. + #[test] + fn apply_prunes_codec_privates_even_when_length_does_not_match_streams() { + let mut t = title(); + t.streams.truncate(4); // video + eng + spa + fra audio + t.codec_privates = vec![ + Some(vec![0xAA]), // video 0x1011 + Some(vec![0x11]), // audio 0x1100 eng + Some(vec![0x22]), // audio 0x1101 spa + Some(vec![0x33]), // audio 0x1102 fra + Some(vec![0xEE]), // trailing extra — describes no declared stream + ]; + let sel = StreamSelection { + audio: PidFilter::Only(vec![0x1102]), + subtitle: PidFilter::All, + }; + sel.apply(&mut t).unwrap(); + + assert_eq!(pids(&t), vec![0x1011, 0x1102], "video + fra audio"); + assert_eq!( + t.codec_privates, + vec![Some(vec![0xAA]), Some(vec![0x33])], + "the retained fra track must keep ITS OWN codec_private, and the \ + trailing entry that describes no stream must not survive the prune" + ); + assert_eq!( + t.codec_privates.len(), + t.streams.len(), + "the two positional vecs must be aligned after apply()" + ); + } /// A PID listed in the WRONG class's filter must fail loud, not validate and /// then quietly vanish. Validation used to scan both audio and subtitle /// streams, so an audio filter naming a subtitle PID passed — and `keeps` diff --git a/src/pes.rs b/src/pes.rs index 102658e..28c34b7 100644 --- a/src/pes.rs +++ b/src/pes.rs @@ -234,6 +234,21 @@ pub trait Stream: Send { fn lost_bytes(&self) -> u64 { 0 } + + /// Sink side: `info().streams` indices this sink PLANNED to carry (and + /// accepted frames for) but could not put in the finished container, valid + /// after [`finish`](Self::finish). Empty for every sink that writes + /// everything it accepted — which is all of them except `mp4://`, whose + /// `finish()` must drop an audio track no frame of which yielded a parseable + /// sample entry (an `stsd` cannot describe it). + /// + /// This exists because such a drop otherwise contradicts the pre-mux plan the + /// crate publishes (`mp4_fit_report`), leaving the caller reporting a + /// successful export of a file missing a stream it was told would be there. + /// The driver folds this into `MuxOutcome::undelivered_streams`. + fn undelivered_streams(&self) -> Vec { + Vec::new() + } } /// Wraps any output stream and counts bytes written. @@ -297,6 +312,10 @@ impl Stream for CountingStream { self.inner.errors() } + fn undelivered_streams(&self) -> Vec { + self.inner.undelivered_streams() + } + fn lost_bytes(&self) -> u64 { self.inner.lost_bytes() }