Skip to content

fix(coreaudio): honour the full timeout when waiting for a sample rate change - #1348

Open
dylanpulver wants to merge 1 commit into
RustAudio:masterfrom
dylanpulver:fix-coreaudio-rate-timeout
Open

fix(coreaudio): honour the full timeout when waiting for a sample rate change#1348
dylanpulver wants to merge 1 commit into
RustAudio:masterfrom
dylanpulver:fix-coreaudio-rate-timeout

Conversation

@dylanpulver

Copy link
Copy Markdown

set_sample_rate waits for RateListener to report the new rate, and re-arms when a notification carries some other rate. The re-arm subtracted start.elapsed() — the total elapsed time — from the already-reduced remaining, so the budget shrank super-linearly and hit zero well before the caller's timeout:

remaining = remaining.checked_sub(start.elapsed())   // total elapsed, every iteration

With a 500 ms timeout and a listener emitting a non-matching rate every few ms, the loop gave up after 82 ms and returned Sample rate update timed out on a device that was still settling. Perversely, the chattier the device, the shorter the effective timeout. Subtracting from the original timeout instead makes it a single deadline; the same measurement then runs the full 500 ms.

Non-matching notifications are reachable: RateListener's callback re-reads the property and sends whatever it gets, including 0.0 when that read fails — which matches no target rate.

The loop is extracted into wait_for_rate so it can be tested without a device. Two tests: one asserts the whole timeout is spent across repeated non-matching events (fails at 82 ms on the current code, and also on a saturating_sub variant of it, so it pins the accounting rather than the helper); one asserts it returns as soon as the target rate arrives. cargo test on macOS goes 18 → 20 passing, including the existing device tests. fmt, clippy --all --all-targets -- -D warnings and cargo doc are clean.

The other backends call recv_timeout(dur) once and never re-arm, so this is CoreAudio-only.

Honest limit: I could not reproduce the intermediate-rate notification on real hardware — the built-in mic and speakers here settle in a single notification — so that half is unverified. The evidence is the extracted unit test and the arithmetic.

…e change

The wait loop subtracted the total elapsed time from the already-reduced
remainder on every iteration, so the budget shrank super-linearly and the
device could be abandoned long before the caller's timeout expired.

@roderickvd roderickvd left a comment

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.

Thanks for your PR for what indeed is a real bug. I propose another solution though, consistent with what we do in ALSA.

Comment thread CHANGELOG.md
- **ASIO**: Fix loading a driver while a previous driver was still unloading.
- **AudioWorklet**: Fix processor construction failures not being reported to `error_callback`.
- **AudioWorklet**: Fix dropouts in output streams when the callback buffer grows.
- **CoreAudio**: A sample rate change no longer gives up early when the device reports other rates first; the caller's timeout is now honoured in full.

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.

Shorter & sweeter would be:

CoreAudio: Fix sample rate changes timing out early when the device reports other rates first.

/// 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.

let mut remaining = timeout;

loop {
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants