allanger/ardour
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Activity

Fix generic-midi controllable race-condition

Browse Source 
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.
This commit is contained in:
Robin Gareus
2019-03-23 16:32:48 +01:00
parent 1d5e5b3523
commit c97116083f
2 changed files with 91 additions and 103 deletions
Download Patch File Download Diff File Expand all files Collapse all files

187
libs/surfaces/generic_midi/midicontrollable.cc
View File

@@ -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);

7
libs/surfaces/generic_midi/midicontrollable.h
View File

@@ -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);