Another game that also exhibits clicking effects when these new NEON converters are active is crispy-doom:

https://github.com/fabiangreffrath/crispy-doom.git

I've disabled the NEON code for now with https://hg.libsdl.org/SDL/rev/dfa5cea01a56 but I will revisit this once 2.0.9 ships.

--ryan.

Is there a small sample program that demonstrates this with a particular sound file? These converters were recently used in Dust: AET's Switch version and it ran just fine... we only used some of the converters, however (S16->F32 and F32->S16).

Created attachment 3534 [details]
patch

Here's a patch that probably fix the issue, but need confirmation as there are several things !

Happens with 8bit wav, while doing:
SDL_Convert AUDIO_U8 -> AUDIO_F32 (using NEON) (SDL_Convert_U8_to_F32_NEON)
SDL_Convert AUDIO_F32 -> AUDIO_S16 (using NEON) (SDL_Convert_F32_to_S16_NEON)


1/ 
"SDL_Convert_U8_to_F32_NEON()" has a blatant issue: mis-placed parameter "one":
vmlsq_f32(a, b, c) is doing r=(a - b * c)
So  vmlsq_f32(a, b, one), should be in fact: vmlsq_f32(one, b, c)

2/ For "SDL_Convert_U8_to_F32_NEON()" and "SDL_Convert_U16_to_F32_NEON()"
formula seems incorrect as it performs: (1 - src * DIVBY128 ) instead of (src * DIVBY128 - 1)

It should use "-1" and "vmlaq_f32"
vmlaq_f32(a, b, c) is doing r=(a + b * c)
to do ( -1  + src * DIVBY128 )

Created attachment 3535 [details]
patch

Update the patch to rename "one" -> "negone"

Also: 

Android-x86 can probably run SSE2 (at least __SSE2__ is defined and it compiles. it runs also on emulator, but I have not device to try it).

Android armeabi-v7a could also run those NEON cvt, but it needs also the fallback!

@Manuel Alfayate Corchete, could you try the patch ?

We ended up merging this patch into FAudio's copy of the U8->F32 converter:

https://github.com/FNA-XNA/FAudio/commit/68b13c1ff04784e31ffffd51d147cb890fe88803

AFAIK this works, but I don't have the original reporter's data to check with.

I am sorry about not testing this sooner, I have been very busy!
But I will do so today or tomorrow: I just have to build rawgl with SDL_Mixer support again (As a side note: I have ported it to sts_mixer instead, since SDL_Mixer is very slow on the Pi3 because of the SLOOOOOW SDL_BuildAudioCVT() that is called inside Mix_LoadWAV_RW()).

No problem, but let us know!

Maybe this could be added to next 2.10 version ?

I'm applying this patch, these fixes look correct to me. If it's a problem, we can reopen this bug!

--ryan.

Hi,

I was testing some games with 2.0.10 today, and I found that this ARM_NEON resampling code is still a big problem. It causes massive audio glitches in devilutionX (Diablo 1 engine using SDL2: https://github.com/diasurgical/devilutionX), and ECWolf, where the closing door audio FX is also broken.
In both cases, the audio is "robotic" and strange.

How to reproduce the issue:
-Build devilutionX against stable 2.0.10 (I cant see any changes in SDL2 repo regarding SDL2 resampling code)
-Talk to any NPC and you will see audio is metallic and robotic, very evidently broken.
-Now, rebuild SDL2 after editing src/audio/SDL_audiotypecvt.c and changing
#define HAVE_NEON_INTRINSICS 1
into
#define HAVE_NEON_INTRINSICS 0
You will see that *audio without HAVE_NEON_INTRINSICS is perfectly correct*.

So I am reopening this issue, as its still clearly unresolved.

Maybe you can upload the sound file, and/or also add some trace in SDL code to see which conversion path is taken

@Sylvain

I have tried recording the ALSA output on the Raspberry PI so I could provide both the traces and file, but I was unable to do so. I tried redirecting the ALSA output to the alsa snd-loop device after building a kernel with this module, then recording from it with arecord. All I got was a blank, silent file.

Anyway, the audio conversion that results in highly broken audio goes thru these two functions each time a sound has to be converted (both functions are called for each single sound conversion):

SDL_Convert_U8_to_F32_NEON
SDL_Convert_F32_to_S16_NEON


This call sequence happens both in DevilutionX and ECWolf, which shows the wrong sounds when NEON_INTRINSICS is active, too.

Some sounds seem affected, and others are unaffected, resulting in good audio. The same call sequence is done for affected and for unaffected sounds.

there is not just the sound file in your game data ?

to debug, since you have the test-case ...

can you replace/copy paste the implementation of 

SDL_Convert_U8_to_F32_NEON
SDL_Convert_F32_to_S16_NEON

by the non NEON, eg:

SDL_Convert_F32_to_S16_scalar
SDL_Convert_F32_to_S16_scalar

so that we hopefully know which one has the issue

by the way the previous patch was applied here
https://hg.libsdl.org/SDL/rev/30204ee1c7c5

@Sylvain:

There is no independent audio data file for the game because Diablo 1 stores all its data in a single file, called diabdat.mpq, which is 500MB. DevilutionX engine uses this data file, and voices are inside it.
-The open source DevilutionX engine is here:
https://github.com/diasurgical/devilutionX
-The diabdat.mpq datafile is inside this ISO you can legally get from archive.org:
https://archive.org/download/Diablo_1996_Blizzard/Diablo%20%281996%29%28Blizzard%29.iso

As for what function could be the problematic one, I did this:
-I swapped the SDL_Convert_U8_to_F32_NEON code with SDL_Convert_U8_to_F32_Scalar.
Voices and FX sounded good!
-Restored original SDL_Convert_U8_to_F32_NEON and then swapped the SDL_Convert_F32_to_S16_NEON code with SDL_Convert_F32_to_S16_Scalar.
Voices and FX sounded wrong again!

So SDL_Convert_U8_to_F32_NEON seems the function to blame, because swapping it with the scalar imlpementation fixes sound problems.

Ok, I think it's the order the sample were stored which was bad:
 https://hg.libsdl.org/SDL/rev/644eeed7ce2f
let me know if this fixes it?


I think it should also be the same to S8_to_F32 ?
maybe also S/F 16_to_F32 ?

and also, I wonder if the NEON checks for src aligned are important: 

/* Make sure src is aligned too. *
if ((((size_t) src) & 15) == 0)
there were false  in my case ?!

> and also, I wonder if the NEON checks for src aligned are important:

I think whenever that's false it just means it can't do any NEON operations at all, so it just falls back and does everything in scalar at the bottom of the function. Ideally you'd have your input/output memory be aligned, but it's not required.

@Sylvain: Ah, yes, it works! Those changes fix the audio problems in DevilutionX and ECWolf. You found the solution. Great!

It is pity the recently released 2.0.10 has inherited the problem, which is pretty much a major audio problem. I suppose its partly my fault for not testing more SDL2 software earlier in the 2.0.10 release cycle...

> It is pity the recently released 2.0.10 has inherited the problem, which is pretty much a major audio problem. I suppose its partly my fault for not testing more SDL2 software earlier in the 2.0.10 release cycle...

For what it's worth, it's all the more reason to enforce the new release cycle!

https://libsdl.org/next.php

We've integrated this into FAudio as well, and nothing appears to have fallen apart:

https://github.com/FNA-XNA/FAudio/commit/a916bf05edf18668748430b994197bca23fabe08

I've dump all outputs of all converting functions, and SDL_Convert_S8_to_F32_NEON was also failing, now fixed in
https://hg.libsdl.org/SDL/rev/33b64882e322

beside, all seems quite clean for NEON. Only sometimes saturation values changes between scalar/neon: 1/0/255/254, but some are expected.

(In reply to Ethan Lee from comment #19)
> > and also, I wonder if the NEON checks for src aligned are important:
> 
> I think whenever that's false it just means it can't do any NEON operations
> at all, so it just falls back and does everything in scalar at the bottom of
> the function. Ideally you'd have your input/output memory be aligned, but
> it's not required.

NEON instructions can operate on aligned or not addresses. When they are aligned, it's simply faster. (unlike SSE where there are aligned/non-aligned instruction for aligned/non-aligned memories). So I would just always do the NEON instruction as long as there are enough samples.

And if src and dst must be the same buffer and have to be aligned at the same time, it might be difficult to happen. I haven't checked all the condition and the code is a little bit confusing ...

I also double-check SSE2 conversion and spot this one, constant was set to 1, instead of -1
https://hg.libsdl.org/SDL/rev/69cc269eec53

Created attachment 3928 [details]
dump audio conversion datas

Attached patch to dump and compare audio datas

Marked as fixed!

@Sylvain: Thanks A LOT for your work on this. SDL2 on ARM is a wonder and it deserves to be cared about!