Stuttering caused by music playback

[_mental_]

With the recent changes in music handling, I encountered occasional micro-stuttering when playing.
A brief investigation revealed that it's caused by contention on MusInfo::CritSec.
This is quite noticeable with VSync enabled and (if you have fast CPU) using Debug configuration.

The problem is ZMusic_FillStream() may take a while to process the next chunk of data.
Other functions called from the main thread, ZMusic_Update() and ZMusic_IsPlaying() in particular, have close to zero costs.
Although, they wait on the mentioned mutex. I saw 20 to 30 ms stalls because of that.
This is really annoying with VSync as 60 FPS becomes 30 every two or three seconds.

At the moment, I don't see how this can be fixed without moving back to old code with a bunch of internal critical sections.
Apparently, this "global" lock solves any kind of race conditions, but it introduced another problem.
In fact, this is quite similar to music playback on the main thread in the early version of OpenAL backend.

[Graf Zahl]

Damn, and I just posted the new release. Well, it'll have to be tossed then.
The problem with those other functions is that internally they are anything but harmless. In particular IsPlaying can do bad things. I think the actual mutex needs to be applied a bit more finely grained, i.e. only in the cases where the function really wants to change things and not when it merely wants to read a global state.

Moving the mutex back into the players won't really help because there's only one song active at any point in time and the mutex is per song anyway.
The main problem here is that Randi's music code was overdesigned OOP and I wasn't able to get rid of anything - and it's precisely these parts that are now causing problems. I think in order to make this better it needs to be a little de-OOP-ified, so that the top level doesn't have to call virtual functions and can't make any intelligent decisions about what to lock and what not.

[Graf Zahl]

So, as long as we can be sure that the streaming thread does not do stupid stuff like actually deleting the player I think it should be doable to take the locks out of Pause, Resume Update, and isPlaying and instead only guard the Stop calls inside these functions. For obvious reasons all Stop calls need to be locked,

Now for the hard part: Check if it actually does these stupid things.
I committed my changes, can you check if this is better?

[_mental_]

Those micro-freezes are gone now. As for checking for races, I don't think that it's possible to verify this quickly. I believe it's OK to do a release in its current state.

[Graf Zahl]

From what I have gathered so far there may be one potential conflict: The now unguarded code tries to read the MIDI pointer in the MidiStreamer class. But that seems to be the only potential source of problems, none of the other status checking functions dereferences a pointer, so if it can be confirmed that the service thread cannot delete that (or outright stop the song) it should all be fine.

[_mental_]

I tested it a bit on Linux machine where music playback crashed pretty often before. Now everything is fine.
Of course, it’s really questionable way to verify a fix for multithreading issue.

[Graf Zahl]

That's why I wasn't rushing a new release build. I won't be able to do another one today, so it'll have to wait a day.