Fix waveform concurrency issues.

* Use const shared pointers for waveforms. * Move all std::vector sizing operations to the Waveform constructor. No vector state is allowed to change after the constructor finishes. * Clarify comments. * Remove waveform mutex usage in WOverview variants. * Use the track's waveform pointer to atomically swap the waveform rather than mutating the waveform in place. * Remove unused code.
mixxxdj · Dec 3, 2014 · 24707ae · 24707ae
1 parent 2dc47ab
commit 24707ae
Show file tree

Hide file tree

Showing 25 changed files with 314 additions and 290 deletions.
diff --git a/src/analyserwaveform.cpp b/src/analyserwaveform.cpp
@@ -1,7 +1,6 @@
 #include <QImage>
 #include <QtDebug>
 #include <QTime>
-#include <QMutexLocker>
 #include <QtDebug>
 
 #include "analyserwaveform.h"
@@ -15,8 +14,6 @@
 
 AnalyserWaveform::AnalyserWaveform(ConfigObject<ConfigValue>* pConfig) :
         m_skipProcessing(false),
-        m_waveform(NULL),
-        m_waveformSummary(NULL),
         m_waveformDataSize(0),
         m_waveformSummaryDataSize(0),
         m_waveformData(NULL),
@@ -60,10 +57,8 @@ bool AnalyserWaveform::initialise(TrackPointer tio, int sampleRate, int totalSam
     m_skipProcessing = false;
 
     m_timer->start();
-    m_waveform = tio->getWaveform();
-    m_waveformSummary = tio->getWaveformSummary();
 
-    if (!m_waveform || !m_waveformSummary || totalSamples == 0) {
+    if (totalSamples == 0) {
         qWarning() << "AnalyserWaveform::initialise - no waveform/waveform summary";
         return false;
     }
@@ -72,35 +67,44 @@ bool AnalyserWaveform::initialise(TrackPointer tio, int sampleRate, int totalSam
     if (loadStored(tio)) {
         m_skipProcessing = true;
     } else {
-        // Now actually initalise the AnalyserWaveform:
-        QMutexLocker waveformLocker(m_waveform->getMutex());
-        QMutexLocker waveformSummaryLocker(m_waveformSummary->getMutex());
-
+        // Now actually initialize the AnalyserWaveform:
         destroyFilters();
         resetFilters(tio, sampleRate);
 
         //TODO (vrince) Do we want to expose this as settings or whatever ?
         const int mainWaveformSampleRate = 441;
         //two visual sample per pixel in full width overview in full hd
         const int summaryWaveformSamples = 2*1920;
-        m_waveform->initalise(sampleRate, totalSamples, mainWaveformSampleRate);
-        m_waveformSummary->initalise(sampleRate, totalSamples,
-                mainWaveformSampleRate, summaryWaveformSamples);
+
+        Waveform* pWaveform = new Waveform(sampleRate, totalSamples,
+                                           mainWaveformSampleRate, -1);
+        Waveform* pWaveformSummary = new Waveform(sampleRate, totalSamples,
+                                                  mainWaveformSampleRate,
+                                                  summaryWaveformSamples);
+        m_waveform = WaveformPointer(pWaveform);
+        m_waveformSummary = WaveformPointer(pWaveformSummary);
+
+        // We must not allow other parts of Mixxx to touch the waveforms while
+        // we are initializing them. Don't set them on the TIO until they are
+        // ready.
+        tio->setWaveform(m_waveform);
+        tio->setWaveformSummary(m_waveformSummary);
 
         m_waveformDataSize = m_waveform->getDataSize();
         m_waveformSummaryDataSize = m_waveformSummary->getDataSize();
 
-        m_waveformData = &m_waveform->at(0);
-        m_waveformSummaryData = &m_waveformSummary->at(0);
+        m_waveformData = pWaveform->data();
+        m_waveformSummaryData = pWaveformSummary->data();
 
-        m_stride.init(m_waveform->getAudioVisualRatio(), m_waveformSummary->getAudioVisualRatio());
+        m_stride.init(m_waveform->getAudioVisualRatio(),
+                      m_waveformSummary->getAudioVisualRatio());
 
         m_currentStride = 0;
         m_currentSummaryStride = 0;
 
         //debug
         //m_waveform->dump();
-        //m_waveformSummary->dump();
+        //pWaveformSummary->dump();
 
     #ifdef TEST_HEAT_MAP
         test_heatMap = new QImage(256,256,QImage::Format_RGB32);
@@ -111,17 +115,14 @@ bool AnalyserWaveform::initialise(TrackPointer tio, int sampleRate, int totalSam
 }
 
 bool AnalyserWaveform::loadStored(TrackPointer tio) const {
-    Waveform* waveform = tio->getWaveform();
-    Waveform* waveformSummary = tio->getWaveformSummary();
-
-    if (!waveform || !waveformSummary) {
-        qWarning() << "AnalyserWaveform::loadStored - no waveform/waveform summary";
-        return false;
-    }
+    ConstWaveformPointer pTrackWaveform = tio->getWaveform();
+    ConstWaveformPointer pTrackWaveformSummary = tio->getWaveformSummary();
+    ConstWaveformPointer pLoadedTrackWaveform;
+    ConstWaveformPointer pLoadedTrackWaveformSummary;
 
     int trackId = tio->getId();
-    bool missingWaveform = waveform->getDataSize() == 0;
-    bool missingWavesummary = waveformSummary->getDataSize() == 0;
+    bool missingWaveform = pTrackWaveform.isNull();
+    bool missingWavesummary = pTrackWaveformSummary.isNull();
 
     if (trackId != -1 && (missingWaveform || missingWavesummary)) {
         QList<AnalysisDao::AnalysisInfo> analyses =
@@ -135,20 +136,19 @@ bool AnalyserWaveform::loadStored(TrackPointer tio) const {
             if (analysis.type == AnalysisDao::TYPE_WAVEFORM) {
                 vc = WaveformFactory::waveformVersionToVersionClass(analysis.version);
                 if (missingWaveform && vc == WaveformFactory::VC_USE) {
-                    if (WaveformFactory::updateWaveformFromAnalysis(waveform, analysis)) {
-                        missingWaveform = false;
-                    }
+                    pLoadedTrackWaveform = ConstWaveformPointer(
+                            WaveformFactory::loadWaveformFromAnalysis(analysis));
+                    missingWaveform = false;
                 } else if (vc != WaveformFactory::VC_KEEP) {
                     // remove all other Analysis except that one we should keep
                     m_analysisDao->deleteAnalysis(analysis.analysisId);
                 }
             } if (analysis.type == AnalysisDao::TYPE_WAVESUMMARY) {
                 vc = WaveformFactory::waveformSummaryVersionToVersionClass(analysis.version);
                 if (missingWavesummary && vc == WaveformFactory::VC_USE) {
-                    if (WaveformFactory::updateWaveformFromAnalysis(waveformSummary, analysis)) {
-                        tio->waveformSummaryNew();
-                        missingWavesummary = false;
-                    }
+                    pLoadedTrackWaveformSummary = ConstWaveformPointer(
+                            WaveformFactory::loadWaveformFromAnalysis(analysis));
+                    missingWavesummary = false;
                 } else if (vc != WaveformFactory::VC_KEEP) {
                     // remove all other Analysis except that one we should keep
                     m_analysisDao->deleteAnalysis(analysis.analysisId);
@@ -160,6 +160,12 @@ bool AnalyserWaveform::loadStored(TrackPointer tio) const {
     // If we don't need to calculate the waveform/wavesummary, skip.
     if (!missingWaveform && !missingWavesummary) {
         qDebug() << "AnalyserWaveform::loadStored - Stored waveform loaded";
+        if (pLoadedTrackWaveform) {
+            tio->setWaveform(pLoadedTrackWaveform);
+        }
+        if (pLoadedTrackWaveformSummary) {
+            tio->setWaveformSummary(pLoadedTrackWaveformSummary);
+        }
         return true;
     }
     return false;
@@ -252,13 +258,13 @@ void AnalyserWaveform::process(const CSAMPLE* buffer, const int bufferLength) {
             m_waveformSummary->setCompletion(m_currentSummaryStride);
 
 #ifdef TEST_HEAT_MAP
-                QPointF point(float(m_strideSummary.m_filteredData[Right][High]),
-                              float(m_strideSummary.m_filteredData[Right][ Mid]));
+                QPointF point(float(m_stride.m_filteredData[Right][High]),
+                              float(m_stride.m_filteredData[Right][ Mid]));
 
                 float norm = sqrt(point.x()*point.x() + point.y()*point.y());
                 point /= norm;
 
-                point *= m_strideSummary.m_filteredData[Right][ Low];
+                point *= m_stride.m_filteredData[Right][ Low];
                 test_heatMap->setPixel(point.toPoint(),0xFF0000FF);
 #endif
         }
@@ -274,33 +280,41 @@ void AnalyserWaveform::cleanup(TrackPointer tio) {
         return;
     }
 
-    if (m_waveform) {
-        m_waveform->reset();
-    }
+    tio->setWaveform(ConstWaveformPointer());
+    m_waveformData = NULL;
+    m_waveformDataSize = 0;
+    m_waveform.clear();
 
-    if (m_waveformSummary) {
-        m_waveformSummary->reset();
-    }
+    tio->setWaveformSummary(ConstWaveformPointer());
+    m_waveformSummaryData = NULL;
+    m_waveformSummaryDataSize = 0;
+    m_waveformSummary.clear();
 }
 
 void AnalyserWaveform::finalise(TrackPointer tio) {
-    if (m_skipProcessing || m_waveform == NULL || m_waveformSummary == NULL) {
+    if (m_skipProcessing) {
         return;
     }
 
-    QMutexLocker waveformLocker(m_waveform->getMutex());
     // Force completion to waveform size
-    m_waveform->setCompletion(m_waveform->getDataSize());
-    m_waveform->setVersion(WaveformFactory::currentWaveformVersion());
-    m_waveform->setDescription(WaveformFactory::currentWaveformDescription());
-    waveformLocker.unlock();
+    if (m_waveform) {
+        m_waveform->setCompletion(m_waveform->getDataSize());
+        m_waveform->setVersion(WaveformFactory::currentWaveformVersion());
+        m_waveform->setDescription(WaveformFactory::currentWaveformDescription());
+        m_waveformData = NULL;
+        m_waveformDataSize = 0;
+        m_waveform.clear();
+    }
 
-    QMutexLocker waveformSummaryLocker(m_waveformSummary->getMutex());
     // Force completion to waveform size
-    m_waveformSummary->setCompletion(m_waveformSummary->getDataSize());
-    m_waveformSummary->setVersion(WaveformFactory::currentWaveformSummaryVersion());
-    m_waveformSummary->setDescription(WaveformFactory::currentWaveformSummaryDescription());
-    waveformSummaryLocker.unlock();
+    if (m_waveformSummary) {
+        m_waveformSummary->setCompletion(m_waveformSummary->getDataSize());
+        m_waveformSummary->setVersion(WaveformFactory::currentWaveformSummaryVersion());
+        m_waveformSummary->setDescription(WaveformFactory::currentWaveformSummaryDescription());
+        m_waveformSummaryData = NULL;
+        m_waveformSummaryDataSize = 0;
+        m_waveformSummary.clear();
+    }
 
 #ifdef TEST_HEAT_MAP
     test_heatMap->save("heatMap.png");

diff --git a/src/analyserwaveform.h b/src/analyserwaveform.h
@@ -163,8 +163,8 @@ class AnalyserWaveform : public Analyser {
   private:
     bool m_skipProcessing;
 
-    Waveform* m_waveform;
-    Waveform* m_waveformSummary;
+    WaveformPointer m_waveform;
+    WaveformPointer m_waveformSummary;
     int m_waveformDataSize;
     int m_waveformSummaryDataSize;
     WaveformData* m_waveformData;

diff --git a/src/library/dao/analysisdao.cpp b/src/library/dao/analysisdao.cpp
@@ -320,8 +320,8 @@ void AnalysisDao::saveTrackAnalyses(TrackInfoObject* pTrack) {
         return;
     }
     const int trackId = pTrack->getId();
-    Waveform* pWaveform = pTrack->getWaveform();
-    Waveform* pWaveSummary = pTrack->getWaveformSummary();
+    ConstWaveformPointer pWaveform = pTrack->getWaveform();
+    ConstWaveformPointer pWaveSummary = pTrack->getWaveformSummary();
 
     // Don't try to save invalid or non-dirty waveforms.
     if (!pWaveform || pWaveform->getDataSize() == 0 || !pWaveform->isDirty() ||

diff --git a/src/trackinfoobject.cpp b/src/trackinfoobject.cpp
@@ -48,8 +48,6 @@ TrackInfoObject::TrackInfoObject(const QString& file,
           m_pSecurityToken(pToken.isNull() ? Sandbox::openSecurityToken(
                   m_fileInfo, true) : pToken),
           m_qMutex(QMutex::Recursive),
-          m_waveform(new Waveform()),
-          m_waveformSummary(new Waveform()),
           m_analyserProgress(-1) {
     initialize(parseHeader, parseCoverArt);
 }
@@ -61,16 +59,12 @@ TrackInfoObject::TrackInfoObject(const QFileInfo& fileInfo,
           m_pSecurityToken(pToken.isNull() ? Sandbox::openSecurityToken(
                   m_fileInfo, true) : pToken),
           m_qMutex(QMutex::Recursive),
-          m_waveform(new Waveform()),
-          m_waveformSummary(new Waveform()),
           m_analyserProgress(-1) {
     initialize(parseHeader, parseCoverArt);
 }
 
 TrackInfoObject::TrackInfoObject(const QDomNode &nodeHeader)
         : m_qMutex(QMutex::Recursive),
-          m_waveform(new Waveform()),
-          m_waveformSummary(new Waveform()),
           m_analyserProgress(-1) {
     QString filename = XmlParse::selectNodeQString(nodeHeader, "Filename");
     QString location = XmlParse::selectNodeQString(nodeHeader, "Filepath") + "/" +  filename;
@@ -145,9 +139,6 @@ TrackInfoObject::~TrackInfoObject() {
     // Notifies TrackDAO and other listeners that this track is about to be
     // deleted and should be saved to the database, removed from caches, etc.
     emit(deleted(this));
-
-    delete m_waveform;
-    delete m_waveformSummary;
 }
 
 void TrackInfoObject::parse(bool parseCoverArt) {
@@ -788,16 +779,21 @@ QString TrackInfoObject::getURL() {
     return m_sURL;
 }
 
-Waveform* TrackInfoObject::getWaveform() {
+ConstWaveformPointer TrackInfoObject::getWaveform() {
     return m_waveform;
 }
 
-Waveform* TrackInfoObject::getWaveformSummary() {
+void TrackInfoObject::setWaveform(ConstWaveformPointer pWaveform) {
+    m_waveform = pWaveform;
+    emit(waveformUpdated());
+}
+
+ConstWaveformPointer TrackInfoObject::getWaveformSummary() const {
     return m_waveformSummary;
 }
 
-// called from the AnalyserQueue Thread
-void TrackInfoObject::waveformSummaryNew() {
+void TrackInfoObject::setWaveformSummary(ConstWaveformPointer pWaveform) {
+    m_waveformSummary = pWaveform;
     emit(waveformSummaryUpdated());
 }
 

diff --git a/src/trackinfoobject.h b/src/trackinfoobject.h
@@ -36,9 +36,9 @@
 #include "track/beats.h"
 #include "track/keys.h"
 #include "util/sandbox.h"
+#include "waveform/waveform.h"
 
 class Cue;
-class Waveform;
 
 class TrackInfoObject;
 typedef QSharedPointer<TrackInfoObject> TrackPointer;
@@ -223,12 +223,11 @@ class TrackInfoObject : public QObject {
     // Set URL for track
     void setURL(const QString& url);
 
-    Waveform* getWaveform();
-    void waveformNew();
+    ConstWaveformPointer getWaveform();
+    void setWaveform(ConstWaveformPointer pWaveform);
 
-    Waveform* getWaveformSummary();
-    const Waveform* getWaveformSummary() const;
-    void waveformSummaryNew();
+    ConstWaveformPointer getWaveformSummary() const;
+    void setWaveformSummary(ConstWaveformPointer pWaveform);
 
     void setAnalyserProgress(int progress);
     int getAnalyserProgress() const;
@@ -396,8 +395,8 @@ class TrackInfoObject : public QObject {
     BeatsPointer m_pBeats;
 
     //Visual waveform data
-    Waveform* const m_waveform;
-    Waveform* const m_waveformSummary;
+    ConstWaveformPointer m_waveform;
+    ConstWaveformPointer m_waveformSummary;
 
     QAtomicInt m_analyserProgress; // in 0.1%
 

diff --git a/src/waveform/renderers/glslwaveformrenderersignal.cpp b/src/waveform/renderers/glslwaveformrenderersignal.cpp
@@ -79,13 +79,13 @@ bool GLSLWaveformRendererSignal::loadShaders() {
 
 bool GLSLWaveformRendererSignal::loadTexture() {
     TrackPointer trackInfo = m_waveformRenderer->getTrackInfo();
-    Waveform* waveform = NULL;
+    ConstWaveformPointer waveform;
     int dataSize = 0;
-    WaveformData* data = NULL;
+    const WaveformData* data = NULL;
 
     if (trackInfo) {
         waveform = trackInfo->getWaveform();
-        if (waveform != NULL) {
+        if (waveform) {
             dataSize = waveform->getDataSize();
             if (dataSize > 1) {
                 data = waveform->data();
@@ -229,8 +229,8 @@ void GLSLWaveformRendererSignal::draw(QPainter* painter, QPaintEvent* /*event*/)
         return;
     }
 
-    const Waveform* waveform = trackInfo->getWaveform();
-    if (waveform == NULL) {
+    ConstWaveformPointer waveform = trackInfo->getWaveform();
+    if (waveform.isNull()) {
         return;
     }
 

diff --git a/src/waveform/renderers/glvsynctestrenderer.cpp b/src/waveform/renderers/glvsynctestrenderer.cpp
@@ -38,8 +38,8 @@ void GLVSyncTestRenderer::draw(QPainter* painter, QPaintEvent* /*event*/) {
         return;
     }
 
-    const Waveform* waveform = pTrack->getWaveform();
-    if (waveform == NULL) {
+    ConstWaveformPointer waveform = pTrack->getWaveform();
+    if (waveform.isNull()) {
         return;
     }
 

diff --git a/src/waveform/renderers/glwaveformrendererfilteredsignal.cpp b/src/waveform/renderers/glwaveformrendererfilteredsignal.cpp
@@ -31,8 +31,8 @@ void GLWaveformRendererFilteredSignal::draw(QPainter* painter, QPaintEvent* /*ev
         return;
     }
 
-    const Waveform* waveform = pTrack->getWaveform();
-    if (waveform == NULL) {
+    ConstWaveformPointer waveform = pTrack->getWaveform();
+    if (waveform.isNull()) {
         return;
     }