moq-net: runtime Timescale/Timestamp; container::Frame keeps source scale#1473
Open
kixelated wants to merge 12 commits into
Open
moq-net: runtime Timescale/Timestamp; container::Frame keeps source scale#1473kixelated wants to merge 12 commits into
kixelated wants to merge 12 commits into
Conversation
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<Timestamp>`. 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) <noreply@anthropic.com>
The previous commit changed the catalog jitter field from
`Option<moq_net::Time>` (transparently a `VarInt` → JSON integer) to
`Option<moq_net::Timestamp>`, 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<Duration>` 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) <noreply@anthropic.com>
Contributor
|
Caution Review failedFailed to post review comments Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
<review_stack_artifact> </review_stack_artifact> 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-net/src/model/time.rs`:
- Around line 285-305: checked_add and checked_sub currently only reject
mismatched scales, allowing two Timescale::UNKNOWN values to combine; update
both functions (checked_add, checked_sub) to also return Err(TimeOverflow) when
either operand's scale is unspecified by checking for the Timescale::UNKNOWN
variant on self.scale (or rhs.scale) before doing the numeric
checked_add/checked_sub; keep the existing mismatched-scale check and overflow
handling but add the early UNKNOWN-scale rejection to prevent arithmetic on
unspecified scales.
- Around line 370-384: The current Ord::cmp for Timestamp falls back to
raw-value ordering when scales differ, causing incorrect ordering for known
mixed scales; update Timestamp::cmp to detect when both scales are known
(neither is Timescale::UNKNOWN) and compare by first normalizing both values to
a common timescale rather than directly comparing raw value. Locate the impl Ord
for Timestamp and its cmp method and implement a safe rescaling: compute a
common target scale (e.g., the finer/smaller unit or an LCM of the two scales),
convert self.value and other.value into that common scale using checked integer
arithmetic or by promoting to a wider integer type to avoid overflow, then
compare the rescaled values; retain the debug_assert for scale compatibility
with UNKNOWN, and only use raw-value fallback when one side is
Timescale::UNKNOWN or when rescaling is provably impossible (handle that path
explicitly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4be9501-c815-499f-9c0b-ddd4fe83f5ee
📒 Files selected for processing (21)
rs/hang/src/catalog/audio/mod.rsrs/hang/src/catalog/video/mod.rsrs/hang/src/container/frame.rsrs/libmoq/src/consume.rsrs/moq-ffi/src/consumer.rsrs/moq-gst/src/source/imp.rsrs/moq-mux/src/catalog/hang/producer.rsrs/moq-mux/src/catalog/msf/consumer.rsrs/moq-mux/src/container/consumer.rsrs/moq-mux/src/container/fmp4/export.rsrs/moq-mux/src/container/fmp4/import.rsrs/moq-mux/src/container/fmp4/mod.rsrs/moq-mux/src/container/jitter.rsrs/moq-mux/src/container/loc/mod.rsrs/moq-mux/src/container/mkv/export.rsrs/moq-mux/src/container/mkv/import.rsrs/moq-mux/src/container/mod.rsrs/moq-mux/src/container/producer.rsrs/moq-net/src/coding/varint.rsrs/moq-net/src/lib.rsrs/moq-net/src/model/time.rs
`serde_with` already provides `DurationMilliSeconds<u64>` which does the same Option<Duration> ↔ 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…-6ae3dd # Conflicts: # rs/moq-mux/src/catalog/hang/producer.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-mux/src/catalog/msf/consumer.rs`:
- Around line 223-226: The jitter mapping currently converts filtered finite
non-negative f64 values with Duration::from_millis(v as u64), which saturates
huge values instead of rejecting them; update the jitter mapping in the consumer
code (the iterator producing jitter: track.jitter.filter(...).map(...)) to use
Duration::try_from_secs_f64(v / 1000.0) and drop entries where the conversion
fails (i.e., map -> FilterMap or filter_map with try_from_secs_f64 returning
Err), and make the same change for both the audio and video jitter mappings so
oversized/invalid MSF jitter values are rejected rather than clamped to
u64::MAX.
In `@rs/moq-net/src/model/time.rs`:
- Around line 371-392: The current Ord::cmp for Timestamp normalizes scales and
can return Ordering::Equal for different (value, scale) pairs, violating the
Ord/Eq contract; update fn cmp(&self, other: &Self) to, after the cross-multiply
comparison, if lhs.cmp(&rhs) == Ordering::Equal, fall back to a deterministic
tie-breaker that mirrors the derived field-wise equality: compare (self.scale.0,
self.value.into_inner()) against (other.scale.0, other.value.into_inner()) and
return that Ordering so cmp returns Equal iff both scale and value fields are
equal (ensuring consistency with PartialEq/Eq/Hash and methods like
from_secs/from_millis).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f82456e-ce7b-4df5-b53b-42d6976bff36
📒 Files selected for processing (8)
rs/hang/src/catalog/audio/mod.rsrs/hang/src/catalog/root.rsrs/hang/src/catalog/video/mod.rsrs/moq-mux/src/catalog/hang/producer.rsrs/moq-mux/src/catalog/msf/consumer.rsrs/moq-mux/src/container/fmp4/import.rsrs/moq-mux/src/container/jitter.rsrs/moq-net/src/model/time.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
rs/moq-mux/src/catalog/msf/consumer.rs (2)
223-226:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace saturating cast with fallible conversion to reject invalid jitter values.
The
as u64cast saturates oversized finitef64jitter tou64::MAXmilliseconds (≈292 million years) instead of treating the catalog value as invalid. UseDuration::try_from_secs_f64(v / 1000.0)with.and_then()to reject unrepresentable values.🔧 Proposed fix
jitter: track .jitter .filter(|v| v.is_finite() && *v >= 0.0) - .map(|v| std::time::Duration::from_millis(v as u64)), + .and_then(|v| std::time::Duration::try_from_secs_f64(v / 1000.0).ok()), }))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-mux/src/catalog/msf/consumer.rs` around lines 223 - 226, The current mapping of jitter uses a saturating cast (v as u64) which turns huge finite f64 values into u64::MAX; change the chain on track.jitter (the filter/map for jitter) to perform a fallible conversion instead: after the existing filter(|v| v.is_finite() && *v >= 0.0) replace map(|v| std::time::Duration::from_millis(v as u64)) with an and_then (or map followed by .ok()) that calls std::time::Duration::try_from_secs_f64(v / 1000.0) and converts the Result to an Option (e.g. .ok()) so unrepresentable jitter values are rejected rather than saturated.
270-273:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace saturating cast with fallible conversion to reject invalid jitter values.
Same issue as the video jitter mapping:
as u64saturates instead of rejecting oversized values.🔧 Proposed fix
jitter: track .jitter .filter(|v| v.is_finite() && *v >= 0.0) - .map(|v| std::time::Duration::from_millis(v as u64)), + .and_then(|v| std::time::Duration::try_from_secs_f64(v / 1000.0).ok()), }))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-mux/src/catalog/msf/consumer.rs` around lines 270 - 273, The jitter mapping currently uses a saturating cast (`v as u64`) which silently clamps oversized values; update the closure in the `jitter: track.jitter.filter(...).map(...)` pipeline to perform a fallible conversion instead: after checking `v.is_finite()` and `*v >= 0.0`, also require `*v <= u64::MAX as f64` (or use an explicit check against `f64::from(u64::MAX)`), then convert with a safe cast (e.g., `let ms = *v as u64` only after the bound check) and call `std::time::Duration::from_millis(ms)`; if the value exceeds the bound, drop it (keep it out of the iterator) so invalid/oversized jitters are rejected rather than saturated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@rs/moq-mux/src/catalog/msf/consumer.rs`:
- Around line 223-226: The current mapping of jitter uses a saturating cast (v
as u64) which turns huge finite f64 values into u64::MAX; change the chain on
track.jitter (the filter/map for jitter) to perform a fallible conversion
instead: after the existing filter(|v| v.is_finite() && *v >= 0.0) replace
map(|v| std::time::Duration::from_millis(v as u64)) with an and_then (or map
followed by .ok()) that calls std::time::Duration::try_from_secs_f64(v / 1000.0)
and converts the Result to an Option (e.g. .ok()) so unrepresentable jitter
values are rejected rather than saturated.
- Around line 270-273: The jitter mapping currently uses a saturating cast (`v
as u64`) which silently clamps oversized values; update the closure in the
`jitter: track.jitter.filter(...).map(...)` pipeline to perform a fallible
conversion instead: after checking `v.is_finite()` and `*v >= 0.0`, also require
`*v <= u64::MAX as f64` (or use an explicit check against
`f64::from(u64::MAX)`), then convert with a safe cast (e.g., `let ms = *v as
u64` only after the bound check) and call
`std::time::Duration::from_millis(ms)`; if the value exceeds the bound, drop it
(keep it out of the iterator) so invalid/oversized jitters are rejected rather
than saturated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82e325c2-c5f2-4acc-a9b4-5b93d1d1377c
📒 Files selected for processing (4)
rs/moq-mux/src/catalog/hang/producer.rsrs/moq-mux/src/catalog/msf/consumer.rsrs/moq-mux/src/container/fmp4/export.rsrs/moq-mux/src/container/mkv/export.rs
Mirror the same pattern hang uses: a `std::time::Duration` field with a `serde_with` adapter, instead of a bare `Option<f64>`. The wire format stays a fractional JSON number of ms via `DurationMilliSecondsWithFrac<f64>` (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<Duration>`, so the bridge is just `dst.jitter = src.jitter`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jitter isn't in the MSF/CMSF drafts at all, so there's no wire-format to preserve. Use `DurationMilliSeconds<u64>` (integer ms) like the hang catalog instead of `DurationMilliSecondsWithFrac<f64>`. 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Two follow-ups from review: **From<Timestamp> 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) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rs/moq-net/src/model/time.rs (1)
135-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject
Timescale::UNKNOWNas the target infrom_scale.
from_scale(_, _, Timescale::UNKNOWN)currently returns0/?for any input because the multiply uses0and the division still succeeds. That silently discards the original timestamp and contradicts the “UNKNOWN-scale conversions fail” contract.Suggested fix
pub const fn from_scale(value: u64, source: Timescale, target: Timescale) -> Result<Self, TimeOverflow> { - if source.0 == 0 { + 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) { Some(value) => Ok(Self { value, scale: target }), None => Err(TimeOverflow), }, None => Err(TimeOverflow), } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-net/src/model/time.rs` around lines 135 - 145, from_scale currently lets Timescale::UNKNOWN through as the target because its underlying .0==0 causes the multiply to produce 0 and silently drop the timestamp; update the from_scale function to explicitly reject Timescale::UNKNOWN by checking target.0 == 0 (similar to the existing source.0 == 0 check) and returning Err(TimeOverflow) when true, so conversions to an unknown scale fail; modify the function around the top checks (in from_scale) to return Err(TimeOverflow) if target.0 == 0 before doing the checked_mul/VarInt conversion.rs/moq-mux/src/container/producer.rs (1)
93-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse buffered min/max PTS for the latency span.
Frame.timestampcan move backward in decode order for B-frames, solast.saturating_sub(first)can shrink and miss the configured latency threshold even though the buffered window already exceeds it. This check should use the buffered min/max timestamp instead of just the first/last entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-mux/src/container/producer.rs` around lines 93 - 102, The latency check currently compares the first and last buffered frame timestamps which can be wrong for B-frames; change it to compute the buffered minimum and maximum timestamp across self.buffer (e.g. iterate self.buffer.iter().map(|f| f.timestamp.as_nanos()) and collect only Ok values), ensure you have at least two valid timestamps, compute min and max from those values, and then compare max.saturating_sub(min) >= self.latency.as_nanos(); update the branch that references self.buffer.first()/last() to use these min/max values and conservatively skip the check if any timestamps are Err or fewer than two valid samples.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-loc/src/lib.rs`:
- Around line 71-95: Error conversions should use thiserror's #[from] for the
simple BoundsExceeded => OutOfRange mapping: modify the Error enum so the
OutOfRange variant carries a BoundsExceeded (or add a dedicated
OutOfRangeFromBounds variant) annotated with #[from] so thiserror derives the
From<BoundsExceeded> impl automatically, then remove the manual impl
From<BoundsExceeded> for Error; keep the current manual impls for DecodeError
and EncodeError (DecodeError::Short => ShortBuffer, other =>
MalformedProperties; EncodeError::Short => ShortBuffer, other => OutOfRange)
because those intentionally collapse cases and should remain as-is.
In `@rs/moq-mux/src/container/mod.rs`:
- Around line 41-45: The comment about LOC timescale is outdated:
loc::Wire::poll_read() now preserves a per-frame LOC timescale, so
Frame.timestamp may not always be in microseconds. Update the doc comment in
container/mod.rs to say that LOC frames default to microseconds only when the
per-frame timescale is absent (or when re-encoding to the LOC wire convention),
and mention that loc::Wire::poll_read() preserves/propagates the explicit
per-frame timescale so exporters can re-emit without forcing micros.
---
Outside diff comments:
In `@rs/moq-mux/src/container/producer.rs`:
- Around line 93-102: The latency check currently compares the first and last
buffered frame timestamps which can be wrong for B-frames; change it to compute
the buffered minimum and maximum timestamp across self.buffer (e.g. iterate
self.buffer.iter().map(|f| f.timestamp.as_nanos()) and collect only Ok values),
ensure you have at least two valid timestamps, compute min and max from those
values, and then compare max.saturating_sub(min) >= self.latency.as_nanos();
update the branch that references self.buffer.first()/last() to use these
min/max values and conservatively skip the check if any timestamps are Err or
fewer than two valid samples.
In `@rs/moq-net/src/model/time.rs`:
- Around line 135-145: from_scale currently lets Timescale::UNKNOWN through as
the target because its underlying .0==0 causes the multiply to produce 0 and
silently drop the timestamp; update the from_scale function to explicitly reject
Timescale::UNKNOWN by checking target.0 == 0 (similar to the existing source.0
== 0 check) and returning Err(TimeOverflow) when true, so conversions to an
unknown scale fail; modify the function around the top checks (in from_scale) to
return Err(TimeOverflow) if target.0 == 0 before doing the checked_mul/VarInt
conversion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fbc3154-1f8b-475d-9bc3-27eb11165b73
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
rs/hang/examples/video.rsrs/moq-loc/Cargo.tomlrs/moq-loc/src/lib.rsrs/moq-msf/src/lib.rsrs/moq-mux/src/catalog/hang/producer.rsrs/moq-mux/src/catalog/msf/consumer.rsrs/moq-mux/src/codec/aac/import.rsrs/moq-mux/src/codec/av1/import.rsrs/moq-mux/src/codec/h264/import.rsrs/moq-mux/src/codec/h265/import.rsrs/moq-mux/src/codec/opus/import.rsrs/moq-mux/src/container/consumer.rsrs/moq-mux/src/container/fmp4/export.rsrs/moq-mux/src/container/fmp4/export_test.rsrs/moq-mux/src/container/fmp4/import.rsrs/moq-mux/src/container/fmp4/mod.rsrs/moq-mux/src/container/jitter.rsrs/moq-mux/src/container/loc/mod.rsrs/moq-mux/src/container/mkv/export_test.rsrs/moq-mux/src/container/mkv/import.rsrs/moq-mux/src/container/mod.rsrs/moq-mux/src/container/producer.rsrs/moq-mux/src/import.rsrs/moq-net/src/model/time.rs
✅ Files skipped from review due to trivial changes (1)
- rs/moq-mux/src/container/mkv/export_test.rs
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) <noreply@anthropic.com>
…-6ae3dd # Conflicts: # rs/moq-mux/src/catalog/hang/producer.rs # rs/moq-mux/src/catalog/msf/consumer.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ports the
Timestamp/Timescaletypes from #1439 (without the wire-format, SUBSCRIBE_OK, or async-subscribe changes) somoq_mux::container::Framecan carry its source format's native scale instead of forcing a lossy detour through microseconds.What changed
New types in
moq-net::model::time:Timescalenewtype aroundu64with named constantsUNKNOWN/SECOND/MILLI/MICRO/NANO.Timestamp { value, scale }carries scale at runtime. Arithmetic panics on scale mismatch;as_*/convertreturnTimeOverflowwhen the scale is unspecified.Time = Timescale<1000>alias is gone; jitter fields on the hang catalog move toOption<Timestamp>.container::Frame.timestamppreserves the importer's scale:mdhd.timescaleinstead of converting to micros; export rescales once atencode_fragmentrather than the legacyas_micros() * scale / 1_000_000round-trip.jitter::MinFrameDurationnormalizes its result to MILLI for the catalog field.Drive-by:
moq-net::codingis nowpubandVarInt::{encode_quic, decode_quic}are public so hang's wire format can encode without reachinglite::Version.as_*/Duration::try_fromshapes.Where lossy conversion still happens (intentional)
hang::container::Frame::encodeloc::Wire::writemkv::export.feed_frameTimestampScaledefaultfmp4::encode_fragmentmdhd.timescalejitter::MinFrameDuration::observeHeads-up for reviewer
The hang catalog
jitterfield changes its JSON shape: was a bare integer via the transparentTimescale<1000>(VarInt)newtype, now a{value, scale}object via the default derive onTimestamp. The existing round-trip test only coversjitter: Noneso it didn't catch this. A custom serde impl would be a focused follow-up if wire compatibility matters.Test plan
cargo test --workspace --exclude moq-gst— all tests pass (141 in moq-mux, including fmp4/mkv round-trips)just check— clippy, fmt, doc, shear, sort all cleangstreamer-1.0pkg-config. Edits there are mechanical (one fallibleDuration::try_from); CI will verify🤖 Generated with Claude Code