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.

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

Add Position and Len for flac #16

Merged
merged 2 commits into from
Mar 3, 2018
Merged

Conversation

cswank
Copy link
Contributor

@cswank cswank commented Feb 26, 2018

d.stream.Info.NSamples is uint64 so calling
int() on it could result in an overflow. I
have no idea how likely that is, but if it happens
then Position() will return -1.

I also don't know if this PR is the intent behind the beep.StreamSeeker interface, but it's working great for the app that I built using beep.

d.stream.Info.NSamples is uint64 so calling
int() on it could result in an overflow.  I
have no idea how likely that is, but if it happens
then Position() will return -1.
@faiface
Copy link
Owner

faiface commented Feb 26, 2018

Cool, thanks! @mewmew, can you confirm this is correct?

@cswank
Copy link
Contributor Author

cswank commented Feb 26, 2018

I think it might not be 100% correct. The songs I've been playing never get to Len(). Position() seems to stop short each time.

@cswank
Copy link
Contributor Author

cswank commented Feb 27, 2018

I think I fixed it with the 2nd commit: update d.pos on EOF error.

@mewmew
Copy link
Contributor

mewmew commented Feb 27, 2018

Cool, thanks! @mewmew, can you confirm this is correct?

Looks correct from what I can tell. Good job @cswank :) Now there is only one method left to implement! Seek.

For reference to anyone feeling brave enough, the upsteam issue tracking the implementation of seek for FLAC files is mewkiz/flac#16. It's up for grabs.

@cswank
Copy link
Contributor Author

cswank commented Feb 27, 2018

My app would benefit from Seek. I'll take a look and see how brave I feel after some reading.

@mewmew
Copy link
Contributor

mewmew commented Feb 27, 2018

My app would benefit from Seek. I'll take a look and see how brave I feel after some reading.

That's great to hear! Preemptively, I've started writing a hymn that will be sung around campfires for generations to come. ♩♪♬♩

@cswank
Copy link
Contributor Author

cswank commented Mar 3, 2018

Hey @faiface, this is a friendly reminder that this PR should be good to go.

@faiface
Copy link
Owner

faiface commented Mar 3, 2018

Oh, sorry, I forgot :) Here we go

@faiface faiface merged commit 4ec0906 into faiface:master Mar 3, 2018
@cswank cswank mentioned this pull request Jan 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants