Update GenericMidiControlProtocol to use shared/weak Controllable pointers

This fixes a race-condition when a controllable is deleted
while sending feedback to the device.

Previously there was a race-condition MIDIControllable::write_feedback()
triggered from rt-thread, processed in Surface-thread and deleting
a route or processor.

This is a first step, currently state-restore is not fully functional
session->controllable_by_id() does not cover all Controllables.

libs/surfaces/generic_midi/generic_midi_control_protocol.cc

@@ -106,10 +106,8 @@ GenericMidiControlProtocol::GenericMidiControlProtocol (Session& s)
 	 * thread
 	 */
 
-#if 0 // XXX temp. disabled for API change
+	Controllable::StartLearning.connect_same_thread (*this, boost::bind (&GenericMidiControlProtocol::start_learning, this, _1));
-Controllab:le::StartLearning.connect_same_thread (*this, boost::bind (&GenericMidiControlProtocol::start_learning, this, _1));
+					 Controllable::StopLearning.connect_same_thread (*this, boost::bind (&GenericMidiControlProtocol::stop_learning, this, _1));
-	Controllable::StopLearning.connect_same_thread (*this, boost::bind (&GenericMidiControlProtocol::stop_learning, this, _1));
-#endif
 
 	/* this signal is emitted by the process() callback, and if
 	 * send_feedback() is going to do anything, it should do it in the
@@ -345,9 +343,10 @@ GenericMidiControlProtocol::_send_feedback ()
 }
 
 bool
-GenericMidiControlProtocol::start_learning (Controllable* c)
+GenericMidiControlProtocol::start_learning (boost::weak_ptr <Controllable> wc)
 {
-	if (c == 0) {
+	boost::shared_ptr<Controllable> c = wc.lock ();
+	if (!c) {
 		return false;
 	}
 
@@ -401,7 +400,7 @@ GenericMidiControlProtocol::start_learning (Controllable* c)
 	}
 
 	if (!mc) {
-		mc = new MIDIControllable (this, *_input_port->parser(), *c, false);
+		mc = new MIDIControllable (this, *_input_port->parser(), c, false);
 		own_mc = true;
 	}
 
@@ -443,8 +442,13 @@ GenericMidiControlProtocol::learning_stopped (MIDIControllable* mc)
 }
 
 void
-GenericMidiControlProtocol::stop_learning (Controllable* c)
+GenericMidiControlProtocol::stop_learning (boost::weak_ptr<PBD::Controllable> wc)
 {
+	boost::shared_ptr<Controllable> c = wc.lock ();
+	if (!c) {
+		return;
+	}
+
 	Glib::Threads::Mutex::Lock lm (pending_lock);
 	Glib::Threads::Mutex::Lock lm2 (controllables_lock);
 	MIDIControllable* dptr = 0;
@@ -627,10 +631,10 @@ GenericMidiControlProtocol::set_state (const XMLNode& node, int version)
 					if ((*niter)->get_property ("id", id)) {
 
 						DEBUG_TRACE (DEBUG::GenericMidi, string_compose ("Relearned binding for session: Control ID: %1\n", id.to_s()));
-						Controllable* c = 0; // Controllable::by_id (id); // XXX temp. disabled for API change
+						boost::shared_ptr<PBD::Controllable> c = session->controllable_by_id (id); // XXX are these all?
 
 						if (c) {
-							MIDIControllable* mc = new MIDIControllable (this, *_input_port->parser(), *c, false);
+							MIDIControllable* mc = new MIDIControllable (this, *_input_port->parser(), c, false);
 
 							if (mc->set_state (**niter, version) == 0) {
 								controllables.push_back (mc);
@@ -1503,9 +1507,9 @@ GenericMidiControlProtocol::input_port() const
 }
 
 void
-GenericMidiControlProtocol::maybe_start_touch (Controllable* controllable)
+GenericMidiControlProtocol::maybe_start_touch (boost::shared_ptr<Controllable> controllable)
 {
-	AutomationControl *actl = dynamic_cast<AutomationControl*> (controllable);
+	boost::shared_ptr<AutomationControl> actl = boost::dynamic_pointer_cast<AutomationControl> (controllable);
 	if (actl) {
 		actl->start_touch (session->audible_sample ());
 	}

libs/surfaces/generic_midi/generic_midi_control_protocol.h

@@ -47,7 +47,7 @@ class MIDIFunction;
 class MIDIAction;
 
 class GenericMidiControlProtocol : public ARDOUR::ControlProtocol {
-  public:
+public:
 	GenericMidiControlProtocol (ARDOUR::Session&);
 	virtual ~GenericMidiControlProtocol();
 
@@ -68,7 +68,7 @@ class GenericMidiControlProtocol : public ARDOUR::ControlProtocol {
 
 	boost::shared_ptr<PBD::Controllable> lookup_controllable (std::string const &) const;
 
-	void maybe_start_touch (PBD::Controllable*);
+	void maybe_start_touch (boost::shared_ptr<PBD::Controllable>);
 
 	XMLNode& get_state ();
 	int set_state (const XMLNode&, int version);
@@ -110,7 +110,7 @@ class GenericMidiControlProtocol : public ARDOUR::ControlProtocol {
 
 	PBD::Signal0<void> ConnectionChange;
 
-  private:
+private:
 	boost::shared_ptr<ARDOUR::Bundle> _input_bundle;
 	boost::shared_ptr<ARDOUR::Bundle> _output_bundle;
 	boost::shared_ptr<ARDOUR::AsyncMIDIPort> _input_port;
@@ -147,8 +147,8 @@ class GenericMidiControlProtocol : public ARDOUR::ControlProtocol {
         Glib::Threads::Mutex controllables_lock;
         Glib::Threads::Mutex pending_lock;
 
-	bool start_learning (PBD::Controllable*);
+	bool start_learning (boost::weak_ptr<PBD::Controllable>);
-	void stop_learning (PBD::Controllable*);
+	void stop_learning (boost::weak_ptr<PBD::Controllable>);
 
 	void learning_stopped (MIDIControllable*);
 

libs/surfaces/generic_midi/midicontrollable.cc

@@ -47,7 +47,6 @@ using namespace ARDOUR;
 
 MIDIControllable::MIDIControllable (GenericMidiControlProtocol* s, MIDI::Parser& p, bool m)
 	: _surface (s)
-	, controllable (0)
 	, _parser (p)
 	, _momentary (m)
 {
@@ -65,12 +64,12 @@ MIDIControllable::MIDIControllable (GenericMidiControlProtocol* s, MIDI::Parser&
 	control_additional = (MIDI::byte) -1;
 }
 
-MIDIControllable::MIDIControllable (GenericMidiControlProtocol* s, MIDI::Parser& p, Controllable& c, bool m)
+MIDIControllable::MIDIControllable (GenericMidiControlProtocol* s, MIDI::Parser& p, boost::shared_ptr<PBD::Controllable> c, bool m)
 	: _surface (s)
 	, _parser (p)
 	, _momentary (m)
 {
-	set_controllable (&c);
+	set_controllable (c);
 
 	_learned = true; /* from controllable */
 	_ctltype = Ctl_Momentary;
@@ -119,29 +118,35 @@ MIDIControllable::drop_external_control ()
 	control_additional = (MIDI::byte) -1;
 }
 
-void
+boost::shared_ptr<PBD::Controllable>
-MIDIControllable::set_controllable (Controllable* c)
+MIDIControllable::get_controllable () const
 {
-	if (c == controllable) {
+	return _controllable.lock ();
+}
+
+void
+MIDIControllable::set_controllable (boost::shared_ptr<PBD::Controllable> c)
+{
+	if (c == _controllable.lock()) {
 		return;
 	}
 
 	controllable_death_connection.disconnect ();
 
-	controllable = c;
+	if (c) {
-
+		_controllable = boost::weak_ptr<PBD::Controllable> (c);
-	if (controllable) {
+		last_controllable_value = c->get_value();
-		last_controllable_value = controllable->get_value();
 	} else {
+		_controllable.reset();
 		last_controllable_value = 0.0f; // is there a better value?
 	}
 
 	last_incoming = 256;
 
-	if (controllable) {
+	if (c) {
-		controllable->Destroyed.connect (controllable_death_connection, MISSING_INVALIDATOR,
+		printf ("MIDIControllable::set %s\n", c->name().c_str());
-						 boost::bind (&MIDIControllable::drop_controllable, this, _1),
+		c->Destroyed.connect_same_thread (controllable_death_connection,
-						 MidiControlUI::instance());
+						 boost::bind (&MIDIControllable::drop_controllable, this, _1));
 	}
 }
 
@@ -171,6 +176,11 @@ MIDIControllable::stop_learning ()
 int
 MIDIControllable::control_to_midi (float val)
 {
+	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
+	if (!controllable) {
+		return 0;
+	}
+
 	if (controllable->is_gain_like()) {
 		return controllable->internal_to_interface (val) * max_value_for_type ();
 	}
@@ -186,7 +196,7 @@ MIDIControllable::control_to_midi (float val)
 			return 0;
 		}
 	} else {
-		AutomationControl *actl = dynamic_cast<AutomationControl*> (controllable);
+		boost::shared_ptr<AutomationControl> actl = boost::dynamic_pointer_cast<AutomationControl> (controllable);
 		if (actl) {
 			control_min = actl->internal_to_interface(control_min);
 			control_max = actl->internal_to_interface(control_max);
@@ -202,6 +212,10 @@ MIDIControllable::control_to_midi (float val)
 float
 MIDIControllable::midi_to_control (int val)
 {
+	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
+	if (!controllable) {
+		return 0;
+	}
 	/* fiddle with MIDI value so that we get an odd number of integer steps
 		and can thus represent "middle" precisely as 0.5. this maps to
 		the range 0..+1.0 (0 to 126)
@@ -219,7 +233,7 @@ MIDIControllable::midi_to_control (int val)
 	float control_range = control_max - control_min;
 	DEBUG_TRACE (DEBUG::GenericMidi, string_compose ("Min %1 Max %2 Range %3\n", control_min, control_max, control_range));
 
-	AutomationControl *actl = dynamic_cast<AutomationControl*> (controllable);
+	boost::shared_ptr<AutomationControl> actl = boost::dynamic_pointer_cast<AutomationControl> (controllable);
 	if (actl) {
 		if (fv == 0.f) return control_min;
 		if (fv == 1.f) return control_max;
@@ -256,7 +270,7 @@ MIDIControllable::lookup_controllable()
 		return -1;
 	}
 
-	set_controllable (c.get ());
+	set_controllable (c);
 
 	return 0;
 }
@@ -264,20 +278,26 @@ MIDIControllable::lookup_controllable()
 void
 MIDIControllable::drop_controllable (Controllable* c)
 {
-	if (c == controllable) {
+	printf ("MIDIControllable::drop_controllable ? %s\n", c->name().c_str());
-		set_controllable (0);
+
+	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
+	if (controllable && c == controllable.get()) {
+		set_controllable (boost::shared_ptr<PBD::Controllable>());
 	}
 }
 
 void
 MIDIControllable::midi_sense_note (Parser &, EventTwoBytes *msg, bool /*is_on*/)
 {
-	if (!controllable) {
+	if (_controllable.expired ()) {
 		if (lookup_controllable()) {
 			return;
 		}
 	}
 
+	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
+	assert (controllable);
+
 	_surface->maybe_start_touch (controllable);
 
 	if (!controllable->is_toggle()) {
@@ -299,12 +319,13 @@ MIDIControllable::midi_sense_note (Parser &, EventTwoBytes *msg, bool /*is_on*/)
 void
 MIDIControllable::midi_sense_controller (Parser &, EventTwoBytes *msg)
 {
-	if (!controllable) {
+	if (_controllable.expired ()) {
 		if (lookup_controllable ()) {
 			return;
 		}
 	}
 
+	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
 	assert (controllable);
 
 	_surface->maybe_start_touch (controllable);
@@ -420,12 +441,15 @@ MIDIControllable::midi_sense_controller (Parser &, EventTwoBytes *msg)
 void
 MIDIControllable::midi_sense_program_change (Parser &, MIDI::byte msg)
 {
-	if (!controllable) {
+	if (_controllable.expired ()) {
 		if (lookup_controllable ()) {
 			return;
 		}
 	}
 
+	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
+	assert (controllable);
+
 	_surface->maybe_start_touch (controllable);
 
 	if (msg == control_additional) {
@@ -446,12 +470,15 @@ MIDIControllable::midi_sense_program_change (Parser &, MIDI::byte msg)
 void
 MIDIControllable::midi_sense_pitchbend (Parser &, pitchbend_t pb)
 {
-	if (!controllable) {
+	if (_controllable.expired ()) {
 		if (lookup_controllable ()) {
 			return;
 		}
 	}
 
+	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
+	assert (controllable);
+
 	_surface->maybe_start_touch (controllable);
 
 	if (!controllable->is_toggle()) {
@@ -482,6 +509,7 @@ MIDIControllable::midi_receiver (Parser &, MIDI::byte *msg, size_t /*len*/)
 	_surface->check_used_event(msg[0], msg[1]);
 	bind_midi ((channel_t) (msg[0] & 0xf), eventType (msg[0] & 0xF0), msg[1]);
 
+	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
 	if (controllable) {
 		controllable->LearningFinished ();
 	}
@@ -491,6 +519,7 @@ void
 MIDIControllable::rpn_value_change (Parser&, uint16_t rpn, float val)
 {
 	if (control_rpn == rpn) {
+		boost::shared_ptr<Controllable> controllable = _controllable.lock ();
 		if (controllable) {
 			controllable->set_value (val, Controllable::UseGroup);
 		}
@@ -501,6 +530,7 @@ void
 MIDIControllable::nrpn_value_change (Parser&, uint16_t nrpn, float val)
 {
 	if (control_nrpn == nrpn) {
+		boost::shared_ptr<Controllable> controllable = _controllable.lock ();
 		if (controllable) {
 			controllable->set_value (val, Controllable::UseGroup);
 		}
@@ -511,6 +541,7 @@ void
 MIDIControllable::rpn_change (Parser&, uint16_t rpn, int dir)
 {
 	if (control_rpn == rpn) {
+		boost::shared_ptr<Controllable> controllable = _controllable.lock ();
 		if (controllable) {
 			/* XXX how to increment/decrement ? */
 			// controllable->set_value (val);
@@ -522,6 +553,7 @@ void
 MIDIControllable::nrpn_change (Parser&, uint16_t nrpn, int dir)
 {
 	if (control_nrpn == nrpn) {
+		boost::shared_ptr<Controllable> controllable = _controllable.lock ();
 		if (controllable) {
 			/* XXX how to increment/decrement ? */
 			// controllable->set_value (val);
@@ -629,10 +661,13 @@ MIDIControllable::bind_midi (channel_t chn, eventType ev, MIDI::byte additional)
 MIDI::byte*
 MIDIControllable::write_feedback (MIDI::byte* buf, int32_t& bufsize, bool /*force*/)
 {
-	if (!controllable || !_surface->get_feedback ()) {
+	if (_controllable.expired () || !_surface->get_feedback ()) {
 		return buf;
 	}
 
+	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
+	assert (controllable);
+
 	float val = controllable->get_value ();
 
 	/* Note that when sending RPN/NPRN we do two things:
@@ -768,7 +803,9 @@ MIDIControllable::get_state ()
 
 	XMLNode* node = new XMLNode ("MIDIControllable");
 
-	if (_current_uri.empty()) {
+	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
+
+	if (_current_uri.empty() && controllable) {
 		node->set_property ("id", controllable->id ());
 	} else {
 		node->set_property ("uri", _current_uri);

libs/surfaces/generic_midi/midicontrollable.h

@@ -43,9 +43,9 @@ namespace ARDOUR {
 
 class MIDIControllable : public PBD::Stateful
 {
-  public:
+public:
-        MIDIControllable (GenericMidiControlProtocol *, MIDI::Parser&, PBD::Controllable&, bool momentary);
+	MIDIControllable (GenericMidiControlProtocol*, MIDI::Parser&, boost::shared_ptr<PBD::Controllable>, bool momentary);
-        MIDIControllable (GenericMidiControlProtocol *, MIDI::Parser&, bool momentary = false);
+	MIDIControllable (GenericMidiControlProtocol*, MIDI::Parser&, bool momentary = false);
 	virtual ~MIDIControllable ();
 
 	int init (const std::string&);
@@ -89,8 +89,8 @@ class MIDIControllable : public PBD::Stateful
 	void set_encoder (Encoder val) { _encoder = val; }
 
 	MIDI::Parser& get_parser() { return _parser; }
-	PBD::Controllable* get_controllable() const { return controllable; }
+	void set_controllable (boost::shared_ptr<PBD::Controllable>);
-	void set_controllable (PBD::Controllable*);
+	boost::shared_ptr<PBD::Controllable> get_controllable () const;
 	const std::string& current_uri() const { return _current_uri; }
 
 	std::string control_description() const { return _control_description; }
@@ -108,16 +108,16 @@ class MIDIControllable : public PBD::Stateful
 	MIDI::eventType get_control_type () { return control_type; }
 	MIDI::byte get_control_additional () { return control_additional; }
 
-        int lookup_controllable();
+	int lookup_controllable();
 
-  private:
+private:
 
 	int max_value_for_type () const;
 
 	GenericMidiControlProtocol* _surface;
-	PBD::Controllable* controllable;
+	boost::weak_ptr<PBD::Controllable> _controllable;
 	std::string     _current_uri;
-        MIDI::Parser&   _parser;
+	MIDI::Parser&   _parser;
 	bool             setting;
 	int              last_value;
 	int              last_incoming;
@@ -159,4 +159,3 @@ class MIDIControllable : public PBD::Stateful
 };
 
 #endif // __gm_midicontrollable_h__
-