cpal icon indicating copy to clipboard operation
cpal copied to clipboard

Add support for OpenBSD platform via sndio host

Open AaronM04 opened this issue 4 years ago • 23 comments

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. :-)

AaronM04 avatar Oct 30 '20 04:10 AaronM04

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:

  1. Having one or more other OpenBSD users review and test the examples. I don't know of any other OpenBSD users personally, but if any are reading this it would help a lot if you could test and review!
  2. Adding a job to the Github CI workflow for OpenBSD + sndio. I'm not sure off the top of my head if it's possible to spin up a OpenBSD instance as easily as it is say Ubuntu. If it turns out not to be feasible with the github workflow, perhaps you have another recommendation for OpenBSD-specific CI? That said, I'd prefer we get this working with the github workflow in order to keep managing CI as simple as possible for maintainers.

mitchmindtree avatar Oct 30 '20 12:10 mitchmindtree

  1. Adding a job to the Github CI workflow for OpenBSD + sndio. I'm not sure off the top of my head if it's possible to spin up a OpenBSD instance as easily as it is say Ubuntu. If it turns out not to be feasible with the github workflow, perhaps you have another recommendation for OpenBSD-specific CI? That said, I'd prefer we get this working with the github workflow in order to keep managing CI as simple as possible for maintainers.

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. https://github.com/google/syzkaller/blob/master/tools/create-openbsd-gce-ci.sh

blackgnezdo avatar Nov 02 '20 06:11 blackgnezdo

  1. Adding a job to the Github CI workflow for OpenBSD + sndio. I'm not sure off the top of my head if it's possible to spin up a OpenBSD instance as easily as it is say Ubuntu. If it turns out not to be feasible with the github workflow, perhaps you have another recommendation for OpenBSD-specific CI? That said, I'd prefer we get this working with the github workflow in order to keep managing CI as simple as possible for maintainers.

FWIW, the sndio library is available on Linux, so you don't need OpenBSD to run CI for it: https://sndio.org/

grayed avatar Nov 03 '20 06:11 grayed

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 sndio-sys as a dependency on linux, but only conditionally. Currently the dependency is conditional on cfg(target_os = "openbsd"). I am not very familiar with this part of Cargo/Rust so I would be very grateful for any assistance with this.

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)

AaronM04 avatar Nov 06 '20 04:11 AaronM04

The one potential complication is getting sndio-sys as a dependency on linux, but only conditionally.

Perhaps we can add a linux-sndio cargo feature, and then conditionally include the sndio host with a cfg that looks something like:

#[cfg(or(target_os = "openbsd", and(target_os = "linux", feature = "linux-sndio")))]

@RustAudio/cpal-maintainers does this sound OK?

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)

Yeah that's totally fine, though perhaps rather than continuously adding new commits you could use git commit --amend on the last commit that adds the workflow and then force push it on each attempt? This is what I normally do when attempting to add a new workflow at least, not sure if there's a nicer way!

mitchmindtree avatar Nov 06 '20 13:11 mitchmindtree

@blackgnezdo I finally was able to return to this. Please review when you have time. Thanks in advance! :smile:

BTW I don't know what CI complaints you are referring to.

AaronM04 avatar Dec 23 '20 05:12 AaronM04

Somehow I missed the notification for this. :/

@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?

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:

doas pkg_add llvm

I added this to https://github.com/mjkillough/sioctl-rs#installation

AaronM04 avatar Jan 20 '21 04:01 AaronM04

The iOS build is failing but I suspect this is not due to any of my commits.

AaronM04 avatar Jan 27 '21 05:01 AaronM04

Somehow I missed the notification for this. :/ ...

Any clues?

Did it work back in November but not anymore? If so, I have no idea what the issue is.

No, I had never tried to build your code before the first time I complained.

If this is the first time you're trying to build it, run:

doas pkg_add llvm

I added this to https://github.com/mjkillough/sioctl-rs#installation

I do have llvm package:

% pkg_info llvm
Information for inst:llvm-10.0.1p8
...

The library is available on the system:

% pkg_info -L llvm | grep 'libclang.*so'
...
/usr/local/lib/libclang-cpp.so.0.0
/usr/local/lib/libclang.so.8.1

Yet cargo build reports (for d419f0fa5cdbaf47ec2e127d9fbe86a471627bf5):

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

Now, I bothered to read the error message and the resolution of my problem is:

% LIBCLANG_PATH=/usr/local/lib/libclang.so.8.1 cargo build
   Compiling sndio-sys v0.0.1
   Compiling cpal v0.12.1 (/home/greg/s/cpal)
    Finished dev [unoptimized + debuginfo] target(s) in 7.29s

I wonder why you don't have to set LIBCLANG_PATH.

blackgnezdo avatar Jan 27 '21 06:01 blackgnezdo

I wonder why you don't have to set LIBCLANG_PATH.

It turns out that I added export LIBCLANG_PATH=/usr/local/lib to my profile and forgot about it...

AaronM04 avatar Jan 27 '21 15:01 AaronM04

I proposed some code shrinking changes a minor refactoring. If this works, I can massage it a bit more.

blackgnezdo avatar Jan 30 '21 05:01 blackgnezdo

@blackgnezdo thanks! It is quite an improvement. I merged your commits into this branch.

AaronM04 avatar Feb 01 '21 20:02 AaronM04

Hi everyone!

Over at https://github.com/jpochyla/psst/pull/191 psst switched to cpal and I'm working on OpenBSD support. I'm using your feature branch to be able to play music at all, but unfortunately playback speed is noticably slow. (before cpal, miniaudio was used and psst played music just fine on OpenBSD.)

My audio/rust/cpal/etc. foo is weak, hence I've only been testing cpal with psst. Can you recommend another (possibly simpler) way of testing cpal's sndio support in this branch? I'd like to help bring this forward in whatever way I can.

I've also rebased your branch onto latest cpal master and retested psst, but with no avail: playback speed is still too slow.

klemensn avatar Nov 18 '21 19:11 klemensn

Hey @klemensn ! :wave:

Can you define "slow"? Which of these is it?

  • Stretched out/longer notes but the same pitch. A 4 minute song takes >4 minutes to play.
  • Lower pitch. A 4 minute song takes >4 minutes to play. If so, it could be that psst is expecting one sample rate but cpal is using another one.
  • Stuttering/choppy playback.

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?

AaronM04 avatar Nov 19 '21 04:11 AaronM04

psst reports 31 seconds of elapsed playback time while 60 seconds have passed on the wall clock.

No other issues like stuttering or so, just playback speed.

Here are the outputs of cpal's examples built with cargo build --examples. beep produces a since, but I don't know which frequency (the source code did not immediately give away this information).

$ ./target/release/examples/feedback
Using input device: "sndio default device"
Using output device: "sndio default device"
Attempting to build both streams with f32 samples and `StreamConfig { channels: 1, sample_rate: SampleRate(48000), buffer_size: Default }`.
Successfully built streams.
Starting the input and output streams with `150` milliseconds of latency.
Playing for 3 seconds... 
input stream fell behind: try increasing latency
input stream fell behind: try increasing latency
input stream fell behind: try increasing latency
input stream fell behind: try increasing latency
input stream fell behind: try increasing latency
input stream fell behind: try increasing latency
input stream fell behind: try increasing latency
input stream fell behind: try increasing latency
input stream fell behind: try increasing latency
input stream fell behind: try increasing latency
input stream fell behind: try increasing latency
input stream fell behind: try increasing latency
input stream fell behind: try increasing latency
input stream fell behind: try increasing latency
Done!
$ ./target/release/examples/enumerate
Supported hosts:
  [Sndio]
Available hosts:
  [Sndio]
sndio
  Default Input Device:
    Some("sndio default device")
  Default Output Device:
    Some("sndio default device")
  Devices: 
  1. "sndio default device"
    Default input stream config:
      SupportedStreamConfig { channels: 1, sample_rate: SampleRate(48000), buffer_size: Range { min: 480, max: 4800 }, sample_format: I16 }
    All supported input stream configs:
      1.1. SupportedStreamConfigRange { channels: 1, min_sample_rate: SampleRate(48000), max_sample_rate: SampleRate(48000), buffer_size: Range { min: 480, max: 4800 }, sample_format: I16 }
      1.2. SupportedStreamConfigRange { channels: 1, min_sample_rate: SampleRate(8000), max_sample_rate: SampleRate(8000), buffer_size: Range { min: 80, max: 800 }, sample_format: I16 }
      1.3. SupportedStreamConfigRange { channels: 1, min_sample_rate: SampleRate(44100), max_sample_rate: SampleRate(44100), buffer_size: Range { min: 441, max: 4410 }, sample_format: I16 }
    Default output stream config:
      SupportedStreamConfig { channels: 1, sample_rate: SampleRate(48000), buffer_size: Range { min: 480, max: 4800 }, sample_format: I16 }
    All supported output stream configs:
      1.1. SupportedStreamConfigRange { channels: 1, min_sample_rate: SampleRate(48000), max_sample_rate: SampleRate(48000), buffer_size: Range { min: 480, max: 4800 }, sample_format: I16 }
      1.2. SupportedStreamConfigRange { channels: 1, min_sample_rate: SampleRate(8000), max_sample_rate: SampleRate(8000), buffer_size: Range { min: 80, max: 800 }, sample_format: I16 }
      1.3. SupportedStreamConfigRange { channels: 1, min_sample_rate: SampleRate(44100), max_sample_rate: SampleRate(44100), buffer_size: Range { min: 441, max: 4410 }, sample_format: I16 }
$ ./target/release/examples/beep
Output device: sndio default device
Default output config: SupportedStreamConfig { channels: 1, sample_rate: SampleRate(48000), buffer_size: Range { min: 480, max: 4800 }, sample_format: I16 }

klemensn avatar Nov 23 '21 22:11 klemensn

FWIW, recording through cpal works correctly on OpenBSD. aucat(1) plays the file in the correct speed, without problems.

$ ./target/release/examples/record_wav
Input device: sndio default device
Default input config: SupportedStreamConfig { channels: 1, sample_rate: SampleRate(48000), buffer_size: Range { min: 480, max: 4800 }, sample_format: I16 }
Begin recording...
Recording /home/kn/src/cpal/recorded.wav complete!
$ aucat -i /home/kn/src/cpal/recorded.wav

klemensn avatar Nov 24 '21 11:11 klemensn

@klemensn the beep example is a 440 Hz sine wave.

I see the same output as you for the feedback and enumerate examples (including the input stream fell behind: try increasing latency lines).

Try talking during the feedback example. Does your echoed voice sound normal?

I'll try it out with psst if I get some more time today.

AaronM04 avatar Nov 24 '21 17:11 AaronM04

@klemensn I tried compiling psst after making it use the cpal from this branch, and I got this error from the souvlaki crate:

error[E0308]: mismatched types
  --> /home/aaron/.cargo/registry/src/github.com-1ecc6299db9ec823/souvlaki-0.4.0/src/platform/empty/mod.rs:13:9
   |
12 |     pub fn new(_config: PlatformConfig) -> Result<Self, Error> {
   |                                            ------------------- expected `Result<platform::platform::MediaControls, platform::platform::Error>` because of return type
13 |         Self
   |         ^^^^
   |         |
   |         expected enum `Result`, found struct `platform::platform::MediaControls`
   |         help: try using a variant of the expected enum: `Ok(Self)`
   |
   = note: expected enum `Result<platform::platform::MediaControls, platform::platform::Error>`
            found struct `platform::platform::MediaControls`

For more information about this error, try `rustc --explain E0308`.
error: could not compile `souvlaki` due to previous error
warning: build failed, waiting for other jobs to finish...
error: build failed

Did you encounter this error? If so, how did you fix it? Thanks.

Also, did you do anything nontrivial when updating this branch to cpal 0.13?

AaronM04 avatar Nov 24 '21 19:11 AaronM04

Use my openbsd branch from https://github.com/jpochyla/psst/pull/191 to build on OpenBSD, it contains all quirks, incl. an update to souvlaki where I fixed the error you ran into.

Speaking during the feedback example plays back as expected, i.e. without any slow down, pitch or anything else.

Thanks for looking into this!

klemensn avatar Nov 24 '21 22:11 klemensn

@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:

[2021-11-29T21:14:24Z ERROR psst_core::audio::output] failed to stop stream: A backend-specific error has occurred: pausing is not implemented

This suggests two things need to be fixed in this branch before psst will be working with cpal on OpenBSD:

  • Fix the sample-rate or stereo-mono confusion. An example of the latter would be psst requesting stereo playback but cpal using mono -- this would cause everything to be one octave lower (half the frequency).

  • ~~Implement pause/resume. Essentially the sndio system needs to be closed and re-created.~~ EDIT: playback continues when I unpause, inexplicably! Maybe we don't need to fix this

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.

AaronM04 avatar Nov 29 '21 21:11 AaronM04

@AaronM04 Thanks a bunch!

@jpochyla just committed further changes, specifically https://github.com/jpochyla/psst/commit/a77465313e9d23dd76d71811d2721c83cca805db https://github.com/jpochyla/psst/commit/df89890dd89283a50113cd8acaa319e64d8d2fac which I rebased onto.

Something's still off as you can see in https://github.com/jpochyla/psst/pull/191/commits/a4faacb6b111e3b9211ee4248b4210d29d359088 but playback speed is normal now and I can properly listen to songs!

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. Pausing does work, though, as does resuming playback.

klemensn avatar Nov 30 '21 00:11 klemensn

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

midnadimple-zz avatar Apr 22 '22 05:04 midnadimple-zz

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:

  1. Resolve the conflict (yes, there is one).
  2. Get another code review, and address any review feedback.
  3. Make sure tests pass
  4. Merge it

AaronM04 avatar Apr 22 '22 05:04 AaronM04