8000 Support macOS compilation with native libraries by chairbender · Pull Request #21 · ahihi/sapf · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 4 commits into from
May 6, 2025

Conversation

chairbender
Copy link
@chairbender chairbender commented Apr 19, 2025

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

  • AudioToolboxBuffers had some issues in how the buffers were being accessed / set which I fixed.
  • Applied this fix which was noted but not yet upstreamed: Stereo >sf not giving correct output file lfnoise/sapf#4 (comment)
  • Thanks to the tests we added I may have found a bug which I guess probably also exists in upstream with the minus operation, which I fixed. I think the official docs for the subD op in accelerate are wrong on the order of parameters. Interestingly the other usage of subD (frac) seemingly has this correct.
  • adjust the meson build flags to get all the frameworks and macOS specific stuff to be used when the relevant flags are specified
  • enable the relevant meson options in the CI build for macOS

@chairbender chairbender marked this pull request as ready for review April 19, 2025 04:10
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'])
Copy link
Owner

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

< 8000 /td>
static std::unique_ptr<AudioToolboxSoundFile> open(const char *path);
static std::unique_ptr<AudioToolboxSoundFile> open(const char* path, double theadSampleRate);
Copy link
Owner
@ahihi ahihi Apr 21, 2025

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
Copy link
Owner

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.

Fix "thead" typo, remove redundant param from meson
build, add cross-platform macOS build alongside
native build.
@chairbender
Copy link
Author

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

@ahihi
Copy link
Owner
ahihi commented Apr 27, 2025

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
@chairbender
Copy link
Author

great, thanks! i think this is ready to merge, but it looks there is a conflict after merging #20. could you rebase?

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 async parameter that determines whether you plan to use async or non-async writing (only one or the other is supported for a given SoundFile, to minimize the resources allocated depending on the use case). Once you do it, you can call the corresponding write or writeAsync method for synchronouse / async output.

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).

@ahihi
Copy link
Owner
ahihi commented May 6, 2025

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!

@ahihi ahihi merged commit 284585c into ahihi:main May 6, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build for OSX in CI with OSX-specific libs macOS support
2 participants
0