Ported Saturation plugin to tracktion engine

[swesterfeld]

I ported the saturation device to the tracktion engine. The only thing that we may want to deal with is that I currently use what the other tracktion plugins use for enum valued properties and that is officially declared to be internal API. So we may want to duplicate the tracktion code in Ase later. The device is tested if you set SFZ=/path/to/some.sfz make run, then a 24db tanh saturation is added automatically (as we currently don't have the UI to do this).

[gemini-code-assist]

Code Review

This pull request introduces the SaturationPlugin, integrating it into the build system and test suite. Key changes include the implementation of the plugin using the Tracktion engine, adding missing reset logic in the DSP code, and updating the track plugin insertion logic to append new plugins to the end of the list. Review feedback highlights a potential null pointer dereference in the test code, suggests using defined identifiers instead of magic strings for parameter IDs, and recommends removing redundant cleanup calls in the plugin destructor. Additionally, using a scoped enum for plugin modes was suggested to improve code clarity.

[tim-janik]

Several issues:

• The PCH files (trkn/tracktion.hh) may only be included in .cc files, ever. See ase/trkn-utils.hh, we spend significant effort to keep this up while allowing references to trkn objects from classes in .hh files.

• I hope you agree that adding env variables to "enable" plugins is not an acceptable UI. AFAIU, the only reason we have this in place for Liquidsfz atm, is because you are working on debugging/integration of that specific plugin, and the immediate goal should be to get rid of this kind of hackery. Extending it is not an option. It seems like working on enabling device list editing through the UI would be in order first.

• Pointing to another plugin "because that's how it is done there" (compressor) cannot be a reason for best/proper practice (it can just be a personal reason for why something was done/copied in the first place). Instead, find out what the proper way for some logic should be, maybe that means copying behavior or fixing it in multiple places. In any case, leave a comment why something is / isn't done, so next time someone looks, the same investigation doesn't need to be repeated.

[swesterfeld]

I'll address your issues in different comments.

About detaching parameters.

Pointing to another plugin "because that's how it is done there" (compressor) cannot be a reason for best/proper practice

---

Ok, LLMs often answer something like (this one is grok):

i have a tracktion plugin with a parameter drive, i used drive->attachToCurrentValue (drive_value); in the constructor, do i need to detach in the destructor? why?

Yes, you should call drive->detachFromCurrentValue(); in the destructor.

Why?

attachToCurrentValue() (on AutomatableParameter or similar Tracktion parameter objects) creates a bidirectional link between the parameter and your juce::Value (or CachedValue) object (drive_value in your case).

This attachment:

• Keeps the Value in sync with the parameter.
• Registers listeners/callbacks internally.
• Can involve reference counting or internal pointers back to your value.

If you don't detach:

• When the plugin is destroyed, the parameter may still hold a reference/pointer to your now-destroyed drive_value.

• This can lead to crashes (use-after-free), dangling pointers, or assertion failures later (especially during plugin deletion, undo manager cleanup, or when the Edit/PluginManager destroys objects).

• Tracktion's parameter system is quite strict about proper lifetime management.

[...]

---

I tried to trigger this:

Basically, the AutomatableParameter for the property (i.e. drive) is ref-counted. So it can live longer than the Plugin itself. So if a juce::ValueTree change on the property happens after the plugin is destroyed, it tries to notify the juce::CachedValue for the parameter. Then, I get an address-sanitizer use-after-free crash. This is my minimal code to make it crash:

diff --git a/ase/project.cc b/ase/project.cc
index c56cd6bd..b6865f4a 100644
--- a/ase/project.cc
+++ b/ase/project.cc
@@ -300,6 +300,16 @@ test_sfz (ProjectImpl *project, te::Edit *edit, const String &filename)
 
   /* insert saturation after liquidsfz */
   auto splugin = trackimpl->create_plugin (SaturationPlugin::xmlTypeName);
+  juce::ValueTree vtree = splugin->state;
+  tracktion::AutomatableParameter::Ptr ptr = splugin->getAutomatableParameterByID ("drive");
+  main_loop->add ([vtree, ptr, i = 0] mutable {
+    vtree.setProperty ("drive", i++ & 1, nullptr);
+    return true;
+  }, std::chrono::seconds (1));
+  main_loop->add ([splugin] {
+    splugin->deleteFromParent();
+    return false;
+  }, std::chrono::seconds (5));
   for (auto* p : splugin->getAutomatableParameters())
     {
       if (p->paramID == "drive")
diff --git a/devices/saturation/saturationplugin.cc b/devices/saturation/saturationplugin.cc
index 69307a4b..81ff4243 100644
--- a/devices/saturation/saturationplugin.cc
+++ b/devices/saturation/saturationplugin.cc
@@ -44,7 +44,7 @@ SaturationPlugin::~SaturationPlugin()
 {
   notifyListenersOfDeletion();
 
-  drive->detachFromCurrentValue();
+  // drive->detachFromCurrentValue();
   mix->detachFromCurrentValue();
   mode->detachFromCurrentValue();
 }

Now whether this could happen in production, I am not entierly certain, because I assume that usually the parameters would not be referenced elsewhere after the plugin has been deleted, and even if they were, a ValueTree update would be uncommon. But I can't exclude the possibility, and even if it doesn't happen now, it could happen in 5 years if we add a fancy new UI feature or update the tracktion engine to a higher version. And these bugs are extremely nasty in practice because this could trigger only in 1 of 1000 cases in some scenario.

So I would recommend to always detach, which is what the other plugins do.

I must admit that I don't like the silent cleanup that the tracktion engine does if you forget to detach, maybe it would be better to at least print a message if the plugin fails to detach(), like so:

diff --git a/trkn/tracktion_engine/model/automation/tracktion_AutomatableParameter.cpp b/trkn/tracktion_engine/model/automation/tracktion_AutomatableParameter.cpp
index 165f31e7..4864e2d0 100644
--- a/trkn/tracktion_engine/model/automation/tracktion_AutomatableParameter.cpp
+++ b/trkn/tracktion_engine/model/automation/tracktion_AutomatableParameter.cpp
@@ -695,7 +695,10 @@ AutomatableParameter::~AutomatableParameter()
     automationSourceList.reset();
 
     if (attachedValue != nullptr)
+    {
+        printf ("AutomatableParameter destroyed, but value still attached (%s)\n", paramName.toRawUTF8());
         attachedValue->detach (this);
+    }
 }
 
 AutomatableParameter::ModifierAssignment::ModifierAssignment (Edit& e, const juce::ValueTree& v)

Then we at least would get a notification if we ever forget to detach one of the parameters.

[swesterfeld]

I hope you agree that adding env variables to "enable" plugins is not an acceptable UI. AFAIU, the only reason we have this in place for Liquidsfz atm, is because you are working on debugging/integration of that specific plugin, and the immediate goal should be to get rid of this kind of hackery. Extending it is not an option. It seems like working on enabling device list editing through the UI would be in order first.

Yes, I totally agree. As soon as the UI gets better, we can step by step reduce and finally remove the hacks. Steps would be

1. A first step would be device creation and deletion using the UI.
2. Then we might want to have a way to change parameters in the UI.
3. Finally also a file dialog for SFZ.
4. Note editing (but I've seen that this kind-of appears to work last time I tried).

I guess at least steps 1 and 2 (and maybe 4) would be necessary before trying to port BlepSynth to tracktion engine, because I doubt I would be able to get the whole plugin port up and running correctly without even being able to test it once.

[swesterfeld]

• The PCH files (trkn/tracktion.hh) may only be included in .cc files, ever. See ase/trkn-utils.hh, we spend significant effort to keep this up while allowing references to trkn objects from classes in .hh files.

Well if you look at the file, it is basically code that is shared between the plugin implementations (which are all in .cc files).

// This Source Code Form is licensed MPL-2.0: http://mozilla.org/MPL/2.0
#pragma once
#include "trkn/tracktion.hh"   // PCH include must come first

namespace Ase {

namespace IDs
{
  #define DECLARE_ID(name)  const juce::Identifier name (#name);

  DECLARE_ID (drive)
  DECLARE_ID (mix)
  DECLARE_ID (mode)
}

}

So what this does is just declaring a few juce::Identifier instances, so that IDs::mix for instance would re-use the same identifier, whether mix is referenced in the context of a reverb, delay or that saturation plugin. We may later also have other convenience functions shared between plugin implementations, for instance to add enum parameters (see TODO comment). I guess we could just remove the #include line which would make it fail to compile if you didn't include tracktion yourself. And/Or we could also move the header to the devices/ folder in order to make it clear that this is code to be used for device implementations.

It is btw possible to just re-declare the juce::Identifiers for each plugin (they will de-duplicate the string under the hood), so if you really just want to get rid of this header, we could do it. But I assume that we'll later on have other code that is shared between plugin implementations (and that needs juce or tracktion) so I don't think doing this would be helpful in the long run.

Edit:

This is just what ./trkn/tracktion_engine/utilities/tracktion_Identifiers.h does, btw. so this is why I wrote the code that way.

[tim-janik]

 if (attachedValue != nullptr)

• {

•    printf ("AutomatableParameter destroyed, but value still attached (%s)\n", paramName.toRawUTF8());
     attachedValue->detach (this);
  

• }

Using printf() pollutes the programs stdout stream, that destroys and entire class of use cases for unix programs (usually the main use cases). Always print warnings / errors / info to stderr. That said, we also dont spam stderr in an uncontrolled fashion, we have a logging system and tracktion is integrated with it. So simply use the tracktion logging macros to print that message. You may be right that having such a message should be in order, so that we can enforce cleaner code, or maybe it'll trigger way too often in tracktion and we don't want it. The only way to find out is to add it in and see what happens. Make this a separate PR and lets see what happens...

[tim-janik]

As soon as the UI gets better, we can step by step reduce and finally remove the hacks.

Ok, then help with getting the needed UI pieces in order first.
Adding more and more unusable hacks that may or may not be remembered to be cleaned up ion the future, or worse that'll need debugging to be rediscovered is untenable.
The CLEANUP and MIGRATION lists the project has to date are already far too long.

[tim-janik]

The only way the "trkn/tracktion.hh" (or "trkn/juce.hh") header may be included is as the second line in a .cc file right after the license, i.e.:

// This Source Code Form is licensed MPL-2.0: http://mozilla.org/MPL/2.0
#include "trkn/tracktion.hh" // PCH include must come first

This is by design, it's simple and won't change.
It is needed for the PCH auto detection machinery to work correctly and to avoid tracktion dependency creep in the ase/ header files.