Fix the clicks heard during instrument note-off .

[mikobuntu]

I'm sure that this has been addressed before somewhere ( I will edit my post later to better suit all of the details ). The thing that is most obvious is the click that you hear using the triple_osc instrument on default in a track. Looking at the waveform in the top_bar waveform view you can notice that the waveform is drifting ( a song export reviewed in audacity shows a crossover of non zero ), where it should really be a uniform sine wave. we can kind of solve the click by enabling the envelope within the instrument [ well not really solving it as such, but at least making it not have that initial click ] . I'm unsure of the terminology, perhaps antialiasing or such, but whatever the term, there really is an underlaying issue within LMMS and always has been. I can remember @pgiblock mentioning something similar a few years ago if he has time to chip in here.

[tresf]

Like minimalist envelope-by-default?

[mikobuntu]

@tresf yeah for now that would solve the "click" problem I guess.

[musikBear]

is that the same as #2047 In respect to the envelope i suggest the default setting to be 'negated' #2092 That makes a nice piano-like note

[grejppi]

@mikobuntu Could you please clarify the point about "crossover of non zero"? 😮🤔

[mikobuntu]

@grejppi . I have further analysed that it is not indeed the start point of the sine-wave that is causing the clicks, but the actual release point that is the issue. The non-zero-point-crossing I referred to ( may not be the exact terminology ) is where there is still some amplitude of the sample at either the start/end of the waveform.

[grejppi]

Oh... I think that's not really an internal issue as much as one of setting more sane default values

[BaraMGB]

Perhaps an oscillator should always finish the cycle?

[tresf]

Perhaps an oscillator should always finish the cycle?

This would cover it sometimes but it could cause unnecessary runoff. I feel like it would be less accurate versus just @sloping/leveling-out/centering the wave (whether through a default envelope or through code).

The approach should also scale to other potentially spike-prone cutoffs. Most DSP is prone to this because most instruments generate waves which could suffer the exact same problem (unless they are coded to avoid this)

[BaraMGB]

I have never heard such a click in any other synthesizer except the lmms synths. I'm pretty sure they finish the cycle for avoiding this. @fundamental how does zyn handle this?

[mikobuntu]

@BaraMGB From doing a live recording of playing random length notes in the triple_osc in Audacity I noticed that the waveform always starts correctly, but depending on whether or not the note ends exactly at zero crossover, the cycle is cutoff at a non zero point, which always causes a clicking sound. I assume that zyn finishes out the cycle. ( i will check this now )

[fundamental]

Any rapid change in volume within zyn results in an interpolation from the current value to the destination value over the course of a processing unit (buffersize in zyn terms). Just a simple linear interpolation to zero over 32 samples should be plenty to avoid the clicking.

[fundamental]

@mikobuntu zyn does not finish the cycle

[mikobuntu]

@fundamental Just seen that from your previous comment, perhaps you could guide the team on how to achieve this with the LMMS synths.

[fundamental]

@mikobuntu without making the commits myself I'm not sure what there is to guide beyond just repeating that parameter values should be interpolated when there are large deviations and linear interpolation over a handful of samples tends to be enough to avoid pops.

[grejppi]

Isn't the issue here just that by default there is no envelope and a note plays at full volume with no attack or release time?

[mikobuntu]

@grejppi I guess that would solve the issue ( for now ? ) . I think I can code this up easily enough in EnvelopeAndLfoParameters.cpp with sane values. I think I'm just wanting to have a permanent or more code based approach, which would cover all aspects. Take for example the sample_track, if the user puts in a sample that already has a click at the start/end if there is an algorhythm in place that can detect and smooth out these clicks would be great. The algorhythm could be used ( i'm sure ) for such things as LFO waveforms too.

[mikobuntu]

@tresf I have made a mess of my pull request ( on my local branch ) . It is showing all my changes , like updating my branch. I had done the patch firstly directly in Github and I believed at the time that it didn't work so I redid it in the terminal, so basically the patch it there ready for review, but there will be annoying messages in there. Is there an easy way to fix this. Thanks :face_with_head_bandage:

[Umcaruje]

@mikobuntu the best way is to actually backup your changes first.

fetch upstream: git fetch upstream

Then, go and reset your branch git reset --hard upstream/master Warning: this will remove all the changes you made and reset your branch

If you were already on a separate branch, skip this step, but otherwise, if you're on your fork's master, I'd highly recommend you switch to a new branch: git checkout -b branchName

Then, do your changes again (from the backup)

after that, you can commit: git commit -am "Commit message"

And then just push your new branch: git push --set-upstream origin branchName

After that you can make your pull request on github and all should look well.

[mikobuntu]

@Umcaruje thank you for such a detailed explanation, I will get to work on it now :)

[mikobuntu]

3090 ( ready for testing )

Ok great @Umcaruje your commands worked perfectly, except for some reason I wasn't able to reset to the upstream branch, but I resolved this by resetting hard to the last good hash.

[tresf]

I resolved this by resetting hard to the last good hash.

This is the technique I use... generally git reset --hard HEAD~20 or something that brings me back in time, then pull back in upstream with git pull --rebase upstream master, etc.

[mikobuntu]

@tresf I had tried the HEAD~( number of steps backward ) but forgot about rebasing.

[Erudition]

My MIDI device allows adjusting the volume of a note whilst playing it. That said, I have to turn this feature off with LMMS because then the non-zero-crossing clicks happen at every sample, causing a buzz of clicks throughout the note. Can't wait for this to be fixed!

[zonkmachine]

@mikobuntu Sorry, I forgot to get back to you after reverting #3090 .

Code after #3090 After the initialisation, isn't it possible to conditionally setm_amountModel to 0.0f if we have an Audio File Processor in the track?

Pseudo code: 'track->instrument->name()'

if( 'track->instrument->name()' == "audiofileprocessor" )
{
    m_amountModel.setValue( 0.0f );
}

Code above goes here.

[zonkmachine]

If we do, there is however one more issue with the 'zero length' notes that bugged #3090. The maximum playing length is right now depending on the envelope setting, even if the amount is set to 0.0 . See my comment on issue 2074 here.

[Erudition]

This is, IMHO, the most important bug in LMMS right now.

It is immediately obvious, even as soon as a complete newbie opens LMMS for the first time, and clicks on a default instrument. Click! Every time. For audio production software, the way things sound is more important than just about anything else.

Not sure why this is tagged as UX. Let's be clear: the issue is LMMS does not interpolate over instantaneous jumps in value and thus has an effect anywhere a sound does not start or end on a zero crossing - even if it's not always noticeable.

When this is fixed internally (not some workaround such as envelopes-by-default), there would be no difference on the UX side. It would just sound right. This is an audio issue even when the track has been exported from the program. Thus, this is an audio issue and a quality issue, but not a UX one.

[tresf]

Not sure why this is tagged as UX.

I agree, this is a regression introduced somewhere, not a UX bug. It's been masked as one because the envelope is a bandaid over the actual problem.

In regards to the severity you're adding to this problem, please remember, these bugs are introduced by volunteers and fixed by volunteers. The vast majority of work happening is because someone that used the software decided to help fix it. Often the original authors of these bugs are long gone and it's up to the community to fix them, like it or not.

So please understand this when you lecture the development team about importance.

I'll classify this as a bug and mark it for 1.2.0 to see if it's possible to iron out before stable release. No promises though, this isn't an area that we have a lot of expertise on, unfortunately. Volunteers always welcome. :)

[lukas-w]

@tresf How is this a regression? Hasn't LMMS always had this issue, i.e. a lack of interpolation?

@Erudition I think missing interpolation of MIDI signals is independent from this problem, even though it has similar symptoms. Better file a separate issue to track it.

[tresf]

@tresf How is this a regression? Hasn't LMMS always had this issue, i.e. a lack of interpolation?

@lukas-w Yes, this statement appears to be correct. Sorry for any confusion. Here's a side-by-side of 1.2.0-RC5 and 0.4.15 both running the default TripleOscillator preset. Both do a fairly abrupt interpolation, which makes audible clicks when the timing is right.

[lukas-w]

@tresf Thanks for confirming. I'd suggest moving it to 1.3.0 then.

[Erudition]

@tresf Don't worry, I understand that. I didn't mean to "add severity" to the problem, just wanted to clarify its importance (!= urgency) where I thought it was understated (as you noticed, it wasn't even tagged as a bug). I don't think of my comment as a "lecture" so much as a bump to keep the bug afloat after seeing no activity since the middle of last year. Perhaps some people are used to immediately using a workaround for this issue and thus ignore it - but this is still one of the first things pointed out when I try to get new people to use LMMS :) (And if all bugs are reported and addressed by volunteers, then there's no "development team" to lecture anyway!)

As far as the band-aid, If there was a version with this I must have missed it. Hmm.

[Erudition]

@lukas-w

@Erudition I think missing interpolation of MIDI signals is independent from this problem, even though it has similar symptoms. Better file a separate issue to track it.

I'm not sure why you tagged me, I never said a word about MIDI.

But come to think of it, that is an issue as well, so sure, it should be a separate bug.

[lukas-w]

@Erudition I was referring to this comment (https://github.com/LMMS/lmms/issues/3086#issuecomment-294708643):

My MIDI device allows adjusting the volume of a note whilst playing it. That said, I have to turn this feature off with LMMS because then the non-zero-crossing clicks happen at every sample, causing a buzz of clicks throughout the note.

[lleroy]

I think the last buffer of samples from the instrument is dropped somewhere:

I wrote out the samples to a file in TripleOscillator::playNote() and in AudioPulseAudio::streamWriteCallback() and as you can see the plugin nicely uses Instrument::applyRelease(), but somehow that last part is not sent to the audio device.

[lleroy]

I think this fixes the problem: delaying the removal of the finished PlayHandles until after the AudioPorts have processed their output, since AudioPort::doProcessing() iterates over the PlayHandles.

diff -ru lmms-1.2.0-orig/src/core/Mixer.cpp lmms-1.2.0/src/core/Mixer.cpp
--- lmms-1.2.0-orig/src/core/Mixer.cpp  2018-11-03 02:43:42.000000000 +0100
+++ lmms-1.2.0/src/core/Mixer.cpp       2019-08-07 14:53:53.193038000 +0200
@@ -439,6 +439,10 @@
        MixerWorkerThread::fillJobQueue<PlayHandleList>( m_playHandles );
        MixerWorkerThread::startAndWaitForJobs();

+       // STAGE 2: process effects of all instrument- and sampletracks
+       MixerWorkerThread::fillJobQueue<QVector<AudioPort *> >( m_audioPorts );
+       MixerWorkerThread::startAndWaitForJobs();
+
        // removed all play handles which are done
        for( PlayHandleList::Iterator it = m_playHandles.begin();
                                                it != m_playHandles.end(); )
@@ -465,11 +469,6 @@
                }
        }

-       // STAGE 2: process effects of all instrument- and sampletracks
-       MixerWorkerThread::fillJobQueue<QVector<AudioPort *> >( m_audioPorts );
-       MixerWorkerThread::startAndWaitForJobs();
-
-
        // STAGE 3: do master mix in FX mixer
        fxMixer->masterMix( m_writeBuf );

[tresf]

@lleroy thanks for looking into this. @PhysSong @JohannesLorenz I assume a PR is the best place to for this change? This bug has been plaguing the software for a very long time.

[JohannesLorenz]

Looks like a valid fix, good find. Indeed, Instrument::applyRelease does the release for TripleOscillator, so this is probably just removed by removing the play handles.

I'd like to check this in a PR, and I'd offer reviewing this.

Btw: Does anyone know why there are no clicks at the "start" of a (phase shifted) sine wave in TripleOscillator? I can't find out where a fade-in would be applied, but can neither hear a click.

[musikBear]

Btw: Does anyone know why....

If the attack-point in the phase-shifted sine, is closer to 0 dB than the Non-phase-shifted sine, the risk of a click artefact would be less, i believe- That opens a new Q: Is non-shifted sine actually in a 'wrong' phase? Rem : 3oc have unwanted 'slide' in phase. The fix for that bug #2047, had to be abandoned: #3292.

[SecondFlight]

The issue in #2047 was not that oscillators start at the wrong phase. I believe they start with the expected phase and desync over time.

If the attack-point in the phase-shifted sine, is closer to 0 dB than the Non-phase-shifted sine

I don't believe this is what they were saying, but I could be wrong.

By "attack point", do you mean the value that the oscillator gives as its first sample?

[musikBear]

By "attack point", do you mean the value that the oscillator gives as its first sample?

well for note off i suppose it would have the sine continue to the 0 value, before the sound is stopped

Green line is note-off. So the red shape should be continued to the black line (stippled) before it really is stopped. the blue shape can be stopped immediately

[SecondFlight]

It's not quite that simple.

Even if a signal stops at 0, extra high-frequency content will come through when the signal is converted from digital back to analog and sent to the speakers:

In the image above, the digital signal is represented by the dots, and the analog output from the D/A converter is represented in blue. The green line is when the oscillator stops.

This is in addition to (~and may be closely related to~) a click that would likely be heard just because the speaker driver is asked to stop suddenly at zero, though I can't back that one up since I don't know the exact math involved.

[he29-net]

It doesn't even need to go through D/A conversion; whenever you have a sharp angle like that in the signal, you are including high frequency components (sine waves) in order to make it.

To get a perfectly sharp change in the signal (e.g. a square wave) in the real world, you would have to include infinitely many of these components, expending infinite amount of energy. That's impossible, so the speaker driver does exactly what SecondFlight pictures above -- it just makes the signal as sharp as possible within its limits. The better the speaker, the smaller and faster the "ringing" will be, and the louder the clicks will become, since it will be able to include more of these high frequency components.

With digital samples, your frequency resolution is not infinite, so you don't need to add up infinite number of sine-waves to get a sharp-looking angle. But you are still using the high-frequency components to make it and that can be easily seen in a spectrogram: AFAIK, the only way to eliminate clicks is to have an appropriate envelope, but I seem to remember that there was some opposition to enabling it by default, because it could break older projects or something?

[JohannesLorenz]

there was some opposition to enabling it by default

Well, it has always been enabled by default.

Just for background info, zyn uses S-formed envelopes for fade-in and linear envelopes for fade-out. The length for fade-in depends on the note played, which results in a punchy, but not clicking sound for all frequencies.

[he29-net]

Oh, I thought there is only the standard "ADSR envelope" (second instrument tab), which is off by default. If the instruments individually manage their own fade-in and fade-out, this should not be a problem. (My mistake, I mainly reacted to the last two posts, without properly understanding the rest.)

[musikBear]

"ADSR envelope" (second instrument tab), which is off by default. imo -a mistake

[JohannesLorenz]

Btw: Does anyone know why there are no clicks at the "start" of a (phase shifted) sine wave in TripleOscillator? I can't find out where a fade-in would be applied, but can neither hear a click.

I just tested. It does clip on fade in if you take 90 degrees phase shifting. OT, but a bug IMO, so I opened #5119 .

[lleroy]

I tried another fix last night... making the "release" smoother (2 x parabola - simple "s" shape, as mentioned above ). I also abused the "release" to apply a similar fade-in at the beginning... I think it sounds a lot less clicky this way.

diff -r -u lmms-1.2.0-orig/src/core/Instrument.cpp lmms-1.2.0/src/core/Instrument.cpp
--- lmms-1.2.0-orig/src/core/Instrument.cpp     2018-11-03 02:43:42.000000000 +0100
+++ lmms-1.2.0/src/core/Instrument.cpp  2019-08-20 09:53:48.632621700 +0200
@@ -99,14 +99,28 @@
        const fpp_t frames = _n->framesLeftForCurrentPeriod();
        const fpp_t fpp = Engine::mixer()->framesPerPeriod();
        const f_cnt_t fl = _n->framesLeft();
+       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() ) ?
                                ( qMax( 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;

[PhysSong]

@lleroy BTW, you can add the code type at the beginning of the code block, ex. ```diff instead of ```.

[michaelgregorius]

Here's my five cents and current take on this issue (and the reason why #6864 got linked):

• All clicks would be fixed if the volume envelopes were activated by default on all instruments.
• Activating the volume envelopes leads to problems with instruments being used in the "Pattern Editor" though because it represents the patterns with the help of very short notes.
• There is no way to fix both problems due to problems in the instrument architecture which you can read about here. Long story short: the current instrument architecture is very rigid and therefore an instrument implementation, e.g. a sampler, cannot decide by itself how a MIDI note is interpreted and how it is used to shape a volume envelope. Or at least it does not have the last word on it and the "Envelope" tab will always somehow interfere.

[musikBear]

All clicks would be fixed if the volume envelopes were activated by default on all instruments.

Exactly. I have advocated that for ages-Even made a PR with it. It was cancelled Then there is the no-length thing in BB-editor. What @michaelgregorius wrote:

instruments being used in the "Pattern Editor" though because it represents the patterns with the help of very short notes.

-actual made speculate: Would it be possible to always activate ENV when an instrument is dragged/ send into _song-edito_r, and always disable it when it is dragged/ send into BB-editor That would a fix