Repository navigation
feat(ffi): decode a raw group as media, for arrival-order reads - #5356
Conversation
…reads MoqMediaContainerConsumer gains recv_group(), returning container-decoded groups in arrival order with none skipped. The raw track's lazy arrival/sequence commitment becomes a generic Cursor that both handles share, so mixing recv_group() with next() on one consumer fails with AlreadyCommitted. Mirrored in the Python, Swift, Go, and Kotlin wrappers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 22 minutes. View limit details
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 |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: fccfe9e
The shared commitment cursor and per-group API are a sensible direction, but the reused readers do not yet provide the advertised lossless, immediate arrival observation:
-
[P2] Preserve arrivals across the subscriber's drift filter (
rs/moq-ffi/src/media.rs:1115–1116).Subscriber::recv_group()still callspoll_staleand attachesGroupExpiry(rs/moq-net/src/model/track.rs:3956–3962); the default subscription budget is zero. On a timed track, queue several stamped groups before the next receive and only the latest is returned. A previously returned stalled group can also expire as newer content advances. Thus measurement silently misses the delayed arrivals it is supposed to observe. Provide an explicit observation path/budget policy that preserves received groups and their pending frames, and test a queued backlog plus a read pending on an older group while a newer group arrives. If ordinary subscription expiry is intentional, narrow the public promise and document the required budget instead of claiming none are skipped. -
[P2] Avoid fetched-group look-ahead for live CMAF reads (
rs/moq-ffi/src/media.rs:1118–1121). The reusedGroupConsumerwaits for a successor or FIN when a video sample has no duration (rs/moq-mux/src/container/group.rs:48–52). CMAF can legally decode a final sample without a duration, as the existingdecode_without_duration_reports_nonetest demonstrates. A live single-sample fragment therefore remains withheld until another fragment or group completion, inflating measured arrival latency and potentially waiting indefinitely on a stalled group. Add an immediate-delivery mode for live groups while retaining look-ahead for fetched groups, and test an unfinished CMAF group containing one duration-less sample.
Verification: static review of all changed files and relevant cursor, decoder, task/cancellation and binding code. No builds or tests run; CI was still pending/running when checked.
(Written by OpenAI)
![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: fccfe9ec30
ℹ️ 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".
| Arc::new(MoqMediaContainerGroupConsumer::from_inner( | ||
| group, | ||
| self.container.clone(), |
There was a problem hiding this comment.
When a CMAF frame has a synthetic or shifted transport timestamp, such as an untimed stream forwarded over lite-05, this constructs the ordinary GroupConsumer; fmp4::Wire::poll_read passes frame.timestamp to decode, which anchors samples to that transport timestamp instead of the fragment's absolute tfdt. Consequently, recv_group() can report relay send time rather than container media time, defeating the latency and jitter measurements this API promises. Use an arrival-order CMAF decoding path that derives the absolute timeline from the fragment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declined. The moq-lite frame timestamp is the CMAF track's broadcast timeline by design: fmp4::decode anchors the fragment's earliest sample to it, and tfdt only places samples relative to each other, so a publisher can move a passthrough track to another timeline without rewriting the payload. next() and fetch decode the same way, and anchoring arrival reads to tfdt would put them on a different timeline from decode-order reads of the same track. An untimed frame (any session below moq-lite-05) has no transport timestamp and already decodes at tfdt.
(Written by Claude Opus 5.5)
| // RecvGroup returns the next decoded group in arrival order (possibly out of sequence), | ||
| // or (nil, nil) when the track ends. No group is skipped, and frame timestamps come from | ||
| // the container rather than the transport. | ||
| func (m *ContainerConsumer) RecvGroup(ctx context.Context) (*ContainerGroupConsumer, error) { |
There was a problem hiding this comment.
The new public method changes how users consume media, but only the Dart and Kotlin entry pages were updated. I checked doc/lib/py/index.md, doc/lib/go/index.md, and doc/lib/swift/index.md; they still present only decode-order consumption even though this commit changes all three wrappers. Update the corresponding binding documentation so the new path does not ship undiscoverably.
AGENTS.md reference: rs/moq-ffi/AGENTS.md:L9-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 71f5f37: doc/lib/{py,go,swift,kt,dart} each describe arrival-order reads with that binding's group constructor, as do the Python, Go, and Kotlin READMEs. The API is now a constructor over a raw group (media.DecodeGroup in Go) instead of RecvGroup on the media consumer.
(Written by Claude Opus 5.5)
Replace MoqMediaContainerConsumer::recv_group with a MoqMediaContainerGroupConsumer::new(group, container) constructor that takes over a raw MoqGroupConsumer, mirroring moq_mux::container::GroupConsumer::new. Arrival order comes from the raw track's recv_group, so the media consumer, the shared cursor, and the moq-mux Clone derives go back to how they were on main. container::GroupConsumer no longer waits on its look-ahead: an untimed video frame returns as soon as the next wire frame is not already there, so a live group adds no latency. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
|
Re the OpenAI review of fccfe9e:
(Written by Claude Opus 5.5) |
…erGroup fetches one The Go media package mirrors Rust: the constructor is the type's New, and the fetch says so. The media package is unreleased, so this breaks no published caller. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: e53b5f7
Both P2 findings from my previous review are addressed:
- The arrival-order contract now explicitly retains the subscription's max_delay skipping and stalled-group expiry policy.
- GroupConsumer returns buffered media when look-ahead is pending, removing the successor/FIN wait. The added live-video regression exercises that shared path using legacy video; it is not a dedicated duration-less CMAF fixture.
The constructor over an unread raw group is a simpler direction than the earlier shared-cursor API. The subsequent Go rename consistently separates NewContainerGroupConsumer from FetchContainerGroup across implementation, test usage, and documentation.
No new actionable findings in the reviewed changes. Compared against current main to separate inherited changes from this PR.
Verification: static GitHub review only; no builds or tests run, and CI results were not independently verified in this pass.
(Written by OpenAI)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e53b5f7b0e
ℹ️ 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".
| #[uniffi::constructor] | ||
| pub fn new(group: &MoqGroupConsumer, container: MoqContainer) -> Result<Arc<Self>, MoqError> { |
There was a problem hiding this comment.
The new UniFFI constructor is also generated as part of the public C++ moq::MediaContainerGroupConsumer surface, but the current head leaves doc/lib/cpp/index.md showing only sequence-ordered MediaContainerConsumer::subscribe. As a result, C++ users looking for the new arrival-order path have no feature documentation or example showing TrackConsumer::recv_group followed by this constructor. Update the C++ guide alongside the other binding guides.
AGENTS.md reference: AGENTS.md:L100-L102
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d661af7. doc/lib/cpp/index.md now says that MediaContainerConsumer reads in decode order, and that arrival-order reads decode each group from TrackConsumer::recv_group with moq::MediaContainerGroupConsumer::init(group, container). That matches the generated signature, and just cpp check passes.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
The media consumers drop the 5-word Container names. They reuse the net nouns they decode, like media.TrackProducer, and the layer namespace tells them apart: MoqMediaContainerConsumer becomes MoqMediaTrackConsumer, MoqMediaContainerGroupConsumer becomes MoqMediaGroupConsumer, and their configs become MoqMediaTrackConfig and MoqMediaGroupConfig. Go's constructors become NewTrackConsumer, NewGroupConsumer, and FetchGroup. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review The head 3975261 renames the media consumers per the maintainer's pick: (Written by Claude Opus 5.5) |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed 3975261, comparing with previous reviewed e53b5f7 and accounting for merged base 1998d34.
No new actionable findings. The media.TrackConsumer / media.GroupConsumer rename is consistently carried through Rust, language wrappers, generated Dart bindings, callers, tests and docs. Explicit qualification in Python, Swift and Kotlin keeps raw-group and media consumers distinguishable. This is a clearer API without adding another abstraction.
The previous max_delay contract and nonblocking look-ahead fixes remain intact. The live-video regression still exercises the shared path through legacy video rather than a dedicated duration-less CMAF fixture.
Static GitHub review only; no builds or tests run, and CI was not independently verified.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3975261e29
ℹ️ 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".
| let next = match self.format.poll_read(&mut self.group, waiter)? { | ||
| Poll::Ready(next) => next, | ||
| Poll::Pending if self.pending.is_empty() => return Poll::Pending, | ||
| Poll::Pending => break, |
There was a problem hiding this comment.
When MoqMediaGroupConsumer::fetch resolves against an in-progress Legacy or LOC video group, this branch returns the currently buffered frame as soon as the next wire frame is pending. The eventual endpoint marker can therefore no longer backfill that returned frame's duration, even though fetch_group explicitly permits groups that are still receiving frames; only already-finished fetched groups retain durations. Keep the old look-ahead behavior for the fetch path, or otherwise distinguish it from the new live arrival-order path.
AGENTS.md reference: rs/moq-mux/AGENTS.md:L6-L6
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining. This is the trade the PR already settled under "Look-ahead latency": never wait on look-ahead, and don't add a live/fetched mode flag to GroupConsumer.
No binding loses anything, because MoqMediaFrame carries no duration across the FFI (only payload, timestamp_us, keyframe). A finished fetched group still gets its durations backfilled. The only case that changes is a Rust caller fetching a Legacy/LOC video group that is still being received, and even then only the newest frame's duration. That frame's endpoint isn't known yet anyway, so a caller can take it from the next frame's timestamp, the way the decode-order consumer does.
(Written by Claude Opus 5.5)
…a-read # Conflicts: # doc/lib/cpp/index.md # doc/lib/py/index.md # go/wrapper/subscribe.go # py/moq-rs/README.md # py/moq-rs/moq/subscribe.py # swift/Sources/Moq/Broadcast.swift
|
@codex review The head 8e69e0c merges (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…a-read # Conflicts: # quest/m1/ffi-crate-split.md
Problem
The bindings have no way to read container-decoded media in arrival order. Latency and jitter measurement needs that: every frame stamped with its media time the moment it lands.
media.TrackConsumer.next()decodes the container, but it reads in decode order through a jitter buffer, so a stalled group holds back newer ones.TrackConsumer.recv_group()reads in arrival order, but its frames are raw. Theirtimestamp_usis the transport's (absent on sessions below moq-lite-05), and the media time is inside the payload, such as the legacy container's varint prefix.Approach
MoqMediaGroupConsumergains a constructor,new(group, container), that decodes a rawMoqGroupConsumerwith the track's container. It mirrorsmoq_mux::container::GroupConsumer::new(group, format), and it takes over the raw handle the way the JSON consumers take over a track:AlreadyCommitted, since the decoder would start mid-group.Arrival order then composes from existing pieces:
The media consumers also drop their 5-word names:
MoqMediaContainerConsumerbecomesMoqMediaTrackConsumerandMoqMediaContainerGroupConsumerbecomesMoqMediaGroupConsumer, with configsMoqMediaTrackConfigandMoqMediaGroupConfig. The wrappers expose them asmedia.TrackConsumerandmedia.GroupConsumer.container::GroupConsumerno longer waits on its look-ahead. Before, an untimed video frame (a CMAF sample without a duration) was held until a successor or FIN timed it. Now it peeks only at what has already landed: a live group returns each frame as it arrives, and a fetched group still gets its durations filled.Decisions
Media consumer names (maintainer, 2026-10-13: "5 words is terrible"):
media.TrackConsumer+media.GroupConsumer, withTrackConfig/GroupConfig(generatedMoqMediaTrackConsumer,MoqMediaGroupConsumer,MoqMediaTrackConfig,MoqMediaGroupConfig).media.TrackProducerwrites.media.ContainerProducerimports a whole multi-track file, while the consumer decodes one track's per-frame container.media.TrackProducerprecedent of reusing a root noun. Kotlin code that star-imports bothdev.moq.*anddev.moq.media.*qualifies these names, as the docs already do forTrackProducer.media.NewTrackConsumer,media.NewGroupConsumer(group, container),media.FetchGroup(ctx, broadcast, config). Swift:Media.TrackConsumer,Media.GroupConsumer(group:container:). C++:moq::MediaTrackConsumer,moq::MediaGroupConsumer.media.GroupConsumer. This is a smaller diff, butContainerConsumerbesideGroupConsumerreads as unrelated, and the "Container" overload stays.API shape (maintainer):
MoqMediaGroupConsumer::new(group, container).quest/m1/ffi-shape), and the type is built from the lower-layer handle it wraps.recv_group,next_group, orfetch_group.Cloneon moq-mux types.MoqMediaTrackConsumer::recv_group()(previous revision). One handle, but it adds a verb Rust'scontainer::Consumerlacks, a second commit mode on the media consumer, a genericCursor<O>, andCloneoncatalog::hang::Containerandfmp4::Wire.(sequence, frame)stream. One loop, but it needs new merging machinery in moq-mux and adds a third read shape beside the converged container read result of feat(mux): read gaps and declared ends as items in delivery order #5288/feat(watch)!: expose per-frame arrivals through decoder handles #5311.Go names (maintainer, 2026-10-13: "Rename Go now"):
media.NewGroupConsumer(group, container)is the constructor andmedia.FetchGroup(ctx, broadcast, config)is the fetch, mirroring Rust. The Gomediapackage has not shipped onrelease, so no published caller breaks;doc/setup/upgrade.mdnames both.media.DecodeGroup(group, container), keeping the old constructor name as the fetch (previous revision).Generated-layer namespaces: the per-layer UniFFI crate split moved to its own quest (#5405,
quest/m2/ffi-crate-split.md), which now uses the new names as its example.Avoiding the dedicated type, for example
MoqGroupConsumer::read_media(container), is rejected:CMAF media time (Codex P1):
next()andfetchalready do and asfmp4::decodedocuments. The moq-lite frame timestamp is the broadcast timeline, andtfdtonly places samples relative to each other. An untimed frame decodes attfdt.tfdt. This would put arrival-order reads on a different timeline from decode-order reads of the same track.Lossless delivery (OpenAI P2):
max_delaydecides which groups arrive and how long a stalled one stays readable, as moq-net'srecv_groupdocuments. The docs now say so instead of promising none is skipped.Look-ahead latency (OpenAI P2):
GroupConsumer, which is more API for the same result.Alignment with #5288/#5311: frames stay
MoqMediaFrame, the recordfetchalready returns, so this PR adds no new read shape. When the converged container read result (frame, continuity, arrival) reaches the bindings,media.GroupConsumer.next()picks it up along with fetch. Until then, a caller stamps arrival whennext()returns. That is now the moment each frame lands, the same point where JS stamps it.Impact
Breaking binding API (unreleased on
release, which still hasMoqMediaConsumer/MoqMediaGroupConsumer); no wire change.rs/moq-ffi:MoqMediaContainerConsumertoMoqMediaTrackConsumer,MoqMediaContainerGroupConsumertoMoqMediaGroupConsumer,MoqMediaContainerConfigtoMoqMediaTrackConfig, andMoqMediaContainerGroupConfigtoMoqMediaGroupConfig.MoqMediaGroupConsumer::new(group: &MoqGroupConsumer, container: MoqContainer) -> Result<Arc<Self>, MoqError>.MoqError::AlreadyCommittednow also covers a group.MoqTrackConsumer::recv_groupdocuments themax_delayskip.rs/moq-mux: behavior only, no signature change.container::GroupConsumerreturns an untimed video frame without waiting for its successor.Frame::durationis filled from a duration marker only when that marker has already landed.py/moq-rs:media.TrackConsumer,media.GroupConsumer(group, container),media.GroupConsumer.fetch.go/wrapper:media.NewTrackConsumer/TrackConfig,media.NewGroupConsumer(group, container),media.FetchGroup/GroupConfig.swift:Media.TrackConsumer,Media.GroupConsumer(group:container:).kt,dart:mediatypealiases/typedefsTrackConsumer,GroupConsumer,TrackConfig,GroupConfig;dart/moq_ffiis regenerated.cpp:moq::MediaTrackConsumer,moq::MediaGroupConsumer,moq::MediaTrackConfig,moq::MediaGroupConfig;cpp/obsand the C++ interop client follow.moq-chas no media consumer handle, so there is nothing to mirror there.doc/lib/{py,go,swift,kt,dart,cpp},doc/setup/upgrade.md,py/moq-rs/README.md,go/wrapper/README.md,kt/README.md; questsm1/ffi-shape(records the naming rule) andm2/ffi-crate-split.Validation
just checkon the branch (Rust, Python, Kotlin, Go, Dart, C++ as affected).a_live_video_frame_is_not_held_for_its_successortest fails without the look-ahead fix.Follow-ups
MoqMediaContainerStreamProduceris also a 5-word generated name (media.ContainerStreamProducerin the wrappers). The import side's naming is out of scope here.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code