Repository navigation
Conversation
Adds in-call participant video: publish the local camera via WHIP and consume remote video via WHEP (H264 over SMB), with self-preview, per-tile video grid, source pinning, return-feed and fullscreen controls, plus the video-enabled toggle in production setup.
e37fafa to
e94e56f
Compare
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 +4752/−261 diff across 69 files with the branch checked out. Feature-scoped work that mirrors the existing audio patterns well, with good coverage of the new pure logic. Two concrete media-lifecycle bugs should be fixed before shipping a camera feature:
High
- Camera capture is never stopped on call exit (privacy/hardware leak).
production-line.tsx:149destructures onlyconst [inputVideoStream] = useVideoInput({...}), discarding the third tuple element — thereset()that stops the camera tracks.useVideoInput.reset(use-video-input.ts:67-72) is the only placevideoInput.getTracks().forEach(t => t.stop())runs, and it's never called. The effect cleanup (:62-64) only setsaborted = true;REMOVE_CALL(global-state-reducer.ts:123) doesn't stopmediaStreamVideoInput; andrtcPeerConnection.close()doesn't stop local getUserMedia tracks. Net: camera stays live + recording indicator on after leaving a video line, until the tab closes. Capture and call the videoreseton exit/unmount. - Frame-monitor
setIntervals leak on unmount.attachShowWhenReady(video-element-factory.ts:470-490) starts a 500ms interval per remote video element, cleared only bystopFrameMonitor, which is called only fromremoveThisElementon trackended/removetrack.useVideoElementscleanup (use-video-elements.ts:11-24) nullssrcObjectbut doesn't callstopFrameMonitor, nor does theuseRtcConnectionteardown. On abrupt teardown/navigation the interval + rVFC loop keep running against detached elements. CallstopFrameMonitorfor every element incleanUpVideo.
Medium
checkbox.tsx:76-87— input madereadOnly+tabIndex={-1}with toggle delegated to wrapperonClick, synthesizing a fakeChangeEventviaas unknown as. Removes keyboard focus/space-to-toggle from a shared primitive (a11y regression + scope creep). Prefer a nativedisabledprop.- PC held in
useState(never recreated) but stream refs are in the effect deps — a stream-ref change wouldclose()the PC and the re-run no-ops behindsignalingState === "closed"guards, dead-ending the call. Latent today (no device-switch re-acquire); add a guard/comment. - No test asserting camera tracks stop / frame monitors clear on exit — exactly where the leaks live.
Nits
- Raw
console.warn/errorin prod paths vs the projectloggerutil;video-element-factory.tsbuilds ~540 lines of tile chrome via imperativecreateElement+inline styles vs Emotionstyled()used elsewhere.
Notes: no user-input URL construction (all via backend API_URL), no new bundle secrets, reducer ERROR change well-contained, CI green. Both High items are small localized fixes.
…n#692) Capture the useVideoInput reset() and invoke it on exit and on unmount so camera tracks (getUserMedia) are stopped, releasing the hardware/indicator. Call stopFrameMonitor for every element in useVideoElements cleanUpVideo so the 500ms frame-monitor setInterval no longer leaks on teardown. 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-manager#315 (backend 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. |
|
#692 covers both High items — I'd written the same two fixes independently and landed on the same Two things still open:
This PR currently conflicts with Unrelated, noticed while in there: |
# Conflicts: # src/components/calls-page/calls-page.tsx # src/components/production-line/use-line-polling.ts # src/components/production-line/use-rtc-connection.ts
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). A large, well-structured video feature that correctly routes all state through the reducer (reusing UPDATE_CALL/extending DEVICES_UPDATED, no ad-hoc action types) and all REST calls through the API object — no blocking architectural or WebRTC violations. But several substantial new modules ship with no tests, there is a camera-track resource leak, and a block of pin-styling code is dead. Per project rules this cluster warrants Needs Changes. CI (build/lint/prettier/tests/e2e) is green — these are correctness/coverage findings, not formatting.
Blocking
- None.
Warnings
- No tests for the largest new modules.
video-element-factory.ts(543 lines: tile factory, draggable PiP, fullscreen, rVFC stall monitor) has no test file;use-video-source-pin.ts(retry/backoff-on-HTTP-425, latest-wins ref) has norenderHooktest;pin-data-channel.tssendPinnedEndpoint(data-channel message shape — a correctness-sensitive SMB contract) is untested;use-video-tile-grid.tsanduse-shadow-rtc-connection.ts(a full second WHIP/WHEP session lifecycle: peer connection, heartbeat, teardown) are untested. These pure/factory/hook units are the highest-value test targets. - Camera-track leak on input change —
use-video-input.ts:45-65. WhenvideoInputIdchanges, the effect cleanup only setsaborted = trueand never stops the previously acquired stream's tracks;setVideoInput(stream)replaces the reference. Switching cameras mid-call leaves the old camera active (indicator light stays on) and the old track detached but unstopped. Cleanup should stop the prior stream's tracks (only the explicitreset()on exit does today). - Dead pin-styling code —
video-element-factory.ts:394-403.updateVideoTileContainerPinnedqueries[data-tile-options-btn], but that attribute is never set on any element created increateVideoTileContainer, so the function always early-returns andcontainer.dataset.pinnedis never set. Pin reordering still works viatile.style.order, so the effect is cosmetic — but this code cannot run as written. - Duplicated sibling-line polling —
production-line.tsx:626-644. Reimplements participant polling as a rawwindow.setInterval(... API.fetchProductionLine ...)every 2s with.catch(() => {}), duplicatinguse-line-polling.tsand silently swallowing all errors.
Suggestions
- Use the project
logger(logger.red/logger.yellow) instead ofconsole.warn/console.error(use-rtc-connection.ts:84,use-video-input.ts:52). use-video-input.ts:54-59— theERRORdispatch omitscallId, so a per-camera failure shows a global banner rather than a call-scoped one.production-line.tsx:620,631,655— inlineimport("./types.ts").TParticipant/TLinerefs; use the existing named import.ice-gathering.tswaitForIceGatheringchanged from reject-on-timeout to resolve-on-timeout (5s→8s). This is a shared function, so it also alters the audio-only flow (a timeout now proceeds to PATCH with partial candidates instead of tearing down). Defensible robustness improvement, but validate against SMB in a real environment.
Cross-repo coupling (hard dependency on intercom-manager #315)
Non-functional in isolation: requires #315's new routes (PATCH .../whep-source, PATCH /session/:id/video-source), the new hasVideo (required)/isWhepReceiver/videoEnabled/whepSourceSessionId response fields (the auto-pin filter p.hasVideo && !p.isWhepReceiver silently no-ops if hasVideo is absent), the HTTP 425 not-ready contract (retry only triggers on 425), and SMB forwarding H264 + honoring PinnedEndpointsChanged. Do not merge ahead of #315 — treat as an atomic pair.
Moving the board item back to Ready for these changes.
# Conflicts: # src/components/production-line/use-line-polling.test.ts
# Conflicts: # src/components/production-line/production-line.tsx # src/components/production-line/use-rtc-connection.test.ts # src/components/production-line/user-list.tsx
Adds in-call participant video: publish the local camera via WHIP and consume remote video via WHEP (H264/VP8 over SMB), with self-preview, per-tile video grid, source pinning, return-feed and fullscreen controls, plus the video-enabled toggle in production setup.
Closes #692
Companion PR (backend): Eyevinn/intercom-manager#315