Added loop point support to sf::Music

[Cobaltergeist]

After working a bit with SFML, I noticed that I couldn't set any custom loop points for sounds, and I need them for a game I'm working on. I spotted issue #177 sitting around, so I figured I would try implementing myself.

For sf::Music / sf::SoundStream, I added a feature to detect a loop point and cut off the streaming of the buffer, and a pair of virtual functions that can allow sf::SoundStream to let its derived class decide where to seek upon looping. It's accomplished entirely within those two classes without doing anything too funny with OpenAL, and I'm happy with what I managed to accomplish with it.

For sf::SoundBuffer / sf::Sound, I ran into some more constraints. OpenAL-Soft has a way to set loop points, but it's not a well-documented feature, and it is also a buffer trait, rather than a source trait. It has a few more restrictions on the allowable values compared to the Music version because of this. It uses an "enum" value called AL_LOOP_POINTS_SOFT defined in "alext.h", which was present in SFML but not included, and I figured it was fair game to use. If not, then I can take the SoundBuffer implementation out.

The inline documentation will explain any interface differences, but they're roughly the same, with getLoopStart(), getLoopEnd(), setLoopPointsFromTime(), and setLoopPointsFromSamples(). Three of those work with sf::Time, to keep everything consistent, the fourth is if you have an exact sample position you want to use.

I hope this isn't frowned upon, but for testing, I augmented the SFML Sound Example to include some additional playings of the resource sounds, and demonstrating some of the extra features of the Music implementations. At least it should be easy to build and test. If any modification or more proof-of-functionality is needed, please let me know!

[TankOs]

Why are there default values for the arguments? For me it seems like "start" and "end" are arguments that are required to be set by the user when setting up a loop. ;)

[TankOs]

should have read the explanation -- however I usually try to understand from source, that's why I wondered. ;) Personally I'd favor a separate function to deactivate a loop. Let's see what the others say about this.

[Cobaltergeist]

The default values are for ease of use when calling. In a lot of cases, I can see a user wanting to loop from the end of the file back to the starting loop-point. And since I define an end value of Zero to mean "end of file," then a caller only needs to pass the loop-start to achieve that effect. Similarly, calling it with no arguments is defined to reset the loop range back to the full duration of the sound.

Basically, those are there to make things simpler, since you won't always need to have 2 explicit time points on hand for some straightforward use cases.

(Before I forget, I should justify why it's safe to turn "end = Zero" into "end = Duration". Basically, if you consider a looping sound, then the beginning of the sound and the end of the sound essentially happen at the same point. So in the case of end, 0 serves as a safe representation of "wherever the default end-of-file loop is").

EDIT: Whoops, I was in the middle of writing this when you replied.

[MarioLiebisch]

I'd probably work with overloads here rather than having a member pair with different names? It could accept std::size_t for sample positions and sf::Time for time positions. It's not like we've got different approaches here (compared to sf::Texture::loadFrom... for example).

I think this should possibly be split into two separate members where one changes the loop entry and one specifies the loop exit.

[TankOs]

I second what Mario suggested regarding overloads.

[TankOs]

At first thanks a lot for your great contribution. I added some comments where I found (minor) issues, however I haven't marked every single spot (e.g. code style issues at multiple locations).

I'm going to test this and give more feedback later.

[LaurentGomila]

Don't forget that OpenAL Soft is not used everywhere:

• it is enforced on Windows
• it is usually the standard OpenAL implementation on Linux
• it is not used on OS X and iOS (they have their own implementation)
• we still don't know if it's going to be used on Android

So you can't use any OpenAL Soft specific extension.

[MarioLiebisch]

• it is enforced on Windows

This is not necessarily true. There might be a system OpenAL (either as part of soundcard drivers or installed standalone), which would be used if SFML's Openal32.dll isn't bundled with the program.

[LaurentGomila]

This setup is not officially supported. This often leads to errors, and the answer is always "use the OpenAL DLL shipped with the SFML SDK".

[Cobaltergeist]

So does that basically mean that the SoundBuffer portion of my implementation is non-portable? I was worried about that. Though I am curious why alext.h was present in its current form with the other AL headers, if it's not safe to use what's in there?

The only other way I can see that being accomplished though is basically doing what I did with Music by modifying the way sound gets queued. It works in that case because Music already had a dedicated thread overseeing that. I wouldn't want to do the same with SoundBuffers since I can see them being used overall a lot more frequently. But then again I also think that they would require custom loop ranges a lot LESS frequently...

So what should we do about this? Try to rework SoundBuffer/Sound with more active control? Or just drop it there and let Music be the exclusive loop point solution?

[LaurentGomila]

Though I am curious why alext.h was present in its current form with the other AL headers, if it's not safe to use what's in there?

These are only the Windows OpenAL headers. And it's there because it's part of OpenAL Soft. Not because I use it.

So yes, it's not safe, unless we're lucky and the OS X / iOS implementations support this extension too, or have their own extension that does the same thing.

Or just drop it there and let Music be the exclusive loop point solution?

Definitely.

[Cobaltergeist]

Okay, I completely reverted sf::SoundBuffer, and I made some implementation detail changes in sf::Music. Everything is now stored as "size_t" in units of channel-dependent samples, and I added a function in SoundFile to seek to a sample position. This is all in my attempt to reduce internal conversions between sf::Time and sample counts.

As for the function interfaces, I changed them to be overloads, with the sample version taking size_t without default values, however, I'm hesitant to remove the default sf::Time parameters, or to separate into setLoopStart and setLoopEnd, for a couple reasons.

For the defaults, it's about ease of use, which I know SFML aims for. I already explained most of that in the earlier comment, but I just think the thought process of "Call the setter with its default values, and it will set the loop to the sound's default range" makes good sense. The loop points are never really "deactivated" as long as getLoop() is true, they just match the whole duration of the file so they get suppressed when they're unset.

As for not separating them, I feel like loopStart and loopEnd are necessarily connected, they form a range. It's kind of like the X and Y components of a Drawable. You don't really have a proper vector without both, and none of the setters take just one at a time. You can still keep one by calling its getter before you set the position or range, but setting them completely separate forgets their purpose. I was even considering making a new pair struct called sf::TimeSpan, but I decided that was overkill. Also, with sf::Music, we have a second thread running around that reads these values. and if it runs into loopEnd it will seek straight to loopStart. I make sure to lock the mutex before setting them, but I can't safely do that with 2 different functions. There's a potential race condition if a loop happens between setting the two, and although it wouldn't risk a segfault, it might send the play position somewhere undesired for a while.

Thoughts?

[Bromeon]

Please change only the necessary parts related to music. Why did you modify std::endl to \n? In general (not here) the buffer isn't flushed, and on some consoles you may not see output immediately.

[Cobaltergeist]

std::endl includes a flush, and I just figured doing it twice in a row was silly. As long as the second one is there, it should be the same either way. I'll change it back though.

[Bromeon]

I'm just not sure how we want to do this in general... Actually what you say makes sense, but then we could also change the upper code (lines 21-24).

I would say let's change as few as possible for this pull request (i.e. only stuff related to audio) and handle unrelated changes another time.

[Bromeon]

I've commented on some code style and implementation issues in the diff, here are some API concerns:

For the defaults, it's about ease of use, which I know SFML aims for.

That doesn't make it easier, only more confusing. From the call music.setLoopPoints(), it's not obvious how the loop points are set or that they are reset. music.setLoopPoints(time) with one argument is even less intuitive, how can I specify two points with one point?

"Simplicity" doesn't mean the user has to write as few code as possible, but that interfaces are expressive and easily understandable, even self-documenting wherever possible. So you should remove the default parameters.

As for not separating them, I feel like loopStart and loopEnd are necessarily connected, they form a range.

That sounds meaningful. It would also allow assertions to check whether the end point is after the start point.

However, it is a bad idea that sf::Time::Zero means different things (beginning and end of music) depending on the context. You should rather introduce a specific constant to denote the end.

Concerning separate function to enable/disable looping, there is already setLoop(). If it is enabled, it will use the loop points as specified (or the whole range if setLoopPoints() has not been called), otherwise it will play just once.

And why did you change sf::Uint64 to std::size_t? The latter may be smaller and brings no real advantages. If std::size_t is a 32 bit type, then "only" 27 hours of music can be played at 44.1kHz. With higher sample rates, it's even less. Since we don't know how the music is possibly used, and it costs nothing to use 64 bit types, I'd suggest to stay with sf::Uint64.

[Cobaltergeist]

I guess some compilers are bound to have size_t be smaller than 64 bits. I see you point, and I'd love to change them back. I originally had Uint64's, but I was advised to change to size_t by an earlier comment, and agreed after seeing that size_t was already being used quite a bit in the Audio module (for example, sf::SoundBuffer's sample vector, which returns size() as a size_t, and sf::SoundFile's getSampleCount, which is used by Music, is a size_t as well).

Looking again though, I can probably change sf::SoundFile to fix this too. Apparently libsndfile uses Int64, and it gets cast to a size_t. I should be able remove that alright, so I'll get to work making everything in Music be guaranteed 64-bits.

As for sf::Time::Zero, I see your point, but in the context of loopEnd, I can't think of anything else Zero could mean. Because in a looping sound, when you're at the end, you're also at the beginning because it's about to replay. Keeping it Zero internally would actually produce the same audio. It's just more straightforward for buffer loading if the loop position is at the "end-version" of that shared point in time.

What else would I /do/ if I saw a zero there? It can be passed in even if I take out the defaults, and it seems silly to fail the whole operation because of it.

[Bromeon]

We should probably think about the naming: FileEnd is not accurate, because a music needn't be loaded from a file. Maybe this enumerator should really be called BufferEnd or MusicEnd or StreamEnd? The type could be called EndType or so...

[Bromeon]

[...] after seeing that size_t was already being used quite a bit in the Audio module (for example, sf::SoundBuffer's sample vector, which returns size() as a size_t, and sf::SoundFile's getSampleCount, which is used by Music, is a size_t as well).

Looking again though, I can probably change sf::SoundFile to fix this too. Apparently libsndfile uses Int64, and it gets cast to a size_t. I should be able remove that alright, so I'll get to work making everything in Music be guaranteed 64-bits.

Hm, let's wait for other opinions on this. I'm personally rather in favor of 64 bit types, since the underlying library uses it and we don't gain a lot by constraining the user to 32 bit.

As for sf::Time::Zero, I see your point, but in the context of loopEnd, I can't think of anything else Zero could mean.

Immediately, it looks like the time zero, denoting an empty interval, i.e. the music would not loop at all. Furthermore, one can't simply have an assertion start < end but must handle this case specifically. In general, we need unnecessary case differentiations. People still have to assume that sf::Time::Zero means the end... Why not use the obvious argument, namely music.getDuration()? It is much more expressive and needs no special handling.

What else would I /do/ if I saw a zero there? It can be passed in even if I take out the defaults, and it seems silly to fail the whole operation because of it.

I think it's rather silly to not fail if the user passes a wrong value. Something like assert(start < end) would be nice, so that debugging developers are immediately notified. By the way, it's enough if this happens in one overload, as long as it's called by the other.

And maybe something else: I would probably use the term "begin" instead of "start", because it's consistent with the STL.

[Cobaltergeist]

I suppose I can fail on end == 0 since the value it would be translated to is easy to obtain anyway, but the thing about start > end is that it's actually a valid case. My overcomplicated example demonstrated this sort of inverted loop, which instead cuts out the range specified. I don't see an immediate use for that, but the way it's implemented doesn't cause any problems, and I don't see a point in explicitly rejecting a case that the class can already handle safely.

Anyway, I'll get to work renaming "start" to "begin" and making the enum's name more understandable.

[Bromeon]

Ah, indeed. But in that case, 0 is not a special value, and I wouldn't specifically check for it... Maybe a short question, is the sample interval half-open, i.e. begin <= index < end?

Also, maybe you could wait on other people's opinion before changing everything, to avoid needless work :)

[Cobaltergeist]

Yeah, the sample interval is half-open. loopEnd can equal the sample count, and that count is the length of the hypothetical 0-indexed sample array that's never gets entirely loaded into memory. So yeah, setting loopBegin to 0 and loopEnd to 1000000, Music will loop from sample 0 to sample 999999.

[Cobaltergeist]

Okay, sorry for disappearing for a few weeks. I've been both busy and waiting for additional comments. The commit messages should cover most of what I did technically, but I'll summarize the state of the design.

• I've merged in and fixed conflicts with master's recent mutex-centric changes to sf::SoundStream.
• The setLoopPoints() function now does not have any defaults, does not treat end == 0 as a special case, and does not turn begin == end into another special case when it's best to just fail.
• m_loopStart and associated variables have been renamed along the lines of m_loopBegin.
• sf::SoundFile and sf::Music now use Uint64 for everything concerning sample counts.
• Conversions between sf::Time and sample counts are now done through helper functions, rather than having the line-hogging conversions repeated.

Hopefully this is close to finishing up. I do have a question though, about the priv::SoundFile belonging to sf::Music. I notice it's stored as a pointer, allocated on the heap in the constructor and deleted in the destructor. Is there any particular reason why it isn't just an outright member?

[eXpl0it3r]

@LaurentGomila, when you find the time, can you give this another review?

[Bromeon]

• Loop point pairs are now externally specified as [offset, length] instead of [begin, end]. sf::Music still uses [begin, end] internally. (Most cases of 'time ranges' use this convention, including the few scattered results I found in google searches about ways to specify loop points for other programs that recognize them.)

I'm not sure if this is actually easier -- it's inconsistent to STL begin-end pairs, and I can imagine that many loop points are specified relative to the beginning or end of the music theme, and not relative to the duration.

Why is there still getSampleCount()? It should only be necessary together with sample overloads (which should be discussed at a later stage).

[Hapaxia]

The format of the loop points is taste only.
Begin/end is similar to STL (iterators).
Start/duration is closer to SFML (position/size).

I prefer the former - especially with sound - but it's trivial to convert between the two types.

Specifying a sound chunk by duration is actually useful if you know how long the sound should be; you can just change the start point. But, as said above, it's simple enough to do end = begin + requiredDuration.

[eXpl0it3r]

This seems like a left over from previous iterations.

[eXpl0it3r]

Minor detail, but I think there's enough room to use timePoints instead of timePts. Alternatively points might be good enough as well.

[LaurentGomila]

Yes, we should really avoid abreviated names. I've seen some others in the code, please fix them all 😛

[eXpl0it3r]

Will this enum be extended at one point or what's the reason for using an enum?

[select-this]

AFAIK the only advantage to using enum in this context is to avoid ODR violations.

[eXpl0it3r]

Whether begin/end or begin/duration is used, is largely personal taste and both will have similar argumentation. For music tracks it makes in my opinion more sense to specify the time in start & duration, as this is how I generally would be thinking of your music sections. Besides, we're not trying to copy the STL or anything, so the comparison doesn't really matter and might be confusing to some.

I'd say we stick with the current design, unless someone has better arguments than personal opinions or the STL.

[LaurentGomila]

Why is SampleSpan public if it's only used in private members? We don't even need a typename since it is used only once.

[LaurentGomila]

Yes, we should really avoid abreviated names. I've seen some others in the code, please fix them all 😛

[LaurentGomila]

Divide by zero should better be checked where the divide is done, i.e. in samplesToTime. Which makes this function much less verbose:

return TimeSpan(samplesToTime(m_loopSpan.offset), samplesToTime(m_loopSpan.length));

[LaurentGomila]

samPts... 😛

[LaurentGomila]

Every sub-expression of the condition should be enclosed in ( ). Same applies to other parts of the code.

[eXpl0it3r]

@Cobaltergeist Any update on this?

[Cobaltergeist]

Sorry for the wait. It's amazing what coding for work does to one's drive to code for personal projects.

Anyway, I've finally rebased and addressed the following concerns:

• Removed getSampleCount()
• Removed public SampleSpan typedef
• Fixed abbreviated parameter and local names
• Moved the divide-by-zero check
• Added some parentheses

I kept NoLoop as-is because enums are a semi-standard way to use constants without introducing either globals or macros. BufferCount uses this same strategy. The only reason NoLoop is on its own (and not in the same block as BufferCount) is because it needs protected instead of private visibility so that classes derived from SoundStream can use it.

[eXpl0it3r]

Thanks for the update! @LaurentGomila can you give this another look?

[LaurentGomila]

I'll do my best.

[Cobaltergeist]

Any more news? I can rebase soon, but if there are any remaining changes I need to make, I'd prefer to address everything in one go.

[LaurentGomila]

Looks good

[Cobaltergeist]

Alright, that should do it. This has been the oldest opened PR for quite some time by now. Let's get this tested and merged so we can put it behind us.

[eXpl0it3r]

A huge thank your for all your work over the past 2 years! 😄
(Stay tuned for potential bugfixes 😉)

[Two-Tone]

I can't believe it's finally been merged. Feels like it has been forever.

[Cobaltergeist]

Feel free to let me know when 3.0 changes open up. Between #1118 and this, I've had to work around an API that's doesn't propagate a lot of helpful information. A breaking-allowed change would let us clean up a lot of this stack.

[eXpl0it3r]

I'm interested in hearing your ideas on a better API approach. If you feel like providing some more feedback, I'll gladly open a thread in the SFML Development forum section.

[Cobaltergeist]

Well, the gist of it concerns the amount of information NOT passed in calls and returns during the whole read()/seek() workflow.

Decision choices throughout my Soundstream-related PRs like #1118's immediateLoop, #1154's retry logic, and this PR's onLoop() function were all focused on a higher-level function trying to discover or infer the state of underlying objects. Things like figuring out whether a short/zero read means EOF, Loop-Point, IO-Error, or just a quirk of the codec reader, when the read() calls only return Int64s and bools. Or having SoundStream figure out where Music wants to seek on a loop point to avoid desyncing of m_samplesProcessed, when seek() returns void. In my opinion, these cases would be better off just exposing more information in return values, but I can't break the APIs to change that yet.

It'd mostly involve changing function signatures, and possibly adding a "Read Result" enum or struct.