-
-
Notifications
You must be signed in to change notification settings - Fork 134
fix: Linux PipeWire engine two-level Frame enum + bounded frame channel (all platforms) #183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Tristan-Stoltz-ERC
wants to merge
9
commits into
CapSoftware:main
Choose a base branch
from
Luminous-Dynamics:fix/linux-engine-two-level-frame-enum
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+167
−53
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
42e0745
fix(linux): use two-level Frame::Video(VideoFrame::...) enum + System…
Tristan-Stoltz-ERC be78534
fix(linux): bound the internal frame channel to stop unbounded memory…
Tristan-Stoltz-ERC 9ed0211
fix: propagate SyncSender to win/mac + avoid blocking send on all eng…
Tristan-Stoltz-ERC 2ae046c
fix(linux): stop the capture loop when the frame receiver disconnects
Tristan-Stoltz-ERC 9b30e91
docs(linux): fix wording nit from @tembo review — retained, not leaked
Tristan-Stoltz-ERC 95e1e9e
refactor(linux): named channel-capacity const + rename exit flag
Tristan-Stoltz-ERC d118c15
Update src/capturer/engine/win/mod.rs
Tristan-Stoltz-ERC 32569f1
Update src/capturer/engine/win/mod.rs
Tristan-Stoltz-ERC 72c78cd
fix(mac): stop doing retain+send work after the frame receiver discon…
Tristan-Stoltz-ERC File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,7 @@ use std::{ | |||||||||||||||||||||||||||
| mpsc::{self, sync_channel, SyncSender}, | ||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||
| thread::JoinHandle, | ||||||||||||||||||||||||||||
| time::Duration, | ||||||||||||||||||||||||||||
| time::{Duration, SystemTime}, | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| use pipewire as pw; | ||||||||||||||||||||||||||||
|
|
@@ -31,7 +31,7 @@ use pw::{ | |||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| use crate::{ | ||||||||||||||||||||||||||||
| capturer::Options, | ||||||||||||||||||||||||||||
| frame::{BGRxFrame, Frame, RGBFrame, RGBxFrame, XBGRFrame}, | ||||||||||||||||||||||||||||
| frame::{BGRxFrame, Frame, RGBFrame, RGBxFrame, VideoFrame, XBGRFrame}, | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| use self::{error::LinCapError, portal::ScreenCastPortal}; | ||||||||||||||||||||||||||||
|
|
@@ -40,11 +40,15 @@ mod error; | |||||||||||||||||||||||||||
| mod portal; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| static CAPTURER_STATE: AtomicU8 = AtomicU8::new(0); | ||||||||||||||||||||||||||||
| static STREAM_STATE_CHANGED_TO_ERROR: AtomicBool = AtomicBool::new(false); | ||||||||||||||||||||||||||||
| // Signals pipewire_capturer's main loop to stop early. Set both on a | ||||||||||||||||||||||||||||
| // genuine PipeWire stream error and (see process_callback) when the frame | ||||||||||||||||||||||||||||
| // receiver has disconnected -- both are "the loop should end now" | ||||||||||||||||||||||||||||
| // conditions, so one flag models the intent accurately rather than two. | ||||||||||||||||||||||||||||
| static STREAM_SHOULD_EXIT: AtomicBool = AtomicBool::new(false); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| #[derive(Clone)] | ||||||||||||||||||||||||||||
| struct ListenerUserData { | ||||||||||||||||||||||||||||
| pub tx: mpsc::Sender<Frame>, | ||||||||||||||||||||||||||||
| pub tx: mpsc::SyncSender<Frame>, | ||||||||||||||||||||||||||||
| pub format: spa::param::video::VideoInfoRaw, | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
@@ -85,7 +89,7 @@ fn state_changed_callback( | |||||||||||||||||||||||||||
| match new { | ||||||||||||||||||||||||||||
| StreamState::Error(e) => { | ||||||||||||||||||||||||||||
| eprintln!("pipewire: State changed to error({e})"); | ||||||||||||||||||||||||||||
| STREAM_STATE_CHANGED_TO_ERROR.store(true, std::sync::atomic::Ordering::Relaxed); | ||||||||||||||||||||||||||||
| STREAM_SHOULD_EXIT.store(true, std::sync::atomic::Ordering::Relaxed); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| _ => {} | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
@@ -133,35 +137,80 @@ fn process_callback(stream: &StreamRef, user_data: &mut ListenerUserData) { | |||||||||||||||||||||||||||
| .to_vec() | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if let Err(e) = match user_data.format.format() { | ||||||||||||||||||||||||||||
| VideoFormat::RGBx => user_data.tx.send(Frame::RGBx(RGBxFrame { | ||||||||||||||||||||||||||||
| display_time: timestamp as u64, | ||||||||||||||||||||||||||||
| width: frame_size.width as i32, | ||||||||||||||||||||||||||||
| height: frame_size.height as i32, | ||||||||||||||||||||||||||||
| data: frame_data, | ||||||||||||||||||||||||||||
| })), | ||||||||||||||||||||||||||||
| VideoFormat::RGB => user_data.tx.send(Frame::RGB(RGBFrame { | ||||||||||||||||||||||||||||
| display_time: timestamp as u64, | ||||||||||||||||||||||||||||
| width: frame_size.width as i32, | ||||||||||||||||||||||||||||
| height: frame_size.height as i32, | ||||||||||||||||||||||||||||
| data: frame_data, | ||||||||||||||||||||||||||||
| })), | ||||||||||||||||||||||||||||
| VideoFormat::xBGR => user_data.tx.send(Frame::XBGR(XBGRFrame { | ||||||||||||||||||||||||||||
| display_time: timestamp as u64, | ||||||||||||||||||||||||||||
| width: frame_size.width as i32, | ||||||||||||||||||||||||||||
| height: frame_size.height as i32, | ||||||||||||||||||||||||||||
| data: frame_data, | ||||||||||||||||||||||||||||
| })), | ||||||||||||||||||||||||||||
| VideoFormat::BGRx => user_data.tx.send(Frame::BGRx(BGRxFrame { | ||||||||||||||||||||||||||||
| display_time: timestamp as u64, | ||||||||||||||||||||||||||||
| width: frame_size.width as i32, | ||||||||||||||||||||||||||||
| height: frame_size.height as i32, | ||||||||||||||||||||||||||||
| data: frame_data, | ||||||||||||||||||||||||||||
| })), | ||||||||||||||||||||||||||||
| // `timestamp` (from spa_meta_header.pts) is a PipeWire monotonic | ||||||||||||||||||||||||||||
| // nanosecond count since an arbitrary reference — not wall-clock. | ||||||||||||||||||||||||||||
| // display_time's SystemTime contract is wall-clock, so we use | ||||||||||||||||||||||||||||
| // SystemTime::now() here (matches what the macOS and Windows | ||||||||||||||||||||||||||||
| // engines do today). Relative frame ordering survives via | ||||||||||||||||||||||||||||
| // channel-send order; sub-millisecond buffer timing is lost. | ||||||||||||||||||||||||||||
| let _ = timestamp; // suppress "unused" warning until we wire pts elsewhere | ||||||||||||||||||||||||||||
| let display_time = SystemTime::now(); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // `try_send`, not `send`: this callback runs on the same thread | ||||||||||||||||||||||||||||
| // that `pipewire_capturer`'s loop uses to poll CAPTURER_STATE | ||||||||||||||||||||||||||||
| // between `pw_loop.iterate()` calls. A blocking `send()` on a | ||||||||||||||||||||||||||||
| // bounded channel would stall this thread whenever the consumer | ||||||||||||||||||||||||||||
| // falls behind, and since `LinuxCapturer::stop_capture()` joins | ||||||||||||||||||||||||||||
| // this exact thread, a full channel + a paused consumer would | ||||||||||||||||||||||||||||
| // make `stop_capture()` hang forever. Dropping the frame under | ||||||||||||||||||||||||||||
| // backpressure (rather than blocking the producer) keeps both | ||||||||||||||||||||||||||||
| // the bounded-memory guarantee and a responsive stop path. | ||||||||||||||||||||||||||||
| let send_result = match user_data.format.format() { | ||||||||||||||||||||||||||||
| VideoFormat::RGBx => { | ||||||||||||||||||||||||||||
| user_data | ||||||||||||||||||||||||||||
| .tx | ||||||||||||||||||||||||||||
| .try_send(Frame::Video(VideoFrame::RGBx(RGBxFrame { | ||||||||||||||||||||||||||||
| display_time, | ||||||||||||||||||||||||||||
| width: frame_size.width as i32, | ||||||||||||||||||||||||||||
| height: frame_size.height as i32, | ||||||||||||||||||||||||||||
| data: frame_data, | ||||||||||||||||||||||||||||
| }))) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| VideoFormat::RGB => { | ||||||||||||||||||||||||||||
| user_data | ||||||||||||||||||||||||||||
| .tx | ||||||||||||||||||||||||||||
| .try_send(Frame::Video(VideoFrame::RGB(RGBFrame { | ||||||||||||||||||||||||||||
| display_time, | ||||||||||||||||||||||||||||
| width: frame_size.width as i32, | ||||||||||||||||||||||||||||
| height: frame_size.height as i32, | ||||||||||||||||||||||||||||
| data: frame_data, | ||||||||||||||||||||||||||||
| }))) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| VideoFormat::xBGR => { | ||||||||||||||||||||||||||||
| user_data | ||||||||||||||||||||||||||||
| .tx | ||||||||||||||||||||||||||||
| .try_send(Frame::Video(VideoFrame::XBGR(XBGRFrame { | ||||||||||||||||||||||||||||
| display_time, | ||||||||||||||||||||||||||||
| width: frame_size.width as i32, | ||||||||||||||||||||||||||||
| height: frame_size.height as i32, | ||||||||||||||||||||||||||||
| data: frame_data, | ||||||||||||||||||||||||||||
| }))) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| VideoFormat::BGRx => { | ||||||||||||||||||||||||||||
| user_data | ||||||||||||||||||||||||||||
| .tx | ||||||||||||||||||||||||||||
| .try_send(Frame::Video(VideoFrame::BGRx(BGRxFrame { | ||||||||||||||||||||||||||||
| display_time, | ||||||||||||||||||||||||||||
| width: frame_size.width as i32, | ||||||||||||||||||||||||||||
| height: frame_size.height as i32, | ||||||||||||||||||||||||||||
| data: frame_data, | ||||||||||||||||||||||||||||
| }))) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| _ => panic!("Unsupported frame format received"), | ||||||||||||||||||||||||||||
| } { | ||||||||||||||||||||||||||||
| eprintln!("{e}"); | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if let Err(mpsc::TrySendError::Disconnected(_)) = send_result { | ||||||||||||||||||||||||||||
| // The consumer (Capturer/rx) is gone, e.g. dropped without | ||||||||||||||||||||||||||||
| // calling stop_capture(). Signal the main loop above to | ||||||||||||||||||||||||||||
| // exit instead of spinning pw_loop.iterate() forever on a | ||||||||||||||||||||||||||||
| // stream nobody will ever read from, and log it once (swap | ||||||||||||||||||||||||||||
| // instead of store) rather than once per incoming frame. | ||||||||||||||||||||||||||||
| if !STREAM_SHOULD_EXIT.swap(true, std::sync::atomic::Ordering::Relaxed) { | ||||||||||||||||||||||||||||
| eprintln!("Frame receiver disconnected"); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| // TrySendError::Full is a silent intentional drop under | ||||||||||||||||||||||||||||
| // backpressure — see comment above. | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||
| eprintln!("Out of buffers"); | ||||||||||||||||||||||||||||
|
|
@@ -173,7 +222,7 @@ fn process_callback(stream: &StreamRef, user_data: &mut ListenerUserData) { | |||||||||||||||||||||||||||
| // TODO: Format negotiation | ||||||||||||||||||||||||||||
| fn pipewire_capturer( | ||||||||||||||||||||||||||||
| options: Options, | ||||||||||||||||||||||||||||
| tx: mpsc::Sender<Frame>, | ||||||||||||||||||||||||||||
| tx: mpsc::SyncSender<Frame>, | ||||||||||||||||||||||||||||
| ready_sender: &SyncSender<bool>, | ||||||||||||||||||||||||||||
| stream_id: u32, | ||||||||||||||||||||||||||||
| ) -> Result<(), LinCapError> { | ||||||||||||||||||||||||||||
|
|
@@ -299,8 +348,8 @@ fn pipewire_capturer( | |||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // User has called Capturer::start() and we start the main loop | ||||||||||||||||||||||||||||
| while CAPTURER_STATE.load(std::sync::atomic::Ordering::Relaxed) == 1 | ||||||||||||||||||||||||||||
| && /* If the stream state got changed to `Error`, we exit. TODO: tell user that we exited */ | ||||||||||||||||||||||||||||
| !STREAM_STATE_CHANGED_TO_ERROR.load(std::sync::atomic::Ordering::Relaxed) | ||||||||||||||||||||||||||||
| && /* Exit early on a PipeWire stream error or a disconnected frame receiver. TODO: tell user that we exited */ | ||||||||||||||||||||||||||||
| !STREAM_SHOULD_EXIT.load(std::sync::atomic::Ordering::Relaxed) | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
|
Comment on lines
349
to
353
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One edge case with
Suggested change
|
||||||||||||||||||||||||||||
| pw_loop.iterate(Duration::from_millis(100)); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
@@ -317,7 +366,7 @@ pub struct LinuxCapturer { | |||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| impl LinuxCapturer { | ||||||||||||||||||||||||||||
| // TODO: Error handling | ||||||||||||||||||||||||||||
| pub fn new(options: &Options, tx: mpsc::Sender<Frame>) -> Self { | ||||||||||||||||||||||||||||
| pub fn new(options: &Options, tx: mpsc::SyncSender<Frame>) -> Self { | ||||||||||||||||||||||||||||
| let connection = | ||||||||||||||||||||||||||||
| dbus::blocking::Connection::new_session().expect("Failed to create dbus connection"); | ||||||||||||||||||||||||||||
| let stream_id = ScreenCastPortal::new(&connection) | ||||||||||||||||||||||||||||
|
|
@@ -360,10 +409,10 @@ impl LinuxCapturer { | |||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| CAPTURER_STATE.store(0, std::sync::atomic::Ordering::Relaxed); | ||||||||||||||||||||||||||||
| STREAM_STATE_CHANGED_TO_ERROR.store(false, std::sync::atomic::Ordering::Relaxed); | ||||||||||||||||||||||||||||
| STREAM_SHOULD_EXIT.store(false, std::sync::atomic::Ordering::Relaxed); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| pub fn create_capturer(options: &Options, tx: mpsc::Sender<Frame>) -> LinuxCapturer { | ||||||||||||||||||||||||||||
| pub fn create_capturer(options: &Options, tx: mpsc::SyncSender<Frame>) -> LinuxCapturer { | ||||||||||||||||||||||||||||
| LinuxCapturer::new(options, tx) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the receiver is disconnected, this will
eprintln!once per frame until the capture thread stops. Since you already haveSTREAM_STATE_CHANGED_TO_ERROR, you can set it here (to exit the loop) and useswapto log only once.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch — applied exactly as suggested in
2ae046c. ReusedSTREAM_STATE_CHANGED_TO_ERRORso a disconnected receiver now ends the capture loop instead of spinning forever, and swapped to log the disconnect once.