feat(obs): vendor the OBS plugin in-tree under cpp/obs#1977
Conversation
Bring the OBS Studio plugin (github.com/moq-dev/obs, GPL-2.0) into the monorepo as a slim copy under a new cpp/ top-level, so it builds against the in-tree rs/libmoq and ships with every libmoq release instead of tracking a separate repo and libmoq version. Vendored the current moq-obs tree (carries the source-teardown UAF fix, portable ffmpeg linkage, and Windows cmake fix), then layered only the monorepo-specific bits: SPDX GPL-2.0-or-later headers on every source file, and MOQ_LOCAL defaulting to the repo root so CMake builds libmoq from rs/libmoq via cargo (no release download). Dropped the upstream build-aux/ and .github/ scaffolding. The plugin statically links libmoq, so libmoq.yml gains obs-build and obs-release jobs: on a libmoq-v* tag it rebuilds the plugin against the just-published libmoq release and cuts a parallel obs-moq-v<version> release (macOS arm64 + Windows x64). Linux stays build-from-source. GPL note: libobs is GPL-2.0 with no linking exception, so any OBS plugin must be GPL. It does not infect the rest of the repo: cpp/obs is a separately-distributable work (GPLv2 mere aggregation) and libmoq stays MIT/Apache for all other consumers. Marked with SPDX headers, an isolated cpp/obs/LICENSE, and a README exception note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kixelated, your pull request is larger than the review limit of 150000 diff characters
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR adds a new C++ OBS Studio plugin (obs-moq) located at cpp/obs, which links the in-tree rs/libmoq crate to enable MoQ-based streaming input/output/service integration in OBS. It includes plugin source (output, source, service, optional Qt dock, logging), a full CMake build system with platform-specific configuration for Linux, macOS, and Windows (including packaging, code signing, and dependency fetching), formatting/lint configs, a build.sh packaging script, and a justfile for developer workflows. Documentation (README, CLAUDE.md, doc/bin/obs.md) is updated to describe the plugin's licensing exception (GPL-2.0-or-later) and build/release process. CI workflow changes add jobs to build the plugin for macOS/Windows and publish a separate obs-moq GitHub release alongside libmoq releases. flake.nix and the root justfile are updated to wire in the new module and dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant releaseJob as release
participant obsBuild as obs-build
participant buildScript as build.sh
participant obsRelease as obs-release
releaseJob->>obsBuild: triggers after libmoq release
obsBuild->>buildScript: parse-version libmoq, run build.sh --libmoq-release
buildScript-->>obsBuild: obs-moq artifact per target
obsBuild->>obsRelease: upload obs-moq-${matrix.target}
obsRelease->>obsRelease: download obs-moq-* artifacts
obsRelease->>obsRelease: create obs-moq-v<version> release
Related PRs: None identified. Suggested labels: cpp, obs-plugin, build-system, ci Suggested reviewers: None identified. Poem 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
just ci runs shfmt/shellcheck over every shell script, and the repo's .editorconfig pins *.sh to 4-space indent. The vendored build.sh used upstream's tab style, failing the format check. Reindent it to the house style (pure whitespace, no logic change); a future upstream re-vendor just re-runs `just fix` to regenerate this deterministically. Also drop fetch-depth: 0 from the obs-build CI job: it only compiles and never touches git history or tags, so a shallow checkout is enough. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (4)
cpp/obs/cmake/common/bootstrap.cmake (1)
34-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead code after
FATAL_ERROR.
message(FATAL_ERROR ...)halts configuration immediately, so thefile(REMOVE_RECURSE ...)cleanup on Line 40 never executes. This is unreachable and appears to be inherited boilerplate; if the intent was to clean up an accidental in-sourceCMakeCache.txt/CMakeFilesbefore erroring, the order needs to be swapped.Suggested fix
if("${CMAKE_CURRENT_BINARY_DIR}" STREQUAL "${CMAKE_CURRENT_SOURCE_DIR}") + file(REMOVE_RECURSE "${CMAKE_CURRENT_SOURCE_DIR}/CMakeCache.txt" "${CMAKE_CURRENT_SOURCE_DIR}/CMakeFiles") message( FATAL_ERROR "In-source builds are not supported. " "Specify a build directory via 'cmake -S <SOURCE DIRECTORY> -B <BUILD_DIRECTORY>' instead." ) - file(REMOVE_RECURSE "${CMAKE_CURRENT_SOURCE_DIR}/CMakeCache.txt" "${CMAKE_CURRENT_SOURCE_DIR}/CMakeFiles") endif()🤖 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 `@cpp/obs/cmake/common/bootstrap.cmake` around lines 34 - 41, The in-source build guard in bootstrap.cmake has unreachable cleanup because `message(FATAL_ERROR ...)` stops execution before `file(REMOVE_RECURSE ...)` can run. Update the `if("${CMAKE_CURRENT_BINARY_DIR}" STREQUAL "${CMAKE_CURRENT_SOURCE_DIR}")` block so any intended cleanup of `CMakeCache.txt` and `CMakeFiles` happens before the fatal error, or remove the dead cleanup entirely if it is no longer needed.cpp/obs/CMakeLists.txt (2)
16-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated FFmpeg linking logic between
BUILD_PLUGINbranches.Lines 24-25 and 34-35 both call
find_package(FFmpeg REQUIRED avcodec avutil swscale swresample)and link the same fourFFmpeg::*targets. Consider extracting this into a small helper/macro to avoid drift if the required FFmpeg components change later.🤖 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 `@cpp/obs/CMakeLists.txt` around lines 16 - 36, The FFmpeg setup in the BUILD_PLUGIN conditional duplicates the same find_package and target_link_libraries logic in both branches, which can drift over time. Refactor the repeated FFmpeg package discovery and linking for obs-moq into a small helper function or CMake macro, and call it from both the BUILD_PLUGIN and non-plugin paths so the required components and linked FFmpeg::* targets stay centralized and consistent.
112-114: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
find_package(Qt6 ...)isn't markedREQUIRED.When
ENABLE_QTis on but Qt6 isn't found,find_packagewill silently succeed (returning not-found), and the subsequenttarget_link_libraries(obs-moq PRIVATE Qt6::Core Qt6::Widgets)will fail with a less-clear "target not found" error rather than a clear Qt6-not-found message.Suggested fix
- find_package(Qt6 COMPONENTS Widgets Core) + find_package(Qt6 REQUIRED COMPONENTS Widgets Core)🤖 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 `@cpp/obs/CMakeLists.txt` around lines 112 - 114, The Qt6 dependency check in the ENABLE_QT block is too weak because `find_package(Qt6 COMPONENTS Widgets Core)` is not REQUIRED, so failures surface later in `target_link_libraries(obs-moq PRIVATE Qt6::Core Qt6::Widgets)` with a confusing missing-target error. Update the `find_package` call in the CMakeLists Qt setup to require Qt6 explicitly, so configuration stops early with a clear Qt6-not-found failure.cpp/obs/cmake/windows/helpers.cmake (1)
57-58: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueResource file attached to
CMAKE_PROJECT_NAME, not the passedtargetparam.
set_target_properties_plugin(target)attaches the generated.rcto${CMAKE_PROJECT_NAME}rather than${target}. This is correct only if the function is always called with the top-level project's plugin target (the common single-target case, matching upstream obs-plugintemplate). If this helper is ever invoked for an additional/secondary target, the.rcwould be misattached.🤖 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 `@cpp/obs/cmake/windows/helpers.cmake` around lines 57 - 58, The generated resource file is being attached to CMAKE_PROJECT_NAME instead of the function’s target parameter in set_target_properties_plugin. Update the helper to use the passed target symbol for the target_sources attachment (and any related property setup) so the .rc follows the actual target being configured, not only the top-level project target.
🤖 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 @.github/workflows/libmoq.yml:
- Around line 192-199: The “Find previous obs-moq tag” step fails on the first
release because the pipeline exits when no prior tag matches, so update the tag
lookup to tolerate an empty result in the workflow step using the prev_tag id
and its shell run block. Make the search for the previous obs-moq-v* tag
non-fatal when grep finds nothing, and always write a tag value to GITHUB_OUTPUT
even if it is empty so the first release can proceed.
In `@cpp/obs/build.sh`:
- Around line 63-96: Normalize the auto-detected TARGET in build.sh before the
case statement so macOS toolchains don’t hit the unsupported branch; cc
-dumpmachine returns version-suffixed Darwin triples, so add canonicalization
for macOS to match the existing macos preset path. Update the TARGET handling
near the cc -dumpmachine default and the later case on TARGET so both local
./build.sh and --libmoq-release use the canonical apple-darwin form expected by
CMakePresets.json and CMakeLists.txt.
In `@cpp/obs/cmake/common/buildspec_common.cmake`:
- Around line 178-187: The failed-download cleanup in the buildspec download
block is unreachable because message(FATAL_ERROR) aborts before file(REMOVE)
runs. Update the logic in the download handling around file(DOWNLOAD) so the
partial archive is removed before calling message(FATAL_ERROR), or switch to a
temporary download path in buildspec_common.cmake and only rename it on success.
Use the existing download_status, error_code, and error_message flow to preserve
the current error handling while ensuring stale files are cleaned up.
- Around line 55-72: The OBS configure step is building several CMake flags as
single strings, which makes execute_process(COMMAND ...) pass invalid argv
entries and literal quotes. Update the argument construction in
buildspec_common.cmake around execute_process, _cmake_arch, and _cmake_extra so
each flag/value is its own command argument instead of concatenated text. Split
the Windows -A option, the -D options, and the macOS
architecture/deployment-target values into separate list elements, and avoid
embedding shell-style quotes in the arguments.
In `@cpp/obs/cmake/common/FindFFmpeg.cmake`:
- Around line 131-142: The FFmpeg finder is still calling pkg-config for all
non-Windows hosts, which lets macOS pick up Homebrew/system FFmpeg hints instead
of the obs-deps copy. Update the logic in FindFFmpeg.cmake around the
pkg_search_module() path to skip pkg-config on Apple builds, or restrict it to
Linux/BSD only, so FFmpeg_${component}_INCLUDE_DIR and
FFmpeg_${component}_LIBRARY are resolved only from the intended obs-deps
locations. Keep the change scoped to the existing pkg-config and
find_path/find_library flow so the macOS routing from cpp/obs/CMakeLists.txt
remains intact.
In `@cpp/obs/cmake/linux/compilerconfig.cmake`:
- Around line 53-63: The GNU-specific warning toggles in compilerconfig.cmake
are using the C compiler version instead of the C++ compiler version, so the
checks under the CMAKE_CXX_COMPILER_ID STREQUAL GNU branch should be switched to
CMAKE_CXX_COMPILER_VERSION. Update both version comparisons in the GNU C++
configuration block so the add_compile_options calls for maybe-uninitialized,
infinite-recursion, and stringop-overflow are driven by the actual C++ compiler
version.
In `@cpp/obs/CMakePresets.json`:
- Around line 22-58: The macOS presets in CMakePresets.json are mislabeled as
universal builds while `CMAKE_OSX_ARCHITECTURES` in the `macos` preset is set to
arm64 only. Update the `macos` preset (and therefore `macos-ci`, which inherits
it) to build both architectures with `CMAKE_OSX_ARCHITECTURES`, or else rename
the `macos` and `macos-ci` preset names/descriptions to explicitly reflect
arm64-only behavior. Use the `macos` preset as the source of truth so the
inherited CI preset stays consistent.
In `@cpp/obs/src/moq-output.cpp`:
- Around line 31-37: The teardown in MoQOutput still allows a use-after-free
because the wait_for timeout can release this before the terminal session
callback has drained. Update MoQOutput::Stop/teardown and the session callback
path so the callback state is owned independently of MoQOutput, or so
destruction blocks until the session is fully detached and outstanding_sessions
reaches zero without timing out. Use the existing session_mutex, session_cv, and
outstanding_sessions symbols to keep the callback-safe state alive until libmoq
is completely finished with it.
- Around line 121-138: The publish failure path in moq-output.cpp leaves the
connected MoQ session alive after moq_session_connect() succeeds, which can
stale the session handle for future retries. In the broadcast publish logic
around session_connect_callback and moq_origin_publish, make sure to close the
active session before returning false on publish failure, either by calling
moq_session_close(session) or by routing through Stop(false), so the terminal
callback can clear the outstanding-session reference.
- Around line 91-108: The terminal path in session_connect_callback only
releases bookkeeping, but it should also stop the output when code <= 0 so OBS
does not keep running after an unexpected close. Update MoQOutput::Stop() and
the session callback in moq-output.cpp so terminal callbacks route through
obs_output_signal_stop() (or Stop()) and ensure the user-initiated Stop() path
does not emit the stop signal twice.
In `@cpp/obs/src/moq-source.cpp`:
- Around line 284-285: The reconnect log in moq-source.cpp is exposing the raw
relay URL via LOG_INFO in the settings-changed path, which can leak credentials
or tokens. Update the logging in the relevant reconnect handler to use a
redacted URL representation instead of url, preserving only safe host/path
information while keeping the broadcast value as-is. Use the existing
reconnect/logging code around the settings-changed message in moq-source::
settings update flow and coordinate the same fix upstream for the vendored
cpp/obs change.
- Around line 225-251: The teardown path in moq_source_disconnect_locked
currently logs a timeout but still destroys synchronization primitives and frees
ctx even when ctx->refs remains nonzero, which can leave libmoq callbacks using
freed state. Update the ctx reference handling so that when the wait in the
teardown block times out, the function does not call pthread_cond_destroy(),
pthread_mutex_destroy(), or bfree(ctx); instead, keep ctx alive until the final
terminal callback releases the last reference, or intentionally leak-and-log on
timeout. Preserve the existing refs_zero/refs mutex flow and only free ctx after
refs reaches zero.
- Around line 1118-1120: The frame timestamp assignment in moq-source.cpp is
using frame_data.timestamp_us directly in ctx->frame.timestamp, but that field
expects nanoseconds. Update the timestamp conversion in the frame output path
before obs_source_output_video so the value is normalized from microseconds to
nanoseconds (for example, by converting in the ctx->frame.timestamp assignment),
keeping the change localized around the frame_data / ctx->frame handling.
- Around line 961-1018: The packet decode path in moq-source.cpp drops delayed
output for codecs with reordering because avcodec_receive_frame() is only called
once and avcodec_send_packet() EAGAIN is treated as a failure. Update the decode
flow around the send/receive logic in the packet handling function to keep
draining avcodec_receive_frame() in a loop until EAGAIN, and if
avcodec_send_packet() returns EAGAIN, drain existing frames first and then
resend the same packet instead of discarding it. Keep the error handling and
consecutive_decode_errors tracking in that same decode path aligned with the new
retry behavior.
In `@cpp/obs/src/obs-moq.cpp`:
- Around line 1-18: Remove the template copyright boilerplate from the header in
obs-moq.cpp; the literal placeholders in the file comment are inaccurate. Update
the top-of-file comment to keep only the SPDX identifier or replace the
placeholder block with the real copyright notice, matching the style used
elsewhere in the vendored OBS code.
---
Nitpick comments:
In `@cpp/obs/cmake/common/bootstrap.cmake`:
- Around line 34-41: The in-source build guard in bootstrap.cmake has
unreachable cleanup because `message(FATAL_ERROR ...)` stops execution before
`file(REMOVE_RECURSE ...)` can run. Update the `if("${CMAKE_CURRENT_BINARY_DIR}"
STREQUAL "${CMAKE_CURRENT_SOURCE_DIR}")` block so any intended cleanup of
`CMakeCache.txt` and `CMakeFiles` happens before the fatal error, or remove the
dead cleanup entirely if it is no longer needed.
In `@cpp/obs/cmake/windows/helpers.cmake`:
- Around line 57-58: The generated resource file is being attached to
CMAKE_PROJECT_NAME instead of the function’s target parameter in
set_target_properties_plugin. Update the helper to use the passed target symbol
for the target_sources attachment (and any related property setup) so the .rc
follows the actual target being configured, not only the top-level project
target.
In `@cpp/obs/CMakeLists.txt`:
- Around line 16-36: The FFmpeg setup in the BUILD_PLUGIN conditional duplicates
the same find_package and target_link_libraries logic in both branches, which
can drift over time. Refactor the repeated FFmpeg package discovery and linking
for obs-moq into a small helper function or CMake macro, and call it from both
the BUILD_PLUGIN and non-plugin paths so the required components and linked
FFmpeg::* targets stay centralized and consistent.
- Around line 112-114: The Qt6 dependency check in the ENABLE_QT block is too
weak because `find_package(Qt6 COMPONENTS Widgets Core)` is not REQUIRED, so
failures surface later in `target_link_libraries(obs-moq PRIVATE Qt6::Core
Qt6::Widgets)` with a confusing missing-target error. Update the `find_package`
call in the CMakeLists Qt setup to require Qt6 explicitly, so configuration
stops early with a clear Qt6-not-found failure.
🪄 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: bfe263f9-6cbc-4c0b-8421-03949c032fe8
📒 Files selected for processing (53)
.github/workflows/libmoq.ymlCLAUDE.mdREADME.mdcpp/obs/.clang-formatcpp/obs/.gersemirccpp/obs/.gitignorecpp/obs/CMakeLists.txtcpp/obs/CMakePresets.jsoncpp/obs/LICENSEcpp/obs/README.mdcpp/obs/build.shcpp/obs/buildspec.jsoncpp/obs/cmake/common/FindFFmpeg.cmakecpp/obs/cmake/common/bootstrap.cmakecpp/obs/cmake/common/buildnumber.cmakecpp/obs/cmake/common/buildspec_common.cmakecpp/obs/cmake/common/ccache.cmakecpp/obs/cmake/common/compiler_common.cmakecpp/obs/cmake/common/helpers_common.cmakecpp/obs/cmake/common/osconfig.cmakecpp/obs/cmake/linux/compilerconfig.cmakecpp/obs/cmake/linux/defaults.cmakecpp/obs/cmake/linux/helpers.cmakecpp/obs/cmake/macos/buildspec.cmakecpp/obs/cmake/macos/compilerconfig.cmakecpp/obs/cmake/macos/defaults.cmakecpp/obs/cmake/macos/helpers.cmakecpp/obs/cmake/macos/resources/ccache-launcher-c.incpp/obs/cmake/macos/resources/ccache-launcher-cxx.incpp/obs/cmake/macos/resources/create-package.cmake.incpp/obs/cmake/macos/resources/distribution.incpp/obs/cmake/macos/resources/installer-macos.pkgproj.incpp/obs/cmake/macos/xcode.cmakecpp/obs/cmake/windows/buildspec.cmakecpp/obs/cmake/windows/compilerconfig.cmakecpp/obs/cmake/windows/defaults.cmakecpp/obs/cmake/windows/helpers.cmakecpp/obs/cmake/windows/resources/resource.rc.incpp/obs/data/locale/en-US.inicpp/obs/justfilecpp/obs/src/logger.hcpp/obs/src/moq-dock.cppcpp/obs/src/moq-dock.hcpp/obs/src/moq-output.cppcpp/obs/src/moq-output.hcpp/obs/src/moq-service.cppcpp/obs/src/moq-service.hcpp/obs/src/moq-source.cppcpp/obs/src/moq-source.hcpp/obs/src/obs-moq.cppdoc/bin/obs.mdflake.nixjustfile
| if [[ -z "$TARGET" ]]; then | ||
| TARGET=$(cc -dumpmachine 2>/dev/null || echo unknown) | ||
| echo "Detected target: $TARGET" | ||
| fi | ||
|
|
||
| # Default the version from buildspec.json's top-level "version" (the nested | ||
| # dependency entries also have "version" keys, hence the leading-indent anchor). | ||
| if [[ -z "$VERSION" ]]; then | ||
| VERSION=$(grep -E '^[[:space:]]{4}"version"' "$SCRIPT_DIR/buildspec.json" | head -1 | sed 's/.*: *"\([^"]*\)".*/\1/') | ||
| echo "Detected version: $VERSION" | ||
| fi | ||
|
|
||
| # Map the target triple to a CMake preset and build tree. | ||
| case "$TARGET" in | ||
| *-linux-*) | ||
| PRESET="ubuntu-x86_64" | ||
| BUILD_DIR="$SCRIPT_DIR/build_x86_64" | ||
| KIND="unix" | ||
| ;; | ||
| *-apple-darwin) | ||
| PRESET="macos" | ||
| BUILD_DIR="$SCRIPT_DIR/build_macos" | ||
| KIND="macos" | ||
| ;; | ||
| *-windows-*) | ||
| PRESET="windows-x64" | ||
| BUILD_DIR="$SCRIPT_DIR/build_x64" | ||
| KIND="windows" | ||
| ;; | ||
| *) | ||
| echo "Unsupported target: $TARGET" >&2 | ||
| exit 1 | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize the auto-detected macOS triple before using it.
The default detection path feeds cc -dumpmachine straight into this case statement, but the macOS branch only matches *-apple-darwin. Apple toolchains report version-suffixed Darwin triples, so local ./build.sh falls into Unsupported target, and --libmoq-release would also forward a non-canonical MOQ_TARGET that does not match the release archive naming used by cpp/obs/CMakePresets.json / cpp/obs/CMakeLists.txt.
💡 Suggested fix
if [[ -z "$TARGET" ]]; then
TARGET=$(cc -dumpmachine 2>/dev/null || echo unknown)
+ case "$TARGET" in
+ arm64-apple-darwin*) TARGET="aarch64-apple-darwin" ;;
+ x86_64-apple-darwin*) TARGET="x86_64-apple-darwin" ;;
+ esac
echo "Detected target: $TARGET"
fi
@@
-*-apple-darwin)
+*-apple-darwin*)
PRESET="macos"
BUILD_DIR="$SCRIPT_DIR/build_macos"
KIND="macos"📝 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.
| if [[ -z "$TARGET" ]]; then | |
| TARGET=$(cc -dumpmachine 2>/dev/null || echo unknown) | |
| echo "Detected target: $TARGET" | |
| fi | |
| # Default the version from buildspec.json's top-level "version" (the nested | |
| # dependency entries also have "version" keys, hence the leading-indent anchor). | |
| if [[ -z "$VERSION" ]]; then | |
| VERSION=$(grep -E '^[[:space:]]{4}"version"' "$SCRIPT_DIR/buildspec.json" | head -1 | sed 's/.*: *"\([^"]*\)".*/\1/') | |
| echo "Detected version: $VERSION" | |
| fi | |
| # Map the target triple to a CMake preset and build tree. | |
| case "$TARGET" in | |
| *-linux-*) | |
| PRESET="ubuntu-x86_64" | |
| BUILD_DIR="$SCRIPT_DIR/build_x86_64" | |
| KIND="unix" | |
| ;; | |
| *-apple-darwin) | |
| PRESET="macos" | |
| BUILD_DIR="$SCRIPT_DIR/build_macos" | |
| KIND="macos" | |
| ;; | |
| *-windows-*) | |
| PRESET="windows-x64" | |
| BUILD_DIR="$SCRIPT_DIR/build_x64" | |
| KIND="windows" | |
| ;; | |
| *) | |
| echo "Unsupported target: $TARGET" >&2 | |
| exit 1 | |
| ;; | |
| esac | |
| if [[ -z "$TARGET" ]]; then | |
| TARGET=$(cc -dumpmachine 2>/dev/null || echo unknown) | |
| case "$TARGET" in | |
| arm64-apple-darwin*) TARGET="aarch64-apple-darwin" ;; | |
| x86_64-apple-darwin*) TARGET="x86_64-apple-darwin" ;; | |
| esac | |
| echo "Detected target: $TARGET" | |
| fi | |
| # Default the version from buildspec.json's top-level "version" (the nested | |
| # dependency entries also have "version" keys, hence the leading-indent anchor). | |
| if [[ -z "$VERSION" ]]; then | |
| VERSION=$(grep -E '^[[:space:]]{4}"version"' "$SCRIPT_DIR/buildspec.json" | head -1 | sed 's/.*: *"\([^"]*\)".*/\1/') | |
| echo "Detected version: $VERSION" | |
| fi | |
| # Map the target triple to a CMake preset and build tree. | |
| case "$TARGET" in | |
| *-linux-*) | |
| PRESET="ubuntu-x86_64" | |
| BUILD_DIR="$SCRIPT_DIR/build_x86_64" | |
| KIND="unix" | |
| ;; | |
| *-apple-darwin*) | |
| PRESET="macos" | |
| BUILD_DIR="$SCRIPT_DIR/build_macos" | |
| KIND="macos" | |
| ;; | |
| *-windows-*) | |
| PRESET="windows-x64" | |
| BUILD_DIR="$SCRIPT_DIR/build_x64" | |
| KIND="windows" | |
| ;; | |
| *) | |
| echo "Unsupported target: $TARGET" >&2 | |
| exit 1 | |
| ;; | |
| esac |
🤖 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 `@cpp/obs/build.sh` around lines 63 - 96, Normalize the auto-detected TARGET in
build.sh before the case statement so macOS toolchains don’t hit the unsupported
branch; cc -dumpmachine returns version-suffixed Darwin triples, so add
canonicalization for macOS to match the existing macos preset path. Update the
TARGET handling near the cc -dumpmachine default and the later case on TARGET so
both local ./build.sh and --libmoq-release use the canonical apple-darwin form
expected by CMakePresets.json and CMakeLists.txt.
| if(OS_WINDOWS) | ||
| set(_cmake_generator "${CMAKE_GENERATOR}") | ||
| set(_cmake_arch "-A ${arch},version=${CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION}") | ||
| set(_cmake_extra "-DCMAKE_SYSTEM_VERSION=${CMAKE_SYSTEM_VERSION} -DCMAKE_ENABLE_SCRIPTING=OFF") | ||
| elseif(OS_MACOS) | ||
| set(_cmake_generator "Xcode") | ||
| set(_cmake_arch "-DCMAKE_OSX_ARCHITECTURES:STRING='arm64;x86_64'") | ||
| set(_cmake_extra "-DCMAKE_OSX_DEPLOYMENT_TARGET=${CMAKE_OSX_DEPLOYMENT_TARGET}") | ||
| endif() | ||
|
|
||
| message(STATUS "Configure ${label} (${arch})") | ||
| execute_process( | ||
| COMMAND | ||
| "${CMAKE_COMMAND}" -S "${dependencies_dir}/${_obs_destination}" -B | ||
| "${dependencies_dir}/${_obs_destination}/build_${arch}" -G ${_cmake_generator} "${_cmake_arch}" | ||
| -DOBS_CMAKE_VERSION:STRING=3.0.0 -DENABLE_PLUGINS:BOOL=OFF -DENABLE_FRONTEND:BOOL=OFF | ||
| -DOBS_VERSION_OVERRIDE:STRING=${_obs_version} "-DCMAKE_PREFIX_PATH='${CMAKE_PREFIX_PATH}'" ${_is_fresh} | ||
| ${_cmake_extra} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="cpp/obs/cmake/common/buildspec_common.cmake"
echo "== outline =="
ast-grep outline "$file" --view expanded || true
echo
echo "== relevant line ranges =="
sed -n '45,90p' "$file"
echo
sed -n '165,205p' "$file"
echo
echo "== search for execute_process and file(REMOVE) =="
rg -n 'execute_process|file\(REMOVE|_cmake_arch|_cmake_extra|CMAKE_PREFIX_PATH|CMAKE_OSX_ARCHITECTURES|CMAKE_ENABLE_SCRIPTING|CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION' "$file"Repository: moq-dev/moq
Length of output: 4943
🌐 Web query:
CMake execute_process COMMAND list arguments are not shell-split message(FATAL_ERROR) stops processing file(DOWNLOAD partial file on failure
💡 Result:
The behaviors you described involve how CMake processes commands and errors: 1. execute_process and shell-splitting: CMake's execute_process command does not use a shell to interpret the COMMAND arguments [1][2]. Instead, it passes the arguments directly to the operating system API as an array of strings (argv-style) on POSIX platforms [1][3]. On Windows, it handles command-line encoding to ensure child processes correctly receive the intended arguments [1][4]. Because no shell is invoked, features like shell operators (e.g., >, |, &&) will not work as expected and are treated as literal arguments to the command [1][4]. To run multiple commands in sequence or use pipes, you should use multiple execute_process calls or explicitly invoke a shell (e.g., "bash", "-c", "your command string") [1][5]. 2. message(FATAL_ERROR) behavior: The message(FATAL_ERROR...) command stops both the processing of the current CMake file and the entire configuration generation process [6][7]. When encountered, CMake exits with a non-zero return code, preventing further script execution or build system generation [6][8]. 3. file(DOWNLOAD) failure: By default, the file(DOWNLOAD...) command treats failure as a fatal error that stops CMake processing [9][10]. Even if you provide a STATUS variable, the command will still terminate if the download fails (for instance, due to a network issue or hash mismatch), leaving no opportunity to check the status variable in the script [10]. CMake does not currently provide a native option to "continue on error" for file(DOWNLOAD) to allow custom error handling within the same script [11][10]. To manage this, developers often use alternative strategies, such as checking for file existence beforehand or wrapping download logic in external scripts [12][10].
Citations:
- 1: https://cmake.org/cmake/help/latest/command/execute_process.html
- 2: https://cmake.org/cmake/help/v4.3/command/execute_process.html
- 3: https://cmake.org/cmake/help/v4.2/command/execute_process.html
- 4: https://cmake.org/cmake/help/v3.28/command/execute_process.html
- 5: https://discourse.cmake.org/t/execute-command-and-arguments-that-contain-with-blank-spaces/807
- 6: https://cmake.org/cmake/help/latest/command/message.html
- 7: https://cmake.org/cmake/help/v3.25/command/message.html
- 8: https://stackoverflow.com/questions/5403636/cause-cmake-to-generate-an-error
- 9: https://discourse.cmake.org/t/file-download-how-to-get-more-informative-error-messages/11118/1
- 10: https://discourse.cmake.org/t/file-download-error-message-question/13935
- 11: https://gitlab.kitware.com/cmake/cmake/-/issues/21790
- 12: https://stackoverflow.com/questions/57192628/can-i-stop-cmake-processing-without-an-error
Split the CMake flags into separate arguments. execute_process(COMMAND ...) passes argv literally, so packing -A, the two -D options, and the quoted macOS values into single strings makes the OBS configure step receive invalid options or literal quotes.
🤖 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 `@cpp/obs/cmake/common/buildspec_common.cmake` around lines 55 - 72, The OBS
configure step is building several CMake flags as single strings, which makes
execute_process(COMMAND ...) pass invalid argv entries and literal quotes.
Update the argument construction in buildspec_common.cmake around
execute_process, _cmake_arch, and _cmake_extra so each flag/value is its own
command argument instead of concatenated text. Split the Windows -A option, the
-D options, and the macOS architecture/deployment-target values into separate
list elements, and avoid embedding shell-style quotes in the arguments.
| if(NOT EXISTS "${dependencies_dir}/${file}") | ||
| message(STATUS "Downloading ${url}") | ||
| file(DOWNLOAD "${url}" "${dependencies_dir}/${file}" STATUS download_status EXPECTED_HASH SHA256=${hash}) | ||
|
|
||
| list(GET download_status 0 error_code) | ||
| list(GET download_status 1 error_message) | ||
| if(error_code GREATER 0) | ||
| message(STATUS "Downloading ${url} - Failure") | ||
| message(FATAL_ERROR "Unable to download ${url}, failed with error: ${error_message}") | ||
| file(REMOVE "${dependencies_dir}/${file}") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '170,195p' cpp/obs/cmake/common/buildspec_common.cmakeRepository: moq-dev/moq
Length of output: 1064
🌐 Web query:
CMake file(DOWNLOAD) failed transfer partial file left behind EXPECTED_HASH message(FATAL_ERROR immediate stop file(REMOVE) unreachable
💡 Result:
The behavior you are describing is a known limitation in how CMake handles file downloads during the configuration stage [1]. When file(DOWNLOAD ...) is used with EXPECTED_HASH, or when it otherwise fails, and you have logic that triggers message(FATAL_ERROR ...), CMake immediately halts execution [1]. Because message(FATAL_ERROR) stops the configuration process, any subsequent commands—including file(REMOVE ...) intended to clean up a partial or corrupted file—are never executed [1]. This leaves the partial or incorrect file on your filesystem. If you run CMake again, the file(DOWNLOAD) command may detect that the file already exists and, depending on the circumstances, potentially skip a re-download or fail again, leaving you stuck with the corrupted local state [2][1]. To solve this issue, it is recommended to adopt one of the following patterns: 1. Atomic Renaming (Recommended): Download the file to a temporary location (e.g., <file>.tmp) first. Only after the file(DOWNLOAD ...) command succeeds (and the hash is verified) should you move or rename the temporary file to the final destination [1]. This ensures that if the download fails or the process is interrupted, your final destination file is never created or overwritten by a partial/corrupted download [1]. 2. Use STATUS to Handle Failures: Instead of letting file(DOWNLOAD) cause an implicit failure, use the STATUS argument to capture the result code [3]. This allows you to check if the download succeeded before proceeding, giving you an opportunity to manually remove any failed files using file(REMOVE ...) before calling message(FATAL_ERROR ...) [3][1]. Example of the STATUS pattern: file(DOWNLOAD STATUS download_status) list(GET download_status 0 status_code) if(NOT status_code EQUAL 0) file(REMOVE ) message(FATAL_ERROR "Download failed: ${download_status}") endif This approach ensures that you retain control over the filesystem state regardless of whether the network request succeeds or fails [3][1].
Citations:
- 1: obsproject/obs-studio@97ee7fa
- 2: https://cmake.org/cmake/help/latest/command/file.html?highlight=file
- 3: https://stackoverflow.com/questions/61255773/cmake-file-download-ignores-errors
Delete the failed archive before aborting. message(FATAL_ERROR ...) stops immediately, so file(REMOVE ...) never runs; a failed download can leave a stale archive behind and block the next configure. Move the removal before the fatal error, or download to a temp path and rename on success.
🤖 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 `@cpp/obs/cmake/common/buildspec_common.cmake` around lines 178 - 187, The
failed-download cleanup in the buildspec download block is unreachable because
message(FATAL_ERROR) aborts before file(REMOVE) runs. Update the logic in the
download handling around file(DOWNLOAD) so the partial archive is removed before
calling message(FATAL_ERROR), or switch to a temporary download path in
buildspec_common.cmake and only rename it on success. Use the existing
download_status, error_code, and error_message flow to preserve the current
error handling while ensuring stale files are cleaned up.
| if(NOT CMAKE_HOST_SYSTEM_NAME MATCHES "Windows") | ||
| find_package(PkgConfig QUIET) | ||
| if(PKG_CONFIG_FOUND) | ||
| pkg_search_module(PC_FFmpeg_${component} QUIET ${component_libname}) | ||
| endif() | ||
| endif() | ||
|
|
||
| find_path( | ||
| FFmpeg_${component}_INCLUDE_DIR | ||
| NAMES ${component_libname}/${component_header} ${component_libname}/version.h | ||
| HINTS ${PC_FFmpeg_${component}_INCLUDE_DIRS} | ||
| PATHS /usr/include /usr/local/include |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Skip pkg-config on macOS in this finder.
cpp/obs/CMakeLists.txt routes Apple builds through this module so it binds the FFmpeg shipped in obs-deps. Querying pkg_search_module() on every non-Windows host feeds Homebrew/system include and library hints back into find_path() and find_library(), so a macOS machine with another FFmpeg installed can silently link against the wrong ABI. Limit the pkg-config path to Linux/BSD, or make it an explicit opt-in.
Suggested fix
- if(NOT CMAKE_HOST_SYSTEM_NAME MATCHES "Windows")
+ if(CMAKE_SYSTEM_NAME MATCHES "Linux|FreeBSD|OpenBSD")
find_package(PkgConfig QUIET)
if(PKG_CONFIG_FOUND)
pkg_search_module(PC_FFmpeg_${component} QUIET ${component_libname})
endif()
endif()Also applies to: 157-161
🤖 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 `@cpp/obs/cmake/common/FindFFmpeg.cmake` around lines 131 - 142, The FFmpeg
finder is still calling pkg-config for all non-Windows hosts, which lets macOS
pick up Homebrew/system FFmpeg hints instead of the obs-deps copy. Update the
logic in FindFFmpeg.cmake around the pkg_search_module() path to skip pkg-config
on Apple builds, or restrict it to Linux/BSD only, so
FFmpeg_${component}_INCLUDE_DIR and FFmpeg_${component}_LIBRARY are resolved
only from the intended obs-deps locations. Keep the change scoped to the
existing pkg-config and find_path/find_library flow so the macOS routing from
cpp/obs/CMakeLists.txt remains intact.
| // Send packet to decoder | ||
| int ret = avcodec_send_packet(ctx->codec_ctx, packet); | ||
| av_packet_free(&packet); | ||
|
|
||
| if (ret < 0) { | ||
| if (ret != AVERROR(EAGAIN)) { | ||
| ctx->consecutive_decode_errors++; | ||
| char errbuf[AV_ERROR_MAX_STRING_SIZE]; | ||
| av_strerror(ret, errbuf, sizeof(errbuf)); | ||
|
|
||
| // If too many consecutive errors, flush decoder and wait for next keyframe | ||
| if (ctx->consecutive_decode_errors >= 5) { | ||
| LOG_WARNING("Too many send errors (%u), flushing decoder and waiting for keyframe", | ||
| ctx->consecutive_decode_errors); | ||
| avcodec_flush_buffers(ctx->codec_ctx); | ||
| ctx->got_keyframe = false; | ||
| ctx->consecutive_decode_errors = 0; | ||
| } else if (ctx->consecutive_decode_errors == 1) { | ||
| LOG_ERROR("Error sending packet to decoder: %s", errbuf); | ||
| } | ||
| } | ||
| pthread_mutex_unlock(&ctx->mutex); | ||
| moq_consume_frame_close(frame_id); | ||
| return; | ||
| } | ||
|
|
||
| // Receive decoded frames | ||
| AVFrame *frame = av_frame_alloc(); | ||
| if (!frame) { | ||
| pthread_mutex_unlock(&ctx->mutex); | ||
| moq_consume_frame_close(frame_id); | ||
| return; | ||
| } | ||
|
|
||
| ret = avcodec_receive_frame(ctx->codec_ctx, frame); | ||
| if (ret < 0) { | ||
| if (ret != AVERROR(EAGAIN)) { | ||
| ctx->consecutive_decode_errors++; | ||
| char errbuf[AV_ERROR_MAX_STRING_SIZE]; | ||
| av_strerror(ret, errbuf, sizeof(errbuf)); | ||
|
|
||
| // If too many consecutive errors, flush decoder and wait for next keyframe | ||
| if (ctx->consecutive_decode_errors >= 5) { | ||
| LOG_WARNING("Too many decode errors (%u), flushing decoder and waiting for keyframe", | ||
| ctx->consecutive_decode_errors); | ||
| avcodec_flush_buffers(ctx->codec_ctx); | ||
| ctx->got_keyframe = false; | ||
| ctx->consecutive_decode_errors = 0; | ||
| } else if (ctx->consecutive_decode_errors == 1) { | ||
| // Only log first error in a sequence | ||
| LOG_ERROR("Error receiving frame from decoder: %s", errbuf); | ||
| } | ||
| } | ||
| av_frame_free(&frame); | ||
| pthread_mutex_unlock(&ctx->mutex); | ||
| moq_consume_frame_close(frame_id); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure and inspect the relevant region.
git ls-files cpp/obs/src/moq-source.cpp
wc -l cpp/obs/src/moq-source.cpp
sed -n '900,1085p' cpp/obs/src/moq-source.cpp
# Search for decoder receive/send patterns and any drain loops in the repo.
rg -n "avcodec_receive_frame|avcodec_send_packet|consecutive_decode_errors|got_keyframe|avcodec_flush_buffers" cpp/obs/src cpp/obs -g '!**/build/**' -g '!**/dist/**'Repository: moq-dev/moq
Length of output: 11118
🌐 Web query:
FFmpeg avcodec_send_packet EAGAIN avcodec_receive_frame loop drain multiple frames per packet documentation
💡 Result:
In the FFmpeg send/receive API, avcodec_send_packet and avcodec_receive_frame are decoupled, requiring a specific loop structure to handle multiple frames produced from a single packet or buffering delays [1][2]. When you call avcodec_send_packet, it may return AVERROR(EAGAIN) if the decoder's internal buffers are full and cannot accept more input until you have read existing output [3]. In this state, you must call avcodec_receive_frame in a loop to drain the available frames [1][2]. Once avcodec_receive_frame returns AVERROR(EAGAIN), it indicates that the decoder has no more frames to output and is ready to accept new input, at which point you should resend the packet that previously triggered the EAGAIN error [1][3]. For scenarios where a single input packet contains multiple frames (e.g., certain audio codecs), you must call avcodec_receive_frame repeatedly after a successful avcodec_send_packet until it returns AVERROR(EAGAIN) [1][3]. Only then should you provide the next packet [1][3]. To drain the codec (e.g., at the end of a stream), you must send a NULL packet to avcodec_send_packet to enter draining mode [1][2]. Once in this mode, continue calling avcodec_receive_frame in a loop until it returns AVERROR_EOF [1][2]. Note that while in draining mode, avcodec_receive_frame will not return AVERROR(EAGAIN) [1][2]. Critically, the FFmpeg API is a strict state machine; a codec is never allowed to return AVERROR(EAGAIN) for both sending and receiving simultaneously, as this would create an invalid state and a potential infinite loop [4][2]. Always prioritize draining output frames when the decoder indicates it cannot accept more input [1][3]. Top results: [1][4][2][3][5]
Citations:
- 1: https://www.ffmpeg.org/doxygen/7.1/group__lavc__encdec.html
- 2: https://ffmpeg.org/doxygen/7.1/group__lavc__encdec.html
- 3: https://ffmpeg.org/doxygen/6.1/group__lavc__decoding.html
- 4: https://ffmpeg.org/pipermail/ffmpeg-cvslog/2017-October/109865.html
- 5: https://github.com/FFmpeg/FFmpeg/blob/master/doc/examples/decode_video.c
Drain and retry the packet instead of dropping it. avcodec_receive_frame() needs a loop until EAGAIN, and a send_packet() EAGAIN means the same packet must be resent after draining. As written, delayed output from codecs with reordering/B-frames can be lost.
🤖 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 `@cpp/obs/src/moq-source.cpp` around lines 961 - 1018, The packet decode path
in moq-source.cpp drops delayed output for codecs with reordering because
avcodec_receive_frame() is only called once and avcodec_send_packet() EAGAIN is
treated as a failure. Update the decode flow around the send/receive logic in
the packet handling function to keep draining avcodec_receive_frame() in a loop
until EAGAIN, and if avcodec_send_packet() returns EAGAIN, drain existing frames
first and then resend the same packet instead of discarding it. Keep the error
handling and consecutive_decode_errors tracking in that same decode path aligned
with the new retry behavior.
Source: Learnings
| /* | ||
| SPDX-License-Identifier: GPL-2.0-or-later | ||
| Plugin Name | ||
| Copyright (C) <Year> <Developer> <Email Address> | ||
|
|
||
| This program is free software; you can redistribute it and/or modify | ||
| it under the terms of the GNU General Public License as published by | ||
| the Free Software Foundation; either version 2 of the License, or | ||
| (at your option) any later version. | ||
|
|
||
| This program is distributed in the hope that it will be useful, | ||
| but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| GNU General Public License for more details. | ||
|
|
||
| You should have received a copy of the GNU General Public License along | ||
| with this program. If not, see <https://www.gnu.org/licenses/> | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the template license boilerplate.
Shipping literal placeholders like <Year> <Developer> <Email Address> leaves the notice inaccurate and diverges from the SPDX-only headers used elsewhere in this vendored tree. Keep the SPDX identifier alone, or replace the template text with the real copyright notice.
🤖 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 `@cpp/obs/src/obs-moq.cpp` around lines 1 - 18, Remove the template copyright
boilerplate from the header in obs-moq.cpp; the literal placeholders in the file
comment are inaccurate. Update the top-of-file comment to keep only the SPDX
identifier or replace the placeholder block with the real copyright notice,
matching the style used elsewhere in the vendored OBS code.
Under GitHub Actions' default `bash -eo pipefail`, `git tag ... | grep -vx "$cur" | head` fails the step when the inaugural obs-moq-v* release has no previous tag (grep exits 1). Use awk so an empty result is exit 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Triage of the CodeRabbit findings. Fixed here (monorepo-authored):
Not fixed here — intentionally routed upstream. Everything under Real bugs worth an upstream fix:
OBS-plugintemplate boilerplate (unchanged from upstream obs-plugintemplate, low-risk / likely intended): Leaving these threads open as a tracking signal rather than resolving them. (Written by Claude Opus 4.8) |
* Fix source/output lifecycle and correctness bugs Four safe, self-contained fixes (surfaced by a CodeRabbit review of the in-tree vendoring in moq-dev/moq#1977): - moq-source: OBS frame.timestamp is nanoseconds but libmoq delivers microseconds, so playback timestamps were 1000x too small, skewing async A/V pacing. Multiply by 1000. - moq-source: redact the relay URL before logging it. URLs can carry credentials/tokens and OBS logs are often shared for support. - moq-source: on the teardown timeout path, leak ctx instead of freeing it. If refs is still nonzero a libmoq callback still references ctx (mutex/cond/refs), so destroying and freeing it is a use-after-free. Leaking on this abnormal path is strictly safer. - moq-output: close the session when moq_origin_publish fails. The session already connected, so returning without closing leaves a live background session and a retry on the same output reuses the stale handle. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * moq-source: strip userinfo at the last '@' when redacting URLs find('@') would leave part of a password behind for a URL like moq://user:p@ss@host; rfind('@') removes everything up to the final authority delimiter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Pull moq-dev/obs#46 (323d597) into the vendored copy: frame timestamp µs->ns, relay-URL redaction in logs, leak-not-free on the source teardown timeout, and close-session-on-publish-failure. Keeps cpp/obs byte-identical to upstream main (plus the SPDX header). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Brings the OBS Studio plugin (github.com/moq-dev/obs, GPL-2.0) into the monorepo as a slim copy under a new
cpp/top-level. The plugin now builds against the in-treers/libmoq(no release download) and ships with every libmoq release, instead of living in a separate repo that has to chase a pinned libmoq version.This re-does the previously-reverted #1787, at explicit request. The earlier revert was over GPL license-scanner noise (see the licensing note below); the tradeoff is being accepted this time.
What changed
cpp/obs/(47 files): vendored the current moq-obs (bb3b128), so it carries the recent upstream fixes (MoQ-source teardown use-after-free fix, portable ffmpeg linkage, Windows cmake install path). Dropped the upstreambuild-aux/and.github/scaffolding.GPL-2.0-or-laterheaders on all 10 source files, plus an isolatedcpp/obs/LICENSE.MOQ_LOCALauto-defaults to the repo root (set(... "${CMAKE_CURRENT_SOURCE_DIR}/../.." CACHE PATH)+ EXISTS guard), so CMake buildslibmoqfromrs/libmoqvia cargo with no config. A consumer buildingcpp/obsin isolation can still override it or fall back to a published release.justfile(mod obs 'cpp/obs'),flake.nix(obsDepsinto the dev shell, Linux-only since nixpkgs marks obs-studio broken on Darwin),CLAUDE.md(/cpp/structure bullet +rs/libmoq C ABI → cpp/obs/srcsync row),README.md(GPL exception note),doc/bin/obs.md(rewritten for the in-tree build).libmoq.ymlgainsobs-build+obs-releasejobs. On alibmoq-v*tag they rebuild the plugin against the just-published libmoq release (matching ABI, no second cargo build) and cut a parallelobs-moq-v<version>release with macOS arm64 + Windows x64 binaries. Linux stays build-from-source.Skipped the old PR's
rs/libmoq/CMakeLists.txtWindows-libs hunk: that already landed onmainvia #1792.Licensing note
libobsis GPL-2.0 with no linking exception, so any distributed OBS plugin must be GPL. It does not infect the rest of the repo:cpp/obsis a separately-distributable work (GPLv2 mere aggregation), andlibmoqand the other moq crates stay MIT/Apache for every other consumer. This is scoped with SPDX headers,cpp/obs/LICENSE, and a README exception. The known cost is that full-filesystem license scanners (Black Duck / FOSSA / ScanCode) will report GPL present in the repo; dependency-graph scanners (cargo-deny, npm) won't, sincecpp/obsisn't in any package graph.Test plan
cpp/obsjustfiles parse;mod obsresolves;flake.nixevaluates on aarch64-darwinactionlint,clang-format,gersemiclean on the vendored treecargo build -p libmoqgreen;MOQ_LOCALresolves to the repo-rootrs/libmoqCross-package sync:
rs/libmoqC ABI ↔cpp/obs/src,doc/bin/obs.md(added to the sync table).(Written by Claude Opus 4.8)