Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- **AudioWorklet**: Fix dropouts in output streams when the callback buffer grows.
- **CoreAudio**: Fix the device running at a different sample rate from the stream on hardware that reports a continuous rate range.
- **CoreAudio**: Fix `supported_configs()` only reporting `F32`, even on hardware that also supports other sample formats.
- **CoreAudio**: Fix sample rate changes timing out early when the device reports other rates first.
- **JACK**: Channel enumeration is capped at the physical system port count again.
- **JACK**: Streams no longer panic when the server delivers a larger period than the negotiated buffer size.
- **PipeWire**: Fix an empty chunk being emitted when a cycle requests no frames.
Expand Down
119 changes: 91 additions & 28 deletions src/host/coreaudio/macos/device.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use std::{
sync::{
Arc, Mutex,
atomic::{AtomicBool, AtomicUsize, Ordering},
mpsc::{RecvTimeoutError, channel},
mpsc::{Receiver, RecvTimeoutError, channel},
},
time::{Duration, Instant},
};
Expand Down Expand Up @@ -191,37 +191,57 @@ fn set_sample_rate(
// Wait for the reported_rate to change.
//
// This should not take longer than a few ms. Use the caller's timeout if provided,
// otherwise default to 1 second. We loop over potentially several events from the
// channel to ensure that we catch the expected change in sample rate.
let mut remaining = timeout.unwrap_or(Duration::from_secs(1));
let start = Instant::now();
loop {
match receiver.recv_timeout(remaining) {
Ok(reported_rate) => {
if (reported_rate - target_sample_rate as f64).abs() < 1.0 {
break;
}
}
Err(RecvTimeoutError::Timeout) => {
return Err(Error::with_message(
ErrorKind::DeviceNotAvailable,
"Sample rate update timed out",
));
}
Err(RecvTimeoutError::Disconnected) => {
return Err(Error::with_message(
ErrorKind::StreamInvalidated,
"Sample rate listener disconnected unexpectedly",
));
// otherwise default to 1 second.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass on the Option; the contract is that a timeout of None is to wait indefinitely.

wait_for_rate(
&receiver,
target_sample_rate,
timeout.unwrap_or(Duration::from_secs(1)),
)?;
// listener dropped here; its Drop impl calls unregister() automatically.
}
Ok(())
}

/// Block until the rate listener reports `target_sample_rate`, giving up after `timeout`.
///
/// Notifications carrying some other rate can arrive first, so `timeout` bounds the whole wait
/// rather than each individual receive.
fn wait_for_rate(
receiver: &Receiver<f64>,
target_sample_rate: SampleRate,
timeout: Duration,
) -> Result<(), Error> {
let deadline = Instant::now() + timeout;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This panics on overflow. Consider:

fn wait_for_rate(
    receiver: &Receiver<f64>,
    target_sample_rate: SampleRate,
    timeout: Option<Duration>,
) -> Result<(), Error> {
    let deadline = timeout.and_then(|t| Instant::now().checked_add(t));

    loop {
        let received = match deadline {
            Some(deadline) => {
                let remaining = deadline.saturating_duration_since(Instant::now());
                if remaining.is_zero() {
                    return Err(Error::with_message(
                        ErrorKind::DeviceNotAvailable,
                        "Sample rate update timed out",
                    ));
                }
                receiver.recv_timeout(remaining)
            }
            None => receiver.recv().map_err(|_| RecvTimeoutError::Disconnected),
        };

        match received {
            // ...
        }
    }
}


loop {
let remaining = deadline.saturating_duration_since(Instant::now());
if remaining.is_zero() {
return Err(Error::with_message(
ErrorKind::DeviceNotAvailable,
"Sample rate update timed out",
));
}

match receiver.recv_timeout(remaining) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still manually implements the deadline by subtracting every loop iteration. In ALSA we've got something prettier by computing one deadline up front and checking saturating_duration_since each iteration:

fn wait_for_rate(
    receiver: &Receiver<f64>,
    target_sample_rate: SampleRate,
    timeout: Duration,
) -> Result<(), Error> {
    let deadline = Instant::now() + timeout;

    loop {
        let remaining = deadline.saturating_duration_since(Instant::now());
        if remaining.is_zero() {
            return Err(Error::with_message(
                ErrorKind::DeviceNotAvailable,
                "Sample rate update timed out",
            ));
        }

        match receiver.recv_timeout(remaining) {
            Ok(reported_rate) => {
                if (reported_rate - target_sample_rate as f64).abs() < 1.0 {
                    return Ok(());
                }
            }
            Err(RecvTimeoutError::Timeout) => {
                return Err(Error::with_message(
                    ErrorKind::DeviceNotAvailable,
                    "Sample rate update timed out",
                ));
            }
            Err(RecvTimeoutError::Disconnected) => {
                return Err(Error::with_message(
                    ErrorKind::StreamInvalidated,
                    "Sample rate listener disconnected unexpectedly",
                ));
            }
        }
    }
}

As bonus, this also gets rid of a blocking recv_timeout(ZERO) edge case.

Ok(reported_rate) => {
if (reported_rate - target_sample_rate as f64).abs() < 1.0 {
return Ok(());
}
}
remaining = remaining
.checked_sub(start.elapsed())
.unwrap_or(Duration::ZERO);
Err(RecvTimeoutError::Timeout) => {
return Err(Error::with_message(
ErrorKind::DeviceNotAvailable,
"Sample rate update timed out",
));
}
Err(RecvTimeoutError::Disconnected) => {
return Err(Error::with_message(
ErrorKind::StreamInvalidated,
"Sample rate listener disconnected unexpectedly",
));
}
}
// listener dropped here; its Drop impl calls unregister() automatically.
}
Ok(())
}

#[derive(Clone, Copy)]
Expand Down Expand Up @@ -1202,3 +1222,46 @@ pub(crate) fn get_device_buffer_frame_size(
)?;
Ok(frames as usize)
}

#[cfg(test)]
mod tests {
use std::sync::mpsc::channel;
use std::time::{Duration, Instant};

use super::wait_for_rate;

/// A listener can report rates other than the target before it reports the new one, e.g. a
/// device stepping through rates. The whole timeout must remain available across those.
#[test]
fn wait_for_rate_honours_the_full_timeout_across_repeated_events() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't wait 500 ms in every test run.

const TIMEOUT: Duration = Duration::from_millis(50);

let (sender, receiver) = channel::<f64>();
let feeder = std::thread::spawn(move || {
while sender.send(44_100.0).is_ok() {
std::thread::sleep(Duration::from_millis(1));
}
});

let start = Instant::now();
assert!(wait_for_rate(&receiver, 48_000, TIMEOUT).is_err());
let elapsed = start.elapsed();

drop(receiver);
let _ = feeder.join();

assert!(
elapsed >= TIMEOUT - Duration::from_millis(10),
"gave up after {elapsed:?}, well before the {TIMEOUT:?} timeout"
);
}

#[test]
fn wait_for_rate_returns_when_the_target_rate_is_reported() {
let (sender, receiver) = channel::<f64>();
sender.send(44_100.0).unwrap();
sender.send(48_000.0).unwrap();

assert!(wait_for_rate(&receiver, 48_000, Duration::from_secs(5)).is_ok());
}
}