Bug in NChanDelay.h appears when AAX plugin "hard" bypassed in Pro Tools - suggested fix

[rexmartin158]

Describe the bug AAX plugins that call SetLatency() render/play audio early when "hard bypassed" in Pro Tools. In other words, AAX Bypass is not time aligned.

To Reproduce Insert any plugin that calls "SetLatency()" into any track in Pro Tools. Toggle the Pro Tools plugin Bypass button on/off and compare to same track without plugin. Audio is properly delay compensated when plugin is ON but when "hard bypassed" (via Pro Tools plugin Bypass button) audio on that track renders and plays early.

So it appears NChanDelayLine is not working properly in AAX/Pro Tools. Not obvious why.

Expected behaviour Toggling DAW's plugin Bypass on/off should not change timing of audio.

IMPORTANT DETAILS Only an issue in AAX/Pro Tools. Plugins work properly in VST2, VST3 and AU versions. Affects both Windows and Mac.

[rexmartin158]

VS2019 debugger shows that when the plugin is bypassed in Pro Tools (using DAW bypass button) mLatency is shown correct; NChannelDelay is being run (via mLatencyDelay->ProcessBlock() in PassThroughBuffers()) - but bypassed audio is not being delayed to match latency for some reason.

Only anomaly I can see is that variable mBypassed shows "false". (I don't see mBypassed used in AAX so this may be irrelevant but still curious.)

[rexmartin158]

Found the problem - and suggested solution.

There appears to be a bug in NChanDelay.h ProcessBlock() whereby "readAddress" can have a negative value. mWriteAddress is initialized to 0 so this line creates a negative value for readAddress on the very first loop:

int32_t readAddress = mWriteAddress - mDTSamples;

readAddress of negative value does not work properly in a ring buffer/delay line.

Since all we need here is a simple ring delay I propose a fix by removing the existing code:

for (auto s = 0; s < nFrames; ++s)
    {
      int32_t readAddress = mWriteAddress - mDTSamples;
      readAddress %= mDTSamples;

      for (auto c = 0; c < mNInChans; c++)
      {
        if (c < mNOutChans)
        {
          T input = inputs[c][s];
          const int offset = c * mDTSamples;
          outputs[c][s] = buffer[offset + readAddress];
          buffer[offset + mWriteAddress] = input;
        }
      }

      mWriteAddress++;
      mWriteAddress %= mDTSamples;
    }

and replacing with this simplified code (where only one read/write pointer is required):

    for (int s = 0; s < nFrames; ++s)
    {
        for (int c = 0; c < mNInChans; c++)
        {
            if (c < mNOutChans)
            {
                double input = inputs[c][s];
                int offset = c * mDTSamples;
                outputs[c][s] = buffer[offset + mWriteAddress];
                buffer[offset + mWriteAddress] = input;
            }
        }

      mWriteAddress++;
      mWriteAddress %= mDTSamples;
    }

This delay line works properly and fixes the Bypass issue in Pro Tools. Not sure how the existing code works in VST2, VST3 and AU - perhaps those DAWs "handle" the negative array pointer whereas Pro Tools does not? IDK, but this fix works for all.

[AlexHarker]

Can you make a pull request for this and link it to this issue please?

The NChannelDelay is only used under VST3 and AAX to implement bypass latency compensation. For VST3 that will only be invoked if the plugin is bypassed using the bypass parameter (rather than host bypass) - it would be good to test that also.

Your logic tracks for the above and the simplified code looks like it should function correctly. As far as I can see the effect of the modulo on a negative number (which it will always be) should be to get the read address to run backwards (reducing rather than increasing) which should sound pretty odd, so I'm surprised that you were getting simply the wrong offset, but I haven't run a test - I'm just thinking it through in my head.

[AlexHarker]

My bad - the delay gets used in AU and other formats too, so this needs a bit more investigation to see why it isn't apparent in more circumstances.

[rexmartin158]

My bad - the delay gets used in AU and other formats too, so this needs a bit more investigation to see why it isn't apparent in more circumstances.

Perhaps some DAWs provide their own delay line for host bypass when latency is reported? IDK

I haven't worked much with Pull Requests but will give a try.

[AlexHarker]

I've reviewed the code, and whilst the simplification is valid, it doesn't seem to change the functionality of the code at all, as readAddress and mWriteAddress always have the same value when used in my testing (logically this should be the case, as we are subtracting the amount we are about to modulo by, although this depends how the modulo is applied when the subtraction is negative as I'd expect it to be all the time).

I don't know why this issue would be occurring in ProTools, as I can't test there, and I don't understand what would cause it (given that the proposed fix shouldn't make any difference at all according to my experience of running it in the debugger). @rexmartin158 - I will make a PR with similar changes soon (as well as others and minus the type change you made to a double which shouldn't be included). If you are happy after that that this issue is resolved please let me know.

[rexmartin158]

Alex,

I’ve been using the suggested change I posted here ever since I posted it. I use Pro Tools almost almost daily and it fixed the issue. No AAX users have complained to me since so it seems it’s working for them as well. I don’t recall how I fault isolated this at the time but I have often found that works in one DAW doesn’t work in another. If you don’t have Pro Tools to test this then you are just speculating.

Rex

On Sun, Nov 5, 2023 at 11:03 AM Alex Harker @.***> wrote:

I've reviewed the code, and whilst the simplification is valid, it doesn't seem to change the functionality of the code at all, as readAddress and mWriteAddress always have the same value when used in my testing (logically this should be the case, as we are subtracting the amount we are about to modulo by, although this depends how the modulo is applied when the subtraction is negative as I'd expect it to be oall the time).

I don't know why this issue would be occurring in ProTools, as I can't test there, and I don't understand what would cause it (given that the proposed fix shouldn't make any difference at all according to my experience of running it in the debugger). @rexmartin158 https://github.com/rexmartin158 - I will make a PR with similar changes soon (as well as others and minus the type change you made to a double which shouldn't be included). If you are happy after that that this issue is resolved please let me know.

— Reply to this email directly, view it on GitHub https://github.com/iPlug2/iPlug2/issues/796#issuecomment-1793818421, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUD4QBCOVNZLH2ZCNNR6F73YC7PJXAVCNFSM5JARKAGKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCNZZGM4DCOBUGIYQ . You are receiving this because you were mentioned.Message ID: @.***>

[rexmartin158]

Alex,

BTW - you can get a copy of the top-tier Pro Tools (Ultimate) by just asking for it. Contact Avid, tell them who you are and what you're doing (plugin developer) and they will set you up with a developer account. The software package they provide (for FREE on a year-by-year subscription) is incredible, especially considering that it's one of the most expensive DAWs out there. (Apple gives nothing to developers for free nor does Steinberg, Presonus or most others). It's extremely generous of Avid, IMO.

I have also purchased copies of Logic, Cubase, Ableton, Fruity Studio, Reaper and others to test the plugins I'm sending out to the world. They all have their own quirks and often what I think should work does not work. They all have to be tested. It's very time consuming, and sometimes leads to conditional code, but it's the only way to make a solid plugin product IME.

Cheers,

Rex

On Sun, Nov 5, 2023 at 11:35 AM Rex Martin @.***> wrote:

Alex,

I’ve been using the suggested change I posted here ever since I posted it. I use Pro Tools almost almost daily and it fixed the issue. No AAX users have complained to me since so it seems it’s working for them as well. I don’t recall how I fault isolated this at the time but I have often found that works in one DAW doesn’t work in another. If you don’t have Pro Tools to test this then you are just speculating.

Rex

On Sun, Nov 5, 2023 at 11:03 AM Alex Harker @.***> wrote:

I've reviewed the code, and whilst the simplification is valid, it doesn't seem to change the functionality of the code at all, as readAddress and mWriteAddress always have the same value when used in my testing (logically this should be the case, as we are subtracting the amount we are about to modulo by, although this depends how the modulo is applied when the subtraction is negative as I'd expect it to be oall the time).

I don't know why this issue would be occurring in ProTools, as I can't test there, and I don't understand what would cause it (given that the proposed fix shouldn't make any difference at all according to my experience of running it in the debugger). @rexmartin158 - I will make a PR with similar changes soon (as well as others and minus the type change you made to a double which shouldn't be included). If you are happy after that that this issue is resolved please let me know.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[AlexHarker]

If you don’t have Pro Tools to test this then you are just speculating.

With respect - running in a different DAW can't change the basic operation of compiled code. There are, of course, many reasons why the host can affect operation, but that's not obviously the case here. What I am reporting here is simply that I don't understand how the change fixed a problem, as I cannot understand what that problem was in the first place and the code functions in its current form as expected. I'm not sure what my speculation was above.

This is somewhat of a moot point, as I have pushed similar changes to those that you made on a PR (with alteration to remove the specific double type that was not optimal in the template method), so the code changes you are using will be included here after merging. However, whilst I can see how it simplifies the code (and that simplification would naturally create less opportunity for error) that doesn't explain the detail of the technical issue discussed here and how that would actually emerge.

[AlexHarker]

OK for completeness after some further investigation, here is the full situation. The modulo with an unsigned value in cases where the latency is a power of two operates correctly, but when the latency takes any other value it will fail. I did not see the mixing of signed and unsigned ints previously. the mixing of signs in the original code, combined with the use of modulos on negative values creates this harder to locate effect.

Here is some code that shows the value of readAddress and mWriteAddress for the same operations on them alone with a power of two delay:

https://onlinegdb.com/eA9Ls7Ya8R

This can be run in the browser and shows that they are always identical. The negative value does not occur because the modulo is with an unsigned int of the same type.

Here is some code that shows the value of readAddress and mWriteAddress for the same operations on them alone for a non power of two delay:

https://onlinegdb.com/N3YpzaQkz

Here we see that although the sign mixing forces readAddress to go upwards (the modulo is taken on a unsigned int) - this will cause an offset in the values that will depend on the overall delay.

The changed code avoids this issue by not taking a modulo of a negative value, and by recognising that because this code does not support changing delay times (the delay is always as long as the buffer) the two addresses will always be identical. The bug is not specific to AAX. This issue will be resolved when the PR is merged.

[rexmartin158]

Alex,

>> The bug is not specific to AAX. I recall now what made this difficult to fault isolate - until I realized what was going on. It seems that most DAWs use their own internal buffer when a plugin is "hard/DAW bypassed" and, therefore, NChannelDelay has no effect (the plugin code is not running in that case).

Pro Tools does require the processing of NChannelDelay and that's where I experienced the issue. AFAIK, Pro Tools seems to be the only DAW that requires this function. Either that or I didn't see it elsewhere as I had already made the change for Pro Tools.

Anyhow, glad you stuck with it and found the issue.

Rex

On Wed, Nov 8, 2023 at 12:18 PM Alex Harker @.***> wrote:

OK for completeness after some further investigation, here is the full situation. The modulo with an unsigned value in cases where the latency is a power of two operates correctly, but when the latency takes any other value it will fail. I did not see the mixing of signed and unsigned ints previously. the mixing of signs in the original code, combined with the use of modulos on negative values creates this harder to locate effect.

Here is some code that shows the value of readAddress and mWriteAddress for the same operations on them alone with a power of two delay:

https://onlinegdb.com/eA9Ls7Ya8R

This can be run in the browser and shows that they are always identical. The negative value does not occur because the modulo is with an unsigned int of the same type.

Here is some code that shows the value of readAddress and mWriteAddress for the same operations on them alone for a non power of two delay:

https://onlinegdb.com/N3YpzaQkz

Here we see that although the sign mixing forces readAddress to go upwards (the modulo is taken on a unsigned int) - this will cause an offset in the values that will depend on the overall delay.

The changed code avoids this issue by not taking a modulo of a negative value, and by recognising that because this code does not support changing delay times (the delay is always as long as the buffer) the two addresses will always be identical. The bug is not specific to AAX. This issue will be resolved when the PR is merged.

— Reply to this email directly, view it on GitHub https://github.com/iPlug2/iPlug2/issues/796#issuecomment-1802589472, or unsubscribe https://github.com/notifications/unsubscribe-auth/AUD4QBDLM3MN5B6DU5KQFHTYDPSINAVCNFSM5JARKAGKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBQGI2TQOJUG4ZA . You are receiving this because you were mentioned.Message ID: @.***>