Add libmoq catalog producer + raw moq-net track API#1533
Conversation
Add the producer counterpart to the catalog consume API in libmoq. Callers can now author catalog renditions directly instead of relying on moq_publish_media_ordered to generate them: - moq_publish_video_config / moq_publish_audio_config: insert or replace a rendition, keyed by name, reusing the existing moq_video_config / moq_audio_config structs as inputs. - moq_publish_video_remove / moq_publish_audio_remove: remove a rendition by name. Each call republishes the catalog automatically via the existing moq-mux catalog Producer guard. Includes unit tests (invalid-handle, null-pointer, and a publish/consume round-trip) and a changelog entry. The generated C header picks up the new functions and their doc comments via cbindgen.
- Drop the identity .into() on hang::Error in the catalog producer map_err calls (clippy useless_conversion). - Reflow the producer method signatures to satisfy rustfmt. - Record the new catalog producer functions under [Unreleased] in the libmoq changelog.
|
Warning Review limit reached
More reviews will be available in 12 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds two families of changes: (1) catalog-rendition producer APIs—Rust Publish methods and C-ABI exports—to insert/replace and remove named video/audio renditions (with CHANGELOG and tests), and (2) a raw (non-media) moq-net track path—Publish storage for Track/Group producers, publish FFI for track/group/frame lifecycle, Consume runtime and FFI for subscribing to raw tracks (async task, frame buffering, frame read/close), an added Error::GroupNotFound mapping, and tests covering invalid ids and end-to-end raw-group/frame delivery. 🚥 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 review The previous review aborted because I pushed a follow-up commit (clippy (Written by Claude) Generated by Claude Code |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/libmoq/src/api.rs`:
- Around line 391-394: The code treats a NULL description pointer as "omit" even
when config.description_len > 0; change both places that check
config.description and call ffi::parse_slice (the blocks that set
video.description) to explicitly reject a NULL pointer when description_len > 0
by returning the appropriate ffi::InvalidPointer error (or equivalent) instead
of skipping parsing; otherwise, proceed to call
ffi::parse_slice(config.description, config.description_len)? and set
video.description = Some(bytes::Bytes::copy_from_slice(...)).
🪄 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: 60763b12-21d3-46be-afb3-1c22c24a32b6
📒 Files selected for processing (4)
rs/libmoq/CHANGELOG.mdrs/libmoq/src/api.rsrs/libmoq/src/publish.rsrs/libmoq/src/test.rs
| if !config.description.is_null() { | ||
| let description = unsafe { ffi::parse_slice(config.description, config.description_len)? }; | ||
| video.description = Some(bytes::Bytes::copy_from_slice(description)); | ||
| } |
There was a problem hiding this comment.
Reject NULL descriptions when description_len > 0.
Both config entry points currently treat description = NULL as “omit the field” even when description_len is non-zero. That silently drops malformed FFI input instead of surfacing InvalidPointer, which makes interop bugs much harder to diagnose.
Suggested validation
- if !config.description.is_null() {
- let description = unsafe { ffi::parse_slice(config.description, config.description_len)? };
- video.description = Some(bytes::Bytes::copy_from_slice(description));
- }
+ match (config.description.is_null(), config.description_len) {
+ (true, 0) => {}
+ (true, _) => return Err(Error::InvalidPointer),
+ (false, _) => {
+ let description = unsafe { ffi::parse_slice(config.description, config.description_len)? };
+ video.description = Some(bytes::Bytes::copy_from_slice(description));
+ }
+ }
...
- if !config.description.is_null() {
- let description = unsafe { ffi::parse_slice(config.description, config.description_len)? };
- audio.description = Some(bytes::Bytes::copy_from_slice(description));
- }
+ match (config.description.is_null(), config.description_len) {
+ (true, 0) => {}
+ (true, _) => return Err(Error::InvalidPointer),
+ (false, _) => {
+ let description = unsafe { ffi::parse_slice(config.description, config.description_len)? };
+ audio.description = Some(bytes::Bytes::copy_from_slice(description));
+ }
+ }Also applies to: 429-432
🤖 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/libmoq/src/api.rs` around lines 391 - 394, The code treats a NULL
description pointer as "omit" even when config.description_len > 0; change both
places that check config.description and call ffi::parse_slice (the blocks that
set video.description) to explicitly reject a NULL pointer when description_len
> 0 by returning the appropriate ffi::InvalidPointer error (or equivalent)
instead of skipping parsing; otherwise, proceed to call
ffi::parse_slice(config.description, config.description_len)? and set
video.description = Some(bytes::Bytes::copy_from_slice(...)).
Expose the bare moq-net track primitives over the C FFI so callers can publish and consume arbitrary (non-media) byte tracks, with no codec, container, or catalog framing. This mirrors moq-ffi's publish_track / subscribe_track and pairs with the catalog producer API: describe a track with moq_publish_*_config, then feed it yourself. Publish side: - moq_publish_track(broadcast, name) -> track handle (create_track) - moq_publish_track_group(track) -> group handle (append_group) - moq_publish_track_frame(track, payload) convenience one-frame group - moq_publish_group_frame(group, payload) / moq_publish_group_close(group) - moq_publish_track_close(track) Consume side: - moq_consume_track(broadcast, name, on_frame, user_data) -> track handle - moq_consume_track_frame(frame, dst) fills moq_frame (timestamp/keyframe are 0/false for raw tracks) - moq_consume_track_frame_close(frame) / moq_consume_track_close(track) The consumer iterates groups in sequence order and delivers every frame within each group, so the consumer sees exactly what the producer wrote regardless of grouping. Adds a GroupNotFound error code (-31), a publish/consume round-trip test, an invalid-handle test, and changelog entries.
kixelated
changed the title
The earlier test added a catalog rendition + catalog-subscribe handshake on the theory that a frame written before the subscription attached would be dropped. That was wrong: moq-net caches the latest group and subscribers replay from the first frame of each group, so a late subscriber still sees it. The original failure was actually the run_raw consumer reading only the first frame of each group (it used read_frame, which skips the rest of the group), so the multi-frame group hung. That's already fixed by iterating every frame per group. Drop the handshake; the test passes reliably without it.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/libmoq/src/publish.rs (1)
144-148: 💤 Low valueGroup producers can leak from the
groupsslab.
track_groupinserts aGroupProducerintoself.groups, buttrack_finishonly removes the track. Any groups created from that track and not explicitly closed viagroup_finishremain in the slab for the lifetime ofPublish. Not user-visible, but it accumulates handles for the common author-then-finish flow. Consider tracking group ownership per-track so finishing a track also drops its outstanding groups.Also applies to: 159-163
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c033a0b5-b90d-4b20-b01d-a1eb621cc670
📒 Files selected for processing (6)
rs/libmoq/CHANGELOG.mdrs/libmoq/src/api.rsrs/libmoq/src/consume.rsrs/libmoq/src/error.rsrs/libmoq/src/publish.rsrs/libmoq/src/test.rs
✅ Files skipped from review due to trivial changes (1)
- rs/libmoq/CHANGELOG.md
2 participants
Summary
Adds two related, additive pieces to libmoq's C API, both mirroring functionality that already exists in
moq-ffibut was missing from the C bindings. Together they let a C caller describe a broadcast's catalog and write/read arbitrary track contents themselves, the producer-side opposite of the existing consume API.1. Catalog producer API
The producer counterpart to
moq_consume_video_config/moq_consume_audio_config. Lets callers author catalog renditions directly instead of relying onmoq_publish_media_orderedto derive them.moq_publish_video_config(broadcast, const moq_video_config*)/moq_publish_audio_config(broadcast, const moq_audio_config*)— insert or replace a rendition, keyed byname.moq_publish_video_remove/moq_publish_audio_remove— remove by name.These reuse the existing
moq_video_config/moq_audio_configstructs as inputs (the same structs the consume side fills as outputs), so producer and consumer are mirror images. Each call republishes the catalog automatically via the existing moq-mux catalogProducerguard.description/coded_width/coded_heightpointers may beNULLto omit those fields.2. Raw moq-net track API
The catalog API above only writes metadata. To actually feed a track, libmoq previously offered only
moq_publish_media_ordered, which requires a knownFramedFormatand owns its own catalog/container. There was no way to publish or consume a non-media track (control channel, JSON metadata, or your own encoded bitstream) over the C bindings.This adds the bare moq-net primitives, mirroring
moq-ffi'spublish_track/subscribe_track(broadcast → track → group → frame, raw bytes, no codec/container/catalog):Publish:
moq_publish_track(broadcast, name, name_len)→ track handle (create_track)moq_publish_track_group(track)→ group handle (append_group)moq_publish_track_frame(track, payload, len)— convenience one-frame-per-group writemoq_publish_group_frame(group, payload, len)/moq_publish_group_close(group)moq_publish_track_close(track)Consume:
moq_consume_track(broadcast, name, name_len, on_frame, user_data)→ track handlemoq_consume_track_frame(frame, moq_frame*)— fillspayload/payload_size;timestamp_us/keyframeare 0/false (not meaningful for raw tracks)moq_consume_track_frame_close(frame)/moq_consume_track_close(track)The consumer iterates groups in sequence order and delivers every frame within each group, so it sees exactly what the producer wrote regardless of grouping. Adds a
GroupNotFounderror code (-31).These two pieces compose: describe a track with
moq_publish_*_config, then push its frames withmoq_publish_track/_frame.Cross-package sync
rs/libmoqCHANGELOG.md; doc comments flow into the cbindgen-generatedmoq.hthatdoc/lib/cpoints atConfined to
rs/libmoqand purely additive (no existing signatures change), so it does not touchrs/moq-ffi, the wire/catalog formats, or the language wrappers — and targetsmain.Test plan
cargo test -p libmoq— 19 tests pass, including:publish_catalog_roundtrip: author video + audio renditions, publish, consume the catalog back (codec, coded dimensions, sample rate, channels), then remove the video rendition and confirm the republished catalog drops it.raw_track_publish_consume: publish a rawdatatrack, consume it viamoq_consume_track, verify a one-frame-per-group write and a multi-frame group both round-trip byte-for-byte, and thattimestamp_us/keyframecome back 0/false.cargo clippy -p libmoq --all-targets— clean.cargo fmt -p libmoq -- --check— clean.moq.hpicks up all new functions (one declaration each).(Written by Claude)