moq-rtmp/srt/rtc: convert to library-only crates#1975
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (22)
💤 Files with no reviewable changes (11)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughThis change removes the standalone Changes
Sequence Diagram(s)Not applicable — this change consists of file deletions, manifest edits, feature-gate renames, and documentation rewording without new observable runtime call flows. Compact metadata: Related issues: None specified Related PRs: None specified Suggested labels: documentation, breaking-change, refactor Suggested reviewers: None specified Poem: 🚥 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 |
|
CodeRabbit and Sourcery were both rate-limited and didn't produce a review, so I ran a local multi-angle review of the diff. Two real issues found and fixed in
Verified clean: Findings I deliberately left as-is: the (Written by Claude Opus 4.8) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/bin/cli.md`:
- Around line 232-235: The fenced code block in the CLI documentation is missing
a language identifier and is triggering the MD040 lint warning. Update the
Markdown fence around the moq-cli usage examples to include an explicit
text/plain language tag while keeping the existing content unchanged. Locate the
block by the moq-cli command examples in the CLI docs and adjust only the fence
syntax.
- Around line 246-281: The RTMP server examples in the CLI docs use the wrong
flag ordering for the command hierarchy: `--bind` and `--tls-generate` must be
shown before the `import` subcommand on `moq-cli rtmp server`, while the
ingest-specific flags like `--rtmp-listen` and `--rtmp-prefix` stay after
`import`. Update the example invocations in the RTMP section to match the
existing `client` pattern and ensure the `server` command parses correctly.
In `@rs/moq-cli/src/rtc.rs`:
- Around line 193-229: The current run_client flow only awaits ctrl_c after
calling Client::subscribe or Client::publish, so it never notices later WebRTC
session shutdowns. Update the session handling in run_client to keep the session
future or a closed handle alive and wait on it together with ctrl_c so
ICE/DTLS/media-path termination is surfaced and the CLI exits when the session
dies; use the existing client.subscribe, client.publish, and run_client symbols
to locate the change.
In `@rs/moq-srt/Cargo.toml`:
- Around line 18-31: The moq_srt crate is still declaring shared libraries
locally instead of consuming them from the workspace. Add anyhow, bytes,
futures, srt-tokio, thiserror, and tracing to the workspace dependency table,
then update this crate’s entries in Cargo.toml to use { workspace = true } for
those symbols so the package consistently shares versions with the rest of the
workspace.
🪄 Autofix (Beta)
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
Run ID: 38982079-5291-4829-868f-74970e044cec
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
Cargo.tomldoc/.vitepress/config.tsdoc/bin/cli.mddoc/bin/index.mddoc/bin/rtc.mddoc/bin/rtmp.mdrs/CLAUDE.mdrs/moq-cli/Cargo.tomlrs/moq-cli/src/main.rsrs/moq-cli/src/rtc.rsrs/moq-cli/src/rtmp.rsrs/moq-cli/src/server.rsrs/moq-cli/src/srt.rsrs/moq-rtc/Cargo.tomlrs/moq-rtc/bin/moq-rtc.rsrs/moq-rtc/src/lib.rsrs/moq-rtmp/Cargo.tomlrs/moq-rtmp/README.mdrs/moq-rtmp/bin/moq-rtmp.rsrs/moq-rtmp/bin/serve.rsrs/moq-rtmp/bin/web.rsrs/moq-rtmp/src/lib.rsrs/moq-rtmp/src/listen.rsrs/moq-rtmp/src/server.rsrs/moq-srt/Cargo.tomlrs/moq-srt/README.mdrs/moq-srt/bin/moq-srt.rsrs/moq-srt/bin/serve.rsrs/moq-srt/bin/web.rsrs/moq-srt/src/lib.rs
💤 Files with no reviewable changes (10)
- doc/bin/rtc.md
- rs/moq-srt/bin/web.rs
- rs/moq-srt/bin/serve.rs
- doc/bin/rtmp.md
- rs/moq-rtc/bin/moq-rtc.rs
- doc/.vitepress/config.ts
- rs/moq-rtmp/bin/moq-rtmp.rs
- rs/moq-srt/bin/moq-srt.rs
- rs/moq-rtmp/bin/serve.rs
- rs/moq-rtmp/bin/web.rs
| /// Dial a remote WHEP (import) / WHIP (export) resource at `url`. | ||
| async fn run_client( | ||
| import: bool, | ||
| url: Url, | ||
| broadcast_name: &str, | ||
| public_addr: Vec<SocketAddr>, | ||
| publisher: moq_net::OriginProducer, | ||
| subscriber: moq_net::OriginConsumer, | ||
| ) -> anyhow::Result<()> { | ||
| let mut config = moq_rtc::client::Config::default(); | ||
| config.ice_candidates = public_addr; | ||
| let client = Client::new(config); | ||
|
|
||
| if import { | ||
| // WHEP client: receive remote RTP, publish it as `broadcast_name`. The | ||
| // announcement lives as long as `broadcast` (moved into the subscribe task) | ||
| // and is withdrawn when that producer closes. | ||
| let broadcast = moq_net::Broadcast::new().produce(); | ||
| let consumer = broadcast.consume(); | ||
| if !publisher.publish_broadcast(broadcast_name, consumer) { | ||
| anyhow::bail!("failed to publish broadcast {broadcast_name}"); | ||
| } | ||
| client.subscribe(url, broadcast).await?; | ||
| } else { | ||
| // WHIP client: read the local broadcast, push it as RTP to the remote. | ||
| let broadcast = subscriber | ||
| .request_broadcast(broadcast_name) | ||
| .await | ||
| .map_err(|_| anyhow::anyhow!("broadcast {} not announced", broadcast_name))?; | ||
| client.publish(url, broadcast).await?; | ||
| } | ||
|
|
||
| // `client.subscribe` / `publish` spawn the session in the background; block on | ||
| // ctrl-c instead of returning so the gateway stays up. | ||
| tokio::signal::ctrl_c().await?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and symbols.
git ls-files 'rs/moq-cli/src/*.rs' 'rs/moq-rtc/src/*.rs' | sed -n '1,120p'
printf '\n--- rtc.rs outline ---\n'
ast-grep outline rs/moq-cli/src/rtc.rs --view expanded
printf '\n--- search subscribe/publish APIs ---\n'
rg -n "async fn (subscribe|publish)\b|struct .*Session|fn closed\b|closed\(" rs/moq-rtc rs/moq-cli -g '!**/target/**'
printf '\n--- search run_client in related transports ---\n'
rg -n "async fn run_client|reconnect\.closed\(|tokio::select!" rs/moq-cli/src -g '!**/target/**'Repository: moq-dev/moq
Length of output: 4461
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant slices once we know the files.
sed -n '1,260p' rs/moq-cli/src/rtc.rs
printf '\n--- moq-rtc client files ---\n'
fd -a 'client.rs' rs/moq-rtc rs -t fRepository: moq-dev/moq
Length of output: 8737
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the CLI call site and the rtc client API slices.
sed -n '1,260p' rs/moq-cli/src/rtc.rs
printf '\n--- moq-rtc files ---\n'
git ls-files 'rs/moq-rtc/src/**/*.rs' 'rs/moq-rtc/src/*.rs' | sed -n '1,120p'
printf '\n--- search for Client and session/closed APIs ---\n'
rg -n "struct Client|impl Client|async fn (subscribe|publish)\b|fn closed\b|closed\(" rs/moq-rtc/src rs/moq-cli/src -g '!**/target/**'Repository: moq-dev/moq
Length of output: 10328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- rs/moq-rtc/src/client/mod.rs ---'
sed -n '1,220p' rs/moq-rtc/src/client/mod.rs
printf '\n%s\n' '--- rs/moq-rtc/src/client/whep.rs ---'
sed -n '1,260p' rs/moq-rtc/src/client/whep.rs
printf '\n%s\n' '--- rs/moq-rtc/src/client/whip.rs ---'
sed -n '1,260p' rs/moq-rtc/src/client/whip.rs
printf '\n%s\n' '--- rs/moq-rtc/src/session.rs ---'
sed -n '1,260p' rs/moq-rtc/src/session.rsRepository: moq-dev/moq
Length of output: 18207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any closed/termination API wired into moq_rtc::Client callers.
rg -n "\.subscribe\(|\.publish\(|closed\(\)" rs/moq-cli rs/moq-rtc -g '!**/target/**'Repository: moq-dev/moq
Length of output: 1159
Surface WebRTC session termination. client.subscribe/publish only return after spawning the session task, so run_client can’t observe later ICE/DTLS failure and just waits on ctrl_c(). Return a closed handle or keep the session future in the select so the CLI exits when the media path dies.
🤖 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-cli/src/rtc.rs` around lines 193 - 229, The current run_client flow
only awaits ctrl_c after calling Client::subscribe or Client::publish, so it
never notices later WebRTC session shutdowns. Update the session handling in
run_client to keep the session future or a closed handle alive and wait on it
together with ctrl_c so ICE/DTLS/media-path termination is surfaced and the CLI
exits when the session dies; use the existing client.subscribe, client.publish,
and run_client symbols to locate the change.
There was a problem hiding this comment.
Leaving this as-is for now: it's ported verbatim from the old moq-rtc bin (which also just blocked on ctrl-c after spawning the session). Surfacing later ICE/DTLS/media-path death would need moq_rtc::Client::{subscribe,publish} to hand back a closed/session handle to select! on, i.e. a moq-rtc API change, which is out of scope for this binary-move PR. Better as a focused follow-up than folded in here.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
@kixelated Makes sense — agreed this is better scoped as a follow-up once moq_rtc::Client::{subscribe,publish} can hand back a closed/session handle. I won't press further on this in the current PR.
Let me know if you'd like me to open a follow-up issue to track adding a closed/session handle to moq_rtc::Client so run_client in rs/moq-cli/src/rtc.rs can select on ICE/DTLS/media-path termination.
🧠 Learnings used
Learnt from: kixelated
Repo: moq-dev/moq PR: 1663
File: rs/moq-relay/src/auth.rs:3036-3038
Timestamp: 2026-06-09T15:16:38.658Z
Learning: In Rust, a `matches!(value, SomeEnum::Variant(_))`-style check where every inner sub-pattern is a wildcard (`_`) does not bind/move any data from `value` (no variables are captured). Therefore, if `value` is still usable after the `matches!` call, suggestions to change the argument to `&value` are likely incorrect/false positives. Only recommend borrowing if the pattern actually binds data (e.g., `Variant(v)`), or if the surrounding code would otherwise cause a move.
Learnt from: kixelated
Repo: moq-dev/moq PR: 1901
File: rs/moq-native/src/tls.rs:116-121
Timestamp: 2026-06-24T15:58:02.567Z
Learning: When reviewing Rust diffs that change a `#[arg(...)]` attribute (e.g., Clap/derive-style CLI args), don’t treat an `env` value rename as a breaking change based solely on reformatting. If a single-line attribute like `#[arg(id = "x", long = "x", env = "MOQ_CLIENT_TLS_ROOT")]` becomes a multiline block, the diff may show the `env = "..."` only on the `+` side. Before flagging a breaking `env`-var rename, compare both the pre- and post-change sides of the diff and confirm the `env` value is identical; only flag if the actual `env` string differs.
Learnt from: kixelated
Repo: moq-dev/moq PR: 1914
File: rs/moq-native/src/tls.rs:80-83
Timestamp: 2026-06-26T05:18:14.127Z
Learning: When reviewing added variants on public Rust enums (especially when the diff hunk is narrow), inspect the full enum declaration (including attributes) before flagging a semver break. If the enum is marked with #[non_exhaustive], adding new variants is non-breaking; only flag semver breaks for exhaustively-typed public enums where downstream pattern matches could be impacted. Example: moq_native::tls::Error in rs/moq-native/src/tls.rs is already #[non_exhaustive], so adding Error::ClientVerifier should not be treated as a semver break.
Learnt from: kixelated
Repo: moq-dev/moq PR: 1914
File: rs/moq-rtmp/src/error.rs:4-6
Timestamp: 2026-06-26T05:22:00.373Z
Learning: When reviewing Rust code in this repo for missing rustdoc on a public item, don’t rely solely on the diff hunk context. If the diff hunk starts at attributes (e.g., `#[derive(...)]`) for a public item, first inspect the full item declaration above those attributes: the type-level doc comment (e.g., `/// ...`) may already exist immediately above the attributes but fall outside the shown diff window. Only flag missing rustdoc if the complete item lacks the appropriate rustdoc comment.
| #[derive(Args, Clone)] | ||
| pub struct Endpoint { | ||
| /// Run a WHIP/WHEP server on this HTTP address. | ||
| #[arg(long = "rtc-listen", env = "MOQ_RTC_LISTEN", conflicts_with = "remote")] |
There was a problem hiding this comment.
Make a client/server command for RTC, sorry.
| udp_bind: SocketAddr, | ||
|
|
||
| /// Optional TLS cert (PEM) for the WHIP/WHEP server. Requires `--rtc-tls-key`. | ||
| #[arg(long = "rtc-tls-cert", env = "MOQ_RTC_TLS_CERT", requires = "tls_key")] |
There was a problem hiding this comment.
don't include --rtc in the long, could you have id = rtc-tls-cert instead?
Or ideally use moq_native::tls
| } | ||
|
|
||
| /// Run a WHIP (import) / WHEP (export) server on `listen`. | ||
| #[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
pls just make a Server struct
| } | ||
|
|
||
| /// Dial a remote WHEP (import) / WHIP (export) resource at `url`. | ||
| async fn run_client( |
There was a problem hiding this comment.
plus just make a Client struct
| /// RTMP -> MoQ: ingest pushed RTMP streams and publish them into MoQ. | ||
| Import { | ||
| #[command(flatten)] | ||
| rtmp: RtmpArgs, |
There was a problem hiding this comment.
If these args are shared, should they be part of RtmpCommand (or higher?)
| /// Address to listen on for plaintext RTMP (`rtmp://`). RTMP's well-known port | ||
| /// is 1935. | ||
| #[arg(long = "rtmp-listen", env = "MOQ_RTMP_LISTEN", default_value = "[::]:1935")] | ||
| listen: SocketAddr, |
There was a problem hiding this comment.
This should only be for the Server command
| rtmps_cert: Option<PathBuf>, | ||
|
|
||
| /// PEM private key for RTMPS. | ||
| #[arg(long = "rtmps-tls-key", env = "MOQ_RTMPS_TLS_KEY")] |
There was a problem hiding this comment.
Try to use moq_native::tls
Remove the standalone `moq-rtmp`, `moq-srt`, and `moq-rtc` binaries and slim the three crates down to libraries. This is the first step of moving the gateway CLIs into `moq-cli`; the CLI surface itself is a follow-up (a unified endpoint model landing alongside RTMP/SRT dial-out client support). - Delete the standalone binaries and their `bin/` helpers. - Drop binary-only deps (moq-native, clap, axum-server, sd-notify, tower-http, the transport-backend feature forwards). - moq-rtmp: the `server` feature gated real library code (RTMPS: `Conn::Tls`, `Server::with_tls`, `Config::tls`, `pub use rustls`) so rename it to `tls` (`default = ["tls"]`); `moq-native` becomes a dev-dependency for the RTMPS test. - moq-srt / moq-rtc: their `server` feature gated nothing in the library, so drop it entirely (`default = []`). - Docs: remove the standalone-binary pages (doc/bin/rtmp.md, rtc.md) and their nav entries; crate READMEs and rs/CLAUDE.md now describe the libraries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kixelated
force-pushed
the
claude/gallant-heisenberg-f95256
branch
from
b84b4b2 to
d466e64
Compare
kixelated
changed the title
1 participant
Converts the three gateway crates —
moq-rtmp,moq-srt,moq-rtc— from lib+bin into library-only crates, removing their standalone binaries.This is the first step of moving the gateway CLIs into
moq-cli. The CLI surface itself is intentionally not in this PR: after design discussion it's being reshaped into a unified endpoint model (protocol-prefixed--rtmp-connect/--srt-listen/… flags +import/export), which lands in a follow-up PR together with new RTMP/SRT dial-out (client) library support.What changed
moq-rtmp,moq-srt,moq-rtc) and theirbin/helpers.moq-native,clap,axum-server,sd-notify,tower-http, and thenoq/quinn/quiche/websocket/irohtransport-backend feature forwards).moq-rtmp: theserverfeature gated real library code (RTMPS — theConn::Tlsvariant,Server::with_tls,Config::tls,pub use rustls), so it's renamed totls(default = ["tls"]).moq-nativebecomes a dev-dependency for the RTMPS test.moq-srt/moq-rtc: theirserverfeature gated nothing in the library, so it's removed entirely (default = []).doc/bin/rtmp.md,doc/bin/rtc.md) and their nav entries; the crate READMEs andrs/CLAUDE.mdnow describe the libraries.Public API
moq-rtmp: featureserver→tls(rename). Nopubitem changes.moq-srt/moq-rtc:serverfeature + transport forwards removed; nopubitem changes.0.0.1crates with no external consumers; the gateway library APIs (run,Server/Request,Client) are untouched.Test plan
cargo check --all-targets,cargo clippy -- -D warnings,cargo fmt --check, rustdoc (-D warnings).cargo test -p moq-rtmp(incl. the RTMPS test, now built via themoq-nativedev-dependency),-p moq-srt,-p moq-rtc.(Written by Claude Opus 4.8)