Code review comment for lp:~mixxxcontributors/mixxx/features_vamp

Hi Tobias,

I really appreciate! Thanks a lot!
I am sure that with your help, this one will be a great feature for 1.11 :-)

> I'm not sure what you mean by the citation. But I think, the average is good
> way to go for at least one reasons: If I count the beats manually, I measure
> the time to count one or two bars (4 or 8 beats). It is very likely that there
> are some very small local BPM variations during the measure. The manual
> computation of the BPM (240.0/<time in seconds for one bar>) takes into
> account the variations. So why not compute the average BPM?

I thought a bit about the computation you did and I agree with you.
Basically, we can do two things in my opinion.
The first one is based on QM-tempotracker.
According to the documentation, such plugin can output local BPM each time it changes.
We may take each value, get the mean and pass as track BPM.

The second one is by following the manual computation you did.
If I understood, you are taking BPM=60/H as track bpm, where H is the harmonic mean of the distance between two beats in seconds.
That is BPM=60/(N*(1/s_1 + 1/s_2 + ... + 1/s_N)) , where s_k represents the distance of the k-th beat from the next one.
Then we get that s_k = (beatpos_{k+1} - beatpos_k) / samplerate , where beatpos_k is the position of the k-th beat in samples and hence:
BPM = (60 * samplerate)/(N*(1/(beatpos_2 - beatpos_1) + 1/(beatpos_3 - beatpos_2) +...+ (beatpos_N - beatpos_{N-1}) .
 This value can be calculated directly from m_beatList. So that it should work for any beat-tracking plugin.
Did I get it?
In case, I do prefer this second way.

> Technically, I can't verify your hack nor can I understand it. I hope that RJ
> or Madjester can have a look at your changes. But I'm not a fan of this hack.
> I'm sure the hack becomes redundant with the average BPMs.
>
> Just for the moment and for testing. Could you create a separate branch based
> on this one and implement my proposal to use the average local BPMs. This is
> just for testing and to see if sync will really syncs (once the beat grids
> have been aligned initially).

Mine was just an attempt to see if that works. I'll revert changes and see what happens if we try a better BPM estimation.

> One more thing: The BPM preferences look complex know, i.e., the advanced
> features. Unless RJ or some other dev overrules me, I think it is better to
> keep the dialogue simple.

I agree with you. The idea was to just give a rough way to switch plugins on-the-fly
and then to write a better configuration dialog, when all will be fixed.

Thanks again,

Vittorio