Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Addressed Seeking Quirks in FLAC Reader #1162

Merged
merged 1 commit into from
Jan 5, 2017
Merged

Addressed Seeking Quirks in FLAC Reader #1162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 21 additions & 14 deletions src/SFML/Audio/SoundFileReaderFlac.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,6 @@ namespace
{
sf::priv::SoundFileReaderFlac::ClientData* data = static_cast<sf::priv::SoundFileReaderFlac::ClientData*>(clientData);

// If there's no output buffer, it means that we are seeking
if (!data->buffer)
return FLAC__STREAM_DECODER_WRITE_STATUS_CONTINUE;

// Reserve memory if we're going to use the leftovers buffer
unsigned int frameSamples = frame->header.blocksize * frame->header.channels;
if (data->remaining < frameSamples)
Expand Down Expand Up @@ -142,15 +138,16 @@ namespace
break;
}

if (data->remaining > 0)
if (data->buffer && data->remaining > 0)
{
// There's room in the output buffer, copy the sample there
// If there's room in the output buffer, copy the sample there
*data->buffer++ = sample;
data->remaining--;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the effect of decrementing data->remaining without adding data to the buffer? Well, is it useful at all to execute this statement if there's no buffer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, the whole function returned early when data->buffer was null. Since I removed that check, I figured I'd put a smaller one around the point where the buffer is actually dereferenced. I know that in practice, data->remaining will always be 0 when data->buffer is null (since that's the seek condition), and the data->leftovers.push_back(sample) path should always be taken then. But I'm always wary of dereferencing a pointer that hasn't been checked locally. So I just covered the single dereferencing line of code with the new check. I didn't touch the decrement because that preserves the countdown to where it will start loading the samples into the always-valid leftover buffer. But that's a bit of moot point since that should never happen anyway.

Maybe I should change the surrounding if condition to if (data->buffer && data->remaining > 0).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I should change the surrounding if condition to if (data->buffer && data->remaining > 0).

That's the way to go in my opinion. I'd also change the comments because now the else block is also for the seeking case.

}
else
{
// We have decoded all the requested samples, put the sample in a temporary buffer until next call
// We are either seeking (null buffer) or have decoded all the requested samples during a
// normal read (0 remaining), so we put the sample in a temporary buffer until next call
data->leftovers.push_back(sample);
}
}
Expand Down Expand Up @@ -210,8 +207,7 @@ bool SoundFileReaderFlac::check(InputStream& stream)
////////////////////////////////////////////////////////////
SoundFileReaderFlac::SoundFileReaderFlac() :
m_decoder(NULL),
m_clientData(),
m_channelCount(0)
m_clientData()
{
}

Expand Down Expand Up @@ -249,9 +245,6 @@ bool SoundFileReaderFlac::open(InputStream& stream, Info& info)
// Retrieve the sound properties
info = m_clientData.info; // was filled in the "metadata" callback

// We must keep the channel count for the seek function
m_channelCount = info.channelCount;

return true;
}

Expand All @@ -267,7 +260,21 @@ void SoundFileReaderFlac::seek(Uint64 sampleOffset)
m_clientData.leftovers.clear();

// FLAC decoder expects absolute sample offset, so we take the channel count out
FLAC__stream_decoder_seek_absolute(m_decoder, sampleOffset / m_channelCount);
if (sampleOffset < m_clientData.info.sampleCount)
{
// The "write" callback will populate the leftovers buffer with the first batch of samples from the
// seek destination, and since we want that data in this typical case, we don't re-clear it afterward
FLAC__stream_decoder_seek_absolute(m_decoder, sampleOffset / m_clientData.info.channelCount);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight improvement: I'd compute and store (and comment) sampleOffset / m_clientData.info.channelCount before the test, instead of duplicating it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this purely for readability? The result of that division op is only used once in the if case and once in the mutually-exclusive else case. It's never repeated in any single call to seek(), so I can't see it optimizing anything.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course it is for maintainability reasons, not for optimizing anything. If the expression ever has to change in the future, there will only be one place to modify, not two. Duplicated code, as small as it is, is never a good thing. Plus, it gives a name to the expression, making more obvious what it represents.

But now that I think about it, the else bock should use m_clientData.info.sampleCount, because the requested sample offset may be greater than that (and in this case, subtracting one sample still leads to EOF and the case that we're trying to avoid).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops. I think m_clientData.info.sampleCount was my original intention, but that somehow got left out while I was cleaning up my implementation. I'll fix that.

}
else
{
// FLAC decoder can't skip straight to EOF, so we short-seek by one sample and skip the rest
FLAC__stream_decoder_seek_absolute(m_decoder, (m_clientData.info.sampleCount / m_clientData.info.channelCount) - 1);
FLAC__stream_decoder_skip_single_frame(m_decoder);

// This was re-populated during the seek, but we're skipping everything in this, so we need it emptied
m_clientData.leftovers.clear();
}
}


Expand All @@ -283,7 +290,7 @@ Uint64 SoundFileReaderFlac::read(Int16* samples, Uint64 maxCount)
if (left > maxCount)
{
// There are more leftovers than needed
std::copy(m_clientData.leftovers.begin(), m_clientData.leftovers.end(), samples);
std::copy(m_clientData.leftovers.begin(), m_clientData.leftovers.begin() + maxCount, samples);
std::vector<Int16> leftovers(m_clientData.leftovers.begin() + maxCount, m_clientData.leftovers.end());
m_clientData.leftovers.swap(leftovers);
return maxCount;
Expand Down
5 changes: 2 additions & 3 deletions src/SFML/Audio/SoundFileReaderFlac.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,8 @@ class SoundFileReaderFlac : public SoundFileReader
////////////////////////////////////////////////////////////
// Member data
////////////////////////////////////////////////////////////
FLAC__StreamDecoder* m_decoder; ///< FLAC decoder
ClientData m_clientData; ///< Structure passed to the decoder callbacks
unsigned int m_channelCount; ///< number of channels of the sound file
FLAC__StreamDecoder* m_decoder; ///< FLAC decoder
ClientData m_clientData; ///< Structure passed to the decoder callbacks
};

} // namespace priv
Expand Down