VCA/SlavableAutomationCtrl re-design:

* remember master-ctrl value on assignment & save with session
* Control/AutomationCtrl only stores ctrl's own value (w/o master)
* virtual AutomationControl::get_value () -> use SlavableAC method
* MasterRecord uses weak-ptr (fixes recursive ~Controllable() deadlock)

8 files changed, 120 insertions, 106 deletions

diff --git a/libs/ardour/ardour/automation_control.h b/libs/ardour/ardour/automation_control.h
index a16929afa5..9879767807 100644
--- a/libs/ardour/ardour/automation_control.h
+++ b/libs/ardour/ardour/automation_control.h
@@ -91,9 +91,10 @@ public:
 	void start_touch(double when);
 	void stop_touch(bool mark, double when);
 
-	/* inherited from PBD::Controllable.
-	 */
-	double get_value () const;
+	/* inherited from PBD::Controllable. */
+	virtual double get_value () const;
+	virtual double get_save_value () const;
+
 	/* inherited from PBD::Controllable.
 	 * Derived classes MUST call ::writable() to verify
 	 * that writing to the parameter is legal at that time.
diff --git a/libs/ardour/ardour/slavable_automation_control.h b/libs/ardour/ardour/slavable_automation_control.h
index ceafc79193..29ab2e1055 100644
--- a/libs/ardour/ardour/slavable_automation_control.h
+++ b/libs/ardour/ardour/slavable_automation_control.h
@@ -84,12 +84,21 @@ protected:
 
 	class MasterRecord {
 	public:
-		MasterRecord (boost::shared_ptr<AutomationControl> gc, double r)
+		MasterRecord (boost::weak_ptr<AutomationControl> gc, double vc, double vm)
 			: _master (gc)
 			, _yn (false)
+			, _val_ctrl (vc)
+			, _val_master (vm)
 		{}
 
-		boost::shared_ptr<AutomationControl> master() const { return _master; }
+		boost::shared_ptr<AutomationControl> master() const { assert(_master.lock()); return _master.lock(); }
+
+		double val_ctrl () const { return _val_ctrl; }
+		double val_master () const { return _val_master; }
+
+		double master_ratio () const { return _val_master == 0 ? master()->get_value() : master()->get_value() / _val_master; }
+
+		int set_state (XMLNode const&, int);
 
 		/* for boolean/toggled controls, we store a boolean value to
 		 * indicate if this master returned true/false (1.0/0.0) from
@@ -99,18 +108,22 @@ protected:
 		bool yn() const { return _yn; }
 		void set_yn (bool yn) { _yn = yn; }
 
-		PBD::ScopedConnection connection;
+		PBD::ScopedConnection changed_connection;
+		PBD::ScopedConnection dropped_connection;
 
   private:
-		boost::shared_ptr<AutomationControl> _master;
+		boost::weak_ptr<AutomationControl> _master;
 		/* holds most recently seen master value for boolean/toggle controls */
 		bool   _yn;
+
+		/* values at time of assignment */
+		double _val_ctrl;
+		double _val_master;
 	};
 
 	mutable Glib::Threads::RWLock master_lock;
 	typedef std::map<PBD::ID,MasterRecord> Masters;
 	Masters _masters;
-	std::map<boost::weak_ptr<AutomationControl>, PBD::ScopedConnection> masters_connections;
 
 	void   master_going_away (boost::weak_ptr<AutomationControl>);
 	double get_value_locked() const;
@@ -121,7 +134,7 @@ protected:
 	virtual bool boolean_automation_run_locked (framepos_t start, pframes_t len);
 	bool boolean_automation_run (framepos_t start, pframes_t len);
 
-	virtual void   master_changed (bool from_self, GroupControlDisposition gcd, boost::shared_ptr<AutomationControl>);
+	virtual void   master_changed (bool from_self, GroupControlDisposition gcd, boost::weak_ptr<AutomationControl>);
 	virtual double get_masters_value_locked () const;
 	virtual void   pre_remove_master (boost::shared_ptr<AutomationControl>) {}
 	virtual void   post_add_master (boost::shared_ptr<AutomationControl>) {}
diff --git a/libs/ardour/ardour/solo_control.h b/libs/ardour/ardour/solo_control.h
index cc86983582..8026e5ea26 100644
--- a/libs/ardour/ardour/solo_control.h
+++ b/libs/ardour/ardour/solo_control.h
@@ -98,7 +98,7 @@ class LIBARDOUR_API SoloControl : public SlavableAutomationControl
 
   protected:
 	void actually_set_value (double, PBD::Controllable::GroupControlDisposition group_override);
-	void master_changed (bool from_self, GroupControlDisposition, boost::shared_ptr<AutomationControl> m);
+	void master_changed (bool from_self, GroupControlDisposition, boost::weak_ptr<AutomationControl> m);
 	void pre_remove_master (boost::shared_ptr<AutomationControl>);
 	void post_add_master (boost::shared_ptr<AutomationControl>);
 
diff --git a/libs/ardour/ardour/solo_isolate_control.h b/libs/ardour/ardour/solo_isolate_control.h
index 1e49469067..190e0cdda3 100644
--- a/libs/ardour/ardour/solo_isolate_control.h
+++ b/libs/ardour/ardour/solo_isolate_control.h
@@ -73,7 +73,7 @@ class LIBARDOUR_API SoloIsolateControl : public SlavableAutomationControl
 	XMLNode& get_state ();
 
   protected:
-	void master_changed (bool from_self, PBD::Controllable::GroupControlDisposition gcd, boost::shared_ptr<AutomationControl>);
+	void master_changed (bool from_self, PBD::Controllable::GroupControlDisposition gcd, boost::weak_ptr<AutomationControl>);
 	void actually_set_value (double, PBD::Controllable::GroupControlDisposition group_override);
 
   private:
diff --git a/libs/ardour/automation_control.cc b/libs/ardour/automation_control.cc
index 4e81de8aaf..055c000bc9 100644
--- a/libs/ardour/automation_control.cc
+++ b/libs/ardour/automation_control.cc
@@ -92,6 +92,13 @@ AutomationControl::get_value() const
 	return Control::get_double (from_list, _session.transport_frame());
 }
 
+double
+AutomationControl::get_save_value() const
+{
+	/* save user-value, not incl masters */
+	return Control::get_double ();
+}
+
 void
 AutomationControl::pre_realtime_queue_stuff (double val, PBD::Controllable::GroupControlDisposition gcd)
 {
diff --git a/libs/ardour/slavable_automation_control.cc b/libs/ardour/slavable_automation_control.cc
index 5ab968f69c..adde7cdac3 100644
--- a/libs/ardour/slavable_automation_control.cc
+++ b/libs/ardour/slavable_automation_control.cc
@@ -57,7 +57,6 @@ SlavableAutomationControl::~SlavableAutomationControl ()
 double
 SlavableAutomationControl::get_masters_value_locked () const
 {
-
 	if (_desc.toggled) {
 		for (Masters::const_iterator mr = _masters.begin(); mr != _masters.end(); ++mr) {
 			if (mr->second.master()->get_value()) {
@@ -70,7 +69,7 @@ SlavableAutomationControl::get_masters_value_locked () const
 		double v = 1.0; /* the masters function as a scaling factor */
 
 		for (Masters::const_iterator mr = _masters.begin(); mr != _masters.end(); ++mr) {
-			v *= mr->second.master()->get_value ();
+			v *= mr->second.master_ratio ();
 		}
 
 		return v;
@@ -146,6 +145,7 @@ SlavableAutomationControl::masters_curve_multiply (framepos_t start, framepos_t
 			= boost::dynamic_pointer_cast<SlavableAutomationControl>(mr->second.master());
 		assert (sc);
 		rv |= sc->masters_curve_multiply (start, end, vec, veclen);
+		apply_gain_to_buffer (vec, veclen, mr->second.master_ratio ());
 	}
 	return rv;
 }
@@ -179,38 +179,19 @@ SlavableAutomationControl::add_master (boost::shared_ptr<AutomationControl> m, b
 	std::pair<Masters::iterator,bool> res;
 
 	{
+		const double master_value = m->get_value();
 		Glib::Threads::RWLock::WriterLock lm (master_lock);
 
-		pair<PBD::ID,MasterRecord> newpair (m->id(), MasterRecord (m, 1.0));
+		pair<PBD::ID,MasterRecord> newpair (m->id(), MasterRecord (boost::weak_ptr<AutomationControl> (m), get_value_locked(), master_value));
 		res = _masters.insert (newpair);
 
 		if (res.second) {
 
-			if (!loading) {
-
-				if (!_desc.toggled) {
-					const double master_value = m->get_value();
-
-					if (master_value == 0.0) {
-						AutomationControl::set_double (0.0, Controllable::NoGroup);
-					} else {
-						/* scale control's own value by
-						   amount that the master will
-						   contribute.
-						*/
-						AutomationControl::set_double ((Control::get_double() / master_value), Controllable::NoGroup);
-					}
-				}
-			}
-
 			/* note that we bind @param m as a weak_ptr<AutomationControl>, thus
 			   avoiding holding a reference to the control in the binding
 			   itself.
 			*/
-			assert (masters_connections.find (boost::weak_ptr<AutomationControl>(m)) == masters_connections.end());
-			PBD::ScopedConnection con;
-			m->DropReferences.connect_same_thread (con, boost::bind (&SlavableAutomationControl::master_going_away, this, boost::weak_ptr<AutomationControl>(m)));
-			masters_connections[boost::weak_ptr<AutomationControl>(m)] = con;
+			m->DropReferences.connect_same_thread (res.first->second.dropped_connection, boost::bind (&SlavableAutomationControl::master_going_away, this, boost::weak_ptr<AutomationControl>(m)));
 
 			/* Store the connection inside the MasterRecord, so
 			   that when we destroy it, the connection is destroyed
@@ -228,7 +209,7 @@ SlavableAutomationControl::add_master (boost::shared_ptr<AutomationControl> m, b
 			   because the change came from the master.
 			*/
 
-			m->Changed.connect_same_thread (res.first->second.connection, boost::bind (&SlavableAutomationControl::master_changed, this, _1, _2, m));
+			m->Changed.connect_same_thread (res.first->second.changed_connection, boost::bind (&SlavableAutomationControl::master_changed, this, _1, _2, boost::weak_ptr<AutomationControl>(m)));
 		}
 	}
 
@@ -295,14 +276,18 @@ SlavableAutomationControl::update_boolean_masters_records (boost::shared_ptr<Aut
 }
 
 void
-SlavableAutomationControl::master_changed (bool /*from_self*/, GroupControlDisposition gcd, boost::shared_ptr<AutomationControl> m)
+SlavableAutomationControl::master_changed (bool /*from_self*/, GroupControlDisposition gcd, boost::weak_ptr<AutomationControl> wm)
 {
+	boost::shared_ptr<AutomationControl> m = wm.lock ();
+	assert (m);
 	Glib::Threads::RWLock::ReaderLock lm (master_lock, Glib::Threads::TRY_LOCK);
 	if (!lm.locked ()) {
 		/* boolean_automation_run_locked () special case */
 		return;
 	}
 	bool send_signal = handle_master_change (m);
+	lm.release (); // update_boolean_masters_records() takes lock
+
 	update_boolean_masters_records (m);
 	if (send_signal) {
 		Changed (false, Controllable::NoGroup); /* EMIT SIGNAL */
@@ -321,38 +306,32 @@ SlavableAutomationControl::master_going_away (boost::weak_ptr<AutomationControl>
 void
 SlavableAutomationControl::remove_master (boost::shared_ptr<AutomationControl> m)
 {
+	if (_session.deletion_in_progress()) {
+		/* no reason to care about new values or sending signals */
+		return;
+	}
+
 	pre_remove_master (m);
+	double new_val = AutomationControl::get_double();
+	const double old_val = new_val;
 
 	{
 		Glib::Threads::RWLock::WriterLock lm (master_lock);
 
-		masters_connections.erase (boost::weak_ptr<AutomationControl>(m));
-		if (!_masters.erase (m->id())) {
-			return;
-		}
-
-		if (!_session.deletion_in_progress()) {
+		Masters::const_iterator mi = _masters.find (m->id ());
 
-			const double master_value = m->get_value ();
+		/* when un-assigning we apply the master-value permanently */
+		if (mi != _masters.end()) {
+			new_val *= mi->second.master_ratio ();
+		}
 
-			if (master_value == 0.0) {
-				/* slave would have been set to 0.0 as well,
-				   so just leave it there, and the user can
-				   bring it back up. this fits with the
-				   "removing a VCA does not change the level" rule.
-				*/
-			} else {
-				/* bump up the control's own value by the level
-				   of the master that is being removed.
-				*/
-				AutomationControl::set_double (AutomationControl::get_double() * master_value, Controllable::NoGroup);
-			}
+		if (!_masters.erase (m->id())) {
+			return;
 		}
 	}
 
-	if (_session.deletion_in_progress()) {
-		/* no reason to care about new values or sending signals */
-		return;
+	if (old_val != new_val) {
+		AutomationControl::set_double (new_val, Controllable::NoGroup);
 	}
 
 	MasterStatusChange (); /* EMIT SIGNAL */
@@ -365,31 +344,32 @@ SlavableAutomationControl::remove_master (boost::shared_ptr<AutomationControl> m
 void
 SlavableAutomationControl::clear_masters ()
 {
-	double current_value;
-	double new_value;
-	bool had_masters = false;
+	if (_session.deletion_in_progress()) {
+		/* no reason to care about new values or sending signals */
+		return;
+	}
+
+	double new_val = AutomationControl::get_double();
+	const double old_val = new_val;
 
 	/* null ptr means "all masters */
 	pre_remove_master (boost::shared_ptr<AutomationControl>());
 
 	{
 		Glib::Threads::RWLock::WriterLock lm (master_lock);
-		current_value = get_value_locked ();
-		if (!_masters.empty()) {
-			had_masters = true;
+		if (_masters.empty()) {
+			return;
 		}
-		_masters.clear ();
-		masters_connections.clear ();
-		new_value = get_value_locked ();
-	}
+		/* permanently apply masters value */
+		new_val *= get_masters_value_locked ();
 
-	if (had_masters) {
-		MasterStatusChange (); /* EMIT SIGNAL */
+		_masters.clear ();
 	}
 
-	if (new_value != current_value) {
-		actually_set_value (current_value, Controllable::UseGroup);
+	if (old_val != new_val) {
+		AutomationControl::set_double (new_val, Controllable::NoGroup);
 	}
+	MasterStatusChange (); /* EMIT SIGNAL */
 
 	/* no need to update boolean masters records, since all MRs will have
 	 * been removed already.
@@ -446,6 +426,11 @@ SlavableAutomationControl::find_next_event_locked (double now, double end, Evora
 bool
 SlavableAutomationControl::handle_master_change (boost::shared_ptr<AutomationControl>)
 {
+	/* Derived classes can implement this for special cases (e.g. mute).
+	 * This method is called with a ReaderLock (master_lock) held.
+	 *
+	 * return true if the changed master value resulted
+	 * in a change of the control itself. */
 	return true; // emit Changed
 }
 
@@ -517,6 +502,23 @@ SlavableAutomationControl::slaved () const
 	return !_masters.empty();
 }
 
+int
+SlavableAutomationControl::MasterRecord::set_state (XMLNode const& n, int)
+{
+	bool yn;
+	double v;
+	if (n.get_property (X_("yn"), yn)) {
+		_yn = yn;
+	}
+	if (n.get_property (X_("val-ctrl"), v)) {
+		_val_ctrl = v;
+	}
+	if (n.get_property (X_("val-master"), v)) {
+		_val_master = v;
+	}
+	return 0;
+}
+
 void
 SlavableAutomationControl::use_saved_master_ratios ()
 {
@@ -526,28 +528,19 @@ SlavableAutomationControl::use_saved_master_ratios ()
 
 	Glib::Threads::RWLock::ReaderLock lm (master_lock);
 
-	/* use stored state, do not recompute */
-
-	if (_desc.toggled) {
-
-		XMLNodeList nlist = _masters_node->children();
-		XMLNodeIterator niter;
-
-		for (niter = nlist.begin(); niter != nlist.end(); ++niter) {
-			ID id_val;
-			bool yn;
-			if (!(*niter)->get_property (X_("id"), id_val) || !(*niter)->get_property (X_("yn"), yn)) {
-				continue;
-		  }
+	XMLNodeList nlist = _masters_node->children();
+	XMLNodeIterator niter;
 
-			Masters::iterator mi = _masters.find (id_val);
-			if (mi != _masters.end()) {
-				mi->second.set_yn (yn);
-			}
+	for (niter = nlist.begin(); niter != nlist.end(); ++niter) {
+		ID id_val;
+		if (!(*niter)->get_property (X_("id"), id_val)) {
+			continue;
 		}
-
-	} else {
-
+		Masters::iterator mi = _masters.find (id_val);
+		if (mi == _masters.end()) {
+			continue;
+		}
+		mi->second.set_state (**niter, Stateful::loading_state_version);
 	}
 
 	delete _masters_node;
@@ -566,23 +559,21 @@ SlavableAutomationControl::get_state ()
 
 	{
 		Glib::Threads::RWLock::ReaderLock lm (master_lock);
-
 		if (!_masters.empty()) {
-
 			XMLNode* masters_node = new XMLNode (X_("masters"));
+			for (Masters::iterator mr = _masters.begin(); mr != _masters.end(); ++mr) {
+				XMLNode* mnode = new XMLNode (X_("master"));
+				mnode->set_property (X_("id"), mr->second.master()->id());
 
-			if (_desc.toggled) {
-				for (Masters::iterator mr = _masters.begin(); mr != _masters.end(); ++mr) {
-					XMLNode* mnode = new XMLNode (X_("master"));
-					mnode->set_property (X_("id"), mr->second.master()->id());
+				if (_desc.toggled) {
 					mnode->set_property (X_("yn"), mr->second.yn());
-					masters_node->add_child_nocopy (*mnode);
+				} else {
+					mnode->set_property (X_("val-ctrl"), mr->second.val_ctrl());
+					mnode->set_property (X_("val-master"), mr->second.val_master());
 				}
-			} else {
-
+				masters_node->add_child_nocopy (*mnode);
+				node.add_child_nocopy (*masters_node);
 			}
-
-			node.add_child_nocopy (*masters_node);
 		}
 	}
 
diff --git a/libs/ardour/solo_control.cc b/libs/ardour/solo_control.cc
index 0b07f88c6b..9d898493dc 100644
--- a/libs/ardour/solo_control.cc
+++ b/libs/ardour/solo_control.cc
@@ -259,8 +259,10 @@ SoloControl::get_state ()
 }
 
 void
-SoloControl::master_changed (bool /*from self*/, GroupControlDisposition, boost::shared_ptr<AutomationControl> m)
+SoloControl::master_changed (bool /*from self*/, GroupControlDisposition, boost::weak_ptr<AutomationControl> wm)
 {
+	boost::shared_ptr<AutomationControl> m = wm.lock ();
+	assert (m);
 	bool send_signal = false;
 
 	_transition_into_solo = 0;
diff --git a/libs/ardour/solo_isolate_control.cc b/libs/ardour/solo_isolate_control.cc
index 90132a951c..9be6e1a7d5 100644
--- a/libs/ardour/solo_isolate_control.cc
+++ b/libs/ardour/solo_isolate_control.cc
@@ -42,7 +42,7 @@ SoloIsolateControl::SoloIsolateControl (Session& session, std::string const & na
 }
 
 void
-SoloIsolateControl::master_changed (bool from_self, PBD::Controllable::GroupControlDisposition gcd, boost::shared_ptr<AutomationControl>)
+SoloIsolateControl::master_changed (bool from_self, PBD::Controllable::GroupControlDisposition gcd, boost::weak_ptr<AutomationControl>)
 {
 	if (!_soloable.can_solo()) {
 		return;