Skip to content
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

Missing libsamplerate binary for Mac M1 #9

Closed
sparkyb opened this issue Feb 18, 2022 · 11 comments
Closed

Missing libsamplerate binary for Mac M1 #9

sparkyb opened this issue Feb 18, 2022 · 11 comments

Comments

@sparkyb
Copy link
Contributor

sparkyb commented Feb 18, 2022

If this project is still being maintained, is there a plan to include a binary for M1 (ARM64) Macs or a "universal" .dynlib?

@nils-werner
Copy link

nils-werner commented Aug 19, 2022

Be aware that on M1 macs/modern versions of macOS you will face an additional issue: The way python-samplerate does callbacks is prohibited on hardened OSes (SELinux, macOS on M1 etc.). So even if you obtain a libsamplerate lib from somewhere else (Homebrew or Conda Staging), you will likely see the error message

MemoryError: Cannot allocate write+execute memory for ffi.callback(). You might be running on a system that prevents this. For more information, see https://cffi.readthedocs.io/en/latest/using.html#callbacks

I have changed the callback to the new style and it is working again.

However, due to the messy situation of

  1. where to obtain the dynlib/so from
  2. this project being practically abandoned

I am not 100% sure how to proceed with any of this.

@pkendrick
Copy link

Thanks for the fix @nils-werner I am currently using your fork, I clone and build libsamplerate then copy the dynlib to the appropriate location, it is a bit of a mess though as not only do I have to build it separately and copy things into the appropriate place, I also have to install from your fork rather than the deployed version. I would be great if we could get this fix into this package, but I also dont know how to proceed.

@tuxu
Copy link
Owner

tuxu commented Oct 25, 2022

Sorry for the radio silence here. I haven't had much time to dedicate to this project. I welcome contributions and would like to keep it alive.

The messiest part of the project is the build of libsamplerate for the target platforms. It doesn't feel great to copy binaries from my local machine and ship them. We should build it from source in a controlled environment, ideally in GitHub Actions. Stumbled across nixcrpkgs few days ago, which can target all platforms from Linux.

If someone wants to help with the maintenance of the project, please get in touch.

@Modiug
Copy link

Modiug commented May 16, 2023

I am using AlmaLinux 8.7 at work where I do not have root access. Here, libsamplerate is NOT installed by default. I don't know how other Python packages handle such dependencies (whether a library can be assumed to be available or not). As this is not a system library, I would suggest to add a binary libsamplerate for Linux as well.

@fakufaku
Copy link
Collaborator

@tuxu I am trying to address this issue by creating pybind11 based bindings that statically link the libsamplerate library.
I have implemented resample and Resampler, and am left with CallbackResampler to do. I am trying to make it a drop-in replacement for python-samplerate`.

The code is here.

The advantages are as follows

  • libsamplerate is built by the setup.py script and statically linked to the package
  • no dependency on a dynamic library, so no need to build for all platforms and distribute the binary
  • binary wheels can be built on github actions automatically and uploaded to pypi for the architectures available on github action
  • if no binary wheel is available, the package can be built locally provided a c++14 compiler is available (i.e., on M1 mac)
  • smaller binding code, pure c++, no need for ctypes
  • pybind11 and libsamplerate are automatically downloaded and built by the makefile, so there are zero python dependencies to install and built the package (beyond setuptools and friends)

@tuxu I'd be grateful if you can provide some feedback on how to proceed. Could this be a successor to your package or should I make it a different package on pypi?

@fakufaku
Copy link
Collaborator

I have added now the bindings for CallbackResampler so that it matches python-samplerate almost 1-to-1.
There are a couple of small differences.

  • I have not implemented the __enter__ __exit__ dunder since they did not really do anything.
  • My implementation accepts 2D array for single channel signals too so that the interface is consistent no matter the number of channels. 1D array are still accepted for single channel signals.

@fakufaku
Copy link
Collaborator

For those interested, I finally put some effort into making the bindings a 1-to-1 drop-in replacement for python-samplerate. I also went ahead, called it samplerate2 and uploaded it to pypi.

code: https://github.com/fakufaku/python-samplerate2

@tuxu
Copy link
Owner

tuxu commented Jul 26, 2023

@fakufaku This looks pretty neat! The only downside about using pybind11 is that you will have to build new wheels for every Python release. I needed a somewhat portable package when I started with this project, which is why I went with CFFI-based bindings to existing shared libraries. However, your GitHub Action together with the CMake-based build with minimal dependencies takes care of that complexity. Nice!

Things would actually become much simpler if we could adopt Py_LIMITED_API. A single wheel could be used with many different Python versions on the same platform. There's no support in pybind11, but nanobind by the same author will support it starting Python 3.12+. Rust's PyO3 already works with it today and might be an option, but would require yet another rewrite...

As for the future of this project: I think best way forward is to join forces, work on merging your changes into python-samplerate and add you as a maintainer here. Looking through your code, it seems like >80% of the work is already done, but there's still some things to do, like porting docstrings and code review.

I have not implemented the enter exit dunder since they did not really do anything.

The context manager gives the user a bit more control over memory allocations. We can guarantee that memory is released (src_delete) after the context is left and don't rely on reference counting. After typical usage like

with CallbackResampler(callback, ratio, converter_type) as resampler:
    ...

resampler still has a refcount > 0. It could matter for larger programs, but for typical usage keeping the state around a bit longer makes no difference.

@fakufaku
Copy link
Collaborator

fakufaku commented Jul 27, 2023

@tuxu Thanks for the feedback, this is very appreciated!

Things would actually become much simpler if we could adopt Py_LIMITED_API.

I didn't know about this! But waiting for 3.12 seems a bit far out there. But when it comes I don't think that switching to nanobind should be a big hurdle.
In the meantime all the package requires is a working C++14 compiler, so that should be pretty portable.
As you mentioned, binary wheels for mac (x86) and windows are built automatically. There are no github runners with M1/M2 mac (that I know of), so this will require to have a compiler installed too.

Rust's PyO3 already works with it today

Interesting, this could be a good opportunity to learn rust... 🦀 But... 😅

The context manager gives the user a bit more control over memory allocations.

Actually, after writing this, I went ahead and implemented it anyway 😄 It was still not de-allocating memory, but I have now checked how you handle the case were the object is re-used outside the context and did it in a similar way.
I have just pushed this to the repo.

As for the future of this project: I think best way forward is to join forces

I also think that would be the best outcome possible! This is a pretty big leap in terms of code and design so I'm not sure how you'd like to proceed.
Maybe:

  • add docstrings to new code
  • overwrite current code
  • keep the doc, etc

I can prepare a pull request and then you could review it.

@tuxu
Copy link
Owner

tuxu commented Jul 27, 2023

Sounds great! I have also just added you as a collaborator for this repo.

@fakufaku
Copy link
Collaborator

fakufaku commented May 7, 2024

Fixed by new version using pybind11 bindings (#10)

@fakufaku fakufaku closed this as completed May 7, 2024
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

No branches or pull requests

6 participants