Misaligned addresses for `pDSPSettings` in armhf/mono.

JohnnyonFlame commented 2 years ago

This causes a SIGSEGV in Bleed 2 (armhf, mono git e1002857) trying to deref the aliased pointers to the ListenerVelocityComponent and EmitterVelocityComponent members of pDSPSettings in CalculateDoppler.

at <unknown> <0xffffffff>
at FAudio:FACT3DCalculate <0x00043>
at Microsoft.Xna.Framework.Audio.Cue:Apply3D <0x000db>
at LoopingCue:set_LeftToRightPan <0x0021f>
at LoopingCue:.ctor <0x0015f>
at Bleed_II.IJCAudioEngine:CreateLoopingCue <0x001cb>
<...>

I am unsure what mechanism causes this to happen, but a few things have successfuly worked around this crash:

Setting Pack = 4 on related classes. (ABI breakage :/)
Casting listenerVelocityComponent, emitterVelocityComponent and DopplerFactor to float __attribute__((aligned(1))). (Absolutely evil...)
Adding [StructLayout(LayoutKind.Sequential, Pack = 4)] to SoundBank.

p.s.: The game still presents a ringing sound artifact during the first level - the same artifact is also reproduced on an unaltered build on AArch64 where the crash doesn't happen. Couldn't reproduce the artifact on amd64 tho, but also didn't investigate it yet.

flibitijibibo commented 2 years ago

This sounds like a Mono issue - it wouldn't be the first time types weren't being marshaled correctly.

That said if it works on AArch64 I would focus on that, even RPi finally moved to 64-bit recently. I don't think we'll ever support armhf on our end.

flibitijibibo commented 2 years ago

Oh, and I would check the sound with a build where NEON is disabled - you can turn this off easily in src/FAudio_internal_simd.c.

JohnnyonFlame commented 2 years ago

This sounds like a Mono issue - it wouldn't be the first time types weren't being marshaled correctly.

That said if it works on AArch64 I would focus on that, even RPi finally moved to 64-bit recently. I don't think we'll ever support armhf on our end.

It's still not great that somehow Mono is deciding to misalign allocated instances at random, despite AArch64 not crashing with it. Trying to find what causes it, but this one seems a bit of an esoteric bug.

Oh, and I would check the sound with a build where NEON is disabled - you can turn this off easily in src/FAudio_internal_simd.c.

forced NEED_SCALAR_CONVERTER_FALLBACKS to 1 and removed the NEON path, no dice.

I've recorded a sample of what it sounds like: https://www.youtube.com/watch?v=hhpyqHnPXI4 (see around the end of the video for reference)

flibitijibibo commented 2 years ago

If it's ARM-only and scalar doesn't work that could be a problem further down the stack than us. SDL_AUDIODRIVER=disk might show that it's the sound driver, and a Valgrind run would reveal references to uninitialized memory in the mixer.

JohnnyonFlame commented 2 years ago

Valgrind is full of errors on libmali (as is expected of this garbage) but that's about it.
With SDL_AUDIODRIVER=disk, playing raw F32/48khz on Audacity shows a bit of a different artifact, instead of ringing, one of the channels starts popping and goes mute, much like you would if you had a bad contact on your headphone's connection.
adding "return;" to apply3D calls seems to have no effect.

I'm not sure if it's music or sustained sound related, but disabling music also makes the sound seemingly behave.

flibitijibibo commented 2 years ago

I'm not sure if I trust any system running on Mali drivers - if there's a Mesa driver try that first, otherwise try running with llvmpipe instead. Mobile drivers are bad enough to where I would believe that a driver is corrupting host memory without causing a crash.

JohnnyonFlame commented 2 years ago

I have the same behavior as SDL_AUDIODRIVER=disk on a Raspberry Pi 4, Mono JIT compiler version 5.18.0.240 (Debian 5.18.0.240+dfsg-3 Sat Apr 20 05:16:08 UTC 2019), with the following gpu drivers:

FNA3D Driver: OpenGL
OpenGL Renderer: V3D 4.2
OpenGL Driver: OpenGL ES 3.1 Mesa 19.3.2
OpenGL Vendor: Broadcom
MojoShader Profile: glsles

Here's a video reference, captured w/ OBS via a HDMI capture card: https://www.youtube.com/watch?v=mWm_0VEBHDg

flibitijibibo commented 2 years ago

The only thing I'm aware of that can affect an entire mix like that is the reverb, which is all scalar: https://github.com/FNA-XNA/FAudio/blob/master/src/FAudioFX_reverb.c

Note that other AArch64 devices like the Switch are already using the current revision successfully, so I would check any other ARM devices available. The facttool may also be useful for playing back Cues without the rest of the game running.

JohnnyonFlame commented 2 years ago

On the original issue, the struct is being reordered in a way that "optimizes" the layout for least space, which ends up throwing dspSettings out of alignment. This patch (or just setting [StructLayout(LayoutKind.Sequential)] so engine forces it into alignment) fixes the alignment for me:

diff --git a/src/Audio/SoundBank.cs b/src/Audio/SoundBank.cs
index f15b6c5..940592e 100644
--- a/src/Audio/SoundBank.cs
+++ b/src/Audio/SoundBank.cs
@@ -18,6 +18,7 @@ namespace Microsoft.Xna.Framework.Audio
        public class SoundBank : IDisposable
        {
                #region Public Properties
+               internal FAudio.F3DAUDIO_DSP_SETTINGS dspSettings;

                public bool IsDisposed
                {
@@ -40,7 +41,6 @@ namespace Microsoft.Xna.Framework.Audio
                #region Internal Variables

                internal AudioEngine engine;
-               internal FAudio.F3DAUDIO_DSP_SETTINGS dspSettings;

                #endregion

The only thing I'm aware of that can affect an entire mix like that is the reverb, which is all scalar: https://github.com/FNA-XNA/FAudio/blob/master/src/FAudioFX_reverb.c

One of the APIs called in Bleed 2 that doesn't seem to be called in the titles where sound is okay is FAudioVoice_SetFilterParameters, I set it to return early and all I can hear ringing now is my tinnitus, so I am guessing it might be something to do with the lo/hi pass filters being used for the slowdown mechanic in the game?

Note that other AArch64 devices like the Switch are already using the current revision successfully, so I would check any other ARM devices available. The facttool may also be useful for playing back Cues without the rest of the game running.

The muting is present in all the devices I have to test. Gameforce Chi, RG351P (Rockchip RK3326), RG552 (RK3399), Amlogic s922x (Odroid N2+) and the Raspberry Pi 4. The PI4 is the only one that gets muted with no ringing, all the others have the ringing.

Tested under both armhf and aarch64 targets. I'll bisect during the next coffee break.

flibitijibibo commented 2 years ago

Definitely file that as a Mono bug, the alignment of the structure in the C# class should have no impact on what gets passed to native code.

The filter is again all scalar and comes directly from the XAudio2 specification, so it may actually be the build itself... does clang come up with something different?

The filter: https://github.com/FNA-XNA/FAudio/blob/master/src/FAudio_internal.c#L599-L637

JohnnyonFlame commented 2 years ago

clang-9, same ringing artifact. Different tuning flags and all.

flibitijibibo commented 2 years ago

You may have to record x86_64 and Aarch64 with SDL_AUDIODRIVER=disk and try to compare the sample frames where it fails. From there the only thing you can do is try to step through the mixer thread at that approximate time and try to break when the samples stop making sense. The thread mostly lives inside src/FAudio_internal.c.

JohnnyonFlame commented 2 years ago

I just managed to reproduce on amd64 - it takes far longer to happen, and similar to the RPI4 there's no ringing, I left my character long enough on the first level lobby, and after a while the audio cuts off.

xUbuntu 20.04.3 LTS, FAudio tag 22.01, SDL2.0.16

flibitijibibo commented 2 years ago

I believe the only thing running in the lobby is music, and the only thing unique to music is the filter. When it cuts out, does slowing down time fix the issue temporarily?

JohnnyonFlame commented 2 years ago

Yes, but only when the audio effect is on in the settings. This seems like some sort of runoff.

flibitijibibo commented 2 years ago

https://github.com/FNA-XNA/FAudio/blob/master/src/FACT_internal.c#L1634-L1638

Something about either Frequency or OneOverQ is wrong, but I don't know what that would be. Frequency is [0, 1], OneOverQ is [0, 1.5]. Bleed 2's latest update uses FAudio 21.01 and I haven't gotten any audio complaints so that would be a starting point to bisect.

flibitijibibo commented 2 years ago

I dumped Bleed 2's filter values and the frequency is normal, the Q factor technically is as well but it's the only value that's non-default. This might work:

diff --git a/src/FACT_internal.c b/src/FACT_internal.c
index 63b191c..ba36ba6 100644
--- a/src/FACT_internal.c
+++ b/src/FACT_internal.c
@@ -1631,6 +1631,13 @@ uint8_t FACT_INTERNAL_UpdateSound(FACTSoundInstance *sound, uint32_t timestamp)
                        {
                                filterParams.OneOverQ = sound->tracks[i].activeWave.baseQFactor;
                        }
+
+                       /* If we let this hit 1.5 the sound degrades after a long time.
+                        * If we find an XAudio2 game that actually depends on 1.5, move
+                        * this clamp to FAudio.c!
+                        */
+                       filterParams.OneOverQ = FAudio_min(filterParams.OneOverQ, 1.499995f);
+
                        FAudioVoice_SetFilterParameters(
                                sound->tracks[i].activeWave.wave->voice,
                                &filterParams,

JohnnyonFlame commented 2 years ago

I dumped Bleed 2's filter values and the frequency is normal, the Q factor technically is as well but it's the only value that's non-default. This might work:

It's far less pronounced, and it doesn't outright mute the audio, but it still produces an odd high pitched noise and a few pops. It's even less pronounced on 44100hz.

flibitijibibo commented 2 years ago

Looked at the location of the value and it's the original value according to the XACT data. So, this may be what we're stuck with:

diff --git a/src/FACT_internal.c b/src/FACT_internal.c
index 63b191c..1e91555 100644
--- a/src/FACT_internal.c
+++ b/src/FACT_internal.c
@@ -463,6 +463,14 @@ void FACT_INTERNAL_GetNextWave(
                        track->frequency,
                        cue->parentBank->parentEngine->audio->master->master.inputSampleRate
                );
+
+               /* FIXME: For some reason the 0.67 Q Factor causes problems, but it's also
+                * the only valid value above 1.0 is 1.5 so just clamp it for now.
+                */
+               trackInst->upcomingWave.baseQFactor = FAudio_min(
+                       trackInst->upcomingWave.baseQFactor,
+                       1.0f
+               );
        }

        /* Try to change loop counter at the very end */

JohnnyonFlame commented 2 years ago

This seems to do it, thanks!

flibitijibibo commented 2 years ago

Applied to master.

FNA-XNA / FAudio

Misaligned addresses for `pDSPSettings` in armhf/mono. #282