From 002e4112ea125631191426e076912902874d21cc Mon Sep 17 00:00:00 2001 From: opencode-agent Date: Sat, 17 Oct 2026 18:11:19 +0000 Subject: [PATCH] WebRTC: bounded ICE gathering and remove dead signaling code (#659) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem ------- WebRTCVideoCell.jsx contained a commented-out trickle-ICE block that POSTed candidates to /api/webrtc/ice, a route that exists in neither LightNVR nor go2rtc's HTTP API. Worse, the log line said "waiting for ICE gathering..." but the code did not wait at all: the offer was POSTed to {go2rtcBaseUrl}/api/webrtc?src=... immediately after setLocalDescription(), so the SDP typically carried zero ICE candidates. Meanwhile pc.onicecandidate and pc.onicegatheringstatechange logged every candidate and state transition with console.log, which floods the console on a 16-camera grid. Findings (current flow, measured) --------------------------------- Verified against go2rtc v1.9.14 with an ffmpeg test source, headless Chromium, and a proxy that mimics LightNVR's /go2rtc/ reverse proxy (median of 3 runs per scenario, timestamps relative to page load): - OLD flow: offer POSTed ~25ms after local description set with **0 candidates** in the SDP (pool size 0, gathering races the fetch). - On a LAN (empty iceServers): ICE connected ~60ms, first frame ~650ms. Change (option (b) from the issue) ---------------------------------- Signaling stays a single HTTP POST to go2rtc /api/webrtc, but the offer is now sent when ICE gathering completes **or a 1s bound expires**, whichever comes first, via a new waitForIceGathering() helper (web/js/utils/webrtc-ice-gathering.js). The cell abort signal also ends the wait so unmount/retry can never hang. Host candidates now actually reach go2rtc inside the offer, and an unreachable STUN/TURN server can only stall the connect by 1s instead of the browser's multi-second gathering timeout. Option (a) (real trickle over go2rtc's /api/ws WebSocket) was rejected: the /go2rtc/ reverse proxy in lightNVR is an HTTP-only libcurl proxy (src/web/api_handlers_go2rtc_proxy.c, go2rtc_proxy_thread.c) with no WebSocket upgrade support, so it would require a non-trivial backend change for marginal benefit over the bounded wait. This change is frontend-only. - Dead commented trickle block deleted; comments now explain why there is no trickle channel. - Per-candidate and gathering-state logs moved from console.log to console.debug (existing convention, e.g. auth-utils.js) — quiet by default, still visible with devtools verbose logging. Measured results (before/after) ------------------------------- median of 3 runs, Chromium headless, LAN: scenario gather cands ICE conn frame OLD no wait, no STUN 0ms 0 60ms 664ms NEW bounded 1s, no STUN 117ms 2* 168ms 697ms OLD no wait, bogus STUN 0ms 0 48ms 734ms NEW bounded 1s, bogus STUN 1001ms 2* 1046ms 1686ms NAIVE full wait, bogus STUN 30001ms 2* 30053ms 30069ms (* = host candidates) — bogus STUN = stun:192.0.2.1:3478 (TEST-NET-1, unreachable). "NAIVE full wait" hit the 30s harness cap; a real browser takes ~5-7s. Bounded gathering keeps the stall at exactly 1s in that case and costs ~30ms on a normal LAN (negligible vs 660ms TTFF), while the offer now carries usable host candidates in both cases. All 5 scenarios connected 3/3 through the /go2rtc/ proxy path. Validation ---------- - cd web && npx vite build: passes (version.js pre-generated by scripts/extract_version.js, as scripts/build_web_vite.sh does). - npm run lint: 0 errors (only pre-existing a11y warnings). - npm test: 373 passed; 5 failures in tests/timeline-utils.spec.js are date-sensitive and reproduce identically on unmodified main. - New tests: web/tests/webrtcIceGathering.spec.js (10) for the bounded wait helper. Not validated here (needs a deployed instance) ---------------------------------------------- - Manual check in Chrome and Firefox against a running lightNVR instance with real cameras, with and without auth. Reviewer plan: on the live page, confirm "waiting for ICE gathering (up to 1000ms)..." then "Sending offer directly to go2rtc ... (ICE gathering: complete)" and that first frames arrive; confirm the console shows no per-candidate logs at default level; re-test with a bogus ice_servers entry via /api/ice-servers config and confirm connect still succeeds within ~1s extra. TTFF before/after is available from /api/telemetry/player (ttff_ms) since player-telemetry already reports it. Rollback -------- Revert this commit; the helper module and its tests are additive only. --- web/js/components/preact/WebRTCVideoCell.jsx | 58 ++++++---- web/js/utils/webrtc-ice-gathering.js | 78 +++++++++++++ web/tests/webrtcIceGathering.spec.js | 111 +++++++++++++++++++ 3 files changed, 225 insertions(+), 22 deletions(-) create mode 100644 web/js/utils/webrtc-ice-gathering.js create mode 100644 web/tests/webrtcIceGathering.spec.js diff --git a/web/js/components/preact/WebRTCVideoCell.jsx b/web/js/components/preact/WebRTCVideoCell.jsx index b1f8af6f..2ccdf9e2 100644 --- a/web/js/components/preact/WebRTCVideoCell.jsx +++ b/web/js/components/preact/WebRTCVideoCell.jsx @@ -23,6 +23,11 @@ import { createPlayerTelemetry } from '../../utils/player-telemetry.js'; import { useAutoRetry } from './useAutoRetry.js'; import { useVideoZoom } from './useVideoZoom.js'; import { streamConnectionGate, priorityForStreamStatus, isGateTimeout, isGateAbort } from '../../utils/stream-connection-gate.js'; +import { + DEFAULT_ICE_GATHERING_TIMEOUT_MS, + ICE_GATHER_OUTCOME, + waitForIceGathering, +} from '../../utils/webrtc-ice-gathering.js'; import { shouldFallbackFullscreenToSubStream } from './liveStreamPolicy.js'; import { LiveTileStatus } from './LiveTileStatus.jsx'; import { PictureInPictureButton } from './PictureInPictureButton.jsx'; @@ -716,32 +721,23 @@ export function WebRTCVideoCell({ pc.oniceconnectionstatechange = handleIceConnectionStateChange; - // Handle ICE gathering state changes + // Handle ICE gathering state changes (debug level: this fires once per + // state transition per cell, which is noisy on multi-camera grids). pc.onicegatheringstatechange = () => { - console.log(`ICE gathering state for stream ${stream.name}: ${pc.iceGatheringState}`); + console.debug(`ICE gathering state for stream ${stream.name}: ${pc.iceGatheringState}`); }; - // Handle ICE candidates - critical for NAT traversal + // Log ICE candidates at debug level. go2rtc's HTTP WebRTC endpoint + // (/api/webrtc) has no trickle-ICE channel — there is no server-side + // route that accepts candidates, so candidates travel to go2rtc inside + // the SDP offer itself (see the bounded gathering wait below). Trickle + // would require go2rtc's WebSocket API (/api/ws), which the HTTP-only + // /go2rtc/ reverse proxy does not forward. pc.onicecandidate = (event) => { if (event.candidate) { - console.log(`ICE candidate for stream ${stream.name}:`, event.candidate.candidate); - - // Send the ICE candidate to the server - // Note: go2rtc typically handles ICE candidates in the SDP exchange, - // but we log them here for debugging purposes - // If trickle ICE is needed, uncomment the code below: - /* - fetch(`/api/webrtc/ice?src=${encodeURIComponent(stream.name)}`, { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - ...(auth ? { 'Authorization': 'Basic ' + auth } : {}) - }, - body: JSON.stringify(event.candidate) - }).catch(err => console.warn('Failed to send ICE candidate:', err)); - */ + console.debug(`ICE candidate for stream ${stream.name}:`, event.candidate.candidate); } else { - console.log(`ICE gathering complete for stream ${stream.name}`); + console.debug(`ICE gathering complete for stream ${stream.name}`); } }; @@ -790,9 +786,27 @@ export function WebRTCVideoCell({ return pc.setLocalDescription(offer); }) .then(() => { - console.log(`Set local description for stream ${stream.name}, waiting for ICE gathering...`); + // ICE gathering runs in the background once the local description is + // set. go2rtc's HTTP endpoint has no trickle channel, so any candidate + // that is going to reach it must be baked into this offer. Wait for + // gathering to complete, bounded so an unreachable STUN/TURN server + // can only stall the connect by the timeout, not by the browser's + // multi-second gathering timeout. + console.log( + `Set local description for stream ${stream.name}, waiting for ICE gathering ` + + `(up to ${DEFAULT_ICE_GATHERING_TIMEOUT_MS}ms)...` + ); + return waitForIceGathering(pc, DEFAULT_ICE_GATHERING_TIMEOUT_MS, cellAbortSignal); + }) + .then((gatherOutcome) => { + if (gatherOutcome === ICE_GATHER_OUTCOME.TIMEOUT) { + console.warn( + `ICE gathering timed out after ${DEFAULT_ICE_GATHERING_TIMEOUT_MS}ms for stream ` + + `${stream.name}, sending offer with partial candidates` + ); + } - console.log(`Sending offer directly to go2rtc for stream ${stream.name}`); + console.log(`Sending offer directly to go2rtc for stream ${stream.name} (ICE gathering: ${gatherOutcome})`); // Send the offer to go2rtc through the shared connection gate. The // gate bounds concurrent offers page-wide and enforces a per-attempt diff --git a/web/js/utils/webrtc-ice-gathering.js b/web/js/utils/webrtc-ice-gathering.js new file mode 100644 index 00000000..ab75a24a --- /dev/null +++ b/web/js/utils/webrtc-ice-gathering.js @@ -0,0 +1,78 @@ +/** + * Bounded ICE gathering helper for the go2rtc HTTP signaling flow. + * + * go2rtc's HTTP endpoint (`POST /api/webrtc?src=...`) has no trickle-ICE + * channel: the SDP offer is the only place client candidates can travel. + * Waiting indefinitely for `icegatheringstate === 'complete'` lets an + * unreachable STUN/TURN server stall the connect for several seconds, so the + * wait is bounded — the offer goes out with whatever candidates exist when + * gathering completes or the timeout expires, whichever comes first. + */ + +export const DEFAULT_ICE_GATHERING_TIMEOUT_MS = 1000; + +export const ICE_GATHER_OUTCOME = { + COMPLETE: 'complete', + TIMEOUT: 'timeout', + ABORTED: 'aborted', +}; + +/** + * Wait for a peer connection's ICE gathering to complete, bounded by a + * timeout. + * + * @param {RTCPeerConnection} pc - The peer connection to watch. + * @param {number} [timeoutMs] - Maximum time to wait before resolving. + * @param {AbortSignal} [abortSignal] - Optional signal; aborting ends the + * wait early (e.g. the cell unmounted or a retry superseded this attempt). + * @returns {Promise} Resolves with one of ICE_GATHER_OUTCOME values. + */ +export function waitForIceGathering( + pc, + timeoutMs = DEFAULT_ICE_GATHERING_TIMEOUT_MS, + abortSignal = null +) { + return new Promise((resolve) => { + if (!pc || pc.iceGatheringState === 'complete') { + resolve(ICE_GATHER_OUTCOME.COMPLETE); + return; + } + if (timeoutMs <= 0) { + resolve(ICE_GATHER_OUTCOME.TIMEOUT); + return; + } + if (abortSignal && abortSignal.aborted) { + resolve(ICE_GATHER_OUTCOME.ABORTED); + return; + } + + let settled = false; + const finish = (outcome) => { + if (settled) return; + settled = true; + clearTimeout(timer); + pc.removeEventListener('icegatheringstatechange', onStateChange); + if (abortSignal) { + abortSignal.removeEventListener('abort', onAbort); + } + resolve(outcome); + }; + const onStateChange = () => { + if (pc.iceGatheringState === 'complete') { + finish(ICE_GATHER_OUTCOME.COMPLETE); + } + }; + const onAbort = () => finish(ICE_GATHER_OUTCOME.ABORTED); + + const timer = setTimeout(() => finish(ICE_GATHER_OUTCOME.TIMEOUT), timeoutMs); + // Under Node (unit tests) setTimeout returns a Timeout object; unref it so + // a pending gather bound never keeps the test process alive. + if (typeof timer === 'object' && typeof timer.unref === 'function') { + timer.unref(); + } + pc.addEventListener('icegatheringstatechange', onStateChange); + if (abortSignal) { + abortSignal.addEventListener('abort', onAbort); + } + }); +} diff --git a/web/tests/webrtcIceGathering.spec.js b/web/tests/webrtcIceGathering.spec.js new file mode 100644 index 00000000..cbec55d6 --- /dev/null +++ b/web/tests/webrtcIceGathering.spec.js @@ -0,0 +1,111 @@ +import { + DEFAULT_ICE_GATHERING_TIMEOUT_MS, + ICE_GATHER_OUTCOME, + waitForIceGathering, +} from '../js/utils/webrtc-ice-gathering.js'; + +function fakePeerConnection(initialState = 'gathering') { + const listeners = {}; + return { + iceGatheringState: initialState, + listenerCount(type) { + return (listeners[type] || []).length; + }, + addEventListener(type, fn) { + listeners[type] = listeners[type] || []; + listeners[type].push(fn); + }, + removeEventListener(type, fn) { + if (!listeners[type]) return; + listeners[type] = listeners[type].filter((l) => l !== fn); + }, + setGatheringState(state) { + this.iceGatheringState = state; + (listeners.icegatheringstatechange || []).slice().forEach((fn) => fn()); + }, + }; +} + +describe('waitForIceGathering', () => { + test('exposes a 1s default bound', () => { + expect(DEFAULT_ICE_GATHERING_TIMEOUT_MS).toBe(1000); + }); + + test('resolves immediately when gathering is already complete', async () => { + const pc = fakePeerConnection('complete'); + await expect(waitForIceGathering(pc, 50)).resolves.toBe(ICE_GATHER_OUTCOME.COMPLETE); + expect(pc.listenerCount('icegatheringstatechange')).toBe(0); + }); + + test('resolves immediately for a missing peer connection', async () => { + await expect(waitForIceGathering(null, 50)).resolves.toBe(ICE_GATHER_OUTCOME.COMPLETE); + }); + + test('resolves complete when gathering finishes before the timeout', async () => { + const pc = fakePeerConnection('gathering'); + const promise = waitForIceGathering(pc, 1000); + expect(pc.listenerCount('icegatheringstatechange')).toBe(1); + pc.setGatheringState('complete'); + await expect(promise).resolves.toBe(ICE_GATHER_OUTCOME.COMPLETE); + expect(pc.listenerCount('icegatheringstatechange')).toBe(0); + }); + + test('resolves timeout when gathering stalls past the bound', async () => { + const pc = fakePeerConnection('gathering'); + await expect(waitForIceGathering(pc, 25)).resolves.toBe(ICE_GATHER_OUTCOME.TIMEOUT); + expect(pc.listenerCount('icegatheringstatechange')).toBe(0); + }); + + test('resolves timeout immediately for a non-positive bound', async () => { + const pc = fakePeerConnection('gathering'); + await expect(waitForIceGathering(pc, 0)).resolves.toBe(ICE_GATHER_OUTCOME.TIMEOUT); + expect(pc.listenerCount('icegatheringstatechange')).toBe(0); + }); + + test('ignores intermediate states that are not complete', async () => { + const pc = fakePeerConnection('new'); + const promise = waitForIceGathering(pc, 250); + let settled = false; + promise.then(() => { + settled = true; + }); + pc.setGatheringState('gathering'); + await new Promise((r) => setTimeout(r, 25)); + expect(settled).toBe(false); + pc.setGatheringState('complete'); + await expect(promise).resolves.toBe(ICE_GATHER_OUTCOME.COMPLETE); + }); + + test('resolves aborted when the abort signal fires', async () => { + const pc = fakePeerConnection('gathering'); + const controller = new AbortController(); + const promise = waitForIceGathering(pc, 1000, controller.signal); + controller.abort(); + await expect(promise).resolves.toBe(ICE_GATHER_OUTCOME.ABORTED); + expect(pc.listenerCount('icegatheringstatechange')).toBe(0); + }); + + test('resolves aborted immediately when the signal is already aborted', async () => { + const pc = fakePeerConnection('gathering'); + const controller = new AbortController(); + controller.abort(); + await expect(waitForIceGathering(pc, 1000, controller.signal)) + .resolves.toBe(ICE_GATHER_OUTCOME.ABORTED); + expect(pc.listenerCount('icegatheringstatechange')).toBe(0); + }); + + test('resolves only once when events race the timeout', async () => { + const pc = fakePeerConnection('gathering'); + const outcomes = []; + const promise = waitForIceGathering(pc, 25).then((outcome) => { + outcomes.push(outcome); + return outcome; + }); + pc.setGatheringState('complete'); + await expect(promise).resolves.toBe(ICE_GATHER_OUTCOME.COMPLETE); + // Fire more transitions after settling; must not throw or re-resolve. + pc.setGatheringState('complete'); + await new Promise((r) => setTimeout(r, 50)); + expect(outcomes).toEqual([ICE_GATHER_OUTCOME.COMPLETE]); + }); +});