Commit Graph
3 Commits
Author SHA1 Message Date
shadowdaoandClaude Sonnet 5 f2a4932eea Fix review findings: stopping-flag race, unpinned SDK download, key-leak via redirect/logs
Code review findings from before merging feat/livekit-integration to main:

- I1: sourceDestroy set self->stopping outside self->mutex, then notified.
  The worker's condition-variable predicate reads `stopping` under that same
  mutex, so the store+notify could land between the worker's predicate check
  and it entering the wait, dropping the notification and leaving the worker
  asleep for its full backoff (up to 30s) with the OBS UI thread blocked in
  worker.join(). Now set under the lock, matching how `generation` is
  already mutated in applySettings.

- I3: the LiveKit SDK archive download in cmake/LiveKitSDK.cmake had no
  SHA256 pin wired up from the top-level CMakeLists.txt, unlike the obs-deps
  bootstrap right next to it. Added real SHA256 hashes -- computed by
  downloading each release archive and running sha256sum -- for every
  triple the pinned v1.10.1 release can resolve to (Linux x64/arm64, macOS
  x64/arm64, Windows x64), keyed by version+triple so a future version bump
  fails loudly (via message(WARNING)) instead of silently going unverified.
  Verified end-to-end locally: a deliberately wrong hash makes the configure
  step fail with a HASH mismatch error. Only Linux was also build-tested in
  this environment; macOS/Windows archives were downloaded and hashed but
  not build-tested here.

- I4: the curl HTTP backend followed up to 3 redirects while the read key
  travels as a URL query parameter, so a malicious/misconfigured redirect
  (including an HTTPS->HTTP downgrade, which curl doesn't refuse by default)
  could leak the key. This client only ever talks to two fixed, first-party
  endpoints, so redirects are disabled outright (CURLOPT_FOLLOWLOCATION 0),
  matching the WinHTTP backend's existing default behavior. Left
  normalizeServerUrl's explicit-http:// pass-through as-is with a comment,
  per review guidance.

- I5: ApiClient::redactedUrl was tested but never called. No current call
  site logs a request URL, so rather than inventing one, added a one-line
  comment marking it a deliberate guard rail for future logging.

- I7: the LiveKit SDK log bridge (livekitLogToObs) wrote SDK messages
  straight into the OBS log. LiveKit's signaling URL carries the access
  token as a query parameter; defensively scrub "access_token=" and "key="
  values before they ever reach obs_log. New ApiClient::redactSensitiveParams
  generalizes redactedUrl's redaction pattern to arbitrary text (not just a
  bare URL), with 6 new unit tests in test_api_client.cpp.

- I2: added a code comment on session.cpp's auto_subscribe=true noting the
  known, unaddressed bandwidth/CPU cost of pulling every participant's
  track in multi-camera rooms, and that per-publication unsubscribe is a
  future optimization. No behavior change (out of scope per review).

Verified: cmake configure + build + `ctest --test-dir build
--output-on-failure` all pass, 6/6 suites (test_api_client now 127 checks,
up from 121).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RL8abRmgFXkVASHkkqiJbE
2026-09-06 22:59:57 -07:00
shadowdaoandClaude Sonnet 5 a484abec61 Make the OBS adapter real: properties UI, connect, and frame output
The stub source becomes an actual streamer-tools camera. On create it reads
server URL / room slug / read key / camera identity from obs_data_t, mints a
subscribe-only token through ApiClient, connects LiveKitSession, and pushes
decoded frames into obs_source_output_video / obs_source_output_audio. The
source is now OBS_SOURCE_ASYNC_VIDEO | OBS_SOURCE_AUDIO |
OBS_SOURCE_DO_NOT_DUPLICATE with an OBS_ICON_TYPE_CAMERA icon.

The file is C++ rather than C now: the core library's API is C++ and the C ABI
shim existed only to avoid that. obs-module.h already declares the module
entry points extern "C", so nothing is lost.

Properties UI: server URL, room slug, a masked read-key field (it is a
credential and is masked everywhere else in streamer-tools), a camera dropdown,
a "Refresh camera list" button, and a status line.

- The dropdown is built from a cache the worker keeps warm on every connect,
  so opening properties never blocks on the network. The button is the
  explicit way to force a round trip, with a shortened 5s timeout -- for which
  ApiClient's two calls gained a timeout_ms parameter.
- The currently-selected identity is always in the list, labelled "(not in
  this room)" if absent, so OBS cannot silently clear a working setting just
  because the room happens to be dark.
- The status line is the OBS_TEXT_INFO property's description (which is what
  OBS actually renders) and switches to the warning info type on a real error.

Threading: OBS's UI and graphics threads are never blocked on the network.
Each source owns a worker thread that mints, connects, and reconnects with
exponential backoff (1s -> 30s), waking early on any settings change via a
generation counter. Frames are pushed from LiveKitSession's reader threads
directly; obs_source_output_video/_audio are thread-safe.

Two details that matter operationally:
 - A null frame is pushed whenever the session leaves Connected, so a camera
   that stopped publishing clears instead of leaving its last frame on screen.
   Leaving stale media up is precisely the failure this plugin exists to avoid.
 - The SDK's own logging is routed into OBS's log file via
   livekit::setLogCallback, instead of stderr where a director would never
   see it. The adapter also logs the first frame and every later geometry
   change, so a log answers "did video ever arrive, and at what size".

Packaging: the build now stages a runnable layout into build/package/ --
the module (RPATH $ORIGIN / @loader_path, so it resolves the LiveKit
libraries from beside itself rather than from the build tree), liblivekit +
liblivekit_ffi, the locale data, and the licence files. third_party/livekit/
carries client-sdk-cpp's Apache-2.0 LICENSE and NOTICE from the pinned tag.

Its README records a correction to the design doc: the "bundled LICENSE.md
with ~28 third-party licence blocks" the doc expects DOES NOT EXIST at
v1.10.1 -- not in any of the five release archives (which contain only
include/, lib/, bin/ and build-info.json) and not in the repo at that tag,
which has only LICENSE and NOTICE. The aggregated third-party notice covering
the WebRTC/OpenH264 code inside liblivekit_ffi.so has not been located, and
that is flagged as an open licensing question rather than papered over.

Verified on Ubuntu 24.04 against real libobs 30.0.2, a real
livekit-server 1.13.6, and a stand-in API serving plugin.routes.ts's exact
shapes, using a headless libobs harness (obs_startup + obs_reset_audio +
obs_reset_video + obs_open_module + obs_source_create):

  registered=1  output_flags=0x87
  [streamer-tools-camera] connected to ws://127.0.0.1:7880 as
      obs:main-room:qY85r9D0PaPt, watching cam-test
  [streamer-tools-camera] video frame 640x360 I420
  camera dropdown has 3 items:
    [0] (no camera selected) =
    [1] Test Camera = cam-test
    [2] Dark Camera (offline) = other-cam
  status: connected (info_type=0)

and with a deliberately wrong read key:

  status: unknown room slug, or the read key is wrong or has been rotated
      (info_type=1)

with retry-and-backoff and no crash. ctest: 6/6 passed.

Still unverified, and the README says so plainly: the OBS GUI on any platform,
macOS/Windows beyond compiling, A/V sync, and end-to-end latency.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RL8abRmgFXkVASHkkqiJbE
2026-09-06 21:54:11 -07:00
shadowdaoandClaude Sonnet 5 bf33966a4e Add the streamer-tools API client, with a real HTTP backend per platform
Implements the two read-key-scoped calls in
apps/server/src/obs/plugin.routes.ts: GET /api/obs/:slug/slots and
POST /api/obs/:slug/token.

Three pieces, all in core/ with no OBS dependency:

- stplugin::json -- a small, strict JSON reader. Hand-rolled rather than
  vendoring nlohmann because the only JSON this plugin ever sees is two
  fixed-shape responses from its own server, and the parser has to build on
  three platforms with no package-manager step in CI. It never throws,
  bounds its recursion (kMaxDepth=32) so a hostile response cannot overflow
  the stack inside OBS, rejects trailing garbage, and returns the caller's
  fallback for wrong-typed access instead of aborting.

- stplugin::HttpClient -- a two-method injectable interface, with libcurl
  behind it on Linux/macOS and WinHTTP on Windows. WinHTTP rather than curl
  on Windows because it ships with the OS and does TLS through SChannel: the
  self-hosted winvm-builder runner has no package manager, and per the
  scaffold README does not even have cmake preinstalled. Both backends cap
  the response body at 4 MiB, keep TLS verification on (the read key is a
  credential), and honour a whole-request timeout.

- stplugin::ApiClient -- maps the responses onto an ApiStatus enum that
  distinguishes NotFound (404), Unavailable (503), NetworkError,
  MalformedResponse and InvalidConfig. It deliberately does not claim to
  know whether a 404 was a wrong key or an unknown slug, because the server
  deliberately does not say. Server URLs are normalised the way an operator
  actually pastes them, defaulting to https so the read key is never sent in
  the clear by accident, and redactedUrl() exists so a URL can be logged
  without the key.

Tests (279 checks across two new suites) run at two levels: a fake
HttpClient covering every response and error branch, and a real loopback
HTTP server on 127.0.0.1 driving the actual platform backend -- so libcurl
on Linux/macOS and WinHTTP on Windows are each exercised in CI rather than
assumed. The loopback cases deliberately include the ones that must not hang
OBS: a truncated JSON body, a connection accepted and closed without a
reply, non-HTTP garbage, a dead port, and a stalled server that has to be
cut off by the client's own timeout.

Verified locally on Ubuntu 24.04:
  ctest --test-dir build --output-on-failure -> 4/4 passed
  test_json: 158 checks passed
  test_api_client: 121 checks passed

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RL8abRmgFXkVASHkkqiJbE
2026-09-06 21:28:55 -07:00