Fix output publish timing and Windows FFmpeg lookup#40
Conversation
WalkthroughThis PR introduces two independent changes. First, the CMake build system is updated to detect FFmpeg dependencies conditionally: on Windows, it directly locates FFmpeg headers and individual libraries (avcodec, avutil, swscale, swresample) using find_path and find_library; on other platforms, it continues using pkg-config results. Second, the MoQOutput class defers broadcast publishing from the 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/moq-output.cppsrc/moq-output.cpp:1:10: fatal error: 'obs.hpp' file not found ... [truncated 2200 characters] ... tend__CTrans.CTrans_funct.instruction in file "src/clang/cTrans.ml" (inlined), line 4765, characters 38-71 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
🤖 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 `@src/moq-output.cpp`:
- Around line 29-33: Update the two logging calls to include the required
"[obs-moq]" prefix: change the LOG_INFO call that logs the publishing path and
the LOG_ERROR call inside the moq_origin_publish error branch so their messages
begin with "[obs-moq]". Locate the LOG_INFO("Publishing broadcast: %s",
path.c_str()) and LOG_ERROR("Failed to publish broadcast to session: %d",
result) statements (near the moq_origin_publish(origin, path.data(),
path.size(), broadcast) call) and prepend the prefix in their format strings so
they read e.g. LOG_INFO("[obs-moq] ...") and LOG_ERROR("[obs-moq] ...").
- Around line 23-39: The PublishBroadcast method currently short-circuits using
the instance-level flag broadcast_published, causing publishes to be skipped
across session restarts; fix by making the published state session-scoped —
either reset broadcast_published when a session stops/starts (e.g., in your
session Start/Stop handlers) or track the last published session (add a
last_published_session id member and compare it to the current origin/session
before skipping). Ensure PublishBroadcast (and moq_origin_publish call) uses the
current origin/session identity (origin, path) and sets the session-scoped flag
after a successful publish instead of relying on a single lifetime-initialized
broadcast_published.
🪄 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: a85dfec5-1420-459d-a966-0c9692b4bb46
📒 Files selected for processing (3)
CMakeLists.txtsrc/moq-output.cppsrc/moq-output.h
| bool MoQOutput::PublishBroadcast() | ||
| { | ||
| if (broadcast_published) { | ||
| return true; | ||
| } | ||
|
|
||
| LOG_INFO("Publishing broadcast: %s", path.c_str()); | ||
|
|
||
| auto result = moq_origin_publish(origin, path.data(), path.size(), broadcast); | ||
| if (result < 0) { | ||
| LOG_ERROR("Failed to publish broadcast to session: %d", result); | ||
| return false; | ||
| } | ||
|
|
||
| broadcast_published = true; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Reset publish state per session, not per object lifetime.
At Line 25, PublishBroadcast() permanently short-circuits once broadcast_published is true, but this flag is only initialized in the constructor (Line 19). On a later stop/start cycle with the same MoQOutput instance, the new session can skip moq_origin_publish(...).
Suggested fix
bool MoQOutput::Start()
{
@@
// Path (broadcast name) is optional; an empty string publishes to the unnamed broadcast.
const char *path_value = obs_service_get_connect_info(service, OBS_SERVICE_CONNECT_INFO_STREAM_KEY);
path = path_value ? path_value : "";
+ broadcast_published = false;
@@
}🤖 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 `@src/moq-output.cpp` around lines 23 - 39, The PublishBroadcast method
currently short-circuits using the instance-level flag broadcast_published,
causing publishes to be skipped across session restarts; fix by making the
published state session-scoped — either reset broadcast_published when a session
stops/starts (e.g., in your session Start/Stop handlers) or track the last
published session (add a last_published_session id member and compare it to the
current origin/session before skipping). Ensure PublishBroadcast (and
moq_origin_publish call) uses the current origin/session identity (origin, path)
and sets the session-scoped flag after a successful publish instead of relying
on a single lifetime-initialized broadcast_published.
| LOG_INFO("Publishing broadcast: %s", path.c_str()); | ||
|
|
||
| auto result = moq_origin_publish(origin, path.data(), path.size(), broadcast); | ||
| if (result < 0) { | ||
| LOG_ERROR("Failed to publish broadcast to session: %d", result); |
There was a problem hiding this comment.
Add the required [obs-moq] prefix to new log messages.
Line 29 and Line 33 use project logging macros but omit the required prefix, which makes filtering/attribution inconsistent.
Suggested fix
- LOG_INFO("Publishing broadcast: %s", path.c_str());
+ LOG_INFO("[obs-moq] Publishing broadcast: %s", path.c_str());
@@
- LOG_ERROR("Failed to publish broadcast to session: %d", result);
+ LOG_ERROR("[obs-moq] Failed to publish broadcast to session: %d", result);As per coding guidelines, **/*.{cpp,h,hpp} must use LOG_DEBUG(), LOG_INFO(), LOG_WARNING(), LOG_ERROR() with [obs-moq] prefix.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LOG_INFO("Publishing broadcast: %s", path.c_str()); | |
| auto result = moq_origin_publish(origin, path.data(), path.size(), broadcast); | |
| if (result < 0) { | |
| LOG_ERROR("Failed to publish broadcast to session: %d", result); | |
| LOG_INFO("[obs-moq] Publishing broadcast: %s", path.c_str()); | |
| auto result = moq_origin_publish(origin, path.data(), path.size(), broadcast); | |
| if (result < 0) { | |
| LOG_ERROR("[obs-moq] Failed to publish broadcast to session: %d", result); |
🤖 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 `@src/moq-output.cpp` around lines 29 - 33, Update the two logging calls to
include the required "[obs-moq]" prefix: change the LOG_INFO call that logs the
publishing path and the LOG_ERROR call inside the moq_origin_publish error
branch so their messages begin with "[obs-moq]". Locate the LOG_INFO("Publishing
broadcast: %s", path.c_str()) and LOG_ERROR("Failed to publish broadcast to
session: %d", result) statements (near the moq_origin_publish(origin,
path.data(), path.size(), broadcast) call) and prepend the prefix in their
format strings so they read e.g. LOG_INFO("[obs-moq] ...") and
LOG_ERROR("[obs-moq] ...").
|
The Cmake config fixes is in this PR for windows and mac. As it seems to be auto-generated and tested for Linux pkgconfig resolving. These resolve from the OBS deps that is already pulled while configuring. There was other changes required for windows like a debug console window needed. |
2 participants
Summary
.depspath with CMake-based header/library discoveryWhy
The output path currently publishes the broadcast before the session has reported a successful connection. This patch moves publication into the session-connected callback so the output path acts only after the session is established, which also matches the general pattern already used by the source path.
On Windows, the plugin build also depended on a pinned
.deps/obs-deps-...directory name for FFmpeg. That works locally, but it is brittle across dependency revisions. Switching to CMake discovery keeps the build tied to the configured dependency search path instead of one extracted folder name.Notes
This change is intentionally scoped to the dependency contract currently vendored by this plugin build. I am not trying to make a broader claim about all libmoq versions or callback semantics outside the version this repository is building against.
If there is a preferred repo-specific pattern for FFmpeg discovery on Windows, I’m happy to adjust the CMake side to match it.
Validation
cmake --build build_x64_26100 --config Release --target obs-moq