-
Notifications
You must be signed in to change notification settings - Fork 44
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
Absolute seek #46
Absolute seek #46
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Robin <[email protected]>
Co-authored-by: Robin <[email protected]>
Co-authored-by: Robin <[email protected]>
Co-authored-by: Robin <[email protected]>
Co-authored-by: Robin <[email protected]>
Co-authored-by: Robin <[email protected]>
It builds on travis: https://travis-ci.org/github/mewkiz/flac/builds/756228717 |
// Seek seeks to the frame containing the given absolute sample number. The | ||
// return value specifies the first sample number of the frame containing | ||
// sampleNum. | ||
func (stream *Stream) Seek(sampleNum uint64) (uint64, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Close to ready for merge. searchFromStart
still needs some love, as it can reach two different panic states.
Edit: it looks like some review comments were hidden because this thread became too long. (i.e. not the resolved comments, but unresolved ones).
In particular: #46 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool to see the abs-seek branch being fuzzed as well!
The last change requested before merge is for searchFromStart
to return prev
instead of panic with unreachable. Other than that, it's just stylistic and personal preferences, so I'm fine to merge after adding return prev
.
Great work @karlek and @cswank! We finally get seek support integrated into the FLAC library.
Co-authored-by: Robin <[email protected]>
Co-authored-by: Robin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Reference implementation for #45
Reference poc: https://github.com/karlek/catamp/tree/seek-poc