Check usage of seek() for non-seekable file formats

[mgeier]

Currently, seek() is used at some points internally, e.g. when calculating self.frames within write().

This doesn't work with non-seekable formats, e.g. 'FLAC'.
seek() returns -1 on error, leading to self.frames being -1.

[bastibe]

That's bad. Is there any alternative?

[mgeier]

I hope so!

I wanted to wait for #59/#60/#65 (because there are changes regarding seek() in #60) and then try to find a solution to the current issue.

[bastibe]

I checked in the documentation and the libsndfile repository. It seems that FLAC files are indeed seekable, but you can't sf_seek(handle, 0, SEEK_END). The issue has been fixed in the meantime, but I'm not sure if the fix made it into our binaries yet.

I also remember using FLAC files a lot with the array interface and never experienced any problems. Would it be possible that you are experiencing the above bug?

[mgeier]

No, it works fine in 'r' mode with the version I'm using (1.0.25 on Debian/jessie).

The problem I described happens in write(). In 'w' mode, libsndfile claims that the FLAC file is not seekable and seek() indeed always returns -1 (not only when seeking to the position after the end).

[bastibe]

Oh, I see. The same happens on 1.0.25/OSX.

If you try to open a flac file in 'rw' mode, it throws an error: This file format does not support read/write mode.. Similarly, we raise an error if you try to read from a 'w' file. I think we should do the same thing for seeking. If the user tries to seek a non-seekable format, throw an error.

As for internal usages of seek, there are three cases:

• tell, which can default to self.frames if seek doesn't work.
• len, which can default to self.frames if seek doesn't work.
• an actual seek, which will fail with an error.

This requires self.frames to work even if seek doesn't. In the FLAC/'w' case this means that write would have to increment self.frames if seek is unavailable.

[mgeier]

If the user tries to seek a non-seekable format, throw an error.

I guess this would make sense, because Python throws an OSError (or IOError in Python 2) in this case, see https://docs.python.org/3.4/library/io.html#io.IOBase.seekable.

But then we should also raise an error in the other cases where sf_seek() returns -1, e.g. when the user specifies an invalid seek position.
We could just use libsndfile's error message, which would be for example: "Internal psf_fseek() failed."
In case of an unseekable file, the error message from libsndfile would be "Seek attempted on unseekable file type.".

I think we could just use our existing _handle_error() mechanism, which should take care of everything.

As for internal usages of seek, there are three cases:

• tell, which can default to self.frames if seek doesn't work.

To be consistent with Python, it should also raise an error.

• len, which can default to self.frames if seek doesn't work.

__len__() does already return self.frames.

• an actual seek, which will fail with an error.

This would mean we'd have to add a bit of code to avoid the error in the suggested changes of #60.
I guess we'd have to add a conditional to each seek() call:

if self.seekable():
    self.seek(whatever)

Alternatively, we could use try/catch, but I guess this would make the code a little harder to read.

[mgeier]

I found out that seekability not only depends on the file format and open mode. A file is also non-seekable if it is a named pipe (see #68). There are probably even other situation where a file could be non-seekable, who knows ...

I don't really know how to handle the seek() call in SoundFile.read(), any ideas?

[bastibe]

If I remember correctly seek is always available in read mode, so the seek in read shouldn't be a problem.

[mgeier]

It's not available when using pipes, neither in read mode nor in write mode, see #68.

[bastibe]

Then I guess we'll have to throw an error if frames is not given.

[mgeier]

I added some commits which should implement the discussed changes.

I re-factored some common functionality into two helper methods _check_frames() and _prepare_read().
I think their usage makes sense, but probably their names can be improved.

For testing the non-seekable stuff, I'm using the 'XI' format because it seems to be always non-seekable.

[bastibe]

This turns out to be quite a bit more complicated than I thought. But I think your handling of these edge cases is very good and helpful! I'll think some more about the helper method names. In general, the naming of our helper methods is somewhat arbitrary. I'll have to think about this some more.

[mgeier]

Any ideas for better names?

If not, I suggest to merge this and improve on the internal names later.