dbalsom / martypc

An IBM PC/XT emulator written in Rust.
Other
580 stars 19 forks source link

ALSA crash on init with EINVAL in snd_pcm_sw_params #66

Open jtsiomb opened 1 year ago

jtsiomb commented 1 year ago

martypc crashes on init on my GNU/Linux system with: thread 'main' panicked at 'Failed to build an output audio stream: BackendSpecific { err: BackendSpecificError { description: "ALSA function 'snd_pcm_sw_params' failed with error 'EINVAL: Invalid argument'" } }', /usr/local/src/martypc/core/src/sound.rs:146:14

I'm running debian GNU/Linux without any audio daemons, just ALSA. The version of martypc I've built is the current git HEAD (f18b73e7a97824b31513dbb1668bc939996a8bde).

Here's the relevant part of a full backtrace, with all the rust unwind and backtrace display noise removed:

16:     0x5555557b5a2d - core::result::Result<T,E>::expect::h5577ddc278659af8
                           at /usr/src/rustc-1.70.0/library/core/src/result.rs:1046:23
17:     0x5555557a86dc - marty_core::sound::SoundPlayer::new::h52d87d0165aed411
                           at /usr/local/src/martypc/core/src/sound.rs:139:29
18:     0x5555557940df - martypc_pixels_desktop::run::h7f3e9de3742bc3cb
                           at /usr/local/src/martypc/frontends/martypc_pixels_desktop/src/lib.rs:523:36
19:     0x5555557917c7 - martypc::main::h2ad35e0e7d3e7dd8
                           at /usr/local/src/martypc/frontends/martypc_pixels_desktop/src/main.rs:38:5
dbalsom commented 1 year ago

your hardware configuration and RUST_LOG=trace output, please.

dbalsom commented 1 year ago

also, do the cpal examples run? https://github.com/RustAudio/cpal

jtsiomb commented 1 year ago

Here's the stderr output when running with RUST_LOG=trace: https://pastebin.com/3tUFTdYS

The cpal examples seem to run fine. The beep example beeps, and the enumerate example produces this output (split into three parts because pastebin refused to accept more than 512kb per paste):

jtsiomb commented 12 months ago

What information do you need about my hardware configuration? Sound hardware is some on-board HDA-compatible on the motherboard: "Audio device: Intel Corporation 100 Series/C230 Series Chipset Family HD Audio Controller (rev 31)"

dbalsom commented 12 months ago

Can you provide the output of the 'enumerate' example from cpal examples?

jtsiomb commented 12 months ago

I did, it's the three-part pastebin above.

dbalsom commented 11 months ago

This seems to be similar to https://github.com/bevyengine/bevy/issues/2699

From the linked article: https://wiki.archlinux.org/title/Advanced_Linux_Sound_Architecture#Dmix

Integrated motherboard sound cards (such as Intel HD Audio), usually do not support hardware mixing. On such cards, software mixing is done by an ALSA plugin called dmix. This feature is enabled automatically if hardware mixing is unavailable.

can you verify if dmix is enabled?

jtsiomb commented 11 months ago

It is, but it's also irrelevant, I'm not trying to play back multiple audio streams, nor is the error about failing to open the dmix device.

dbalsom commented 11 months ago

are you using any particular distribution I could attempt to repo with? Nevermind, I missed the 'debian' in your initial message.

Is there a reason you're not running any audio daemon?

jtsiomb commented 11 months ago

Debian yes, but it shouldn't make any difference, as the default debian setup is very different to what I'm running.

I'm not running audio daemons because they are unnecessary bloat. The native ALSA kernel interface is more than sufficient for what I need for audio output, and I prefer to err towards simplicity.

dbalsom commented 11 months ago

Fair enough.

MartyPC is opening the audio device twice in rapid succession, and I think that is the problem. The long story short is it was a result of trying to encapsulate cpal's callback generics in a generic SoundPlayer struct. But to create the SoundPlayer struct I have to know the type of the sample format, which I need to ask cpal for, and it opens the device to retrieve that. The sample rate is returned only to be passed to the SoundPlayer's new() method, which opens the device again with the specified sample rate. This is not a great design, but didn't cause problems if the audio device can be opened multiple times.

Theoretically the device should be closed when the device struct is dropped at the end of the first function that reports the sample rate, but I suppose that it takes a non-zero amount of time to actually close the device in a manner such that it can be opened again.

I think the solution is to return the device handle along with the sample rate from ::get_sample_rate, and then I can pass both to new(), so we don't have to reopen the device when creating a SoundPlayer.

jtsiomb commented 11 months ago

Interesting. I tried adding thread::sleep_ms(10000) after the get_sample_format call in frontends/martypc_pixels_desktop/src/libs.rs but aside from the 10 second pause, it didn't make a difference, so I'm not sure it's a matter of time or inability to re-open the device.

I commented out the whole block, including get_sample_format and the match block, and replaced it with let sp = SoundPlayer::new::<i16>(); and it still failed in exactly the same way. I'm sure 16bit signed samples are supported, but just in case I also tried both other variants, and no difference. Btw I then also printed the result of get_sample_format and it is F32.

I hope this helps. If you want me to test some experiment let me know, but I'm not at all familiar with rust, so spell out any changes, or give me a patch to apply.

dbalsom commented 11 months ago

let's just see what this prints out sound.rs.patch

jtsiomb commented 11 months ago
Default audio device: default
Default audio config: SupportedStreamConfig { channels: 2, sample_rate: SampleRate(44100), buffer_size: Range { min: 170, max: 1466015503 }, sample_format: F32 }
fuel-pcbox commented 11 months ago

TBH I don't think this should count as a bug, most Linux software requires an audio daemon to run properly, including emulators. The fact that you're not running one because you think it's "unnecessary bloat" is a you problem, not a problem of the software for not being able to handle it.

jtsiomb commented 11 months ago

most Linux software requires an audio daemon to run properly

Citation needed.

Obviously how important compatibility with a variety of setups is to a project, is up to the author to decide. But Linux has an audio interface, it's called ALSA, it doesn't require any intermediate daemons to play audio, and despite your assertion, most programs work with it just fine.

Since most GNU/Linux distributions install pulse audio by default, and most users don't bother to change that, often the ALSA code path of many projects gets very little testing. And while most projects intend to support ALSA, sometimes unnoticed regressions crop up in the non-pulse audio code path. That's why I considered this to be a bug, and thought I should report it.

dbalsom commented 11 months ago

I would have probably said this was an unsupported configuration had not the cpal examples worked. But they do, so that implies that I am doing something that can be corrected, and if it is minor I am willing to do it. But if it isn't opening the device twice, I am not sure.

I have been considering an alternative SDL frontend and I have been slowly factoring pieces of the current wgpu frontend into common front end libraries to enable sharing code between the two frontends. I will note based on the logs provided that I anticipate even if the sound problem was resolved, MartyPC may run poorly on this system. wgpu did not initialize vulkan, and we're falling back to the opengl backend, which based on other issues opened likely will have poor performance.

So this may be another case where an SDL front end will work better and hopefully address the sound issue as well.

dbalsom commented 10 months ago

I plan to switch from using cpal directly to using rodio, which uses cpal under the hood but I will not have to do the awkward generic-instantiation initialization dance myself, which is almost worth it just by itself.

Whether that will fix things or not...?

rodio provides a mixing facility, which I will eventually need to implement multiple audio sources (pc speaker + dac + opl emulation, as well as any sounds the front end might want to play (floppy noises? who knows))

rodio has some examples, if you could check if they run?

In any case 0.2.0 will make sound optional, so you can at least run the emulator regardless.

jtsiomb commented 10 months ago

Yeap, I just compiled rodio and its examples work fine.

And I agree, being able to run the emulator without audio at all, would be a good thing to add.

dbalsom commented 10 months ago

In the meantime until I can implement rodio, I made the change to re-use the device handle, whether that helps or not. If not, at least now you can disable audio with --noaudio

jtsiomb commented 10 months ago

Thank you, having that option is always good.

Unfortunately I can't test this. It complains that epaint requires rustc 1.72, while I have 1.70 which is what debian unstable packages at the moment. I'll get back to you as soon as 1.72 becomes available in debian.

peterfirefly commented 10 months ago

Unfortunately I can't test this. It complains that epaint requires rustc 1.72, while I have 1.70 which is what debian unstable packages at the moment. I'll get back to you as soon as 1.72 becomes available in debian.

You can easily install 1.72 (or any other relevant version) yourself -- and easily remove it or upgrade it if you like -- with the rustup tool. Rustup itself is also easy to install, either from the rust people:

https://rustup.rs/

or via 'apt install rustup' on trixie and sid:

https://packages.debian.org/search?suite=default&section=all&arch=any&searchon=names&keywords=rustup

It is normal for people to use rustup to always use the newest stable version of rust (or sometimes a recent nightly version or sometimes a specific older version). Rustup is the official tool for doing this.