-
Notifications
You must be signed in to change notification settings - Fork 2
Support macOS compilation with native libraries #21
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
Conversation
d6018ff
to
8ec0fa0
Compare
meson.build
Outdated
endif | ||
|
||
if get_option('cocoa') | ||
add_project_arguments('-DSAPF_COCOA', language: 'cpp') | ||
sources += 'src/makeImage.mm' | ||
add_project_arguments('-DSAPF_COCOA', '-std=c++17', language: ['cpp', 'objcpp']) |
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.
why is -std=c++17
needed here? we should have it in cpp_args
regardless
include/AudioToolboxSoundFile.hpp
Outdated
< 8000 /td> |
|
|
static std::unique_ptr<AudioToolboxSoundFile> open(const char *path); | ||
static std::unique_ptr<AudioToolboxSoundFile> open(const char* path, double theadSampleRate); |
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.
typo: "thead" with no 'r' (also in other files)
run: meson setup --buildtype release -Daccelerate=true | ||
-Daudiotoolbox=true -Dapple_lock=true -Dcarbon=true | ||
-Dcocoa=true -Dcorefoundation=true -Dcoremidi=true | ||
-Dmach_time=true -Ddispatch=true -Dmanta=false build |
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.
its not essential for this PR, but just noting that it would be nice to do the cross-platform build on macOS too. i guess it will require some rejigging of the build matrix, which will also be helpful for Nix builds which are not currently in CI.
All the issues should be resolved and I added cross-platform + native macOS builds. IDK why the CI isn't running for this PR but here's proof of it working: https://github.com/chairbender/sapf/actions/runs/14585318799 |
great, thanks! i think this is ready to merge, but it looks there is a conflict after merging #20. could you rebase? |
# Conflicts: # src/Play.cpp # src/SndfileSoundFile.cpp
13b12ab
to
3b57e06
Compare
b27ec29
to
cbe3a06
Compare
Should be fixed now! I moved the async writer into SoundFile as well since that made more sense along with the other SoundFile refactorings introduced in this PR. When calling SoundFile::create, you now pass an Note I'm not very experienced with rebasing type git workflows yet, and I'm not sure I follow when you ask me to rebase - rebasing would cause duplicate commits to be pushed if I try to rebase this branch on to latest main, which IIUC is usually not desirable. So maybe I'm misunderstanding the workflow you're suggesting. If you have some clarification or information I can check out lmk! (Instead, for now, I just merged it and squashed the post-merge commits after I fixed the various issues). |
indeed duplicate commits are not desirable. not sure what caused them here, but such situations can often be resolved with an interactive rebase for example. i know how to do it, but not really how to explain the process in writing 😄 anyway, this is ok, i will squash the whole PR into one and merge. thanks! |
Fixes #6
Fixes #19
It now builds with all of the macOS-specific libraries enabled. CI was updated to use these as well.
Everything should work (midi, file reading / writing...) but I don't happen to have a Manta lying around so can't test that.
Trying to summarize the specific fixes needed for this to work...
Play / AudioToolboxSoundFile / AUPlayerBackend
I did a bit of refactoring to AudioToolboxSoundFile / AUPlayerBackend. AUPlayerBackend has an xaf field, but AudioToolboxSoundFile has an mXAF field as well. I reworked it so we only need the mXAF field and removed the other. This is also why the destructors are changed.
Player now has a SoundFile field (so we can get the mXAF), and I also moved the path field into SoundFile itself since it seemed to make more sense to live there since it's the path of the SoundFile.
I moved some declarations / definitions around in Play.cpp to make it all work. There were some issues with things being declared before being used.
I'm going to have to make some tweaks to my existing audio reading / writing PRs to accommodate this slight change in the SoundFile structure but I think it's for the best - it makes more sense for Player to have a direct SoundFile reference rather than just a path IMO and for the path to travel together with the SoundFile.
Other Fixes