Switch from vendored alsa-sys to alsa crate #386

alexmoon · 2020-04-06T18:59:26Z

This is another attempt to switch to the alsa crate fixing #346 (see also #308). I've attempted to change as little code as possible and keep consistency with the calls to the underlying library.

This change removes a lot of unsafe from cpal (though it still exists in the alsa crate itself) and enables RAII for the ALSA resources. It also fixes #377, allowing the use of the alsa crate and cpal in the same program.

est31 · 2020-04-06T19:01:29Z

Related PR by @mitchmindtree : #375

mitchmindtree

Thanks @alexmoon ! I agree it makes sense to share this work between the two projects, rather than have CPAL roll its own high-level solution.

One concern I have is the CPAL/RustAudio maintainers having access to the alsa crate in the case that we need to make changes or fixes. @diwic, is providing access to RustAudio maintainers something you might be open to if cpal were to switch over to alsa as in this PR? We would still of course await your review on PRs etc, but this would at least give us a path forward in the case that you become unexpectedly unavailable or busy for example.

Alternatively, would you be interested in moving the project under the RustAudio umbrella and continuing to lead the project as a RustAudio admin? @est31 @piedoom would this be OK with you?

Also @diwic I noticed you brought up alsa integration in the past at #346 , thanks for that! And sorry you haven't had a proper response until now!

@alexmoon also, it's probably best not to remove the alsa-sys crate as I believe alsa depends on this crate :)

I'll revisit the work in #375 once we've made a decision on this PR. If we do decide to go with alsa , I'll update #375 to only update alsa-sys itself and not touch any cpal code.

mitchmindtree · 2020-04-10T19:26:43Z

Cargo.toml

-alsa-sys = { version = "0.1", path = "alsa-sys" }
-libc = "0.2"
+alsa = "0.4.1"
+nix = "0.15.0"


Do we need nix here? It looks like it's only used to check some errno's - could we get away with using libc for this?

nix wraps the errno's in an enum, so I think it makes more sense to match against that enum than to cast the value to an i32 and then compare to libc's constant. The alsa crate already pulls in nix as a dependency so it doesn't increase the overall dependency count to include it here. It would be more convenient if the alsa crate would re-export nix's Error and Errno types to avoid any issues with version conflicts.

est31 · 2020-04-10T19:59:58Z

Alternatively, would you be interested in moving the project under the RustAudio umbrella and continuing to lead the project as a RustAudio admin? @est31 @piedoom would this be OK with you?

Always happy about addition of new projects! 👍

alexmoon · 2020-04-10T20:00:53Z

Thanks @alexmoon ! I agree it makes sense to share this work between the two projects, rather than have CPAL roll its own high-level solution.

Thanks!

@alexmoon also, it's probably best not to remove the alsa-sys crate as I believe alsa depends on this crate :)

I believe the alsa-sys that alsa depends on is https://crates.io/crates/alsa-sys which points to https://github.com/diwic/alsa-sys, not here. Having two different versions of alsa-sys floating around was one of my motivations for this PR.

mitchmindtree · 2020-04-10T20:14:43Z

I believe the alsa-sys that alsa depends on is https://crates.io/crates/alsa-sys which points to https://github.com/diwic/alsa-sys, not here.

Oh wow I wasn't aware of this second repository. Even more surprising, both repos describe the same package and their authors publish access to the same crates.io instance. Would definitely be good to resolve this, I would have been quite surprised to run into this when attempting to publish #375!

diwic · 2020-04-11T18:55:00Z

Hi!

So let's start with alsa-sys:

Oh wow I wasn't aware of this second repository. Even more surprising, both repos describe the same package and their authors publish access to the same crates.io instance.

I asked for co-maintainership in #222 - and since, I've been maintaining alsa-sys by myself, so if any of you would do an upload of that, it would be very surprising to me!

Alternatively, would you be interested in moving the project under the RustAudio umbrella and continuing to lead the project as a RustAudio admin?

So I'm definitely interested in collaborating more with you, but I need to know a little bit more of what I'm getting myself into :-)

mitchmindtree · 2020-04-12T14:31:01Z

I asked for co-maintainership in #222 - and since, I've been maintaining alsa-sys by myself, so if any of you would do an upload of that, it would be very surprising to me!

Ahhh thanks for sharing, this does ring a bell! I guess we just forgot to eventually move the crate itself out! I would be more than happy to add you as a maintainer to CPAL by the way too! I think it would be nice for you to have the freedom to downstream your alsa and alsa-sys patches and updates. I'll send you an invite now in case you are still interested!

but I need to know a little bit more of what I'm getting myself into :-)

Of course! Perhaps unsurprisingly, RustAudio is a community of devs interested in audio development in Rust :) The intention is to attempt to share the burden and reduce the bus-factor for a variety of crates across the rust audio ecosystem by working together. Another side-goal is to improve the visibility of crates, proviidng a sort of one-stop-shop where folks interested in Rust audio dev can find a suite of useful tools in one place under the RustAudio org. If you have any other Qs feel free to ask (you can also find my email on my github user if you prefer).

All that said, I'm equally happy for the crates to remain under your personal repo and would totally understand if you'd prefer it that way! My main concern was whether or not it might be tricky to upstream patches etc when necessary, but it's clear to me that you're welcoming of collaboration having requested to do so here in the past 😅

I'll test and have a closer look at this PR tonight and then maybe we can land it and move forward if no-one else has any concerns?

diwic · 2020-04-12T14:56:45Z

@mitchmindtree

This all sounds like good goals to me!

I hope I'm not hijacking the PR by asking: Are there any prerequisites to moving crates to under the RustAudio umbrella? E g, must have clippy, rustfmt, free of rustc warnings, CI setup, a specific code of conduct, etc etc.. or is the individual maintainer free to choose?

mitchmindtree · 2020-04-12T15:39:25Z

Are there any prerequisites to moving crates to under the RustAudio umbrella? E g, must have clippy, rustfmt, free of rustc warnings, CI setup, a specific code of conduct, etc etc.. or is the individual maintainer free to choose?

The organisation itself is quite ad-hoc and we've no official standards of this kind at the org level, currently it's very much up to the individual maintainer to take responsibility for these things for each repository.

mitchmindtree · 2020-04-12T16:32:20Z

@alexmoon are you able to confirm each of the examples work for you locally? If so, would you mind sharing your current linux setup?

When attempting to run the beep.rs and record_wav.rs examples I'm getting a panic! related to a file descriptor in a bad state. If I unwrap the result returned by stream.play() on line 43 of the beep.rs example, I get the following panic immediately after running:

     Running `/home/mindtree/programming/rust/cpal/target/debug/examples/beep`
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: BackendSpecific { err: BackendSpecificError { description: "ALSA function \'snd_pcm_pause\' failed with error \'EBADFD: File descriptor in bad state\'" } }', examples/beep.rs:43:5
stack backtrace:                                                                                                                              
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.40/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print_fmt
             at src/libstd/sys_common/backtrace.rs:77                                                                                         
   3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
             at src/libstd/sys_common/backtrace.rs:59
   4: core::fmt::write            
             at src/libcore/fmt/mod.rs:1052
   5: std::io::Write::write_fmt
             at src/libstd/io/mod.rs:1426    
   6: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:62
   7: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:49
   8: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:204
   9: std::panicking::default_hook
             at src/libstd/panicking.rs:224                                                                                                   
  10: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:472
  11: rust_begin_unwind
             at src/libstd/panicking.rs:380                                                                                                   
  12: core::panicking::panic_fmt
             at src/libcore/panicking.rs:85
  13: core::option::expect_none_failed
             at src/libcore/option.rs:1199
  14: core::result::Result<T,E>::unwrap
             at /rustc/c2d141df59703393c0c683abc259f9a8c3be041a/src/libcore/result.rs:963
  15: beep::run
             at examples/beep.rs:43
  16: beep::main
             at examples/beep.rs:14
  17: std::rt::lang_start::{{closure}}
             at /rustc/c2d141df59703393c0c683abc259f9a8c3be041a/src/libstd/rt.rs:67
  18: std::rt::lang_start_internal::{{closure}}
             at src/libstd/rt.rs:52
  19: std::panicking::try::do_call
             at src/libstd/panicking.rs:305
  20: __rust_maybe_catch_panic
             at src/libpanic_unwind/lib.rs:86
  21: std::panicking::try
             at src/libstd/panicking.rs:281
  22: std::panic::catch_unwind
             at src/libstd/panic.rs:394
  23: std::rt::lang_start_internal
             at src/libstd/rt.rs:51
  24: std::rt::lang_start
             at /rustc/c2d141df59703393c0c683abc259f9a8c3be041a/src/libstd/rt.rs:67
  25: main
  26: __libc_start_main
  27: _start

Any idea what might be going on here? I imagine there's a chance that cpal might have been doing something incorrectly unchecked that perhaps alsa is now panic!ing early on, but it would be good to solve this before landing the PR.

diwic · 2020-04-12T16:48:14Z

@mitchmindtree

Okay, let's go for it! 🙂

Could you invite/add me to rustaudio:cpal-administrators and I'll add that team as owner to the alsa crate? That should give you a backup in case I become long term unavailable/busy.
And if our collaboration works out fine, in some time I'll consider also moving the repositories themselves under the RustAudio organisation.

diwic · 2020-04-12T17:02:52Z

src/host/alsa/mod.rs

-    }
+) -> Result<alsa::pcm::HwParams<'a>, BackendSpecificError> {
+    let hw_params = alsa::pcm::HwParams::any(pcm_handle)?;
+    hw_params.set_access(alsa::pcm::Access::RWInterleaved)?;

    let sample_format = if cfg!(target_endian = "big") {


alsa::pcm::Format::S16() (and U32() and U16) returns the native endian, so this can be simplified.

alexmoon · 2020-04-12T17:24:08Z

@alexmoon are you able to confirm each of the examples work for you locally? If so, would you mind sharing your current linux setup?

When attempting to run the beep.rs and record_wav.rs examples I'm getting a panic! related to a file descriptor in a bad state. If I unwrap the result returned by stream.play() on line 43 of the beep.rs example, I get the following panic immediately after running:

Any idea what might be going on here? I imagine there's a chance that cpal might have been doing something incorrectly unchecked that perhaps alsa is now panic!ing early on, but it would be good to solve this before landing the PR.

You're right. Previously Stream::play() and Stream::pause() were ignoring errors and my patch added a ? to propagate the errors (it seems like most ALSA devices don't support pausing). I've added a quick fix to just ignore the errors to match previous behavior. All the samples work for me with this change. Sorry for the oversight.

It would probably be better to use the can_pause field. I also have a more extensive update in progress that, among other things, implements play and pause within the Stream so it works for all ALSA devices.

diwic · 2020-04-12T17:30:38Z

You're right. Previously Stream::play() and Stream::pause() were ignoring errors and my patch added a ? to propagate the errors (it seems like most ALSA devices don't support pausing).

I was debugging this in parallel. ALSA devices do support pausing, but you were calling "play" when the device was already running (i e not paused), which is why the function fails.

mitchmindtree · 2020-04-12T17:33:46Z

Could you invite/add me to rustaudio:cpal-administrators and I'll add that team as owner to the alsa crate? That should give you a backup in case I become long term unavailable/busy.
And if our collaboration works out fine, in some time I'll consider also moving the repositories themselves under the RustAudio organisation.

@diwic sounds great to me! And thank you both for diving into the play/pause stuff!

alexmoon · 2020-04-13T02:40:52Z

You're right. Previously Stream::play() and Stream::pause() were ignoring errors and my patch added a ? to propagate the errors (it seems like most ALSA devices don't support pausing).

I was debugging this in parallel. ALSA devices do support pausing, but you were calling "play" when the device was already running (i e not paused), which is why the function fails.

Interesting, and that makes sense. So are you saying that the PCM::pause() call should succeed even if HwParams::can_pause() returns false? In that case it's be easy enough to test the stream state before calling PCM::pause().

My suggestion would be to keep the behavior that matches what the current implementation is doing in this pull request (i.e. ignore the error) and then do a follow-up pull request to clean up some of these corners of the implementation. That way if we run into any compatibility issues with users of cpal it'll be easier to bisect if it's due to the change to the alsa crate or due to changes in how cpal interfaces with alsa.

diwic · 2020-04-13T04:53:47Z

So are you saying that the PCM::pause() call should succeed even if HwParams::can_pause() returns false? In that case it's be easy enough to test the stream state before calling PCM::pause().

can_pause should return true for common soundcards, including the PulseAudio plugin used by default in most distros.

Investigating further, the beep example calls stream.play()?, since apparently it expects the stream to paused.

But the stream is explicitly started inside build_stream_inner, which seems like a bad idea - for playback streams at least - because starting a stream with no data in it leads to an immediate underrun! Also since the start_threshold is set to zero the stream will auto-start at first sample write anyway...

So (at least) three things are causing the stream to start:

handle.start() inside build_stream_inner
set_start_threshold(0) starts the stream as soon as data is written (I think?! the usual value for this behaviour is set_start_threshold(1), I've never seen 0 before)
stream.start() from beep.rs

The best behaviour (IMO) is to fill the buffer completely first and then start the stream. I'm not sure how to change things around in order not to break existing code though.

My suggestion would be to keep the behavior that matches what the current implementation is doing in this pull request (i.e. ignore the error) and then do a follow-up pull request to clean up some of these corners of the implementation. That way if we run into any compatibility issues with users of cpal it'll be easier to bisect if it's due to the change to the alsa crate or due to changes in how cpal interfaces with alsa.

Sounds good to me.

alexmoon · 2020-04-13T20:32:18Z

@mitchmindtree Do you know what's up with the CircleCI build error? I don't think it has anything to do with these changes...

mitchmindtree · 2020-04-14T09:38:44Z

Do you know what's up with the CircleCI build error?

I think it's left over from the days when we used CircleCI to test windows. I believe the CircleCI configuration file was removed, but never disabled on the site itself. It looks like a recent update must have caused it to start getting picked up again regardless of missing the config. I'll see if I can disable it later on.

I'm busy with some contract work atm but will test this PR again asap!

mitchmindtree · 2020-04-15T15:55:46Z

OK this is looking great! All the examples seem to be running well. Happy to land this :)

The `alsa-sys` crate repository has lived [here][1] for quite a while. This commit removes the unnecessary step from the CI workflow now that alsa-sys has finally been removed as of RustAudio#386. [1]: https://github.com/diwic/alsa-sys

Switch from vendored alsa-sys to alsa crate

62d540d

mitchmindtree reviewed Apr 10, 2020

View reviewed changes

mitchmindtree mentioned this pull request Apr 10, 2020

Use bindgen to generate alsa-sys bindings #375

Closed

This was referenced Apr 12, 2020

Use alsa-rs for alsa support rather than alsa-sys #308

Closed

old alsa-sys is vendored #377

Closed

diwic reviewed Apr 12, 2020

View reviewed changes

Ignore play/pause errors for ALSA Stream

b386e63

Merge remote-tracking branch 'upstream/master' into alsa

5da315f

mitchmindtree mentioned this pull request Apr 14, 2020

in simple_audio example pause does not work nannou-org/nannou#541

Open

mitchmindtree merged commit 7df5b4e into RustAudio:master Apr 15, 2020

mitchmindtree mentioned this pull request Apr 15, 2020

Remove alsa-sys publish step from cpal publish github workflow #390

Merged

mitchmindtree mentioned this pull request Apr 15, 2020

Migrate from alsa-sys to alsa? #346

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch from vendored alsa-sys to alsa crate #386

Switch from vendored alsa-sys to alsa crate #386

alexmoon commented Apr 6, 2020

est31 commented Apr 6, 2020

mitchmindtree left a comment

mitchmindtree Apr 10, 2020

alexmoon Apr 10, 2020

est31 commented Apr 10, 2020

alexmoon commented Apr 10, 2020

mitchmindtree commented Apr 10, 2020

diwic commented Apr 11, 2020

mitchmindtree commented Apr 12, 2020

diwic commented Apr 12, 2020

mitchmindtree commented Apr 12, 2020

mitchmindtree commented Apr 12, 2020

diwic commented Apr 12, 2020

diwic Apr 12, 2020

alexmoon commented Apr 12, 2020

diwic commented Apr 12, 2020

mitchmindtree commented Apr 12, 2020

alexmoon commented Apr 13, 2020

diwic commented Apr 13, 2020 •

edited

Loading

alexmoon commented Apr 13, 2020

mitchmindtree commented Apr 14, 2020

mitchmindtree commented Apr 15, 2020

Switch from vendored alsa-sys to alsa crate #386

Switch from vendored alsa-sys to alsa crate #386

Conversation

alexmoon commented Apr 6, 2020

est31 commented Apr 6, 2020

mitchmindtree left a comment

Choose a reason for hiding this comment

mitchmindtree Apr 10, 2020

Choose a reason for hiding this comment

alexmoon Apr 10, 2020

Choose a reason for hiding this comment

est31 commented Apr 10, 2020

alexmoon commented Apr 10, 2020

mitchmindtree commented Apr 10, 2020

diwic commented Apr 11, 2020

mitchmindtree commented Apr 12, 2020

diwic commented Apr 12, 2020

mitchmindtree commented Apr 12, 2020

mitchmindtree commented Apr 12, 2020

diwic commented Apr 12, 2020

diwic Apr 12, 2020

Choose a reason for hiding this comment

alexmoon commented Apr 12, 2020

diwic commented Apr 12, 2020

mitchmindtree commented Apr 12, 2020

alexmoon commented Apr 13, 2020

diwic commented Apr 13, 2020 • edited Loading

alexmoon commented Apr 13, 2020

mitchmindtree commented Apr 14, 2020

mitchmindtree commented Apr 15, 2020

diwic commented Apr 13, 2020 •

edited

Loading