Fix instrument release fade-out

[lukas-w]

This fixes #3086 by addressing two bugs leading to clicks on instrument note-off in most instruments.

The first bug was introduced as part of a refactoring done by Vesa via 857de8d and caused each note play handle's last buffer being dropped because the play handles were deleted before being processed in AudioPort as discovered by @lleroy in #3086 (comment).

The second bug looks like a typo that has always been there since the fade-out was introduced in 0243338 and was a misplaced parenthesis in Instrument::applyRelease breaking the calculation of the note-off envelope's start frame, sometimes putting it outside of the buffer. I'd love a second set of eyes on this one because I'm not sure I really understand the calculation. I think it could just be replaced by _n->framesBeforeRelease() and in my tests f is always equal to that at the start of the loop, but maybe I'm missing some edge case here.

Before/after examples copied from #3086 (comment):

• Triple Oscillator
  
• BitInvader
  

[zonkmachine]

I've been testing a bug in the Sample Track where not the whole sample is rendered and it also seem to be fixed in this PR. I'm not done setting up a proper test case for this but a quick test gives me the following result.

Original soundfile, 2 seconds 440 Hz sinewave.
Exported from a Sample Track, commit before this.
Same export repeated on this PR.

[michaelgregorius]

@lukas-w, is the lower TripleOsciallator waveform from after the fix? I am asking because it look like some form of DC offset is introduced towards the end, i.e. the waveform is not oscillating around the zero line.

That computation you have mentioned looks complicated indeed. When looking at it I found it surprising that the Instrument class needs to have knowledge about low-level technicalities like the frames per period of the audio engine. Other plugin interfaces, like VST, CLAP, etc. just request a certain number of samples from the plugin and the plugin does not have to care about the rest. It only has to manage its own internal state and feeds the buffers it is handed.

[zonkmachine]

some form of DC offset

I reacted to this too. It's just multiple sinewaves of different frequencies together though. We're good... this time.

[michaelgregorius]

Is it expected behavior that the release ramps are of different lengths and cut off before they completely go to zero?

I have created some visualizations by (temporarily) replacing the buffer assignments in Instrument::applyRelease. Just replace

buf[f][ch] *= fac;

with

buf[f][ch] = fac;

The release stage audio signal will then contain the ramp values which can be visually inspected, e.g. in Audacity.

Here are the results of several low sine notes created with TripleOscillator:

The last one is very short and the screenshot contains part of the actual signal before release:

In the code it seems that the ramp is not a constant number of samples that interpolate between 1 and 0 but that it's dependent on lots of the other variables. I also wonder if it works as intended across buffer boundaries with very low buffer sizes.

[lukas-w]

Is it expected behavior that the release ramps are of different lengths and cut off before they completely go to zero?

That's strange. I admit I didn't really take a closer look at the release ramp and how it's calculated. If the implementation is just flawed, I'm all for simply replacing those handful of lines with something that makes more sense to us.

[michaelgregorius]

There are also artifacts that don't look like simple fade outs (this is a sine signal without the code changes described above!):

Test data was repeated A1 notes with a TripleOsc set to one sine. Note lenghts were eights with a sixteenth of pause in between.

Most notes look as follows and I am not sure if this is expected with a linear fade out. The curves at the end look strangely inverted:

[zonkmachine]

Most notes look as follows and I am not sure if this is expected with a linear fade out. The curves at the end look strangely inverted:

I think that looks like what I'd expected on a lower note and a fast decay. Try a higher note and see if you can get a decaying note over more than one wave cycle.

[michaelgregorius]

The decay of the release ramp is linear and with a linear release ramp there shouldn't be any quick changes in direction like the one seen in the last image. I have tested this with the following Desmos graph: https://www.desmos.com/calculator/msc6zsiefc

In the graph only the quadrants to the right of the y axis are relevant. The graph shows a sine signal that is attenuated linearly starting at x=0. The parameters do the following:

• m: The x value at which the release is fully applied, i.e. where the signal will be multiplied with zero. Ignore everything in the graph for larger values of x.
• n: Phase offset for the sine signal to make sure that quick direction changes do not occur when the attenuation starts at another point in the sine signal.

Example image:

Edit: Added direct screenshot of the graph.
Edit 2: Improved graph: the red graph now shows the original signal, the green graph the attenuation curve and and the black graph the resulting signal.

[zonkmachine]

The decay of the release ramp is linear and with a linear release ramp there shouldn't be any quick changes in direction like the one seen in the last image.

It looks like the desiredReleaseFrames is a fixed value. In TripleOscillator the value is 128 frames. If you export with the default 44.1 kHz sample rate you will get 128 / 44100 = 0.003 or 3 ms. The selected area in the Audacity shot below is 110 samples. The release started a bit before this so 128 samples is maybe not enough. Just bump it up a bit?

A1 release on master
A1 release on this PR
Higher frequency (A3?) release on this PR. This is what I meant. It looks like a proper decaying signal on higher frequencies.

One problem with a fixed number of release frames is that the release time will change with the sample rate.

[michaelgregorius]

You're right @zonkmachine. It looks like the release should be defined in milliseconds which is then converted into a number of samples based on the sample rate.

I also wonder if this problem can be solved in the Instrument class in the first place. If I understand correctly, it's the NotePlayHandle class that knows when the release stage is entered and that has control over how many samples are requested from the instrument during the release phase.

Perhaps a helper class that's used by NotePlayHandle can solve the problem. The helper class would work as follows:

• It has a counter that's initialized to 0 during construction.
• It is configured with a max counter value during construction.
• There is a reset method which resets the counter to 0. It could for example be called at the beginning of the release phase.
• The class has a currentValue method that uses the counter in conjunction with the max counter value to calculate a current value, e.g. for a linear interpolation or an exponential decay (this variant would not really need the counter but only the max value).
• The class has a tick method that increases the counter value whenever it is called. In our case it will be called for each sample that has been processed.

An instance of this class is then used by NotePlayHandle:

• When NotePlayHandle enters the release phase it calls reset on the helper.
• For each sample of the release phase it calls currentValue and multiplies the output signal with that value.
• After each sample it call tick.

When the number of release samples of the instrument have been reached the current value should be 0 or close to 0 for the exponential decay.

Edit: more detailed description of the helper class and its usage.

[zonkmachine]

Is it expected behavior that the release ramps are of different lengths and cut off before they completely go to zero?

Excuse the noise, I was on the wrong branch. 😢
I cannot replicate this on the release-envelope branch. The release ramp is a straight line from 1.0 to 0.0 on every note.
@michaelgregorius How often did the output show one that cuts off in your tests?

[michaelgregorius]

Ok, that's strange. I now also get the expected ramps from 1 to 0 for almost all notes if I apply the "output the release ramp"-patch, @zonkmachine. Except for the last note in my test file:

Here's the test file that I have used with a short buffer size of 32: 6908-FixReleaseEnv-TestFile.zip.

[zonkmachine]

It's easier to follow the wave/'release ramp' if you output them side by side:

buf[f][0] *= fac;
buf[f][1] = fac;

[michaelgregorius]

The only explanation that I can find for the ramps shown towards the beginning of this thread is that I must accidentally have been on some other branch as well. Which is strange because I vividly remember that I noticed that the fix/release-envelope branch is in the main LMMS repo itself and not in Lukas' clone. Seems like I am really getting old. 😅

[zonkmachine]

Here's the test file that I have used with a short buffer size of 32: 6908-FixReleaseEnv-TestFile.zip.

32 samples is what the idle 1.0 beginning part of the release ramp is and if I use a buffer size of 64 I get a 64 samples width.
The glitch seem to carve itself backwards by one buffer size. The tail of the signal seem to look the same.

Buffer size: top 32, bottom 64

[zonkmachine]

This bug is on master. Here is my earlier 64 buffer size run side by side with the same on master.

[zonkmachine]

@michaelgregorius Can you test if this fix takes care of the problem?

diff --git a/src/core/Instrument.cpp b/src/core/Instrument.cpp
index fd729e3ab..1ff01535e 100644
--- a/src/core/Instrument.cpp
+++ b/src/core/Instrument.cpp
@@ -182,7 +182,7 @@ void Instrument::applyRelease( sampleFrame * buf, const NotePlayHandle * _n )
        const fpp_t frames = _n->framesLeftForCurrentPeriod();
        const fpp_t fpp = Engine::audioEngine()->framesPerPeriod();
        const f_cnt_t fl = _n->framesLeft();
-       if( fl <= desiredReleaseFrames()+fpp )
+       if (fl < desiredReleaseFrames() + fpp)
        {
                for( fpp_t f = (fpp_t)( ( fl > desiredReleaseFrames() ) ?
                                (std::max(fpp - desiredReleaseFrames(), 0) +

[zonkmachine]

Most notes look as follows and I am not sure if this is expected with a linear fade out. The curves at the end look strangely inverted:

I misunderstood this earlier. No, this doesn't look right.
Edit: Looks like expected

[zonkmachine]

I misunderstood this earlier. No, this doesn't look right.

I'm not 100% confident but I printed out the value of fac and it showed a straight line from 1.0 to 0.0 all the time. So this would then be mathematically correct. I think it could be unintuitive to us because we're not used to those short decays in a musical context.

[zonkmachine]

I tested @lleroy 's attempt at a smoother release presented here. Below is a mashup of ten releases of sinewaves of various frequencies. PS. In this test I had also forgotten that I had increased the release time to 256 frames in the Triple Oscillator.
By judging visually it's a clear improvement.

Upper - This PR with 256 frame release
Lower - This PR with 256 frame release and smoother shape

[zonkmachine]

Spectrogram of the first four notes from the last post. Definitely less transient noise at release.

[zonkmachine]

I have read the code to the best of my abilities and I believe that this is correct. Testing has showed no new issues and the relief of the clicking on note off is noticeable. As shown in the discussion in this thread it's also visibly improved.

[michaelgregorius]

I am now definitively on the branch and I am still getting the hard changes of direction in the signal:

To get some better insight I have improved the Desmos graph a bit: https://www.desmos.com/calculator/vzq5goyugd. It now shows the original signal, the attenuation curve and the resulting signal. The graph now also shows the signal before the attenuation. For certain settings the hard changes can be reproduced with the graph:

In this case it is caused by an attenuation that does not even last half of a cycle.

As @zonkmachine has noted the attenuation time should ideally be defined in milliseconds and not in a fixed number of samples. So I propose to choose a time that will take around one cycle of a lower note, e.g. 40 Hz. This would mean an attenuation time of 25 ms. At a sample rate of 44100 Hz this would mean that the attenuation is applied over 44100/40 = 1102,5 samples.

Visually this would look as follows:

[zonkmachine]

@lukas-w @michaelgregorius This is already an improvement and I think it should be merged. The rest can be fixed up later.
I would be happy if you could test my fix above, #6908 (comment), which I think can go into this PR.

[michaelgregorius]

Hi @zonkmachine, I can test you fix. However, I am a bit out of the loop now about which specific problem it is supposed to fix. 😅

Can you please describe the expected difference if it is applied to the branch of this PR?

[zonkmachine]

Can you please describe the expected difference if it is applied to the branch of this PR?

This issue with the release ramp hanging on 1.0 for one buffer described here: #6908 (comment)
and here: #6908 (comment)

[michaelgregorius]

Good news: I cannot reproduce the ramps hanging at 1.0 anymore.
Bad news: I cannot reproduce the problem regardless of whether the patch is applied or not.

I have used this PRs branch with and without the patch.

@zonkmachine, do you have a test file reproduces the problem reliably?

[zonkmachine]

@zonkmachine, do you have a test file reproduces the problem reliably?

I'll see if I can fix one for you. I think the issue basically is that we're one off and depending on the buffer size, so it may be that it's much easier to reproduce with smaller buffer sizes. Are you still on 32 ?

[michaelgregorius]

I have been using a buffer size of 64 because it seemed that this would make the glitches more prominent (comment). However, now I have tried again with a size of 32 and was able to reproduce the problem without the patch but not when it is applied. 🥳

[zonkmachine]

Here is a test project: sinearp.mmp.txt
It's set to 140 bpm and has none of these glitches in it. If I set it to 133 bpm there are two and at 127 there are eight.

[michaelgregorius]

Thanks for the project! Same results here: the problem can be reproduced without the patch but it cannot be reproduced with the patch applied. Looks like the patch should go in. 👍

[zonkmachine]

Nice job with those Desmos graphs! Very enlightening.

So I propose to choose a time that will take around one cycle of a lower note, e.g. 40 Hz. This would mean an attenuation time of 25 ms. At a sample rate of 44100 Hz this would mean that the attenuation is applied over 44100/40 = 1102,5 samples.

That sounds like a wee bit too much. Did you try the smoother, s-shape release linked to above? lleroy's code rebased on this PR below. When testing I also bumped the 3osc release to 256 and that combo greatly reduced the issue.

diff --git a/src/core/Instrument.cpp b/src/core/Instrument.cpp
index fd729e3ab..7badee7db 100644
--- a/src/core/Instrument.cpp
+++ b/src/core/Instrument.cpp
@@ -182,14 +182,30 @@ void Instrument::applyRelease( sampleFrame * buf, const NotePlayHandle * _n )
        const fpp_t frames = _n->framesLeftForCurrentPeriod();
        const fpp_t fpp = Engine::audioEngine()->framesPerPeriod();
        const f_cnt_t fl = _n->framesLeft();
-       if( fl <= desiredReleaseFrames()+fpp )
+
+       if( _n->totalFramesPlayed() == 0 )
+       {
+                  const fpp_t nf = qMin( fpp, frames);
+                  for( fpp_t f = 0 ; f < nf; ++f )
+                  {
+                          float fac = (float)( f) / nf;
+                          if (fac<0.5) fac = 2*fac*fac; else fac = 1 - 2*(1-fac)*(1-fac);
+                          for( ch_cnt_t ch = 0; ch < DEFAULT_CHANNELS; ++ch )
+                          {
+                                  buf[f][ch] *= fac;
+                          }
+                  }
+       }
+
+       if( fl < desiredReleaseFrames()+fpp )
        {
                for( fpp_t f = (fpp_t)( ( fl > desiredReleaseFrames() ) ?
                                (std::max(fpp - desiredReleaseFrames(), 0) +
                                        fl) % fpp : 0); f < frames; ++f)
                {
-                       const float fac = (float)( fl-f-1 ) /
-                                                       desiredReleaseFrames();
+                       float fac = (float)( fl-f-1 ) / desiredReleaseFrames();
+            if (fac<0.5) { fac = 2*fac*fac; }
+            else { fac = 1 - 2*(1-fac)*(1-fac); }
                        for( ch_cnt_t ch = 0; ch < DEFAULT_CHANNELS; ++ch )
                        {
                                buf[f][ch] *= fac;
(END)

[lukas-w]

Thanks @michaelgregorius & @zonkmachine for all your testing. :) I pushed the proposed "hanging" fix as bf5f4a7.

Does anyone else find this function really hard to read though? It should be simple but my head hurts trying to understand it. If you don't mind, I'd like to push the rewrite seen below. It should be equivalent to the existing one, at least the sinearp render comes out identical as confirmed using md5sum.

void Instrument::applyRelease( sampleFrame * buf, const NotePlayHandle * _n )
{
	const fpp_t fpp = Engine::audioEngine()->framesPerPeriod();
	const auto releaseFrames = desiredReleaseFrames();

	const auto endFrame = _n->framesLeft();
	const auto startFrame = std::max(0, endFrame - releaseFrames);

	for (auto f = startFrame; f < endFrame && f < fpp; f++)
	{
		const float fac = (float)(endFrame - f - 1) / (float)releaseFrames;
		for (ch_cnt_t ch = 0; ch < DEFAULT_CHANNELS; ch++)
		{
			buf[f][ch] *= fac;
		}
	}
}

b752eed7498c041c91c88444295f813e  sinearp-new-imp-x1.wav
d6a6bf1766a66e250a8c6e0b4163c1ea  sinearp-new-imp-x2.wav
b752eed7498c041c91c88444295f813e  sinearp-zonk-patch-x1.wav
d6a6bf1766a66e250a8c6e0b4163c1ea  sinearp-zonk-patch-x2.wav

Also, I propose dropping the -1 in the factor calculation, making it (endFrame - f) / releaseFrames. Without this modification, the release ramp ends one frame early when compared to having no release ramp. See the list of sample values below, the left column shows the unmodified release factor, the right column shows the raw signal ending one frame later.

0.02341	-0.12958
0.01559	-0.15182
0.00778	-0.17322
0.00000	-0.19366
0.00000	0.00000
0.00000	0.00000

Here's a different signal with the change applied:

0.02341	0.17831
0.01559	0.18219
0.00778	0.18600
0.00000	0.00000
0.00000	0.00000

I know the difference is negligible but somehow it bugs me because I believe this incorrect shift by 1 frame to be the same logic error that lead to the release hanging bug.

I'm happy to open separate pull requests for this if it makes things simpler.

For the other proposed changes (sample-rate-independent release duration, different release shapes), I'd prefer to keep their discussion and implementation out of this PR to not bloat the scope.

[zonkmachine]

Thanks for the merge!

For the other proposed changes (sample-rate-independent release duration, different release shapes), I'd prefer to keep their discussion and implementation out of this PR to not bloat the scope.

👍

I'm happy to open separate pull requests for this if it makes things simpler.

No, it sounds reasonable to fix it here. I'm testing this right now.

I know the difference is negligible but somehow it bugs me because I believe this incorrect shift by 1 frame to be the same logic error that lead to the release hanging bug.

Yes, I had my eyes on this one too but couldn't work it out. After the expose above I know why. Also a reasonable fix.

[michaelgregorius]

Hi @lukas-w, the original method is definitively hard to read. I wanted to experiment with implementing an exponential decay but could not wrap my head around what is happening in that method and how to integrate the new code.

I have to admit that I find the whole structure around playing notes, etc. rather weird and hard to understand. For example in TripleOscillator::playNote you can find the following consecutive statements at the very end:

applyFadeIn(_working_buffer, _n);
applyRelease( _working_buffer, _n );

It looks as if first a fade in is applied to the buffer followed by an immediate release. I assume that this is not what's really happening and that both implementation will check the NotePlayHandle to determine if the fade in/release has to be really applied in that call. However, if that's the case I find the separation of responsibilities really weird. It seems like the instrument has to check over and over again whether it can really create some sound now even if the actual buffer is miles away from an actual note.

[zonkmachine]

...

void Instrument::applyRelease( sampleFrame * buf, const NotePlayHandle * _n )
{
	const fpp_t fpp = Engine::audioEngine()->framesPerPeriod();
	const auto releaseFrames = desiredReleaseFrames();

	const auto endFrame = _n->framesLeft();
	const auto startFrame = std::max(0, endFrame - releaseFrames);

	for (auto f = startFrame; f < endFrame && f < fpp; f++)
	{
		const float fac = (float)(endFrame - f - 1) / (float)releaseFrames;
		for (ch_cnt_t ch = 0; ch < DEFAULT_CHANNELS; ch++)
		{
			buf[f][ch] *= fac;
		}
	}
}

...

Also, I propose dropping the -1 in the factor calculation, making it (endFrame - f) / releaseFrames. Without this modification, the release ramp ends one frame early when compared to having no release ramp. See the list of sample values below, the left column shows the unmodified release factor, the right column shows the raw signal ending one frame later.

@lukas-w I have now tested the reworked applyRelease() and the dropped - 1 for hours, especially inspecting the exported soundwaves in Audacity, and haven't experienced any glitches. Please push these changes.

[lukas-w]

It looks as if first a fade in is applied to the buffer followed by an immediate release. I assume that this is not what's really happening and that both implementation will check the NotePlayHandle to determine if the fade in/release has to be really applied in that call. However, if that's the case I find the separation of responsibilities really weird. It seems like the instrument has to check over and over again whether it can really create some sound now even if the actual buffer is miles away from an actual note.

I don't know, this is one of the few places in LMMS's code that feel somewhat intuitive to me. 😅 I'd say it's an instrument's responsibility to produce a signal based on the information about the note to be played, i.e. start time, release time, frequency, intensity/volume etc. That information is provided in NotePlayHandle along with the requested time window for which the signal is requested from the instrument (which if you ask me is breaking single responsibility in NotePlayHandle and should be separated but that's getting off-topic). Having applyRelease in the Instrument base class just provides a default implementation for specific instrument to apply a simple linear release to their output. It's a convenient place for the method but it could just as well be a generic (non-member) function somewhere else as it doesn't actually rely on Instrument's state.

Please push these changes.

Pushed, will merge assuming there are no further objections.

[zonkmachine]

Pushed, will merge assuming there are no further objections.

Has tested it some more. Just works, please merge!

[michaelgregorius]

I don't know, this is one of the few places in LMMS's code that feel somewhat intuitive to me. 😅 I'd say it's an instrument's responsibility to produce a signal based on the information about the note to be played, i.e. start time, release time, frequency, intensity/volume etc.

I think the reason that it feels strange to me is that I am more used to the separation of concerns as it is defined by many plugin interfaces. Here it's also an instrument's responsibility to produce sound based on some information but in this case the information is already more "integrated". In most cases the instruments gets handed some buffers and information about the MIDI events as they occur with regards to that buffer (ideally with sample-accuracy). So an instrument for example receives information like "at sample 42 there's a 'note-on' event". As a consequence an instrument does not have to perform calculations using all kinds of information about frames. These calculations would be done outside of the instrument, i.e. in the DAW code that feeds the plugins with the integrated information.

That information is provided in NotePlayHandle along with the requested time window for which the signal is requested from the instrument (which if you ask me is breaking single responsibility in NotePlayHandle and should be separated but that's getting off-topic).

Doesn't the NotePlayHandle also contain information about when the note ends? If that's the case then I wonder how playing live notes works in that design because when playing live the end of a note is not known in advance. With the plugin interface it works because each instrument only get the corresponding events and changes it's internal state based on these events. I can for example press a key on my keyboard, the instrument gets a note-on event, starts playing sound and if I released the note after a minute it would get a note-off event and stop the note.

[lukas-w]

Doesn't the NotePlayHandle also contain information about when the note ends? If that's the case then I wonder how playing live notes works in that design because when playing live the end of a note is not known in advance.

It does have a bool m_released member that as far as I understand is switched to true as soon as the note ends, put please don't ask me how applyRelease works without even reading that 😅. I. You're right though, in the context of live performance, the more functional style of this is less intuitive than a state-full instrument that simply reacts to events.