feat(bindings): fetch and decode a retained media group - #2827
Conversation
|
Warning Review limit reached
Next review available in: 22 seconds You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (22)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8468bcaef0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Unlike [`Self::subscribe_media`], this does not create a live subscription or apply | ||
| /// latency-based group skipping. The returned consumer reads exactly the requested group | ||
| /// until [`MoqMediaGroupConsumer::next`] returns `None`. | ||
| pub async fn fetch_media_group( |
There was a problem hiding this comment.
When this FFI API ships, the ergonomic Python and Go packages still cannot use it: py/moq-rs/moq/subscribe.py and go/wrapper/moq/subscribe.go expose raw fetch_group and live subscribe_media, but have no fetched-media consumer or method, and rs/libmoq plus the Python, Go, and C docs are also unchanged. Add the corresponding wrappers, exports, tests, and documentation so the public binding surfaces remain synchronized. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L170-L174
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| /// Fetch one complete group and decode its track container into media frames. | ||
| public func fetchMediaGroup( |
There was a problem hiding this comment.
The new Swift API will not reach package users because this commit leaves swift/VERSION at 0.4.3, which was already assigned by commit 05c7bf3. The checked release-swift-lib.yml treats an existing mirror tag as the release gate and skips publication, so bump the wrapper version alongside these source changes. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
Add `MoqBroadcastConsumer.fetch_media_group`: fetch one retained group by sequence and decode its track container into media frames, without opening a live subscription or applying latency-based group skipping. This sits between the existing `fetch_group` (raw byte payloads) and `subscribe_media` (live, container-decoded), reusing `MoqFetchGroupOptions` for delivery priority. The container is parsed before the fetch is issued so invalid CMAF init data does not leave a dynamic group request waiting for a consumer that can never read it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Wrap `fetch_media_group` as `BroadcastConsumer.fetchMediaGroup` and add `MediaGroupConsumer`, an `AsyncSequence` over the finite group's decoded frames that cancels the native read when iteration ends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Alias `MediaGroupConsumer` and add a `frames()` Flow over a fetched group, matching the cancellation handling of the existing consumer flows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`fetch_media_group` decoded its group inline in moq-ffi: a pending queue, a `Container::read` loop, and a hand-rolled copy of the first-frame keyframe invariant that `container::Consumer` already implements. That put a decoding primitive in the bindings layer, where no Rust caller can reach it, and left the invariant stated in two places. Move it into `moq_mux::container::GroupConsumer`: the group-scoped counterpart to `container::Consumer`, reading exactly the group it is handed with no subscription, group ordering, or latency skipping. `read`/`poll_read` mirror the track-level consumer, so moq-hls can use it to serve a segment from a FETCH without going through a live subscription. moq-ffi now wraps it, and the keyframe invariant lives only in moq-mux. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Swift wrapper stayed at 0.4.3, which #2738 already published: the moq-dev/moq-swift mirror carries that tag, and release-swift-lib.yml treats an existing mirror tag as the release gate. `fetchMediaGroup` and `MediaGroupConsumer` would have been skipped at publish and never reached package users. Bump to 0.4.4 and move the three canonical install snippets with it, the way #2738 did. The Kotlin wrapper is bumped correctly, but doc/lib/kt/index.md still installed 0.4.2, so the primary install page pointed at a wrapper without the new `frames()` extension. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The FFI method shipped to Swift and Kotlin only, leaving the ergonomic Python and Go packages with `fetch_group` and live `subscribe_media` but no decoded retained-group counterpart. That breaks the moq-ffi cross-package contract, and #2738 set the precedent that an FFI addition lands with its py/go wrappers. Both mirror their existing siblings: Python gets a `MediaGroupConsumer` async iterator and context manager next to `GroupConsumer`, Go gets a `MediaGroupConsumer` with `Sequence`/`Next`/`Frames`/`Cancel`. libmoq is still untouched: it is a hand-written C ABI rather than a generated binding, and #2738 did not extend it either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The smoke test called `media.finish()` before `fetchMediaGroup`, but finishing a media producer unpublishes its track, so the fetch missed with NotFound. Fetch while the track is still published, then finish to close the group so the stream terminates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8468bca to
b1ede7a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1ede7aaaa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """ | ||
| return GroupConsumer(await self._inner.fetch_group(name, sequence, options)) | ||
|
|
||
| async def fetch_media_group( |
There was a problem hiding this comment.
When this lands on main, the newly added fetch_media_group wrapper will not reach PyPI users because py/moq-rs/pyproject.toml remains at 0.4.4, which was already assigned by f91e3bb. The release gate in .github/workflows/release-py.yml lines 112-119 treats an existing PyPI version as a no-op, and lines 134-138 publish only when that check reports the version absent. Bump the moq-rs version alongside this API addition. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
2 participants
Summary
This is the
fetch_media_grouphalf of #2764, split out so it can land onmainnow. The first four commits are Qizot's, rebased and stripped of the timeline surface.Fetch one retained group by sequence and decode it through the rendition's container, without opening a live subscription. It sits between the two things we already have:
fetch_grouphands back raw payloads,subscribe_mediadecodes but only live. Neither lets a binding client pull a single past group as media frames, which is what a seek, a thumbnail, or a segment-at-a-time exporter needs.Why the timeline part is not here
The rest of #2764 exposes
hang::catalog::Timelineand its group index through the FFI. That model is per media track onmain(VideoConfig.timeline/AudioConfig.timeline, one record per group), anddevhas already replaced it:maindev{group, pts}{segment, pts, duration, tracks}duration_max<rendition>.timeline.ztimeline.z(DEFAULT_NAME)Shipping the
mainshape through uniffi meansMoqVideo.timeline/MoqAudio.timelinehave to be removed later andMoqTimelineEntryreshaped, which is a semver break in publisheddev.moq:moqand Swift packages rather than an additive change. Better to wrap the segment model once it settles. Nothing here depends on it.Public API changes
All additive, hence
main.moq-muxaddscontainer::GroupConsumer.moq-ffiaddsMoqMediaGroupConsumerandMoqBroadcastConsumer.fetch_media_group.MediaGroupConsumerandBroadcastConsumer.fetchMediaGroup.MediaGroupConsumeralias and aframes()Flow.MediaGroupConsumerandBroadcastConsumer.fetch_media_group.MediaGroupConsumerandBroadcastConsumer.FetchMediaGroup.The refactor
Qizot's version decoded the group inline in moq-ffi: a pending queue, a
Container::readloop, and a hand-rolled copy of the first-frame keyframe invariant thatcontainer::Consumeralready implements. That put a decoding primitive in the bindings layer where no Rust caller can reach it, and stated the invariant in two places.moq_mux::container::GroupConsumeris the group-scoped counterpart tocontainer::Consumer: it reads exactly the group it is handed, with no subscription, group ordering, or latency skipping, and mirrorsread/poll_read. moq-hls wants this same primitive to serve a segment from a FETCH. moq-ffi now wraps it, and the keyframe invariant lives in one place.Release metadata
swift/VERSIONgoes to 0.4.4. It was left at 0.4.3, which #2738 already published: the moq-dev/moq-swift mirror carries that tag, andrelease-swift-lib.ymlgates publication on the absence of a mirror tag, so the new Swift API would have been skipped at release. The three canonical install snippets move with it, as they did in #2738.doc/lib/kt/index.mdalso still installed the pre-bump 0.4.2 Kotlin wrapper, which does not contain the newframes()extension.Test plan
cargo nextest run -p moq-mux(two new tests: a legacy group read to completion, and a CMAF fragment whose batch is handed back one frame at a time)cargo nextest run -p moq-ffi(56 passing, including three fetch-media-group tests carried over from feat(bindings): expose media timelines #2764)just py check(ruff, pyright, docs build)just go check(bindings regenerated,go vet/go build/go test -race)just check/just testNote that
just checkdoes not compile moq-ffi or libmoq, so the moq-ffi tests above were run directly.Cross-package sync
Swift, Kotlin, Python, and Go wrappers are updated along with
doc/lib/{swift,kt,py,go}and the Swift/Kotlin READMEs.rs/libmoqanddoc/lib/care unchanged: libmoq is a hand-written C ABI rather than a generated binding, and #2738 (the most recent comparable moq-ffi addition) extended the generated bindings without touching it. Adding a C entry point for this is a separate design decision, not a sync chore.Original PR: #2764. The first four commits are authored by @Qizot.
(Written by Opus 5)