Repository navigation
Agent: [Agent] WebRTC: fix ICE gathering/trickle and remove dead signaling code - #661
Merged
Merged
Conversation
…sor#659) 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.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #659
Agent-generated change from a maintainer-labeled issue. Review the diff and CI results before merging. Close the issue only when its acceptance criteria are complete.
DigitalOcean session:
lightnvr-public-issue-659-39054207904.