Remove sf::AudioResource

[vittorioromeo]

I am likely missing something, but it seems like sf::AudioResource is not needed anymore. These are the assumptions I followed:

1. There is only one global sf::AudioDevice;
2. The purpose of sf::AudioResource is to [de]initialize an sf::AudioDevice when needed.
3. An sf::AudioDevice is only needed when we want to play audio or music or change audio device settings (i.e. we want to access the members of sf::AudioDevice).

If so, then there's no need for sf::AudioResource, and we can just turn sf::AudioDevice into a Meyers' singleton and initialize it on first use.

I figured it'd be quicker to create a PR and have it reviewed rather than try to figure it out in a longer discussion :)

EDIT: as @JonnyPtn this doesn't allow the audio device to be destroyed and re-initialized if all sound sources are destroyed and then a new one is created later.

EDIT2: AFAICS the only way you could re-initialize a device in the past was to ensure that all your sound sources (i.e. sf::Sound and sf::SoundStream) were destroyed, and then create a new one. So you had little control over it.

We could easily add an API to this PR to actually re-initialize the audio device on demand.

EDIT3: This PR also is a step in the direction of having multiple audio devices.

[ChrisThrasher]

This is an enticing idea to me. I always love PRs that remove code. Ultimately Binary will have to make the final call since I don't understand the new Audio module well enough to confidently say whether this is a valid change to make.

[coveralls]

Pull Request Test Coverage Report for Build 8952186746

Details

• 0 of 42 (0.0%) changed or added relevant lines in 4 files are covered.
• 6 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.09%) to 51.985%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/SFML/Audio/Sound.cpp	0	1	0.0%
src/SFML/Audio/SoundStream.cpp	0	1	0.0%
src/SFML/Audio/Listener.cpp	0	12	0.0%
src/SFML/Audio/AudioDevice.cpp	0	28	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/SFML/Audio/AudioDevice.cpp	6	0.0%

Totals
Change from base Build 8952122764:	0.09%
Covered Lines:	10474
Relevant Lines:	18984

---

💛 - Coveralls

[Bambo-Borris]

I would love to see the API for re-initializing the audio devices on demand, I think power users producing applications with SFML would really benefit from this. If you'd be willing to implement such a feature I'd be more than happy to test it out in my current project and give feedback. Can test it on a couple of different environments also.

[binary1248]

AudioResource is just a migrated form of AlResource. Based on the comments in the old AlResource/AudioDevice implementation, I have to assume that Laurent came up with the mechanism because anything else wouldn't have worked with OpenAL. Bear in mind that was more than 15 years ago, using a library that had to always be dynamically linked, with an OpenGL-inspired API. Maybe there were issues with the initialization/deinitialization of the OpenAL context if it was done too early/late but it is hard to tell now.

I personally don't really care how things are implemented internally, as long as everything still works without any side-effects and doesn't impose unnecessary restrictions on users (such as not being allowed to create audio objects at namespace scope) just to make our own lives easier. Ease-of-use should always come before maintainability, especially with a library that has the word "Simple" in its name.

After having another quick look through the miniaudio header, it seems like there might be a way to reconnect the engine to a new device without having to tear everything down. It would require implementing a lot of stuff that we got for free, so investigating this requirement will take a bit more time.

[eXpl0it3r]

This seems to cause issues on Windows. Now our tests don't terminate anymore on Windows. Not sure if it's just a bug in the implementation or if that's the reason it had been implemented like that.

command timed out: 1200 seconds without output running b'cmake --build . --config Debug --target install runtests -- -k -j 4', attempting to kill

[binary1248]

So based on several hours of investigating, I have ended up with the same results as described in this article.

Basically, static object destructors are registered in a list that is maintained per module (.exe or .dll) and are called as part of atexit() in some indefinite order. Miniaudio maintains internal threads that it needs to process audio. When shutting miniaudio down, it has to wait for these threads to cleanly exit in order to clean up properly. The problem with running the cleanup as part of atexit() is that at least for .dlls atexit() is processed only as part of the UCRT shutdown which calls ExitProcess(). ExitProcess() first terminates all threads except the calling one and proceeds with processing the per-module atexit() lists after. Because miniaudio still waits for the non-existent worker thread to signal it has exited it will wait forever resulting in what we see happen here.

This can be reproduced with the following code:

// dll.cpp
#include <Windows.h>
#include <thread>
#include <iostream>

struct S
{
	S() : t([this]()
	{
		std::cout << "Thread Start" << std::endl;
		SetEvent(running);
		WaitForSingleObject(stop, INFINITE);
		SetEvent(stopped);
		std::cout << "Thread End" << std::endl;
	})
	{
		WaitForSingleObject(running, INFINITE);
		std::cout << "S()" << std::endl;
	}

	~S()
	{
		SetEvent(stop);
		std::cout << "~S()" << std::endl;
		WaitForSingleObject(stopped, INFINITE);
		t.join();
	}

	HANDLE running = CreateEventA(nullptr, FALSE, FALSE, nullptr);
	HANDLE stop = CreateEventA(nullptr, FALSE, FALSE, nullptr);
	HANDLE stopped = CreateEventA(nullptr, FALSE, FALSE, nullptr);
	std::thread t;
};

__declspec(dllexport) void foo()
{
	static S s;
}

// exe.cpp
__declspec(dllimport) void foo();

int main()
{
	foo();
}

This will hang at program exit the same way the audio tests do when run with the change in this PR. "Thread End" is never output since the thread was terminated by ExitProcess().

One could argue that miniaudio should check if the thread is still alive before waiting for the event. However, I wouldn't call it clean to allow any thread to get prematurely terminated and skip proper cleanup of resources, whether it is our own threads or those in dependency libraries. The fact that AudioDevice indirectly owns internal threads basically makes it ineligible to be given static lifetime if we want to avoid leaks/undefined behaviour at program termination. Furthermore, miniaudio being a C library exempts it from having to consider C++ static object destruction semantics, and I can't imagine the author making an exception just to support such a use case.

[eXpl0it3r]

Closing this, as this doesn't seem to work on Windows.

Maybe we can add some comments to the AudioResource to document that situation.

[eXpl0it3r]

I highly recommend you run the tests of a shared Windows build before pushing, because this will leave our CI builds hanging for 6h until they timeout. It will also directly give you feedback if the change actually works or not. 😉

I've cancelled the hung builds.

[vittorioromeo]

I highly recommend you run the tests of a shared Windows build before pushing, because this will leave our CI builds having for 6h until they timeout. It will also directly give you feedback if the change actually works or not. 😉

I've cancelled the hung builds.

Ugh, sorry -- I didn't realize the issue was specifically with the shared build. Let me see if I can reproduce locally...

[vittorioromeo]

Some notes:

• The order of static destruction and atexit-registered functions seems to be defined, in the sense that:

  void f()
  {
       std::atexit([]{ std::cout << "atexit"; });
       static S s;
  }
  
  void g()
  {
       static S s;
       std::atexit([]{ std::cout << "atexit"; });
  }

  • f will print "atexit ~S()";
  • g will print "~S() atexit".

• This doesn't help with our issue though, because:

  • Regardless of whether ma_engine_uninit is in ~AudioResource() or a cleanup function registered with atexit, it will still run during the whole atexit procedure;
  • ma_engine_uninit invokes `ma_device_stop', that's where the process hangs;
  • ma_device_stop calls ma_event_wait(&pDevice->stopEvent) -- this hangs undefinitely because the worker thread never calls ma_event_signal(&pDevice->stopEvent), I think because the thread was forcefully killed and not joined.

• To support our use case, miniaudio would have to do this:

  DWORD result = WaitForSingleObject(pDevice->thread, 0);
  if (result != WAIT_OBJECT_0) { ma_event_wait(&pDevice->stopEvent); } 

I'm going to send a PR and see what happens :)

[binary1248]

Letting a thread be killed, no matter in which context, is the literal definition of asking for undefined behaviour. You can't know in what state everything was left in when the operating system decided to pull the CPU out underneath the running code. Assuming that you can still continue executing other parts of code like nothing happened is... daring to say the least. This is a recipe for spurious crashes on program shutdown that nobody will be able to debug let alone solve. Because we are dealing with the sound interface of the operating system, who knows if this might even lead to leaks or other subtle side effects within it that might affect other applications. I seriously recommend against going down this route.

[vittorioromeo]

Letting a thread be killed, no matter in which context, is the literal definition of asking for undefined behaviour. You can't know in what state everything was left in when the operating system decided to pull the CPU out underneath the running code. Assuming that you can still continue executing other parts of code like nothing happened is... daring to say the least. This is a recipe for spurious crashes on program shutdown that nobody will be able to debug let alone solve. Because we are dealing with the sound interface of the operating system, who knows if this might even lead to leaks or other subtle side effects within it that might affect other applications. I seriously recommend against going down this route.

Thank you, I am starting to realize that this isn't the way to solve the issue.

What would it take for miniaudio to cleanly join the threads? Is it possible at all for miniaudio to internally detect program termination and join the threads before that?

Let's assume we can change miniaudio's internals as much as we like.

[binary1248]

No... all running threads besides the main one have to be joined before atexit() is called which in turn calls ExitProcess() which in turn terminates all non-main threads. The only clean solution in theory and practice is making sure all worker threads are joined before main() exits, there is no other way in this system.

[vittorioromeo]

No... all running threads besides the main one have to be joined before atexit() is called which in turn calls ExitProcess() which in turn terminates all non-main threads. The only clean solution in theory and practice is making sure all worker threads are joined before main() exits, there is no other way in this system.

So is it correct to say that the only way this could ever work is if the actual main function of the process was edited to contain a function call to miniaudio's cleanup procedure?

[binary1248]

Well... the main() thread has to tell the worker thread to end itself and wait for it before ending main(), that is the only clean way. Even if there was a separate "cleanup routine" that could be called by someone else, any kind of code you write will have preconditions and postconditions. Thread termination throws all assumptions out the window. Running code that touches this radioactive state in retrospect basically has unpredictable behaviour. The only safe thing you could do if threads had to be terminated would be to tell the operating system to deallocate memory and clean up any open handles, which is exactly what Windows does.

Because WASAPI is a COM interface, it will be much harder to clean up after dirty termination, which is why we should avoid getting into this scenario in the first place. In my experience COM is known for not playing nice with multi-threading. The easiest way to use it is to contain all calls to COM within a single thread, which is exactly why miniaudio is built the way it is with a seperate worker thread for the backend. Juggling between threads for whatever reason when dealing with COM is a recipe for a bad time.