[WIP] Rhythm Analyzer #2877
[WIP] Rhythm Analyzer #2877
Conversation
| void | ||
| DownBeat::findDownBeats(const float *audio, | ||
| std::vector<double> | ||
| DownBeat::beatsSD(const float *audio, |
There was a problem hiding this comment.
What is "SD"? Please spell out this abbreviation.
There was a problem hiding this comment.
Just some minor initial comments.
I noticed that you directly modified qm-dsp files. Please remember that we need to push those changes upstream and place a .patch file in lib/qm-dsp until then. Otherwise they might get lost with the next update of external code!
| for (int i = dbind; i < (int)beats.size(); i += timesig) { | ||
| downbeats.push_back(i); | ||
| } | ||
| return m_beatsd; |
There was a problem hiding this comment.
Why return a copy and store it as a member? If we store it as a member we can return a const-ref.
src/analyzer/analyzerrhythm.cpp
Outdated
| // leaving the outer for for now as it might be useful later | ||
| std::tuple<int, int> AnalyzerRhythm::computeMeter(std::vector<double> &beatsSD) { | ||
| // lower is included, higher excluded [) | ||
| int lowerBeatsPerBar = 4, higherBeatsPerBar = 5; |
There was a problem hiding this comment.
Split onto separate lines
src/analyzer/analyzerrhythm.cpp
Outdated
| size_t downbeatPositions = 0; | ||
| // convert beats positions from df increments to frams | ||
| for (size_t i = 0; i < beats.size(); ++i) { | ||
| double result = (beats.at(i) * m_stepSize) - m_stepSize / 2; |
There was a problem hiding this comment.
Try to avoid literals in the code. There is an engine constant for the fixed number (2) of channels.
src/analyzer/analyzerrhythm.cpp
Outdated
| //qDebug() << bpb; | ||
| //qDebug() << firstDownbeat; | ||
|
|
||
| constexpr bool useDownbeatOnly = true; |
There was a problem hiding this comment.
Move all tuneable constants into an anonymous namespace at the top of the file.
src/analyzer/analyzerrhythm.h
Outdated
|
|
||
| #include "analyzer/plugins/buffering_utils.h" | ||
| #include "util/samplebuffer.h" | ||
| #include "util/memory.h" |
There was a problem hiding this comment.
You can simply use <memory> in new code
We need to think this through. I made these small changes for having a nicer API to work with avoiding unnecessary processing, as the code I have deleted is moved into the rhythm analyzer itself, with some smalls modifications to compute the beats per bar together with the downbeat position. This didn't work very well. So there is no point in moving it to the library and pushing it upstream. Also, there is no point in pushing it without that code because it losses it's main functionality. However, as I move forward I suspect we would be better of forking qm-dsp and building a custom version that uses it's lower level methods (autocorrelation function, beats spectral difference) but moves the higher levels calculations (beats, downbeats, etc.) to the rhythm analyzer itself. I believe this would allow us better performance and improved accuracy. While having nice implementations of these higher levels features I have the impression that qm-dsp was not built as a "true" library for having all these features computed sequentially, together and in a performance-critical environment like Mixxx. It seems more like a collection of reference implementations of the algorithms described in their papers for proving they work as shown. So I believe we would better of having our own "library" using qm-dsp methods as a building block than to rely on it as a library. |
|
in this state of the project we should not limit ourselves by a chunky API. I it is a good idea to change it as it is needed. Later we can discuss with QM upstream how it is best way to contribute the wok back. |
|
Yeah, I'm all for upstreaming the changes. It would be good to only make changes to QM in separate commits and prefix the commit messages with |
|
We are currently in an experimental stage, where the upstream changes are quite unclear. |
|
@crisclacerda: Do you have receipt for manual testing? Do I need to merge other banches first? |
|
No, just build it and it will make a beat grid with only downbeats positions. |
|
I agree with @Holzhaus that it would be a good idea to separate changes to the upstream library into separate commits. |
|
This last commit fixes a big bug that was messing with the results and they should be pretty decent now. |
|
|
||
| namespace { | ||
|
|
||
| // This determines the resolution of the resulting BeatMap. |
There was a problem hiding this comment.
BeatMap is a legacy term. Should we start using the term Beats or BeatVector?
src/analyzer/analyzerrhythm.cpp
Outdated
| m_iMaxSamplesToProcess = m_iTotalSamples; | ||
| m_iCurrentSample = 0; | ||
|
|
||
| m_stepSize = m_iSampleRate * kStepSecs; |
There was a problem hiding this comment.
m_stepSize can be derived from m_iSampleRate.
Can we introduce a function getStepSize() const to calculate this on-demand rather than storing it in a member variable?
There was a problem hiding this comment.
Minimizing mutable state is a good idea.
src/analyzer/analyzerrhythm.cpp
Outdated
| m_iMaxSamplesToProcess(0), | ||
| m_iCurrentSample(0), | ||
| m_iMinBpm(0), | ||
| m_iMaxBpm(9999) { |
There was a problem hiding this comment.
| m_iMaxBpm(9999) { | |
| m_iMaxBpm(kMaxSupportedBpm) { |
src/analyzer/analyzerrhythm.cpp
Outdated
| } | ||
|
|
||
| bool AnalyzerRhythm::shouldAnalyze(TrackPointer pTrack) const { | ||
|
|
|
Can we add test cases with actual tracks? Normal + edge cases. |
|
Thanks for your review @hacksdump as well, looking forward to integrating this with your work! |
Yaays, I will work on that! |
We could use CC-licensed tracks, but I'd rather not add large MP3 files to this repo. Or are you talking about manual tests? |
First preference should be programmatically generated tracks. |
| loadSettings(); | ||
|
|
||
| // TODO(Cristiano) Create remaining connections.. | ||
| connect(bBeatsAndTempoEnabled, SIGNAL(stateChanged(int)), |
There was a problem hiding this comment.
These SIGNAL/SLOT macros are legacy from before Qt 5. Use the Qt 5 functor signal/slot syntax for new code.
https://wiki.qt.io/New_Signal_Slot_Syntax
There was a problem hiding this comment.
Can you point to me somewhere the new syntax is used in Mixxx? Wasn't able to make sense of it by only looking into the qt website example.
There was a problem hiding this comment.
I'd be okay with including short audio files in the Git repository. We don't need 5 minute long MP3s for tests, do we? |
src/analyzer/analyzerrhythm.cpp
Outdated
| --nonZeroCount; | ||
| } | ||
|
|
||
| std::vector<double> df; |
|
I cannot build this: Using Qt 5.13.2 |
That's the reason. |
|
Implemented a multi-feature beat detection. The analysis is only very slightly slower but accuracy has improved. |
|
There are a handful of old review comments that have not yet been addressed. Please don't let those fall through the cracks. |
src/analyzer/analyzerrhythm.cpp
Outdated
| bestBpm = .0; | ||
| bin = 0; | ||
| for (int k = tempogramMinBin; k <= tempogramMaxBin; k++){ | ||
| float w = ((float)k/tempogramFftLength)*(tempogramInputSampleRate); |
There was a problem hiding this comment.
Please use static_cast instead of C-style casts
…ams values as vamp plugin
* Adjust the Boom vs Tshak compensation curve with more test tracks * Use a smmoothing filter for the floating threshold after detecting a beat * Look also to the falling edge of a beat when finding SD peaks
| @@ -450,7 +460,18 @@ void AnalyzerRhythm::computeMeter() { | |||
| bestHierarchy = meter; | |||
| } | |||
| } | |||
| qDebug() << bestHierarchy; | |||
| auto pulseValues = accumulatedPulsesLenghtsAndWeights.keyValueBegin(); | |||
There was a problem hiding this comment.
Do we want to use auto here? I have no idea what type pulseValues is from looking at the right hand side of the assignment.
| auto beatsAtLeft = QVector<double>::fromStdVector(std::vector<double>( | ||
| m_resultBeats.begin() + beatStart, m_resultBeats.begin() + beatStart + middle)); |
There was a problem hiding this comment.
/home/jan/Projects/mixxx/src/analyzer/analyzerrhythmbpm.cpp:174:49: warning: ‘static QVector<T> QVector<T>::fromStdVector(const std::vector<T>&) [with T = double]’ is deprecated: Use QVector<T>(vector.begin(), vector.end()) instead. [-Wdeprecated-declarations]
174 | auto beatsAtLeft = QVector<double>::fromStdVector(std::vector<double>
| m_iMinBpm(0), | ||
| m_iMaxBpm(9999), | ||
| m_noveltyCurveMinV(pow(10, kNoveltyCurveMinDB / 20.0)) { | ||
| } |
There was a problem hiding this comment.
| } | |
| Q_UNUSED(pConfig); | |
| } |
| std::vector<double> AnalyzerRhythm::computeSnapGrid() { | ||
| int size = m_detectionResults.size(); | ||
|
|
||
| int dfType = 3; // ComplexSD |
| // Find peak beats within a window of 9 SDs (100 ms) | ||
| // This limits the detection result to 600 BPM | ||
| for (int i = 0; i < size; ++i) { | ||
| double beat = 0; |
| double bestBpm; | ||
| int bin; |
There was a problem hiding this comment.
Set but not used. Please always build with -DDEBUG_ASSERTIONS_FATAL=ON -DWARNINGS_FATAL=ON to catch these issues during development.
|
Cool, the beatgrid detection works quite good for my non-const tempo test track Robert Ouimet - Rainbow's Refunked. Beat positions look correct. In contrast, Serato completely failed to detect the beatgrid on this one when I tried it. |
|
On the other hand, the detected tempo on both T. Williams & James Jacob feat. Tim Deluxe - The Learning Process and MVZZIK - Lotta Love fluctuate quite a lot, especially in (but not limited to) the quiet sections without an audible beat. |
|
In the current state it's just the raw beat detector output, same as 2.3 beatmap. |
|
The UX in this branch is quite confusing. Why are there two beat analyzer pages in the preferences? Does the old one do anything? If not, why is it still there? It seems the beatgrid has to be manually cleared to get a track to analyze, otherwise a constant grid is generated from the BPM in the file tag?? |
Why is this still split between separate branches?? I don't understand what your plan is here. |
This is still very experimental and rough. Downbeat detection with QM downbeat alone has a very low accuracy, beats per bar detection is still unfinished and so is phrases. Although we can already "see" then on the meter code, we still need code to pick the right one..
The preferences there was just a quick mock and actually do not do anything. The beat detection is disabled in this branch. And the rhythm detection settings aren't connect to anything. It will always use the new analyzer unless there is still already beats object saved for the track. I have worked on some improvements in the preferences in a local branch. That will be useful in the early beta stages of this, the beat detection keep workings as it was, but by enabling the rhythm detection it disable the beat detection and use this instead? |
FWIW this bug is also present in master. If I disable the "assume constant tempo" option, a constant grid is still generated from the BPM file tag. |
I don't understand. Remove the old analyzer and its preferences entirely otherwise this is really confusing to use. |
|
This PR is marked as stale because it has been open 90 days with no activity. |
What is this?
A new analyzer capable of detecting the beats positions in frames and the first beat of a measure, of a phrase and of a section. A measure is a combination of beats, a phrase is a combination of measures that have a complete musical sense. A section is a combination of phrases that has a complete musical sense and represents a major structural part of track.
The new analyzer is designed to work with new beats class #2861
What this implement/fixes?