Fixed inconsistent seek behavior in SoundStream

[Cobaltergeist]

Sorry for being so spotty with my updates to the Loop Points. One of the things I was tasked to do was isolate my bugfixes for sf::SoundStream into a separate PR so that #629 could be decided independently.

Now, for anyone who doesn't know what this is about, here's a summary of what I addressed in this PR, namely two edge-cases that break the consistency of m_samplesProcessed and one regression when setPlayingOffset() and play() are used when the sound is stopped:

I only found the first one because I was using a file that was an exact numbers of seconds long, which matches the SoundStream buffer size. Currently, if onGetData() reads to the exact end of the file, it will return true due to the full read, even though there's no more data available afterward. However, this means that the buffer isn't properly marked as an "end". When it's time to read the next buffer after that, it will read 0 bytes and return false. This will trigger a seek to 0 (in the current version without loop points), and flag the buffer as an "end". But since the read was 0, the buffer then gets refilled with 1 second of post-seek data. The two buffers get played normally, but now the 2nd one is flagged as the end, even though it contains beginning data. And when it gets unloaded, the sample counter gets reset one second too late. This creates a "lag" that persists over the playback, forever reporting an incorrect play position.

My fix involved adding m_tellPos, in sf::Music to keep track of the sample position in the underlying file. Now, if the position afterwards lands on the EOF, onGetData() will return false, even if the a read fills the whole buffer. I couldn't fix it in sf::SoundStream itself, because modifying the signature of onSeek() would break the API. But sf::Music is close enough to the underlying file to track its position, and has enough leverage with its return value from onGetData() so it can force a zero-seek at the right time.

A slightly different case involved a similar error when the play position starts at the end-of-file. It causes the same "delayed zero-read" effect, though the offset corrects itself after a loop iteration. Since no sound is loaded yet, there's no rightful owner of the "end" status, so my solution is the immediateLoop parameter in fillAndPushBuffer() it's only set to true during the fillQueue() call as the thread starts up. It essentially gives fillAndPushBuffer() permission to immediately adjust m_samplesProcessed to the correct post-loop value, instead of deferring things until the refilled buffer gets unloaded a second too late.

These fixes combined lead to an included easy-fix of #966, since sf::Music() can now clamp a seek overrun to the EOF, invoking the new reset behavior at the next onGetData().

I'd argue that the last "bug" is a regression. setPlayingOffset() used to always begin playing the sound when it was called. But when that behavior was removed, the ability to manually start playing afterward from the new offset wasn't added. I found this issue during my Loop Point testing, when I tried to set the "play start" by calling setPlayingOffset() between openFromFile() and play(). I noticed that the music always began from the start regardless, due to the onSeek(Time::Zero); call in play() and I didn't see any proper reason for it. The paused and playing cases were already covered, and if the music was stopped, then there are 3 possibilities. Either the music was newly loaded (already 0), newly-stopped (already 0), or had its offset explicitly adjusted after loading or stopping (why would you overrule that?). So I removed the onSeek(Time::Zero); line, and moved the m_samplesProcessed = 0; line to initialize(), to make sure case 1 is still covered.

So that's my summary of the bugs, in isolation from #629. I'll replace the code in that PR with fast-forward from this one soon.

EDIT: Here's a new test.cpp. It's just my newest Loop Point one with the actual loop point calls disabled by a commented-out macro at the top. I still recommend using it with count.wav.

EDIT by @mantognini: both the source code and sound file in one archive

[mantognini]

I can confirm that the current master version suffer from these three issues

[...] the sample counter gets reset one second too late. This creates a "lag" that persists over the playback, forever reporting an incorrect play position. [...]

and

[...] A slightly different case involved a similar error when the play position starts at the end-of-file. It causes the same "delayed zero-read" effect, though the offset corrects itself after a loop iteration. [...]

and

[...] when I tried to set the "play start" by calling setPlayingOffset() between openFromFile() and play(). I noticed that the music always began from the start regardless [...]

and that your code (which I haven't yet looked at) fixes them! 👍

[Cobaltergeist]

Any updates on this? I'd prefer to see this merged, or at least approved, before I go back and update the loop points PR. But it doesn't sound like anyone's fully inspected my code yet.

Since the fix is reproducible, hopefully it's just a matter of syntax and other nitpicks, if anything. And I'd love to see this make it into 2.4.

[mantognini]

I'm not so much familiar with the internal of the audio module, so it'd be better if someone else reviewed this. And I'm afraid targeting 2.4 will only postpone its release... :-/

[LaurentGomila]

I was away the whole week, I'll try to review your code ASAP (and fully read the PR description :).

But I've seen something that should be removed: you added a public function to sf::Music which is not needed to fix the bugs (or I missed something?). Don't mix unrelated things in a single PR. If you want to add something to the public API, then you should first discuss it on the forum.

[Cobaltergeist]

Whoops, getSampleCount() might have been a part of the loop points PR that I forgot to exclude when cutting things down for this one. It made more sense as an addition there since sample counts are more relevant. I'll go remove it from this one.

[eXpl0it3r]

Can confirm that it works on my Windows 10 machine just fine.

[Cobaltergeist]

Just checking in since it's been a little while. I'm glad to see 2 confirmed fix cases, but has anyone had a chance to vet the code convention and style?

[eXpl0it3r]

I would have hoped that @LaurentGomila could take some time to look at it, since he's most familiar with the audio module.

The style looks okay to me, except for the line break which seems unnecessary - we don't have a line width limit afaik.

[LaurentGomila]

Sorry, my free time is really limited and when I get some, I usually have other (personal) things to do.

// Make sure we don't divide by 0
if (getSampleRate() != 0 && getChannelCount() != 0)
    return m_file.getDuration();

This should rather be done in sf::InputSoundFile directly.

My fix involved adding m_tellPos, in sf::Music to keep track of the sample position in the underlying file.

I would rather add a getCurrentPosition() (or any other name) function to sf::InputSoundFile, which can easily be used to test for EOF. That would be more generic and useful than your sf::Music fix.
Then we can only keep the EOF test in sf::Music::onGetData and skip the current one, which was just a workaround to the lack of proper EOF detection.

The second fix of this PR requires more attention, I'll have a look at it later.

[eXpl0it3r]

I think Laurent has some good points there. Are you willing to update your code @Cobaltergeist?

[Cobaltergeist]

Okay, I went and moved the sample position tracking, including the counter itself, the overrun clamping and the divide-by-0 checks, into sf::InputSoundFile. Now, sf::Music gets all the info needed for its EOF check from m_file and it doesn't have to hold the m_tellPos counter as a workaround.

As well as the getSampleOffset() member function I use, I also added a getTimeOffset() function to sf::InputSoundFile for the sake of interface consistency. I noticed that sf::InputSoundFile tends to expose sample and time versions for the same data input/output (see: getSampleCount() + getDuration(), as well as the 2 overloads for seek()), and decided to follow suit. If you want me to leave it out instead, let me know and I'll rmeove it.

[LaurentGomila]

or...

m_sampleOffset = std::min(sampleOffset, m_sampleCount);

[Cobaltergeist]

When I wrote that I was unsure whether all c++03 implementations supported uint64 as the parameter type.

But looking at it now, the standard says it's been templatized since at least c++98, so I'll go and make the switch.

[eXpl0it3r]

Added some "cosmetic" comments. 😉

[Cobaltergeist]

@LaurentGomila sorry for not replying directly to your line, but I cover something important in the 2nd half of this comment that I don't want hidden when I update the diff.

I'd argue against completely omitting the first sample-read check simply for the sake of defensive programming. Especially since we allow users to add custom stream classes and sound readers that the underlying InputSoundFile might be using. If the read somehow errors out, we need to know.

What if we change it to data.sampleCount != 0? That way we increase flexibility a bit. If a custom stream encounters a short-read but stays valid, we won't reset the loop. But on a condition where data becomes completely unavailable, we can still propagate a reset-to-zero back to SoundStream.

Actually, speaking of error cases, let me check what does happen when a file becomes inaccessible mid-stream...

Oh jeez, it causes a segfault... Looks like fillAndPushBuffer enters infinite recursion and overflows the stack when it encounters an unreadable looping sound. Seems like an easy fix that would fit this PR's scope though. I see 2 ways that basically accomplish the same thing:

1. I add another parameter to fillAndPushBuffer, something like bool isRetry or Uint32 retryCount. I'll set it to only be true/positive in the recursive retry, and prevent further recursion once it becomes true/maxRetries.
2. I add the retry flag/counter as a local variable in fillAndPushBuffer, change if (!onGetData(data)) to while (!onGetData(data)), and add break, continue, and ++retryCount within the new loop as appropriate.

The latter saves stack frames and makes a retry counter easier to follow if we use one, but it abuses looping semantics a bit. Up to you guys which one I go for.

[LaurentGomila]

I'd argue against completely omitting the first sample-read check simply for the sake of defensive programming. Especially since we allow users to add custom stream classes and sound readers that the underlying InputSoundFile might be using. If the read somehow errors out, we need to know.

It depends how we want the music to behave.

In case we read less bytes than requested, I don't think we should stop the music, the back-end may have chosen to return less samples for some reason, but still be valid and able to return new data on the next call.

In the case where no sample was returned at all, and we are not at EOF, it's less obvious. The input file may be stuck for some reason, and we may freeze ther music if we just keep on trying to read. I think stopping makes sense in this case.

So, it seems like we both agree on how to change this piece of code ;)

Actually, speaking of error cases, let me check what does happen when a file becomes inaccessible mid-stream...
Oh jeez, it causes a segfault...

Let's address this issue in another PR, since it's a different thing. This way we can finally end and merge this one.

[Cobaltergeist]

Alright, I merged up to the latest master and performed the cosmetic changes:

• Changed the first EOF check condition to data.sampleCount != 0
• Tweaked the if statements in the new InputSoundFile getters
• De-aligned the other declarations in SoundStream::initialize for consistency

I have code for the segfault fix, but I'll include that in another PR once this one is merged.

[Cobaltergeist]

Sorry to bump this, but is there any news?

Test suite results? Comments or approval from @LaurentGomila? Prospects of making the next merge list?

I can't cleanly make the segfault fix PR until this is merged, since it modifies fillAndPushBuffer() in a way that would definitely conflict with my addition of the immediateLoop parameter if done separately. Not a huge conflict, but one that would definitely require manual resolution on my part, to whichever change comes last. Or do you guys think it's worthwhile to make the PR now anyway?

[MarioLiebisch]

@Cobaltergeist Assuming you've got your code written already (or it's not that complex to do; not really up to date here right now), just create your pull request so people can try it standalone, comment on it, etc. and then just rebase it in case this PR is merged first. Just make sure to note the possible conflict in your PR description (e.g. something like this: These changes might conflict with #1118.)

[Cobaltergeist]

I noticed that @binary1248 ran some tests, and a few failed due to not recognizing std::min(). I believe that was caused by me not including <algorithm> so I went back and fixed that.

[Cobaltergeist]

Does anyone want to weigh in on my findings? I'm only touching the code if we agree that I need to compensate for unreliable readers.

[eXpl0it3r]

Can this/does this need fixing in the readers themselves?

[Cobaltergeist]

The FLAC reader will indeed need fixing at some point. I'm just not sure that it falls into the scope of this PR. While I guess it can fit the definition of "Fixing Inconsistent Seek Behavior", it's a bit more outside the body of code already being modified than I'd like.

At the very least's it's gonna require a bit more digging. Is that worth delaying this PR's merge? While my code could be doing a bit more to work around violated assumptions, I don't think its assumptions themselves are unreasonable.

My current impression is that I should add a safeguard for the offset overrun case to my PR now (which can be done with 2 lines in InputSoundFile::read), and we can put the fix for the early-EOF case in a separate PR where we fix the FLAC reader itself.

[Cobaltergeist]

Actually I'm gonna revise my stance on this. I've managed to track down and fix the issue with the FLAC reader. The change isn't huge, and it fits the spirit of this PR, so unless I hear objections, I'd like to include it in this commit.

The problem was that the underlying FLAC decoder has 2 seeking quirks that SoundFileReaderFlac::seek() and streamWrite() overlook.

First, every seek operation triggers a streamWrite() callback. Previously, this case would simply cause the function to return, but it turns out that it actually needs to do some work. When seeking, you'll land mid-block more often than not. And when that happens, the decoder feeds all the samples from the seek destination to the end of its block into streamWrite(). Ignoring those bytes is incorrect, as the next read call actually starts at the beginning of the next block. By skipping this partial block, we leave the stream at a later point than higher-level trackers (namely InputSoundFile) expect it to be. This causes the "early" short-read. To fix this I changed streamWrite() to act on these seek-decodes, and dump the output samples into the leftovers vector, where they will be picked up in the next SoundFileReaderFlac::read().

Second, the FLAC decoder does not like seeking straight to EOF. The workaround is easy. I just seek to one sample before the EOF, call FLAC__stream_decoder_skip_single_frame(), and re-clear the leftovers vector. In the process of fixing this one, I noticed that the m_channelCount variable added to address the forum post you linked is actually redundant. I removed it in favor of the already-available m_clientData.info.channelCount.

Please let me know if it's worthwhile to include my new fix in this PR, or if we should just get this one merged while I make a new one.

[LaurentGomila]

Sounds good.

Although linked to the same issue, these two changes fix two distinct bugs, so I'd say make another PR for the FLAC reader.

[Cobaltergeist]

So how about I just add the following safety checks to InputSoundFile::read(), and we'll be good to merge this one:

Uint64 InputSoundFile::read(Int16* samples, Uint64 maxCount)
{
    Uint64 readSamples = 0;
    maxCount = std::min(maxCount, m_sampleCount - m_sampleOffset);
    if (m_reader && samples && maxCount)
        readSamples = m_reader->read(samples, maxCount);
    m_sampleOffset += std::min(readSamples, maxCount);
    return readSamples;
}

I'll put up the other PR soon.

[LaurentGomila]

Can you explain again why readSamples can be higher than maxCount?

And I'd also add some comments, as these extra checks may not look intuitive at first sight.

[Cobaltergeist]

With a compliant stream, an invalid readSamples shouldn't ever really happen. It's mostly just maxCount that needs to be clamped, so that we can force a short-read if we're counting on one to occur. That was the main problem with the FLAC overrun case.

But yeah, I'll leave the first clamp in, and add a comment.

[Cobaltergeist]

That should do it. With maxCount clamped (and commented), the desync should be made rarer, until the FLAC reader fix eliminates it. I'll make that PR tomorrow.

As for this one, it should be ready to merge once the tests are re-run.

[LaurentGomila]

Do we really need to clamp maxCount before reading? I mean, if the underlying reader is correctly implemented, it cannot return more samples than what remains until EOF. It feels really weird to handle that at this level. If it's just a matter of fixing one or more reader(s), I'd prefer to focus on them and keep the higher-level code clean.

[Cobaltergeist]

After seeking what happened with the non-compliant FLAC reader, my logic boiled down to this:

Now that InputSoundFile tracks the seek position, it's now responsible for keeping that counter consistent and synchronized. Audio decoders can be tricky to get right, plus we allow users to provide their own.

Currently, read() forwards whatever maxCount value it's given (aka the full sample buffer size), even if the expected number of remaining samples in the file (m_sampleCount - m_sampleOffset) is less than that. It then blindly adds the returned sample count to m_sampleOffset. It's essentially permitting the underlying stream to read maxCount but trusting it to return less. If a implementation is slightly off, it could return more samples than the remainder. After that, m_sampleOffset will end up greater than m_sampleCount. Currently, this only desyncs the loop indicators and play position, and doesn't cause anything to explode, but that's not the sort of consistency violation I'd want to risk.

Basically, I see this as InputSoundFile protecting its internal consistency from the results of an external call gone slightly amiss. Something to prevent small potential errors from growing.

[LaurentGomila]

To me, it is just hiding potential flaws in readers. I can't imagine any valid situation where this check would be useful. We shouldn't be afraid to dig deeper if the true problem is there -- in reader(s).

Yes, users can provide their own readers. And if we compensate for (hide) logic errors originating in readers, these users (and us as well) will never get a chance to spot them and provide a proper fix.

[Cobaltergeist]

Well, I guess I'll just go ahead and revert that...

I just need to get this thing merged and over with.

[LaurentGomila]

Agreed.

Thank you again for your help, and for your patience 👍

[Cobaltergeist]

Hold on. I just tried to re-apply the FLAC changes without the maxCount clamp, and something weird is happening. Let me look into this before you merge it, and make sure it's just the reader and not something here that I missed.

[Cobaltergeist]

Ignore me, everything's fine! I just messed up a git command and didn't reapply things properly. I redid it the right way, and things are behaving again.

The current commit looks good to merge.

[eXpl0it3r]

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns (again 😉).

[eXpl0it3r]

Merged in 4856aea on branch 2.4.x

Thanks for all the work you put into this! 👍