Changing tempos support #2930

crisclacerda · 2020-07-10T22:24:55Z

This is my working product submission for GSoC 2020.
While the core of my project was adding a new rhythm analyzer #2877 that is still a working a progress.

The new analyzer however relies heavy on the current beat detector which works quite well, but suffers from two main issues that his pull request tries to solve and are necessary for detecting downbeat and phrases.

Fluctuating beat lengths (tempo) around a center a value.
Loosing track of "right" beat on percussion less or effects heavy parts.

Previously Mixxx solved this issue by not allowing any tempo changes and making a perfect fixed tempo grid using the beat detector output only to compute the average tempo and the most likely beat phase, while that work very well for tracks that have a unique constant tempo it failed heavy on tracks that have a abrupt tempo change or an unsteady tempo.

This pull request on the other hand attempts to find regions of stable tempo and make a independently grid for each of them. To allow unsteady tempos, instead of making a fixed tempo grid it tries to attempts to a grid for all the beats that fall within a 25ms threshold which is close to the human hearing perception threshold of distinguishable sounds.

It works very well for unsteady tempo and is able to follow smooth or abrupt tempo changes well while keep a constant grid, but still fails on some case when the beat detector output is "very" problematic and does not completely eliminate the need for current fixed tempo grid approach.

This can still be improved by combining the fixed grid approach with the stable regions and make one fixed grid instead of the ironing approach of 25ms, the challenge is then to determine which algorithm to use.

Some comparisons with the 2.3 results, showing where it does well and where it still fails: (blue is 2.3, red this PR)

Tracks with unsteady tempos:

Tracks that have more than one constant tempo:

Track with one constant tempo:

src/track/beatstats.h

src/track/beatutils.cpp

Be-ing · 2020-07-11T03:12:02Z

My impressions from some brief testing. First, wow! I can set useful loops on jazz tracks! This is going to be awesome with #2194!

In general, this does quite well with tracks that have a constant tempo. However, it often gets thrown off in sections where there is not a prominent repeating rhythm even though I know the tempo there is really the same as the section before because the whole track was clearly made in a DAW set at one specific tempo. I have no ideas how the analyzer could do better in these cases other than the user manually telling it that there is a constant tempo, so maybe #2804 would be useful after all. This does well enough that I think we can use it by default and leave it to the user to manually force a constant tempo when desired.

The analyzer frequently says the tempo is double what it really is. I think this happens more often than in master, but I haven't tried to quantify that.

Be-ing · 2020-07-11T03:20:16Z

I suspect the stability of the momentary BPM shown to the user could improve considerably when we have the downbeats and can report the tempo of the current bar.

Be-ing · 2020-07-11T03:25:56Z

I have no ideas how the analyzer could do better in these cases other than the user manually telling it that there is a constant tempo

Perhaps if the confidence in the beat positions is low, disregard the detected beats for that phrase or section and extrapolate from the surroundings. This could obviate the need for much manual intervention compared to the algorithm in this PR. Of course, it would help to have the sections and phrases identified for this.

daschuer

This looks like a good progress, Thank you.
I have left some comments.

Is this ready for master merge finally?

What is the input? The original QM beats with the ironed tempo changes?
We need to consider the right stack of changes ...

What are your ideas?

daschuer · 2020-07-12T12:36:44Z

src/track/beatfactory.cpp

@@ -119,7 +119,8 @@ mixxx::BeatsPointer BeatFactory::makePreferredBeats(const Track& track,
        pGrid->setSubVersion(subVersion);
        return mixxx::BeatsPointer(pGrid, &BeatFactory::deleteBeats);
    } else if (version == BEAT_MAP_VERSION) {
-        mixxx::BeatMap* pBeatMap = new mixxx::BeatMap(track, iSampleRate, beats);
+        auto fixedBeats = BeatUtils::FixBeatmap(beats, iSampleRate, iMinBpm, iMaxBpm);
+        mixxx::BeatMap* pBeatMap = new mixxx::BeatMap(track, iSampleRate, fixedBeats);
        pBeatMap->setSubVersion(subVersion);
        return mixxx::BeatsPointer(pBeatMap, &BeatFactory::deleteBeats);


Legacy issue:

I think we should immediately hand over the raw pointer after construction to clarify the ownership.
subVersion can also be set during the constructor.

return mixxx::BeatsPointer( new mixxx::BeatMap(track, iSampleRate, fixedBeats, subVersion), &BeatFactory::deleteBeats);

The same above.

No constructor of beatmap receives subversion as parameter, since we are getting rid of that anyways I think it's not worth the trouble, right?

daschuer · 2020-07-12T12:39:13Z

src/track/beatutils.cpp

 #include "util/math.h"

 // we are generous and assume the global_BPM to be at most 0.05 BPM far away
 // from the correct one
-#define BPM_ERROR 0.05
+constexpr double kBpmError = 0.05;


kMaxBpmError

daschuer · 2020-07-12T12:41:24Z

src/track/beatutils.cpp

@@ -29,20 +31,178 @@ const int kHistogramDecimalPlaces = 2;
 const double kHistogramDecimalScale = pow(10.0, kHistogramDecimalPlaces);
 const double kBpmFilterTolerance = 1.0;

+QMap<int, double> BeatUtils::findTempoChanges(
+    QMap<double, int> tempoFrequency, QList<double> tempoList) {


This whole PR contains some formatting issues. Did you try to install our pre-commit hook?

Only clang-format.
I will double check that

daschuer · 2020-07-12T12:45:58Z

src/track/beatutils.cpp

+    // Here we are going to track the tempo changes over the track
+    for (double tempo : tempoList) {
+        currentBeat += 1;
+        double newStableTempo = filterTempo(tempo);


Due to the median we introduce a delay of 1/2 median window. Is this considered?

Yes, this is considered when assigning currrentBeat value to lastBeatChanhge

daschuer · 2020-07-12T12:49:43Z

src/track/beatutils.cpp

+        if (newStableTempo == stableTemposAndPositions.last()) {
+            continue;
+        } else if (stableTemposAndPositions.last() != tempoFrequency.lastKey() and
+                newStableTempo == (tempoFrequency.find(stableTemposAndPositions.last()) + 1).key()) {


This is hard to understand. I guess we need some more comments.

daschuer · 2020-07-12T12:52:12Z

src/track/beatutils.cpp

+    MovingMedian filterTempo(5); // 5 is the lenght our window
+    int currentBeat = -1;
+    int lastBeatChange = 0;
+    QMap<int, double> stableTemposAndPositions;


Is this "stableTemposByPosition"?

Yes, key is position of a new stable tempo and value the tempo

daschuer · 2020-07-12T12:58:48Z

src/track/beatutils.cpp

+            // because find will return an iterator pointing to end that we will *
+            if (tempoFrequency.contains(newStableTempo)) {
+                lastBeatChange = currentBeat - filterTempo.lag();
+                stableTemposAndPositions[lastBeatChange] = newStableTempo;


I wonder why the kMaxSecsPhaseError is not relevant here?

Lets say we have a continuously increasing tempo. We can keep the grid constant until the kMaxSecsPhaseError is exceeded, right?

Once we are behind by 25 ms we can try to catch up with a faster tempo, but will fall behind soon because of the increasing tempo.

So we are always behind ... :-/ difficult.

What do you think?

Here we do not care about the beat positions just trying to understand tempo changes.

daschuer · 2020-07-12T13:01:49Z

src/track/beatutils.cpp

+        auto splittedAtTempoChange = QVector<double>::fromStdVector(std::vector<double>(
+                rawBeats.begin() + beatStart, rawBeats.begin() + beatEnd));
+        double bpm = calculateBpm(splittedAtTempoChange, sampleRate, minBpm, maxBpm);
+        fixedBeats << calculateFixedTempoBeatMap(splittedAtTempoChange, sampleRate, bpm);


Cool, Can we force a minimum fixed region of one measure? Can we align the change to downbeats?
That makes musically pretty much sense. But do we have the info?

That's for #2877, we dont have that info here

Be-ing · 2020-07-13T11:41:39Z

I am still unclear what the benefit of this PR is versus #2877. #2877 is already writing the output data to the legacy BeatMap class.

crisclacerda · 2020-07-13T11:42:14Z

This looks like a good progress, Thank you.
I have left some comments.

Is this ready for master merge finally?

What is the input? The original QM beats with the ironed tempo changes?
We need to consider the right stack of changes ...

What are your ideas?

After addressing the issues you mention I believe this is is ready to merge.
The input is just the QM beats.

Be-ing · 2020-07-13T11:42:17Z

I am concerned that splitting the work between these two branches is creating more work for no good reason.

crisclacerda · 2020-07-13T11:43:35Z

I am still unclear what the benefit of this PR is versus #2877. #2877 is already writing the output data to the legacy BeatMap class.

This is, almost, ready to merge and is self contained.
#2877 there is still tons of work to be done.
Especially now that I want to add the new features while integrating it to the new beats class.

crisclacerda · 2020-07-13T11:47:56Z

Since, it's a hard requirement for us to have code merged into master by the end of summer, in a worst case scenario this can be that code.

Be-ing · 2020-07-13T11:49:58Z

If you and @daschuer think this is a good path forward I'm okay with it. I'm just a little concerned that continuing to work on this is merely creating extra work to integrate it back into #2877.

crisclacerda · 2020-07-13T15:02:09Z

If you and @daschuer think this is a good path forward I'm okay with it. I'm just a little concerned that continuing to work on this is merely creating extra work to integrate it back into #2877.

Also, #2877 can be a experimental, work only, branch for implementing the new features, but then each one can be merged in a individual PR like this one?

crisclacerda · 2020-07-17T00:15:04Z

@ywwg did you get a chance to try this?

crisclacerda · 2020-07-17T00:16:15Z

This should be ready to merge now?
Would be great to get this on master so we can get more feedback for #2877 finally

Be-ing · 2020-07-30T02:14:18Z

src/track/beatutils.cpp

+void BeatUtils::RemoveSmallArrhythmic(
+        QVector<double>& rawBeats,  int sampleRate, QMap<int, double> &stableTemposByPosition) {
+    // A common problem the analyzer has is to detect arrhythmic regions
+    // of tacks with a constant tempo as in a different unsteady tempo. 


Suggested change

// of tacks with a constant tempo as in a different unsteady tempo.

// of tracks with a constant tempo as a different, unsteady tempo.

Be-ing · 2020-07-30T02:17:31Z

src/track/beatutils.cpp

+    // We arbitraly remove these arrhythmic regions if they are short than 16s
+    auto positionsWithTempoChange = stableTemposByPosition.keys();
+    auto tempoValues = stableTemposByPosition.values();
+    QVector<double> fixedBeats;


I'm unclear which meaning of "fixed" this is using. Are the beats "fixed" as in corrected, or "fixed" as in unmoved?

changed to anchored

src/track/beatutils.cpp

Be-ing · 2020-07-30T03:02:04Z

The new commit is a big improvement for handling sections without loud rhythms. It still sometimes detects significant speed ups in these sections, but I don't see the huge fluctuations like there were before.

daschuer

Some more comments

daschuer · 2020-07-31T00:00:46Z

src/track/beatstats.cpp

+            max = tempo.value();
+        }
+    }
+    return mode;


This produces a compiler warning:

/home/daniel/workspace/advanced_autodj_2/src/track/beatstats.cpp:29:12: warning: ‘mode’ may be used uninitialized in this function [-Wmaybe-uninitialized] return mode;

daschuer · 2020-07-31T00:28:16Z

src/track/beatutils.cpp

+    int beatsToFilterMeterChanges =  (10 / (60 / median)) * 2; 
+    if (!(beatsToFilterMeterChanges % 2)) {beatsToFilterMeterChanges += 1;}
+    MovingMedian filterTempo(beatsToFilterMeterChanges); 
+    MovingMode stabilzeTempo(beatsToFilterMeterChanges);


Can we write the lines above as

auto medianFilterTempo = MovingMedian(beatsToFilterMeterChanges); auto modeFilterTempo = MovingMode(beatsToFilterMeterChanges);

And with improved names?
This is more pleasant to read.

daschuer · 2020-07-31T00:35:24Z

src/track/beatutils.cpp

+    }
+    // Since we use the median as a guess first and last tempo
+    // And it can not have values outside from tempoFrequency
+    const double median = computeSampleMedian(sortedTempoList);


is this a BPM value?
Can we call it "medianBpm" to clarify this?

daschuer · 2020-07-31T00:38:47Z

src/track/beatutils.cpp

+    // Forming a meter perception takes a few seconds, so we assume sections of consistent 
+    // metrical structure to be at least around 10s long. So we use a window of the double
+    // of that in our filtering.  
+    int beatsToFilterMeterChanges =  (10 / (60 / median)) * 2; 


beats... sounds as it is a list of beats.
This is the window size for the moving filters, right?
Can we find a name that reflects this?

FilteringWindowInBeats? Do you have suggestions?

src/track/beatstats.cpp

daschuer · 2020-07-31T00:58:48Z

src/track/beatutils.cpp

+                rawBeats.begin(), rawBeats.begin() + positionsWithTempoChange[1]));
+
+    for (int i = 2; i < positionsWithTempoChange.size(); i+=1) {
+        int smallInBeats =  16 / (60 / tempoValues[i-1]);


what is this value bpm?

daschuer · 2020-07-31T01:00:08Z

src/track/beatutils.cpp

+        if (lenghtOfChange <= smallInBeats) {
+            stableTemposByPosition.remove(limitAtLeft);
+            double beatOffset = rawBeats[limitAtLeft];
+            double beatLength = floor(((60.0 * sampleRate) / previousTempo) + 0.5);


what is the rounding for?

I think that needs to be removed.

If we add beats here, the later ironing will treat them as strong reference points and will apply the 0,25 ms rule to it. But that is not necessary. We can just advance to the next beat and put them later exactly on the calculated average.

hm, just removing the beats was my first idea but that does not work without an additional structure to keep track that we need to add the beats in this region later, and the big jump will result in very large beat length that will completely mess the average so removing them adds a lot of bookkeeping.
Maybe we can set the beats to -1 or something like that and use that idea of beats not increasing as a way to tell not to consider these beats on iron code and just add the avarage length. I will try that, should work...

Holzhaus · 2020-07-31T09:58:31Z

I'm getting a bunch of warnings with Qt 5.15:

/home/jan/Projects/mixxx/src/track/beatstats.cpp: In member function ‘virtual double MovingMode::compute()’:
/home/jan/Projects/mixxx/src/track/beatstats.cpp:29:12: warning: ‘mode’ may be used uninitialized in this function [-Wmaybe-uninitialized]
   29 |     return mode;
      |            ^~~~
/home/jan/Projects/mixxx/src/track/beatutils.cpp: In static member function ‘static void BeatUtils::RemoveSmallArrhythmic(QVector<double>&, int, QMap<int, double>&)’:
/home/jan/Projects/mixxx/src/track/beatutils.cpp:98:36: 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]
   98 |     fixedBeats << QVector<double>::fromStdVector(std::vector<double>(
      |                                    ^~~~~~~~~~~~~
In file included from /usr/include/qt/QtCore/qlist.h:48,
                 from /usr/include/qt/QtCore/qhash.h:46,
                 from /usr/include/qt/QtCore/qdebug.h:45,
                 from /usr/include/qt/QtCore/QtDebug:1,
                 from /home/jan/Projects/mixxx/src/track/beatutils.cpp:9:
/usr/include/qt/QtCore/qvector.h:306:30: note: declared here
  306 |     static inline QVector<T> fromStdVector(const std::vector<T> &vector)
      |                              ^~~~~~~~~~~~~
/home/jan/Projects/mixxx/src/track/beatutils.cpp:99:82: 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]
   99 |                 rawBeats.begin(), rawBeats.begin() + positionsWithTempoChange[1]));
      |                                                                                  ^
In file included from /usr/include/qt/QtCore/qlist.h:48,
                 from /usr/include/qt/QtCore/qhash.h:46,
                 from /usr/include/qt/QtCore/qdebug.h:45,
                 from /usr/include/qt/QtCore/QtDebug:1,
                 from /home/jan/Projects/mixxx/src/track/beatutils.cpp:9:
/usr/include/qt/QtCore/qvector.h:306:30: note: declared here
  306 |     static inline QVector<T> fromStdVector(const std::vector<T> &vector)
      |                              ^~~~~~~~~~~~~
/home/jan/Projects/mixxx/src/track/beatutils.cpp:118:44: 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]
  118 |             fixedBeats << QVector<double>::fromStdVector(std::vector<double>(
      |                                            ^~~~~~~~~~~~~
In file included from /usr/include/qt/QtCore/qlist.h:48,
                 from /usr/include/qt/QtCore/qhash.h:46,
                 from /usr/include/qt/QtCore/qdebug.h:45,
                 from /usr/include/qt/QtCore/QtDebug:1,
                 from /home/jan/Projects/mixxx/src/track/beatutils.cpp:9:
/usr/include/qt/QtCore/qvector.h:306:30: note: declared here
  306 |     static inline QVector<T> fromStdVector(const std::vector<T> &vector)
      |                              ^~~~~~~~~~~~~
/home/jan/Projects/mixxx/src/track/beatutils.cpp:120:68: 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]
  120 |                     rawBeats.begin() + positionsWithTempoChange[i]));
      |                                                                    ^
In file included from /usr/include/qt/QtCore/qlist.h:48,
                 from /usr/include/qt/QtCore/qhash.h:46,
                 from /usr/include/qt/QtCore/qdebug.h:45,
                 from /usr/include/qt/QtCore/QtDebug:1,
                 from /home/jan/Projects/mixxx/src/track/beatutils.cpp:9:
/usr/include/qt/QtCore/qvector.h:306:30: note: declared here
  306 |     static inline QVector<T> fromStdVector(const std::vector<T> &vector)
      |                              ^~~~~~~~~~~~~
/home/jan/Projects/mixxx/src/track/beatutils.cpp: In static member function ‘static QVector<double> BeatUtils::FixBeatmap(QVector<double>&, int, double, double)’:
/home/jan/Projects/mixxx/src/track/beatutils.cpp:148:55: 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]
  148 |         auto splittedAtTempoChange = QVector<double>::fromStdVector(std::vector<double>(
      |                                                       ^~~~~~~~~~~~~
In file included from /usr/include/qt/QtCore/qlist.h:48,
                 from /usr/include/qt/QtCore/qhash.h:46,
                 from /usr/include/qt/QtCore/qdebug.h:45,
                 from /usr/include/qt/QtCore/QtDebug:1,
                 from /home/jan/Projects/mixxx/src/track/beatutils.cpp:9:
/usr/include/qt/QtCore/qvector.h:306:30: note: declared here
  306 |     static inline QVector<T> fromStdVector(const std::vector<T> &vector)
      |                              ^~~~~~~~~~~~~
/home/jan/Projects/mixxx/src/track/beatutils.cpp:149:74: 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]
  149 |                 rawBeats.begin() + beatStart, rawBeats.begin() + beatEnd));
      |                                                                          ^
In file included from /usr/include/qt/QtCore/qlist.h:48,
                 from /usr/include/qt/QtCore/qhash.h:46,
                 from /usr/include/qt/QtCore/qdebug.h:45,
                 from /usr/include/qt/QtCore/QtDebug:1,
                 from /home/jan/Projects/mixxx/src/track/beatutils.cpp:9:
/usr/include/qt/QtCore/qvector.h:306:30: note: declared here
  306 |     static inline QVector<T> fromStdVector(const std::vector<T> &vector)
      |                              ^~~~~~~~~~~~~

daschuer · 2020-07-31T14:17:29Z

Here are my test:

From the compare results, I think this is not yet in a merge-able state.
As you can see in the Kahn track, it fails to keep the fist tempo change.

Instead of a smoothly floating bpm, we have corrective spikes with a different bpm.
I think we need to change the strategy.

I think to something like this.
Tweak for

minimum Bpm changes
limit BPM changes to almost unnoticeable 0,3 %
limit beat offset to 25 ms as almost unnoticeable.
allow single beats to be off by 100 ms as as almost unnoticeable.

Than, pick a beat in the middle and keep it const until 0,25 ms offset is exceeded. at both ends.
Than correct the bpm by 0,3 % at half way to each sides.

Mask single exceptional beats 25 ms ... 100 ms difference to the const grid build up by two neighbors at each side,

Find const max region: 
|------------------------M--------|
Adjust Tempo at half, or after a beat that is at almost max in the opposide direction. 
|-------------|-----------M----|--
Move from there to the next 25 ms exceeded beat
|-------------|
Still the same beat? Adjust tempo in the middle 
|------|------|
Works? 
|------| 
Again ..

crisclacerda · 2020-07-31T17:31:39Z

Instead of a smoothly floating bpm, we have corrective spikes with a different bpm.
I think we need to change the strategy.

Yes, I agree that this is strategy is not very good. I will work on improve this.
Thanks for the nice graphic and the suggestions =)

crisclacerda · 2020-07-31T17:34:39Z

I'm getting a bunch of warnings with Qt 5.15:

These are because of the new range constructors that Qt introduced on 5.14 that deprecates the fromStd construct. But since we need to keep it compatible with Qt 5.12 I used the old version. Is there another solution for this?

daschuer · 2020-08-01T00:20:44Z

Here is the document:
https://drive.google.com/file/d/1p-gMO6mCXNlbMvfeiTtaiL6MJeODRoGn/view?usp=sharing

I have tried to implement my idea in a spreadsheet:

blue: oiginal
red: this PR
purple: the 25 ms threshold.

This is a version with constant regions not more than 25 ms off.
Unfortunately the tempo changes introduces a notable pitch change.
This is a topic to discuss. Pitch change Vs off beat notes ...

Comments and small styles changes Co-authored-by: Be <be.0@gmx.com> Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>

daschuer · 2020-08-30T21:54:52Z

@Be-ing

However it is not yet good enough at marking constant tempos for arrhythmic regions to remove the need for manual input from the user to tell the analyzer whether to assume a constant tempo or not.

Do you have sample tracks?

daschuer · 2020-08-30T22:11:07Z

I had a look into "The Learning Process" https://www.youtube.com/watch?v=7pMtdXx-mME
It has two issues that we can not fix in this issue, because we apply the additional changes after the QM beat detector.
The beat detector tries hard to extract beats from the vocals, we cannot distinguish these beats from a region with real beats.
In addition QM applies a smoothing to the beat changes. this can also not reverted afterwards.
We have ideas to improve that.

The other issue is that in the second part the Tshack beats are marked and not the IMHO more correct Boom beats.
We have an experimental fix for this. I am unsure if it is good enough to become a merge-able PR with only the whitening fix.
@crisclacerda: What do you think?

Be-ing · 2020-08-30T23:32:25Z

src/track/beatfactory.cpp

+    resultHeader = "\nRaw beat lenght\n";
+    debugFile.write(resultHeader.toLocal8Bit());
+    debugFile.write(sRawBeatLenght.toLocal8Bit());
+    resultHeader = "\nCorrected beat lenght\n";


Suggested change

resultHeader = "\nCorrected beat lenght\n";

resultHeader = "\nCorrected beat length\n";

Be-ing · 2020-08-30T23:32:37Z

src/track/beatfactory.cpp

+    resultHeader = "\nCorrected beats\n";
+    debugFile.write(resultHeader.toLocal8Bit());
+    debugFile.write(sCorrectedBeats.toLocal8Bit());
+    resultHeader = "\nRaw beat lenght\n";


Suggested change

resultHeader = "\nRaw beat lenght\n";

resultHeader = "\nRaw beat length\n";

…ate type and some more comments, names and styles improvements

src/track/beatutils.cpp

daschuer · 2020-09-04T11:21:48Z

@crisclacerda: Will you find time to finish the PR? I would like to take this into account when doing the final evaluations.

Co-authored-by: Be <be.0@gmx.com>

crisclacerda · 2020-09-06T05:04:38Z

I think it should be almost ready now.. All review issues have been addressed.
Only need to fix msvc fast build that do not recognize isnan still, ideas?
@daschuer, @Be-ing , @Holzhaus

Be-ing · 2020-09-06T10:31:32Z

There are still a handful of unresolved review comments. Please mark them resolved if you have fixed them or they're no longer relevant.

daschuer · 2020-09-06T11:22:29Z

I have added crisclacerda#1 do make the mode calculation more universal, please have a look.
Than LGTM.

daschuer · 2020-09-06T11:24:40Z

I have some ideas to improve the Arrythmic removal code. But this part will probably become obsolete once we have the down-beat/phase detection in place.

Holzhaus · 2020-09-06T14:01:01Z

src/preferences/dialog/dlgprefbeats.cpp

+    connect(bIron, SIGNAL(stateChanged(int)), this, SLOT(ironingEnabled(int)));
+    connect(bRemoveArrythmic, SIGNAL(stateChanged(int)), this, SLOT(removeArrythmicEnabled(int)));


Please use new-style signal/slot syntax.

Holzhaus · 2020-09-06T14:02:56Z

src/util/descriptivestatistics.h

+    // The median is the middle value of a sorted sequence
+    // If sequence is even the mean of both middle values.
+    static double median(const QList<double>& sortedItems);
+    // The mode is most repeated value in a sequence
+    static double mode(const QHash<int, int>& frequencyOfValues);


Suggested change

// The median is the middle value of a sorted sequence

// If sequence is even the mean of both middle values.

static double median(const QList<double>& sortedItems);

// The mode is most repeated value in a sequence

static double mode(const QHash<int, int>& frequencyOfValues);

/// The median is the middle value of a sorted sequence

/// If sequence is even the mean of both middle values.

static double median(const QList<double>& sortedItems);

/// The mode is most repeated value in a sequence

static double mode(const QHash<int, int>& frequencyOfValues);

poelzi · 2021-01-19T17:55:09Z

I gave this branch a test and tried to analyze 4k songs today.
I had one crash, unfortunatelly the core dump was non complete :(

#0  0x0000000000b00b72 in TrackDAO::onPurgingTracks (this=0x8, trackIds=...) at /nix/store/jlyya93wib5a0vi8vj7ydxpbcky69fvy-qtbase-5.15.0-dev/include/QtCore/qarraydata.h:59
#1  0x00007fead40850f0 in ?? ()
#2  0x00007fead40850f0 in ?? ()
#3  0x00007fead4954f80 in ?? ()
#4  0x00007fea04d1b728 in ?? ()
#5  0x0000000000af73df in QTypedArrayData<unsigned short>::deallocate (data=0x7febfa81d920) at /nix/store/jlyya93wib5a0vi8vj7ydxpbcky69fvy-qtbase-5.15.0-dev/include/QtCore/qarraydata.h:239
#6  QString::~QString (this=0x7fea04d1b6d0) at /nix/store/jlyya93wib5a0vi8vj7ydxpbcky69fvy-qtbase-5.15.0-dev/include/QtCore/qstring.h:1301
#7  TrackDAO::getTrackIdByLocation (this=<optimized out>, location=...) at /home/poelzi/Projects/mixxx-test/src/library/dao/trackdao.cpp:127
#8  0x00007fea04d1b901 in ?? ()
#9  0x00000000000000fa in ?? ()
#10 0x0000000000af7f1d in TrackDAO::resolveTrackIds (this=0x7fea04d1b920, trackFiles=..., flags=...) at /home/poelzi/Projects/mixxx-test/src/library/dao/trackdao.cpp:166
#11 0x00007febf9f5e293 in ?? ()
#12 0x00007fead4023820 in ?? ()
#13 0x0000000000b86f9a in QMessageLogContext::QMessageLogContext (this=0x7fea04d1b900, fileName=<optimized out>, lineNumber=159, functionName=<optimized out>, categoryName=<optimized out>)
    at /nix/store/jlyya93wib5a0vi8vj7ydxpbcky69fvy-qtbase-5.15.0-dev/include/QtCore/qlogging.h:68
#14 QMessageLogger::QMessageLogger (this=0x7fea04d1b900, file=<optimized out>, line=159, function=<optimized out>)
    at /nix/store/jlyya93wib5a0vi8vj7ydxpbcky69fvy-qtbase-5.15.0-dev/include/QtCore/qlogging.h:91
#15 BulkController::open (this=0xfa) at /home/poelzi/Projects/mixxx-test/src/controllers/bulk/bulkcontroller.cpp:159
#16 0x00007fea0efd1e00 in ?? ()
#17 0x00007fead40237f0 in ?? ()
#18 0x00007fead4023820 in ?? ()
#19 0x00007fead40237f0 in ?? ()
#20 0x000000000074b1a8 in Ui_DlgDeveloperTools::setupUi (this=0x7fea0f03f410, DlgDeveloperTools=0x14b011362fb69700) at mixxx-lib_autogen/include/dialog/ui_dlgdevelopertoolsdlg.h:68
#21 0x00007fea0f2af4f0 in ?? ()
#22 0x00007fea0f03f410 in ?? ()
#23 0x00007fea0f2af4f0 in ?? ()
#24 0x00007fea0f03f410 in ?? ()
#25 0x0000000000000000 in ?? ()

From the 4k tracks, 99.9% are fixed bpm tracks.
So far, the tracks I looked at, the beatgrid is always off by some samples, better then the grossly off placed grid from master, but not spot on. Then segments fluctuate for example from 149.9-150.3 while the track is 150 everywhere, the beatgrid is then at the end of the segment.

Some tracks ara analyzed as 96.67, after correcting with 2/3 they are somethinge like 148.00189362, so they get desynced at the end of the track.

daschuer · 2021-01-19T22:29:55Z

Thank you for you test results, they are very helpful

The crash happens in TrackDAO::onPurgingTracks, so probably unrelated to this PR.
This assertion fails https://github.com/qt/qtbase/blob/08d6cb7673aa51bc0532d71db4134f4912e14769/src/corelib/tools/qarraydata.h#L59
qarraydata.h is the base class for strings.

Are you able to reproduce the crash?

It sounds if we basically have the same jitter issue like with constant beat grid. This is good news, because we don't do rounding here. But If we add a rounding that is below the sample rate, we can likely fix the issue.

Can you provide two or more tracks that demonstrate the issue the best? Can you also explain where on a beat you like to put the beat marker? A screenshot would be nice, showing the analyzer result and the manual tweaked beats side by side.

github-actions · 2021-04-20T00:38:54Z

This PR is marked as stale because it has been open 90 days with no activity.

Changing tempos support

4236315

Holzhaus reviewed Jul 10, 2020

View reviewed changes

better styling

21adb5b

daschuer requested changes Jul 12, 2020

View reviewed changes

better comments and names

4b60566

crisclacerda requested a review from daschuer July 17, 2020 00:14

Remove Small Arrhythmic regions of tracks that have const tempo

8cd56e6

Be-ing reviewed Jul 30, 2020

View reviewed changes

src/track/beatutils.cpp Outdated Show resolved Hide resolved

daschuer requested changes Jul 31, 2020

View reviewed changes

crisclacerda mentioned this pull request Aug 20, 2020

[WIP] Rhythm Analyzer #2877

Draft

5 tasks

crisclacerda and others added 2 commits August 30, 2020 18:33

style fixes on statistics files

266eade

Apply suggestions from code review

da6d275

Comments and small styles changes Co-authored-by: Be <be.0@gmx.com> Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>

Be-ing reviewed Aug 30, 2020

View reviewed changes

switch to const refs when possible, added \n before eof, use Sample r…

7f4f0df

…ate type and some more comments, names and styles improvements

Be-ing reviewed Aug 31, 2020

View reviewed changes

src/track/beatutils.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Aug 31, 2020

View reviewed changes

src/track/beatutils.cpp Outdated Show resolved Hide resolved

Be-ing reviewed Aug 31, 2020

View reviewed changes

src/track/beatutils.cpp Outdated Show resolved Hide resolved

crisclacerda and others added 4 commits September 4, 2020 13:55

Apply suggestions from code review

9dfb9a0

Co-authored-by: Be <be.0@gmx.com>

use the beat lenght instead of bpm for processing

ea016e0

failed to add windowedstatistics.h on previous commit

583cbaa

add todo to remove new preferences before 2.4

1aa3fbc

Holzhaus requested changes Sep 6, 2020

View reviewed changes

Holzhaus marked this pull request as draft September 8, 2020 23:04

Be-ing changed the base branch from master to main October 23, 2020 23:15

daschuer mentioned this pull request Jan 19, 2021

Lock BPM updates #3012

Closed

daschuer mentioned this pull request Feb 12, 2021

Ironing beats #3626

Merged

github-actions bot added the stale Stale issues that haven't been updated for a long time. label Apr 20, 2021

Swiftb0y mentioned this pull request Jun 1, 2024

Multiple beat grid on a single track #13308

Open

ronso0 mentioned this pull request Jan 30, 2025

Mark first of every four bars of beatgrid #10164

Open

	// of tacks with a constant tempo as in a different unsteady tempo.
	// of tracks with a constant tempo as a different, unsteady tempo.

	resultHeader = "\nCorrected beat lenght\n";
	resultHeader = "\nCorrected beat length\n";

	resultHeader = "\nRaw beat lenght\n";
	resultHeader = "\nRaw beat length\n";

		connect(bIron, SIGNAL(stateChanged(int)), this, SLOT(ironingEnabled(int)));
		connect(bRemoveArrythmic, SIGNAL(stateChanged(int)), this, SLOT(removeArrythmicEnabled(int)));

Changing tempos support #2930

Are you sure you want to change the base?

Changing tempos support #2930

Conversation

crisclacerda commented Jul 10, 2020 • edited Loading

Be-ing commented Jul 11, 2020

Be-ing commented Jul 11, 2020

Be-ing commented Jul 11, 2020 • edited Loading

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Jul 13, 2020

crisclacerda commented Jul 13, 2020

Be-ing commented Jul 13, 2020

crisclacerda commented Jul 13, 2020

crisclacerda commented Jul 13, 2020

Be-ing commented Jul 13, 2020

crisclacerda commented Jul 13, 2020

crisclacerda commented Jul 17, 2020

crisclacerda commented Jul 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Be-ing commented Jul 30, 2020

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus commented Jul 31, 2020

daschuer commented Jul 31, 2020 • edited Loading

crisclacerda commented Jul 31, 2020

crisclacerda commented Jul 31, 2020

daschuer commented Aug 1, 2020 • edited Loading

daschuer commented Aug 30, 2020

daschuer commented Aug 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Sep 4, 2020

crisclacerda commented Sep 6, 2020

Be-ing commented Sep 6, 2020

daschuer commented Sep 6, 2020

daschuer commented Sep 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poelzi commented Jan 19, 2021

daschuer commented Jan 19, 2021

github-actions bot commented Apr 20, 2021

crisclacerda commented Jul 10, 2020 •

edited

Loading

Be-ing commented Jul 11, 2020 •

edited

Loading

daschuer commented Jul 31, 2020 •

edited

Loading

daschuer commented Aug 1, 2020 •

edited

Loading