Use typed ordered::Consumer for video/audio in moq-ffi#1163
Use typed ordered::Consumer for video/audio in moq-ffi#1163Qizot wants to merge 1 commit intomoq-dev:mainfrom
Conversation
Replace the generic `subscribe_media` with typed `subscribe_video` and `subscribe_audio` methods that accept catalog config and use `moq_mux::ordered::Consumer<T>` directly. Also fix CMAF PTS calculation to use DTS + CTS offset for correct timestamp decoding. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
WalkthroughThe pull request refactors the media consumer API to differentiate between video and audio streams. The 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ 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: 1
🧹 Nitpick comments (1)
rs/moq-mux/src/cmaf/container.rs (1)
45-46: Add a direct regression test for non-zero CTS offsets.This is the right fix, but none of the shown tests exercise a CMAF sample where
cts != 0, so a future regression back to raw DTS handling would slip through. A focused decode test here with a positive CTS offset would make this change much safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rs/moq-mux/src/cmaf/container.rs` around lines 45 - 46, Add a regression test that constructs/decodes a CMAF sample with a non-zero CTS offset and asserts the resulting Timestamp equals DTS + CTS (not raw DTS); specifically exercise the pts calculation that uses "let pts = dts as i64 + entry.cts.unwrap_or(0) as i64" and the call to Timestamp::from_scale so future changes don't regress to using raw dts. Create a focused unit test that sets entry.cts to a positive value, runs the same decode/path used by the existing CMAF tests, and assert the produced timestamp (or sample PTS) equals the expected scaled value computed from dts + cts. Ensure the test fails if code ignores entry.cts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rs/moq-ffi/src/media.rs`:
- Around line 33-51: The current TryFrom implementations for MoqVideo ->
hang::catalog::VideoConfig (and the analogous MoqAudio ->
hang::catalog::AudioConfig) drop timing metadata by hardcoding jitter: None and
optimize_for_latency: None; update the FFI types MoqVideo and MoqAudio to
include jitter and optimize_for_latency fields, ensure convert_catalog()
populates those fields from the importer, and change the TryFrom implementations
to map MoqVideo.jitter -> hang::catalog::VideoConfig.jitter and
MoqVideo.optimize_for_latency -> optimize_for_latency (and the analogous
MoqAudio mappings) instead of setting them to None so the rebuilt configs
preserve computed jitter/latency preferences when fed into
moq_mux::ordered::Consumer.
---
Nitpick comments:
In `@rs/moq-mux/src/cmaf/container.rs`:
- Around line 45-46: Add a regression test that constructs/decodes a CMAF sample
with a non-zero CTS offset and asserts the resulting Timestamp equals DTS + CTS
(not raw DTS); specifically exercise the pts calculation that uses "let pts =
dts as i64 + entry.cts.unwrap_or(0) as i64" and the call to
Timestamp::from_scale so future changes don't regress to using raw dts. Create a
focused unit test that sets entry.cts to a positive value, runs the same
decode/path used by the existing CMAF tests, and assert the produced timestamp
(or sample PTS) equals the expected scaled value computed from dts + cts. Ensure
the test fails if code ignores entry.cts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c675377-0946-40e0-8b60-b5bc0dbdd9de
📒 Files selected for processing (4)
rs/moq-ffi/src/consumer.rsrs/moq-ffi/src/media.rsrs/moq-ffi/src/test.rsrs/moq-mux/src/cmaf/container.rs
| impl TryFrom<MoqVideo> for hang::catalog::VideoConfig { | ||
| type Error = crate::error::MoqError; | ||
|
|
||
| fn try_from(v: MoqVideo) -> Result<Self, Self::Error> { | ||
| Ok(Self { | ||
| codec: v.codec.parse().map_err(|e: hang::Error| crate::error::MoqError::Codec(e.to_string()))?, | ||
| description: v.description.map(bytes::Bytes::from), | ||
| coded_width: v.coded.as_ref().map(|d| d.width), | ||
| coded_height: v.coded.as_ref().map(|d| d.height), | ||
| display_ratio_width: v.display_ratio.as_ref().map(|d| d.width), | ||
| display_ratio_height: v.display_ratio.as_ref().map(|d| d.height), | ||
| bitrate: v.bitrate, | ||
| framerate: v.framerate, | ||
| optimize_for_latency: None, | ||
| container: v.container.into(), | ||
| jitter: None, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- New FFI TryFrom impls ---\n'
sed -n '33,67p' rs/moq-ffi/src/media.rs
printf '\n--- FFI catalog export path ---\n'
sed -n '107,165p' rs/moq-ffi/src/media.rs
printf '\n--- VideoConfig semantics ---\n'
sed -n '130,150p' rs/hang/src/catalog/video/mod.rs
printf '\n--- AudioConfig semantics ---\n'
sed -n '88,100p' rs/hang/src/catalog/audio/mod.rs
printf '\n--- fMP4 importer computing jitter ---\n'
sed -n '639,667p' rs/moq-mux/src/import/fmp4.rsRepository: moq-dev/moq
Length of output: 5101
Expose and preserve jitter and optimize_for_latency in FFI conversions.
The TryFrom<MoqVideo> and TryFrom<MoqAudio> implementations lose timing metadata by hardcoding jitter: None and optimize_for_latency: None. This is problematic because:
- The internal catalog types (
hang::catalog::VideoConfig/AudioConfig) explicitly document these fields and their semantics (e.g., jitter defaults to None means "flush immediately", but can be set to frame duration, b-frame budget, or segment duration). - The fMP4 importer actively computes and updates jitter based on fragment boundaries and frame rates.
- Your PR now feeds these rebuilt configs directly into
moq_mux::ordered::Consumer, so the loss changes playback/buffering behavior for tracks with computed jitter or latency preferences.
The FFI types MoqVideo and MoqAudio must expose these fields. Then populate them in convert_catalog() and carry them through the TryFrom implementations instead of hardcoding None.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rs/moq-ffi/src/media.rs` around lines 33 - 51, The current TryFrom
implementations for MoqVideo -> hang::catalog::VideoConfig (and the analogous
MoqAudio -> hang::catalog::AudioConfig) drop timing metadata by hardcoding
jitter: None and optimize_for_latency: None; update the FFI types MoqVideo and
MoqAudio to include jitter and optimize_for_latency fields, ensure
convert_catalog() populates those fields from the importer, and change the
TryFrom implementations to map MoqVideo.jitter ->
hang::catalog::VideoConfig.jitter and MoqVideo.optimize_for_latency ->
optimize_for_latency (and the analogous MoqAudio mappings) instead of setting
them to None so the rebuilt configs preserve computed jitter/latency preferences
when fed into moq_mux::ordered::Consumer.
1 participant
Replace the generic
subscribe_mediawith typedsubscribe_videoandsubscribe_audiomethods that accept catalog config and usemoq_mux::ordered::Consumer<T>directly. Also fix CMAF PTS calculation to use DTS + CTS offset for correct timestamp decoding.