-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[WIP] Snapgrid #2917
base: main
Are you sure you want to change the base?
[WIP] Snapgrid #2917
Conversation
Most of the implementation is copied and slighly cleaned for consistency from beatutils. The rest comes from mixxxdj#2847
Change needed to do multi feature detection without redoing the STFTs
The change in qm-dsp broke queen mary beats
… into rhythm-detector
This is an interesting idea. On the community call the other day we briefly discussed an idea to store the analysis results separately from the user adjusted beats so the analysis results could be used to help the infer right/left features. Repsys seems to do something like this from the video demo, but I have yet to look at the repsys code in detail. This "snapgrid" may serve that purpose if we store it as an immutable part of the protobuf. As for whether it would be helpful to show this on the waveform sometimes, we can defer that decision until @hacksdump's project is further along. |
@satchelspencer we got this idea from Repsys. We're working on building a new beatgrid format and analyzer in Mixxx capable of working with variable tempos. We'd be curious to get your input on this work. |
I propose we call this the "sound grid" to contrast with the beat grid which marks musical time. @hacksdump give this a try. I think we should show this unobtrusively on the scrolling waveforms, for example small lines at the top & bottom of the waveforms, when it is relevant. This could be when moving the downbeats, cue points, and loop boundaries. We may consider turning quantize into a 3 state switch: off, beat quantize, sound quantize. Or maybe we could have beatloops and sync quantize to the beatgrid whereas cues and manual loops quantize to the sound grid. |
I think this is only useful for editing the beatgrid. The issue is that the snap grid is the raw onset detection output with a moving threshold. It is not ironed. Different instruments have a different form of unsets and we have the quantize jitter. An alternative name would be "Onsets". Or "Raw Onsets" or such. |
"Onsets" sounds good to me. |
What about "transients"? At least it seems like that's what you mean with "onsets". |
Onsets is the special term for exactly this: |
Can you elaborate on this? In my testing of #2877, the biggest problem was the analyzer detecting a very incorrect tempo when there are no loud rhythmic elements but the tempo is actually constant. Could this help with that somehow? |
Yes. This is in contrast to the queen Mary algorithm, which has no expectation of skipped or added beats build in. The new approach is to not move beats out of the onset region, to not produce double beats when beatmatching. |
Great, if we can overcome the problem of inaccurately detecting tempo changes when there are not prominent beats, I think users will not need to manually tell Mixxx to assume a constant tempo. |
* 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
This PR is marked as stale because it has been open 90 days with no activity. |
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.
There are merge conflicts and a bunch of compiler warnings now.
</widget> | ||
</item> | ||
<item row="6" column="0"> | ||
<widget class="QLabel" name="label_21"> |
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.
AutoUic: /home/jan/Projects/mixxx/src/preferences/dialog/dlgprefrhythm.ui: Warning: The name 'label_21' (QLabel) is already in use, defaulting to 'label_211'.
max = tempos.value(); | ||
} | ||
} | ||
return mode; |
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.
If tempos.hasNext()
is false, then mode is returned uninitialized.
|
||
} // namespace | ||
|
||
AnalyzerRhythm::AnalyzerRhythm(UserSettingsPointer pConfig) |
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.
/home/jan/Projects/mixxx/src/analyzer/analyzerrhythm.cpp: In constructor ‘AnalyzerRhythm::AnalyzerRhythm(UserSettingsPointer)’:
/home/jan/Projects/mixxx/src/analyzer/analyzerrhythm.cpp:52:52: error: unused parameter ‘pConfig’ [-Werror=unused-parameter]
52 | AnalyzerRhythm::AnalyzerRhythm(UserSettingsPointer pConfig)
| ~~~~~~~~~~~~~~~~~~~~^~~~~~~
std::vector<double> AnalyzerRhythm::computeSnapGrid() { | ||
int size = m_detectionResults.size(); | ||
|
||
int dfType = 3; // ComplexSD |
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.
unused variable
// here we detect if the segment has a constant tempo or not | ||
if (partLenght > m_beatsPerBar * 2) { | ||
int middle = partLenght / 2; | ||
auto beatsAtLeft = QVector<double>::fromStdVector(std::vector<double>( |
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.
/home/jan/Projects/mixxx/src/analyzer/analyzerrhythmbpm.cpp: In member function ‘std::tuple<QVector<double>, QMap<int, double> > AnalyzerRhythm::FixBeatsPositions()’:
/home/jan/Projects/mixxx/src/analyzer/analyzerrhythmbpm.cpp:158:49: error: ‘static QVector<T> QVector<T>::fromStdVector(const std::vector<T>&) [with T = double]’ is deprecated: Use QVector<T>(vector.begin(), vector.end()) instead. [-Werror=deprecated-declarations]
158 | auto beatsAtLeft = QVector<double>::fromStdVector(std::vector<double>(
| ^~~~~~~~~~~~~
Interesting, I will give this a try soon. |
This PR is marked as stale because it has been open 90 days with no activity. |
This is a proof of concept for a new beat detection strategy.
This PR is just an experimental PR on top of #2877
Currently we "move" the detected beats to positions they are considered musical reasonable.
Due to this for instance a loud onset that has nothing to do with the rhythm can trick the detection algorithm, because it is considered as beat.
Here we collect all possible onsets and experiential show it in beat grid.
The user can now try with looping these onsets what are actual beats.
Instead of shifting loud onsets it will snap to another possible onset.
This avoids notable double beat issues when syncing, because we are not off-beat, but just snapped to a less loud beat.
We still need to allow shifting in a small region, because of quantitation noise and jitter due to a live drummer or different characteristics of the drums. But this can now limited to a small unnoteabe region of +- 25 ms.
This algorithm is a good foundation for omitted, skipped or added beat detection.
I have tested this with
"Kahn - Helter Skelter"
https://www.youtube.com/watch?v=-byQwJ2Uoh0
At 0:31.80 the rhythm is shifted. The original qm algorithm has hard times to detect that. It is trying to iron out this region, which is musical wrong. Instead the beat grid should be snapped to the new rhythm.
Known Issues:
In a "Boom / Tshack" Rhythm the spectral difference is a lot more sensitive for Tshack than for Boom. I have used thresshold_smoothing to adjust the sensitivity. For detecting all Booms I had to increase the sensitivity to 0.05. This creates some false positives in the first region of the track. Here a value of 0.02 works perfect.
In other tracks with a deeper Boom, we need more sensitivity to detect it. Detecting the Booms is very important, because thy are likely the downbeats.
I think we should use this snapGrid when manually edit the beat grid. I am not sure if we should actually show it, because you can already predict the snap positions from the waveform.
@hacksdump @crisclacerda
What do you think?