fix(core): Comprehensive fixes and performance optimizations#254
Conversation
![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 various stability, performance, and security improvements across the Selkies codebase. Key changes include offloading blocking PulseAudio and GPU monitoring calls to executors, implementing robust bounds and validation on client-supplied parameters (such as display dimensions and scroll magnitudes) to prevent resource exhaustion, and introducing Base64 encoding for file paths to safely handle special characters. Additionally, it resolves potential memory leaks by clearing timers on reconnect, handles out-of-order JPEG stripe decodes, and ensures player slot uniqueness. The review feedback highlights several opportunities to further improve robustness, such as using a FIFO queue to prevent race conditions in asynchronous frame ID tagging, adding defensive length checks on message tokens to avoid IndexError crashes, and wrapping integer conversions in try-except blocks to handle malformed client inputs gracefully.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
ehfd
changed the title
ehfd
force-pushed
the
comprehensive-fixes
branch
2 times, most recently
from
6d9238d to
c3f3a8f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several performance optimizations, security enhancements, and robustness fixes across the C interposer, Python backend, and frontend. Key updates include zero-copy video rendering using track generators, stuck-key recovery via client heartbeats, and safer parsing of SDP/RTP packets. Review feedback points out a critical issue in joystick_interposer.c where copying the interposer struct to a local stack variable causes state modifications in ioctl handlers to be lost. Additionally, a potential race condition in selkies.py was identified where self.pcmflux_capture_loop could be cleared mid-execution, which can be resolved by snapshotting the loop reference.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
ehfd
force-pushed
the
comprehensive-fixes
branch
from
c3f3a8f to
fcd4473
Compare
|
While monitoring Sentry on the stable build, I caught two unhandled exceptions that might be worth porting into this rewrite: AttributeError: 'NoneType' object has no attribute 'type' in the GStreamer bus callback when message is None during stream resets. I looked through the diff for #254 and didn't see an equivalent guard in the new files (e.g. a not self.stop_server.done() check before set_result) — apologies if I missed it since it's a large rewrite, but flagging in case it's still relevant. |
ehfd
force-pushed
the
comprehensive-fixes
branch
from
fcd4473 to
dde1530
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ehfd
force-pushed
the
comprehensive-fixes
branch
8 times, most recently
from
903fa0a to
058b61f
Compare
There was a problem hiding this comment.
Pull request overview
This PR is a broad hardening/performance pass across Selkies’ WebRTC stack (SDP/RTP/DTLS/SCTP), signaling lifecycle, input/clipboard paths, and the web client/dashboard, with additional security tightening (notably command_enabled defaulting to False and stricter control-plane auth routing).
Changes:
- Harden SDP/signaling/RTP parsing and connection lifecycle handling to tolerate malformed inputs without taking down shared connections.
- Add Opus RED (RFC 2198) support plus codec negotiation/filtering updates.
- Improve operational behavior: bounded stats CSV growth + deterministic draining, more resilient clipboard/keyboard handling, and multiple client UX/perf fixes.
Reviewed changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/selkies/webrtc/sdp.py | Makes SDP parsing more defensive (ValueError/guards) and more tolerant of malformed attributes. |
| src/selkies/webrtc/rtp.py | Adds length checks in RTP header extension parsing and validates REMB buffer length. |
| src/selkies/webrtc/rtcsctptransport.py | Increases SCTP max message size capability to support larger DataChannel payloads. |
| src/selkies/webrtc/rtcpeerconnection.py | Updates codec filtering logic to account for Opus RED wrapping semantics. |
| src/selkies/webrtc/rtcdtlstransport.py | Treats struct.error from packet parsing as non-fatal (drop + debug log). |
| src/selkies/webrtc/codecs/red.py | Introduces an Opus RED encoder implementation (RFC 2198 framing). |
| src/selkies/webrtc/codecs/init.py | Registers RED codec, adds encoder wiring, and helper predicates for RED detection. |
| src/selkies/webrtc_utils.py | Reworks WebRTC stats CSV writing (bounded growth, stable headers, dedicated executor + deterministic drain). |
| src/selkies/webrtc_signaling.py | Prevents malformed/unrecognized signaling messages from tearing down shared connections. |
| src/selkies/webrtc_mode.py | Adjusts component initialization, cursor resend behavior, settings sanitization/clamping, and async metrics CSV init/teardown. |
| src/selkies/stream_server.py | Tightens auth middleware routing (exact path match, master token gating) and guards cleanup on extraction failure. |
| src/selkies/signaling_server.py | Makes peer registration/eviction more resilient and reduces lock-holding across slow socket I/O. |
| src/selkies/settings.py | Tightens defaults/bounds (min/max), adds audio redundancy settings, and makes command_enabled opt-in by default. |
| src/selkies/rtc.py | Adds SDP bandwidth munging improvements, zero-copy packet wrapping, and serializes DataChannel message dispatch for ordering. |
| src/selkies/media_pipeline.py | Makes pixelflux/pcmflux imports optional, improves startup/cleanup behavior, and adds pipeline-start callback hooks. |
| src/selkies/input_handler.py | Improves input robustness (stuck-key recovery, key repeat, clipboard transfer IDs/kinds, queue bounding, base64 path handling). |
| src/selkies/display_utils.py | Validates resolution input before cvt/gtf invocation for clearer failures. |
| src/README.md | Updates architecture description to match pixelflux/pcmflux-based media capture/encoding. |
| README.md | Adds a brief client capability highlight summary. |
| pyproject.toml | Updates package metadata (Python version floor and classifiers) and description. |
| docs/usage.md | Documents clipboard behavior and browser support expectations. |
| docs/faq.md | Updates clipboard guidance to reflect new fallback behavior (removes older Firefox workaround requirement). |
| addons/selkies-web-core/selkies-wr-core.js | Improves clipboard parity, logging caps, file upload path encoding, timer cleanup, and settings persistence fixes. |
| addons/selkies-web-core/selkies-core.js | Adds a one-time localStorage key migration for the corrected prefix sanitizer. |
| addons/selkies-web-core/README.md | Documents new rendering paths, clipboard behavior, and related client/server features. |
| addons/selkies-web-core/lib/webrtc.js | Renames client class and hardens message handling + adds stats fields and better error reporting. |
| addons/selkies-web-core/lib/signaling.js | Renames signaling class and updates documentation pointers. |
| addons/selkies-web-core/lib/input.js | Adds held-key heartbeat, improves modifier healing, and refines wheel/scroll behavior to better distinguish wheels vs trackpads. |
| addons/selkies-dashboard/src/components/Sidebar.jsx | Fixes audio bitrate type coercion and adds master-token prompting for /switch when needed. |
| addons/js-interposer/joystick_interposer.c | Reduces lock contention, improves short-read handling, expands libc interposition coverage, and fixes ioctl/property behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several improvements across the platform, including support for older glibc runtimes in the joystick interposer, a one-time localStorage migration, Safari/Firefox clipboard parity, and offscreen worker video rendering. It also vendors the python-xlib library. The code review identified several critical issues: a potential buffer overflow in the joystick interposer when the requested read size is smaller than the stashed length, audio corruption in shared mode due to incorrect header assumptions in extractPrimaryOpusBuffer, a potential TypeError in the sidebar's audio bitrate handling, and a crash in unix_connect.py when parsing IPv6 peer addresses.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements across the Selkies streaming platform, including C-level interposer improvements for old glibc compatibility, partial-event stashing to prevent stream desynchronization, and faithful Xbox 360 controller emulation. On the web client side, it adds a worker-based video rendering pipeline for Safari and Firefox, stuck-key prevention via heartbeats, improved trackpad/mouse wheel detection, and robust multi-part clipboard synchronization for non-Chromium browsers. It also vendors python-xlib and updates the Python requirement to >=3.9. Feedback on these changes highlights two important issues: a potential buffer overflow vulnerability in the C interposer's read path if the destination buffer is smaller than the event size, and a potential unhandled TypeError in the WebRTC client's metrics loop if it fires after disconnection.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Updated 2026-07-08: pixelflux no longer needs source-built ffmpeg/x264/libjpeg-turbo/wayland (system ffmpeg 6.0–8.1 is accepted, libjpeg-turbo is vendored statically, wayland is dlopen'd) — the builder stage is now just distro dev packages + rustup. The control plane moved under For anyone else wanting to second-eye test this stack on the linuxserver.io images: below is the Dockerfile we've been using, sanitised to build directly on All three branches advance by amend + force-push, so bump the pins together. Dockerfile# Example: build and test the comprehensive-fixes stack on top of the
# linuxserver.io Selkies base image, patching the stock image in place:
#
# - selkies-project/selkies#254 (comprehensive-fixes: selkies python + frontend)
# - linuxserver/pixelflux#18 (comprehensive-fixes: pure-Rust rewrite)
# - linuxserver/pcmflux#5 (comprehensive-fixes: pure-Rust, AudioPlayback)
#
# This is a sanitised copy of the recipe we use for second-eye testing #254 —
# our real builds start from a downstream image based on
# baseimage-selkies:ubuntunoble, which is what this stack has actually been
# exercised on (Intel/VA-API and NVIDIA/NVENC, Firefox + Chrome clients).
#
# Since the first version of this example, pixelflux stopped pinning ffmpeg
# (any system 6.0-8.1 works, only avcodec/avfilter needed), statically
# vendors libjpeg-turbo, links noble's libx264-dev, and dlopens wayland — so
# the x264/libjpeg-turbo/ffmpeg/wayland source builds are all gone in favour
# of noble's distro packages, and the builder stage is just deps + rustup.
#
# All three source branches are moving targets that advance by amend +
# force-push; the SHAs below pin what we tested. Bump them together — the
# selkies python side needs the matching native-module features
# (deferred_free / OwnedFrame / request_idr_frame / AudioPlayback).
ARG SELKIES_REPO=https://github.com/selkies-project/selkies
ARG SELKIES_HASH=da1b55e7148a074816286eb6d54ee1e500fcfb53
ARG PIXELFLUX_HASH=ab248a32d63bec09786ce5f7406fc4182875485a
ARG PCMFLUX_HASH=0cbadcb334dbc190ba4525da091401276671beeb
ARG BASE_IMAGE=ghcr.io/linuxserver/baseimage-selkies:ubuntunoble
########################################################################
# Frontend: mirrors the base image's own packaging flow.
########################################################################
FROM node:20-alpine AS frontend
RUN apk add --no-cache git
ARG SELKIES_REPO
ARG SELKIES_HASH
RUN mkdir /src && cd /src && \
git init && git remote add origin "${SELKIES_REPO}.git" && \
git fetch --depth 1 origin "${SELKIES_HASH}" && git checkout FETCH_HEAD
# The postbuild step (gendb.js) fetches the SDL game-controller DB from
# raw.githubusercontent.com and swallows fetch failures (logs + exit 0),
# silently skipping dist/jsdb — which the dashboard stage's cp then trips
# over. Retry it once and fail loudly here instead.
RUN cd /src/addons/selkies-web-core && \
npm install && npm run build && \
{ [ -d dist/jsdb ] || node gendb.js; } && \
{ [ -d dist/jsdb ] || { echo 'ERROR: dist/jsdb missing - gendb.js could not fetch SDL_GameControllerDB (transient network?). Re-run the build.'; exit 1; }; }
# The final stage keeps the base image's nginx/ fancyindex header/footer
# copies (required by the LSIO runtime: init-nginx seds footer.html and
# default.conf references both), so they are not packaged from source here.
RUN cd /src/addons/selkies-dashboard && \
cp ../selkies-web-core/dist/selkies-core.js src/ && \
npm install && npm run build && \
mkdir -p dist/src && \
cp ../selkies-web-core/dist/selkies-core.js dist/src/ && \
cp ../universal-touch-gamepad/universalTouchGamepad.js dist/src/ && \
cp -r ../selkies-web-core/dist/jsdb dist/
########################################################################
# Native modules: build pixelflux/pcmflux wheels from the PR branches.
#
# Both are pure-Rust PyO3 extensions (no prebuilt wheels for these
# branches). Building FROM the same base image guarantees the wheels match
# /lsiopy's python and the base glibc.
#
# The dep list mirrors pixelflux's [tool.cibuildwheel] before-all:
# ffmpeg-sys-next accepts system ffmpeg 6.0-8.1 (noble: 6.1) with only
# avcodec/avfilter needed; x264-sys links noble's libx264-dev; turbojpeg-sys
# statically builds its vendored libjpeg-turbo (hence cmake + nasm); wayland
# is dlopen'd at runtime, so headers suffice.
########################################################################
FROM ${BASE_IMAGE} AS native-builder
ENV DEBIAN_FRONTEND=noninteractive
RUN apt-get update && apt-get install -y --no-install-recommends \
build-essential clang libclang-dev curl git nasm cmake pkg-config \
libpixman-1-dev libx11-dev libxext-dev libxfixes-dev \
libva-dev libdrm-dev libinput-dev libxkbcommon-dev libgbm-dev \
libwayland-dev libudev-dev \
libavcodec-dev libavfilter-dev libx264-dev \
libpulse-dev libopus-dev
# noble's rustc is too old for the PyO3 0.27 crate graph. The LSIO base image
# sets HOME=/config, so don't let rustup pick $HOME/.cargo — pin both trees
# somewhere fixed and PATH that instead.
ENV RUSTUP_HOME=/opt/rustup \
CARGO_HOME=/opt/cargo \
PATH=/opt/cargo/bin:$PATH
# Download-then-run, NOT `curl | sh`: a failed download would feed sh an empty
# script, which exits 0 and silently installs nothing.
RUN curl --proto '=https' --tlsv1.2 -sSf -o /tmp/rustup-init.sh https://sh.rustup.rs && \
sh /tmp/rustup-init.sh -y --profile minimal --no-modify-path && \
rm /tmp/rustup-init.sh
RUN cargo --version && rustc --version
# The PR heads live on ehfd's forks (linuxserver/pixelflux#18,
# linuxserver/pcmflux#5); init+fetch pins an arbitrary SHA on any git.
ARG PIXELFLUX_HASH
ARG PCMFLUX_HASH
RUN mkdir /tmp/pixelflux && cd /tmp/pixelflux && \
git init && git remote add origin https://github.com/ehfd/pixelflux.git && \
git fetch --depth 1 origin "${PIXELFLUX_HASH}" && git checkout FETCH_HEAD && \
pip wheel --no-deps -w /wheels .
RUN mkdir /tmp/pcmflux && cd /tmp/pcmflux && \
git init && git remote add origin https://github.com/ehfd/pcmflux.git && \
git fetch --depth 1 origin "${PCMFLUX_HASH}" && git checkout FETCH_HEAD && \
pip wheel --no-deps -w /wheels .
# The joystick interposer and fake-udev shim were both reworked on
# comprehensive-fixes; the base image's copies predate that, so rebuild them
# from the same selkies checkout the python side comes from.
ARG SELKIES_REPO
ARG SELKIES_HASH
RUN mkdir /tmp/selkies && cd /tmp/selkies && \
git init && git remote add origin "${SELKIES_REPO}.git" && \
git fetch --depth 1 origin "${SELKIES_HASH}" && git checkout FETCH_HEAD && \
cd addons/js-interposer && \
gcc -shared -fPIC -o selkies_joystick_interposer.so joystick_interposer.c -ldl -pthread && \
cd ../fake-udev && \
make
########################################################################
# Final: patch the base image in place.
########################################################################
FROM ${BASE_IMAGE}
ARG SELKIES_REPO
ARG SELKIES_HASH
ENV DEBIAN_FRONTEND=noninteractive
# Runtime halves of the distro libs the wheels link against (noble ffmpeg 6.1
# sonames + x264; libjpeg-turbo is statically vendored inside the pixelflux
# wheel). NVIDIA driver libs (libcuda, libnvidia-encode) come from the
# container runtime as usual — the Rust NVENC bindings dlopen them.
RUN apt-get update && apt-get install -y --no-install-recommends \
libavcodec60 libavfilter9 libx264-164 libopus0 && \
rm -rf /var/lib/apt/lists/*
COPY --from=native-builder /wheels /tmp/wheels
RUN pip install --no-deps --force-reinstall /tmp/wheels/*.whl && rm -rf /tmp/wheels
# selkies python package. 'six' is a new runtime dep on comprehensive-fixes
# (its vendored python-Xlib needs it), so install it explicitly. (pynput /
# GPUtil / pasimple / python-xlib were dropped upstream; the stale copies in
# the image are harmless.)
RUN pip install --no-deps --force-reinstall \
"${SELKIES_REPO}/archive/${SELKIES_HASH}.zip" && \
pip install six && \
rm -rf /tmp/* ~/.cache
# Replace the whole dashboard build (content-hashed assets must stay
# consistent), but keep the base image's nginx/ fancyindex header/footer,
# which the LSIO runtime requires (init-nginx seds footer.html; default.conf
# references both).
RUN cp -a /usr/share/selkies/selkies-dashboard/nginx /tmp/nginx-keep && \
rm -rf /usr/share/selkies/selkies-dashboard
COPY --from=frontend /src/addons/selkies-dashboard/dist/ \
/usr/share/selkies/selkies-dashboard/
RUN mkdir -p /usr/share/selkies/selkies-dashboard/nginx && \
cp -a /tmp/nginx-keep/. /usr/share/selkies/selkies-dashboard/nginx/ && \
rm -rf /tmp/nginx-keep
# The LSIO nginx only proxies SUBFOLDERwebsocket* to the backend; the
# comprehensive-fixes control plane now lives under SUBFOLDERapi/ (status,
# health, tokens, switch — one rule routes them all, present and future).
# The SUBFOLDER and CWS placeholders are substituted by the image's init
# scripts at container start.
#
# NOTE: anything in the image that talks to the old control endpoints must
# follow this move too — e.g. a token-provisioning helper POSTing to
# /tokens must now POST to {prefix}/api/tokens on the websocket port, and
# until that first successful POST the server's config gate rejects all
# websocket connections.
RUN sed -i "s|location SUBFOLDERwebsocket {|\
location SUBFOLDERapi {\n\
proxy_pass http://127.0.0.1:CWS;\n\
proxy_set_header Host \$host;\n\
proxy_set_header X-Real-IP \$remote_addr;\n\
proxy_set_header X-Forwarded-For \$proxy_add_x_forwarded_for;\n\
proxy_set_header X-Forwarded-Proto \$scheme;\n\
}\n\n\
location SUBFOLDERwebsocket {|g" \
/defaults/default.conf
# Interposer + fake-udev rebuilt against the comprehensive-fixes protocol.
COPY --from=native-builder /tmp/selkies/addons/js-interposer/selkies_joystick_interposer.so \
/usr/lib/selkies_joystick_interposer.so
COPY --from=native-builder /tmp/selkies/addons/fake-udev/libudev.so.1.0.0-fake \
/opt/lib/libudev.so.1.0.0-fake
# comprehensive-fixes' unified server fronts the websocket with basic auth by
# default (ubuntu/mypasswd unless SELKIES_BASIC_AUTH_* is set). Browsers
# cannot attach an Authorization header to a WebSocket upgrade; the server
# now skips basic auth on websocket paths once a master token is provisioned,
# but a deployment without token provisioning still locks browser clients
# out unless this is disabled. (The LSIO images never set the enable env, and
# nginx-level auth via CUSTOM_USER/PASSWORD is available instead.)
ENV SELKIES_ENABLE_BASIC_AUTH=false🤖 Generated with Claude Code |
|
Two more findings from testing on the lsio-based image, both about failure opacity rather than the happy path: 1. WebSocket upgrades rejected by basic auth are silent on both ends. In 2. When pixelflux fails to import, capture start dies with an unrelated-looking 🤖 Generated with Claude Code |
ehfd
force-pushed
the
comprehensive-fixes
branch
2 times, most recently
from
155eb0b to
de77558
Compare
|
Fixes incorporated across all three PRs. Current commits: Selkies: pixelflux: pcmflux: I am building a temporary image (https://gitlab.nrp-nautilus.io/ehf/selkies-lsio/-/blob/main/Dockerfile) which is available on Run |
Co-authored-by: Ryan Mounce <ryan@mounce.com.au>
ehfd
force-pushed
the
comprehensive-fixes
branch
2 times, most recently
from
9fcf897 to
14802af
Compare
ehfd
force-pushed
the
comprehensive-fixes
branch
4 times, most recently
from
32a3087 to
6f4e5c7
Compare
ehfd
force-pushed
the
comprehensive-fixes
branch
9 times, most recently
from
89a8252 to
3bab531
Compare
Co-authored-by: vishalkadam47 <vishalkadam472@gmail.com>
Must be tested in tandem to linuxserver/pixelflux#18 and
linuxserver/pcmflux#5. Interface and API/ABI are backward-incompatible.
Fixes #273
Fixes #271
Fixes #270
Fixes #269
Fixes #268
Fixes #267
Fixes #266
Fixes #265
Fixes #264
Fixes #261
Fixes #259
Fixes #258
Fixes #257
Fixes #245
Fixes #241
Fixes #234
Fixes #222
Fixes #220
Fixes #214
Fixes #200
Fixes #169
Fixes #157
Fixes #153
Fixes #145
Fixes #85