Fix time offset in key and beat analysis #2152

daschuer · 2019-06-09T06:36:53Z

Now this first sample is placed in the middle of the FFT window to not skip the first samples.

…et in key analysis.

src/analyzer/plugins/buffering_utils.cpp

uklotzde · 2019-06-10T15:03:23Z

I'm still unsure about what the correct offset is? Don't want to dig deeper into this class.

What about the frameOverlapFactor in AnalyzerQueenMaryKey? Does this serve a similar purpose and we actually don't need to do it on our own here?

daschuer · 2019-06-10T21:37:24Z

The frameOverlapFactor is a new config parameter, defaults to 1 and is 1/m_stepSize.
Here we need t make sure that the first sample is not pushed into the edges of the first hamming window having no effect.

…set of samples, starting with the first samples.

uklotzde · 2019-06-12T19:54:12Z

src/track/keyutils.cpp

    const int iTotalFrames = iTotalSamples / 2;
-    QMap<mixxx::track::io::key::ChromaticKey, double> key_histogram;
+    QMap<mixxx::track::io::key::ChromaticKey, double> key_occurrence;


This is still a histogram and not a distribution if you only normalize the sums for debug output (see below). Also the term occurrence doesn't fit here and doesn't tell me what it actually means.

Ups, yes i was confused about what a histogram is.

src/analyzer/plugins/buffering_utils.cpp

src/track/keyutils.h

daschuer · 2019-06-13T19:19:55Z

Done.

src/analyzer/plugins/buffering_utils.cpp

uklotzde

Wrong abstraction level

daschuer

I am not sure, because the class has exactly this purpose: DownmixAndOverlapHelper()
It is supposed to do the overlap thing right.
But I am open to fix an abstraction issue.

I am not sure if we have the same understanding of the issue.

Let's say the we have a window size of 4 and a step size of one.

Original it stats like this:

####
_####
__####
___####

With this I have noticed that the fist result is significant for the third step. This offset I am trying to fix by. Due to windowing, the most significant pint is the middle of the window.

Now we have

__####
___####
____####
_____####

And the raw chroma detection results fits to the waveform. We have some delays due to the moving everage and Median Filter later, but that is not the point here.

In this an the previous version we have 2x step size undetected. This becomes worse due to the moving Everage and median filter. This is not yet addressed in this PR, but I can add it.

Do you have suggestion how to improve the abstraction.

uklotzde · 2019-06-14T19:25:51Z

Effectively you prepend a period (length = windowSize / 2) of silence to the input range to center the first window on the beginning of the input. But then you should do the same at the end of the input range, i.e. append the same amount of silence to the end of the buffer and keep sliding the window and processing until that extra silence has been consumed, too. This could only be done in finalize(), because only then you know that all available input has been consumed.

What I still don't get is how those partial windows filled with silence should improve the overall analysis results? Wouldn't it be possible that the results could even get worse, because you don't analyze the actual signal? Why pad the signal with extra silence if it may already start or end with almost silence? Are you able to verify and prove the improvements? Otherwise I would recommend to keep the code simple and comprehensible by analyzing only complete windows of actual input data.

… well.

daschuer · 2019-06-26T17:11:55Z

OK, the zero padding in finalize is now in place.

Wouldn't it be possible that the results could even get worse, because you don't analyze the actual signal?

Most tracks starts and end with silence so, so we do not alter the track if we add more silence at the beginning and the end. An issue are live recordings or vinyl rips because they start with a noise floor.

The key detector will see a rectangular jump, containing all frequency. This does not effect the detection result because it adds the same offset to all key bins.

The beat detector detects a false beat at 0:00 when the noise starts in this case. But this was already the case before this PR, because the internal variables of the beat detector are initialized with zero anyway.
The same happens at the end.

I will prepare an other PR, that does a plausibility check for the first and the last beat.

Due to the Hanning window, the first and last samples are almost silenced anyway, so add If we assume that we hear silence before and after the track, it

Why pad the signal with extra silence if it may already start or end with almost silence? Are you able to verify and prove the improvements? Otherwise I would recommend to keep the code simple and comprehensible by analyzing only complete windows of actual input data.

daschuer · 2019-07-02T20:09:47Z

The AppVeyor issues are unrelated. Is this OK now?

src/analyzer/plugins/analyzerqueenmarykey.cpp

uklotzde · 2019-07-02T22:17:36Z

src/analyzer/plugins/buffering_utils.cpp

+    size_t framesToFillWindow = m_windowSize - m_bufferWritePosition;
+    size_t numInputFrames = framesToFillWindow;
+    if (numInputFrames < m_windowSize / 2 - 1) {
+        // -1 ensures that silence < m_stepSize ramains unprocessed


typo (and I don't understand this remark)

use math_max()

daschuer · 2019-07-03T21:39:02Z

OK, I have added also a plausibility check for the first beat.

uklotzde · 2019-07-05T08:51:07Z

src/analyzer/plugins/buffering_utils.cpp

@@ -27,12 +27,11 @@ bool DownmixAndOverlapHelper::processStereoSamples(const CSAMPLE* pInput, size_t
 bool DownmixAndOverlapHelper::finalize() {
    // We need to append at least m_windowSize / 2 - m_stepSize silence
    // to have a valid analysis results for the last track samples.
+    // Since we go in fixed steps up to "m_stepSize - 1" samples remains
+    // unrocessed. That th reason we use "m_windowSize / 2 - 1" below


Some minor typos in comment

uklotzde · 2019-07-05T08:52:50Z

src/analyzer/plugins/analyzerqueenmarybeats.cpp

+    // the beat if this is the case.
+    size_t firstBeat = 0;
+    if (beats.size() >= 3) {
+        if (m_detectionResults.at(beats.at(0)) <


3 times beats.at(0)?

uklotzde · 2019-07-05T08:53:51Z

src/analyzer/plugins/analyzerqueenmarybeats.cpp

+            int diff = (beats.at(1) - beats.at(0)) - (beats.at(2) - beats.at(1));
+            // we don't allow a signifcant tempo change after the first beat
+            if (diff > 2 || diff < -2) {
+                // firt beat is off grid. Skip it.


uklotzde · 2019-07-05T08:58:28Z

src/analyzer/plugins/analyzerqueenmarybeats.cpp

+            // following beats. Skip it.
+            firstBeat = 1;
+        } else {
+            int diff = (beats.at(1) - beats.at(0)) - (beats.at(2) - beats.at(1));


Why in the else part? Shouldn't this be applied regardless if firstBeat = 0 or 1, i.e. after shifting the index of the first beat?

...but then we would shift the first beat once more. Not sure what the intention was here.

The issue I am trying to solve is that the fist beat is often off.
It is detected when a noise starts, or it is detected if the track does not start at a beat.
If the automatic detected cue uses this beat, the track is cued in off beat.
The code drops this beat by these two conditions. Thinking about it, it is probably sufficient to use the time condition only.
What do you think?

I'm not familiar with the details of beat detection algorithm and would follow your recommendation. Just tried to verify the code and check for inconsistencies or unintended behaviour.

daschuer · 2019-07-19T12:00:56Z

Is there anything else to do before merge?

daschuer · 2019-07-21T22:37:13Z

Thank you for the reminder .. done.

…be anywhere in the range.

daschuer · 2019-07-28T10:34:06Z

I have tweaked the results a bit, because I notices that the beat seems to be late sometimes.
An other pending issue for a late beat is that the algorithm triggers on a rising and falling edge of a beat. Some times it picks the falling edge even tough the rising edge sounds more like a beat.
This can be fixed in a separate PR.

uklotzde · 2019-08-08T07:36:10Z

No issues so far. I didn't check if the beat detection has actually been improved, since it might only fix certain edge cases.

Thank you for working on all these musical analysis algorithms, Daniel!

LGTM

daschuer added 2 commits June 9, 2019 08:31

make sure first sample is sentered in the fft window to fix start ofs…

169ca80

…et in key analysis.

Show key occurrence in debug message as seconds

1ce3e74

daschuer force-pushed the analyzer_start branch from dd388c3 to 1ce3e74 Compare June 9, 2019 07:22

uklotzde reviewed Jun 9, 2019

View reviewed changes

src/analyzer/plugins/buffering_utils.cpp Outdated Show resolved Hide resolved

uklotzde changed the title ~~fix time ofset in key analysis~~ Fix time offset in key analysis Jun 9, 2019

remove special case of window size = step size. This uses a diffrent …

ca7055a

…set of samples, starting with the first samples.

uklotzde reviewed Jun 12, 2019

View reviewed changes

src/analyzer/plugins/buffering_utils.cpp Outdated Show resolved Hide resolved

uklotzde reviewed Jun 12, 2019

View reviewed changes

src/track/keyutils.h Outdated Show resolved Hide resolved

uklotzde added this to the 2.3.0 milestone Jun 12, 2019

daschuer added 2 commits June 12, 2019 22:58

revert rename, now back to Histogram

1c0a248

use c++11 assign() and remove obsolete const parameter

6ec246f

uklotzde reviewed Jun 13, 2019

View reviewed changes

src/analyzer/plugins/buffering_utils.cpp Show resolved Hide resolved

uklotzde suggested changes Jun 14, 2019

View reviewed changes

daschuer commented Jun 14, 2019

View reviewed changes

daschuer added 4 commits June 18, 2019 23:02

Remove undocumented offset in qm beat detector

67d8ecc

pass last partial chunk to the analyzer as well. Fixes lp1833306

9939dbd

Pad the last buffer for analysis with zeros to allow to analyze it as…

0af3ddd

… well.

Merge remote-tracking branch 'upstream/master' into analyzer_start

0d3ca5f

uklotzde reviewed Jul 2, 2019

View reviewed changes

src/analyzer/plugins/analyzerqueenmarykey.cpp Show resolved Hide resolved

uklotzde reviewed Jul 2, 2019

View reviewed changes

daschuer added 2 commits July 3, 2019 23:36

Use math_max() and improve comments

b099b7a

Check first detected beat for plausibility

944702c

uklotzde reviewed Jul 5, 2019

View reviewed changes

daschuer changed the title ~~Fix time offset in key analysis~~ Fix time offset in key and beat analysis Jul 19, 2019

daschuer added 2 commits July 22, 2019 00:27

Improve comments.

e5f7548

Fix copy and past typo

e1351b7

shift beatgrid 1/2 step size to the front, since the actual beat can …

a84bfc2

…be anywhere in the range.

uklotzde approved these changes Aug 6, 2019

View reviewed changes

uklotzde merged commit 58f7a1e into mixxxdj:master Aug 8, 2019

Holzhaus added this to Done in 2.3 release Mar 14, 2020

daschuer deleted the analyzer_start branch September 26, 2021 17:36

mixxxbot mentioned this pull request Aug 23, 2022

Beatgrid off often,vamp plugin parameter. #9695

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix time offset in key and beat analysis #2152

Fix time offset in key and beat analysis #2152

daschuer commented Jun 9, 2019

uklotzde commented Jun 10, 2019

daschuer commented Jun 10, 2019

uklotzde Jun 12, 2019

daschuer Jun 12, 2019

daschuer commented Jun 13, 2019

uklotzde left a comment

daschuer left a comment

uklotzde commented Jun 14, 2019 •

edited

daschuer commented Jun 26, 2019

daschuer commented Jul 2, 2019

uklotzde Jul 2, 2019 •

edited

daschuer commented Jul 3, 2019

uklotzde Jul 5, 2019

uklotzde Jul 20, 2019

uklotzde Jul 5, 2019

uklotzde Jul 20, 2019

uklotzde Jul 5, 2019

uklotzde Jul 20, 2019

uklotzde Jul 5, 2019

uklotzde Jul 5, 2019

daschuer Jul 5, 2019

uklotzde Jul 6, 2019

daschuer commented Jul 19, 2019

daschuer commented Jul 21, 2019

daschuer commented Jul 28, 2019

uklotzde commented Aug 8, 2019

Fix time offset in key and beat analysis #2152

Fix time offset in key and beat analysis #2152

Conversation

daschuer commented Jun 9, 2019

uklotzde commented Jun 10, 2019

daschuer commented Jun 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jun 13, 2019

uklotzde left a comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

uklotzde commented Jun 14, 2019 • edited

daschuer commented Jun 26, 2019

daschuer commented Jul 2, 2019

uklotzde Jul 2, 2019 • edited

Choose a reason for hiding this comment

daschuer commented Jul 3, 2019

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

daschuer commented Jul 19, 2019

daschuer commented Jul 21, 2019

daschuer commented Jul 28, 2019

uklotzde commented Aug 8, 2019

uklotzde commented Jun 14, 2019 •

edited

uklotzde Jul 2, 2019 •

edited