moq-rtmp/moq-srt: add client (dial-out) role#1982
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
Both crates previously ran only a listener (server) role. This adds the mirror dial-out (client) role so a caller can bridge a remote RTMP/SRT server to a MoQ origin in either direction, reusing the existing FLV / MPEG-TS <-> moq-mux plumbing; only the client transport is new. moq-rtmp: `Client::connect` / `Client::with_stream` run the RTMP client handshake + connect, then `Client::publish` pushes a MoQ broadcast out to the remote (restream) and `Client::pull` plays a remote stream into an origin (ingest). Generic over the `Stream` trait for rtmps:// / BYO transport. Loopback test round-trips a broadcast through the crate's own server. moq-srt: `dial::publish` / `dial::pull` free functions dial as an SRT caller with the standard `#!::r=<resource>,m=publish|request` stream id and reuse the server's `serve_publish` / `serve_subscribe` (now pub(crate)). Loopback tests prove the caller handshake and m= mode routing. Library-level only: not yet wired to a CLI. Both dial paths compile without the `server` feature. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kixelated
force-pushed
the
claude/rtmp-srt-client
branch
from
f156295 to
6536994
Compare
|
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 selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughChangesThis pull request adds dial-out client modules to the RTMP and SRT crates. The RTMP crate now exposes a Compact Metadata 🚥 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: 2
🧹 Nitpick comments (1)
rs/moq-srt/src/dial.rs (1)
36-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftPrefer an options/config type before freezing these new public entrypoints.
publishandpullare brand-new public APIs, but each already takes five positional parameters. Any near-term additions like auth fields, raw stream-id overrides, reconnect policy, or timeout/TLS knobs will now force signature churn. Please wrap these inputs in a small config type or builder while this surface is still new.As per coding guidelines, "Before exposing a new public type, function, field, or enum variant, prefer smaller, more future-proof API shapes such as config structs, options objects, enums/newtypes, typestate/builder patterns, owned teardown handles, and non-exhaustive public types when appropriate."
🤖 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-srt/src/dial.rs` around lines 36 - 63, The new public entrypoints in `publish` and `pull` are too parameter-heavy and should be reshaped before they harden into stable APIs. Refactor these functions to accept a small config/options type or builder that groups `addr`, `resource`, `latency`, `path`, and the `OriginConsumer`/`OriginProducer` target, so future additions like auth, stream-id overrides, reconnect, or timeout/TLS settings won’t require signature churn. Keep the existing behavior in `call`, `serve_subscribe`, and `serve_publish`, but move the public surface toward a more extensible shape.Source: Coding guidelines
🤖 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 `@rs/moq-rtmp/src/dial.rs`:
- Around line 89-93: Preserve the leftover handshake bytes in dial::connect_flow
around client_handshake and ClientSession::new, since
HandshakeProcessResult::Completed may include remaining_bytes that must not be
dropped. Update client_handshake to return those bytes, then after creating the
session, feed them into session.handle_input() before processing the initial
work queue so the RTMP stream stays aligned.
In `@rs/moq-srt/src/dial.rs`:
- Around line 70-72: The SRT stream id built in call() is taking resource
directly into the #!::r=...,m=... string, which allows delimiter characters to
break routing. Update call() to sanitize/escape resource before passing it into
format!, or reject invalid values up front, and keep the m component unchanged
via mode.as_str(). Ensure the fix is applied at the stream_id construction site
in call().
---
Nitpick comments:
In `@rs/moq-srt/src/dial.rs`:
- Around line 36-63: The new public entrypoints in `publish` and `pull` are too
parameter-heavy and should be reshaped before they harden into stable APIs.
Refactor these functions to accept a small config/options type or builder that
groups `addr`, `resource`, `latency`, `path`, and the
`OriginConsumer`/`OriginProducer` target, so future additions like auth,
stream-id overrides, reconnect, or timeout/TLS settings won’t require signature
churn. Keep the existing behavior in `call`, `serve_subscribe`, and
`serve_publish`, but move the public surface toward a more extensible shape.
🪄 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: 06eb4806-fb56-4454-bdde-82ec0d38672b
📒 Files selected for processing (5)
rs/moq-rtmp/src/dial.rsrs/moq-rtmp/src/lib.rsrs/moq-srt/src/dial.rsrs/moq-srt/src/lib.rsrs/moq-srt/src/server.rs
…tream-id) - moq-rtmp: surface a refused publish/play. rml_rtmp reports it as an onStatus failure code (UnhandleableOnStatusCode), not a Rejected event, so the client hung instead of erroring. Fail on error codes (Failed/NotFound/BadName/Denied/ Rejected/Unauthorized) while ignoring benign progress codes (e.g. NetStream.Play.Reset, which precedes .Start). - moq-rtmp: send a tcUrl (rtmp://host/app) on connect. YouTube/Twitch/some nginx-rtmp configs reject a connect without one; rml_rtmp only emits it when ClientSessionConfig::tc_url is set. - moq-rtmp: preserve the handshake's remaining_bytes and feed them to the session. The final handshake read can carry the first RTMP chunks; dropping them desyncs chunk parsing. - moq-srt: reject a resource containing ',' or '=' (the #!::r=…,m=… stream-id delimiters) instead of emitting a corrupt, misrouting stream id. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Since the bots were rate-limited when this opened, I also ran a local correctness review of the two
On the options-struct nitpick (5-param (Written by Claude Opus 4.8) |
1 participant
Adds the dial-out (client) role to
moq-rtmpandmoq-srt, which previously could only listen. A caller can now bridge a remote RTMP/SRT server to a MoQ origin in either direction, reusing the existing FLV / MPEG-TS <->moq-muxplumbing — only the client transport is new. Library-only; no CLI yet.API
moq-rtmp (
moq_rtmp::Client):connect(addr, app)(orwith_streamforrtmps:/// BYO transport), then.publish(key, broadcast)(MoQ -> remote, restream) or.pull(key, &origin, path)(remote -> MoQ, ingest). Two-phase because RTMPconnectis direction-agnostic; direction = the method you call. Generic over the crate'sStreamtrait.moq-srt (
moq_srt::dial):publish(addr, resource, latency, &consumer, path)andpull(addr, resource, latency, &producer, path). Free functions — the SRTm=mode is fixed at dial time (#!::r=<resource>,m=publish|request). Reuses the server'sserve_publish/serve_subscribe(nowpub(crate)).Why now
This is the "client half" the upcoming unified
moq-cliendpoint CLI needs for--rtmp-connect/--srt-connect. Landing it as its own library PR keeps that CLI PR focused. (WebRTC already had a dial-outClient; this brings RTMP/SRT to parity.)Test plan
moq-rtmp: loopback test dials the crate's ownServer, plays a broadcast, republishes it end-to-end. 16 tests pass.moq-srt: two loopback tests prove the caller handshake andm=publish/m=requestrouting. 9 tests pass.cargo clippy -- -D warnings,cargo fmt; both dial paths compile with--no-default-features.(Written by Claude Opus 4.8)