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

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 :slight_smile:

3 Likes

fantastic

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.

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 :wink:

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

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 :).

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

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.
1 Like

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

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?

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

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

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).

Right, I even proposed that in another thread regarding ADSR :see_no_evil: 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 :wink:

The LogSmoothedValue class uses multiplications instead of additions :wink:

@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 :slight_smile: Are you telling me that you had crashes during your tests ?

It does (again using the debug project with a small timer update value), although now I feel I missed something obvious everyone knows :). Here it is anyway:

When the processFifo() method in juce_Convolution.cpp calls requestsType.setUnchecked (numRequests, type) in its while loop with numRequests = 256, it fires the jassert (isPositiveAndBelow (indexToChange, values.size())); in juce_Array.h (l.524). Juce version 5.4.1.

1 Like

Ok I think I’ve found the issue. At line 490, it should be while (getNumRemainingEntries() > 0 && numRequests < fifoSize) instead of while (getNumRemainingEntries() > 0).

Thanks for spotted it !

Hi @IvanC,

I submitted a Pull Request to expose the IR switch ramp duration. I find it convenient for VST users to define a plugin “Impulse Response update rate” to fit their current needs (zipper noise free, highly responsive, etc.). Let me know if it doesn’t make sense with what you had in mind for the Convolution class implementation.

1 Like

This would be really useful for me as well. It’d be great if it could be integrated into the main JUCE repo. Thanks for the work on this, @davidpq.

I think that would be a great addition, and your code seems good to me !

1 Like

Hi there,

I just wanted to ask, if there still is no viable solution for this convolution IR update artifacts problem yet?

My code uses a lot of IR updates and I recognized this “zipper noise” as well.

I also could not find a LogSmoothedValue class in JUCE 7 or an inclusion of the Pull Request from @davidpq.

I hope to hear from you!

Best regard
Marco