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.

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

Fixed seeking in multi channel FLAC files. #1041

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@Foaly
Contributor

Foaly commented Jan 17, 2016

I was able to reproduce and fix #1040. As suggested in the forum thread you simply had to take out the channel count, because the FLAC decoder expected the absolute sample count.

I have one more improvement though: Update the documentation for sf::InputSoundFile::seek. There are two overloads. One taking a sf::Time and the other one a sf::UInt64 sample count. I think it would be helpful to make it clear in the doc that the sf::Time one uses an absolute value (not taking the channel count into account) and the sf::UInt64 overload uses a relative offset which considers the channel count. I mean it all makes sense, but I think we should document it a bit better.
Same goes for the sf::SoundFileReader::seek(). The documentation could give a little heads up that the channel Count has to be taken into account.
I suggest something like this:

////////////////////////////////////////////////////////////
/// \brief Change the current read position to the given sample offset
///
/// Attention: Bear in mind that the sample offset takes,
/// the channel count into account. The total number 
/// of samples is caculated like this: 
/// `sampleCount * sampleRate * channelCount`
///
/// If the given offset exceeds to total number of samples,
/// this function must jump to the end of the file.
///
/// \param sampleOffset Index of the sample to jump to, relative to the beginning
///
////////////////////////////////////////////////////////////

If nobody has anything against it, I will update the documentation.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Jan 19, 2016

Contributor

I added the documentation.

Contributor

Foaly commented Jan 19, 2016

I added the documentation.

@TankOs TankOs added this to the 2.4 milestone Jan 21, 2016

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 21, 2016

Member

Thanks @Foaly, this looks good. I have added some comments on minor formatting issues. :)

Member

TankOs commented Jan 21, 2016

Thanks @Foaly, this looks good. I have added some comments on minor formatting issues. :)

Maximilian Wagenbach
Fixed seeking in multi channel FLAC files.
Updated seek() documentation.
@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Jan 22, 2016

Contributor

Thank you for the feedback. I updated the documentation. Should be ready to merge now!

Contributor

Foaly commented Jan 22, 2016

Thank you for the feedback. I updated the documentation. Should be ready to merge now!

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 22, 2016

Member

Thanks a bunch. Btw, is this also applicable to other formats?

Member

TankOs commented Jan 22, 2016

Thanks a bunch. Btw, is this also applicable to other formats?

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Jan 22, 2016

Contributor

I don't quiet understand. What do you mean?

Contributor

Foaly commented Jan 22, 2016

I don't quiet understand. What do you mean?

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 22, 2016

Member

I was just wondering if the same fix would also apply to the other readers, except SoundFileReaderFlac. I haven't checked them myself, but you probably did.

Member

TankOs commented Jan 22, 2016

I was just wondering if the same fix would also apply to the other readers, except SoundFileReaderFlac. I haven't checked them myself, but you probably did.

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Jan 22, 2016

Contributor

Aaah ok. Yes I did check .ogg and .wav with a stereo sound file. The problem doesn't exist there. SoundFileReaderOgg does the exact same thing (dividing by channel count). This fix is actually based on the OGG implementation. For WAV we seek directly on the file stream, so the channel count has to be included.

Contributor

Foaly commented Jan 22, 2016

Aaah ok. Yes I did check .ogg and .wav with a stereo sound file. The problem doesn't exist there. SoundFileReaderOgg does the exact same thing (dividing by channel count). This fix is actually based on the OGG implementation. For WAV we seek directly on the file stream, so the channel count has to be included.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 22, 2016

Member

Perfect, thanks for looking that up. :) In that case: 👍 🐈

Member

TankOs commented Jan 22, 2016

Perfect, thanks for looking that up. :) In that case: 👍 🐈

@Foaly

This comment has been minimized.

Show comment
Hide comment
@Foaly

Foaly Feb 2, 2016

Contributor

Bump

Contributor

Foaly commented Feb 2, 2016

Bump

@eXpl0it3r eXpl0it3r added s:accepted and removed s:undecided labels Feb 18, 2016

@eXpl0it3r eXpl0it3r self-assigned this Feb 18, 2016

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 18, 2016

Member

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

Member

eXpl0it3r commented Feb 18, 2016

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

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 22, 2016

Member

Merged in c78c810

Member

eXpl0it3r commented Feb 22, 2016

Merged in c78c810

@eXpl0it3r eXpl0it3r closed this Feb 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment