# HG changeset patch
# User Chris Cannam
# Date 1569925368 -3600
# Node ID 13bd41bd8a175370c7e1c48670fc1d7c3cabbfe7
# Parent  e8b552549225a6b0178c53853cea14141dc81958
Some work on making Model classes thread-safe in typical use - and documenting this. Some of the implementations are simpler now that EventSeries is thread-safe

diff -r e8b552549225 -r 13bd41bd8a17 base/EventSeries.cpp
--- a/base/EventSeries.cpp	Tue Oct 01 11:21:56 2019 +0100
+++ b/base/EventSeries.cpp	Tue Oct 01 11:22:48 2019 +0100
@@ -544,8 +544,7 @@
 EventSeries::getEventByIndex(int index) const
 {
     QMutexLocker locker(&m_mutex);
-
-    if (index < 0 || index >= count()) {
+    if (!in_range_for(m_events, index)) {
         throw std::logic_error("index out of range");
     }
     return m_events[index];
@@ -555,7 +554,6 @@
 EventSeries::getIndexForEvent(const Event &e) const
 {
     QMutexLocker locker(&m_mutex);
-
     auto pitr = lower_bound(m_events.begin(), m_events.end(), e);
     auto d = distance(m_events.begin(), pitr);
     if (d < 0 || d > INT_MAX) return 0;
diff -r e8b552549225 -r 13bd41bd8a17 data/model/ImageModel.h
--- a/data/model/ImageModel.h	Tue Oct 01 11:21:56 2019 +0100
+++ b/data/model/ImageModel.h	Tue Oct 01 11:22:48 2019 +0100
@@ -76,10 +76,8 @@
 
     void setCompletion(int completion, bool update = true) {
         
-        {   QMutexLocker locker(&m_mutex);
-            if (m_completion == completion) return;
-            m_completion = completion;
-        }
+        if (m_completion == completion) return;
+        m_completion = completion;
 
         if (update) {
             m_notifier.makeDeferredNotifications();
@@ -141,18 +139,12 @@
      * Editing methods.
      */
     void add(Event e) override {
-
-        {   QMutexLocker locker(&m_mutex);
-            m_events.add(e.withoutDuration().withoutValue().withoutLevel());
-        }
-        
+        m_events.add(e.withoutDuration().withoutValue().withoutLevel());
         m_notifier.update(e.getFrame(), m_resolution);
     }
     
     void remove(Event e) override {
-        {   QMutexLocker locker(&m_mutex);
-            m_events.remove(e);
-        }
+        m_events.remove(e);
         emit modelChangedWithin(getId(),
                                 e.getFrame(), e.getFrame() + m_resolution);
     }
@@ -281,11 +273,9 @@
     int m_resolution;
 
     DeferredNotifier m_notifier;
-    int m_completion;
+    std::atomic<int> m_completion;
 
     EventSeries m_events;
-
-    mutable QMutex m_mutex;  
 };
 
 
diff -r e8b552549225 -r 13bd41bd8a17 data/model/Model.cpp
--- a/data/model/Model.cpp	Tue Oct 01 11:21:56 2019 +0100
+++ b/data/model/Model.cpp	Tue Oct 01 11:22:48 2019 +0100
@@ -30,6 +30,8 @@
 void
 Model::setSourceModel(ModelId modelId)
 {
+    QMutexLocker locker(&m_mutex);
+    
     m_sourceModel = modelId;
 
     auto model = ModelById::get(m_sourceModel);
@@ -42,6 +44,8 @@
 void
 Model::setAlignment(ModelId alignmentModel)
 {
+    QMutexLocker locker(&m_mutex);
+
     SVDEBUG << "Model(" << this << "): accepting alignment model "
             << alignmentModel << endl;
 
@@ -67,12 +71,14 @@
 const ModelId
 Model::getAlignment() const
 {
+    QMutexLocker locker(&m_mutex);
     return m_alignmentModel;
 }
 
 const ModelId
 Model::getAlignmentReference() const
 {
+    QMutexLocker locker(&m_mutex);
     auto model = ModelById::getAs<AlignmentModel>(m_alignmentModel);
     if (model) return model->getReferenceModel();
     else return {};
@@ -81,10 +87,17 @@
 sv_frame_t
 Model::alignToReference(sv_frame_t frame) const
 {
-    auto alignmentModel = ModelById::getAs<AlignmentModel>(m_alignmentModel);
+    ModelId alignmentModelId, sourceModelId;
+    {
+        QMutexLocker locker(&m_mutex);
+        alignmentModelId = m_alignmentModel;
+        sourceModelId = m_sourceModel;
+    }
+    
+    auto alignmentModel = ModelById::getAs<AlignmentModel>(alignmentModelId);
     
     if (!alignmentModel) {
-        auto sourceModel = ModelById::get(m_sourceModel);
+        auto sourceModel = ModelById::get(sourceModelId);
         if (sourceModel) {
             return sourceModel->alignToReference(frame);
         }
@@ -102,10 +115,17 @@
 sv_frame_t
 Model::alignFromReference(sv_frame_t refFrame) const
 {
-    auto alignmentModel = ModelById::getAs<AlignmentModel>(m_alignmentModel);
-    
+    ModelId alignmentModelId, sourceModelId;
+    {
+        QMutexLocker locker(&m_mutex);
+        alignmentModelId = m_alignmentModel;
+        sourceModelId = m_sourceModel;
+    }
+
+    auto alignmentModel = ModelById::getAs<AlignmentModel>(alignmentModelId);
+   
     if (!alignmentModel) {
-        auto sourceModel = ModelById::get(m_sourceModel);
+        auto sourceModel = ModelById::get(sourceModelId);
         if (sourceModel) {
             return sourceModel->alignFromReference(refFrame);
         }
@@ -120,16 +140,23 @@
 int
 Model::getAlignmentCompletion() const
 {
+    ModelId alignmentModelId, sourceModelId;
+    {
+        QMutexLocker locker(&m_mutex);
+        alignmentModelId = m_alignmentModel;
+        sourceModelId = m_sourceModel;
+    }
+
 #ifdef DEBUG_COMPLETION
     SVCERR << "Model(" << this
            << ")::getAlignmentCompletion: m_alignmentModel = "
            << m_alignmentModel << endl;
 #endif
 
-    auto alignmentModel = ModelById::getAs<AlignmentModel>(m_alignmentModel);
+    auto alignmentModel = ModelById::getAs<AlignmentModel>(alignmentModelId);
     
     if (!alignmentModel) {
-        auto sourceModel = ModelById::get(m_sourceModel);
+        auto sourceModel = ModelById::get(sourceModelId);
         if (sourceModel) {
             return sourceModel->getAlignmentCompletion();
         }
@@ -149,6 +176,7 @@
 QString
 Model::getTitle() const
 {
+    QMutexLocker locker(&m_mutex);
     auto source = ModelById::get(m_sourceModel);
     if (source) return source->getTitle();
     else return "";
@@ -157,6 +185,7 @@
 QString
 Model::getMaker() const
 {
+    QMutexLocker locker(&m_mutex);
     auto source = ModelById::get(m_sourceModel);
     if (source) return source->getMaker();
     else return "";
@@ -165,6 +194,7 @@
 QString
 Model::getLocation() const
 {
+    QMutexLocker locker(&m_mutex);
     auto source = ModelById::get(m_sourceModel);
     if (source) return source->getLocation();
     else return "";
diff -r e8b552549225 -r 13bd41bd8a17 data/model/Model.h
--- a/data/model/Model.h	Tue Oct 01 11:21:56 2019 +0100
+++ b/data/model/Model.h	Tue Oct 01 11:22:48 2019 +0100
@@ -17,7 +17,10 @@
 #define SV_MODEL_H
 
 #include <vector>
+#include <atomic>
+
 #include <QObject>
+#include <QMutex>
 
 #include "base/ById.h"
 #include "base/XmlExportable.h"
@@ -31,6 +34,19 @@
 /** 
  * Model is the base class for all data models that represent any sort
  * of data on a time scale based on an audio frame rate.
+ *
+ * Model classes are expected to be thread-safe, particularly with
+ * regard to content rather than metadata (e.g. populating a model
+ * from a transform running in a background thread while displaying it
+ * in a UI layer).
+ *
+ * Never store a pointer to a model unless it is completely private to
+ * the code owning it. Models should be referred to using their
+ * ModelId id and looked up from the ById pool when needed. This
+ * returns a shared pointer, which ensures a sufficient lifespan for a
+ * transient operation locally. See notes in ById.h. The document
+ * unregisters models when it is closed, and then they are deleted
+ * when the last shared pointer instance expires.
  */
 class Model : public QObject,
               public WithTypedId<Model>,
@@ -301,17 +317,17 @@
     void alignmentModelCompletionChanged(ModelId);
     
 protected:
-    Model() :
-        m_extendTo(0) { }
+    Model() : m_extendTo(0) { }
 
     // Not provided.
     Model(const Model &) =delete;
     Model &operator=(const Model &) =delete;
 
+    mutable QMutex m_mutex;
     ModelId m_sourceModel;
     ModelId m_alignmentModel;
     QString m_typeUri;
-    sv_frame_t m_extendTo;
+    std::atomic<sv_frame_t> m_extendTo;
 };
 
 typedef Model::Id ModelId;
diff -r e8b552549225 -r 13bd41bd8a17 data/model/NoteModel.h
--- a/data/model/NoteModel.h	Tue Oct 01 11:21:56 2019 +0100
+++ b/data/model/NoteModel.h	Tue Oct 01 11:22:48 2019 +0100
@@ -28,7 +28,6 @@
 #include "base/Pitch.h"
 #include "system/System.h"
 
-#include <QMutex>
 #include <QMutexLocker>
 
 class NoteModel : public Model,
@@ -56,7 +55,6 @@
         m_haveExtents(false),
         m_valueQuantization(0),
         m_units(""),
-        m_extendTo(0),
         m_notifier(this,
                    getId(),
                    notifyOnAdd ?
@@ -83,7 +81,6 @@
         m_haveExtents(true),
         m_valueQuantization(0),
         m_units(""),
-        m_extendTo(0),
         m_notifier(this,
                    getId(),
                    notifyOnAdd ?
@@ -105,9 +102,11 @@
     bool isOK() const override { return true; }
     
     sv_frame_t getStartFrame() const override {
+        QMutexLocker locker(&m_mutex);
         return m_events.getStartFrame();
     }
     sv_frame_t getTrueEndFrame() const override {
+        QMutexLocker locker(&m_mutex);
         if (m_events.isEmpty()) return 0;
         sv_frame_t e = m_events.getEndFrame();
         if (e % m_resolution == 0) return e;
@@ -122,8 +121,12 @@
         return "elecpiano";
     }
 
-    QString getScaleUnits() const { return m_units; }
+    QString getScaleUnits() const {
+        QMutexLocker locker(&m_mutex);
+        return m_units;
+    }
     void setScaleUnits(QString units) {
+        QMutexLocker locker(&m_mutex);
         m_units = units;
         UnitDatabase::getInstance()->registerUnit(units);
     }
@@ -138,7 +141,7 @@
 
     void setCompletion(int completion, bool update = true) {
 
-        {   QMutexLocker locker(&m_mutex);
+        {
             if (m_completion == completion) return;
             m_completion = completion;
         }
@@ -202,20 +205,16 @@
 
         bool allChange = false;
            
-        {
-            QMutexLocker locker(&m_mutex);
-            m_events.add(e);
-
-            float v = e.getValue();
-            if (!ISNAN(v) && !ISINF(v)) {
-                if (!m_haveExtents || v < m_valueMinimum) {
-                    m_valueMinimum = v; allChange = true;
-                }
-                if (!m_haveExtents || v > m_valueMaximum) {
-                    m_valueMaximum = v; allChange = true;
-                }
-                m_haveExtents = true;
+        m_events.add(e);
+        float v = e.getValue();
+        if (!ISNAN(v) && !ISINF(v)) {
+            if (!m_haveExtents || v < m_valueMinimum) {
+                m_valueMinimum = v; allChange = true;
             }
+            if (!m_haveExtents || v > m_valueMaximum) {
+                m_valueMaximum = v; allChange = true;
+            }
+            m_haveExtents = true;
         }
         
         m_notifier.update(e.getFrame(), e.getDuration() + m_resolution);
@@ -226,10 +225,7 @@
     }
     
     void remove(Event e) override {
-        {
-            QMutexLocker locker(&m_mutex);
-            m_events.remove(e);
-        }
+        m_events.remove(e);
         emit modelChangedWithin(getId(),
                                 e.getFrame(),
                                 e.getFrame() + e.getDuration() + m_resolution);
@@ -408,21 +404,15 @@
     sv_samplerate_t m_sampleRate;
     int m_resolution;
 
-    float m_valueMinimum;
-    float m_valueMaximum;
-    bool m_haveExtents;
+    std::atomic<float> m_valueMinimum;
+    std::atomic<float> m_valueMaximum;
+    std::atomic<bool> m_haveExtents;
     float m_valueQuantization;
     QString m_units;
-    sv_frame_t m_extendTo;
     DeferredNotifier m_notifier;
-    int m_completion;
+    std::atomic<int> m_completion;
 
     EventSeries m_events;
-
-    mutable QMutex m_mutex;
-
-    //!!! do we have general docs for ownership and synchronisation of models?
-    // this might be a good opportunity to stop using bare pointers to them
 };
 
 #endif
diff -r e8b552549225 -r 13bd41bd8a17 data/model/RegionModel.h
--- a/data/model/RegionModel.h	Tue Oct 01 11:21:56 2019 +0100
+++ b/data/model/RegionModel.h	Tue Oct 01 11:22:48 2019 +0100
@@ -26,8 +26,6 @@
 
 #include "system/System.h"
 
-#include <QMutex>
-
 /**
  * RegionModel -- a model for intervals associated with a value, which
  * we call regions for no very compelling reason.
@@ -113,8 +111,7 @@
 
     void setCompletion(int completion, bool update = true) {
 
-        {   QMutexLocker locker(&m_mutex);
-            if (m_completion == completion) return;
+        {   if (m_completion == completion) return;
             m_completion = completion;
         }
 
@@ -176,24 +173,21 @@
 
         bool allChange = false;
            
-        {
-            QMutexLocker locker(&m_mutex);
-            m_events.add(e);
-
-            float v = e.getValue();
-            if (!ISNAN(v) && !ISINF(v)) {
-                if (!m_haveExtents || v < m_valueMinimum) {
-                    m_valueMinimum = v; allChange = true;
-                }
-                if (!m_haveExtents || v > m_valueMaximum) {
-                    m_valueMaximum = v; allChange = true;
-                }
-                m_haveExtents = true;
+        m_events.add(e);
+        
+        float v = e.getValue();
+        if (!ISNAN(v) && !ISINF(v)) {
+            if (!m_haveExtents || v < m_valueMinimum) {
+                m_valueMinimum = v; allChange = true;
             }
-
-            if (e.hasValue() && e.getValue() != 0.f) {
-                m_haveDistinctValues = true;
+            if (!m_haveExtents || v > m_valueMaximum) {
+                m_valueMaximum = v; allChange = true;
             }
+            m_haveExtents = true;
+        }
+        
+        if (e.hasValue() && e.getValue() != 0.f) {
+            m_haveDistinctValues = true;
         }
         
         m_notifier.update(e.getFrame(), e.getDuration() + m_resolution);
@@ -204,10 +198,7 @@
     }
     
     void remove(Event e) override {
-        {
-            QMutexLocker locker(&m_mutex);
-            m_events.remove(e);
-        }
+        m_events.remove(e);
         emit modelChangedWithin(getId(),
                                 e.getFrame(),
                                 e.getFrame() + e.getDuration() + m_resolution);
@@ -347,18 +338,16 @@
     sv_samplerate_t m_sampleRate;
     int m_resolution;
 
-    float m_valueMinimum;
-    float m_valueMaximum;
-    bool m_haveExtents;
+    std::atomic<float> m_valueMinimum;
+    std::atomic<float> m_valueMaximum;
+    std::atomic<bool> m_haveExtents;
     float m_valueQuantization;
     bool m_haveDistinctValues;
     QString m_units;
     DeferredNotifier m_notifier;
-    int m_completion;
+    std::atomic<int> m_completion;
 
     EventSeries m_events;
-
-    mutable QMutex m_mutex;
 };
 
 #endif
diff -r e8b552549225 -r 13bd41bd8a17 data/model/SparseOneDimensionalModel.h
--- a/data/model/SparseOneDimensionalModel.h	Tue Oct 01 11:21:56 2019 +0100
+++ b/data/model/SparseOneDimensionalModel.h	Tue Oct 01 11:22:48 2019 +0100
@@ -89,10 +89,8 @@
 
     void setCompletion(int completion, bool update = true) {
         
-        {   QMutexLocker locker(&m_mutex);
-            if (m_completion == completion) return;
-            m_completion = completion;
-        }
+        if (m_completion == completion) return;
+        m_completion = completion;
 
         if (update) {
             m_notifier.makeDeferredNotifications();
@@ -152,21 +150,17 @@
      */
     void add(Event e) override {
 
-        {   QMutexLocker locker(&m_mutex);
-            m_events.add(e.withoutValue().withoutDuration());
+        m_events.add(e.withoutValue().withoutDuration());
 
-            if (e.getLabel() != "") {
-                m_haveTextLabels = true;
-            }
+        if (e.getLabel() != "") {
+            m_haveTextLabels = true;
         }
         
         m_notifier.update(e.getFrame(), m_resolution);
     }
     
     void remove(Event e) override {
-        {   QMutexLocker locker(&m_mutex);
-            m_events.remove(e);
-        }
+        m_events.remove(e);
         emit modelChangedWithin(getId(),
                                 e.getFrame(), e.getFrame() + m_resolution);
     }
@@ -311,16 +305,13 @@
     sv_samplerate_t m_sampleRate;
     int m_resolution;
 
-    bool m_haveTextLabels;
+    std::atomic<bool> m_haveTextLabels;
     DeferredNotifier m_notifier;
-    int m_completion;
+    std::atomic<int> m_completion;
 
     EventSeries m_events;
-
-    mutable QMutex m_mutex;  
 };
 
 #endif
 
 
-    
diff -r e8b552549225 -r 13bd41bd8a17 data/model/SparseTimeValueModel.h
--- a/data/model/SparseTimeValueModel.h	Tue Oct 01 11:21:56 2019 +0100
+++ b/data/model/SparseTimeValueModel.h	Tue Oct 01 11:22:48 2019 +0100
@@ -105,8 +105,12 @@
     bool canPlay() const override { return true; }
     bool getDefaultPlayAudible() const override { return false; } // user must unmute
 
-    QString getScaleUnits() const { return m_units; }
+    QString getScaleUnits() const {
+        QMutexLocker locker(&m_mutex);
+        return m_units;
+    }
     void setScaleUnits(QString units) {
+        QMutexLocker locker(&m_mutex);
         m_units = units;
         UnitDatabase::getInstance()->registerUnit(units);
     }
@@ -120,7 +124,7 @@
 
     void setCompletion(int completion, bool update = true) {
         
-        {   QMutexLocker locker(&m_mutex);
+        {
             if (m_completion == completion) return;
             m_completion = completion;
         }
@@ -185,23 +189,21 @@
 
         bool allChange = false;
            
-        {   QMutexLocker locker(&m_mutex);
-            m_events.add(e.withoutDuration()); // can't have duration here
+        m_events.add(e.withoutDuration()); // can't have duration here
 
-            if (e.getLabel() != "") {
-                m_haveTextLabels = true;
+        if (e.getLabel() != "") {
+            m_haveTextLabels = true;
+        }
+
+        float v = e.getValue();
+        if (!ISNAN(v) && !ISINF(v)) {
+            if (!m_haveExtents || v < m_valueMinimum) {
+                m_valueMinimum = v; allChange = true;
             }
-
-            float v = e.getValue();
-            if (!ISNAN(v) && !ISINF(v)) {
-                if (!m_haveExtents || v < m_valueMinimum) {
-                    m_valueMinimum = v; allChange = true;
-                }
-                if (!m_haveExtents || v > m_valueMaximum) {
-                    m_valueMaximum = v; allChange = true;
-                }
-                m_haveExtents = true;
+            if (!m_haveExtents || v > m_valueMaximum) {
+                m_valueMaximum = v; allChange = true;
             }
+            m_haveExtents = true;
         }
         
         m_notifier.update(e.getFrame(), m_resolution);
@@ -212,10 +214,7 @@
     }
     
     void remove(Event e) override {
-        {
-            QMutexLocker locker(&m_mutex);
-            m_events.remove(e);
-        }
+        m_events.remove(e);
         emit modelChangedWithin(getId(),
                                 e.getFrame(), e.getFrame() + m_resolution);
     }
@@ -344,20 +343,17 @@
     sv_samplerate_t m_sampleRate;
     int m_resolution;
 
-    float m_valueMinimum;
-    float m_valueMaximum;
-    bool m_haveExtents;
-    bool m_haveTextLabels;
+    std::atomic<float> m_valueMinimum;
+    std::atomic<float> m_valueMaximum;
+    std::atomic<bool> m_haveExtents;
+    std::atomic<bool> m_haveTextLabels;
     QString m_units;
     DeferredNotifier m_notifier;
-    int m_completion;
+    std::atomic<int> m_completion;
 
     EventSeries m_events;
-
-    mutable QMutex m_mutex;  
 };
 
-
 #endif