Qm rounding fix

[daschuer]

Originally the chords and the chroma data have 3 bins per key and center at 1.

010|000|000| C-Chord
595|000|000| Chroma
950|000|005| Result

This means the first correlation has a Maximum at 0. And will warp around if the maximum is slightly shifted by > 1/6 halve tone.

To compensate this I have shiftet the chroma by one.
010|000|000| C-Chord
059|500|000| Chroma
595|000|000| Result

This way we have C without wrap around at 0, 1 and 2 and can use integer math to get the final key value.

This was tested with a B minor chord and a 440 Hz (A) and 523,25 Hz (C) sinus.

I have put a patch into the qm-dsp folder.

[Be-ing]

I'm okay with modifying the library source for now, but I think it would be better to submit a pull request upstream than keep a patch in our repository.

[daschuer]

To be clear this fixes a serious bug.

All originally empty key tracks in your library can be C minor or B minor. Uwes patch makes them all B minor.
There is also the issue that C major is only detected if it is +-1/6 halfe note exact. All others are detected with +-1/2 halfe note precision. B major can be detected a C minor.

Only fuzzy detection is effected.
If the track is well tuned and uses typical chords the issue is not visible.

Which type of tracks where effected by the way? That are probably out of tune or untypical chord tracks. The current implementation assumes that all tracks are played in tune 440.

Earlier Versions of Mixxx are effected as well.

[daschuer]

I will get in contact with upstream.

[daschuer]

@Be-ing: do you get different results with you effected tracks with this PR?
Can you give some examples?

[uklotzde]

Thank you @daschuer for picking this up! I wasn't familiar with the topic.

[daschuer]

Fixed. I had to rebase this PR, because I need a single commit to export the patch.

[uklotzde]

LGTM. Do we need to give the advice for reanalyzing all keys?

[daschuer]

I am unsure western tracks on a full key will be not changed. Tracks that are not on ±-⅓ key may move around.

These tracks are imho unusable for hanonic mixing. Does it make sense to remove the key for them? Or use the full key with the next smaller probability.

I have tryed to find out the reason for this.
An hit some smell deeper in the code.
The implementation is also not at all CPU optimized.

It is itching me to refactor it a bit. But that's has a regression risk.

Normally if you analyze a sine wave on a full key, the chroma should have a maximum at a full key. This is not the case.
So either I do something wrong or there is ab other rounding issue.

I have already found one. The sample rate of 44100 is decided by 8 and rounded to a full integer. This introduced a pitch of 0,01 %. Negnectable though...

[uklotzde]

There is also the open issue that the key analyzer is not thread-safe. So, yes, there might be some ugly secrets hidden in the code. I would leave it as is for now, until we find a more robust and accurate replacement.

[uklotzde]

I've re-analyzed a few tracks with keys and all keys were detected like before.

4 tracks with previously no keys detected:

• quick fix: 6B, 10B, 6A, 6B
• this fix: 6B, 10B, 6A, 6B

Unfortunately I don't have an example were this fix actually matters. But that's good, because we don't need to think about any public advice to re-analyze the keys. Only very few tracks might actually be affected.

Merge?

[daschuer]

IMHO yes. I am working on a thread save PR and digging into more rounding issues.
If this patch has no effect with real tracks, does it suite for 2.2.2 as well? What did we with the returned 25 there?

[uklotzde]

master aborted analysis when encountering the first invalid key (triggered by the return value of the callback passed to DownmixAndOverlapHelper), whereas 2.2 continues processing and simply ignores the invalid keys while collecting the results.

A backport is not needed and I also would not recommend it due to the major code changes when removing the Vamp plugins.

[uklotzde]

LGTM

[daschuer]

Thank you for clarifications and review.