Remove VAMP plugin support. #926
Merged
Conversation
This file contains 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
Member
rryan
commented
Apr 17, 2016
•
rryan
commented
- Removes VAMP entirely.
- Moves QM-DSP to lib/qm-dsp.
- Introduces AnalyzerPlugin, a class that individual algorithms for extracting features can sub-class -- with implementations for Queen Mary beats/key detectors and SoundTouch.
- AnalyzerBeats / AnalyzerKey use user preferences to decide which AnalyzerPlugin to use.
- Totally removed manual ConfigKey use for beat / key detection preferences -- now using a wrapper class like @daschuer introduced for ReplayGain preferences.
windowing of buffers. The Queen Mary detectors expect 50% overlapping windows of samples to be fed to them. They also use double samples instead of float.
Uses the qm-dsp library to calculate the key mode of the track. This is a re-write of the VAMP plugin so the parameters used are the same.
Uses SoundTouch and qm-dsp libraries to calculate BPM and do beat tracking. Replicates the logic of our VAMP plugins so the parameters to the algorithms are the same.
* Introduce BeatDetectionSettings helper class. * Remove UserSettingsPointer direct usage from DlgPrefBeats, AnalyzerBeats, AnalysisFeature, and Upgrade. * Delete beat_preferences.h.
* Introduce KeyDetectionSettings. * Use KeyDetectionSettings in DlgPrefKey and AnalyzerKey. * Delete key_preferences.h.
|
GitHub: |
build/depends.py
Outdated
| @@ -620,11 +672,13 @@ def sources(self, build): | |||
| "preferences/dialog/dlgprefsound.cpp", | |||
| "preferences/dialog/dlgprefsounditem.cpp", | |||
| "preferences/dialog/dlgprefwaveform.cpp", | |||
| 'preferences/dialog/dlgprefbeats.cpp', | |||
There was a problem hiding this comment.
Singe quotes, here and in some other places below.
|
Is it possible to rebase the external code changes out? |
The history is already a little complicated to do that. What about using: git diff rryan/remove-vamp mixxxdj/master src Or a graphical difftool? The changes to src/ are not that complicated. |
Or maybe my git rebase skills are too weak :) I can't think of an easy way. Do you have one? |
|
You could squash commits that are just basic removal, and squash the other commits that are changes. Github now supports easy viewing of individual commits inside one PR |
|
(git rebase -i for fun editor-window fine-tuning of the squashing) |
Yea, but I don't see how that's different from the situation now (I have a few bulk removal commits and then some smaller ones). Changing the ordering is tricky because there are merge commits with mass removals in master intermingled so reordering goes to hell immediately with git rebase -i. |
|
Ugh, I'm not good with the fast clicking today :). |
|
Nothing else from the review: LGTM. |
|
Address sanitizer: |
|
Build warning: I tried |
|
Hm, happens in master after #1624. Batch analysis with SoundTouch. /cc: @uklotzde |
|
Remove VAMP dependency from Ubuntu AppVeyor dependency list. |
Oops, bad merge at some point. |
Hm, also a bad merge. Fixed. |
|
Batch analysis goes fwoosh!! 🙌 |
The conflicts were actually pretty minimal, we touched mostly disjoint sets of files -- the only place we conflicted was where we both fixed the same issue in src/library/analysisfeature.h and I just reverted my changes in favor of @uklotzde's. |
|
Okay, I'll merge this now. |
|
Ubuntu Jenkins build failed. I think it's related to this, but I'm not certain: |
I remember that we experienced crashes once in a while in getRemainingFeatures() when running the VAMP plugins in a multi-threaded scenario without additional synchronization! |
|
I also noticed that the legacy SoundTouch beat detector was selected as the default instead of VAMP QM after starting Mixxx with this PR merged. |
That was due to an incorrect merge conflict resolution and fixed in 260071e. I suggest doing a clean checkout of current master for testing. |
Be-ing
added a commit
to Be-ing/mixxx
that referenced
this pull request
Apr 21, 2020
I think this was a regression when removing VAMP plugins in PR mixxxdj#926. The AllowBpmAboveRange setting is not exposed in the preferences window and I never recall it being shown to the user, nor anyone asking for it, so remove it.
This was referenced Aug 22, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.