fabiangreffrath / woof

Woof! is a continuation of the Boom/MBF bloodline of Doom source ports.
GNU General Public License v2.0
209 stars 35 forks source link

Audio issue on AmigaOS4 (big-endian) #1285

Open samo790 opened 10 months ago

samo790 commented 10 months ago

We had just ported Woof 12.0 on AmigaOS4 (PowerPC)... no particular issue to report, but there is a problem with the audio. Apparently we can use SFX and no MUSIC, or only MUSIC without SFX. The reason is that If we start Woof by activating both, the game will start by after a while the game will freeze Unfortunely when it freeze, we doesn't either get any debugging information, we can still move the cursor, enter and / or exit the screen, but the system / screen has kind of frozen.

Any idea?

rfomin commented 10 months ago

What do you use for music, Fluidsynth? What version of OpenAl-Soft are you using? You can try to select OPL emulation for MIDI player.

samo790 commented 10 months ago

If i'm not mistaken actually we are using OpenAl-Soft 1.18 I just sent a bump to this thread to the programmer who did the port :-)

fabiangreffrath commented 10 months ago

I'd also be interested in any porting issues that you encountered. Our code is supposed to be endianess clean, and also clean of any sizeof(long) or char signedness issues, so whatever was necessary to get it to compile on Amiga OS, please let us know.

samo790 commented 10 months ago

Of course yes :-) For the moment there were no need for significant changes.. only this problem with the audio remains but we're trying to figure out what's causing it :-) Woof has recently been ported also to MorphOS PPC (another Amiga compatible system) and there don't seem to be any problems with the audio ... quite probably its something very specific, perhaps our OpenAL lib

fabiangreffrath commented 10 months ago

oof has recently been ported also to MorphOS PPC (another Amiga compatible system) and there don't seem to be any problems with the audio ... quite probably its something very specific, perhaps our OpenAL lib

I have contacted the porter. The only things they changed were related to file path handling:

https://github.com/fabiangreffrath/woof/compare/master...BeWorld2018:woof:master

rfomin commented 10 months ago

My only theory is problems with thread in i_oalmusic.c or a with OpenAL-Soft. I need Woof's terminal output or information about the selected MIDI player.

fabiangreffrath commented 10 months ago

My only theory is problems with thread in i_oalmusic.c or a with OpenAL-Soft. I need Woof's terminal output or information about the selected MIDI player.

There are some error paths in StartPlayer() and UpdatePlayer() that should probably reset the player_thread_running variable.

3246251196 commented 10 months ago

Log of running the game until crash:

Woof 12.0.0 (built on Dec  5 2023)

M_LoadDefaults: Load system defaults.
 default file: ENVARC:/woof/woof.cfg

IWAD found: doom1.wad
DOOM Shareware version

V_Init: allocate screens.
W_Init: Init WADfiles.
 adding doom1.wad

M_Init: Init miscellaneous info.
R_Init: Init DOOM refresh daemon - [                ]..............
P_Init: Init Playloop state.
I_Init: Setting up machine state.
I_InitJoystick: Initialize game controller.
I_InitSound:
 Using 'ahi.device' @ 44100 Hz.
 Precaching all sound effects... done.
NET_Init: Init network subsystem.
D_CheckNetGame: Checking network game status.
 startskill 3  deathmatch: 0  startmap: 1  startepisode: 1
 player 1 of 1 (1 nodes)
S_Init: Setting up sound.
HU_Init: Setting up heads up display.
ST_Init: Init status bar.

OPL_Init: Using driver 'SDL'.
S_ChangeMusic: D_INTRO (doom1.wad)
S_ChangeMusic: D_E1M5 (doom1.wad)
P_SetupLevel: E1M5 (doom1.wad), Doom nodes, Doom 1.9 complevel
G_DoPlayDemo: Playing demo with Doom 1.9 (109) compatibility.

SDL2: 2.25.0 vorbis: 1.3.7 FLAC: 1.3.2 ogg: 1.3.3

Statically linked on the AmigaOne system.

32 bit system.

===

Either of these options on their own work: -nosound -nosfx -nomusic

Once there is a need for music and sfx then the game will freeze. The length of time from start for the freeze to occur is random, but generally happens within 30 seconds; more likely within 10.

===

I do also need to change the paths for things too. As you can see: ENVARC:/woof/woof.cfg needs to be ENVARC:woof/woof.cfg. I doubt that this is a path issue though...

Perhaps you can suggest some printf debugging locations to test. BTW, I do notice that some structures that are used in the i_oplmusic.c file use packed structures. I believe this has caused some issues for us in the past.

===

On a side note: I did need to properly initialise SDL_TIMER to get this to work on the AmigaOne system. Firstly, I am not sure if I should be doing that since it is not done in the code; secondly, most other systems are a lot more permissive than AmigaOne - which requires proper init/destruction of SDL modules before finally quitting.

rfomin commented 10 months ago

I do notice that some structures that are used in the i_oplmusic.c file use packed structures. I believe this has caused some issues for us in the past.

We also use packing in WAD code and other places, but the game runs without music.

I did need to properly initialise SDL_TIMER to get this to work on the AmigaOne system. Firstly, I am not sure if I should be doing that since it is not done in the code; secondly, most other systems are a lot more permissive than AmigaOne - which requires proper init/destruction of SDL modules before finally quitting.

Yes, on Windows almost no initiation is required for some reason. Also we use timer with nanosecond precision SDL_GetPerformanceCounter. I'm curious if it works properly on Amiga.

The issue is probably in i_oalmusic.c file. Try to increase delay here:

diff --git a/src/i_oalmusic.c b/src/i_oalmusic.c
index ce4e333..1c5a0d8 100644
--- a/src/i_oalmusic.c
+++ b/src/i_oalmusic.c
@@ -199,7 +199,7 @@ static int PlayerThread(void *unused)

     while (player_thread_running && UpdatePlayer())
     {
-        SDL_Delay(1);
+        SDL_Delay(100);
     }

     return 0;

It's possible to remove that thread entirely.

3246251196 commented 10 months ago

@rfomin On a single test it seems to have progressed further than usual. I am using the 12.0 release + this patch: https://github.com/fabiangreffrath/woof/pull/1304.

Let me do some more testing / @samo790 I will send you a binary to test.

3246251196 commented 10 months ago

@fabiangreffrath Would you also consider a patch for proper init/de-init of SDL_TIMER?

samo790 commented 10 months ago

@3246251196 Ok thanks :-)

rfomin commented 10 months ago

On a single test it seems to have progressed further than usual.

But still a crash occurs? Alternatively, we can remove the music player thread completely and update the OpenAL stream in the main loop. But it works fine on other platforms, maybe the problem is in the OpenAL port?

3246251196 commented 10 months ago

There is no crash. But I have not extensively tested it. So far, your patch looks promising.

Can you just succinctly describe how you patch may have solved the issue?

Regards,

rfomin commented 10 months ago

Can you just succinctly describe how you patch may have solved the issue?

I was sloppy with the music player thread when using the alSourceQueueBuffers API, since OpenAL is supposed to be thread safe. But maybe it was just luck that it works on other platforms. Using mutexes is the right approach, it prevents possible deadlocks.

fabiangreffrath commented 10 months ago

@fabiangreffrath Would you also consider a patch for proper init/de-init of SDL_TIMER?

Yes, of course. I appreciate every portability fix.

3246251196 commented 10 months ago

@rfomin So your patch definitely improves things in so far as the demo / playing lasts a lot longer. But, eventually, it does freeze again. Can you possibly look into more, or provide some printf debugging that would help?

rfomin commented 9 months ago

I've tried removing the music player thread here: #1320 The OPL3 emulation is quite heavy, we don't want it in the main thread. Does Amiga have multicore CPU?

I think we need to narrow it down. Try using the OGG music pack (just put it in autoload directory or run woof -file doom_sc55_ogg.zip). If you have built an xmp library, you can also try WAD with tracker music: idgames

rfomin commented 9 months ago

Does Chocolate Doom (version 3.0 with OPL emulation) work on this machine?

samo790 commented 9 months ago

@rfomin we only have an old port of Chocolate Doom (2.2) http://os4depot.net/?function=showfile&file=game/fps/chocolate-doom.lha

rfomin commented 9 months ago

we only have an old port of Chocolate Doom (2.2)

I'm asking because it's not clear where the problem is, in the OpenAL streaming or in the OPL emulation. Please try the other music formats I mentioned in the https://github.com/fabiangreffrath/woof/issues/1285#issuecomment-1849376078

samo790 commented 9 months ago

@rfomin Ok, will try them... :-)

samo790 commented 9 months ago

PR #1320 patch which removes the sound thread seems fixing the issue under AmigaOS 4, no more freeze when using audio + music :-)

rfomin commented 9 months ago

PR #1320 patch which removes the sound thread seems fixing the issue under AmigaOS 4, no more freeze when using audio + music :-)

Have you tried other music formats without PR #1320?

samo790 commented 9 months ago

@rfomin With the old binary by starting from shell with woof -file doom_sc55_ogg.zip i can see the ogg files loading, but as soon as it reach the intro screen it just freeze :-( Instead with latest binary that include PR #1320 everything works

Btw, ogg files are inside a music folder, if i want to load them automatically instead of via shell, do i need to just copy that "music" folder into autoload dir, or they need to be extracted first?

fabiangreffrath commented 9 months ago

So, this appears to be a general threading error on either the OS or openal-soft side. Since literally no other system has ever shown up a bug like this, it would be interesting to see what difference an updated openal-soft library would make.

rfomin commented 9 months ago

Btw, ogg files are inside a music folder, if i want to load them automatically instead of via shell, do i need to just copy that "music" folder into autoload dir, or they need to be extracted first?

Extract *.ogg files to autoload/doom.wad directory. Thanks for testing!

So, this appears to be a general threading error on either the OS or openal-soft side.

I have a theory that there is a faulty track switching sequence. I will try to come up with a patch later.

samo790 commented 9 months ago

@rfomin Thanks, copied them on autoload/doom.wad folder and works! Btw, i just opened a (possibile) bugreport in our OpenAL repository, let's see what comes out :-)

rfomin commented 9 months ago

@samo790 I have updated #1304, please test it.

3246251196 commented 9 months ago

@rfomin Thank you for the effort, but we still get a freeze eventually.

With #1320 we have something that works. If you want me to consider any other changes then I can try them. I am just interested in getting a port out for AmigaOS4 so I am not invested in finding the root cause.

@fabiangreffrath The following are the full set of differences that I made compared to the main repository at TAG 12.0.2. You may or may not be interested in the various cleanups that I needed to perform in order to prevent leaking memory on the AmigaOne machines.

changes_for_amigaos4.diff.txt

I seem to have to append .txt for github to allow me to upload it.

fabiangreffrath commented 9 months ago

@fabiangreffrath The following are the full set of differences that I made compared to the main repository at TAG 12.0.2. You may or may not be interested in the various cleanups that I needed to perform in order to prevent leaking memory on the AmigaOne machines.

Thank you for this! I think we can use some of these changes in the master branch, especially the SDL timer initialization. Regarding the cleanups of the SDL resources at exit, we once had this in the code, but decided to remove it again, because on a typical shared memory OS this would mean to "clean up the house before it gets teared down".

fabiangreffrath commented 9 months ago

BTW, is there a common precompiler macro that one could check for that covers both __amigaos4__ and __MORPHOS__ cases?

3246251196 commented 9 months ago

@fabiangreffrath Hi, there is no common macro.

Just remember that the patch I sent above was in addition to PR #1320.

Regards,

rfomin commented 9 months ago

Just remember that the patch I sent above was in addition to PR #1320.

I think we don't want to remove the music player thread, as OPL emulation will affect the performance of the main game loop. It's hard to say what's wrong, I've tried all my ideas in the #1304

3246251196 commented 9 months ago

That's fine. Thanks for the help. So long as we have something that works. I could try with newer libraries etc, but I am working on other ports and things.

The point is: if you were thinking of merging in any AmigaOS4 specific code I would think twice since there is a dependency on the #1320 patch. Unless that was officially merged without all the commenting out hacks and an option existed for single threadedness: all of which is probably not worth the hassle.

I guess we can close this issue, @samo790.

samo790 commented 9 months ago

@3246251196 Yes, general issue persists but for the moment we have a workaround :-)

fabiangreffrath commented 8 months ago

Can we close this or is anything left to do here?

samo790 commented 8 months ago

Well, the specific problem has been solved so I think it can be closed, but I don't understand if there is a way to fix the music player thread or if on AmigaOS4 we should manage it differently... also it remains to be decided whether or not to implement the specific diffs to compile it on AmigaOS4 but this can be done by opening a new ticket ...

rfomin commented 8 months ago

I don't understand if there is a way to fix the music player thread or if on AmigaOS4 we should manage it differently...

I have found another problem with my mutex approach: c744db3 So maybe the current master will work better.

samo790 commented 8 months ago

Excellent... then I would suggest keeping this ticket open a little longer to see if your fix solves our freeze problem :-)

samo790 commented 8 months ago

Just noticed you removed mutex in #1430 Hope this will not give any side effects under AmigaOS4 :-)

rfomin commented 8 months ago

Just noticed you removed mutex in #1430 Hope this will not give any side effects under AmigaOS4 :-)

It didn't solve the AmigaOS4 problem and affected performance on other platforms, so it seems like it was a bad idea. We will improve the sound system later.

3246251196 commented 7 months ago

Hi. Just to be clear. We are currently using 12.0.2 + the patch in #1320.

I think I may just wait for a new release which @samo790 said was coming soon?

Are we saying that we should just try the new release with no patch changes (other than those that are necessary for AOS4, of course)?

rfomin commented 7 months ago

Are we saying that we should just try the new release with no patch changes (other than those that are necessary for AOS4, of course)?

I have removed the player mutex as it affects performance on other platforms. So it would work the same way as before. We have plans to improve this in the future.

rfomin commented 7 months ago

@3246251196 @samo790 We have simplified the inner workings of the OPL module here: #1531, maybe now it will work with the current master.

3246251196 commented 7 months ago

I think that the changes in #1531 should respect the following in src/CMakeLists.txt:

if(FluidSynth_FOUND)
    target_sources(woof PRIVATE i_flmusic.c)
    target_link_libraries(woof PRIVATE FluidSynth::libfluidsynth)
endif()

A change in #1531 means that in the function I_OAL_SetMusicVolume(), a pointer to stream_fl_module is sought even if FluidSynth is not found during the CMake initialisation and this leads to an undefined reference because the translation unit, of course, is not compiled.

@fabiangreffrath @rfomin

3246251196 commented 7 months ago

1531 requires the finding of FluidSynth?

If so, I would have to investigate why this is not being found for our Amiga port.

rfomin commented 7 months ago

A change in #1531 means that in the function I_OAL_SetMusicVolume(), a pointer to stream_fl_module is sought even if FluidSynth is not found during the CMake initialisation and this leads to an undefined reference because the translation unit, of course, is not compiled.

You're right, fixed in https://github.com/fabiangreffrath/woof/commit/b42ecbcd37ceeb514963e002878a8f46ae8b0c07

https://github.com/fabiangreffrath/woof/pull/1531 requires the finding of FluidSynth?

No, Fluidsynth is optional. I recommend using it as it is much faster than the OPL emulation and with the right soundfont can sound very close to the Roland SC-55. We require Fluidsynth 2.2.0 or higher, which might be hard to build (it has GLib dependency).

3246251196 commented 5 months ago

Updated the base code from 12.0.2 to 14.5 with out any patches mentioned above (changes remain that are necessary for the Amiga port of course). I see again that within some random amount of seconds when the demo loads up the system freezes.

I guess we should continue to apply #1320. I will have to see if that patch cleanly applies to the updated code.

rfomin commented 5 months ago

Updated the base code from 12.0.2 to 14.5 with out any patches mentioned above (changes remain that are necessary for the Amiga port of course). I see again that within some random amount of seconds when the demo loads up the system freezes.

Thanks for testing. It seems to be an OpenAL Soft problem on the Amiga system, there's not much we can do.