ADSR sustain and release stage not working properly

Audio Plugins
#1

Dear all,

I am trying to implement an audio unit synth. I am having a weird behaviour with the built-in ADSR of JUCE. Essentially, when I am setting the sustain parameter to 0 the release stage is not happening, and also when I am setting the sustain below 0.1 for example, the release time is longer than the settings. I am not sure what I am doing wrong here…
Thanks a lot!

This my code of the synth voice:

/*
  ==============================================================================

    SynthVoice.h
    Created: 7 Apr 2019 8:02:12pm
    Author:  Giuliano Anzani

  ==============================================================================
*/

#pragma once
#include "../JuceLibraryCode/JuceHeader.h"
#include "SynthSound.h"
#include "maximilian.h"

class SynthVoice : public SynthesiserVoice
{
public:
     
     bool canPlaySound (SynthesiserSound* sound) override
     {
         return dynamic_cast<SynthSound*>(sound) != nullptr;
     }

    //========================================
    
    void setADSRSampleRate(double sampleRate)
    {
        adsr.setSampleRate(sampleRate);
    }

    void getEnvelopeParam (float* attack, float* decay, float* sustain, float* release)
    {
        // envelope
        adsrParams.attack = *attack;
        adsrParams.decay = *decay;
        adsrParams.sustain = *sustain;
        adsrParams.release = *release;
    }
    
    void getMasterParam (float *mGain)
    {
        masterGain.setTargetValue(double (*mGain));
    }
    
    void setMasterSmoothing (double samplerate)
    {
        masterGain.reset(samplerate, 0.1);
    }
      
    //========================================
    
    void startNote (int midiNoteNumber, float velocity, SynthesiserSound* sound, int currentPitchWheelPosition) override
    {
        // start the envelope
        adsr.noteOn();
        level = velocity;
        frequency = MidiMessage::getMidiNoteInHertz(midiNoteNumber);
    }
    
    //========================================
    
        void stopNote (float velocity, bool allowTailOff) override
        {
            if(allowTailOff)
            {
                adsr.noteOff();
            }
            else
            {
                clearCurrentNote();
            }
        }
    
    //========================================
    // Audio callback
    void renderNextBlock (AudioBuffer<float> &outputBuffer, int startSample, int numSamples) override
    {
        adsr.setParameters(adsrParams);
        
        if(adsr.isActive() != 0) {
            for (int sample = 0; sample < numSamples; ++sample)
            {
//                adsr.setParameters(adsrParams);
                
                // sin test
                double theWave = osc1.sinewave(frequency) * 0.5;
                
                for (int channel = 0; channel < outputBuffer.getNumChannels(); ++channel)
                {
                    outputBuffer.addSample(channel, startSample, theWave * masterGain.getNextValue() * adsr.getNextSample());
                }
                ++startSample;
            }
        }
        else
        {
            clearCurrentNote();
        }
            
        
        
    }
    
    //========================================

private:
    double level;
    double frequency;
    SmoothedValue<double, ValueSmoothingTypes::Linear> masterGain;
    
    maxiOsc osc1; // from maximilian lib
    ADSR adsr; // JUCE envelope
    ADSR::Parameters adsrParams;
};

EDIT (I forgot a piece :slight_smile:)
here is the part of the code in the processor in which I am updating the samplerate for the ADSR:

void SynthFrameworkAudioProcessor::processBlock (AudioBuffer<float>& buffer, MidiBuffer& midiMessages)
{
    ScopedNoDenormals noDenormals;
    for (int i = 0; i < mySynth.getNumVoices(); i++)
    {
        if ((myVoice = dynamic_cast<SynthVoice*>(mySynth.getVoice(i))))
        {
            // set samplerate ADSR
            myVoice->setADSRSampleRate(lastSampleRate);
            // set parameters
            myVoice->getEnvelopeParam(
                                      // envelope
                                      tree.getRawParameterValue(ATTACK_ID),
                                      tree.getRawParameterValue(DECAY_ID),
                                      tree.getRawParameterValue(SUSTAIN_ID),
                                      tree.getRawParameterValue(RELEASE_ID)
                                      );
            myVoice->getMasterParam(tree.getRawParameterValue(MASTER_ID));
        }
    }
    
    
    buffer.clear();
    // audio callback from SynthVoice.h
    mySynth.renderNextBlock(buffer, midiMessages, 0, buffer.getNumSamples());
    
}
#2

If you have a sustain level of 0 then it’s expected that you won’t have a release stage as the release is the time it takes for the signal to return to 0, which it is already at.

Are you using the latest tip of the develop branch? There have been a few changes to the ADSR class that may be related to the other release time issue you’re seeing. I’ve put together a quick test using the develop branch and I’m seeing the expected release time for sustain levels < 0.1s.

#3

Thanks a lot for the answer!

I was thinking in the case of a percussive sound, in which, the sustain stage is not necessary ( I believe). After checking the source code of the JUCE ADSR I think that this extra check for the releaseRate (releaseRate > 0.0f)) in the noteOff function is not necessary, but I didn’t try yet, could be a solution?.

void noteOff()
    {
        if (currentState != State::idle)
        {
            if (releaseRate > 0.0f)
            {
                if (currentState != State::sustain)
                    releaseRate = static_cast<float> (envelopeVal / (currentParameters.release * sr));

                currentState = State::release;
            }
            else
            {
                reset();
            }
        }
    }

What I am thinking is that when you release the note you just need to take the last value of envelope and start the release stage.

Regarding this issue, I realised that I was updating the parameter of the envelope in the audio callback. Moving it in the noteOn function solved the problem:

void startNote (int midiNoteNumber, float velocity, SynthesiserSound* sound, int currentPitchWheelPosition) override
    {
        // update the parameter of the ADSR and...
        adsr.setParameters(adsrParams);
        // ...start the envelope
        adsr.noteOn();

        level = velocity;
        frequency = MidiMessage::getMidiNoteInHertz(midiNoteNumber);
        gendy1.setFrequency(frequency);
    }
#4

I opened a Pull Request here implementing a fix for this issue.

My solution was to perform the calculation of releaseRate within the noteOff() function, using envelopeVal to set the decay time. Thus all notes will take the same time to decay after noteOff().

#5

This is not true for most synths.
The release should start decreasing the amplitude from where it is at the time of the note off, doesn’t matter if you were in the attack, decay, or sustain stage.
As @giuaaa said, in a percussive sound envelope you usually find that the sustain is zero, and the attack and decay values are the same, so the sound is identical no matter how long one holds the key.
I came here because I had the same issue, just my two cents.

Edit: Oh, happy to see it was fixed in 5.4.7, thanx!

#6

There is still an issue within the recalculate rates function. The release rate is calculated based on parameters.sustain instead of the envelope value at the start of the release period which isn’t always the sustain value in the cases of releasing a note during its attack or decay phase.

Here is an easy fix:

    void recalculateRates() noexcept
    {
        auto getRate = [] (float distance, float timeInSeconds, double sr)
        {
            return timeInSeconds > 0.0f ? (float) (distance / (timeInSeconds * sr)) : -1.0f;
        };

        attackRate  = getRate (1.0f, parameters.attack, sampleRate);
        decayRate   = getRate (1.0f - parameters.sustain, parameters.decay, sampleRate);
        releaseRate = getRate (releaseVal, parameters.release, sampleRate);

        if ((state == State::attack && attackRate <= 0.0f)
            || (state == State::decay && (decayRate <= 0.0f || envelopeVal <= parameters.sustain))
            || (state == State::release && releaseRate <= 0.0f))
        {
            goToNextState();
        }
    }

    float envelopeVal = 0.0f, attackRate = 0.0f, decayRate = 0.0f, releaseRate = 0.0f, releaseVal = 0.0f;

    void noteOff() noexcept
    {
        if (state != State::idle)
        {
            if (parameters.release > 0.0f)
            {
                releaseVal = envelopeVal;
                releaseRate = (float) (envelopeVal / (parameters.release * sampleRate));
                state = State::release;
            }
            else
            {
                reset();
            }
        }
    }
#7

Can you please state the issue with the ADSR class in terms of the observed behaviour?

For example by showing a time plot of the envelope and saying what envelope is produced, when the note on and note off happens, and what the expected envelope is.

I have a hunch that what you mean is that the release rate should be calculated based on parameters.sustain, always, as opposed to how it is now, which is calculating it based on the current envelopeVal. Would you say that’s correct?

The problem I see with the current implementation, is that the release will always last parameters.release seconds, even if you release from a very low level.

I think it would be more intuitive if the steepness of the release stage would be constant.

Can you maybe check if the attached patch produces a more favourable output? With these changes it hopefully behaves more intuitively in the corner cases related to early releasing.

adsr.patch (2.1 KB)

#8

just my two cents. This is a big breaking changes that should probably be enabled only with a flag.

#9

That’s a fair point!

#10

Hi,

The issue that is present is that when the parameters are updated during a release that was triggered by a note off prior to reaching the sustain stage results in a click (the values don’t actually have to change, recalculateRates() just needs to be called). I have this working in my example as I am storing the current envelope value as the “releaseVal” variable and calculating the releaseRate from that. “releaseVal” is updated upon note off so will always calculate the correct release.

I haven’t yet tested your patch but it seems odd that you suggest the release wouldn’t take the designated release time. I’ve always thought of release as the time it takes an envelope to go to nothing rather than the time it would take to go from 1 to 0.

Cheers,
David

#11

I understand the issue now, thanks, I’ll take another look into this. You needn’t try the previous patch.

#12

I’ve done some more testing, including your proposed changes and for future reference I am leaving my thoughts here.

Your change seems to address a very particular corner case: when we call setParameters, with an unchanged release value, during a note being actively released. In this case your change would let the note be released with an unchanging slope as if the parameters weren’t even touched.

In cases when the release value is changed, the slope would still change just in a different way, and that means that this would still be a breaking change.

Probably an easier to reason about behaviour would be if the effects of setParameters wouldn’t apply until after the ADSR reached the idle state. If a note was released and is in the process of being rolled off, we should let it roll off. The new parameters should only apply to the next note. Something like this could be emulated by using multiple ADSR objects now. Or if it is clear that some other implementation is objectively more consistent, less affected by corner cases and closer to some industry wide ideal that is ADSR, we could put that in an ADSR2 class.