Fix ctrl-list guard-points and concurrency issues

* lock list when editing (prevent concurrent modification of insert
  iterator
* don't add a guard-point if an event is already present between the
  target and guard-point-position
* remove existing automation-events (old guard points) when
  touching automation w/o change
* don't unset "new write pass" when not rolling
  (fixes issues when not rolling but locating with write-enabled)

diff --git a/libs/evoral/evoral/ControlList.hpp b/libs/evoral/evoral/ControlList.hpp
index 8df5004b39eb0b27ad5f9eba5bbd225d36449425..4aececf40dab8085aa7bb8b261d5a0f4a92551da 100644 (file)
--- a/libs/evoral/evoral/ControlList.hpp
+++ b/libs/evoral/evoral/ControlList.hpp
@@ -358,7 +358,7 @@ private:
 
        void unlocked_remove_duplicates ();
        void unlocked_invalidate_insert_iterator ();
-       void add_guard_point (double when);
+       void add_guard_point (double when, double offset);
 };
 
 } // namespace Evoral

diff --git a/libs/evoral/src/ControlList.cpp b/libs/evoral/src/ControlList.cpp
index a0660ee00079aa486ad8bf5dc60344bf67d7767a..0bba1dba11585acb9d81b17b334f665e84571eb2 100644 (file)
--- a/libs/evoral/src/ControlList.cpp
+++ b/libs/evoral/src/ControlList.cpp
@@ -27,6 +27,8 @@
 #define isnan_local std::isnan
 #endif
 
+#define GUARD_POINT_DELTA 64
+
 #include <cassert>
 #include <cmath>
 #include <iostream>
@@ -488,13 +490,37 @@ ControlList::set_in_write_pass (bool yn, bool add_point, double when)
        _in_write_pass = yn;
 
        if (yn && add_point) {
-               add_guard_point (when);
+               Glib::Threads::RWLock::WriterLock lm (_lock);
+               add_guard_point (when, 0);
        }
 }
 
 void
-ControlList::add_guard_point (double when)
+ControlList::add_guard_point (double when, double offset)
 {
+       // caller needs to hold writer-lock
+       if (offset < 0 && when < offset) {
+               return;
+       }
+       assert (offset <= 0);
+
+       if (offset != 0) {
+               /* check if there are points between when + offset .. when */
+               ControlEvent cp (when + offset, 0.0);
+               iterator s;
+               iterator e;
+               if ((s = lower_bound (_events.begin(), _events.end(), &cp, time_comparator)) != _events.end()) {
+                       cp.when = when;
+                       e = lower_bound (_events.begin(), _events.end(), &cp, time_comparator);
+                       if (s != e) {
+                               DEBUG_TRACE (DEBUG::ControlList, string_compose ("@%1 add_guard_point, none added, found event between %2 and %3\n", this, when - offset, when));
+                               return;
+                       }
+               }
+       }
+
+       when += offset;
+
        ControlEvent cp (when, 0.0);
        most_recent_insert_iterator = lower_bound (_events.begin(), _events.end(), &cp, time_comparator);
 
@@ -524,8 +550,7 @@ ControlList::add_guard_point (double when)
                ++most_recent_insert_iterator;
        } else {
 
-               /* insert a new control event at the right spot
-                */
+               /* insert a new control event at the right spot */
 
                DEBUG_TRACE (DEBUG::ControlList, string_compose ("@%1 insert eval-value %2 just before iterator @ %3\n",
                                                                 this, eval_value, (*most_recent_insert_iterator)->when));
@@ -540,9 +565,12 @@ ControlList::add_guard_point (double when)
                ++most_recent_insert_iterator;
        }
 
-       /* don't do this again till the next write pass */
-
-       new_write_pass = false;
+       /* don't do this again till the next write pass,
+        * unless we're not in a write-pass (transport stopped)
+        */
+       if (_in_write_pass) {
+               new_write_pass = false;
+       }
 }
 
 bool
@@ -554,48 +582,49 @@ ControlList::in_write_pass () const
 bool
 ControlList::editor_add (double when, double value, bool with_guard)
 {
-       /* this is for making changes from a graphical line editor
-       */
+       /* this is for making changes from a graphical line editor */
+       {
+               Glib::Threads::RWLock::WriterLock lm (_lock);
 
-       ControlEvent cp (when, 0.0f);
-       iterator i = lower_bound (_events.begin(), _events.end(), &cp, time_comparator);
+               ControlEvent cp (when, 0.0f);
+               iterator i = lower_bound (_events.begin(), _events.end(), &cp, time_comparator);
 
-       if (i != _events.end () && (*i)->when == when) {
-               return false;
-       }
+               if (i != _events.end () && (*i)->when == when) {
+                       return false;
+               }
 
-       /* clamp new value to allowed range */
-       value = std::min ((double)_desc.upper, std::max ((double)_desc.lower, value));
+               /* clamp new value to allowed range */
+               value = std::min ((double)_desc.upper, std::max ((double)_desc.lower, value));
 
-       if (_events.empty()) {
+               if (_events.empty()) {
 
-               /* as long as the point we're adding is not at zero,
-                * add an "anchor" point there.
-                */
+                       /* as long as the point we're adding is not at zero,
+                        * add an "anchor" point there.
+                        */
 
-               if (when >= 1) {
-                       _events.insert (_events.end(), new ControlEvent (0, value));
-                       DEBUG_TRACE (DEBUG::ControlList, string_compose ("@%1 added value %2 at zero\n", this, value));
+                       if (when >= 1) {
+                               _events.insert (_events.end(), new ControlEvent (0, value));
+                               DEBUG_TRACE (DEBUG::ControlList, string_compose ("@%1 added value %2 at zero\n", this, value));
+                       }
                }
-       }
 
-       insert_position = when;
-       if (with_guard) {
-               if (when > 64) {
-                       add_guard_point (when - 64);
+               insert_position = when;
+               if (with_guard) {
+                       add_guard_point (when, -GUARD_POINT_DELTA);
+                       maybe_add_insert_guard (when);
+                       i = lower_bound (_events.begin(), _events.end(), &cp, time_comparator);
                }
-               maybe_add_insert_guard (when);
-       }
 
-       iterator result;
-       DEBUG_TRACE (DEBUG::ControlList, string_compose ("editor_add: actually add when= %1 value= %2\n", when, value));
-       result = _events.insert (i, new ControlEvent (when, value));
+               iterator result;
+               DEBUG_TRACE (DEBUG::ControlList, string_compose ("editor_add: actually add when= %1 value= %2\n", when, value));
+               result = _events.insert (i, new ControlEvent (when, value));
 
-       if (i == result) {
-               return false;
-       }
+               if (i == result) {
+                       return false;
+               }
 
-       mark_dirty ();
+               mark_dirty ();
+       }
        maybe_signal_changed ();
 
        return true;
@@ -604,17 +633,18 @@ ControlList::editor_add (double when, double value, bool with_guard)
 void
 ControlList::maybe_add_insert_guard (double when)
 {
+       // caller needs to hold writer-lock
        if (most_recent_insert_iterator != _events.end()) {
-               if ((*most_recent_insert_iterator)->when - when > 64) {
+               if ((*most_recent_insert_iterator)->when - when > GUARD_POINT_DELTA) {
                        /* Next control point is some distance from where our new point is
                           going to go, so add a new point to avoid changing the shape of
                           the line too much.  The insert iterator needs to point to the
                           new control point so that our insert will happen correctly. */
-                       most_recent_insert_iterator = _events.insert (
-                               most_recent_insert_iterator,
-                               new ControlEvent (when + 64, (*most_recent_insert_iterator)->value));
+                       most_recent_insert_iterator = _events.insert ( most_recent_insert_iterator,
+                                       new ControlEvent (when + GUARD_POINT_DELTA, (*most_recent_insert_iterator)->value));
+
                        DEBUG_TRACE (DEBUG::ControlList, string_compose ("@%1 added insert guard point @ %2 = %3\n",
-                                                                        this, when + 64,
+                                                                        this, when + GUARD_POINT_DELTA,
                                                                         (*most_recent_insert_iterator)->value));
                }
        }
@@ -624,6 +654,7 @@ ControlList::maybe_add_insert_guard (double when)
 bool
 ControlList::maybe_insert_straight_line (double when, double value)
 {
+       // caller needs to hold writer-lock
        if (_events.empty()) {
                return false;
        }
@@ -652,6 +683,7 @@ ControlList::maybe_insert_straight_line (double when, double value)
 ControlList::iterator
 ControlList::erase_from_iterator_to (iterator iter, double when)
 {
+       // caller needs to hold writer-lock
        while (iter != _events.end()) {
                if ((*iter)->when < when) {
                        DEBUG_TRACE (DEBUG::ControlList, string_compose ("@%1 erase existing @ %2\n", this, (*iter)->when));
@@ -706,7 +738,7 @@ ControlList::add (double when, double value, bool with_guards, bool with_initial
                        /* first write in a write pass: add guard point if requested */
 
                        if (with_guards) {
-                               add_guard_point (insert_position);
+                               add_guard_point (insert_position, 0);
                                did_write_during_pass = true;
                        } else {
                                /* not adding a guard, but we need to set iterator appropriately */
@@ -725,9 +757,11 @@ ControlList::add (double when, double value, bool with_guards, bool with_initial
                                ++most_recent_insert_iterator;
                        }
 
-                       most_recent_insert_iterator = erase_from_iterator_to(most_recent_insert_iterator, when);
                        if (with_guards) {
+                               most_recent_insert_iterator = erase_from_iterator_to (most_recent_insert_iterator, when + GUARD_POINT_DELTA);
                                maybe_add_insert_guard (when);
+                       } else {
+                               most_recent_insert_iterator = erase_from_iterator_to(most_recent_insert_iterator, when);
                        }
 
                } else if (!_in_write_pass) {
@@ -793,20 +827,19 @@ ControlList::add (double when, double value, bool with_guards, bool with_initial
                                }
 
                                if (have_point1 && have_point2) {
+                                       DEBUG_TRACE (DEBUG::ControlList, string_compose ("@%1 no change: move existing at %3 to %2\n", this, when, (*most_recent_insert_iterator)->when));
                                        (*most_recent_insert_iterator)->when = when;
                                        done = true;
                                } else {
                                        ++most_recent_insert_iterator;
                                }
                        }
-                       //done = maybe_insert_straight_line (when, value) || done;
-                       /* if the transport is stopped, add guard points (?) */
-                       if (!done && !_in_write_pass && when > 64) {
-                               add_guard_point (when - 64);
-                               maybe_add_insert_guard (when);
-                       }
 
-                       if (with_guards) {
+                       /* if the transport is stopped, add guard points */
+                       if (!done && !_in_write_pass) {
+                               add_guard_point (when, -GUARD_POINT_DELTA);
+                               maybe_add_insert_guard (when);
+                       } else if (with_guards) {
                                maybe_add_insert_guard (when);
                        }