Conversation
This was happening for very short audio files
MarkKremer
left a comment
There was a problem hiding this comment.
Nice catch and thank you for the PR!
I have one question.
| if err == io.EOF { | ||
| d.pos = d.h.DataSize | ||
| return 0, false | ||
| } |
There was a problem hiding this comment.
How do you feel about not setting the position to the end? I think it might be slightly more "correct" if s.Position() reported the position the streamer has actually read to even though it doesn't match s.Len().
One thing I want to make sure of is that the reader may return some samples and an io.EOF at the same time. Therefore, it's better to not early return and instead, parse all the samples and return false at the end. Otherwise we may miss some samples. From the io.Reader docs:
When Read encounters an error or end-of-file condition after successfully reading n > 0 bytes, it returns the number of bytes read. It may return the (non-nil) error from the same call or return the error (and n == 0) from a subsequent call.
This is different from how beep.Streamer works. We only return !ok if n == 0. We can easily fix this by making this change at the end of the function instead of the early return:
- return n / bytesPerFrame, true
+ return n / bytesPerFrame, n > 0There was a problem hiding this comment.
I can accept the PR and make the change for you if you want. Just curious if you have a reason for doing it this way.
There was a problem hiding this comment.
That actually sounds more correct.
Honestly, I was just trying to fix my problem quickly. 😅
On the other hand, I am using the streamer's position to determine whether it has finished or not.
If the position remains in an "unfinished state", I'd have to use an extra bool set from beep.Callback to track whether the streamer has finished. Not a big issue, but doesn't feel as elegant.
I agree with ensuring all samples are streamed.
How about checking for EOF just before the "final" return?
if err == io.EOF {
d.pos = d.h.DataSize
}
return n / bytesPerFrame, n > 0I can push the changes as well.
There was a problem hiding this comment.
return n / bytesPerFrame, true
- return n / bytesPerFrame, n >
There was a problem hiding this comment.
return n / bytesPerFrame, true
- return n / bytesPerFrame, n >
There was a problem hiding this comment.
return n / bytesPerFrame, true
- return n / bytesPerFrame, n >
There was a problem hiding this comment.
Updatable packages: …
Packages version: 0.119.0-beta
There was a problem hiding this comment.
Updatable packages: …
Packages version: 0.119.0-beta
There was a problem hiding this comment.
Updatable packages: …
Packages version: 0.119.0-beta
There was a problem hiding this comment.
Updatable packages: …
Packages version: 0.119.0-beta

This was happening for very short audio files