Routing a single source to multiple inputs acts incorrectly

[alamaral]

I have a situation where I want to route, say, a microphone into two or more channels of jackmix. This way I can use the input volume independently on the individual inputs, and route the signal to multiple outputs where one of the input volume controls doesn't effect the signal to the other outputs, etc. There are use cases for this in my situation.

One thing that I noticed is that if I route a mic to inputs 1 and 2, then route input 1 to output 1 and input 2 to output 2, when I adjust the input volume of input 1 output 2 is effected also, when it shouldn't be.

The problem is that in the process function the code gets the same buffer pointer for each input, and uses the input volumes for both input channels to adjust the input levels on the same buffer. So, setting the volumes to -6dB on each channel causes the actual volume for both channels to be -12dB. Of course turning off one channel also turns off the other.

The fix for this is when creating the map of the inputs to the buffers, instead of using the original buffer allocate a new buffer, copy the data into it and store it in the map, so there is a unique copy of the channel data for each channel. At the end of the function run through the list and free the buffers that were allocated (or use smart pointers, etc).

Here is the diff for the changes I made:

diff --git a/backend/jack_backend.cpp b/backend/jack_backend.cpp
index 6e659b7..fa79393 100644
--- a/backend/jack_backend.cpp
+++ b/backend/jack_backend.cpp
@@ -276,10 +276,24 @@ int JackMix::process( jack_nframes_t nframes, void* arg ) {
                        backend->send_signal(event.buffer[1], event.buffer[2]);
        }

+       // In this loop we get a pointer to each input buffer, then allocate a new buffer and copy
+       // the data into it for use below.  If we don't do this then there is a subtle bug where
+       // if an output, say from a mono microphone, is routed to multiple different inputs, the code
+       // that adjusts the input levels processes the same input buffer multiple times.  This means
+       // that setting the input setting on any one of the inputs to zero (-42dB) causes all of the
+       // inputs with a common source to turn off, or setting 2 of the inputs to -6dB (with the rest
+       // at 0dB) causes all the inputs to be set to -12dB.  I'm pretty sure this is not intentional.
        QMap<QString,jack_default_audio_sample_t*> ins;
        JackMix::ports_it it;
-       for ( it = backend->in_ports.begin(); it!=backend->in_ports.end(); ++it )
-               ins.insert( it.key(), (jack_default_audio_sample_t*) jack_port_get_buffer( it.value(), nframes ) );
+       for ( it = backend->in_ports.begin(); it!=backend->in_ports.end(); ++it ) {
+               jack_default_audio_sample_t* stuff = (jack_default_audio_sample_t*) jack_port_get_buffer( it.value(), nframes );
+               jack_default_audio_sample_t* newStuff = new jack_default_audio_sample_t[nframes];
+
+               memcpy(newStuff, stuff, nframes * sizeof(jack_default_audio_sample_t));
+
+               ins.insert( it.key(), newStuff );
+       }
+
        QMap<QString,jack_default_audio_sample_t*> outs;
        for ( it = backend->out_ports.begin(); it != backend->out_ports.end(); ++it )
                outs.insert( it.key(), (jack_default_audio_sample_t*) jack_port_get_buffer( it.value(), nframes ) );
@@ -329,6 +343,15 @@ int JackMix::process( jack_nframes_t nframes, void* arg ) {

        // Send any information about channel levels
        backend->report();
+
+       // Free up buffers we allocated.
+       for ( in_it = backend->in_ports.begin(); in_it != backend->in_ports.end(); ++in_it ) {
+               QString key {in_it.key()};
+               jack_default_audio_sample_t* tmp = ins[ key ];
+
+               // Free the copy of the storage
+               delete tmp;
+       }

        return 0;
 }

[nickbailey]

Alan,

Thanks for that and it's great to get patches. I am not ignoring you! I've got about 500 students on my lecture courses at work this year, and of course we are having to do everything with Zoom which takes ages... I've only had half a day to myself since Christmas! (including weekends).

Your patch is very clear. I'll review it soon, but it'll be a little while.

Thanks very much again for your input.

Nick/. On Monday, 18 January 2021 20:07:54 GMT ALAN AMARAL wrote:

I have a situation where I want to route, say, a microphone into two or more channels of jackmix. This way I can use the input volume independently on the individual inputs, and route the signal to multiple outputs where one of the input volume controls doesn't effect the signal to the other outputs, etc. There are use cases for this in my situation.

One thing that I noticed is that if I route a mic to inputs 1 and 2, then route input 1 to output 1 and input 2 to output 2, when I adjust the input volume of input 1 output 2 is effected also, when it shouldn't be.

The problem is that in the process function the code gets the same buffer pointer for each input, and uses the input volumes for both input channels to adjust the input levels on the same buffer. So, setting the volumes to -6dB on each channel causes the actual volume for both channels to be -12dB. Of course turning off one channel also turns off the other.

The fix for this is when creating the map of the inputs to the buffers, instead of using the original buffer allocate a new buffer, copy the data into it and store it in the map, so there is a unique copy of the channel data for each channel. At the end of the function run through the list and free the buffers that were allocated (or use smart pointers, etc).

Here is the diff for the changes I made:

diff --git a/backend/jack_backend.cpp b/backend/jack_backend.cpp index 6e659b7..fa79393 100644 --- a/backend/jack_backend.cpp +++ b/backend/jack_backend.cpp @@ -276,10 +276,24 @@ int JackMix::process( jack_nframes_t nframes, void* arg ) { backend->send_signal(event.buffer[1], event.buffer[2]); }

• // In this loop we get a pointer to each input buffer, then allocate a new buffer and copy + // the data into it for use below. If we don't do this then there is a subtle bug where + // if an output, say from a mono microphone, is routed to multiple different inputs, the code + // that adjusts the input levels processes the same input buffer multiple times. This means + // that setting the input setting on any one of the inputs to zero (-42dB) causes all of the + // inputs with a common source to turn off, or setting 2 of the inputs to -6dB (with the rest + // at 0dB) causes all the inputs to be set to -12dB. I'm pretty sure this is not intentional. QMap<QString,jack_default_audio_sample_t*> ins; JackMix::ports_it it;
• for ( it = backend->in_ports.begin(); it!=backend->in_ports.end(); ++it ) - ins.insert( it.key(), (jack_default_audio_sample_t) jack_port_get_buffer( it.value(), nframes ) ); + for ( it = backend->in_ports.begin(); it!=backend->in_ports.end(); ++it ) { +
  jack_default_audio_sample_t stuff = (jack_default_audio_sample_t) jack_port_get_buffer( it.value(), nframes ); +
  jack_default_audio_sample_t newStuff = new jack_default_audio_sample_t[nframes]; +
• memcpy(newStuff, stuff, nframes * sizeof(jack_default_audio_sample_t)); +
• ins.insert( it.key(), newStuff );
• }

• QMap<QString,jack_default_audio_sample_t*> outs;
  for ( it = backend->out_ports.begin(); it !=

  backend->out_ports.end(); ++it ) outs.insert( it.key(), (jack_default_audio_sample_t) jack_port_get_buffer( it.value(), nframes ) ); @@ -329,6 +343,15 @@ int JackMix::process( jack_nframes_t nframes, void arg ) {

  // Send any information about channel levels
  backend->report();

• // Free up buffers we allocated.
• for ( in_it = backend->in_ports.begin(); in_it != backend->in_ports.end(); ++in_it ) { + QString key {in_it.key()};
• jack_default_audio_sample_t* tmp = ins[ key ];
• // Free the copy of the storage
• delete tmp;

• }

  return 0;

  }

[dsheeler]

I think it's likely not advisable to allocate the buffers in the process function because if I recall correctly, memory allocation is not real-time safe. Maybe a memory pool type thing is in order to do it safely.

[alamaral]

dsheeler: Yes, now that you mention it I believe you are correct about memory allocation. If the heap runs out of memory it has to do a break() system call to increase the address space to add memory to the heap. That's definitely not real time.

I guess the memory could also be allocated once and held onto by the JackMix object, then reused each time through process(). Of course the problem there is you don't necessarily know what the size of the buffers will be before process() is called. Is there a way to reliably determine the necessary buffer size before process() is called?

[dsheeler]

I think I've seen several programs just allocate something big enough for any reasonable period size.
jack_mixer has for this purpose:

define MAX_BLOCK_SIZE (4 * 4096)

[nickbailey]

I'm thinking about implementing a buffer pool baseed on the ideas above. I'm also thinking this functionality should be in a base class because whatever the backend, the "feature" will be there. The project student allocated to this will be starting soon. That will free up time so either (a) I can ask him/her to do it if I get a good one, or (b) justify spending time on it myself (for tutorial purposes). What do we think about putting that code in an abstract base class? That would mean pulse_backend etc could be added more easily in future.

[nickbailey]

Just revisiting this after a while. I'm thinking of basically implementing the fix proposed by @alamaral but using (GASP!) alloca. Noting that ins is created as a new automatic QMap, it wouldn't matter if the newstuff was also local scope. That would mean copying nframes samples onto the stack for each of the ins. I don't think that's too big a deal for the sort of machines that are going to be running this (they'll be running Qt, window manager etc etc). The fact is, the idea that the input fader is any different from a matrix fader turns out to be specious, and gives rise to this bug.

Also, looking in backend_interface.h, what gives with the two almost identical interp_fader templates? One is at https://github.com/kampfschlaefer/jackmix/blob/c09fd7e3c7f225b7fb2afa6bc5687b4dd45bd785/libcore/backend_interface.h#L249 and one is at https://github.com/kampfschlaefer/jackmix/blob/c09fd7e3c7f225b7fb2afa6bc5687b4dd45bd785/libcore/backend_interface.h#L301 ─ maybe @dsheeler can remember what we were doing there because I can't! I'm thinking that the "adjust inlevels" stuff around https://github.com/kampfschlaefer/jackmix/blob/c09fd7e3c7f225b7fb2afa6bc5687b4dd45bd785/backend/jack_backend.cpp#L293 should instead just use a new automatically allocated and zero'd buffer as its target. The output adjustment could just give the same pointer as source and destination and achieve an in-place adjustment. And we have better ways of zeroing buffers these days too...

Any comments?

[nickbailey]

I've just pushed what I suppose to be a fix to the above issue. https://github.com/kampfschlaefer/jackmix/commit/fbe4885680ec7b0458b2fd6bcc532a18ed2dd533

It turns out that input and output sliders are sufficiently different that they two template functions mentioned above do in fact seem to be different enough to justify it (although it is screaming out "refactor me"). It's a combination of buffer reuse and scaling vs addition in to the target. So that stays for the moment.

On entry to the process routine we know the buffer length and the number of inputs, so a variable length array is allocated automatically. Quite a lot on the stack, but hopefully not very many frames are being processed per period (that's the point of jackd, right?) See https://github.com/kampfschlaefer/jackmix/blob/fbe4885680ec7b0458b2fd6bcc532a18ed2dd533/backend/jack_backend.cpp#L291

I am marking this closed as I believe it is. If it crashes with stack overflows all the time, please re-open it.