Record inited SDL subsystems and use this info during uninit

[ivan-mogilko]

This is done after comments in #2166. Unfortunately I realized the problem too late, after merging the pr.

Keep record of the SDL subsystems we init in a static variable inside sys_main, and use this info when shutting SDL subsystems down. The idea is that this is incapsulated in sys_main, so that external code would not interfere with this.
Somewhere in the future we could remake sys_main into a singleton class, for instance.

EDIT: i have a deja vu, because this looks like something I was trying to add a while ago... but maybe this was in a different program.

@ericoporto could you please also try that this still works as expected?

This should be applied to a 3.6.0 branch too.

[ivan-mogilko]

Erm, also, why do we init "game controller" in 3.* if the engine has no support for controllers or joysticks in this version? I suppose plugins that implement joystick should init this by themselves?

[ericoporto]

Erm, also, why do we init "game controller" in 3.* if the engine has no support for controllers or joysticks in this version?

I didn't knew why, since the code was like this I just assumed there was some reason you knew. If there's is no need for it in 3.x just remove it! It's necessary on 4.x because of the gamepad API.

Btw, I think whatever we use at init the subsystems, quitting the subsystems should be the same call just reversed in order. The reason is if you look how the init subsystem is implemented it will initialize and ref count some things more than what you ask through the flags (I think Event increments for whatever system you ask, and some also increment the timer, controller will increment joystick, ...) - the way it works is if it's already initialized it simply increments the ref count. So if the calls are not mirrored a system may not get sufficient decrements.

In theory, in SDL_Quit it takes care of things that aren't properly released before, so maybe this doesn't matter as much. I couldn't tell if we are actually meant to use SDL_Quit or not, but I think it's necessary in the non-desktop ports.

[ivan-mogilko]

I didn't knew why, since the code was like this I just assumed there was some reason you knew.

Oh. Maybe this was just in original sonneveld's draft, and I copied without giving it a thought.

Btw, I think whatever we use at init the subsystems, quitting the subsystems should be the same call just reversed in order.

I probably understand what do you mean, but just in case, you mean the separate Init calls? Because order of flags in one call is inessential.

[ericoporto]

I probably understand what do you mean, but just in case, you mean the separate Init calls? Because order of flags in one call is inessential.

Yeah, separate. Just because if you do something like "init events" and then "init gamecontroller", you end up with events +2, joystick +1, gamecontroller +1, and then doing "quit (events + gamecontroller)", will now set you to events +1, joystick 0, gamecontroller 0. This is just so that the accounting of these are correct.

See: https://github.com/libsdl-org/SDL/blob/6fc17b0e2b421c6f8b996b28f76447ea0956c08e/src/SDL.c#L190-L193

This means that when the audio opens, we have also a +1 in events, so closing audio and everything else in ONE call, will actually keep a dangling +1 in events. Like...

SDL_InitSubSystem(SDL_INIT_AUDIO); // +1 audio , +1 events
SDL_InitSubSystem(SDL_INIT_VIDEO | SDL_INIT_EVENTS | SDL_INIT_TIMER); // +1 video, +1 events, +1 timer
SDL_QuitSubSystem(SDL_INIT_VIDEO | SDL_INIT_AUDIO | SDL_INIT_EVENTS | SDL_INIT_TIMER); // -1 video, -1 audio, -1 events, -1 timer
// end with 0 video, 0 audio, 1 events, 0 timer

Btw, I could only understand this after reading it's source code in SDL.c...

[ivan-mogilko]

I removed controllers in this pr, but this will have to be redone for ags4, or if we need to get controller events in 3.* for any reason.

But this is easy to do.

[ericoporto]

Hi, I added a bit of detail above. I don't know the way to go here, if it's an array that gets consumed reversely on quit or some object that wraps the init/quit to account for disposal, or something else.

The flag approach I think may hit the issue I mentioned.

Also, this all may be non-issue because I think SDL_Quit will smash everything in the end, but I think the API is meant to not rely on SDL_Quit - I can't tell what would be the issue though.

[ivan-mogilko]

Sorry, reverted back to previous branch version. I need to think this over again, because I'm unsure about this problem.

[ivan-mogilko]

So, I wrote a stack unwinding version here:
ivan-mogilko@70f2e5d

That works, but then I started to doubt if this is really necessary.
The point of this reference counting is mainly to not disable something that is used by a parallel component (like a plugin).
In the worst case, things will not get disabled until SDL_Quit, and some extra components will stay working for a while longer. I do not think that may be an issue.
Docs for SDL_Quit say that you should call SDL_Quit anyway:
https://wiki.libsdl.org/SDL2/SDL_Quit

The reason why I don't want to write a more complex code for this is that this case is not about some dynamically allocated and unallocated objects, but something that is inited and uninited only once per engine run, at the very start and very end.

Also, frankly, the situation you describe above looks like a bug in SDL2, and perhaps may be reported to get fixed.
Because it means that SDL2 itself breaks its own ref counting.

[ericoporto]

wrote a stack unwinding version here

I don't understand that flag that you try to do to avoid already initialized systems, but that looks wrong it would be just the push to the stack as it will increment the ref count if it initializes successfully OR if it's already initialized. That looks almost correct, if you delete that.

Docs for SDL_Quit say that you should call SDL_Quit anyway

We can just leave at that then. Maybe the quit of subsystems is only relevant for libraries or if you want to do some sort of check after the exit of a system but before ending the application.

[ivan-mogilko]

I've been investigating and testing this issue with refcounts that you found, and realized that things may be worse, as there's also an opposite scenario which may lead to subsystems removed too early.
I wrote a ticket in SDL2's repo, for I now believe it's a serious error:
libsdl-org/SDL#8381

I think this will not be a problem for us at the moment, because we only have 1 subsystem which may be turned on and off separately (AUDIO), but things may become inconvenient if we have more of these.