Hotfix screen share - Handling some cases not working on Windows#29
Hotfix screen share - Handling some cases not working on Windows#29
Conversation
WalkthroughAdds Windows-specific device enumeration and robust localtunnel spawning with multiple fallback commands, tracks FFmpeg and tunnel PIDs in Changes
Sequence DiagramsequenceDiagram
participant Client
participant HLS as HLS Server
participant FFmpeg
participant Localtunnel
participant OS
Client->>HLS: start_hls_server_cmd()
activate HLS
HLS->>HLS: list_ffmpeg_devices() (platform-specific)
HLS->>HLS: check_localtunnel() (Windows: probe Node.js)
HLS->>FFmpeg: spawn FFmpeg (video [+ audio inputs on Windows])
activate FFmpeg
FFmpeg-->>HLS: process started
HLS->>HLS: capture ffmpeg_pid
rect rgb(200,220,255)
Note over HLS,Localtunnel: Windows: try cmd /C npx → npx.cmd → npx
end
HLS->>Localtunnel: spawn npx localtunnel
activate Localtunnel
Localtunnel-->>HLS: stdout (URL/domain)
HLS->>HLS: start_localtunnel_common() parse output
HLS->>HLS: capture tunnel_pid
HLS-->>Client: return HlsServerHandle (with PIDs)
deactivate HLS
Client->>HLS: stop_hls_server_cmd()
activate HLS
rect rgb(255,220,220)
Note over HLS,OS: kill_process_forcefully uses PID-aware termination<br/>Windows: taskkill for process tree
end
HLS->>OS: kill_process_forcefully(ffmpeg_pid) → FFmpeg stops
HLS->>OS: kill_process_forcefully(tunnel_pid) → Localtunnel stops
HLS->>HLS: stop HTTP server & cleanup
HLS-->>Client: stopped
deactivate HLS
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Larry8668
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src-tauri/src/lib.rs(8 hunks)
🔇 Additional comments (8)
src-tauri/src/lib.rs (8)
577-580: LGTM!The addition of PID tracking fields is appropriate for Windows process tree termination. Using
Option<u32>correctly handles cases where PID retrieval might fail.
974-1100: LGTM! Good refactoring of common logic.Extracting the localtunnel output parsing into a shared function improves maintainability. The timeout handling and stream restoration logic is appropriate for dealing with unpredictable localtunnel output.
1589-1600: LGTM!FFmpeg PID capture is implemented correctly using
child.id(), with appropriate logging for both success and failure cases.
1609-1626: LGTM!Tunnel PID capture follows the same pattern as FFmpeg, correctly extracting the PID and including it in the returned tuple.
1633-1636: LGTM!PIDs are correctly stored in the
HlsServerHandlefor later use during cleanup.
1660-1692: LGTM with minor concern on error handling.The
kill_process_tree_windowsfunction correctly usestaskkill /F /T /PIDto terminate process trees. The handling of "not found" errors as success (lines 1678-1680) is reasonable since the process may already be terminated. However, treating all other stderr warnings as success (lines 1682-1683) might mask real issues.Consider logging warnings more prominently or collecting them for debugging purposes.
1751-1786: LGTM! Improved cleanup with detailed logging.The enhanced
stop_hls_server_cmdfunction now uses the newkill_process_forcefullyhelper with stored PIDs, and provides detailed logging at each step. The error handling is appropriate—logging warnings but continuing with cleanup ensures maximum cleanup even if individual components fail.Note: This function depends on the fix to
kill_process_forcefullymentioned in the previous comment.
722-831: Windows device enumeration implementation is correct for the documented FFmpeg dshow format.The parsing logic accurately matches the actual FFmpeg dshow output structure. The code correctly:
- Identifies the audio devices section
- Extracts device names from quoted strings matching the pattern
[dshow @ ...] "Device Name"- Returns devices in the expected JSON format
The
"Alternative"filter on the extracted device name is not necessary based on the documented output format, though it's a harmless defensive measure. Error handling is present for FFmpeg failures.Consider verifying on an actual Windows system that devices enumerate consistently, since parsing depends on output format stability.
| // On Windows, try cmd.exe /c npx first, then direct npx | ||
| #[cfg(target_os = "windows")] | ||
| { | ||
| // Try cmd.exe /c npx (Windows command prompt) | ||
| let npx_check = Command::new("cmd") | ||
| .args(&["/C", "npx", "--version"]) | ||
| .output() | ||
| .await; | ||
|
|
||
| if npx_check.is_ok() && npx_check.as_ref().unwrap().status.success() { | ||
| return Ok(true); | ||
| } | ||
|
|
||
| // Fallback: try direct npx (might work if Node.js is in PATH) | ||
| let npx_direct = Command::new("npx") | ||
| .arg("--version") | ||
| .output() | ||
| .await; | ||
|
|
||
| if npx_direct.is_ok() && npx_direct.as_ref().unwrap().status.success() { | ||
| return Ok(true); | ||
| } | ||
|
|
||
| // Check if Node.js is installed by checking common locations | ||
| let node_check = Command::new("cmd") | ||
| .args(&["/C", "node", "--version"]) | ||
| .output() | ||
| .await; | ||
|
|
||
| if node_check.is_ok() && node_check.as_ref().unwrap().status.success() { | ||
| // Node.js exists but npx might not be in PATH | ||
| // This is still considered "available" as npx comes with npm | ||
| return Ok(true); | ||
| } | ||
|
|
||
| return Ok(false); | ||
| } | ||
|
|
||
| // Try to run localtunnel --help (this will download it if needed, but we just check if it works) | ||
| // Actually, we'll just check if npx works - localtunnel will be downloaded on first use | ||
| Ok(true) | ||
| #[cfg(not(target_os = "windows"))] | ||
| { | ||
| // Check if npx is available (macOS/Linux) | ||
| let npx_check = Command::new("npx") | ||
| .arg("--version") | ||
| .output() | ||
| .await; | ||
|
|
||
| if npx_check.is_err() { | ||
| return Ok(false); | ||
| } | ||
|
|
||
| Ok(true) | ||
| } | ||
| } |
There was a problem hiding this comment.
Review the Node.js fallback logic.
The multiple fallback methods for Windows are pragmatic, but the Node.js check (lines 860-870) may give false positives. If node --version succeeds but npx is not in PATH, the function returns Ok(true), suggesting localtunnel is available when it might not be accessible.
Consider either:
- Removing the Node.js fallback and only checking
npxavailability - Updating the error message in
start_localtunnelto guide users when Node.js exists butnpxis not in PATH
🤖 Prompt for AI Agents
In src-tauri/src/lib.rs around lines 837 to 889, the Windows fallback that
treats a successful `node --version` as evidence that `npx` is available can
produce false positives; remove the Node.js fallback and only consider `npx`
availability. Specifically, delete the `node_check` block (lines ~860-870) and
its return-true path so the Windows branch first tries `cmd /C npx --version`,
then direct `npx --version`, and if neither succeeds return Ok(false); keep the
non-Windows branch unchanged. This ensures the function only returns true when
`npx` is actually runnable.
| async fn start_localtunnel(port: u16) -> anyhow::Result<(tokio::process::Child, String, String)> { | ||
| let mut cmd = Command::new("npx"); | ||
| cmd.args(&["-y", "localtunnel", "--port", &port.to_string()]); | ||
| cmd.stdout(Stdio::piped()); | ||
| cmd.stderr(Stdio::piped()); | ||
| #[cfg(target_os = "windows")] | ||
| { | ||
| // On Windows, try multiple methods to run npx | ||
| // Method 1: Try cmd.exe /C npx (works if npx is in PATH) | ||
| let mut cmd = Command::new("cmd"); | ||
| cmd.args(&["/C", "npx", "-y", "localtunnel", "--port", &port.to_string()]); | ||
| cmd.stdout(Stdio::piped()); | ||
| cmd.stderr(Stdio::piped()); | ||
| cmd.stdin(Stdio::null()); // Prevent cmd from waiting for input | ||
|
|
||
| match cmd.spawn() { | ||
| Ok(child) => { | ||
| eprintln!("✅ Started localtunnel via cmd.exe /C npx"); | ||
| return start_localtunnel_common(child, port).await; | ||
| } | ||
| Err(e1) => { | ||
| eprintln!("⚠️ Failed to start localtunnel via cmd.exe /C npx: {}", e1); | ||
|
|
||
| // Method 2: Try npx.cmd directly (Windows-specific) | ||
| let mut cmd2 = Command::new("npx.cmd"); | ||
| cmd2.args(&["-y", "localtunnel", "--port", &port.to_string()]); | ||
| cmd2.stdout(Stdio::piped()); | ||
| cmd2.stderr(Stdio::piped()); | ||
|
|
||
| match cmd2.spawn() { | ||
| Ok(child) => { | ||
| eprintln!("✅ Started localtunnel via npx.cmd"); | ||
| return start_localtunnel_common(child, port).await; | ||
| } | ||
| Err(e2) => { | ||
| eprintln!("⚠️ Failed to start localtunnel via npx.cmd: {}", e2); | ||
|
|
||
| // Method 3: Try npx directly (might work if Node.js is in PATH) | ||
| let mut cmd3 = Command::new("npx"); | ||
| cmd3.args(&["-y", "localtunnel", "--port", &port.to_string()]); | ||
| cmd3.stdout(Stdio::piped()); | ||
| cmd3.stderr(Stdio::piped()); | ||
|
|
||
| match cmd3.spawn() { | ||
| Ok(child) => { | ||
| eprintln!("✅ Started localtunnel via npx directly"); | ||
| return start_localtunnel_common(child, port).await; | ||
| } | ||
| Err(e3) => { | ||
| eprintln!("⚠️ Failed to start localtunnel via npx directly: {}", e3); | ||
| return Err(anyhow::anyhow!( | ||
| "Failed to start localtunnel. Tried multiple methods:\n\ | ||
| 1. cmd.exe /C npx: {}\n\ | ||
| 2. npx.cmd: {}\n\ | ||
| 3. npx: {}\n\n\ | ||
| Make sure Node.js and npm are installed and in your PATH.\n\ | ||
| On Windows, install Node.js from https://nodejs.org/ and restart your terminal/application.", | ||
| e1, e2, e3 | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let mut child = cmd.spawn()?; | ||
| #[cfg(not(target_os = "windows"))] | ||
| { | ||
| // On macOS/Linux, use npx directly | ||
| let mut cmd = Command::new("npx"); | ||
| cmd.args(&["-y", "localtunnel", "--port", &port.to_string()]); | ||
| cmd.stdout(Stdio::piped()); | ||
| cmd.stderr(Stdio::piped()); | ||
|
|
||
| let child = cmd.spawn().map_err(|e| { | ||
| anyhow::anyhow!( | ||
| "Failed to start localtunnel: {}. Make sure Node.js and npm are installed and in your PATH.", | ||
| e | ||
| ) | ||
| })?; | ||
|
|
||
| return start_localtunnel_common(child, port).await; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider setting stdin to null() on all platforms.
The Windows implementation sets stdin(Stdio::null()) on line 901 to prevent cmd.exe from waiting for input. However, the non-Windows implementation (lines 955-971) does not set stdin. If npx or localtunnel ever prompts for input, this could cause the process to hang.
Apply this diff to add stdin handling to non-Windows platforms:
#[cfg(not(target_os = "windows"))]
{
// On macOS/Linux, use npx directly
let mut cmd = Command::new("npx");
cmd.args(&["-y", "localtunnel", "--port", &port.to_string()]);
cmd.stdout(Stdio::piped());
cmd.stderr(Stdio::piped());
+ cmd.stdin(Stdio::null());
let child = cmd.spawn().map_err(|e| {🤖 Prompt for AI Agents
In src-tauri/src/lib.rs around lines 892 to 972, the non-Windows branch that
spawns `npx localtunnel` does not set stdin to null, so the child could hang if
the process prompts for input; update the command builder in the
#[cfg(not(target_os = "windows"))] block to call stdin(Stdio::null()) along with
stdout(Stdio::piped()) and stderr(Stdio::piped()) before spawning, then spawn
and await start_localtunnel_common as before so the child cannot block waiting
for input.
| // Helper function to kill a process (cross-platform) | ||
| async fn kill_process_forcefully(child: &mut tokio::process::Child, pid: Option<u32>) -> Result<(), String> { | ||
| #[cfg(target_os = "windows")] | ||
| { | ||
| // On Windows, use taskkill to kill process tree | ||
| if let Some(pid) = pid { | ||
| // Try to get PID from child if not provided | ||
| let actual_pid = pid; | ||
| if let Err(e) = kill_process_tree_windows(actual_pid).await { | ||
| eprintln!("⚠️ Failed to kill process tree, trying direct kill: {}", e); | ||
| // Fallback to direct kill | ||
| if let Err(e) = child.kill().await { | ||
| eprintln!("⚠️ Direct kill also failed: {}", e); | ||
| return Err(format!("Failed to kill process: {}", e)); | ||
| } | ||
| } | ||
| } else { | ||
| // No PID, try direct kill | ||
| if let Err(e) = child.kill().await { | ||
| eprintln!("⚠️ Direct kill failed: {}", e); | ||
| return Err(format!("Failed to kill process: {}", e)); | ||
| } | ||
| } | ||
|
|
||
| // Wait a bit for process to terminate | ||
| tokio::time::sleep(tokio::time::Duration::from_millis(500)).await; | ||
|
|
||
| // Try to wait for the process to exit | ||
| let _ = child.wait().await; | ||
| } | ||
|
|
||
| #[cfg(not(target_os = "windows"))] | ||
| { | ||
| // On macOS/Linux, direct kill should work | ||
| if let Err(e) = child.kill().await { | ||
| eprintln!("⚠️ Failed to kill process: {}", e); | ||
| return Err(format!("Failed to kill process: {}", e)); | ||
| } | ||
|
|
||
| // Wait for process to terminate | ||
| let _ = child.wait().await; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Critical: Fix useless variable shadowing and orphaned process risk.
Line 1701 contains let actual_pid = pid; which simply shadows the parameter without adding value. This appears to be leftover from refactoring.
More importantly, on Windows when pid is None (lines 1710-1716), the function falls back to child.kill(), which on Windows does NOT kill child processes. This means:
- FFmpeg started via
cmd.exemay leavecmd.exealive npxspawned processes may leave Node.js child processes orphaned
Apply this diff to fix both issues:
async fn kill_process_forcefully(child: &mut tokio::process::Child, pid: Option<u32>) -> Result<(), String> {
#[cfg(target_os = "windows")]
{
// On Windows, use taskkill to kill process tree
if let Some(pid) = pid {
- // Try to get PID from child if not provided
- let actual_pid = pid;
- if let Err(e) = kill_process_tree_windows(actual_pid).await {
+ if let Err(e) = kill_process_tree_windows(pid).await {
eprintln!("⚠️ Failed to kill process tree, trying direct kill: {}", e);
// Fallback to direct kill
if let Err(e) = child.kill().await {
eprintln!("⚠️ Direct kill also failed: {}", e);
return Err(format!("Failed to kill process: {}", e));
}
}
} else {
- // No PID, try direct kill
+ // No PID available - try to get it from child
+ if let Some(child_pid) = child.id() {
+ eprintln!("⚠️ Using PID from child: {}", child_pid);
+ if let Err(e) = kill_process_tree_windows(child_pid).await {
+ eprintln!("⚠️ Failed to kill process tree: {}", e);
+ return Err(format!("Failed to kill process: {}", e));
+ }
+ } else {
+ eprintln!("⚠️ No PID available, using direct kill (may orphan children)");
- if let Err(e) = child.kill().await {
- eprintln!("⚠️ Direct kill failed: {}", e);
- return Err(format!("Failed to kill process: {}", e));
+ if let Err(e) = child.kill().await {
+ eprintln!("⚠️ Direct kill failed: {}", e);
+ return Err(format!("Failed to kill process: {}", e));
+ }
}
}🤖 Prompt for AI Agents
In src-tauri/src/lib.rs around lines 1694 to 1738, remove the useless shadowing
of pid and ensure Windows handles orphaned child processes: replace the current
logic that does `let actual_pid = pid` and the branch that simply falls back to
child.kill() when pid is None with code that first tries to obtain a PID from
the parameter or from child.id(), call kill_process_tree_windows with that PID
when available, and only if no PID can be determined fall back to child.kill();
if kill_process_tree_windows fails, still attempt child.kill() as a fallback and
propagate a clear error on total failure. Ensure no variable shadowing (use a
single pid_or_child variable) and keep the wait/sleep behavior after kill
attempts.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src-tauri/src/lib.rs (3)
888-898: Critical: Node.js fallback creates false positives.The Node.js check on lines 888-898 returns
Ok(true)ifnode --versionsucceeds, even whennpxis not in PATH. This indicates that localtunnel is available when it might not be accessible vianpx, leading to failures whenstart_localtunnelis called.The previous review correctly identified this issue. Remove the Node.js fallback and only check for
npxavailability.Apply this diff:
- // Check if Node.js is installed by checking common locations - let node_check = Command::new("cmd") - .args(&["/C", "node", "--version"]) - .output() - .await; - - if node_check.is_ok() && node_check.as_ref().unwrap().status.success() { - // Node.js exists but npx might not be in PATH - // This is still considered "available" as npx comes with npm - return Ok(true); - } - return Ok(false);
987-989: Add stdin(Stdio::null()) to prevent potential hangs.The non-Windows implementation does not set stdin to null, which could cause the process to hang if
npxorlocaltunnelprompts for input. The Windows implementation correctly sets this on line 929.As flagged in the previous review, add stdin handling to non-Windows platforms.
Apply this diff:
let mut cmd = Command::new("npx"); cmd.args(&["-y", "localtunnel", "--port", &port.to_string()]); cmd.stdout(Stdio::piped()); cmd.stderr(Stdio::piped()); + cmd.stdin(Stdio::null());
1867-1911: Critical: Useless shadowing and orphaned process risk remain unaddressed.The issues flagged in the previous review are still present:
- Line 1874:
let actual_pid = pid;is useless variable shadowing- Lines 1883-1889: When
pidisNone, the fallback tochild.kill()does NOT kill child processes on Windows, which can orphancmd.exeand Node.js processesAs noted in the previous review, you should attempt to get the PID from
child.id()when the parameter isNone.Apply this diff:
#[cfg(target_os = "windows")] { // On Windows, use taskkill to kill process tree if let Some(pid) = pid { - // Try to get PID from child if not provided - let actual_pid = pid; - if let Err(e) = kill_process_tree_windows(actual_pid).await { + if let Err(e) = kill_process_tree_windows(pid).await { eprintln!("⚠️ Failed to kill process tree, trying direct kill: {}", e); // Fallback to direct kill if let Err(e) = child.kill().await { eprintln!("⚠️ Direct kill also failed: {}", e); return Err(format!("Failed to kill process: {}", e)); } } } else { - // No PID, try direct kill + // No PID available - try to get it from child + if let Some(child_pid) = child.id() { + eprintln!("⚠️ Using PID from child: {}", child_pid); + if let Err(e) = kill_process_tree_windows(child_pid).await { + eprintln!("⚠️ Failed to kill process tree: {}", e); + return Err(format!("Failed to kill process: {}", e)); + } + } else { + eprintln!("⚠️ No PID available, using direct kill (may orphan children)"); if let Err(e) = child.kill().await { eprintln!("⚠️ Direct kill failed: {}", e); return Err(format!("Failed to kill process: {}", e)); } + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src-tauri/src/lib.rs(11 hunks)
🔇 Additional comments (5)
src-tauri/src/lib.rs (5)
577-577: LGTM: PID fields added for Windows process management.The addition of
ffmpeg_pidandtunnel_pidfields enables proper process tree termination on Windows, which is essential for preventing orphaned child processes.Also applies to: 580-580
1002-1128: LGTM: Well-structured shared parsing logic.The refactored
start_localtunnel_commonfunction properly handles stream reading with timeouts and carefully restores unconsumed streams. The URL parsing logic is robust with multiple checks for edge cases.
1230-1242: LGTM: Windows video input correctly configured.The Windows path now correctly indicates that audio input is handled separately in
start_ffmpeg(lines 1329-1401), addressing the concern from the previous review about incomplete audio implementation.
1762-1809: LGTM: PIDs properly captured and stored.The PID capture logic correctly uses
child.id()and gracefully handles the case where PIDs are not available, with appropriate logging for debugging.
1924-1959: Cleanup logic is well-structured.The stop logic properly uses the new kill helpers and passes the stored PIDs for Windows process tree termination. Error handling is appropriate with warnings instead of hard failures. However, the effectiveness depends on fixing the critical issues in
kill_process_forcefully.
| for line in stderr.lines() { | ||
| if line.contains("[dshow @") && line.contains('"') && !line.contains("Alternative") { | ||
| if let Some(quote_start) = line.find('"') { | ||
| let after_quote = &line[quote_start + 1..]; | ||
| if let Some(quote_end) = after_quote.find('"') { | ||
| let device_name = after_quote[..quote_end].to_string(); | ||
| if !device_name.is_empty() && !device_name.contains("USB") && !device_name.contains("UVC") { | ||
| // Skip video devices (webcams), only capture audio devices | ||
| // This is a heuristic - dshow doesn't clearly separate audio/video | ||
| eprintln!(" ✓ Found dshow device: [{}] \"{}\"", device_index, device_name); | ||
| audio_devices.push(serde_json::json!({ | ||
| "index": device_index, | ||
| "name": device_name | ||
| })); | ||
| device_index += 1; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Review the device filtering heuristic.
The heuristic on line 805 filters out any device name containing "USB" or "UVC", assuming these are video devices (webcams). However, USB audio interfaces are common and would be incorrectly excluded by this logic. This could prevent users from selecting valid audio input devices.
Consider:
- Removing the USB/UVC filter entirely and letting users see all dshow devices
- Using a more precise detection method (e.g., checking if the device type is explicitly audio)
- Keeping both audio and video devices in separate lists if dshow doesn't clearly distinguish them
- if !device_name.is_empty() && !device_name.contains("USB") && !device_name.contains("UVC") {
- // Skip video devices (webcams), only capture audio devices
- // This is a heuristic - dshow doesn't clearly separate audio/video
+ if !device_name.is_empty() {
+ // Note: dshow doesn't clearly separate audio/video devices
+ // Include all devices and let the user choose
eprintln!(" ✓ Found dshow device: [{}] \"{}\"", device_index, device_name);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for line in stderr.lines() { | |
| if line.contains("[dshow @") && line.contains('"') && !line.contains("Alternative") { | |
| if let Some(quote_start) = line.find('"') { | |
| let after_quote = &line[quote_start + 1..]; | |
| if let Some(quote_end) = after_quote.find('"') { | |
| let device_name = after_quote[..quote_end].to_string(); | |
| if !device_name.is_empty() && !device_name.contains("USB") && !device_name.contains("UVC") { | |
| // Skip video devices (webcams), only capture audio devices | |
| // This is a heuristic - dshow doesn't clearly separate audio/video | |
| eprintln!(" ✓ Found dshow device: [{}] \"{}\"", device_index, device_name); | |
| audio_devices.push(serde_json::json!({ | |
| "index": device_index, | |
| "name": device_name | |
| })); | |
| device_index += 1; | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| for line in stderr.lines() { | |
| if line.contains("[dshow @") && line.contains('"') && !line.contains("Alternative") { | |
| if let Some(quote_start) = line.find('"') { | |
| let after_quote = &line[quote_start + 1..]; | |
| if let Some(quote_end) = after_quote.find('"') { | |
| let device_name = after_quote[..quote_end].to_string(); | |
| if !device_name.is_empty() { | |
| // Note: dshow doesn't clearly separate audio/video devices | |
| // Include all devices and let the user choose | |
| eprintln!(" ✓ Found dshow device: [{}] \"{}\"", device_index, device_name); | |
| audio_devices.push(serde_json::json!({ | |
| "index": device_index, | |
| "name": device_name | |
| })); | |
| device_index += 1; | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src-tauri/src/lib.rs around lines 799 to 819, the current dshow parsing
heuristics exclude any device whose name contains "USB" or "UVC", which will
wrongly filter out legitimate USB audio interfaces; remove the
!device_name.contains("USB") && !device_name.contains("UVC") check so USB
devices are not excluded, and instead either (a) include all parsed dshow
devices in the audio_devices list (letting the UI/user choose), or (b) attempt a
more precise check by inspecting the original stderr line for explicit audio
markers (e.g., "audio" / "audio capture" or other dshow type tokens) and only
filter based on those, or (c) collect all parsed devices into separate audio and
video lists (or tag each device with a "type" field) so the UI can present or
filter them accurately.
| for line in stderr.lines() { | ||
| if line.contains("[dshow @") && line.contains('"') && !line.contains("Alternative") { | ||
| if let Some(quote_start) = line.find('"') { | ||
| let after_quote = &line[quote_start + 1..]; | ||
| if let Some(quote_end) = after_quote.find('"') { | ||
| let device_name = after_quote[..quote_end].to_string(); | ||
| if !device_name.is_empty() && !device_name.contains("USB") && !device_name.contains("UVC") { | ||
| audio_devices.push(device_name); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Same fragile filtering heuristic duplicated.
This function duplicates the same USB/UVC filtering logic from list_ffmpeg_devices (line 1193), which incorrectly excludes USB audio interfaces. Additionally, the device enumeration logic is duplicated across both functions.
Consider:
- Removing the USB/UVC filter (same issue as flagged earlier)
- Extracting the device enumeration logic into a shared helper function to avoid duplication
🤖 Prompt for AI Agents
In src-tauri/src/lib.rs around lines 1187 to 1199, the ffmpeg stderr parsing
duplicates a fragile USB/UVC exclusion and the same device-enumeration logic
found in list_ffmpeg_devices; remove the checks that exclude device names
containing "USB" or "UVC" and refactor by extracting the parsing/enumeration
code into a shared helper function (e.g., parse_ffmpeg_device_lines or
enumerate_ffmpeg_devices) that accepts stderr text and returns a Vec<String>,
then replace the duplicated loop in both locations to call this helper.
| #[cfg(target_os = "windows")] | ||
| { | ||
| // On Windows, we need separate inputs for video (gdigrab) and audio (dshow) | ||
| // Parse device string: "video_index:audio_index" | ||
| let audio_index = device | ||
| .and_then(|d| d.split(':').nth(1)) | ||
| .and_then(|s| s.parse::<usize>().ok()) | ||
| .unwrap_or(0); | ||
|
|
||
| // Add video input (gdigrab) | ||
| args.extend(get_ffmpeg_input_args(device)); | ||
|
|
||
| // Get audio device name and add audio input | ||
| // Try wasapi first (if supported), fallback to dshow | ||
| if let Some(audio_device_name) = get_windows_audio_device_name(audio_index).await { | ||
| eprintln!("🔊 Using audio device: \"{}\" (index: {})", audio_device_name, audio_index); | ||
|
|
||
| // Check if wasapi is actually supported | ||
| let wasapi_test = Command::new("ffmpeg") | ||
| .args(&["-f", "wasapi", "-list_devices", "true", "-i", "dummy"]) | ||
| .output() | ||
| .await; | ||
|
|
||
| let use_wasapi = wasapi_test.is_ok() && | ||
| !String::from_utf8_lossy(&wasapi_test.as_ref().unwrap().stderr) | ||
| .contains("Unknown input format"); | ||
|
|
||
| if use_wasapi { | ||
| // Use wasapi (Windows Audio Session API) | ||
| args.extend(vec![ | ||
| "-f".to_string(), | ||
| "wasapi".to_string(), | ||
| "-i".to_string(), | ||
| format!("{}", audio_device_name), // wasapi uses device name directly | ||
| ]); | ||
| } else { | ||
| // Fallback to dshow | ||
| if audio_device_name == "default" || audio_device_name == "Default Audio Device" { | ||
| args.extend(vec![ | ||
| "-f".to_string(), | ||
| "dshow".to_string(), | ||
| "-i".to_string(), | ||
| "audio=default".to_string(), | ||
| ]); | ||
| } else { | ||
| args.extend(vec![ | ||
| "-f".to_string(), | ||
| "dshow".to_string(), | ||
| "-i".to_string(), | ||
| format!("audio=\"{}\"", audio_device_name), | ||
| ]); | ||
| } | ||
| } | ||
| } else { | ||
| eprintln!("⚠️ Could not find audio device at index {}, using default", audio_index); | ||
| // Use dshow default (wasapi likely not available) | ||
| args.extend(vec![ | ||
| "-f".to_string(), | ||
| "dshow".to_string(), | ||
| "-i".to_string(), | ||
| "audio=default".to_string(), | ||
| ]); | ||
| } | ||
|
|
||
| // Add stream mapping for Windows (map video from input 0, audio from input 1) | ||
| args.extend(vec![ | ||
| "-map".to_string(), | ||
| "0:v".to_string(), // Video from first input (gdigrab) | ||
| "-map".to_string(), | ||
| "1:a".to_string(), // Audio from second input (dshow) | ||
| ]); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Comprehensive Windows audio implementation, but validate device string format.
The Windows multi-input handling is well-implemented with proper wasapi/dshow detection and fallbacks. However, the device string parsing on lines 1333-1336 assumes the format "video_index:audio_index" without validation.
Consider adding validation:
let audio_index = device
.and_then(|d| d.split(':').nth(1))
.and_then(|s| s.parse::<usize>().ok())
.unwrap_or(0);
+
+ // Log if device string format is unexpected
+ if let Some(d) = device {
+ if !d.contains(':') {
+ eprintln!("⚠️ Warning: Unexpected device format '{}', expected 'video:audio'", d);
+ }
+ }🤖 Prompt for AI Agents
In src-tauri/src/lib.rs around lines 1329 to 1401, the code assumes device
string is "video_index:audio_index" when extracting audio_index (lines
~1333-1336); add validation so malformed or single-part strings don't panic or
misparse. Change the extraction to check whether device is Some and contains a
':' before splitting, attempt to parse the second part with proper error
handling (Result/ok), clamp or fallback negative/invalid parses to 0, and log a
warning when the device string is invalid or missing the audio index; if device
is provided without audio part treat it as video-only and use default audio
index 0. Ensure subsequent uses (get_ffmpeg_input_args(device) and audio device
lookup) still behave correctly when device is Some(video_only) or None.
1 participant
Summary by CodeRabbit