ALSA: fix buffer size / period size configuration #917
Conversation
abique
force-pushed
the
alsa-period-cfg
branch
from
9879b3c to
62324d5
Compare
With ALSA, the buffer size is `nperiod * period_size`. We want 2 periods ideally. The default is now set to 1024 samples which is a reasonable default, yet still too large for real-time audio. The default value now uses the same code path as custom values. Fixes RustAudio#913
abique
force-pushed
the
alsa-period-cfg
branch
from
62324d5 to
8d86b11
Compare
|
It is now a month that this PR is opened. |
|
@est31 Hi, any chance this PR gets merged or closed with a comment? |
|
Hey, I tested this PR and I don't think it works as expected because the PR is not explicitly setting the buffer size at all and I received unexpectedly huge buffers (~1MB) whereas with fixed buffer setting I'm expecting to get a buffer of that exact size. Full disclosure: we are using alsa's pipewire device and we were struggling with getting buffers of the right size. Because of the former |
|
Buffer size isn't relevant. |
Can you elaborate? Are you perhaps suggesting that the buffer contains a period size worth of actual samples and the rest is just junk that can be ignored? We use a webrtc processor for AEC and it seems to require fixed sized buffers. We tried doing buffering ourselves on top of non-fixed buffers given to us by cpal but that introduced timing jitter that prevented AEC to work correctly. With portaudio we get fixed size buffers and AEC works correctly. To be sure, I'm a beginner when it comes to audio, so it's very much possible I misunderstand some core concepts but it sure does appear that buffer sizes are relevant at least in some cases. |
|
An alsa buffer, is a continuous memory used as a circular buffer. What is important is the period size (in frames) that defines your latency. So there's nothing wrong with a bigger buffer, it just takes more memory. For alsa, period size and num periods defines your latency and working context. |
|
Thank you @abique, that makes a lot of sense! It would be nice to also improve the |
|
@abique doing backlog grooming as new maintainer. Yes the current calculation is off and your proposal is better. I'm just thinking that the period size could be even more optimally calculated by taking into account the sampling rate. At high sampling rates, 1024 frames is short and increases risk of stutter, correct? I also agree with @mbernat that the documentation can use improvement, particularly because buffer size and period size do not mean the same thing in Alsa land. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors ALSA audio buffer configuration to properly handle buffer size and period size calculations. The changes ensure that both fixed and default buffer sizes follow the same code path and implement the ALSA requirement that buffer size equals nperiod * period_size with an ideal of 2 periods.
- Unifies buffer size handling for both fixed and default configurations
- Sets a more reasonable default period size of 1024 samples
- Explicitly configures 2 periods for optimal ALSA performance
| BufferSize::Fixed(v) => v as alsa::pcm::Frames, | ||
| BufferSize::Default => { | ||
| // These values together represent a moderate latency and wakeup interval. | ||
| // Without them, we are at the mercy of the device | ||
| hw_params.set_period_time_near(25_000, alsa::ValueOr::Nearest)?; | ||
| hw_params.set_buffer_time_near(100_000, alsa::ValueOr::Nearest)?; | ||
| // This value represent a moderate latency and wakeup interval. |
There was a problem hiding this comment.
The Fixed buffer size case is treating the buffer size as period size directly, but according to the PR description and ALSA documentation, buffer size should be nperiod * period_size. For 2 periods, the period size should be v / 2, not v.
Copilot uses AI. Check for mistakes.
| hw_params.set_period_time_near(25_000, alsa::ValueOr::Nearest)?; | ||
| hw_params.set_buffer_time_near(100_000, alsa::ValueOr::Nearest)?; | ||
| // This value represent a moderate latency and wakeup interval. | ||
| 1024 as alsa::pcm::Frames |
There was a problem hiding this comment.
[nitpick] The explicit cast as alsa::pcm::Frames is unnecessary here since 1024 can be directly assigned to the alsa::pcm::Frames type. Consider using just 1024.
| 1024 as alsa::pcm::Frames | |
| 1024 |
Copilot uses AI. Check for mistakes.
|
Hey, I'm not feeling like continuing this PR, so feel free to take the changes and finish it. |
|
Understood. Apologies for the late response and thank you for your earlier efforts 🙏 |
|
Superseded by #990 |
This mostly reverts #990 for device compatibility, by letting ALSA calculate the period size from the device default period count. Forcing the period count to 2 caused underruns on some systems.
With ALSA, the buffer size is
nperiod * period_size. We want 2 periods ideally.The default is now set to 1024 samples which is a reasonable default, yet still too large for real-time audio.
Also the default value uses the same code path as custom values.
Fixes #913
Depends on #915 for successful compilation.
Replaces #914