SDL mixer callback incorrectly calling SDL mixer functions

[lucianposton]

Looks like we're calling sdl mixer functions in sdl mixer callbacks when we shouldn't be. See Mix_HookMusicFinished doc at https://www.libsdl.org/projects/SDL_mixer/docs/SDL_mixer_69.html

And our Mix_HookMusicFinished callback: https://github.com/CleverRaven/Cataclysm-DDA/blob/master/src/sdltiles.cpp#L2094-L2123

[remyroy]

@CIB What do you think about this?

[CIB]

Huh.. I didn't even realize. :D

Found this so far:

http://forums.libsdl.org/viewtopic.php?t=5269&sid=9edbfc44f9509375a2353230923dc605

The best suggestion I see there is to handle it in the SDL main loop by sending a custom event. Of course that only works if the main loop never blocks.

[lucianposton]

An issue with using the event loop is we still have getch()s everywhere. Not everything uses SDL_PollEvent() yet.

We probably could get by with a simple boolean check in the main game loop, game::do_turn(). With this approach, the music could stop playing if the user remains still / AFKs, but I don't see that as a big issue. I find the constant background music obnoxious at times, so breaks in it might be beneficial (although that decision is probably best left up to the soundpack).

Another option is a polling thread specifically for the music playlist.

[kevingranade]

A thread to handle the transition seems like the only thing that works properly and will be robust. Overhauling how blocking is handled in 99% of the code to support 1% of the code is a bit overboard. Said thread would block on a mutex released by the end callback, when released it would check a "next song" value maintained by sound.cpp, use its value to trigger the next song, then block of the mutex again until that song ends. Any method (s) in sound.cpp that set "next song" would need to acquire a lock to write to it, but otherwise it could be updated any time the main thread desires.

[mark7]

The SDL_mixer library apparently also isn't being initialized or deinitialized by Cataclysm. Looking at the library source, probably currently no big impact other than a one-off leak due to not being deinitialized, but the library could always start breaking.

Not quite the same issue as the one here, but I'll assuage my guilt for hijacking the issue by noting that init and deinit needs to be synchronized with any thread making SDL_mixer calls and that anyone adding a thread probably should be fixing this as well.