cannam / expressive-means

Expressive Means Vamp Plugins
GNU General Public License v2.0
6 stars 1 forks source link

Question on pitch change onset detection again... #19

Closed FrithjofVollmer closed 1 year ago

FrithjofVollmer commented 1 year ago

Dear Chris,

while searching for portamento and vibrato adapter presets apt for singing, something curious occurred to me which is related to pitch change onset detection again: Please consider this recording, it's a new one in the test material folder https://www.icloud.com/iclouddrive/0b1ZehCHibEWhr1c4R7MhsHlQ#Expressive_Means_Plugins_(sharing_folder) named "1902 Caruso at 14.74 but may be found at multiple other instances:

Screenshot 2023-05-04 at 13 10 13

...for some reason, the "Onsets" output founds a pitch change at the end of notes, even though the pitch difference function (red) does not fall below the threshold preset of 15 Cents (at this specific point, it is at 34 Cents). Based on the logic, it shouldn't identify the onset before falling below that threshold. Moreover, changing the threshold (even to very low numbers) doesn't have a significant effect on the results.

Do you have an idea what I am missing here? (Maybe two conflicting rules I am not aware of – or is there something we missed so far?)

FrithjofVollmer commented 1 year ago

*Edit: Changing the threshold does have an effect in the other direction (setting it to 100 for instance would eliminate most pitch change onsets), but if set to e.g. 5 Cents, the difference-function-has-to-fall-below-threshold rule apparently does not apply...

cannam commented 1 year ago

I believe this is an accident of having a long gap in the pitch track following that point - longer than the pitch filter length. We don't seem to have tested on signals with such long gaps in them yet!

the pitch difference function (red) does not fall below the threshold preset of 15 Cents (at this specific point, it is at 34 Cents)

Ah, this is what happens when you have tweaks upon your tweaks! Do you remember

    // "pitch change locations are reported c. 20 ms delayed on
    // average. Maybe we could just hard code this value as a
    // compensation? --> pitch change onsets are reported as being 20
    // ms before they are actually recognised"

The function falls below the threshold about 20 ms later than the onset, just as it should!

To fix this may just call for yet another tweak to ensure that it doesn't think there's a new pitch simply because all of the following few pitch readings are equally absent, but I think I am out of time to look at it today. Hopefully simple though.

FrithjofVollmer commented 1 year ago

The function falls below the threshold about 20 ms later than the onset, just as it should!

Ah, right! Knew I was missing something like that :)

To fix this may just call for yet another tweak to ensure that it doesn't think there's a new pitch simply because all of the following few pitch readings are equally absent, but I think I am out of time to look at it today. Hopefully simple though.

Thanks, Chris!

cannam commented 1 year ago

Fixed

FrithjofVollmer commented 1 year ago

Works great, thanks!

FrithjofVollmer commented 1 year ago

https://github.com/cannam/expressive-means/issues/19#issuecomment-1535098691

..."20 ms rule" has been removed by commit 4c2b1db – turned out to be rather problematic anyways.