Fix for reading certain Ogg files on Linux via file-like objects.#436
Fix for reading certain Ogg files on Linux via file-like objects.#436
Conversation
| af.write(np.random.rand(2, 44100)) | ||
|
|
||
| # Add some random garbage past the end of the file to trigger the bug: | ||
| buf.write(b"\x00\x00") |
There was a problem hiding this comment.
This was the only reliable way I could find to repro the bug in test, apart from including a proprietary audio file as part of the tests. Trailing null bytes should not prevent the file from being parsed.
There was a problem hiding this comment.
If it's supposed to trigger the bug but now this PR fixed it, should we actually test this with a simple
with pedalboard.io.AudioFile(buf) as af: # <- the buffer with garbage in it.. which should now succeed parsing without any change
assert af.samplerate == 44100
and we can keep the other assert with a different but perfectly fine buffer
with pedalboard.io.AudioFile(ErrnoTriggeringFileLike(ok_buffer_made_not_ok_with_wrapper_class)) as af:
assert af.samplerate == 44100
There was a problem hiding this comment.
Not sure I fully follow - the code here does fail on Linux if I comment out the errno = 0 fix in the code under test.
There was a problem hiding this comment.
the code here does fail on Linux if I comment out the errno = 0 fix in the code under test.
This makes sense 👍
What I was challenging is this part:
# Add some random garbage past the end of the file to trigger the bug:
buf.write(b"\x00\x00")
I would argue you don't need to add random garbage because the test buffer is wrapped in ErrnoTriggeringFileLike which would trigger the bug in any case with any buffer, clean or not. (If I understand the class properly)
I was then suggesting to actually test without the ErrnoTriggeringFileLike and an unhealthy buffer :)
not important though!
hyperc54
left a comment
There was a problem hiding this comment.
Great dig and explanation, thank you
| af.write(np.random.rand(2, 44100)) | ||
|
|
||
| # Add some random garbage past the end of the file to trigger the bug: | ||
| buf.write(b"\x00\x00") |
There was a problem hiding this comment.
If it's supposed to trigger the bug but now this PR fixed it, should we actually test this with a simple
with pedalboard.io.AudioFile(buf) as af: # <- the buffer with garbage in it.. which should now succeed parsing without any change
assert af.samplerate == 44100
and we can keep the other assert with a different but perfectly fine buffer
with pedalboard.io.AudioFile(ErrnoTriggeringFileLike(ok_buffer_made_not_ok_with_wrapper_class)) as af:
assert af.samplerate == 44100
This PR fixes a subtle bug where certain Ogg Vorbis files can fail to decode when passed to Pedalboard via Python file-like objects, particularly on Linux.
The bug stems from an interaction between three layers of code:
read(),seek(), andtell().errnovariable as part of normal operation, but this error is not enough to cause the Python wrapper(s) to throw exceptions.libvorbisdecoder (specifically invorbisfile.c) checkserrnoafter I/O operations to detect errors:This results in valid Ogg Vorbis files failing to parse, as - if that last
libvorbisread returns 0 bytes, but some other code seterrno- then the entirereadcall fails. This can be spurred on by appending a couple null bytes to the end of the file, which I do in test here.