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

Invalid SDL_AUDIODRIVER environment variable may prevent audio to initialize #1692

Closed
ericoporto opened this issue Jun 18, 2022 · 8 comments · Fixed by #1747
Closed

Invalid SDL_AUDIODRIVER environment variable may prevent audio to initialize #1692

ericoporto opened this issue Jun 18, 2022 · 8 comments · Fixed by #1747
Labels
backend: sdl2 related to sdl2 library context: audio type: bug unexpected/erroneous behavior in the existing functionality
Milestone

Comments

@ericoporto
Copy link
Member

Describe the bug
If you set an environment variable SDL_AUDIODRIVER to an invalid value, AGS may not initialize, due to SDL complaining the system does not exist.

AGS Version
Current master (5c3a5ea).

Game
It can happen in any game.

To Reproduce
Steps to reproduce the behavior:

  1. On Windows, click on System Properties
  2. Then on Environment Variables and then create a new one name SDL_AUDIODRIVER and set it to an invalid value (say, dsound2).
  3. Start AGS, it will pop-up an error message

Expected behavior
The expected behavior is for AGS to figure a good known driver and start anyway.

Desktop:

  • OS: Windows
  • Version 10

Additional context
I am not sure if this is an AGS bug, so I opened it upstream too libsdl-org/SDL#5818

@ivan-mogilko ivan-mogilko added this to the 3.6.0 milestone Jun 18, 2022
@ericoporto
Copy link
Member Author

ericoporto commented Jun 18, 2022

So they only adjusted the case of dsound, but not being able to not fail, not sure how to approach here, but AGS used to not fail and instead figure out a valid audio driver.

I also am not fully sure, but I believe we are initializing the SDL Audio twice, once in sys_main and another time in audio_core, through MojoAL. The MojoAL initialization also pass an additional information describing what the driver must have.

The following code is wrong, but I had this idea of iterating to find a working audio driver, but this would have to happen in audio_core in case my previous observation is correct.

Of course, we may also decide this is a non issue and simply close this.

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jun 18, 2022

I also am not fully sure, but I believe we are initializing the SDL Audio twice, once in sys_main and another time in audio_core, through MojoAL. The MojoAL initialization also pass an additional information describing what the driver must have.

MojoAL passes the information when it initializes device, not driver, these are different things. For the device initialization it passes its name (optional), frequency, etc.

For more information:
https://wiki.libsdl.org/SDL_AudioInit <--- this is called by the engine at startup
https://wiki.libsdl.org/SDL_OpenAudioDevice <--- this is called by mojoal when creating context

EDIT: in mojoAl's code I can see a call to SDL_InitSubSystem(SDL_INIT_AUDIO), but not SDL_AudioInit, so likely it is supposed to use default driver if one was not specified earlier. I don't think SDL2 will reinitialize an audio driver for the second time (from documentation it seems that if subsystem is ready, then SDL2 should just increase subsystem's ref count and skip), but this may be double checked through code or debugging of course.

@ivan-mogilko ivan-mogilko added type: bug unexpected/erroneous behavior in the existing functionality context: audio backend: sdl2 related to sdl2 library labels Jun 19, 2022
@ericoporto
Copy link
Member Author

ericoporto commented Jun 21, 2022

I don't think SDL2 will reinitialize an audio driver for the second time (from documentation it seems that if subsystem is ready, then SDL2 should just increase subsystem's ref count and skip)

This is correct from the documentation, indeed. But this is what is weird, if I set an environment variable SDL_AUDIODRIVER with an invalid value (like dsound2), and set the audio driver to WASAPI in winsetup, I get the following log:

...
Audio driver: wasapi
Failed to initialize audio system: AudioCore: error opening device
Audio is disabled
...

I followed this with the debugger and turns out SDL_WasInit internally returns false and it tries to initialize the audio subsystem again. Need to check with current master of SDL2 to see if this bug was fixed or not...

Edit: it's not. Maybe they don't consider it a bug. Perhaps we should override using something like SDL_setenv(SDL_AUDIODRIVER, "DRIVER NAME") instead, and then hope SDL_InitSubsystem uses the driver that was set in this way.

I opened an additional upstream bug regarding this specifically to see if this is intended behavior or not: libsdl-org/SDL#5841

@ericoporto
Copy link
Member Author

ericoporto commented Jul 22, 2022

Apparently the result is just never use SDL_AudioInit. Maybe we need to override those weird environment variables of sdl2 with some value for each driver and clean it when selecting AUTO.

I opened an additional issue in MojoAL to see if there's something specific in MojoAL I am missing: icculus/mojoAL#17

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jul 22, 2022

@ericoporto tbh I'm getting confused by this issue thread. The original problem was that AGS fails to init if the requested driver id is not valid (not recognized by SDL). But is the mojoAl's problem is a separate one? If so, may it be opened as a separate ticket? Because frankly I still dont fully understand the problem with mojoAL, even after reading the linked issues. Maybe some details are missing in the description.

Was the original problem solved though?

@ericoporto
Copy link
Member Author

if (!driver_name.IsEmpty())
{
res = SDL_AudioInit(driver_name.GetCStr()) == 0;
if (!res)
Debug::Printf(kDbgMsg_Error, "Failed to initialize audio driver %s; error: %s",
driver_name.GetCStr(), SDL_GetError());
}

These lines are wrong and instead it should write an empty SDL_setenv or the specific driver name selected. SDL_AudioInit should not be used since it's not ref counted.

Was the original problem solved though?

In SDL 2.24, it will accept dsound as a valid spec of directsound for SDL_AUDIODRIVER environment variable which previously caused AGS to not load. This environment variable is used in SDL 1.

@ivan-mogilko
Copy link
Contributor

ivan-mogilko commented Jul 22, 2022

EDIT: sorry, I have to gather this from multiple comments, because I already forgot what the problem was. Re-reading everything once more: the problem is that SDL_AudioInit does not set a flag inside SDL2 saying that audio system is initialized, and so when MojoAL wants to initialize it (using SDL_InitSubSystem), SDL2 cannot tell that audio is ready, so it tries to initialize again, potentially a different driver id, taken from the enviromental variable.

So, I have got a question. If we must use SDL_InitSubSystem to let SDL2 set the ref count, and mojoAl detect that, then may we do this in sys_main:

  • set enviroment variable (SDL_AUDIODRIVER, is it?)
  • call SDL_InitSubSystem(SDL_INIT_AUDIO)
  • on failure - retry with every other known "valid" device, like you suggested some time ago.

Will this work?

EDIT: Hmm, maybe this is precisely what you were suggesting above.

@ivan-mogilko
Copy link
Contributor

@ericoporto opened a pr: #1747

Note, I found there's no need to have your own list of drivers, as SDL2 already tries this list if the env SDL_AUDIODRIVER is null.
If we still need to do this manually, we may use SDL_GetNumAudioDrivers / SDL_GetAudioDriver(index) to get this exact list (https://wiki.libsdl.org/SDL_GetAudioDriver).

(Of course if above list is bad for some reason, in that case we could still have our own, as a last resort, but hopefully that won't be necessary)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: sdl2 related to sdl2 library context: audio type: bug unexpected/erroneous behavior in the existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants