Music::pause() threading behavior

[Nexus]

I store several music themes in the same archive file and load them with sf::InputStream. As you know, sf::Music runs in its own thread which constantly invokes the stream's methods to read new chunks of data.

No two music themes are playing concurrently, as that would lead to race conditions in the file stream. However, some themes remain paused while others play -- and this is already enough to trigger race conditions. Other than SoundStream::stop(), SoundStream::pause() does not wait for the thread to finish reading its current chunk. So, what essentially happens is that the old (paused) music is still reading while the new (playing) one also starts reading.

I'm wondering whether pause() should be blocking like stop(). It would be more difficult to implement however, as the thread continues running in pause status, so we can't just call thread.wait().

What do you think? With the current SFML implementation I see only two options, both of which are rather hacks:

• Invoke SoundStream::getStatus() repeatedly until it's set to Paused (spin lock style). Apart from burning CPU resources, this approach is very sensitive to changes in the implementation: if we decide to cache the status and already set it in pause(), the assumption will break. I'm not even sure if it works at the moment.
• Instead of pause() + play(), call stop() + play() + setPlayingOffset(). This is also wasting resources because it unnecessarily performs the thread destruction/construction and the streaming intialization.

[eXpl0it3r]

SFML doesn't provide any thread-safety guarantees, however since this is "our" thread, we probably should try to prevent such a case.

As with the question on GitHub about the destruction of a music object, I wonder if we have to loosen the binding between sf::Music and the stream.

While binary1248 is right with the producer and consumer, I wonder how one should go about when using sf::Music as a "single" playback instance, where you load different streams and exchange them on the fly etc.
Such a system will cause issues with pause() and the destructor if you unload the last song before the application ends. To prevent the later you'd always have to load a dummy stream, since you can't detach/unload a stream.
So the question is: Is that kind of use of sf::Music undesired? What guarantees can we make about sf::Music then?

[Nexus]

As already said on GitHub, I don't consider that one a stream-specific issue; the same applies to files and memory buffers.

The way how SFML currently loads resources, through bool Resource::loadFromXy() member functions, is not ideal from several standpoints. Error handling is one, but more important in my opinion is that the library is not really clear about reusing the instance (as you say). It is not obvious if loadFromXy() can be applied multiple times, and if so, whether it truly resets the state of the whole instance.

If I had to design a new API for SFML 3, I would use the named constructor idiom combined with move semantics:

sf::Music music; // empty
music = sf::Music::fromFile("music.ogg"); // construction + move
...
music = sf::Music::fromStream(stream); // reuse instance
...
music = sf::Music(); // early destruction

As you see, this kind of code makes it obvious that the whole instance is reassigned. Plus, features like explicit resource release (last line) effectively come for free.


I think the pause() issue mentioned in this thread is not directly related to the loading/lifetime issues.

The underlying problem is that pause() and play() are asynchronous operations, while stop() is synchronous. The difference between pause() and stop() is rather subtle -- from a user point of view, the former maintains the playing offset while the latter resets it to the beginning. Other things, like the fact that the thread is destroyed in stop(), are rather implementation details. The documentation states almost nothing about the threading behavior, so one really has to check the code in order to see what happens.

The problem with asynchronous operations is that they add certain complexity. Things are running concurrently, and with the current API there's no good way to be notified when they're finished (only the spinlock workaround I mentioned). I think the reason why stop() is synchronous is defined behavior on destruction: when the music is destroyed while still playing, it has to wait for the streaming thread to finish reading before it can safely deallocate its resources.

I assume the reason why pause() is not synchronous is to avoid blocking the main thread. And in most cases, this is just fine, like play() it can run in the background. But there are advanced use cases like mine where this doesn't work, and if the implementation stays as is, the behavior should at least be clearly documented. Just because most users happily work with separate files, this doesn't mean we can always assume that.