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...