Artifacts on IR update of dsp::Convolution in VST - JUCE 5.3.2 OSX 10.12

[davidpq]

Dear community,

I am experiencing artifacts when updating the IR of a dsp::Convolution, used to design a room reverb VST under OSX 10.12 with JUCE 5.3.2. A full version of the debug project along with an audio recording of the artefacts is available there.

All there is to it is a slightly modified JUCE template VST, adding wave format reader and dsp::Convolution in the PluginProcessor.h, a timer triggered updateIR() method in PluginProcessor.cpp:

void DebugDspConvAudioProcessor::updateIr()
{
    // auto update IR file name
    auto irFileName = "";
    if( irCount % 2 == 0 )
        irFileName = "Studio_5_A0_01_BF_W_withoutDirect.wav";
    else
        irFileName = "Studio_5_A1_01_BF_W_withoutDirect.wav";
    irCount += 1;
    auto irFile = getResourceFile (irFileName);
        
    // read data into buffer and load IR to convolution
    std::unique_ptr<AudioFormatReader> reader (wavFormat.createReaderFor (irFile.createInputStream(), false));
    if (reader.get() != nullptr)
    {
        reader->read(&tmpBuffer, 0, (int)maxSize, 0, true, true);
        convolution.copyAndLoadImpulseResponseFromBuffer(tmpBuffer, localSampleRate, false, true, true, maxSize);
    }
    
}

and the convolution itself at the end of PluginProcessor.cpp processBlock:

    monoBuffer.copyFrom(0, 0, buffer, 0, 0, buffer.getNumSamples());
    dsp::AudioBlock<float> block (monoBuffer);
    convolution.process (dsp::ProcessContextReplacing<float> ( block ));
    for (auto i = 0; i < totalNumInputChannels; ++i)
        buffer.copyFrom(i, 0, monoBuffer, 0, 0, buffer.getNumSamples());

If anyone has an idea of what I am doing wrong here, I’ll sure take it!
Thanks for your attention,
David PQ

[IvanC]

Hello ! Questions :

• Have you tried to download the last version of JUCE on the develop branch to see if the problem is still there in this context ? We did a few fixes on the Convolution class recently…
• And finally, did you try to load your IR using loadImpulseResponse, providing the File object as an argument ?

[davidpq]

Hi Ivan,

• Just tried with JUCE develop branch (vst created from scratch with associated projucer) at commit 09cc2e00d3020f20c9f20d6270cd437d2087b041, same artifacts.

• Using loadImpulseResponse for IR loading (code used below) did not remove the artifacts, using either file input:

    // method 2: directly load IR from file on disk
    convolution.loadImpulseResponse(irFile, false, false, maxSize);

or memoryBlock input:

    // method 3: read data into memory block and load IR to convolution
    std::unique_ptr<InputStream> assetInputStream (irFile.createInputStream());
    if (assetInputStream != nullptr)
    {
        irData.reset();
        assetInputStream->readIntoMemoryBlock (irData);
        convolution.loadImpulseResponse (irData.getData(), irData.getSize(), false, true, maxSize);
    }

(tried with and without trimming and normalization)

[IvanC]

So I did run some tests with your project and the material you provided for witnessing the issue (that’s probably one of the most detailed and convient for testing bug report I have seen, thanks for that !). Indeed, the plug-in isn’t doing anything wrong, but on the Reaper project, with the IR switching every 500 ms we get a noisy “ploc” every single time.

It appears that my Convolution has not really been made for fast switching with any possible IR. It uses a very simple way to do the transition between the use of previous IR and next IR, which is running two convolution processes at the same time, with their outputs being faded in/out linearly for 50 ms. It worked quite well with all the things I have ever fed it, knowing that it has never been made for very long IRs / reverberation but more for ones with a size < 1 sec, since it does not use (yet) not uniform partitioned algorithms for reasons I gave in another topic. In fact, I used this quite exclusively nowadays on guitar cabinet impulses, which look like a peak and then some samples fading in amplitude up to zero very quickly.

However, the two IRs that you have been using are specifically ones which will make my smoothing algorithm fail, because the second one doesn’t look like the cabinet IR I used. It’s a direct reply, and then nothing, and then a reverb/cabinet like IR. If I set the smoothing time a little longer, I’m able to make the switching artefact not significant anymore. In juce_Convolution.cpp, try for example 0.5 instead of 0.05 in lines 997 and 1001.

Anyway, that issue is a DSP one, it means that I should either provide a way in the API to se the smoothing time, or that I should replace the LinearSmoothedValue there with something more useful, for example a class smoothing linearly in the dB domain. I’ll do some testing next week.

[danielrudrich]

Hi @davidpq, nice to see you here.

I also think the 50ms linear ramp, which is basically a triangle function multiplied to the signal, will cause the plop noises. With a longer ramp, the AM would be not as distinct (in the frequency domain), but then there are up to 3 IRs at the same time, when crossfade time get’s near the switching interval.

A logarithmic crossfade (equal power or equal gain, depending on the IR?) should solve this, at least make the artifacts less pronounced.

[lob]

Hi,
Also, if you keep your sliders and values non logarithmic, for example keep the frequency slider in octaves rather than frequency (but displayed as frequency via the lambda) then you can still process the automation linearly in blocks (a fade out of one frequency value plus a fade in of the next, all times the audio) rather than using a one sample at a time logarithmic cross fade. After that, if you need, make all of the newly derived values logarithmic with block functions.
Believe it or not, this should be less CPU heavy, that is until you decide it needs a further anti-alias smoothing filter. The filtering can be a bonus offline bounce feature.
Ivan, I am not well versed on non-FIR type filters. Would you suggest optional offline bouncing upsampling with filterHalfBandPolyphaseIIR as your ultimate solution when sharp modulation is not desired?
What do you think of chkn’s solution?

chkn suggests-
May 2015
I usually also do also some low pass filtering to the smoothed parameter, to remove sharp edges
coeff = (float)(exp(log(0.01f)/( millisecondsToReach99percent * sampleRate * 0.001)));
and then per sample
float processLP(float inVal)
{
smoothed = coeff * (smoothed - inVal) + inVal;
return smoothed;
}

[IvanC]

Hello there ! Check what I’m working on right now :

I’m going to create a topic and a blog post about this work soon

[lob]

fantastic

[davidpq]

Hi Ivan,

Ran into another interesting issue on that front (pulled latest JUCE commits from the git a minute ago).

Increasing the smoothing time in juce_Convolution.cpp, or reducing the timerUpdateIntervalInMs in the PluginProcessor.h of the debug project (to e.g. 1 ms) will result in an overflow of the fifo stack (l. 489), which size is hardcoded in juce_Convolution.cpp to 256.

[IvanC]

Thanks for the report. I’ll do a few updates + fixes on the DSP module after the ADC18, and I’ve just added this issue in my list

[chkn]

Will this also “fix” the use of unsigned types for channel and samples indexes?

Why does the dsp-module uses unsigned types for sample/channel indexes?

why? all other juce routines using signed integers signed types are better for any kind of offset calculation, with results below 0

[davidpq]

Evening Ivan,

Would you perchance have an idea for a quick fix on that 256 fifo size limitation (other than increasing the hardcoded value)? It’ll sure spoil my happy digging, still, if ever you have a one-liner to drop the oldest fifo requests to make room for new ones and avoid the overflow, I’m taking it. Screw the fun :).

[IvanC]

Oops sorry about the delay, I’ll have a look next week, and I’ll implement my LogSmoothedValue class there as well

[IvanC]

Hello ! I’m currently working on the Convolution class fixes. Here is what I’m doing:

• I have created a class called LogSmoothedValue. It works like LinearSmoothedValue, but with a different kind of ramp (spoiler: log instead of linear), and this is in practice very useful to handle IR changes in the Convolution class without changing the length of the ramp.
• I don’t want to drop oldest FIFO requests to make room for new ones. That’s not how a FIFO and the whole class should work. If the FIFO is full, and you are still asking new changes, then that’s the thing which is wrong. So I have increased the FIFO size, and the code will fire an assertion when overflow is happening. Updating every 1 ms the IR in a class which can handle IR changes slowly by very definition is wrong whatever the viewpoint.
• I won’t let the user modify the interpolation time, because in my opinion it won’t be useful with the new LogSmoothValue class handling the IR changes. And this way I prevent related FIFO size issues at the same time.
• Some people suggested that the class shouldn’t allocate memory during run-time / processing, and I’ve seen issues with that behaviour specifically in Pro-Tools recently, so now the first time the FIFO will be processed won’t be during the first processSamples call, but during the prepare function first call.

[IvanC]

I’ve sent a big update to the JUCE guys today, I might create a topic about that upcoming LogSmoothedValue class at some point

[danielrudrich]

Thanks @IvanC, looking forward to that LogSmoothedValue class, with you programming it I expect it to be quite performant. Did you use look-up tables or any other efficient way to calculate the log()s?

[chkn]

You only need a multiplication *request* dsp::Oscillator should use a log frequency fade

[davidpq]

The approach makes sense. The only issue I see is with the:

@IvanC

[…] and the code will fire an assertion when overflow is happening. Updating every 1 ms the IR in a class which can handle IR changes slowly by very definition is wrong whatever the viewpoint.

Where there’s no safety net for e.g. end users with slow machines: the plugin will crash, period. Devs will end up relying on a throttle mechanism. I would suggest (I love these “suggestions” when I’m not actually concerned with the implementation) to have this throttle mechanism implemented in the class. It’d be disabled by default with a comment on the “crashing line” that says why it crashed and that this mechanism can be enabled, though at the dev’s own risk (socket like packet drop warning).

[danielrudrich]

Right, I even proposed that in another thread regarding ADSR Multiplication instead of addition.
Similar to logarithmic sweeps, one could argue to call it ExponentiallySmoothedValue (as exponential sweeps), but that would become a quite philosophical discussion

[IvanC]

The LogSmoothedValue class uses multiplications instead of additions

@davidpq : if the FIFO is full, the plug-in won’t crash, it’s just that the Convolution class won’t add any new request. That’s the theory Are you telling me that you had crashes during your tests ?