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

Added a seekstart() to WAV.read_header to start at the beginning of t… #108

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sprague252
Copy link

This is a fix for open issue #98 in which a call to wavread with a IOStream that is not at the beginning of the file causes an error. The fix is simple: add a seekstart() command at the beginning of read_header to go to the beginning of the file before reading the header. Since wavread calls read_header, this resets the stream to the beginning of the file when the user calls wavread.

@ATell-SoundTheory
Copy link

What if the user doesn't want to parse the WAV file from the beginning of the stream? Embedded files are not that uncommon. I think it's better to remind the user that the seek position needs to be the start of the header before calling the corresponing functions.

@sprague252
Copy link
Author

What if the user doesn't want to parse the WAV file from the beginning of the stream? Embedded files are not that uncommon. I think it's better to remind the user that the seek position needs to be the start of the header before calling the corresponing functions.

That is one way to handle this. The problem is that the read_header function errors unless the stream is at the the beginning of the file. Otherwise, it does not read the b"RIFF" at the beginning. If you want to maintain the file position, read_header could read and save the current position, do its thing, and then restore the position. It would be problematic if the file position is not at the start of a sample or something, but you could just let the user assume responsibility for that.

@sprague252
Copy link
Author

Also, I am pretty sure the subrange parameter in wavread relies on being at the beginning of the file. (I have not tested this.)

@sprague252
Copy link
Author

Embedded files are not that uncommon.

I see your point here. I have never encountered an embedded WAVE file, but I suppose it is possible. In this case, you would want the stream to point to the beginning of the embedded file when wavread is called. wavread then calls read_header, and read_header finds the b"RIFF" at the beginning of the embedded file. My change is not going to give this behavior. I would have no problem if you rejected my push to maintain this behavior as long as there is clear documentation of how to use it.

@ssfrr
Copy link

ssfrr commented Feb 8, 2023

Yeah, from an API standpoint I think it’s reasonable that it’s the responsibility of the caller to have the stream at the beginning of the file being read.

also not all streams are seekable (e.g. streaming over a network socket)

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