feat(obs): the OBS plugin on the generated C++ bindings - #4281
Conversation
cpp/obs links cpp/moq instead of libmoq. The output and source hold moq:: objects, async work is a future whose continuation runs on a worker thread each output and source owns, and teardown stops that worker instead of counting outstanding callbacks. obs_module_unload calls moq::shutdown(). Tests run against the real moq-ffi over an in-process relay. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
One moq:: namespace for every CMake package: find_package(moq-cpp) links moq::cpp (was moq-cpp::moq), matching moq::c from find_package(moq-c). The internal imported staticlib is moq::ffi. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cpp/obs/VERSION (0.7.0) versions the plugin instead of the moq-cpp release it links, so obs-moq tags don't go backwards, and a C++ release refuses to reuse a plugin version that already names another commit. CONTRIBUTING.md records both version sources. libmoq-shutdown is about any host that unloads the C library, since OBS no longer links it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Decisions
(Written by Opus 5.5) |
# Conflicts: # CLAUDE.md # cpp/obs/justfile # cpp/obs/src/moq-output.cpp # cpp/obs/test/moq-output-test.cpp
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Decisions:
(Written by Opus 5.5) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd7b3e5ba8
ℹ️ 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".
| moq_source_install_decoder_locked(ctx, std::move(decoder)); | ||
| conn->video = std::make_shared<Track>("Video", rendition->first, rendition->second.container, | ||
| moq_source_decode_frame); | ||
| moq_source_subscribe_track(ctx, conn, conn->video, rendition->second.broadcast); |
There was a problem hiding this comment.
When a catalog update arrives during playback, these lines immediately destroy the working decoder and cancel the current video track before the asynchronous resolve() and subscribe_media() calls succeed. An unavailable sibling broadcast or rejected subscription therefore freezes video until another catalog update, and even successful metadata updates introduce a gap and force another keyframe wait; retain the current track and decoder until moq_source_on_subscribed has a valid replacement. The audio path at lines 721-724 has the same ordering problem.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this as is for this PR: it keeps the libmoq version's behavior. That version also installed the new decoder and cancelled the old track as soon as moq_consume_video returned, before any frame from the replacement arrived, and an async subscribe failure left no video either. Holding the old track until the new one yields (or skipping the resubscribe when the rendition didn't change) would be an improvement over today, so it goes to the maintainer as a follow-up rather than growing the migration.
(Written by Claude Opus 5.5)
| obs-build: | ||
| name: OBS plugin (${{ matrix.target }}) | ||
| needs: release | ||
| runs-on: ${{ matrix.os }} |
There was a problem hiding this comment.
During the nightly workflow_call and manual dry-run, release is skipped by its github.ref_type == 'tag' condition, and GitHub documents that a job needing a skipped job is also skipped unless its condition permits continuation. Consequently this obs-build matrix never tests macOS or Windows packaging before a release tag, despite the workflow's stated dry-run contract; depend on build for non-tag runs or add an explicit tag-aware condition. GitHub Actions documentation.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this as is. obs-build passes --moq-release, which fetches the cpp-v<version> archives from the release that release just published, so a dry-run has nothing to link against. It also matches libmoq.yml, which this job moved from. The plugin's compile is gated by obs.yml on PRs and by the nightly on Linux; the macOS and Windows builds are on the manual list for the cpp line's final review.
(Written by Claude Opus 5.5)
| - name: Parse version | ||
| id: parse | ||
| shell: bash | ||
| run: .github/scripts/release.sh parse-version cpp |
There was a problem hiding this comment.
When this job runs outside a tag, release.sh parse-version cpp follows its dry-run path and reads rs/cpp/Cargo.toml, which does not exist; the intended nightly/manual OBS package build will therefore fail before compilation. Read cpp/moq/VERSION, as the workflow's existing version check does, or extend the helper to understand the C++ package layout.
AGENTS.md reference: AGENTS.md:L23-L23
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in acd315f: the job reads cpp/moq/VERSION, which the build job has already checked against the tag. It only runs on a tag today, where parse-version cpp worked, but the file is the source of truth either way.
(Written by Claude Opus 5.5)
| } | ||
|
|
||
| const auto &raw = connection.stats; | ||
| const moq::ConnectionStats raw = current->stats(); |
There was a problem hiding this comment.
If Stop(), a restart, or a disconnect occurs after the live session is copied under mutex but before this assignment, the method returns true with statistics from the retired session and the dock reports the output as connected. The previous implementation rechecked the attempt and session handle before overwriting the caller's snapshot, and its regression test explicitly exercised this race; restore an equivalent identity/live check after stats() returns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, restored in acd315f: after stats() returns, TryGetConnectionStats rechecks under mutex that the session is still the current, live one before handing back the snapshot. No regression test: the window sits inside the synchronous moq-ffi call, and the real-relay tests have no seam to hold it open the way the old stubbed moq_session_snapshot did.
(Written by Claude Opus 5.5)
…rsion file TryGetConnectionStats rechecks that the session it read is still the live one before handing back the snapshot, as the libmoq version did. The OBS release job reads cpp/moq/VERSION instead of release.sh parse-version, whose off-tag path looks for a Cargo.toml. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # quest/m1/cpp/README.md
#4292 gave moq::Error Rust's Display text, so the hand-kept MoQDescribe copy goes. The failure-code classifier, its one remaining user, moves into moq-output.cpp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # CONTRIBUTING.md # quest/m1/README.md # quest/m1/cpp/README.md # quest/m1/kt-jvm-exit.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19bc290994
ℹ️ 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".
| while (!done()) { | ||
| if (std::chrono::steady_clock::now() > deadline) | ||
| return false; | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(2)); |
There was a problem hiding this comment.
The newly added OBS unit suites use this helper repeatedly to poll asynchronous state with real-time sleeps and a 10-second deadline, while several tests also add fixed sleeps to prove that callbacks did not arrive. Under slower CI or ThreadSanitizer runs, valid behavior can exceed those timing windows and fail nondeterministically, while successful runs incur unnecessary delays. Use fixture signals, condition variables, or mocked time so completion and absence of callbacks are observed deterministically.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this as is. These suites run the real moq-ffi over real QUIC against an in-process relay, and nothing in C++ can mock the clock of moq-ffi's tokio runtime. WaitFor waits on a condition with a 10 s ceiling; it doesn't use a sleep to synchronize. A pass takes as long as the event does, and only a run 10 s slower than expected fails (TSan ran clean 5 times). The short fixed sleeps check that a callback did not arrive, so a late callback can only make them pass wrongly, never fail. Swapping polling for fixture signals where the plugin exposes one would be a fine follow-up, but it's out of scope for the migration.
(Written by Claude Opus 5.5)
|
Summary: I merged
(Written by Claude Opus 5.5) |
1 participant
Problem:
cpp/obsreached MoQ through libmoq'sinthandles and C callbacks on one shared runtime thread, which is why it carried generation counters, heapSessionRef/callback_statetrampolines, a condvar refcount with a two-second destroy backstop, and "never take signal_mutex inside a libmoq call" lock-order rules. It also never stopped the runtime before OBS unloads the module. Quest:quest/m1/cpp/obs.md(deleted here).Approach
CMakeLists.txtaddscpp/moqin-tree forMOQ_LOCAL, or fetches thecpp-v<version>release archive andfind_package(moq-cpp), and linksmoq::cpp(renamed in feat(cpp)!: export the C++ package as moq::cpp #4297). The hand-listed Windows system libs are gone: the package carries them.just obs compile/_unitbuild and installcpp/moqto render the headers.MoQWorker(src/moq-worker.h), one thread per output and per source, is the executor passed to everythen(). No continuation runs on the moq-ffi runtime, a slow one (FFmpeg decode, a frontend handling a signal inline) only delays its own object, andStop()drops queued tasks and joins, so nothing runs after destroy.moq::Client::connectthen asession->status()loop. The attempt counter becomes pointer identity on the ownedAttempt, whose drop cancels the pending call; a result already queued on the worker is discarded by that identity check.signal_mutexstays, since OBS itself needs a stop and a failure report serialized;SessionState/SessionRef/Detachare gone. Media goes throughBroadcastProducer::publish_video/audio,write_frame,cut,flushunder a smallmedia_mutex.Connectionobject owns client, session, announced broadcast, catalog, and aTrackper rendition; dropping it cancels everything. Refcounts, condvar, backstop timeout, generation and attempt counters are deleted. FFmpeg decode is unchanged.obs_module_unloadlogs and callsmoq::shutdown().test/moq-test-relay.h, which also serves the http:// fingerprint). Kept scenarios: stop during connect, failure racing Stop (report window held open), re-entrant Stop from the signal, OBS-driven restart, failures outliving the output (x100), destroy during connect / waiting for an announce / mid-stream. New: real publish into the relay's catalog, reconnect after a relay drop, module unload stopping the runtime (test/obs-moq-test.cpp). TSan needstest/tsan.supp, which suppresses reports with a Rust frame (uninstrumented).libmoq.ymltorelease-cpp.yml(build.sh --moq-release);obs.ymltriggers oncpp/moq/**andrs/moq-ffi/**too. The plugin keeps its own version incpp/obs/VERSION(0.7.0, replacing buildspec.json's unused 0.0.1), and a C++ release refuses to cut anobs-moq-v<version>that already names another commit.CONTRIBUTING.mdgains a# Versionssection for moq-cpp and OBS (reconciles with chore: pin shared skills; document package versions in CONTRIBUTING #4272).Impact
quest/m1/cpp/client-config.mdandquest/m1/cpp/session-report.md.Alternatives
obs_queue_task) as executors, as the quest plan suggested: decode would stall rendering, and there is no "output thread" in OBS. A worker per object gives the same guarantees and a join at destroy.uniffi_moq_ffi_*symbols in tests: brittle, and it would no longer exercise the real cancellation and threading.Follow-ups
quest/m1/cpp/client-config.md,quest/m1/cpp/session-report.md(both must land before the line merges).moq::Error::to_string()(feat(cpp): moq::Error prints Rust's message #4292), so the hand-kept mapping is gone.AGENTS.mdCross-Package Sync now listscpp/obs/srcanddoc/bin/obs.mdunderrs/moq-ffi.quest/m3/libmoq-shutdown.md(moved to m3 onmain) is re-scoped to any host that unloads the C library.Checks on Linux:
just obs compile,just obs ci,just obs test(TSan, 5 runs clean),just obs check,just check.(Written by Opus 5.5)
🤖 Generated with Claude Code