feat(bindings): expose retained media group fetching - #2764
Conversation
Expose generic media and timeline consumers. Keep group selection and playback policy at the application layer.
Build Android moq-ffi artifacts with the existing android-logcat feature so native logs are available through logcat.
WalkthroughThe FFI now represents catalog timelines and timeline entries. It supports timeline subscriptions and finite media-group fetching with container-aware frame decoding, buffering, sequencing, and cancellation. Kotlin adds typealiases and 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@doc/lib/kt/moq.md`:
- Around line 236-238: Update the fetchMediaGroup call in the timeline entry
example to pass the required fourth FetchGroupOptions argument, using null or
FetchGroupOptions() as appropriate, while preserving the existing group and
frame collection flow.
In `@kt/moq/src/jvmAndAndroidTest/kotlin/dev/moq/SmokeTest.kt`:
- Around line 118-135: Move the media completion call in the smoke test so
`media.finish()` runs immediately after `writeFrame` and before
`it.frames().toList()` collects the finite flow. Keep `broadcast.finish()`
before timeline-entry collection and preserve the existing frame assertions.
In `@rs/moq-ffi/src/consumer.rs`:
- Around line 158-161: Document the exported FFI surface by adding rustdoc
comments to the public MoqTimelineConsumer struct and its
MoqTimelineConsumer::cancel method, describing their purpose and cancellation
behavior. Ensure both exported Rust items are covered without changing their
implementation.
- Around line 1-5: Add a module-level `//!` documentation comment at the
beginning of the Rust module root before the imports in consumer.rs, describing
the module’s purpose and following the repository’s documentation conventions.
In `@rs/moq-ffi/src/media.rs`:
- Around line 30-32: Document every newly exported field involved in the
timeline bindings: add concise Rust doc comments to the fields of MoqTimeline,
the MoqVideo::timeline and MoqAudio::timeline fields, and the MoqTimelineEntry
fields. Keep the descriptions short and accurate for the generated binding API.
In `@rs/moq-ffi/src/test.rs`:
- Around line 497-512: Update the async tests around
fetch_media_group_decodes_multiple_cmaf_samples and
fetch_media_group_rejects_invalid_container_before_fetching: call
tokio::time::pause() at the start of the decoding test, and wrap the
invalid-container fetch_media_group request in a short tokio::time::timeout
before asserting the expected MoqError::Codec result.
🪄 Autofix
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: 4f65c0e7-f2a8-4c0c-947e-f20559d4d8ca
📒 Files selected for processing (15)
doc/lib/kt/moq.mddoc/lib/swift/moq.mdkt/README.mdkt/gradle.propertieskt/moq/src/jvmAndAndroidMain/kotlin/dev/moq/Aliases.ktkt/moq/src/jvmAndAndroidMain/kotlin/dev/moq/Flows.ktkt/moq/src/jvmAndAndroidTest/kotlin/dev/moq/SmokeTest.ktrs/moq-ffi/build.shrs/moq-ffi/src/audio.rsrs/moq-ffi/src/consumer.rsrs/moq-ffi/src/media.rsrs/moq-ffi/src/test.rsswift/Sources/Moq/Aliases.swiftswift/Sources/Moq/Broadcast.swiftswift/Sources/Moq/Media.swift
| consumer.subscribeTimeline(timeline).entries().collect { entry -> | ||
| consumer.fetchMediaGroup(name, entry.group, audio.container).use { group -> | ||
| group.frames().collect { frame -> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f -e kt . kt/moq | xargs -r rg -n -C 8 \
'\bfetchMediaGroup\s*\(|FetchGroupOptions'Repository: moq-dev/moq
Length of output: 6063
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Kotlin declarations and call sites ---'
rg -n -C 12 'fetchMediaGroup|fun fetchGroup|FetchGroupOptions' kt --glob '*.kt'
printf '%s\n' '--- Generated/API declaration sources ---'
rg -n -C 10 'fetch_media_group|fetchMediaGroup|FetchGroupOptions|MoqFetchGroupOptions' . \
--glob '!target/**' --glob '!build/**' --glob '!node_modules/**'Repository: moq-dev/moq
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Rust UniFFI fetch declarations ---'
rg -n -A 45 -B 12 \
'fetch_media_group|fetchMediaGroup|fetch_group|MoqFetchGroupOptions' \
rs/moq-ffi/src/consumer.rs
printf '%s\n' '--- Kotlin documentation context ---'
sed -n '210,255p' doc/lib/kt/moq.md
printf '%s\n' '--- Kotlin build and binding-generation configuration ---'
rg -n -C 8 \
'uniffi|bindgen|moqffi|moq-ffi|generated' \
kt --glob '*.gradle' --glob '*.gradle.kts' --glob '*.toml' --glob '*.properties' --glob '*.md'Repository: moq-dev/moq
Length of output: 32804
🌐 Web query:
moq-dev/moq Kotlin fetchMediaGroup MoqFetchGroupOptions generated Kotlin signature
💡 Result:
The project moq-dev/moq provides ergonomic Kotlin bindings for Media over QUIC [1]. While the library utilizes UniFFI to generate bindings from a Rust core [2][3], the specific API signature for fetching groups—often discussed in the context of the MoQ transport protocol [4]—is structured around Fetch control messages [5][6]. Based on current project structures and available documentation, MoqFetchGroupOptions is not a standard, high-level signature found in the public dev.moq:moq Kotlin library. Instead, the library manages fetches via native-to-Kotlin FFI bindings that mirror the underlying moq-transport message structure [2][1]. In the broader moq ecosystem (including implementations like moqtail that share design patterns with moq-dev), fetching is typically handled by passing an object containing parameters such as priority, group order, and track location [5][7][8]. In the moq-dev/moq Kotlin bindings, you should look to use the Kotlin idiomatic APIs (typically involving Coroutines and Flow) provided by the dev.moq:moq artifact [2][1]. Because the Kotlin bindings are auto-generated from the Rust core [2][3], the specific class or function signatures are subject to the current state of the moq-ffi layer [9]. To identify the exact generated signature for your version, you can inspect the generated Kotlin source files usually located in build/generated/source/uniffi after a clean build of your project [3].
Citations:
- 1: https://central.sonatype.com/artifact/dev.moq/moq
- 2: https://doc.moq.dev/
- 3: https://github.com/moq-dev/web-transport/tree/main/kt
- 4: https://moq-wg.github.io/moq-transport/draft-ietf-moq-transport.html
- 5: https://mintlify.wiki/moqtail/moqtail/api/fetch
- 6: https://docs.rs/moq-transport/latest/moq_transport/message/struct.Fetch.html
- 7: https://mintlify.wiki/moqtail/moqtail/subscriber/fetching
- 8: https://github.com/moqtail/moqtail/blob/4d5f0011/libs/moqtail-ts/src/client/types.ts
- 9: main...dev
🌐 Web query:
UniFFI Kotlin generated default argument Option parameter uniffi default None
💡 Result:
In UniFFI, you can specify None as a default value for Option<T> parameters when using proc-macros, which translates to null in the generated Kotlin code [1][2][3]. For function or method arguments, you can define the default in the #[uniffi::export] attribute using the default argument [2][3]. For example: #[uniffi::export(default(max_splits = None))] pub fn split(max_splits: Option) -> Vec { //... } This will generate a Kotlin function signature with a nullable parameter defaulted to null: fun split(maxSplits: UInt? = null): List {... } When using UDL files, optional arguments can be defined similarly. While null is the equivalent in Kotlin bindings, the UniFFI documentation specifically notes that when using proc-macros, you use the None literal to signify the default value for an Option<T> type [1][2]. For record fields, you can also assign defaults in a similar manner, ensuring the generated Kotlin data class correctly initializes the field to null if None is provided as the default [4][3].
Citations:
- 1: https://mozilla.github.io/uniffi-rs/latest/types/defaults.html
- 2: https://mozilla.github.io/uniffi-rs/0.29/proc_macro/functions.html
- 3: https://mozilla.github.io/uniffi-rs/proc_macro/index.html
- 4: https://mozilla.github.io/uniffi-rs/next/types/records.html
Pass the fetch options argument.
fetchMediaGroup requires a fourth nullable FetchGroupOptions parameter with no default. Add null or FetchGroupOptions() so the example compiles.
(Written by )
🤖 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 `@doc/lib/kt/moq.md` around lines 236 - 238, Update the fetchMediaGroup call in
the timeline entry example to pass the required fourth FetchGroupOptions
argument, using null or FetchGroupOptions() as appropriate, while preserving the
existing group and frame collection flow.
| media.writeFrame(Frame(payload = "opus frame".encodeToByteArray(), timestampUs = 5_000_000uL)) | ||
|
|
||
| val fetched: MediaGroupConsumer = consumer.fetchMediaGroup( | ||
| name, | ||
| 0uL, | ||
| audio.container, | ||
| FetchGroupOptions(priority = 3u), | ||
| ) | ||
| fetched.use { | ||
| assertEquals(0uL, it.sequence()) | ||
| val frames = it.frames().toList() | ||
| assertEquals(1, frames.size) | ||
| val frame = frames.single() | ||
| assertEquals("opus frame", frame.payload.decodeToString()) | ||
| assertEquals(5_000_000uL, frame.timestampUs) | ||
| } | ||
|
|
||
| media.finish() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Finish the media group before collecting the finite Flow.
it.frames().toList() returns only after next() returns null. The group remains open until media.finish() at Line 135, but that line is unreachable while collection waits.
Call media.finish() after writeFrame and before collecting frames(). Keep broadcast.finish() before collecting timeline entries.
(Written by CodeRabbit)
🤖 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 `@kt/moq/src/jvmAndAndroidTest/kotlin/dev/moq/SmokeTest.kt` around lines 118 -
135, Move the media completion call in the smoke test so `media.finish()` runs
immediately after `writeFrame` and before `it.frames().toList()` collects the
finite flow. Keep `broadcast.finish()` before timeline-entry collection and
preserve the existing frame assertions.
| use std::collections::VecDeque; | ||
| use std::sync::Arc; | ||
|
|
||
| use bytes::Buf; | ||
| use moq_mux::container::Container as _; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a module-level documentation comment.
Add a //! comment before the imports. rs/moq-ffi/src/consumer.rs is a Rust module root.
As per coding guidelines, **/*.rs: “add a module-level //! doc comment to every Rust module root.”
(Written by CodeRabbit)
🤖 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-ffi/src/consumer.rs` around lines 1 - 5, Add a module-level `//!`
documentation comment at the beginning of the Rust module root before the
imports in consumer.rs, describing the module’s purpose and following the
repository’s documentation conventions.
Source: Coding guidelines
| #[derive(uniffi::Object)] | ||
| pub struct MoqTimelineConsumer { | ||
| task: Task<Timeline>, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the exported timeline consumer surface.
Add rustdoc for MoqTimelineConsumer and MoqTimelineConsumer::cancel. Both are public FFI items.
As per coding guidelines, **/*.rs: “Document every exported Rust item.”
(Written by CodeRabbit)
Also applies to: 338-340
🤖 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-ffi/src/consumer.rs` around lines 158 - 161, Document the exported FFI
surface by adding rustdoc comments to the public MoqTimelineConsumer struct and
its MoqTimelineConsumer::cancel method, describing their purpose and
cancellation behavior. Ensure both exported Rust items are covered without
changing their implementation.
Source: Coding guidelines
| pub track: String, | ||
| pub timescale: u32, | ||
| pub wall: Option<u64>, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the newly exported record fields.
Add short field documentation for MoqTimeline, MoqVideo::timeline, MoqAudio::timeline, and MoqTimelineEntry. These fields are exposed through generated binding APIs.
As per coding guidelines, **/*.rs: “Document every exported Rust item.”
(Written by CodeRabbit)
Also applies to: 116-116, 127-134
🤖 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-ffi/src/media.rs` around lines 30 - 32, Document every newly exported
field involved in the timeline bindings: add concise Rust doc comments to the
fields of MoqTimeline, the MoqVideo::timeline and MoqAudio::timeline fields, and
the MoqTimelineEntry fields. Keep the descriptions short and accurate for the
generated binding API.
Source: Coding guidelines
| #[tokio::test] | ||
| async fn fetch_media_group_rejects_invalid_container_before_fetching() { | ||
| let mut broadcast = moq_net::broadcast::Info::new().produce(); | ||
| let _track = broadcast.create_track("media", None).unwrap(); | ||
| let consumer = MoqBroadcastConsumer::new(broadcast.consume()); | ||
|
|
||
| let result = consumer | ||
| .fetch_media_group( | ||
| "media".into(), | ||
| 0, | ||
| crate::media::MoqContainer::Cmaf { init: Vec::new() }, | ||
| None, | ||
| ) | ||
| .await; | ||
|
|
||
| assert!(matches!(result, Err(MoqError::Codec(_)))); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make the async failure checks deterministic.
Call tokio::time::pause() at the start of fetch_media_group_decodes_multiple_cmaf_samples.
Wrap the invalid-container request in a short tokio::time::timeout. If container validation regresses, fetch_group can wait for the dynamic group and stall this test instead of producing a direct assertion failure.
As per coding guidelines, rs/**/src/**/*.rs: “Async tests that depend on time must call tokio::time::pause() first for deterministic timer behavior.”
(Written by CodeRabbit)
Also applies to: 515-577
🤖 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-ffi/src/test.rs` around lines 497 - 512, Update the async tests around
fetch_media_group_decodes_multiple_cmaf_samples and
fetch_media_group_rejects_invalid_container_before_fetching: call
tokio::time::pause() at the start of the decoding test, and wrap the
invalid-container fetch_media_group request in a short tokio::time::timeout
before asserting the expected MoqError::Codec result.
Source: Coding guidelines
1 participant
Summary
AsyncSequenceand KotlinFlowhelpers for finite media groups and timelines.android-logcatfeature.Public API changes
All changes are additive and target
main.moq-ffiaddsMoqTimeline,MoqTimelineEntry,MoqTimelineConsumer, andMoqMediaGroupConsumer; addsMoqBroadcastConsumer.fetch_media_groupandsubscribe_timeline; and adds optionaltimelinemetadata toMoqVideoandMoqAudio.Timeline,TimelineEntry,TimelineConsumer, andMediaGroupConsumer; and addsBroadcastConsumer.fetchMediaGroupandsubscribeTimeline.Timeline,TimelineEntry,TimelineConsumer, andMediaGroupConsumeraliases; and addsframes()andentries()Flow helpers.Test plan
cargo test -p moq-fficargo test -p moq-mux import::track::testscargo fmt --all -- --checkjust kt checkjust swift checkgit diff --checkThe full Kotlin check passed before the final base refresh. Post-rebase UniFFI Kotlin generation passed, while the local shell no longer exposed the repository's required Gradle 8 executable; CI will rerun the full Kotlin matrix on the final commit.
Cross-package sync
The Kotlin and Swift wrapper READMEs and library docs are updated.
libmoq, Python, Go, and their docs are unchanged because this change does not add a C ABI or hand-written Python/Go wrapper surface.Post-Deploy Monitoring & Validation
No additional operational monitoring required. This is an additive library API change covered by FFI and language-wrapper tests.
(Written by GPT-5)