Repository navigation
Switch the desktop encoder on SIGUSR2 without dropping the session - #96
Conversation
set-encoder and set-codec notify the running host through host.pid. Running desktop streams restart only the video encoder (first frame IDR, negotiated codec kept, VAAPI drops 4:4:4 to 4:2:0 with CaptureSettingSync). A codec change is logged and applies to the next desktop connection. SIGUSR2 does not re-probe.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7074e43f59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Install the handler before publishing the pid: SIGUSR1's default action terminates the process. | ||
| // Install the handlers before publishing the pid: the default action of either signal terminates the process. | ||
| let Ok(mut usr1) = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::user_defined1()) else { return }; | ||
| let Ok(mut usr2) = tokio::signal::unix::signal(tokio::signal::unix::SignalKind::user_defined2()) else { return }; |
There was a problem hiding this comment.
When the normal host runs with the supported UU_REMOTE_ASSIST=0 setting, or account-assistance setup fails, serve never calls present_on_usr1 because both call sites are tied to an Assist session. The new SIGUSR2 handler and write_host_pid() are therefore absent, so set-encoder silently cannot notify or switch a live stream in that configuration; register the reload handler/pid for every normal host independently of the assistance UI.
AGENTS.md reference: AGENTS.md:L117-L117
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
dd2e24b：install_host_signals 在 host 启动时、remote assist 之前安装 SIGUSR2 并写 host.pid；UU_REMOTE_ASSIST=0 或 assist 初始化失败时也能处理 SIGUSR2。assist 之后只追加 SIGUSR1。新增测试 sigusr2_handler_installs_without_assist。
| } | ||
| // In-process engines honor this on the first frame; the process pipeline starts on an IDR. | ||
| idr.store(true, std::sync::atomic::Ordering::Relaxed); | ||
| reader.restart(open(size, live_selection, current)).await; |
There was a problem hiding this comment.
After this switches a connected stream using live_selection, any subsequent viewer FPS/quality/resolution update enters the restart branch below and calls open(..., selection, ...) with the immutable connect-time snapshot; the geometry and H.264 fallback restarts do the same. Because live_selection still records the new encoder, later reload checks see no difference and never repair the revert, so all restart paths need to use the current live selection.
AGENTS.md reference: AGENTS.md:L117-L117
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
dd2e24b：viewer 的 FPS/画质/分辨率重启、几何变化重启和 H.264 回退都改用 live_selection（最近一次 SIGUSR2 切换后的值），不再用建连时的快照。
| let (mut window_frames, mut window_skipped) = (0u64, 0u64); | ||
| let mut send_interrupted=false; | ||
| 'stream: loop { | ||
| if reload.has_changed().unwrap_or(false) || reload_pending { |
There was a problem hiding this comment.
After the first SIGUSR2 notification, watch::Receiver::has_changed() remains true until the receiver version is marked seen via borrow_and_update or changed, but this receiver is never advanced. Consequently every trip around the stream loop—normally once per frame—calls blocking selection(), rereads/parses the settings file, and repeats NVIDIA filesystem checks for the rest of the session; mark the notification observed before processing the reload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
dd2e24b：流循环在读 selection() 前调用 reload.borrow_and_update()，每次通知只处理一次。新增测试 mark_reload_seen_clears_has_changed。
| let mut seen_codec_pref = selection.codec_pref; | ||
| let reload = crate::video_select::subscribe_reload(); | ||
| // A SIGUSR2 that arrived before this stream subscribed is still visible as a fresh selection(). | ||
| let mut reload_pending = true; |
There was a problem hiding this comment.
If the background auto probe is still running when WebRtcSession::new snapshots the legacy encoder, it can finish during HEVC/ICE setup; this unconditional pending refresh then re-reads the cache and restarts that same peer on the probe result even though no SIGUSR2 occurred. Capability/chroma reporting and codec selection remain based on the old per-peer snapshot, and sessions begun before probe completion are meant to retain the legacy choice, so carry or compare the reload generation and refresh only when a real pre-subscription signal occurred.
AGENTS.md reference: AGENTS.md:L125-L125
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
dd2e24b：WebRtcSession::new 记录快照时的 reload generation，只有 generation 变化（真的收到 SIGUSR2）才做启动时刷新；建连期间后台探测完成不算 reload，这一路连接保留快照。新增测试 probe_finish_is_not_a_reload_since_the_session_snapshot。
…er on restarts, consume reload notifications
|
@codex review |
1 similar comment
|
@codex review |
|
已审 dd2e24b。Codex 上一轮的 4 条都处理到位：SIGUSR2 不再依赖 assist、重启路径统一用
另外，main 刚合并了 #97，在 改完后 CI 全绿、Codex 对 dd2e24b 之后的提交没有未决意见，我就合并。真实客户端下的切换时序，等发版后再找 NVIDIA 机器实测。 🤖 Generated with Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd2e24bbfc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let _audio_cleanup=virtual_audio::Cleanup; | ||
| log::info!("Starting uu-remote-linux host v{}...", env!("CARGO_PKG_VERSION")); | ||
| // Before host.pid, and before assist: a missing or failed assist session must still handle SIGUSR2. | ||
| let _ = install_host_signals(None); |
There was a problem hiding this comment.
When the host runs with UU_REMOTE_ASSIST=0, or while assistance setup is pending or has failed, this call passes None, so install_host_signals installs only SIGUSR2 but still writes host.pid. show_running_host() treats that PID as SIGUSR1-ready and sends SIGUSR1; because its default action is termination, uuremote-server show kills/restarts the host and returns success instead of opening or failing gracefully. Register a safe SIGUSR1 handler before publishing the shared PID.
AGENTS.md reference: AGENTS.md:L133-L133
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5911a80: install_host_signals registers a no-op SIGUSR1 handler and the SIGUSR2 handler before host.pid is written, including when remote assist is off, pending, or failed.
| /// Publish the generation so every subscribed desktop stream wakes. | ||
| pub fn publish_reload() { | ||
| notify_reload(); | ||
| let _ = reload_watch().send(reload_generation_value()); |
There was a problem hiding this comment.
On a second or later desktop connection, the previous reload receiver can already be gone while the new stream is still negotiating and has not subscribed. watch::Sender::send returns an error without replacing the stored value when there are no receivers, so ignoring that error here leaves the watch value at the session snapshot even though the atomic generation advanced; a real set-encoder signal during this window is then missed by reload_since_session. Use send_replace/send_modify, or compare against the atomic generation directly.
AGENTS.md reference: AGENTS.md:L117-L117
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5911a80: publish_reload uses send_replace, so a SIGUSR2 with no subscribed desktop stream still updates the watch value.
| format!("{:?}", live_selection.encoder).to_ascii_lowercase(), | ||
| format!("{encoder:?}").to_ascii_lowercase(), | ||
| crate::video_select::encoder_source_label(fresh.encoder_source)); | ||
| live_selection.encoder = encoder; |
There was a problem hiding this comment.
After a live CPU/NVENC-to-VAAPI switch, only this stream-local live_selection is updated; the CONTROL/TEXT RPC worker still calls handle_rpc_for with the connect-time capture_selection. If the viewer then requests 4:4:4, that worker accepts and syncs 4:4:4 based on the old encoder while read_encoder silently forces VAAPI to 4:2:0, so the reported chroma no longer matches the bitstream; the reverse switch also keeps rejecting a now-supported request. Share the current encoder with the RPC handler.
AGENTS.md reference: AGENTS.md:L125-L125
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Recorded under Known limitations in the PR description: a live switch shares the new encoder with later capture-setting RPCs only when that switch also sends CaptureSettingSync; a chroma-preserving switch leaves those RPCs on the connect-time encoder until the next connection.
| let path = super::host_pid_path(); | ||
| if let Some(dir) = path.parent() { std::fs::create_dir_all(dir).unwrap(); } | ||
| assert!(super::install_host_signals(None)); | ||
| let pid = std::fs::read_to_string(&path).unwrap(); |
There was a problem hiding this comment.
When cargo test runs without a UU_CONFIG override, this call resolves the normal ~/.config/uuremote/host.pid, truncates it to the test process PID, and never restores it. That disconnects show and set-* from a concurrently running host; the test also skips config::test_env_lock, so it can race tests that temporarily change UU_CONFIG and read a different path from the one the function writes. Point UU_CONFIG at a temporary path under the lock and clean it up.
AGENTS.md reference: AGENTS.md:L55-L55
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5911a80: the signal-install test points UU_CONFIG at a temp directory under config::test_env_lock and removes it afterward.
| } | ||
| write_host_pid(); | ||
| tokio::spawn(async move { while usr1.recv().await.is_some() { ui.present(); } }); | ||
| true |
There was a problem hiding this comment.
For a host authenticated solely through UU_* environment credentials, or using a fresh UU_CONFIG parent, the config directory need not exist when startup first calls this function. write_host_pid() silently fails because it does not create the parent, yet this returns success and is never retried when assistance is disabled; set-encoder later creates the directory for host_settings.json but finds no PID, so it cannot notify the running host. Create the parent before writing or propagate the failure for retry.
AGENTS.md reference: AGENTS.md:L28-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5911a80: write_host_pid creates the config directory before writing host.pid.
…hen the host is not notified
|
Addressed in 5911a80, with origin/main (#97) merged in 54844ef:
Also in 5911a80: the Codex findings on dd2e24b (replied inline). Tests: |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54844ef8cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| current.chroma = plan.chroma; | ||
| settings.borrow_and_update(); | ||
| if let Some(sync) = sync_tx.as_ref() { | ||
| let native = x11_resized(&display, portal_stream, size).unwrap_or(size); |
There was a problem hiding this comment.
When the native Wayland backend has switched to a second output with different dimensions and a 4:4:4 stream is then switched to VAAPI, x11_resized always returns None for wayland: displays, so this falls back to the original connection size. read_encoder uses current.source_id for the actual output, while sync_payload serializes this stale value into fields 5/6/9/10; the viewer is therefore told the old resolution with the 4:2:0 downgrade. Derive native from the active Wayland source, as the capture-setting path does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b9de098: the sync payload's native size now comes from wayland::sync_native, which reads the output at current.source_id (the same source read_encoder captures) for wayland: displays and keeps the connection size otherwise. Covered by sync_native_follows_the_wayland_source_id.
| fn probe_finish_is_not_a_reload_since_the_session_snapshot() { | ||
| let at_session = reload_generation_value(); | ||
| // A finished background probe updates the cache only. It does not bump the generation. | ||
| assert!(!reload_since_session(at_session, reload_generation_value())); |
There was a problem hiding this comment.
Under the default parallel cargo test run, this assertion shares the process-global reload generation with reload_does_not_clear_the_probe_cache, which calls notify_reload() without a common lock. If that test increments the generation between at_session and this assertion, the no-reload assertion fails even though the product behavior is correct; serialize all generation-mutating tests or make the generation local/injectable.
AGENTS.md reference: AGENTS.md:L66-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b9de098: every test that reads or bumps the reload generation (reload_does_not_clear_the_probe_cache, probe_finish_is_not_a_reload_since_the_session_snapshot, mark_reload_seen_clears_has_changed) now holds RELOAD_TEST_LOCK.
…-generation tests
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9de0988e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| tokio::spawn(async move { | ||
| while let Some(payload) = chroma_sync_rx.recv().await { | ||
| let timestamp = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap_or_default().as_secs(); | ||
| let seq = sync_seq.fetch_add(1, std::sync::atomic::Ordering::Relaxed); | ||
| let _ = sync_out.send(crate::initialization::envelope(seq, timestamp, 25, &payload)); |
There was a problem hiding this comment.
If the supported TEXT-channel dispatch applies 4:4:4 before CONTROL has opened and a SIGUSR2 then switches the stream to VAAPI, stream_desktop_at queues the downgrade in chroma_sync_rx. This task begins draining as soon as the remote CONTROL channel is announced, but webrtc-rs invokes that announcement before handle_open; RTCDataChannel::send calls ensure_open() and returns ErrClosedPipe while the channel is Connecting (vendor/webrtc/src/data_channel/mod.rs:364-371), so the outbound loop at lines 469-473 exits and drops both Message 25 and all later CONTROL replies. Start draining from CONTROL's on_open (or explicitly wait for Open) so the required actual-chroma sync cannot tear down the outbound queue.
AGENTS.md reference: AGENTS.md:L193-L197
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e3588c4: the CONTROL outbound drain is no longer spawned when the channel is announced. It now starts from CONTROL's existing on_open callback (webrtc-rs keeps a single open handler, so it is added there rather than registering a second one). TEXT, FILE and BINARY still drain immediately. Replies queued before Open, including the Message 25 chroma sync, stay in the unbounded queue until the drain starts. The receiver is taken once (take_outbound_once, unit-tested). cargo test --bin uuremote-server: 245 passed, 5 ignored.
…plies are not dropped
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
感谢 @ferstar！完整路径比对、未通知时的提示，以及 Codex 后续提的几条（Wayland 原生尺寸、reload 测试串行、CONTROL 打开后再发送）都处理得很到位。已 squash 合并，会随下一版发布；真实客户端下的切换时序发版后我们再实机确认。 🤖 Generated with Claude Code |
Summary
set-encoderandset-codecwritehost_settings.jsonand, whenhost.pidnames a running copy of this binary, sendSIGUSR2. Each live desktop stream restarts its encoder withEncoderTask::restart(sequence kept, first frame an IDR) and keeps the codec negotiated for that session. A codec-only change is logged and applied on the next desktop connection.SIGUSR2does not re-probe. VAAPI with an active 4:4:4 stream encodes 4:2:0 and sends Message 25 CaptureSettingSync (field 3). A failed start uses the existing in-process → FFmpeg process → CPU chain and leaves the peer up.What changed
video_select::plan_switchdecides restart vs deferred codec vs no-op, including the VAAPI 4:4:4 downgrade.stream_desktopwatches a process-wide generation and restarts only that stream's encoder. Later viewer FPS, quality, and resolution restarts use the encoder selected by the last live switch. The startup refresh runs only when the reload generation moved since the session snapshot.set-encoderandset-codecboth signalhost.pid. The SIGUSR2 handler and pid file are installed for every normal host before assist setup, includingUU_REMOTE_ASSIST=0. Codec output still says the preference applies on the next desktop connection.Not included
UU_VIDEO_ENCODER/ explicit file values vs v0.2.23) is unchanged.Compatibility
With no signal, stream startup still uses the selection taken when the peer is created.
UU_VIDEO_ENCODERand explicitvideo_encodervalues keep the same priority.SIGUSR1remainsshow.Known limitations
A live switch does not re-send device-capability / sender-parameter events, so the viewer shows the connect-time encoder until the next connection. The shared encoder used by capture-setting RPCs is updated when that switch also sends CaptureSettingSync; a switch that leaves chroma unchanged keeps the connect-time encoder for later 4:4:4 requests until the next connection. A hand-sent
SIGUSR2after a codec-only edit logs the deferred-codec line on each running desktop stream and does not restart video. Real-viewer timing (picture back within about a second, file transfer and terminal left running) is not covered by these unit tests.Tests
cargo test --locked: 243 passed, 0 failed, 5 ignored.uu-remote-linux(bin "uuremote-server" test) generated 21 warnings.cargo test --locked --features inproc-av: 247 passed, 0 failed, 5 ignored. Same target generated 21 warnings.cargo test --locked --features wayland,portal: 251 passed, 0 failed, 7 ignored. Same target generated 18 warnings (portal stubs that are dead on the default build are compiled in).