Kicker - Glitch on key off

[zonkmachine]

System Information

Ubuntu 22.04

LMMS Version(s)

Tested on 1.2.2 up to 71dd300f4

Bug Summary

Kicker has a glitch around the time where key off is without the envelope engaged. You see a glitch below 2/3 into the sample.

Steps To Reproduce

Project that demonstrates the issue: KickerGlitch.mmp.txt <-- remove .txt

Screenshots / Minimum Reproducible Project

Please search the issue tracker for existing bug reports before submitting your own.

• [X] I have searched all existing issues and confirmed that this is not a duplicate.

[michaelgregorius]

If I comment out the line

so->update( _working_buffer + offset, frames, Engine::audioEngine()->processingSampleRate() );

in KickerInstrument::playNote and change the code in the release section to output the attenuation factor

_working_buffer[f+offset][0] = fac;
_working_buffer[f+offset][1] = fac;

then I get the following output:

I assume that there's either a jump with regards to releaseFramesDone in the note or with regards to desiredReleaseFrames in the release section.

[zonkmachine]

I assume that there's either a jump with regards to releaseFramesDone in the note or with regards to desiredReleaseFrames in the release section.

Yeah, it looks like one of them lacks the offset.

[michaelgregorius]

I have checked this some more and I think the problem is that done is not incrementing in full number of frames at the beginning of the release but that the for-loop processes a full number of frames each time. Outputting done and desired in the for-loop gives the following output:

Done: 0. Desired: 530
Done: 190. Desired: 530
Done: 446. Desired: 530

The value of frames is 256 each time. So the first jump is 190 frames (0 -> 190) and the second one 256 frames (190 -> 446). At the end of the first phase the value of fac has been processed for 256 samples though and has the value 0.51886791. At the start of the second phase it is calculated using done = 190 and the value is calculated as 0.641509414 which is larger than the value at the end of the first phase.

I am not really sure yet if the problem is within Kicker itself or even outside of it.

[michaelgregorius]

I assume that there's either a jump with regards to releaseFramesDone in the note or with regards to desiredReleaseFrames in the release section.

Yeah, it looks like one of them lacks the offset.

To be frank I don't really know that the offset is and I find it insane that every plugin has to juggle around with all these values and has to do all the calculations for itself. Code like that is repeated over and over throughout the code base which gives a large potential for bugs like the one reported here. Ideally the calculations should be implemented correctly in one place and that code should be reused by every plugin. The current state is really ridiculous. :frowning_face:

[zonkmachine]

The tests above were present on a buffer size of 256. The glitch seem to disappear completely at a buffer size of 32. There's a hint of it on the second kick at buffer size 128.

[michaelgregorius]

Hi @zonkmachine, it's definitively related to the buffer size. I think the problem is that the outside "framework code" and Kicker have a different understanding on how many release frames have already been applied on the buffer(s).

In my example above the frame size was 256. Kicker applies the release as soon as _n->isReleased() is true and then applies a release on the full buffer, i.e. 256 frames, instead of on only the number of frames that are associated with the the release phase. The framework code (or the note) however assumes that only 190 frames of release have been applied and reports this via releaseFramesDone. This leads to the discrepancy of 66 frames when the second buffer of the release is processed.

Smaller buffer sizes just seem to reduce the potential discrepancy because the difference between what the note thinks was applied and what Kicker thinks can at most be the buffer size.

In my opinion each plugin should keep track of the state because that's also how it's done in most other plugin formats (VST, CLAP, etc.). With these formats the plugin is only informed that a "(MIDI) Note Off" event has occurred at frame x and it can then interpret this however it wants. In many cases an internal ADSR would be informed to switch to its release stage but it could also do something completely different, e.g. open or close a gate.

The assumption of the Note class that a certain number of release frames are processed is already quite limiting in LMMS. And the fact that there is no clear separation of responsibilities leads to the problems like in this issue.

I assume that LMMS' "strange" architecture where each note is processed independently and where the same note can sound several times is one of the reasons why the known plugin formats are so hard to integrate as "natively" as they are integrated in other DAWs.

Ok, I'll end my rant here. :sweat_smile: It's just sad because the current architecture, the code that's repeated over and over again, etc. is holding the software back.

[michaelgregorius]

@zonkmachine: Fixed by #7226.