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

Make flac.NewSeek() use a buffered reader too #72

Merged
merged 4 commits into from
Aug 8, 2024

Conversation

MarkKremer
Copy link
Collaborator

For one of the users of Beep, I discovered that flac.NewSeek() is very slow and caused stuttering. After inspection, flac.NewSeek() didn't seem to use a buffered reader (bufio.Reader) like it's brother flac.New() does. This caused millions of relatively slow syscalls.

Unfortunately bufio doesn't have a io.ReadSeeker, so I wrote my own based on bufio.Reader.

Let me know if I need to unit test this in some way.

Which is similar to how flac.New() works.
@mewmew
Copy link
Member

mewmew commented Aug 6, 2024

Great work!

For practical non-stuttering use of seek, having an underlying buffer is definitely needed. Great to see you spear-heading this Mark.

Let me know if I need to unit test this in some way.

As for unit tests, it would definitely be good to have a few unit tests related to the added seek support in your adaptation of the *bufio.Reader.

My suggestion would be to add a internal/bufseekio/test_readseeker.go unit test e.g. func TestSeek(t *testing.T) { ... }.

Try to ensure that subsequent seek calls "land" on the right offset, and that subsequent reads give the intended bytes. Also, try to check extremes, e.g. seek with negative offsets, seek at end/after end of buffer.

Cheers,
Robin

@MarkKremer
Copy link
Collaborator Author

MarkKremer commented Aug 6, 2024

Here ya go!

Edit: actually, I would like to see if I can improve them somewhat. The tests seem verbose & I think the out of range parts can be tested more. Let me have another stab at it.

@MarkKremer
Copy link
Collaborator Author

OK, I think I'm happy with it. :)

Copy link
Member

@mewmew mewmew left a comment

Choose a reason for hiding this comment

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

LGTM.

@mewmew mewmew merged commit 4109984 into mewkiz:master Aug 8, 2024
2 checks passed
@MarkKremer MarkKremer deleted the buffered-seek-decoder branch August 8, 2024 11:08
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.

2 participants