rename ParameterChanged signal in Plugin to ParameterChangedExternally to reflect its intent, and clean up the result.

The signal exists to notify listeners that something outside of the host's control (e.g. a plugin's own GUI for AU or VST)
has modified a plugin parameter. Previous code had strange feedback loops and ambiguous semantics.

Significant modification of LV2 GUI updating was required.

Still to be tested for feedback loop issues: AudioUnits

gtk2_ardour/lv2_plugin_ui.cc

@@ -20,7 +20,9 @@
 #include "ardour/lv2_plugin.h"
 #include "ardour/session.h"
 #include "pbd/error.h"
+#include "pbd/stacktrace.h"
 
+#include "gui_thread.h"
 #include "lv2_plugin_ui.h"
 #include "timers.h"
 
@@ -53,6 +55,9 @@ LV2PluginUI::write_from_ui(void*       controller,
 		}
 
 		boost::shared_ptr<AutomationControl> ac = me->_controllables[port_index];
+		/* Cache our local copy of the last value received from the GUI */
+		me->_values[port_index] = *(const float*) buffer;
+		/* Now update the control itself */
 		if (ac) {
 			ac->set_value(*(const float*)buffer);
 		}
@@ -120,26 +125,18 @@ LV2PluginUI::on_external_ui_closed(void* controller)
 }
 
 void
-LV2PluginUI::parameter_changed(uint32_t port_index, float val)
+LV2PluginUI::control_changed (uint32_t port_index)
 {
-	PlugUIBase::parameter_changed(port_index, val);
+	/* Must run in GUI thread because we modify _updates with no lock */
-
+	if (_lv2->get_parameter (port_index) != _values[port_index]) {
-	if (val != _values[port_index]) {
+		/* current plugin parameter does not match last value received
-		parameter_update(port_index, val);
+		   from GUI, so queue an update to push it to the GUI during
+		   our regular timeout.
+		*/
+		_updates.insert (port_index);
 	}
 }
 
-void
-LV2PluginUI::parameter_update(uint32_t port_index, float val)
-{
-	if (!_inst) {
-		return;
-	}
-
-	suil_instance_port_event((SuilInstance*)_inst, port_index, 4, 0, &val);
-	_values[port_index] = val;
-}
-
 bool
 LV2PluginUI::start_updating(GdkEventAny*)
 {
@@ -183,13 +180,14 @@ LV2PluginUI::output_update()
 		}
 	}
 
-	/* FIXME only works with control output ports (which is all we support now anyway) */
+	if (_inst) {
-	uint32_t nports = _output_ports.size();
+		for (Updates::iterator i = _updates.begin(); i != _updates.end(); ++i) {
-	for (uint32_t i = 0; i < nports; ++i) {
+			float val = _lv2->get_parameter (*i);
-		uint32_t index = _output_ports[i];
+			/* push current value to the GUI */
-		parameter_changed(index, _lv2->get_parameter(index));
+			suil_instance_port_event ((SuilInstance*)_inst, (*i), 4, 0, &val);
+		}
+		_updates.clear ();
 	}
-
 }
 
 LV2PluginUI::LV2PluginUI(boost::shared_ptr<PluginInsert> pi,
@@ -358,12 +356,14 @@ LV2PluginUI::lv2ui_instantiate(const std::string& title)
 		bool     ok;
 		uint32_t port = _lv2->nth_parameter(i, ok);
 		if (ok) {
-			_values[port]        = _lv2->get_parameter(port);
 			_controllables[port] = boost::dynamic_pointer_cast<ARDOUR::AutomationControl> (
 				insert->control(Evoral::Parameter(PluginAutomation, 0, port)));
 
-			if (_lv2->parameter_is_control(port) && _lv2->parameter_is_input(port)) {
+			/* FIXME only works with control output ports (which is all we support now anyway) */
-				parameter_update(port, _values[port]);
+			if (_controllables[port] && _lv2->parameter_is_control(port) && _lv2->parameter_is_input(port)) {
+				_controllables[port]->Changed.connect (control_connections, invalidator (*this), boost::bind (&LV2PluginUI::control_changed, this, port), gui_context());
+				/* queue for first update ("push") to GUI */
+				_updates.insert (port);
 			}
 		}
 	}
@@ -401,9 +401,7 @@ LV2PluginUI::lv2ui_free()
 
 LV2PluginUI::~LV2PluginUI ()
 {
-	if (_values) {
+	delete [] _values;
-		delete[] _values;
-	}
 
 	_message_update_connection.disconnect();
 	_screen_update_connection.disconnect();

gtk2_ardour/lv2_plugin_ui.h

@@ -26,6 +26,7 @@
 
 #include <list>
 #include <map>
+#include <set>
 #include <vector>
 
 #include <gtkmm/widget.h>
@@ -64,7 +65,7 @@ class LV2PluginUI : public PlugUIBase, public Gtk::VBox
 
   private:
 
-	void parameter_changed (uint32_t, float);
+	void control_changed (uint32_t);
 
 	typedef boost::shared_ptr<ARDOUR::AutomationControl> ControllableRef;
 
@@ -85,6 +86,8 @@ class LV2PluginUI : public PlugUIBase, public Gtk::VBox
 	LV2_Feature                          _parent_feature;
 	Gtk::Window*                         _win_ptr;
 	void*                                _inst;
+	typedef std::set<uint32_t> Updates;
+	Updates                              _updates;
 
 	static void on_external_ui_closed(void* controller);
 

gtk2_ardour/plugin_ui.cc

@@ -484,7 +484,7 @@ PlugUIBase::PlugUIBase (boost::shared_ptr<PluginInsert> pi)
 	plugin->PresetAdded.connect (*this, invalidator (*this), boost::bind (&PlugUIBase::preset_added_or_removed, this), gui_context ());
 	plugin->PresetRemoved.connect (*this, invalidator (*this), boost::bind (&PlugUIBase::preset_added_or_removed, this), gui_context ());
 	plugin->PresetLoaded.connect (*this, invalidator (*this), boost::bind (&PlugUIBase::update_preset, this), gui_context ());
-	plugin->ParameterChanged.connect (*this, invalidator (*this), boost::bind (&PlugUIBase::parameter_changed, this, _1, _2), gui_context ());
+	plugin->PresetDirty.connect (*this, invalidator (*this), boost::bind (&PlugUIBase::update_preset_modified, this), gui_context ());
 
 	insert->AutomationStateChanged.connect (*this, invalidator (*this), boost::bind (&PlugUIBase::automation_state_changed, this), gui_context());
 
@@ -813,12 +813,6 @@ PlugUIBase::update_preset_modified ()
 	}
 }
 
-void
-PlugUIBase::parameter_changed (uint32_t, float)
-{
-	update_preset_modified ();
-}
-
 void
 PlugUIBase::preset_added_or_removed ()
 {

gtk2_ardour/plugin_ui.h

@@ -170,7 +170,6 @@ class PlugUIBase : public virtual sigc::trackable, public PBD::ScopedConnectionL
 	void processor_active_changed (boost::weak_ptr<ARDOUR::Processor> p);
 	void plugin_going_away ();
 	void automation_state_changed ();
-	virtual void parameter_changed (uint32_t, float);
 	void preset_added_or_removed ();
 	void update_preset_modified ();
 

libs/ardour/ardour/plugin.h

@@ -208,10 +208,17 @@ class LIBARDOUR_API Plugin : public PBD::StatefulDestructible, public Latent
 	/** Emitted when a preset has been loaded */
 	PBD::Signal0<void> PresetLoaded;
 
+	/** Emitted when a parameter is altered in a way that may have
+	 *  changed the settings with respect to any loaded preset.
+	 */
+	PBD::Signal0<void> PresetDirty;
+
 	virtual bool has_editor () const = 0;
 
-	/** Emitted when any parameter changes */
+	/** Emitted when a parameter is altered by something outside of our
-	PBD::Signal2<void, uint32_t, float> ParameterChanged;
+	 * control, most typically a Plugin GUI/editor
+	 */
+	PBD::Signal2<void, uint32_t, float> ParameterChangedExternally;
 
 	virtual bool configure_io (ChanCount /*in*/, ChanCount /*out*/) { return true; }
 
@@ -272,9 +279,18 @@ class LIBARDOUR_API Plugin : public PBD::StatefulDestructible, public Latent
 protected:
 
 	friend class PluginInsert;
+	friend class Session;
 
+	/* Called when a parameter of the plugin is changed outside of this
+	 * host's control (typical via a plugin's own GUI/editor)
+	 */
+	void parameter_changed_externally (uint32_t which, float val);
+
+	/* should be overridden by plugin API specific derived types to
+	 * actually implement changing the parameter. The derived type should
+	 * call this after the change is made.
+	 */
 	virtual void set_parameter (uint32_t which, float val);
-	virtual void set_parameter_automated (uint32_t which, float val);
 
 	/** Do the actual saving of the current plugin settings to a preset of the provided name.
 	 *  Should return a URI on success, or an empty string on failure.

libs/ardour/ardour/plugin_insert.h

@@ -96,6 +96,7 @@ class LIBARDOUR_API PluginInsert : public Processor
 
 		void set_value (double val);
 		double get_value (void) const;
+		void catch_up_with_external_value (double val);
 		XMLNode& get_state();
 
 	private:
@@ -164,10 +165,9 @@ class LIBARDOUR_API PluginInsert : public Processor
 	/* disallow copy construction */
 	PluginInsert (const PluginInsert&);
 
-	void parameter_changed (uint32_t, float);
+	void parameter_changed_externally (uint32_t, float);
 
 	void  set_parameter (Evoral::Parameter param, float val);
-	float get_parameter (Evoral::Parameter param);
 
 	float default_parameter_value (const Evoral::Parameter& param);
 

libs/ardour/plugin.cc

@@ -356,19 +356,21 @@ Plugin::clear_preset ()
 	PresetLoaded (); /* EMIT SIGNAL */
 }
 
-/** @param val `plugin' value */
 void
-Plugin::set_parameter (uint32_t which, float)
+Plugin::set_parameter (uint32_t /* which */, float /* value */)
 {
 	_parameter_changed_since_last_preset = true;
 	_session.set_dirty ();
-	ParameterChanged (which, get_parameter (which)); /* EMIT SIGNAL */
+	PresetDirty (); /* EMIT SIGNAL */
 }
 
 void
-Plugin::set_parameter_automated (uint32_t which, float val)
+Plugin::parameter_changed_externally (uint32_t which, float /* value */)
 {
-	Plugin::set_parameter (which, val);
+	_parameter_changed_since_last_preset = true;
+	_session.set_dirty ();
+	ParameterChangedExternally (which, get_parameter (which)); /* EMIT SIGNAL */
+	PresetDirty (); /* EMIT SIGNAL */
 }
 
 int

libs/ardour/plugin_insert.cc

@@ -264,26 +264,53 @@ PluginInsert::create_automatable_parameters ()
 		}
 	}
 }
-
+/** Called when something outside of this host has modified a plugin
+ * parameter. Responsible for propagating the change to two places:
+ *
+ *   1) anything listening to the Control itself
+ *   2) any replicated plugins that make up this PluginInsert.
+ *
+ * The PluginInsert is connected to the ParameterChangedExternally signal for
+ * the first (primary) plugin, and here broadcasts that change to any others.
+ *
+ * XXX We should probably drop this whole replication idea (Paul, October 2015)
+ * since it isn't used by sensible plugin APIs (AU, LV2).
+ */
 void
-PluginInsert::parameter_changed (uint32_t which, float val)
+PluginInsert::parameter_changed_externally (uint32_t which, float val)
 {
 	boost::shared_ptr<AutomationControl> ac = automation_control (Evoral::Parameter (PluginAutomation, 0, which));
 
-	if (ac) {
+	/* First propagation: alter the underlying value of the control,
-		ac->set_value (val);
+	 * without telling the plugin(s) that own/use it to set it.
+	 */
 
-                Plugins::iterator i = _plugins.begin();
+	if (!ac) {
+		return;
+	}
 
-                /* don't set the first plugin, just all the slaves */
+	boost::shared_ptr<PluginControl> pc = boost::dynamic_pointer_cast<PluginControl> (ac);
 
-                if (i != _plugins.end()) {
+	if (pc) {
-                        ++i;
+		pc->catch_up_with_external_value (val);
-                        for (; i != _plugins.end(); ++i) {
+	}
-                                (*i)->set_parameter (which, val);
+
-                        }
+	/* Second propagation: tell all plugins except the first to
-                }
+	   update the value of this parameter. For sane plugin APIs,
-        }
+	   there are no other plugins, so this is a no-op in those
+	   cases.
+	*/
+
+	Plugins::iterator i = _plugins.begin();
+
+	/* don't set the first plugin, just all the slaves */
+
+	if (i != _plugins.end()) {
+		++i;
+		for (; i != _plugins.end(); ++i) {
+			(*i)->set_parameter (which, val);
+		}
+	}
 }
 
 int
@@ -506,41 +533,6 @@ PluginInsert::run (BufferSet& bufs, framepos_t start_frame, framepos_t /*end_fra
 
 }
 
-void
-PluginInsert::set_parameter (Evoral::Parameter param, float val)
-{
-	if (param.type() != PluginAutomation) {
-		return;
-	}
-
-	/* the others will be set from the event triggered by this */
-
-	_plugins[0]->set_parameter (param.id(), val);
-
-	boost::shared_ptr<AutomationControl> ac
-			= boost::dynamic_pointer_cast<AutomationControl>(control(param));
-
-	if (ac) {
-		ac->set_value(val);
-	} else {
-		warning << "set_parameter called for nonexistent parameter "
-			<< EventTypeMap::instance().to_symbol(param) << endmsg;
-	}
-
-	_session.set_dirty();
-}
-
-float
-PluginInsert::get_parameter (Evoral::Parameter param)
-{
-	if (param.type() != PluginAutomation) {
-		return 0.0;
-	} else {
-		assert (!_plugins.empty ());
-		return _plugins[0]->get_parameter (param.id());
-	}
-}
-
 void
 PluginInsert::automation_run (BufferSet& bufs, framepos_t start, pframes_t nframes)
 {
@@ -1317,6 +1309,12 @@ PluginInsert::PluginControl::set_value (double user_val)
 	AutomationControl::set_value (user_val);
 }
 
+void
+PluginInsert::PluginControl::catch_up_with_external_value (double user_val)
+{
+	AutomationControl::set_value (user_val);
+}
+
 XMLNode&
 PluginInsert::PluginControl::get_state ()
 {
@@ -1333,8 +1331,13 @@ PluginInsert::PluginControl::get_state ()
 double
 PluginInsert::PluginControl::get_value () const
 {
-	/* FIXME: probably should be taking out some lock here.. */
+	boost::shared_ptr<Plugin> plugin = _plugin->plugin (0);
-	return _plugin->get_parameter (_list->parameter());
+
+	if (!plugin) {
+		return 0.0;
+	}
+
+	return plugin->get_parameter (_list->parameter().id());
 }
 
 PluginInsert::PluginPropertyControl::PluginPropertyControl (PluginInsert*                     p,
@@ -1430,7 +1433,7 @@ PluginInsert::add_plugin (boost::shared_ptr<Plugin> plugin)
                 /* first (and probably only) plugin instance - connect to relevant signals
                  */
 
-		plugin->ParameterChanged.connect_same_thread (*this, boost::bind (&PluginInsert::parameter_changed, this, _1, _2));
+		plugin->ParameterChangedExternally.connect_same_thread (*this, boost::bind (&PluginInsert::parameter_changed_externally, this, _1, _2));
                 plugin->StartTouch.connect_same_thread (*this, boost::bind (&PluginInsert::start_touch, this, _1));
                 plugin->EndTouch.connect_same_thread (*this, boost::bind (&PluginInsert::end_touch, this, _1));
 	}

libs/ardour/session_vst.cc

@@ -86,7 +86,7 @@ intptr_t Session::vst_callback (
 		SHOW_CALLBACK ("audioMasterAutomate");
 		// index, value, returns 0
 		if (plug) {
-			plug->set_parameter_automated (index, opt);
+			plug->parameter_changed_externally (index, opt);
 		}
 		return 0;
 

libs/ardour/source.cc

@@ -178,7 +178,7 @@ Source::set_been_analysed (bool yn)
 			yn = false;
 		}
 	}
-	if (yn != _analysed); {
+	if (yn != _analysed) {
 		Glib::Threads::Mutex::Lock lm (_analysis_lock);
 		_analysed = yn;
 	}

libs/ardour/vst_plugin.cc

@@ -117,12 +117,6 @@ VSTPlugin::set_parameter (uint32_t which, float newval)
 	}
 }
 
-void
-VSTPlugin::set_parameter_automated (uint32_t which, float newval)
-{
-	Plugin::set_parameter_automated (which, newval);
-}
-
 uint32_t
 VSTPlugin::nth_parameter (uint32_t n, bool& ok) const
 {