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

Switch to soundfile for mp3 reading #18

Merged
merged 2 commits into from
Mar 22, 2023
Merged

Switch to soundfile for mp3 reading #18

merged 2 commits into from
Mar 22, 2023

Conversation

bogdanteleaga
Copy link
Collaborator

Fixes #16

Might be worth it switching to wav files in the future, but this is a little bit difficult at the moment so let's do this for the competition.

neutone_sdk/audio.py Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
neutone_sdk/audio.py Outdated Show resolved Hide resolved
neutone_sdk/audio.py Outdated Show resolved Hide resolved
@christhetree
Copy link
Collaborator

Thanks for the changes, just a few quick comments.

@bogdanteleaga
Copy link
Collaborator Author

Did a bit more refactoring taking in some ideas from the review, let me know what you think!

neutone_sdk/audio.py Outdated Show resolved Hide resolved
neutone_sdk/audio.py Outdated Show resolved Hide resolved
neutone_sdk/audio.py Outdated Show resolved Hide resolved
neutone_sdk/audio.py Outdated Show resolved Hide resolved
neutone_sdk/audio.py Outdated Show resolved Hide resolved
@christhetree
Copy link
Collaborator

Thanks, I added a few more comments

@bogdanteleaga
Copy link
Collaborator Author

I think there's two more things left:

  • Should we leave this write_mp3 public or make it private
  • Should we completely remove freeze? It's False by default right now

Let me know what you think

@christhetree
Copy link
Collaborator

I think it's fine leaving the write_mp3 public since it's extra work to extract the audio and sr from the AudioSample, if a user is doing that you'd think they would also notice the to_mp3() method available to them. I'll take care of the freezing in a PR I'm opening up in a few minutes where I'll remove the flag in the examples and display a warning if people use it.

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.

[P0] Torch 1.12 and 1.13 prevent model wrapping
2 participants