Conversation
📋 PR Review Helper📱 Mobile App Build✅ Ready to test! (commit 🕶️ ASG Client Build✅ Ready to test! (commit 🔀 Test Locallygh pr checkout 2246 |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
|
Hey awesome work so far. A few things to note:
Right now createLiveInput() computes the real RTMP URL into a local variable, then throws it away and stuffs the SRT URL into the rtmpUrl field:
This means managed streaming can never send an actual RTMP URL anymore. Every other part of the code reads rtmpUrl expecting RTMP but gets srt://.... which works but isn't documented well.
Here's how I'd finish the PR (probably just feed this to claude and it can 1shot... lmk what you think): Streaming Protocol Refactor: Unified Multi-Protocol SupportContextThe current streaming pipeline was built entirely around RTMP — every field is named We need to properly support 5 streaming modes:
WebRTC runs only on This is a clean break. All RTMP-specific naming is replaced. No deprecated aliases, no backwards compat shims. SDK Public API// Direct streaming — you provide the URL, glasses push directly to it
session.camera.startDirectStream({
streamUrl: "rtmp://your-server.com/live/key", // or srt:// or https:// (WHIP)
video?: VideoConfig,
audio?: AudioConfig,
sound?: boolean,
})
// Livestreaming — Cloudflare handles infra, restreaming, HLS/DASH playback
session.camera.startLivestream({
quality?: "720p" | "1080p",
enableWebRTC?: boolean, // use WHIP to Cloudflare instead of SRT
restreamDestinations?: [ // Cloudflare restreams to these
{ url: "rtmp://youtube.com/live/KEY", name: "YouTube" },
{ url: "rtmp://twitch.tv/app/KEY", name: "Twitch" },
],
video?: VideoConfig,
audio?: AudioConfig,
sound?: boolean,
})
// Returns: { hlsUrl, dashUrl, webrtcPlaybackUrl?, streamId }
session.camera.stopDirectStream()
session.camera.stopLivestream()Design PrincipleThe ArchitectureLivestream (Managed) FlowCloud picks ingest protocol (SRT by default, WHIP if Changes by LayerLayer 1: SDK Types (
|
| File | Change |
|---|---|
cloud/packages/sdk/src/types/message-types.ts |
Replace START_RTMP_STREAM → START_STREAM, etc. |
cloud/packages/sdk/src/types/messages/cloud-to-glasses.ts |
Replace StartRtmpStream → StartStream |
cloud/packages/sdk/src/types/messages/glasses-to-cloud.ts |
Replace RtmpStreamStatus → StreamStatus |
cloud/packages/sdk/src/types/messages/app-to-cloud.ts |
RtmpStreamRequest → DirectStreamRequest, ManagedStreamRequest → LivestreamRequest |
cloud/packages/sdk/src/types/messages/cloud-to-app.ts |
ManagedStreamStatus → LivestreamStatus |
cloud/packages/sdk/src/app/session/modules/camera.ts |
startRtmpStream → startDirectStream, startManagedStream → startLivestream |
cloud/packages/sdk/src/app/session/modules/camera-managed-extension.ts |
Rename to camera-livestream-extension.ts |
cloud/packages/cloud/src/services/streaming/CloudflareStreamService.ts |
Fix LiveInputResult, return all URLs |
cloud/packages/cloud/src/services/streaming/ManagedStreamingExtension.ts |
Rename to LivestreamExtension.ts, use StartStream |
cloud/packages/cloud/src/services/session/UnmanagedStreamingExtension.ts |
Rename to DirectStreamingExtension.ts, use StartStream |
cloud/packages/cloud/src/services/session/handlers/glasses-message-handler.ts |
Rename handler, use stream_status |
cloud/packages/cloud/src/services/streaming/StreamRegistry.ts |
Update field names |
mobile/src/services/SocketComms.ts |
Replace handle_start_rtmp_stream → handle_start_stream |
mobile/modules/core/src/CoreModule.ts |
Replace startRtmpStream → startStream |
mobile/modules/core/android/.../CoreModule.kt |
Replace bridge functions |
mobile/modules/core/android/.../CoreManager.kt |
Replace delegation methods |
asg_client/.../handlers/StreamCommandHandler.java |
New file, replaces RtmpCommandHandler |
asg_client/.../handlers/RtmpCommandHandler.java |
Delete |
asg_client/.../services/RtmpStreamingService.java |
Rename mRtmpUrl → mStreamUrl |
asg_client/.../services/WebRtcStreamingService.java |
Wire into StreamCommandHandler |
asg_client/.../utils/ServiceConstants.java |
Replace RTMP constants |
Verification
- SDK build:
cd cloud && bun run build— verify no type errors - Cloud lint:
cd cloud/packages/cloud && bun run lint - Cloud tests:
cd cloud && bun run test - asg_client build:
cd asg_client && ./gradlew assembleDebug - Grep for remnants:
rg "rtmpUrl|startRtmpStream|START_RTMP|RTMP_STREAM|ManagedStream|UnmanagedStream" --type ts --type java --type kotlin— should return zero hits outside of comments/docs - Manual: Test each protocol with asg_client test scripts
Deploying mentra-live-ota-site with
|
| Latest commit: |
c4f179f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c6411b9f.mentra-live-ota-site.pages.dev |
| Branch Preview URL: | https://ya-replace-rtmp-with-srt.mentra-live-ota-site.pages.dev |
Deploying mentra-store-dev with
|
| Latest commit: |
c4f179f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://47f5953d.augmentos-appstore-2.pages.dev |
| Branch Preview URL: | https://ya-replace-rtmp-with-srt.augmentos-appstore-2.pages.dev |
Deploying dev-augmentos-console with
|
| Latest commit: |
c4f179f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://567928ff.dev-augmentos-console.pages.dev |
| Branch Preview URL: | https://ya-replace-rtmp-with-srt.dev-augmentos-console.pages.dev |
Deploying prod-augmentos-account with
|
| Latest commit: |
c4f179f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5817ebf0.augmentos-e84.pages.dev |
| Branch Preview URL: | https://ya-replace-rtmp-with-srt.augmentos-e84.pages.dev |
71afd75 to
0bf1250
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4f179faa2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| type: CloudToGlassesMessageType.START_STREAM, | ||
| sessionId: userSession.sessionId, | ||
| rtmpUrl: liveInput.rtmpUrl, // Cloudflare ingest URL | ||
| streamUrl: liveInput.srtUrl!, // Cloudflare ingest URL |
There was a problem hiding this comment.
Guard against missing SRT ingest URL before start
startManagedStream always sends streamUrl: liveInput.srtUrl!, but CloudflareStreamService.createLiveInput() makes srtUrl optional and can return undefined when Cloudflare does not provide SRT fields. In that case we send an invalid START_STREAM payload to glasses, so managed streaming fails to start even though an ingest URL (rtmpUrl) is available. Add an explicit null check with fallback/error handling before building the start message.
Useful? React with 👍 / 👎.
| String streamId = getActiveStreamId(); | ||
| if (streamId != null && !streamId.isEmpty()) status.put("streamId", streamId); |
There was a problem hiding this comment.
Use source service stream IDs in status callbacks
The shared callback tags status events with getActiveStreamId(), which picks whichever service currently has an ID (RTMP, then SRT, then WHIP). During protocol switches, a delayed stop/error callback from the previous service can therefore be emitted with the new stream's ID, causing cloud-side state to mark the wrong stream as stopped/errored. Stream IDs should come from the emitting service (or per-service callbacks), not global active-service lookup.
Useful? React with 👍 / 👎.
| } else if (WhipStreamingService.isStreaming()) { | ||
| WhipStreamingService.stopStreaming(context); |
There was a problem hiding this comment.
Stop WHIP sessions even when reconnecting
handleStopCommand only stops WHIP when WhipStreamingService.isStreaming() is true, but that method returns false during reconnecting/starting states. If a user sends stop_stream while WHIP is reconnecting, the handler reports not_streaming and leaves the reconnect loop running, so the stream can come back unexpectedly. Include reconnecting/starting states (or stop unconditionally) for WHIP stop handling.
Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.