added feature to automatically apply a delay to panned channels

[SReichelt]

The feature comes in the form of a new session property that specifies the delay time in ms/100%, 0 being the default so nothing changes for existing sessions. The delay is applied to the output channel with the lower percentage, proportionally to the difference in percentage between the two channels.

At the moment, the feature has been implemented for the 1in2out and 2in2out panners. The width control of the 2in2out panner is ignored. VBAP support can easily be added later.

Other minor changes made along the way:

• Use sqrt(1/2) instead of -3 dB for the pan law, as it is more accurate and available as a compile-time constant.
• The gain interpolation that is used when the panner is moved no longer remembers any state across calls to distribute_one(). This seemed to be unintentional and slightly broken.

To try this feature, open an existing session that includes panned tracks. Start playback, preferably through headphones, open the "Misc" tab of the session properties, and drag the slider to the right.

[SReichelt]

I have fixed the problematic memory allocation in real-time code. However, there is still the question whether these should be separate panners. Arguments in favor of the current solution:

• There is currently no way for the user to select different panners.
• If the user has to do more than to define a single session property, the feature becomes less useful. After all, it can already be simulated using a separate bus with a delay line plugin (per panner). Having a global session property is not just a matter of convenience; it also improves consistency within the session.
• Implementing the feature as separate panners is difficult (though not impossible) to do without code duplication. (In fact, there was already some duplication between the 1in2out and 2in2out panners, which I have factored out into the PanDelayBuffer class.)

[x42]

since early jan'14 ardour3 allows to select the panner-plugin (e.g. on stereo tracks you can currently choose between 2in2out panning and balance control). The infrastructure is done and current development in that area is only concerned with default settings and control-sharing (sends, internal panners for monitoring etc)..

Ideally these new delay-based panners would become separate panner plugins, mainly because delay-based panning only really makes sense for the 1in2out panner and stereo-balance, if at all. It's conceptually inappropriate for the Stereo Panner:
http://manual.ardour.org/mixing/panning/stereo_panner/#caveat
and likewise for planned future panner-modules (e.g. ambisonics). If a user wants delay s/he should consciously enable it (select panner).

[SReichelt]

Thanks for this info. I have updated the implementation accordingly. I managed to solve the code duplication problem I mentioned above, with the minor drawback that automation of delay-based panners is a little less efficient now. (I can personally live with that.)

However, the caveat you cite doesn't really apply, as the delay is added after panning. (It is a problem if you use a plugin to achieve the same thing. In theory, it also becomes a problem if a signal runs through two panners that are not in their default position, but in practice, this is both unlikely and only noticeable in extreme configurations.)

I have made the necessary changes to the "Stereo Balance" panner, but not added a version "with delay," as this panner differs from "Mono to Stereo" and "Equal Power Stereo" panners in an important way: The two latter ones follow the "pan law," for example if you pan the signal completely to one side, that side is amplified by 3 dB (assuming two equal signals in the case of the stereo panner). The "Stereo Balance" panner does not do this, so applying this feature would create a slight inconsistency in the relationship between loudness and delay. Would it be OK to change the "Stereo Balance" panner to follow the pan law as well?

I wasn't sure sure whether I could just add a "flags" field to the PanPluginDescriptor structure (which would break binary compatibility, of course). So I abused the highest bit of the "priority" field instead, but of course I would be equally happy with any other solution.

By the way, panner automation currently seems to be broken in "touch" mode. The panner moves but does not affect the audio signal.

[rgareus]

I just had a quick glance and it looks good in general.

Why did you modify the balance? I think it would be generally preferable to add 'balance+delay' as separate new panner plugin.

Also the priority for new alternative panners should be lower (not or'ed with some value) to provide backwards compatibility with old sessions that did not have these panners.

I'll read in more detail tomorow or saturaday.

[rgareus]

This is a no go. The original code can be vectorized. The new code calls a function for every audio-sample which not only has at least two if() branches but potentially calls more functions, for every audio-sample, for every track.. This is unacceptable.

The same applied to the inner loop of other panners.

[x42]

All panner modules are independent dynamically loadable modules (plugins). The _delay variants of the panners cannot depend on the corresponding plain panner being loaded.

I just tried the patch and it fails to load the new panners
libpan1in2out_delay.so: undefined symbol: _ZN6ARDOUR13Panner1in2out9get_stateEv

(note, libpan1in2out.so is already loaded, but since it's a plugin, the symbols are not mapped in global memory space).

I'm yet unsure what the best solution to this is, while also avoiding code duplication. Ideally every panner would be independent and have its own subdirectory in libs/panners/.

[x42]

This should be removed. 'use-delay-panners' is too specific. There should (and will be) be generic code to select some panner as default.

A patch to add delay-based panners should only add files in libs/panners/ moving the delaylines to libardour might be OK to reduce code-duplication. but changing panner_shell and panner_manager is out.

[SReichelt]

Lots of things to reply to... Hope I haven't overlooked anything.

Why did you modify the balance? I think it would be generally preferable to add 'balance+delay' as separate new panner plugin.

The modification is just a preparation to do exactly that (similarly to the modifications in the other panners). For the reason why I didn't actually add that particular panner, see my previous post.

Also the priority for new alternative panners should be lower (not or'ed with some value) to provide backwards compatibility with old sessions that did not have these panners.

By default, these panners are skipped by the new code in panner_manager.cc. The or'ed value should really be a flag, but as I said, I wasn't sure whether it would be OK to add a field to the structure. It might be obsolete anyway if things are done differently (see below).

This is a no go. The original code can be vectorized. The new code calls a function for every audio-sample which not only has at least two if() branches but potentially calls more functions, for every audio-sample, for every track.. This is unacceptable.

You are probably correct about the auto-vectorization; I need to fix that (except in panner_2in2out.cc maybe?). The function calls only happen if the panner is actually a delay-based one; otherwise it's just a single if (which the compiler could, in theory, even pull out of the loop). That's the performance tradeoff I was talking about. I don't see any better solution using normal inheritance, but there are solutions based on

• code duplication (for the entire panner, I think),
• templates (replacing PanDistributionBuffer by a class template parameter),
• preprocessor magic (and compiling the .cc files twice with different preprocessor defines).
  I hadn't thought of the last possibility, and now I believe that would be the least painful one. Does it seem reasonable?

All panner modules are independent dynamically loadable modules (plugins). The _delay variants of the panners cannot depend on the corresponding plain panner being loaded.

OK, I thought that if I referenced the library, the symbols would always be resolved. It worked on my machine; maybe it depends on the order in which the panners are loaded. Anyway, I guess this can be fixed easily by including the object file in both libraries instead (except that the plugin descriptor needs to be moved out of the way).

This should be removed. 'use-delay-panners' is too specific. There should (and will be) be generic code to select some panner as default.

I don't care about this part (the entire "part 3" commit) very much, but I think the other session property ('panning-delay') is a bit hard to understand without this context. In some previous post, I explained why this shouldn't be a per-panner setting. The problem now is that by default it doesn't have any effect. If there will be a way to change the default panner(s) in the future, I can more or less revert that commit, but I probably need to add some explanation to the 'panning-delay' property (and I should rename it to be more specific).

[rgareus]

On 02/27/2014 07:42 PM, Sebastian Reichelt wrote:

By default, these panners are skipped by the new code in panner_manager.cc.

Well, they should not be. A user can select whatever panner s/he deems
appropriate. They should always be available.

The only case that is currently not solved is "which default panner to
use". There is ongoing discussion about this. (e.g. by default, the
master bus should not have any active panner at all, and it should be
balance by default,...)

The idea is to make the current 'priority' field, which is part of the
fixed plugin-descriptor, dynamic for each session.

For a new track, the panner matching port-in/out count with the highest
priority wins. Explicit selection always trumps this. It is yet unclear
if we should have two priority numbers (one for tracks one for busses).
Once the discussion concludes the implementation will be easy.

This is a no go. The original code can be vectorized. The new code calls a function for every audio-sample which not only has at least two if() branches but potentially calls more functions, for every audio-sample, for every track.. This is unacceptable.

You are probably correct about the auto-vectorization; I need to fix that (except in panner_2in2out.cc maybe?). The function calls only happen if the panner is actually a delay-based one; otherwise it's just a single if (which the compiler could, in theory, even pull out of the loop). That's the performance tradeoff I was talking about.

The compiler won't pull it out. any 'if clause' usually prevents
vectorization. You can check with gcc's -ftree-vectorizer-verbose=7
option.

The compiler cannot know that 'active' can't change without performing a
complete static analysis. 'active' could theoretically change at any
time, except that ardour's current implementation regarding plugin
bypass is per cycle.

Anyway it's a major performance issue. Every route has a panner, if you
use the monitoring section even two. Internal deliveries do have hidden
ones as do Aux and External sends. While the average session may have
only 8 tracks. 64 track sessions with monitoring section are very common
and > 500 track sessions not unheard of.

For the same reason the default meter is written in custom SSE assembler
code and the filters for RMS and K-meters use an explicit loop-unrolled
implementation. All per-sample code is highly optimized in ardour.

I don't see any better solution using normal inheritance, [..]

• code duplication (for the entire panner, I think),
• templates (replacing PanDistributionBuffer by a class template parameter),
• preprocessor magic (and compiling the .cc files twice with different preprocessor defines).

Either is preferable for the eventual implementation :)

I hadn't thought of the last possibility, and now I believe that would be the least painful one. Does it seem reasonable?

yes, that's a good idea. It can also be used to make the variants
2in2out and 2in2out_delay not depend on each other. The shared code
could become a pre-processor template that's included in both with the
class-name being set though a #define.

I just did a pragmatic test, I've rebased your patch, cleaned it up a
bit and - for now - simply copy+pasted shared code. The good news: it
works properly, I checked a few test-tones with a scope. Great!

I've just pushed that to the 'delaypannertest' branch in ardour's git,
in case you're interested.

[SReichelt]

By default, these panners are skipped by the new code in panner_manager.cc.

Well, they should not be. A user can select whatever panner s/he deems appropriate. They should always be available.

Sorry, I was being unclear. They were only skipped when looking for the default panner, unless a session property was set. Of course, they could still be selected manually. So it was basically an ad-hoc implementation of a dynamic priority. Anyway...

The idea is to make the current 'priority' field, which is part of the fixed plugin-descriptor, dynamic for each session.

This sounds good. I'll leave it up to you (or whoever else) to decide how the delay-based panners should fit in there. I still think it would be nice to be able to select delay-based panning for the entire session in some way, but I don't have any opinions about the proper UI to do it.

by default, the master bus should not have any active panner at all, and it should be balance by default

This really makes me think the balance panner should follow the pan law as well (see one of my previous posts), as in:

diff --git a/libs/panners/stereobalance/panner_balance.cc b/libs/panners/stereobalance/panner_balance.cc
index 55fdcd5..856db89 100644
--- a/libs/panners/stereobalance/panner_balance.cc
+++ b/libs/panners/stereobalance/panner_balance.cc
@@ -140,18 +140,11 @@ Pannerbalance::update ()
        return;
    }

-   double const pos = position();
-
-   if (pos == .5) {
-       desired_gain[0] = 1.0;
-       desired_gain[1] = 1.0;
-   } else if (pos > .5) {
-       desired_gain[0] = 2 - 2. * pos;
-       desired_gain[1] = 1.0;
-   } else {
-       desired_gain[0] = 1.0;
-       desired_gain[1] = 2. * pos;
-   }
+   float const panR = position();
+   float const panL = 1.0f - panR;
+
+   desired_gain[0] = panL * (_pan_law_scale * panL + 1.0f - _pan_law_scale);
+   desired_gain[1] = panR * (_pan_law_scale * panR + 1.0f - _pan_law_scale);
 }

 bool
@@ -222,15 +215,8 @@ Pannerbalance::distribute_one_automated (AudioBuffer& srcbuf, BufferSet& obufs,
            pos = 1.0f - pos;
        }

-       float gain;
-       if (pos < .5f) {
-           gain = 2.0f * pos;
-       } else {
-           gain = 1.0f;
-       }
-
        dist_buf[which]->set_pan_position(pos);
-       dst[n] += dist_buf[which]->process(src[n] * gain);
+       dst[n] += dist_buf[which]->process(src[n] * pos * (_pan_law_scale * pos + 1.0f - _pan_law_scale));
    }

    /* XXX it would be nice to mark the buffer as written to */

(most of it shamelessly copied from panner_1in1out.cc)

The compiler cannot know that 'active' can't change without performing a complete static analysis. 'active' could theoretically change at any time, except that ardour's current implementation regarding plugin bypass is per cycle.

It's not really important any more, but this wasn't the 'active' flag of the panner. It was always false except in the delay-based panners.
AFAIK, unless something is declared 'volatile', the compiler can assume that it won't be changed by a different thread. Since the entire loop was inlined in this case, it was (in theory) possible to check locally that it could never change from false to true. But I admit it is not likely that a compiler would actually do that. I didn't check the outcome, and in any case it doesn't matter anymore.

Anyway it's a major performance issue. Every route has a panner, if you use the monitoring section even two. Internal deliveries do have hidden ones as do Aux and External sends. While the average session may have only 8 tracks. 64 track sessions with monitoring section are very common and > 500 track sessions not unheard of.

I agree that it is an issue, and I fixed it now according to what we had discussed. But you are making it look like I didn't think about performance at all, while this is probably the aspect I spent the most time on. The vectorization problem only affected panners with automation; the non-automation case is highly optimized both with and without delay. And in fact, I just checked that the corresponding code in the 2in2out panner is not vectorized because of the following line:

panR = max(0.f, min(1.f, panR));

(Apparently that line was added recently; I just noticed that it got merged incorrectly.)

yes, that's a good idea. It can also be used to make the variants 2in2out and 2in2out_delay not depend on each other. The shared code could become a pre-processor template that's included in both with the class-name being set though a #define.

Done.

I've just pushed that to the 'delaypannertest' branch in ardour's git, in case you're interested.

Thanks. I'm not really familiar with git, so: Should I fork that branch and then continue working on that, or can I just continue pushing to the master branch in my own repo?

[SReichelt]

I just rebased my cloned repository to Ardour 4. Not sure if GitHub handled this correctly. There should be just two commits now. Any chance of getting this into mainline?

[x42]

The issue with this is that it modifies existing panners.
This should really be a dedicated new panner module (now that ardour allows to select and choose panners).

[SReichelt]

That was true in the original implementation, but I changed it during the discussion above. The panners are selectable now. The remaining modifications to existing code are just to avoid code duplication.

[Anutrix]

Status?

[luzpaz]

Is this PR still relevant ?

[SReichelt]

Not sure if you are asking me. I can't really answer that question, it depends on whether this feature is wanted or not.