-
Notifications
You must be signed in to change notification settings - Fork 502
fix(pulseaudio): release server-side stream on Stream::drop #1189
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ use std::{ | |
| }; | ||
|
|
||
| use futures::executor::block_on; | ||
| use futures::FutureExt as _; | ||
| use pulseaudio::{protocol, AsPlaybackSource}; | ||
|
|
||
| use crate::{ | ||
|
|
@@ -51,21 +52,46 @@ enum StreamInner { | |
| Record(pulseaudio::RecordStream, Instant, LatencyHandle), | ||
| } | ||
|
|
||
| pub struct Stream(StreamInner); | ||
| pub struct Stream { | ||
| inner: StreamInner, | ||
| workers: Vec<std::thread::JoinHandle<()>>, | ||
|
Member
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. If this is always two workers then |
||
| } | ||
|
|
||
| impl Drop for Stream { | ||
| fn drop(&mut self) { | ||
| match &mut self.0 { | ||
| StreamInner::Playback(_, _, handle) | StreamInner::Record(_, _, handle) => { | ||
| handle.cancel() | ||
| match &mut self.inner { | ||
| StreamInner::Playback(stream, _, handle) => { | ||
| handle.cancel(); | ||
| // Tear down the play_all driver thread spawned in | ||
|
Member
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. This reads as quite verbose. Consider trimming it to a line or two. |
||
| // new_playback. It is parked in source_eof(), which only | ||
| // resolves when the source returns 0 from poll_read — but | ||
| // the data-callback wrapper installed there always reports | ||
| // a full buffer, so without an external nudge the thread | ||
| // never exits and keeps the PlaybackStream Arc alive, | ||
| // leaking the server-side stream. Queueing a delete causes | ||
| // the reactor to drop the source's eof_tx, waking play_all | ||
| // with cancellation. now_or_never matches the existing | ||
| // pattern in pulseaudio::PlaybackStream's own Drop. | ||
| let _ = stream.clone().delete().now_or_never(); | ||
| } | ||
| StreamInner::Record(_, _, handle) => { | ||
| handle.cancel(); | ||
| } | ||
| } | ||
| for handle in self.workers.drain(..) { | ||
| // Prevent self-join: a worker thread may surface an error | ||
| // through the user's error_callback, and that callback may | ||
| // drop the Stream — in which case we'd be joining ourselves. | ||
| if handle.thread().id() != std::thread::current().id() { | ||
| let _ = handle.join(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl StreamTrait for Stream { | ||
| fn play(&self) -> Result<(), Error> { | ||
| match &self.0 { | ||
| match &self.inner { | ||
| StreamInner::Playback(stream, _, handle) => { | ||
| block_on(stream.uncork()).map_err(Error::from)?; | ||
| handle.notify(); | ||
|
|
@@ -80,12 +106,12 @@ impl StreamTrait for Stream { | |
| } | ||
|
|
||
| fn pause(&self) -> Result<(), Error> { | ||
| let res = match &self.0 { | ||
| let res = match &self.inner { | ||
| StreamInner::Playback(stream, _, _) => block_on(stream.cork()), | ||
| StreamInner::Record(stream, _, _) => block_on(stream.cork()), | ||
| }; | ||
| res.map_err(Error::from)?; | ||
| match &self.0 { | ||
| match &self.inner { | ||
| StreamInner::Playback(_, _, handle) | StreamInner::Record(_, _, handle) => { | ||
| handle.notify() | ||
| } | ||
|
|
@@ -94,15 +120,15 @@ impl StreamTrait for Stream { | |
| } | ||
|
|
||
| fn now(&self) -> StreamInstant { | ||
| let start = match &self.0 { | ||
| let start = match &self.inner { | ||
| StreamInner::Playback(_, start, _) | StreamInner::Record(_, start, _) => *start, | ||
| }; | ||
| let elapsed = start.elapsed(); | ||
| StreamInstant::new(elapsed.as_secs(), elapsed.subsec_nanos()) | ||
| } | ||
|
|
||
| fn buffer_size(&self) -> Result<FrameCount, Error> { | ||
| let (spec, bytes) = match &self.0 { | ||
| let (spec, bytes) = match &self.inner { | ||
| StreamInner::Playback(s, _, _) => ( | ||
| s.sample_spec(), | ||
| s.buffer_attr().minimum_request_length as usize, | ||
|
|
@@ -219,9 +245,18 @@ impl Stream { | |
| // when the stream is stopped by the user. | ||
| let stream_clone = stream.clone(); | ||
| let error_callback_clone = error_callback.clone(); | ||
| std::thread::spawn(move || { | ||
| let cancel_driver = handle.cancel.clone(); | ||
| let driver_handle = std::thread::spawn(move || { | ||
| if let Err(e) = block_on(stream_clone.play_all()) { | ||
| emit_error(&error_callback_clone, Error::from(e)); | ||
| // Stream::drop sets the cancel flag and then queues | ||
|
Member
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. Seems like it could be more to the point, too. |
||
| // DeletePlaybackStream, which makes the reactor drop the | ||
| // source's eof_tx and surfaces here as ClientError::Disconnected. | ||
| // That is teardown noise, not a real error — suppress | ||
| // emit_error in the cancel case. The latency thread already | ||
| // does the equivalent check before each timing_info poll. | ||
| if !cancel_driver.load(atomic::Ordering::Relaxed) { | ||
| emit_error(&error_callback_clone, Error::from(e)); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
|
|
@@ -231,7 +266,7 @@ impl Stream { | |
| let stream_clone = stream.clone(); | ||
| let latency_clone = current_latency_micros.clone(); | ||
| let poll_clone = last_poll_micros.clone(); | ||
| std::thread::spawn(move || loop { | ||
| let latency_handle = std::thread::spawn(move || loop { | ||
| if cancel_thread.load(atomic::Ordering::Relaxed) { | ||
| break; | ||
| } | ||
|
|
@@ -265,7 +300,10 @@ impl Stream { | |
| *guard = false; | ||
| }); | ||
|
|
||
| Ok(Self(StreamInner::Playback(stream, start, handle))) | ||
| Ok(Self { | ||
| inner: StreamInner::Playback(stream, start, handle), | ||
| workers: vec![driver_handle, latency_handle], | ||
| }) | ||
| } | ||
|
|
||
| pub fn new_record<D, E>( | ||
|
|
@@ -352,7 +390,7 @@ impl Stream { | |
| let stream_clone = stream.clone(); | ||
| let latency_clone = current_latency_micros.clone(); | ||
| let poll_clone = last_poll_micros.clone(); | ||
| std::thread::spawn(move || loop { | ||
| let latency_handle = std::thread::spawn(move || loop { | ||
| if cancel_thread.load(atomic::Ordering::Relaxed) { | ||
| break; | ||
| } | ||
|
|
@@ -386,7 +424,10 @@ impl Stream { | |
| *guard = false; | ||
| }); | ||
|
|
||
| Ok(Self(StreamInner::Record(stream, start, handle))) | ||
| Ok(Self { | ||
| inner: StreamInner::Record(stream, start, handle), | ||
| workers: vec![latency_handle], | ||
| }) | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
Much too long. Please ensure consistency with the existing entries.