From f6754b209e856f31b67ae3da555ef5fbdaa6fe34 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 23 May 2026 15:17:42 -0700 Subject: [PATCH 01/12] moq-net: replace const-generic Timescale with runtime Timestamp Ports the Timestamp/Timescale types from #1439 (without the wire-format, SUBSCRIBE_OK, or async-subscribe changes) so container::Frame can carry its source format's native scale instead of forcing a lossy detour through microseconds. - `Timescale` newtype around u64 with UNKNOWN/SECOND/MILLI/MICRO/NANO constants. - `Timestamp { value, scale }` carries scale at runtime; arithmetic panics on scale mismatch, `as_*`/`convert` return TimeOverflow on unspecified scales. - The old `Time = Timescale<1000>` alias is gone; jitter fields move to `Option`. container::Frame.timestamp now preserves the importer's scale: - fmp4 import stamps frames at the source `mdhd.timescale` instead of converting to micros, and export rescales once at `encode_fragment` rather than the legacy `as_micros() * scale / 1_000_000` round-trip. - mkv import keeps NANO; export rescales to MILLI at the wire boundary. - legacy and LOC encoders convert to MICRO at the wire boundary (legacy has no on-wire scale; LOC's catalog convention is micros). LOC decode preserves any per-frame timescale property. - jitter::MinFrameDuration normalizes its result to MILLI for the catalog field. Drive-by: - moq-net's `coding` module is now `pub` and `VarInt::{encode_quic, decode_quic}` are public so hang's wire format can encode without reaching `lite::Version`. - Container producer/consumer's latency/duration checks compare in nanoseconds via the fallible API. - libmoq, moq-ffi, moq-gst updated for the new fallible `as_*` / `Duration::try_from` shapes. `cargo test --workspace --exclude moq-gst` passes (all 141 moq-mux tests); `just check` clean. moq-gst couldn't be built locally without gstreamer-1.0 pkg-config; the edits there are mechanical. Note: the hang catalog `jitter` field changes its JSON shape (was a bare integer via the transparent Timescale<1000>(VarInt) newtype, now a `{value, scale}` object via the default derive on Timestamp). The existing round-trip test only covers `jitter: None` so it didn't catch this; a custom serde impl would be a follow-up if wire compatibility matters. Co-Authored-By: Claude Opus 4.7 (1M context) --- rs/hang/src/catalog/audio/mod.rs | 2 +- rs/hang/src/catalog/video/mod.rs | 2 +- rs/hang/src/container/frame.rs | 24 +- rs/libmoq/src/consume.rs | 2 +- rs/moq-ffi/src/consumer.rs | 1 + rs/moq-gst/src/source/imp.rs | 11 +- rs/moq-mux/src/catalog/hang/producer.rs | 8 +- rs/moq-mux/src/catalog/msf/consumer.rs | 4 +- rs/moq-mux/src/container/consumer.rs | 8 +- rs/moq-mux/src/container/fmp4/export.rs | 11 +- rs/moq-mux/src/container/fmp4/import.rs | 10 +- rs/moq-mux/src/container/fmp4/mod.rs | 19 +- rs/moq-mux/src/container/jitter.rs | 18 +- rs/moq-mux/src/container/loc/mod.rs | 13 +- rs/moq-mux/src/container/mkv/export.rs | 7 +- rs/moq-mux/src/container/mkv/import.rs | 3 +- rs/moq-mux/src/container/mod.rs | 17 +- rs/moq-mux/src/container/producer.rs | 14 +- rs/moq-net/src/coding/varint.rs | 4 +- rs/moq-net/src/lib.rs | 2 +- rs/moq-net/src/model/time.rs | 864 +++++++++++------------- 21 files changed, 532 insertions(+), 512 deletions(-) diff --git a/rs/hang/src/catalog/audio/mod.rs b/rs/hang/src/catalog/audio/mod.rs index 5fd8cd6dd..b88a686b1 100644 --- a/rs/hang/src/catalog/audio/mod.rs +++ b/rs/hang/src/catalog/audio/mod.rs @@ -88,5 +88,5 @@ pub struct AudioConfig { /// NOTE: The audio "frame" duration depends on the codec, sample rate, etc. /// ex: AAC often uses 1024 samples per frame, so at 44100Hz, this would be 1024/44100 = 23ms #[serde(default)] - pub jitter: Option, + pub jitter: Option, } diff --git a/rs/hang/src/catalog/video/mod.rs b/rs/hang/src/catalog/video/mod.rs index f3f221066..97ba63920 100644 --- a/rs/hang/src/catalog/video/mod.rs +++ b/rs/hang/src/catalog/video/mod.rs @@ -139,5 +139,5 @@ pub struct VideoConfig { /// - If there can be up to 3 b-frames in a row, this would be 3 * 1000/fps. /// - If frames are buffered into 2s segments, this would be 2s. #[serde(default)] - pub jitter: Option, + pub jitter: Option, } diff --git a/rs/hang/src/container/frame.rs b/rs/hang/src/container/frame.rs index 6cbd2cd93..5842c740c 100644 --- a/rs/hang/src/container/frame.rs +++ b/rs/hang/src/container/frame.rs @@ -1,9 +1,16 @@ use bytes::{Buf, Bytes, BytesMut}; use derive_more::Debug; +use moq_net::coding::VarInt; use crate::Error; -pub type Timestamp = moq_net::Timescale<1_000_000>; +pub use moq_net::{Timescale, Timestamp}; + +/// Canonical timescale for the hang legacy wire format: microseconds. +/// +/// The legacy container's on-wire timestamp is a single VarInt with no scale tag, +/// so encoders normalize to this scale and decoders attach it. +pub const TIMESCALE: Timescale = Timescale::MICRO; /// A media frame with a timestamp and codec-specific payload. /// @@ -30,9 +37,16 @@ pub struct Frame { impl Frame { /// Encode the frame to the given group as a single moq-lite frame: /// VarInt timestamp prefix followed by the raw codec payload. + /// + /// The timestamp is normalized to [`TIMESCALE`] (microseconds) on the wire so + /// peers using a different source scale (e.g. nanoseconds from MKV) can decode + /// without knowing the producer's internal scale. pub fn encode(&self, group: &mut moq_net::GroupProducer) -> Result<(), Error> { + let timestamp = self.timestamp.convert(TIMESCALE)?; + let value = VarInt::try_from(timestamp.value()).map_err(moq_net::Error::from)?; + let mut header = BytesMut::new(); - self.timestamp.encode(&mut header).map_err(moq_net::Error::from)?; + value.encode_quic(&mut header).map_err(moq_net::Error::from)?; let size = header.len() + self.payload.len(); @@ -45,8 +59,12 @@ impl Frame { } /// Decode a frame from raw bytes (VarInt timestamp prefix + payload). + /// + /// Attaches [`TIMESCALE`] (microseconds) to the decoded timestamp, matching what + /// [`Self::encode`] writes. pub fn decode(mut buf: impl Buf) -> Result { - let timestamp = Timestamp::decode(&mut buf)?; + let value: u64 = VarInt::decode_quic(&mut buf).map_err(moq_net::Error::from)?.into(); + let timestamp = Timestamp::new(value, TIMESCALE)?; let payload = buf.copy_to_bytes(buf.remaining()); Ok(Self { timestamp, payload }) diff --git a/rs/libmoq/src/consume.rs b/rs/libmoq/src/consume.rs index 13cc53f38..7f426185d 100644 --- a/rs/libmoq/src/consume.rs +++ b/rs/libmoq/src/consume.rs @@ -329,7 +329,7 @@ impl Consume { pub fn frame(&self, frame: Id, dst: &mut moq_frame) -> Result<(), Error> { let f = self.frame.get(frame).ok_or(Error::FrameNotFound)?; - let timestamp_us = f.timestamp.as_micros().try_into().map_err(|_| moq_net::TimeOverflow)?; + let timestamp_us = f.timestamp.as_micros()?.try_into().map_err(|_| moq_net::TimeOverflow)?; *dst = moq_frame { payload: f.payload.as_ptr(), diff --git a/rs/moq-ffi/src/consumer.rs b/rs/moq-ffi/src/consumer.rs index c3729f941..49a216f89 100644 --- a/rs/moq-ffi/src/consumer.rs +++ b/rs/moq-ffi/src/consumer.rs @@ -56,6 +56,7 @@ impl Media { let timestamp_us: u64 = frame .timestamp .as_micros() + .map_err(|_| MoqError::Codec("timestamp scale unspecified".into()))? .try_into() .map_err(|_| MoqError::Codec("timestamp overflow".into()))?; diff --git a/rs/moq-gst/src/source/imp.rs b/rs/moq-gst/src/source/imp.rs index 013ec73a9..3f9b7a81d 100644 --- a/rs/moq-gst/src/source/imp.rs +++ b/rs/moq-gst/src/source/imp.rs @@ -525,10 +525,13 @@ async fn run_track_pump( let buffer_mut = buffer.get_mut().unwrap(); let pts = match reference_ts { - Some(reference) => { - let delta: Duration = (timestamp - reference).into(); - gst::ClockTime::from_nseconds(delta.as_nanos() as u64) - } + Some(reference) => match timestamp + .checked_sub(reference) + .and_then(|d| Duration::try_from(d)) + { + Ok(delta) => gst::ClockTime::from_nseconds(delta.as_nanos() as u64), + Err(_) => gst::ClockTime::ZERO, + }, None => { reference_ts = Some(timestamp); gst::ClockTime::ZERO diff --git a/rs/moq-mux/src/catalog/hang/producer.rs b/rs/moq-mux/src/catalog/hang/producer.rs index 26d3e5504..036f0a5cb 100644 --- a/rs/moq-mux/src/catalog/hang/producer.rs +++ b/rs/moq-mux/src/catalog/hang/producer.rs @@ -173,7 +173,7 @@ fn to_msf(catalog: &hang::Catalog) -> moq_msf::Catalog { alt_group: if has_multiple_video { Some(1) } else { None }, max_grp_sap_starting_type: sap_type, max_obj_sap_starting_type: sap_type, - jitter: config.jitter.map(|t| t.as_millis() as f64), + jitter: config.jitter.and_then(|t| t.as_millis().ok()).map(|ms| ms as f64), }); } @@ -209,7 +209,7 @@ fn to_msf(catalog: &hang::Catalog) -> moq_msf::Catalog { alt_group: if has_multiple_audio { Some(1) } else { None }, max_grp_sap_starting_type: Some(1), max_obj_sap_starting_type: Some(1), - jitter: config.jitter.map(|t| t.as_millis() as f64), + jitter: config.jitter.and_then(|t| t.as_millis().ok()).map(|ms| ms as f64), }); } @@ -431,7 +431,7 @@ mod test { framerate: Some(30.0), optimize_for_latency: None, container: Container::Legacy, - jitter: Some(moq_net::Time::from_millis_unchecked(100)), + jitter: Some(moq_net::Timestamp::from_millis_unchecked(100)), }, ); @@ -445,7 +445,7 @@ mod test { bitrate: None, description: None, container: Container::Legacy, - jitter: Some(moq_net::Time::from_millis_unchecked(40)), + jitter: Some(moq_net::Timestamp::from_millis_unchecked(40)), }, ); diff --git a/rs/moq-mux/src/catalog/msf/consumer.rs b/rs/moq-mux/src/catalog/msf/consumer.rs index 548d91abe..a1543747d 100644 --- a/rs/moq-mux/src/catalog/msf/consumer.rs +++ b/rs/moq-mux/src/catalog/msf/consumer.rs @@ -223,7 +223,7 @@ fn video_config_from_msf(track: &moq_msf::Track) -> anyhow::Result= 0.0) - .and_then(|v| moq_net::Time::from_millis(v as u64).ok()), + .and_then(|v| moq_net::Timestamp::from_millis(v as u64).ok()), })) } @@ -270,7 +270,7 @@ fn audio_config_from_msf(track: &moq_msf::Track) -> anyhow::Result= 0.0) - .and_then(|v| moq_net::Time::from_millis(v as u64).ok()), + .and_then(|v| moq_net::Timestamp::from_millis(v as u64).ok()), })) } diff --git a/rs/moq-mux/src/container/consumer.rs b/rs/moq-mux/src/container/consumer.rs index c8dac45d5..46b53c9b9 100644 --- a/rs/moq-mux/src/container/consumer.rs +++ b/rs/moq-mux/src/container/consumer.rs @@ -127,7 +127,7 @@ impl Consumer { && current.sequence <= self.current { match current.poll_min_timestamp(waiter, &self.format) { - Poll::Ready(Ok(ts)) => Some::(ts.into()), + Poll::Ready(Ok(ts)) => std::time::Duration::try_from(ts).ok(), _ => None, } } else { @@ -155,7 +155,9 @@ impl Consumer { } if let Poll::Ready(Ok(ts)) = group.poll_max_timestamp(waiter, &self.format) { - max_timestamp = max_timestamp.max(ts.into()); + if let Ok(duration) = std::time::Duration::try_from(ts) { + max_timestamp = max_timestamp.max(duration); + } break; // We know older groups won't be newer than this. } } @@ -887,7 +889,7 @@ mod tests { let frames = read_all(&mut consumer).await.unwrap(); assert_eq!(frames.len(), 1); assert_eq!(frames[0].timestamp, ts(one_hour)); - assert_eq!(frames[0].timestamp.as_micros(), one_hour as u128); + assert_eq!(frames[0].timestamp.as_micros().unwrap(), one_hour as u128); } #[tokio::test] diff --git a/rs/moq-mux/src/container/fmp4/export.rs b/rs/moq-mux/src/container/fmp4/export.rs index 536c7dd63..8adfef74a 100644 --- a/rs/moq-mux/src/container/fmp4/export.rs +++ b/rs/moq-mux/src/container/fmp4/export.rs @@ -449,8 +449,13 @@ fn should_flush(track: &Fmp4Track, frame: &Frame, fragment_duration: Option true, Some(d) => { let first = track.buffer.first().unwrap(); - let delta_us = frame.timestamp.as_micros().saturating_sub(first.timestamp.as_micros()); - delta_us >= d.as_micros() + // Compare in nanoseconds so the check works regardless of the source + // scale; an unspecified scale conservatively skips the duration cap and + // lets the next keyframe (or `None` fallback) flush. + match (first.timestamp.as_nanos(), frame.timestamp.as_nanos()) { + (Ok(first_ns), Ok(frame_ns)) => frame_ns.saturating_sub(first_ns) >= d.as_nanos(), + _ => false, + } } // No video keyframe will ever arrive to roll the fragment, so for // audio-only broadcasts in `None` mode we fall back to per-frame @@ -466,7 +471,7 @@ fn encode_fragment(track: &mut Fmp4Track, frames: Vec) -> anyhow::Result< track.sequence_number += 1; Ok(crate::container::fmp4::encode_fragment( track.track_id, - track.timescale, + crate::container::Timescale::new(track.timescale), seq, &frames, )?) diff --git a/rs/moq-mux/src/container/fmp4/import.rs b/rs/moq-mux/src/container/fmp4/import.rs index f819c2ce0..22eed4e5f 100644 --- a/rs/moq-mux/src/container/fmp4/import.rs +++ b/rs/moq-mux/src/container/fmp4/import.rs @@ -453,7 +453,7 @@ impl Import { let tfdt = traf.tfdt.as_ref().context("missing tfdt box")?; let mut dts = tfdt.base_media_decode_time; - let timescale = trak.mdia.mdhd.timescale as u64; + let timescale = crate::container::Timescale::new(trak.mdia.mdhd.timescale as u64); let mut offset = traf.tfhd.base_data_offset.unwrap_or_default() as usize; let mut track_data_start: Option = None; @@ -501,7 +501,9 @@ impl Import { .unwrap_or(tfhd.default_sample_size.unwrap_or(default_sample_size)) as usize; let pts = (dts as i64 + entry.cts.unwrap_or_default() as i64) as u64; - let timestamp = crate::container::Timestamp::from_scale(pts, timescale)?; + // Preserve the fmp4 track's native timescale so a passthrough re-emit + // doesn't go through a lossy microsecond detour. + let timestamp = crate::container::Timestamp::new(pts, timescale)?; if offset + size > mdat.data.len() { anyhow::bail!("invalid data offset"); @@ -642,7 +644,7 @@ impl Import { .renditions .get_mut(&track.track.name) .context("missing video config")?; - config.jitter = Some(jitter.convert()?); + config.jitter = Some(jitter.convert(crate::container::Timescale::MILLI)?); } TrackKind::Audio => { let config = catalog @@ -650,7 +652,7 @@ impl Import { .renditions .get_mut(&track.track.name) .context("missing audio config")?; - config.jitter = Some(jitter.convert()?); + config.jitter = Some(jitter.convert(crate::container::Timescale::MILLI)?); } } } diff --git a/rs/moq-mux/src/container/fmp4/mod.rs b/rs/moq-mux/src/container/fmp4/mod.rs index ff2c7d437..93c0d8e27 100644 --- a/rs/moq-mux/src/container/fmp4/mod.rs +++ b/rs/moq-mux/src/container/fmp4/mod.rs @@ -109,7 +109,7 @@ impl Container for Wire { type Error = Error; fn write(&self, group: &mut moq_net::GroupProducer, frames: &[Frame]) -> Result<(), Self::Error> { - let timescale = self.trak.mdia.mdhd.timescale as u64; + let timescale = crate::container::Timescale::new(self.trak.mdia.mdhd.timescale as u64); let track_id = self.trak.tkhd.track_id; encode(group, frames, timescale, track_id) } @@ -125,12 +125,12 @@ impl Container for Wire { return Poll::Ready(Ok(None)); }; - let timescale = self.trak.mdia.mdhd.timescale as u64; + let timescale = crate::container::Timescale::new(self.trak.mdia.mdhd.timescale as u64); Poll::Ready(Ok(Some(decode(data, timescale)?))) } } -pub(crate) fn decode(data: Bytes, timescale: u64) -> Result, Error> { +pub(crate) fn decode(data: Bytes, timescale: crate::container::Timescale) -> Result, Error> { use mp4_atom::DecodeMaybe; let mut cursor = std::io::Cursor::new(&data); @@ -169,7 +169,8 @@ pub(crate) fn decode(data: Bytes, timescale: u64) -> Result, Error> { let cts = entry.cts.unwrap_or_default() as i64; let pts = dts.checked_add_signed(cts).ok_or(Error::PtsOverflow)?; - let timestamp = Timestamp::from_scale(pts, timescale)?; + // Preserve the fmp4 track's native scale through the pipeline. + let timestamp = Timestamp::new(pts, timescale)?; let payload = Bytes::copy_from_slice(&mdat_data[offset..end]); let flags = entry.flags.unwrap_or(0); // depends_on_no_other (bits 24-25 == 0x2) means keyframe @@ -192,7 +193,7 @@ pub(crate) fn decode(data: Bytes, timescale: u64) -> Result, Error> { pub(crate) fn encode( group: &mut moq_net::GroupProducer, frames: &[Frame], - timescale: u64, + timescale: crate::container::Timescale, track_id: u32, ) -> Result<(), Error> { if frames.is_empty() { @@ -218,7 +219,7 @@ pub(crate) fn encode( /// Returns an empty `Bytes` when `frames` is empty. pub(crate) fn encode_fragment( track_id: u32, - timescale: u64, + timescale: crate::container::Timescale, sequence_number: u32, frames: &[Frame], ) -> Result { @@ -228,7 +229,11 @@ pub(crate) fn encode_fragment( return Ok(Bytes::new()); } - let dts = (frames[0].timestamp.as_micros() * timescale as u128 / 1_000_000) as u64; + // Re-express the first frame's timestamp at the target track's scale. When the + // importer preserved the source scale (the common passthrough case), this is a + // no-op; otherwise it's a single rescale rather than the legacy `micros * scale + // / 1_000_000` round-trip. + let dts = frames[0].timestamp.as_scale(timescale)? as u64; let entries: Vec<_> = frames .iter() diff --git a/rs/moq-mux/src/container/jitter.rs b/rs/moq-mux/src/container/jitter.rs index 4d3e67299..f733dbea3 100644 --- a/rs/moq-mux/src/container/jitter.rs +++ b/rs/moq-mux/src/container/jitter.rs @@ -18,19 +18,23 @@ impl MinFrameDuration { /// Record a new frame timestamp. /// - /// Returns the new minimum-frame-duration as a `moq_net::Time` if it - /// changed, so the caller can persist it on the catalog rendition. Returns - /// `None` when this is the first observation, the timestamps are - /// non-monotonic, or the new gap is no smaller than the recorded minimum. - pub fn observe(&mut self, ts: Timestamp) -> Option { + /// Returns the new minimum-frame-duration as a millisecond-scale [`Timestamp`] + /// if it changed, so the caller can persist it on the catalog rendition. + /// Returns `None` when this is the first observation, the timestamps are + /// non-monotonic, the new gap is no smaller than the recorded minimum, or + /// the scale conversion to milliseconds overflows. + pub fn observe(&mut self, ts: Timestamp) -> Option { let last = self.last_timestamp.replace(ts)?; let duration = ts.checked_sub(last).ok()?; - if duration >= self.min_duration.unwrap_or(Timestamp::MAX) { + // Sentinel max preserves the source scale on the first real observation. + if let Some(min) = self.min_duration + && duration >= min + { return None; } self.min_duration = Some(duration); - duration.convert().ok() + duration.convert(moq_net::Timescale::MILLI).ok() } } diff --git a/rs/moq-mux/src/container/loc/mod.rs b/rs/moq-mux/src/container/loc/mod.rs index 5efc3dbb6..3a3dfb534 100644 --- a/rs/moq-mux/src/container/loc/mod.rs +++ b/rs/moq-mux/src/container/loc/mod.rs @@ -7,20 +7,21 @@ use std::task::Poll; -use crate::container::{Container, Frame, Timestamp}; +use crate::container::{Container, Frame, TIMESCALE, Timescale, Timestamp}; /// LOC wire format. Each moq frame holds one LOC frame. #[derive(Default)] pub struct Wire; -const DEFAULT_TIMESCALE: u64 = 1_000_000; - impl Container for Wire { type Error = crate::Error; fn write(&self, group: &mut moq_net::GroupProducer, frames: &[Frame]) -> Result<(), Self::Error> { for frame in frames { - let data = moq_loc::encode(frame.timestamp.as_micros() as u64, &frame.payload)?; + // LOC's wire format omits per-frame timescale by convention; the catalog + // default is microseconds, so convert at the boundary. + let timestamp = frame.timestamp.convert(TIMESCALE).map_err(hang::Error::from)?; + let data = moq_loc::encode(timestamp.value(), &frame.payload)?; let mut chunked = group.create_frame(data.len().into())?; chunked.write(data)?; @@ -41,8 +42,8 @@ impl Container for Wire { }; let loc = moq_loc::decode(data)?; - let timescale = loc.timescale.unwrap_or(DEFAULT_TIMESCALE); - let timestamp = Timestamp::from_scale(loc.timestamp, timescale).map_err(hang::Error::from)?; + let scale = loc.timescale.map(Timescale::new).unwrap_or(TIMESCALE); + let timestamp = Timestamp::new(loc.timestamp, scale).map_err(hang::Error::from)?; Poll::Ready(Ok(Some(vec![Frame { timestamp, diff --git a/rs/moq-mux/src/container/mkv/export.rs b/rs/moq-mux/src/container/mkv/export.rs index 9ef5dd8e0..787f77c8b 100644 --- a/rs/moq-mux/src/container/mkv/export.rs +++ b/rs/moq-mux/src/container/mkv/export.rs @@ -447,7 +447,12 @@ impl Export { let kind = track.kind; let payload = &frame.payload; - let frame_ticks: u64 = (frame.timestamp.as_micros() / 1_000) + // MKV's wire scale is ms (TIMESTAMP_SCALE_NS = 1_000_000). Re-express the + // frame's timestamp directly at MILLI rather than going through micros. + let frame_ticks: u64 = frame + .timestamp + .as_millis() + .context("timestamp scale unspecified")? .try_into() .context("timestamp doesn't fit in u64 ms")?; diff --git a/rs/moq-mux/src/container/mkv/import.rs b/rs/moq-mux/src/container/mkv/import.rs index 984446723..f962fa5e8 100644 --- a/rs/moq-mux/src/container/mkv/import.rs +++ b/rs/moq-mux/src/container/mkv/import.rs @@ -344,7 +344,8 @@ impl Import { return Ok(()); }; - // Compute PTS in nanoseconds, then convert to the Timestamp's microsecond timescale. + // Compute PTS in MKV's native nanosecond units and stamp it on the + // timestamp at NANO scale so a passthrough re-emit preserves precision. let block_ticks = (self.cluster_timestamp as i64) + (rel_ts as i64); anyhow::ensure!(block_ticks >= 0, "negative block timestamp"); diff --git a/rs/moq-mux/src/container/mod.rs b/rs/moq-mux/src/container/mod.rs index 1050202e7..5b06401d3 100644 --- a/rs/moq-mux/src/container/mod.rs +++ b/rs/moq-mux/src/container/mod.rs @@ -29,9 +29,12 @@ pub use consumer::Consumer; pub use producer::Producer; pub(crate) use source::{CatalogSource, ExportSource}; -/// Microsecond presentation timestamp, the canonical timebase for media -/// frames in moq-mux. -pub type Timestamp = moq_net::Timescale<1_000_000>; +pub use moq_net::{Timescale, Timestamp}; + +/// Canonical timescale for the hang catalog and legacy/LOC wire formats: +/// microseconds. Containers with a richer wire format (CMAF, MKV) carry their +/// own timescale per-track instead. +pub const TIMESCALE: Timescale = Timescale::MICRO; /// A decoded media frame: timestamp, payload bytes, keyframe flag. /// @@ -42,8 +45,12 @@ pub type Timestamp = moq_net::Timescale<1_000_000>; pub struct Frame { /// Presentation timestamp. /// - /// Microsecond precision. Frames within a track must be in *decode* - /// order, not display order. B-frames may have non-monotonic + /// Carries its own [`Timescale`]; importers preserve the source format's + /// native scale (e.g. nanoseconds from MKV, the mdhd timescale from fMP4) + /// so exporters can re-emit without a lossy detour through microseconds. + /// Wire formats that lack an on-wire scale ([`legacy`], [`loc`]) normalize + /// to [`TIMESCALE`] at the encode boundary. Frames within a track must be + /// in *decode* order, not display order. B-frames may have non-monotonic /// presentation timestamps. pub timestamp: Timestamp, diff --git a/rs/moq-mux/src/container/producer.rs b/rs/moq-mux/src/container/producer.rs index 2c53fc1d2..b08f34b7d 100644 --- a/rs/moq-mux/src/container/producer.rs +++ b/rs/moq-mux/src/container/producer.rs @@ -90,12 +90,16 @@ impl Producer { } else { self.buffer.push(frame); - // Check if buffered duration exceeds latency. + // Check if buffered duration exceeds latency. Compare in nanoseconds so the + // check works regardless of the frame's native scale; if either timestamp's + // scale is unspecified we conservatively skip the check and let the next + // keyframe (or `finish()`) flush. if self.buffer.len() >= 2 { - let first_ts: std::time::Duration = self.buffer.first().unwrap().timestamp.into(); - let last_ts: std::time::Duration = self.buffer.last().unwrap().timestamp.into(); - - if last_ts.saturating_sub(first_ts) >= self.latency { + let first = self.buffer.first().unwrap().timestamp.as_nanos(); + let last = self.buffer.last().unwrap().timestamp.as_nanos(); + if let (Ok(first), Ok(last)) = (first, last) + && last.saturating_sub(first) >= self.latency.as_nanos() + { self.flush()?; } } diff --git a/rs/moq-net/src/coding/varint.rs b/rs/moq-net/src/coding/varint.rs index 81e6b8ca8..bd35d9f45 100644 --- a/rs/moq-net/src/coding/varint.rs +++ b/rs/moq-net/src/coding/varint.rs @@ -169,7 +169,7 @@ impl fmt::Display for VarInt { impl VarInt { /// Decode a QUIC-style varint (2-bit length tag in top bits). - fn decode_quic(r: &mut R) -> Result { + pub fn decode_quic(r: &mut R) -> Result { if !r.has_remaining() { return Err(DecodeError::Short); } @@ -210,7 +210,7 @@ impl VarInt { } /// Encode a QUIC-style varint (2-bit length tag in top bits). - fn encode_quic(&self, w: &mut W) -> Result<(), EncodeError> { + pub fn encode_quic(&self, w: &mut W) -> Result<(), EncodeError> { let remaining = w.remaining_mut(); if self.0 < (1u64 << 6) { if remaining < 1 { diff --git a/rs/moq-net/src/lib.rs b/rs/moq-net/src/lib.rs index cff083c27..5c43c5ebb 100644 --- a/rs/moq-net/src/lib.rs +++ b/rs/moq-net/src/lib.rs @@ -49,7 +49,7 @@ //! standard [`std::task::Waker`] API and any [`std::task::Waker`] is a valid driver. mod client; -mod coding; +pub mod coding; mod error; mod ietf; mod lite; diff --git a/rs/moq-net/src/model/time.rs b/rs/moq-net/src/model/time.rs index c24629cff..9076a2d3b 100644 --- a/rs/moq-net/src/model/time.rs +++ b/rs/moq-net/src/model/time.rs @@ -1,65 +1,156 @@ use rand::Rng; -use crate::Error; use crate::coding::{Decode, DecodeError, Encode, EncodeError, VarInt}; use std::sync::LazyLock; use std::time::{SystemTime, UNIX_EPOCH}; -/// A timestamp representing the presentation time in milliseconds. -/// -/// The underlying implementation supports any scale, but everything uses milliseconds by default. -pub type Time = Timescale<1_000>; - -/// Returned when a [`Timescale`] operation would exceed the QUIC VarInt range -/// (`2^62 - 1`) or overflow during scale conversion or arithmetic. +/// Returned when a [`Timestamp`] operation would exceed the QUIC VarInt range +/// (`2^62 - 1`), overflow during scale conversion or arithmetic, hit a divide +/// by zero from an unspecified ([`Timescale::UNKNOWN`]) scale, or attempt +/// arithmetic between timestamps with mismatched scales. #[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)] #[error("time overflow")] pub struct TimeOverflow; -/// A timestamp representing the presentation time in a given scale. ex. 1000 for milliseconds. +/// Units per second used by a track for frame timestamps. /// -/// All timestamps within a track are relative, so zero for one track is not zero for another. -/// Values are constrained to fit within a QUIC VarInt (2^62) so they can be encoded and decoded easily. +/// Newtype around `u64`. Use the named constants ([`Self::SECOND`], [`Self::MILLI`], +/// [`Self::MICRO`], [`Self::NANO`]) instead of writing raw integers at call sites. /// -/// This is [std::time::Instant] and [std::time::Duration] merged into one type for simplicity. -#[derive(Clone, Default, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +/// [`Self::UNKNOWN`] (raw value `0`) denotes an unspecified scale, produced by +/// [`Timestamp::ZERO`] and [`Timestamp::MAX`] sentinels. Unit conversions against +/// an unknown scale return [`TimeOverflow`] to avoid divide by zero. +#[derive(Clone, Copy, Default, PartialEq, Eq, Hash, PartialOrd, Ord)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] -pub struct Timescale(VarInt); +pub struct Timescale(pub u64); + +impl Timescale { + /// Unspecified scale. Conversions involving this return [`TimeOverflow`]. + pub const UNKNOWN: Self = Self(0); + /// One unit per second (`1`). + pub const SECOND: Self = Self(1); + /// 1,000 units per second (`1_000`). + pub const MILLI: Self = Self(1_000); + /// 1,000,000 units per second (`1_000_000`). Common default for media tracks. + pub const MICRO: Self = Self(1_000_000); + /// 1,000,000,000 units per second (`1_000_000_000`). + pub const NANO: Self = Self(1_000_000_000); + + /// Construct a timescale from a raw value (units per second). `0` means [`Self::UNKNOWN`]. + pub const fn new(units_per_second: u64) -> Self { + Self(units_per_second) + } + + /// The raw units-per-second value. + pub const fn as_u64(self) -> u64 { + self.0 + } + + /// Whether this is [`Self::UNKNOWN`] (raw value `0`). + pub const fn is_unknown(self) -> bool { + self.0 == 0 + } +} + +impl From for Timescale { + fn from(units_per_second: u64) -> Self { + Self(units_per_second) + } +} -impl Timescale { - /// The maximum representable instant. - pub const MAX: Self = Self(VarInt::MAX); +impl From for u64 { + fn from(scale: Timescale) -> Self { + scale.0 + } +} - /// The minimum representable instant. - pub const ZERO: Self = Self(VarInt::ZERO); +impl std::fmt::Debug for Timescale { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match *self { + Self::UNKNOWN => write!(f, "Timescale::UNKNOWN"), + Self::SECOND => write!(f, "Timescale::SECOND"), + Self::MILLI => write!(f, "Timescale::MILLI"), + Self::MICRO => write!(f, "Timescale::MICRO"), + Self::NANO => write!(f, "Timescale::NANO"), + Self(n) => write!(f, "Timescale({n})"), + } + } +} - /// Construct a timestamp directly from a value in this scale's units. Infallible - /// because any `u32` fits within the 62-bit varint range. - pub const fn new(value: u32) -> Self { - Self(VarInt::from_u32(value)) +impl std::fmt::Display for Timescale { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) } +} - /// Construct a timestamp directly from a value in this scale's units. Returns - /// [`TimeOverflow`] if `value` exceeds the 62-bit varint range. - pub const fn new_u64(value: u64) -> Result { +/// A timestamp in a track's timescale (units per second). +/// +/// All timestamps within a track are relative, so zero for one track is not zero for another. +/// The underlying value is constrained to fit within a QUIC VarInt (`2^62 - 1`) so it can be +/// encoded and decoded easily; the scale is carried alongside so frames from different +/// sources can be compared and converted without lossy detours through a single fixed scale. +/// +/// [`Timescale::UNKNOWN`] denotes an unspecified timescale, produced by [`Timestamp::ZERO`] +/// and [`Timestamp::MAX`]. Unit conversions and arithmetic against an unknown scale return +/// [`TimeOverflow`] to avoid divide by zero. +#[derive(Clone, Default, Copy, PartialEq, Eq, Hash)] +#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] +pub struct Timestamp { + value: VarInt, + scale: Timescale, +} + +impl Timestamp { + /// A zero timestamp with an unspecified scale. + /// + /// Useful as a sentinel min: comparisons against unspecified-scale timestamps + /// compare raw values, so `Timestamp::ZERO < t` is true for any `t` whose + /// `value > 0`. See [`Self::cmp`]. + pub const ZERO: Self = Self { + value: VarInt::ZERO, + scale: Timescale::UNKNOWN, + }; + + /// The maximum representable timestamp value, with an unspecified scale. + /// + /// Useful as a sentinel max: `t < Timestamp::MAX` is true for any `t` whose + /// `value < VarInt::MAX`. See [`Self::cmp`]. + pub const MAX: Self = Self { + value: VarInt::MAX, + scale: Timescale::UNKNOWN, + }; + + /// Construct a timestamp directly from a raw value at the given scale. + pub const fn new(value: u64, scale: Timescale) -> Result { match VarInt::from_u64(value) { - Some(varint) => Ok(Self(varint)), + Some(value) => Ok(Self { value, scale }), None => Err(TimeOverflow), } } - /// Convert a number of seconds to a timestamp, returning an error if the timestamp would overflow. - pub const fn from_secs(seconds: u64) -> Result { - // Not using from_scale because it'll be slightly faster - match seconds.checked_mul(SCALE) { - Some(value) => Self::new_u64(value), + /// Convert `value` measured at `source` (units per second) to a timestamp at `target`. + /// + /// Returns [`TimeOverflow`] on overflow or if `source` is [`Timescale::UNKNOWN`]. + pub const fn from_scale(value: u64, source: Timescale, target: Timescale) -> Result { + if source.0 == 0 { + return Err(TimeOverflow); + } + match (value as u128).checked_mul(target.0 as u128) { + Some(scaled) => match VarInt::from_u128(scaled / source.0 as u128) { + Some(value) => Ok(Self { value, scale: target }), + None => Err(TimeOverflow), + }, None => Err(TimeOverflow), } } - /// Like [`Self::from_secs`] but panics on overflow. Intended for `const` - /// initializers where overflow indicates a bug, not a runtime condition. + /// Convert a number of seconds to a timestamp at [`Timescale::SECOND`]. + pub const fn from_secs(seconds: u64) -> Result { + Self::new(seconds, Timescale::SECOND) + } + + /// Like [`Self::from_secs`] but panics on overflow. pub const fn from_secs_unchecked(seconds: u64) -> Self { match Self::from_secs(seconds) { Ok(time) => time, @@ -67,187 +158,197 @@ impl Timescale { } } - /// Convert a number of milliseconds to a timestamp, returning an error if the timestamp would overflow. + /// Convert a number of milliseconds to a timestamp at [`Timescale::MILLI`]. pub const fn from_millis(millis: u64) -> Result { - Self::from_scale(millis, 1000) + Self::new(millis, Timescale::MILLI) } /// Like [`Self::from_millis`] but panics on overflow. pub const fn from_millis_unchecked(millis: u64) -> Self { - Self::from_scale_unchecked(millis, 1000) + match Self::from_millis(millis) { + Ok(time) => time, + Err(_) => panic!("time overflow"), + } } - /// Convert a number of microseconds to a timestamp, returning an error on overflow. + /// Convert a number of microseconds to a timestamp at [`Timescale::MICRO`]. pub const fn from_micros(micros: u64) -> Result { - Self::from_scale(micros, 1_000_000) + Self::new(micros, Timescale::MICRO) } /// Like [`Self::from_micros`] but panics on overflow. pub const fn from_micros_unchecked(micros: u64) -> Self { - Self::from_scale_unchecked(micros, 1_000_000) + match Self::from_micros(micros) { + Ok(time) => time, + Err(_) => panic!("time overflow"), + } } - /// Convert a number of nanoseconds to a timestamp, returning an error on overflow. + /// Convert a number of nanoseconds to a timestamp at [`Timescale::NANO`]. pub const fn from_nanos(nanos: u64) -> Result { - Self::from_scale(nanos, 1_000_000_000) + Self::new(nanos, Timescale::NANO) } /// Like [`Self::from_nanos`] but panics on overflow. pub const fn from_nanos_unchecked(nanos: u64) -> Self { - Self::from_scale_unchecked(nanos, 1_000_000_000) + match Self::from_nanos(nanos) { + Ok(time) => time, + Err(_) => panic!("time overflow"), + } } - /// Construct from `value` measured at the given `scale` (units per second), rescaling - /// to `SCALE`. Returns [`TimeOverflow`] if the rescaled value exceeds 2^62. - pub const fn from_scale(value: u64, scale: u64) -> Result { - match VarInt::from_u128(value as u128 * SCALE as u128 / scale as u128) { - Some(varint) => Ok(Self(varint)), - None => Err(TimeOverflow), - } + /// The raw value in the timestamp's own scale. + pub const fn value(self) -> u64 { + self.value.into_inner() } - /// Like [`Self::from_scale`] but accepts a `u128` source value. - pub const fn from_scale_u128(value: u128, scale: u64) -> Result { - match value.checked_mul(SCALE as u128) { - Some(value) => match VarInt::from_u128(value / scale as u128) { - Some(varint) => Ok(Self(varint)), - None => Err(TimeOverflow), - }, - None => Err(TimeOverflow), - } + /// The scale (units per second) attached to this timestamp. + pub const fn scale(self) -> Timescale { + self.scale } - /// Like [`Self::from_scale`] but panics on overflow. - pub const fn from_scale_unchecked(value: u64, scale: u64) -> Self { - match Self::from_scale(value, scale) { - Ok(time) => time, - Err(_) => panic!("time overflow"), + /// Whether the scale is [`Timescale::UNKNOWN`]. Unit conversions and cross-scale + /// arithmetic against this timestamp return [`TimeOverflow`]. + pub const fn is_unspecified(self) -> bool { + self.scale.0 == 0 + } + + /// Whether the raw value is zero. Does not consider scale. + pub const fn is_zero(self) -> bool { + self.value.into_inner() == 0 + } + + /// Re-express this timestamp at a new scale. Returns [`TimeOverflow`] if the new + /// value would exceed `2^62 - 1`, the source scale is unspecified, or `new_scale` + /// is [`Timescale::UNKNOWN`]. + pub const fn convert(self, new_scale: Timescale) -> Result { + if self.scale.0 == 0 || new_scale.0 == 0 { + return Err(TimeOverflow); + } + if self.scale.0 == new_scale.0 { + return Ok(self); } + Self::from_scale(self.value.into_inner(), self.scale, new_scale) } - /// Get the timestamp as seconds. - pub const fn as_secs(self) -> u64 { - self.0.into_inner() / SCALE + /// The value re-expressed at `target` as a `u128`. Returns [`TimeOverflow`] + /// if the source scale is unspecified or `target` is [`Timescale::UNKNOWN`]. + pub const fn as_scale(self, target: Timescale) -> Result { + if self.scale.0 == 0 || target.0 == 0 { + return Err(TimeOverflow); + } + Ok(self.value.into_inner() as u128 * target.0 as u128 / self.scale.0 as u128) } - /// Get the timestamp as milliseconds. - // - // This returns a u128 to avoid a possible overflow when SCALE < 250 - pub const fn as_millis(self) -> u128 { - self.as_scale(1000) + /// The value re-expressed in seconds. Returns [`TimeOverflow`] if the scale is + /// unspecified. + pub const fn as_secs(self) -> Result { + if self.scale.0 == 0 { + return Err(TimeOverflow); + } + Ok(self.value.into_inner() / self.scale.0) } - /// Get the timestamp as microseconds. - pub const fn as_micros(self) -> u128 { - self.as_scale(1_000_000) + /// The value re-expressed in milliseconds. Returns [`TimeOverflow`] if the scale + /// is unspecified. + pub const fn as_millis(self) -> Result { + self.as_scale(Timescale::MILLI) } - /// Get the timestamp as nanoseconds. - pub const fn as_nanos(self) -> u128 { - self.as_scale(1_000_000_000) + /// The value re-expressed in microseconds. Returns [`TimeOverflow`] if the scale + /// is unspecified. + pub const fn as_micros(self) -> Result { + self.as_scale(Timescale::MICRO) } - /// Convert this timestamp to the given `scale` (units per second). - pub const fn as_scale(self, scale: u64) -> u128 { - self.0.into_inner() as u128 * scale as u128 / SCALE as u128 + /// The value re-expressed in nanoseconds. Returns [`TimeOverflow`] if the scale + /// is unspecified. + pub const fn as_nanos(self) -> Result { + self.as_scale(Timescale::NANO) } - /// Get the maximum of two timestamps. + /// Return the larger of two timestamps. + /// + /// Panics if the scales differ. Use [`Self::convert`] first if you need to compare + /// across scales. pub const fn max(self, other: Self) -> Self { - if self.0.into_inner() > other.0.into_inner() { + assert!(self.scale.0 == other.scale.0, "mismatched timestamp scales"); + if self.value.into_inner() > other.value.into_inner() { self } else { other } } - /// Add two timestamps, returning [`TimeOverflow`] if the sum exceeds 2^62. + /// Add two timestamps. Returns [`TimeOverflow`] if the sum exceeds `2^62 - 1` or + /// if the scales differ. pub const fn checked_add(self, rhs: Self) -> Result { - let lhs = self.0.into_inner(); - let rhs = rhs.0.into_inner(); - match lhs.checked_add(rhs) { - Some(result) => Self::new_u64(result), + if self.scale.0 != rhs.scale.0 { + return Err(TimeOverflow); + } + match self.value.into_inner().checked_add(rhs.value.into_inner()) { + Some(result) => Self::new(result, self.scale), None => Err(TimeOverflow), } } - /// Subtract `rhs` from `self`, returning [`TimeOverflow`] if `rhs > self`. + /// Subtract `rhs` from `self`. Returns [`TimeOverflow`] if `rhs > self` or if the + /// scales differ. pub const fn checked_sub(self, rhs: Self) -> Result { - let lhs = self.0.into_inner(); - let rhs = rhs.0.into_inner(); - match lhs.checked_sub(rhs) { - Some(result) => Self::new_u64(result), + if self.scale.0 != rhs.scale.0 { + return Err(TimeOverflow); + } + match self.value.into_inner().checked_sub(rhs.value.into_inner()) { + Some(result) => Self::new(result, self.scale), None => Err(TimeOverflow), } } - /// Whether this timestamp is [`Self::ZERO`]. - pub const fn is_zero(self) -> bool { - self.0.into_inner() == 0 - } - - /// Current time as a timestamp, derived from [`tokio::time::Instant::now`] so - /// it honors `tokio::time::pause` in tests. + /// Current time, expressed in microseconds ([`Timescale::MICRO`]). Uses + /// [`tokio::time::Instant::now`] so it honors `tokio::time::pause` in tests. pub fn now() -> Self { - // We use tokio so it can be stubbed for testing. tokio::time::Instant::now().into() } - - /// Convert this timestamp to a different scale. - /// - /// This allows converting between different TimeScale types, for example from milliseconds to microseconds. - /// Note that converting to a coarser scale may lose precision due to integer division. - pub const fn convert(self) -> Result, TimeOverflow> { - let value = self.0.into_inner(); - // Convert from SCALE to NEW_SCALE: value * NEW_SCALE / SCALE - match (value as u128).checked_mul(NEW_SCALE as u128) { - Some(v) => match v.checked_div(SCALE as u128) { - Some(v) => match VarInt::from_u128(v) { - Some(varint) => Ok(Timescale(varint)), - None => Err(TimeOverflow), - }, - None => Err(TimeOverflow), - }, - None => Err(TimeOverflow), - } - } - - /// Encode this timestamp as a QUIC varint. Version-independent. - pub fn encode(&self, w: &mut W) -> Result<(), EncodeError> { - // Version-independent: uses QUIC varint encoding. - self.0.encode(w, crate::lite::Version::Lite01)?; - Ok(()) - } - - /// Decode a timestamp from a QUIC varint. Version-independent. - pub fn decode(r: &mut R) -> Result { - // Version-independent: uses QUIC varint encoding. - let v = VarInt::decode(r, crate::lite::Version::Lite01)?; - Ok(Self(v)) - } } -impl TryFrom for Timescale { +impl TryFrom for Timestamp { type Error = TimeOverflow; + /// Convert a [`std::time::Duration`] into a nanosecond-scale timestamp. fn try_from(duration: std::time::Duration) -> Result { - Self::from_scale_u128(duration.as_nanos(), 1_000_000_000) + match VarInt::from_u128(duration.as_nanos()) { + Some(value) => Ok(Self { + value, + scale: Timescale::NANO, + }), + None => Err(TimeOverflow), + } } } -impl From> for std::time::Duration { - fn from(time: Timescale) -> Self { - std::time::Duration::new(time.as_secs(), (time.as_nanos() % 1_000_000_000) as u32) +impl TryFrom for std::time::Duration { + type Error = TimeOverflow; + + fn try_from(time: Timestamp) -> Result { + let secs = time.as_secs()?; + let nanos = time.as_nanos()?; + Ok(std::time::Duration::new(secs, (nanos % 1_000_000_000) as u32)) } } -impl std::fmt::Debug for Timescale { +impl std::fmt::Debug for Timestamp { #[allow(clippy::manual_is_multiple_of)] // is_multiple_of is unstable in Rust 1.85 fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let nanos = self.as_nanos(); + if self.scale.0 == 0 { + return write!(f, "{}/?", self.value.into_inner()); + } + + let nanos = match self.as_nanos() { + Ok(n) => n, + Err(_) => return write!(f, "{}/{}", self.value.into_inner(), self.scale), + }; - // Choose the largest unit where we don't need decimal places - // Check from largest to smallest unit + // Choose the largest unit where we don't need decimal places. if nanos % 1_000_000_000 == 0 { write!(f, "{}s", nanos / 1_000_000_000) } else if nanos % 1_000_000 == 0 { @@ -260,29 +361,51 @@ impl std::fmt::Debug for Timescale { } } -impl std::ops::Add for Timescale { +impl PartialOrd for Timestamp { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for Timestamp { + /// Compare by raw value. Debug-asserts that scales are compatible (same, or + /// one is unspecified). In release, cross-scale comparisons return a result + /// based on the raw value, which is meaningful only when one side is a + /// [`Timescale::UNKNOWN`] sentinel ([`Self::ZERO`] or [`Self::MAX`]). + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + debug_assert!( + self.scale.0 == other.scale.0 || self.scale.0 == 0 || other.scale.0 == 0, + "comparing timestamps with mismatched scales: {} vs {}", + self.scale, + other.scale, + ); + self.value.cmp(&other.value) + } +} + +impl std::ops::Add for Timestamp { type Output = Self; fn add(self, rhs: Self) -> Self { - self.checked_add(rhs).expect("time overflow") + self.checked_add(rhs).expect("time overflow or scale mismatch") } } -impl std::ops::AddAssign for Timescale { +impl std::ops::AddAssign for Timestamp { fn add_assign(&mut self, rhs: Self) { *self = *self + rhs; } } -impl std::ops::Sub for Timescale { +impl std::ops::Sub for Timestamp { type Output = Self; fn sub(self, rhs: Self) -> Self { - self.checked_sub(rhs).expect("time overflow") + self.checked_sub(rhs).expect("time overflow or scale mismatch") } } -impl std::ops::SubAssign for Timescale { +impl std::ops::SubAssign for Timestamp { fn sub_assign(&mut self, rhs: Self) { *self = *self - rhs; } @@ -297,43 +420,49 @@ static TIME_ANCHOR: LazyLock<(std::time::Instant, SystemTime)> = LazyLock::new(| (std::time::Instant::now(), SystemTime::now() - jitter) }); -// Convert an Instant to a Unix timestamp -impl From for Timescale { +impl From for Timestamp { + /// Convert an [`std::time::Instant`] into a microsecond-scale timestamp anchored to a + /// jittered wall-clock reference (see `TIME_ANCHOR`). fn from(instant: std::time::Instant) -> Self { let (anchor_instant, anchor_system) = *TIME_ANCHOR; - // Conver the instant to a SystemTime. let system = match instant.checked_duration_since(anchor_instant) { Some(forward) => anchor_system + forward, None => anchor_system - anchor_instant.duration_since(instant), }; - // Convert the SystemTime to a Unix timestamp in nanoseconds. - // We'll then convert that to the desired scale. - system + let duration = system .duration_since(UNIX_EPOCH) - .expect("dude your clock is earlier than 1970") - .try_into() - .expect("dude your clock is later than 2116") + .expect("dude your clock is earlier than 1970"); + + Self::from_micros(duration.as_micros() as u64).expect("dude your clock is later than 2116") } } -impl From for Timescale { +impl From for Timestamp { fn from(instant: tokio::time::Instant) -> Self { instant.into_std().into() } } -impl Decode for Timescale { +/// Decode a timestamp's raw value as a varint, attaching [`Timescale::UNKNOWN`]. +/// +/// Callers that need a meaningful scale should attach it after decoding (e.g. via +/// [`Timestamp::new`] with the track's [`Timescale`]). +impl Decode for Timestamp { fn decode(r: &mut R, version: crate::Version) -> Result { - let v = VarInt::decode(r, version)?; - Ok(Self(v)) + let value = VarInt::decode(r, version)?; + Ok(Self { + value, + scale: Timescale::UNKNOWN, + }) } } -impl Encode for Timescale { +/// Encode a timestamp's raw value as a varint. The scale is **not** serialized. +impl Encode for Timestamp { fn encode(&self, w: &mut W, version: crate::Version) -> Result<(), EncodeError> { - self.0.encode(w, version)?; + self.value.encode(w, version)?; Ok(()) } } @@ -344,377 +473,210 @@ mod tests { #[test] fn test_from_secs() { - let time = Time::from_secs(5).unwrap(); - assert_eq!(time.as_secs(), 5); - assert_eq!(time.as_millis(), 5000); - assert_eq!(time.as_micros(), 5_000_000); - assert_eq!(time.as_nanos(), 5_000_000_000); + let time = Timestamp::from_secs(5).unwrap(); + assert_eq!(time.scale(), Timescale::SECOND); + assert_eq!(time.as_secs().unwrap(), 5); + assert_eq!(time.as_millis().unwrap(), 5000); + assert_eq!(time.as_micros().unwrap(), 5_000_000); + assert_eq!(time.as_nanos().unwrap(), 5_000_000_000); } #[test] fn test_from_millis() { - let time = Time::from_millis(5000).unwrap(); - assert_eq!(time.as_secs(), 5); - assert_eq!(time.as_millis(), 5000); + let time = Timestamp::from_millis(5000).unwrap(); + assert_eq!(time.scale(), Timescale::MILLI); + assert_eq!(time.as_secs().unwrap(), 5); + assert_eq!(time.as_millis().unwrap(), 5000); } #[test] fn test_from_micros() { - let time = Time::from_micros(5_000_000).unwrap(); - assert_eq!(time.as_secs(), 5); - assert_eq!(time.as_millis(), 5000); - assert_eq!(time.as_micros(), 5_000_000); + let time = Timestamp::from_micros(5_000_000).unwrap(); + assert_eq!(time.scale(), Timescale::MICRO); + assert_eq!(time.as_secs().unwrap(), 5); + assert_eq!(time.as_micros().unwrap(), 5_000_000); } #[test] fn test_from_nanos() { - let time = Time::from_nanos(5_000_000_000).unwrap(); - assert_eq!(time.as_secs(), 5); - assert_eq!(time.as_millis(), 5000); - assert_eq!(time.as_micros(), 5_000_000); - assert_eq!(time.as_nanos(), 5_000_000_000); + let time = Timestamp::from_nanos(5_000_000_000).unwrap(); + assert_eq!(time.scale(), Timescale::NANO); + assert_eq!(time.as_secs().unwrap(), 5); + assert_eq!(time.as_nanos().unwrap(), 5_000_000_000); } #[test] - fn test_zero() { - let time = Time::ZERO; - assert_eq!(time.as_secs(), 0); - assert_eq!(time.as_millis(), 0); - assert_eq!(time.as_micros(), 0); - assert_eq!(time.as_nanos(), 0); + fn test_zero_unspecified() { + let time = Timestamp::ZERO; + assert!(time.is_unspecified()); assert!(time.is_zero()); + assert!(time.as_secs().is_err()); + assert!(time.as_millis().is_err()); + assert!(time.as_micros().is_err()); + assert!(time.as_nanos().is_err()); } #[test] - fn test_roundtrip_millis() { - let values = [0, 1, 100, 1000, 999999, 1_000_000_000]; - for &val in &values { - let time = Time::from_millis(val).unwrap(); - assert_eq!(time.as_millis(), val as u128); - } + fn test_zero_at_scale() { + let time = Timestamp::from_millis(0).unwrap(); + assert!(!time.is_unspecified()); + assert!(time.is_zero()); + assert_eq!(time.as_millis().unwrap(), 0); } #[test] - fn test_roundtrip_micros() { - // Note: values < 1000 will lose precision when converting to milliseconds (SCALE=1000) - let values = [0, 1000, 1_000_000, 1_000_000_000]; - for &val in &values { - let time = Time::from_micros(val).unwrap(); - assert_eq!(time.as_micros(), val as u128); - } + fn test_convert_to_finer() { + let time_ms = Timestamp::from_millis(5000).unwrap(); + let time_us = time_ms.convert(Timescale::MICRO).unwrap(); + assert_eq!(time_us.scale(), Timescale::MICRO); + assert_eq!(time_us.as_micros().unwrap(), 5_000_000); } #[test] - fn test_different_scale_seconds() { - type TimeInSeconds = Timescale<1>; - let time = TimeInSeconds::from_secs(5).unwrap(); - assert_eq!(time.as_secs(), 5); - assert_eq!(time.as_millis(), 5000); + fn test_convert_to_coarser() { + let time_ms = Timestamp::from_millis(5000).unwrap(); + let time_s = time_ms.convert(Timescale::SECOND).unwrap(); + assert_eq!(time_s.scale(), Timescale::SECOND); + assert_eq!(time_s.as_secs().unwrap(), 5); } #[test] - fn test_different_scale_microseconds() { - type TimeInMicros = Timescale<1_000_000>; - let time = TimeInMicros::from_micros(5_000_000).unwrap(); - assert_eq!(time.as_secs(), 5); - assert_eq!(time.as_micros(), 5_000_000); + fn test_convert_precision_loss() { + // 1234 ms = 1.234 s, rounds down to 1 s + let time_ms = Timestamp::from_millis(1234).unwrap(); + let time_s = time_ms.convert(Timescale::SECOND).unwrap(); + assert_eq!(time_s.as_secs().unwrap(), 1); } #[test] - fn test_scale_conversion() { - // Converting 5000 milliseconds at scale 1000 to scale 1000 (should be identity) - let time = Time::from_scale(5000, 1000).unwrap(); - assert_eq!(time.as_millis(), 5000); - assert_eq!(time.as_secs(), 5); - - // Converting 5 seconds at scale 1 to scale 1000 - let time = Time::from_scale(5, 1).unwrap(); - assert_eq!(time.as_millis(), 5000); - assert_eq!(time.as_secs(), 5); + fn test_convert_roundtrip() { + let original = Timestamp::from_millis(5000).unwrap(); + let as_micros = original.convert(Timescale::MICRO).unwrap(); + let back = as_micros.convert(Timescale::MILLI).unwrap(); + assert_eq!(original.value(), back.value()); + assert_eq!(original.scale(), back.scale()); } #[test] - fn test_add() { - let a = Time::from_secs(3).unwrap(); - let b = Time::from_secs(2).unwrap(); - let c = a + b; - assert_eq!(c.as_secs(), 5); - assert_eq!(c.as_millis(), 5000); + fn test_convert_same_scale() { + let time = Timestamp::from_millis(5000).unwrap(); + let converted = time.convert(Timescale::MILLI).unwrap(); + assert_eq!(time, converted); } #[test] - fn test_sub() { - let a = Time::from_secs(5).unwrap(); - let b = Time::from_secs(2).unwrap(); - let c = a - b; - assert_eq!(c.as_secs(), 3); - assert_eq!(c.as_millis(), 3000); + fn test_convert_unspecified_rejected() { + let zero = Timestamp::ZERO; + assert!(zero.convert(Timescale::MILLI).is_err()); + + let time = Timestamp::from_millis(5).unwrap(); + assert!(time.convert(Timescale::UNKNOWN).is_err()); } #[test] - fn test_checked_add() { - let a = Time::from_millis(1000).unwrap(); - let b = Time::from_millis(2000).unwrap(); + fn test_add_same_scale() { + let a = Timestamp::from_millis(1000).unwrap(); + let b = Timestamp::from_millis(2000).unwrap(); let c = a.checked_add(b).unwrap(); - assert_eq!(c.as_millis(), 3000); + assert_eq!(c.as_millis().unwrap(), 3000); + assert_eq!(c.scale(), Timescale::MILLI); } #[test] - fn test_checked_sub() { - let a = Time::from_millis(5000).unwrap(); - let b = Time::from_millis(2000).unwrap(); - let c = a.checked_sub(b).unwrap(); - assert_eq!(c.as_millis(), 3000); + fn test_add_mismatched_scale() { + let a = Timestamp::from_millis(1000).unwrap(); + let b = Timestamp::from_micros(1000).unwrap(); + assert!(a.checked_add(b).is_err()); } #[test] - fn test_checked_sub_underflow() { - let a = Time::from_millis(1000).unwrap(); - let b = Time::from_millis(2000).unwrap(); + fn test_sub_underflow() { + let a = Timestamp::from_millis(1000).unwrap(); + let b = Timestamp::from_millis(2000).unwrap(); assert!(a.checked_sub(b).is_err()); } #[test] - fn test_max() { - let a = Time::from_secs(5).unwrap(); - let b = Time::from_secs(10).unwrap(); + fn test_max_same_scale() { + let a = Timestamp::from_secs(5).unwrap(); + let b = Timestamp::from_secs(10).unwrap(); assert_eq!(a.max(b), b); assert_eq!(b.max(a), b); } #[test] - fn test_duration_conversion() { - let duration = std::time::Duration::from_secs(5); - let time: Time = duration.try_into().unwrap(); - assert_eq!(time.as_secs(), 5); - assert_eq!(time.as_millis(), 5000); - - let duration_back: std::time::Duration = time.into(); - assert_eq!(duration_back.as_secs(), 5); - } - - #[test] - fn test_duration_with_nanos() { - let duration = std::time::Duration::new(5, 500_000_000); // 5.5 seconds - let time: Time = duration.try_into().unwrap(); - assert_eq!(time.as_millis(), 5500); - - let duration_back: std::time::Duration = time.into(); - assert_eq!(duration_back.as_millis(), 5500); - } - - #[test] - fn test_fractional_conversion() { - // Test that 1500 millis = 1.5 seconds - let time = Time::from_millis(1500).unwrap(); - assert_eq!(time.as_secs(), 1); // Integer division - assert_eq!(time.as_millis(), 1500); - assert_eq!(time.as_micros(), 1_500_000); + #[should_panic(expected = "mismatched timestamp scales")] + fn test_max_mismatched_scale_panics() { + let a = Timestamp::from_millis(1).unwrap(); + let b = Timestamp::from_secs(1).unwrap(); + let _ = a.max(b); } #[test] - fn test_precision_loss() { - // When converting from a finer scale to coarser, we lose precision - // 1234 micros = 1.234 millis, which rounds down to 1 millisecond internally - // When converting back, we get 1000 micros, not the original 1234 - let time = Time::from_micros(1234).unwrap(); - assert_eq!(time.as_millis(), 1); // 1234 micros = 1.234 millis, rounds to 1 - assert_eq!(time.as_micros(), 1000); // Precision lost: 1 milli = 1000 micros - } - - #[test] - fn test_scale_boundaries() { - // Test values near scale boundaries - let time = Time::from_millis(999).unwrap(); - assert_eq!(time.as_secs(), 0); - assert_eq!(time.as_millis(), 999); - - let time = Time::from_millis(1000).unwrap(); - assert_eq!(time.as_secs(), 1); - assert_eq!(time.as_millis(), 1000); - - let time = Time::from_millis(1001).unwrap(); - assert_eq!(time.as_secs(), 1); - assert_eq!(time.as_millis(), 1001); - } - - #[test] - fn test_large_values() { - // Test with large but valid values - let large_secs = 1_000_000_000u64; // ~31 years - let time = Time::from_secs(large_secs).unwrap(); - assert_eq!(time.as_secs(), large_secs); - } - - #[test] - fn test_new() { - let time = Time::new(5000); // 5000 in the current scale (millis) - assert_eq!(time.as_millis(), 5000); - assert_eq!(time.as_secs(), 5); - } - - #[test] - fn test_new_u64() { - let time = Time::new_u64(5000).unwrap(); - assert_eq!(time.as_millis(), 5000); - } - - #[test] - fn test_ordering() { - let a = Time::from_secs(1).unwrap(); - let b = Time::from_secs(2).unwrap(); + fn test_ordering_same_scale() { + let a = Timestamp::from_secs(1).unwrap(); + let b = Timestamp::from_secs(2).unwrap(); assert!(a < b); assert!(b > a); assert_eq!(a, a); } #[test] - fn test_unchecked_variants() { - let time = Time::from_secs_unchecked(5); - assert_eq!(time.as_secs(), 5); - - let time = Time::from_millis_unchecked(5000); - assert_eq!(time.as_millis(), 5000); - - let time = Time::from_micros_unchecked(5_000_000); - assert_eq!(time.as_micros(), 5_000_000); - - let time = Time::from_nanos_unchecked(5_000_000_000); - assert_eq!(time.as_nanos(), 5_000_000_000); - - let time = Time::from_scale_unchecked(5000, 1000); - assert_eq!(time.as_millis(), 5000); + fn test_ordering_against_sentinels() { + // ZERO and MAX act as universal sentinels because their scale is 0. + let t = Timestamp::from_millis(100).unwrap(); + assert!(Timestamp::ZERO < t); + assert!(t < Timestamp::MAX); + assert!(Timestamp::ZERO < Timestamp::MAX); } #[test] - fn test_as_scale() { - let time = Time::from_secs(1).unwrap(); - // 1 second in scale 1000 = 1000 - assert_eq!(time.as_scale(1000), 1000); - // 1 second in scale 1 = 1 - assert_eq!(time.as_scale(1), 1); - // 1 second in scale 1_000_000 = 1_000_000 - assert_eq!(time.as_scale(1_000_000), 1_000_000); - } - - #[test] - fn test_convert_to_finer() { - // Convert from milliseconds to microseconds (coarser to finer) - type TimeInMillis = Timescale<1_000>; - type TimeInMicros = Timescale<1_000_000>; - - let time_millis = TimeInMillis::from_millis(5000).unwrap(); - let time_micros: TimeInMicros = time_millis.convert().unwrap(); - - assert_eq!(time_micros.as_millis(), 5000); - assert_eq!(time_micros.as_micros(), 5_000_000); - } - - #[test] - fn test_convert_to_coarser() { - // Convert from milliseconds to seconds (finer to coarser) - type TimeInMillis = Timescale<1_000>; - type TimeInSeconds = Timescale<1>; - - let time_millis = TimeInMillis::from_millis(5000).unwrap(); - let time_secs: TimeInSeconds = time_millis.convert().unwrap(); - - assert_eq!(time_secs.as_secs(), 5); - assert_eq!(time_secs.as_millis(), 5000); - } - - #[test] - fn test_convert_precision_loss() { - // Converting 1234 millis to seconds loses precision - type TimeInMillis = Timescale<1_000>; - type TimeInSeconds = Timescale<1>; - - let time_millis = TimeInMillis::from_millis(1234).unwrap(); - let time_secs: TimeInSeconds = time_millis.convert().unwrap(); + fn test_duration_conversion() { + let duration = std::time::Duration::from_secs(5); + let time: Timestamp = duration.try_into().unwrap(); + assert_eq!(time.scale(), Timescale::NANO); + assert_eq!(time.as_secs().unwrap(), 5); - // 1234 millis = 1.234 seconds, rounds down to 1 second - assert_eq!(time_secs.as_secs(), 1); - assert_eq!(time_secs.as_millis(), 1000); // Lost 234 millis + let duration_back: std::time::Duration = time.try_into().unwrap(); + assert_eq!(duration_back.as_secs(), 5); } #[test] - fn test_convert_roundtrip() { - // Converting to finer and back should preserve value - type TimeInMillis = Timescale<1_000>; - type TimeInMicros = Timescale<1_000_000>; - - let original = TimeInMillis::from_millis(5000).unwrap(); - let as_micros: TimeInMicros = original.convert().unwrap(); - let back_to_millis: TimeInMillis = as_micros.convert().unwrap(); + fn test_debug_format_units() { + let t = Timestamp::from_millis(100_000).unwrap(); + assert_eq!(format!("{:?}", t), "100s"); - assert_eq!(original.as_millis(), back_to_millis.as_millis()); - } + let t = Timestamp::from_millis(100).unwrap(); + assert_eq!(format!("{:?}", t), "100ms"); - #[test] - fn test_convert_same_scale() { - // Converting to the same scale should be identity - type TimeInMillis = Timescale<1_000>; + let t = Timestamp::from_micros(1500).unwrap(); + assert_eq!(format!("{:?}", t), "1500µs"); - let time = TimeInMillis::from_millis(5000).unwrap(); - let converted: TimeInMillis = time.convert().unwrap(); + let t = Timestamp::from_micros(1000).unwrap(); + assert_eq!(format!("{:?}", t), "1ms"); - assert_eq!(time.as_millis(), converted.as_millis()); + let t = Timestamp::ZERO; + assert_eq!(format!("{:?}", t), "0/?"); } #[test] - fn test_convert_microseconds_to_nanoseconds() { - type TimeInMicros = Timescale<1_000_000>; - type TimeInNanos = Timescale<1_000_000_000>; - - let time_micros = TimeInMicros::from_micros(5_000_000).unwrap(); - let time_nanos: TimeInNanos = time_micros.convert().unwrap(); - - assert_eq!(time_nanos.as_micros(), 5_000_000); - assert_eq!(time_nanos.as_nanos(), 5_000_000_000); + fn test_new() { + let t = Timestamp::new(5000, Timescale::MILLI).unwrap(); + assert_eq!(t.value(), 5000); + assert_eq!(t.scale(), Timescale::MILLI); + assert_eq!(t.as_millis().unwrap(), 5000); } #[test] - fn test_convert_custom_scales() { - // Test with unusual custom scales - type TimeScale60 = Timescale<60>; // 60Hz - type TimeScale90 = Timescale<90>; // 90Hz - - let time60 = TimeScale60::from_scale(120, 60).unwrap(); // 2 seconds at 60Hz - let time90: TimeScale90 = time60.convert().unwrap(); - - // Both should represent 2 seconds - assert_eq!(time60.as_secs(), 2); - assert_eq!(time90.as_secs(), 2); + fn test_from_scale_custom() { + // 120 units at 60Hz = 2 seconds, expressed at 1000Hz = 2000 ms. + let t = Timestamp::from_scale(120, Timescale::new(60), Timescale::MILLI).unwrap(); + assert_eq!(t.scale(), Timescale::MILLI); + assert_eq!(t.as_millis().unwrap(), 2000); } #[test] - fn test_debug_format_units() { - // Test that Debug chooses appropriate units based on value - - // Milliseconds that are clean seconds - let t = Time::from_millis(100000).unwrap(); - assert_eq!(format!("{:?}", t), "100s"); - - let t = Time::from_millis(1000).unwrap(); - assert_eq!(format!("{:?}", t), "1s"); - - // Milliseconds that are clean milliseconds - let t = Time::from_millis(100).unwrap(); - assert_eq!(format!("{:?}", t), "100ms"); - - let t = Time::from_millis(5500).unwrap(); - assert_eq!(format!("{:?}", t), "5500ms"); - - // Zero - let t = Time::ZERO; - assert_eq!(format!("{:?}", t), "0s"); - - // Test with microsecond-scale time - type TimeMicros = Timescale<1_000_000>; - let t = TimeMicros::from_micros(1500).unwrap(); - assert_eq!(format!("{:?}", t), "1500µs"); - - let t = TimeMicros::from_micros(1000).unwrap(); - assert_eq!(format!("{:?}", t), "1ms"); + fn test_from_scale_zero_source() { + assert!(Timestamp::from_scale(5, Timescale::UNKNOWN, Timescale::MILLI).is_err()); } } From 1e5a398b445b81cd82936dedd2024ce2921c7eac Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 23 May 2026 15:25:32 -0700 Subject: [PATCH 02/12] hang catalog jitter: Option serialized as integer ms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit changed the catalog jitter field from `Option` (transparently a `VarInt` → JSON integer) to `Option`, which derive-serialized as `{"value": N, "scale": 1000}` and broke the on-wire shape. Switch the field to `std::time::Duration` and apply a small `duration_millis` serde adapter so it serializes as a bare integer number of milliseconds, matching the historical format. Sub-ms precision is truncated, which is fine for jitter. `MinFrameDuration::observe` now returns `Option` directly via `Duration::try_from(timestamp)`. The fmp4 importer's internal jitter accumulator stays at the source scale and converts once at the catalog write boundary. Adds a round-trip test that pins the integer-ms shape so this regresses loudly if the serde adapter is ever dropped. Co-Authored-By: Claude Opus 4.7 (1M context) --- rs/hang/src/catalog/audio/mod.rs | 6 +- rs/hang/src/catalog/duration_millis.rs | 28 ++++++++ rs/hang/src/catalog/mod.rs | 1 + rs/hang/src/catalog/root.rs | 88 +++++++++++++++++++++++++ rs/hang/src/catalog/video/mod.rs | 6 +- rs/moq-mux/src/catalog/hang/producer.rs | 8 +-- rs/moq-mux/src/catalog/msf/consumer.rs | 4 +- rs/moq-mux/src/container/fmp4/import.rs | 4 +- rs/moq-mux/src/container/jitter.rs | 17 ++--- 9 files changed, 142 insertions(+), 20 deletions(-) create mode 100644 rs/hang/src/catalog/duration_millis.rs diff --git a/rs/hang/src/catalog/audio/mod.rs b/rs/hang/src/catalog/audio/mod.rs index b88a686b1..a76585805 100644 --- a/rs/hang/src/catalog/audio/mod.rs +++ b/rs/hang/src/catalog/audio/mod.rs @@ -85,8 +85,10 @@ pub struct AudioConfig { /// The player's jitter buffer should be larger than this value. /// If not provided, the player should assume each frame is flushed immediately. /// + /// Serialized as an integer number of milliseconds (sub-ms precision is truncated). + /// /// NOTE: The audio "frame" duration depends on the codec, sample rate, etc. /// ex: AAC often uses 1024 samples per frame, so at 44100Hz, this would be 1024/44100 = 23ms - #[serde(default)] - pub jitter: Option, + #[serde(default, with = "crate::catalog::duration_millis")] + pub jitter: Option, } diff --git a/rs/hang/src/catalog/duration_millis.rs b/rs/hang/src/catalog/duration_millis.rs new file mode 100644 index 000000000..a258c6689 --- /dev/null +++ b/rs/hang/src/catalog/duration_millis.rs @@ -0,0 +1,28 @@ +//! Serde adapter for `Option` ↔ JSON integer milliseconds. +//! +//! The hang catalog historically serialized its jitter fields as a bare integer +//! number of milliseconds (e.g. `"jitter": 100`). `std::time::Duration`'s default +//! serde impl would write `{"secs": 0, "nanos": 100_000_000}` instead, so apply +//! this module via `#[serde(default, with = "duration_millis")]` to preserve the +//! original on-wire shape: +//! +//! - `None` → omitted (with `#[serde(default)]`) +//! - `Some(duration)` → integer milliseconds (truncated; sub-ms precision is lost) +//! +//! Sub-millisecond precision isn't meaningful for the jitter use case, so the +//! truncation is fine. + +use std::time::Duration; + +use serde::{Deserialize, Deserializer, Serialize, Serializer}; + +pub fn serialize(value: &Option, ser: S) -> Result { + match value { + Some(d) => (d.as_millis() as u64).serialize(ser), + None => ser.serialize_none(), + } +} + +pub fn deserialize<'de, D: Deserializer<'de>>(de: D) -> Result, D::Error> { + Ok(Option::::deserialize(de)?.map(Duration::from_millis)) +} diff --git a/rs/hang/src/catalog/mod.rs b/rs/hang/src/catalog/mod.rs index 906feb350..d5df1d13c 100644 --- a/rs/hang/src/catalog/mod.rs +++ b/rs/hang/src/catalog/mod.rs @@ -7,6 +7,7 @@ mod audio; mod chat; mod container; +mod duration_millis; mod preview; mod root; mod user; diff --git a/rs/hang/src/catalog/root.rs b/rs/hang/src/catalog/root.rs index 888bce77c..2a8569aab 100644 --- a/rs/hang/src/catalog/root.rs +++ b/rs/hang/src/catalog/root.rs @@ -180,4 +180,92 @@ mod test { let output = decoded.to_string().expect("failed to encode"); assert_eq!(encoded, output, "wrong encoded output"); } + + /// Lock in the on-wire shape of the jitter field: a bare integer number + /// of milliseconds. If `Option` ever loses the `duration_millis` + /// serde adapter, this regresses to serde's default `{secs, nanos}` shape. + #[test] + fn jitter_serialized_as_millis() { + let mut encoded = r#"{ + "video": { + "renditions": { + "video": { + "codec": "avc1.64001f", + "container": {"kind": "legacy"}, + "jitter": 100 + } + } + }, + "audio": { + "renditions": { + "audio": { + "codec": "opus", + "sampleRate": 48000, + "numberOfChannels": 2, + "container": {"kind": "legacy"}, + "jitter": 40 + } + } + } + }"# + .to_string(); + encoded.retain(|c| !c.is_whitespace()); + + let mut video_renditions = BTreeMap::new(); + video_renditions.insert( + "video".to_string(), + VideoConfig { + codec: H264 { + profile: 0x64, + constraints: 0x00, + level: 0x1f, + inline: false, + } + .into(), + description: None, + coded_width: None, + coded_height: None, + display_ratio_width: None, + display_ratio_height: None, + bitrate: None, + framerate: None, + optimize_for_latency: None, + container: Container::Legacy, + jitter: Some(std::time::Duration::from_millis(100)), + }, + ); + + let mut audio_renditions = BTreeMap::new(); + audio_renditions.insert( + "audio".to_string(), + AudioConfig { + codec: Opus, + sample_rate: 48_000, + channel_count: 2, + bitrate: None, + description: None, + container: Container::Legacy, + jitter: Some(std::time::Duration::from_millis(40)), + }, + ); + + let catalog = Catalog { + video: Video { + renditions: video_renditions, + display: None, + rotation: None, + flip: None, + }, + audio: Audio { + renditions: audio_renditions, + }, + ..Default::default() + }; + + let decoded = Catalog::from_str(&encoded).expect("failed to decode"); + assert_eq!(catalog, decoded, "decode mismatch"); + + let output = catalog.to_string().expect("failed to encode"); + assert_eq!(encoded, output, "encode mismatch"); + } } diff --git a/rs/hang/src/catalog/video/mod.rs b/rs/hang/src/catalog/video/mod.rs index 97ba63920..afd52fb0e 100644 --- a/rs/hang/src/catalog/video/mod.rs +++ b/rs/hang/src/catalog/video/mod.rs @@ -134,10 +134,12 @@ pub struct VideoConfig { /// The player's jitter buffer should be larger than this value. /// If not provided, the player should assume each frame is flushed immediately. /// + /// Serialized as an integer number of milliseconds (sub-ms precision is truncated). + /// /// ex: /// - If each frame is flushed immediately, this would be 1000/fps. /// - If there can be up to 3 b-frames in a row, this would be 3 * 1000/fps. /// - If frames are buffered into 2s segments, this would be 2s. - #[serde(default)] - pub jitter: Option, + #[serde(default, with = "crate::catalog::duration_millis")] + pub jitter: Option, } diff --git a/rs/moq-mux/src/catalog/hang/producer.rs b/rs/moq-mux/src/catalog/hang/producer.rs index 036f0a5cb..ce2a8ceaa 100644 --- a/rs/moq-mux/src/catalog/hang/producer.rs +++ b/rs/moq-mux/src/catalog/hang/producer.rs @@ -173,7 +173,7 @@ fn to_msf(catalog: &hang::Catalog) -> moq_msf::Catalog { alt_group: if has_multiple_video { Some(1) } else { None }, max_grp_sap_starting_type: sap_type, max_obj_sap_starting_type: sap_type, - jitter: config.jitter.and_then(|t| t.as_millis().ok()).map(|ms| ms as f64), + jitter: config.jitter.map(|d| d.as_millis() as f64), }); } @@ -209,7 +209,7 @@ fn to_msf(catalog: &hang::Catalog) -> moq_msf::Catalog { alt_group: if has_multiple_audio { Some(1) } else { None }, max_grp_sap_starting_type: Some(1), max_obj_sap_starting_type: Some(1), - jitter: config.jitter.and_then(|t| t.as_millis().ok()).map(|ms| ms as f64), + jitter: config.jitter.map(|d| d.as_millis() as f64), }); } @@ -431,7 +431,7 @@ mod test { framerate: Some(30.0), optimize_for_latency: None, container: Container::Legacy, - jitter: Some(moq_net::Timestamp::from_millis_unchecked(100)), + jitter: Some(std::time::Duration::from_millis(100)), }, ); @@ -445,7 +445,7 @@ mod test { bitrate: None, description: None, container: Container::Legacy, - jitter: Some(moq_net::Timestamp::from_millis_unchecked(40)), + jitter: Some(std::time::Duration::from_millis(40)), }, ); diff --git a/rs/moq-mux/src/catalog/msf/consumer.rs b/rs/moq-mux/src/catalog/msf/consumer.rs index a1543747d..9ee6b17d5 100644 --- a/rs/moq-mux/src/catalog/msf/consumer.rs +++ b/rs/moq-mux/src/catalog/msf/consumer.rs @@ -223,7 +223,7 @@ fn video_config_from_msf(track: &moq_msf::Track) -> anyhow::Result= 0.0) - .and_then(|v| moq_net::Timestamp::from_millis(v as u64).ok()), + .map(|v| std::time::Duration::from_millis(v as u64)), })) } @@ -270,7 +270,7 @@ fn audio_config_from_msf(track: &moq_msf::Track) -> anyhow::Result= 0.0) - .and_then(|v| moq_net::Timestamp::from_millis(v as u64).ok()), + .map(|v| std::time::Duration::from_millis(v as u64)), })) } diff --git a/rs/moq-mux/src/container/fmp4/import.rs b/rs/moq-mux/src/container/fmp4/import.rs index 22eed4e5f..87ed06128 100644 --- a/rs/moq-mux/src/container/fmp4/import.rs +++ b/rs/moq-mux/src/container/fmp4/import.rs @@ -644,7 +644,7 @@ impl Import { .renditions .get_mut(&track.track.name) .context("missing video config")?; - config.jitter = Some(jitter.convert(crate::container::Timescale::MILLI)?); + config.jitter = Some(std::time::Duration::try_from(jitter)?); } TrackKind::Audio => { let config = catalog @@ -652,7 +652,7 @@ impl Import { .renditions .get_mut(&track.track.name) .context("missing audio config")?; - config.jitter = Some(jitter.convert(crate::container::Timescale::MILLI)?); + config.jitter = Some(std::time::Duration::try_from(jitter)?); } } } diff --git a/rs/moq-mux/src/container/jitter.rs b/rs/moq-mux/src/container/jitter.rs index f733dbea3..a227c03e4 100644 --- a/rs/moq-mux/src/container/jitter.rs +++ b/rs/moq-mux/src/container/jitter.rs @@ -1,3 +1,5 @@ +use std::time::Duration; + use crate::container::Timestamp; /// Tracks the minimum duration between consecutive frames. @@ -18,16 +20,15 @@ impl MinFrameDuration { /// Record a new frame timestamp. /// - /// Returns the new minimum-frame-duration as a millisecond-scale [`Timestamp`] - /// if it changed, so the caller can persist it on the catalog rendition. - /// Returns `None` when this is the first observation, the timestamps are - /// non-monotonic, the new gap is no smaller than the recorded minimum, or - /// the scale conversion to milliseconds overflows. - pub fn observe(&mut self, ts: Timestamp) -> Option { + /// Returns the new minimum-frame-duration as a [`Duration`] if it changed, so + /// the caller can persist it on the catalog rendition. Returns `None` when this + /// is the first observation, the timestamps are non-monotonic, the new gap is + /// no smaller than the recorded minimum, or the timestamp's scale is + /// unspecified. + pub fn observe(&mut self, ts: Timestamp) -> Option { let last = self.last_timestamp.replace(ts)?; let duration = ts.checked_sub(last).ok()?; - // Sentinel max preserves the source scale on the first real observation. if let Some(min) = self.min_duration && duration >= min { @@ -35,6 +36,6 @@ impl MinFrameDuration { } self.min_duration = Some(duration); - duration.convert(moq_net::Timescale::MILLI).ok() + Duration::try_from(duration).ok() } } From 00bfa306c084b03563f33e875c078c09c4fa23e5 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 23 May 2026 15:27:24 -0700 Subject: [PATCH 03/12] hang catalog: use serde_with::DurationMilliSeconds for jitter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `serde_with` already provides `DurationMilliSeconds` which does the same Option ↔ integer-ms conversion. Drop the hand-rolled `duration_millis` adapter and use the library helper instead. The integer-ms round-trip test from the previous commit still passes, so the on-wire shape is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- rs/hang/src/catalog/audio/mod.rs | 5 +++-- rs/hang/src/catalog/duration_millis.rs | 28 -------------------------- rs/hang/src/catalog/mod.rs | 1 - rs/hang/src/catalog/video/mod.rs | 5 +++-- 4 files changed, 6 insertions(+), 33 deletions(-) delete mode 100644 rs/hang/src/catalog/duration_millis.rs diff --git a/rs/hang/src/catalog/audio/mod.rs b/rs/hang/src/catalog/audio/mod.rs index a76585805..e35a76107 100644 --- a/rs/hang/src/catalog/audio/mod.rs +++ b/rs/hang/src/catalog/audio/mod.rs @@ -9,7 +9,7 @@ use std::collections::{BTreeMap, btree_map}; use bytes::Bytes; use serde::{Deserialize, Serialize}; -use serde_with::{DisplayFromStr, hex::Hex}; +use serde_with::{DisplayFromStr, DurationMilliSeconds, hex::Hex}; use crate::catalog::Container; @@ -89,6 +89,7 @@ pub struct AudioConfig { /// /// NOTE: The audio "frame" duration depends on the codec, sample rate, etc. /// ex: AAC often uses 1024 samples per frame, so at 44100Hz, this would be 1024/44100 = 23ms - #[serde(default, with = "crate::catalog::duration_millis")] + #[serde_as(as = "Option>")] + #[serde(default)] pub jitter: Option, } diff --git a/rs/hang/src/catalog/duration_millis.rs b/rs/hang/src/catalog/duration_millis.rs deleted file mode 100644 index a258c6689..000000000 --- a/rs/hang/src/catalog/duration_millis.rs +++ /dev/null @@ -1,28 +0,0 @@ -//! Serde adapter for `Option` ↔ JSON integer milliseconds. -//! -//! The hang catalog historically serialized its jitter fields as a bare integer -//! number of milliseconds (e.g. `"jitter": 100`). `std::time::Duration`'s default -//! serde impl would write `{"secs": 0, "nanos": 100_000_000}` instead, so apply -//! this module via `#[serde(default, with = "duration_millis")]` to preserve the -//! original on-wire shape: -//! -//! - `None` → omitted (with `#[serde(default)]`) -//! - `Some(duration)` → integer milliseconds (truncated; sub-ms precision is lost) -//! -//! Sub-millisecond precision isn't meaningful for the jitter use case, so the -//! truncation is fine. - -use std::time::Duration; - -use serde::{Deserialize, Deserializer, Serialize, Serializer}; - -pub fn serialize(value: &Option, ser: S) -> Result { - match value { - Some(d) => (d.as_millis() as u64).serialize(ser), - None => ser.serialize_none(), - } -} - -pub fn deserialize<'de, D: Deserializer<'de>>(de: D) -> Result, D::Error> { - Ok(Option::::deserialize(de)?.map(Duration::from_millis)) -} diff --git a/rs/hang/src/catalog/mod.rs b/rs/hang/src/catalog/mod.rs index d5df1d13c..906feb350 100644 --- a/rs/hang/src/catalog/mod.rs +++ b/rs/hang/src/catalog/mod.rs @@ -7,7 +7,6 @@ mod audio; mod chat; mod container; -mod duration_millis; mod preview; mod root; mod user; diff --git a/rs/hang/src/catalog/video/mod.rs b/rs/hang/src/catalog/video/mod.rs index afd52fb0e..0c2e13130 100644 --- a/rs/hang/src/catalog/video/mod.rs +++ b/rs/hang/src/catalog/video/mod.rs @@ -14,7 +14,7 @@ use std::collections::{BTreeMap, btree_map}; use bytes::Bytes; use serde::{Deserialize, Serialize}; -use serde_with::{DisplayFromStr, hex::Hex}; +use serde_with::{DisplayFromStr, DurationMilliSeconds, hex::Hex}; use crate::catalog::Container; @@ -140,6 +140,7 @@ pub struct VideoConfig { /// - If each frame is flushed immediately, this would be 1000/fps. /// - If there can be up to 3 b-frames in a row, this would be 3 * 1000/fps. /// - If frames are buffered into 2s segments, this would be 2s. - #[serde(default, with = "crate::catalog::duration_millis")] + #[serde_as(as = "Option>")] + #[serde(default)] pub jitter: Option, } From 5ac1cd9c49a34aed0ea5a630b8ca157187e31db1 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 23 May 2026 15:34:49 -0700 Subject: [PATCH 04/12] moq-net: tighten Timestamp arithmetic and cross-scale ordering Two fixes for issues CodeRabbit flagged on #1473: 1. `checked_add` / `checked_sub` now reject [`Timescale::UNKNOWN`] operands instead of treating them like any matching scale. Without this, wire-decoded timestamps that haven't had a scale attached yet would silently combine into "valid" arithmetic results. 2. `Ord::cmp` now cross-normalizes when both scales are known, using 128-bit cross-multiplication (`a*s2 vs b*s1`). The previous raw-value fallback was a release-build hazard: `1s < 2ms` would compare as `1 < 2` once the `debug_assert!` got compiled out. This matters for the three `min_by_key(|ts| *ts)` sites in fmp4/mkv exporters, which pick the next track to emit across per-track frames that now carry their own native scale. UNKNOWN-scale sentinel comparisons (`Timestamp::ZERO` / `Timestamp::MAX`) keep their raw-value fallback so existing `unwrap_or(ZERO/MAX)` patterns still work. Adds tests covering both fixes plus a mixed-scale sort regression test. Co-Authored-By: Claude Opus 4.7 (1M context) --- rs/moq-net/src/model/time.rs | 92 +++++++++++++++++++++++++++++------- 1 file changed, 75 insertions(+), 17 deletions(-) diff --git a/rs/moq-net/src/model/time.rs b/rs/moq-net/src/model/time.rs index 9076a2d3b..f30655d13 100644 --- a/rs/moq-net/src/model/time.rs +++ b/rs/moq-net/src/model/time.rs @@ -280,10 +280,10 @@ impl Timestamp { } } - /// Add two timestamps. Returns [`TimeOverflow`] if the sum exceeds `2^62 - 1` or - /// if the scales differ. + /// Add two timestamps. Returns [`TimeOverflow`] if the sum exceeds `2^62 - 1`, + /// if either scale is [`Timescale::UNKNOWN`], or if the scales differ. pub const fn checked_add(self, rhs: Self) -> Result { - if self.scale.0 != rhs.scale.0 { + if self.scale.0 == 0 || rhs.scale.0 == 0 || self.scale.0 != rhs.scale.0 { return Err(TimeOverflow); } match self.value.into_inner().checked_add(rhs.value.into_inner()) { @@ -292,10 +292,10 @@ impl Timestamp { } } - /// Subtract `rhs` from `self`. Returns [`TimeOverflow`] if `rhs > self` or if the - /// scales differ. + /// Subtract `rhs` from `self`. Returns [`TimeOverflow`] if `rhs > self`, if either + /// scale is [`Timescale::UNKNOWN`], or if the scales differ. pub const fn checked_sub(self, rhs: Self) -> Result { - if self.scale.0 != rhs.scale.0 { + if self.scale.0 == 0 || rhs.scale.0 == 0 || self.scale.0 != rhs.scale.0 { return Err(TimeOverflow); } match self.value.into_inner().checked_sub(rhs.value.into_inner()) { @@ -368,18 +368,28 @@ impl PartialOrd for Timestamp { } impl Ord for Timestamp { - /// Compare by raw value. Debug-asserts that scales are compatible (same, or - /// one is unspecified). In release, cross-scale comparisons return a result - /// based on the raw value, which is meaningful only when one side is a - /// [`Timescale::UNKNOWN`] sentinel ([`Self::ZERO`] or [`Self::MAX`]). + /// Compare two timestamps, normalizing across scales when both are known. + /// + /// - If both scales are equal, compares raw values directly. + /// - If one side is [`Timescale::UNKNOWN`] ([`Self::ZERO`] / [`Self::MAX`] sentinels), + /// compares raw values; the sentinel's `0` / `VarInt::MAX` ensures the expected + /// ordering against any concrete timestamp. + /// - If both scales are known but differ, cross-multiplies in 128-bit so e.g. + /// `1s > 2ms` orders correctly. Required by the `min_by_key` call sites in the + /// fmp4/mkv exporters, which pick the next track to emit across mixed-scale + /// per-track frames. + /// + /// Note: derived `PartialEq`/`Eq` compare fields, so two cross-scale timestamps + /// that order as `Equal` here (e.g. `from_secs(1)` and `from_millis(1000)`) still + /// compare `!=`. This inconsistency only matters for code that mixes scales in a + /// `BTreeMap`/`BTreeSet` keyed on `Timestamp`, which the codebase doesn't do. fn cmp(&self, other: &Self) -> std::cmp::Ordering { - debug_assert!( - self.scale.0 == other.scale.0 || self.scale.0 == 0 || other.scale.0 == 0, - "comparing timestamps with mismatched scales: {} vs {}", - self.scale, - other.scale, - ); - self.value.cmp(&other.value) + if self.scale.0 == other.scale.0 || self.scale.0 == 0 || other.scale.0 == 0 { + return self.value.cmp(&other.value); + } + let lhs = self.value.into_inner() as u128 * other.scale.0 as u128; + let rhs = other.value.into_inner() as u128 * self.scale.0 as u128; + lhs.cmp(&rhs) } } @@ -596,6 +606,27 @@ mod tests { assert!(a.checked_sub(b).is_err()); } + #[test] + fn test_add_unspecified_scale_rejected() { + // Two ZERO sentinels (both UNKNOWN scale) must not combine into a meaningful + // result, otherwise wire-decoded timestamps that haven't had a scale attached + // yet would silently behave like real arithmetic operands. + assert!(Timestamp::ZERO.checked_add(Timestamp::ZERO).is_err()); + + let t = Timestamp::from_millis(100).unwrap(); + assert!(t.checked_add(Timestamp::ZERO).is_err()); + assert!(Timestamp::ZERO.checked_add(t).is_err()); + } + + #[test] + fn test_sub_unspecified_scale_rejected() { + assert!(Timestamp::ZERO.checked_sub(Timestamp::ZERO).is_err()); + + let t = Timestamp::from_millis(100).unwrap(); + assert!(t.checked_sub(Timestamp::ZERO).is_err()); + assert!(Timestamp::ZERO.checked_sub(t).is_err()); + } + #[test] fn test_max_same_scale() { let a = Timestamp::from_secs(5).unwrap(); @@ -630,6 +661,33 @@ mod tests { assert!(Timestamp::ZERO < Timestamp::MAX); } + #[test] + fn test_ordering_across_known_scales() { + // Cross-scale ordering must normalize to a common scale rather than fall back + // to raw values. Without this, 1s would order as < 2ms (1 < 2) and the + // fmp4/mkv exporter's `min_by_key(|ts| *ts)` over per-track frames would silently + // pick the wrong track when tracks have different native scales. + let one_sec = Timestamp::from_secs(1).unwrap(); + let two_ms = Timestamp::from_millis(2).unwrap(); + assert!(one_sec > two_ms); + assert!(two_ms < one_sec); + + // Equivalent values across scales compare as Equal under cmp. + let one_sec_b = Timestamp::from_millis(1000).unwrap(); + assert_eq!(one_sec.cmp(&one_sec_b), std::cmp::Ordering::Equal); + + // Sorting a mixed-scale slice puts entries in correct temporal order. + let mut items = [ + Timestamp::from_secs(2).unwrap(), + Timestamp::from_millis(500).unwrap(), + Timestamp::from_micros(1_500_000).unwrap(), + ]; + items.sort(); + assert_eq!(items[0], Timestamp::from_millis(500).unwrap()); + assert_eq!(items[1], Timestamp::from_micros(1_500_000).unwrap()); + assert_eq!(items[2], Timestamp::from_secs(2).unwrap()); + } + #[test] fn test_duration_conversion() { let duration = std::time::Duration::from_secs(5); From 716f0e97fccb9aa4811db461ea9e5c7be9edf08a Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 23 May 2026 15:49:42 -0700 Subject: [PATCH 05/12] moq-msf: jitter Option, matches hang catalog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mirror the same pattern hang uses: a `std::time::Duration` field with a `serde_with` adapter, instead of a bare `Option`. The wire format stays a fractional JSON number of ms via `DurationMilliSecondsWithFrac` (existing JS consumers and the `15.5` round-trip test are unaffected); only the Rust type changes. Lets the moq-mux ↔ MSF bridges drop their explicit `Duration → f64 → Duration` conversions: both sides are now `Option`, so the bridge is just `dst.jitter = src.jitter`. Co-Authored-By: Claude Opus 4.7 (1M context) --- rs/moq-msf/src/lib.rs | 15 +++++++++++---- rs/moq-mux/src/catalog/hang/producer.rs | 8 ++++---- rs/moq-mux/src/catalog/msf/consumer.rs | 18 ++---------------- 3 files changed, 17 insertions(+), 24 deletions(-) diff --git a/rs/moq-msf/src/lib.rs b/rs/moq-msf/src/lib.rs index 196bc6952..4ebab6cd0 100644 --- a/rs/moq-msf/src/lib.rs +++ b/rs/moq-msf/src/lib.rs @@ -10,8 +10,10 @@ use std::fmt; use std::str::FromStr; +use std::time::Duration; use serde::{Deserialize, Serialize}; +use serde_with::DurationMilliSecondsWithFrac; /// The default track name for the MSF catalog. pub const DEFAULT_NAME: &str = "catalog"; @@ -35,6 +37,7 @@ pub struct Catalog { /// then assign whichever optional fields they need; struct-literal /// construction (with or without `..base`) is not available outside this /// crate. +#[serde_with::serde_as] #[serde_with::skip_serializing_none] #[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] #[serde(rename_all = "camelCase")] @@ -94,8 +97,12 @@ pub struct Track { #[serde(rename = "maxObjSapStartingType")] pub max_obj_sap_starting_type: Option, - /// Jitter in milliseconds (non-standard extension, matches JS implementation). - pub jitter: Option, + /// Jitter (non-standard extension, matches JS implementation). + /// + /// Serialized as a JSON number of milliseconds with fractional precision + /// (e.g. `15.5`), so it stays wire-compatible with existing JS consumers. + #[serde_as(as = "Option>")] + pub jitter: Option, } impl Catalog { @@ -438,7 +445,7 @@ mod test { alt_group: None, max_grp_sap_starting_type: Some(1), max_obj_sap_starting_type: Some(2), - jitter: Some(15.5), + jitter: Some(Duration::from_micros(15_500)), } } @@ -503,6 +510,6 @@ mod test { assert_eq!(original, parsed); assert_eq!(parsed.tracks[0].max_grp_sap_starting_type, Some(1)); assert_eq!(parsed.tracks[0].max_obj_sap_starting_type, Some(2)); - assert_eq!(parsed.tracks[0].jitter, Some(15.5)); + assert_eq!(parsed.tracks[0].jitter, Some(Duration::from_micros(15_500))); } } diff --git a/rs/moq-mux/src/catalog/hang/producer.rs b/rs/moq-mux/src/catalog/hang/producer.rs index 91210afda..30220dd83 100644 --- a/rs/moq-mux/src/catalog/hang/producer.rs +++ b/rs/moq-mux/src/catalog/hang/producer.rs @@ -169,7 +169,7 @@ fn to_msf(catalog: &hang::Catalog) -> moq_msf::Catalog { track.alt_group = if has_multiple_video { Some(1) } else { None }; track.max_grp_sap_starting_type = sap_type; track.max_obj_sap_starting_type = sap_type; - track.jitter = config.jitter.map(|d| d.as_millis() as f64); + track.jitter = config.jitter; tracks.push(track); } @@ -200,7 +200,7 @@ fn to_msf(catalog: &hang::Catalog) -> moq_msf::Catalog { track.alt_group = if has_multiple_audio { Some(1) } else { None }; track.max_grp_sap_starting_type = Some(1); track.max_obj_sap_starting_type = Some(1); - track.jitter = config.jitter.map(|d| d.as_millis() as f64); + track.jitter = config.jitter; tracks.push(track); } @@ -460,13 +460,13 @@ mod test { // H.264 may carry B-frames, so SAP starting type is 2. assert_eq!(video.max_grp_sap_starting_type, Some(2)); assert_eq!(video.max_obj_sap_starting_type, Some(2)); - assert_eq!(video.jitter, Some(100.0)); + assert_eq!(video.jitter, Some(std::time::Duration::from_millis(100))); let audio = &msf.tracks[1]; assert_eq!(audio.role, Some(moq_msf::Role::Audio)); assert_eq!(audio.max_grp_sap_starting_type, Some(1)); assert_eq!(audio.max_obj_sap_starting_type, Some(1)); - assert_eq!(audio.jitter, Some(40.0)); + assert_eq!(audio.jitter, Some(std::time::Duration::from_millis(40))); } #[test] diff --git a/rs/moq-mux/src/catalog/msf/consumer.rs b/rs/moq-mux/src/catalog/msf/consumer.rs index c854d818f..c8c882f67 100644 --- a/rs/moq-mux/src/catalog/msf/consumer.rs +++ b/rs/moq-mux/src/catalog/msf/consumer.rs @@ -216,14 +216,7 @@ fn video_config_from_msf(track: &moq_msf::Track) -> anyhow::Result= 0.0) - .map(|v| std::time::Duration::from_millis(v as u64)), + jitter: track.jitter, })) } @@ -263,14 +256,7 @@ fn audio_config_from_msf(track: &moq_msf::Track) -> anyhow::Result= 0.0) - .map(|v| std::time::Duration::from_millis(v as u64)), + jitter: track.jitter, })) } From 71078c0d45e118e4588424b5bec0ad3e9347394f Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 23 May 2026 15:51:29 -0700 Subject: [PATCH 06/12] moq-msf: drop fractional jitter precision, match hang exactly jitter isn't in the MSF/CMSF drafts at all, so there's no wire-format to preserve. Use `DurationMilliSeconds` (integer ms) like the hang catalog instead of `DurationMilliSecondsWithFrac`. Sub-ms precision isn't meaningful for jitter and the two catalogs now agree on shape. Co-Authored-By: Claude Opus 4.7 (1M context) --- rs/moq-msf/src/lib.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/rs/moq-msf/src/lib.rs b/rs/moq-msf/src/lib.rs index 4ebab6cd0..a298a3846 100644 --- a/rs/moq-msf/src/lib.rs +++ b/rs/moq-msf/src/lib.rs @@ -13,7 +13,7 @@ use std::str::FromStr; use std::time::Duration; use serde::{Deserialize, Serialize}; -use serde_with::DurationMilliSecondsWithFrac; +use serde_with::DurationMilliSeconds; /// The default track name for the MSF catalog. pub const DEFAULT_NAME: &str = "catalog"; @@ -97,11 +97,11 @@ pub struct Track { #[serde(rename = "maxObjSapStartingType")] pub max_obj_sap_starting_type: Option, - /// Jitter (non-standard extension, matches JS implementation). + /// Jitter (non-standard extension; not in the MSF/CMSF drafts). /// - /// Serialized as a JSON number of milliseconds with fractional precision - /// (e.g. `15.5`), so it stays wire-compatible with existing JS consumers. - #[serde_as(as = "Option>")] + /// Serialized as a JSON integer number of milliseconds, matching the hang + /// catalog. Sub-ms precision isn't meaningful for jitter. + #[serde_as(as = "Option>")] pub jitter: Option, } @@ -445,7 +445,7 @@ mod test { alt_group: None, max_grp_sap_starting_type: Some(1), max_obj_sap_starting_type: Some(2), - jitter: Some(Duration::from_micros(15_500)), + jitter: Some(Duration::from_millis(15)), } } @@ -464,7 +464,7 @@ mod test { let track = &value["tracks"][0]; assert_eq!(track.get("maxGrpSapStartingType"), Some(&serde_json::json!(1))); assert_eq!(track.get("maxObjSapStartingType"), Some(&serde_json::json!(2))); - assert_eq!(track.get("jitter"), Some(&serde_json::json!(15.5))); + assert_eq!(track.get("jitter"), Some(&serde_json::json!(15))); // Snake-case names must NOT appear on the wire. assert!(track.get("max_grp_sap_starting_type").is_none()); @@ -510,6 +510,6 @@ mod test { assert_eq!(original, parsed); assert_eq!(parsed.tracks[0].max_grp_sap_starting_type, Some(1)); assert_eq!(parsed.tracks[0].max_obj_sap_starting_type, Some(2)); - assert_eq!(parsed.tracks[0].jitter, Some(Duration::from_micros(15_500))); + assert_eq!(parsed.tracks[0].jitter, Some(Duration::from_millis(15))); } } From c9939662ae0df850dd0c44023853eabc4c5720fa Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 23 May 2026 15:54:33 -0700 Subject: [PATCH 07/12] moq-net: Timestamp::cmp tie-breaks to satisfy Ord/Eq contract When two timestamps cross-mult to the same temporal value but have different (value, scale) representations (e.g. `from_secs(1)` and `from_millis(1000)`), cmp used to return Ordering::Equal while the derived PartialEq/Eq said they were unequal. That violates Rust's Ord/Eq contract and would break ordered-collection keys. Add a `(scale, value)` tie-break after the cross-multiply so cmp returns Equal iff the fields are equal, matching derived Eq. Test updated: `from_secs(1).cmp(&from_millis(1000))` now asserts `!= Equal`, and adds the round-trip `cmp(a, b) == Equal iff a == b`. Co-Authored-By: Claude Opus 4.7 (1M context) --- rs/moq-net/src/model/time.rs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/rs/moq-net/src/model/time.rs b/rs/moq-net/src/model/time.rs index f30655d13..e16b13914 100644 --- a/rs/moq-net/src/model/time.rs +++ b/rs/moq-net/src/model/time.rs @@ -379,10 +379,11 @@ impl Ord for Timestamp { /// fmp4/mkv exporters, which pick the next track to emit across mixed-scale /// per-track frames. /// - /// Note: derived `PartialEq`/`Eq` compare fields, so two cross-scale timestamps - /// that order as `Equal` here (e.g. `from_secs(1)` and `from_millis(1000)`) still - /// compare `!=`. This inconsistency only matters for code that mixes scales in a - /// `BTreeMap`/`BTreeSet` keyed on `Timestamp`, which the codebase doesn't do. + /// When the cross-scale comparison would otherwise be `Equal` (e.g. `from_secs(1)` + /// vs `from_millis(1000)`), we break ties by `(scale, value)` so the result agrees + /// with derived `PartialEq`/`Eq`/`Hash` (which are field-wise). Without that tie + /// break, `cmp` could return `Equal` for fields that aren't equal, violating + /// Rust's `Ord`/`Eq` contract for ordered collection keys. fn cmp(&self, other: &Self) -> std::cmp::Ordering { if self.scale.0 == other.scale.0 || self.scale.0 == 0 || other.scale.0 == 0 { return self.value.cmp(&other.value); @@ -390,6 +391,8 @@ impl Ord for Timestamp { let lhs = self.value.into_inner() as u128 * other.scale.0 as u128; let rhs = other.value.into_inner() as u128 * self.scale.0 as u128; lhs.cmp(&rhs) + .then_with(|| self.scale.0.cmp(&other.scale.0)) + .then_with(|| self.value.cmp(&other.value)) } } @@ -672,9 +675,15 @@ mod tests { assert!(one_sec > two_ms); assert!(two_ms < one_sec); - // Equivalent values across scales compare as Equal under cmp. + // Temporally-equivalent timestamps with different (value, scale) representations + // must NOT cmp as Equal: derived Eq compares fields, so cmp returning Equal + // here would violate the Ord/Eq contract for ordered collection keys. The + // tie-breaker resolves them by scale to keep ordering deterministic. let one_sec_b = Timestamp::from_millis(1000).unwrap(); - assert_eq!(one_sec.cmp(&one_sec_b), std::cmp::Ordering::Equal); + assert_ne!(one_sec.cmp(&one_sec_b), std::cmp::Ordering::Equal); + assert_ne!(one_sec, one_sec_b); + // And the tie-break agrees with PartialEq: cmp(a, b) == Equal iff a == b. + assert_eq!(one_sec.cmp(&one_sec), std::cmp::Ordering::Equal); // Sorting a mixed-scale slice puts entries in correct temporal order. let mut items = [ From 8a2f3eb8e9ace1faa5a71a9e74c85b4adcdf9106 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 23 May 2026 16:04:21 -0700 Subject: [PATCH 08/12] Drop container Timestamp re-export, dedup moq-loc varint Two cleanups: 1. **moq-mux::container**: removed the `pub use moq_net::{Timescale, Timestamp};` re-export and the `pub const TIMESCALE: Timescale = Timescale::MICRO;` constant. Containers now carry their own native timescale (fmp4 uses `mdhd.timescale`, mkv uses nanoseconds, legacy/LOC use microseconds), so there's no single "moq-mux container timescale" worth advertising. Call sites use `moq_net::{Timestamp, Timescale}` directly. LOC's microsecond fallback moves into a private `DEFAULT_TIMESCALE` const in `loc/mod.rs` since that's the only place that needed it. 2. **moq-loc**: drop the hand-rolled QUIC varint implementation and use `moq_net::coding::VarInt::{encode_quic, decode_quic}` instead. Adds `moq-net` as a dep, maps moq-net's `DecodeError` / `EncodeError` / `BoundsExceeded` into the local `Error` enum, keeps the existing public API and all 7 tests passing unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) --- Cargo.lock | 1 + rs/hang/examples/video.rs | 6 +- rs/moq-loc/Cargo.toml | 1 + rs/moq-loc/src/lib.rs | 158 +++++++------------ rs/moq-mux/src/codec/aac/import.rs | 8 +- rs/moq-mux/src/codec/av1/import.rs | 14 +- rs/moq-mux/src/codec/h264/import.rs | 18 +-- rs/moq-mux/src/codec/h265/import.rs | 14 +- rs/moq-mux/src/codec/opus/import.rs | 8 +- rs/moq-mux/src/container/consumer.rs | 4 +- rs/moq-mux/src/container/fmp4/export.rs | 2 +- rs/moq-mux/src/container/fmp4/export_test.rs | 2 +- rs/moq-mux/src/container/fmp4/import.rs | 6 +- rs/moq-mux/src/container/fmp4/mod.rs | 14 +- rs/moq-mux/src/container/jitter.rs | 2 +- rs/moq-mux/src/container/loc/mod.rs | 12 +- rs/moq-mux/src/container/mkv/export_test.rs | 2 +- rs/moq-mux/src/container/mkv/import.rs | 2 +- rs/moq-mux/src/container/mod.rs | 21 +-- rs/moq-mux/src/container/producer.rs | 2 +- rs/moq-mux/src/import.rs | 2 +- 21 files changed, 127 insertions(+), 172 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 428d647d9..069eb956e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3747,6 +3747,7 @@ name = "moq-loc" version = "0.1.0" dependencies = [ "bytes", + "moq-net", "thiserror 2.0.18", ] diff --git a/rs/hang/examples/video.rs b/rs/hang/examples/video.rs index e87b53f36..f1d0b73ef 100644 --- a/rs/hang/examples/video.rs +++ b/rs/hang/examples/video.rs @@ -114,7 +114,7 @@ async fn run_broadcast(origin: moq_net::OriginProducer) -> anyhow::Result<()> { // Not real frames of course. The first frame is a keyframe and starts the first group. let frame = moq_mux::container::Frame { - timestamp: moq_mux::container::Timestamp::from_secs(1).unwrap(), + timestamp: moq_net::Timestamp::from_secs(1).unwrap(), payload: Bytes::from_static(b"keyframe NAL data"), keyframe: true, }; @@ -123,7 +123,7 @@ async fn run_broadcast(origin: moq_net::OriginProducer) -> anyhow::Result<()> { tokio::time::sleep(tokio::time::Duration::from_secs(1)).await; let frame = moq_mux::container::Frame { - timestamp: moq_mux::container::Timestamp::from_secs(2).unwrap(), + timestamp: moq_net::Timestamp::from_secs(2).unwrap(), payload: Bytes::from_static(b"delta NAL data"), keyframe: false, }; @@ -133,7 +133,7 @@ async fn run_broadcast(origin: moq_net::OriginProducer) -> anyhow::Result<()> { // Marking this frame as a keyframe closes the current group and starts a new one. let frame = moq_mux::container::Frame { - timestamp: moq_mux::container::Timestamp::from_secs(3).unwrap(), + timestamp: moq_net::Timestamp::from_secs(3).unwrap(), payload: Bytes::from_static(b"keyframe NAL data"), keyframe: true, }; diff --git a/rs/moq-loc/Cargo.toml b/rs/moq-loc/Cargo.toml index 221755f9d..22a8f261a 100644 --- a/rs/moq-loc/Cargo.toml +++ b/rs/moq-loc/Cargo.toml @@ -17,4 +17,5 @@ doctest = false [dependencies] bytes = "1" +moq-net = { workspace = true } thiserror = "2" diff --git a/rs/moq-loc/src/lib.rs b/rs/moq-loc/src/lib.rs index efde0b057..d263a9fab 100644 --- a/rs/moq-loc/src/lib.rs +++ b/rs/moq-loc/src/lib.rs @@ -22,18 +22,15 @@ //! encode. Public properties are not handled here. They belong in the MoQ //! object header and are stripped by the transport layer. //! -//! Varint encoding is QUIC-style throughout, matching the rest of the moq -//! stack. +//! Varint encoding is QUIC-style throughout via [`moq_net::coding::VarInt`]. use bytes::{Buf, Bytes, BytesMut}; +use moq_net::coding::{BoundsExceeded, DecodeError, EncodeError, VarInt}; /// Property IDs recognized by this implementation. const PROP_TIMESTAMP: u64 = 0x06; const PROP_TIMESCALE: u64 = 0x08; -/// Maximum value representable as a 62-bit QUIC varint. -const VARINT_MAX: u64 = (1u64 << 62) - 1; - /// A decoded LOC frame. #[derive(Clone, Debug)] pub struct Frame { @@ -71,12 +68,38 @@ pub enum Error { OutOfRange, } +impl From for Error { + fn from(err: DecodeError) -> Self { + match err { + DecodeError::Short => Self::ShortBuffer, + // Any other decode error (invalid value, etc.) maps to a malformed + // property block; the caller can't distinguish them anyway. + _ => Self::MalformedProperties, + } + } +} + +impl From for Error { + fn from(err: EncodeError) -> Self { + match err { + EncodeError::Short => Self::ShortBuffer, + _ => Self::OutOfRange, + } + } +} + +impl From for Error { + fn from(_: BoundsExceeded) -> Self { + Self::OutOfRange + } +} + /// Decode a LOC frame. /// /// Consumes the properties_length prefix, walks the bounded property block, /// and returns the remainder as `payload`. pub fn decode(mut buf: Bytes) -> Result { - let properties_length = read_varint(&mut buf)?; + let properties_length: u64 = VarInt::decode_quic(&mut buf)?.into(); let properties_length: usize = properties_length.try_into().map_err(|_| Error::MalformedProperties)?; if properties_length > buf.remaining() { @@ -91,7 +114,7 @@ pub fn decode(mut buf: Bytes) -> Result { let mut first = true; while props.has_remaining() { - let delta = read_varint(&mut props)?; + let delta: u64 = VarInt::decode_quic(&mut props)?.into(); let abs = if first { first = false; delta @@ -101,7 +124,7 @@ pub fn decode(mut buf: Bytes) -> Result { prev_type = abs; if abs % 2 == 0 { - let value = read_varint(&mut props)?; + let value: u64 = VarInt::decode_quic(&mut props)?.into(); match abs { PROP_TIMESTAMP => timestamp = Some(value), PROP_TIMESCALE => { @@ -113,7 +136,7 @@ pub fn decode(mut buf: Bytes) -> Result { _ => {} } } else { - let len = read_varint(&mut props)?; + let len: u64 = VarInt::decode_quic(&mut props)?.into(); let len: usize = len.try_into().map_err(|_| Error::MalformedProperties)?; if len > props.remaining() { return Err(Error::MalformedProperties); @@ -139,87 +162,26 @@ pub fn decode(mut buf: Bytes) -> Result { /// catalog timescale to interpret `timestamp`. pub fn encode(timestamp: u64, payload: &[u8]) -> Result { let mut props = BytesMut::with_capacity(16); - write_varint(&mut props, PROP_TIMESTAMP)?; - write_varint(&mut props, timestamp)?; + VarInt::try_from(PROP_TIMESTAMP)?.encode_quic(&mut props)?; + VarInt::try_from(timestamp)?.encode_quic(&mut props)?; let mut out = BytesMut::with_capacity(props.len() + payload.len() + 8); - write_varint(&mut out, props.len() as u64)?; + VarInt::try_from(props.len() as u64)?.encode_quic(&mut out)?; out.extend_from_slice(&props); out.extend_from_slice(payload); Ok(out.freeze()) } -/// Decode a QUIC-style varint (2-bit length tag in top bits). -fn read_varint(buf: &mut B) -> Result { - if !buf.has_remaining() { - return Err(Error::ShortBuffer); - } - let b = buf.get_u8(); - let tag = b >> 6; - let mut bytes = [0u8; 8]; - bytes[0] = b & 0b0011_1111; - let value = match tag { - 0b00 => u64::from(bytes[0]), - 0b01 => { - if buf.remaining() < 1 { - return Err(Error::ShortBuffer); - } - buf.copy_to_slice(&mut bytes[1..2]); - u64::from(u16::from_be_bytes(bytes[..2].try_into().unwrap())) - } - 0b10 => { - if buf.remaining() < 3 { - return Err(Error::ShortBuffer); - } - buf.copy_to_slice(&mut bytes[1..4]); - u64::from(u32::from_be_bytes(bytes[..4].try_into().unwrap())) - } - 0b11 => { - if buf.remaining() < 7 { - return Err(Error::ShortBuffer); - } - buf.copy_to_slice(&mut bytes[1..8]); - u64::from_be_bytes(bytes) - } - _ => unreachable!(), - }; - Ok(value) -} - -/// Encode a QUIC-style varint (2-bit length tag in top bits). -fn write_varint(buf: &mut B, value: u64) -> Result<(), Error> { - if value > VARINT_MAX { - return Err(Error::OutOfRange); - } - if value < (1u64 << 6) { - if buf.remaining_mut() < 1 { - return Err(Error::ShortBuffer); - } - buf.put_u8(value as u8); - } else if value < (1u64 << 14) { - if buf.remaining_mut() < 2 { - return Err(Error::ShortBuffer); - } - buf.put_u16(value as u16 | 0b01 << 14); - } else if value < (1u64 << 30) { - if buf.remaining_mut() < 4 { - return Err(Error::ShortBuffer); - } - buf.put_u32(value as u32 | 0b10 << 30); - } else { - if buf.remaining_mut() < 8 { - return Err(Error::ShortBuffer); - } - buf.put_u64(value | 0b11 << 62); - } - Ok(()) -} - #[cfg(test)] mod tests { use super::*; + /// Test helper: write a u64 as a QUIC varint into `buf`. + fn write_varint(buf: &mut BytesMut, value: u64) { + VarInt::try_from(value).unwrap().encode_quic(buf).unwrap(); + } + #[test] fn roundtrip() { let payload = Bytes::from_static(b"hello world"); @@ -235,13 +197,13 @@ mod tests { fn decode_per_frame_timescale() { // Manually craft: properties = [delta=0x06 timestamp=96000, delta=0x02 (abs=0x08) timescale=48000] let mut props = BytesMut::new(); - write_varint(&mut props, PROP_TIMESTAMP).unwrap(); - write_varint(&mut props, 96_000).unwrap(); - write_varint(&mut props, PROP_TIMESCALE - PROP_TIMESTAMP).unwrap(); // delta = 2 - write_varint(&mut props, 48_000).unwrap(); + write_varint(&mut props, PROP_TIMESTAMP); + write_varint(&mut props, 96_000); + write_varint(&mut props, PROP_TIMESCALE - PROP_TIMESTAMP); // delta = 2 + write_varint(&mut props, 48_000); let mut frame = BytesMut::new(); - write_varint(&mut frame, props.len() as u64).unwrap(); + write_varint(&mut frame, props.len() as u64); frame.extend_from_slice(&props); frame.extend_from_slice(b"payload"); @@ -255,14 +217,14 @@ mod tests { fn decode_skips_video_config() { // properties = [delta=0x06 timestamp=10, delta=0x07 (abs=0x0d, video config) bytes=[1,2,3]] let mut props = BytesMut::new(); - write_varint(&mut props, PROP_TIMESTAMP).unwrap(); - write_varint(&mut props, 10).unwrap(); - write_varint(&mut props, 0x0d - PROP_TIMESTAMP).unwrap(); // delta = 7 -> abs 0x0d (Video Config) - write_varint(&mut props, 3).unwrap(); // length + write_varint(&mut props, PROP_TIMESTAMP); + write_varint(&mut props, 10); + write_varint(&mut props, 0x0d - PROP_TIMESTAMP); // delta = 7 -> abs 0x0d (Video Config) + write_varint(&mut props, 3); // length props.extend_from_slice(&[0x01, 0x02, 0x03]); let mut frame = BytesMut::new(); - write_varint(&mut frame, props.len() as u64).unwrap(); + write_varint(&mut frame, props.len() as u64); frame.extend_from_slice(&props); frame.extend_from_slice(b"data"); @@ -276,11 +238,11 @@ mod tests { fn decode_missing_timestamp_errors() { // properties = [delta=0x08 timescale=1000], no timestamp let mut props = BytesMut::new(); - write_varint(&mut props, PROP_TIMESCALE).unwrap(); - write_varint(&mut props, 1000).unwrap(); + write_varint(&mut props, PROP_TIMESCALE); + write_varint(&mut props, 1000); let mut frame = BytesMut::new(); - write_varint(&mut frame, props.len() as u64).unwrap(); + write_varint(&mut frame, props.len() as u64); frame.extend_from_slice(&props); frame.extend_from_slice(b"x"); @@ -290,7 +252,7 @@ mod tests { #[test] fn decode_empty_properties_errors() { let mut frame = BytesMut::new(); - write_varint(&mut frame, 0).unwrap(); + write_varint(&mut frame, 0); frame.extend_from_slice(b"payload"); assert!(matches!(decode(frame.freeze()), Err(Error::MissingTimestamp))); @@ -300,13 +262,13 @@ mod tests { fn decode_rejects_zero_timescale() { // Per-frame 0x08 timescale of 0 is invalid (would divide by zero). let mut props = BytesMut::new(); - write_varint(&mut props, PROP_TIMESTAMP).unwrap(); - write_varint(&mut props, 10).unwrap(); - write_varint(&mut props, PROP_TIMESCALE - PROP_TIMESTAMP).unwrap(); - write_varint(&mut props, 0).unwrap(); + write_varint(&mut props, PROP_TIMESTAMP); + write_varint(&mut props, 10); + write_varint(&mut props, PROP_TIMESCALE - PROP_TIMESTAMP); + write_varint(&mut props, 0); let mut frame = BytesMut::new(); - write_varint(&mut frame, props.len() as u64).unwrap(); + write_varint(&mut frame, props.len() as u64); frame.extend_from_slice(&props); frame.extend_from_slice(b"x"); @@ -316,7 +278,7 @@ mod tests { #[test] fn decode_overflowing_properties_length_errors() { let mut frame = BytesMut::new(); - write_varint(&mut frame, 100).unwrap(); // claims 100 bytes of properties + write_varint(&mut frame, 100); // claims 100 bytes of properties frame.extend_from_slice(&[0x06]); // only 1 byte follows assert!(matches!(decode(frame.freeze()), Err(Error::MalformedProperties))); diff --git a/rs/moq-mux/src/codec/aac/import.rs b/rs/moq-mux/src/codec/aac/import.rs index d1981f62d..b447d22d9 100644 --- a/rs/moq-mux/src/codec/aac/import.rs +++ b/rs/moq-mux/src/codec/aac/import.rs @@ -56,7 +56,7 @@ impl Import { Ok(()) } - pub fn decode(&mut self, buf: &mut T, pts: Option) -> anyhow::Result<()> { + pub fn decode(&mut self, buf: &mut T, pts: Option) -> anyhow::Result<()> { let pts = self.pts(pts)?; // Collect the input into a contiguous Bytes payload. @@ -82,15 +82,13 @@ impl Import { Ok(()) } - fn pts(&mut self, hint: Option) -> anyhow::Result { + fn pts(&mut self, hint: Option) -> anyhow::Result { if let Some(pts) = hint { return Ok(pts); } let zero = self.zero.get_or_insert_with(tokio::time::Instant::now); - Ok(crate::container::Timestamp::from_micros( - zero.elapsed().as_micros() as u64 - )?) + Ok(moq_net::Timestamp::from_micros(zero.elapsed().as_micros() as u64)?) } } diff --git a/rs/moq-mux/src/codec/av1/import.rs b/rs/moq-mux/src/codec/av1/import.rs index 0886b4513..d189c33dd 100644 --- a/rs/moq-mux/src/codec/av1/import.rs +++ b/rs/moq-mux/src/codec/av1/import.rs @@ -263,7 +263,7 @@ impl Import { pub fn decode_stream>( &mut self, buf: &mut T, - pts: Option, + pts: Option, ) -> anyhow::Result<()> { let obus = ObuIterator::new(buf); @@ -280,7 +280,7 @@ impl Import { pub fn decode_frame>( &mut self, buf: &mut T, - pts: Option, + pts: Option, ) -> anyhow::Result<()> { let pts = self.pts(pts)?; let mut obus = ObuIterator::new(buf); @@ -298,7 +298,7 @@ impl Import { Ok(()) } - fn decode_obu(&mut self, obu_data: Bytes, pts: Option) -> anyhow::Result<()> { + fn decode_obu(&mut self, obu_data: Bytes, pts: Option) -> anyhow::Result<()> { anyhow::ensure!(!obu_data.is_empty(), "OBU is too short"); // Parse OBU header - this consumes header + extension + LEB128 size @@ -381,7 +381,7 @@ impl Import { Ok(()) } - fn maybe_start_frame(&mut self, pts: Option) -> anyhow::Result<()> { + fn maybe_start_frame(&mut self, pts: Option) -> anyhow::Result<()> { if !self.current.contains_frame { return Ok(()); } @@ -425,15 +425,13 @@ impl Import { self.track.is_some() } - fn pts(&mut self, hint: Option) -> anyhow::Result { + fn pts(&mut self, hint: Option) -> anyhow::Result { if let Some(pts) = hint { return Ok(pts); } let zero = self.zero.get_or_insert_with(tokio::time::Instant::now); - Ok(crate::container::Timestamp::from_micros( - zero.elapsed().as_micros() as u64 - )?) + Ok(moq_net::Timestamp::from_micros(zero.elapsed().as_micros() as u64)?) } } diff --git a/rs/moq-mux/src/codec/h264/import.rs b/rs/moq-mux/src/codec/h264/import.rs index 817f4aab0..589783e81 100644 --- a/rs/moq-mux/src/codec/h264/import.rs +++ b/rs/moq-mux/src/codec/h264/import.rs @@ -215,7 +215,7 @@ impl Import { pub fn decode_stream>( &mut self, buf: &mut T, - pts: Option, + pts: Option, ) -> anyhow::Result<()> { anyhow::ensure!(matches!(self.state, State::Avc3 { .. }), "decode_stream is avc3 only"); let pts = self.pts(pts)?; @@ -234,7 +234,7 @@ impl Import { pub fn decode_frame>( &mut self, buf: &mut T, - pts: Option, + pts: Option, ) -> anyhow::Result<()> { match &self.state { State::Avc1 { .. } => self.decode_avc1(buf, pts), @@ -246,7 +246,7 @@ impl Import { fn decode_avc1>( &mut self, buf: &mut T, - pts: Option, + pts: Option, ) -> anyhow::Result<()> { let State::Avc1 { length_size } = self.state else { unreachable!("checked by decode_frame") @@ -278,7 +278,7 @@ impl Import { fn decode_avc3_frame>( &mut self, buf: &mut T, - pts: Option, + pts: Option, ) -> anyhow::Result<()> { let pts = self.pts(pts)?; let mut nals = NalIterator::new(buf); @@ -292,7 +292,7 @@ impl Import { Ok(()) } - fn decode_nal(&mut self, nal: Bytes, pts: Option) -> anyhow::Result<()> { + fn decode_nal(&mut self, nal: Bytes, pts: Option) -> anyhow::Result<()> { let header = nal.first().context("NAL unit is too short")?; let forbidden_zero_bit = (header >> 7) & 1; anyhow::ensure!(forbidden_zero_bit == 0, "forbidden zero bit is not zero"); @@ -414,7 +414,7 @@ impl Import { Ok(()) } - fn maybe_start_frame(&mut self, pts: Option) -> anyhow::Result<()> { + fn maybe_start_frame(&mut self, pts: Option) -> anyhow::Result<()> { let State::Avc3 { current, .. } = &mut self.state else { return Ok(()); }; @@ -477,14 +477,12 @@ impl Import { Ok(()) } - fn pts(&mut self, hint: Option) -> anyhow::Result { + fn pts(&mut self, hint: Option) -> anyhow::Result { if let Some(pts) = hint { return Ok(pts); } let zero = self.zero.get_or_insert_with(tokio::time::Instant::now); - Ok(crate::container::Timestamp::from_micros( - zero.elapsed().as_micros() as u64 - )?) + Ok(moq_net::Timestamp::from_micros(zero.elapsed().as_micros() as u64)?) } } diff --git a/rs/moq-mux/src/codec/h265/import.rs b/rs/moq-mux/src/codec/h265/import.rs index 67daa6f95..9a309fcce 100644 --- a/rs/moq-mux/src/codec/h265/import.rs +++ b/rs/moq-mux/src/codec/h265/import.rs @@ -134,7 +134,7 @@ impl Import { pub fn decode_stream>( &mut self, buf: &mut T, - pts: Option, + pts: Option, ) -> anyhow::Result<()> { let pts = self.pts(pts)?; @@ -158,7 +158,7 @@ impl Import { pub fn decode_frame>( &mut self, buf: &mut T, - pts: Option, + pts: Option, ) -> anyhow::Result<()> { let pts = self.pts(pts)?; // Iterate over the NAL units in the buffer based on start codes. @@ -182,7 +182,7 @@ impl Import { /// Decode a single NAL unit. Only reads the first header byte to extract nal_unit_type, /// Ignores nuh_layer_id and nuh_temporal_id_plus1. - fn decode_nal(&mut self, nal: Bytes, pts: Option) -> anyhow::Result<()> { + fn decode_nal(&mut self, nal: Bytes, pts: Option) -> anyhow::Result<()> { anyhow::ensure!(nal.len() >= 2, "NAL unit is too short"); // u16 header: [forbidden_zero_bit(1) | nal_unit_type(6) | nuh_layer_id(6) | nuh_temporal_id_plus1(3)] let header = nal.first().context("NAL unit is too short")?; @@ -293,7 +293,7 @@ impl Import { Ok(()) } - fn maybe_start_frame(&mut self, pts: Option) -> anyhow::Result<()> { + fn maybe_start_frame(&mut self, pts: Option) -> anyhow::Result<()> { // If we haven't seen any slices, we shouldn't flush yet. if !self.current.contains_slice { return Ok(()); @@ -338,15 +338,13 @@ impl Import { self.track.is_some() } - fn pts(&mut self, hint: Option) -> anyhow::Result { + fn pts(&mut self, hint: Option) -> anyhow::Result { if let Some(pts) = hint { return Ok(pts); } let zero = self.zero.get_or_insert_with(tokio::time::Instant::now); - Ok(crate::container::Timestamp::from_micros( - zero.elapsed().as_micros() as u64 - )?) + Ok(moq_net::Timestamp::from_micros(zero.elapsed().as_micros() as u64)?) } } diff --git a/rs/moq-mux/src/codec/opus/import.rs b/rs/moq-mux/src/codec/opus/import.rs index 673277a33..558ac1efc 100644 --- a/rs/moq-mux/src/codec/opus/import.rs +++ b/rs/moq-mux/src/codec/opus/import.rs @@ -54,7 +54,7 @@ impl Import { Ok(()) } - pub fn decode(&mut self, buf: &mut T, pts: Option) -> anyhow::Result<()> { + pub fn decode(&mut self, buf: &mut T, pts: Option) -> anyhow::Result<()> { let pts = self.pts(pts)?; // Collect the input into a contiguous Bytes payload. @@ -80,15 +80,13 @@ impl Import { Ok(()) } - fn pts(&mut self, hint: Option) -> anyhow::Result { + fn pts(&mut self, hint: Option) -> anyhow::Result { if let Some(pts) = hint { return Ok(pts); } let zero = self.zero.get_or_insert_with(tokio::time::Instant::now); - Ok(crate::container::Timestamp::from_micros( - zero.elapsed().as_micros() as u64 - )?) + Ok(moq_net::Timestamp::from_micros(zero.elapsed().as_micros() as u64)?) } } diff --git a/rs/moq-mux/src/container/consumer.rs b/rs/moq-mux/src/container/consumer.rs index 46b53c9b9..70d8fa451 100644 --- a/rs/moq-mux/src/container/consumer.rs +++ b/rs/moq-mux/src/container/consumer.rs @@ -1,7 +1,9 @@ use std::collections::VecDeque; use std::task::{Poll, ready}; -use super::{Container, Frame, Timestamp}; +use moq_net::Timestamp; + +use super::{Container, Frame}; /// Decode a moq-lite track into a stream of media [`Frame`]s in latency-bounded /// presentation order. diff --git a/rs/moq-mux/src/container/fmp4/export.rs b/rs/moq-mux/src/container/fmp4/export.rs index 00046fcc7..41e5029a6 100644 --- a/rs/moq-mux/src/container/fmp4/export.rs +++ b/rs/moq-mux/src/container/fmp4/export.rs @@ -483,7 +483,7 @@ fn encode_fragment(track: &mut Fmp4Track, frames: Vec) -> anyhow::Result< track.sequence_number += 1; Ok(crate::container::fmp4::encode_fragment( track.track_id, - crate::container::Timescale::new(track.timescale), + moq_net::Timescale::new(track.timescale), seq, &frames, )?) diff --git a/rs/moq-mux/src/container/fmp4/export_test.rs b/rs/moq-mux/src/container/fmp4/export_test.rs index 0ff727e69..5c7fff1d5 100644 --- a/rs/moq-mux/src/container/fmp4/export_test.rs +++ b/rs/moq-mux/src/container/fmp4/export_test.rs @@ -16,8 +16,8 @@ use mp4_atom::{DecodeMaybe, Encode}; /// entry whose avcC is built from the inline SPS+PPS. #[tokio::test(start_paused = true)] async fn avc3_source_to_cmaf_export_roundtrip() { - use crate::container::Timestamp; use hang::catalog::{Container, H264, VideoConfig}; + use moq_net::Timestamp; let broadcast = moq_net::Broadcast::new(); let mut producer = broadcast.produce(); diff --git a/rs/moq-mux/src/container/fmp4/import.rs b/rs/moq-mux/src/container/fmp4/import.rs index 87ed06128..a4191864b 100644 --- a/rs/moq-mux/src/container/fmp4/import.rs +++ b/rs/moq-mux/src/container/fmp4/import.rs @@ -1,7 +1,7 @@ -use crate::container::Timestamp; use anyhow::Context; use bytes::{Buf, Bytes, BytesMut}; use hang::catalog::{AAC, AudioCodec, AudioConfig, Container, H264, H265, VP9, VideoCodec, VideoConfig}; +use moq_net::Timestamp; use mp4_atom::{Any, Atom, DecodeMaybe, Encode, Mdat, Moof, Moov, Trak}; use std::collections::HashMap; use tokio::io::{AsyncRead, AsyncReadExt}; @@ -453,7 +453,7 @@ impl Import { let tfdt = traf.tfdt.as_ref().context("missing tfdt box")?; let mut dts = tfdt.base_media_decode_time; - let timescale = crate::container::Timescale::new(trak.mdia.mdhd.timescale as u64); + let timescale = moq_net::Timescale::new(trak.mdia.mdhd.timescale as u64); let mut offset = traf.tfhd.base_data_offset.unwrap_or_default() as usize; let mut track_data_start: Option = None; @@ -503,7 +503,7 @@ impl Import { let pts = (dts as i64 + entry.cts.unwrap_or_default() as i64) as u64; // Preserve the fmp4 track's native timescale so a passthrough re-emit // doesn't go through a lossy microsecond detour. - let timestamp = crate::container::Timestamp::new(pts, timescale)?; + let timestamp = moq_net::Timestamp::new(pts, timescale)?; if offset + size > mdat.data.len() { anyhow::bail!("invalid data offset"); diff --git a/rs/moq-mux/src/container/fmp4/mod.rs b/rs/moq-mux/src/container/fmp4/mod.rs index 93c0d8e27..527877642 100644 --- a/rs/moq-mux/src/container/fmp4/mod.rs +++ b/rs/moq-mux/src/container/fmp4/mod.rs @@ -23,7 +23,9 @@ use bytes::Bytes; use hang::catalog::{AudioCodec, AudioConfig, VideoCodec, VideoConfig}; use mp4_atom::Atom; -use crate::container::{Container, Frame, Timestamp}; +use moq_net::Timestamp; + +use crate::container::{Container, Frame}; #[derive(Debug, thiserror::Error)] #[non_exhaustive] @@ -109,7 +111,7 @@ impl Container for Wire { type Error = Error; fn write(&self, group: &mut moq_net::GroupProducer, frames: &[Frame]) -> Result<(), Self::Error> { - let timescale = crate::container::Timescale::new(self.trak.mdia.mdhd.timescale as u64); + let timescale = moq_net::Timescale::new(self.trak.mdia.mdhd.timescale as u64); let track_id = self.trak.tkhd.track_id; encode(group, frames, timescale, track_id) } @@ -125,12 +127,12 @@ impl Container for Wire { return Poll::Ready(Ok(None)); }; - let timescale = crate::container::Timescale::new(self.trak.mdia.mdhd.timescale as u64); + let timescale = moq_net::Timescale::new(self.trak.mdia.mdhd.timescale as u64); Poll::Ready(Ok(Some(decode(data, timescale)?))) } } -pub(crate) fn decode(data: Bytes, timescale: crate::container::Timescale) -> Result, Error> { +pub(crate) fn decode(data: Bytes, timescale: moq_net::Timescale) -> Result, Error> { use mp4_atom::DecodeMaybe; let mut cursor = std::io::Cursor::new(&data); @@ -193,7 +195,7 @@ pub(crate) fn decode(data: Bytes, timescale: crate::container::Timescale) -> Res pub(crate) fn encode( group: &mut moq_net::GroupProducer, frames: &[Frame], - timescale: crate::container::Timescale, + timescale: moq_net::Timescale, track_id: u32, ) -> Result<(), Error> { if frames.is_empty() { @@ -219,7 +221,7 @@ pub(crate) fn encode( /// Returns an empty `Bytes` when `frames` is empty. pub(crate) fn encode_fragment( track_id: u32, - timescale: crate::container::Timescale, + timescale: moq_net::Timescale, sequence_number: u32, frames: &[Frame], ) -> Result { diff --git a/rs/moq-mux/src/container/jitter.rs b/rs/moq-mux/src/container/jitter.rs index a227c03e4..6df040484 100644 --- a/rs/moq-mux/src/container/jitter.rs +++ b/rs/moq-mux/src/container/jitter.rs @@ -1,6 +1,6 @@ use std::time::Duration; -use crate::container::Timestamp; +use moq_net::Timestamp; /// Tracks the minimum duration between consecutive frames. /// diff --git a/rs/moq-mux/src/container/loc/mod.rs b/rs/moq-mux/src/container/loc/mod.rs index 3a3dfb534..5ffb7d93b 100644 --- a/rs/moq-mux/src/container/loc/mod.rs +++ b/rs/moq-mux/src/container/loc/mod.rs @@ -7,7 +7,13 @@ use std::task::Poll; -use crate::container::{Container, Frame, TIMESCALE, Timescale, Timestamp}; +use moq_net::{Timescale, Timestamp}; + +use crate::container::{Container, Frame}; + +/// LOC's catalog convention: timestamps are in microseconds when no per-frame +/// 0x08 timescale property is present. +const DEFAULT_TIMESCALE: Timescale = Timescale::MICRO; /// LOC wire format. Each moq frame holds one LOC frame. #[derive(Default)] @@ -20,7 +26,7 @@ impl Container for Wire { for frame in frames { // LOC's wire format omits per-frame timescale by convention; the catalog // default is microseconds, so convert at the boundary. - let timestamp = frame.timestamp.convert(TIMESCALE).map_err(hang::Error::from)?; + let timestamp = frame.timestamp.convert(DEFAULT_TIMESCALE).map_err(hang::Error::from)?; let data = moq_loc::encode(timestamp.value(), &frame.payload)?; let mut chunked = group.create_frame(data.len().into())?; @@ -42,7 +48,7 @@ impl Container for Wire { }; let loc = moq_loc::decode(data)?; - let scale = loc.timescale.map(Timescale::new).unwrap_or(TIMESCALE); + let scale = loc.timescale.map(Timescale::new).unwrap_or(DEFAULT_TIMESCALE); let timestamp = Timestamp::new(loc.timestamp, scale).map_err(hang::Error::from)?; Poll::Ready(Ok(Some(vec![Frame { diff --git a/rs/moq-mux/src/container/mkv/export_test.rs b/rs/moq-mux/src/container/mkv/export_test.rs index 2b235336a..9a3a81d00 100644 --- a/rs/moq-mux/src/container/mkv/export_test.rs +++ b/rs/moq-mux/src/container/mkv/export_test.rs @@ -266,8 +266,8 @@ async fn export_avc3_source_synthesizes_avcc_and_length_prefixes() { // Annex-B with inline SPS+PPS before keyframes. The exporter must // (a) defer the header until SPS+PPS arrive, (b) emit avcC in CodecPrivate, // (c) length-prefix the sample bytes in each SimpleBlock. - use crate::container::Timestamp; use hang::catalog::{Container, H264, VideoConfig}; + use moq_net::Timestamp; let broadcast = moq_net::Broadcast::new(); let mut producer = broadcast.produce(); diff --git a/rs/moq-mux/src/container/mkv/import.rs b/rs/moq-mux/src/container/mkv/import.rs index f962fa5e8..db2aa5896 100644 --- a/rs/moq-mux/src/container/mkv/import.rs +++ b/rs/moq-mux/src/container/mkv/import.rs @@ -2,10 +2,10 @@ use std::collections::HashMap; use std::convert::TryFrom; use std::io::Cursor; -use crate::container::Timestamp; use anyhow::Context; use bytes::{Buf, Bytes, BytesMut}; use hang::catalog::{AAC, AudioCodec, AudioConfig, Container, H264, H265, VP9, VideoCodec, VideoConfig}; +use moq_net::Timestamp; use mp4_atom::Atom; use tokio::io::{AsyncRead, AsyncReadExt}; use webm_iterable::WebmIterator; diff --git a/rs/moq-mux/src/container/mod.rs b/rs/moq-mux/src/container/mod.rs index 5b06401d3..ad46b2f66 100644 --- a/rs/moq-mux/src/container/mod.rs +++ b/rs/moq-mux/src/container/mod.rs @@ -29,13 +29,6 @@ pub use consumer::Consumer; pub use producer::Producer; pub(crate) use source::{CatalogSource, ExportSource}; -pub use moq_net::{Timescale, Timestamp}; - -/// Canonical timescale for the hang catalog and legacy/LOC wire formats: -/// microseconds. Containers with a richer wire format (CMAF, MKV) carry their -/// own timescale per-track instead. -pub const TIMESCALE: Timescale = Timescale::MICRO; - /// A decoded media frame: timestamp, payload bytes, keyframe flag. /// /// `payload` is the raw codec bitstream that gets handed to the decoder. @@ -45,14 +38,12 @@ pub const TIMESCALE: Timescale = Timescale::MICRO; pub struct Frame { /// Presentation timestamp. /// - /// Carries its own [`Timescale`]; importers preserve the source format's - /// native scale (e.g. nanoseconds from MKV, the mdhd timescale from fMP4) - /// so exporters can re-emit without a lossy detour through microseconds. - /// Wire formats that lack an on-wire scale ([`legacy`], [`loc`]) normalize - /// to [`TIMESCALE`] at the encode boundary. Frames within a track must be - /// in *decode* order, not display order. B-frames may have non-monotonic - /// presentation timestamps. - pub timestamp: Timestamp, + /// Each container picks its own native scale: fmp4 uses the source + /// `mdhd.timescale`, mkv uses nanoseconds, legacy/LOC use microseconds. + /// Importers preserve the source scale so exporters can re-emit without a + /// lossy detour. Frames within a track must be in *decode* order, not + /// display order. B-frames may have non-monotonic presentation timestamps. + pub timestamp: moq_net::Timestamp, /// Encoded codec payload. pub payload: Bytes, diff --git a/rs/moq-mux/src/container/producer.rs b/rs/moq-mux/src/container/producer.rs index b08f34b7d..c77b26c00 100644 --- a/rs/moq-mux/src/container/producer.rs +++ b/rs/moq-mux/src/container/producer.rs @@ -163,7 +163,7 @@ mod tests { use super::*; use crate::catalog::hang::Container; - use crate::container::Timestamp; + use moq_net::Timestamp; fn frame(timestamp_us: u64, keyframe: bool) -> Frame { Frame { diff --git a/rs/moq-mux/src/import.rs b/rs/moq-mux/src/import.rs index 57ca5d7b4..c722c3be1 100644 --- a/rs/moq-mux/src/import.rs +++ b/rs/moq-mux/src/import.rs @@ -189,7 +189,7 @@ impl Framed { pub fn decode_frame>( &mut self, buf: &mut T, - pts: Option, + pts: Option, ) -> anyhow::Result<()> { match self.decoder { FramedKind::H264(ref mut decoder) => decoder.decode_frame(buf, pts)?, From 6aed9f09b2563ca7db03f4a5e7348fe4d91d818d Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 23 May 2026 16:14:18 -0700 Subject: [PATCH 09/12] moq-net: From for Duration; un-export coding module Two follow-ups from review: **From for Duration**: revert the TryFrom I introduced during the port back to an infallible From, panicking on Timescale::UNKNOWN. The only unspecified-scale timestamps are the ZERO/MAX sentinels, which nobody converts to Duration. Lets the container::Producer latency check drop its as_nanos() / Result-juggling in favour of a clean `(last - first).into() >= self.latency` shape, and cleans up the .try_into().ok() callers in container::Consumer, container::jitter, fmp4 import, and moq-gst source. **coding module**: switch `pub mod coding` back to private and just re-export `VarInt` at the top level alongside the existing `BoundsExceeded`/`DecodeError`/`EncodeError`. Exposing the whole module just for VarInt leaked size/stream/reader/writer/version internals that shouldn't be part of moq-net's public surface. Downstream callers (hang, moq-loc) now use `moq_net::VarInt` directly. Co-Authored-By: Claude Opus 4.7 (1M context) --- rs/hang/src/container/frame.rs | 2 +- rs/moq-gst/src/source/imp.rs | 10 +++++----- rs/moq-loc/src/lib.rs | 4 ++-- rs/moq-mux/src/container/consumer.rs | 6 ++---- rs/moq-mux/src/container/fmp4/import.rs | 4 ++-- rs/moq-mux/src/container/jitter.rs | 7 +++---- rs/moq-mux/src/container/producer.rs | 13 ++++--------- rs/moq-net/src/lib.rs | 4 ++-- rs/moq-net/src/model/time.rs | 23 +++++++++++++++-------- 9 files changed, 36 insertions(+), 37 deletions(-) diff --git a/rs/hang/src/container/frame.rs b/rs/hang/src/container/frame.rs index 5842c740c..fbc8ca127 100644 --- a/rs/hang/src/container/frame.rs +++ b/rs/hang/src/container/frame.rs @@ -1,6 +1,6 @@ use bytes::{Buf, Bytes, BytesMut}; use derive_more::Debug; -use moq_net::coding::VarInt; +use moq_net::VarInt; use crate::Error; diff --git a/rs/moq-gst/src/source/imp.rs b/rs/moq-gst/src/source/imp.rs index 3f9b7a81d..b1985904e 100644 --- a/rs/moq-gst/src/source/imp.rs +++ b/rs/moq-gst/src/source/imp.rs @@ -525,11 +525,11 @@ async fn run_track_pump( let buffer_mut = buffer.get_mut().unwrap(); let pts = match reference_ts { - Some(reference) => match timestamp - .checked_sub(reference) - .and_then(|d| Duration::try_from(d)) - { - Ok(delta) => gst::ClockTime::from_nseconds(delta.as_nanos() as u64), + Some(reference) => match timestamp.checked_sub(reference) { + Ok(delta) => { + let d: Duration = delta.into(); + gst::ClockTime::from_nseconds(d.as_nanos() as u64) + } Err(_) => gst::ClockTime::ZERO, }, None => { diff --git a/rs/moq-loc/src/lib.rs b/rs/moq-loc/src/lib.rs index d263a9fab..597ba5ca1 100644 --- a/rs/moq-loc/src/lib.rs +++ b/rs/moq-loc/src/lib.rs @@ -22,10 +22,10 @@ //! encode. Public properties are not handled here. They belong in the MoQ //! object header and are stripped by the transport layer. //! -//! Varint encoding is QUIC-style throughout via [`moq_net::coding::VarInt`]. +//! Varint encoding is QUIC-style throughout via [`moq_net::VarInt`]. use bytes::{Buf, Bytes, BytesMut}; -use moq_net::coding::{BoundsExceeded, DecodeError, EncodeError, VarInt}; +use moq_net::{BoundsExceeded, DecodeError, EncodeError, VarInt}; /// Property IDs recognized by this implementation. const PROP_TIMESTAMP: u64 = 0x06; diff --git a/rs/moq-mux/src/container/consumer.rs b/rs/moq-mux/src/container/consumer.rs index 70d8fa451..7521f2933 100644 --- a/rs/moq-mux/src/container/consumer.rs +++ b/rs/moq-mux/src/container/consumer.rs @@ -129,7 +129,7 @@ impl Consumer { && current.sequence <= self.current { match current.poll_min_timestamp(waiter, &self.format) { - Poll::Ready(Ok(ts)) => std::time::Duration::try_from(ts).ok(), + Poll::Ready(Ok(ts)) => Some(std::time::Duration::from(ts)), _ => None, } } else { @@ -157,9 +157,7 @@ impl Consumer { } if let Poll::Ready(Ok(ts)) = group.poll_max_timestamp(waiter, &self.format) { - if let Ok(duration) = std::time::Duration::try_from(ts) { - max_timestamp = max_timestamp.max(duration); - } + max_timestamp = max_timestamp.max(ts.into()); break; // We know older groups won't be newer than this. } } diff --git a/rs/moq-mux/src/container/fmp4/import.rs b/rs/moq-mux/src/container/fmp4/import.rs index a4191864b..ea5e61e64 100644 --- a/rs/moq-mux/src/container/fmp4/import.rs +++ b/rs/moq-mux/src/container/fmp4/import.rs @@ -644,7 +644,7 @@ impl Import { .renditions .get_mut(&track.track.name) .context("missing video config")?; - config.jitter = Some(std::time::Duration::try_from(jitter)?); + config.jitter = Some(jitter.into()); } TrackKind::Audio => { let config = catalog @@ -652,7 +652,7 @@ impl Import { .renditions .get_mut(&track.track.name) .context("missing audio config")?; - config.jitter = Some(std::time::Duration::try_from(jitter)?); + config.jitter = Some(jitter.into()); } } } diff --git a/rs/moq-mux/src/container/jitter.rs b/rs/moq-mux/src/container/jitter.rs index 6df040484..e026b3776 100644 --- a/rs/moq-mux/src/container/jitter.rs +++ b/rs/moq-mux/src/container/jitter.rs @@ -22,9 +22,8 @@ impl MinFrameDuration { /// /// Returns the new minimum-frame-duration as a [`Duration`] if it changed, so /// the caller can persist it on the catalog rendition. Returns `None` when this - /// is the first observation, the timestamps are non-monotonic, the new gap is - /// no smaller than the recorded minimum, or the timestamp's scale is - /// unspecified. + /// is the first observation, the timestamps are non-monotonic, or the new gap + /// is no smaller than the recorded minimum. pub fn observe(&mut self, ts: Timestamp) -> Option { let last = self.last_timestamp.replace(ts)?; let duration = ts.checked_sub(last).ok()?; @@ -36,6 +35,6 @@ impl MinFrameDuration { } self.min_duration = Some(duration); - Duration::try_from(duration).ok() + Some(duration.into()) } } diff --git a/rs/moq-mux/src/container/producer.rs b/rs/moq-mux/src/container/producer.rs index c77b26c00..68c4ad68d 100644 --- a/rs/moq-mux/src/container/producer.rs +++ b/rs/moq-mux/src/container/producer.rs @@ -90,16 +90,11 @@ impl Producer { } else { self.buffer.push(frame); - // Check if buffered duration exceeds latency. Compare in nanoseconds so the - // check works regardless of the frame's native scale; if either timestamp's - // scale is unspecified we conservatively skip the check and let the next - // keyframe (or `finish()`) flush. + // Flush if the buffered span has reached the latency budget. if self.buffer.len() >= 2 { - let first = self.buffer.first().unwrap().timestamp.as_nanos(); - let last = self.buffer.last().unwrap().timestamp.as_nanos(); - if let (Ok(first), Ok(last)) = (first, last) - && last.saturating_sub(first) >= self.latency.as_nanos() - { + let first: std::time::Duration = self.buffer.first().unwrap().timestamp.into(); + let last: std::time::Duration = self.buffer.last().unwrap().timestamp.into(); + if last.saturating_sub(first) >= self.latency { self.flush()?; } } diff --git a/rs/moq-net/src/lib.rs b/rs/moq-net/src/lib.rs index 5c43c5ebb..523123a66 100644 --- a/rs/moq-net/src/lib.rs +++ b/rs/moq-net/src/lib.rs @@ -49,7 +49,7 @@ //! standard [`std::task::Waker`] API and any [`std::task::Waker`] is a valid driver. mod client; -pub mod coding; +mod coding; mod error; mod ietf; mod lite; @@ -62,7 +62,7 @@ mod stats; mod version; pub use client::*; -pub use coding::{BoundsExceeded, DecodeError, EncodeError}; +pub use coding::{BoundsExceeded, DecodeError, EncodeError, VarInt}; pub use error::*; pub use model::*; pub use path::*; diff --git a/rs/moq-net/src/model/time.rs b/rs/moq-net/src/model/time.rs index e16b13914..946594000 100644 --- a/rs/moq-net/src/model/time.rs +++ b/rs/moq-net/src/model/time.rs @@ -326,13 +326,20 @@ impl TryFrom for Timestamp { } } -impl TryFrom for std::time::Duration { - type Error = TimeOverflow; - - fn try_from(time: Timestamp) -> Result { - let secs = time.as_secs()?; - let nanos = time.as_nanos()?; - Ok(std::time::Duration::new(secs, (nanos % 1_000_000_000) as u32)) +impl From for std::time::Duration { + /// Panics if the timestamp's scale is [`Timescale::UNKNOWN`]. + /// + /// In practice every concrete timestamp produced by an importer carries a known + /// scale; only the [`Timestamp::ZERO`] / [`Timestamp::MAX`] sentinels lack one, + /// and those never get converted to a `Duration`. + fn from(time: Timestamp) -> Self { + let secs = time + .as_secs() + .expect("cannot convert unspecified-scale Timestamp to Duration"); + let nanos = time + .as_nanos() + .expect("cannot convert unspecified-scale Timestamp to Duration"); + std::time::Duration::new(secs, (nanos % 1_000_000_000) as u32) } } @@ -704,7 +711,7 @@ mod tests { assert_eq!(time.scale(), Timescale::NANO); assert_eq!(time.as_secs().unwrap(), 5); - let duration_back: std::time::Duration = time.try_into().unwrap(); + let duration_back: std::time::Duration = time.into(); assert_eq!(duration_back.as_secs(), 5); } From 6aece1bd136b5e0514034932c7f43be7e80b69a3 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Sat, 23 May 2026 16:18:45 -0700 Subject: [PATCH 10/12] moq-net + moq-mux: scale/B-frame robustness + doc/thiserror nits Four fixes from review: 1. **Timestamp::from_scale**: reject UNKNOWN as the *target* scale, not just the source. Previously `from_scale(_, MILLI, UNKNOWN)` would multiply by 0 and silently return a 0/?-scale timestamp, contradicting the "UNKNOWN-scale conversions fail" contract. Test added. 2. **container::Producer + fmp4::should_flush**: latency check now uses min/max PTS across the buffered window rather than first/last. Frames within a track are in *decode* order; with B-frames the PTS can move backwards, so `last - first` can shrink and miss the latency threshold. Min/max correctly captures the presentation span. 3. **container::Frame doc**: clarified that LOC frames preserve any per-frame wire timescale on decode rather than always landing as micros, matching what `loc::Wire::poll_read` actually does. 4. **moq-loc Error**: the simple BoundsExceeded -> OutOfRange mapping moves to thiserror's `#[from]`. DecodeError/EncodeError keep their manual impls (they intentionally collapse Short vs everything-else). Co-Authored-By: Claude Opus 4.7 (1M context) --- rs/moq-loc/src/lib.rs | 17 ++++++----------- rs/moq-mux/src/container/fmp4/export.rs | 21 ++++++++++++++------- rs/moq-mux/src/container/mod.rs | 9 +++++---- rs/moq-mux/src/container/producer.rs | 13 +++++++++---- rs/moq-net/src/model/time.rs | 13 +++++++++++-- 5 files changed, 45 insertions(+), 28 deletions(-) diff --git a/rs/moq-loc/src/lib.rs b/rs/moq-loc/src/lib.rs index 597ba5ca1..5ac039b9b 100644 --- a/rs/moq-loc/src/lib.rs +++ b/rs/moq-loc/src/lib.rs @@ -64,16 +64,17 @@ pub enum Error { ShortBuffer, /// A value exceeds the 62-bit varint range. - #[error("value out of range")] - OutOfRange, + #[error("value out of range: {0}")] + OutOfRange(#[from] BoundsExceeded), } +// DecodeError / EncodeError intentionally collapse into ShortBuffer vs the +// caller's catch-all variant, so they stay as manual From impls; #[from] can't +// express that mapping. impl From for Error { fn from(err: DecodeError) -> Self { match err { DecodeError::Short => Self::ShortBuffer, - // Any other decode error (invalid value, etc.) maps to a malformed - // property block; the caller can't distinguish them anyway. _ => Self::MalformedProperties, } } @@ -83,17 +84,11 @@ impl From for Error { fn from(err: EncodeError) -> Self { match err { EncodeError::Short => Self::ShortBuffer, - _ => Self::OutOfRange, + _ => Self::OutOfRange(BoundsExceeded), } } } -impl From for Error { - fn from(_: BoundsExceeded) -> Self { - Self::OutOfRange - } -} - /// Decode a LOC frame. /// /// Consumes the properties_length prefix, walks the bounded property block, diff --git a/rs/moq-mux/src/container/fmp4/export.rs b/rs/moq-mux/src/container/fmp4/export.rs index 41e5029a6..d69cf270c 100644 --- a/rs/moq-mux/src/container/fmp4/export.rs +++ b/rs/moq-mux/src/container/fmp4/export.rs @@ -460,14 +460,21 @@ fn should_flush(track: &Fmp4Track, frame: &Frame, fragment_duration: Option true, Some(d) => { - let first = track.buffer.first().unwrap(); - // Compare in nanoseconds so the check works regardless of the source - // scale; an unspecified scale conservatively skips the duration cap and - // lets the next keyframe (or `None` fallback) flush. - match (first.timestamp.as_nanos(), frame.timestamp.as_nanos()) { - (Ok(first_ns), Ok(frame_ns)) => frame_ns.saturating_sub(first_ns) >= d.as_nanos(), - _ => false, + // Frames within a track are in *decode* order; B-frames have + // non-monotonic PTS, so the span of the buffer is min..max of all + // PTS, not just first..incoming. + let mut min = Duration::from(frame.timestamp); + let mut max = min; + for f in &track.buffer { + let pts = Duration::from(f.timestamp); + if pts < min { + min = pts; + } + if pts > max { + max = pts; + } } + max.saturating_sub(min) >= d } // No video keyframe will ever arrive to roll the fragment, so for // audio-only broadcasts in `None` mode we fall back to per-frame diff --git a/rs/moq-mux/src/container/mod.rs b/rs/moq-mux/src/container/mod.rs index ad46b2f66..cfb87651b 100644 --- a/rs/moq-mux/src/container/mod.rs +++ b/rs/moq-mux/src/container/mod.rs @@ -39,10 +39,11 @@ pub struct Frame { /// Presentation timestamp. /// /// Each container picks its own native scale: fmp4 uses the source - /// `mdhd.timescale`, mkv uses nanoseconds, legacy/LOC use microseconds. - /// Importers preserve the source scale so exporters can re-emit without a - /// lossy detour. Frames within a track must be in *decode* order, not - /// display order. B-frames may have non-monotonic presentation timestamps. + /// `mdhd.timescale`, mkv uses nanoseconds, legacy is fixed at microseconds. + /// LOC defaults to microseconds but a decoded frame keeps whatever per-frame + /// timescale the wire carried, so an exporter can re-emit without forcing + /// micros. Frames within a track must be in *decode* order, not display + /// order. B-frames may have non-monotonic presentation timestamps. pub timestamp: moq_net::Timestamp, /// Encoded codec payload. diff --git a/rs/moq-mux/src/container/producer.rs b/rs/moq-mux/src/container/producer.rs index 68c4ad68d..34527f54e 100644 --- a/rs/moq-mux/src/container/producer.rs +++ b/rs/moq-mux/src/container/producer.rs @@ -90,11 +90,16 @@ impl Producer { } else { self.buffer.push(frame); - // Flush if the buffered span has reached the latency budget. + // Flush if the buffered span has reached the latency budget. Compute + // min/max across the buffer rather than first/last: frames within a track + // are in *decode* order, and B-frames have non-monotonic PTS, so + // `last - first` can shrink as a B-frame lands between two earlier-PTS + // frames. The min/max pair captures the actual presentation span. if self.buffer.len() >= 2 { - let first: std::time::Duration = self.buffer.first().unwrap().timestamp.into(); - let last: std::time::Duration = self.buffer.last().unwrap().timestamp.into(); - if last.saturating_sub(first) >= self.latency { + let mut iter = self.buffer.iter().map(|f| std::time::Duration::from(f.timestamp)); + let first = iter.next().unwrap(); + let (min, max) = iter.fold((first, first), |(min, max), d| (min.min(d), max.max(d))); + if max.saturating_sub(min) >= self.latency { self.flush()?; } } diff --git a/rs/moq-net/src/model/time.rs b/rs/moq-net/src/model/time.rs index 946594000..de1c0d122 100644 --- a/rs/moq-net/src/model/time.rs +++ b/rs/moq-net/src/model/time.rs @@ -131,9 +131,10 @@ impl Timestamp { /// Convert `value` measured at `source` (units per second) to a timestamp at `target`. /// - /// Returns [`TimeOverflow`] on overflow or if `source` is [`Timescale::UNKNOWN`]. + /// Returns [`TimeOverflow`] on overflow, or if either `source` or `target` is + /// [`Timescale::UNKNOWN`]. pub const fn from_scale(value: u64, source: Timescale, target: Timescale) -> Result { - if source.0 == 0 { + if source.0 == 0 || target.0 == 0 { return Err(TimeOverflow); } match (value as u128).checked_mul(target.0 as u128) { @@ -753,4 +754,12 @@ mod tests { fn test_from_scale_zero_source() { assert!(Timestamp::from_scale(5, Timescale::UNKNOWN, Timescale::MILLI).is_err()); } + + #[test] + fn test_from_scale_zero_target() { + // Otherwise the multiply-by-zero silently discards the value and we'd + // hand back a 0/?-scale timestamp, breaking the UNKNOWN-conversions-fail + // contract. + assert!(Timestamp::from_scale(5, Timescale::MILLI, Timescale::UNKNOWN).is_err()); + } } From 2d0f818e7e3aa952ae406a76e24cf40bd6ded86d Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 28 May 2026 11:54:23 -0700 Subject: [PATCH 11/12] moq-net: Timescale is NonZero; drop UNKNOWN sentinel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Encode "no scale" with `Option` at the type level instead of a magic zero value. `Timescale` is now a newtype around `NonZero`, which makes the divide-by-`scale` arithmetic on `Timestamp` safe by construction — there is no runtime path that can hit divide-by-zero. Cascading API simplifications: - Removed `Timescale::UNKNOWN`, `Timescale::is_unknown`, `Timestamp::ZERO`, `Timestamp::MAX`, `Timestamp::is_unspecified`. - `Timestamp::as_secs` / `as_millis` / `as_micros` / `as_nanos` / `as_scale` are now infallible (scale is always known). - `Timestamp::convert` and `Timestamp::from_scale` only fail on varint overflow, not on UNKNOWN. - `Timestamp::cmp` no longer needs the UNKNOWN sentinel branch. - `From for Duration` is infallible. - Dropped the `Decode`/`Encode` impls on Timestamp (callers decode a `VarInt` and attach a Timescale via `new`; no in-tree caller used the trait impls anymore). - `Timescale::new(u64)` returns `Option` (was infallible); added `TryFrom` for the `?` ergonomics. Call-site updates: - fmp4: `mdhd.timescale == 0` now surfaces as `Error::InvalidTimescale` rather than silently producing a 0/?-scale timestamp. - fmp4 import: sentinel `unwrap_or(Timestamp::ZERO/MAX)` patterns rewritten as `Option::is_none_or` checks. - LOC: per-frame timescale=Some(0) is rejected upstream by `moq_loc::decode`, so `loc.timescale.and_then(Timescale::new)` is total in practice; falls back to the catalog default if absent. - libmoq / moq-ffi / moq-audio / mkv-export: drop the now-dead `as_*().map_err(...)?` adapters. Co-Authored-By: Claude Opus 4.7 (1M context) --- rs/libmoq/src/consume.rs | 2 +- rs/moq-audio/src/consumer.rs | 1 - rs/moq-ffi/src/consumer.rs | 1 - rs/moq-mux/src/container/consumer.rs | 2 +- rs/moq-mux/src/container/fmp4/export.rs | 3 +- rs/moq-mux/src/container/fmp4/import.rs | 11 +- rs/moq-mux/src/container/fmp4/mod.rs | 9 +- rs/moq-mux/src/container/loc/mod.rs | 5 +- rs/moq-mux/src/container/mkv/export.rs | 1 - rs/moq-net/src/model/time.rs | 377 ++++++++---------------- 10 files changed, 136 insertions(+), 276 deletions(-) diff --git a/rs/libmoq/src/consume.rs b/rs/libmoq/src/consume.rs index 51bcedbf3..69b5cbfb1 100644 --- a/rs/libmoq/src/consume.rs +++ b/rs/libmoq/src/consume.rs @@ -329,7 +329,7 @@ impl Consume { pub fn frame(&self, frame: Id, dst: &mut moq_frame) -> Result<(), Error> { let f = self.frame.get(frame).ok_or(Error::FrameNotFound)?; - let timestamp_us = f.timestamp.as_micros()?.try_into().map_err(|_| moq_net::TimeOverflow)?; + let timestamp_us = f.timestamp.as_micros().try_into().map_err(|_| moq_net::TimeOverflow)?; *dst = moq_frame { payload: f.payload.as_ptr(), diff --git a/rs/moq-audio/src/consumer.rs b/rs/moq-audio/src/consumer.rs index 6409c296e..7c6b95efa 100644 --- a/rs/moq-audio/src/consumer.rs +++ b/rs/moq-audio/src/consumer.rs @@ -93,7 +93,6 @@ impl AudioConsumer { let ts_us: u64 = mux_frame .timestamp .as_micros() - .map_err(|_| AudioError::Unsupported("timestamp scale unspecified".into()))? .try_into() .map_err(|_| AudioError::Unsupported("timestamp overflow".into()))?; diff --git a/rs/moq-ffi/src/consumer.rs b/rs/moq-ffi/src/consumer.rs index 0357690a5..93cb4a3a2 100644 --- a/rs/moq-ffi/src/consumer.rs +++ b/rs/moq-ffi/src/consumer.rs @@ -62,7 +62,6 @@ impl Media { let timestamp_us: u64 = frame .timestamp .as_micros() - .map_err(|_| MoqError::Codec("timestamp scale unspecified".into()))? .try_into() .map_err(|_| MoqError::Codec("timestamp overflow".into()))?; diff --git a/rs/moq-mux/src/container/consumer.rs b/rs/moq-mux/src/container/consumer.rs index 7521f2933..5f63ec70d 100644 --- a/rs/moq-mux/src/container/consumer.rs +++ b/rs/moq-mux/src/container/consumer.rs @@ -889,7 +889,7 @@ mod tests { let frames = read_all(&mut consumer).await.unwrap(); assert_eq!(frames.len(), 1); assert_eq!(frames[0].timestamp, ts(one_hour)); - assert_eq!(frames[0].timestamp.as_micros().unwrap(), one_hour as u128); + assert_eq!(frames[0].timestamp.as_micros(), one_hour as u128); } #[tokio::test] diff --git a/rs/moq-mux/src/container/fmp4/export.rs b/rs/moq-mux/src/container/fmp4/export.rs index d69cf270c..d5b1716bf 100644 --- a/rs/moq-mux/src/container/fmp4/export.rs +++ b/rs/moq-mux/src/container/fmp4/export.rs @@ -488,9 +488,10 @@ fn encode_fragment(track: &mut Fmp4Track, frames: Vec) -> anyhow::Result< anyhow::ensure!(!frames.is_empty(), "encode_fragment called with no frames"); let seq = track.sequence_number; track.sequence_number += 1; + let timescale = moq_net::Timescale::new(track.timescale).context("track timescale must be non-zero")?; Ok(crate::container::fmp4::encode_fragment( track.track_id, - moq_net::Timescale::new(track.timescale), + timescale, seq, &frames, )?) diff --git a/rs/moq-mux/src/container/fmp4/import.rs b/rs/moq-mux/src/container/fmp4/import.rs index c61e14836..e38c8d113 100644 --- a/rs/moq-mux/src/container/fmp4/import.rs +++ b/rs/moq-mux/src/container/fmp4/import.rs @@ -411,7 +411,8 @@ impl Import { let tfdt = traf.tfdt.as_ref().context("missing tfdt box")?; let mut dts = tfdt.base_media_decode_time; - let timescale = moq_net::Timescale::new(trak.mdia.mdhd.timescale as u64); + let timescale = moq_net::Timescale::new(trak.mdia.mdhd.timescale as u64) + .context("fmp4 mdhd.timescale must be non-zero")?; let mut offset = traf.tfhd.base_data_offset.unwrap_or_default() as usize; let mut track_data_start: Option = None; @@ -478,16 +479,16 @@ impl Import { contains_keyframe |= keyframe; - if timestamp >= max_timestamp.unwrap_or(Timestamp::ZERO) { + if max_timestamp.is_none_or(|max| timestamp >= max) { max_timestamp = Some(timestamp); } - if timestamp <= min_timestamp.unwrap_or(Timestamp::MAX) { + if min_timestamp.is_none_or(|min| timestamp <= min) { min_timestamp = Some(timestamp); } if let Some(last_timestamp) = track.last_timestamp && let Ok(duration) = timestamp.checked_sub(last_timestamp) - && duration < track.min_duration.unwrap_or(Timestamp::MAX) + && track.min_duration.is_none_or(|min| duration < min) { track.min_duration = Some(duration); } @@ -593,7 +594,7 @@ impl Import { if let (Some(min), Some(max), Some(min_duration)) = (min_timestamp, max_timestamp, track.min_duration) { let jitter = max - min + min_duration; - if jitter < track.jitter.unwrap_or(Timestamp::MAX) { + if track.jitter.is_none_or(|j| jitter < j) { track.jitter = Some(jitter); let mut catalog = self.catalog.lock(); diff --git a/rs/moq-mux/src/container/fmp4/mod.rs b/rs/moq-mux/src/container/fmp4/mod.rs index 527877642..462f8245b 100644 --- a/rs/moq-mux/src/container/fmp4/mod.rs +++ b/rs/moq-mux/src/container/fmp4/mod.rs @@ -65,6 +65,9 @@ pub enum Error { #[error("can't synthesize CMAF init for {0}")] UnsupportedSynthesis(String), + + #[error("invalid mdhd.timescale (must be non-zero)")] + InvalidTimescale, } /// CMAF container: encodes/decodes a single track's moof+mdat fragments. @@ -111,7 +114,7 @@ impl Container for Wire { type Error = Error; fn write(&self, group: &mut moq_net::GroupProducer, frames: &[Frame]) -> Result<(), Self::Error> { - let timescale = moq_net::Timescale::new(self.trak.mdia.mdhd.timescale as u64); + let timescale = moq_net::Timescale::new(self.trak.mdia.mdhd.timescale as u64).ok_or(Error::InvalidTimescale)?; let track_id = self.trak.tkhd.track_id; encode(group, frames, timescale, track_id) } @@ -127,7 +130,7 @@ impl Container for Wire { return Poll::Ready(Ok(None)); }; - let timescale = moq_net::Timescale::new(self.trak.mdia.mdhd.timescale as u64); + let timescale = moq_net::Timescale::new(self.trak.mdia.mdhd.timescale as u64).ok_or(Error::InvalidTimescale)?; Poll::Ready(Ok(Some(decode(data, timescale)?))) } } @@ -235,7 +238,7 @@ pub(crate) fn encode_fragment( // importer preserved the source scale (the common passthrough case), this is a // no-op; otherwise it's a single rescale rather than the legacy `micros * scale // / 1_000_000` round-trip. - let dts = frames[0].timestamp.as_scale(timescale)? as u64; + let dts = frames[0].timestamp.as_scale(timescale) as u64; let entries: Vec<_> = frames .iter() diff --git a/rs/moq-mux/src/container/loc/mod.rs b/rs/moq-mux/src/container/loc/mod.rs index 5ffb7d93b..f1e3e8902 100644 --- a/rs/moq-mux/src/container/loc/mod.rs +++ b/rs/moq-mux/src/container/loc/mod.rs @@ -48,7 +48,10 @@ impl Container for Wire { }; let loc = moq_loc::decode(data)?; - let scale = loc.timescale.map(Timescale::new).unwrap_or(DEFAULT_TIMESCALE); + // `loc.timescale == Some(0)` is a malformed wire (caught by moq_loc::decode itself), + // so any Some(_) we see here is non-zero. Falling back to the catalog default + // keeps this code path infallible. + let scale = loc.timescale.and_then(Timescale::new).unwrap_or(DEFAULT_TIMESCALE); let timestamp = Timestamp::new(loc.timestamp, scale).map_err(hang::Error::from)?; Poll::Ready(Ok(Some(vec![Frame { diff --git a/rs/moq-mux/src/container/mkv/export.rs b/rs/moq-mux/src/container/mkv/export.rs index 7a8a34f23..38c934f2b 100644 --- a/rs/moq-mux/src/container/mkv/export.rs +++ b/rs/moq-mux/src/container/mkv/export.rs @@ -464,7 +464,6 @@ impl Export { let frame_ticks: u64 = frame .timestamp .as_millis() - .context("timestamp scale unspecified")? .try_into() .context("timestamp doesn't fit in u64 ms")?; diff --git a/rs/moq-net/src/model/time.rs b/rs/moq-net/src/model/time.rs index de1c0d122..c8b37f3c1 100644 --- a/rs/moq-net/src/model/time.rs +++ b/rs/moq-net/src/model/time.rs @@ -1,13 +1,13 @@ -use rand::Rng; - -use crate::coding::{Decode, DecodeError, Encode, EncodeError, VarInt}; - +use std::num::NonZero; use std::sync::LazyLock; use std::time::{SystemTime, UNIX_EPOCH}; +use rand::Rng; + +use crate::coding::VarInt; + /// Returned when a [`Timestamp`] operation would exceed the QUIC VarInt range -/// (`2^62 - 1`), overflow during scale conversion or arithmetic, hit a divide -/// by zero from an unspecified ([`Timescale::UNKNOWN`]) scale, or attempt +/// (`2^62 - 1`), overflow during scale conversion or arithmetic, or attempt /// arithmetic between timestamps with mismatched scales. #[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)] #[error("time overflow")] @@ -15,51 +15,70 @@ pub struct TimeOverflow; /// Units per second used by a track for frame timestamps. /// -/// Newtype around `u64`. Use the named constants ([`Self::SECOND`], [`Self::MILLI`], -/// [`Self::MICRO`], [`Self::NANO`]) instead of writing raw integers at call sites. -/// -/// [`Self::UNKNOWN`] (raw value `0`) denotes an unspecified scale, produced by -/// [`Timestamp::ZERO`] and [`Timestamp::MAX`] sentinels. Unit conversions against -/// an unknown scale return [`TimeOverflow`] to avoid divide by zero. -#[derive(Clone, Copy, Default, PartialEq, Eq, Hash, PartialOrd, Ord)] +/// Newtype around [`NonZero`] — zero is structurally impossible, so the +/// arithmetic on [`Timestamp`] can divide by `self.scale` without ever risking +/// a divide by zero. Use the named constants ([`Self::SECOND`], [`Self::MILLI`], +/// [`Self::MICRO`], [`Self::NANO`]) instead of writing raw integers at call sites; +/// for runtime values, use [`Self::new`] which returns `None` for `0`. +#[derive(Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] -pub struct Timescale(pub u64); +pub struct Timescale(NonZero); impl Timescale { - /// Unspecified scale. Conversions involving this return [`TimeOverflow`]. - pub const UNKNOWN: Self = Self(0); /// One unit per second (`1`). - pub const SECOND: Self = Self(1); + pub const SECOND: Self = Self(NonZero::::MIN); /// 1,000 units per second (`1_000`). - pub const MILLI: Self = Self(1_000); + pub const MILLI: Self = match NonZero::new(1_000) { + Some(n) => Self(n), + None => unreachable!(), + }; /// 1,000,000 units per second (`1_000_000`). Common default for media tracks. - pub const MICRO: Self = Self(1_000_000); + pub const MICRO: Self = match NonZero::new(1_000_000) { + Some(n) => Self(n), + None => unreachable!(), + }; /// 1,000,000,000 units per second (`1_000_000_000`). - pub const NANO: Self = Self(1_000_000_000); + pub const NANO: Self = match NonZero::new(1_000_000_000) { + Some(n) => Self(n), + None => unreachable!(), + }; - /// Construct a timescale from a raw value (units per second). `0` means [`Self::UNKNOWN`]. - pub const fn new(units_per_second: u64) -> Self { - Self(units_per_second) + /// Construct a timescale from a raw value (units per second). Returns `None` if + /// `units_per_second == 0`. + pub const fn new(units_per_second: u64) -> Option { + match NonZero::new(units_per_second) { + Some(n) => Some(Self(n)), + None => None, + } } - /// The raw units-per-second value. + /// The raw units-per-second value (always non-zero). pub const fn as_u64(self) -> u64 { - self.0 + self.0.get() } +} + +impl TryFrom for Timescale { + type Error = TimeOverflow; - /// Whether this is [`Self::UNKNOWN`] (raw value `0`). - pub const fn is_unknown(self) -> bool { - self.0 == 0 + fn try_from(units_per_second: u64) -> Result { + Self::new(units_per_second).ok_or(TimeOverflow) } } -impl From for Timescale { - fn from(units_per_second: u64) -> Self { +impl From> for Timescale { + fn from(units_per_second: NonZero) -> Self { Self(units_per_second) } } impl From for u64 { + fn from(scale: Timescale) -> Self { + scale.0.get() + } +} + +impl From for NonZero { fn from(scale: Timescale) -> Self { scale.0 } @@ -68,7 +87,6 @@ impl From for u64 { impl std::fmt::Debug for Timescale { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match *self { - Self::UNKNOWN => write!(f, "Timescale::UNKNOWN"), Self::SECOND => write!(f, "Timescale::SECOND"), Self::MILLI => write!(f, "Timescale::MILLI"), Self::MICRO => write!(f, "Timescale::MICRO"), @@ -91,10 +109,10 @@ impl std::fmt::Display for Timescale { /// encoded and decoded easily; the scale is carried alongside so frames from different /// sources can be compared and converted without lossy detours through a single fixed scale. /// -/// [`Timescale::UNKNOWN`] denotes an unspecified timescale, produced by [`Timestamp::ZERO`] -/// and [`Timestamp::MAX`]. Unit conversions and arithmetic against an unknown scale return -/// [`TimeOverflow`] to avoid divide by zero. -#[derive(Clone, Default, Copy, PartialEq, Eq, Hash)] +/// The scale is a [`Timescale`] (always non-zero), so unit conversions (`as_secs`, `as_millis`, +/// etc.) are infallible. Use [`Option`] at call sites that need a "missing" sentinel +/// instead of relying on a magic value. +#[derive(Clone, Copy, PartialEq, Eq, Hash)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub struct Timestamp { value: VarInt, @@ -102,26 +120,8 @@ pub struct Timestamp { } impl Timestamp { - /// A zero timestamp with an unspecified scale. - /// - /// Useful as a sentinel min: comparisons against unspecified-scale timestamps - /// compare raw values, so `Timestamp::ZERO < t` is true for any `t` whose - /// `value > 0`. See [`Self::cmp`]. - pub const ZERO: Self = Self { - value: VarInt::ZERO, - scale: Timescale::UNKNOWN, - }; - - /// The maximum representable timestamp value, with an unspecified scale. - /// - /// Useful as a sentinel max: `t < Timestamp::MAX` is true for any `t` whose - /// `value < VarInt::MAX`. See [`Self::cmp`]. - pub const MAX: Self = Self { - value: VarInt::MAX, - scale: Timescale::UNKNOWN, - }; - /// Construct a timestamp directly from a raw value at the given scale. + /// Returns [`TimeOverflow`] if `value` exceeds `2^62 - 1`. pub const fn new(value: u64, scale: Timescale) -> Result { match VarInt::from_u64(value) { Some(value) => Ok(Self { value, scale }), @@ -130,15 +130,10 @@ impl Timestamp { } /// Convert `value` measured at `source` (units per second) to a timestamp at `target`. - /// - /// Returns [`TimeOverflow`] on overflow, or if either `source` or `target` is - /// [`Timescale::UNKNOWN`]. + /// Returns [`TimeOverflow`] on overflow. pub const fn from_scale(value: u64, source: Timescale, target: Timescale) -> Result { - if source.0 == 0 || target.0 == 0 { - return Err(TimeOverflow); - } - match (value as u128).checked_mul(target.0 as u128) { - Some(scaled) => match VarInt::from_u128(scaled / source.0 as u128) { + match (value as u128).checked_mul(target.0.get() as u128) { + Some(scaled) => match VarInt::from_u128(scaled / source.0.get() as u128) { Some(value) => Ok(Self { value, scale: target }), None => Err(TimeOverflow), }, @@ -208,63 +203,42 @@ impl Timestamp { self.scale } - /// Whether the scale is [`Timescale::UNKNOWN`]. Unit conversions and cross-scale - /// arithmetic against this timestamp return [`TimeOverflow`]. - pub const fn is_unspecified(self) -> bool { - self.scale.0 == 0 - } - /// Whether the raw value is zero. Does not consider scale. pub const fn is_zero(self) -> bool { self.value.into_inner() == 0 } /// Re-express this timestamp at a new scale. Returns [`TimeOverflow`] if the new - /// value would exceed `2^62 - 1`, the source scale is unspecified, or `new_scale` - /// is [`Timescale::UNKNOWN`]. + /// value would exceed `2^62 - 1`. pub const fn convert(self, new_scale: Timescale) -> Result { - if self.scale.0 == 0 || new_scale.0 == 0 { - return Err(TimeOverflow); - } - if self.scale.0 == new_scale.0 { + if self.scale.0.get() == new_scale.0.get() { return Ok(self); } Self::from_scale(self.value.into_inner(), self.scale, new_scale) } - /// The value re-expressed at `target` as a `u128`. Returns [`TimeOverflow`] - /// if the source scale is unspecified or `target` is [`Timescale::UNKNOWN`]. - pub const fn as_scale(self, target: Timescale) -> Result { - if self.scale.0 == 0 || target.0 == 0 { - return Err(TimeOverflow); - } - Ok(self.value.into_inner() as u128 * target.0 as u128 / self.scale.0 as u128) + /// The value re-expressed at `target` as a `u128`. + pub const fn as_scale(self, target: Timescale) -> u128 { + self.value.into_inner() as u128 * target.0.get() as u128 / self.scale.0.get() as u128 } - /// The value re-expressed in seconds. Returns [`TimeOverflow`] if the scale is - /// unspecified. - pub const fn as_secs(self) -> Result { - if self.scale.0 == 0 { - return Err(TimeOverflow); - } - Ok(self.value.into_inner() / self.scale.0) + /// The value re-expressed in seconds. + pub const fn as_secs(self) -> u64 { + self.value.into_inner() / self.scale.0.get() } - /// The value re-expressed in milliseconds. Returns [`TimeOverflow`] if the scale - /// is unspecified. - pub const fn as_millis(self) -> Result { + /// The value re-expressed in milliseconds. + pub const fn as_millis(self) -> u128 { self.as_scale(Timescale::MILLI) } - /// The value re-expressed in microseconds. Returns [`TimeOverflow`] if the scale - /// is unspecified. - pub const fn as_micros(self) -> Result { + /// The value re-expressed in microseconds. + pub const fn as_micros(self) -> u128 { self.as_scale(Timescale::MICRO) } - /// The value re-expressed in nanoseconds. Returns [`TimeOverflow`] if the scale - /// is unspecified. - pub const fn as_nanos(self) -> Result { + /// The value re-expressed in nanoseconds. + pub const fn as_nanos(self) -> u128 { self.as_scale(Timescale::NANO) } @@ -273,7 +247,7 @@ impl Timestamp { /// Panics if the scales differ. Use [`Self::convert`] first if you need to compare /// across scales. pub const fn max(self, other: Self) -> Self { - assert!(self.scale.0 == other.scale.0, "mismatched timestamp scales"); + assert!(self.scale.0.get() == other.scale.0.get(), "mismatched timestamp scales"); if self.value.into_inner() > other.value.into_inner() { self } else { @@ -281,10 +255,10 @@ impl Timestamp { } } - /// Add two timestamps. Returns [`TimeOverflow`] if the sum exceeds `2^62 - 1`, - /// if either scale is [`Timescale::UNKNOWN`], or if the scales differ. + /// Add two timestamps. Returns [`TimeOverflow`] if the sum exceeds `2^62 - 1` or + /// if the scales differ. pub const fn checked_add(self, rhs: Self) -> Result { - if self.scale.0 == 0 || rhs.scale.0 == 0 || self.scale.0 != rhs.scale.0 { + if self.scale.0.get() != rhs.scale.0.get() { return Err(TimeOverflow); } match self.value.into_inner().checked_add(rhs.value.into_inner()) { @@ -293,10 +267,10 @@ impl Timestamp { } } - /// Subtract `rhs` from `self`. Returns [`TimeOverflow`] if `rhs > self`, if either - /// scale is [`Timescale::UNKNOWN`], or if the scales differ. + /// Subtract `rhs` from `self`. Returns [`TimeOverflow`] if `rhs > self` or if the + /// scales differ. pub const fn checked_sub(self, rhs: Self) -> Result { - if self.scale.0 == 0 || rhs.scale.0 == 0 || self.scale.0 != rhs.scale.0 { + if self.scale.0.get() != rhs.scale.0.get() { return Err(TimeOverflow); } match self.value.into_inner().checked_sub(rhs.value.into_inner()) { @@ -328,33 +302,16 @@ impl TryFrom for Timestamp { } impl From for std::time::Duration { - /// Panics if the timestamp's scale is [`Timescale::UNKNOWN`]. - /// - /// In practice every concrete timestamp produced by an importer carries a known - /// scale; only the [`Timestamp::ZERO`] / [`Timestamp::MAX`] sentinels lack one, - /// and those never get converted to a `Duration`. fn from(time: Timestamp) -> Self { - let secs = time - .as_secs() - .expect("cannot convert unspecified-scale Timestamp to Duration"); - let nanos = time - .as_nanos() - .expect("cannot convert unspecified-scale Timestamp to Duration"); - std::time::Duration::new(secs, (nanos % 1_000_000_000) as u32) + let nanos = time.as_nanos(); + std::time::Duration::new(time.as_secs(), (nanos % 1_000_000_000) as u32) } } impl std::fmt::Debug for Timestamp { #[allow(clippy::manual_is_multiple_of)] // is_multiple_of is unstable in Rust 1.85 fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - if self.scale.0 == 0 { - return write!(f, "{}/?", self.value.into_inner()); - } - - let nanos = match self.as_nanos() { - Ok(n) => n, - Err(_) => return write!(f, "{}/{}", self.value.into_inner(), self.scale), - }; + let nanos = self.as_nanos(); // Choose the largest unit where we don't need decimal places. if nanos % 1_000_000_000 == 0 { @@ -376,30 +333,22 @@ impl PartialOrd for Timestamp { } impl Ord for Timestamp { - /// Compare two timestamps, normalizing across scales when both are known. + /// Compare two timestamps, normalizing across scales when they differ. /// /// - If both scales are equal, compares raw values directly. - /// - If one side is [`Timescale::UNKNOWN`] ([`Self::ZERO`] / [`Self::MAX`] sentinels), - /// compares raw values; the sentinel's `0` / `VarInt::MAX` ensures the expected - /// ordering against any concrete timestamp. - /// - If both scales are known but differ, cross-multiplies in 128-bit so e.g. - /// `1s > 2ms` orders correctly. Required by the `min_by_key` call sites in the - /// fmp4/mkv exporters, which pick the next track to emit across mixed-scale - /// per-track frames. + /// - Otherwise cross-multiplies in 128-bit so e.g. `1s > 2ms` orders correctly. /// /// When the cross-scale comparison would otherwise be `Equal` (e.g. `from_secs(1)` - /// vs `from_millis(1000)`), we break ties by `(scale, value)` so the result agrees - /// with derived `PartialEq`/`Eq`/`Hash` (which are field-wise). Without that tie - /// break, `cmp` could return `Equal` for fields that aren't equal, violating - /// Rust's `Ord`/`Eq` contract for ordered collection keys. + /// vs `from_millis(1000)`), breaks ties by `(scale, value)` so the result agrees + /// with derived `PartialEq`/`Eq`/`Hash` (which are field-wise). fn cmp(&self, other: &Self) -> std::cmp::Ordering { - if self.scale.0 == other.scale.0 || self.scale.0 == 0 || other.scale.0 == 0 { + if self.scale.0.get() == other.scale.0.get() { return self.value.cmp(&other.value); } - let lhs = self.value.into_inner() as u128 * other.scale.0 as u128; - let rhs = other.value.into_inner() as u128 * self.scale.0 as u128; + let lhs = self.value.into_inner() as u128 * other.scale.0.get() as u128; + let rhs = other.value.into_inner() as u128 * self.scale.0.get() as u128; lhs.cmp(&rhs) - .then_with(|| self.scale.0.cmp(&other.scale.0)) + .then_with(|| self.scale.0.get().cmp(&other.scale.0.get())) .then_with(|| self.value.cmp(&other.value)) } } @@ -466,28 +415,6 @@ impl From for Timestamp { } } -/// Decode a timestamp's raw value as a varint, attaching [`Timescale::UNKNOWN`]. -/// -/// Callers that need a meaningful scale should attach it after decoding (e.g. via -/// [`Timestamp::new`] with the track's [`Timescale`]). -impl Decode for Timestamp { - fn decode(r: &mut R, version: crate::Version) -> Result { - let value = VarInt::decode(r, version)?; - Ok(Self { - value, - scale: Timescale::UNKNOWN, - }) - } -} - -/// Encode a timestamp's raw value as a varint. The scale is **not** serialized. -impl Encode for Timestamp { - fn encode(&self, w: &mut W, version: crate::Version) -> Result<(), EncodeError> { - self.value.encode(w, version)?; - Ok(()) - } -} - #[cfg(test)] mod tests { use super::*; @@ -496,53 +423,41 @@ mod tests { fn test_from_secs() { let time = Timestamp::from_secs(5).unwrap(); assert_eq!(time.scale(), Timescale::SECOND); - assert_eq!(time.as_secs().unwrap(), 5); - assert_eq!(time.as_millis().unwrap(), 5000); - assert_eq!(time.as_micros().unwrap(), 5_000_000); - assert_eq!(time.as_nanos().unwrap(), 5_000_000_000); + assert_eq!(time.as_secs(), 5); + assert_eq!(time.as_millis(), 5000); + assert_eq!(time.as_micros(), 5_000_000); + assert_eq!(time.as_nanos(), 5_000_000_000); } #[test] fn test_from_millis() { let time = Timestamp::from_millis(5000).unwrap(); assert_eq!(time.scale(), Timescale::MILLI); - assert_eq!(time.as_secs().unwrap(), 5); - assert_eq!(time.as_millis().unwrap(), 5000); + assert_eq!(time.as_secs(), 5); + assert_eq!(time.as_millis(), 5000); } #[test] fn test_from_micros() { let time = Timestamp::from_micros(5_000_000).unwrap(); assert_eq!(time.scale(), Timescale::MICRO); - assert_eq!(time.as_secs().unwrap(), 5); - assert_eq!(time.as_micros().unwrap(), 5_000_000); + assert_eq!(time.as_secs(), 5); + assert_eq!(time.as_micros(), 5_000_000); } #[test] fn test_from_nanos() { let time = Timestamp::from_nanos(5_000_000_000).unwrap(); assert_eq!(time.scale(), Timescale::NANO); - assert_eq!(time.as_secs().unwrap(), 5); - assert_eq!(time.as_nanos().unwrap(), 5_000_000_000); + assert_eq!(time.as_secs(), 5); + assert_eq!(time.as_nanos(), 5_000_000_000); } #[test] - fn test_zero_unspecified() { - let time = Timestamp::ZERO; - assert!(time.is_unspecified()); - assert!(time.is_zero()); - assert!(time.as_secs().is_err()); - assert!(time.as_millis().is_err()); - assert!(time.as_micros().is_err()); - assert!(time.as_nanos().is_err()); - } - - #[test] - fn test_zero_at_scale() { - let time = Timestamp::from_millis(0).unwrap(); - assert!(!time.is_unspecified()); - assert!(time.is_zero()); - assert_eq!(time.as_millis().unwrap(), 0); + fn test_timescale_new_rejects_zero() { + assert!(Timescale::new(0).is_none()); + assert_eq!(Timescale::new(1), Some(Timescale::SECOND)); + assert_eq!(Timescale::new(1_000), Some(Timescale::MILLI)); } #[test] @@ -550,7 +465,7 @@ mod tests { let time_ms = Timestamp::from_millis(5000).unwrap(); let time_us = time_ms.convert(Timescale::MICRO).unwrap(); assert_eq!(time_us.scale(), Timescale::MICRO); - assert_eq!(time_us.as_micros().unwrap(), 5_000_000); + assert_eq!(time_us.as_micros(), 5_000_000); } #[test] @@ -558,7 +473,7 @@ mod tests { let time_ms = Timestamp::from_millis(5000).unwrap(); let time_s = time_ms.convert(Timescale::SECOND).unwrap(); assert_eq!(time_s.scale(), Timescale::SECOND); - assert_eq!(time_s.as_secs().unwrap(), 5); + assert_eq!(time_s.as_secs(), 5); } #[test] @@ -566,7 +481,7 @@ mod tests { // 1234 ms = 1.234 s, rounds down to 1 s let time_ms = Timestamp::from_millis(1234).unwrap(); let time_s = time_ms.convert(Timescale::SECOND).unwrap(); - assert_eq!(time_s.as_secs().unwrap(), 1); + assert_eq!(time_s.as_secs(), 1); } #[test] @@ -585,21 +500,12 @@ mod tests { assert_eq!(time, converted); } - #[test] - fn test_convert_unspecified_rejected() { - let zero = Timestamp::ZERO; - assert!(zero.convert(Timescale::MILLI).is_err()); - - let time = Timestamp::from_millis(5).unwrap(); - assert!(time.convert(Timescale::UNKNOWN).is_err()); - } - #[test] fn test_add_same_scale() { let a = Timestamp::from_millis(1000).unwrap(); let b = Timestamp::from_millis(2000).unwrap(); let c = a.checked_add(b).unwrap(); - assert_eq!(c.as_millis().unwrap(), 3000); + assert_eq!(c.as_millis(), 3000); assert_eq!(c.scale(), Timescale::MILLI); } @@ -617,27 +523,6 @@ mod tests { assert!(a.checked_sub(b).is_err()); } - #[test] - fn test_add_unspecified_scale_rejected() { - // Two ZERO sentinels (both UNKNOWN scale) must not combine into a meaningful - // result, otherwise wire-decoded timestamps that haven't had a scale attached - // yet would silently behave like real arithmetic operands. - assert!(Timestamp::ZERO.checked_add(Timestamp::ZERO).is_err()); - - let t = Timestamp::from_millis(100).unwrap(); - assert!(t.checked_add(Timestamp::ZERO).is_err()); - assert!(Timestamp::ZERO.checked_add(t).is_err()); - } - - #[test] - fn test_sub_unspecified_scale_rejected() { - assert!(Timestamp::ZERO.checked_sub(Timestamp::ZERO).is_err()); - - let t = Timestamp::from_millis(100).unwrap(); - assert!(t.checked_sub(Timestamp::ZERO).is_err()); - assert!(Timestamp::ZERO.checked_sub(t).is_err()); - } - #[test] fn test_max_same_scale() { let a = Timestamp::from_secs(5).unwrap(); @@ -663,37 +548,22 @@ mod tests { assert_eq!(a, a); } - #[test] - fn test_ordering_against_sentinels() { - // ZERO and MAX act as universal sentinels because their scale is 0. - let t = Timestamp::from_millis(100).unwrap(); - assert!(Timestamp::ZERO < t); - assert!(t < Timestamp::MAX); - assert!(Timestamp::ZERO < Timestamp::MAX); - } - #[test] fn test_ordering_across_known_scales() { - // Cross-scale ordering must normalize to a common scale rather than fall back - // to raw values. Without this, 1s would order as < 2ms (1 < 2) and the - // fmp4/mkv exporter's `min_by_key(|ts| *ts)` over per-track frames would silently - // pick the wrong track when tracks have different native scales. + // Cross-scale ordering normalizes to a common scale. let one_sec = Timestamp::from_secs(1).unwrap(); let two_ms = Timestamp::from_millis(2).unwrap(); assert!(one_sec > two_ms); assert!(two_ms < one_sec); - // Temporally-equivalent timestamps with different (value, scale) representations - // must NOT cmp as Equal: derived Eq compares fields, so cmp returning Equal - // here would violate the Ord/Eq contract for ordered collection keys. The - // tie-breaker resolves them by scale to keep ordering deterministic. + // Temporally-equivalent timestamps with different representations are NOT + // Equal under cmp: derived Eq compares fields, and Ord must agree. let one_sec_b = Timestamp::from_millis(1000).unwrap(); assert_ne!(one_sec.cmp(&one_sec_b), std::cmp::Ordering::Equal); assert_ne!(one_sec, one_sec_b); - // And the tie-break agrees with PartialEq: cmp(a, b) == Equal iff a == b. assert_eq!(one_sec.cmp(&one_sec), std::cmp::Ordering::Equal); - // Sorting a mixed-scale slice puts entries in correct temporal order. + // Mixed-scale sort lands in correct temporal order. let mut items = [ Timestamp::from_secs(2).unwrap(), Timestamp::from_millis(500).unwrap(), @@ -710,7 +580,7 @@ mod tests { let duration = std::time::Duration::from_secs(5); let time: Timestamp = duration.try_into().unwrap(); assert_eq!(time.scale(), Timescale::NANO); - assert_eq!(time.as_secs().unwrap(), 5); + assert_eq!(time.as_secs(), 5); let duration_back: std::time::Duration = time.into(); assert_eq!(duration_back.as_secs(), 5); @@ -729,9 +599,6 @@ mod tests { let t = Timestamp::from_micros(1000).unwrap(); assert_eq!(format!("{:?}", t), "1ms"); - - let t = Timestamp::ZERO; - assert_eq!(format!("{:?}", t), "0/?"); } #[test] @@ -739,27 +606,15 @@ mod tests { let t = Timestamp::new(5000, Timescale::MILLI).unwrap(); assert_eq!(t.value(), 5000); assert_eq!(t.scale(), Timescale::MILLI); - assert_eq!(t.as_millis().unwrap(), 5000); + assert_eq!(t.as_millis(), 5000); } #[test] fn test_from_scale_custom() { // 120 units at 60Hz = 2 seconds, expressed at 1000Hz = 2000 ms. - let t = Timestamp::from_scale(120, Timescale::new(60), Timescale::MILLI).unwrap(); + let scale_60 = Timescale::new(60).unwrap(); + let t = Timestamp::from_scale(120, scale_60, Timescale::MILLI).unwrap(); assert_eq!(t.scale(), Timescale::MILLI); - assert_eq!(t.as_millis().unwrap(), 2000); - } - - #[test] - fn test_from_scale_zero_source() { - assert!(Timestamp::from_scale(5, Timescale::UNKNOWN, Timescale::MILLI).is_err()); - } - - #[test] - fn test_from_scale_zero_target() { - // Otherwise the multiply-by-zero silently discards the value and we'd - // hand back a 0/?-scale timestamp, breaking the UNKNOWN-conversions-fail - // contract. - assert!(Timestamp::from_scale(5, Timescale::MILLI, Timescale::UNKNOWN).is_err()); + assert_eq!(t.as_millis(), 2000); } } From 8dfcd86ae9358eba85b5acfabef5d02b2e5a1382 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 28 May 2026 12:31:07 -0700 Subject: [PATCH 12/12] moq-net: Timescale::new -> Result; drop Timestamp::from_scale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from review: 1. `Timescale::new(u64)` returns `Result` instead of `Option`. Rejects `0` (would divide by zero) and now also rejects values past `2^62 - 1` so the constraint stays symmetric with Timestamp's raw value. Result is friendlier than Option here because `?` propagation works at the call sites without `.ok_or` adapters. 2. `Timestamp::from_scale(value, source, target)` is removed. `Timestamp::new(value, source)?.convert(target)` covers the same ground with the natural argument order (Timestamp first, target scale second) and one fewer API to learn. Side effects: - fmp4/mod.rs: drop the `Error::InvalidTimescale` variant; the existing `TimestampOverflow(#[from] TimeOverflow)` already covers the path. - Call sites that used `.ok_or(...)` on the old Option-returning `new` switch to `?` (or `.context(...)` for anyhow paths). - LOC fallback uses `loc.timescale.and_then(|s| Timescale::new(s).ok())` to keep the catalog-default path total. - Test renamed `test_from_scale_custom` → `test_custom_scale_convert`, added overflow/zero coverage to `test_timescale_new_rejects_*`. Co-Authored-By: Claude Opus 4.7 (1M context) --- rs/moq-mux/src/container/fmp4/export.rs | 2 +- rs/moq-mux/src/container/fmp4/import.rs | 4 +- rs/moq-mux/src/container/fmp4/mod.rs | 7 +-- rs/moq-mux/src/container/loc/mod.rs | 5 +- rs/moq-net/src/model/time.rs | 66 +++++++++++++++---------- 5 files changed, 49 insertions(+), 35 deletions(-) diff --git a/rs/moq-mux/src/container/fmp4/export.rs b/rs/moq-mux/src/container/fmp4/export.rs index d5b1716bf..376812fff 100644 --- a/rs/moq-mux/src/container/fmp4/export.rs +++ b/rs/moq-mux/src/container/fmp4/export.rs @@ -488,7 +488,7 @@ fn encode_fragment(track: &mut Fmp4Track, frames: Vec) -> anyhow::Result< anyhow::ensure!(!frames.is_empty(), "encode_fragment called with no frames"); let seq = track.sequence_number; track.sequence_number += 1; - let timescale = moq_net::Timescale::new(track.timescale).context("track timescale must be non-zero")?; + let timescale = moq_net::Timescale::new(track.timescale).context("invalid track timescale")?; Ok(crate::container::fmp4::encode_fragment( track.track_id, timescale, diff --git a/rs/moq-mux/src/container/fmp4/import.rs b/rs/moq-mux/src/container/fmp4/import.rs index e38c8d113..535af7fdd 100644 --- a/rs/moq-mux/src/container/fmp4/import.rs +++ b/rs/moq-mux/src/container/fmp4/import.rs @@ -411,8 +411,8 @@ impl Import { let tfdt = traf.tfdt.as_ref().context("missing tfdt box")?; let mut dts = tfdt.base_media_decode_time; - let timescale = moq_net::Timescale::new(trak.mdia.mdhd.timescale as u64) - .context("fmp4 mdhd.timescale must be non-zero")?; + let timescale = + moq_net::Timescale::new(trak.mdia.mdhd.timescale as u64).context("invalid fmp4 mdhd.timescale")?; let mut offset = traf.tfhd.base_data_offset.unwrap_or_default() as usize; let mut track_data_start: Option = None; diff --git a/rs/moq-mux/src/container/fmp4/mod.rs b/rs/moq-mux/src/container/fmp4/mod.rs index 462f8245b..2d2dfc48e 100644 --- a/rs/moq-mux/src/container/fmp4/mod.rs +++ b/rs/moq-mux/src/container/fmp4/mod.rs @@ -65,9 +65,6 @@ pub enum Error { #[error("can't synthesize CMAF init for {0}")] UnsupportedSynthesis(String), - - #[error("invalid mdhd.timescale (must be non-zero)")] - InvalidTimescale, } /// CMAF container: encodes/decodes a single track's moof+mdat fragments. @@ -114,7 +111,7 @@ impl Container for Wire { type Error = Error; fn write(&self, group: &mut moq_net::GroupProducer, frames: &[Frame]) -> Result<(), Self::Error> { - let timescale = moq_net::Timescale::new(self.trak.mdia.mdhd.timescale as u64).ok_or(Error::InvalidTimescale)?; + let timescale = moq_net::Timescale::new(self.trak.mdia.mdhd.timescale as u64)?; let track_id = self.trak.tkhd.track_id; encode(group, frames, timescale, track_id) } @@ -130,7 +127,7 @@ impl Container for Wire { return Poll::Ready(Ok(None)); }; - let timescale = moq_net::Timescale::new(self.trak.mdia.mdhd.timescale as u64).ok_or(Error::InvalidTimescale)?; + let timescale = moq_net::Timescale::new(self.trak.mdia.mdhd.timescale as u64)?; Poll::Ready(Ok(Some(decode(data, timescale)?))) } } diff --git a/rs/moq-mux/src/container/loc/mod.rs b/rs/moq-mux/src/container/loc/mod.rs index f1e3e8902..d2efe4336 100644 --- a/rs/moq-mux/src/container/loc/mod.rs +++ b/rs/moq-mux/src/container/loc/mod.rs @@ -51,7 +51,10 @@ impl Container for Wire { // `loc.timescale == Some(0)` is a malformed wire (caught by moq_loc::decode itself), // so any Some(_) we see here is non-zero. Falling back to the catalog default // keeps this code path infallible. - let scale = loc.timescale.and_then(Timescale::new).unwrap_or(DEFAULT_TIMESCALE); + let scale = loc + .timescale + .and_then(|s| Timescale::new(s).ok()) + .unwrap_or(DEFAULT_TIMESCALE); let timestamp = Timestamp::new(loc.timestamp, scale).map_err(hang::Error::from)?; Poll::Ready(Ok(Some(vec![Frame { diff --git a/rs/moq-net/src/model/time.rs b/rs/moq-net/src/model/time.rs index c8b37f3c1..1d47addfa 100644 --- a/rs/moq-net/src/model/time.rs +++ b/rs/moq-net/src/model/time.rs @@ -19,7 +19,8 @@ pub struct TimeOverflow; /// arithmetic on [`Timestamp`] can divide by `self.scale` without ever risking /// a divide by zero. Use the named constants ([`Self::SECOND`], [`Self::MILLI`], /// [`Self::MICRO`], [`Self::NANO`]) instead of writing raw integers at call sites; -/// for runtime values, use [`Self::new`] which returns `None` for `0`. +/// for runtime values, use [`Self::new`] which returns [`TimeOverflow`] for `0` or +/// for values past the QUIC varint range. #[derive(Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub struct Timescale(NonZero); @@ -43,12 +44,19 @@ impl Timescale { None => unreachable!(), }; - /// Construct a timescale from a raw value (units per second). Returns `None` if - /// `units_per_second == 0`. - pub const fn new(units_per_second: u64) -> Option { + /// Construct a timescale from a raw value (units per second). + /// + /// Returns [`TimeOverflow`] if `units_per_second` is `0` (would divide by zero) + /// or exceeds `2^62 - 1` (the QUIC varint range, matching [`Timestamp`] values). + pub const fn new(units_per_second: u64) -> Result { + // Reject values that wouldn't fit in a QUIC varint, keeping the constraint + // symmetric with Timestamp's raw value. + if VarInt::from_u64(units_per_second).is_none() { + return Err(TimeOverflow); + } match NonZero::new(units_per_second) { - Some(n) => Some(Self(n)), - None => None, + Some(n) => Ok(Self(n)), + None => Err(TimeOverflow), } } @@ -62,7 +70,7 @@ impl TryFrom for Timescale { type Error = TimeOverflow; fn try_from(units_per_second: u64) -> Result { - Self::new(units_per_second).ok_or(TimeOverflow) + Self::new(units_per_second) } } @@ -129,18 +137,6 @@ impl Timestamp { } } - /// Convert `value` measured at `source` (units per second) to a timestamp at `target`. - /// Returns [`TimeOverflow`] on overflow. - pub const fn from_scale(value: u64, source: Timescale, target: Timescale) -> Result { - match (value as u128).checked_mul(target.0.get() as u128) { - Some(scaled) => match VarInt::from_u128(scaled / source.0.get() as u128) { - Some(value) => Ok(Self { value, scale: target }), - None => Err(TimeOverflow), - }, - None => Err(TimeOverflow), - } - } - /// Convert a number of seconds to a timestamp at [`Timescale::SECOND`]. pub const fn from_secs(seconds: u64) -> Result { Self::new(seconds, Timescale::SECOND) @@ -214,7 +210,16 @@ impl Timestamp { if self.scale.0.get() == new_scale.0.get() { return Ok(self); } - Self::from_scale(self.value.into_inner(), self.scale, new_scale) + match (self.value.into_inner() as u128).checked_mul(new_scale.0.get() as u128) { + Some(scaled) => match VarInt::from_u128(scaled / self.scale.0.get() as u128) { + Some(value) => Ok(Self { + value, + scale: new_scale, + }), + None => Err(TimeOverflow), + }, + None => Err(TimeOverflow), + } } /// The value re-expressed at `target` as a `u128`. @@ -454,10 +459,16 @@ mod tests { } #[test] - fn test_timescale_new_rejects_zero() { - assert!(Timescale::new(0).is_none()); - assert_eq!(Timescale::new(1), Some(Timescale::SECOND)); - assert_eq!(Timescale::new(1_000), Some(Timescale::MILLI)); + fn test_timescale_new_rejects_zero_and_overflow() { + assert!(Timescale::new(0).is_err()); + assert!(Timescale::new(1).is_ok()); + assert_eq!(Timescale::new(1).unwrap(), Timescale::SECOND); + assert_eq!(Timescale::new(1_000).unwrap(), Timescale::MILLI); + + // Above the QUIC varint range. + assert!(Timescale::new(1u64 << 62).is_err()); + // Right at the top of the varint range is still valid. + assert!(Timescale::new((1u64 << 62) - 1).is_ok()); } #[test] @@ -610,10 +621,13 @@ mod tests { } #[test] - fn test_from_scale_custom() { + fn test_custom_scale_convert() { // 120 units at 60Hz = 2 seconds, expressed at 1000Hz = 2000 ms. let scale_60 = Timescale::new(60).unwrap(); - let t = Timestamp::from_scale(120, scale_60, Timescale::MILLI).unwrap(); + let t = Timestamp::new(120, scale_60) + .unwrap() + .convert(Timescale::MILLI) + .unwrap(); assert_eq!(t.scale(), Timescale::MILLI); assert_eq!(t.as_millis(), 2000); }