Fix debug assert in CachingReader

src/engine/cachingreader.cpp

@@ -56,12 +56,12 @@ CachingReader::CachingReader(QString group,
           // The capacity of the back channel must be equal to the number of
           // allocated chunks, because the worker use writeBlocking(). Otherwise
           // the worker could get stuck in a hot loop!!!
-          m_stateFIFO(kNumberOfCachedChunksInMemory),
-          m_state(State::Idle),
+          m_readerStatusUpdateFIFO(kNumberOfCachedChunksInMemory),
+          m_state(STATE_IDLE),
           m_mruCachingReaderChunk(nullptr),
           m_lruCachingReaderChunk(nullptr),
           m_sampleBuffer(CachingReaderChunk::kSamples * kNumberOfCachedChunksInMemory),
-          m_worker(group, &m_chunkReadRequestFIFO, &m_stateFIFO) {
+          m_worker(group, &m_chunkReadRequestFIFO, &m_readerStatusUpdateFIFO) {
 
     m_allocatedCachingReaderChunks.reserve(kNumberOfCachedChunksInMemory);
     // Divide up the allocated raw memory buffer into total_chunks
@@ -200,43 +200,44 @@ CachingReaderChunkForOwner* CachingReader::lookupChunkAndFreshen(SINT chunkIndex
     return pChunk;
 }
 
+// Invoked from the UI thread!!
 void CachingReader::newTrack(TrackPointer pTrack) {
-    // Feed the track to the worker as soon as possible
-    // to get ready while the reader switches its internal
-    // state. There are no race conditions, because the
-    // reader polls the worker.
-    m_worker.newTrack(pTrack);
-    m_worker.workReady();
-    // Don't accept any new read requests until the current
-    // track has been unloaded and the new track has been
-    // loaded.
-    m_state = State::TrackLoading;
-    // Free all chunks with sample data from the current track.
-    freeAllChunks();
+    auto newState = pTrack ? STATE_TRACK_LOADING : STATE_TRACK_UNLOADING;
+    auto oldState = m_state.fetchAndStoreAcquire(newState);
+
+    // TODO():
+    // BaseTrackPlayerImpl::slotLoadTrack() distributes the new track via
+    // emit(loadingTrack(pNewTrack, pOldTrack));
+    // but the newTrack may change if we load a new track while the previous one
+    // is still loading. This leads to inconsistent states for example a different
+    // track in the Mixx Title and the Deck label.
+    if (oldState == STATE_TRACK_LOADING &&
+            newState == STATE_TRACK_LOADING) {
+        kLogger.warning()
+                << "Loading a new track while loading a track may lead to inconsistent states";
+    }
+    m_worker.newTrack(std::move(pTrack));
 }
 
 void CachingReader::process() {
     ReaderStatusUpdate update;
-    while (m_stateFIFO.read(&update, 1) == 1) {
-        DEBUG_ASSERT(m_state != State::Idle);
+    while (m_readerStatusUpdateFIFO.read(&update, 1) == 1) {
         auto pChunk = update.takeFromWorker();
         if (pChunk) {
             // Result of a read request (with a chunk)
+            DEBUG_ASSERT(m_state.load() != STATE_IDLE);
             DEBUG_ASSERT(
                     update.status == CHUNK_READ_SUCCESS ||
                     update.status == CHUNK_READ_EOF ||
                     update.status == CHUNK_READ_INVALID ||
                     update.status == CHUNK_READ_DISCARDED);
-            if (m_state == State::TrackLoading) {
-                // All chunks have been freed before loading the next track!
-                DEBUG_ASSERT(!m_mruCachingReaderChunk);
-                DEBUG_ASSERT(!m_lruCachingReaderChunk);
+            if (m_state.load() == STATE_TRACK_LOADING) {
                 // Discard all results from pending read requests for the
                 // previous track before the next track has been loaded.
                 freeChunk(pChunk);
                 continue;
             }
-            DEBUG_ASSERT(m_state == State::TrackLoaded);
+            DEBUG_ASSERT(m_state.load() == STATE_TRACK_LOADED);
             if (update.status == CHUNK_READ_SUCCESS) {
                 // Insert or freshen the chunk in the MRU/LRU list after
                 // obtaining ownership from the worker.
@@ -253,16 +254,33 @@ void CachingReader::process() {
             }
         } else {
             // State update (without a chunk)
-            DEBUG_ASSERT(!m_mruCachingReaderChunk);
-            DEBUG_ASSERT(!m_lruCachingReaderChunk);
             if (update.status == TRACK_LOADED) {
-                m_state = State::TrackLoaded;
+                // We have a new Track ready to go.
+                // Assert that we either have had STATE_TRACK_LOADING before and all
+                // chunks in the m_readerStatusUpdateFIFO have been discarded.
+                // or the cache has been already cleared.
+                // In case of two consecutive load events, we receive two consecutive
+                // TRACK_LOADED without a chunk in between, assert this here.
+                DEBUG_ASSERT(m_state.load() == STATE_TRACK_LOADING ||
+                        (m_state.load() == STATE_TRACK_LOADED &&
+                         !m_mruCachingReaderChunk && !m_lruCachingReaderChunk));
+                // now purge also the recently used chunk list from the old track.
+                if (m_mruCachingReaderChunk || m_lruCachingReaderChunk) {
+                    DEBUG_ASSERT(m_state.load() == STATE_TRACK_LOADING);
+                    freeAllChunks();
+                }
+                // Reset the readable frame index range
+                m_readableFrameIndexRange = update.readableFrameIndexRange();
+                m_state.storeRelease(STATE_TRACK_LOADED);
             } else {
                 DEBUG_ASSERT(update.status == TRACK_UNLOADED);
-                m_state = State::Idle;
+                // This message could be processed later when a new
+                // track is already loading! In this case the TRACK_LOADED will
+                // be the very next status update.
+                if (!m_state.testAndSetRelease(STATE_TRACK_UNLOADING, STATE_IDLE)) {
+                    DEBUG_ASSERT(m_state.load() == STATE_TRACK_LOADING);
+                }
             }
-            // Reset the readable frame index range
-            m_readableFrameIndexRange = update.readableFrameIndexRange();
         }
     }
 }
@@ -286,7 +304,7 @@ CachingReader::ReadResult CachingReader::read(SINT startSample, SINT numSamples,
     }
 
     // If no track is loaded, don't do anything.
-    if (m_state != State::TrackLoaded) {
+    if (m_state.load() != STATE_TRACK_LOADED) {
         return ReadResult::UNAVAILABLE;
     }
 
@@ -483,7 +501,7 @@ CachingReader::ReadResult CachingReader::read(SINT startSample, SINT numSamples,
 
 void CachingReader::hintAndMaybeWake(const HintVector& hintList) {
     // If no file is loaded, skip.
-    if (m_state != State::TrackLoaded) {
+    if (m_state.load() != STATE_TRACK_LOADED) {
         return;
     }
 

src/engine/cachingreader.h

@@ -4,7 +4,7 @@
 #ifndef ENGINE_CACHINGREADER_H
 #define ENGINE_CACHINGREADER_H
 
-#include <QtDebug>
+#include <QAtomicInt>
 #include <QList>
 #include <QVector>
 #include <QLinkedList>
@@ -78,9 +78,9 @@ class CachingReader : public QObject {
     // Construct a CachingReader with the given group.
     CachingReader(QString group,
                   UserSettingsPointer _config);
-    virtual ~CachingReader();
+    ~CachingReader() override;
 
-    virtual void process();
+    void process();
 
     enum class ReadResult {
         // No samples read and buffer untouched(!), try again later in case of a cache miss
@@ -101,12 +101,12 @@ class CachingReader : public QObject {
     // that is not in the cache. If any hints do request a chunk not in cache,
     // then wake the reader so that it can process them. Must only be called
     // from the engine callback.
-    virtual void hintAndMaybeWake(const HintVector& hintList);
+    void hintAndMaybeWake(const HintVector& hintList);
 
     // Request that the CachingReader load a new track. These requests are
     // processed in the work thread, so the reader must be woken up via wake()
     // for this to take effect.
-    virtual void newTrack(TrackPointer pTrack);
+    void newTrack(TrackPointer pTrack);
 
     void setScheduler(EngineWorkerScheduler* pScheduler) {
         m_worker.setScheduler(pScheduler);
@@ -124,7 +124,7 @@ class CachingReader : public QObject {
     // Thread-safe FIFOs for communication between the engine callback and
     // reader thread.
     FIFO<CachingReaderChunkReadRequest> m_chunkReadRequestFIFO;
-    FIFO<ReaderStatusUpdate> m_stateFIFO;
+    FIFO<ReaderStatusUpdate> m_readerStatusUpdateFIFO;
 
     // Looks for the provided chunk number in the index of in-memory chunks and
     // returns it if it is present. If not, returns nullptr. If it is present then
@@ -151,12 +151,13 @@ class CachingReader : public QObject {
     // Gets a chunk from the free list, frees the LRU CachingReaderChunk if none available.
     CachingReaderChunkForOwner* allocateChunkExpireLRU(SINT chunkIndex);
 
-    enum class State {
-        Idle,
-        TrackLoading,
-        TrackLoaded,
+    enum State {
+        STATE_IDLE,
+        STATE_TRACK_LOADING,
+        STATE_TRACK_UNLOADING,
+        STATE_TRACK_LOADED,
     };
-    State m_state;
+    QAtomicInt m_state;
 
     // Keeps track of all CachingReaderChunks we've allocated.
     QVector<CachingReaderChunkForOwner*> m_chunks;

src/engine/cachingreaderworker.cpp

@@ -29,9 +29,6 @@ CachingReaderWorker::CachingReaderWorker(
           m_stop(0) {
 }
 
-CachingReaderWorker::~CachingReaderWorker() {
-}
-
 ReaderStatusUpdate CachingReaderWorker::processReadRequest(
         const CachingReaderChunkReadRequest& request) {
     CachingReaderChunk* pChunk = request.chunk;
@@ -87,9 +84,12 @@ ReaderStatusUpdate CachingReaderWorker::processReadRequest(
 
 // WARNING: Always called from a different thread (GUI)
 void CachingReaderWorker::newTrack(TrackPointer pTrack) {
-    QMutexLocker locker(&m_newTrackMutex);
-    m_pNewTrack = pTrack;
-    m_newTrackAvailable = true;
+    {
+        QMutexLocker locker(&m_newTrackMutex);
+        m_pNewTrack = pTrack;
+        m_newTrackAvailable = true;
+    }
+    workReady();
 }
 
 void CachingReaderWorker::run() {
@@ -135,7 +135,7 @@ void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) {
 
     if (!pTrack) {
         // If no new track is available then we are done
-        const auto update = ReaderStatusUpdate::trackNotLoaded();
+        const auto update = ReaderStatusUpdate::trackUnloaded();
         m_pReaderStatusFIFO->writeBlocking(&update, 1);
         return;
     }
@@ -149,7 +149,7 @@ void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) {
                  << m_group
                  << "File not found"
                  << filename;
-        const auto update = ReaderStatusUpdate::trackNotLoaded();
+        const auto update = ReaderStatusUpdate::trackUnloaded();
         m_pReaderStatusFIFO->writeBlocking(&update, 1);
         emit trackLoadFailed(
             pTrack, QString("The file '%1' could not be found.")
@@ -165,7 +165,7 @@ void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) {
                 << m_group
                 << "Failed to open file"
                 << filename;
-        const auto update = ReaderStatusUpdate::trackNotLoaded();
+        const auto update = ReaderStatusUpdate::trackUnloaded();
         m_pReaderStatusFIFO->writeBlocking(&update, 1);
         emit trackLoadFailed(
             pTrack, QString("The file '%1' could not be loaded").arg(filename));
@@ -182,7 +182,7 @@ void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) {
                 << m_group
                 << "Failed to open empty file"
                 << filename;
-        const auto update = ReaderStatusUpdate::trackNotLoaded();
+        const auto update = ReaderStatusUpdate::trackUnloaded();
         m_pReaderStatusFIFO->writeBlocking(&update, 1);
         emit trackLoadFailed(
             pTrack, QString("The file '%1' is empty and could not be loaded").arg(filename));

src/engine/cachingreaderworker.h

@@ -69,7 +69,7 @@ typedef struct ReaderStatusUpdate {
         return update;
     }
 
-    static ReaderStatusUpdate trackNotLoaded() {
+    static ReaderStatusUpdate trackUnloaded() {
         ReaderStatusUpdate update;
         update.init(TRACK_UNLOADED, nullptr, mixxx::IndexRange());
         return update;
@@ -101,14 +101,14 @@ class CachingReaderWorker : public EngineWorker {
     CachingReaderWorker(QString group,
             FIFO<CachingReaderChunkReadRequest>* pChunkReadRequestFIFO,
             FIFO<ReaderStatusUpdate>* pReaderStatusFIFO);
-    virtual ~CachingReaderWorker();
+    ~CachingReaderWorker() override = default;
 
     // Request to load a new track. wake() must be called afterwards.
-    virtual void newTrack(TrackPointer pTrack);
+    void newTrack(TrackPointer pTrack);
 
     // Run upkeep operations like loading tracks and reading from file. Run by a
     // thread pool via the EngineWorkerScheduler.
-    virtual void run();
+    void run() override;
 
     void quitWait();