search

NativeInstruments / ni-media

NI Media is a C++ library for reading and writing audio streams.
MIT License
235 stars 34 forks source link

Support gaps in wav and aiff files #26

Closed stk-ableton closed 5 years ago

stk-ableton commented 5 years ago

Currently, NI Media assumes that the chunks of .wav and .aiff files are exactly as long as the data they contain; in other words, after reading the data of a chunk it assumes that the stream position is now correct for reading the next chunk. In practice this is not always true; I have seen .wav files that have chunks that are longer than the data they contain, i.e. there are gaps (unused bytes) between chunks. I'm not sure these are valid according to the specification (http://www.martinreddy.net/gfx/2d/IFF.txt is somewhat vague about this), but every piece of audio software I have tried supports them, so NI Media should, too.

I wasn't sure how to cover the changes with tests; the last commit shows a hack that I created for my own use while working on the fix. Please advise what to do for real instead.

marcrambo commented 5 years ago

Hey there! Thanks a lot for the contribution!

Looks like some tests are failing on osx related to your changes: https://travis-ci.org/NativeInstruments/ni-media/jobs/518658015. Could you please have a look?

Unfortunately the windows tests are failing due to some unrelated vcpkg error. I will fix the vcpkg error on another PR.

Regarding the tests: It would be great to incorporate your python script into our generate_test_files.py. Then we can generate additional wav files with gaps and add them to our reference files, so they will be part of our test tuite. I can assist with that if you need help!

Thanks! Marc

stk-ableton commented 5 years ago

Thanks for the review. I pushed a commit that adds creating files with gaps to generate_test_files.py. I chose to create them for only some of the files so that there's not too much redundancy. Feel free to request any changes to what I did there. Once you're happy with it, I'll drop the earlier "WIP" commit.

Regarding the travis failure: I'm having a hard time believing that they are caused by my changes. By the time it gets to the error ("Could not read 'FORM' tag" and "Could not read 'RIFF' tag", respectively), it hasn't hit any code that I have changed. I'm afraid I have no idea what's going on there.

stk-ableton commented 5 years ago

Regarding the Travis failure again: it looks to me like the git lfs pull command fails:

Scanner error: missing object: 176a458f94e0ea5272ce67c36bf30b6be9caf623
Errors logged to /Users/travis/build/NativeInstruments/ni-media/.git/lfs/logs/20190415T083638.387903.log
Use `git lfs logs last` to view the log.

I don't understand why git lfs pull doesn't fail with an exit code in this case, and I don't understand why the error seems to happen consistently on the Mac row but not the Linux row. In any case, I don't think I can do much about it.

marcrambo commented 5 years ago

Hum, that's strange. I will have a closer look over the weekend and hopefully come back with a fix.

marcrambo commented 5 years ago

I have merged a fix for the CI issues. If you pull latest master into your branch the tests should pass.

stk-ableton commented 5 years ago

Thanks! I rebased the branch and dropped the "WIP" commit; it's ready to merge from my point of view. But do have a look at how I changed your generate_test_files.py script and let me know if you're happy with that.

stk-ableton commented 5 years ago

The AppVeyor failure looks like a timeout to me. I don't see a way for me to retrigger the job, could you do that?

marcrambo commented 5 years ago

I couldn't find a way to re-trigger the build either:( I would suggest removing the *.mp4 and *.ogg files and pushing these changes. This should trigger a new build

stk-ableton commented 5 years ago

I dropped the changes to the *.mp4 and *.ogg, and we're green now. 😄

marcrambo commented 5 years ago

Great! Thanks again for the contribution!