Created attachment 3337 [details]
fix-audio-mem-leak.patch

This patch is dangerous:  What if the user only wants to disable
the audio subsys and there are still others doing events?

I see your point and I can agree the patch seems a bit incomplete, now that I looked closer at it. It definitely lacks the similar change in the SDL_InitSubSystem function, but I do not know how to address your concern of quitting the audio subsys in safer manner.

The simplest test case that can already show the leak is:

    SDL_Init(SDL_INIT_AUDIO);
    SQL_Quit();

It will be enough to fire the audio device registration hooks (SourceInfoCallback -> SDL_AddAudioDevice) and create a leak.

What is the SDL_AddAudioDevice used for? I could still create a stream even without events being initialised, so maybe the event creation should be skipped, unless SDL_WasInit(SDL_INIT_EVENTS)?

Nevertheless, the same case would happen in case someone had VIDEO and JOYSTICK initialised, but decided to disable one of them.

Looking forward to your comments.

Ryan and Sam can most certainly say more than I can.  However,
SDL_Quit() already does SDL_QuitSubSystem(SDL_INIT_EVERYTHING)
that is supposed to be enough for a program-quit case. How are
you hitting a memory leak from within SDL even if you do call
SDL_Quit()?

> What is the SDL_AddAudioDevice used for?

src/audio/SDL_sysaudio.h says:
/* Audio targets should call this as devices are added to the system (such as
   a USB headset being plugged in), and should also be called for
   for every device found during DetectDevices(). */

Following snippet in case of the Linux system I'm on displays 4 unfreed allocations.

#include <SDL2/SDL.h>

int main()
{
    for (int i = 0; i < 1; i++)
    {
        int init = SDL_Init(SDL_INIT_AUDIO);
        if (init < 0)
        {
            fprintf(stderr, "Failed to init SDL\n");
            return 1;
        }

        SDL_Quit();
    }
    printf("Num allocations: %d\n", SDL_GetNumAllocations());
    return 0;
}

2 of those come from SDL_PushEvent_REAL that were registered via SourceInfoCallback and SinkInfoCallback, for each iteration of the loop. You can modify the loop counter to easily see the amount of unfreed allocations going up.

Changing SDL_Init's arguments to SDL_INIT_AUDIO | SDL_INIT_EVENTS reduces that number to 2, which are not part of this bug report, and stays stable for the lifetime of application.

Perhaps each subsystem should keep an additional count of subsystem it depends on? That way you could safely disable joystick, audio and video without any of them turning off events for other subsystems.

I'm not sure anymore how my patch fixed the issue for me yesterday, perhaps I overlooked something when I was tired in the evening, as your comment regarding  SDL_QuitSubSystem(SDL_INIT_EVERYTHING) seems to cover the case in the patch.

What may have happened is that I somehow mishandled the patch, which was meant to introduce the addition of SDL_INIT_EVENTS in SDL_InitSubSystem instead of SDL_QuitSubSystem. That would be much less invasive and address the immediate problem, but still leaving the joystick|video|audio-turn-off-events problem on the table.

Created attachment 3338 [details]
fix-audio-init.patch

Fix audio init not enabling events

Looking at SDL_QuitSubSystem() more closely, your suggested fix
replicates the behavior SDL_INIT_JOYSTICK and SDL_INIT_VIDEO, and
SDL_INIT_EVENTS does perform a
		SDL_PrivateShouldQuitSubsystem(SDL_INIT_EVENTS)
check, so my initial objection seems wrong.  What I still do not
understand is how SDL_quit() doesn't clear everything.

I'm getting out of the way and leaving Ryan and Sam to comment
on things.

SDL_Init(SDL_INIT_AUDIO);
SDL_Quit();

doesn't clean up everything because the events get created and they only get cleaned up through the call to SDL_StopEventLoop() which never happens, because events were never initialised in the first place and SDL_PrivateShouldQuitSubsystem(SDL_INIT_EVENTS) only schedules cleanup if we're the last (refcounted) user of given subsystem or there are still users, but we're in SDL_Quit(). The problem here is since we never initialised the events subsys, it doesn't matter that we're in SDL_Quit(), because SDL_PrivateShouldQuitSubsystem(SDL_INIT_EVENTS) already considers events subsys closed.

With your testcase modified to:

#include "SDL.h"
int main(void)
{
    for (int i = 0; i < 4; i++)
    {
        int init = SDL_Init(SDL_INIT_AUDIO);
        if (init < 0) {
            __builtin_printf("Failed to init SDL Audio\n");
            return 1;
        }
        SDL_QuitSubSystem(SDL_INIT_AUDIO);
    }
    __builtin_printf("Num allocations (#1): %d\n", SDL_GetNumAllocations());

    SDL_Quit();
    __builtin_printf("Num allocations (#2): %d\n", SDL_GetNumAllocations());

    return 0;
}


Using your new patch, I get:
Num allocations (#1): 14
Num allocations (#2): 2

Using your new patch + your old patch, I get:
Num allocations (#1): 2
Num allocations (#2): 2

Hmm.

Your results are not surprising, keep in mind with the new patch there's a refcount enabled for events subsys, so in case of "new only" you get the behaviour we initially discussed, i.e. SDL_Quit() closes all of the subsystems, even if their refcount is not equal to 1 (i.e. we may not be the last ones calling it), even in absence of SDL_INIT_EVENTS bit in flags when issuing SDL_QuitSubSystem(SDL_INIT_AUDIO);

The old patch added the SDL_INIT_EVENTS in that latter case, which is why you see "2" in both cases, but it comes at a cost of possibly closing events subsys even when they may still be other users (video, joystick or client).