spline: fix deadlock, logic error

Tracking rarely deadlocked when saving mappings.

Investigating it further also shown how a wrong bundle was used for
Accela's splines.

1 files changed, 155 insertions, 140 deletions

diff --git a/spline/spline.cpp b/spline/spline.cpp
index 50812bad..3ec3b5dc 100644
--- a/spline/spline.cpp
+++ b/spline/spline.cpp
@@ -1,11 +1,3 @@
-/* Copyright (c) 2012-2016, Stanislaw Halik <sthalik@misaki.pl>
-
- * Permission to use, copy, modify, and/or distribute this
- * software for any purpose with or without fee is hereby granted,
- * provided that the above copyright notice and this permission
- * notice appear in all copies.
- */
-
 #include "spline.hpp"
 #include "compat/math.hpp"
 
@@ -18,7 +10,6 @@
 
 #include <QObject>
 #include <QMutexLocker>
-#include <QCoreApplication>
 #include <QPointF>
 #include <QSettings>
 #include <QString>
@@ -27,6 +18,7 @@
 
 namespace spline_detail {
 
+settings::~settings() = default;
 base_spline_::~base_spline_() = default;
 base_spline::~base_spline() = default;
 spline_modify_mixin::~spline_modify_mixin() = default;
@@ -40,7 +32,6 @@ spline::spline(const QString& name, const QString& axis_name, Axis axis)
 spline::~spline()
 {
     QMutexLocker l(&mtx);
-
     disconnect_signals();
 }
 
@@ -48,46 +39,53 @@ spline::spline() : spline(QString{}, QString{}, Axis(-1)) {}
 
 void spline::set_tracking_active(bool value)
 {
-    QMutexLocker l(&mtx);
-    activep = value;
+    std::shared_ptr<settings> S;
+    {
+        QMutexLocker l(&mtx);
+        S = s;
+        activep = value;
+    }
+    emit S->recomputed();
 }
 
 bundle spline::get_bundle()
 {
-    QMutexLocker l(&mtx); // avoid logic errors due to changes in value<t> data
+    QMutexLocker l(&mtx);
     return s->b;
 }
 
 void spline::clear()
 {
+    std::shared_ptr<settings> S;
     {
         QMutexLocker l(&mtx);
+        S = s;
         s->points = {};
+        points = {};
+        update_interp_data();
     }
-
-    invalidate_settings();
+    emit S->recomputed();
 }
 
-float spline::get_value(double x)
+double spline::get_value(double x) const
 {
-    QMutexLocker foo(&mtx);
+    QMutexLocker l(&mtx);
 
-    const float ret = get_value_no_save(x);
-    last_input_value.setX(std::fabs(x));
-    last_input_value.setY(double(std::fabs(ret)));
+    const double ret = get_value_no_save(x);
+    last_input_value = { std::fabs(x), std::fabs((double)ret) };
     return ret;
 }
 
-float spline::get_value_no_save(double x) const
+double spline::get_value_no_save(double x) const
 {
-    QMutexLocker foo(&mtx);
+    QMutexLocker l(&mtx);
 
-    float  q  = float(x * bucket_size_coefficient(points));
+    double q  = x * bucket_size_coefficient(points);
     int    xi = (int)q;
-    float  yi = get_value_internal(xi);
-    float  yiplus1 = get_value_internal(xi+1);
-    float  f = (q-xi);
-    float  ret = yiplus1 * f + yi * (1.0f - f); // at least do a linear interpolation.
+    double yi = get_value_internal(xi);
+    double yiplus1 = get_value_internal(xi+1);
+    double f = (q-xi);
+    double ret = yiplus1 * f + yi * (1 - f); // at least do a linear interpolation.
     return ret;
 }
 
@@ -95,43 +93,47 @@ bool spline::get_last_value(QPointF& point)
 {
     QMutexLocker foo(&mtx);
     point = last_input_value;
-    return activep;
+    return activep && point.y() >= 0;
 }
 
-float spline::get_value_internal(int x) const
+double spline::get_value_internal(int x) const
 {
-    if (!validp)
-    {
-        update_interp_data();
-        validp = true;
-    }
-
     const float sign = signum(x);
     x = std::abs(x);
-    const float ret_ = data[std::min(unsigned(x), unsigned(value_count)-1u)];
+    const float ret_ = data[std::min(unsigned(x), value_count - 1)];
     return sign * clamp(ret_, 0, 1000);
 }
 
-QPointF spline::ensure_in_bounds(const QList<QPointF>& points, int i)
+void spline::ensure_in_bounds(const QList<QPointF>& points, int i, f& x, f& y)
 {
     const int sz = points.size();
 
     if (i < 0 || sz == 0)
-        return {};
-
-    if (i < sz)
-        return points[i];
-
-    return points[sz - 1];
+    {
+        x = 0;
+        y = 0;
+    }
+    else if (i < sz)
+    {
+        const QPointF& pt = points[i];
+        x = (f)pt.x();
+        y = (f)pt.y();
+    }
+    else
+    {
+        const QPointF& pt = points[sz - 1];
+        x = (f)pt.x();
+        y = (f)pt.y();
+    }
 }
 
 int spline::element_count(const QList<QPointF>& points, double max_input)
 {
     const unsigned sz = (unsigned)points.size();
-    for (unsigned k = 0; k < sz; k++)
+    for (int k = sz-1; k >= 0; k--)
     {
         const QPointF& pt = points[k];
-        if (max_input > 1e-4 && pt.x() - 1e-2 > max_input)
+        if (max_input > 1e-4 && pt.x() - 1e-4 >= max_input)
             return k;
     }
     return sz;
@@ -148,15 +150,11 @@ void spline::update_interp_data() const
     ensure_valid(list);
     const int sz = list.size();
 
-    const double maxx = max_input();
-
     if (list.isEmpty())
-        list.prepend({ maxx, max_output() });
+        list.prepend({ max_input(), max_output() });
 
     const double c = bucket_size_coefficient(list);
-    const double c_interp = c * 30;
-
-    enum { magic_fill_value = -255 };
+    const double c_ = c * c_interp;
 
     for (unsigned i = 0; i < value_count; i++)
         data[i] = magic_fill_value;
@@ -166,35 +164,35 @@ void spline::update_interp_data() const
         const QPointF& pt = list[0];
         const double x = pt.x();
         const double y = pt.y();
-        const unsigned max = clamp(uround(x * c), 0, value_count-2);
+        const unsigned max = clamp(uround(x * c), 0, value_count-1);
 
         for (unsigned k = 0; k <= max; k++)
             data[k] = float(y * k / max); // no need for bresenham
     }
     else
     {
-        if (list[0].x() > 1e-2 && list[0].x() <= maxx)
-            list.push_front(QPointF(0, 0));
+        if (list[0].x() > 1e-2)
+            list.push_front({});
 
         // now this is hella expensive due to `c_interp'
         for (int i = 0; i < sz; i++)
         {
-            const QPointF p0 = ensure_in_bounds(list, i - 1);
-            const QPointF p1 = ensure_in_bounds(list, i + 0);
-            const QPointF p2 = ensure_in_bounds(list, i + 1);
-            const QPointF p3 = ensure_in_bounds(list, i + 2);
-            const double p0_x = p0.x(), p1_x = p1.x(), p2_x = p2.x(), p3_x = p3.x();
-            const double p0_y = p0.y(), p1_y = p1.y(), p2_y = p2.y(), p3_y = p3.y();
-
-            const double cx[4] = {
+            f p0_x, p1_x, p2_x, p3_x;
+            f p0_y, p1_y, p2_y, p3_y;
+
+            ensure_in_bounds(list, i - 1, p0_x, p0_y);
+            ensure_in_bounds(list, i + 0, p1_x, p1_y);
+            ensure_in_bounds(list, i + 1, p2_x, p2_y);
+            ensure_in_bounds(list, i + 2, p3_x, p3_y);
+
+            const f cx[4] = {
                 2 * p1_x, // 1
                 -p0_x + p2_x, // t
                 2 * p0_x - 5 * p1_x + 4 * p2_x - p3_x, // t^2
                 -p0_x + 3 * p1_x - 3 * p2_x + p3_x, // t3
             };
 
-            const double cy[4] =
-            {
+            const f cy[4] = {
                 2 * p1_y, // 1
                 -p0_y + p2_y, // t
                 2 * p0_y - 5 * p1_y + 4 * p2_y - p3_y, // t^2
@@ -202,36 +200,37 @@ void spline::update_interp_data() const
             };
 
             // multiplier helps fill in all the x's needed
-            const unsigned end{int(c_interp * (p2_x - p1_x)) + 1u};
+            const unsigned end = (unsigned)(c_ * (p2_x - p1_x)) + 1;
+            const f end_(end);
 
             for (unsigned k = 0; k <= end; k++)
             {
-                const double t = k / double(end);
-                const double t2 = t*t;
-                const double t3 = t*t*t;
+                const f t = k / end_;
+                const f t2 = t*t;
+                const f t3 = t*t*t;
 
-                const int x = int(.5 * c * (cx[0] + cx[1] * t + cx[2] * t2 + cx[3] * t3));
-                const float y = float(.5 * (cy[0] + cy[1] * t + cy[2] * t2 + cy[3] * t3));
+                const unsigned x = unsigned(f(.5) * c * (cx[0] + cx[1] * t + cx[2] * t2 + cx[3] * t3));
+                const float y = (float)(f(.5) * (cy[0] + cy[1] * t + cy[2] * t2 + cy[3] * t3));
 
-                if (unsigned(x) < value_count)
+                if (x < value_count)
                     data[x] = y;
             }
         }
     }
 
-    double maxy = max_output();
+    float maxy = (float)max_output();
     float last = 0;
 
 #ifdef __clang__
 #   pragma clang diagnostic push
-#   pragma clang diagnostic ignored "-Wfloat-equal"
+#   pragma clang diagnostic ignored "-Wfloat-equal" // stupid clang
 #endif
 
-    for (unsigned i = 0; i < unsigned(value_count); i++)
+    for (unsigned i = 0; i < value_count; i++)
     {
         if (data[i] == magic_fill_value)
             data[i] = last;
-        data[i] = clamp(data[i], 0, (float)maxy);
+        data[i] = clamp(data[i], 0, maxy);
         last = data[i];
     }
 
@@ -242,58 +241,72 @@ void spline::update_interp_data() const
 
 void spline::remove_point(int i)
 {
-    QMutexLocker foo(&mtx);
+    std::shared_ptr<settings> S;
+    {
+        QMutexLocker foo(&mtx);
+        S = s;
 
-    const int sz = element_count(points, max_input());
+        const int sz = element_count(points, max_input());
 
-    if (i >= 0 && i < sz)
-    {
-        points.erase(points.begin() + i);
-        s->points = points;
-        validp = false;
+        if (i >= 0 && i < sz)
+        {
+            points.erase(points.begin() + i);
+            s->points = points;
+            update_interp_data();
+        }
     }
+
+    emit S->recomputed();
 }
 
 void spline::add_point(QPointF pt)
 {
-    QMutexLocker foo(&mtx);
-
-    points.push_back(pt);
-    std::stable_sort(points.begin(), points.end(), sort_fn);
-    s->points = points;
-    validp = false;
+    std::shared_ptr<settings> S;
+    {
+        QMutexLocker foo(&mtx);
+        S = s;
+        points.push_back(pt);
+        std::stable_sort(points.begin(), points.end(), sort_fn);
+        s->points = points;
+        update_interp_data();
+    }
+    emit S->recomputed();
 }
 
 void spline::add_point(double x, double y)
 {
-    add_point(QPointF(x, y));
+    add_point({ x, y });
 }
 
 void spline::move_point(int idx, QPointF pt)
 {
-    QMutexLocker foo(&mtx);
+    std::shared_ptr<settings> S;
+    {
+        QMutexLocker foo(&mtx);
+        S = s;
 
-    const int sz = element_count(points, max_input());
+        const int sz = element_count(points, max_input());
 
-    if (idx >= 0 && idx < sz)
-    {
-        points[idx] = pt;
-        std::stable_sort(points.begin(), points.end(), sort_fn);
-        s->points = points;
-        validp = false;
+        if (idx >= 0 && idx < sz)
+        {
+            points[idx] = pt;
+            std::stable_sort(points.begin(), points.end(), sort_fn);
+            s->points = points;
+            update_interp_data();
+        }
     }
+    emit S->recomputed();
 }
 
-const base_spline_::points_t& spline::get_points() const
+const points_t& spline::get_points() const
 {
-    QMutexLocker foo(&mtx);
     return points;
 }
 
 int spline::get_point_count() const
 {
     QMutexLocker foo(&mtx);
-    return element_count(points, s->opts.clamp_x_);
+    return element_count(points, clamp_x);
 }
 
 void spline::reload()
@@ -308,66 +321,69 @@ void spline::save()
     s->b->save();
 }
 
+void spline::invalidate_settings_()
+{
+    points = s->points;
+    clamp_x = s->opts.clamp_x_;
+    clamp_y = s->opts.clamp_y_;
+    update_interp_data();
+}
+
 void spline::invalidate_settings()
 {
+    std::shared_ptr<settings> S;
     {
         QMutexLocker l(&mtx);
-        validp = false;
-        points = s->points;
+        S = s;
+        invalidate_settings_();
     }
-    emit s->recomputed();
+    emit S->recomputed();
 }
 
 void spline::set_bundle(bundle b, const QString& axis_name, Axis axis)
 {
-    QMutexLocker l(&mtx);
+    if (!b)
+        b = make_bundle({});
+
+    std::shared_ptr<settings> S;
 
-    // gets called from ctor hence the need for nullptr checks
-    // the sentinel settings/bundle objects don't need any further branching once created
-    if (!s || s->b != b)
     {
-        disconnect_signals();
+        QMutexLocker l(&mtx);
 
-        if (!b)
-            b = make_bundle(QString{});
+        disconnect_signals();
         s = std::make_shared<settings>(b, axis_name, axis);
-
-        conn_changed = QObject::connect(b.get(), &bundle_::changed,
-                                        s.get(), [&] { invalidate_settings(); }, Qt::QueuedConnection);
-        // this isn't strictly necessary for the spline but helps the widget
-        conn_maxx = QObject::connect(&s->opts.clamp_x_, value_::value_changed<int>(),
-                                     ctx.get(), [&](double) { invalidate_settings(); }, Qt::QueuedConnection);
-        conn_maxy = QObject::connect(&s->opts.clamp_y_, value_::value_changed<int>(),
-                                     ctx.get(), [&](double) { invalidate_settings(); }, Qt::QueuedConnection);
-
-        invalidate_settings();
+        invalidate_settings_();
+        S = s;
+
+        conn_points = QObject::connect(&s->points, value_::value_changed<QList<QPointF>>(),
+                                       ctx.get(), [this] { invalidate_settings(); }, Qt::DirectConnection);
+        conn_maxx   = QObject::connect(&s->opts.clamp_x_, value_::value_changed<int>(),
+                                       ctx.get(), [this](double) { invalidate_settings(); }, Qt::DirectConnection);
+        conn_maxy   = QObject::connect(&s->opts.clamp_y_, value_::value_changed<int>(),
+                                       ctx.get(), [this](double) { invalidate_settings(); }, Qt::DirectConnection);
     }
+
+    emit S->recomputed();
 }
 
 double spline::max_input() const
 {
     QMutexLocker l(&mtx);
-    using m = axis_opts::max_clamp;
-    const value<m>& clamp = s->opts.clamp_x_;
-    if (clamp == m::x1000 && !points.empty())
-        return points[points.size() - 1].x();
-    return std::fabs(clamp.to<double>());
+    if (clamp_x == axis_opts::x1000)
+        return std::fmax(1, points.empty() ? 0 : points[points.size() - 1].x());
+    return std::fabs((double)clamp_x);
 }
 
 double spline::max_output() const
 {
     QMutexLocker l(&mtx);
-    using m = axis_opts::max_clamp;
-    const value<m>& clamp = s->opts.clamp_y_;
-    if (clamp == m::x1000 && !points.empty())
-        return points[points.size() - 1].y();
-    return std::fabs(clamp.to<double>());
+    if (clamp_y == axis_opts::x1000 && !points.empty())
+        return std::fmax(1, points.empty() ? 0 : points[points.size() - 1].y());
+    return std::fabs((double)clamp_y);
 }
 
 void spline::ensure_valid(points_t& list) const
 {
-    QMutexLocker foo(&mtx);
-
     std::stable_sort(list.begin(), list.end(), sort_fn);
 
     const unsigned sz = (unsigned)list.size();
@@ -438,14 +454,15 @@ std::shared_ptr<const base_settings> spline::get_settings() const
 double spline::bucket_size_coefficient(const QList<QPointF>& points) const
 {
     constexpr double eps = 1e-4;
-
     const double maxx = max_input();
 
     if (maxx < eps)
         return 0;
 
-    // needed to fill the buckets up to the last control point.
-    // space between that point and max_x doesn't matter.
+    // buckets are only used between 0 and the rightmost point's
+    // x coordinate. even if `clamp_x' is set to a much larger value,
+    // buckets retain more precision lower the x coordinate of the
+    // last point.
 
     const int sz = element_count(points, maxx);
     const double last_x = sz ? points[sz - 1].x() : maxx;
@@ -455,19 +472,17 @@ double spline::bucket_size_coefficient(const QList<QPointF>& points) const
 
 void spline::disconnect_signals()
 {
-    if (conn_changed)
+    if (conn_points)
     {
-        QObject::disconnect(conn_changed); conn_changed = {};
+        QObject::disconnect(conn_points); conn_points = {};
         QObject::disconnect(conn_maxx); conn_maxx = {};
         QObject::disconnect(conn_maxy); conn_maxy = {};
     }
 }
 
 settings::settings(bundle const& b, const QString& axis_name, Axis idx):
-    b(b ? b : make_bundle("")),
+    b(b ? b : make_bundle({})),
     opts(axis_name, idx)
 {}
 
-settings::~settings() = default;
-
 } // ns spline_detail