Merge pull request #1289 from uklotzde/analysis_fixes

Analysis Fixes

src/analyzer/analyzerqueue.cpp

@@ -7,11 +7,13 @@
 #include "analyzer/analyzergain.h"
 #include "analyzer/analyzerebur128.h"
 #include "analyzer/analyzerwaveform.h"
+#include "library/dao/analysisdao.h"
 #include "mixer/playerinfo.h"
 #include "sources/soundsourceproxy.h"
 #include "track/track.h"
 #include "util/compatibility.h"
 #include "util/db/dbconnectionpooler.h"
+#include "util/db/dbconnectionpooled.h"
 #include "util/event.h"
 #include "util/timer.h"
 #include "util/trace.h"
@@ -51,7 +53,8 @@ AnalyzerQueue::AnalyzerQueue(
           m_queue_size(0) {
 
     if (mode != Mode::WithoutWaveform) {
-        m_pAnalyzers.push_back(std::make_unique<AnalyzerWaveform>(pConfig));
+        m_pAnalysisDao = std::make_unique<AnalysisDao>(pConfig);
+        m_pAnalyzers.push_back(std::make_unique<AnalyzerWaveform>(m_pAnalysisDao.get()));
     }
     m_pAnalyzers.push_back(std::make_unique<AnalyzerGain>(pConfig));
     m_pAnalyzers.push_back(std::make_unique<AnalyzerEbur128>(pConfig));
@@ -197,7 +200,7 @@ bool AnalyzerQueue::doAnalysis(TrackPointer tio, mixxx::AudioSourcePointer pAudi
 
         const SINT framesRead =
                 pAudioSource->readSampleFramesStereo(
-                        kAnalysisFramesPerBlock,
+                        framesToRead,
                         &m_sampleBuffer);
         DEBUG_ASSERT(framesRead <= framesToRead);
         frameIndex += framesRead;
@@ -299,12 +302,25 @@ void AnalyzerQueue::run() {
 }
 
 void AnalyzerQueue::execThread() {
-    const mixxx::DbConnectionPooler dbConnection(m_pDbConnectionPool);
-    if (!dbConnection) {
-        kLogger.warning()
-                << "Failed to open database connection for analyzer queue";
-        kLogger.debug() << "Exiting thread";
-        return;
+    // The thread-local database connection for waveform analysis must not
+    // be closed before returning from this function. Therefore the
+    // DbConnectionPooler is defined at this outer function scope,
+    // independent of whether a database connection will be opened
+    // or not.
+    mixxx::DbConnectionPooler dbConnectionPooler;
+    // m_pAnalysisDao remains null if no analyzer needs database access.
+    // Currently only waveform analyses makes use of it.
+    if (m_pAnalysisDao) {
+        dbConnectionPooler = mixxx::DbConnectionPooler(m_pDbConnectionPool); // move assignment
+        if (!dbConnectionPooler.isPooling()) {
+            kLogger.warning()
+                    << "Failed to obtain database connection for analyzer queue thread";
+            return;
+        }
+        // Obtain and use the newly created database connection within this thread
+        QSqlDatabase dbConnection = mixxx::DbConnectionPooled(m_pDbConnectionPool);
+        DEBUG_ASSERT(dbConnection.isOpen());
+        m_pAnalysisDao->initialize(dbConnection);
     }
 
     m_progressInfo.current_track.reset();
@@ -380,6 +396,13 @@ void AnalyzerQueue::execThread() {
         }
         emptyCheck();
     }
+
+    if (m_pAnalysisDao) {
+        // Invalidate reference to the thread-local database connection
+        // that will be closed soon. Not necessary, just in case ;)
+        m_pAnalysisDao->initialize(QSqlDatabase());
+    }
+
     emit(queueEmpty()); // emit in case of exit;
 }
 

src/analyzer/analyzerqueue.h

@@ -16,6 +16,7 @@
 #include "util/memory.h"
 
 class Analyzer;
+class AnalysisDao;
 
 class AnalyzerQueue : public QThread {
     Q_OBJECT
@@ -60,6 +61,8 @@ class AnalyzerQueue : public QThread {
 
     mixxx::DbConnectionPoolPtr m_pDbConnectionPool;
 
+    std::unique_ptr<AnalysisDao> m_pAnalysisDao;
+
     typedef std::unique_ptr<Analyzer> AnalyzerPtr;
     std::vector<AnalyzerPtr> m_pAnalyzers;
 

src/analyzer/analyzerwaveform.cpp

@@ -16,14 +16,15 @@ mixxx::Logger kLogger("AnalyzerWaveform");
 } // anonymous
 
 AnalyzerWaveform::AnalyzerWaveform(
-        const UserSettingsPointer& pConfig) :
-        m_analysisDao(pConfig),
+        AnalysisDao* pAnalysisDao) :
+        m_pAnalysisDao(pAnalysisDao),
         m_skipProcessing(false),
         m_waveformData(nullptr),
         m_waveformSummaryData(nullptr),
         m_stride(0, 0),
         m_currentStride(0),
         m_currentSummaryStride(0) {
+    DEBUG_ASSERT(m_pAnalysisDao); // mandatory
     m_filter[0] = 0;
     m_filter[1] = 0;
     m_filter[2] = 0;
@@ -102,7 +103,7 @@ bool AnalyzerWaveform::isDisabledOrLoadStoredSuccess(TrackPointer tio) const {
 
     if (trackId.isValid() && (missingWaveform || missingWavesummary)) {
         QList<AnalysisDao::AnalysisInfo> analyses =
-                m_analysisDao.getAnalysesForTrack(trackId);
+                m_pAnalysisDao->getAnalysesForTrack(trackId);
 
         QListIterator<AnalysisDao::AnalysisInfo> it(analyses);
         while (it.hasNext()) {
@@ -117,7 +118,7 @@ bool AnalyzerWaveform::isDisabledOrLoadStoredSuccess(TrackPointer tio) const {
                     missingWaveform = false;
                 } else if (vc != WaveformFactory::VC_KEEP) {
                     // remove all other Analysis except that one we should keep
-                    m_analysisDao.deleteAnalysis(analysis.analysisId);
+                    m_pAnalysisDao->deleteAnalysis(analysis.analysisId);
                 }
             } if (analysis.type == AnalysisDao::TYPE_WAVESUMMARY) {
                 vc = WaveformFactory::waveformSummaryVersionToVersionClass(analysis.version);
@@ -127,7 +128,7 @@ bool AnalyzerWaveform::isDisabledOrLoadStoredSuccess(TrackPointer tio) const {
                     missingWavesummary = false;
                 } else if (vc != WaveformFactory::VC_KEEP) {
                     // remove all other Analysis except that one we should keep
-                    m_analysisDao.deleteAnalysis(analysis.analysisId);
+                    m_pAnalysisDao->deleteAnalysis(analysis.analysisId);
                 }
             }
         }
@@ -309,7 +310,7 @@ void AnalyzerWaveform::finalize(TrackPointer tio) {
     // waveforms (i.e. if the config setting was disabled in a previous scan)
     // and then it is not called. The other analyzers have signals which control
     // the update of their data.
-    m_analysisDao.saveTrackAnalyses(*tio);
+    m_pAnalysisDao->saveTrackAnalyses(*tio);
 
     kLogger.debug() << "Waveform generation for track" << tio->getId() << "done"
              << m_timer.elapsed().debugSecondsWithUnit();

src/analyzer/analyzerwaveform.h

@@ -7,14 +7,14 @@
 
 #include "analyzer/analyzer.h"
 #include "waveform/waveform.h"
-#include "library/dao/analysisdao.h"
 #include "util/math.h"
 #include "util/performancetimer.h"
 
 //NOTS vrince some test to segment sound, to apply color in the waveform
 //#define TEST_HEAT_MAP
 
 class EngineFilterIIRBase;
+class AnalysisDao;
 
 inline CSAMPLE scaleSignal(CSAMPLE invalue, FilterIndex index = FilterCount) {
     if (invalue == 0.0) {
@@ -137,7 +137,7 @@ struct WaveformStride {
 class AnalyzerWaveform : public Analyzer {
   public:
     explicit AnalyzerWaveform(
-            const UserSettingsPointer& pConfig);
+            AnalysisDao* pAnalysisDao);
     ~AnalyzerWaveform() override;
 
     bool initialize(TrackPointer tio, int sampleRate, int totalSamples) override;
@@ -154,7 +154,7 @@ class AnalyzerWaveform : public Analyzer {
     void destroyFilters();
     void storeIfGreater(float* pDest, float source);
 
-    mutable AnalysisDao m_analysisDao;
+    AnalysisDao* m_pAnalysisDao;
 
     bool m_skipProcessing;
 

src/preferences/upgrade.cpp

@@ -366,7 +366,7 @@ UserSettingsPointer Upgrade::versionUpgrade(const QString& settingsPath) {
             MixxxDb mixxxDb(config);
             const mixxx::DbConnectionPooler dbConnectionPooler(
                     mixxxDb.connectionPool());
-            if (dbConnectionPooler) {
+            if (dbConnectionPooler.isPooling()) {
                 QSqlDatabase dbConnection = mixxx::DbConnectionPooled(mixxxDb.connectionPool());
                 DEBUG_ASSERT(dbConnection.isOpen());
                 if (MixxxDb::initDatabaseSchema(dbConnection)) {

src/test/analyserwaveformtest.cpp

@@ -18,7 +18,8 @@ namespace {
 class AnalyzerWaveformTest: public MixxxTest {
   protected:
     AnalyzerWaveformTest()
-      : aw(config()),
+      : analysisDao(config()),
+        aw(&analysisDao),
         bigbuf(nullptr),
         canaryBigBuf(nullptr) {
     }
@@ -49,6 +50,7 @@ class AnalyzerWaveformTest: public MixxxTest {
     }
 
   protected:
+    AnalysisDao analysisDao;
     AnalyzerWaveform aw;
     TrackPointer tio;
     CSAMPLE* bigbuf;

src/test/dbconnectionpool_test.cpp

@@ -0,0 +1,37 @@
+#include <gtest/gtest.h>
+
+#include "test/mixxxtest.h"
+
+#include "database/mixxxdb.h"
+#include "util/db/dbconnectionpooler.h"
+#include "util/db/dbconnectionpooled.h"
+
+#include "library/dao/settingsdao.h"
+
+#include "util/assert.h"
+
+
+class DbConnectionPoolTest : public MixxxTest {
+  protected:
+    DbConnectionPoolTest()
+            : m_mixxxDb(config()) {
+    }
+
+  protected:
+    const MixxxDb m_mixxxDb;
+};
+
+TEST_F(DbConnectionPoolTest, MoveSemantics) {
+    mixxx::DbConnectionPooler p1(m_mixxxDb.connectionPool());
+    EXPECT_TRUE(p1.isPooling());
+
+    // Move construction
+    mixxx::DbConnectionPooler p2(std::move(p1));
+    EXPECT_FALSE(p1.isPooling());
+    EXPECT_TRUE(p2.isPooling());
+
+    // Move assignment
+    p1 = std::move(p2);
+    EXPECT_TRUE(p1.isPooling());
+    EXPECT_FALSE(p2.isPooling());
+}

src/util/db/dbconnectionpooler.cpp

@@ -15,7 +15,8 @@ DbConnectionPooler::DbConnectionPooler(
         DbConnectionPoolPtr pDbConnectionPool) {
     if (pDbConnectionPool && pDbConnectionPool->createThreadLocalConnection()) {
         // m_pDbConnectionPool indicates if the thread-local connection has actually
-        // been created during construction. Otherwise this instance is non-functional.
+        // been created during construction. Otherwise this instance does not store
+        // any reference to the connection pool and is non-functional.
         m_pDbConnectionPool = std::move(pDbConnectionPool);
     }
 }

src/util/db/dbconnectionpooler.h

@@ -21,22 +21,35 @@ class DbConnectionPooler final {
   public:
     explicit DbConnectionPooler(
             DbConnectionPoolPtr pDbConnectionPool = DbConnectionPoolPtr());
-    DbConnectionPooler(
-            DbConnectionPooler&& other) = default;
+    DbConnectionPooler(const DbConnectionPooler&) = delete;
+#if !defined(_MSC_VER) || _MSC_VER > 1900
+    DbConnectionPooler(DbConnectionPooler&&) = default;
+#else
+    // Workaround for Visual Studio 2015 (and before)
+    DbConnectionPooler(DbConnectionPooler&& other)
+        : m_pDbConnectionPool(std::move(other.m_pDbConnectionPool)) {
+    }
+#endif
     ~DbConnectionPooler();
 
     // Checks if a thread-local connection has actually been created
-    // during construction. Otherwise this instance does not store
-    // any reference to the connection pool and is non-functional.
-    explicit operator bool() const {
+    // during construction and is owned by this instance.
+    bool isPooling() const {
         return static_cast<bool>(m_pDbConnectionPool);
     }
 
-  private:
-    DbConnectionPooler(const DbConnectionPooler&) = delete;
     DbConnectionPooler& operator=(const DbConnectionPooler&) = delete;
-    DbConnectionPooler& operator=(DbConnectionPooler&&) = delete;
+#if !defined(_MSC_VER) || _MSC_VER > 1900
+    DbConnectionPooler& operator=(DbConnectionPooler&&) = default;
+#else
+    // Workaround for Visual Studio 2015 (and before)
+    DbConnectionPooler& operator=(DbConnectionPooler&& other) {
+        m_pDbConnectionPool = std::move(other.m_pDbConnectionPool);
+        return *this;
+    }
+#endif
 
+  private:
     // Prevent heap allocation
     static void * operator new(std::size_t);
     static void * operator new[](std::size_t);