libretro / pcsx_rearmed

ARM optimized PCSX fork
GNU General Public License v2.0
164 stars 116 forks source link

Audio is broken on big-endian #817

Closed pcercuei closed 5 months ago

pcercuei commented 5 months ago

PCSX-ReARMed Version

r22-1571-g5ee1c9269d

Your device

GameCube

Operating System of your device

Other (consoles, etc.)

CPU architecture

Other

Issue description

Absolutely no audio in Rayman; missing "Staaaart your engines" sound in CTR, etc.

Step-by-step reproduction and logs

Bisected to 740eb9bcad421e4c8621d007271abdf44c1ed38f.

I wonder if we should start using the le16_t / le32_t data types I added for Unai in libpcsxcore as well, for everything that points to little-endian memory? Then the endianness issues would turn into compilation errors.

notaz commented 5 months ago

That commit has a bug which was corrected in 1e98ada32a77394ce8d14a33d689680764d8e41a so it might be something else. It's hard to see how this can break audio completely when it only deals with cdda and xa. Could it be incomplete rebuilds when bisecting as the makefiles lack proper deps?

pcercuei commented 5 months ago

CubeSX uses this repo as a subrepo that we sync up regularly, and CubeSX uses CMake, so it shouldn't be a problem of incomplete rebuild (as CMake detects the dependencies properly, except for .c includes).

1e98ada does not fix the sound issue I am seeing. Also, I'm pretty sure CTR's "Start your engines" sound is not CDDA anyway.

EDIT: To be precise, the audio is not broken completely; for instance you'd still hear the background music in CTR.

notaz commented 5 months ago

To be precise, the audio is not broken completely; for instance you'd still hear the background music in CTR.

You said it was for Rayman.

Anyway I took my time to set up a qemu-system-ppc Debian vm and "Start your engines" plays just fine. Could it be some CubeSX integration issue? Something with the compiler (try turn off optimizations)? Something else?

pcercuei commented 5 months ago

If it was anything else, it wouldn't explain why I could bisect the issue to that exact commit though.

notaz commented 5 months ago

Anything else than what exactly, big endian? __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ is certainly true in the emulated setup I tried today, so the real issue is unknown. The changes of that commit don't affect endianess of anything anyway. Reverting is not an option as it'll reintroduce some glitches.

Can you try this patch:

diff --git a/plugins/dfsound/xa.c b/plugins/dfsound/xa.c
index 6b5433fb..14706743 100644
--- a/plugins/dfsound/xa.c
+++ b/plugins/dfsound/xa.c
@@ -104,6 +104,7 @@ INLINE void MixCD(int *SSumLR, int *RVB, int ns_to, int decode_pos)
     r = (r1 * vrr + l1 * vlr) >> 15;
     ssat32_to_16(l);
     ssat32_to_16(r);
+    l = (short)v, r = (short)(v >> 16);
     if (spu.spuCtrl & CTRL_CD)
     {
      SSumLR[ns+0] += l;

If that doesn't help, can you set l and r to rand() to and see if that gives noise? Also check if that code gets called at all.

pcercuei commented 5 months ago

Looks like here I always fall into the

if ((spu.cdv.ll | spu.cdv.lr | spu.cdv.rl | spu.cdv.rr) == 0) {
...
}

at the top of MixCD.

pcercuei commented 5 months ago

Ok found it, it's because you added a new SPUsetCDvol and CubeSX needs to wire it somehow.

(No idea what it was calling then instead of that function, because I wasn't having any linker errors...)

notaz commented 5 months ago

It's the silly PSEMU plugin interface that calls everything through function pointers with automatic fallback to empty functions. I was thinking to get rid of them but standalone still has possibility to change GPU ans SPU plugins using .so files on runtime so it all is still there.