Extreme danger of perforating eardrum from audioDeviceIOCallback

[PeterEmanuelRoos]

@p-i

int codeFor(float* v, size_t N) {
    int nGood = 0;   
    for(float* p = v; p < v + N; ++p)
        nGood += (int)(*p >= -1 && *p <= 1);

This is not C++ - you’re messing with at least three types in the last two lines…

Why the strange pointer approach when you can also use a simple index in the v vector?

You compare float elements with int values.

And you are not aware of the standard abs () function?

The counter itself is not used, because any out-of-range value will result in a negative outcome.

Why not stop at the first out-of-range value? You keep counting, even after a clipped value.

Then you use a C-cast on a boolean expression, to cast it to an int value 1, to be used as an increment. Thus “knowing” that a “true” is implemented the same as a “1” (but how many bytes?)

static_cast <int> (expression)

is proper C++ - but you don’t need that counting construct at all.

This is a much simpler, more readable and more efficient example:

    ... codeFor (const float* v, size_t n)
    {
	    for (size_t i = 0; i < n; ++i)
		    if (std::abs (v[i]) > 1.0f)
			    return false;   // or an enum value

Your method should return the defined enum as type, not an int. I’ll skip the use of the struct there…

Does your compiler not warn you? Is it set to a very old language standard?

I have been harsh in this thread, I said sorry twice and then tried to stay out of it, but if you now try to advise / educate people with such poor code, you deserve some serious feedback.

[p-i]

@PeterEmanuelRoos You’re clearly an experienced audio engineer, and your technical contributions are of course of value. So why dilute the thread with ego-driven condescending clutter? To what benefit is your last paragraph? And of what value is an apology if you are not dropping the attitude that created the situation that led to the apology? You’re still doing it. Just stop doing the thing, please. Move the mind a different way, maybe. Let’s keep this on the rails.

I’m posting up code. If it sucks, hopefully someone will flag it. Maybe something useful will come of it. Same deal as umpteen posts a week from users of this forum. From whence do you infer the pretention of “seeking to advise/educate”? I suppose I am seeking to educate myself.

The loop looks wierd because I’m aiming to minimize typical CPU load, as that’s a concern on realtime-audio-render-thread. Most blocks are expected to be within the [-1.f, +1.f] range, so I want that loop fast. I’m no expert on optimization, so I’m being conservative. An if introduces branching, although I’m aware that modern CPUs can optimize for when the same branch is reached consecutively.

So while I can’t be certain my code is more efficient than a cleaner approach, I can be confident it is not significantly less. If an engineer cares to critique finding the sweetspot between structure and performance here over unknown chipsets, I am grateful.

There’s another gotcha: comparisons break on NaN;

    const float NaN = numeric_limits<float>::quiet_NaN();
    if(abs(NaN) > 1.f)
        cout << "This doesn't hit, as NaN comparisons return `false`";

This has a trickle-through effect too: max(NaN, 1.f) != max(1.f, NaN). Ouch.

I have to be aware of NaN, inf and maybe HUGE_VALF. Tests here: Compiler Explorer

If all values are numeric, but not all within [-1.f, +1.f] I think it makes sense to DBG the range. Useful debug data.

[p-i]

I did a little digging regarding the failure of JUCE to pre-zero the output buffer whenever it is reallocated. I’ll copy my message over from Discord, so as to keep everything in one place.

If I set a breakpoint in audioDeviceIOCallback and walk back up the stack, I find myself here:

juce-framework/JUCE/blob/master/modules/juce_audio_devices/audio_io/juce_AudioDeviceManager.cpp#L932

In this code AFAICS JUCE is rendering audio to a temporary buffer, before writing to the output buffer.

        tempBuffer.setSize (jmax (1, numOutputChannels), jmax (1, numSamples), false, false, true);

Now if that temporary buffer were zeroed by/following this operation ONLY IF IT WAS REALLOCATED / RESIZED, it would remove the risk that an engineer fails to manually zero the output buffer when implementing audioDeviceIOCallback. This would have negligable performance hit, as it’s a sparse operation.

AFAICS the penultimate flag is:

        @param clearExtraSpace      if this is true, then any extra channels or space that is
                                    allocated will be also be cleared. If false, then this space is left
                                    uninitialised.

So wouldn’t it be a no-brainer to simply switch it to true?

By logging the address of the float** outputChannelData param coming into audioDeviceIOCallback I notice it does not seem to change value up to route-change.

So it’s a one-time overhead up to route-change. AFAICS.

Does it remove all the landmines from the playing field? Nope. But it takes out the biggest one.

[Rincewind]

@p-i could you explain what your thoughts about „quitting“ was? I think in general its a good idea to try to capture that wrongdoing, but accessing a member after the dtor is called isn‘t really doing anything, right. When the memory is deallocated it will just result in an set fault.

[PeterEmanuelRoos]

My latest reply about your code quality was not so much to educate you, but to warn others (beginners?) that your posts should not be taken as good examples of how to approach audio coding.

Like how you file “reports” or suggest that there are issues in the Juce convolution code. While it is so obvious that you are making mistakes yourself.

So indeed, I am flagging this code sample of yours, like you suggest.

[p-i]

@Rincewind I have e.g.

class Mic
: public AudioIODeviceCallback
    juce::OwnedArray<RingResonator> resonators;

… and in audioDeviceIOCallback I pipe mic-input through each resonator.

(I have to work from memory as the fault isn’t reproducing now).
I found that upon quitting, I’d get an EXC_BAD_ACCESS.
Inspecting resonators I’d see that it doesn’t contain the full number of elements.
So I presumed that the base-class was firing audioDeviceIOCallback after the destruction of resonators had started.

Setting a quitting flag fixed it, and I didn’t investigate further.

[Rincewind]

But the base class isn‘t firing itself. The AudioDeviceManager does. Your problem is indicating, that you are destroying your callback, before removing it from the device manager.

[p-i]

Indeed, it was a getDeviceManager().addAudioCallback(&mic); without a matching call to getDeviceManager().removeAudioCallback(&mic);