Thread safety and efficiency

[bartolsthoorn]

Thread safety and efficiency

[bartolsthoorn]

Thank you for your message and the time you put into writing it, but I disagree with some of your points, considering the following:

1. NVDSP is completely stand-alone and does not rely on Novocaine. It simply processes raw audio samples, and can be used with any audio set-up. Any argument about Novocaine itself is irrelevant. So why is NVDSP called "Novocaine" DSP? At the time NVDSP was created, there simply wasn't another library providing the same functionality Novocaine does.
2. When NVDSP was released in 2012, TAAE was still at a very early development stage, and for quite some time there was no other library out there providing anything like TAAE. Also, NVDSP is a very minimal library. It deliberately is not as feature rich as TAAE and it should, in my opinion, not be compared to TAAE.
3. The audio thread calculates the coefficients right before processing a buffer, so a race condition like you described is impossible. You can change the parameters of the filters outside the audio thread, but the new coefficients will only be calculated once a new buffer is processed.
4. The glitches you might hear are either the real-time routine taking too long, or samples distorting (too much gain). NVDSP is very small and minimal and can be used in any way you want, which means you can mess up, but it is very easy in use because of its minimalism, and it allows the programmer to build extra safety checks where necessary.

But thank you for taking the time to voice your thoughts. I am sure the NVDSP code can be optimised though, and I am interested to hear about battery saving techniques or other code improvements.

[crontab]

On point 3, here is what you have:

- (void) setCenterFrequency:(float)_centerFrequency {
    centerFrequency = _centerFrequency;
    [self calculateCoefficients];
}

- (void) calculateCoefficients {
    if ((centerFrequency != 0.0f) && (Q != 0.0f)) {

        [self intermediateVariables:centerFrequency Q:Q];

        float A = sqrt(pow(10.0f, (G/20.0f)));

        a0 = (1  + (alpha / A));
        b0 = (1 + (alpha * A))     / a0; 
        b1 = (-2 * omegaC)         / a0; 
        b2 = (1 - (alpha * A))     / a0;
        a1 = (-2 * omegaC)         / a0; 
        a2 = (1 - alpha / A)       / a0;

        [super setCoefficients];
    }
}

On the one hand, setting the frequency from the audio thread at each cycle is hyper-overkill. Otherwise setting it from the main thread (e.g. as a result of a user moving a slider) introduces a race condition, because you are setting 5 or more parameters.

Either way, like I said NVDSP was not designed with efficiency in mind, and the above is not the only problem with it. How about dynamic allocation of buffers within the audio thread, etc.

Quite honestly, fixing these things would require a complete rewrite of the code.

I'd suggest that you leave this issue open so that the users are aware. It would be unfair with respect to your users to sweep these things under the carpet by closing the issues I open. But of course it's up to you at the end of the day ;)

[bartolsthoorn]

Ah now I understand your point about the race condition, sorry I missed it the first time. I must say that I never noticed any glitches when moving a slider, but I can imagine it can cause problems. By the way, I closed the other issue because it is fixed, I added the buffer offset and figured out why it was needed but not noticeable in audio.

I will re-open this issue so someone else may take a look at it in the future.

[crontab]

Bart, I think a few things can be fixed straight away.

Firstly, things that can be done outside the loop should be done outside the loop. For your filters you can recommend your users to set the parameters (frequency, gain etc.) from within the main thread but not the audio callback routine (And you could fix it in your example app in the first place). This way the coefficients will be calculated only when something is changed.

Next would be to pass the coefficients to the audio thread. What I did in my library was: have two versions of the coefficient arrays: userCoeffs and realTimeCoeffs. You also declare an int coeffsCopied. The idea is that when the user sets parameters and userCoeffs get recalculated, you drop the coeffsCopied flag.

Then in filterContiguousData you do something like this:

if (OSAtomicCompareAndSwap32(0, 1, &self->coeffsCopied))
    self->realTimeCoeffs = self->userCoeffs;

and go on feeding self->realTimeCoeffs to vDSP.

Thus, copying of the coefficients happens only once after they get modified by the user. This way you avoid race conditions and avoid locking.

Another obvious thing to do is to kill dynamic allocations in filterContiguousData. These buffers can be allocated/re-allocated only when numFrames don't fit the buffer. You can start with 0 in fact, so that next time you see numFrames is not zero you do your first allocation. In reality this will happen only once for each filter during app's execution, as CoreAudio usually sends the same amount of data (e.g. 512 on OSX and I think it's 4096 on iOS).

[bartolsthoorn]

Thank you for the clear instructions on what to change. I do not have time to implement this, unfortunately, but maybe someone else will come along and perform the changes you proposed.

[rweichler]

I think I may have a suggestion for implementing what @crontab is suggesting here:

if you want to implement a multi-band EQ you could avoid copying vDSP_deq22() buffers for each of the filters by swapping them.

Basically, the problem lies in this code:

- (void) filterContiguousData: (float *)data numFrames:(UInt32)numFrames channel:(UInt32)channel {

    // Provide buffer for processing
    float tInputBuffer[numFrames + 2];
    float tOutputBuffer[numFrames + 2];

    // Copy the data
    memcpy(tInputBuffer, gInputKeepBuffer[channel], 2 * sizeof(float));
    memcpy(tOutputBuffer, gOutputKeepBuffer[channel], 2 * sizeof(float));
    memcpy(&(tInputBuffer[2]), data, numFrames * sizeof(float)); //HUGE OVERHEAD

    // Do the processing
    vDSP_deq22(tInputBuffer, 1, coefficients, tOutputBuffer, 1, numFrames);

    // Copy the data
    memcpy(data, tOutputBuffer + 2, numFrames * sizeof(float)); //HUGE OVERHEAD
    memcpy(gInputKeepBuffer[channel], &(tInputBuffer[numFrames]), 2 * sizeof(float));
    memcpy(gOutputKeepBuffer[channel], &(tOutputBuffer[numFrames]), 2 * sizeof(float));
}

So with those two memcpy's I commented, you only really need to do them once. One time before you do the filtering, and one time after you do the filtering. You can just set tInputBuffer to be equal to tOutputBuffer and just pass it into the next time you filter it.

The problem is there really isn't a good way of doing without making the API a bit harder to use. It would basically go from this:

// apply the filter
for (int i = 0; i < 10; i++) {
    [PEQ[i] filterData:outData numFrames:numFrames numChannels:numChannels];
}

to something like this:

NVDSPBuffer *buffer = [NVDSPBuffer
                bufferWithData:outData
                numFrames:numFrames
                numChannels:numChannels];
// apply the filter
for (int i = 0; i < 10; i++) {
    [PEQ[i] filterBuffer:buffer];
}
[buffer copyBuffer:outData];

You basically have the temporary buffers stored in this NVDSPBuffer object which only copies to and from the actual sample data when its only absolutely necessary.

But yeah ugliness is definitely a tradeoff. Maybe make it optional? I dunno.

[bartolsthoorn]

Thanks a lot for your suggestion. I think I personally would like to encourage people to actually adjust NVDSP to their liking and to keep NVDSP in its current, very minimal, elegant state. I would also happily merge a performance-optimised version (with possibly a different API) in a separate branch.

[bartolsthoorn]

Alternatively, I would also recommend a performance-optimised fork in the README for that matter.

[spiderweb62]

I am trying to convert your code into Swift, and I wasn't sure about these 2 lines. Reading on the Japanese translated link, it looks like we need the last 2 samples (unfiltered and filtered) in the KeepBuffer, should:
memcpy(gInputKeepBuffer[channel], &(tInputBuffer[numFrames]), 2 * sizeof(float)); memcpy(gOutputKeepBuffer[channel], &(tOutputBuffer[numFrames]), 2 * sizeof(float));

Be more like:
memcpy(gInputKeepBuffer[channel], &(tInputBuffer[numFrames - 2]), 2 * sizeof(float)); memcpy(gOutputKeepBuffer[channel], &(tOutputBuffer[numFrames - 2]), 2 * sizeof(float));

I have done some testing on Playground and I confirmed the latter.

[bartolsthoorn]

@spiderweb62
I'm not sure so of that, but I wrote this 2 years ago so who knows haha! The 2 samples are needed in order to keep the input and output signals continuous.

The Japanese source does not use the -2 and note that the tInputBuffer is actually allocated with +2: https://github.com/bartolsthoorn/NVDSP/blob/master/NVDSP.mm#L69-L70
Also, see the vDSP_deq22 docs: https://developer.apple.com/reference/accelerate/1450297-vdsp_deq22?language=objc, which shows N + 2 elements are need in order to calculate N new elements (which I assume are then calculated all the way to the end of the N+2 buffer, because:

 memcpy(data, tOutputBuffer + 2, numFrames * sizeof(float));

skips the first two samples.

By the way, this seems to be unrelated to this issue so I'm unsure why you brought it up here.

[spiderweb62]

Oops, you're right, sorry! I guess this would go into the general comments section (if there would be one) :-)