Pr/moq rust caller grouping - #2485
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @AaronKohlmeier, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
WalkthroughAdds global TLS verification and reconnect backoff configuration for future sessions, passing those settings into client connection setup. Extends media importers and producers with keyframe grouping controls and explicit synchronized group boundaries. Adds exported C APIs for configuring session behavior and controlling media grouping, with separate handling for track and container media. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches⚔️ Resolve merge conflicts
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rs/moq-mux/src/container/fmp4/import.rs (1)
707-714: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep grouping comments concise and current.
Replace historical remediation text with the current invariant.
rs/moq-mux/src/container/fmp4/import.rs#L707-L714: state the three conditions that open a group without namingmoq_publish_media_groupor prior stream-storm behavior.rs/moq-mux/src/container/producer.rs#L343-L346: describe the verified disabled-grouping behavior without referencing the prior bug.As per coding guidelines, implementation comments must be brief and describe current code state rather than historical migration context.
🤖 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/fmp4/import.rs` around lines 707 - 714, Shorten the grouping comments at rs/moq-mux/src/container/fmp4/import.rs lines 707-714 to state only that a group opens at an explicit caller boundary, a video keyframe, or when no group is open; remove references to moq_publish_media_group and prior stream-storm behavior. At rs/moq-mux/src/container/producer.rs lines 343-346, describe the verified disabled-grouping behavior without mentioning the historical bug.Source: Coding guidelines
🤖 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/container/producer.rs`:
- Around line 46-51: Update the public documentation for the keyframe grouping
option in the producer API to remove the deprecated finish_group reference and
describe explicit grouping using cut and seek only. Apply the same change to the
corresponding documentation block noted in the review, preserving the existing
behavior description.
- Around line 102-118: The grouping APIs currently expose positional booleans;
replace them with a shared named policy enum, such as Grouping::Keyframe and
Grouping::Explicit, so the meaning is explicit. Update
rs/moq-mux/src/container/producer.rs lines 102-118 to accept and store the enum,
rs/moq-mux/src/codec/aac/import.rs lines 53-70 to accept and forward it, and
rs/moq-mux/src/import/track.rs lines 448-454 to expose the same policy through
the track facade; preserve the existing keyframe-driven versus caller-driven
behavior.
---
Nitpick comments:
In `@rs/moq-mux/src/container/fmp4/import.rs`:
- Around line 707-714: Shorten the grouping comments at
rs/moq-mux/src/container/fmp4/import.rs lines 707-714 to state only that a group
opens at an explicit caller boundary, a video keyframe, or when no group is
open; remove references to moq_publish_media_group and prior stream-storm
behavior. At rs/moq-mux/src/container/producer.rs lines 343-346, describe the
verified disabled-grouping behavior without mentioning the historical bug.
🪄 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 Plus
Run ID: bbbd9b09-780b-4d34-b530-3aa23cbb12f2
📒 Files selected for processing (8)
rs/libmoq/src/api.rsrs/libmoq/src/publish.rsrs/libmoq/src/session.rsrs/libmoq/src/state.rsrs/moq-mux/src/codec/aac/import.rsrs/moq-mux/src/container/fmp4/import.rsrs/moq-mux/src/container/producer.rsrs/moq-mux/src/import/track.rs
| /// Whether a keyframe closes the current group and starts a new one. | ||
| /// | ||
| /// Default `true`: every keyframe rolls the group (video GOP semantics). Set `false` for | ||
| /// tracks where every frame is independently decodable (e.g. audio) so a keyframe does *not* | ||
| /// force a new group; grouping is then driven explicitly by [`finish_group`](Self::finish_group) | ||
| /// / [`seek`](Self::seek) (i.e. the caller's segment boundaries) rather than per frame. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove deprecated API references from public docs.
finish_group is deprecated and hidden, but these public docs still promote it. Document cut and seek only.
As per coding guidelines, deprecated APIs must not appear in public API documentation.
Also applies to: 102-108
🤖 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 46 - 51, Update the public
documentation for the keyframe grouping option in the producer API to remove the
deprecated finish_group reference and describe explicit grouping using cut and
seek only. Apply the same change to the corresponding documentation block noted
in the review, preserving the existing behavior description.
Source: Coding guidelines
| /// Control whether a keyframe closes the current group and starts a new one. | ||
| /// | ||
| /// Default `true` (video GOP semantics). Pass `false` for tracks where every frame is a | ||
| /// keyframe (e.g. audio): the frames stay flagged as keyframes, but grouping is driven by | ||
| /// [`finish_group`](Self::finish_group) / [`seek`](Self::seek) instead of rolling a new group | ||
| /// per frame, so the caller's segment boundaries define the groups. Without an explicit | ||
| /// boundary the frames accumulate into the current group. | ||
| pub fn with_keyframe_grouping(mut self, enabled: bool) -> Self { | ||
| self.keyframe_starts_group = enabled; | ||
| self | ||
| } | ||
|
|
||
| /// Set whether a keyframe rolls a new group. See [`with_keyframe_grouping`](Self::with_keyframe_grouping). | ||
| /// Post-construction form, for importers that build the producer before knowing the policy. | ||
| pub fn set_keyframe_grouping(&mut self, enabled: bool) { | ||
| self.keyframe_starts_group = enabled; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use a named grouping policy instead of positional booleans.
A public bool obscures whether true means automatic or caller-driven grouping. Expose a named enum such as Grouping::Keyframe and Grouping::Explicit, then use it consistently across these APIs.
rs/moq-mux/src/container/producer.rs#L102-L118: replace publicenabled: boolgrouping methods with the named policy.rs/moq-mux/src/codec/aac/import.rs#L53-L70: accept and forward the same named policy.rs/moq-mux/src/import/track.rs#L448-L454: expose the named policy at the track facade.
As per coding guidelines, public APIs should use types that make misuse unrepresentable and avoid primitive positional options.
📍 Affects 3 files
rs/moq-mux/src/container/producer.rs#L102-L118(this comment)rs/moq-mux/src/codec/aac/import.rs#L53-L70rs/moq-mux/src/import/track.rs#L448-L454
🤖 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 102 - 118, The grouping
APIs currently expose positional booleans; replace them with a shared named
policy enum, such as Grouping::Keyframe and Grouping::Explicit, so the meaning
is explicit. Update rs/moq-mux/src/container/producer.rs lines 102-118 to accept
and store the enum, rs/moq-mux/src/codec/aac/import.rs lines 53-70 to accept and
forward it, and rs/moq-mux/src/import/track.rs lines 448-454 to expose the same
policy through the track facade; preserve the existing keyframe-driven versus
caller-driven behavior.
Source: Coding guidelines
| keyframe: true, | ||
| duration: None, | ||
| })?; | ||
| self.track.cut(None)?; | ||
| self.rendition.record_frame(timestamp, bytes); | ||
| if !self.manual_groups { |
There was a problem hiding this comment.
hmm, I'm not sure about this shape but it's close.
We could require the caller to cut manually. It's kind of lame that we automatically cut every frame into its own stream. In fact, fMP4 imports should not be automatically cutting like this; they should preserve fragment boundaries.
Or we could also have a max_group_duration or something that cuts automatically after some duration, with None being manual_groups.
There was a problem hiding this comment.
Using this in an encoder i want full control of the content and groups. I plan to always us manual control. But I dont want to break anyone else using this so when someone isn't doing manual control my intention was to leave the functionality as is to prevent breaking the previous contract for anyone not using manual control.
But also I'm not just doing fmp4 control, i'm playing with hang too. And i have the same issue with hang. As production it makes sense to make it easier for players to make audio and video align by default when they join. That is you cant join an audio stream half way through the video streams and be misaligned by half a video group. Instead, following the patterns of hls as an example. Audio group edges should follow video group edges/durations.
There was a problem hiding this comment.
First off a disclaimer, I'm not entirely sure what to do about audio. It feels wrong.
But also I'm not just doing fmp4 control, i'm playing with hang too. And i have the same issue with hang. As production it makes sense to make it easier for players to make audio and video align by default when they join. That is you cant join an audio stream half way through the video streams and be misaligned by half a video group. Instead, following the patterns of hls as an example. Audio group edges should follow video group edges/durations.
The problem is that aligning audio groups with video groups increases latency, as it causes HoLB between audio frames. It obviously makes sense for HLS because it's required, and in makes sense for VOD when we want guaranteed A/V at specific timestamps.
But even in your example of joining a live stream, we don't actually want to start playback at the latest keyframe. Real-time playback will download the keyframe and decode/drop as fast as possible until it caches up to the live playhead. We could do the same for audio but it's a waste of bandwidth, and the current behavior of a group per frame is closer to optimal. Not that it really matters though because the audio bitrate is so low.
One option is to have a IETF sub-group per audio frame, and a group per segment instead. But that causes the same late join behavior as above, as subscriptions start at the latest group, but we really want audio to start at the latest sub-group (whatever that means).
I'm thinking maybe layering the concept of Segment on top of the object model instead. Like the timeline track would produce an update per segment, not per group (super noisy anyway). ex. timeline segment 5 (timestamp 25) for video is group 5, while timeline segment 5 for audio is group 123-156. Audio is still lossy for real-time, but can be bundled into a nice aligned package for HLS.
And the HLS converter would use the timeline only for generating playlist.m3u8 files. I don't want it downloading media, only the segment generation should do that.
There was a problem hiding this comment.
Using this in an encoder i want full control of the content and groups. I plan to always us manual control. But I dont want to break anyone else using this so when someone isn't doing manual control my intention was to leave the functionality as is to prevent breaking the previous contract for anyone not using manual control.
And yeah I'm okay with breaking the contract a little bit. Not everybody wants a group per audio frame, so it seems weird to force that behavior. I'm okay with a breaking change because manual control seems like a better API, but you're right we should rename the function or something so it fails to compile.
Audio codecs flag every frame a keyframe, so the codec importers cut a group (a QUIC stream) per packet inside decode(). That storms streams and desyncs co-multiplexed video, and bakes the grouping policy into the primitive rather than leaving it to the caller. Make the audio importers (aac, opus, flac, mp3) dumb: decode() writes a frame and never cuts. container::Producer gains without_keyframe_grouping() so a keyframe only anchors a group for audio instead of rolling one per frame; the caller bounds groups via cut()/seek(). The per-packet bitrate window moves fully into decode(), so bitrate/jitter detection is unaffected and can't be clobbered by a per-frame cut. The grouping policy moves to the callers: - import::Track cuts after each audio frame by default, so libmoq / FFI / gst / cli stay behavior-identical, with set_manual_grouping() to opt into caller-driven boundaries. - ts (AAC + Opus) and moq-rtc cut explicitly after each decode. - flv, mkv, moq-audio already cut per frame; fMP4 keeps its own fragment-boundary grouping. Default wire output is unchanged everywhere; caller-driven audio grouping is now possible. Reimplements the idea from #2485 on current main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Audio codecs flag every frame a keyframe, so the codec importers cut a group (a QUIC stream) per packet inside decode(). That storms streams and desyncs co-multiplexed video, and bakes the grouping policy into the primitive rather than leaving it to the caller. Make the audio importers (aac, opus, flac, mp3) dumb: decode() writes a frame and never cuts. container::Producer gains without_keyframe_grouping() so a keyframe only anchors a group for audio instead of rolling one per frame; the caller bounds groups via cut()/seek(). The per-packet bitrate window moves fully into decode(), so bitrate/jitter detection is unaffected and can't be clobbered by a per-frame cut. The grouping policy moves to the callers: - import::Track cuts after each audio frame by default, so libmoq / FFI / gst / cli stay behavior-identical, with set_manual_grouping() to opt into caller-driven boundaries. - ts (AAC + Opus) and moq-rtc cut explicitly after each decode. - flv, mkv, moq-audio already cut per frame; fMP4 keeps its own fragment-boundary grouping. Default wire output is unchanged everywhere; caller-driven audio grouping is now possible. Reimplements the idea from #2485 on current main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Audio codecs flag every frame a keyframe, so the codec importers cut a group (a QUIC stream) per packet inside decode(). That storms streams and desyncs co-multiplexed video, and bakes the grouping policy into the primitive rather than leaving it to the caller. Make the audio importers (aac, opus, flac, mp3) dumb: decode() writes a frame and never cuts. container::Producer gains without_keyframe_grouping() so a keyframe only anchors a group for audio instead of rolling one per frame; the caller bounds groups via cut()/seek(). The per-packet bitrate window moves fully into decode(), so bitrate/jitter detection is unaffected and can't be clobbered by a per-frame cut. The grouping policy moves to the callers: - import::Track cuts after each audio frame by default, so libmoq / FFI / gst / cli stay behavior-identical, with set_manual_grouping() to opt into caller-driven boundaries. - ts (AAC + Opus) and moq-rtc cut explicitly after each decode. - flv, mkv, moq-audio already cut per frame; fMP4 keeps its own fragment-boundary grouping. Default wire output is unchanged everywhere; caller-driven audio grouping is now possible. Reimplements the idea from #2485 on current main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Audio codecs flag every frame a keyframe, so the codec importers cut a group (a QUIC stream) per packet inside decode(). That storms streams and desyncs co-multiplexed video, and bakes the grouping policy into the primitive rather than leaving it to the caller. Make the audio importers (aac, opus, flac, mp3) dumb: decode() writes a frame and never cuts. container::Producer gains without_keyframe_grouping() so a keyframe only anchors a group for audio instead of rolling one per frame; the caller bounds groups via cut()/seek(). The per-packet bitrate window moves fully into decode(), so bitrate/jitter detection is unaffected and can't be clobbered by a per-frame cut. The grouping policy moves to the callers: - import::Track cuts after each audio frame by default, so libmoq / FFI / gst / cli stay behavior-identical, with set_manual_grouping() to opt into caller-driven boundaries. - ts (AAC + Opus) and moq-rtc cut explicitly after each decode. - flv, mkv, moq-audio already cut per frame; fMP4 keeps its own fragment-boundary grouping. Default wire output is unchanged everywhere; caller-driven audio grouping is now possible. Reimplements the idea from #2485 on current main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Audio codecs flag every frame a keyframe, so the codec importers cut a group (a QUIC stream) per packet inside decode(). That storms streams and desyncs co-multiplexed video, and bakes the grouping policy into the primitive rather than leaving it to the caller. Make the audio importers (aac, opus, flac, mp3, legacy) dumb: decode() writes a frame and never cuts. The keyframe bit stays authoritative (keyframe = start a new group), so rather than a producer flag that ignores it, audio marks only the first frame of each group a keyframe via container::Producer::needs_keyframe(); the rest extend the group. The caller bounds groups via cut()/seek(). The per-packet bitrate window lives in decode(), and an explicit cut(Some(end)) finalizes it at the boundary, so bitrate/jitter detection is unaffected. The grouping policy moves to the callers: - import::Track cuts after each audio frame by default, so libmoq / FFI / gst / cli stay behavior-identical, with set_manual_grouping() to opt into caller-driven boundaries. - ts (AAC/Opus/legacy) and moq-rtc cut explicitly after each decode. - flv, mkv, moq-audio already cut per frame; fMP4 keeps its own fragment-boundary grouping. Default wire output is unchanged everywhere; caller-driven audio grouping is now possible. Reimplements the idea from #2485 on current main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Audio codecs flag every frame a keyframe, so the codec importers cut a group (a QUIC stream) per packet inside decode(). That storms streams and desyncs co-multiplexed video, and bakes the grouping policy into the primitive rather than leaving it to the caller. Make the audio importers (aac, opus, flac, mp3, legacy) dumb: decode() writes a frame and never cuts. The keyframe bit stays authoritative (keyframe = start a new group), so rather than a producer flag that ignores it, audio marks only the first frame of each group a keyframe via container::Producer::needs_keyframe(); the rest extend the group. The caller bounds groups via cut()/seek(). The per-packet bitrate window lives in decode(), and an explicit cut(Some(end)) finalizes it at the boundary, so bitrate/jitter detection is unaffected. The grouping decision moves to the callers, with no mode flags: - import::Track cuts after each audio frame, so libmoq / FFI / gst / cli get one group (one QUIC stream) per packet. - ts (AAC/Opus/legacy) and moq-rtc cut explicitly after each decode. - flv, mkv, moq-audio already cut per frame; fMP4 keeps its own fragment-boundary grouping. - A caller that wants multi-frame audio groups (a segment cadence) drives a codec importer directly and bounds groups with its cut/seek. Default wire output is unchanged everywhere; caller-driven audio grouping is now possible via the codec importers. Reimplements the idea from #2485 on current main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
By default both the codec-importer facade and the fMP4 container open a group per audio frame/fragment (one QUIC stream each). An origin feeding a packager or ad-splicer needs the opposite: audio grouped to a segment cadence so both tracks share real, aligned cut points to splice at. Add an opt-in on both surfaces libmoq exposes: - import::Track::set_manual_grouping: when set, decode() stops cutting after each audio frame, so frames accumulate into the current group and the caller bounds them via cut()/seek(). - container::fmp4::Import (and import::Container) set_manual_grouping: when set, an audio fragment no longer opens a group; a group opens only at an explicit caller boundary (seek), a video keyframe, or when none is open yet. Audio fragments accumulate into the segment's group. Default off on both, so every existing caller is unchanged. No effect on video, which groups by its own keyframes. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Expose the group controls a native (C/C++) publisher needs, and make the video decoder optional. - moq_publish_media_group / Publish::media_seek: force a synchronized group boundary across all tracks of a media importer, opening the next group at an explicit sequence. Lets a caller drive aligned, explicitly-numbered groups (dual-pipeline alignment, explicit numbering such as epoch-PTS ids). - moq_publish_media_manual_grouping / Publish::media_manual_grouping: opt a media importer into caller-driven audio grouping. For a codec track (import::Track) audio frames accumulate into the current group instead of one group (one QUIC stream) per packet; for the fMP4 container passthrough (import::Container) audio fragments accumulate into the caller's group instead of one per fragment. The caller bounds groups via moq_publish_media_group. Default off, so existing callers are unchanged. - hw-video: moq-video moves behind a default-off hw-video feature, so a stock build needs no libva/CUDA. moq_consume_video_raw* stay in the ABI and return Unsupported without the feature. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Additive C-ABI for native (C/C++) callers, threaded through to moq-native's client config: - moq_tls_disable_verify(disable): skip TLS certificate verification for future sessions (dev / self-signed relays). Global; call before moq_session_connect. - moq_backoff_config(initial_ms, multiplier, max_ms, timeout_ms): configure the exponential backoff applied to session reconnects. timeout_ms = 0 retries forever. Both stash their value in State; moq_session_connect threads them into Session::connect, which sets ClientConfig.tls.disable_verify / ClientConfig.backoff before init. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
a1261a9 to
7e4f168
Compare
2 participants
Caller-driven group boundaries for audio + libmoq opt-in knobs
Lets a native publisher drive explicit, synchronized MoQ group boundaries instead of opening a new group (and QUIC stream) per audio frame — AAC flags every frame a keyframe, which was storming streams and desyncing co-multiplexed video. The spec only requires a group to start with a keyframe, not a new group at every one.
Features
moq-mux — caller-driven audio grouping. container::Producer gains keyframe_starts_group (default true, unchanged) + with_/set_keyframe_grouping; when off, keyframes accumulate and the caller bounds groups via seek/cut. AAC importer opts in; fmp4 import starts a group only at an explicit boundary, a video keyframe, or when none is open (audio rides the segment cadence, not one group per fragment).
libmoq — group-boundary C-ABI. moq_publish_media_group (forces a synchronized, explicitly-numbered boundary across a media importer's tracks) and moq_publish_media_keyframe_grouping (opt a track into caller-driven audio grouping).
libmoq — hardware video is now opt-in. moq-video moves behind a default-off hw-video feature, so a stock build needs no libva/CUDA; moq_consume_video_raw* stay in the ABI and return Unsupported without it.
libmoq — connection knobs. moq_tls_disable_verify (dev / self-signed relays) and moq_backoff_config (reconnect backoff)