-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: add SignalSmith keylock engine #15902
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
Draft
acolombier
wants to merge
9
commits into
mixxxdj:main
Choose a base branch
from
acolombier:feat/signalsmith-stretch
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
224c6f3
feat: add SignalSmith keylock engine
acolombier b39a8e7
chore: bump up the minimum CMake version to 3.24
acolombier b3cfe16
fixup! feat: add SignalSmith keylock engine
acolombier dabeac9
fixup! chore: bump up the minimum CMake version to 3.24
acolombier 2e00ee3
fixup! feat: add SignalSmith keylock engine
acolombier fba8154
fixup! feat: add SignalSmith keylock engine
acolombier 044935a
Merge remote-tracking branch 'upstream/main' into feat/signalsmith-st…
acolombier 2244ee5
fixup! feat: add SignalSmith keylock engine
acolombier 600e262
fixup! feat: add SignalSmith keylock engine
acolombier File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This does not work with debian. We either need to contribute a debian folder for signal smith or copy signalsmith to our lib folder.
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.
What is the issue with debian? I guess the copy option could be viable.
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.
They do not allow to download third party software during the build process.
Uh oh!
There was an error while loading. Please reload this page.
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.
Oh this is going to be a pain for #15888. Though TagLib merged to contribution this morning and I assume will likely release this in the next version, Debian will likely not upgrade to it till the next major release (same goes for Ubuntu).
Not a problem for SignalSmith tho, so let's discuss that there. Is there any objection with using submodules? The SignalSmith stretcher internally relies on submodules too.
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.
Submodule do not really fit to the modular concept of vcpkg, Debian and RPM.
Having them in our lib folder is IMHO an unnecessary hassle. We don't have submodules yet. I prefer a literal copy in that case.
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.
I can try to contact the author to see if he has any interest.
Uh oh!
There was an error while loading. Please reload this page.
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.
It depends. Currently while it is under active development a lib folder copy is better.
When I look after years to Rubberband and Soundtouch the VCPKG and Linux Distro maintained version is "better". Original we had Soundtouch in our lib folder and have removed it as soon a suitable version was found in all distros.
The Linux maintainer anyway try to rip off everything from our libs folder. So we need to keep an eye on the distro maintained version anyway.
See also the mess with "libshout"
In case of taglib we can also decide if we want to ship it via lib folder or PPA.
The same is an issue with Portaudio which has now Pulse support, but is not yet released to use it.
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.
Thanks for the explanation.
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.
same for fedora/rpms afaik.
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.
Note that Geraint (the library author) came back to me and mentioned they haven't had interest to maintain a Debian package at present, but were happy for us to vendor the code and add a copy to
lib. They also offered to double license the library in case MIT wasn't ideal, but I believe we should be fine with the current license.I will capture that as an issue. It should be pretty straight forward to setup if we want to make SignalSmith stretch part of the stable release, but this solution allows us to easily update during the trial.