[markusnygard]

Important

Depends on #621. Please merge #621 first.

Why: a commentator's page opens several WHEP sessions at once (the return picture and one or
more previews), and each binds its Strom session id into the same guest session doc. Without a
retry on CouchDB's 409, all but the first binding are lost, and the guest can't tear those
sessions down (the review's critical finding). The fix, bindWhepSession, also fixes a bug that is
already on main: the guest page opens the return picture and talkback at the same time, and the
return-picture binding has no retry. So it is its own small PR, #621, mergeable without the product
decisions this PR still needs.

Until #621 is merged, the single commit here includes the same change. Once it is merged, I'll
rebase, and this PR will carry only the commentator preview.

Motivation

For remote commentary, operators often delay incoming sources with the per-source time offsets in the vision mixer, so that commentary and picture line up on air. Every WHEP output (program, multiview, monitor, guest returns) comes after those offsets. A remote commentator (an Open Live guest) commenting on, say, a stadium camera therefore sees that source late, and needs to watch it undelayed.

We are building a remote commentary client on top of Open Live guest calling and Open Intercom, and this is the one piece we cannot do from outside Open Live. The use case and the product questions are in #622.

What this adds

• Flag: commentatorPreview?: boolean on a production's source assignment (POST /api/v1/productions/:id/sources). It is accepted on SRT, EFP and clip sources, whose input block has raw pads; any other source kind gets 400.
• Flow: for each flagged source, the flow generator adds a builtin.videoenc (4 Mbit/s, like Enc View) and a low-latency builtin.whep_output (audio_video, one audio track).
  • Both are fed from the input block's raw video_out and audio pad, the same pads that feed its time_offset blocks. So the preview is before the offsets and outside the mixer, and its audio is the source's own, undelayed. For SRT it is the audio stream selected for the mixer (audio-stream selection).
  • Endpoint ids are made unique per production (whep-preview-{pad}-{suffix}), like the return endpoints.
• Activation stores previewWhepUrls ({ mixerInput, sourceId, url, endpointId }) on the production; deactivation clears them.
• WHEP signaling routes use the same server-side target derivation as the return picture, never /whep-proxy?target=. The return-picture proxy is refactored into a shared proxyScopedWhep / teardownScopedWhep, with no change in behaviour or log lines.
  • Crew (shared key): POST/DELETE /api/v1/productions/:id/previews/:mixerInput/whep[/:sessionId]
  • Guest (invite token): POST/DELETE /api/v1/guests/:inviteId/previews/:mixerInput/whep[/:sessionId]
• Scoping: a guest may watch any commentator preview of their own invite's production, and nothing else.
  • Session ids are bound to the guest session (previewWhepSessionIds, through bindWhepSession from fix(returns): keep the return-picture session binding on a revision conflict #621), so a guest can only tear down their own sessions, also when they open several at once.
  • Unflagging a source stops new viewers immediately, even before the next activation. So does re-assigning a flagged input to another source while active: the running output still carries the old source, so it is not served under the new source's name until the next activation.
• Discovery: the guest join and slot responses include previews: [{ mixerInput, label, url }]; feeds is unchanged.
• Docs: OpenAPI, and a "Commentator preview" section in docs/specs/guest-calling-intercom.md (including concurrent binding, re-posting a preview, and the source swap).

Cost

One extra 4 Mbit/s encode and one WHEP output per flagged source, whether or not anyone is watching. That's why it is opt-in and off by default.

Tests

• Flow generator: for SRT, EFP and clip sources, the encoder and WHEP blocks are linked from the input's raw pads, not the offset blocks. The preview carries the audio stream selected for the mixer. Endpoint ids are unique. Nothing is added for unflagged sources, or for WHIP, HTML and test-pattern sources.
• Activation and deactivation: activation stores previewWhepUrls; deactivation clears them.
• Assignment API: the flag is persisted on SRT, EFP and clip sources, and other kinds get 400.
• Crew route:
  • proxies to the right target and rewrites Location
  • returns 404 for an unflagged input and 409 for an inactive production
  • tears down cleanly, and refuses guest tokens
• Guest route (the session mock enforces _rev like CouchDB):
  • works only within the guest's own production
  • returns 404 for unflagged or stale inputs, and for an input re-assigned to another source while active, and 409 when inactive
  • returns 401 without a token, with the shared key, or after the guest leaves
  • binds sessions, so a guest can delete their own (204) but not another guest's (403)
  • two previews and the return picture opened at once, all reading the same revision: all three are bound and each can be deleted (fails without the retry)
  • a guest who leaves while the offer is in flight is not bound
• Join and slot responses include previews.

Full suite: 1995 tests in 176 files pass, and tsc --noEmit and the build are clean.

Not yet verified

This has only been tested with Strom and CouchDB mocked, not against a live Strom. It relies on two assumptions we'd appreciate a check on:

• Strom fans a block's raw video_out / audio pad out to several links, as pgm_out already does in the template.
• The clip player's audio_out is raw audio.

We have no Strom access on our OSC tenant. Could a maintainer run it on openlivedev? What to check: a production with a flagged SRT or EFP source and a flagged clip source activates and reaches playing, and a WHEP client on /api/v1/productions/:id/previews/video_in_N/whep receives video and audio.

HTML and test-pattern sources are left out, because their raw pads are on bare GStreamer elements (cefdemux, audiotestsrc).

🤖 Generated with Claude Code

[eyevinn-pr-reviewer]

PR Review: Add opt-in commentator preview: undelayed per-source WHEP output for remote commentators

Prior findings

#	Source	Finding	Status
-	-	No earlier reviews, issue comments or inline comments on this PR	n/a

Verdict: REQUEST CHANGES

Thanks for this, @markusnygard. It's a careful, well-documented contribution. The security model matches the existing return-picture design, the refactor is clean, and the tests are thorough. I'm requesting changes for three reasons: one concrete bug in the guest session binding, the Strom assumptions you flagged yourself, and the fact that this adds a new guest-facing API contract without a linked issue. Under this project's workflow, a new contract like that needs a maintainer product decision before merge. None of this is a judgement on the quality of the code.

Critical issues (must fix before merge)

• Preview session binding is lost when a guest opens previews concurrently (src/routes/returns.ts, guest POST .../previews/:mixerInput/whep → bindGuestSession). bindGuestSession spreads the GuestSessionDoc that was read once, at the start of the request, by resolveGuestSession (it includes _rev), and inserts it after the upstream WHEP round-trip. Suppose a commentator client opens two previews in parallel, or a preview and the return picture. Both requests read rev N. The first insert succeeds, and the second gets a CouchDB 409. That failure is only logged (Preview WHEP: guest session-id bind failed), so the second previewWhepSessionIds[mixerInput] is never stored. The guest's later DELETE of that session returns 403, and the Strom session lingers until ICE/consent timeout. This does fail closed, so it is not a security problem. But it breaks teardown in the multi-preview case this feature is built for. Suggested fix: on 409, re-read the session doc by _id, re-check state !== 'left', merge the one key into the fresh previewWhepSessionIds (or set returnWhepSessionId), and retry a bounded number of times (like MAX_DB_WRITE_RETRIES in productions.ts). Please also add a test where the session mock enforces _rev. The current sessionsStore mock accepts any rev, which is why the suite doesn't catch this.

• The Strom assumptions need live verification before merge. As your "Not yet verified" section says, the flow has only been exercised against a mocked Strom. I checked both assumptions against main, and both look plausible:

  • A raw block pad fanning out to several links: pgm_out already feeds both Enc PGM and Enc View in src/lib/default-flow.ts:120-121, and mixer main_out fans out to every WHEP output.
  • The clip player's audio_out being raw: on main it already feeds a time_offset block that goes into the mixer (flow-generator.ts:872).

  Still, "plausible by analogy" isn't enough for a change that edits the generated flow of every production with a flagged source. If Strom rejects the topology, activation fails (Strom returns a 502 on a bad pipeline). Please attach evidence from a real Strom: a flagged SRT or EFP source and a flagged clip source, the flow reaching playing, and a WHEP client receiving both video and audio. Alternatively, a maintainer can run this on the openlivedev instance.

Needs a maintainer product decision (not something the contributor can resolve alone)

• This PR adds a new public API contract: the commentatorPreview assignment flag, two crew routes, two guest routes, and the previews field on the guest join/slot responses. It also sets the product semantics in docs/specs/guest-calling-intercom.md itself. There is no linked issue. Under the project's epic workflow, a new API contract that other repos will consume needs a maintainer-approved spec before implementation: open-live-studio will need a toggle, and join-live or your own client will consume previews. These are the points that need a decision:

  1. Who may watch. As written, every guest of the production, including an interviewee or any WHIP contributor, can watch every flagged source undelayed. Is production-wide access acceptable, or should previews be limited per invite (for example, a commentator role or an allow-list on the invite)?
  2. Always-on cost. Each flagged source adds an extra 4 Mbit/s encode and a WHEP output for the whole production, whether or not anyone is watching, on a GPU that may be shared. Is per-source opt-in the right control, or should there be a cap?
  3. Contract shape. Is it OK to add previews as a required field on the join/slot response schemas, and to use /previews/:mixerInput/whep as the route naming?

  @markusnygard, could you open an issue on Eyevinn/open-live describing the remote-commentary use case and link it here? That gives maintainers a place to make these calls. If they accept the current design, the spec section in this PR can stand as written.

Warnings (should fix)

• Stale target after a live source swap. previewTarget checks the current assignment's flag but returns the URL that was stored at activation. Suppose an operator re-assigns a flagged mixerInput to a different SRT/EFP/clip source with commentatorPreview: true while the production is active. The guest then sees the new source's name as label (guestPreviewFeeds), but the video and audio are still the old source's tap until the next activation. A fix is to store sourceId in each previewWhepUrls entry and require it to match the current assignment in previewTarget. Alternatively, document it as a known limitation, as is already done for marking a source.
• Rebinding the same input overwrites the earlier session id. If a guest re-POSTs a preview for an input without first DELETEing the old session (a reconnect, for example), the earlier session can no longer be torn down by the guest. This matches the existing returnWhepSessionId behaviour, so it isn't a regression. It's worth a sentence in the spec.

Nits

• In the shared proxyScopedWhep path, log messages changed from return picture WHEP: ... to Return WHEP: .... That's harmless, but anyone with log alerts or searches on the old strings would need to update them.

What was verified

• Read the full diff (15 files) against main at 2fc18a0 (the merge base, which is also current main). There is one commit, with a co-author trailer, and no unrelated changes. No secrets, credentials, eval/child_process, new dependencies or lockfile changes.
• Auth and scoping:
  • The new guest-exempt regex in server.ts isGuestTokenAuthedPath covers only previews/[^/]+/whep(/[^/]+)?. Both guest handlers verify the token through resolveGuestSession, which checks the HMAC, revocation, the stored hash, the stored expiry and that the session is live.
  • Both guest handlers also check who.invite._id === :inviteId and take the production from invite.productionId, never from the request.
  • The crew paths are not in guest-scope.ts's guest-eligible families, so a guest token falls through to 401 (also covered by a test). Only POST and DELETE are registered under the exempt prefix.
• SSRF: the WHEP target always comes from previewWhepUrls, which are built server-side as ${config.stromUrl}/whep/${endpointId}. The target is re-checked with assertSameStromOrigin. Teardown rebuilds origin/whep/:sessionId from the stored URL, never from client input. Guest DELETE requires previewWhepSessionIds[mixerInput] === :sessionId (otherwise 403). A prototype-key mixerInput such as __proto__ gets a 404 from previewTarget before any bind happens.
• Return-picture refactor: it really is behaviour-preserving. The target is still returnStromUrl(doc, mixerInput), the origin-check label is still 'Return target', the bound doc is the same {...guestSession, returnWhepSessionId, updatedAt}, and teardown uses the same base. Only the log strings changed.
• Flow generator:
  • Pad names match what main already uses: SRT/EFP inputId:video_out / audio_out_0, clip playerId:video_out / audio_out, videoenc video_in → encoded_out, whep_output video_in / audio_in, and bitrate: 4000 as on Enc View.
  • The preview blocks are pushed after the template WHEP audio-rewiring loop and the endpoint-id uniquification pass, so neither touches them.
  • Endpoint ids already carry the production suffix. The new encoders join encBlocks, which only shifts where the loudness block and drains are placed.
• Activation writes previewWhepUrls unconditionally (it is cleared when empty), and deactivatedDoc clears it. The flag is validated at assignment time (400 for other source types, 503 on a DB error) and checked again in the flow generator.
• Local checks (fresh clone of the PR head): pnpm install --frozen-lockfile ✔, pnpm run typecheck ✔, pnpm run build ✔, npx vitest run ✔ (130 files / 1331 tests passed).
• CI: build-and-test (run 39143781766) passed.

[birme]

This PR's branch now has a real merge conflict with main (after today's merges to main), so GitHub's automated "Update branch" can't refresh it and this repo's strict required status checks keep it from merging as-is. Please merge current main into your branch and resolve the conflicts (a normal merge commit is fine, no force-push needed). Nothing else about the PR's review state changes because of this note.

[markusnygard]

Thanks for the thorough review.

⚠️ This PR needs #621 to be merged first. The concurrent-binding fix you asked for turned out to be
a bug on main as well: the guest page opens the return picture and talkback at the same time, and the
return-picture binding has no 409 retry. So it is now its own small PR, #621. Until #621 is merged, the
single commit here includes the same change; once it's merged I'll rebase, and this PR will only carry
the commentator preview.

The branch is one commit on top of current main (force-pushed, replacing the earlier commit and the
merge commits), with:

• Rebased on main and resolved the conflicts (talkback routes in isGuestTokenAuthedPath, ICE servers
  on join, record next to commentatorPreview on assignments, input recording in the flow generator). The
  preview now taps the selected audio stream (audioPad from the audio-stream selection), so it carries
  the same sound the mixer gets, before its offset. Activation tolerates a flow result without
  previewWhepEntries, so the new recording tests' mocks don't need to know about it.
• Concurrent binding (critical): preview session ids are bound through bindWhepSession from fix(returns): keep the return-picture session binding on a revision conflict #621: on a
  409 it re-reads the session doc, requires state !== 'left', applies only its own key
  (previewWhepSessionIds[mixerInput] merged into the fresh map) and retries, at most 4 writes.
  • The session mock in commentator-preview-routes.test.ts now enforces _rev like CouchDB.
  • New test: two previews and the return picture opened at once, with the Strom POSTs held until all three
    have read the same revision; all three are bound and each can be deleted. It fails on the previous code.
  • New test: a guest who leaves while the offer is in flight is not bound.
• Stale target after a live source swap: previewWhepUrls entries carry the sourceId they were built
  for, and previewTarget requires it to match the current assignment. Re-assigning a flagged input to
  another source while active stops new viewers (and drops it from previews) until the next activation.
  Test added.
• Rebinding overwrites the earlier id: documented in the spec, with the source swap and the concurrent
  binding.
• Nit: return-picture log lines are back to return picture WHEP: …; previews log preview WHEP: ….

pnpm run typecheck, pnpm run build and npx vitest run (176 files, 1995 tests) pass locally.

Product decisions: opened #622 with the remote-commentary use case and the three questions.

Real Strom: I can't run this on a real Strom myself (our OSC tenant has no Strom access). Could a
maintainer run it on openlivedev? What to check: a production with a flagged SRT or EFP source and a flagged
clip source activates and reaches playing, and a WHEP client on /api/v1/productions/:id/previews/video_in_N/whep
receives video and audio.

[eyevinn-developer]

@markusnygard #621 (bindWhepSession, revision-conflict handling) is merged to main as 353e25d. Could you rebase this PR onto current main so it carries only the commentator-preview change? src/lib/db-errors.ts, src/routes/returns.ts and src/__tests__/returns-guest-auth.test.ts should then drop out of the diff, or only contain what #542 itself needs.

Once that's in, #624 tracks the rest: verifying against a live Strom on openlivedev, then re-reviewing against the 2026-10-19 findings.

[eyevinn-developer]

Hi @markusnygard. #621 merged into main today (2026-11-05 08:25Z), and this branch now conflicts with main in one file: src/routes/returns.ts. GitHub's update-branch refuses with HTTP 422 ("merge conflict between base and head"), and the branch is on your fork, so only you can push the fix.

This is a textual conflict only. Your commit 2cbadb3 already carries #621's bindWhepSession retry, isDocConflictError and tests byte for byte, so src/lib/db-errors.ts and returns-guest-auth.test.ts merge cleanly. The conflict happens because #621 changed the proxyReturnPictureWhep call site inline, while this PR parameterised it into proxyScopedWhep.

To resolve: git merge origin/main, keep this PR's side of returns.ts, and optionally take #621's shorter call-site comment:

@@ -128,10 +128,7 @@ async function proxyScopedWhep(args: {
   // Bind this WHEP session id to the guest's live session (issue #380) so the
   // matching DELETE can verify a guest caller only tears down their OWN
-  // session — never another guest's :sessionId. Best-effort: a persist failure
-  // here does not fail the (already-established) upstream session; the guest
-  // just fails closed on their next DELETE instead of silently trusting an
-  // unbound id (see the DELETE handlers below).
+  // session — never another guest's :sessionId.
   if (bindGuestSession && sessionId) {

We checked that resolution locally: pnpm typecheck passes, and pnpm test passes 2042 of 2042 tests. Once the branch is level with main, #624 continues with the live-Strom verification on openlivedev and the re-review.

[eyevinn-developer]

Closing this as superseded by #626.

On #624 (2026-11-05), @svensson00 ruled: "If there's no push by 2026-11-09, go with (b): carry #542 forward on an Eyevinn/open-live branch with Markus's authorship kept, close #542 as superseded, then run the live-Strom verification and re-review here." There's been no push since then (the head is still 271d2bf), so the team has done that.

@markusnygard, #626 is your commit 2cbadb3, cherry-picked onto current main with you still the git author. The src/routes/returns.ts conflict is resolved exactly as posted above: your side of the file, with #621's shorter call-site comment. #621's code now comes only from main, so the diff carries only the commentator preview. Typecheck and build are clean, and 2042/2042 tests pass. The only other change is a : string annotation on the TEST_API_KEY test fixture, needed to get past our commit guard's secret pattern.

The live-Strom verification and the re-review against the 2026-10-19 findings continue on #626. Thanks for the contribution, and you're welcome to follow along or comment there.