search

c4dm / qm-dsp

A C++ library of functions for DSP and Music Informatics purposes. Used by the QM Vamp Plugins amongst other things.
https://code.soundsoftware.ac.uk/projects/qm-dsp
GNU General Public License v2.0
52 stars 11 forks source link

Qm shift 1/3 #3

Open daschuer opened 5 years ago

daschuer commented 5 years ago

Testing your patches, I have noticed that the lower border frequency of C3 has no Gauss bell form, because the lower 1/3 bin is missing. Now the detection starts one bin earlier. This makes the code also easier to understand IMHO.

cannam commented 5 years ago

The 1/3 bin shift seems like a sensible idea - I'll take a look, thanks.

With regard to style fixes, please refer to the codestyle-and-tidy branch, into which I am currently accumulating a large number of code style fixes.

daschuer commented 5 years ago

OK, the style fixes where all included in the codestyle-and-tidy branch. I have removed the commit here.

daschuer commented 5 years ago

It was probably not too sensible. Now my 440 Hz test wave is off by ~ -10 ¢ There must be an other rounding issue somewhere

daschuer commented 5 years ago

OK, I have only tricked myself and tested a version without the first commit. This one fixes the off by ~ -10 ¢ issue:

only the first commit. The 440 Hz signal is almost centered. 

raw chroma: 
2.1864e-06 1.98328e-06 2.3192e-06 
3.22382e-06 2.58403e-06 2.77546e-06 
3.48479e-06 4.99738e-06 2.26164e-06 
3.63347e-06 3.80177e-06 1.71302e-06 
2.82505e-06 4.37589e-06 2.38337e-06 
3.37997e-06 4.08856e-06 2.25101e-06 
2.07105e-06 3.23208e-06 4.72169e-06 
4.41947e-06 5.44775e-06 3.95811e-06 
7.21986e-06 0.0036204 0.432004 
1 0.44585 0.00541741 
3.78441e-06 5.83631e-06 3.84269e-06 
3.64386e-06 2.46435e-06 2.48727e-06

Both commits. The 440 Hz is shifted by one bins and still almost centered. 

raw chroma: 
2.08649e-06 2.18636e-06 1.98326e-06 
2.31919e-06 3.22381e-06 2.58403e-06 
2.77548e-06 3.4848e-06 4.99737e-06 
2.26162e-06 3.63351e-06 3.80177e-06 
1.71299e-06 2.82514e-06 4.37597e-06 
2.38334e-06 3.38004e-06 4.08852e-06 
2.25108e-06 2.07109e-06 3.23214e-06 
4.72168e-06 4.4196e-06 5.44783e-06 
3.95839e-06 7.22041e-06 0.0036207 
0.432011 1 0.445844 
0.0054171 3.78408e-06 5.83602e-06 
3.84263e-06 3.64383e-06 2.46433e-06

First commit removed: The 440 Hz is off pitch. 

raw chroma: 
1.34152e-06 2.23106e-06 2.05634e-06 
1.92096e-06 3.04384e-06 2.58473e-06 
3.41087e-06 3.61391e-06 5.37381e-06 
2.91883e-06 3.62912e-06 3.14179e-06 
1.5454e-06 2.21627e-06 4.28804e-06 
2.5081e-06 3.36046e-06 3.84076e-06 
2.26447e-06 1.44288e-06 3.10097e-06 
3.92471e-06 4.09539e-06 3.42547e-06 
2.19289e-06 6.30409e-06 0.00293616 
0.430991 1 0.501545 
0.0164639 7.53878e-06 2.99073e-06 
3.81511e-06 2.4753e-06 2.66593e-06
cannam commented 4 years ago

An update about this fix:

I submitted two versions of the QM Key Detector Vamp plugin to the MIREX 2019 evaluation of music analysis methods. The first was the same version as had been submitted in previous years. The second was "v5" of the plugin, which is not yet an officially released version of the plugin, but which was built against the current qm-dsp code with this fix in it (rev 9b18a2294638e). I acknowledged your fix in the submission notes.

The results have just been published, and can be found here: https://www.music-ir.org/mirex/wiki/2019:Audio_Key_Detection_Results. The two versions are labelled as submissions CN1 and CN2 respectively in that page. As you can see, the version with your fix performs substantially better than the previous one in all datasets - a nice real-world validation. Thank you again for reporting and working on this.

daschuer commented 4 years ago

Good news. Thank you for the update.

Did the comment and close button strikes you? I think this PR is still relevant.

cannam commented 4 years ago

Oh sorry - I did close it, but I think I had probably lost track of what was in the PR. Re-opening, will try to take another look.

Holzhaus commented 3 years ago

Oh sorry - I did close it, but I think I had probably lost track of what was in the PR. Re-opening, will try to take another look.

Ping. We already hoped we could drop our custom patches when 1.8.0 landed, but then we saw that this has not been merged yet.

JoergAtGithub commented 1 year ago

@cannam It would be nice to get an official bugfix release of qm-dsp containing the PRs #3 and #6.

cannam commented 1 year ago

There are two problems here - neither insurmountable but both real.

The first is that I left QM in 2020, and so can't really make an official release. I have continued to maintain some stuff voluntarily, e.g. things you can find within the sonic-visualiser Github organisation, most of which I wrote. But this library is something that I worked on maintaining while at QM but that was largely written by other people, so it feels rather different.

That said, I am fairly sure that nobody else is about to make an official release either, and I can of course commit to this repo, so I could create a not-quite-an-official-release tag here if that would be good enough for your purposes.

The second problem - and the reason I didn't get around to merging these before - is testing. The other (more substantial) set of fixes from Daniel that preceded this PR did get merged and tested, but when it came to looking again at this one I could no longer remember what its test status was.

I'm wary (having made mistakes before) of merging even obviously correct patches without checking that they don't make the results worse - especially of course when it's not my name on the code. Sometimes a patch will fix something in one place that another patch already fixed somewhere else, undoing the fix. Sometimes (as was my concern for #6 in the tempo tracker) the code works by accident and a fix breaks it, etc. So while acknowledging that these patches look sensible and are "probably right", it's sadly not just a case of hitting the merge button.

I would like to get this dealt with though... Do you still happen to have test cases for the key detector lying around that you can use to check that this PR (when applied against the current head, rather than the code it was originally written against) improves matters?

JoergAtGithub commented 1 year ago

That said, I am fairly sure that nobody else is about to make an official release either, and I can of course commit to this repo, so I could create a not-quite-an-official-release tag here if that would be good enough for your purposes.

Merging these commits would be a first step, but the problem is, that all the package managers (Linux-Distributions, VCPKG, Homebrew, etc.) ship always the last official release. And until then, a project that uses qm-dsp needs to use a vendored local copy with these bug-fixes.