Repository navigation
Conversation
Adds per-line video to the intercom: WHIP publish and WHEP consume of H264 over SMB, video-source pinning with SSRC whitelisting, and the supporting session/line model fields (videoEnabled, hasVideo, isWhepReceiver, whepSourceSessionId) with pin reconciliation when a publisher leaves. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
5846e58 to
c5398f1
Compare
The WHIP/WHEP answer preferred H264 whenever the client offered it, without ever consulting what the bridge can actually carry. SMB's compiled default is VP8, so any deployment that does not set codec.videoCodec advertises VP8 only -- while every browser, OBS and whip-mpegts offers H264. The publisher then encoded H264 that SMB could not forward, and every receiver got working audio with permanently black video and no error on any code path: the pin, the ssrc-whitelist and the keyframe request all succeeded, each side individually self-consistent. Select the most preferred codec present in BOTH the offer and SMB's advertised payload-types. Applied in configureEndpointForWhipWhep and createWhipWhepAnswer alike: these must agree, since the answer decides what the publisher encodes and the configure decides what SMB expects, and a divergence between them is precisely the silent failure. Reject explicitly when there is no overlap, naming both sides, and log the negotiation so a mismatch is visible. When SMB advertises no video payload-types the previous preference order is kept unchanged, so existing deployments are unaffected. Verified on the OSC review env: a browser WHIP publisher against the VP8-only catalog SMB reported outbound-rtp codec video/H264 while a pinned consumer received no inbound video stream at all. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
hasVideo advertises a session as a pin source. The WHIP path set it from offerHasVideo -- the mere presence of a video m-line -- while the SSRCs it depends on were stored only conditionally, when the offer's FID group or a usable a=ssrc actually parsed. An offer with a video m-line but no parseable SSRCs therefore persisted hasVideo:true with an empty video.ssrcs. The publisher showed up as pinnable in the UI, but every pin to it resolved to an empty ssrc-whitelist and 425ed forever; the frontend retries 425 a few times, gives up silently, and leaves the tile on the previously pinned source. The browser path already guards against this (api_productions.ts) and its comment claims the WHIP path uses the same rule -- it did not. Bind the flip to the SSRCs actually persisted, and warn when an offer carries video that cannot be advertised, which was previously silent and left no way to recover the offer's shape from the logs afterwards. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Match the comment style established in "fix: clean up": short and factual about what the code does, rather than multi-line rationale blocks. Comments only -- no behaviour change, and the tests are untouched. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
birme
left a comment
There was a problem hiding this comment.
Automated code-reviewer verdict (daily-backlog-pr Phase 3): NEEDS CHANGES
Reviewed the full +2611/−191 diff across 24 files. Substantial, mostly careful WebRTC/SDP work with strong test coverage in several areas — but two real defects should block:
Blocking
src/api_productions.ts(~:1126) —reply.code(204)lost its.send(). The diff changesreply.code(204).send()→reply.code(204)on thePATCH /session/:sessionIdhandler (the browser's WebRTC handshake finalize call). The handler isasyncand now neither calls.send()nor returns a payload, so Fastify never flushes the response and the request hangs until socket timeout. Critical connect path, no test covers it. Restore.send()(and note 204 isn't in the declared response schema — 200/400/500 only).
High
- WHIP publisher DELETE does not reconcile pinned receivers (
src/api_whip.ts~:328). The/session/:sessionIdDELETE correctly doesgetReceiversPinnedToSession→reconfigureEndpoint(strip stalessrc-whitelist) →updateSessionVideoPin(..., null), but the WHIP DELETE path (how a publisher actually leaves) only callsclearWhepSourceIfPinnedand skips the receiver-whitelist reconciliation. Receivers pinned to the departed publisher keep a stale whitelist → frozen video until renegotiation. The headline "pin reconciliation when a publisher leaves" feature is only wired into one of the two publisher-removal paths.
Medium
- Unbounded
pinnedSessionIdstrings on newSetLineWhepSourceRequest/SetSessionVideoSourceRequestschemas (nomaxLength) — consistency/known-concern class here (low exploitability: used only as CouchDB_idlookup). VideoSmbPayloadParameterswidened toType.Record(Type.String(), Type.String())— removes key validation; add a comment / value-length bound.GET /session/:sessionId/namehas noschemablock at all (params or response) — inconsistent with every other route; new unauthenticated read surface.
Nits
- No tests for the 204 answer path (would have caught the blocker),
PATCH .../whep-source, orGET .../name. anytyping whereSfuVideoStreamexists; positional-boolean growth inallocateEndpoint/createEndpoint.
Good: correct ISmbProtocol usage, the transport-media find-by-fingerprint fix, keyframe cycle, NaN feedback guard, and codec-overlap rejection are solid and tested. Fix the blocker + WHIP-delete gap and this is close.
…P delete (Eyevinn#341) Restore reply.code(204).send() (and the 204 schema) on PATCH /session/:sessionId so Fastify flushes the WebRTC handshake finalize response instead of hanging until socket timeout. Mirror the /session/:sessionId DELETE receiver-whitelist reconciliation into the WHIP publisher DELETE path so receivers pinned to a departed publisher have their stale ssrc-whitelist stripped instead of freezing. Co-Authored-By: Claude Opus 4.7 <[email protected]>
|
This PR looks like part of a multi-repo/multi-PR dependency stack together with Eyevinn/intercom-frontend#684 (frontend video support). Holding off on automated review/merge here — this needs a human (or the relevant implementation agent) to assess the whole stack together, not a per-PR pass. |
Follow-up to Eyevinn#341, which fixed the two defects the review called blocking but left the schema findings and the WHIP-side tests. - PATCH /session/:sessionId returns 410 when the session is gone, but 410 was still undeclared (the response schema listed 204/400/500). - GET /session/:sessionId/name had no schema block at all, unlike every other route. - pinnedSessionId was unbounded on SetLineWhepSourceRequest and SetSessionVideoSourceRequest; bound to 200 like the WHIP param schemas. - VideoSmbPayloadParameters was widened to an unbounded record. The keys genuinely cannot be enumerated (they are codec-specific: x-google-* for VP8, profile-level-id / packetization-mode for H264), so it stays a record, but keys and values are now bounded and the reason is written down. Tests: the 410 answer path, GET .../name, and the WHIP delete reconciliation from Eyevinn#341 — including that a rejecting bridge still lets the session delete through. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Thanks — #341 covered the two items you flagged as blocking. Pushed a small follow-up for the
|
birme
left a comment
There was a problem hiding this comment.
Code Review — video support (WHIP/WHEP, H264) — Needs Changes
Reviewed by daily-backlog-pr (Phase 3). Substantial, well-documented SDP/WebRTC work with strong test coverage; the video path is cleanly gated behind videoEnabled/offerHasVideo so audio-only lines are largely unaffected. One blocking regression plus a few warnings before merge. CI (lint/pretty/ts) is green — these are correctness findings, not formatting.
Blocking
src/api_productions.ts:521and:1297— reintroduces the documented Sprint-5isActiveregression. Both participant maps now emitisActive: s.isWhip ? true : Boolean(s.isActive).backend-rulesrecords that this override was removed and the correct pattern is!!s.isActive/Boolean(s.isActive)("With MongoDB, WHIP/WHEP sessions never showed as inactive because of this override — this is now corrected.").maincurrently has the fixed form. Worse, this PR setsisWhip=truefor WHEP receivers too (api_whep.tscreateUserSession(..., true /*isWhip*/, true /*isWhepReceiver*/)), so both WHIP publishers and WHEP receivers would be reported permanently active and never age out.toUserResponse(:45-48) andproduction_manager.ts:589correctly useBoolean(...), so these two sites look like an accidental revert. UseisActive: Boolean(s.isActive); if always-active is genuinely intended for video sources, reconcile it withbackend-rulesand apply it consistently.
Warnings
src/api_productions.ts:1232andsrc/api_whip.ts:380— pin-reconciliation wrapsgetReceiversPinnedToSession+reconfigureEndpointin an emptycatch {}. An SMB reconfigure failure leaves a stalessrc-whitelist(video freezes) with no log. Not blocking the delete is fine, but at leastLog().warn(err).src/smb.ts(sendEndpointAction→reconfigureEndpoint/requestKeyframe) — new SMB REST calls have no request timeout (onlygetConferencesWithUsersis timed).requestKeyframeissues two sequential reconfigure PUTs; if SMB is unreachable these can block indefinitely. Add anAbortSignal.timeoutas elsewhere.- Unjustified
anyon SMB payload fields —sourceVideo: any,streams: any[],subscribeToVideo.streams: any[](api_whep.ts:147,161,163,api_productions.ts:1083,api_productions_core_functions.ts:163). Shapes are known (SfuVideoStream[]); prefer typing againstSmbEndpointDescription['video'].
Suggestions
parseInt(ssrcsSplit[0/1])atapi_productions_core_functions.ts:251,256,260omit radix10(inconsistent with the rest of the file).production_manager.ts:335-340,352—(s as any)._id/pinnedVideoSessionIdcasts can drop now thatpinnedVideoSessionIdis on the model.connection.ts:245— RTX PT advertised without a per-SSRCa=ssrc-group:FID; confirm with domain knowledge that SMB-originated offers don't need the FID group.- Good hardening noted: the
media.rtp?.[0]guard (:229-234) and thetransportMediafind-by-fingerprint||iceUfragfix are genuine correctness improvements.
Cross-repo coupling (with intercom-frontend #684)
This changes the wire contract: PATCH /session/:sessionId now 204 (was 200: string); new required hasVideo field + optional isWhepReceiver/videoEnabled/whepSourceSessionId; new routes (whep-source, video-source with 409/425 semantics, GET /session/:id/name); PT 96/97 normalization and H264 profile-level-id forcing. Merge as a coordinated pair with #684 and smoke-test that an existing audio-only production still negotiates.
Moving the board item back to Ready for these changes.
Conflicts were additive on both sides and resolved by keeping both: - src/smb.ts, src/mock-smb-protocol.ts: this branch's reconfigureEndpoint/requestKeyframe (plus the sendEndpointAction refactor of configureEndpoint) alongside main's deleteEndpoint. - src/db/couchdb.test.ts: the session-id/document-id separation suite alongside main's share-link suite. Textually clean files that broke semantically, fixed test-side so main's stricter validation wins: - api_whep.test.ts: stub clearWhepSourceIfPinned, which the WHEP DELETE handler now calls. - api_validation.test.ts: stub updateUserEndpoint and updateSessionHasVideo, added to the PATCH /session handler here. - api_whip.test.ts: use numeric production/line ids and MOCK_SESSION_ID so requests satisfy main's ^[0-9]+$ and UUID param patterns. Also corrects a stale assertion that already failed on main: Fastify 5 returns 404, not 414, when a path param exceeds maxParamLength, because the route stops matching. Co-Authored-By: Claude Opus 5 <[email protected]>
Adds per-line video to the intercom: WHIP publish and WHEP consume of H264/VP8 over SMB, video-source pinning with SSRC whitelisting, and the supporting session/line model fields (videoEnabled, hasVideo, isWhepReceiver, whepSourceSessionId) with pin reconciliation when a publisher leaves.
Closes #341
Companion PR (frontend): Eyevinn/intercom-frontend#684