-
Notifications
You must be signed in to change notification settings - Fork 14
feat: support aiff files #59
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
base: master
Are you sure you want to change the base?
Conversation
|
Can you also add an aiff file to the test files, so we can verify that support works as intended? |
|
I merged the FileIO PR. Does it need a new release for this to merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds support for AIFF (Audio Interchange File Format) files to LibSndFile.jl. Although libsndfile natively supports AIFF, it was not previously enabled in LibSndFile.jl. The PR description also mentions a planned follow-up PR to FileIO.jl to register the AIFF file extension.
Changes:
- Added
format"AIFF"to the supported formats list in LibSndFile.jl - Implemented format code mapping for AIFF in libsndfile_h.jl
- Added comprehensive test coverage including read/write tests and FileIO integration tests
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/LibSndFile.jl | Added format"AIFF" to the supported_formats tuple, enabling AIFF support throughout the library via metaprogramming |
| src/libsndfile_h.jl | Added formatcode mapping for AIFF format type to the existing SF_FORMAT_AIFF constant |
| test/runtests.jl | Added comprehensive test coverage including AIFF reading/writing tests, test helper functions, reference file path, and FileIO integration test |
| test/440left_880right_0.5amp.aiff | Added binary reference test file with the same audio content as other format reference files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry I didn't notice your reply. I added a new test AIFF file and made some minor updates to runtests.jl |
Yes, FileIO needs a new release for the CI to pass. |
|
FileIO tests seem to be failing (perhaps not related to this). I'm happy to help with merging PRs - but may not be able to debug the issue. Would be nice to see this all through though! |
|
I checked the CI logs, and the error confirms that the tests are failing simply because the new FileIO version hasn't been released yet. The CI is using the currently installed version of FileIO, which doesn't yet have the AIFF registry entry we merged. It doesn't know how to handle .aiff files, leading to the Could you please tag a new release for FileIO? Once that's available, the tests should pass after a re-run. |
|
FileIO tests themselves need fixing. I opened an issue there. |
|
Registered a new upstream version. This will need FileIO 1.18.0 |
|
Thanks for the release! |
|
1.6 macOS latest is failing, but it doesn't seem to be a code issue. Looking at the logs, the runner is arm64, but the job requires 1.6. I don't think Julia 1.6 had an arm64 version for macOS. Either way, I think this can probably be ignored. |
Although
libsndfilesupports AIFF, it was not enabled inLibSndFile.jl. This PR addsformat"AIFF"to thesupported_formatslist and implements the necessary format code mapping insrc/LibSndFile.jl.I will also submit a PR to FileIO.jl to register the AIFF file extension.