Repository navigation
fix(recording): resolve PipeWire system audio routing timeout on Linux (#3514) - #3534
devpryan7792 wants to merge 2 commits into
Conversation
CapSoftware#3514) Signed-off-by: Pradyumn jha <[email protected]>
|
@devpryan7792 is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let Ok(outputs) = current_process_source_outputs() { | ||
| if outputs | ||
| .iter() | ||
| .any(|output| output.id == source_output && output.source == source_index) | ||
| { |
There was a problem hiding this comment.
The new nested if checks violate the repository’s instruction to combine conditions and its denied collapsible_if lint. Linux Clippy checks will reject this code. Combine the conditions with let chains here, in the failed-command fallback, and in the polling loop.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/recording/src/sources/screen_capture/linux.rs
Line: 2256-2260
Comment:
**Nested conditions fail Clippy**
The new nested `if` checks violate the repository’s instruction to combine conditions and its denied `collapsible_if` lint. Linux Clippy checks will reject this code. Combine the conditions with let chains here, in the failed-command fallback, and in the polling loop.
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| )? | ||
| } | ||
| }; | ||
| // Fast path: if the stream is already routed to the requested source, return immediately. |
There was a problem hiding this comment.
The new comments at lines 2255 and 2299 only narrate the code. AGENTS.md explicitly bans comments that restate what the code does. Remove these two comments before merging to meet that requirement. Keep the PipeWire timing explanation, which gives useful platform context.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/recording/src/sources/screen_capture/linux.rs
Line: 2255
Comment:
**Comments repeat the code**
The new comments at lines 2255 and 2299 only narrate the code. `AGENTS.md` explicitly bans comments that restate what the code does. Remove these two comments before merging to meet that requirement. Keep the PipeWire timing explanation, which gives useful platform context.
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| assert!(outputs.iter().any(|o| o.id == 1128 && o.source == 47)); | ||
| assert!(!outputs.iter().any(|o| o.id == 1128 && o.source == 61)); | ||
| assert!(!outputs.iter().any(|o| o.id == 1130 && o.source == 47)); |
There was a problem hiding this comment.
already_matching_source_output_is_identified tests a copied expression rather than production code. It still passes if the new early return is deleted or its condition is broken. Test the routing function with an injected command runner so the test proves that an already-matching stream returns successfully without running move-source-output.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/recording/src/sources/screen_capture/linux.rs
Line: 3451-3453
Comment:
**Test misses the early return**
`already_matching_source_output_is_identified` tests a copied expression rather than production code. It still passes if the new early return is deleted or its condition is broken. Test the routing function with an injected command runner so the test proves that an already-matching stream returns successfully without running `move-source-output`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.- Use is_ok_and to eliminate nested conditions and comply with collapsible_if clippy rules - Remove narrative comments per AGENTS.md style guide while preserving PipeWire timing documentation - Update already_matching_source_output_is_identified test to directly verify source_output_route_matches Signed-off-by: Pradyumn jha <[email protected]>
Problem
On Linux installations running PipeWire / WirePlumber (such as Linux Mint, Arch, Ubuntu 24.04), system audio capture can fail during recording startup with:
Two distinct race conditions caused this failure:
move_pactl_source_outputunconditionally issuedpactl move-source-outputwithout verifying whether the route was already satisfied.source_output_route_matchesreturned an error that prematurely aborted the retry loop.Fix
move_pactl_source_outputnow checks if the stream output is already mapped to the desiredsource_index. If already matched, it returnsOk(())immediately without issuing redundant move commands or causing WirePlumber state churn.pactl move-source-outputreturns non-zero, it re-verifies if the stream is already routed to the destination before failing.Testing
already_matching_source_output_is_identifiedincrates/recording/src/sources/screen_capture/linux.rsverifying that matching source outputs are detected accurately.routing_acknowledgement_requires_the_requested_sourceand pactl helper unit tests.cargo fmt --check.Closes #3514
Fix the denied nested conditions and the comment-policy violations before merging.
Findings
Fix with agent prompt
Summary
This PR avoids redundant Linux audio moves and gives PipeWire more time to confirm a route.
Reviews (1) · Last reviewed commit: "fix(recording): resolve PipeWire system ..." · Reviewed by Greptile