-
Notifications
You must be signed in to change notification settings - Fork 361
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.
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
Add support for OpenBSD platform via sndio host #493
base: master
Are you sure you want to change the base?
Conversation
Thanks for the epic PR @AaronM04! I don't have time to dig into this myself at the moment, not to mention my lack of experience with sndio & OpenBSD, however two steps that would help to land this are:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Responding to review request since I happen to care about OpenBSD and Rust :)
The change is fairly massive so I didn't make it all the way to the end. Depending on your motivation and the prevailing conventions of RustAudio, some comments may be too nit-picky.
src/host/sndio/mod.rs
Outdated
/// Buffer overrun/underrun behavior -- ignore/sync/error? | ||
behavior: BufferXrunBehavior, | ||
|
||
/// If a buffer size was chosen, contains that value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The abundance of Optional suggests a huge state space. I suspect it can be reduced by refactoring InnerState
type into an enum with a pruned set of fields that apply to specific stages of of the state machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blackgnezdo I have fixed this. Please take a look when you get a chance. Thanks!
src/host/sndio/mod.rs
Outdated
let hdl = unsafe { | ||
// The transmute is needed because this C string is *const u8 in one place but *const i8 in another place. | ||
let devany_ptr = mem::transmute::<_, *const i8>(sndio_sys::SIO_DEVANY as *const _); | ||
let nonblocking = true as i32; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might as we be written as 1 with an increase in readability, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. I tend to favor boolean values as Rust bool
s, though the cast may be too confusing here.
src/host/sndio/mod.rs
Outdated
) | ||
}; | ||
if hdl.is_null() { | ||
return Err(SndioError::DeviceNotAvailable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do better and somehow report the errno? Probably applies everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try that here but when I checked errno
for an error from sio_setpar
it didn't set it.
I don't know what it would take to get an OpenBSD CI on github, but as a point of reference, here's what it takes to get a GCE image of OpenBSD. |
FWIW, the sndio library is available on Linux, so you don't need OpenBSD to run CI for it: https://sndio.org/ |
For CI, I think it will be more practical to use sndio on Linux (as @grayed mentioned) than to setup an OpenBSD VM as part of the github workflow. The one potential complication is getting As for testing the workflow, should I just push to this branch? (it could be a lot of commits if it's trial and error that way) |
Perhaps we can add a #[cfg(or(target_os = "openbsd", and(target_os = "linux", feature = "linux-sndio")))] @RustAudio/cpal-maintainers does this sound OK?
Yeah that's totally fine, though perhaps rather than continuously adding new commits you could use |
d426f69
to
eeafe6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noted a few more things, but the whole change is too big to read in one sitting :(
Also, there's a bunch of CI generated complaints that probably need to be addressed.
src/host/sndio/mod.rs
Outdated
|
||
fn open(&mut self) -> Result<(), SndioError> { | ||
match self { | ||
InnerState::Opened { .. } | InnerState::Running { .. } => Ok(()), // Already opened |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it a good idea to permit a duplicate open? I'd complain and return an SndioError.
src/host/sndio/mod.rs
Outdated
// Transition to running | ||
par.appbufsz = buffer_size as u32; | ||
hdl.set_params(&par)?; | ||
let mut tmp: HashMap<u32, sndio_sys::sio_par> = HashMap::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does turbo-fish not work here? It should be shorter.
src/host/sndio/mod.rs
Outdated
hdl: SioHdl, | ||
|
||
/// Map of sample rate to parameters. | ||
sample_rate_to_par: HashMap<u32, sndio_sys::sio_par>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HashMap<u32, sndio_sys::sio_par>
type seems pervasive. It probably deserves a well-named alias or even a newtype. I'm also partial to creating aliases for numeric types such that a sample rate and every other kind of u32 aren't easy to mix-up. Between these two, I'd probably have here:
rate_parameters: RateParameterMap,
unsafe impl Send for SioHdl {} | ||
|
||
/// The shared state between Device and Stream. Responsible for closing handle when dropped. | ||
enum InnerState { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/host/sndio/mod.rs
Outdated
|
||
unsafe impl Send for SioHdl {} | ||
|
||
/// The shared state between Device and Stream. Responsible for closing handle when dropped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should also list the permitted transitions unless they are fully controlled by the type impl?
} | ||
} | ||
|
||
unsafe impl Send for SioHdl {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably deserves a comment explaining unsafe
: why it has to be unsafe, also why and under which assumptions it's OK.
|
||
inner_state.setup_stream(config)?; | ||
|
||
let idx; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combine declaration with assignment.
src/host/sndio/mod.rs
Outdated
} | ||
|
||
pub fn set_xrun_behavior(&mut self, b: BufferXrunBehavior) -> Result<(), SndioError> { | ||
let mut inner_state = self.inner_state.lock().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there's a way to return an error, why unwrap rather than complain? Seems to apply in all the cases below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it this way because only panics on the previous thread to have locked the mutex can cause an error to be returned from lock. I suppose it still makes sense to return a "thread panicked" error here.
} | ||
|
||
/// Create an output stream. | ||
fn build_output_stream_raw<D, E>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like 50 lines of code largely similar to build_input_stream_raw
, any chance you can figure out the common parts and de-dup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent a lot of time trying this out and unfortunately I don't think this can be de-duped without adding another layer of Box
around the callbacks, which would impact performance (albeit not by much).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(well, there are macros but that feels almost like obfuscating the code to me)
src/host/sndio/mod.rs
Outdated
|
||
fn supported_config_from_par(par: &sndio_sys::sio_par, num_channels: u32) -> SupportedStreamConfig { | ||
SupportedStreamConfig { | ||
channels: num_channels as u16, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be a true comment to add? Otherwise it's not clear why u32 suddenly becomes u16.
// Conversion is courtesy of type mismatch between sndio and RustAudio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, added verbatim!
* Add SampleRateMap with SampleRate keys rather than raw HashMap with u32 keys. * Use FrameCount type alias rather than usize for buffer size. * More robust error handling. * Document InnerState state transitions.
@blackgnezdo I finally was able to return to this. Please review when you have time. Thanks in advance! 😄 BTW I don't know what CI complaints you are referring to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AaronM04, I wanted to play with the code but cargo build
fails for me on openbsd-amd64-current:
Compiling sndio-sys v0.0.1
error: failed to run custom build command for `sndio-sys v0.0.1`
Caused by:
process didn't exit successfully: `/home/greg/s/cpal/target/debug/build/sndio-sys-c905c01512cca331/build-script-build` (exit code: 101)
--- stdout
cargo:rustc-link-lib=sndio
cargo:rerun-if-changed=wrapper.h
--- stderr
thread 'main' panicked at 'Unable to find libclang: "couldn\'t find any valid shared libraries matching: [\'libclang.so\', \'libclang.so.*\'], set the `LIBCLANG_PATH` environment variable to a path where one of these files can be found (invalid: [])"', /home/greg/.cargo/registry/src/github.com-1ecc6299db9ec823/bindgen-0.53.3/src/lib.rs:1956:31
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Any clues?
Somehow I missed the notification for this. :/
Did it work back in November but not anymore? If so, I have no idea what the issue is. If this is the first time you're trying to build it, run:
I added this to https://github.com/mjkillough/sioctl-rs#installation |
The iOS build is failing but I suspect this is not due to any of my commits. |
ae8d7b8
to
b344503
Compare
b344503
to
2e00be6
Compare
No, I had never tried to build your code before the first time I complained.
I do have llvm package:
The library is available on the system:
Yet
Now, I bothered to read the error message and the resolution of my problem is:
I wonder why you don't have to set |
It turns out that I added |
I proposed some code shrinking changes a minor refactoring. If this works, I can massage it a bit more. |
Avoids duplicating information with: * TypeSampleFormat to avoid redundant values * Sample trait for value conversion * Common data_from_vec fn Indexing typically means a runtime check whereas iterators don't.
@blackgnezdo thanks! It is quite an improvement. I merged your commits into this branch. |
Done with editing Cargo.toml followed by `cargo update -p cpal`, which, sadly, change a whole bunch of other dependencies. This makes audio play, but two issues remain: 1. Playback speed is noticably slow. 2. Toggling play -> shows this error, although playback is successfully paused: ``` [2021-11-17T16:03:04Z ERROR psst_core::audio::output] failed to stop stream: A backend-specific error has occurred: pausing is not implemented ``` "Add support for OpenBSD platform via sndio host" RustAudio/cpal#493 is based on an older cpal release; maybe rebasing on top of HEAD and therefore bringing in the cpal changes that current psst already uses helps with/fixes any of the two issues?
Done with editing Cargo.toml followed by `cargo update -p cpal`, which, sadly, change a whole bunch of other dependencies. This makes audio play, but two issues remain: 1. Playback speed is noticably slow. 2. Toggling play -> shows this error, although playback is successfully paused: ``` [2021-11-17T16:03:04Z ERROR psst_core::audio::output] failed to stop stream: A backend-specific error has occurred: pausing is not implemented ``` "Add support for OpenBSD platform via sndio host" RustAudio/cpal#493 is based on an older cpal release; maybe rebasing on top of HEAD and therefore bringing in the cpal changes that current psst already uses helps with/fixes any of the two issues?
Hi everyone! Over at jpochyla/psst#191 psst switched to cpal and I'm working on OpenBSD support. My audio/rust/cpal/etc. foo is weak, hence I've only been testing cpal with psst. I've also rebased your branch onto latest cpal master and retested psst, but with no avail: playback speed is still too slow. |
Hey @klemensn ! 👋 Can you define "slow"? Which of these is it?
Lastly, is this an OpenBSD-specific problem with psst+cpal? I suspect not, but I just wanted to be sure. EDIT: have you tried the example cpal programs on OpenBSD? What happens with those? |
No other issues like stuttering or so, just playback speed. Here are the outputs of
|
FWIW, recording through
|
@klemensn the I see the same output as you for the Try talking during the I'll try it out with |
@klemensn I tried compiling
Did you encounter this error? If so, how did you fix it? Thanks. Also, did you do anything nontrivial when updating this branch to |
Use my Speaking during the Thanks for looking into this! |
@klemensn I was able to reproduce the issue with your psst branch (I did make one change to a Cargo.toml to use my local cpal repository though). I played a song I am familiar with and the voices sounded very low pitch (stretched out). I think it is either a sample-rate confusion or a stereo-mono confusion. I also noticed an error when hitting pause:
This suggests two things need to be fixed in this branch before psst will be working with cpal on OpenBSD:
Unfortunately, my time is over-committed so I can't take the lead in fixing these. However, I can definitely help another Rust developer understand the code that I wrote. I can also suggest where to do debugging or add print statements to better understand this. |
Done with editing Cargo.toml followed by `cargo update -p cpal`, which, sadly, change a whole bunch of other dependencies. This makes audio play, but two issues remain: 1. Playback speed is noticably slow. 2. Toggling play -> shows this error, although playback is successfully paused: ``` [2021-11-17T16:03:04Z ERROR psst_core::audio::output] failed to stop stream: A backend-specific error has occurred: pausing is not implemented ``` "Add support for OpenBSD platform via sndio host" RustAudio/cpal#493 is based on an older cpal release; maybe rebasing on top of HEAD and therefore bringing in the cpal changes that current psst already uses helps with/fixes any of the two issues?
@AaronM04 Thanks a bunch! @jpochyla just committed further changes, specifically Something's still off as you can see in So it seems as if @jpochyla followed your suggestion to fix "stereo-mono confusion". The pause issue you mentioned is known (to me); I have not done anything yet to fix it. |
Done with editing Cargo.toml followed by `cargo update -p cpal`, which, sadly, change a whole bunch of other dependencies. This makes audio play, but two issues remain: 1. Playback speed is noticably slow. 2. Toggling play -> shows this error, although playback is successfully paused: ``` [2021-11-17T16:03:04Z ERROR psst_core::audio::output] failed to stop stream: A backend-specific error has occurred: pausing is not implemented ``` "Add support for OpenBSD platform via sndio host" RustAudio/cpal#493 is based on an older cpal release; maybe rebasing on top of HEAD and therefore bringing in the cpal changes that current psst already uses helps with/fixes any of the two issues?
Done with editing Cargo.toml followed by `cargo update -p cpal`, which, sadly, change a whole bunch of other dependencies. This makes audio play, but two issues remain: 1. Playback speed is noticably slow. 2. Toggling play -> shows this error, although playback is successfully paused: ``` [2021-11-17T16:03:04Z ERROR psst_core::audio::output] failed to stop stream: A backend-specific error has occurred: pausing is not implemented ``` "Add support for OpenBSD platform via sndio host" RustAudio/cpal#493 is based on an older cpal release; maybe rebasing on top of HEAD and therefore bringing in the cpal changes that current psst already uses helps with/fixes any of the two issues?
Done with editing Cargo.toml followed by `cargo update -p cpal`, which, sadly, change a whole bunch of other dependencies. This makes audio play, but two issues remain: 1. Playback speed is noticably slow. 2. Toggling play -> shows this error, although playback is successfully paused: ``` [2021-11-17T16:03:04Z ERROR psst_core::audio::output] failed to stop stream: A backend-specific error has occurred: pausing is not implemented ``` "Add support for OpenBSD platform via sndio host" RustAudio/cpal#493 is based on an older cpal release; maybe rebasing on top of HEAD and therefore bringing in the cpal changes that current psst already uses helps with/fixes any of the two issues?
Done with editing Cargo.toml followed by `cargo update -p cpal`, which, sadly, change a whole bunch of other dependencies. This makes audio play, but two issues remain: 1. Playback speed is noticably slow. 2. Toggling play -> shows this error, although playback is successfully paused: ``` [2021-11-17T16:03:04Z ERROR psst_core::audio::output] failed to stop stream: A backend-specific error has occurred: pausing is not implemented ``` "Add support for OpenBSD platform via sndio host" RustAudio/cpal#493 is based on an older cpal release; maybe rebasing on top of HEAD and therefore bringing in the cpal changes that current psst already uses helps with/fixes any of the two issues?
Done with editing Cargo.toml followed by `cargo update -p cpal`, which, sadly, change a whole bunch of other dependencies. This makes audio play, but two issues remain: 1. Playback speed is noticably slow. 2. Toggling play -> shows this error, although playback is successfully paused: ``` [2021-11-17T16:03:04Z ERROR psst_core::audio::output] failed to stop stream: A backend-specific error has occurred: pausing is not implemented ``` "Add support for OpenBSD platform via sndio host" RustAudio/cpal#493 is based on an older cpal release; maybe rebasing on top of HEAD and therefore bringing in the cpal changes that current psst already uses helps with/fixes any of the two issues?
just wanting to ping this request. i've tested it on my own repo. it fully merges into master and the examples run well. this should be noticed |
Hi @midnadimple . Thanks for the ping. I think what this needs is another developer familiar with Rust, because I don't have much time to spend on this unfortunately. There's only a bit of work left:
|
This adds support for the sndio host which is the default and only supported host on OpenBSD. I have tested all the examples for cpal and rodio, with no issues observed. If there are any improvements needed to bring this PR up to the standards of the cpal project, please let me know. :-)