sarah-walker-pcem / arculator

Arculator
http://b-em.bbcmicro.com/arculator
GNU General Public License v2.0
54 stars 23 forks source link

No sound on Linux since OpenAL support was removed #30

Open sjnewbury opened 1 year ago

sjnewbury commented 1 year ago

The SDL sound system isn't getting initialised for some reason. Reverting the removal of OpenAL support makes it work again.

Sophira commented 1 year ago

For what it's worth, sound support on Linux is working for me, both when compiled from the git repo and from the release archive. Is SDL working for you on other games/packages?

(I'm not the maintainer of the repo, but I do compile and run Arculator fairly regularly on Linux and personally haven't noticed any issues with sound lately.)

Sophira commented 1 year ago

For the record, btw, on my system where the sound support is working, I'm running Arculator on Gentoo with these SDL packages and USE flags:

dev-perl/Alien-SDL-1.446.0-r1
dev-perl/SDL-2.548.0-r1
dev-python/pygame_sdl2-7.3.5-r2
media-libs/libsdl-1.2.15_p20221201      X, alsa, joystick, opengl, sound, video, xv
media-libs/libsdl2-2.24.2           X, alsa, dbus, haptic, ibus, joystick, opengl, sound, threads, udev, video, vulkan
media-libs/sdl-gfx-2.0.26-r1
media-libs/sdl-image-1.2.12_p20220527-r1    gif, jpeg, png
media-libs/sdl-mixer-1.2.12_p20221010       flac, fluidsynth, mad, midi, mod, modplug, mp3, vorbis, wav
media-libs/sdl-net-1.2.8_p20221010
media-libs/sdl-pango-0.1.2-r1
media-libs/sdl-sound-1.0.3_p20220525        flac, modplug, mp3, vorbis
media-libs/sdl-ttf-2.0.11_p20220525     X
media-libs/sdl2-image-2.0.5_p20210328-r1    jpeg, png
media-libs/sdl2-mixer-2.0.4-r3          flac, fluidsynth, mad, midi, mod, modplug, mp3, vorbis, wav
media-libs/sdl2-net-2.0.1
media-libs/sdl2-ttf-2.20.0          X, harfbuzz

I'm also using ALSA without anything in between, so no PulseAudio or PipeWire. (I'm planning to switch to PipeWire but haven't gotten around to it yet.)

sjnewbury commented 1 year ago

I'm using Gentoo too, but with PipeWire. I tried with different SDL_AUDIODRIVER=alsa|pulseaudio|pipewire options which worked with other software, it never came up in the PipeWire sound sources list, while OpenAL did work with ALSOFT_DRIVERS=alsa|pulseaudio|pipewire|sdl2. I'm pretty sure I tried it with native ALSA too.

I'll try it again since you're confirming it's working for you.

sjnewbury commented 1 year ago

Still doesn't work. However, the Lark podule does work with the various backends configured by SDL_AUDIODRIVER. VIDC audio doesn't.

Sophira commented 1 year ago

You mention that VIDC audio doesn't work; does the floppy disc drive sound work for you? (It feels like it would be really odd if it did when the other sounds don't, but it's worth checking.)

sjnewbury commented 1 year ago

@Sophira No, floppy sound isn't working either. I take it you're using the current git master version built from source?

I'll add some debugging messages and try to figure it out.

sjnewbury commented 1 year ago

Okay, well SDL_OpenAudioDevice() is failing, returning "audio_device=0". The code doesn't currently report the error from SDL, so I'll add something and see what's up.

sjnewbury commented 1 year ago

The error from SDL for the SDL_OpenAudioDevice() is "Audio subsystem is not initialized"

sjnewbury commented 1 year ago

@Sophira Adding SDL_Init(SDL_INIT_AUDIO) to src/sound_sdl2.c "fixed" it. I'm not sure why SDL isn't already initialised from the SDL_Init(SDL_INIT_EVERYTHING) in wx-app.cc [different thread?], and especially how it's working for you without it???

Sophira commented 1 year ago

Again, I apologise for the delay in responding!

Yes, I'm compiling from git master.

I propose we try to work out what's going on here. My understanding is that SDL_Init would normally error out if SDL initialisation didn't work, but at the moment we don't seem to have any error-checking code for that. So I'd suggest we work on that first - it may be that the SDL error may have more information once we output it.

One moment!

Sophira commented 1 year ago

I added a minimal error check at https://github.com/Sophira/arculator/commit/49df0f8729170db36dd605d52f4ae4b7d3b1db21 for if SDL couldn't be initialised, which should also print whatever error SDL returns. Could you try applying that? For me the SDL initialisation succeeds past that as normal and displays the machine list window, but for you it might fail with an error - if so, it would be helpful to know what that error is.

sjnewbury commented 1 year ago

@Sophira That check for success is probably a really good idea! I'll give it a go and see what it says. I actually don't know if a partial initialisation is even possible, I'd need to check the SDL documentation. It's definitely at least partially initialising since it is working other than the audio!

FWIW the podules already call SDL_Init for the subsystem they are using, and all it does if it is already initialised is increase the reference count. In fact shouldn't the SDL_Init() in wx-app only initialise what it needs rather than "SDL_INIT_EVERYTHING" anyway?

sjnewbury commented 1 year ago

Interesting... SDL could not be initialised: SDL not built with haptic (force feedback) support

I guess that explains it. It is indeed true, I don't have USE=haptic. I also don't have any haptic hardware on my laptop, so... umm...

I guess it's technically an SDL2 bug. If haptic is required for SDL_INIT_EVERYTHING even if disabled, then disabling it shouldn't be possible! Even so, as I wrote above, shouldn't we only init what we actually need, when we need it?

It's also kind of amusing that it apparently initialises one subsystem after another and "works" for what has already started, but stops initialising any more subsystems after one fails. I guess it's always a good idea to check for error states! ;-)

sjnewbury commented 1 year ago

I've swapped SDL_INIT_EVERYTHING for SDL_INIT_VIDEO which automatically enables SDL_INIT_EVENTS, and added SDL_Init(SDL_INIT_JOYSTICK) to wx-sdl2-joystick.c so it matches the code in podules/. All the SDL_Init() calls should be checked for success IMHO, although I don't think it should be fatal if joysticks aren't available. For audio, I'm not so sure since users would probably expect it to work.

Anyway, by doing this nothing broke, and it now works for me without haptic support.

I'll wait for feedback before updating the PR.

Sophira commented 1 year ago

I can confirm that I have media-libs/libsdl2 compiled with haptic support, so yes, that would explain what's going on here. Apparently some of my other packages need it. (Including wine for some reason? Which is odd. I do have some force-feedback devices though.)

I agree that Arculator should only be initialising subsystems we're actually using - and the fact that you haven't had that error before now with other programs suggests that initialising specific subsystems only is, in fact, the way they're doing it, too.

I think we should be initialising things when the program starts, though, so you may want to also initialise audio at the start too. And definitely error-check the SDL_Init calls!

Come to think of it, we should probably take a thorough look to see what SDL subsystems are in use. For example, I can see that the joystick subsystem is in use because src/wx-sdl2-joystick.c uses it. I'll try to do a thorough check and see what I can find.

[edit: Oh, I see you already mentioned joystick support, oops! Sorry, I didn't see that.]

sjnewbury commented 1 year ago

I wasn't sure whether it would be preferred to initialise all subsystems used by the core at the beginning or as they come up. I don't think it really makes any difference other than the ability to soft-fail if a subsystem is unavailable, say joystick support is disabled in SDL2 (it does have a USEflag!).

It should also be taken into account the idea of podules being distributed as "plugins" we were discussing in the system-wide support issue. An out-of-tree podule using an SDL subsystem which isn't already initialised would have to have it's own SDL_Init(). The in-tree podules actually currently do that anyway. So there will always be potentially extra SDL_Init() calls.

There should also be SDL_QuitSubSystem() calls when shutting down, especially in the podule code.

sjnewbury commented 1 year ago

I went ahead and made an attempt to handle each SDL subsystem usage so refcounts are added and removed accordingly and failures to initialise audio and joystick are soft-fails. Maybe a non-debug message would be better though..? Theoretically it should reduce memory usage, at least when no machine is running! Maybe it's overkill since no podules currently use anything not already in the core so no actual resources would be freed, although architecturally feels like an improvement to me as long as any potential out-of-tree podules ensure they Init and Quit subsystems if such code ever appears using something else. Haptics maybe? :-P

https://github.com/sjnewbury/arculator/commit/415c178a67a305430d68bcdc68941a8b687f48d7

Sophira commented 1 year ago

It should also be taken into account the idea of podules being distributed as "plugins" we were discussing in the system-wide support issue. An out-of-tree podule using an SDL subsystem which isn't already initialised would have to have it's own SDL_Init(). The in-tree podules actually currently do that anyway. So there will always be potentially extra SDL_Init() calls.

That's a very good point - thanks for bringing it up. Okay, in that case, I agree - initialise SDL subsystems when needed.

Of course, the current code is written assuming everything got initialised correctly. For video that isn't a problem since the diff you reference will exit if that particular subsystem doesn't get initialised (which makes sense to me - there's not much point in running Arculator if it can't display anything, hence why I made it exit in the first place), but for the other subsystems we may need to add something to defend against if the subsystems didn't get initialised.

Or maybe we don't? The code seems to be okay for now, but of course we haven't tested with things like joystick support disabled, etc. So... maybe that's something we should try.

sjnewbury commented 1 year ago

Yes, definitely need to try building SDL with various breakage :-)

Thankfully we both use Gentoo so that's easy!

sjnewbury commented 1 year ago

Maybe I should create an Issue for SDL about the SDL_INIT_EVERYTHING being broken if haptic support isn't enabled? It would make much more sense if it initialised everything enabled. Or maybe it's deliberately broken?

sjnewbury commented 1 year ago

Yes, definitely need to try building SDL with various breakage :-)

Thankfully we both use Gentoo so that's easy!

Needs more work... disabling sound in SDL2 results in a segfault!

sjnewbury commented 1 year ago

It's the oak_scsi podule sound effects! It crashes when it calls sound_out_buffer(). Good job I'm testing various podules!

sjnewbury commented 1 year ago

Added a check to bail-out if sdl_sound is uninitialised in sound_out_sdl2.c sound_out_buffer()

sjnewbury commented 1 year ago

https://github.com/sjnewbury/arculator/commits/sdl-fix

sjnewbury commented 1 year ago

Going to try USE=-joystick

Sophira commented 1 year ago

Maybe I should create an Issue for SDL about the SDL_INIT_EVERYTHING being broken if haptic support isn't enabled? It would make much more sense if it initialised everything enabled. Or maybe it's deliberately broken?

Pretty sure it's deliberate. SDL_INIT_EVERYTHING includes SDL_INIT_HAPTIC, and the behaviour is that if anything fails, the whole SDL_Init fails. I would hazard a guess that the fact that some subsystems still work after that is just undefined behaviour.

sjnewbury commented 1 year ago

Seems to be okay with -joystick. Although, I'm not convinced about my refcount handling. As it is I just skip the joystick_close() in both the core and podule/common code if the subsystem isn't running. But there is no matching of whether it was started by the core or by a podule. In practice both are probably going to either succeed or fail, but I don't like it.

sjnewbury commented 1 year ago

Maybe I should create an Issue for SDL about the SDL_INIT_EVERYTHING being broken if haptic support isn't enabled? It would make much more sense if it initialised everything enabled. Or maybe it's deliberately broken?

Pretty sure it's deliberate. SDL_INIT_EVERYTHING includes SDL_INIT_HAPTIC, and the behaviour is that if anything fails, the whole SDL_Init fails. I would hazard a guess that the fact that some subsystems still work after that is just undefined behaviour.

I'm sure it is undefined behaviour. It's not uncommon for projects to only support their official distributed binaries, and they will always have everything enabled. SDL is has always been good with respect to Linux distribution specifics given its history, but the project has a much broader scope today and these things creep in.

Sophira commented 1 year ago

No, I'm talking about SDL_Init(SDL_INIT_EVERYTHING) failing in general, not just whether or not support is compiled in (which happens to always cause SDL_Init(SDL_INIT_EVERYTHING) to fail.)

[edit: Also, I'm headed to bed now... good night! I won't be around for several hours.]

sjnewbury commented 1 year ago

Just did a google for it, I see what you might mean by it failing in general: https://discourse.libsdl.org/t/sdl2-sdl-init-sdl-init-everything-failing/19250

Good night!