moq-net: make Group/Frame producers track-only constructible#1636
Merged
Conversation
`GroupProducer::new_with_timescale` leaked into the public API. It only existed because `GroupProducer`/`FrameProducer` were publicly constructible and a group needs its parent track's timescale to validate frame timestamps. Close every public construction door instead of just that one method: - Collapse `GroupProducer::new` + `new_with_timescale` into a single `pub(crate) fn new(info, timescale)`. - Make `FrameProducer::new` and `Frame::produce` `pub(crate)`; gate the test-only `Group::produce` behind `#[cfg(test)]`. - Remove the unused `From<Group> for GroupProducer` and `From<Frame> for FrameProducer` impls. Groups now only come from a `TrackProducer` and frames only from a `GroupProducer::create_frame`, so the timescale is always the track's negotiated value. The `append_frame` timestamp/timescale check stays a hard error (`Error::TimestampMismatch`): with construction gated, a mismatch is a genuine producer bug rather than a recoverable condition. This only restricts construction, not close/abort. Group and Frame handles stay independently closeable and shareable. No external churn: every caller already builds via `create_group` / `create_frame`, including the lite subscriber receive path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
6dcce9e to
15d4c18
Compare
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
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
GroupProducer::new_with_timescaleleaked into the public API. It only existed becauseGroupProducer/FrameProducerwere publicly constructible and a group needs its parent track's timescale to validate frame timestamps. Rather than un-leak that one method, this closes every public construction door so groups/frames can only be built through a track.GroupProducer::new+new_with_timescaleinto a singlepub(crate) fn new(info, timescale).FrameProducer::newandFrame::producepub(crate); gate the test-onlyGroup::producebehind#[cfg(test)].From<Group> for GroupProducerandFrom<Frame> for FrameProducerimpls.TrackProducercall sites.Groups now only come from a
TrackProducerand frames only fromGroupProducer::create_frame, so the timescale is always the track's negotiated value. Theappend_frametimestamp/timescale check stays a hard error (Error::TimestampMismatch): with construction gated, a scale mismatch (or a missing/extra timestamp) is a genuine producer bug, not a recoverable condition.Why this shape
Timestamp(nou64inFrame). The self-describing scale is load-bearing:Timestamp::convert(), the zigzag delta encode in the lite publisher'sserve_frame, and hang'sconvert(TIMESCALE)all depend on it. A bareu64wouldn't remove the wrong-timescale footgun, it would make it silent and uncatchable.publisher.rs. Once construction is track-gated the group always knows the negotiated timescale, so the per-byte hot path doesn't need to second-guess it.serve_frame'sframe.timestamp.expect(...)still rests on the model-layer invariant.Compatibility
This is a breaking change to
rs/moq-net's public Rust API (hence targetingdev), but there is zero external churn: every caller in the workspace already builds viacreate_group/create_frame, including the lite subscriber receive path. No JS/wire/catalog changes, so no Cross-Package Sync rows apply.Test plan
cargo test -p moq-net --lib(365 passed)cargo build --workspacecargo clippy -p moq-net --all-targets(clean)cargo fmt -p moq-net🤖 Generated with Claude Code
(Written by Claude)