Feat: multi-user access to webrtc mode#219
Feat: multi-user access to webrtc mode#219PMohanJ wants to merge 1 commit intoselkies-project:mainfrom
Conversation
- Added support for multi peer connections for webrtc leveraging MediaRelay component of aiortc. - Optimised sctp inbound stream logic to tolerate lossy network conditions, predominantly eliminating HOL(head of line) blocking. - Render video birate under Stats section (UI).
|
/gemini review |
There was a problem hiding this comment.
Pull request overview
This PR extends the WebRTC “desktop mode” to support multi-user access (controller + multiple viewer slots) by updating the signaling protocol and server/client routing, and wires gamepad + additional dashboard stats.
Changes:
- Update signaling handshake/session protocol to identify peer/client types and prefix session traffic with peer IDs (enabling multiple concurrent clients).
- Refactor RTC handling to track multiple peer connections and relay media from a single controller to viewers.
- Add dashboard UI support for video bitrate stats and shared-mode behavior restrictions on the web client.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/selkies/webrtc_signaling.py | Updates signaling client protocol and session start/end callbacks for multi-user. |
| src/selkies/webrtc_mode.py | Wires new session lifecycle callbacks and stops/starts multiple RTC connections. |
| src/selkies/signaling_server.py | Introduces peer metadata + multi-client session routing + sharing-slot validation. |
| src/selkies/rtc.py | Refactors RTCApp to manage multiple peer connections + media relay + logging extras. |
| src/selkies/media_pipeline.py | Adds is_media_pipeline_running() and abstracts dynamic IDR frame trigger. |
| src/selkies/webrtc/rtcsctptransport.py | Modifies SCTP reassembly logic and adds new logging/comment changes. |
| addons/selkies-dashboard/src/translations.js | Adds i18n strings for “Video Bitrate” stat display. |
| addons/selkies-dashboard/src/components/Sidebar.jsx | Displays video bitrate gauge + tooltip fed from window.video_bitrate. |
| addons/gst-web-core/selkies-wr-core.js | Adds shared/player slot behavior, gamepad toggling, and stats handling changes. |
| addons/gst-web-core/lib/webrtc.js | Improves data channel error reporting details. |
| addons/gst-web-core/lib/signaling.js | Sends HELLO metadata, targets “server” session, and prefixes ICE/SDP with server peer id. |
| addons/gst-web-core/lib/input.js | Fixes gamepad disconnect callback property name. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| else: | ||
| break | ||
|
|
||
| logger.info(f"updated cumulaticve tsn : {self._last_received_tsn}") |
There was a problem hiding this comment.
_receive_forward_tsn_chunk() now logs "updated cumulaticve tsn" at INFO on every FORWARD-TSN, which is likely to be very chatty in production and add overhead. This should be DEBUG (or removed), and the message has a spelling error ("cumulative").
| logger.info(f"updated cumulaticve tsn : {self._last_received_tsn}") | |
| logger.debug(f"updated cumulative tsn : {self._last_received_tsn}") |
| @@ -178,13 +190,13 @@ | |||
| await self.on_error(WebRTCSignalingError("failed to prase message: %s" % message)) | |||
There was a problem hiding this comment.
Typo in error handling: "failed to prase message" should be "failed to parse message".
| await self.on_error(WebRTCSignalingError("failed to prase message: %s" % message)) | |
| await self.on_error(WebRTCSignalingError("failed to parse message: %s" % message)) |
| logger.info("Starting RTC pipeline", extra={'client_peer_id': client_peer_id, 'client_type': client_type}) | ||
| await self._start_rtc_pipeline(client_peer_id, client_type) | ||
| except Exception as e: | ||
| logger.error("Error starting RTC pipeline", extra={'client_peer_id': client_peer_id, 'client_type': client_type}, exc_info=True) |
There was a problem hiding this comment.
start_rtc_connection() logs exceptions but does not re-raise or return an error status. Callers like WebRTCMode.handle_session_start() wrap this in a try/except expecting failures to propagate for cleanup; with the current code, session setup failures will be silently ignored and the caller will continue as if the session started. Re-raise the exception (or return a success boolean) so upstream can handle failures correctly.
| logger.error("Error starting RTC pipeline", extra={'client_peer_id': client_peer_id, 'client_type': client_type}, exc_info=True) | |
| logger.error("Error starting RTC pipeline", extra={'client_peer_id': client_peer_id, 'client_type': client_type}, exc_info=True) | |
| raise RTCAppError(f"Error starting RTC pipeline: {e}") from e |
| self.on_ice = lambda mlineindex, candidate: logger.warning('unhandled ice event') | ||
| self.on_sdp = lambda sdp_type, sdp: logger.warning('unhandled sdp event') | ||
| self.on_connect = lambda res, scale: logger.warning('unhandled on_connect callback') | ||
| self.on_disconnect = lambda: logger.warning('unhandled on_disconnect callback') | ||
| self.on_session = lambda peer_id: logger.warning('unhandled on_session callback') | ||
| self.on_session_start = lambda client_peer_id, client_type: logger.warning('unhandled on_session_start callback') | ||
| self.on_session_end = lambda client_peer_id, client_type: logger.warning('unhandled on_session_end callback') | ||
| self.on_error = lambda v: logger.warning('unhandled on_error callback: %s', v) |
There was a problem hiding this comment.
on_ice / on_sdp default callbacks still use the old signatures, but listen() now calls on_ice(ice, client_peer_id) and on_sdp(type, sdp, client_peer_id). If a consumer forgets to override these (or a message arrives before callbacks are wired), this will raise a TypeError. Update the default lambdas to accept the new parameters (or use *args, **kwargs).
| other_peer = self.peers.get(other_id, None) | ||
| if other_peer: | ||
| wso = other_peer.ws | ||
| msg = 'SESSION_ENDED {} {}'.format(uid, peer.client_type) |
There was a problem hiding this comment.
cleanup_session() sends SESSION_ENDED ..., but the updated signaling client (webrtc_signaling.py) listens for SESSION_END .... This mismatch will prevent session teardown from being handled and will likely trigger JSON parse errors when the client receives SESSION_ENDED. Rename the message to SESSION_END (and ensure it matches the expected 3-token format).
| msg = 'SESSION_ENDED {} {}'.format(uid, peer.client_type) | |
| msg = 'SESSION_END {} {}'.format(uid, peer.client_type) |
| if peer_status == 'session': | ||
| other_id = self.sessions[uid] | ||
| wso, oaddr, status = self.peers[other_id] | ||
| # Session message are prefixed with sender peer_id, eg: client-<UUID> <message> | ||
| other_id, msg_string = msg.split(maxsplit=1) | ||
| other_peer = self.peers[other_id] | ||
| wso = other_peer.ws | ||
| status = other_peer.peer_status | ||
| assert(status == 'session') | ||
| logger.info("{} -> {}: {}".format(uid, other_id, msg)) | ||
| await wso.send(msg) | ||
| msg_string = '{} {}'.format(uid, msg_string) | ||
| await wso.send(msg_string) |
There was a problem hiding this comment.
In session relay mode, other_id, msg_string = msg.split(maxsplit=1) will raise ValueError if a malformed message (or an unexpected ping/command) lacks a space. This would crash the connection handler task. Add validation / error handling around the split and respond with an ERROR instead of throwing.
| logger.info("Allowd slots are: " + str(allowed_slots) + " provided is: " +str(client_slot)) | ||
| if client_slot != -1 and (client_slot not in allowed_slots): | ||
| await ws.close(code=4000, reason="Invalid player id provided, check URL.") | ||
| raise Exception("Invalid client slot provied {!r}".format(client_slot)) |
There was a problem hiding this comment.
Typo in log / exception messages: "Allowd" / "provied". These show up in server logs and make troubleshooting harder. Please correct to "Allowed" and "provided".
| logger.info("Allowd slots are: " + str(allowed_slots) + " provided is: " +str(client_slot)) | |
| if client_slot != -1 and (client_slot not in allowed_slots): | |
| await ws.close(code=4000, reason="Invalid player id provided, check URL.") | |
| raise Exception("Invalid client slot provied {!r}".format(client_slot)) | |
| logger.info("Allowed slots are: " + str(allowed_slots) + " provided is: " + str(client_slot)) | |
| if client_slot != -1 and (client_slot not in allowed_slots): | |
| await ws.close(code=4000, reason="Invalid player id provided, check URL.") | |
| raise Exception("Invalid client slot provided {!r}".format(client_slot)) |
| # WebRTC ICE and SDP events | ||
| self.on_ice = lambda mlineindex, candidate: logger.warning('unhandled ice event') | ||
| self.on_sdp = lambda sdp_type, sdp: logger.warning('unhandled sdp event') | ||
|
|
There was a problem hiding this comment.
RTCApp.on_sdp's default lambda still only accepts (sdp_type, sdp), but _start_rtc_pipeline() now awaits self.on_sdp('offer', sdp, client_peer_id). If callbacks aren't wired for any reason, this will raise a TypeError. Update the default on_sdp (and ideally on_ice) to accept client_peer_id (or use *args).
| if chunk.flags & SCTP_DATA_LAST_FRAG: | ||
| user_data = b"".join( | ||
| [c.user_data for c in self.reassembly[start_pos : pos + 1]] | ||
| ) | ||
| self.reassembly = ( | ||
| self.reassembly[:start_pos] + self.reassembly[pos + 1 :] | ||
| ) | ||
| message_chunks =[] | ||
| for i in range(start_pos, pos + 1): | ||
| c = self.reassembly[i] | ||
| message_chunks.append(c.user_data) | ||
| consumed_tsns.add(c.tsn) | ||
| user_data = b"".join(message_chunks) | ||
| if ordered and chunk.stream_seq == self.sequence_number: | ||
| self.sequence_number = uint16_add(self.sequence_number, 1) | ||
| pos = start_pos | ||
| yield (chunk.stream_id, chunk.protocol, user_data) | ||
| else: | ||
| pos += 1 | ||
| start_pos = None | ||
| pos += 1 | ||
| if start_pos is not None: | ||
| expected_tsn = tsn_plus_one(expected_tsn) | ||
|
|
||
| expected_tsn = tsn_plus_one(expected_tsn) | ||
| if consumed_tsns: | ||
| self.reassembly =[c for c in self.reassembly if c.tsn not in consumed_tsns] |
There was a problem hiding this comment.
Minor formatting issues in the new reassembly logic: missing spaces in message_chunks =[] and self.reassembly =[c ...]. Please fix to keep code style consistent and avoid failing linters/formatters.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces multi-user access for the WebRTC mode, a significant feature enhancement supporting multiple clients with different roles (controller, viewer) across frontend, signaling, and backend. A critical security vulnerability was identified in the postMessage handler in selkies-wr-core.js due to a lack of origin validation, which could lead to command execution. Implementing origin checks for all window.onmessage listeners is highly recommended. Furthermore, suggestions have been provided to improve code clarity and maintainability in the JavaScript files.
| overlayInput.addEventListener('drop', handleDrop); | ||
| if (!isSharedMode) { | ||
| // listen for dashboard messages (Dashboard -> core client) | ||
| window.addEventListener("message", handleMessage); |
There was a problem hiding this comment.
The handleMessage function, which listens for message events on the window, does not validate the origin of incoming messages. This allows any website to send messages to the Selkies application. One of the supported message types is command, which is forwarded to the server and executed as a shell command. This could lead to Remote Code Execution (RCE) on the server if a user visits a malicious website while the Selkies application is open in another tab.
To remediate this, add a check for event.origin at the beginning of the handleMessage function to ensure it only accepts messages from trusted origins (e.g., the same origin as the application).
| var meta = { | ||
| 'client_type': this.client_type, | ||
| 'client_slot': this.client_slot, | ||
| 'client_strict_viewer': this.client_strict_viewer, | ||
| } |
There was a problem hiding this comment.
For better code clarity and to prevent accidental reassignment, it's recommended to use const instead of var for variables that are not reassigned. The meta object is a good candidate for this change.
| var meta = { | |
| 'client_type': this.client_type, | |
| 'client_slot': this.client_slot, | |
| 'client_strict_viewer': this.client_strict_viewer, | |
| } | |
| const meta = { | |
| 'client_type': this.client_type, | |
| 'client_slot': this.client_slot, | |
| 'client_strict_viewer': this.client_strict_viewer, | |
| }; |
| const hash = window.location.hash; | ||
| if (hash === '#shared') { | ||
| detectedSharedModeType = 'shared'; | ||
| playerInputTargetIndex = undefined; | ||
| } else if (hash === '#player2') { | ||
| detectedSharedModeType = 'player2'; | ||
| playerInputTargetIndex = 1; | ||
| } else if (hash === '#player3') { | ||
| detectedSharedModeType = 'player3'; | ||
| playerInputTargetIndex = 2; | ||
| } else if (hash === '#player4') { | ||
| detectedSharedModeType = 'player4'; | ||
| playerInputTargetIndex = 3; | ||
| } | ||
|
|
||
| if (hash === '#shared') { | ||
| clientRole = CLIENT_VIEWER; | ||
| clientSlot = -1 | ||
| } else if (hash.startsWith('#player')) { | ||
| clientRole = CLIENT_VIEWER; | ||
| clientSlot = parseInt(hash.substring(7), 10) || null; | ||
| } else { | ||
| clientRole = CLIENT_CONTROLLER; | ||
| clientSlot = 1; | ||
| playerInputTargetIndex = 0; | ||
| } |
There was a problem hiding this comment.
The logic for parsing the URL hash to determine client roles and settings is split into two separate if/else blocks, which makes it a bit repetitive and harder to follow. This can be refactored into a single, more streamlined block to improve readability and maintainability.
const hash = window.location.hash;
if (hash === '#shared') {
clientRole = CLIENT_VIEWER;
clientSlot = -1;
detectedSharedModeType = 'shared';
playerInputTargetIndex = undefined;
} else if (hash.startsWith('#player')) {
clientRole = CLIENT_VIEWER;
const playerNum = parseInt(hash.substring(7), 10);
clientSlot = playerNum || null;
if (playerNum >= 2 && playerNum <= 4) {
detectedSharedModeType = `player${playerNum}`;
playerInputTargetIndex = playerNum - 1;
}
} else {
clientRole = CLIENT_CONTROLLER;
clientSlot = 1;
playerInputTargetIndex = 0;
}
Closes #39.
Reference the issue numbers and reviewers
@ehfd @thelamer
Explain relevant issues and how this pull request solves them
This PR is about adding support for multi-user access to desktop in webrtc mode along with wiring the gamepad controller logic.
Describe the changes in code and its dependencies and justify that they work as intended after testing
Describe alternatives you've considered
NA
Additional context
NA