From 913b0b6b20518e6eaabd569c405579574cc4963d Mon Sep 17 00:00:00 2001 From: Mollusk Date: Fri, 26 Jun 2026 17:54:06 -0400 Subject: [PATCH] feat(screenshare): pass --strict-audio + surface app_audio drop warning (A23 P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consumes the new pixelpass --strict-audio mode + app_audio events (pixelpass 85fdebe) to close Codex's A23 P2: the per-app pick alone was best-effort — pixelpass would fall back to whole-desktop audio before the app routed and again if it stopped, both reintroducing the call echo. - host_args appends --strict-audio alongside --app=, so pixelpass never mirrors the desktop for our share: viewers hear only the chosen app or silence. - Parse the app_audio JSON event (routed/lost) into PixelpassEvent; spawn_host takes an optional notices channel and the stdout drain forwards events on it. - Core spawns a forwarder (only when an app is selected) mapping routed/lost to UiEvent::ShareAudioActive(bool); the channel/task self-terminate on host EOF. - App tracks share_audio_dropped and shows a transient warning under the "Sharing your screen" badge when the chosen app's audio stops ("viewers hear silence until it plays again"). Reset on start/stop/room-leave. In-process mpsc only — no wire/GOSSIP_PROTO change. 435 lib tests (+2: app_audio parse + the dropped-flag state machine), clippy --all-targets + release clean. Co-Authored-By: Claude Opus 4.8 --- src/app/mod.rs | 79 ++++++++++++++++++++++++++++++++++-- src/core/messages.rs | 5 +++ src/core/mod.rs | 28 ++++++++++++- src/screenshare/mod.rs | 90 ++++++++++++++++++++++++++++++++++-------- 4 files changed, 181 insertions(+), 21 deletions(-) diff --git a/src/app/mod.rs b/src/app/mod.rs index bb37374..04a55af 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -588,6 +588,11 @@ pub struct AppState { /// the picker (and re-confirming) during that startup window. Cleared on /// `ScreenShareStarted`, `ScreenShareStopped`, or any `Error`. share_starting: bool, + /// While sharing a specific app's audio (A23 strict mode): `true` when that + /// app's audio has stopped, so viewers currently hear silence. Drives a + /// transient warning. Always `false` for whole-desktop shares (pixelpass + /// emits no `app_audio` events then) and when not sharing. + share_audio_dropped: bool, /// Whether the Chat drawer is open (drawer layout only). drawer_chat_open: bool, /// Raw mic level (normalized RMS, `0.0..=1.0`) for the settings meter. @@ -661,6 +666,7 @@ impl AppState { self.share_audio_apps.clear(); self.share_audio_selection = None; self.share_starting = false; + self.share_audio_dropped = false; } fn custom_sound_path(&self, sound: Sound) -> &str { @@ -786,6 +792,7 @@ impl Default for AppState { share_audio_apps: Vec::new(), share_audio_selection: None, share_starting: false, + share_audio_dropped: false, drawer_chat_open: false, mic_level: 0.0, mic_test_active: false, @@ -1375,6 +1382,7 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { UiEvent::ScreenShareStarted => { state.self_sharing = true; state.share_starting = false; + state.share_audio_dropped = false; // Defensive: ensure no picker lingers across a successful start. state.share_picker_open = false; state.status_message = "Sharing your screen".to_string(); @@ -1382,8 +1390,13 @@ fn update(state: &mut AppState, message: AppMessage) -> Task { UiEvent::ScreenShareStopped => { state.self_sharing = false; state.share_starting = false; + state.share_audio_dropped = false; state.status_message = "Screen share stopped".to_string(); } + UiEvent::ShareAudioActive(active) => { + // Per-app audio routed/lost (only while sharing a chosen app). + state.share_audio_dropped = !active; + } UiEvent::IdentityStatus { node_id, persisted, error } => { state.self_node_id = Some(node_id); state.identity_persisted = persisted; @@ -4069,16 +4082,32 @@ fn view(state: &AppState) -> Element<'_, AppMessage> { }; el }, - // Live "you're sharing" badge — only present while sharing. + // Live "you're sharing" badge — only present while sharing. When + // sharing a specific app whose audio has dropped (A23 strict + // mode), a warning line is added: viewers hear silence, not the + // call, until that app plays again. { let el: Element<'_, AppMessage> = if state.self_sharing { - row![ + let badge = row![ icon(IconKind::Live, 14.0, color_red), text("Sharing your screen").size(13).color(color_red), ] .spacing(6) - .align_y(iced::alignment::Vertical::Center) - .into() + .align_y(iced::alignment::Vertical::Center); + if state.share_audio_dropped { + column![ + badge, + text( + "⚠ Shared app's audio stopped — viewers hear silence until it plays again" + ) + .size(11) + .color(color_yellow), + ] + .spacing(3) + .into() + } else { + badge.into() + } } else { iced::widget::Space::new().width(0.0).height(0.0).into() }; @@ -6317,6 +6346,7 @@ mod tests { state.share_audio_apps = vec!["Firefox".to_string()]; state.share_audio_selection = Some("Firefox".to_string()); state.share_starting = true; + state.share_audio_dropped = true; state.clip_status.lock().unwrap().playing_id = Some(attachment_id); state.reset_room_state(); @@ -6341,6 +6371,7 @@ mod tests { assert!(state.share_audio_apps.is_empty()); assert!(state.share_audio_selection.is_none()); assert!(!state.share_starting); + assert!(!state.share_audio_dropped); for _ in 0..50 { if crate::audio::clip_player::status_snapshot(&state.clip_status).playing_id.is_none() { @@ -6409,6 +6440,46 @@ mod tests { assert!(state.share_picker_open); } + #[test] + fn share_audio_dropped_tracks_app_audio_events() { + // While sharing a specific app, app_audio lost/routed toggles the warning + // flag; start and stop both reset it so it can't linger across sessions. + let mut state = AppState::default(); + + // Start sharing — flag is clear. + let _ = update( + &mut state, + AppMessage::UiEventReceived(UiEvent::ScreenShareStarted), + ); + assert!(!state.share_audio_dropped); + + // The chosen app's audio stops → warning on. + let _ = update( + &mut state, + AppMessage::UiEventReceived(UiEvent::ShareAudioActive(false)), + ); + assert!(state.share_audio_dropped); + + // It plays again → warning off. + let _ = update( + &mut state, + AppMessage::UiEventReceived(UiEvent::ShareAudioActive(true)), + ); + assert!(!state.share_audio_dropped); + + // Drop again, then stop sharing → flag reset regardless. + let _ = update( + &mut state, + AppMessage::UiEventReceived(UiEvent::ShareAudioActive(false)), + ); + assert!(state.share_audio_dropped); + let _ = update( + &mut state, + AppMessage::UiEventReceived(UiEvent::ScreenShareStopped), + ); + assert!(!state.share_audio_dropped); + } + #[test] fn peer_gate_persists_when_on_and_clears_when_off() { let mut config = AppConfig::default(); diff --git a/src/core/messages.rs b/src/core/messages.rs index c6f888d..2a920c7 100644 --- a/src/core/messages.rs +++ b/src/core/messages.rs @@ -155,6 +155,11 @@ pub enum UiEvent { ScreenShareStarted, /// Our own screen share stopped (or failed to start). ScreenShareStopped, + /// Per-app screen-share audio routing state (A23). `true` = the app we chose + /// is now reaching viewers; `false` = its audio stopped, so under our strict + /// run viewers currently hear silence. The UI shows a transient warning while + /// `false`. Only meaningful while sharing a specific app (not whole-desktop). + ShareAudioActive(bool), /// Our node identity (W7): the current node id string, and whether it is /// PERSISTED to disk. Sent once at startup and again after a regenerate. /// `persisted = false` means the key file couldn't be read/written and we're diff --git a/src/core/mod.rs b/src/core/mod.rs index ee6caaa..2add083 100644 --- a/src/core/mod.rs +++ b/src/core/mod.rs @@ -2660,7 +2660,33 @@ async fn run_core_loop( continue; } }; - match crate::screenshare::spawn_host(&bin, audio_app.as_deref()).await { + // Forward pixelpass `app_audio` events (only emitted when an app + // is selected) to the UI so it can warn when the chosen app's + // audio drops. The channel closes when the host dies (drain hits + // EOF), ending the forwarder task on its own. + let notices = audio_app.as_deref().map(|_| { + let (tx, mut rx) = + tokio::sync::mpsc::unbounded_channel::(); + let ui_tx_notices = ui_tx.clone(); + tokio::spawn(async move { + while let Some(ev) = rx.recv().await { + let active = match ev { + crate::screenshare::PixelpassEvent::AppAudioRouted => true, + crate::screenshare::PixelpassEvent::AppAudioLost => false, + _ => continue, + }; + if ui_tx_notices + .send(UiEvent::ShareAudioActive(active)) + .await + .is_err() + { + break; + } + } + }); + tx + }); + match crate::screenshare::spawn_host(&bin, audio_app.as_deref(), notices).await { Ok((child, ticket)) => { crate::log_msg("Screen share host started"); session.screenshare_host = Some(child); diff --git a/src/screenshare/mod.rs b/src/screenshare/mod.rs index 7845e1a..7294ed5 100644 --- a/src/screenshare/mod.rs +++ b/src/screenshare/mod.rs @@ -68,6 +68,12 @@ pub enum PixelpassEvent { CaptureStarted, /// Host: capture pipeline torn down (on last viewer). CaptureStopped, + /// Host (per-app audio): the chosen app's audio is now reaching viewers. + AppAudioRouted, + /// Host (per-app audio): the chosen app's last audio stream went away. Under + /// our `--strict-audio` run this means viewers now hear silence (not the call + /// echo) until the app produces audio again — we surface it as a warning. + AppAudioLost, /// A recognized event we don't act on (e.g. `host_info`). Other, } @@ -102,6 +108,11 @@ pub fn parse_pixelpass_event(line: &str) -> Option { Some("stopped") => PixelpassEvent::CaptureStopped, _ => PixelpassEvent::Other, }, + "app_audio" => match v.get("state").and_then(|s| s.as_str()) { + Some("routed") => PixelpassEvent::AppAudioRouted, + Some("lost") => PixelpassEvent::AppAudioLost, + _ => PixelpassEvent::Other, + }, _ => PixelpassEvent::Other, }; Some(ev) @@ -112,9 +123,16 @@ fn json_u32(v: &serde_json::Value, key: &str) -> u32 { } /// Build the argv for a pixelpass *host*. Always `--host --output json`; when -/// `audio_app` is `Some`, append `--app=` so pixelpass captures only that -/// app's audio instead of the whole desktop sink monitor (which contains our own -/// call playout → the viewer would hear themselves echoed back, backlog A23). +/// `audio_app` is `Some`, append `--app= --strict-audio` so pixelpass +/// captures only that app's audio instead of the whole desktop sink monitor +/// (which contains our own call playout → the viewer would hear themselves +/// echoed back, backlog A23). +/// +/// `--strict-audio` is what makes the fix a guarantee rather than best-effort: +/// without it, pixelpass falls back to the whole-desktop loopback before the +/// app's first stream routes and again if the app's audio later stops — both of +/// which reintroduce the echo. With it, the viewer hears only the chosen app (or +/// silence), and pixelpass emits `app_audio` events we surface as a warning. /// /// The name is passed in the single-token `--app=` form so a value that /// happens to begin with `-` can never be reparsed as a pixelpass flag (clap @@ -129,6 +147,7 @@ pub fn host_args(audio_app: Option<&str>) -> Vec { ]; if let Some(name) = audio_app.and_then(sanitize_app_name) { args.push(format!("--app={name}")); + args.push("--strict-audio".to_string()); } args } @@ -265,7 +284,11 @@ pub fn is_available(config_override: Option<&str>) -> bool { /// drained in a background task so a full pipe can't stall the host. We do /// **not** pass `--max-viewers`: pixelpass bandwidth-measures its own safe cap, /// protecting the sharer's uplink, and refuses extras with `viewer_refused`. -pub async fn spawn_host(bin: &Path, audio_app: Option<&str>) -> std::io::Result<(Child, String)> { +pub async fn spawn_host( + bin: &Path, + audio_app: Option<&str>, + notices: Option>, +) -> std::io::Result<(Child, String)> { let mut child = Command::new(bin) .args(host_args(audio_app)) .stdin(Stdio::null()) @@ -299,7 +322,7 @@ pub async fn spawn_host(bin: &Path, audio_app: Option<&str>) -> std::io::Result< } }; - drain_in_background(lines, "host"); + drain_in_background(lines, "host", notices); Ok((child, ticket)) } @@ -346,7 +369,7 @@ pub async fn spawn_viewer(bin: &Path, ticket: &str) -> std::io::Result { return Err(e); } - drain_in_background(lines, "viewer"); + drain_in_background(lines, "viewer", None); Ok(child) } @@ -381,15 +404,24 @@ where } /// Keep reading the child's stdout to EOF in the background so a full pipe can't -/// stall it; log notable events for diagnostics. -fn drain_in_background(mut lines: tokio::io::Lines>, role: &'static str) -where +/// stall it; log notable events for diagnostics. When `notices` is `Some`, each +/// parsed event is also forwarded to the caller (the core, which translates the +/// `app_audio` ones into a UI warning); a send failure (receiver dropped) just +/// stops forwarding, draining continues. The task ends on EOF (child exited). +fn drain_in_background( + mut lines: tokio::io::Lines>, + role: &'static str, + notices: Option>, +) where R: tokio::io::AsyncRead + Unpin + Send + 'static, { tokio::spawn(async move { while let Ok(Some(line)) = lines.next_line().await { if let Some(ev) = parse_pixelpass_event(&line) { crate::log_msg(&format!("pixelpass {role}: {}", event_for_log(&ev))); + if let Some(tx) = ¬ices { + let _ = tx.send(ev); + } } } }); @@ -408,6 +440,8 @@ fn event_for_log(ev: &PixelpassEvent) -> String { PixelpassEvent::Refused(reason) => format!("viewer_refused reason={reason:?}"), PixelpassEvent::CaptureStarted => "capture_started".to_string(), PixelpassEvent::CaptureStopped => "capture_stopped".to_string(), + PixelpassEvent::AppAudioRouted => "app_audio_routed".to_string(), + PixelpassEvent::AppAudioLost => "app_audio_lost".to_string(), PixelpassEvent::Other => "other".to_string(), } } @@ -489,16 +523,17 @@ mod tests { #[test] fn host_args_with_app_appends_single_token_flag() { // The chosen app rides in the `--app=` single-token form so a - // name beginning with `-` can never be reparsed as a flag (A23). + // name beginning with `-` can never be reparsed as a flag (A23), plus + // `--strict-audio` so pixelpass never falls back to whole-desktop audio. assert_eq!( host_args(Some("Firefox")), - vec!["--host", "--output", "json", "--app=Firefox"] - ); - assert_eq!( - host_args(Some("-rm -rf")).last().unwrap(), - "--app=-rm -rf", - "hyphen-leading name stays a single token bound to --app" + vec!["--host", "--output", "json", "--app=Firefox", "--strict-audio"] ); + // The hyphen-leading name is still bound to --app as a single token; + // --strict-audio is the trailing flag. + let args = host_args(Some("-rm -rf")); + assert_eq!(args[3], "--app=-rm -rf"); + assert_eq!(args[4], "--strict-audio"); } #[test] @@ -645,6 +680,29 @@ mod tests { ); } + #[test] + fn parses_app_audio_states() { + // The wire contract from pixelpass's --strict-audio run (A23): routed = + // the chosen app's audio is live; lost = it stopped (viewers now silent). + assert_eq!( + parse_pixelpass_event(r#"{"event":"app_audio","state":"routed"}"#), + Some(PixelpassEvent::AppAudioRouted) + ); + assert_eq!( + parse_pixelpass_event(r#"{"event":"app_audio","state":"lost"}"#), + Some(PixelpassEvent::AppAudioLost) + ); + // Unknown / missing state is recognized-but-unused, not a parse failure. + assert_eq!( + parse_pixelpass_event(r#"{"event":"app_audio","state":"weird"}"#), + Some(PixelpassEvent::Other) + ); + assert_eq!( + parse_pixelpass_event(r#"{"event":"app_audio"}"#), + Some(PixelpassEvent::Other) + ); + } + #[test] fn recognized_but_unused_event_is_other() { assert_eq!(