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

Seek for flac #111

Merged
merged 3 commits into from
Aug 17, 2021
Merged

Seek for flac #111

merged 3 commits into from
Aug 17, 2021

Conversation

cswank
Copy link
Contributor

@cswank cswank commented Jan 28, 2021

No description provided.

@@ -23,7 +23,14 @@ func Decode(r io.Reader) (s beep.StreamSeekCloser, format beep.Format, err error
}
}
}()
d.stream, err = flac.New(r)

rs, seeker := r.(io.ReadSeeker)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this part needs some thought.

Calling flac.Seek results in the use of a bufio.Buffer and NewSeek does not. According to the kindly mewkiz/flac folks (@mewmew) this could result in performance issues if *os.File is passed in as the io.Reader. It might be better to enable Seek more explicitly but I'm not sure how that might be implemented cleanly. Thoughts?

Copy link
Contributor

@mewmew mewmew Jan 28, 2021

Choose a reason for hiding this comment

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

It's probably fine for now. Calling seek won't work on a bufio.Reader as you mentioned. Lets first see if anyone run into real world performance issues with this, and then we can try to think of a solution.

Great to see seek support added in Beep! Good job @cswank.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good to me. Thanks @mewmew .

@@ -138,8 +145,11 @@ func (d *decoder) Position() int {
return d.pos
}

// p represents flac sample num perhaps?
Copy link
Contributor Author

@cswank cswank Jan 28, 2021

Choose a reason for hiding this comment

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

I left this comment because it looked like the API for seek has not really been determined? I'm not sure if sample number is really what you had in mind, but that's how the mewkiz/flac Seek works.

@cswank cswank mentioned this pull request Jan 28, 2021
jypelle added a commit to jypelle/mifasol that referenced this pull request Feb 14, 2021
@mewmew
Copy link
Contributor

mewmew commented Jun 23, 2021

@cswank, is this ready for merge? If so, lets ping @faiface :)

@cswank
Copy link
Contributor Author

cswank commented Jun 23, 2021

Yes, it’s ready. Yoo-hoo @faiface!

@dusk125 dusk125 merged commit 1c98bf6 into faiface:master Aug 17, 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.

None yet

3 participants