Skip to content

[Alsa Host] Set appropriate start threshold depending on the direction #433

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

Closed
sniperrifle2004 wants to merge 2 commits into from
Closed

[Alsa Host] Set appropriate start threshold depending on the direction #433

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
656b568
Set appropriate start threshold depending on the direction
sniperrifle2004 Jun 19, 2020
3d1ecbe
Fix capture not starting, but constantly polling
sniperrifle2004 Jun 24, 2020
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
19 changes: 16 additions & 3 deletions src/host/alsa/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ impl Device {
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 (_buffer_len, period_len) = set_sw_params_from_format(&handle, conf, stream_type)?;

handle.prepare()?;

Expand All @@ -211,7 +211,12 @@ impl Device {
_ => None,
};

handle.start()?;
// For capture streams, starts can't be delayed until the
// read calls since the input_stream_worker won't call those
// if the pcm is not started
if stream_type == alsa::Direction::Capture {
handle.start()?
}
Copy link
Contributor

@alexmoon alexmoon Nov 18, 2020

Choose a reason for hiding this comment

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

If you set the start_threshold for capture streams to 1, I think you would not have to call start() here at all.

Copy link
Member

@mitchmindtree mitchmindtree Dec 6, 2020

Choose a reason for hiding this comment

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

@alexmoon any chance you might be interested in getting this work over the line in a new PR? My understanding on this is a bit vague, but it sounds like this might help with some of the initial underruns that can occur when kicking off an ALSA stream?

Copy link
Contributor

@alexmoon alexmoon Dec 7, 2020

Choose a reason for hiding this comment

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

@mitchmindtree yes, I can work on that. I'd probably like to combine this change with #431 since they're related and wait until #506 is merged so we don't have too many outstanding PRs touching the same files.

Copy link
Author

@sniperrifle2004 sniperrifle2004 Dec 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@alexmoon You may be right. I vaguely recall trying 1 and it not working. I then hypothesized that it was due to readi not getting called at all if the pcm is not running and the start_threshold for recording relying on that to switch the pcm to running. But that memory is shaky at best and I am not an expert.

Copy link
Author

@sniperrifle2004 sniperrifle2004 Dec 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@alexmoon Well it seems like I actually have some evidence to back up that memory in commit 3d1ecbe . The log message also goes into the technical details of the hypothesis. Note also that I thought it worked in the earlier commit since I had not actually removed the handle.start()? yet.

Copy link
Contributor

@alexmoon alexmoon Dec 10, 2020

Choose a reason for hiding this comment

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

Yes, that seems to be correct. In my other implementation I was using readi() and a start_threshold of 1 worked correctly.


let stream_inner = StreamInner {
channel: handle,
Expand Down Expand Up @@ -905,9 +910,9 @@ fn set_hw_params_from_format<'a>(
fn set_sw_params_from_format(
pcm_handle: &alsa::pcm::PCM,
config: &StreamConfig,
stream_type: alsa::Direction,
) -> Result<(usize, usize), BackendSpecificError> {
let sw_params = pcm_handle.sw_params_current()?;
sw_params.set_start_threshold(0)?;

let (buffer_len, period_len) = {
let (buffer, period) = pcm_handle.get_params()?;
Expand All @@ -916,6 +921,14 @@ fn set_sw_params_from_format(
description: "initialization resulted in a null buffer".to_string(),
});
}
match stream_type {
alsa::Direction::Playback => {
// Don't start playback until the buffer is filled
sw_params.set_start_threshold((buffer - period) as alsa::pcm::Frames)?
}
// For capture streams rely on an explicit start
alsa::Direction::Capture => sw_params.set_start_threshold(i32::MAX.into())?,
}
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;
Expand Down