Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsigned audio formats output wrong silence value #3401

Closed
SDLBugzilla opened this issue Feb 11, 2021 · 0 comments
Closed

Unsigned audio formats output wrong silence value #3401

SDLBugzilla opened this issue Feb 11, 2021 · 0 comments

Comments

@SDLBugzilla
Copy link
Collaborator

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: 2.0.10
Reported for operating system, platform: Windows 10, All

Comments on the original bug report:

On 2019-09-17 12:18:28 +0000, Florian Behr wrote:

Created attachment 3971
repro case

When creating an audio device on Windows 10 with an unsigned sample format sdl produces an audio "pop" and whining noise instead of silence.

The simple reproduction case is attached, and an audacity screencap showing the problem can be seen here: https://imgur.com/a/08VEVNK

the problem appears to be that in the process of initializing the audio device the silence value does not get set properly on the SDL_AudioSpec, but there are actually two different bugs for U8 and U16.

The U16 case is the simpler one, the silence value is set in SDL_audio.c:SDL_Calculate_AudioSpec(). U16 formats are just not even considered here.

The U8 case is a bit more complex. The U8 silence value is initially set correctly in SDL_audio.c:SDL_Calculate_AudioSpec() called in SDL_audio.c:prepare_audiospec():1210, and passed out of the function to the user correctly, but is then internally mangled during WASAPI initialization. The SDL_AudioSpec that is used internally from this point on is a copy of the one passed back out to the user.

At SDL_wasapi.c:WASAPI_PrepDevice():564 the SDL format on the SDL_AudioSpec is replaced for what appears to be a WASAPI native format value, although I'm not entirely sure of that. But whatever the new value signifies it is different from the SDL defined AUDIO_U8.

Then later at SDL_wasapi.c:WASAPI_PrepDevice():615 there's another call of SDL_audio.c:SDL_Calculate_AudioSpec() to recalculate potential changes, but since at this point the format value of the SDL_AudioSpec is not recognized by the switch statement, the silence value is set to the default 0 for signed formats, instead of the correct 128. This faulty silence value is then used for filling samples sent to the audio device, and presumably produces the whining noise as it forces the speaker into holding and end-position.

On 2019-09-20 20:47:33 +0000, Ryan C. Gordon wrote:

We're changing how we do SDL release versions; now releases will be even numbers (2.0.10, 2.0.12, etc), and as soon as we tag a release, we'll move the internal version number to an odd number (2.0.12 ships, we tag the latest in revision control as 2.0.13 immediately, which will become 2.0.14 on release, etc).

As such, I'm moving the bugs tagged with target-2.0.11 to target 2.0.12. Sorry if you get a lot of email from this change!

Thanks,
--ryan.

On 2019-09-20 20:48:44 +0000, Ryan C. Gordon wrote:

We're changing how we do SDL release versions; now releases will be even numbers (2.0.10, 2.0.12, etc), and as soon as we tag a release, we'll move the internal version number to an odd number (2.0.12 ships, we tag the latest in revision control as 2.0.13 immediately, which will become 2.0.14 on release, etc).

As such, I'm moving the bugs tagged with target-2.0.11 to target 2.0.12. Sorry if you get a lot of email from this change!

Thanks,
--ryan.

On 2019-09-25 19:24:49 +0000, Ryan C. Gordon wrote:

(In reply to Florian Behr from comment # 0)

The U16 case is the simpler one, the silence value is set in
SDL_audio.c:SDL_Calculate_AudioSpec(). U16 formats are just not even
considered here.

We have a deeper problem, though: SDL_AudioSpec::silence is a Uint8. It can't fit the silence value for Uint16 (0x8000), and this value is generally used with memset() out in the wild anyhow, so even if we could change SDL_AudioSpec--we can't without breaking binary compatibility--it wouldn't be useful to do so.

I propose that we just report the silence value as 0x80 for both U8 and U16. For U8, it'll be perfect silence, and for U16, it's...close enough (samples will clear to 0x8080, which is 32896, which is 128 above silence (or 0.1953 percent above silence). It's not perfect, but it'll do, I suppose.

For 2.1, we can change how the silence field works, or maybe just remove U16 support.

--ryan.

On 2019-09-25 19:54:37 +0000, Ryan C. Gordon wrote:

I propose that we just report the silence value as 0x80 for both U8 and U16.
For U8, it'll be perfect silence, and for U16, it's...close enough (samples
will clear to 0x8080, which is 32896, which is 128 above silence (or 0.1953
percent above silence). It's not perfect, but it'll do, I suppose.

This is how it works in https://hg.libsdl.org/SDL/rev/599ed6e984d2, now. I'm still looking at the WASAPI issue.

--ryan.

On 2020-01-21 21:37:20 +0000, Ryan C. Gordon wrote:

(In reply to Ryan C. Gordon from comment # 4)

This is how it works in https://hg.libsdl.org/SDL/rev/599ed6e984d2, now. I'm
still looking at the WASAPI issue.

This appears to work on WASAPI now, either through the previous fix, or something else, or a misunderstanding (among other things, internally, SDL keeps an AudioSpec for what it wants to feed the hardware and what it expects the callback to feed to SDL: if you were looking at device->spec and saw silence is 0, that was correct, because WASAPI wants float32 data, but device->callbackspec should have a silence of 0x80, since the callback is feeding us Uint8 data).

While tracking this down, though, I noticed that SDL_LoadWAV doesn't set the silence fields correctly for the loaded file, always initializing it to zero, which is now fixed in https://hg.libsdl.org/SDL/rev/f0750959bb29 .

If I've misunderstood, please reopen this bug, but I think we're good to go otherwise.

--ryan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant