JUCE6 Convolver assert while loading from memory

Hi guys,

I’m updating an already released product to JUCE 6 and I’m facing an issue with the new Convolver class.

In this product, we load our IR as “custom” files, which are base64 encoded WAV files. The user can change the IR via a list.

The method I wrote to load the file is:

if(file.existsAsFile() && file.getFileExtension() == ".myIR")
{
    MemoryBlock mb;
    std::unique_ptr<FileInputStream> fis = file.createInputStream();
    fis->readIntoMemoryBlock(mb);
                
    MemoryOutputStream outStream;
    bool result = juce::Base64::convertFromBase64(outStream, mb.toString());
                
    const double maxSamples = 200.0 * 44100.0 * 0.001;
    auto maxSize = static_cast<size_t> (roundToInt (sRate * (maxSamples / 44100.0))); //170ms
    convolver.loadImpulseResponse(outStream.getData(), outStream.getDataSize(), dsp::Convolution::Stereo::yes, dsp::Convolution::Trim::no, maxSize);
}

The only difference between J5 and J6 is in the use of the enum classes for loadImpulseResponse. All the rest is identical: we pass the specs in prepareToPlay, reset the convolver in AudioProcessor::reset() and so on.

The assert is triggered by ResamplingAudioSource::setResamplingRatio, because samplesInPerOutputSample is 0. That method is called by resampleImpulseResponse in the convolver, which is called by makeEngine. What I noticed is that originalSampleRate gets a value of 0, while it should be the specs’ sample rate if it cannot get the correct SR from the file.

Maybe my method is wrong, since I can’t deduct the sample rate of the file, but I had no problems with the old convolver.

Thanks!

I just checked out JUCE 5 and gave this a bit of a test. To emulate loading a file with a sample rate of 0, I modified copyAudioStreamInAudioBuffer so that the line that previously set currentInfo.originalSampleRate now reads:

            currentInfo.originalSampleRate = 0;

When I prepare the convolution in prepareToPlay, and then use the following processBlock implementation, I hit the ResamplingAudioSource assertion (again, in JUCE 5).

    void processBlock (juce::AudioBuffer<float>& buffer, juce::MidiBuffer&) override
    {                    
        const double maxSamples = 200.0 * 44100.0 * 0.001;
        auto maxSize = static_cast<size_t> (roundToInt (getSampleRate() * (maxSamples / 44100.0)));
        convolution.loadImpulseResponse (BinaryData::Impulse1_wav,
                                         BinaryData::Impulse1_wavSize,
                                         true,
                                         false,
                                         maxSize);

        juce::dsp::AudioBlock<float> block (buffer);
        juce::dsp::ProcessContextReplacing<float> context (block);
        convolution.process (context);
    }

Are you able to stick a breakpoint in copyAudioStreamInAudioBuffer and check what sample rate was being detected in JUCE 5? In JUCE 6, the file loading happens in loadStreamToBuffer, and it would be interesting to see whether the sample rate detected in that function matches that under JUCE 5.

Some tangentially related (but important) points: the version of loadImpulseResponse accepting ‘binaryData’ in JUCE 6 expects that the binaryData itself has a lifetime at least as long as the convolution object. This is so that the data can be loaded asynchronously on a background thread, without allocating on the audio thread. In the JUCE 5 convolution, this version of loadImpulseResponse allocated, making it unsuitable for real-time usage. This being the case, I wonder whether the issue here is simply that the convolution is trying to read into the buffer of outStream, which has gone out of scope by the point that the read begins, which would make this ‘undefined behaviour’.

Another important point is that the convolution member functions are not thread safe. That is, it’s unsafe to call loadImpulseResponse on the main thread while process is being called on the audio thread. If you want to load new impulse responses during playback, you should use some wait-free mechanism to queue the IR updates from the main thread, and then apply them during the processBlock call on the audio thread (check out the ConvolutionDemo in JUCE 6 for an example of this). This change was made because the threading model in the JUCE 5 convolution wasn’t really clear, and it wasn’t possible to make every function call both thread-safe and wait-free. To make the convolution as versatile as possible, we’ve opted to make all the functions wait-free (i.e. no allocations or locks), which means that users must supply their own synchronisation if they wish to update the IR during playback.

Hopefully that all makes sense. Let me know if you have any questions, and I’ll try to answer them.

Thank you very much, reuk. Yes, I suspect that the convolver on J5 were allocating the data passed to loadImpulseResponse, while yours transfer ownership and it’s most likely to make undefined behaviour since my outStream gets out of scope once the IR file is loaded. I’ll look closely to what you done in the convolution example and try to implement a wait-free mechanism to load our files.

Thanks again!

I rewrote the method to use the wait-free mechanics shown in the example file and it works like a charm. Here’s the modified version of my methods.

void loadImpulseFromMemoryFile(const File& file)
{
    if(file.existsAsFile() && file.getFileExtension() == ".myIR")
    {
        MemoryBlock mb;
        std::unique_ptr<FileInputStream> fis = file.createInputStream();
        fis->readIntoMemoryBlock(mb);
        
        MemoryOutputStream outStream;
        bool result = juce::Base64::convertFromBase64(outStream, mb.toString());
        
        if(result)
        {
            std::unique_ptr<MemoryInputStream> inputStream;
            inputStream.reset(new MemoryInputStream(outStream.getData(), outStream.getDataSize(), false));
            
            AudioFormatManager manager;
            manager.registerBasicFormats();
            std::unique_ptr<AudioFormatReader> reader { manager.createReaderFor (std::move (inputStream)) };
            
            if (reader == nullptr)
            {
                jassertfalse;
                return;
            }
            
            const double maxSamples = 200.0 * 44100.0 * 0.001;
            auto maxSize = roundToInt (sRate * (maxSamples / 44100.0)); //170ms
            maxSize = jmin(maxSize, static_cast<int>(reader->lengthInSamples));
            
            AudioBuffer<float> buffer (static_cast<int> (reader->numChannels),
                                       static_cast<int> (maxSize));
            reader->read (buffer.getArrayOfWritePointers(), buffer.getNumChannels(), 0, buffer.getNumSamples());
            
            bufferTransfer.set (BufferWithSampleRate { std::move (buffer), reader->sampleRate });
        }
    }
}

//============================================================================
void processContext(dsp::ProcessContextReplacing<float> context) noexcept
{
    ScopedNoDenormals noDenormals;
    
    // Load a new IR if there's one available. Note that this doesn't lock or allocate!
    bufferTransfer.get ([this] (BufferWithSampleRate& buf)
    {
        convolver.loadImpulseResponse (std::move (buf.buffer),
                                        buf.sampleRate,
                                        dsp::Convolution::Stereo::yes,
                                        dsp::Convolution::Trim::no,
                                        dsp::Convolution::Normalise::yes);
    });
    
    convolver.process(context);
}

Another question: is there an example of how to use the new non-uniformly partitioned convolution method? I would like to test this new class with long IR files, since the default convolution technique gets very slow with long IRs.

Thanks!

Great, glad to hear you got it working! Thanks for your understanding, I know it’s not much fun to rewrite code in response to a library change.

Using the new non-uniform mode should be as simple as passing Convolution::NonUniform { desiredHeadSize } to the constructor of the convolution.

Thanks reuk. HeaderSize is in samples?

That’s right, yes.

Hi @reuk,

I’m facing an issue when using NonUniform convolution. I declared the convolution as a global using a unique_ptr

std::unique_ptr<dsp::Convolution> convolver;

Then, I initialize it this way:

convolver.reset(new dsp::Convolution(dsp::Convolution::NonUniform { headSize }));

It seems to work, but suddenly (especially if I change presets in my plugin) it stops loading the IR files without firing any asserts or errors, so I’m clueless on debugging this.
That won’t happen if I don’t use NonUniform and I declare it as instance of the class

dsp::Convolution convolver;

Now, I wanted to verify if is the unique_ptr causing this, but if I declare the convolver this way, it won’t build

dsp::Convolution convolver(const dsp::Convolution::NonUniform{1024});

I tried different ways, but nothing worked.

Any advice?
Thanks,

It’s probably not a good idea to make any part of your DSP code global. If you have multiple instances of your plugin running in the same process (which is a definite possibility depending on the host) they will share the same global instances, which is probably not you want.

I recommend adding the Convolution as a data member of your AudioProcessor or some other class which is itself a (nested) data member of the AudioProcessor. Then, it can be initialised like so:

juce::dsp::Convolution convolver { juce::dsp::Convolution::NonUniform { 1024 } };

Alternatively, you could declare the data member like so:

juce::dsp::Convolution convolver;

Then, initialise the convolution in the class constructor initialiser list:

MyClassWhichOwnsAConvolver()
    : convolver (juce::dsp::Convolution::NonUniform { 1024 })
{}

You could also check the DSPModulePluginDemo to see some example uses of the Convolution class which use NonUniform processing.

Thanks. Looks like I totally missed using brackets instead of parenthesis during declaration.
Forgive my bad terminology. I meant that I declare the convolver as a private class member.

Hi @reuk, I’m facing a new problem
the plugin works fine, but as soon as I start pluginval (@dave96 ) to test it, it crashes instantly without providing any crash report. On debug, I get the convolution firing an assertion in AudioBlock:

jassert (newOffset + newLength <= numSamples);

It looks that newLength is always twice numSamples. That’s happening on both uniformed and non uniformed convolution engines.

The assert comes from MultichannelEngine::processSamples

const AudioBlock<float> fullTailBlock (tailBuffer);
const auto tailBlock = fullTailBlock.getSubBlock (0, (size_t) numSamples);

fullTailBlock is half the size of numSamples.

Thanks

EDIT

Convolution background loader thread using NonUniform engine, malloc break

    ~AppleFFT() override
    {
        if (fftSetup != nullptr)
        {
            vDSP_destroy_fftsetup (fftSetup);
            fftSetup = nullptr;
        }
    }
1 Like

Have you tried running pluginval from the IDE as described here: https://github.com/Tracktion/pluginval/blob/develop/docs/Debugging%20a%20failed%20validation.md

Hi dave,
yes, I have. My usual workflow involves building the plugin in running mode, then build and run pluginval in debug from the IDE. Updating to pluginval 0.2.7 and JUCE 6.0.1 fixed pluginval crash, but I’m still getting these errors while testing.

The size of fullTailBlock should always be at least the maximumBufferSize used to prepare the Convolution. If numSamples is greater than the size of fullTailBlock, that suggests that the input and output buffer size is larger than the maxBlockSize passed to the Convolution’s prepare, which would be a violation of the Convolution’s contract.

Please can you check that the Convolution is always prepared properly before process is first called, and that the block size passed to Convolution::process is never greater than the maximumBufferSize passed to Convolution::prepare?

1 Like

Thanks, @reuk. Convolution is always prepared in prepareToPlay and it’s one of the first rows we have there:

auto channels = static_cast<uint32> (jmin(getMainBusNumInputChannels(), getMainBusNumOutputChannels()));
    dsp::ProcessSpec spec{ sampleRate, static_cast<uint32> (samplesPerBlock), channels };
    
    oversampling->initProcessing(static_cast<size_t> (samplesPerBlock));
    convolver.prepareToPlay(spec); 

I’ll check the block sizes and get back to you here.

Given that the jassert is complaining about a power-of-two discrepency in the buffersize, it might be worth checking you’re passing the size of the oversampled buffer rather than the plain blocksize, if you’re running the convolution with oversampling.

That’s not the case. The convolver is used after the oversampled process, when the buffer has been decimated to the original sample rate.

EDIT: I forgot to mention that while using NonUniform convolution engine, sometimes the malloc break points to the HeapBlock destroyer in the Convolution Background Thread.

Here is another one

Always involving the NonUniform engine. If I switch back to the Uniform one, I don’t get these errors.

I built the attached plugin PIP as a VST3 with Address Sanitizer enabled, and then ran it under a build of Pluginval from the current develop, also built with Asan. The plugin passes Pluginval with a strictness of 10, so I guess this test isn’t a good model of the issue you’re seeing. Without seeing all of the code, it’s difficult to suggest anything concrete, though.

Have you tried building everything with Address Sanitizer? At the moment, the segfault may occur an indeterminate amount of time after the first instance of undefined behaviour, so the stack trace isn’t very useful. Asan should halt on the first instance of UB, though, which would provide more useful information.

/*******************************************************************************
 The block below describes the properties of this PIP. A PIP is a short snippet
 of code that can be read by the Projucer and used to generate a JUCE project.

 BEGIN_JUCE_PIP_METADATA

  name:             convolutiondemo
  version:          0.0.1

  dependencies:     juce_dsp
  exporters:        XCODE_MAC

  moduleFlags:      JUCE_STRICT_REFCOUNTEDPOINTER=1

  type:             AudioProcessor
  mainClass:        MyPlugin

 END_JUCE_PIP_METADATA

*******************************************************************************/

#pragma once


class MyPlugin  : public juce::AudioProcessor
{
public:
    MyPlugin()
        : AudioProcessor (BusesProperties().withInput  ("Input",  juce::AudioChannelSet::stereo())
                                           .withOutput ("Output", juce::AudioChannelSet::stereo()))
    {}

    void prepareToPlay (double sr, int bs) override
    {
        convolution.prepare ({ sr, static_cast<uint32_t> (bs), 2 });
    }

    void releaseResources() override {}

    void processBlock (juce::AudioBuffer<float>& buffer, juce::MidiBuffer&) override
    {
        juce::ScopedNoDenormals noDenormals;
        auto totalNumInputChannels  = getTotalNumInputChannels();
        auto totalNumOutputChannels = getTotalNumOutputChannels();

        for (auto i = totalNumInputChannels; i < totalNumOutputChannels; ++i)
            buffer.clear (i, 0, buffer.getNumSamples());

        for (int channel = 0; channel < totalNumInputChannels; ++channel)
        {
            auto* channelData = buffer.getWritePointer (channel);
        }

        // Allocates (so not a good idea in production code), but here
        // we're just testing to see whether loading impulses
        // during playback is problematic.
        convolution.loadImpulseResponse (juce::AudioBuffer<float> (2, 2048),
                                         getSampleRate(),
                                         juce::dsp::Convolution::Stereo::yes,
                                         juce::dsp::Convolution::Trim::no,
                                         juce::dsp::Convolution::Normalise::no);

        juce::dsp::AudioBlock<float> block (buffer);
        juce::dsp::ProcessContextReplacing<float> context (block);
        convolution.process (context);
    }

    juce::AudioProcessorEditor* createEditor() override          { return nullptr; }
    bool hasEditor() const override                              { return false;   }

    const juce::String getName() const override                  { return "convolutiondemo"; }
    bool acceptsMidi() const override                            { return false; }
    bool producesMidi() const override                           { return false; }
    double getTailLengthSeconds() const override                 { return 0; }

    int getNumPrograms() override                                { return 1; }
    int getCurrentProgram() override                             { return 0; }
    void setCurrentProgram (int) override                        {}
    const juce::String getProgramName (int) override             { return {}; }
    void changeProgramName (int, const juce::String&) override   {}

    void getStateInformation (juce::MemoryBlock& destData) override {}

    void setStateInformation (const void* data, int sizeInBytes) override {}

    bool isBusesLayoutSupported (const BusesLayout& layouts) const override
    {
        if (layouts.getMainOutputChannelSet() != juce::AudioChannelSet::stereo())
            return false;

        if (layouts.getMainOutputChannelSet() != layouts.getMainInputChannelSet())
            return false;

        return true;
    }

    juce::dsp::Convolution convolution { juce::dsp::Convolution::NonUniform { 1024 } };
};

Thanks for the PIP, @reuk. I’ve rebuilt everything with Asan enabled on both my plugin and pluginval, and the result is a heap buffer overflow in the same points as the stacks I posted earlier as screenshots.

I also tried with the PIP. It happens rarely, but still happens. Here’s a log with Asan report and some pluginval details (including the seed, to replicate the test). This time the VST2 version triggered the error.
convolutiondemo-1f32aa39-4.txt (54.4 KB)

Also, sometimes, another allocation error is triggered since the VST3 process is allocating a string in process while using getHostType().