Conversation
WalkthroughThis pull request refactors the MOQ output module from a broadcast-centric to an origin-centric publishing architecture. Key changes include: version bump to 0.2.0 in CMakePresets.json, restructured publishing flow in moq-output.cpp with new origin member and lifecycle management, updated API calls for session connection and media track operations, addition of a private origin field in moq-output.h, reduced codec support to AAC and H264 only in moq-service.cpp, and adjusted logging call signature in obs-moq.cpp to include string length parameter. Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/moq-output.cpp (2)
10-22: Missing error handling for origin/broadcast creation.
moq_origin_create()andmoq_publish_create()may return negative error codes on failure. The destructor unconditionally callsmoq_publish_close(broadcast)andmoq_origin_close(origin), which could cause undefined behavior if these handles are invalid.🔎 Suggested approach
Consider validating the return values and either:
- Throw an exception if creation fails, or
- Add guards in the destructor similar to how
session,video, andaudioare guarded with> 0checks inStop().MoQOutput::~MoQOutput() { - moq_publish_close(broadcast); - moq_origin_close(origin); + if (broadcast > 0) { + moq_publish_close(broadcast); + } + if (origin > 0) { + moq_origin_close(origin); + } Stop(); }
108-128: Handles not reset after closing—potential double-close.After closing
session,video, andaudio, their values remain positive. IfStop()is called multiple times (e.g., explicitly then via destructor), the handles will be closed again.🔎 Suggested fix
void MoQOutput::Stop(bool signal) { // Close the session if (session > 0) { moq_session_close(session); + session = 0; } if (video > 0) { moq_publish_media_close(video); + video = 0; } if (audio > 0) { moq_publish_media_close(audio); + audio = 0; }
🧹 Nitpick comments (1)
src/obs-moq.cpp (1)
38-39: Consider usingstrlento avoid manual length synchronization.Hardcoding the string length (
4) is fragile—if the log level string changes, the length must be updated manually. Usingstrlen("info")makes this self-maintaining.🔎 Suggested fix
// Use RUST_LOG env var for more verbose output - // The second argument is the string length of the first argument. - moq_log_level("info", 4); + moq_log_level("info", strlen("info"));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CMakePresets.json(2 hunks)src/moq-output.cpp(7 hunks)src/moq-output.h(1 hunks)src/moq-service.cpp(1 hunks)src/obs-moq.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/moq-output.cpp (1)
src/moq-output.h (1)
MoQOutput(8-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Project 🧱 / Build for macOS 🍏
- GitHub Check: Build Project 🧱 / Build for Ubuntu 🐧 (ubuntu-24.04)
- GitHub Check: Build Project 🧱 / Build for Windows 🪟
🔇 Additional comments (6)
CMakePresets.json (1)
16-16: LGTM!Version bump to
0.2.0is appropriate given the significant API refactor to the new origin-centric publishing architecture.src/moq-service.cpp (1)
3-5: Verify intentional codec reduction.The supported codecs have been reduced to only
aacandh264, removingopus,hevc, andav1. This is a breaking change for users relying on the removed codecs.Ensure this limitation is intentional and aligns with the new libmoq API capabilities. The TODO comment suggests this may be temporary.
src/moq-output.h (1)
43-43: LGTM!The new
originmember appropriately supports the origin-centric publishing flow and is consistently typed with other handle members.src/moq-output.cpp (3)
85-101: LGTM!The new origin-centric publish flow is correctly implemented. The session connects with the origin handle, and the broadcast is published to the origin.
Note: The TODO at line 96 about missing unpublish function is tracked.
156-164: LGTM!The
moq_publish_media_frameAPI is correctly used with proper error handling.
218-224: LGTM!Good use of
strlen(codec)for the length parameter—this is self-maintaining unlike hardcoded values.
| MoQOutput::~MoQOutput() | ||
| { | ||
| moq_broadcast_close(broadcast); | ||
| moq_publish_close(broadcast); | ||
| moq_origin_close(origin); | ||
|
|
||
| Stop(); | ||
| } |
There was a problem hiding this comment.
Resource cleanup order may be incorrect.
moq_publish_close(broadcast) is called before Stop(), but Stop() closes the video/audio tracks that were created under the broadcast. Closing the parent (broadcast) before its children (tracks) may cause issues.
Consider calling Stop() first:
🔎 Suggested fix
MoQOutput::~MoQOutput()
{
+ Stop();
+
moq_publish_close(broadcast);
moq_origin_close(origin);
-
- Stop();
}🤖 Prompt for AI Agents
In src/moq-output.cpp around lines 24 to 30, the destructor currently calls
moq_publish_close(broadcast) and moq_origin_close(origin) before Stop(), which
can close the broadcast parent while its video/audio tracks are still open;
change the order so Stop() is called first to shut down and release tracks, then
call moq_publish_close(broadcast) and moq_origin_close(origin) afterward (retain
any existing null/validity checks and error handling when closing resources).
1 participant
No description provided.