Repository navigation
Start the desktop on a tool connection when the viewer sends capture_desktop (#99) - #101
Conversation
…desktop (#99) With a port-mapping connection open, the Mac viewer (4.42.0) does not offer a new desktop peer; it sends SimpleAction 4 capture_desktop on the existing capture_type=9 peer and waits for video, which the host never sent. A tool peer now keeps its desktop video unstarted and becomes the desktop on capture_desktop: the main loop checks the backend (headless and Portal are skipped with a log line), applies the peer's controller platform, closes peers that were offered as desktops, then the peer takes the input lease, starts the H.264 stream it negotiated, counts the session and repeats the desktop CONTROL initialization, lock state, clipboard and cursor. Port mapping, files and terminal keep running on the same peer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
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. |
|
@codex review |
![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: 5787d4cd20
ℹ️ 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 superseded: Vec<String> = sessions.iter().filter(|(id, old)| id.as_str() != ice && old.desktop) | ||
| .map(|(id, _)| id.clone()).collect(); |
There was a problem hiding this comment.
When a second tool connection sends capture_desktop, a previously promoted tool still has old.desktop == false even though old.role.is_desktop() is true, so this filter leaves its encoder and desktop role running while the new peer takes the input lease. The old viewer then continues receiving video without control, session accounting sees multiple desktops, and a subsequent desktop offer likewise fails to supersede it; stop/demote the old desktop role while preserving its underlying tool connection.
AGENTS.md reference: AGENTS.md:L30-L31
Useful? React with 👍 / 👎.
| if peer.capture.init_error.is_some() { | ||
| log::info!("Desktop request on a tool connection ignored: desktop capture unavailable"); | ||
| continue; |
There was a problem hiding this comment.
If desktop capture failed when the server started—for example because X11 was not ready—the tool peer retains ScreenCapture::unavailable, and this branch permanently rejects capture_desktop even after the display recovers. Ordinary desktop offers explicitly retry ScreenCapture::new() on this condition at lines 974–979, so promotion should perform equivalent recovery rather than requiring the user to establish a separate desktop peer.
Useful? React with 👍 / 👎.
A newer desktop (offer or another capture_desktop) now stops a promoted tool peer's video, input lease, session count and cursor sync while its port mapping, files and terminal keep running; capture_desktop on it promotes it again. Peers offered as desktops still close. capture_desktop on a tool peer whose capture was unavailable when offered retries ScreenCapture::new like a desktop offer, and the peer streams, maps input and reports sizes from the new capture. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed both Codex findings in f5fd9a6:
@codex review 🤖 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: f5fd9a6bdb
ℹ️ 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 { | ||
| if channel.label() == "TEXT_DATA_CHANNEL" { | ||
| if !desktop { return; } | ||
| if !desktop && !role.wait_desktop(&channel).await { return; } |
There was a problem hiding this comment.
When a tool connection opens CONTROL/TEXT before capture_desktop, its initial echo is answered while clipboard_ready is false; this wait later starts the clipboard bridge, but promotion never replays the echo with ff_clipboard. Because viewers only use declared features, clipboard remains disabled for the lifetime of the promoted desktop even after the bridge becomes ready. Replay the capability response when promotion initializes the bridge, as the Portal readiness path does.
AGENTS.md reference: AGENTS.md:L183-L186
Useful? React with 👍 / 👎.
| self.role.active.send_replace(false); | ||
| if let Some(session) = self.role.input.lock().unwrap().take() { session.disconnected(); } |
There was a problem hiding this comment.
If a promoted tool peer has a TEXT channel, its clipboard task passes the promotion wait once and then runs until that channel closes. demote() deliberately keeps the peer connection open and only flips active for video/cursor handling, so after another desktop takes over the superseded peer can still receive host clipboard offers and apply incoming clipboard updates. The clipboard bridge and its inbound routing should be stopped on demotion and restarted on a later promotion.
Useful? React with 👍 / 👎.
…99) The TEXT clipboard bridge of a promoted tool peer now stops when a newer desktop demotes it (no host offers, incoming updates are dropped) and starts again on the next capture_desktop. After promotion CONTROL declares ff_clipboard again once the bridge is ready, since the viewer's echo was answered before the clipboard existed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed both clipboard findings in c1201a1:
@codex review 🤖 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: c1201a1f3f
ℹ️ 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 screen = if peer.capture.init_error.is_some() { | ||
| match tokio::task::spawn_blocking(ScreenCapture::new).await { | ||
| Ok(Ok(screen)) => { capture = Arc::new(screen); Some(capture.clone()) }, | ||
| Ok(Err(error)) => { log::info!("Desktop request on a tool connection ignored: {error}"); continue; }, | ||
| Err(_) => { log::warn!("Desktop initialization worker failed"); continue; }, | ||
| } | ||
| } else { None }; |
There was a problem hiding this comment.
When the X11 display size changes after this tool peer was created but before capture_desktop is received, this branch rebuilds the capture only if its original initialization failed. Unlike the normal desktop-offer path at lines 972–982, it never compares x11_size() with the cached dimensions, so DesktopStream and the CONTROL initialization can retain the old size; when the root became larger, x11grab can continue successfully while showing only the old top-left region and advertising the wrong resolution. Apply the same resized check before promotion.
AGENTS.md reference: AGENTS.md:L123-L123
Useful? React with 👍 / 👎.
| self.role.active.send_replace(true); | ||
| let stream = self.role.stream.lock().unwrap().clone(); | ||
| if let Some(stream) = stream { stream.spawn(self.role.screen.lock().unwrap().clone(), self.role.active.subscribe()); } | ||
| if !cfg!(test) { track_session(self.pc.clone(), self.actions.clone(), self.role.active.subscribe()); } |
There was a problem hiding this comment.
If a demoted tool peer is promoted again before its previous track_session task reaches the one-second poll at lines 122–123, this starts a second tracker while the shared PeerActions.started flag is still true. The second session_started() is therefore ignored, after which the old tracker calls session_ended() and decrements the desktop count while the re-promoted stream remains active; that can restore mute/virtual audio and trigger auto-lock mid-session, and the new tracker cannot restore the count. End or generation-fence the previous tracker before starting another one.
AGENTS.md reference: AGENTS.md:L304-L310
Useful? React with 👍 / 👎.
…e_desktop (#99) A peer's session tracker now starts once and follows every promotion and demotion, so a quick demote and re-promote cannot end the desktop count of a stream that is still running. capture_desktop rebuilds the capture when the X11 root size changed since the peer opened, as a desktop offer does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed in ce03b4c:
@codex review 🤖 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: ce03b4c1c0
ℹ️ 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".
| *self.role.input.lock().unwrap() = Some(session); | ||
| self.role.active.send_replace(true); | ||
| let stream = self.role.stream.lock().unwrap().clone(); | ||
| if let Some(stream) = stream { stream.spawn(self.role.screen.lock().unwrap().clone(), self.role.active.subscribe()); } |
There was a problem hiding this comment.
When a promoted tool peer is demoted and re-promoted quickly, the existing stream task can miss the transient active=false because a watch channel retains only its latest value. This line then starts a second stream_desktop_at task while the first remains active, leaving two encoders writing concurrently to the same video track and potentially causing excess resource usage and corrupted playback. Use a generation/cancellation mechanism or wait for the prior stream task to terminate before spawning its replacement.
Useful? React with 👍 / 👎.
The stream task's abort handle is kept on the peer; demotion aborts it and a new promotion replaces it, so a demote and re-promote faster than the watch can report never runs two encoders on one track. Dropping the stream stops its encoder (kill_on_drop) and capture workers (stop-on-drop). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed in 305d42f: Fence the previous stream before starting another. The peer now keeps the abort handle of the stream it started. Demotion aborts it, and a new promotion aborts any previous one before starting its own. A demote followed by a re-promotion faster than the watch reports can therefore no longer run two encoders on the same track. Dropping the stream future stops the encoder (
@codex review 🤖 Generated with Claude Code |
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
Fixes the case in #99: with a port-mapping connection open, the Mac viewer (4.42.0) opens the desktop by sending SimpleAction 4
capture_desktopon the existingcapture_type=9peer instead of offering a new desktop peer. The host ignored it, so the window stayed on 「正在传输画面」.capture_desktopthe main loop checks the backend (headless and Portal are skipped with a log line), applies the controller platform, and closes peers that were offered as desktops. The peer then takes the input lease, starts the H.264 stream, counts the session, and repeats the desktop CONTROL initialization, lock state, clipboard and cursor.Tests:
cargo test --offlinepasses on macOS (247 passed) and Debian (247 passed, 6 ignored). Addscapture_desktop_on_a_port_mapping_peer_promotes_it_once. Not yet tested with a real Mac viewer.🤖 Generated with Claude Code