We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 4210

Summary: [Patch] SSE2-based converter makes junk result of S32 -> Float
Product: SDL Reporter: Vitaly Novichkov <admin>
Component: audioAssignee: Ryan C. Gordon <icculus>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: major    
Priority: P2 CC: sezeroz
Version: HG 2.1   
Hardware: x86_64   
OS: All   
Attachments: Patch which fixes correctess of SSE2 S32 to F32 sound conversion

Description Vitaly Novichkov 2018-07-01 16:17:18 UTC 
At the HG state e604fe493d45, 64-bit assemblies are using SSE2-based resampler, produces junk sound when converting the S32 -> Float32 -> S16 chain. The `NEED_SCALAR_CONVERTER_FALLBACKS` thing works perfectly.

If I will find a reason that caused this mistake, I'll send a patch by myself.
Comment 1 Vitaly Novichkov 2018-07-02 00:43:50 UTC 
Okay, I did the analysis of input data and I began to log source and final results of SDL_Convert_S32_to_F32_SSE2's function, and what I have next:

SRC: 1053578242 RES: 1.970398

Then, I did a manual calculation of the value:

(1053578242>>8) * 0.00000011920930376163766

and I got next result: 0.490610658

So, it's a pobably was used some incorrect SSE2 call which was produced the incorrect result in final.
Comment 2 Vitaly Novichkov 2018-07-02 00:50:50 UTC 
Okay, as I never expired in SSEx, I looked for the documentation that describes SSE2 calls.

I have found next:

"Intrinsic",    | "Operation"  | "Shift Type" | "Corresponding Instruction"

_mm_srli_epi32  | "Shift right"| "Logical"    | "PSRLD"    <-- Was used in a code

_mm_srai_epi32  | "Shift right"| "Arithmetic" | "PSRAD"

I did replaced `_mm_srli_epi32` with `_mm_srai_epi32` in the 536'th line of SDL_audiotypecvt.c and everything became fine!

Gotta go make the patch for this...
Comment 3 Vitaly Novichkov 2018-07-02 00:55:08 UTC 
Created attachment 3267 [details]
Patch which fixes correctess of SSE2 S32 to F32 sound conversion
Comment 4 Vitaly Novichkov 2018-07-02 01:17:05 UTC 
P.S. For a future, make the unit test that generates random data for each test. Copy generated buffer into second copy. Convert the first copy with the scalar converter which gives 100% correct result. Then, convert the second copy of original with SSE2 (or also with ARM NEON on a different unit test). After this run, compare both results, and if it's a difference between of them, fail the test.
Comment 5 Ryan C. Gordon 2018-07-04 17:00:46 UTC 
This patch is now https://hg.libsdl.org/SDL/rev/523e803a0192, thanks!

--ryan.
Comment 6 Vitaly Novichkov 2018-07-04 17:07:28 UTC 
Thanks! :3

Anyway, please take my tip to make the unit test to verify the correctness of optimized resamplers to avoid bugs like this in future. Otherwise, I'll create it by myself, maybe I'll catch some another bug in a place I wasn't touched yet...