From c97116083fd51e1d3108cabf92c890c45292fbab Mon Sep 17 00:00:00 2001 From: Robin Gareus Date: Sat, 23 Mar 2019 16:32:48 +0100 Subject: 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. --- libs/surfaces/generic_midi/midicontrollable.cc | 187 ++++++++++++------------- libs/surfaces/generic_midi/midicontrollable.h | 7 +- 2 files changed, 91 insertions(+), 103 deletions(-) (limited to 'libs/surfaces/generic_midi') 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 MIDIControllable::get_controllable () const { - return _controllable.lock (); + return _controllable; } void MIDIControllable::set_controllable (boost::shared_ptr 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 (c); + _controllable = c; last_controllable_value = c->get_value(); } else { _controllable.reset(); @@ -144,8 +145,9 @@ MIDIControllable::set_controllable (boost::shared_ptr 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.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 actl = boost::dynamic_pointer_cast (controllable); + boost::shared_ptr actl = boost::dynamic_pointer_cast (_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.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 actl = boost::dynamic_pointer_cast (controllable); + boost::shared_ptr actl = boost::dynamic_pointer_cast (_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.lock (); - if (controllable && c == controllable.get()) { - set_controllable (boost::shared_ptr()); - } + set_controllable (boost::shared_ptr()); } 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.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.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.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.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.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.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.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.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.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.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.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 _controllable; + boost::shared_ptr _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); -- cgit v1.2.3