feat(bindings): expose media timelines - #2764
Conversation
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
|
Taking this over and putting it on hold for now. The current branch exposes timeline metadata/subscriptions and retained media-group fetching through The existing review findings can remain open until we resume the work. |
e9ef8d3 to
308ed39
Compare
kixelated
changed the title
308ed39 to
fe03c59
Compare
|
Rebased onto current The retained media-group fetch/decode work already merged in #2827, so I removed that duplicated implementation during conflict resolution. I also dropped the unrelated Android logcat commit and reworded the remaining four commits around the timeline-only scope. Validation passed with |
2 participants
Summary
moq-ffi.AsyncSequenceand KotlinFlowwrappers for timeline entries.Status
HOLD: Keep this PR in draft until the timeline model and DVR behavior are finalized. The public API may need to change as that design settles.
Public API changes
moq-ffiaddsMoqTimeline,MoqTimelineEntry,MoqTimelineConsumer, andMoqBroadcastConsumer.subscribe_timeline.MoqVideoandMoqAudioexpose optional catalog timeline metadata.Timeline,TimelineEntry,TimelineConsumer, andBroadcastConsumer.subscribeTimeline.entries()Flow helper.Validation
nix develop --command just checknix develop --command just rs test -p moq-ffi(58 tests passed)Cross-package sync
Swift and Kotlin wrappers and their documentation are updated. Python, Go, and C wrapper work is deferred until the timeline and DVR API is finalized.