From c236956884e3c7b9a47536f98eb7eb5a126a26e4 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 18 Apr 2018 16:39:07 +0200 Subject: [PATCH 01/25] Fix and enable multi-threaded analysis --- build/depends.py | 7 +- src/analyzer/analyzerebur128.h | 4 + src/analyzer/analyzergain.h | 4 + src/analyzer/analyzerprogress.h | 22 ++ src/analyzer/analyzerqueue.cpp | 468 ------------------------ src/analyzer/analyzerqueue.h | 91 ----- src/analyzer/analyzerthread.cpp | 326 +++++++++++++++++ src/analyzer/analyzerthread.h | 143 ++++++++ src/analyzer/trackanalysisscheduler.cpp | 276 ++++++++++++++ src/analyzer/trackanalysisscheduler.h | 174 +++++++++ src/control/controlvalue.h | 111 ++++-- src/library/analysisfeature.cpp | 153 ++++---- src/library/analysisfeature.h | 25 +- src/library/dlganalysis.cpp | 71 ++-- src/library/dlganalysis.h | 15 +- src/library/library.cpp | 12 +- src/library/library.h | 9 +- src/mixer/basetrackplayer.cpp | 1 - src/mixer/playermanager.cpp | 102 ++++-- src/mixer/playermanager.h | 18 +- src/mixxx.cpp | 1 - src/preferences/replaygainsettings.cpp | 13 +- src/preferences/replaygainsettings.h | 1 + src/skin/legacyskinparser.cpp | 6 +- src/track/track.cpp | 19 +- src/track/track.h | 4 - src/util/workerthread.cpp | 164 +++++++++ src/util/workerthread.h | 131 +++++++ src/util/workerthreadscheduler.cpp | 52 +++ src/util/workerthreadscheduler.h | 32 ++ src/widget/woverview.cpp | 61 +-- src/widget/woverview.h | 18 +- src/widget/woverviewhsv.cpp | 9 +- src/widget/woverviewhsv.h | 6 +- src/widget/woverviewlmh.cpp | 9 +- src/widget/woverviewlmh.h | 6 +- src/widget/woverviewrgb.cpp | 9 +- src/widget/woverviewrgb.h | 6 +- 38 files changed, 1718 insertions(+), 861 deletions(-) create mode 100644 src/analyzer/analyzerprogress.h delete mode 100644 src/analyzer/analyzerqueue.cpp delete mode 100644 src/analyzer/analyzerqueue.h create mode 100644 src/analyzer/analyzerthread.cpp create mode 100644 src/analyzer/analyzerthread.h create mode 100644 src/analyzer/trackanalysisscheduler.cpp create mode 100644 src/analyzer/trackanalysisscheduler.h create mode 100644 src/util/workerthread.cpp create mode 100644 src/util/workerthread.h create mode 100644 src/util/workerthreadscheduler.cpp create mode 100644 src/util/workerthreadscheduler.h diff --git a/build/depends.py b/build/depends.py index 694e76c7e3f..fea3ee1f792 100644 --- a/build/depends.py +++ b/build/depends.py @@ -772,7 +772,8 @@ def sources(self, build): "src/engine/cachingreaderchunk.cpp", "src/engine/cachingreaderworker.cpp", - "src/analyzer/analyzerqueue.cpp", + "src/analyzer/trackanalysisscheduler.cpp", + "src/analyzer/analyzerthread.cpp", "src/analyzer/analyzerwaveform.cpp", "src/analyzer/analyzergain.cpp", "src/analyzer/analyzerebur128.cpp", @@ -982,7 +983,7 @@ def sources(self, build): "src/library/bpmdelegate.cpp", "src/library/previewbuttondelegate.cpp", "src/library/coverartdelegate.cpp", - "src/library/tableitemdelegate.cpp", + "src/library/tableitemdelegate.cpp", "src/library/treeitemmodel.cpp", "src/library/treeitem.cpp", @@ -1153,6 +1154,8 @@ def sources(self, build): "src/util/indexrange.cpp", "src/util/desktophelper.cpp", "src/util/widgetrendertimer.cpp", + "src/util/workerthread.cpp", + "src/util/workerthreadscheduler.cpp", ] proto_args = { diff --git a/src/analyzer/analyzerebur128.h b/src/analyzer/analyzerebur128.h index 60e996eb01b..fa8edc07607 100644 --- a/src/analyzer/analyzerebur128.h +++ b/src/analyzer/analyzerebur128.h @@ -11,6 +11,10 @@ class AnalyzerEbur128 : public Analyzer { AnalyzerEbur128(UserSettingsPointer pConfig); virtual ~AnalyzerEbur128(); + static bool isEnabled(const ReplayGainSettings& rgSettings) { + return rgSettings.isAnalyzerEnabled(2); + } + bool initialize(TrackPointer tio, int sampleRate, int totalSamples) override; bool isDisabledOrLoadStoredSuccess(TrackPointer tio) const override; void process(const CSAMPLE* pIn, const int iLen) override; diff --git a/src/analyzer/analyzergain.h b/src/analyzer/analyzergain.h index 247b2bcc75f..d2958adfef5 100644 --- a/src/analyzer/analyzergain.h +++ b/src/analyzer/analyzergain.h @@ -18,6 +18,10 @@ class AnalyzerGain : public Analyzer { AnalyzerGain(UserSettingsPointer pConfig); virtual ~AnalyzerGain(); + static bool isEnabled(const ReplayGainSettings& rgSettings) { + return rgSettings.isAnalyzerEnabled(1); + } + bool initialize(TrackPointer tio, int sampleRate, int totalSamples) override; bool isDisabledOrLoadStoredSuccess(TrackPointer tio) const override; void process(const CSAMPLE* pIn, const int iLen) override; diff --git a/src/analyzer/analyzerprogress.h b/src/analyzer/analyzerprogress.h new file mode 100644 index 00000000000..b9b5673245f --- /dev/null +++ b/src/analyzer/analyzerprogress.h @@ -0,0 +1,22 @@ +#pragma once + +#include "util/math.h" + + +typedef double AnalyzerProgress; + +constexpr AnalyzerProgress kAnalyzerProgressUnknown = -1.0; +constexpr AnalyzerProgress kAnalyzerProgressNone = 0.0; // 0.0 % +constexpr AnalyzerProgress kAnalyzerProgressHalf = 0.5; // 50.0 % +constexpr AnalyzerProgress kAnalyzerProgressFinalizing = 0.95; // 95.0 % +constexpr AnalyzerProgress kAnalyzerProgressDone = 1.0; // 100.0% + +Q_DECLARE_METATYPE(AnalyzerProgress); + +// Integer [0, 100] +inline +int analyzerProgressPercent(AnalyzerProgress analyzerProgress) { + DEBUG_ASSERT(analyzerProgress >= kAnalyzerProgressNone); + return int((100 * (math_min(analyzerProgress, kAnalyzerProgressDone) - kAnalyzerProgressNone)) / + (kAnalyzerProgressDone - kAnalyzerProgressNone)); +} diff --git a/src/analyzer/analyzerqueue.cpp b/src/analyzer/analyzerqueue.cpp deleted file mode 100644 index f574f79cd9e..00000000000 --- a/src/analyzer/analyzerqueue.cpp +++ /dev/null @@ -1,468 +0,0 @@ -#include "analyzer/analyzerqueue.h" - -#ifdef __VAMP__ -#include "analyzer/analyzerbeats.h" -#include "analyzer/analyzerkey.h" -#endif -#include "analyzer/analyzergain.h" -#include "analyzer/analyzerebur128.h" -#include "analyzer/analyzerwaveform.h" -#include "library/dao/analysisdao.h" -#include "engine/engine.h" -#include "mixer/playerinfo.h" -#include "sources/soundsourceproxy.h" -#include "sources/audiosourcestereoproxy.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" -#include "util/logger.h" - -// Measured in 0.1%, -// 0 for no progress during finalize -// 1 to display the text "finalizing" -// 100 for 10% step after finalize -#define FINALIZE_PROMILLE 1 - -namespace { - -mixxx::Logger kLogger("AnalyzerQueue"); - -// Analysis is done in blocks. -// We need to use a smaller block size, because on Linux the AnalyzerQueue -// can starve the CPU of its resources, resulting in xruns. A block size -// of 4096 frames per block seems to do fine. -const mixxx::AudioSignal::ChannelCount kAnalysisChannels(mixxx::kEngineChannelCount); -const SINT kAnalysisFramesPerBlock = 4096; -const SINT kAnalysisSamplesPerBlock = - kAnalysisFramesPerBlock * kAnalysisChannels; - -QAtomicInt s_instanceCounter(0); - -} // anonymous namespace - -AnalyzerQueue::AnalyzerQueue( - mixxx::DbConnectionPoolPtr pDbConnectionPool, - const UserSettingsPointer& pConfig, - Mode mode) - : m_pDbConnectionPool(std::move(pDbConnectionPool)), - m_exit(false), - m_aiCheckPriorities(false), - m_sampleBuffer(kAnalysisSamplesPerBlock), - m_queue_size(0) { - - if (mode != Mode::WithoutWaveform) { - m_pAnalysisDao = std::make_unique(pConfig); - m_pAnalyzers.push_back(std::make_unique(m_pAnalysisDao.get())); - } - m_pAnalyzers.push_back(std::make_unique(pConfig)); - m_pAnalyzers.push_back(std::make_unique(pConfig)); -#ifdef __VAMP__ - m_pAnalyzers.push_back(std::make_unique(pConfig)); - m_pAnalyzers.push_back(std::make_unique(pConfig)); -#endif - - connect(this, SIGNAL(updateProgress()), - this, SLOT(slotUpdateProgress())); - - start(QThread::LowPriority); -} - -AnalyzerQueue::~AnalyzerQueue() { - stop(); - m_progressInfo.sema.release(); - wait(); //Wait until thread has actually stopped before proceeding. -} - -// This is called from the AnalyzerQueue thread -bool AnalyzerQueue::isLoadedTrackWaiting(TrackPointer analysingTrack) { - const PlayerInfo& info = PlayerInfo::instance(); - TrackPointer pTrack; - bool trackWaiting = false; - QList progress100List; - QList progress0List; - - QMutexLocker locked(&m_qm); - QMutableListIterator it(m_queuedTracks); - while (it.hasNext()) { - TrackPointer& pTrack = it.next(); - if (!pTrack) { - it.remove(); - continue; - } - if (!trackWaiting) { - trackWaiting = info.isTrackLoaded(pTrack); - } - // try to load waveforms for all new tracks first - // and remove them from queue if already analysed - // This avoids waiting for a running analysis for those tracks. - int progress = pTrack->getAnalyzerProgress(); - if (progress < 0) { - // Load stored analysis - bool processTrack = false; - for (auto const& pAnalyzer: m_pAnalyzers) { - if (!pAnalyzer->isDisabledOrLoadStoredSuccess(pTrack)) { - processTrack = true; - } - } - if (!processTrack) { - progress100List.append(pTrack); - it.remove(); // since pTrack is a reference it is invalid now. - } else { - progress0List.append(pTrack); - } - } else if (progress == 1000) { - it.remove(); - } - } - - locked.unlock(); - - // update progress after unlock to avoid a deadlock - foreach (TrackPointer pTrack, progress100List) { - emitUpdateProgress(pTrack, 1000); - } - foreach (TrackPointer pTrack, progress0List) { - emitUpdateProgress(pTrack, 0); - } - - if (info.isTrackLoaded(analysingTrack)) { - return false; - } - return trackWaiting; -} - -// This is called from the AnalyzerQueue thread -// The returned track might be NULL, up to the caller to check. -TrackPointer AnalyzerQueue::dequeueNextBlocking() { - QMutexLocker locked(&m_qm); - if (m_queuedTracks.isEmpty()) { - Event::end("AnalyzerQueue process"); - m_qwait.wait(&m_qm); - Event::start("AnalyzerQueue process"); - - if (m_exit) { - return TrackPointer(); - } - } - - const PlayerInfo& info = PlayerInfo::instance(); - TrackPointer pLoadTrack; - QMutableListIterator it(m_queuedTracks); - while (it.hasNext()) { - TrackPointer& pTrack = it.next(); - DEBUG_ASSERT(pTrack); - // Prioritize tracks that are loaded. - if (info.isTrackLoaded(pTrack)) { - kLogger.debug() << "Prioritizing" << pTrack->getTitle() << pTrack->getLocation(); - pLoadTrack = pTrack; - it.remove(); - break; - } - } - - if (!pLoadTrack && !m_queuedTracks.isEmpty()) { - // no prioritized track found, use head track - pLoadTrack = m_queuedTracks.dequeue(); - } - - return pLoadTrack; -} - -// This is called from the AnalyzerQueue thread -bool AnalyzerQueue::doAnalysis( - TrackPointer pTrack, - mixxx::AudioSourcePointer pAudioSource) { - - QTime progressUpdateInhibitTimer; - progressUpdateInhibitTimer.start(); // Inhibit Updates for 60 milliseconds - - mixxx::AudioSourceStereoProxy audioSourceProxy( - pAudioSource, - kAnalysisFramesPerBlock); - DEBUG_ASSERT(audioSourceProxy.channelCount() == kAnalysisChannels); - - mixxx::IndexRange remainingFrames = pAudioSource->frameIndexRange(); - bool dieflag = false; - bool cancelled = false; - while (!dieflag && !remainingFrames.empty()) { - ScopedTimer t("AnalyzerQueue::doAnalysis block"); - - const auto inputFrameIndexRange = - remainingFrames.splitAndShrinkFront( - math_min(kAnalysisFramesPerBlock, remainingFrames.length())); - DEBUG_ASSERT(!inputFrameIndexRange.empty()); - const auto readableSampleFrames = - audioSourceProxy.readSampleFrames( - mixxx::WritableSampleFrames( - inputFrameIndexRange, - mixxx::SampleBuffer::WritableSlice(m_sampleBuffer))); - // To compare apples to apples, let's only look at blocks that are - // the full block size. - if (readableSampleFrames.frameLength() == kAnalysisFramesPerBlock) { - // Complete analysis block of audio samples has been read. - for (auto const& pAnalyzer: m_pAnalyzers) { - pAnalyzer->process( - readableSampleFrames.readableData(), - readableSampleFrames.readableLength()); - } - } else { - // Partial analysis block of audio samples has been read. - // This should only happen at the end of an audio stream, - // otherwise a decoding error must have occurred. - if (!remainingFrames.empty()) { - // EOF not reached -> Maybe a corrupt file? - kLogger.warning() - << "Aborting analysis after failed to read sample data from" - << pTrack->getLocation() - << ": expected frames =" << inputFrameIndexRange - << ", actual frames =" << readableSampleFrames.frameIndexRange(); - dieflag = true; // abort - cancelled = false; // completed, no retry - } - } - - // emit progress updates - // During the doAnalysis function it goes only to 100% - FINALIZE_PERCENT - // because the finalize functions will take also some time - //fp div here prevents insane signed overflow - const double frameProgress = - double(pAudioSource->frameLength() - remainingFrames.length()) / - double(pAudioSource->frameLength()); - int progressPromille = frameProgress * (1000 - FINALIZE_PROMILLE); - - if (m_progressInfo.track_progress != progressPromille) { - if (progressUpdateInhibitTimer.elapsed() > 60) { - // Inhibit Updates for 60 milliseconds - emitUpdateProgress(pTrack, progressPromille); - progressUpdateInhibitTimer.start(); - } - } - - // Since this is a background analysis queue, we should co-operatively - // yield every now and then to try and reduce CPU contention. The - // analyzer queue is CPU intensive so we want to get out of the way of - // the audio callback thread. - //QThread::yieldCurrentThread(); - //QThread::usleep(10); - - // has something new entered the queue? - if (m_aiCheckPriorities.fetchAndStoreAcquire(false)) { - if (isLoadedTrackWaiting(pTrack)) { - kLogger.debug() << "Interrupting analysis to give preference to a loaded track."; - dieflag = true; - cancelled = true; - } - } - - if (m_exit) { - dieflag = true; - cancelled = true; - } - - // Ignore blocks in which we decided to bail for stats purposes. - if (dieflag || cancelled) { - t.cancel(); - } - } - - return !cancelled; //don't return !dieflag or we might reanalyze over and over -} - -void AnalyzerQueue::stop() { - m_exit = true; - QMutexLocker locked(&m_qm); - m_qwait.wakeAll(); -} - -void AnalyzerQueue::run() { - // If there are no analyzers, don't waste time running. - if (m_pAnalyzers.empty()) { - return; - } - - const int instanceId = s_instanceCounter.fetchAndAddAcquire(1) + 1; - QThread::currentThread()->setObjectName(QString("AnalyzerQueue %1").arg(instanceId)); - - kLogger.debug() << "Entering thread"; - - execThread(); - - kLogger.debug() << "Exiting thread"; -} - -void AnalyzerQueue::execThread() { - // 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(); - m_progressInfo.track_progress = 0; - m_progressInfo.queue_size = 0; - m_progressInfo.sema.release(); // Initialize with one - - while (!m_exit) { - TrackPointer nextTrack = dequeueNextBlocking(); - - // It's important to check for m_exit here in case we decided to exit - // while blocking for a new track. - if (m_exit) { - break; - } - - // If the track is NULL, try to get the next one. - // Could happen if the track was queued but then deleted. - // Or if dequeueNextBlocking is unblocked by exit == true - if (!nextTrack) { - emptyCheck(); - continue; - } - - kLogger.debug() << "Analyzing" << nextTrack->getTitle() << nextTrack->getLocation(); - - Trace trace("AnalyzerQueue analyzing track"); - - // Get the audio - mixxx::AudioSource::OpenParams openParams; - openParams.setChannelCount(kAnalysisChannels); - auto pAudioSource = SoundSourceProxy(nextTrack).openAudioSource(openParams); - if (!pAudioSource) { - kLogger.warning() - << "Failed to open file for analyzing:" - << nextTrack->getLocation(); - emptyCheck(); - continue; - } - - bool processTrack = false; - for (auto const& pAnalyzer: m_pAnalyzers) { - // Make sure not to short-circuit initialize(...) - if (pAnalyzer->initialize( - nextTrack, - pAudioSource->sampleRate(), - pAudioSource->frameLength() * kAnalysisChannels)) { - processTrack = true; - } - } - - updateSize(); - - if (processTrack) { - emitUpdateProgress(nextTrack, 0); - bool completed = doAnalysis(nextTrack, pAudioSource); - if (!completed) { - // This track was cancelled - for (auto const& pAnalyzer: m_pAnalyzers) { - pAnalyzer->cleanup(nextTrack); - } - queueAnalyseTrack(nextTrack); - emitUpdateProgress(nextTrack, 0); - } else { - // 100% - FINALIZE_PERCENT finished - emitUpdateProgress(nextTrack, 1000 - FINALIZE_PROMILLE); - // This takes around 3 sec on a Atom Netbook - for (auto const& pAnalyzer: m_pAnalyzers) { - pAnalyzer->finalize(nextTrack); - } - emit(trackDone(nextTrack)); - emitUpdateProgress(nextTrack, 1000); // 100% - } - } else { - emitUpdateProgress(nextTrack, 1000); // 100% - kLogger.debug() << "Skipping track analysis because no analyzer initialized."; - } - 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; -} - -void AnalyzerQueue::emptyCheck() { - updateSize(); - if (m_queue_size == 0) { - emit(queueEmpty()); // emit asynchrony for no deadlock - } -} - -void AnalyzerQueue::updateSize() { - QMutexLocker locked(&m_qm); - m_queue_size = m_queuedTracks.size(); -} - -// This is called from the AnalyzerQueue thread -void AnalyzerQueue::emitUpdateProgress(TrackPointer track, int progress) { - if (!m_exit) { - // First tryAcqire will have always success because sema is initialized with on - // The following tries will success if the previous signal was processed in the GUI Thread - // This prevent the AnalysisQueue from filling up the GUI Thread event Queue - // 100 % is emitted in any case - if (progress < 1000 - FINALIZE_PROMILLE && progress > 0) { - // Signals during processing are not required in any case - if (!m_progressInfo.sema.tryAcquire()) { - return; - } - } else { - m_progressInfo.sema.acquire(); - } - m_progressInfo.current_track = track; - m_progressInfo.track_progress = progress; - m_progressInfo.queue_size = m_queue_size; - emit(updateProgress()); - } -} - -//slot -void AnalyzerQueue::slotUpdateProgress() { - if (m_progressInfo.current_track) { - m_progressInfo.current_track->setAnalyzerProgress(m_progressInfo.track_progress); - m_progressInfo.current_track.reset(); - } - emit(trackProgress(m_progressInfo.track_progress / 10)); - if (m_progressInfo.track_progress == 1000) { - emit(trackFinished(m_progressInfo.queue_size)); - } - m_progressInfo.sema.release(); -} - -void AnalyzerQueue::slotAnalyseTrack(TrackPointer pTrack) { - // This slot is called from the decks and and samplers when the track was loaded. - queueAnalyseTrack(pTrack); - m_aiCheckPriorities = true; -} - -// This is called from the GUI and from the AnalyzerQueue thread -void AnalyzerQueue::queueAnalyseTrack(TrackPointer pTrack) { - if (pTrack) { - QMutexLocker locked(&m_qm); - if (!m_queuedTracks.contains(pTrack)) { - m_queuedTracks.enqueue(pTrack); - m_qwait.wakeAll(); - } - } -} diff --git a/src/analyzer/analyzerqueue.h b/src/analyzer/analyzerqueue.h deleted file mode 100644 index 41bd403b85e..00000000000 --- a/src/analyzer/analyzerqueue.h +++ /dev/null @@ -1,91 +0,0 @@ -#ifndef ANALYZER_ANALYZERQUEUE_H -#define ANALYZER_ANALYZERQUEUE_H - -#include -#include -#include -#include - -#include - -#include "preferences/usersettings.h" -#include "sources/audiosource.h" -#include "track/track.h" -#include "util/db/dbconnectionpool.h" -#include "util/samplebuffer.h" -#include "util/memory.h" - -class Analyzer; -class AnalysisDao; - -class AnalyzerQueue : public QThread { - Q_OBJECT - - public: - enum class Mode { - Default, - WithoutWaveform, - }; - - AnalyzerQueue( - mixxx::DbConnectionPoolPtr pDbConnectionPool, - const UserSettingsPointer& pConfig, - Mode mode = Mode::Default); - ~AnalyzerQueue() override; - - void stop(); - void queueAnalyseTrack(TrackPointer tio); - - public slots: - void slotAnalyseTrack(TrackPointer tio); - void slotUpdateProgress(); - - signals: - void trackProgress(int progress); - void trackDone(TrackPointer track); - void trackFinished(int size); - // Signals from AnalyzerQueue Thread: - void queueEmpty(); - void updateProgress(); - - protected: - void run(); - - private: - struct progress_info { - TrackPointer current_track; - int track_progress; // in 0.1 % - int queue_size; - QSemaphore sema; - }; - - mixxx::DbConnectionPoolPtr m_pDbConnectionPool; - - std::unique_ptr m_pAnalysisDao; - - typedef std::unique_ptr AnalyzerPtr; - std::vector m_pAnalyzers; - - void execThread(); - - bool isLoadedTrackWaiting(TrackPointer analysingTrack); - TrackPointer dequeueNextBlocking(); - bool doAnalysis(TrackPointer tio, mixxx::AudioSourcePointer pAudioSource); - void emitUpdateProgress(TrackPointer tio, int progress); - void emptyCheck(); - void updateSize(); - - bool m_exit; - QAtomicInt m_aiCheckPriorities; - - mixxx::SampleBuffer m_sampleBuffer; - - // The processing queue and associated mutex - QQueue m_queuedTracks; - QMutex m_qm; - QWaitCondition m_qwait; - struct progress_info m_progressInfo; - int m_queue_size; -}; - -#endif /* ANALYZER_ANALYZERQUEUE_H */ diff --git a/src/analyzer/analyzerthread.cpp b/src/analyzer/analyzerthread.cpp new file mode 100644 index 00000000000..4951fe6fdb4 --- /dev/null +++ b/src/analyzer/analyzerthread.cpp @@ -0,0 +1,326 @@ +#include "analyzer/analyzerthread.h" + +#ifdef __VAMP__ +#include "analyzer/analyzerbeats.h" +#include "analyzer/analyzerkey.h" +#endif +#include "analyzer/analyzergain.h" +#include "analyzer/analyzerebur128.h" +#include "analyzer/analyzerwaveform.h" + +#include "library/dao/analysisdao.h" + +#include "engine/engine.h" + +#include "sources/soundsourceproxy.h" +#include "sources/audiosourcestereoproxy.h" + +#include "util/db/dbconnectionpooler.h" +#include "util/db/dbconnectionpooled.h" +#include "util/logger.h" +#include "util/timer.h" + + +namespace { + +mixxx::Logger kLogger("AnalyzerThread"); + +// Analysis is done in blocks. +// We need to use a smaller block size, because on Linux the AnalyzerThread +// can starve the CPU of its resources, resulting in xruns. A block size +// of 4096 frames per block seems to do fine. +constexpr mixxx::AudioSignal::ChannelCount kAnalysisChannels = mixxx::kEngineChannelCount; +constexpr SINT kAnalysisFramesPerBlock = 4096; +const SINT kAnalysisSamplesPerBlock = + kAnalysisFramesPerBlock * kAnalysisChannels; + +// Maximum frequency of progress updates while busy +const mixxx::Duration kBusyProgressInhibitDuration = mixxx::Duration::fromMillis(60); + +void deleteAnalyzerThread(AnalyzerThread* plainPtr) { + if (plainPtr) { + plainPtr->deleteAfterFinished(); + } +} + +} // anonymous namespace + +//static +AnalyzerThread::Pointer AnalyzerThread::nullPointer() { + return Pointer(nullptr, [](AnalyzerThread*){}); +} + +//static +AnalyzerThread::Pointer AnalyzerThread::createInstance( + int id, + mixxx::DbConnectionPoolPtr dbConnectionPool, + UserSettingsPointer pConfig, + AnalyzerMode mode) { + return Pointer(new AnalyzerThread( + id, + dbConnectionPool, + pConfig, + mode), + deleteAnalyzerThread); +} + +AnalyzerThread::AnalyzerThread( + int id, + mixxx::DbConnectionPoolPtr dbConnectionPool, + UserSettingsPointer pConfig, + AnalyzerMode mode) + : WorkerThread(QString("AnalyzerThread %1").arg(id)), + m_id(id), + m_dbConnectionPool(std::move(dbConnectionPool)), + m_pConfig(std::move(pConfig)), + m_mode(mode), + m_sampleBuffer(kAnalysisSamplesPerBlock), + m_emittedState(AnalyzerThreadState::Void) { + m_lastBusyProgressEmittedTimer.start(); + m_nextTrack.setValue(TrackPointer()); + // The types are registered multiple times although once would be sufficient + qRegisterMetaType(); + // AnalyzerProgress is just an alias/typedef and must be registered explicitly + // by name! + qRegisterMetaType("AnalyzerProgress"); +} + +void AnalyzerThread::doRun() { + std::unique_ptr pAnalysisDao; + if (m_mode != AnalyzerMode::WithBeatsWithoutWaveform) { + pAnalysisDao = std::make_unique(m_pConfig); + m_analyzers.push_back(std::make_unique(pAnalysisDao.get())); + } + if (AnalyzerGain::isEnabled(ReplayGainSettings(m_pConfig))) { + m_analyzers.push_back(std::make_unique(m_pConfig)); + } + if (AnalyzerEbur128::isEnabled(ReplayGainSettings(m_pConfig))) { + m_analyzers.push_back(std::make_unique(m_pConfig)); + } +#ifdef __VAMP__ + const bool enforceBpmDetection = m_mode != AnalyzerMode::Default; + m_analyzers.push_back(std::make_unique(m_pConfig, enforceBpmDetection)); + m_analyzers.push_back(std::make_unique(m_pConfig)); +#endif + DEBUG_ASSERT(!m_analyzers.empty()); + kLogger.debug() << "Activated" << m_analyzers.size() << "analyzers"; + + // This thread-local database connection for pAnalysisDao + // must not be closed before returning from this function. + // Therefore the DbConnectionPooler is defined outside of + // the conditional if block. + mixxx::DbConnectionPooler dbConnectionPooler; + if (pAnalysisDao) { + dbConnectionPooler = mixxx::DbConnectionPooler(m_dbConnectionPool); // 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_dbConnectionPool); + DEBUG_ASSERT(dbConnection.isOpen()); + pAnalysisDao->initialize(dbConnection); + } + + mixxx::AudioSource::OpenParams openParams; + openParams.setChannelCount(kAnalysisChannels); + + while (waitUntilWorkItemsFetched()) { + DEBUG_ASSERT(m_currentTrack); + kLogger.debug() << "Analyzing" << m_currentTrack->getLocation(); + + // Get the audio + const auto audioSource = + SoundSourceProxy(m_currentTrack).openAudioSource(openParams); + if (!audioSource) { + kLogger.warning() + << "Failed to open file for analyzing:" + << m_currentTrack->getLocation(); + emitDoneProgress(kAnalyzerProgressUnknown); + continue; + } + + bool processTrack = false; + for (auto const& analyzer: m_analyzers) { + // Make sure not to short-circuit initialize(...) + if (analyzer->initialize( + m_currentTrack, + audioSource->sampleRate(), + audioSource->frameLength() * kAnalysisChannels)) { + processTrack = true; + } + } + + if (processTrack) { + const auto analysisResult = analyzeAudioSource(audioSource); + DEBUG_ASSERT(analysisResult != AnalysisResult::Pending); + if ((analysisResult == AnalysisResult::Complete) || + (analysisResult == AnalysisResult::Partial)) { + // The analysis has been finished, and is either complete without + // any errors or partial if it has been aborted due to a corrupt + // audio file. In both cases don't reanalyze tracks during this + // session. A partial analysis would otherwise be repeated again + // and again, because it is very unlikely that the error vanishes + // suddenly. + emitBusyProgress(kAnalyzerProgressFinalizing); + // This takes around 3 sec on a Atom Netbook + for (auto const& analyzer: m_analyzers) { + analyzer->finalize(m_currentTrack); + } + emitDoneProgress(kAnalyzerProgressDone); + } else { + for (auto const& analyzer: m_analyzers) { + analyzer->cleanup(m_currentTrack); + } + emitDoneProgress(kAnalyzerProgressUnknown); + } + } else { + kLogger.debug() << "Skipping track analysis because no analyzer initialized."; + emitDoneProgress(kAnalyzerProgressDone); + } + } + DEBUG_ASSERT(isStopping()); + + m_analyzers.clear(); + + emitProgress(AnalyzerThreadState::Exit); +} + +void AnalyzerThread::submitNextTrack(TrackPointer nextTrack) { + DEBUG_ASSERT(!m_nextTrack.getValue()); + m_nextTrack.setValue(std::move(nextTrack)); +} + +WorkerThread::FetchWorkResult AnalyzerThread::tryFetchWorkItems() { + DEBUG_ASSERT(!m_currentTrack); + m_currentTrack = m_nextTrack.getValueOnce(); + if (m_currentTrack) { + return FetchWorkResult::Ready; + } else { + if (m_emittedState != AnalyzerThreadState::Idle) { + // Only send the idle signal once when entering the + // idle state from another state. + emitProgress(AnalyzerThreadState::Idle); + } + return FetchWorkResult::Idle; + } +} + +AnalyzerThread::AnalysisResult AnalyzerThread::analyzeAudioSource( + const mixxx::AudioSourcePointer& audioSource) { + DEBUG_ASSERT(m_currentTrack); + + mixxx::AudioSourceStereoProxy audioSourceProxy( + audioSource, + kAnalysisFramesPerBlock); + DEBUG_ASSERT(audioSourceProxy.channelCount() == kAnalysisChannels); + + // Analysis starts now + emitBusyProgress(kAnalyzerProgressNone); + + mixxx::IndexRange remainingFrames = audioSource->frameIndexRange(); + auto result = remainingFrames.empty() ? AnalysisResult::Complete : AnalysisResult::Pending; + while (result == AnalysisResult::Pending) { + sleepWhileSuspended(); + if (isStopping()) { + return AnalysisResult::Cancelled; + } + + // 1st step: Decode next chunk of audio data + const auto inputFrameIndexRange = + remainingFrames.splitAndShrinkFront( + math_min(kAnalysisFramesPerBlock, remainingFrames.length())); + DEBUG_ASSERT(!inputFrameIndexRange.empty()); + const auto readableSampleFrames = + audioSourceProxy.readSampleFrames( + mixxx::WritableSampleFrames( + inputFrameIndexRange, + mixxx::SampleBuffer::WritableSlice(m_sampleBuffer))); + + sleepWhileSuspended(); + if (isStopping()) { + return AnalysisResult::Cancelled; + } + + // 2nd: step: Analyze chunk of decoded audio data + if (readableSampleFrames.frameLength() == kAnalysisFramesPerBlock) { + // Complete chunk of audio samples has been read for analysis + for (auto const& analyzer: m_analyzers) { + analyzer->process( + readableSampleFrames.readableData(), + readableSampleFrames.readableLength()); + } + if (remainingFrames.empty()) { + result = AnalysisResult::Complete; + } + } else { + // Partial chunk of audio samples has been read. + // This should only happen at the end of an audio stream, + // otherwise a decoding error must have occurred. + if (remainingFrames.empty()) { + result = AnalysisResult::Complete; + } else { + // EOF not reached -> Maybe a corrupt file? + kLogger.warning() + << "Aborting analysis after failure to read sample data:" + << "expected frames =" << inputFrameIndexRange + << ", actual frames =" << readableSampleFrames.frameIndexRange(); + result = AnalysisResult::Partial; + } + } + + // Don't check again for paused/stopped and simply finish the + // current iteration by emitting progress. + + // 3rd step: Update & emit progress + const double frameProgress = + double(audioSource->frameLength() - remainingFrames.length()) / + double(audioSource->frameLength()); + const AnalyzerProgress progress = + frameProgress * + (kAnalyzerProgressFinalizing - kAnalyzerProgressNone); + emitBusyProgress(progress); + } + + return result; +} + +void AnalyzerThread::emitBusyProgress(AnalyzerProgress busyProgress) { + DEBUG_ASSERT(m_currentTrack); + if ((m_emittedState == AnalyzerThreadState::Busy) && + (m_lastBusyProgressEmittedTimer.elapsed() < kBusyProgressInhibitDuration)) { + // Don't emit progress signal while still busy and the + // previous progress signal has been emitted just recently. + // This should keep the host thread responsive and prevents + // to overwhelm it with too frequent progress signals. + return; + } + m_lastBusyProgressEmittedTimer.restart(); + emitProgress(AnalyzerThreadState::Busy, m_currentTrack->getId(), busyProgress); + DEBUG_ASSERT(m_emittedState == AnalyzerThreadState::Busy); +} + +void AnalyzerThread::emitDoneProgress(AnalyzerProgress doneProgress) { + DEBUG_ASSERT(m_currentTrack); + // Release all references of the track before emitting the signal + // to ensure that the last reference is not dropped in this worker + // thread that might trigger database actions! The TrackAnalysisScheduler + // must store a TrackPointer until receiving the Done signal. + TrackId trackId = m_currentTrack->getId(); + m_currentTrack.reset(); + emitProgress(AnalyzerThreadState::Done, trackId, doneProgress); +} + +void AnalyzerThread::emitProgress(AnalyzerThreadState state) { + DEBUG_ASSERT(!m_currentTrack); + emitProgress(state, TrackId(), kAnalyzerProgressUnknown); +} + +void AnalyzerThread::emitProgress(AnalyzerThreadState state, TrackId trackId, AnalyzerProgress trackProgress) { + DEBUG_ASSERT(!m_currentTrack || (state == AnalyzerThreadState::Busy)); + DEBUG_ASSERT(!m_currentTrack || (m_currentTrack->getId() == trackId)); + DEBUG_ASSERT(trackId.isValid() || (trackProgress == kAnalyzerProgressUnknown)); + m_emittedState = state; + emit progress(m_id, m_emittedState, trackId, trackProgress); +} diff --git a/src/analyzer/analyzerthread.h b/src/analyzer/analyzerthread.h new file mode 100644 index 00000000000..fc2d8c201b5 --- /dev/null +++ b/src/analyzer/analyzerthread.h @@ -0,0 +1,143 @@ +#pragma once + +#include + +#include "util/workerthread.h" + +#include "analyzer/analyzerprogress.h" +#include "analyzer/analyzer.h" +#include "control/controlvalue.h" +#include "preferences/usersettings.h" +#include "sources/audiosource.h" +#include "track/track.h" +#include "util/db/dbconnectionpool.h" +#include "util/performancetimer.h" +#include "util/samplebuffer.h" +#include "util/memory.h" + + +enum class AnalyzerMode { + Default, + WithBeats, + WithBeatsWithoutWaveform, +}; + +enum class AnalyzerThreadState { + Void, + Idle, + Busy, + Done, + Exit, +}; + +Q_DECLARE_TYPEINFO(AnalyzerThreadState, Q_MOVABLE_TYPE); +Q_DECLARE_METATYPE(AnalyzerThreadState); + +// Atomic control values are used for transferring data between the +// host and the worker thread, e.g. the next track to be analyzed or +// the current analyzer progress that can be read independent of any +// progress signal +// +// The frequency of progress signal is limited to avoid flooding the +// signal queued connection between the internal worker thread and +// the host, which might otherwise cause unresponsiveness of the host. +class AnalyzerThread : public WorkerThread { + Q_OBJECT + + public: + typedef std::unique_ptr Pointer; + static Pointer nullPointer(); + + static Pointer createInstance( + int id, + mixxx::DbConnectionPoolPtr dbConnectionPool, + UserSettingsPointer pConfig, + AnalyzerMode mode = AnalyzerMode::Default); + + /*private*/ AnalyzerThread( + int id, + mixxx::DbConnectionPoolPtr dbConnectionPool, + UserSettingsPointer pConfig, + AnalyzerMode mode); + ~AnalyzerThread() override = default; + + int id() const { + return m_id; + } + + // Submits the next track to the worker thread without + // blocking. This is only allowed after a progress() signal + // with state Idle has been received to avoid overwriting + // a previously sent track that has not been received by the + // worker thread, yet. + void submitNextTrack(TrackPointer nextTrack); + + signals: + // Use a single signal for progress updates to ensure that all signals + // are queued and received in the same order as emitted from the internal + // worker thread. Different signals would otherwise end up in different + // queued connections which are processed in an undefined order! + // TODO(uklotzde): Encapsulate all signal parameters into an + // AnalyzerThreadProgress object and register it as a new meta type. + void progress(int threadId, AnalyzerThreadState threadState, TrackId trackId, AnalyzerProgress trackProgress); + + protected: + void doRun() override; + + FetchWorkResult tryFetchWorkItems() override; + + private: + ///////////////////////////////////////////////////////////////////////// + // Immutable values and pointers (objects are thread-safe) + const int m_id; + const mixxx::DbConnectionPoolPtr m_dbConnectionPool; + const UserSettingsPointer m_pConfig; + const AnalyzerMode m_mode; + + ///////////////////////////////////////////////////////////////////////// + // Thread-safe atomic values + + // There is only one consumer (namely the worker thread) and one producer + // (the host thread) for this value. Therefore default ring buffer size + // is sufficient. + ControlValueAtomic m_nextTrack; + + ///////////////////////////////////////////////////////////////////////// + // Thread local: Only used in the constructor/destructor and within + // run() by the worker thread. + + typedef std::unique_ptr AnalyzerPtr; + std::vector m_analyzers; + + mixxx::SampleBuffer m_sampleBuffer; + + TrackPointer m_currentTrack; + + AnalyzerThreadState m_emittedState; + + PerformanceTimer m_lastBusyProgressEmittedTimer; + + enum class AnalysisResult { + Pending, + Partial, + Complete, + Cancelled, + }; + AnalysisResult analyzeAudioSource( + const mixxx::AudioSourcePointer& audioSource); + + // Blocks the worker thread until a next track becomes available + TrackPointer receiveNextTrack(); + + // Conditionally emit a progress() signal while busy (frequency is limited) + void emitBusyProgress(AnalyzerProgress busyProgress); + + // Unconditionally emits a progress() signal when done + void emitDoneProgress(AnalyzerProgress doneProgress); + + // Unconditionally emits any kind of progress() signal if not current track is present + void emitProgress(AnalyzerThreadState state); + + // Unconditionally emits any kind of progress() signal + void emitProgress(AnalyzerThreadState state, TrackId trackId, AnalyzerProgress trackProgress); +}; diff --git a/src/analyzer/trackanalysisscheduler.cpp b/src/analyzer/trackanalysisscheduler.cpp new file mode 100644 index 00000000000..b3fd0971b72 --- /dev/null +++ b/src/analyzer/trackanalysisscheduler.cpp @@ -0,0 +1,276 @@ +#include "analyzer/trackanalysisscheduler.h" + +#include "library/library.h" +#include "library/trackcollection.h" + +#include "util/logger.h" + + +namespace { + +mixxx::Logger kLogger("TrackAnalysisScheduler"); + +constexpr QThread::Priority kWorkerThreadPriority = QThread::LowPriority; + +// Maximum frequency of progress updates +constexpr std::chrono::milliseconds kProgressInhibitDuration(100); + +void deleteTrackAnalysisScheduler(TrackAnalysisScheduler* plainPtr) { + if (plainPtr) { + // Trigger stop + plainPtr->stop(); + // Release ownership and let Qt delete the queue later + plainPtr->deleteLater(); + } +} + +} // anonymous namespace + +//static +TrackAnalysisScheduler::Pointer TrackAnalysisScheduler::nullPointer() { + return Pointer(nullptr, [](TrackAnalysisScheduler*){}); +} + +//static +TrackAnalysisScheduler::Pointer TrackAnalysisScheduler::createInstance( + Library* library, + int numWorkerThreads, + const UserSettingsPointer& pConfig, + AnalyzerMode mode) { + return Pointer(new TrackAnalysisScheduler( + library, + numWorkerThreads, + pConfig, + mode), + deleteTrackAnalysisScheduler); +} + +TrackAnalysisScheduler::TrackAnalysisScheduler( + Library* library, + int numWorkerThreads, + const UserSettingsPointer& pConfig, + AnalyzerMode mode) + : m_library(library), + m_currentTrackProgress(kAnalyzerProgressUnknown), + m_currentTrackNumber(0), + m_finishedTracksCount(0), + m_dequeuedTracksCount(0), + // The first signal should always be emitted + m_lastProgressEmittedAt(Clock::now() - kProgressInhibitDuration) { + VERIFY_OR_DEBUG_ASSERT(numWorkerThreads > 0) { + kLogger.warning() + << "Invalid number of worker threads:" + << numWorkerThreads; + } else { + kLogger.debug() + << "Starting" + << numWorkerThreads + << "worker threads"; + } + // 1st pass: Create worker threads + m_workers.reserve(numWorkerThreads); + for (int threadId = 0; threadId < numWorkerThreads; ++threadId) { + m_workers.emplace_back(AnalyzerThread::createInstance( + threadId, + library->dbConnectionPool(), + pConfig, + mode)); + connect(m_workers.back().thread(), SIGNAL(progress(int, AnalyzerThreadState, TrackId, AnalyzerProgress)), + this, SLOT(onWorkerThreadProgress(int, AnalyzerThreadState, TrackId, AnalyzerProgress))); + } + // 2nd pass: Start worker threads in a suspended state + for (const auto& worker: m_workers) { + worker.thread()->suspend(); + worker.thread()->start(kWorkerThreadPriority); + } +} + +TrackAnalysisScheduler::~TrackAnalysisScheduler() { + kLogger.debug() << "Destroying"; +} + +void TrackAnalysisScheduler::emitProgressOrFinished() { + // The finished() signal is emitted regardless of when the last + // signal has been emitted + if (allTracksFinished()) { + emit finished(); + return; + } + + const auto now = Clock::now(); + if (now < (m_lastProgressEmittedAt + kProgressInhibitDuration)) { + // Inhibit signal + return; + } + m_lastProgressEmittedAt = now; + + if (allTracksFinished()) { + m_currentTrackProgress = kAnalyzerProgressDone; + } else { + AnalyzerProgress workerProgressSum = 0; + int workerProgressCount = 0; + for (const auto& worker: m_workers) { + const AnalyzerProgress workerProgress = worker.analyzerProgress(); + if (workerProgress >= kAnalyzerProgressNone) { + workerProgressSum += workerProgress; + ++workerProgressCount; + } + } + // The following algorithm/heuristic is used for calculating the + // amortized analysis progress (current track number + current + // track progress) across all worker threads. It results in a + // simple and almost linear progress display when multiple threads + // are running in parallel. It also covers the expected behavior + // for the single-threaded case. The receiver of progress updates + // should not need to know how many threads are actually processing + // tracks concurrently behind the scenes. + if (workerProgressCount > 0) { + DEBUG_ASSERT(kAnalyzerProgressNone == 0); + DEBUG_ASSERT(kAnalyzerProgressDone == 1); + const int inProgressCount = + math_max(1, int(std::ceil(workerProgressSum))); + const AnalyzerProgress currentTrackProgress = + workerProgressSum - std::floor(workerProgressSum); + // The calculation of inProgressCount is only an approximation. + // In some situations the calculated virtual current track number + // = m_finishedTracksCount + inProgressCount exceeds the upper + // bound m_dequeuedTracksCount. Using the minimum of both values + // is an appropriate choice for reporting continuous progress. + const int currentTrackNumber = + math_min(m_finishedTracksCount + inProgressCount, m_dequeuedTracksCount); + // The combination of the values current count (primary) and current + // progress (secondary) should never decrease to avoid confusion + if (m_currentTrackNumber < currentTrackNumber) { + m_currentTrackNumber = currentTrackNumber; + // Unconditional progress update + m_currentTrackProgress = currentTrackProgress; + } else if (m_currentTrackNumber == currentTrackNumber) { + // Conditional progress update if current count didn't change + if (m_currentTrackProgress >= kAnalyzerProgressNone) { + // Current progress should not decrease while the count is constant + m_currentTrackProgress = math_max(m_currentTrackProgress, currentTrackProgress); + } else { + // Initialize current progress + m_currentTrackProgress = currentTrackProgress; + } + } + } else { + if (m_currentTrackNumber < m_finishedTracksCount) { + m_currentTrackNumber = m_finishedTracksCount; + } + } + } + const int totalTracksCount = + m_dequeuedTracksCount + m_queuedTrackIds.size(); + DEBUG_ASSERT(m_finishedTracksCount <= m_currentTrackNumber); + DEBUG_ASSERT(m_currentTrackNumber <= m_dequeuedTracksCount); + DEBUG_ASSERT(m_dequeuedTracksCount <= totalTracksCount); + emit progress( + m_currentTrackProgress, + m_currentTrackNumber, + totalTracksCount); +} + +void TrackAnalysisScheduler::onWorkerThreadProgress(int threadId, AnalyzerThreadState threadState, TrackId trackId, AnalyzerProgress analyzerProgress) { + auto& worker = m_workers.at(threadId); + switch (threadState) { + case AnalyzerThreadState::Void: + break; + case AnalyzerThreadState::Idle: + worker.receiveThreadIdle(); + submitNextTrack(&worker); + break; + case AnalyzerThreadState::Busy: + worker.receiveAnalyzerProgress(trackId, analyzerProgress); + emit trackProgress(trackId, analyzerProgress); + break; + case AnalyzerThreadState::Done: + worker.receiveAnalyzerProgress(trackId, analyzerProgress); + emit trackProgress(trackId, analyzerProgress); + ++m_finishedTracksCount; + DEBUG_ASSERT(m_finishedTracksCount <= m_dequeuedTracksCount); + break; + case AnalyzerThreadState::Exit: + worker.receiveThreadExit(); + DEBUG_ASSERT(!worker); + break; + default: + DEBUG_ASSERT(!"Unhandled signal from worker thread"); + } + emitProgressOrFinished(); +} + +void TrackAnalysisScheduler::scheduleTrackById(TrackId trackId) { + VERIFY_OR_DEBUG_ASSERT(trackId.isValid()) { + qWarning() + << "Cannot schedule track with invalid id" + << trackId; + return; + } + m_queuedTrackIds.push_back(trackId); + // Don't wake up the suspended thread now to avoid race conditions + // if multiple threads are added in a row by calling this function + // multiple times. The caller is responsible to finish the scheduling + // of multiple tracks with resume(). +} + +void TrackAnalysisScheduler::suspend() { + kLogger.debug() << "Suspending"; + for (auto& worker: m_workers) { + worker.suspendThread(); + } +} + +void TrackAnalysisScheduler::resume() { + kLogger.debug() << "Resuming"; + for (auto& worker: m_workers) { + if (worker.threadIdle()) { + submitNextTrack(&worker); + } + worker.resumeThread(); + } +} + +bool TrackAnalysisScheduler::submitNextTrack(Worker* worker) { + DEBUG_ASSERT(worker); + while (!m_queuedTrackIds.empty()) { + TrackId nextTrackId = m_queuedTrackIds.front(); + DEBUG_ASSERT(nextTrackId.isValid()); + TrackPointer nextTrack = loadTrackById(nextTrackId); + if (!nextTrack) { + // Skip unloadable track + m_queuedTrackIds.pop_front(); + ++m_dequeuedTracksCount; + ++m_finishedTracksCount; + continue; + } + worker->submitNextTrack(std::move(nextTrack)); + m_queuedTrackIds.pop_front(); + ++m_dequeuedTracksCount; + worker->wakeThread(); + return true; + } + DEBUG_ASSERT(m_finishedTracksCount <= m_dequeuedTracksCount); + return false; +} + +void TrackAnalysisScheduler::stop() { + kLogger.debug() << "Stopping"; + for (auto& worker: m_workers) { + worker.stopThread(); + } +} + +TrackPointer TrackAnalysisScheduler::loadTrackById(TrackId trackId) { + VERIFY_OR_DEBUG_ASSERT(trackId.isValid()) { + return TrackPointer(); + } + TrackPointer track = + m_library->trackCollection().getTrackDAO().getTrack(trackId); + if (!track) { + kLogger.warning() + << "Failed to load track with id" + << trackId; + } + return track; +} diff --git a/src/analyzer/trackanalysisscheduler.h b/src/analyzer/trackanalysisscheduler.h new file mode 100644 index 00000000000..beb50e9a107 --- /dev/null +++ b/src/analyzer/trackanalysisscheduler.h @@ -0,0 +1,174 @@ +#pragma once + +#include +#include + +#include "analyzer/analyzerthread.h" + +#include "util/memory.h" + + +// forward declaration(s) +class Library; + +class TrackAnalysisScheduler : public QObject { + Q_OBJECT + + public: + typedef std::unique_ptr Pointer; + static Pointer nullPointer(); + + static Pointer createInstance( + Library* library, + int numWorkerThreads, + const UserSettingsPointer& pConfig, + AnalyzerMode mode = AnalyzerMode::Default); + + /*private*/ TrackAnalysisScheduler( + Library* library, + int numWorkerThreads, + const UserSettingsPointer& pConfig, + AnalyzerMode mode); + ~TrackAnalysisScheduler() override; + + // Stops a running analysis and discards all enqueued tracks. + void stop(); + + public slots: + // Schedule tracks one by one. After all tracks have been scheduled + // the caller must invoke resume() once. + void scheduleTrackById(TrackId trackId); + + void suspend(); + + // After scheduling tracks the analysis must be resumed once. + // Resume must also be called after suspending the analysis. + void resume(); + + signals: + // Progress for individual tracks is passed-through from the workers + void trackProgress(TrackId trackId, AnalyzerProgress analyzerProgress); + // Current average progress for all scheduled tracks and from all workers + void progress(AnalyzerProgress currentTrackProgress, int currentTrackNumber, int totalTracksCount); + void finished(); + + private slots: + void onWorkerThreadProgress(int threadId, AnalyzerThreadState threadState, TrackId trackId, AnalyzerProgress analyzerProgress); + + private: + // Owns an analyzer thread and buffers the most recent progress update + // received from this thread during analysis. It does not need to be + // thread-safe, because all functions are invoked from the host thread + // that runs the TrackAnalysisScheduler. + class Worker { + public: + explicit Worker(AnalyzerThread::Pointer thread = AnalyzerThread::nullPointer()) + : m_thread(std::move(thread)), + m_threadIdle(false), + m_analyzerProgress(kAnalyzerProgressUnknown) { + } + Worker(const Worker&) = delete; + Worker(Worker&&) = default; + + operator bool() const { + return static_cast(m_thread); + } + + AnalyzerThread* thread() const { + DEBUG_ASSERT(m_thread); + return m_thread.get(); + } + + bool threadIdle() const { + DEBUG_ASSERT(m_thread); + return m_threadIdle; + } + + AnalyzerProgress analyzerProgress() const { + return m_analyzerProgress; + } + + void submitNextTrack(TrackPointer track) { + DEBUG_ASSERT(track); + DEBUG_ASSERT(m_thread); + DEBUG_ASSERT(m_threadIdle); + m_thread->submitNextTrack(std::move(track)); + m_threadIdle = false; + } + + void wakeThread() { + if (m_thread) { + m_thread->wake(); + } + } + + void suspendThread() { + if (m_thread) { + m_thread->suspend(); + } + } + + void resumeThread() { + if (m_thread) { + m_thread->resume(); + } + } + + void stopThread() { + if (m_thread) { + m_thread->stop(); + } + } + + void receiveThreadIdle() { + DEBUG_ASSERT(m_thread); + DEBUG_ASSERT(!m_threadIdle); + m_threadIdle = true; + m_analyzerProgress = kAnalyzerProgressUnknown; + } + + void receiveAnalyzerProgress(TrackId /*trackId*/, AnalyzerProgress analyzerProgress) { + DEBUG_ASSERT(m_thread); + DEBUG_ASSERT(!m_threadIdle); + m_analyzerProgress = analyzerProgress; + } + + void receiveThreadExit() { + DEBUG_ASSERT(m_thread); + m_thread.reset(); + m_threadIdle = false; + m_analyzerProgress = kAnalyzerProgressUnknown; + } + + private: + AnalyzerThread::Pointer m_thread; + bool m_threadIdle; + AnalyzerProgress m_analyzerProgress; + }; + + TrackPointer loadTrackById(TrackId trackId); + bool submitNextTrack(Worker* worker); + void emitProgressOrFinished(); + + bool allTracksFinished() const { + DEBUG_ASSERT(m_finishedTracksCount <= m_dequeuedTracksCount); + return m_queuedTrackIds.empty() && (m_finishedTracksCount == m_dequeuedTracksCount); + } + + Library* m_library; + + std::vector m_workers; + + std::deque m_queuedTrackIds; + + AnalyzerProgress m_currentTrackProgress; + + int m_currentTrackNumber; + + int m_finishedTracksCount; + + int m_dequeuedTracksCount; + + typedef std::chrono::steady_clock Clock; + Clock::time_point m_lastProgressEmittedAt; +}; diff --git a/src/control/controlvalue.h b/src/control/controlvalue.h index 7a13247f16e..48be063a1df 100644 --- a/src/control/controlvalue.h +++ b/src/control/controlvalue.h @@ -1,17 +1,16 @@ -#ifndef CONTROLVALUE_H -#define CONTROLVALUE_H +#pragma once #include #include #include -#include "util/compatibility.h" #include "util/assert.h" +#include "util/compatibility.h" // for lock free access, this value has to be >= the number of value using threads // value must be a fraction of an integer -const int cRingSize = 8; +const int cDefaultRingSize = 8; // there are basically unlimited readers allowed at each ring element // but we have to count them so max() is just fine. // NOTE(rryan): Wrapping max with parentheses avoids conflict with the max macro @@ -25,22 +24,45 @@ const int cReaderSlotCnt = (std::numeric_limits::max)(); // readers or a write is occurring. A write to the value will fail if // m_readerSlots is not equal to cReaderSlotCnt (e.g. there is an active // reader). -template +template class ControlRingValue { public: - ControlRingValue() - : m_value(T()), - m_readerSlots(cReaderSlotCnt) { + ControlRingValue() : m_readerSlots(cReaderSlotCnt) { } + // Tries to copy the stored value if a reader slot is available. + // This is operation can be repeated multiple times for the same + // slot, because the stored value is preserved. bool tryGet(T* value) const { // Read while consuming one readerSlot - bool hasSlot = (m_readerSlots.fetchAndAddAcquire(-1) > 0); - if (hasSlot) { + if (m_readerSlots.fetchAndAddAcquire(-1) > 0) { *value = m_value; + m_readerSlots.fetchAndAddRelease(1); + return true; + } else { + // Otherwise a writer is active. The writer will reset + // the counter in m_readerSlots when releasing the lock + // and we must not re-add the substracted value here! + return false; + } + } + + // A destructive read operation that tries to move the stored + // value into the provided argument if a reader slot is available. + // This is operation should not be repeated once it returned true, + // because the stored value becomes invalid after it has been moved. + bool tryGetOnce(T* value) { + // Read while consuming one readerSlot + if (m_readerSlots.fetchAndAddAcquire(-1) > 0) { + *value = std::move(m_value); + m_readerSlots.fetchAndAddRelease(1); + return true; + } else { + // Otherwise a writer is active. The writer will reset + // the counter in m_readerSlots when releasing the lock + // and we must not re-add the substracted value here! + return false; } - (void)m_readerSlots.fetchAndAddRelease(1); - return hasSlot; } bool trySet(const T& value) { @@ -49,9 +71,10 @@ class ControlRingValue { m_value = value; m_readerSlots.fetchAndAddRelease(cReaderSlotCnt); return true; + } else { + return false; } - return false; - } + } private: T m_value; @@ -64,13 +87,13 @@ class ControlRingValue { // ring-buffer of ControlRingValues and a read pointer and write pointer to // provide getValue()/setValue() methods which *sacrifice perfect consistency* // for the benefit of wait-free read/write access to a value. -template +template class ControlValueAtomicBase { public: inline T getValue() const { - T value = T(); - unsigned int index = static_cast(load_atomic(m_readIndex)) % (cRingSize); - while (m_ring[index].tryGet(&value) == false) { + T value; + unsigned int index = static_cast(load_atomic(m_readIndex)) % cRingSize; + while (!m_ring[index].tryGet(&value)) { // We are here if // 1) there are more then cReaderSlotCnt reader (get) reading the same value or // 2) the formerly current value is locked by a writer @@ -79,7 +102,24 @@ class ControlValueAtomicBase { // m_currentIndex and in the mean while a reader locks the formally current value // because it has written cRingSize times. Reading the less recent value will fix // it because it is now actually the current value. - index = (index - 1) % (cRingSize); + index = (index - 1) % cRingSize; + } + return value; + } + + inline T getValueOnce() { + T value; + unsigned int index = static_cast(load_atomic(m_readIndex)) % cRingSize; + while (!m_ring[index].tryGetOnce(&value)) { + // We are here if + // 1) there are more then cReaderSlotCnt reader (get) reading the same value or + // 2) the formerly current value is locked by a writer + // Case 1 does not happen because we have enough (0x7fffffff) reader slots. + // Case 2 happens when the a reader is delayed after reading the + // m_currentIndex and in the mean while a reader locks the formally current value + // because it has written cRingSize times. Reading the less recent value will fix + // it because it is now actually the current value. + index = (index - 1) % cRingSize; } return value; } @@ -89,20 +129,17 @@ class ControlValueAtomicBase { // This test is const and will be mad only at compile time unsigned int index; do { - index = (unsigned int)m_writeIndex.fetchAndAddAcquire(1) - % (cRingSize); + index = static_cast(m_writeIndex.fetchAndAddAcquire(1)) % cRingSize; // This will be repeated if the value is locked // 1) by another writer writing at the same time or // 2) a delayed reader is still blocking the formerly current value // In both cases writing to the next value will fix it. } while (!m_ring[index].trySet(value)); - m_readIndex = (int)index; + m_readIndex = index; } protected: - ControlValueAtomicBase() - : m_readIndex(0), - m_writeIndex(1) { + ControlValueAtomicBase() : m_readIndex(0), m_writeIndex(1) { // NOTE(rryan): Wrapping max with parentheses avoids conflict with the // max macro defined in windows.h. DEBUG_ASSERT(((std::numeric_limits::max)() % cRingSize) == (cRingSize - 1)); @@ -119,25 +156,27 @@ class ControlValueAtomicBase { // Specialized template for types that are deemed to be atomic on the target // architecture. Instead of using a read/write ring to guarantee atomicity, // direct assignment/read of an aligned member variable is used. -template +template class ControlValueAtomicBase { public: inline T getValue() const { return m_value; } + inline T getValueOnce() { + return std::move(m_value); + } + inline void setValue(const T& value) { m_value = value; } protected: - ControlValueAtomicBase() - : m_value(T()) { - } + ControlValueAtomicBase() = default; private: #if defined(__GNUC__) - T m_value __attribute__ ((aligned(sizeof(void*)))); + T m_value __attribute__((aligned(sizeof(void*)))); #elif defined(_MSC_VER) #ifdef _WIN64 T __declspec(align(8)) m_value; @@ -154,14 +193,8 @@ class ControlValueAtomicBase { // ControlValueAtomicBase to use. For types where sizeof(T) <= sizeof(void*), // the specialized implementation of ControlValueAtomicBase for types that are // atomic on the architecture is used. -template -class ControlValueAtomic - : public ControlValueAtomicBase { +template +class ControlValueAtomic : public ControlValueAtomicBase { public: - - ControlValueAtomic() - : ControlValueAtomicBase() { - } + ControlValueAtomic() = default; }; - -#endif /* CONTROLVALUE_H */ diff --git a/src/library/analysisfeature.cpp b/src/library/analysisfeature.cpp index a56279a2efa..bf51122c73f 100644 --- a/src/library/analysisfeature.cpp +++ b/src/library/analysisfeature.cpp @@ -13,23 +13,36 @@ #include "library/dlganalysis.h" #include "widget/wlibrary.h" #include "controllers/keyboard/keyboardeventfilter.h" -#include "analyzer/analyzerqueue.h" #include "sources/soundsourceproxy.h" #include "util/dnd.h" #include "util/debug.h" const QString AnalysisFeature::m_sAnalysisViewName = QString("Analysis"); -AnalysisFeature::AnalysisFeature(Library* parent, - UserSettingsPointer pConfig, - TrackCollection* pTrackCollection) : - LibraryFeature(parent), +namespace { + +// Utilize all available cores for batch analysis of tracks +const int kNumberOfAnalyzerThreads = math_max(1, QThread::idealThreadCount()); + +inline +AnalyzerMode getAnalyzerMode( + const UserSettingsPointer& pConfig) { + if (pConfig->getValue(ConfigKey("[Library]", "EnableWaveformGenerationWithAnalysis"), true)) { + return AnalyzerMode::WithBeats; + } else { + return AnalyzerMode::WithBeatsWithoutWaveform; + } +} + +} // anonymous namespace + +AnalysisFeature::AnalysisFeature( + Library* parent, + UserSettingsPointer pConfig) + : LibraryFeature(parent), + m_library(parent), m_pConfig(pConfig), - m_pLibrary(parent), - m_pDbConnectionPool(parent->dbConnectionPool()), - m_pTrackCollection(pTrackCollection), - m_pAnalyzerQueue(nullptr), - m_iOldBpmEnabled(0), + m_pTrackAnalysisScheduler(TrackAnalysisScheduler::nullPointer()), m_analysisTitleName(tr("Analyze")), m_pAnalysisView(nullptr) { setTitleDefault(); @@ -38,20 +51,18 @@ AnalysisFeature::AnalysisFeature(Library* parent, AnalysisFeature::~AnalysisFeature() { // TODO(XXX) delete these //delete m_pLibraryTableModel; - cleanupAnalyzer(); } - void AnalysisFeature::setTitleDefault() { m_Title = m_analysisTitleName; emit(featureIsLoading(this, false)); } -void AnalysisFeature::setTitleProgress(int trackNum, int totalNum) { +void AnalysisFeature::setTitleProgress(int currentTrackNumber, int totalTracksCount) { m_Title = QString("%1 (%2 / %3)") .arg(m_analysisTitleName) - .arg(QString::number(trackNum)) - .arg(QString::number(totalNum)); + .arg(QString::number(currentTrackNumber)) + .arg(QString::number(totalTracksCount)); emit(featureIsLoading(this, false)); } @@ -67,8 +78,7 @@ void AnalysisFeature::bindWidget(WLibrary* libraryWidget, KeyboardEventFilter* keyboard) { m_pAnalysisView = new DlgAnalysis(libraryWidget, m_pConfig, - m_pLibrary, - m_pTrackCollection); + m_library); connect(m_pAnalysisView, SIGNAL(loadTrack(TrackPointer)), this, SIGNAL(loadTrack(TrackPointer))); connect(m_pAnalysisView, SIGNAL(loadTrackToPlayer(TrackPointer, QString)), @@ -82,15 +92,12 @@ void AnalysisFeature::bindWidget(WLibrary* libraryWidget, this, SIGNAL(trackSelected(TrackPointer))); connect(this, SIGNAL(analysisActive(bool)), - m_pAnalysisView, SLOT(analysisActive(bool))); - connect(this, SIGNAL(trackAnalysisStarted(int)), - m_pAnalysisView, SLOT(trackAnalysisStarted(int))); + m_pAnalysisView, SLOT(slotAnalysisActive(bool))); m_pAnalysisView->installEventFilter(keyboard); // Let the DlgAnalysis know whether or not analysis is active. - bool bAnalysisActive = m_pAnalyzerQueue != NULL; - emit(analysisActive(bAnalysisActive)); + emit(analysisActive(static_cast(m_pTrackAnalysisScheduler))); libraryWidget->registerView(m_sAnalysisViewName, m_pAnalysisView); } @@ -114,88 +121,80 @@ void AnalysisFeature::activate() { emit(enableCoverArtDisplay(true)); } -namespace { - inline - AnalyzerQueue::Mode getAnalyzerQueueMode( - const UserSettingsPointer& pConfig) { - if (pConfig->getValue(ConfigKey("[Library]", "EnableWaveformGenerationWithAnalysis"), true)) { - return AnalyzerQueue::Mode::Default; - } else { - return AnalyzerQueue::Mode::WithoutWaveform; - } - } -} // anonymous namespace - void AnalysisFeature::analyzeTracks(QList trackIds) { - if (m_pAnalyzerQueue == NULL) { - // Save the old BPM detection prefs setting (on or off) - m_iOldBpmEnabled = m_pConfig->getValueString(ConfigKey("[BPM]","BPMDetectionEnabled")).toInt(); - // Force BPM detection to be on. - m_pConfig->set(ConfigKey("[BPM]","BPMDetectionEnabled"), ConfigValue(1)); - // Note: this sucks... we should refactor the prefs/analyzer to fix this hacky bit ^^^^. - - m_pAnalyzerQueue = new AnalyzerQueue( - m_pDbConnectionPool, + if (!m_pTrackAnalysisScheduler) { + m_pTrackAnalysisScheduler = TrackAnalysisScheduler::createInstance( + m_library, + kNumberOfAnalyzerThreads, m_pConfig, - getAnalyzerQueueMode(m_pConfig)); + getAnalyzerMode(m_pConfig)); - connect(m_pAnalyzerQueue, SIGNAL(trackProgress(int)), - m_pAnalysisView, SLOT(trackAnalysisProgress(int))); - connect(m_pAnalyzerQueue, SIGNAL(trackFinished(int)), - this, SLOT(slotProgressUpdate(int))); - connect(m_pAnalyzerQueue, SIGNAL(trackFinished(int)), - m_pAnalysisView, SLOT(trackAnalysisFinished(int))); + connect(m_pTrackAnalysisScheduler.get(), SIGNAL(progress(AnalyzerProgress, int, int)), + m_pAnalysisView, SLOT(onTrackAnalysisSchedulerProgress(AnalyzerProgress, int, int))); + connect(m_pTrackAnalysisScheduler.get(), SIGNAL(progress(AnalyzerProgress, int, int)), + this, SLOT(onTrackAnalysisSchedulerProgress(AnalyzerProgress, int, int))); + connect(m_pTrackAnalysisScheduler.get(), SIGNAL(finished()), + this, SLOT(stopAnalysis())); - connect(m_pAnalyzerQueue, SIGNAL(queueEmpty()), - this, SLOT(cleanupAnalyzer())); emit(analysisActive(true)); } for (const auto& trackId: trackIds) { - TrackPointer pTrack = m_pTrackCollection->getTrackDAO().getTrack(trackId); - if (pTrack) { - //qDebug() << this << "Queueing track for analysis" << pTrack->getLocation(); - m_pAnalyzerQueue->queueAnalyseTrack(pTrack); + if (trackId.isValid()) { + m_pTrackAnalysisScheduler->scheduleTrackById(trackId); } } - if (trackIds.size() > 0) { - setTitleProgress(0, trackIds.size()); + m_pTrackAnalysisScheduler->resume(); +} + +void AnalysisFeature::onTrackAnalysisSchedulerProgress( + AnalyzerProgress /*currentTrackProgress*/, + int currentTrackNumber, + int totalTracksCount) { + // Ignore any delayed progress updates after the analysis + // has already been stopped. + if (m_pTrackAnalysisScheduler) { + if (totalTracksCount > 0) { + setTitleProgress(currentTrackNumber, totalTracksCount); + } else { + setTitleDefault(); + } } - emit(trackAnalysisStarted(trackIds.size())); } -void AnalysisFeature::slotProgressUpdate(int num_left) { - int num_tracks = m_pAnalysisView->getNumTracks(); - if (num_left > 0) { - int currentTrack = num_tracks - num_left + 1; - setTitleProgress(currentTrack, num_tracks); +void AnalysisFeature::suspendAnalysis() { + //qDebug() << this << "suspendAnalysis"; + if (m_pTrackAnalysisScheduler) { + m_pTrackAnalysisScheduler->suspend(); } } -void AnalysisFeature::stopAnalysis() { - //qDebug() << this << "stopAnalysis()"; - if (m_pAnalyzerQueue != NULL) { - m_pAnalyzerQueue->stop(); +void AnalysisFeature::resumeAnalysis() { + //qDebug() << this << "resumeAnalysis"; + if (m_pTrackAnalysisScheduler) { + m_pTrackAnalysisScheduler->resume(); } } -void AnalysisFeature::cleanupAnalyzer() { +void AnalysisFeature::stopAnalysis() { + //qDebug() << this << "stopAnalysis()"; + if (m_pTrackAnalysisScheduler) { + // Free resources by abandoning the queue after the batch analysis + // has completed. Batch analysis are not started very frequently + // during a session and should be avoided while performing live. + // If the user decides to start a new batch analysis the setup costs + // for creating the queue with its worker threads are acceptable. + m_pTrackAnalysisScheduler.reset(); + } setTitleDefault(); emit(analysisActive(false)); - if (m_pAnalyzerQueue != NULL) { - m_pAnalyzerQueue->stop(); - m_pAnalyzerQueue->deleteLater(); - m_pAnalyzerQueue = NULL; - // Restore old BPM detection setting for preferences... - m_pConfig->set(ConfigKey("[BPM]","BPMDetectionEnabled"), ConfigValue(m_iOldBpmEnabled)); - } } bool AnalysisFeature::dropAccept(QList urls, QObject* pSource) { Q_UNUSED(pSource); QList files = DragAndDropHelper::supportedTracksFromUrls(urls, false, true); // Adds track, does not insert duplicates, handles unremoving logic. - QList trackIds = m_pTrackCollection->getTrackDAO().addMultipleTracks(files, true); + QList trackIds = m_library->trackCollection().getTrackDAO().addMultipleTracks(files, true); analyzeTracks(trackIds); return trackIds.size() > 0; } diff --git a/src/library/analysisfeature.h b/src/library/analysisfeature.h index 74d6480b8c5..4af45da37e2 100644 --- a/src/library/analysisfeature.h +++ b/src/library/analysisfeature.h @@ -15,19 +15,17 @@ #include "library/libraryfeature.h" #include "library/dlganalysis.h" #include "library/treeitemmodel.h" +#include "analyzer/trackanalysisscheduler.h" #include "preferences/usersettings.h" -#include "util/db/dbconnectionpool.h" class Library; class TrackCollection; -class AnalyzerQueue; class AnalysisFeature : public LibraryFeature { Q_OBJECT public: AnalysisFeature(Library* parent, - UserSettingsPointer pConfig, - TrackCollection* pTrackCollection); + UserSettingsPointer pConfig); virtual ~AnalysisFeature(); QVariant title(); @@ -43,16 +41,17 @@ class AnalysisFeature : public LibraryFeature { signals: void analysisActive(bool bActive); - void trackAnalysisStarted(int size); public slots: void activate(); void analyzeTracks(QList trackIds); + void suspendAnalysis(); + void resumeAnalysis(); + private slots: - void slotProgressUpdate(int num_left); + void onTrackAnalysisSchedulerProgress(AnalyzerProgress currentTrackProgress, int currentTrackNumber, int totalTracksCount); void stopAnalysis(); - void cleanupAnalyzer(); private: // Sets the title of this feature to the default name, given by @@ -62,15 +61,13 @@ class AnalysisFeature : public LibraryFeature { // Sets the title of this feature to the default name followed by (x / y) // where x is the current track being analyzed and y is the total number of // tracks in the job - void setTitleProgress(int trackNum, int totalNum); + void setTitleProgress(int currentTrackNumber, int totalTracksCount); + + Library* m_library; UserSettingsPointer m_pConfig; - Library* m_pLibrary; - mixxx::DbConnectionPoolPtr m_pDbConnectionPool; - TrackCollection* m_pTrackCollection; - AnalyzerQueue* m_pAnalyzerQueue; - // Used to temporarily enable BPM detection in the prefs before we analyse - int m_iOldBpmEnabled; + TrackAnalysisScheduler::Pointer m_pTrackAnalysisScheduler; + // The title returned by title() QVariant m_Title; TreeItemModel m_childModel; diff --git a/src/library/dlganalysis.cpp b/src/library/dlganalysis.cpp index 8279c0c286b..7f85e59c846 100644 --- a/src/library/dlganalysis.cpp +++ b/src/library/dlganalysis.cpp @@ -3,27 +3,25 @@ #include "widget/wwidget.h" #include "widget/wskincolor.h" #include "widget/wanalysislibrarytableview.h" +#include "analyzer/analyzerprogress.h" #include "library/dao/trackschema.h" #include "library/trackcollection.h" #include "library/dlganalysis.h" #include "library/library.h" #include "util/assert.h" -DlgAnalysis::DlgAnalysis(QWidget* pParent, +DlgAnalysis::DlgAnalysis(QWidget* parent, UserSettingsPointer pConfig, - Library* pLibrary, - TrackCollection* pTrackCollection) - : QWidget(pParent), + Library* pLibrary) + : QWidget(parent), m_pConfig(pConfig), - m_pTrackCollection(pTrackCollection), - m_bAnalysisActive(false), - m_tracksInQueue(0), - m_currentTrack(0) { + m_pTrackCollection(&pLibrary->trackCollection()), + m_bAnalysisActive(false) { setupUi(this); m_songsButtonGroup.addButton(radioButtonRecentlyAdded); m_songsButtonGroup.addButton(radioButtonAllSongs); - m_pAnalysisLibraryTableView = new WAnalysisLibraryTableView(this, pConfig, pTrackCollection); + m_pAnalysisLibraryTableView = new WAnalysisLibraryTableView(this, pConfig, m_pTrackCollection); connect(m_pAnalysisLibraryTableView, SIGNAL(loadTrack(TrackPointer)), this, SIGNAL(loadTrack(TrackPointer))); connect(m_pAnalysisLibraryTableView, SIGNAL(loadTrackToPlayer(TrackPointer, QString)), @@ -40,7 +38,7 @@ DlgAnalysis::DlgAnalysis(QWidget* pParent, box->insertWidget(1, m_pAnalysisLibraryTableView); } - m_pAnalysisLibraryTableModel = new AnalysisLibraryTableModel(this, pTrackCollection); + m_pAnalysisLibraryTableModel = new AnalysisLibraryTableModel(this, m_pTrackCollection); m_pAnalysisLibraryTableView->loadTrackModel(m_pAnalysisLibraryTableModel); connect(radioButtonRecentlyAdded, SIGNAL(clicked()), @@ -52,8 +50,6 @@ DlgAnalysis::DlgAnalysis(QWidget* pParent, // started up. Accounts for 0.2% of skin creation time. Get rid of this! radioButtonRecentlyAdded->click(); - labelProgress->setText(""); - pushButtonAnalyze->setEnabled(false); connect(pushButtonAnalyze, SIGNAL(clicked()), this, SLOT(analyze())); @@ -71,9 +67,8 @@ DlgAnalysis::DlgAnalysis(QWidget* pParent, m_pAnalysisLibraryTableView, SLOT(setTrackTableRowHeight(int))); connect(pLibrary, SIGNAL(setSelectedClick(bool)), m_pAnalysisLibraryTableView, SLOT(setSelectedClick(bool))); -} -DlgAnalysis::~DlgAnalysis() { + slotAnalysisActive(m_bAnalysisActive); } void DlgAnalysis::onShow() { @@ -143,13 +138,12 @@ void DlgAnalysis::analyze() { trackIds.append(trackId); } } - m_currentTrack = 1; emit(analyzeTracks(trackIds)); } } -void DlgAnalysis::analysisActive(bool bActive) { - qDebug() << this << "analysisActive" << bActive; +void DlgAnalysis::slotAnalysisActive(bool bActive) { + //qDebug() << this << "slotAnalysisActive" << bActive; m_bAnalysisActive = bActive; if (bActive) { pushButtonAnalyze->setEnabled(true); @@ -162,33 +156,28 @@ void DlgAnalysis::analysisActive(bool bActive) { } } -// slot -void DlgAnalysis::trackAnalysisFinished(int size) { - qDebug() << "Analysis finished" << size << "tracks left"; - if (size > 0) { - m_currentTrack = m_tracksInQueue - size + 1; - } -} - -// slot -void DlgAnalysis::trackAnalysisProgress(int progress) { - if (m_bAnalysisActive) { - QString text = tr("Analyzing %1/%2 %3%").arg( - QString::number(m_currentTrack), - QString::number(m_tracksInQueue), - QString::number(progress)); - labelProgress->setText(text); +void DlgAnalysis::onTrackAnalysisSchedulerProgress( + AnalyzerProgress analyzerProgress, int finishedCount, int totalCount) { + //qDebug() << this << "onTrackAnalysisSchedulerProgress" << analyzerProgress << finishedCount << totalCount; + if (labelProgress->isEnabled()) { + QString progressText; + if (analyzerProgress >= kAnalyzerProgressNone) { + QString progressPercent = QString::number( + analyzerProgressPercent(analyzerProgress)); + progressText = tr("Analyzing %1% %2/%3").arg( + progressPercent, + QString::number(finishedCount), + QString::number(totalCount)); + } else { + // Omit to display any percentage + progressText = tr("Analyzing %1/%2").arg( + QString::number(finishedCount), + QString::number(totalCount)); + } + labelProgress->setText(progressText); } } -int DlgAnalysis::getNumTracks() { - return m_tracksInQueue; -} - -void DlgAnalysis::trackAnalysisStarted(int size) { - m_tracksInQueue = size; -} - void DlgAnalysis::showRecentSongs() { m_pAnalysisLibraryTableModel->showRecentSongs(); } diff --git a/src/library/dlganalysis.h b/src/library/dlganalysis.h index f1adea81404..d0241368a3c 100644 --- a/src/library/dlganalysis.h +++ b/src/library/dlganalysis.h @@ -9,6 +9,7 @@ #include "library/libraryview.h" #include "library/trackcollection.h" #include "library/ui_dlganalysis.h" +#include "analyzer/analyzerprogress.h" class AnalysisLibraryTableModel; class WAnalysisLibraryTableView; @@ -19,9 +20,8 @@ class DlgAnalysis : public QWidget, public Ui::DlgAnalysis, public virtual Libra public: DlgAnalysis(QWidget *parent, UserSettingsPointer pConfig, - Library* pLibrary, - TrackCollection* pTrackCollection); - ~DlgAnalysis() override; + Library* pLibrary); + ~DlgAnalysis() override = default; void onSearch(const QString& text) override; void onShow() override; @@ -35,20 +35,17 @@ class DlgAnalysis : public QWidget, public Ui::DlgAnalysis, public virtual Libra inline const QString currentSearch() { return m_pAnalysisLibraryTableModel->currentSearch(); } - int getNumTracks(); public slots: void tableSelectionChanged(const QItemSelection& selected, const QItemSelection& deselected); void selectAll(); void analyze(); - void trackAnalysisFinished(int size); - void trackAnalysisProgress(int progress); - void trackAnalysisStarted(int size); + void slotAnalysisActive(bool bActive); + void onTrackAnalysisSchedulerProgress(AnalyzerProgress analyzerProgress, int finishedCount, int totalCount); void showRecentSongs(); void showAllSongs(); void installEventFilter(QObject* pFilter); - void analysisActive(bool bActive); signals: void loadTrack(TrackPointer pTrack); @@ -65,8 +62,6 @@ class DlgAnalysis : public QWidget, public Ui::DlgAnalysis, public virtual Libra QButtonGroup m_songsButtonGroup; WAnalysisLibraryTableView* m_pAnalysisLibraryTableView; AnalysisLibraryTableModel* m_pAnalysisLibraryTableModel; - int m_tracksInQueue; - int m_currentTrack; }; #endif //DLGTRIAGE_H diff --git a/src/library/library.cpp b/src/library/library.cpp index c47f813b3c9..0892d5aed1e 100644 --- a/src/library/library.cpp +++ b/src/library/library.cpp @@ -63,7 +63,7 @@ Library::Library( QObject* parent, UserSettingsPointer pConfig, mixxx::DbConnectionPoolPtr pDbConnectionPool, - PlayerManagerInterface* pPlayerManager, + PlayerManager* pPlayerManager, RecordingManager* pRecordingManager) : m_pConfig(pConfig), m_pDbConnectionPool(pDbConnectionPool), @@ -124,12 +124,20 @@ Library::Library( addFeature(browseFeature); addFeature(new RecordingFeature(this, pConfig, m_pTrackCollection, pRecordingManager)); addFeature(new SetlogFeature(this, pConfig, m_pTrackCollection)); - m_pAnalysisFeature = new AnalysisFeature(this, pConfig, m_pTrackCollection); + + m_pAnalysisFeature = new AnalysisFeature(this, pConfig); connect(m_pPlaylistFeature, SIGNAL(analyzeTracks(QList)), m_pAnalysisFeature, SLOT(analyzeTracks(QList))); connect(m_pCrateFeature, SIGNAL(analyzeTracks(QList)), m_pAnalysisFeature, SLOT(analyzeTracks(QList))); addFeature(m_pAnalysisFeature); + // Suspend a batch analysis while an ad-hoc analysis of + // loaded tracks is in progress and resume it afterwards. + connect(pPlayerManager, SIGNAL(trackAnalyzerProgress(TrackId, AnalyzerProgress)), + m_pAnalysisFeature, SLOT(suspendAnalysis())); + connect(pPlayerManager, SIGNAL(trackAnalyzerIdle()), + m_pAnalysisFeature, SLOT(resumeAnalysis())); + //iTunes and Rhythmbox should be last until we no longer have an obnoxious //messagebox popup when you select them. (This forces you to reach for your //mouse or keyboard if you're using MIDI control and you scroll through them...) diff --git a/src/library/library.h b/src/library/library.h index e8e2a01d611..09e1e7e9daa 100644 --- a/src/library/library.h +++ b/src/library/library.h @@ -34,7 +34,7 @@ class PlaylistFeature; class CrateFeature; class LibraryControl; class KeyboardEventFilter; -class PlayerManagerInterface; +class PlayerManager; class Library: public QObject, public virtual /*implements*/ GlobalTrackCacheSaver { @@ -48,7 +48,7 @@ class Library: public QObject, Library(QObject* parent, UserSettingsPointer pConfig, mixxx::DbConnectionPoolPtr pDbConnectionPool, - PlayerManagerInterface* pPlayerManager, + PlayerManager* pPlayerManager, RecordingManager* pRecordingManager); ~Library() override; @@ -56,6 +56,11 @@ class Library: public QObject, return m_pDbConnectionPool; } + TrackCollection& trackCollection() { + DEBUG_ASSERT(m_pTrackCollection); + return *m_pTrackCollection; + } + void bindWidget(WLibrary* libraryWidget, KeyboardEventFilter* pKeyboard); void bindSidebarWidget(WLibrarySidebar* sidebarWidget); diff --git a/src/mixer/basetrackplayer.cpp b/src/mixer/basetrackplayer.cpp index 1a836dcf928..0e97d32af3c 100644 --- a/src/mixer/basetrackplayer.cpp +++ b/src/mixer/basetrackplayer.cpp @@ -13,7 +13,6 @@ #include "engine/enginemaster.h" #include "track/beatgrid.h" #include "waveform/renderers/waveformwidgetrenderer.h" -#include "analyzer/analyzerqueue.h" #include "util/platform.h" #include "util/sandbox.h" #include "effects/effectsmanager.h" diff --git a/src/mixer/playermanager.cpp b/src/mixer/playermanager.cpp index 866c3f7b12b..ec083e2f458 100644 --- a/src/mixer/playermanager.cpp +++ b/src/mixer/playermanager.cpp @@ -4,7 +4,6 @@ #include -#include "analyzer/analyzerqueue.h" #include "control/controlobject.h" #include "control/controlobject.h" #include "effects/effectsmanager.h" @@ -21,9 +20,20 @@ #include "soundio/soundmanager.h" #include "track/track.h" #include "util/assert.h" +#include "util/logger.h" #include "util/stat.h" #include "util/sleepableqthread.h" + +namespace { + +const mixxx::Logger kLogger("PlayerManager"); + +// Utilize half of the available cores for adhoc analysis of tracks +const int kNumberOfAnalyzerThreads = math_max(1, QThread::idealThreadCount() / 2); + +} // anonymous namespace + //static QAtomicPointer PlayerManager::m_pCOPNumDecks; //static @@ -42,7 +52,6 @@ PlayerManager::PlayerManager(UserSettingsPointer pConfig, m_pEngine(pEngine), // NOTE(XXX) LegacySkinParser relies on these controls being Controls // and not ControlProxies. - m_pAnalyzerQueue(nullptr), m_pCONumDecks(new ControlObject( ConfigKey("[Master]", "num_decks"), true, true)), m_pCONumSamplers(new ControlObject( @@ -52,7 +61,8 @@ PlayerManager::PlayerManager(UserSettingsPointer pConfig, m_pCONumMicrophones(new ControlObject( ConfigKey("[Master]", "num_microphones"), true, true)), m_pCONumAuxiliaries(new ControlObject( - ConfigKey("[Master]", "num_auxiliaries"), true, true)) { + ConfigKey("[Master]", "num_auxiliaries"), true, true)), + m_pTrackAnalysisScheduler(TrackAnalysisScheduler::nullPointer()) { m_pCONumDecks->connectValueChangeRequest(this, SLOT(slotChangeNumDecks(double)), Qt::DirectConnection); m_pCONumSamplers->connectValueChangeRequest(this, @@ -69,6 +79,8 @@ PlayerManager::PlayerManager(UserSettingsPointer pConfig, } PlayerManager::~PlayerManager() { + kLogger.debug() << "Destroying"; + QMutexLocker locker(&m_mutex); m_pSamplerBank->saveSamplerBankToPath( @@ -90,9 +102,8 @@ PlayerManager::~PlayerManager() { delete m_pCONumPreviewDecks; delete m_pCONumMicrophones; delete m_pCONumAuxiliaries; - if (m_pAnalyzerQueue) { - delete m_pAnalyzerQueue; - } + + m_pTrackAnalysisScheduler.reset(); } void PlayerManager::bindToLibrary(Library* pLibrary) { @@ -104,27 +115,36 @@ void PlayerManager::bindToLibrary(Library* pLibrary) { connect(this, SIGNAL(loadLocationToPlayer(QString, QString)), pLibrary, SLOT(slotLoadLocationToPlayer(QString, QString))); - m_pAnalyzerQueue = new AnalyzerQueue(pLibrary->dbConnectionPool(), m_pConfig); + DEBUG_ASSERT(!m_pTrackAnalysisScheduler); + m_pTrackAnalysisScheduler = TrackAnalysisScheduler::createInstance( + pLibrary, + kNumberOfAnalyzerThreads, + m_pConfig); + + connect(m_pTrackAnalysisScheduler.get(), SIGNAL(trackProgress(TrackId, AnalyzerProgress)), + this, SLOT(onTrackAnalysisProgress(TrackId, AnalyzerProgress))); + connect(m_pTrackAnalysisScheduler.get(), SIGNAL(finished()), + this, SLOT(onTrackAnalysisFinished())); // Connect the player to the analyzer queue so that loaded tracks are - // analysed. + // analyzed. foreach(Deck* pDeck, m_decks) { connect(pDeck, SIGNAL(newTrackLoaded(TrackPointer)), - m_pAnalyzerQueue, SLOT(slotAnalyseTrack(TrackPointer))); + this, SLOT(slotAnalyzeTrack(TrackPointer))); } // Connect the player to the analyzer queue so that loaded tracks are - // analysed. + // analyzed. foreach(Sampler* pSampler, m_samplers) { connect(pSampler, SIGNAL(newTrackLoaded(TrackPointer)), - m_pAnalyzerQueue, SLOT(slotAnalyseTrack(TrackPointer))); + this, SLOT(slotAnalyzeTrack(TrackPointer))); } // Connect the player to the analyzer queue so that loaded tracks are - // analysed. + // analyzed. foreach(PreviewDeck* pPreviewDeck, m_preview_decks) { connect(pPreviewDeck, SIGNAL(newTrackLoaded(TrackPointer)), - m_pAnalyzerQueue, SLOT(slotAnalyseTrack(TrackPointer))); + this, SLOT(slotAnalyzeTrack(TrackPointer))); } } @@ -244,7 +264,7 @@ void PlayerManager::slotChangeNumDecks(double v) { if (num < m_decks.size()) { // The request was invalid -- reset the value. - qDebug() << "Ignoring request to reduce the number of decks to" << num; + kLogger.debug() << "Ignoring request to reduce the number of decks to" << num; return; } @@ -262,7 +282,7 @@ void PlayerManager::slotChangeNumSamplers(double v) { int num = (int)v; if (num < m_samplers.size()) { // The request was invalid -- don't set the value. - qDebug() << "Ignoring request to reduce the number of samplers to" << num; + kLogger.debug() << "Ignoring request to reduce the number of samplers to" << num; return; } @@ -277,7 +297,7 @@ void PlayerManager::slotChangeNumPreviewDecks(double v) { int num = (int)v; if (num < m_preview_decks.size()) { // The request was invalid -- don't set the value. - qDebug() << "Ignoring request to reduce the number of preview decks to" << num; + kLogger.debug() << "Ignoring request to reduce the number of preview decks to" << num; return; } while (m_preview_decks.size() < num) { @@ -291,7 +311,7 @@ void PlayerManager::slotChangeNumMicrophones(double v) { int num = (int)v; if (num < m_microphones.size()) { // The request was invalid -- don't set the value. - qDebug() << "Ignoring request to reduce the number of microphones to" << num; + kLogger.debug() << "Ignoring request to reduce the number of microphones to" << num; return; } while (m_microphones.size() < num) { @@ -305,7 +325,7 @@ void PlayerManager::slotChangeNumAuxiliaries(double v) { int num = (int)v; if (num < m_auxiliaries.size()) { // The request was invalid -- don't set the value. - qDebug() << "Ignoring request to reduce the number of auxiliaries to" << num; + kLogger.debug() << "Ignoring request to reduce the number of auxiliaries to" << num; return; } while (m_auxiliaries.size() < num) { @@ -345,9 +365,9 @@ void PlayerManager::addDeckInner() { connect(pDeck, SIGNAL(noVinylControlInputConfigured()), this, SIGNAL(noVinylControlInputConfigured())); - if (m_pAnalyzerQueue) { + if (m_pTrackAnalysisScheduler) { connect(pDeck, SIGNAL(newTrackLoaded(TrackPointer)), - m_pAnalyzerQueue, SLOT(slotAnalyseTrack(TrackPointer))); + this, SLOT(slotAnalyzeTrack(TrackPointer))); } m_players[group] = pDeck; @@ -406,9 +426,9 @@ void PlayerManager::addSamplerInner() { Sampler* pSampler = new Sampler(this, m_pConfig, m_pEngine, m_pEffectsManager, orientation, group); - if (m_pAnalyzerQueue) { + if (m_pTrackAnalysisScheduler) { connect(pSampler, SIGNAL(newTrackLoaded(TrackPointer)), - m_pAnalyzerQueue, SLOT(slotAnalyseTrack(TrackPointer))); + this, SLOT(slotAnalyzeTrack(TrackPointer))); } m_players[group] = pSampler; @@ -433,9 +453,9 @@ void PlayerManager::addPreviewDeckInner() { PreviewDeck* pPreviewDeck = new PreviewDeck(this, m_pConfig, m_pEngine, m_pEffectsManager, orientation, group); - if (m_pAnalyzerQueue) { + if (m_pTrackAnalysisScheduler) { connect(pPreviewDeck, SIGNAL(newTrackLoaded(TrackPointer)), - m_pAnalyzerQueue, SLOT(slotAnalyseTrack(TrackPointer))); + this, SLOT(slotAnalyzeTrack(TrackPointer))); } m_players[group] = pPreviewDeck; @@ -484,7 +504,7 @@ BaseTrackPlayer* PlayerManager::getPlayer(QString group) const { Deck* PlayerManager::getDeck(unsigned int deck) const { QMutexLocker locker(&m_mutex); if (deck < 1 || deck > numDecks()) { - qWarning() << "Warning PlayerManager::getDeck() called with invalid index: " + kLogger.warning() << "Warning getDeck() called with invalid index: " << deck; return NULL; } @@ -494,7 +514,7 @@ Deck* PlayerManager::getDeck(unsigned int deck) const { PreviewDeck* PlayerManager::getPreviewDeck(unsigned int libPreviewPlayer) const { QMutexLocker locker(&m_mutex); if (libPreviewPlayer < 1 || libPreviewPlayer > numPreviewDecks()) { - qWarning() << "Warning PlayerManager::getPreviewDeck() called with invalid index: " + kLogger.warning() << "Warning getPreviewDeck() called with invalid index: " << libPreviewPlayer; return NULL; } @@ -504,7 +524,7 @@ PreviewDeck* PlayerManager::getPreviewDeck(unsigned int libPreviewPlayer) const Sampler* PlayerManager::getSampler(unsigned int sampler) const { QMutexLocker locker(&m_mutex); if (sampler < 1 || sampler > numSamplers()) { - qWarning() << "Warning PlayerManager::getSampler() called with invalid index: " + kLogger.warning() << "Warning getSampler() called with invalid index: " << sampler; return NULL; } @@ -514,7 +534,7 @@ Sampler* PlayerManager::getSampler(unsigned int sampler) const { Microphone* PlayerManager::getMicrophone(unsigned int microphone) const { QMutexLocker locker(&m_mutex); if (microphone < 1 || microphone >= static_cast(m_microphones.size())) { - qWarning() << "Warning PlayerManager::getMicrophone() called with invalid index: " + kLogger.warning() << "Warning getMicrophone() called with invalid index: " << microphone; return NULL; } @@ -524,7 +544,7 @@ Microphone* PlayerManager::getMicrophone(unsigned int microphone) const { Auxiliary* PlayerManager::getAuxiliary(unsigned int auxiliary) const { QMutexLocker locker(&m_mutex); if (auxiliary < 1 || auxiliary > static_cast(m_auxiliaries.size())) { - qWarning() << "Warning PlayerManager::getAuxiliary() called with invalid index: " + kLogger.warning() << "Warning getAuxiliary() called with invalid index: " << auxiliary; return NULL; } @@ -537,7 +557,7 @@ void PlayerManager::slotLoadTrackToPlayer(TrackPointer pTrack, QString group, bo BaseTrackPlayer* pPlayer = getPlayer(group); if (pPlayer == NULL) { - qWarning() << "Invalid group argument " << group << " to slotLoadTrackToPlayer."; + kLogger.warning() << "Invalid group argument " << group << " to slotLoadTrackToPlayer."; return; } @@ -596,3 +616,25 @@ void PlayerManager::slotLoadTrackIntoNextAvailableSampler(TrackPointer pTrack) { ++it; } } + +void PlayerManager::slotAnalyzeTrack(TrackPointer track) { + VERIFY_OR_DEBUG_ASSERT(track) { + return; + } + if (m_pTrackAnalysisScheduler) { + m_pTrackAnalysisScheduler->scheduleTrackById(track->getId()); + m_pTrackAnalysisScheduler->resume(); + // The first progress signal will suspend a running batch analysis + // until all loaded tracks have been analyzed. Emit it once just now + // before any signals from the analyzer queue arrive. + emit trackAnalyzerProgress(track->getId(), kAnalyzerProgressUnknown); + } +} + +void PlayerManager::onTrackAnalysisProgress(TrackId trackId, AnalyzerProgress analyzerProgress) { + emit trackAnalyzerProgress(trackId, analyzerProgress); +} + +void PlayerManager::onTrackAnalysisFinished() { + emit trackAnalyzerIdle(); +} diff --git a/src/mixer/playermanager.h b/src/mixer/playermanager.h index 98d8ba60ef7..a95f2298e05 100644 --- a/src/mixer/playermanager.h +++ b/src/mixer/playermanager.h @@ -9,10 +9,10 @@ #include #include +#include "analyzer/trackanalysisscheduler.h" #include "preferences/usersettings.h" #include "track/track.h" -class AnalyzerQueue; class Auxiliary; class BaseTrackPlayer; class ControlObject; @@ -136,8 +136,8 @@ class PlayerManager : public QObject, public PlayerManagerInterface { // Get the auxiliary by its number. Auxiliaries are numbered starting with 1. Auxiliary* getAuxiliary(unsigned int auxiliary) const; - // Binds signals between PlayerManager and Library. Does not store a pointer - // to the Library. + // Binds signals between PlayerManager and Library. The library + // must exist at least for the lifetime of this instance. void bindToLibrary(Library* pLibrary); // Returns the group for the ith sampler where i is zero indexed @@ -199,6 +199,12 @@ class PlayerManager : public QObject, public PlayerManagerInterface { void slotChangeNumMicrophones(double v); void slotChangeNumAuxiliaries(double v); + private slots: + void slotAnalyzeTrack(TrackPointer track); + + void onTrackAnalysisProgress(TrackId trackId, AnalyzerProgress analyzerProgress); + void onTrackAnalysisFinished(); + signals: void loadLocationToPlayer(QString location, QString group); @@ -217,6 +223,9 @@ class PlayerManager : public QObject, public PlayerManagerInterface { // Emitted when the number of decks changes. void numberOfDecksChanged(int decks); + void trackAnalyzerProgress(TrackId trackId, AnalyzerProgress analyzerProgress); + void trackAnalyzerIdle(); + private: TrackPointer lookupTrack(QString location); // Must hold m_mutex before calling this method. Internal method that @@ -243,13 +252,14 @@ class PlayerManager : public QObject, public PlayerManagerInterface { EffectsManager* m_pEffectsManager; EngineMaster* m_pEngine; SamplerBank* m_pSamplerBank; - AnalyzerQueue* m_pAnalyzerQueue; ControlObject* m_pCONumDecks; ControlObject* m_pCONumSamplers; ControlObject* m_pCONumPreviewDecks; ControlObject* m_pCONumMicrophones; ControlObject* m_pCONumAuxiliaries; + TrackAnalysisScheduler::Pointer m_pTrackAnalysisScheduler; + QList m_decks; QList m_samplers; QList m_preview_decks; diff --git a/src/mixxx.cpp b/src/mixxx.cpp index 69903ffd228..c4fcbe2c5e9 100644 --- a/src/mixxx.cpp +++ b/src/mixxx.cpp @@ -25,7 +25,6 @@ #include #include -#include "analyzer/analyzerqueue.h" #include "dialog/dlgabout.h" #include "preferences/dialog/dlgpreferences.h" #include "preferences/dialog/dlgprefeq.h" diff --git a/src/preferences/replaygainsettings.cpp b/src/preferences/replaygainsettings.cpp index ec8c9792b53..8168d5ff142 100644 --- a/src/preferences/replaygainsettings.cpp +++ b/src/preferences/replaygainsettings.cpp @@ -83,13 +83,14 @@ void ReplayGainSettings::setReplayGainReanalyze(bool value) { ConfigValue(value)); } -bool ReplayGainSettings::isAnalyzerDisabled(int version, TrackPointer tio) const { - int prefversion = getReplayGainAnalyzerVersion(); - bool analyzerEnabled = getReplayGainAnalyzerEnabled() && (version == prefversion); - bool reanalyze = getReplayGainReanalyze(); +bool ReplayGainSettings::isAnalyzerEnabled(int version) const { + return getReplayGainAnalyzerEnabled() + && (version == getReplayGainAnalyzerVersion()); +} - if (analyzerEnabled) { - if (reanalyze) { +bool ReplayGainSettings::isAnalyzerDisabled(int version, TrackPointer tio) const { + if (isAnalyzerEnabled(version)) { + if (getReplayGainReanalyze()) { // ignore stored replay gain return false; } diff --git a/src/preferences/replaygainsettings.h b/src/preferences/replaygainsettings.h index 2ae3317370f..541249f7478 100644 --- a/src/preferences/replaygainsettings.h +++ b/src/preferences/replaygainsettings.h @@ -21,6 +21,7 @@ class ReplayGainSettings { bool getReplayGainReanalyze() const; void setReplayGainReanalyze(bool value); + bool isAnalyzerEnabled(int version) const; bool isAnalyzerDisabled(int version, TrackPointer tio) const; private: diff --git a/src/skin/legacyskinparser.cpp b/src/skin/legacyskinparser.cpp index f277a630592..3c31976a5d4 100644 --- a/src/skin/legacyskinparser.cpp +++ b/src/skin/legacyskinparser.cpp @@ -928,11 +928,11 @@ QWidget* LegacySkinParser::parseOverview(const QDomElement& node) { // "RGB" = "2", "HSV" = "1" or "Filtered" = "0" (LMH) waveform overview type int type = m_pConfig->getValue(ConfigKey("[Waveform]","WaveformOverviewType"), 2); if (type == 0) { - overviewWidget = new WOverviewLMH(pSafeChannelStr, m_pConfig, m_pParent); + overviewWidget = new WOverviewLMH(pSafeChannelStr, m_pPlayerManager, m_pConfig, m_pParent); } else if (type == 1) { - overviewWidget = new WOverviewHSV(pSafeChannelStr, m_pConfig, m_pParent); + overviewWidget = new WOverviewHSV(pSafeChannelStr, m_pPlayerManager, m_pConfig, m_pParent); } else { - overviewWidget = new WOverviewRGB(pSafeChannelStr, m_pConfig, m_pParent); + overviewWidget = new WOverviewRGB(pSafeChannelStr, m_pPlayerManager, m_pConfig, m_pParent); } connect(overviewWidget, SIGNAL(trackDropped(QString, QString)), diff --git a/src/track/track.cpp b/src/track/track.cpp index 1da3ed8ee75..6f604e3f905 100644 --- a/src/track/track.cpp +++ b/src/track/track.cpp @@ -6,11 +6,10 @@ #include "track/track.h" #include "track/trackref.h" - #include "track/beatfactory.h" + #include "util/assert.h" #include "util/logger.h" -#include "util/compatibility.h" namespace { @@ -69,8 +68,7 @@ Track::Track( m_pSecurityToken(openSecurityToken(m_fileInfo, std::move(pSecurityToken))), m_record(trackId), m_bDirty(false), - m_bMarkedForMetadataExport(false), - m_analyzerProgress(-1) { + m_bMarkedForMetadataExport(false) { if (kLogStats && kLogger.debugEnabled()) { long numberOfInstancesBefore = s_numberOfInstances.fetch_add(1); kLogger.debug() @@ -711,19 +709,6 @@ void Track::setWaveformSummary(ConstWaveformPointer pWaveform) { emit(waveformSummaryUpdated()); } -void Track::setAnalyzerProgress(int progress) { - // progress in 0 .. 1000. QAtomicInt so no need for lock. - int oldProgress = m_analyzerProgress.fetchAndStoreAcquire(progress); - if (progress != oldProgress) { - emit(analyzerProgress(progress)); - } -} - -int Track::getAnalyzerProgress() const { - // QAtomicInt so no need for lock. - return load_atomic(m_analyzerProgress); -} - void Track::setCuePoint(double cue) { QMutexLocker lock(&m_qMutex); if (compareAndSet(&m_record.refCuePoint(), cue)) { diff --git a/src/track/track.h b/src/track/track.h index 21b65b3317f..6f686856dd8 100644 --- a/src/track/track.h +++ b/src/track/track.h @@ -1,6 +1,5 @@ #pragma once -#include #include #include #include @@ -307,7 +306,6 @@ class Track : public QObject { void waveformUpdated(); void waveformSummaryUpdated(); void coverArtUpdated(); - void analyzerProgress(int progress); void bpmUpdated(double bpm); void beatsUpdated(); void keyUpdated(double key); @@ -383,8 +381,6 @@ class Track : public QObject { ConstWaveformPointer m_waveform; ConstWaveformPointer m_waveformSummary; - QAtomicInt m_analyzerProgress; // in 0.1% - friend class TrackDAO; friend class GlobalTrackCache; friend class GlobalTrackCacheResolver; diff --git a/src/util/workerthread.cpp b/src/util/workerthread.cpp new file mode 100644 index 00000000000..a2ca086cffc --- /dev/null +++ b/src/util/workerthread.cpp @@ -0,0 +1,164 @@ +#include "util/workerthread.h" + + +namespace { + +// Enable trace logging only temporary for debugging purposes +// during development! +constexpr bool kEnableTraceLogging = false; + +inline +void logTrace(const mixxx::Logger& log, const char* msg) { + if (kEnableTraceLogging) { + log.trace() << (msg); + } +} + +std::atomic s_threadCounter(0); + +} // anonymous namespace + +WorkerThread::WorkerThread( + const QString& name) + : m_name(name), + m_logger(m_name.isEmpty() ? "WorkerThread" : m_name.toLatin1().constData()), + m_suspend(false), + m_stop(false) { +} + +WorkerThread::~WorkerThread() { + m_logger.debug() << "Destroying"; + VERIFY_OR_DEBUG_ASSERT(isFinished()) { + stop(); + m_logger.warning() << "Waiting until finished"; + // The following operation will block the calling thread! + wait(); + DEBUG_ASSERT(isFinished()); + } +} + +void WorkerThread::deleteAfterFinished() { + if (!isFinished()) { + connect(this, SIGNAL(finished()), this, SLOT(deleteLater())); + } + if (isFinished()) { + // Already finished or just finished in the meantime. Calling + // deleteLater() twice is safe, though. + deleteLater(); + } +} + +void WorkerThread::run() { + if (isStopping()) { + return; + } + + const int threadNumber = s_threadCounter.fetch_add(1) + 1; + const QString threadName = + m_name.isEmpty() ? QString::number(threadNumber) : QString("%1 #%2").arg(m_name, QString::number(threadNumber)); + QThread::currentThread()->setObjectName(threadName); + + m_logger.debug() << "Running"; + + doRun(); + + m_logger.debug() << "Exiting"; + + m_stop.store(true); +} + +void WorkerThread::suspend() { + logTrace(m_logger, "Suspending"); + m_suspend.store(true); +} + +void WorkerThread::resume() { + bool suspended = true; + // Reset value: true -> false + if (m_suspend.compare_exchange_strong(suspended, false)) { + logTrace(m_logger, "Resuming"); + // The thread might just be preparing to suspend after + // reading detecting that m_suspend was true. To avoid + // a race condition we need to acquire the mutex that + // is associated with the wait condition, before + // signalling the condition. Otherwise the signal + // of the wait condition might arrive before the + // thread actually got suspended. + std::unique_lock locked(m_sleepMutex); + wake(); + } else { + // Just in case, wake up the thread even if it wasn't + // explicitly suspended without locking the mutex. The + // thread will suspend itself if it is idle. + wake(); + } +} + +void WorkerThread::wake() { + m_logger.debug() << "Waking up"; + m_sleepWaitCond.notify_one(); +} + +void WorkerThread::stop() { + m_logger.debug() << "Stopping"; + m_stop.store(true); + // Wake up the thread to make sure that the stop flag is + // detected and the thread commits suicide by exiting the + // run loop in exec(). Resuming will reset the suspend flag + // to wake up not only an idle but also a suspended thread! + resume(); +} + +void WorkerThread::sleepWhileSuspended() { + DEBUG_ASSERT(QThread::currentThread() == this); + // The suspend flag is always reset after the stop flag has been set, + // so we don't need to check it separately here. + if (!m_suspend.load()) { + // Early exit without locking the mutex + return; + } + std::unique_lock locked(m_sleepMutex); + sleepWhileSuspended(&locked); +} + +void WorkerThread::sleepWhileSuspended(std::unique_lock* locked) { + DEBUG_ASSERT(locked); + while (m_suspend.load()) { + logTrace(m_logger, "Sleeping while suspended"); + m_sleepWaitCond.wait(*locked) ; + logTrace(m_logger, "Continuing after sleeping while suspended"); + } +} + +bool WorkerThread::waitUntilWorkItemsFetched() { + if (isStopping()) { + // Early exit without locking the mutex + return false; + } + // Keep the mutex locked while idle or suspended + std::unique_lock locked(m_sleepMutex); + while (!isStopping()) { + FetchWorkResult fetchWorkResult = tryFetchWorkItems(); + switch (fetchWorkResult) { + case FetchWorkResult::Ready: + logTrace(m_logger, "Work items fetched and ready"); + return true; + case FetchWorkResult::Idle: + logTrace(m_logger, "Sleeping while idle"); + m_sleepWaitCond.wait(locked) ; + logTrace(m_logger, "Continuing after slept while idle"); + break; + case FetchWorkResult::Suspend: + logTrace(m_logger, "Suspending while idle"); + suspend(); + sleepWhileSuspended(&locked); + logTrace(m_logger, "Continuing after suspended while idle"); + break; + case FetchWorkResult::Stop: + logTrace(m_logger, "Stopping after trying to fetch work items"); + stop(); + break; + } + } + return false; +} diff --git a/src/util/workerthread.h b/src/util/workerthread.h new file mode 100644 index 00000000000..04eebcbfb1a --- /dev/null +++ b/src/util/workerthread.h @@ -0,0 +1,131 @@ +#pragma once + +#include +#include +#include + +#include + +#include "util/logger.h" + + +// A worker thread without an event loop. +// +// This object lives in the creating thread of the host, i.e. does not +// run its own event loop. It does not does not use slots for communication +// with its host which would otherwise still be executed in the host's +// thread. +// +// Signals emitted from the internal worker thread by derived classes +// will queued connections. Communication in the opposite direction is +// accomplished by using lock-free types to avoid locking the host +// thread through priority inversion. Lock-free types might also used +// for any shared state that is read from the host thread after being +// notified about changes. +// +// Derived classes or their owners are responsible to start the thread +// with the desired priority. +class WorkerThread : public QThread { + Q_OBJECT + + public: + explicit WorkerThread( + const QString& name = QString()); + // The destructor must be triggered by calling deleteLater() to + // ensure that the thread has already finished and is not running + // while destroyed! Connect finished() to deleteAfter() and then + // call stop() on the running worker thread explicitly to trigger + // the destruction. Use deleteAfterFinished() for this purpose. + ~WorkerThread() override; + + void deleteAfterFinished(); + + const QString& name() const { + return m_name; + } + + // Commands the thread to suspend itself asap. + void suspend(); + + // Resumes a suspended thread by waking it up. + void resume(); + + // Wakes up a sleeping thread. If the thread has been suspended + // it will fall asleep again. A suspended thread needs to be + // resumed. + void wake(); + + // Commands the thread to stop asap. This action is irreversible, + // i.e. the thread cannot be restarted once it has been stopped. + void stop(); + + // Non-blocking atomic read of the stop flag which indicates that + // the thread is stopping, i.e. it will soon exit or already has + // exited the run loop. + bool isStopping() const { + return m_stop.load(); + } + + protected: + void run() final; + + // The internal run loop. Not to be confused with the Qt event + // loop since the worker thread doesn't have one! + // An implementation may exit this loop after all work is done, + // which in turn exits and terminates the thread. The loop should + // also be left asap when isStopping() returns true. This condition + // should be checked repeatedly during execution of the loop and + // especially before starting any expensive subtasks. + virtual void doRun() = 0; + + enum class FetchWorkResult { + Ready, + Idle, + Suspend, + Stop, + }; + + // Non-blocking function that determines whether the worker thread + // is idle (i.e. no new tasks have been scheduled) and should be + // either suspended until resumed or put to sleep until woken up. + // + // Implementing classes are able to control what to do if no more + // work is currently available. Returning FetchWorkResult::Idle + // preserves the current suspend state and just puts the thread + // to sleep until wake() is called. Returning FetchWorkResult::Suspend + // will suspend the thread until resume() is called. Returning + // FetchWorkResult::Stop will stop the worker thread. + // + // Implementing classes are responsible for storing the fetched + // work items internally for later processing during + // doRun(). + // + // The stop flag does not have to be checked when entering this function, + // because it has already been checked just before the invocation. Though + // the fetch operation may check again before starting any expensive + // internal subtask. + virtual FetchWorkResult tryFetchWorkItems() = 0; + + // Blocks while idle and not stopped. Returns true when new work items + // for processing have been fetched and false if the thread has been + // stopped while waiting. + bool waitUntilWorkItemsFetched(); + + // Blocks the worker thread while the suspend flag is set. + // This function must not be called from tryFetchWorkItems() + // to avoid a deadlock on the non-recursive mutex! + void sleepWhileSuspended(); + + private: + void sleepWhileSuspended(std::unique_lock* locked); + + const QString m_name; + + const mixxx::Logger m_logger; + + std::atomic m_suspend; + std::atomic m_stop; + + std::mutex m_sleepMutex; + std::condition_variable m_sleepWaitCond; +}; diff --git a/src/util/workerthreadscheduler.cpp b/src/util/workerthreadscheduler.cpp new file mode 100644 index 00000000000..9030ef7f649 --- /dev/null +++ b/src/util/workerthreadscheduler.cpp @@ -0,0 +1,52 @@ +#include "util/workerthreadscheduler.h" + +#include "util/workerthread.h" + + +WorkerThreadScheduler::WorkerThreadScheduler( + int maxWorkers, + const QString& name) + : WorkerThread(name.isEmpty() ? QString("WorkerThreadScheduler") : name), + m_scheduledWorkers(maxWorkers), + m_fetchedWorker(nullptr) { +} + +bool WorkerThreadScheduler::scheduleWorker(WorkerThread* worker) { + DEBUG_ASSERT(worker); + const auto written = m_scheduledWorkers.write(&worker, 1) == 1; + DEBUG_ASSERT((written == 0) || (written == 1)); + return written == 1; +} + +bool WorkerThreadScheduler::resumeWorkers() { + // Resume the scheduler thread if workers have been scheduled + // in the meantime + if (m_scheduledWorkers.readAvailable() > 0) { + resume(); + return true; + } else { + return false; + } +} + +WorkerThread::FetchWorkResult WorkerThreadScheduler::tryFetchWorkItems() { + DEBUG_ASSERT(!m_fetchedWorker); + WorkerThread* worker; + if (m_scheduledWorkers.read(&worker, 1) == 1) { + DEBUG_ASSERT(worker); + m_fetchedWorker = worker; + return FetchWorkResult::Ready; + } else { + // Suspend the thread after all scheduled workers have + // have been resumed. + return FetchWorkResult::Suspend; + } +} + +void WorkerThreadScheduler::doRun() { + while (waitUntilWorkItemsFetched()) { + m_fetchedWorker->resume(); + m_fetchedWorker = nullptr; + } + DEBUG_ASSERT(isStopping()); +} diff --git a/src/util/workerthreadscheduler.h b/src/util/workerthreadscheduler.h new file mode 100644 index 00000000000..25625e52e33 --- /dev/null +++ b/src/util/workerthreadscheduler.h @@ -0,0 +1,32 @@ +#pragma once + +#include "util/workerthread.h" +#include "util/fifo.h" + + +class WorkerThread; + +// Non-blocking scheduler for worker threads which itself runs +// as a worker thread. The maximum number of worker threads is +// limited. +class WorkerThreadScheduler : public WorkerThread { + public: + explicit WorkerThreadScheduler( + int maxWorkers, + const QString& name = QString()); + ~WorkerThreadScheduler() override = default; + + bool scheduleWorker(WorkerThread* worker); + + bool resumeWorkers(); + + protected: + void doRun() override; + + FetchWorkResult tryFetchWorkItems() override; + + private: + FIFO m_scheduledWorkers; + + WorkerThread* m_fetchedWorker; +}; diff --git a/src/widget/woverview.cpp b/src/widget/woverview.cpp index 4872d950f0d..8f17c3f4a51 100644 --- a/src/widget/woverview.cpp +++ b/src/widget/woverview.cpp @@ -26,6 +26,7 @@ #include "wskincolor.h" #include "widget/controlwidgetconnection.h" #include "track/track.h" +#include "analyzer/analyzerprogress.h" #include "util/math.h" #include "util/timer.h" #include "util/dnd.h" @@ -33,13 +34,17 @@ #include "waveform/waveform.h" #include "waveform/waveformwidgetfactory.h" -WOverview::WOverview(const char *pGroup, UserSettingsPointer pConfig, QWidget* parent) : +WOverview::WOverview( + const char* group, + PlayerManager* pPlayerManager, + UserSettingsPointer pConfig, + QWidget* parent) : WWidget(parent), m_actualCompletion(0), m_pixmapDone(false), m_waveformPeak(-1.0), m_diffGain(0), - m_group(pGroup), + m_group(group), m_pConfig(pConfig), m_endOfTrack(false), m_bDrag(false), @@ -47,8 +52,7 @@ WOverview::WOverview(const char *pGroup, UserSettingsPointer pConfig, QWidget* p m_orientation(Qt::Horizontal), m_a(1.0), m_b(0.0), - m_dAnalyzerProgress(1.0), - m_bAnalyzerFinalizing(false), + m_analyzerProgress(kAnalyzerProgressUnknown), m_trackLoaded(false), m_scaleFactor(1.0) { m_endOfTrackControl = new ControlProxy( @@ -59,6 +63,9 @@ WOverview::WOverview(const char *pGroup, UserSettingsPointer pConfig, QWidget* p new ControlProxy(m_group, "track_samples", this); m_playControl = new ControlProxy(m_group, "play", this); setAcceptDrops(true); + + connect(pPlayerManager, SIGNAL(trackAnalyzerProgress(TrackId, AnalyzerProgress)), + this, SLOT(onTrackAnalyzerProgress(TrackId, AnalyzerProgress))); } void WOverview::setup(const QDomNode& node, const SkinContext& context) { @@ -172,7 +179,7 @@ void WOverview::slotWaveformSummaryUpdated() { } else { // Null waveform pointer means waveform was cleared. m_waveformSourceImage = QImage(); - m_dAnalyzerProgress = 1.0; + m_analyzerProgress = kAnalyzerProgressUnknown; m_actualCompletion = 0; m_waveformPeak = -1.0; m_pixmapDone = false; @@ -181,19 +188,14 @@ void WOverview::slotWaveformSummaryUpdated() { } } -void WOverview::slotAnalyzerProgress(int progress) { - if (!m_pCurrentTrack) { +void WOverview::onTrackAnalyzerProgress(TrackId trackId, AnalyzerProgress analyzerProgress) { + if (!m_pCurrentTrack || (m_pCurrentTrack->getId() != trackId)) { return; } - double analyzerProgress = progress / 1000.0; - bool finalizing = progress == 999; - bool updateNeeded = drawNextPixmapPart(); - // progress 0 .. 1000 - if (updateNeeded || (m_dAnalyzerProgress != analyzerProgress)) { - m_dAnalyzerProgress = analyzerProgress; - m_bAnalyzerFinalizing = finalizing; + if (updateNeeded || (m_analyzerProgress != analyzerProgress)) { + m_analyzerProgress = analyzerProgress; update(); } } @@ -210,12 +212,10 @@ void WOverview::slotLoadingTrack(TrackPointer pNewTrack, TrackPointer pOldTrack) if (m_pCurrentTrack != nullptr) { disconnect(m_pCurrentTrack.get(), SIGNAL(waveformSummaryUpdated()), this, SLOT(slotWaveformSummaryUpdated())); - disconnect(m_pCurrentTrack.get(), SIGNAL(analyzerProgress(int)), - this, SLOT(slotAnalyzerProgress(int))); } m_waveformSourceImage = QImage(); - m_dAnalyzerProgress = 1.0; + m_analyzerProgress = kAnalyzerProgressUnknown; m_actualCompletion = 0; m_waveformPeak = -1.0; m_pixmapDone = false; @@ -228,10 +228,6 @@ void WOverview::slotLoadingTrack(TrackPointer pNewTrack, TrackPointer pOldTrack) connect(pNewTrack.get(), SIGNAL(waveformSummaryUpdated()), this, SLOT(slotWaveformSummaryUpdated())); - connect(pNewTrack.get(), SIGNAL(analyzerProgress(int)), - this, SLOT(slotAnalyzerProgress(int))); - - slotAnalyzerProgress(pNewTrack->getAnalyzerProgress()); } else { m_pCurrentTrack.reset(); m_pWaveform.clear(); @@ -350,21 +346,28 @@ void WOverview::paintEvent(QPaintEvent * /*unused*/) { } } - if (m_dAnalyzerProgress < 1.0) { + if ((m_analyzerProgress >= kAnalyzerProgressNone) && + (m_analyzerProgress < kAnalyzerProgressDone)) { // Paint analyzer Progress painter.setPen(QPen(m_signalColors.getAxesColor(), 3 * m_scaleFactor)); - if (m_dAnalyzerProgress > 0.0) { + if (m_analyzerProgress > kAnalyzerProgressNone) { if (m_orientation == Qt::Horizontal) { - painter.drawLine(m_dAnalyzerProgress * width(), height() / 2, - width(), height() / 2); + painter.drawLine( + width() * m_analyzerProgress, + height() / 2, + width(), + height() / 2); } else { - painter.drawLine(width() / 2 , m_dAnalyzerProgress * height(), - width() / 2, height()); + painter.drawLine( + width() / 2 , + height() * m_analyzerProgress, + width() / 2, + height()); } } - if (m_dAnalyzerProgress <= 0.5) { // remove text after progress by wf is recognizable + if (m_analyzerProgress <= kAnalyzerProgressHalf) { // remove text after progress by wf is recognizable if (m_trackLoaded) { //: Text on waveform overview when file is playable but no waveform is visible paintText(tr("Ready to play, analyzing .."), &painter); @@ -372,7 +375,7 @@ void WOverview::paintEvent(QPaintEvent * /*unused*/) { //: Text on waveform overview when file is cached from source paintText(tr("Loading track .."), &painter); } - } else if (m_bAnalyzerFinalizing) { + } else if (m_analyzerProgress >= kAnalyzerProgressFinalizing) { //: Text on waveform overview during finalizing of waveform analysis paintText(tr("Finalizing .."), &painter); } diff --git a/src/widget/woverview.h b/src/widget/woverview.h index 952caf75130..625a5bbf264 100644 --- a/src/widget/woverview.h +++ b/src/widget/woverview.h @@ -20,32 +20,36 @@ #include "track/track.h" #include "widget/wwidget.h" +#include "analyzer/analyzerprogress.h" #include "waveform/renderers/waveformsignalcolors.h" #include "waveform/renderers/waveformmarkset.h" #include "waveform/renderers/waveformmarkrange.h" #include "skin/skincontext.h" -// Waveform overview display -// @author Tue Haste Andersen -class Waveform; +class PlayerManager; class WOverview : public WWidget { Q_OBJECT public: - WOverview(const char* pGroup, UserSettingsPointer pConfig, QWidget* parent=nullptr); - void setup(const QDomNode& node, const SkinContext& context); public slots: void onConnectedControlChanged(double dParameter, double dValue) override; void slotTrackLoaded(TrackPointer pTrack); void slotLoadingTrack(TrackPointer pNewTrack, TrackPointer pOldTrack); + void onTrackAnalyzerProgress(TrackId trackId, AnalyzerProgress analyzerProgress); signals: void trackDropped(QString filename, QString group); protected: + WOverview( + const char* group, + PlayerManager* pPlayerManager, + UserSettingsPointer pConfig, + QWidget* parent = nullptr); + void mouseMoveEvent(QMouseEvent *e) override; void mouseReleaseEvent(QMouseEvent *e) override; void mousePressEvent(QMouseEvent *e) override; @@ -86,7 +90,6 @@ class WOverview : public WWidget { void onMarkRangeChange(double v); void slotWaveformSummaryUpdated(); - void slotAnalyzerProgress(int progress); private: // Append the waveform overview pixmap according to available data in waveform @@ -129,8 +132,7 @@ class WOverview : public WWidget { double m_a; double m_b; - double m_dAnalyzerProgress; - bool m_bAnalyzerFinalizing; + AnalyzerProgress m_analyzerProgress; bool m_trackLoaded; double m_scaleFactor; }; diff --git a/src/widget/woverviewhsv.cpp b/src/widget/woverviewhsv.cpp index e125737f3c4..00e830ceda4 100644 --- a/src/widget/woverviewhsv.cpp +++ b/src/widget/woverviewhsv.cpp @@ -7,9 +7,12 @@ #include "util/math.h" #include "waveform/waveform.h" -WOverviewHSV::WOverviewHSV(const char* pGroup, - UserSettingsPointer pConfig, QWidget* parent) - : WOverview(pGroup, pConfig, parent) { +WOverviewHSV::WOverviewHSV( + const char* group, + PlayerManager* pPlayerManager, + UserSettingsPointer pConfig, + QWidget* parent) + : WOverview(group, pPlayerManager, pConfig, parent) { } bool WOverviewHSV::drawNextPixmapPart() { diff --git a/src/widget/woverviewhsv.h b/src/widget/woverviewhsv.h index e6f85d5fd36..7b59639c65a 100644 --- a/src/widget/woverviewhsv.h +++ b/src/widget/woverviewhsv.h @@ -5,7 +5,11 @@ class WOverviewHSV : public WOverview { public: - WOverviewHSV(const char *pGroup, UserSettingsPointer pConfig, QWidget* parent); + WOverviewHSV( + const char* group, + PlayerManager* pPlayerManager, + UserSettingsPointer pConfig, + QWidget* parent = nullptr); private: bool drawNextPixmapPart() override; diff --git a/src/widget/woverviewlmh.cpp b/src/widget/woverviewlmh.cpp index bf838f2e33f..833b86339cc 100644 --- a/src/widget/woverviewlmh.cpp +++ b/src/widget/woverviewlmh.cpp @@ -8,9 +8,12 @@ #include "util/math.h" #include "waveform/waveform.h" -WOverviewLMH::WOverviewLMH(const char *pGroup, - UserSettingsPointer pConfig, QWidget * parent) - : WOverview(pGroup, pConfig, parent) { +WOverviewLMH::WOverviewLMH( + const char* group, + PlayerManager* pPlayerManager, + UserSettingsPointer pConfig, + QWidget* parent) + : WOverview(group, pPlayerManager, pConfig, parent) { } diff --git a/src/widget/woverviewlmh.h b/src/widget/woverviewlmh.h index e144f764890..c6ed4cb16f3 100644 --- a/src/widget/woverviewlmh.h +++ b/src/widget/woverviewlmh.h @@ -5,7 +5,11 @@ class WOverviewLMH : public WOverview { public: - WOverviewLMH(const char *pGroup, UserSettingsPointer pConfig, QWidget* parent); + WOverviewLMH( + const char* group, + PlayerManager* pPlayerManager, + UserSettingsPointer pConfig, + QWidget* parent = nullptr); private: bool drawNextPixmapPart() override; diff --git a/src/widget/woverviewrgb.cpp b/src/widget/woverviewrgb.cpp index 6a4c348efc7..e6519da967b 100644 --- a/src/widget/woverviewrgb.cpp +++ b/src/widget/woverviewrgb.cpp @@ -6,9 +6,12 @@ #include "util/math.h" #include "waveform/waveform.h" -WOverviewRGB::WOverviewRGB(const char* pGroup, - UserSettingsPointer pConfig, QWidget* parent) - : WOverview(pGroup, pConfig, parent) { +WOverviewRGB::WOverviewRGB( + const char* group, + PlayerManager* pPlayerManager, + UserSettingsPointer pConfig, + QWidget* parent) + : WOverview(group, pPlayerManager, pConfig, parent) { } bool WOverviewRGB::drawNextPixmapPart() { diff --git a/src/widget/woverviewrgb.h b/src/widget/woverviewrgb.h index 646886e442d..3a9382efcad 100644 --- a/src/widget/woverviewrgb.h +++ b/src/widget/woverviewrgb.h @@ -5,7 +5,11 @@ class WOverviewRGB : public WOverview { public: - WOverviewRGB(const char *pGroup, UserSettingsPointer pConfig, QWidget* parent); + WOverviewRGB( + const char* group, + PlayerManager* pPlayerManager, + UserSettingsPointer pConfig, + QWidget* parent = nullptr); private: bool drawNextPixmapPart() override; From 9449ed965f5e8cb9483daf922d1f95152546c089 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 21 Nov 2018 15:25:26 +0100 Subject: [PATCH 02/25] Rename constant --- src/control/controlvalue.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/control/controlvalue.h b/src/control/controlvalue.h index 48be063a1df..4b340aa2a74 100644 --- a/src/control/controlvalue.h +++ b/src/control/controlvalue.h @@ -10,7 +10,7 @@ // for lock free access, this value has to be >= the number of value using threads // value must be a fraction of an integer -const int cDefaultRingSize = 8; +const int kDefaultRingSize = 8; // there are basically unlimited readers allowed at each ring element // but we have to count them so max() is just fine. // NOTE(rryan): Wrapping max with parentheses avoids conflict with the max macro @@ -193,7 +193,7 @@ class ControlValueAtomicBase { // ControlValueAtomicBase to use. For types where sizeof(T) <= sizeof(void*), // the specialized implementation of ControlValueAtomicBase for types that are // atomic on the architecture is used. -template +template class ControlValueAtomic : public ControlValueAtomicBase { public: ControlValueAtomic() = default; From 7290aba1c8ea188544475d5546428c47e8e8254b Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 21 Nov 2018 15:34:31 +0100 Subject: [PATCH 03/25] Round analyzer progress percentage --- src/analyzer/analyzerprogress.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/analyzer/analyzerprogress.h b/src/analyzer/analyzerprogress.h index b9b5673245f..40484932ee2 100644 --- a/src/analyzer/analyzerprogress.h +++ b/src/analyzer/analyzerprogress.h @@ -17,6 +17,9 @@ Q_DECLARE_METATYPE(AnalyzerProgress); inline int analyzerProgressPercent(AnalyzerProgress analyzerProgress) { DEBUG_ASSERT(analyzerProgress >= kAnalyzerProgressNone); - return int((100 * (math_min(analyzerProgress, kAnalyzerProgressDone) - kAnalyzerProgressNone)) / - (kAnalyzerProgressDone - kAnalyzerProgressNone)); + const auto analyzerProgressClamped = + math_min(analyzerProgress, kAnalyzerProgressDone); + return static_cast(std::round( + 100 * (analyzerProgressClamped - kAnalyzerProgressNone) / + (kAnalyzerProgressDone - kAnalyzerProgressNone))); } From d1497ac81bcc6069a425a7597d0a560ef8c67cae Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 21 Nov 2018 16:01:06 +0100 Subject: [PATCH 04/25] Use type-safe signal/slot connections --- src/analyzer/trackanalysisscheduler.cpp | 10 ++++++--- src/library/analysisfeature.cpp | 12 +++++----- src/library/library.cpp | 29 ++++++++++++++++++------- src/library/library.h | 4 ++++ src/mixer/playermanager.cpp | 8 +++---- src/util/workerthread.cpp | 2 +- src/widget/woverview.cpp | 4 ++-- 7 files changed, 45 insertions(+), 24 deletions(-) diff --git a/src/analyzer/trackanalysisscheduler.cpp b/src/analyzer/trackanalysisscheduler.cpp index b3fd0971b72..562e2e95df3 100644 --- a/src/analyzer/trackanalysisscheduler.cpp +++ b/src/analyzer/trackanalysisscheduler.cpp @@ -75,8 +75,8 @@ TrackAnalysisScheduler::TrackAnalysisScheduler( library->dbConnectionPool(), pConfig, mode)); - connect(m_workers.back().thread(), SIGNAL(progress(int, AnalyzerThreadState, TrackId, AnalyzerProgress)), - this, SLOT(onWorkerThreadProgress(int, AnalyzerThreadState, TrackId, AnalyzerProgress))); + connect(m_workers.back().thread(), &AnalyzerThread::progress, + this, &TrackAnalysisScheduler::onWorkerThreadProgress); } // 2nd pass: Start worker threads in a suspended state for (const auto& worker: m_workers) { @@ -171,7 +171,11 @@ void TrackAnalysisScheduler::emitProgressOrFinished() { totalTracksCount); } -void TrackAnalysisScheduler::onWorkerThreadProgress(int threadId, AnalyzerThreadState threadState, TrackId trackId, AnalyzerProgress analyzerProgress) { +void TrackAnalysisScheduler::onWorkerThreadProgress( + int threadId, + AnalyzerThreadState threadState, + TrackId trackId, + AnalyzerProgress analyzerProgress) { auto& worker = m_workers.at(threadId); switch (threadState) { case AnalyzerThreadState::Void: diff --git a/src/library/analysisfeature.cpp b/src/library/analysisfeature.cpp index f37afb3e053..b2a967a4177 100644 --- a/src/library/analysisfeature.cpp +++ b/src/library/analysisfeature.cpp @@ -130,12 +130,12 @@ void AnalysisFeature::analyzeTracks(QList trackIds) { m_pConfig, getAnalyzerMode(m_pConfig)); - connect(m_pTrackAnalysisScheduler.get(), SIGNAL(progress(AnalyzerProgress, int, int)), - m_pAnalysisView, SLOT(onTrackAnalysisSchedulerProgress(AnalyzerProgress, int, int))); - connect(m_pTrackAnalysisScheduler.get(), SIGNAL(progress(AnalyzerProgress, int, int)), - this, SLOT(onTrackAnalysisSchedulerProgress(AnalyzerProgress, int, int))); - connect(m_pTrackAnalysisScheduler.get(), SIGNAL(finished()), - this, SLOT(stopAnalysis())); + connect(m_pTrackAnalysisScheduler.get(), &TrackAnalysisScheduler::progress, + m_pAnalysisView, &DlgAnalysis::onTrackAnalysisSchedulerProgress); + connect(m_pTrackAnalysisScheduler.get(), &TrackAnalysisScheduler::progress, + this, &AnalysisFeature::onTrackAnalysisSchedulerProgress); + connect(m_pTrackAnalysisScheduler.get(), &TrackAnalysisScheduler::finished, + this, &AnalysisFeature::stopAnalysis); emit(analysisActive(true)); } diff --git a/src/library/library.cpp b/src/library/library.cpp index 52e19cc716a..8a07715b7ca 100644 --- a/src/library/library.cpp +++ b/src/library/library.cpp @@ -127,17 +127,17 @@ Library::Library( addFeature(new SetlogFeature(this, pConfig, m_pTrackCollection)); m_pAnalysisFeature = new AnalysisFeature(this, pConfig); - connect(m_pPlaylistFeature, SIGNAL(analyzeTracks(QList)), - m_pAnalysisFeature, SLOT(analyzeTracks(QList))); - connect(m_pCrateFeature, SIGNAL(analyzeTracks(QList)), - m_pAnalysisFeature, SLOT(analyzeTracks(QList))); + connect(m_pPlaylistFeature, &PlaylistFeature::analyzeTracks, + m_pAnalysisFeature, &AnalysisFeature::analyzeTracks); + connect(m_pCrateFeature, &CrateFeature::analyzeTracks, + m_pAnalysisFeature, &AnalysisFeature::analyzeTracks); addFeature(m_pAnalysisFeature); // Suspend a batch analysis while an ad-hoc analysis of // loaded tracks is in progress and resume it afterwards. - connect(pPlayerManager, SIGNAL(trackAnalyzerProgress(TrackId, AnalyzerProgress)), - m_pAnalysisFeature, SLOT(suspendAnalysis())); - connect(pPlayerManager, SIGNAL(trackAnalyzerIdle()), - m_pAnalysisFeature, SLOT(resumeAnalysis())); + connect(pPlayerManager, &PlayerManager::trackAnalyzerProgress, + this, &Library::onPlayerManagerTrackAnalyzerProgress); + connect(pPlayerManager, &PlayerManager::trackAnalyzerIdle, + this, &Library::onPlayerManagerTrackAnalyzerIdle); //iTunes and Rhythmbox should be last until we no longer have an obnoxious //messagebox popup when you select them. (This forces you to reach for your @@ -299,6 +299,19 @@ void Library::addFeature(LibraryFeature* feature) { this, SIGNAL(trackSelected(TrackPointer))); } +void Library::onPlayerManagerTrackAnalyzerProgress( + TrackId /*trackId*/,AnalyzerProgress /*analyzerProgress*/) { + if (m_pAnalysisFeature) { + m_pAnalysisFeature->suspendAnalysis(); + } +} + +void Library::onPlayerManagerTrackAnalyzerIdle() { + if (m_pAnalysisFeature) { + m_pAnalysisFeature->resumeAnalysis(); + } +} + void Library::slotShowTrackModel(QAbstractItemModel* model) { //qDebug() << "Library::slotShowTrackModel" << model; TrackModel* trackModel = dynamic_cast(model); diff --git a/src/library/library.h b/src/library/library.h index 0376c5043b1..70b51e1d7d0 100644 --- a/src/library/library.h +++ b/src/library/library.h @@ -129,6 +129,10 @@ class Library: public QObject, void scanStarted(); void scanFinished(); + private slots: + void onPlayerManagerTrackAnalyzerProgress(TrackId trackId, AnalyzerProgress analyzerProgress); + void onPlayerManagerTrackAnalyzerIdle(); + private: // Callback for GlobalTrackCache void saveCachedTrack(Track* pTrack) noexcept override; diff --git a/src/mixer/playermanager.cpp b/src/mixer/playermanager.cpp index ec083e2f458..34b4795059d 100644 --- a/src/mixer/playermanager.cpp +++ b/src/mixer/playermanager.cpp @@ -121,10 +121,10 @@ void PlayerManager::bindToLibrary(Library* pLibrary) { kNumberOfAnalyzerThreads, m_pConfig); - connect(m_pTrackAnalysisScheduler.get(), SIGNAL(trackProgress(TrackId, AnalyzerProgress)), - this, SLOT(onTrackAnalysisProgress(TrackId, AnalyzerProgress))); - connect(m_pTrackAnalysisScheduler.get(), SIGNAL(finished()), - this, SLOT(onTrackAnalysisFinished())); + connect(m_pTrackAnalysisScheduler.get(), &TrackAnalysisScheduler::trackProgress, + this, &PlayerManager::onTrackAnalysisProgress); + connect(m_pTrackAnalysisScheduler.get(), &TrackAnalysisScheduler::finished, + this, &PlayerManager::onTrackAnalysisFinished); // Connect the player to the analyzer queue so that loaded tracks are // analyzed. diff --git a/src/util/workerthread.cpp b/src/util/workerthread.cpp index a2ca086cffc..df5cefcf250 100644 --- a/src/util/workerthread.cpp +++ b/src/util/workerthread.cpp @@ -39,7 +39,7 @@ WorkerThread::~WorkerThread() { void WorkerThread::deleteAfterFinished() { if (!isFinished()) { - connect(this, SIGNAL(finished()), this, SLOT(deleteLater())); + connect(this, &WorkerThread::finished, this, &WorkerThread::deleteLater); } if (isFinished()) { // Already finished or just finished in the meantime. Calling diff --git a/src/widget/woverview.cpp b/src/widget/woverview.cpp index 298a4387bc1..c078d0c518d 100644 --- a/src/widget/woverview.cpp +++ b/src/widget/woverview.cpp @@ -64,8 +64,8 @@ WOverview::WOverview( m_playControl = new ControlProxy(m_group, "play", this); setAcceptDrops(true); - connect(pPlayerManager, SIGNAL(trackAnalyzerProgress(TrackId, AnalyzerProgress)), - this, SLOT(onTrackAnalyzerProgress(TrackId, AnalyzerProgress))); + connect(pPlayerManager, &PlayerManager::trackAnalyzerProgress, + this, &WOverview::onTrackAnalyzerProgress); } void WOverview::setup(const QDomNode& node, const SkinContext& context) { From ca2e910d593beddad0d7a3d0009bea35296f481a Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 22 Nov 2018 13:06:40 +0100 Subject: [PATCH 05/25] Replace static factory function with subclass + default constructor --- src/analyzer/analyzerthread.cpp | 5 ++--- src/analyzer/analyzerthread.h | 6 +++++- src/analyzer/trackanalysisscheduler.cpp | 5 ++--- src/analyzer/trackanalysisscheduler.h | 8 ++++++-- src/library/analysisfeature.cpp | 2 +- src/mixer/playermanager.cpp | 2 +- 6 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/analyzer/analyzerthread.cpp b/src/analyzer/analyzerthread.cpp index 4951fe6fdb4..55cbb440fe6 100644 --- a/src/analyzer/analyzerthread.cpp +++ b/src/analyzer/analyzerthread.cpp @@ -45,9 +45,8 @@ void deleteAnalyzerThread(AnalyzerThread* plainPtr) { } // anonymous namespace -//static -AnalyzerThread::Pointer AnalyzerThread::nullPointer() { - return Pointer(nullptr, [](AnalyzerThread*){}); +AnalyzerThread::NullPointer::NullPointer() + : Pointer(nullptr, [](AnalyzerThread*){}) { } //static diff --git a/src/analyzer/analyzerthread.h b/src/analyzer/analyzerthread.h index fc2d8c201b5..c4e4c36d151 100644 --- a/src/analyzer/analyzerthread.h +++ b/src/analyzer/analyzerthread.h @@ -46,7 +46,11 @@ class AnalyzerThread : public WorkerThread { public: typedef std::unique_ptr Pointer; - static Pointer nullPointer(); + // Subclass that provides a default constructor and nothing else + class NullPointer: public Pointer { + public: + NullPointer(); + }; static Pointer createInstance( int id, diff --git a/src/analyzer/trackanalysisscheduler.cpp b/src/analyzer/trackanalysisscheduler.cpp index 562e2e95df3..84449d7ef78 100644 --- a/src/analyzer/trackanalysisscheduler.cpp +++ b/src/analyzer/trackanalysisscheduler.cpp @@ -26,9 +26,8 @@ void deleteTrackAnalysisScheduler(TrackAnalysisScheduler* plainPtr) { } // anonymous namespace -//static -TrackAnalysisScheduler::Pointer TrackAnalysisScheduler::nullPointer() { - return Pointer(nullptr, [](TrackAnalysisScheduler*){}); +TrackAnalysisScheduler::NullPointer::NullPointer() + : Pointer(nullptr, [](TrackAnalysisScheduler*){}) { } //static diff --git a/src/analyzer/trackanalysisscheduler.h b/src/analyzer/trackanalysisscheduler.h index beb50e9a107..d24418b0545 100644 --- a/src/analyzer/trackanalysisscheduler.h +++ b/src/analyzer/trackanalysisscheduler.h @@ -16,7 +16,11 @@ class TrackAnalysisScheduler : public QObject { public: typedef std::unique_ptr Pointer; - static Pointer nullPointer(); + // Subclass that provides a default constructor and nothing else + class NullPointer: public Pointer { + public: + NullPointer(); + }; static Pointer createInstance( Library* library, @@ -62,7 +66,7 @@ class TrackAnalysisScheduler : public QObject { // that runs the TrackAnalysisScheduler. class Worker { public: - explicit Worker(AnalyzerThread::Pointer thread = AnalyzerThread::nullPointer()) + explicit Worker(AnalyzerThread::Pointer thread = AnalyzerThread::NullPointer()) : m_thread(std::move(thread)), m_threadIdle(false), m_analyzerProgress(kAnalyzerProgressUnknown) { diff --git a/src/library/analysisfeature.cpp b/src/library/analysisfeature.cpp index b2a967a4177..07edeb9bcec 100644 --- a/src/library/analysisfeature.cpp +++ b/src/library/analysisfeature.cpp @@ -42,7 +42,7 @@ AnalysisFeature::AnalysisFeature( : LibraryFeature(parent), m_library(parent), m_pConfig(pConfig), - m_pTrackAnalysisScheduler(TrackAnalysisScheduler::nullPointer()), + m_pTrackAnalysisScheduler(TrackAnalysisScheduler::NullPointer()), m_analysisTitleName(tr("Analyze")), m_pAnalysisView(nullptr), m_icon(":/images/library/ic_library_prepare.svg") { diff --git a/src/mixer/playermanager.cpp b/src/mixer/playermanager.cpp index 34b4795059d..a47dc9de9aa 100644 --- a/src/mixer/playermanager.cpp +++ b/src/mixer/playermanager.cpp @@ -62,7 +62,7 @@ PlayerManager::PlayerManager(UserSettingsPointer pConfig, ConfigKey("[Master]", "num_microphones"), true, true)), m_pCONumAuxiliaries(new ControlObject( ConfigKey("[Master]", "num_auxiliaries"), true, true)), - m_pTrackAnalysisScheduler(TrackAnalysisScheduler::nullPointer()) { + m_pTrackAnalysisScheduler(TrackAnalysisScheduler::NullPointer()) { m_pCONumDecks->connectValueChangeRequest(this, SLOT(slotChangeNumDecks(double)), Qt::DirectConnection); m_pCONumSamplers->connectValueChangeRequest(this, From eb8e9acf1141501bdeac9dc90933613dfe16d828 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 22 Nov 2018 21:35:37 +0100 Subject: [PATCH 06/25] Add warning for reading atomic control values only once --- src/control/controlvalue.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/control/controlvalue.h b/src/control/controlvalue.h index 4b340aa2a74..1381b387332 100644 --- a/src/control/controlvalue.h +++ b/src/control/controlvalue.h @@ -51,6 +51,8 @@ class ControlRingValue { // value into the provided argument if a reader slot is available. // This is operation should not be repeated once it returned true, // because the stored value becomes invalid after it has been moved. + // WARNING: The value is only received EXACTLY ONCE in a multi producer/ + // single consumer (mpsc) scenario!! bool tryGetOnce(T* value) { // Read while consuming one readerSlot if (m_readerSlots.fetchAndAddAcquire(-1) > 0) { @@ -107,6 +109,8 @@ class ControlValueAtomicBase { return value; } + // WARNING: The value is only received EXACTLY ONCE in a multi producer/ + // single consumer (mpsc) scenario!! inline T getValueOnce() { T value; unsigned int index = static_cast(load_atomic(m_readIndex)) % cRingSize; From 6961c761ef8fa98355fd904689fe2d4a606795b4 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 22 Nov 2018 23:15:01 +0100 Subject: [PATCH 07/25] Replace ControlValue with new, lock-free MpscFifo class --- src/analyzer/analyzerthread.cpp | 11 ++--- src/analyzer/analyzerthread.h | 10 ++-- src/analyzer/trackanalysisscheduler.cpp | 7 ++- src/analyzer/trackanalysisscheduler.h | 10 ++-- src/control/controlvalue.h | 59 +++++----------------- src/util/mpscfifo.h | 66 +++++++++++++++++++++++++ 6 files changed, 100 insertions(+), 63 deletions(-) create mode 100644 src/util/mpscfifo.h diff --git a/src/analyzer/analyzerthread.cpp b/src/analyzer/analyzerthread.cpp index 55cbb440fe6..068265ccf45 100644 --- a/src/analyzer/analyzerthread.cpp +++ b/src/analyzer/analyzerthread.cpp @@ -76,7 +76,6 @@ AnalyzerThread::AnalyzerThread( m_sampleBuffer(kAnalysisSamplesPerBlock), m_emittedState(AnalyzerThreadState::Void) { m_lastBusyProgressEmittedTimer.start(); - m_nextTrack.setValue(TrackPointer()); // The types are registered multiple times although once would be sufficient qRegisterMetaType(); // AnalyzerProgress is just an alias/typedef and must be registered explicitly @@ -186,15 +185,15 @@ void AnalyzerThread::doRun() { emitProgress(AnalyzerThreadState::Exit); } -void AnalyzerThread::submitNextTrack(TrackPointer nextTrack) { - DEBUG_ASSERT(!m_nextTrack.getValue()); - m_nextTrack.setValue(std::move(nextTrack)); +bool AnalyzerThread::submitNextTrack(TrackPointer nextTrack) { + DEBUG_ASSERT(nextTrack); + return m_nextTrack.enqueue(std::move(nextTrack)); } WorkerThread::FetchWorkResult AnalyzerThread::tryFetchWorkItems() { DEBUG_ASSERT(!m_currentTrack); - m_currentTrack = m_nextTrack.getValueOnce(); - if (m_currentTrack) { + if (m_nextTrack.dequeue(&m_currentTrack)) { + DEBUG_ASSERT(m_currentTrack); return FetchWorkResult::Ready; } else { if (m_emittedState != AnalyzerThreadState::Idle) { diff --git a/src/analyzer/analyzerthread.h b/src/analyzer/analyzerthread.h index c4e4c36d151..2aa11f29cf8 100644 --- a/src/analyzer/analyzerthread.h +++ b/src/analyzer/analyzerthread.h @@ -6,7 +6,6 @@ #include "analyzer/analyzerprogress.h" #include "analyzer/analyzer.h" -#include "control/controlvalue.h" #include "preferences/usersettings.h" #include "sources/audiosource.h" #include "track/track.h" @@ -14,6 +13,7 @@ #include "util/performancetimer.h" #include "util/samplebuffer.h" #include "util/memory.h" +#include "util/mpscfifo.h" enum class AnalyzerMode { @@ -74,7 +74,7 @@ class AnalyzerThread : public WorkerThread { // with state Idle has been received to avoid overwriting // a previously sent track that has not been received by the // worker thread, yet. - void submitNextTrack(TrackPointer nextTrack); + bool submitNextTrack(TrackPointer nextTrack); signals: // Use a single signal for progress updates to ensure that all signals @@ -102,9 +102,9 @@ class AnalyzerThread : public WorkerThread { // Thread-safe atomic values // There is only one consumer (namely the worker thread) and one producer - // (the host thread) for this value. Therefore default ring buffer size - // is sufficient. - ControlValueAtomic m_nextTrack; + // (the host thread) for this value. A single value is written and read + // in turn so the minimum capacity is sufficient. + MpscFifo m_nextTrack; ///////////////////////////////////////////////////////////////////////// // Thread local: Only used in the constructor/destructor and within diff --git a/src/analyzer/trackanalysisscheduler.cpp b/src/analyzer/trackanalysisscheduler.cpp index 84449d7ef78..419e31535f4 100644 --- a/src/analyzer/trackanalysisscheduler.cpp +++ b/src/analyzer/trackanalysisscheduler.cpp @@ -247,7 +247,12 @@ bool TrackAnalysisScheduler::submitNextTrack(Worker* worker) { ++m_finishedTracksCount; continue; } - worker->submitNextTrack(std::move(nextTrack)); + VERIFY_OR_DEBUG_ASSERT(worker->submitNextTrack(std::move(nextTrack))) { + // This should never happen + kLogger.warning() << "Failed to submit next track"; + // Retry to avoid skipping the track + continue; + } m_queuedTrackIds.pop_front(); ++m_dequeuedTracksCount; worker->wakeThread(); diff --git a/src/analyzer/trackanalysisscheduler.h b/src/analyzer/trackanalysisscheduler.h index d24418b0545..168b3877699 100644 --- a/src/analyzer/trackanalysisscheduler.h +++ b/src/analyzer/trackanalysisscheduler.h @@ -92,12 +92,16 @@ class TrackAnalysisScheduler : public QObject { return m_analyzerProgress; } - void submitNextTrack(TrackPointer track) { + bool submitNextTrack(TrackPointer track) { DEBUG_ASSERT(track); DEBUG_ASSERT(m_thread); DEBUG_ASSERT(m_threadIdle); - m_thread->submitNextTrack(std::move(track)); - m_threadIdle = false; + if (m_thread->submitNextTrack(std::move(track))) { + m_threadIdle = false; + return true; + } else { + return false; + } } void wakeThread() { diff --git a/src/control/controlvalue.h b/src/control/controlvalue.h index 1381b387332..e2c49f4177b 100644 --- a/src/control/controlvalue.h +++ b/src/control/controlvalue.h @@ -10,28 +10,29 @@ // for lock free access, this value has to be >= the number of value using threads // value must be a fraction of an integer -const int kDefaultRingSize = 8; +constexpr int kDefaultRingSize = 8; // there are basically unlimited readers allowed at each ring element // but we have to count them so max() is just fine. // NOTE(rryan): Wrapping max with parentheses avoids conflict with the max macro // defined in windows.h. -const int cReaderSlotCnt = (std::numeric_limits::max)(); +constexpr int kMaxReaderSlots = (std::numeric_limits::max)(); // A single instance of a value of type T along with an atomic integer which // tracks the current number of readers or writers of the slot. The value -// m_readerSlots starts at cReaderSlotCnt and counts down to 0. If the value is +// m_readerSlots starts at kMaxReaderSlots and counts down to 0. If the value is // 0 or less then reads to the value fail because there are either too many // readers or a write is occurring. A write to the value will fail if -// m_readerSlots is not equal to cReaderSlotCnt (e.g. there is an active +// m_readerSlots is not equal to kMaxReaderSlots (e.g. there is an active // reader). template class ControlRingValue { public: - ControlRingValue() : m_readerSlots(cReaderSlotCnt) { + ControlRingValue() + : m_readerSlots(kMaxReaderSlots) { } // Tries to copy the stored value if a reader slot is available. - // This is operation can be repeated multiple times for the same + // This operation can be repeated multiple times for the same // slot, because the stored value is preserved. bool tryGet(T* value) const { // Read while consuming one readerSlot @@ -47,31 +48,11 @@ class ControlRingValue { } } - // A destructive read operation that tries to move the stored - // value into the provided argument if a reader slot is available. - // This is operation should not be repeated once it returned true, - // because the stored value becomes invalid after it has been moved. - // WARNING: The value is only received EXACTLY ONCE in a multi producer/ - // single consumer (mpsc) scenario!! - bool tryGetOnce(T* value) { - // Read while consuming one readerSlot - if (m_readerSlots.fetchAndAddAcquire(-1) > 0) { - *value = std::move(m_value); - m_readerSlots.fetchAndAddRelease(1); - return true; - } else { - // Otherwise a writer is active. The writer will reset - // the counter in m_readerSlots when releasing the lock - // and we must not re-add the substracted value here! - return false; - } - } - bool trySet(const T& value) { // try to lock this element entirely for reading - if (m_readerSlots.testAndSetAcquire(cReaderSlotCnt, 0)) { + if (m_readerSlots.testAndSetAcquire(kMaxReaderSlots, 0)) { m_value = value; - m_readerSlots.fetchAndAddRelease(cReaderSlotCnt); + m_readerSlots.fetchAndAddRelease(kMaxReaderSlots); return true; } else { return false; @@ -80,6 +61,7 @@ class ControlRingValue { private: T m_value; + int m_maxReaderSlots; mutable QAtomicInt m_readerSlots; }; @@ -97,26 +79,7 @@ class ControlValueAtomicBase { unsigned int index = static_cast(load_atomic(m_readIndex)) % cRingSize; while (!m_ring[index].tryGet(&value)) { // We are here if - // 1) there are more then cReaderSlotCnt reader (get) reading the same value or - // 2) the formerly current value is locked by a writer - // Case 1 does not happen because we have enough (0x7fffffff) reader slots. - // Case 2 happens when the a reader is delayed after reading the - // m_currentIndex and in the mean while a reader locks the formally current value - // because it has written cRingSize times. Reading the less recent value will fix - // it because it is now actually the current value. - index = (index - 1) % cRingSize; - } - return value; - } - - // WARNING: The value is only received EXACTLY ONCE in a multi producer/ - // single consumer (mpsc) scenario!! - inline T getValueOnce() { - T value; - unsigned int index = static_cast(load_atomic(m_readIndex)) % cRingSize; - while (!m_ring[index].tryGetOnce(&value)) { - // We are here if - // 1) there are more then cReaderSlotCnt reader (get) reading the same value or + // 1) there are more then kMaxReaderSlots reader (get) reading the same value or // 2) the formerly current value is locked by a writer // Case 1 does not happen because we have enough (0x7fffffff) reader slots. // Case 2 happens when the a reader is delayed after reading the diff --git a/src/util/mpscfifo.h b/src/util/mpscfifo.h new file mode 100644 index 00000000000..b654189e762 --- /dev/null +++ b/src/util/mpscfifo.h @@ -0,0 +1,66 @@ +#pragma once + +#include + +#include "util/assert.h" + +// Lock-free FIFO for multiple producers/writers and a single(!) consumer/reader. +template +class MpscFifo { + public: + MpscFifo() + : m_enqueueSize(0), + m_dequeueSize(0), + m_headIndex(0), + m_tailIndex(0) { + static_assert(capacity >= 2, "capacity too low"); + } + + bool enqueue(T value) { + if (m_enqueueSize.fetchAndAddAcquire(1) >= capacity) { + // Queue is full -> Restore size and abort + m_enqueueSize.fetchAndSubRelease(1); + return false; + } else { + int headIndex = m_headIndex.fetchAndAddAcquire(1); + DEBUG_ASSERT(headIndex >= 0); + m_buffer[headIndex % capacity] = std::move(value); + // Allow the reader to access the enqueued value + m_dequeueSize.fetchAndAddRelease(1); + return true; + } + } + + bool dequeue(T* value) { + if (m_dequeueSize.fetchAndSubAcquire(1) <= 0) { + // Queue is empty -> Restore size and abort + m_dequeueSize.fetchAndAddRelease(1); + return false; + } else { + DEBUG_ASSERT(m_tailIndex >= 0); + DEBUG_ASSERT(m_tailIndex < capacity); + *value = std::move(m_buffer[m_tailIndex]); + if (++m_tailIndex >= capacity) { + m_tailIndex %= capacity; + // Prevent unlimited growth and overflow of m_headIndex which + // is already ahead of m_tailIndex. + int headIndex; + do { + headIndex = m_headIndex.load(); + DEBUG_ASSERT(headIndex >= 0); + } while ((headIndex >= capacity) && + !m_headIndex.testAndSetOrdered(headIndex, headIndex % capacity)); + } + // Allow the writer to overwrite the dequeued value + m_enqueueSize.fetchAndSubRelease(1); + return true; + } + } + + private: + T m_buffer[capacity]; + QAtomicInteger m_enqueueSize; + QAtomicInteger m_dequeueSize; + QAtomicInteger m_headIndex; + int m_tailIndex; +}; From 266884e1b1a7992ac861ce2517ff810f872c0fc5 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 23 Nov 2018 00:07:47 +0100 Subject: [PATCH 08/25] Update comments about analysis parameterization --- src/analyzer/analyzerthread.cpp | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/analyzer/analyzerthread.cpp b/src/analyzer/analyzerthread.cpp index 068265ccf45..597e889de22 100644 --- a/src/analyzer/analyzerthread.cpp +++ b/src/analyzer/analyzerthread.cpp @@ -25,16 +25,22 @@ namespace { mixxx::Logger kLogger("AnalyzerThread"); -// Analysis is done in blocks. -// We need to use a smaller block size, because on Linux the AnalyzerThread -// can starve the CPU of its resources, resulting in xruns. A block size -// of 4096 frames per block seems to do fine. +// NOTE(uklotzde, 2018-11-23): The parameterization for the analyzers +// has not been touched while transforming the code from single- to +// multi-threaded processing! Feel free to adjust this if justified. + +// Analysis is done in blocks to avoid dynamic allocation of memory +// depending on the track length. A block size of 4096 frames per block +// seems to do fine. Signal processing during analysis uses the same, +// fixed number of channels like the engine does, usually 2 = stereo. constexpr mixxx::AudioSignal::ChannelCount kAnalysisChannels = mixxx::kEngineChannelCount; constexpr SINT kAnalysisFramesPerBlock = 4096; -const SINT kAnalysisSamplesPerBlock = +constexpr SINT kAnalysisSamplesPerBlock = kAnalysisFramesPerBlock * kAnalysisChannels; -// Maximum frequency of progress updates while busy +// Maximum frequency of progress updates while busy. A value of 60 ms +// results in ~17 progress updates per second which is sufficient for +// continuous feedback. const mixxx::Duration kBusyProgressInhibitDuration = mixxx::Duration::fromMillis(60); void deleteAnalyzerThread(AnalyzerThread* plainPtr) { From 41925b68b8e73b97a3400b08fa2d17028d6db694 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 23 Nov 2018 00:24:36 +0100 Subject: [PATCH 09/25] Revert constexpr --- src/analyzer/analyzerthread.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/analyzer/analyzerthread.cpp b/src/analyzer/analyzerthread.cpp index 597e889de22..b835384720a 100644 --- a/src/analyzer/analyzerthread.cpp +++ b/src/analyzer/analyzerthread.cpp @@ -35,7 +35,7 @@ mixxx::Logger kLogger("AnalyzerThread"); // fixed number of channels like the engine does, usually 2 = stereo. constexpr mixxx::AudioSignal::ChannelCount kAnalysisChannels = mixxx::kEngineChannelCount; constexpr SINT kAnalysisFramesPerBlock = 4096; -constexpr SINT kAnalysisSamplesPerBlock = +const SINT kAnalysisSamplesPerBlock = kAnalysisFramesPerBlock * kAnalysisChannels; // Maximum frequency of progress updates while busy. A value of 60 ms From 965d52be5c221230225e2f058174f1819ced1d5e Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 24 Dec 2018 10:38:28 +0100 Subject: [PATCH 10/25] Replace QAtomicInteger with QAtomicInt ...for backwards compatibility --- src/util/mpscfifo.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/util/mpscfifo.h b/src/util/mpscfifo.h index b654189e762..1bf573327b7 100644 --- a/src/util/mpscfifo.h +++ b/src/util/mpscfifo.h @@ -1,6 +1,6 @@ #pragma once -#include +#include #include "util/assert.h" @@ -59,8 +59,8 @@ class MpscFifo { private: T m_buffer[capacity]; - QAtomicInteger m_enqueueSize; - QAtomicInteger m_dequeueSize; - QAtomicInteger m_headIndex; + QAtomicInt m_enqueueSize; + QAtomicInt m_dequeueSize; + QAtomicInt m_headIndex; int m_tailIndex; }; From 460670dfe130326bcd20ba1c0c5058f1dde56a6d Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 24 Dec 2018 10:59:29 +0100 Subject: [PATCH 11/25] Register meta-types only once --- src/analyzer/analyzerthread.cpp | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/analyzer/analyzerthread.cpp b/src/analyzer/analyzerthread.cpp index b835384720a..b8ff79d3844 100644 --- a/src/analyzer/analyzerthread.cpp +++ b/src/analyzer/analyzerthread.cpp @@ -1,5 +1,7 @@ #include "analyzer/analyzerthread.h" +#include + #ifdef __VAMP__ #include "analyzer/analyzerbeats.h" #include "analyzer/analyzerkey.h" @@ -49,6 +51,15 @@ void deleteAnalyzerThread(AnalyzerThread* plainPtr) { } } +std::once_flag registerMetaTypesOnceFlag; + +void registerMetaTypesOnce() { + qRegisterMetaType(); + // AnalyzerProgress is just an alias/typedef and must be registered explicitly + // by name! + qRegisterMetaType("AnalyzerProgress"); +} + } // anonymous namespace AnalyzerThread::NullPointer::NullPointer() @@ -81,12 +92,8 @@ AnalyzerThread::AnalyzerThread( m_mode(mode), m_sampleBuffer(kAnalysisSamplesPerBlock), m_emittedState(AnalyzerThreadState::Void) { + std::call_once(registerMetaTypesOnceFlag, registerMetaTypesOnce); m_lastBusyProgressEmittedTimer.start(); - // The types are registered multiple times although once would be sufficient - qRegisterMetaType(); - // AnalyzerProgress is just an alias/typedef and must be registered explicitly - // by name! - qRegisterMetaType("AnalyzerProgress"); } void AnalyzerThread::doRun() { From 5fb877e8610509f7216c255134cf25e4bfd551d9 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 24 Dec 2018 11:07:09 +0100 Subject: [PATCH 12/25] Add a TODO comment for scheduling updates with the vsync timer --- src/analyzer/analyzerthread.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/analyzer/analyzerthread.cpp b/src/analyzer/analyzerthread.cpp index b8ff79d3844..8ea5725e78e 100644 --- a/src/analyzer/analyzerthread.cpp +++ b/src/analyzer/analyzerthread.cpp @@ -31,6 +31,9 @@ mixxx::Logger kLogger("AnalyzerThread"); // has not been touched while transforming the code from single- to // multi-threaded processing! Feel free to adjust this if justified. +/// TODO(XXX): Use the vsync timer for the purpose of sending updates +// to the UI thread with a limited rate?? + // Analysis is done in blocks to avoid dynamic allocation of memory // depending on the track length. A block size of 4096 frames per block // seems to do fine. Signal processing during analysis uses the same, From 738ae8589699053e9270605e03b8b53d72dbe790 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 24 Dec 2018 15:00:30 +0100 Subject: [PATCH 13/25] Replace conditional else branch with early exit --- src/control/controlvalue.h | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/control/controlvalue.h b/src/control/controlvalue.h index e2c49f4177b..216d4e6e412 100644 --- a/src/control/controlvalue.h +++ b/src/control/controlvalue.h @@ -40,12 +40,11 @@ class ControlRingValue { *value = m_value; m_readerSlots.fetchAndAddRelease(1); return true; - } else { - // Otherwise a writer is active. The writer will reset - // the counter in m_readerSlots when releasing the lock - // and we must not re-add the substracted value here! - return false; } + // Otherwise a writer is active. The writer will reset + // the counter in m_readerSlots when releasing the lock + // and we must not re-add the substracted value here! + return false; } bool trySet(const T& value) { @@ -54,9 +53,8 @@ class ControlRingValue { m_value = value; m_readerSlots.fetchAndAddRelease(kMaxReaderSlots); return true; - } else { - return false; } + return false; } private: From 45d05c11ca30463078234bf410a3ffbc1801e579 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 25 Dec 2018 11:33:19 +0100 Subject: [PATCH 14/25] Fix release of writer lock on ControlValue --- src/control/controlvalue.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/control/controlvalue.h b/src/control/controlvalue.h index 216d4e6e412..b95a8c43772 100644 --- a/src/control/controlvalue.h +++ b/src/control/controlvalue.h @@ -51,7 +51,11 @@ class ControlRingValue { // try to lock this element entirely for reading if (m_readerSlots.testAndSetAcquire(kMaxReaderSlots, 0)) { m_value = value; - m_readerSlots.fetchAndAddRelease(kMaxReaderSlots); +#if QT_VERSION >= QT_VERSION_CHECK(5, 5, 0) + m_readerSlots.storeRelease(kMaxReaderSlots); +#else + m_readerSlots.fetchAndSetRelease(kMaxReaderSlots); +#endif return true; } return false; From aadcca48a2f33b316cd1c0986fd9ba06553dffe2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Sch=C3=BCrmann?= Date: Sun, 23 Dec 2018 21:44:22 +0100 Subject: [PATCH 15/25] Fix use of QAtomicInt for Qt5.2 --- src/control/controlvalue.h | 2 +- src/util/mpscfifo.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/control/controlvalue.h b/src/control/controlvalue.h index b95a8c43772..51c10dc15b5 100644 --- a/src/control/controlvalue.h +++ b/src/control/controlvalue.h @@ -54,7 +54,7 @@ class ControlRingValue { #if QT_VERSION >= QT_VERSION_CHECK(5, 5, 0) m_readerSlots.storeRelease(kMaxReaderSlots); #else - m_readerSlots.fetchAndSetRelease(kMaxReaderSlots); + m_readerSlots.fetchAndStoreRelease(kMaxReaderSlots); #endif return true; } diff --git a/src/util/mpscfifo.h b/src/util/mpscfifo.h index 1bf573327b7..4cce3c8ccd1 100644 --- a/src/util/mpscfifo.h +++ b/src/util/mpscfifo.h @@ -19,7 +19,7 @@ class MpscFifo { bool enqueue(T value) { if (m_enqueueSize.fetchAndAddAcquire(1) >= capacity) { // Queue is full -> Restore size and abort - m_enqueueSize.fetchAndSubRelease(1); + m_enqueueSize.fetchAndAddRelease(-1); return false; } else { int headIndex = m_headIndex.fetchAndAddAcquire(1); @@ -32,7 +32,7 @@ class MpscFifo { } bool dequeue(T* value) { - if (m_dequeueSize.fetchAndSubAcquire(1) <= 0) { + if (m_dequeueSize.fetchAndAddAcquire(-1) <= 0) { // Queue is empty -> Restore size and abort m_dequeueSize.fetchAndAddRelease(1); return false; @@ -52,7 +52,7 @@ class MpscFifo { !m_headIndex.testAndSetOrdered(headIndex, headIndex % capacity)); } // Allow the writer to overwrite the dequeued value - m_enqueueSize.fetchAndSubRelease(1); + m_enqueueSize.fetchAndAddRelease(-1); return true; } } From b96dc8dd89006975f9996ab526c1d645257f24fc Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 26 Dec 2018 11:55:00 +0100 Subject: [PATCH 16/25] Re-add slot count unconditionally --- src/control/controlvalue.h | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/control/controlvalue.h b/src/control/controlvalue.h index 51c10dc15b5..4a0d5c555b0 100644 --- a/src/control/controlvalue.h +++ b/src/control/controlvalue.h @@ -35,27 +35,25 @@ class ControlRingValue { // This operation can be repeated multiple times for the same // slot, because the stored value is preserved. bool tryGet(T* value) const { + bool result = false; // Read while consuming one readerSlot if (m_readerSlots.fetchAndAddAcquire(-1) > 0) { + // Reader slot has been acquired, no writer is active *value = m_value; - m_readerSlots.fetchAndAddRelease(1); - return true; + result = true; } - // Otherwise a writer is active. The writer will reset - // the counter in m_readerSlots when releasing the lock - // and we must not re-add the substracted value here! - return false; + m_readerSlots.fetchAndAddRelease(1); + return result; } bool trySet(const T& value) { // try to lock this element entirely for reading if (m_readerSlots.testAndSetAcquire(kMaxReaderSlots, 0)) { m_value = value; -#if QT_VERSION >= QT_VERSION_CHECK(5, 5, 0) - m_readerSlots.storeRelease(kMaxReaderSlots); -#else - m_readerSlots.fetchAndStoreRelease(kMaxReaderSlots); -#endif + // We need to re-add kMaxReaderSlots instead of storing it + // to keep the balance if readers have decreased the number + // of slots in the meantime! + m_readerSlots.fetchAndAddRelease(kMaxReaderSlots); return true; } return false; From 0202f78f7bb7b8eebbed57891afaf9164117abc4 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 26 Dec 2018 16:04:31 +0100 Subject: [PATCH 17/25] Start timer when executing the worker thread --- src/analyzer/analyzerthread.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/analyzer/analyzerthread.cpp b/src/analyzer/analyzerthread.cpp index 8ea5725e78e..6a6b006dce7 100644 --- a/src/analyzer/analyzerthread.cpp +++ b/src/analyzer/analyzerthread.cpp @@ -96,7 +96,6 @@ AnalyzerThread::AnalyzerThread( m_sampleBuffer(kAnalysisSamplesPerBlock), m_emittedState(AnalyzerThreadState::Void) { std::call_once(registerMetaTypesOnceFlag, registerMetaTypesOnce); - m_lastBusyProgressEmittedTimer.start(); } void AnalyzerThread::doRun() { @@ -119,6 +118,8 @@ void AnalyzerThread::doRun() { DEBUG_ASSERT(!m_analyzers.empty()); kLogger.debug() << "Activated" << m_analyzers.size() << "analyzers"; + m_lastBusyProgressEmittedTimer.start(); + // This thread-local database connection for pAnalysisDao // must not be closed before returning from this function. // Therefore the DbConnectionPooler is defined outside of From fdf8ee23953816baff356dc18defc5f3b0ab742d Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 26 Dec 2018 16:35:19 +0100 Subject: [PATCH 18/25] Use a bitfield for selecting and enabling analyzers --- src/analyzer/analyzerthread.cpp | 14 ++++++++------ src/analyzer/analyzerthread.h | 15 ++++++++------- src/analyzer/trackanalysisscheduler.cpp | 8 ++++---- src/analyzer/trackanalysisscheduler.h | 4 ++-- src/library/analysisfeature.cpp | 15 ++++++++++----- src/mixer/playermanager.cpp | 3 ++- 6 files changed, 34 insertions(+), 25 deletions(-) diff --git a/src/analyzer/analyzerthread.cpp b/src/analyzer/analyzerthread.cpp index 6a6b006dce7..a86414850b1 100644 --- a/src/analyzer/analyzerthread.cpp +++ b/src/analyzer/analyzerthread.cpp @@ -74,12 +74,12 @@ AnalyzerThread::Pointer AnalyzerThread::createInstance( int id, mixxx::DbConnectionPoolPtr dbConnectionPool, UserSettingsPointer pConfig, - AnalyzerMode mode) { + AnalyzerModeFlags modeFlags) { return Pointer(new AnalyzerThread( id, dbConnectionPool, pConfig, - mode), + modeFlags), deleteAnalyzerThread); } @@ -87,12 +87,12 @@ AnalyzerThread::AnalyzerThread( int id, mixxx::DbConnectionPoolPtr dbConnectionPool, UserSettingsPointer pConfig, - AnalyzerMode mode) + AnalyzerModeFlags modeFlags) : WorkerThread(QString("AnalyzerThread %1").arg(id)), m_id(id), m_dbConnectionPool(std::move(dbConnectionPool)), m_pConfig(std::move(pConfig)), - m_mode(mode), + m_modeFlags(modeFlags), m_sampleBuffer(kAnalysisSamplesPerBlock), m_emittedState(AnalyzerThreadState::Void) { std::call_once(registerMetaTypesOnceFlag, registerMetaTypesOnce); @@ -100,7 +100,7 @@ AnalyzerThread::AnalyzerThread( void AnalyzerThread::doRun() { std::unique_ptr pAnalysisDao; - if (m_mode != AnalyzerMode::WithBeatsWithoutWaveform) { + if (m_modeFlags & AnalyzerModeFlags::WithWaveform) { pAnalysisDao = std::make_unique(m_pConfig); m_analyzers.push_back(std::make_unique(pAnalysisDao.get())); } @@ -111,7 +111,9 @@ void AnalyzerThread::doRun() { m_analyzers.push_back(std::make_unique(m_pConfig)); } #ifdef __VAMP__ - const bool enforceBpmDetection = m_mode != AnalyzerMode::Default; + // BPM detection might be disabled in the config, but can be overriden + // and enabled by explicitly setting the mode flag. + const bool enforceBpmDetection = (m_modeFlags & AnalyzerModeFlags::WithBeats) != 0; m_analyzers.push_back(std::make_unique(m_pConfig, enforceBpmDetection)); m_analyzers.push_back(std::make_unique(m_pConfig)); #endif diff --git a/src/analyzer/analyzerthread.h b/src/analyzer/analyzerthread.h index 2aa11f29cf8..ee4707e5d32 100644 --- a/src/analyzer/analyzerthread.h +++ b/src/analyzer/analyzerthread.h @@ -16,10 +16,11 @@ #include "util/mpscfifo.h" -enum class AnalyzerMode { - Default, - WithBeats, - WithBeatsWithoutWaveform, +enum AnalyzerModeFlags { + None = 0x00, + WithBeats = 0x01, + WithWaveform = 0x02, + All = WithBeats | WithWaveform, }; enum class AnalyzerThreadState { @@ -56,13 +57,13 @@ class AnalyzerThread : public WorkerThread { int id, mixxx::DbConnectionPoolPtr dbConnectionPool, UserSettingsPointer pConfig, - AnalyzerMode mode = AnalyzerMode::Default); + AnalyzerModeFlags modeFlags); /*private*/ AnalyzerThread( int id, mixxx::DbConnectionPoolPtr dbConnectionPool, UserSettingsPointer pConfig, - AnalyzerMode mode); + AnalyzerModeFlags modeFlags); ~AnalyzerThread() override = default; int id() const { @@ -96,7 +97,7 @@ class AnalyzerThread : public WorkerThread { const int m_id; const mixxx::DbConnectionPoolPtr m_dbConnectionPool; const UserSettingsPointer m_pConfig; - const AnalyzerMode m_mode; + const AnalyzerModeFlags m_modeFlags; ///////////////////////////////////////////////////////////////////////// // Thread-safe atomic values diff --git a/src/analyzer/trackanalysisscheduler.cpp b/src/analyzer/trackanalysisscheduler.cpp index 419e31535f4..9ed3c3ef335 100644 --- a/src/analyzer/trackanalysisscheduler.cpp +++ b/src/analyzer/trackanalysisscheduler.cpp @@ -35,12 +35,12 @@ TrackAnalysisScheduler::Pointer TrackAnalysisScheduler::createInstance( Library* library, int numWorkerThreads, const UserSettingsPointer& pConfig, - AnalyzerMode mode) { + AnalyzerModeFlags modeFlags) { return Pointer(new TrackAnalysisScheduler( library, numWorkerThreads, pConfig, - mode), + modeFlags), deleteTrackAnalysisScheduler); } @@ -48,7 +48,7 @@ TrackAnalysisScheduler::TrackAnalysisScheduler( Library* library, int numWorkerThreads, const UserSettingsPointer& pConfig, - AnalyzerMode mode) + AnalyzerModeFlags modeFlags) : m_library(library), m_currentTrackProgress(kAnalyzerProgressUnknown), m_currentTrackNumber(0), @@ -73,7 +73,7 @@ TrackAnalysisScheduler::TrackAnalysisScheduler( threadId, library->dbConnectionPool(), pConfig, - mode)); + modeFlags)); connect(m_workers.back().thread(), &AnalyzerThread::progress, this, &TrackAnalysisScheduler::onWorkerThreadProgress); } diff --git a/src/analyzer/trackanalysisscheduler.h b/src/analyzer/trackanalysisscheduler.h index 168b3877699..7d173a12514 100644 --- a/src/analyzer/trackanalysisscheduler.h +++ b/src/analyzer/trackanalysisscheduler.h @@ -26,13 +26,13 @@ class TrackAnalysisScheduler : public QObject { Library* library, int numWorkerThreads, const UserSettingsPointer& pConfig, - AnalyzerMode mode = AnalyzerMode::Default); + AnalyzerModeFlags modeFlags); /*private*/ TrackAnalysisScheduler( Library* library, int numWorkerThreads, const UserSettingsPointer& pConfig, - AnalyzerMode mode); + AnalyzerModeFlags modeFlags); ~TrackAnalysisScheduler() override; // Stops a running analysis and discards all enqueued tracks. diff --git a/src/library/analysisfeature.cpp b/src/library/analysisfeature.cpp index 07edeb9bcec..9d9e77e42f5 100644 --- a/src/library/analysisfeature.cpp +++ b/src/library/analysisfeature.cpp @@ -25,13 +25,18 @@ namespace { const int kNumberOfAnalyzerThreads = math_max(1, QThread::idealThreadCount()); inline -AnalyzerMode getAnalyzerMode( +AnalyzerModeFlags getAnalyzerModeFlags( const UserSettingsPointer& pConfig) { + // Always enable at least BPM detection for batch analysis, even if disabled + // in the config for ad-hoc analysis of tracks. + // NOTE(uklotzde, 2018-12-26): The previous comment just states the status-quo + // of the existing code. We should rethink the configuration of analyzers when + // refactoring/redesigning the analyzer framework. + int modeFlags = AnalyzerModeFlags::WithBeats; if (pConfig->getValue(ConfigKey("[Library]", "EnableWaveformGenerationWithAnalysis"), true)) { - return AnalyzerMode::WithBeats; - } else { - return AnalyzerMode::WithBeatsWithoutWaveform; + modeFlags |= AnalyzerModeFlags::WithWaveform; } + return static_cast(modeFlags); } } // anonymous namespace @@ -128,7 +133,7 @@ void AnalysisFeature::analyzeTracks(QList trackIds) { m_library, kNumberOfAnalyzerThreads, m_pConfig, - getAnalyzerMode(m_pConfig)); + getAnalyzerModeFlags(m_pConfig)); connect(m_pTrackAnalysisScheduler.get(), &TrackAnalysisScheduler::progress, m_pAnalysisView, &DlgAnalysis::onTrackAnalysisSchedulerProgress); diff --git a/src/mixer/playermanager.cpp b/src/mixer/playermanager.cpp index a47dc9de9aa..3a3775b5004 100644 --- a/src/mixer/playermanager.cpp +++ b/src/mixer/playermanager.cpp @@ -119,7 +119,8 @@ void PlayerManager::bindToLibrary(Library* pLibrary) { m_pTrackAnalysisScheduler = TrackAnalysisScheduler::createInstance( pLibrary, kNumberOfAnalyzerThreads, - m_pConfig); + m_pConfig, + AnalyzerModeFlags::WithWaveform); connect(m_pTrackAnalysisScheduler.get(), &TrackAnalysisScheduler::trackProgress, this, &PlayerManager::onTrackAnalysisProgress); From 9578bf6dc72811c16280c033bc34064a6b1cb973 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 26 Dec 2018 17:32:45 +0100 Subject: [PATCH 19/25] Rename functions: "receive()" -> "on()" --- src/analyzer/trackanalysisscheduler.cpp | 8 ++++---- src/analyzer/trackanalysisscheduler.h | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/analyzer/trackanalysisscheduler.cpp b/src/analyzer/trackanalysisscheduler.cpp index 9ed3c3ef335..d88809b1c4b 100644 --- a/src/analyzer/trackanalysisscheduler.cpp +++ b/src/analyzer/trackanalysisscheduler.cpp @@ -180,21 +180,21 @@ void TrackAnalysisScheduler::onWorkerThreadProgress( case AnalyzerThreadState::Void: break; case AnalyzerThreadState::Idle: - worker.receiveThreadIdle(); + worker.onThreadIdle(); submitNextTrack(&worker); break; case AnalyzerThreadState::Busy: - worker.receiveAnalyzerProgress(trackId, analyzerProgress); + worker.onAnalyzerProgress(trackId, analyzerProgress); emit trackProgress(trackId, analyzerProgress); break; case AnalyzerThreadState::Done: - worker.receiveAnalyzerProgress(trackId, analyzerProgress); + worker.onAnalyzerProgress(trackId, analyzerProgress); emit trackProgress(trackId, analyzerProgress); ++m_finishedTracksCount; DEBUG_ASSERT(m_finishedTracksCount <= m_dequeuedTracksCount); break; case AnalyzerThreadState::Exit: - worker.receiveThreadExit(); + worker.onThreadExit(); DEBUG_ASSERT(!worker); break; default: diff --git a/src/analyzer/trackanalysisscheduler.h b/src/analyzer/trackanalysisscheduler.h index 7d173a12514..2aedaea29b6 100644 --- a/src/analyzer/trackanalysisscheduler.h +++ b/src/analyzer/trackanalysisscheduler.h @@ -128,20 +128,20 @@ class TrackAnalysisScheduler : public QObject { } } - void receiveThreadIdle() { + void onThreadIdle() { DEBUG_ASSERT(m_thread); DEBUG_ASSERT(!m_threadIdle); m_threadIdle = true; m_analyzerProgress = kAnalyzerProgressUnknown; } - void receiveAnalyzerProgress(TrackId /*trackId*/, AnalyzerProgress analyzerProgress) { + void onAnalyzerProgress(TrackId /*trackId*/, AnalyzerProgress analyzerProgress) { DEBUG_ASSERT(m_thread); DEBUG_ASSERT(!m_threadIdle); m_analyzerProgress = analyzerProgress; } - void receiveThreadExit() { + void onThreadExit() { DEBUG_ASSERT(m_thread); m_thread.reset(); m_threadIdle = false; From 4ba1f39c9be165679bf40ee03dcdd89dde54130f Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Wed, 26 Dec 2018 18:41:03 +0100 Subject: [PATCH 20/25] Don't store a plain pointer as member --- src/analyzer/analyzerthread.cpp | 32 ++++++++++++------------------- src/analyzer/analyzerwaveform.cpp | 28 +++++++++++++-------------- src/analyzer/analyzerwaveform.h | 10 ++++++---- src/test/analyserwaveformtest.cpp | 4 +--- 4 files changed, 33 insertions(+), 41 deletions(-) diff --git a/src/analyzer/analyzerthread.cpp b/src/analyzer/analyzerthread.cpp index a86414850b1..7f2b490b614 100644 --- a/src/analyzer/analyzerthread.cpp +++ b/src/analyzer/analyzerthread.cpp @@ -100,9 +100,19 @@ AnalyzerThread::AnalyzerThread( void AnalyzerThread::doRun() { std::unique_ptr pAnalysisDao; + // The thread-local database connection must not be closed + // before returning from this function. + mixxx::DbConnectionPooler dbConnectionPooler; + if (m_modeFlags & AnalyzerModeFlags::WithWaveform) { - pAnalysisDao = std::make_unique(m_pConfig); - m_analyzers.push_back(std::make_unique(pAnalysisDao.get())); + dbConnectionPooler = mixxx::DbConnectionPooler(m_dbConnectionPool); // move assignment + if (!dbConnectionPooler.isPooling()) { + kLogger.warning() + << "Failed to obtain database connection for analyzer thread"; + return; + } + QSqlDatabase dbConnection = mixxx::DbConnectionPooled(m_dbConnectionPool); + m_analyzers.push_back(std::make_unique(m_pConfig, dbConnection)); } if (AnalyzerGain::isEnabled(ReplayGainSettings(m_pConfig))) { m_analyzers.push_back(std::make_unique(m_pConfig)); @@ -122,24 +132,6 @@ void AnalyzerThread::doRun() { m_lastBusyProgressEmittedTimer.start(); - // This thread-local database connection for pAnalysisDao - // must not be closed before returning from this function. - // Therefore the DbConnectionPooler is defined outside of - // the conditional if block. - mixxx::DbConnectionPooler dbConnectionPooler; - if (pAnalysisDao) { - dbConnectionPooler = mixxx::DbConnectionPooler(m_dbConnectionPool); // 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_dbConnectionPool); - DEBUG_ASSERT(dbConnection.isOpen()); - pAnalysisDao->initialize(dbConnection); - } - mixxx::AudioSource::OpenParams openParams; openParams.setChannelCount(kAnalysisChannels); diff --git a/src/analyzer/analyzerwaveform.cpp b/src/analyzer/analyzerwaveform.cpp index c610e1d8a07..11c4da93b09 100644 --- a/src/analyzer/analyzerwaveform.cpp +++ b/src/analyzer/analyzerwaveform.cpp @@ -4,7 +4,6 @@ #include "engine/enginefilterbutterworth8.h" #include "engine/enginefilterbessel4.h" #include "library/trackcollection.h" -#include "library/dao/analysisdao.h" #include "track/track.h" #include "waveform/waveformfactory.h" #include "util/logger.h" @@ -16,18 +15,19 @@ mixxx::Logger kLogger("AnalyzerWaveform"); } // anonymous AnalyzerWaveform::AnalyzerWaveform( - 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 + UserSettingsPointer pConfig, + QSqlDatabase dbConnection) + : m_analysisDao(pConfig), + m_skipProcessing(false), + m_waveformData(nullptr), + m_waveformSummaryData(nullptr), + m_stride(0, 0), + m_currentStride(0), + m_currentSummaryStride(0) { m_filter[0] = 0; m_filter[1] = 0; m_filter[2] = 0; + m_analysisDao.initialize(dbConnection); } AnalyzerWaveform::~AnalyzerWaveform() { @@ -103,7 +103,7 @@ bool AnalyzerWaveform::isDisabledOrLoadStoredSuccess(TrackPointer tio) const { if (trackId.isValid() && (missingWaveform || missingWavesummary)) { QList analyses = - m_pAnalysisDao->getAnalysesForTrack(trackId); + m_analysisDao.getAnalysesForTrack(trackId); QListIterator it(analyses); while (it.hasNext()) { @@ -118,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_pAnalysisDao->deleteAnalysis(analysis.analysisId); + m_analysisDao.deleteAnalysis(analysis.analysisId); } } if (analysis.type == AnalysisDao::TYPE_WAVESUMMARY) { vc = WaveformFactory::waveformSummaryVersionToVersionClass(analysis.version); @@ -128,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_pAnalysisDao->deleteAnalysis(analysis.analysisId); + m_analysisDao.deleteAnalysis(analysis.analysisId); } } } @@ -310,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_pAnalysisDao->saveTrackAnalyses( + m_analysisDao.saveTrackAnalyses( tio->getId(), tio->getWaveform(), tio->getWaveformSummary()); diff --git a/src/analyzer/analyzerwaveform.h b/src/analyzer/analyzerwaveform.h index fe1834e56ec..f448766933b 100644 --- a/src/analyzer/analyzerwaveform.h +++ b/src/analyzer/analyzerwaveform.h @@ -2,11 +2,13 @@ #define ANALYZER_ANALYZERWAVEFORM_H #include +#include #include #include "analyzer/analyzer.h" #include "waveform/waveform.h" +#include "library/dao/analysisdao.h" #include "util/math.h" #include "util/performancetimer.h" @@ -14,7 +16,6 @@ //#define TEST_HEAT_MAP class EngineFilterIIRBase; -class AnalysisDao; inline CSAMPLE scaleSignal(CSAMPLE invalue, FilterIndex index = FilterCount) { if (invalue == 0.0) { @@ -136,8 +137,9 @@ struct WaveformStride { class AnalyzerWaveform : public Analyzer { public: - explicit AnalyzerWaveform( - AnalysisDao* pAnalysisDao); + AnalyzerWaveform( + UserSettingsPointer pConfig, + QSqlDatabase dbConnection); ~AnalyzerWaveform() override; bool initialize(TrackPointer tio, int sampleRate, int totalSamples) override; @@ -154,7 +156,7 @@ class AnalyzerWaveform : public Analyzer { void destroyFilters(); void storeIfGreater(float* pDest, float source); - AnalysisDao* m_pAnalysisDao; + mutable AnalysisDao m_analysisDao; bool m_skipProcessing; diff --git a/src/test/analyserwaveformtest.cpp b/src/test/analyserwaveformtest.cpp index 8e40121dbb8..38607490ade 100644 --- a/src/test/analyserwaveformtest.cpp +++ b/src/test/analyserwaveformtest.cpp @@ -18,8 +18,7 @@ namespace { class AnalyzerWaveformTest: public MixxxTest { protected: AnalyzerWaveformTest() - : analysisDao(config()), - aw(&analysisDao), + : aw(config(), QSqlDatabase()), bigbuf(nullptr), canaryBigBuf(nullptr) { } @@ -50,7 +49,6 @@ class AnalyzerWaveformTest: public MixxxTest { } protected: - AnalysisDao analysisDao; AnalyzerWaveform aw; TrackPointer tio; CSAMPLE* bigbuf; From 78cef8f73aacf923bbc2e8058c6d2eaa9c9607d6 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 28 Dec 2018 12:03:49 +0100 Subject: [PATCH 21/25] Fix comments and add debug logs --- src/analyzer/analyzerthread.cpp | 2 ++ src/track/globaltrackcache.h | 2 +- src/util/workerthread.cpp | 2 +- src/util/workerthread.h | 2 +- 4 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/analyzer/analyzerthread.cpp b/src/analyzer/analyzerthread.cpp index 7f2b490b614..28024851005 100644 --- a/src/analyzer/analyzerthread.cpp +++ b/src/analyzer/analyzerthread.cpp @@ -189,10 +189,12 @@ void AnalyzerThread::doRun() { emitDoneProgress(kAnalyzerProgressDone); } } + DEBUG_ASSERT(!m_currentTrack); DEBUG_ASSERT(isStopping()); m_analyzers.clear(); + kLogger.debug() << "Exiting worker thread"; emitProgress(AnalyzerThreadState::Exit); } diff --git a/src/track/globaltrackcache.h b/src/track/globaltrackcache.h index bc4caa30321..12c7da4daf0 100644 --- a/src/track/globaltrackcache.h +++ b/src/track/globaltrackcache.h @@ -34,7 +34,7 @@ class /*interface*/ GlobalTrackCacheRelocator { class GlobalTrackCacheEntry final { // We need to hold two shared pointers, the deletingPtr is - // responsible for the lifetime of the Track object itselfe. + // responsible for the lifetime of the Track object itself. // The second one counts the references outside Mixxx, if it // is not longer referenced, the track is saved and evicted // from the cache. diff --git a/src/util/workerthread.cpp b/src/util/workerthread.cpp index df5cefcf250..036107e504c 100644 --- a/src/util/workerthread.cpp +++ b/src/util/workerthread.cpp @@ -78,7 +78,7 @@ void WorkerThread::resume() { if (m_suspend.compare_exchange_strong(suspended, false)) { logTrace(m_logger, "Resuming"); // The thread might just be preparing to suspend after - // reading detecting that m_suspend was true. To avoid + // loading and detecting that m_suspend was true. To avoid // a race condition we need to acquire the mutex that // is associated with the wait condition, before // signalling the condition. Otherwise the signal diff --git a/src/util/workerthread.h b/src/util/workerthread.h index 04eebcbfb1a..bc8f387ea39 100644 --- a/src/util/workerthread.h +++ b/src/util/workerthread.h @@ -12,7 +12,7 @@ // A worker thread without an event loop. // // This object lives in the creating thread of the host, i.e. does not -// run its own event loop. It does not does not use slots for communication +// run its own event loop. It does not use slots for communication // with its host which would otherwise still be executed in the host's // thread. // From dcf6301b457ebe60c1a08c9b3a433d304ef34e6a Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 28 Dec 2018 12:07:06 +0100 Subject: [PATCH 22/25] Stop worker threads before exiting --- src/analyzer/trackanalysisscheduler.cpp | 2 ++ src/library/analysisfeature.cpp | 7 ++++--- src/library/analysisfeature.h | 4 +++- src/library/library.cpp | 8 ++++++++ src/library/library.h | 2 ++ src/mixer/playermanager.cpp | 5 ++++- src/mixxx.cpp | 3 +++ 7 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/analyzer/trackanalysisscheduler.cpp b/src/analyzer/trackanalysisscheduler.cpp index d88809b1c4b..cb48a52e83b 100644 --- a/src/analyzer/trackanalysisscheduler.cpp +++ b/src/analyzer/trackanalysisscheduler.cpp @@ -264,9 +264,11 @@ bool TrackAnalysisScheduler::submitNextTrack(Worker* worker) { void TrackAnalysisScheduler::stop() { kLogger.debug() << "Stopping"; + m_queuedTrackIds.clear(); for (auto& worker: m_workers) { worker.stopThread(); } + m_workers.clear(); } TrackPointer TrackAnalysisScheduler::loadTrackById(TrackId trackId) { diff --git a/src/library/analysisfeature.cpp b/src/library/analysisfeature.cpp index 9d9e77e42f5..83de1a8b621 100644 --- a/src/library/analysisfeature.cpp +++ b/src/library/analysisfeature.cpp @@ -54,9 +54,10 @@ AnalysisFeature::AnalysisFeature( setTitleDefault(); } -AnalysisFeature::~AnalysisFeature() { - // TODO(XXX) delete these - //delete m_pLibraryTableModel; +void AnalysisFeature::stop() { + if (m_pTrackAnalysisScheduler) { + m_pTrackAnalysisScheduler->stop(); + } } void AnalysisFeature::setTitleDefault() { diff --git a/src/library/analysisfeature.h b/src/library/analysisfeature.h index 29d97f7be2c..89690d27023 100644 --- a/src/library/analysisfeature.h +++ b/src/library/analysisfeature.h @@ -26,7 +26,9 @@ class AnalysisFeature : public LibraryFeature { public: AnalysisFeature(Library* parent, UserSettingsPointer pConfig); - virtual ~AnalysisFeature(); + ~AnalysisFeature() override = default; + + void stop(); QVariant title(); QIcon getIcon(); diff --git a/src/library/library.cpp b/src/library/library.cpp index 8a07715b7ca..d95416941dc 100644 --- a/src/library/library.cpp +++ b/src/library/library.cpp @@ -211,6 +211,14 @@ Library::~Library() { delete m_pTrackCollection; } +void Library::stopFeatures() { + if (m_pAnalysisFeature) { + m_pAnalysisFeature->stop(); + m_pAnalysisFeature = nullptr; + } + m_scanner.slotCancel(); +} + void Library::bindSidebarWidget(WLibrarySidebar* pSidebarWidget) { m_pLibraryControl->bindSidebarWidget(pSidebarWidget); diff --git a/src/library/library.h b/src/library/library.h index 70b51e1d7d0..fa686a1406f 100644 --- a/src/library/library.h +++ b/src/library/library.h @@ -51,6 +51,8 @@ class Library: public QObject, RecordingManager* pRecordingManager); ~Library() override; + void stopFeatures(); + mixxx::DbConnectionPoolPtr dbConnectionPool() const { return m_pDbConnectionPool; } diff --git a/src/mixer/playermanager.cpp b/src/mixer/playermanager.cpp index 3a3775b5004..d2b7bb4d56c 100644 --- a/src/mixer/playermanager.cpp +++ b/src/mixer/playermanager.cpp @@ -103,7 +103,10 @@ PlayerManager::~PlayerManager() { delete m_pCONumMicrophones; delete m_pCONumAuxiliaries; - m_pTrackAnalysisScheduler.reset(); + if (m_pTrackAnalysisScheduler) { + m_pTrackAnalysisScheduler->stop(); + m_pTrackAnalysisScheduler.reset(); + } } void PlayerManager::bindToLibrary(Library* pLibrary) { diff --git a/src/mixxx.cpp b/src/mixxx.cpp index 69a03c454ca..438f4ed0b9d 100644 --- a/src/mixxx.cpp +++ b/src/mixxx.cpp @@ -603,6 +603,9 @@ void MixxxMainWindow::finalize() { qWarning() << "WMainMenuBar was not deleted by our sendPostedEvents trick."; } + qDebug() << t.elapsed(false).debugMillisWithUnit() << "stopping pending Library tasks"; + m_pLibrary->stopFeatures(); + // SoundManager depend on Engine and Config qDebug() << t.elapsed(false).debugMillisWithUnit() << "deleting SoundManager"; delete m_pSoundManager; From d7bc6fbf94a667881e2dda9776398b65bfc9f722 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 28 Dec 2018 12:10:42 +0100 Subject: [PATCH 23/25] Prevent a deadlock when exiting main() --- src/mixxx.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/mixxx.cpp b/src/mixxx.cpp index 438f4ed0b9d..6c01cdb2691 100644 --- a/src/mixxx.cpp +++ b/src/mixxx.cpp @@ -728,6 +728,10 @@ void MixxxMainWindow::finalize() { // Report the total time we have been running. m_runtime_timer.elapsed(true); StatsManager::destroy(); + + // NOTE(uklotzde, 2018-12-28): Finally destroy the singleton instance + // to prevent a when while exiting the main() function! + GlobalTrackCache::destroyInstance(); } bool MixxxMainWindow::initializeDatabase() { From 96136a3e96c3dbbd85b09208e916eec65762cf77 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 28 Dec 2018 17:42:55 +0100 Subject: [PATCH 24/25] Reword and amend comment --- src/mixxx.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/mixxx.cpp b/src/mixxx.cpp index 6c01cdb2691..e78c1477bb4 100644 --- a/src/mixxx.cpp +++ b/src/mixxx.cpp @@ -730,7 +730,8 @@ void MixxxMainWindow::finalize() { StatsManager::destroy(); // NOTE(uklotzde, 2018-12-28): Finally destroy the singleton instance - // to prevent a when while exiting the main() function! + // to prevent a deadlock when exiting the main() function! The actual + // cause of the deadlock is still unclear. GlobalTrackCache::destroyInstance(); } From 78d2db8c32a8251552f34aada2118de0df537c90 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Sat, 29 Dec 2018 10:11:11 +0100 Subject: [PATCH 25/25] Fix use after free error --- src/analyzer/trackanalysisscheduler.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/analyzer/trackanalysisscheduler.cpp b/src/analyzer/trackanalysisscheduler.cpp index cb48a52e83b..7624a71417e 100644 --- a/src/analyzer/trackanalysisscheduler.cpp +++ b/src/analyzer/trackanalysisscheduler.cpp @@ -268,7 +268,8 @@ void TrackAnalysisScheduler::stop() { for (auto& worker: m_workers) { worker.stopThread(); } - m_workers.clear(); + // The worker threads are still running at this point + // and m_workers must not be modified! } TrackPointer TrackAnalysisScheduler::loadTrackById(TrackId trackId) {