Fix generic-midi controllable race-condition

Continued work after e9b36f2bea. Prefer a shared_ptr<>.

MIDIControllable::write_feedback() runs in realtime context, directly
from the main process-thread. Synchronizing weak-pointers and deletion
across threads does not work reliably. Retaining a shared_ptr<> for
controllables that are in use can solve this.

2 files changed, 91 insertions, 103 deletions

diff --git a/libs/surfaces/generic_midi/midicontrollable.cc b/libs/surfaces/generic_midi/midicontrollable.cc
index a110f32904..741cd0c781 100644
--- a/libs/surfaces/generic_midi/midicontrollable.cc
+++ b/libs/surfaces/generic_midi/midicontrollable.cc
@@ -121,20 +121,21 @@ MIDIControllable::drop_external_control ()
 boost::shared_ptr<PBD::Controllable>
 MIDIControllable::get_controllable () const
 {
-	return _controllable.lock ();
+	return _controllable;
 }
 
 void
 MIDIControllable::set_controllable (boost::shared_ptr<PBD::Controllable> c)
 {
-	if (c == _controllable.lock()) {
+	Glib::Threads::Mutex::Lock lm (controllable_lock);
+	if (c && c == _controllable) {
 		return;
 	}
 
-	controllable_death_connection.disconnect ();
+	controllable_death_connections.drop_connections ();
 
 	if (c) {
-		_controllable = boost::weak_ptr<PBD::Controllable> (c);
+		_controllable = c;
 		last_controllable_value = c->get_value();
 	} else {
 		_controllable.reset();
@@ -144,8 +145,9 @@ MIDIControllable::set_controllable (boost::shared_ptr<PBD::Controllable> c)
 	last_incoming = 256;
 
 	if (c) {
-		c->Destroyed.connect_same_thread (controllable_death_connection,
-						 boost::bind (&MIDIControllable::drop_controllable, this, _1));
+		c->DropReferences.connect (controllable_death_connections, MISSING_INVALIDATOR,
+						 boost::bind (&MIDIControllable::drop_controllable, this),
+						 MidiControlUI::instance());
 	}
 }
 
@@ -175,27 +177,26 @@ MIDIControllable::stop_learning ()
 int
 MIDIControllable::control_to_midi (float val)
 {
-	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
-	if (!controllable) {
+	if (!_controllable) {
 		return 0;
 	}
 
-	if (controllable->is_gain_like()) {
-		return controllable->internal_to_interface (val) * max_value_for_type ();
+	if (_controllable->is_gain_like()) {
+		return _controllable->internal_to_interface (val) * max_value_for_type ();
 	}
 
-	float control_min = controllable->lower ();
-	float control_max = controllable->upper ();
+	float control_min = _controllable->lower ();
+	float control_max = _controllable->upper ();
 	float control_range = control_max - control_min;
 
-	if (controllable->is_toggle()) {
+	if (_controllable->is_toggle()) {
 		if (val >= (control_min + (control_range/2.0f))) {
 			return max_value_for_type();
 		} else {
 			return 0;
 		}
 	} else {
-		boost::shared_ptr<AutomationControl> actl = boost::dynamic_pointer_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);
@@ -211,8 +212,7 @@ MIDIControllable::control_to_midi (float val)
 float
 MIDIControllable::midi_to_control (int val)
 {
-	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
-	if (!controllable) {
+	if (!_controllable) {
 		return 0;
 	}
 	/* fiddle with MIDI value so that we get an odd number of integer steps
@@ -222,17 +222,17 @@ MIDIControllable::midi_to_control (int val)
 
 	float fv = (val == 0 ? 0 : float (val - 1) / (max_value_for_type() - 1));
 
-	if (controllable->is_gain_like()) {
-		return controllable->interface_to_internal (fv);
+	if (_controllable->is_gain_like()) {
+		return _controllable->interface_to_internal (fv);
 	}
 	DEBUG_TRACE (DEBUG::GenericMidi, string_compose ("Raw value %1 float %2\n", val, fv));
 
-	float control_min = controllable->lower ();
-	float control_max = controllable->upper ();
+	float control_min = _controllable->lower ();
+	float control_max = _controllable->upper ();
 	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));
 
-	boost::shared_ptr<AutomationControl> actl = boost::dynamic_pointer_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;
@@ -275,61 +275,56 @@ MIDIControllable::lookup_controllable()
 }
 
 void
-MIDIControllable::drop_controllable (Controllable* c)
+MIDIControllable::drop_controllable ()
 {
-	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
-	if (controllable && c == controllable.get()) {
-		set_controllable (boost::shared_ptr<PBD::Controllable>());
-	}
+	set_controllable (boost::shared_ptr<PBD::Controllable>());
 }
 
 void
 MIDIControllable::midi_sense_note (Parser &, EventTwoBytes *msg, bool /*is_on*/)
 {
-	if (_controllable.expired ()) {
+	if (!_controllable) {
 		if (lookup_controllable()) {
 			return;
 		}
 	}
 
-	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
-	assert (controllable);
+	assert (_controllable);
 
-	_surface->maybe_start_touch (controllable);
+	_surface->maybe_start_touch (_controllable);
 
-	if (!controllable->is_toggle()) {
+	if (!_controllable->is_toggle()) {
 		if (control_additional == msg->note_number) {
-			controllable->set_value (midi_to_control (msg->velocity), Controllable::UseGroup);
+			_controllable->set_value (midi_to_control (msg->velocity), Controllable::UseGroup);
 			DEBUG_TRACE (DEBUG::GenericMidi, string_compose ("Note %1 value %2  %3\n", (int) msg->note_number, (float) midi_to_control (msg->velocity), current_uri() ));
 		}
 	} else {
 		if (control_additional == msg->note_number) {
-			float new_value = controllable->get_value() > 0.5f ? 0.0f : 1.0f;
-			controllable->set_value (new_value, Controllable::UseGroup);
+			float new_value = _controllable->get_value() > 0.5f ? 0.0f : 1.0f;
+			_controllable->set_value (new_value, Controllable::UseGroup);
 			DEBUG_TRACE (DEBUG::GenericMidi, string_compose ("Note %1 Value %2  %3\n", (int) msg->note_number, (float) new_value, current_uri()));
 		}
 	}
 
-	last_value = (MIDI::byte) (controllable->get_value() * 127.0); // to prevent feedback fights
+	last_value = (MIDI::byte) (_controllable->get_value() * 127.0); // to prevent feedback fights
 }
 
 void
 MIDIControllable::midi_sense_controller (Parser &, EventTwoBytes *msg)
 {
-	if (_controllable.expired ()) {
+	if (!_controllable) {
 		if (lookup_controllable ()) {
 			return;
 		}
 	}
 
-	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
-	assert (controllable);
+	assert (_controllable);
 
-	_surface->maybe_start_touch (controllable);
+	_surface->maybe_start_touch (_controllable);
 
 	if (control_additional == msg->controller_number) {
 
-		if (!controllable->is_toggle()) {
+		if (!_controllable->is_toggle()) {
 			if (get_encoder() == No_enc) {
 				float new_value = msg->value;
 				float max_value = max(last_controllable_value, new_value);
@@ -339,8 +334,8 @@ MIDIControllable::midi_sense_controller (Parser &, EventTwoBytes *msg)
 
 				bool const in_sync = (
 					range < threshold &&
-					controllable->get_value() <= midi_to_control(max_value) &&
-					controllable->get_value() >= midi_to_control(min_value)
+					_controllable->get_value() <= midi_to_control(max_value) &&
+					_controllable->get_value() >= midi_to_control(min_value)
 					);
 
 				/* If the surface is not motorised, we try to prevent jumps when
@@ -349,7 +344,7 @@ MIDIControllable::midi_sense_controller (Parser &, EventTwoBytes *msg)
 				*/
 
 				if (in_sync || _surface->motorised ()) {
-					controllable->set_value (midi_to_control (new_value), Controllable::UseGroup);
+					_controllable->set_value (midi_to_control (new_value), Controllable::UseGroup);
 				}
 				DEBUG_TRACE (DEBUG::GenericMidi, string_compose ("MIDI CC %1 value %2  %3\n", (int) msg->controller_number, (float) midi_to_control(new_value), current_uri() ));
 
@@ -359,30 +354,30 @@ MIDIControllable::midi_sense_controller (Parser &, EventTwoBytes *msg)
 				switch (get_encoder()) {
 					case Enc_L:
 						if (msg->value > 0x40) {
-							controllable->set_value (midi_to_control (last_value - offset + 1), Controllable::UseGroup);
+							_controllable->set_value (midi_to_control (last_value - offset + 1), Controllable::UseGroup);
 						} else {
-							controllable->set_value (midi_to_control (last_value + offset + 1), Controllable::UseGroup);
+							_controllable->set_value (midi_to_control (last_value + offset + 1), Controllable::UseGroup);
 						}
 						break;
 					case Enc_R:
 						if (msg->value > 0x40) {
-							controllable->set_value (midi_to_control (last_value + offset + 1), Controllable::UseGroup);
+							_controllable->set_value (midi_to_control (last_value + offset + 1), Controllable::UseGroup);
 						} else {
-							controllable->set_value (midi_to_control (last_value - offset + 1), Controllable::UseGroup);
+							_controllable->set_value (midi_to_control (last_value - offset + 1), Controllable::UseGroup);
 						}
 						break;
 					case Enc_2:
 						if (msg->value > 0x40) {
-							controllable->set_value (midi_to_control (last_value - (0x7f - msg->value) + 1), Controllable::UseGroup);
+							_controllable->set_value (midi_to_control (last_value - (0x7f - msg->value) + 1), Controllable::UseGroup);
 						} else {
-							controllable->set_value (midi_to_control (last_value + offset + 1), Controllable::UseGroup);
+							_controllable->set_value (midi_to_control (last_value + offset + 1), Controllable::UseGroup);
 						}
 						break;
 					case Enc_B:
 						if (msg->value > 0x40) {
-							controllable->set_value (midi_to_control (last_value + offset + 1), Controllable::UseGroup);
+							_controllable->set_value (midi_to_control (last_value + offset + 1), Controllable::UseGroup);
 						} else {
-							controllable->set_value (midi_to_control (last_value - (0x40 - offset)), Controllable::UseGroup);
+							_controllable->set_value (midi_to_control (last_value - (0x40 - offset)), Controllable::UseGroup);
 						}
 						break;
 					default:
@@ -400,9 +395,9 @@ MIDIControllable::midi_sense_controller (Parser &, EventTwoBytes *msg)
 					/* relax ... first incoming message */
 				} else {
 					if (msg->value > last_incoming) {
-						controllable->set_value (1.0, Controllable::UseGroup);
+						_controllable->set_value (1.0, Controllable::UseGroup);
 					} else {
-						controllable->set_value (0.0, Controllable::UseGroup);
+						_controllable->set_value (0.0, Controllable::UseGroup);
 					}
 					DEBUG_TRACE (DEBUG::GenericMidi, string_compose ("dial Midi CC %1 value 1  %2\n", (int) msg->controller_number, current_uri()));
 				}
@@ -413,7 +408,7 @@ MIDIControllable::midi_sense_controller (Parser &, EventTwoBytes *msg)
 				 * time they are pressed.
 				 */
 				if (msg->value >= 0x40) {
-					controllable->set_value (controllable->get_value() >= 0.5 ? 0.0 : 1.0, Controllable::UseGroup);
+					_controllable->set_value (_controllable->get_value() >= 0.5 ? 0.0 : 1.0, Controllable::UseGroup);
 					DEBUG_TRACE (DEBUG::GenericMidi, string_compose ("toggle Midi CC %1 value 1  %2\n", (int) msg->controller_number, current_uri()));
 				}
 				break;
@@ -422,76 +417,74 @@ MIDIControllable::midi_sense_controller (Parser &, EventTwoBytes *msg)
 				   maintain state (i.e. they know they were pressed) and then send zero the next time.
 				*/
 				if (msg->value >= 0x40) {
-					controllable->set_value (controllable->get_value() >= 0.5 ? 0.0 : 1.0, Controllable::UseGroup);
+					_controllable->set_value (_controllable->get_value() >= 0.5 ? 0.0 : 1.0, Controllable::UseGroup);
 				} else {
-					controllable->set_value (0.0, Controllable::NoGroup);
+					_controllable->set_value (0.0, Controllable::NoGroup);
 					DEBUG_TRACE (DEBUG::GenericMidi, string_compose ("Midi CC %1 value 0  %2\n", (int) msg->controller_number, current_uri()));
 					break;
 				}
 			}
 		}
 
-		last_value = (MIDI::byte) (control_to_midi(controllable->get_value())); // to prevent feedback fights
+		last_value = (MIDI::byte) (control_to_midi(_controllable->get_value())); // to prevent feedback fights
 	}
 }
 
 void
 MIDIControllable::midi_sense_program_change (Parser &, MIDI::byte msg)
 {
-	if (_controllable.expired ()) {
+	if (!_controllable) {
 		if (lookup_controllable ()) {
 			return;
 		}
 	}
 
-	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
-	assert (controllable);
+	assert (_controllable);
 
-	_surface->maybe_start_touch (controllable);
+	_surface->maybe_start_touch (_controllable);
 
 	if (msg == control_additional) {
 
-		if (!controllable->is_toggle()) {
-			controllable->set_value (1.0, Controllable::UseGroup);
+		if (!_controllable->is_toggle()) {
+			_controllable->set_value (1.0, Controllable::UseGroup);
 			DEBUG_TRACE (DEBUG::GenericMidi, string_compose ("MIDI program %1 value 1.0  %3\n", (int) msg, current_uri() ));
 		} else  {
-			float new_value = controllable->get_value() > 0.5f ? 0.0f : 1.0f;
-			controllable->set_value (new_value, Controllable::UseGroup);
+			float new_value = _controllable->get_value() > 0.5f ? 0.0f : 1.0f;
+			_controllable->set_value (new_value, Controllable::UseGroup);
 			DEBUG_TRACE (DEBUG::GenericMidi, string_compose ("MIDI program %1 value %2  %3\n", (int) msg, (float) new_value, current_uri()));
 		}
 	}
 
-	last_value = (MIDI::byte) (controllable->get_value() * 127.0); // to prevent feedback fights
+	last_value = (MIDI::byte) (_controllable->get_value() * 127.0); // to prevent feedback fights
 }
 
 void
 MIDIControllable::midi_sense_pitchbend (Parser &, pitchbend_t pb)
 {
-	if (_controllable.expired ()) {
+	if (!_controllable) {
 		if (lookup_controllable ()) {
 			return;
 		}
 	}
 
-	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
-	assert (controllable);
+	assert (_controllable);
 
-	_surface->maybe_start_touch (controllable);
+	_surface->maybe_start_touch (_controllable);
 
-	if (!controllable->is_toggle()) {
-		controllable->set_value (midi_to_control (pb), Controllable::UseGroup);
+	if (!_controllable->is_toggle()) {
+		_controllable->set_value (midi_to_control (pb), Controllable::UseGroup);
 		DEBUG_TRACE (DEBUG::GenericMidi, string_compose ("MIDI pitchbend %1 value %2  %3\n", (int) control_channel, (float) midi_to_control (pb), current_uri() ));
 	} else {
 		if (pb > 8065.0f) {
-			controllable->set_value (1, Controllable::UseGroup);
+			_controllable->set_value (1, Controllable::UseGroup);
 			DEBUG_TRACE (DEBUG::GenericMidi, string_compose ("Midi pitchbend %1 value 1  %2\n", (int) control_channel, current_uri()));
 		} else {
-			controllable->set_value (0, Controllable::UseGroup);
+			_controllable->set_value (0, Controllable::UseGroup);
 			DEBUG_TRACE (DEBUG::GenericMidi, string_compose ("Midi pitchbend %1 value 0  %2\n", (int) control_channel, current_uri()));
 		}
 	}
 
-	last_value = control_to_midi (controllable->get_value ());
+	last_value = control_to_midi (_controllable->get_value ());
 }
 
 void
@@ -506,9 +499,8 @@ 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 ();
+	if (_controllable) {
+		_controllable->LearningFinished ();
 	}
 }
 
@@ -516,9 +508,8 @@ 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);
+		if (_controllable) {
+			_controllable->set_value (val, Controllable::UseGroup);
 		}
 	}
 }
@@ -527,9 +518,8 @@ 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);
+		if (_controllable) {
+			_controllable->set_value (val, Controllable::UseGroup);
 		}
 	}
 }
@@ -538,10 +528,9 @@ void
 MIDIControllable::rpn_change (Parser&, uint16_t rpn, int dir)
 {
 	if (control_rpn == rpn) {
-		boost::shared_ptr<Controllable> controllable = _controllable.lock ();
-		if (controllable) {
+		if (_controllable) {
 			/* XXX how to increment/decrement ? */
-			// controllable->set_value (val);
+			// _controllable->set_value (val);
 		}
 	}
 }
@@ -550,10 +539,9 @@ void
 MIDIControllable::nrpn_change (Parser&, uint16_t nrpn, int dir)
 {
 	if (control_nrpn == nrpn) {
-		boost::shared_ptr<Controllable> controllable = _controllable.lock ();
-		if (controllable) {
+		if (_controllable) {
 			/* XXX how to increment/decrement ? */
-			// controllable->set_value (val);
+			// _controllable->set_value (val);
 		}
 	}
 }
@@ -658,14 +646,15 @@ 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.expired () || !_surface->get_feedback ()) {
+	Glib::Threads::Mutex::Lock lm (controllable_lock, Glib::Threads::TRY_LOCK);
+	if (!lm.locked ()) {
+		return buf;
+	}
+	if (!_controllable || !_surface->get_feedback ()) {
 		return buf;
 	}
 
-	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
-	assert (controllable);
-
-	float val = controllable->get_value ();
+	float val = _controllable->get_value ();
 
 	/* Note that when sending RPN/NPRN we do two things:
 	 *
@@ -800,15 +789,13 @@ MIDIControllable::get_state ()
 
 	XMLNode* node = new XMLNode ("MIDIControllable");
 
-	boost::shared_ptr<Controllable> controllable = _controllable.lock ();
-
-	if (_current_uri.empty() && controllable) {
-		node->set_property ("id", controllable->id ());
+	if (_current_uri.empty() && _controllable) {
+		node->set_property ("id", _controllable->id ());
 	} else {
 		node->set_property ("uri", _current_uri);
 	}
 
-	if (controllable) {
+	if (_controllable) {
 		snprintf (buf, sizeof(buf), "0x%x", (int) control_type);
 		node->set_property ("event", (const char *)buf);
 		node->set_property ("channel", (int16_t)control_channel);
diff --git a/libs/surfaces/generic_midi/midicontrollable.h b/libs/surfaces/generic_midi/midicontrollable.h
index a3f4eaabc2..47cb4f0572 100644
--- a/libs/surfaces/generic_midi/midicontrollable.h
+++ b/libs/surfaces/generic_midi/midicontrollable.h
@@ -115,7 +115,7 @@ private:
 	int max_value_for_type () const;
 
 	GenericMidiControlProtocol* _surface;
-	boost::weak_ptr<PBD::Controllable> _controllable;
+	boost::shared_ptr<PBD::Controllable> _controllable;
 	std::string     _current_uri;
 	MIDI::Parser&   _parser;
 	bool             setting;
@@ -130,7 +130,7 @@ private:
 	int              midi_msg_id;      /* controller ID or note number */
 	PBD::ScopedConnection midi_sense_connection[2];
 	PBD::ScopedConnection midi_learn_connection;
-        PBD::ScopedConnection controllable_death_connection;
+	PBD::ScopedConnectionList controllable_death_connections;
 	/** the type of MIDI message that is used for this control */
 	MIDI::eventType  control_type;
 	MIDI::byte       control_additional;
@@ -142,7 +142,8 @@ private:
 	std::string     _what;
 	bool            _bank_relative;
 
-	void drop_controllable (PBD::Controllable*);
+	void drop_controllable ();
+	Glib::Threads::Mutex controllable_lock;
 
 	void midi_receiver (MIDI::Parser &p, MIDI::byte *, size_t);
 	void midi_sense_note (MIDI::Parser &, MIDI::EventTwoBytes *, bool is_on);