Skip to content

[ALSA] Improve stream setup parameters. #582

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.

Sign up for GitHub

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

Merged
est31 merged 1 commit into from
Jun 17, 2021
Merged

[ALSA] Improve stream setup parameters. #582

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
66 changes: 35 additions & 31 deletions src/host/alsa/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,11 +251,8 @@ impl Device {
Err((e, _)) => return Err(e.into()),
Ok(handle) => handle,
};
let can_pause = {
let hw_params = set_hw_params_from_format(&handle, conf, sample_format)?;
hw_params.can_pause()
};
let (_buffer_len, period_len) = set_sw_params_from_format(&handle, conf)?;
let can_pause = set_hw_params_from_format(&handle, conf, sample_format)?;
let period_len = set_sw_params_from_format(&handle, conf, stream_type)?;

handle.prepare()?;

Expand All @@ -277,7 +274,9 @@ impl Device {
_ => None,
};

handle.start()?;
if let alsa::Direction::Capture = stream_type {
handle.start()?;
}

let stream_inner = StreamInner {
channel: handle,
Expand Down Expand Up @@ -420,10 +419,10 @@ impl Device {
supported_formats.len() * supported_channels.len() * sample_rates.len(),
);
for &sample_format in supported_formats.iter() {
for channels in supported_channels.iter() {
for &channels in supported_channels.iter() {
for &(min_rate, max_rate) in sample_rates.iter() {
output.push(SupportedStreamConfigRange {
channels: *channels,
channels,
min_sample_rate: SampleRate(min_rate as u32),
max_sample_rate: SampleRate(max_rate as u32),
buffer_size: buffer_size_range.clone(),
Expand Down Expand Up @@ -856,14 +855,13 @@ fn stream_timestamp(
None => {
let trigger_ts = status.get_trigger_htstamp();
let ts = status.get_htstamp();
let nanos = timespec_diff_nanos(ts, trigger_ts)
.try_into()
.unwrap_or_else(|_| {
panic!(
"get_htstamp `{:?}` was earlier than get_trigger_htstamp `{:?}`",
ts, trigger_ts
);
});
let nanos = timespec_diff_nanos(ts, trigger_ts);
if nanos < 0 {
panic!(
"get_htstamp `{:?}` was earlier than get_trigger_htstamp `{:?}`",
ts, trigger_ts
);
}
Ok(crate::StreamInstant::from_nanos(nanos))
}
Some(creation) => {
Expand Down Expand Up @@ -966,11 +964,11 @@ impl StreamTrait for Stream {
}
}

fn set_hw_params_from_format<'a>(
pcm_handle: &'a alsa::pcm::PCM,
fn set_hw_params_from_format(
pcm_handle: &alsa::pcm::PCM,
config: &StreamConfig,
sample_format: SampleFormat,
) -> Result<alsa::pcm::HwParams<'a>, BackendSpecificError> {
) -> Result<bool, BackendSpecificError> {
let hw_params = alsa::pcm::HwParams::any(pcm_handle)?;
hw_params.set_access(alsa::pcm::Access::RWInterleaved)?;

Expand All @@ -993,7 +991,10 @@ fn set_hw_params_from_format<'a>(
hw_params.set_channels(config.channels as u32)?;

match config.buffer_size {
BufferSize::Fixed(v) => hw_params.set_buffer_size(v as alsa::pcm::Frames)?,
BufferSize::Fixed(v) => {
hw_params.set_period_size_near((v / 4) as alsa::pcm::Frames, alsa::ValueOr::Nearest)?;
hw_params.set_buffer_size(v as alsa::pcm::Frames)?;
}
Comment on lines -996 to +997
Copy link

@LukasKalbertodt LukasKalbertodt Sep 4, 2022

Choose a reason for hiding this comment

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

Summary: I think this causes a regression when using odd buffer sizes. It's not a big deal for me because I can just choose a more reasonable size, but I thought you might wanna know.

I just updated cpal in a project of mine from 0.13.3 to 0.13.4 and this caused a regression. With 0.13.3 everything worked, but with 0.13.4 I get this error:

A backend-specific error has occurred: ALSA function 'snd_pcm_hw_params_set_buffer_size' failed with error 'EINVAL: Invalid argument'
  • Ubuntu 20.04
  • SupportedStreamConfig::buffer_size is Range { min: 3, max: 4194304 }
  • StreamConfig { channels: 2, sample_rate: SampleRate(44100), buffer_size: Fixed(735) }
  • f32 samples

The problem here is the weird buffer size. Changing that to 1024 (for example) fixes the error and everything works again.

I'm commenting on this line change because I think this caused the regression (seems to be the only ALSA related change between the two versions).

Unfortunately, I don't have the time to provide a minimal example. In case you want to try with the project I mentioned:

Details

this project at 61990b4ccab688a62172ef7836ea15b60f1cc942. The commit works, but try updating cpal. To actually run this, you have to run testroms/download.sh and then cargo run -- testroms/blargg/dmg_sound/dmg_sound.gb.

Copy link
Member Author

@est31 est31 Sep 4, 2022

Choose a reason for hiding this comment

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

Hmm I'm trying to think of examples where you need odd buffer sizes. Maybe it would be good to forbid their use? Maybe it should be documented then.

Copy link

@LukasKalbertodt LukasKalbertodt Sep 5, 2022

Choose a reason for hiding this comment

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

Sounds like a solution to me. I also cannot come up with a use case for that -- even numbers should suffice. (Tho I'm not really experienced with audio...). But yeah, that would certainly be better than a non-descriptive "backend-specific error".

BufferSize::Default => {
// These values together represent a moderate latency and wakeup interval.
// Without them we are at the mercy of the device
Expand All @@ -1004,37 +1005,40 @@ fn set_hw_params_from_format<'a>(

pcm_handle.hw_params(&hw_params)?;

Ok(hw_params)
Ok(hw_params.can_pause())
}

fn set_sw_params_from_format(
pcm_handle: &alsa::pcm::PCM,
config: &StreamConfig,
) -> Result<(usize, usize), BackendSpecificError> {
stream_type: alsa::Direction,
) -> Result<usize, BackendSpecificError> {
let sw_params = pcm_handle.sw_params_current()?;
sw_params.set_start_threshold(0)?;

let (buffer_len, period_len) = {
let period_len = {
let (buffer, period) = pcm_handle.get_params()?;
if buffer == 0 {
return Err(BackendSpecificError {
description: "initialization resulted in a null buffer".to_string(),
});
}
sw_params.set_avail_min(period as alsa::pcm::Frames)?;
let buffer = buffer as usize * config.channels as usize;
let period = period as usize * config.channels as usize;
(buffer, period)

let start_threshold = match stream_type {
alsa::Direction::Playback => buffer - period,
alsa::Direction::Capture => 1,
};
sw_params.set_start_threshold(start_threshold.try_into().unwrap())?;

period as usize * config.channels as usize
};

sw_params.set_tstamp_mode(true)?;
// TODO:
// Pending new version of alsa-sys and alsa-rs getting published.
// sw_params.set_tstamp_type(alsa::pcm::TstampType::MonotonicRaw)?;
sw_params.set_tstamp_type(alsa::pcm::TstampType::MonotonicRaw)?;

pcm_handle.sw_params(&sw_params)?;

Ok((buffer_len, period_len))
Ok(period_len)
}

impl From<alsa::Error> for BackendSpecificError {
Expand Down