Merge remote-tracking branch 'upstream/2.2'

11 files changed, 295 insertions, 171 deletions

diff --git a/README b/README
index 748a746d57..7294b784f1 100644
--- a/README
+++ b/README
@@ -39,6 +39,7 @@ Mixxx has the following dependencies:
 - libvorbis
 - libvorbisfile
 - libsndfile
+- libmodplug
 - libflac
 - libopus
 - libshout
diff --git a/build/debian/control b/build/debian/control
index d076fa2830..b45f8a52cb 100644
--- a/build/debian/control
+++ b/build/debian/control
@@ -49,6 +49,7 @@ Build-Depends: debhelper (>= 9),
                libhidapi-dev,
                libupower-glib-dev,
                liblilv-dev,
+               libmodplug-dev
                libmp3lame-dev,
 # for running mixxx-test
                xvfb
diff --git a/build/features.py b/build/features.py
index 51d489060f..31114c39fa 100644
--- a/build/features.py
+++ b/build/features.py
@@ -314,29 +314,42 @@ class ModPlug(Feature):
         return "Modplug module decoder plugin"
 
     def enabled(self, build):
-        build.flags['modplug'] = util.get_flags(build.env, 'modplug', 0)
+        # Default to enabled on but only throw an error if it was explicitly
+        # requested and is not available.
+        if 'modplug' in build.flags:
+            return int(build.flags['modplug']) > 0
+        build.flags['modplug'] = util.get_flags(build.env, 'modplug', 1)
         if int(build.flags['modplug']):
             return True
         return False
 
     def add_options(self, build, vars):
         vars.Add('modplug',
-                 'Set to 1 to enable libmodplug based module tracker support.', 0)
+                 'Set to 1 to enable libmodplug based module tracker support.',
+                 1)
 
     def configure(self, build, conf):
         if not self.enabled(build):
             return
 
-        build.env.Append(CPPDEFINES='__MODPLUG__')
+        # Only block the configure if modplug was explicitly requested.
+        explicit = 'modplug' in SCons.ARGUMENTS
 
-        have_modplug_h = conf.CheckHeader('libmodplug/modplug.h')
-        have_modplug = conf.CheckLib(['modplug', 'libmodplug'], autoadd=True)
+        if not conf.CheckHeader('libmodplug/modplug.h'):
+            if explicit:
+                raise Exception('Could not find libmodplug development headers.')
+            else:
+                build.flags['modplug'] = 0
+            return
 
-        if not have_modplug_h:
-            raise Exception('Could not find libmodplug development headers.')
+        if not conf.CheckLib(['modplug', 'libmodplug'], autoadd=True):
+            if explicit:
+                raise Exception('Could not find libmodplug shared library.')
+            else:
+                build.flags['modplug'] = 0
+            return
 
-        if not have_modplug:
-            raise Exception('Could not find libmodplug shared library.')
+        build.env.Append(CPPDEFINES='__MODPLUG__')
 
     def sources(self, build):
         depends.Qt.uic(build)('src/preferences/dialog/dlgprefmodplugdlg.ui')
diff --git a/src/engine/cachingreader/cachingreader.cpp b/src/engine/cachingreader/cachingreader.cpp
index 14b32b843b..2599d8bacd 100644
--- a/src/engine/cachingreader/cachingreader.cpp
+++ b/src/engine/cachingreader/cachingreader.cpp
@@ -21,9 +21,21 @@ mixxx::Logger kLogger("CachingReader");
 // TODO() Do we suffer cache misses if we use an audio buffer of above 23 ms?
 const SINT kDefaultHintFrames = 1024;
 
-// currently CachingReaderWorker::kCachingReaderChunkLength is 65536 (0x10000);
-// For 80 chunks we need 5242880 (0x500000) bytes (5 MiB) of Memory
-//static
+// With CachingReaderChunk::kFrames = 8192 each chunk consumes
+// 8192 frames * 2 channels/frame * 4-bytes per sample = 65 kB.
+//
+//     80 chunks ->  5120 KB =  5 MB
+//
+// Each deck (including sample decks) will use their own CachingReader.
+// Consequently the total memory required for all allocated chunks depends
+// on the number of decks. The amount of memory reserved for a single
+// CachingReader must be multiplied by the number of decks to calculate
+// the total amount!
+//
+// NOTE(uklotzde, 2019-09-05): Reduce this number to just few chunks
+// (kNumberOfCachedChunksInMemory = 1, 2, 3, ...) for testing purposes
+// to verify that the MRU/LRU cache works as expected. Even though
+// massive drop outs are expected to occur Mixxx should run reliably!
 const SINT kNumberOfCachedChunksInMemory = 80;
 
 } // anonymous namespace
@@ -32,8 +44,20 @@ const SINT kNumberOfCachedChunksInMemory = 80;
 CachingReader::CachingReader(QString group,
                              UserSettingsPointer config)
         : m_pConfig(config),
-          m_chunkReadRequestFIFO(1024),
-          m_readerStatusFIFO(1024),
+          // Limit the number of in-flight requests to the worker. This should
+          // prevent to overload the worker when it is not able to fetch those
+          // requests from the FIFO timely. Otherwise outdated requests pile up
+          // in the FIFO and it would take a long time to process them, just to
+          // discard the results that most likely have already become obsolete.
+          // TODO(XXX): Ideally the request FIFO would be implemented as a ring
+          // buffer, where new requests replace old requests when full. Those
+          // old requests need to be returned immediately to the CachingReader
+          // that must take ownership and free them!!!
+          m_chunkReadRequestFIFO(kNumberOfCachedChunksInMemory / 4),
+          // 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_readerStatusFIFO(kNumberOfCachedChunksInMemory),
           m_readerStatus(INVALID),
           m_mruCachingReaderChunk(nullptr),
           m_lruCachingReaderChunk(nullptr),
@@ -74,8 +98,16 @@ CachingReader::~CachingReader() {
     qDeleteAll(m_chunks);
 }
 
+void CachingReader::freeChunkFromList(CachingReaderChunkForOwner* pChunk) {
+    pChunk->removeFromList(
+            &m_mruCachingReaderChunk,
+            &m_lruCachingReaderChunk);
+    pChunk->free();
+    m_freeChunks.push_back(pChunk);
+}
+
 void CachingReader::freeChunk(CachingReaderChunkForOwner* pChunk) {
-    DEBUG_ASSERT(pChunk != nullptr);
+    DEBUG_ASSERT(pChunk);
     DEBUG_ASSERT(pChunk->getState() != CachingReaderChunkForOwner::READ_PENDING);
 
     const int removed = m_allocatedCachingReaderChunks.remove(pChunk->getIndex());
@@ -84,10 +116,7 @@ void CachingReader::freeChunk(CachingReaderChunkForOwner* pChunk) {
     // because sometime you free a chunk right after you allocated it.
     DEBUG_ASSERT(removed <= 1);
 
-    pChunk->removeFromList(
-            &m_mruCachingReaderChunk, &m_lruCachingReaderChunk);
-    pChunk->free();
-    m_freeChunks.push_back(pChunk);
+    freeChunkFromList(pChunk);
 }
 
 void CachingReader::freeAllChunks() {
@@ -99,15 +128,13 @@ void CachingReader::freeAllChunks() {
         }
 
         if (pChunk->getState() != CachingReaderChunkForOwner::FREE) {
-            pChunk->removeFromList(
-                    &m_mruCachingReaderChunk, &m_lruCachingReaderChunk);
-            pChunk->free();
-            m_freeChunks.push_back(pChunk);
+            freeChunkFromList(pChunk);
         }
     }
+    DEBUG_ASSERT(!m_mruCachingReaderChunk);
+    DEBUG_ASSERT(!m_lruCachingReaderChunk);
 
     m_allocatedCachingReaderChunks.clear();
-    m_mruCachingReaderChunk = nullptr;
 }
 
 CachingReaderChunkForOwner* CachingReader::allocateChunk(SINT chunkIndex) {
@@ -117,62 +144,59 @@ CachingReaderChunkForOwner* CachingReader::allocateChunk(SINT chunkIndex) {
     CachingReaderChunkForOwner* pChunk = m_freeChunks.takeFirst();
     pChunk->init(chunkIndex);
 
-    //kLogger.debug() << "Allocating chunk" << pChunk << pChunk->getIndex();
     m_allocatedCachingReaderChunks.insert(chunkIndex, pChunk);
 
     return pChunk;
 }
 
 CachingReaderChunkForOwner* CachingReader::allocateChunkExpireLRU(SINT chunkIndex) {
-    CachingReaderChunkForOwner* pChunk = allocateChunk(chunkIndex);
+    auto pChunk = allocateChunk(chunkIndex);
     if (!pChunk) {
-        if (m_lruCachingReaderChunk == nullptr) {
-            kLogger.warning() << "ERROR: No LRU chunk to free in allocateChunkExpireLRU.";
-            return nullptr;
+        if (m_lruCachingReaderChunk) {
+            freeChunk(m_lruCachingReaderChunk);
+            pChunk = allocateChunk(chunkIndex);
+        } else {
+            kLogger.warning() << "No cached LRU chunk available for freeing";
         }
-        freeChunk(m_lruCachingReaderChunk);
-        pChunk = allocateChunk(chunkIndex);
     }
-    //kLogger.debug() << "allocateChunkExpireLRU" << chunk << pChunk;
+    if (kLogger.traceEnabled()) {
+        kLogger.trace() << "allocateChunkExpireLRU" << chunkIndex << pChunk;
+    }
     return pChunk;
 }
 
 CachingReaderChunkForOwner* CachingReader::lookupChunk(SINT chunkIndex) {
     // Defaults to nullptr if it's not in the hash.
-    CachingReaderChunkForOwner* chunk = m_allocatedCachingReaderChunks.value(chunkIndex, nullptr);
-
-    // Make sure the allocated number matches the indexed chunk number.
-    DEBUG_ASSERT(chunk == nullptr || chunkIndex == chunk->getIndex());
-
-    return chunk;
+    auto pChunk = m_allocatedCachingReaderChunks.value(chunkIndex, nullptr);
+    DEBUG_ASSERT(!pChunk || pChunk->getIndex() == chunkIndex);
+    return pChunk;
 }
 
 void CachingReader::freshenChunk(CachingReaderChunkForOwner* pChunk) {
-    DEBUG_ASSERT(pChunk != nullptr);
-    DEBUG_ASSERT(pChunk->getState() != CachingReaderChunkForOwner::READ_PENDING);
-
-    // Remove the chunk from the LRU list
-    pChunk->removeFromList(&m_mruCachingReaderChunk, &m_lruCachingReaderChunk);
-
-    // Adjust the least-recently-used item before inserting the
-    // chunk as the new most-recently-used item.
-    if (m_lruCachingReaderChunk == nullptr) {
-        if (m_mruCachingReaderChunk == nullptr) {
-            m_lruCachingReaderChunk = pChunk;
-        } else {
-            m_lruCachingReaderChunk = m_mruCachingReaderChunk;
-        }
+    DEBUG_ASSERT(pChunk);
+    DEBUG_ASSERT(pChunk->getState() == CachingReaderChunkForOwner::READY);
+    if (kLogger.traceEnabled()) {
+        kLogger.trace()
+                << "freshenChunk()"
+                << pChunk->getIndex()
+                << pChunk;
     }
 
-    // Insert the chunk as the new most-recently-used item.
-    pChunk->insertIntoListBefore(m_mruCachingReaderChunk);
-    m_mruCachingReaderChunk = pChunk;
+    // Remove the chunk from the MRU/LRU list
+    pChunk->removeFromList(
+            &m_mruCachingReaderChunk,
+            &m_lruCachingReaderChunk);
+
+    // Reinsert has new head of MRU list
+    pChunk->insertIntoListBefore(
+            &m_mruCachingReaderChunk,
+            &m_lruCachingReaderChunk,
+            m_mruCachingReaderChunk);
 }
 
 CachingReaderChunkForOwner* CachingReader::lookupChunkAndFreshen(SINT chunkIndex) {
-    CachingReaderChunkForOwner* pChunk = lookupChunk(chunkIndex);
-    if (pChunk &&
-            (pChunk->getState() != CachingReaderChunkForOwner::READ_PENDING)) {
+    auto pChunk = lookupChunk(chunkIndex);
+    if (pChunk && (pChunk->getState() == CachingReaderChunkForOwner::READY)) {
         freshenChunk(pChunk);
     }
     return pChunk;
@@ -184,15 +208,11 @@ void CachingReader::newTrack(TrackPointer pTrack) {
 }
 
 void CachingReader::process() {
-    ReaderStatusUpdate status;
-    while (m_readerStatusFIFO.read(&status, 1) == 1) {
-        CachingReaderChunkForOwner* pChunk = static_cast<CachingReaderChunkForOwner*>(status.chunk);
+    ReaderStatusUpdate update;
+    while (m_readerStatusFIFO.read(&update, 1) == 1) {
+        auto pChunk = update.takeFromWorker();
         if (pChunk) {
-            // Take over control of the chunk from the worker.
-            // This has to be done before freeing all chunks
-            // after a new track has been loaded (see below)!
-            pChunk->takeFromWorker();
-            if (status.status == CHUNK_READ_SUCCESS) {
+            if (update.status == CHUNK_READ_SUCCESS) {
                 // Insert or freshen the chunk in the MRU/LRU list after
                 // obtaining ownership from the worker.
                 freshenChunk(pChunk);
@@ -201,12 +221,12 @@ void CachingReader::process() {
                 freeChunk(pChunk);
             }
         }
-        if (status.status == TRACK_NOT_LOADED) {
-            m_readerStatus = status.status;
-        } else if (status.status == TRACK_LOADED) {
-            m_readerStatus = status.status;
+        if (update.status == TRACK_NOT_LOADED) {
+            m_readerStatus = update.status;
+        } else if (update.status == TRACK_LOADED) {
+            m_readerStatus = update.status;
             // Reset the max. readable frame index
-            m_readableFrameIndexRange = status.readableFrameIndexRange();
+            m_readableFrameIndexRange = update.readableFrameIndexRange();
             // Free all chunks with sample data from a previous track
             freeAllChunks();
         }
@@ -214,7 +234,7 @@ void CachingReader::process() {
             // Adjust the readable frame index range after loading or reading
             m_readableFrameIndexRange = intersect(
                     m_readableFrameIndexRange,
-                    status.readableFrameIndexRange());
+                    update.readableFrameIndexRange());
         } else {
             // Reset the readable frame index range
             m_readableFrameIndexRange = mixxx::IndexRange();
@@ -230,9 +250,10 @@ CachingReader::ReadResult CachingReader::read(SINT startSample, SINT numSamples,
             // Refuse to read from an invalid number of samples
             (numSamples % CachingReaderChunk::kChannels == 0) && (numSamples >= 0)) {
         kLogger.critical()
-                << "read() invalid arguments:"
+                << "Invalid arguments for read():"
                 << "startSample =" << startSample
-                << "numSamples =" << numSamples;
+                << "numSamples =" << numSamples
+                << "reverse =" << reverse;
         return ReadResult::UNAVAILABLE;
     }
     VERIFY_OR_DEBUG_ASSERT(buffer) {
@@ -283,10 +304,12 @@ CachingReader::ReadResult CachingReader::read(SINT startSample, SINT numSamples,
                             remainingFrameIndexRange.start(),
                             m_readableFrameIndexRange.start());
             DEBUG_ASSERT(prerollFrameIndexRange.length() <= remainingFrameIndexRange.length());
-            kLogger.debug()
-                    << "Prepending"
-                    << prerollFrameIndexRange.length()
-                    << "frames of silence";
+            if (kLogger.traceEnabled()) {
+                kLogger.trace()
+                        << "Prepending"
+                        << prerollFrameIndexRange.length()
+                        << "frames of silence";
+            }
             const SINT prerollFrames = prerollFrameIndexRange.length();
             const SINT prerollSamples = CachingReaderChunk::frames2samples(prerollFrames);
             DEBUG_ASSERT(samplesRemaining >= prerollSamples);
@@ -475,27 +498,33 @@ void CachingReader::hintAndMaybeWake(const HintVector& hintList) {
         const int lastChunkIndex = CachingReaderChunk::indexForFrame(readableFrameIndexRange.end() - 1);
         for (int chunkIndex = firstChunkIndex; chunkIndex <= lastChunkIndex; ++chunkIndex) {
             CachingReaderChunkForOwner* pChunk = lookupChunk(chunkIndex);
-            if (pChunk == nullptr) {
+            if (!pChunk) {
                 shouldWake = true;
                 pChunk = allocateChunkExpireLRU(chunkIndex);
-                if (pChunk == nullptr) {
-                    kLogger.warning() << "ERROR: Couldn't allocate spare CachingReaderChunk to make CachingReaderChunkReadRequest.";
+                if (!pChunk) {
+                    kLogger.warning()
+                            << "Failed to allocate chunk"
+                            << chunkIndex
+                            << "for read request";
                     continue;
                 }
                 // Do not insert the allocated chunk into the MRU/LRU list,
                 // because it will be handed over to the worker immediately
                 CachingReaderChunkReadRequest request;
                 request.giveToWorker(pChunk);
-                // kLogger.debug() << "Requesting read of chunk" << current << "into" << pChunk;
-                // kLogger.debug() << "Requesting read into " << request.chunk->data;
+                if (kLogger.traceEnabled()) {
+                    kLogger.trace()
+                            << "Requesting read of chunk"
+                            << request.chunk;
+                }
                 if (m_chunkReadRequestFIFO.write(&request, 1) != 1) {
-                    kLogger.warning() << "ERROR: Could not submit read request for "
-                             << chunkIndex;
+                    kLogger.warning()
+                            << "Failed to submit read request for chunk"
+                            << chunkIndex;
                     // Revoke the chunk from the worker and free it
                     pChunk->takeFromWorker();
                     freeChunk(pChunk);
                 }
-                //kLogger.debug() << "Checking chunk " << current << " shouldWake:" << shouldWake << " chunksToRead" << m_chunksToRead.size();
             } else if (pChunk->getState() == CachingReaderChunkForOwner::READY) {
                 // This will cause the chunk to be 'freshened' in the cache. The
                 // chunk will be moved to the end of the LRU list.
diff --git a/src/engine/cachingreader/cachingreader.h b/src/engine/cachingreader/cachingreader.h
index 036f9de8d7..82a86b8d9a 100644
--- a/src/engine/cachingreader/cachingreader.h
+++ b/src/engine/cachingreader/cachingreader.h
@@ -140,6 +140,7 @@ class CachingReader : public QObject {
 
     // Returns a CachingReaderChunk to the free list
     void freeChunk(CachingReaderChunkForOwner* pChunk);
+    void freeChunkFromList(CachingReaderChunkForOwner* pChunk);
 
     // Returns all allocated chunks to the free list
     void freeAllChunks();
diff --git a/src/engine/cachingreader/cachingreaderchunk.cpp b/src/engine/cachingreader/cachingreaderchunk.cpp
index f750ddaf1f..23456282f3 100644
--- a/src/engine/cachingreader/cachingreaderchunk.cpp
+++ b/src/engine/cachingreader/cachingreaderchunk.cpp
@@ -33,14 +33,12 @@ const SINT CachingReaderChunk::kSamples =
 CachingReaderChunk::CachingReaderChunk(
         mixxx::SampleBuffer::WritableSlice sampleBuffer)
         : m_index(kInvalidChunkIndex),
-          m_sampleBuffer(sampleBuffer) {
-    DEBUG_ASSERT(sampleBuffer.length() == kSamples);
-}
-
-CachingReaderChunk::~CachingReaderChunk() {
+          m_sampleBuffer(std::move(sampleBuffer)) {
+    DEBUG_ASSERT(m_sampleBuffer.length() == kSamples);
 }
 
 void CachingReaderChunk::init(SINT index) {
+    DEBUG_ASSERT(m_index == kInvalidChunkIndex || index == kInvalidChunkIndex);
     m_index = index;
     m_bufferedSampleFrames.frameIndexRange() = mixxx::IndexRange();
 }
@@ -48,6 +46,7 @@ void CachingReaderChunk::init(SINT index) {
 // Frame index range of this chunk for the given audio source.
 mixxx::IndexRange CachingReaderChunk::frameIndexRange(
         const mixxx::AudioSourcePointer& pAudioSource) const {
+    DEBUG_ASSERT(m_index != kInvalidChunkIndex);
     if (!pAudioSource) {
         return mixxx::IndexRange();
     }
@@ -62,6 +61,7 @@ mixxx::IndexRange CachingReaderChunk::frameIndexRange(
 mixxx::IndexRange CachingReaderChunk::bufferSampleFrames(
         const mixxx::AudioSourcePointer& pAudioSource,
         mixxx::SampleBuffer::WritableSlice tempOutputBuffer) {
+    DEBUG_ASSERT(m_index != kInvalidChunkIndex);
     const auto sourceFrameIndexRange = frameIndexRange(pAudioSource);
     mixxx::AudioSourceStereoProxy audioSourceProxy(
             pAudioSource,
@@ -79,6 +79,7 @@ mixxx::IndexRange CachingReaderChunk::bufferSampleFrames(
 mixxx::IndexRange CachingReaderChunk::readBufferedSampleFrames(
         CSAMPLE* sampleBuffer,
         const mixxx::IndexRange& frameIndexRange) const {
+    DEBUG_ASSERT(m_index != kInvalidChunkIndex);
     const auto copyableFrameIndexRange =
             intersect(frameIndexRange, m_bufferedSampleFrames.frameIndexRange());
     if (!copyableFrameIndexRange.empty()) {
@@ -98,6 +99,7 @@ mixxx::IndexRange CachingReaderChunk::readBufferedSampleFrames(
 mixxx::IndexRange CachingReaderChunk::readBufferedSampleFramesReverse(
         CSAMPLE* reverseSampleBuffer,
         const mixxx::IndexRange& frameIndexRange) const {
+    DEBUG_ASSERT(m_index != kInvalidChunkIndex);
     const auto copyableFrameIndexRange =
             intersect(frameIndexRange, m_bufferedSampleFrames.frameIndexRange());
     if (!copyableFrameIndexRange.empty()) {
@@ -116,68 +118,130 @@ mixxx::IndexRange CachingReaderChunk::readBufferedSampleFramesReverse(
 
 CachingReaderChunkForOwner::CachingReaderChunkForOwner(
         mixxx::SampleBuffer::WritableSlice sampleBuffer)
-        : CachingReaderChunk(sampleBuffer),
+        : CachingReaderChunk(std::move(sampleBuffer)),
           m_state(FREE),
           m_pPrev(nullptr),
           m_pNext(nullptr) {
 }
 
-CachingReaderChunkForOwner::~CachingReaderChunkForOwner() {
-}
-
 void CachingReaderChunkForOwner::init(SINT index) {
-    DEBUG_ASSERT(READ_PENDING != m_state);
+    // Must not be accessed by a worker!
+    DEBUG_ASSERT(m_state != READ_PENDING);
+    // Must not be referenced in MRU/LRU list!
+    DEBUG_ASSERT(!m_pNext);
+    DEBUG_ASSERT(!m_pPrev);
+
     CachingReaderChunk::init(index);
     m_state = READY;
 }
 
 void CachingReaderChunkForOwner::free() {
-    DEBUG_ASSERT(READ_PENDING != m_state);
+    // Must not be accessed by a worker!
+    DEBUG_ASSERT(m_state != READ_PENDING);
+    // Must not be referenced in MRU/LRU list!
+    DEBUG_ASSERT(!m_pNext);
+    DEBUG_ASSERT(!m_pPrev);
+
     CachingReaderChunk::init(kInvalidChunkIndex);
     m_state = FREE;
 }
 
 void CachingReaderChunkForOwner::insertIntoListBefore(
+        CachingReaderChunkForOwner** ppHead,
+        CachingReaderChunkForOwner** ppTail,
         CachingReaderChunkForOwner* pBefore) {
-    DEBUG_ASSERT(m_pNext == nullptr);
-    DEBUG_ASSERT(m_pPrev == nullptr);
-    DEBUG_ASSERT(m_state != READ_PENDING); // Must not be accessed by a worker!
+    DEBUG_ASSERT(m_state == READY);
+    // Both head and tail need to be adjusted
+    DEBUG_ASSERT(ppHead);
+    DEBUG_ASSERT(ppTail);
+    // Cannot insert before itself
+    DEBUG_ASSERT(this != pBefore);
+    // Must not yet be referenced in MRU/LRU list
+    DEBUG_ASSERT(this != *ppHead);
+    DEBUG_ASSERT(this != *ppTail);
+    DEBUG_ASSERT(!m_pNext);
+    DEBUG_ASSERT(!m_pPrev);
+    if (kLogger.traceEnabled()) {
+        kLogger.trace()
+                << "insertIntoListBefore()"
+                << this
+                << ppHead << *ppHead
+                << ppTail << *ppTail
+                << pBefore;
+    }
 
-    m_pNext = pBefore;
     if (pBefore) {
-        if (pBefore->m_pPrev) {
-            m_pPrev = pBefore->m_pPrev;
-            DEBUG_ASSERT(m_pPrev->m_pNext == pBefore);
+        // List must already contain one or more item, i.e. has both
+        // a head and a tail
+        DEBUG_ASSERT(*ppHead);
+        DEBUG_ASSERT(*ppTail);
+        m_pPrev = pBefore->m_pPrev;
+        pBefore->m_pPrev = this;
+        m_pNext = pBefore;
+        if (*ppHead == pBefore) {
+            // Replace head
+            *ppHead = this;
+        }
+    } else {
+        // Append as new tail
+        m_pPrev = *ppTail;
+        *ppTail = this;
+        if (m_pPrev) {
             m_pPrev->m_pNext = this;
         }
-        pBefore->m_pPrev = this;
+        if (!*ppHead) {
+            // Initialize new head if the list was empty before
+            *ppHead = this;
+        }
     }
 }
 
 void CachingReaderChunkForOwner::removeFromList(
         CachingReaderChunkForOwner** ppHead,
         CachingReaderChunkForOwner** ppTail) {
-    // Remove this chunk from the double-linked list...
-    CachingReaderChunkForOwner* pNext = m_pNext;
-    CachingReaderChunkForOwner* pPrev = m_pPrev;
-    m_pNext = nullptr;
+    DEBUG_ASSERT(m_state == READY);
+    // Both head and tail need to be adjusted
+    DEBUG_ASSERT(ppHead);
+    DEBUG_ASSERT(ppTail);
+    if (kLogger.traceEnabled()) {
+        kLogger.trace()
+                << "removeFromList()"
+                << this
+                << ppHead << *ppHead
+                << ppTail << *ppTail;
+    }
+
+    // Disconnect this chunk from the double-linked list
+    const auto pPrev = m_pPrev;
+    const auto pNext = m_pNext;
     m_pPrev = nullptr;
+    m_pNext = nullptr;
 
-    // ...reconnect the remaining list elements...
-    if (pNext) {
-        DEBUG_ASSERT(this == pNext->m_pPrev);
-        pNext->m_pPrev = pPrev;
-    }
+    // Reconnect the adjacent list items and adjust head/tail if needed
     if (pPrev) {
         DEBUG_ASSERT(this == pPrev->m_pNext);
         pPrev->m_pNext = pNext;
+    } else {
+        // Only the current head item doesn't have a predecessor
+        if (this == *ppHead) {
+            // pNext becomes the new head
+            *ppHead = pNext;
+        } else {
+            // Item was not part the list and must not have any successor
+            DEBUG_ASSERT(!pPrev);
+        }
     }
-
-    // ...and adjust head/tail.
-    if (ppHead && (this == *ppHead)) {
-        *ppHead = pNext;
-    }
-    if (ppTail && (this == *ppTail)) {
-        *ppTail = pPrev;
+    if (pNext) {
+        DEBUG_ASSERT(this == pNext->m_pPrev);
+        pNext->m_pPrev = pPrev;
+    } else {
+        // Only the current tail item doesn't have a successor
+        if (this == *ppTail) {
+            // pPrev becomes the new tail
+            *ppTail = pPrev;
+        } else {
+            // Item was not part the list and must not have any predecessor
+            DEBUG_ASSERT(!pPrev);
+        }
     }
 }
diff --git a/src/engine/cachingreader/cachingreaderchunk.h b/src/engine/cachingreader/cachingreaderchunk.h
index 463e736666..9f44ec3d7b 100644
--- a/src/engine/cachingreader/cachingreaderchunk.h
+++ b/src/engine/cachingreader/cachingreaderchunk.h
@@ -67,7 +67,7 @@ public:
 protected:
     explicit CachingReaderChunk(
             mixxx::SampleBuffer::WritableSlice sampleBuffer);
-    virtual ~CachingReaderChunk();
+    virtual ~CachingReaderChunk() = default;
 
     void init(SINT index);
 
@@ -91,7 +91,7 @@ class CachingReaderChunkForOwner: public CachingReaderChunk {
 public:
     explicit CachingReaderChunkForOwner(
             mixxx::SampleBuffer::WritableSlice sampleBuffer);
-    ~CachingReaderChunkForOwner() override;
+    ~CachingReaderChunkForOwner() override = default;
 
     void init(SINT index);
     void free();
@@ -108,24 +108,30 @@ public:
 
     // The state is controlled by the cache as the owner of each chunk!
     void giveToWorker() {
-        DEBUG_ASSERT(READY == m_state);
+        // Must not be referenced in MRU/LRU list!
+        DEBUG_ASSERT(!m_pPrev);
+        DEBUG_ASSERT(!m_pNext);
+        DEBUG_ASSERT(m_state == READY);
         m_state = READ_PENDING;
     }
     void takeFromWorker() {
-        DEBUG_ASSERT(READ_PENDING == m_state);
+        // Must not be referenced in MRU/LRU list!
+        DEBUG_ASSERT(!m_pPrev);
+        DEBUG_ASSERT(!m_pNext);
+        DEBUG_ASSERT(m_state == READ_PENDING);
         m_state = READY;
     }
 
     // Inserts a chunk into the double-linked list before the
-    // given chunk. If the list is currently empty simply pass
-    // pBefore = nullptr. Please note that if pBefore points to
-    // the head of the current list this chunk becomes the new
-    // head of the list.
+    // given chunk and adjusts the head/tail pointers. The
+    // chunk is inserted at the tail of the list if
+    // pBefore == nullptr.
     void insertIntoListBefore(
+            CachingReaderChunkForOwner** ppHead,
+            CachingReaderChunkForOwner** ppTail,
             CachingReaderChunkForOwner* pBefore);
-    // Removes a chunk from the double-linked list and optionally
-    // adjusts head/tail pointers. Pass ppHead/ppTail = nullptr if
-    // you prefer to adjust those pointers manually.
+    // Removes a chunk from the double-linked list and adjusts
+    // the head/tail pointers.
     void removeFromList(
             CachingReaderChunkForOwner** ppHead,
             CachingReaderChunkForOwner** ppTail);
diff --git a/src/engine/cachingreader/cachingreaderworker.cpp b/src/engine/cachingreader/cachingreaderworker.cpp
index 899c11e617..777601d2a6 100644
--- a/src/engine/cachingreader/cachingreaderworker.cpp
+++ b/src/engine/cachingreader/cachingreaderworker.cpp
@@ -55,6 +55,7 @@ ReaderStatusUpdate CachingReaderWorker::processReadRequest(
     ReaderStatus status = bufferedFrameIndexRange.empty() ? CHUNK_READ_EOF : CHUNK_READ_SUCCESS;
     if (chunkFrameIndexRange != bufferedFrameIndexRange) {
         kLogger.warning()
+                << m_group
                 << "Failed to read chunk samples for frame index range:"
                 << "actual =" << bufferedFrameIndexRange
                 << ", expected =" << chunkFrameIndexRange;
@@ -120,27 +121,15 @@ void CachingReaderWorker::run() {
     }
 }
 
-namespace {
-
-mixxx::AudioSourcePointer openAudioSourceForReading(const TrackPointer& pTrack, const mixxx::AudioSource::OpenParams& params) {
-    auto pAudioSource = SoundSourceProxy(pTrack).openAudioSource(params);
-    if (!pAudioSource) {
-        kLogger.warning() << "Failed to open file:" << pTrack->getLocation();
-    }
-    return pAudioSource;
-}
-
-} // anonymous namespace
-
 void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) {
-    ReaderStatusUpdate status;
-    status.init(TRACK_NOT_LOADED);
+    ReaderStatusUpdate update;
+    update.init(TRACK_NOT_LOADED);
 
     if (!pTrack) {
         // Unload track
         m_pAudioSource.reset(); // Close open file handles
         m_readableFrameIndexRange = mixxx::IndexRange();
-        m_pReaderStatusFIFO->writeBlocking(&status, 1);
+        m_pReaderStatusFIFO->writeBlocking(&update, 1);
         return;
     }
 
@@ -149,10 +138,11 @@ void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) {
 
     QString filename = pTrack->getLocation();
     if (filename.isEmpty() || !pTrack->checkFileExists()) {
-        // Must unlock before emitting to avoid deadlock
-        kLogger.debug() << m_group << "loadTrack() load failed for\""
-                 << filename << "\", unlocked reader lock";
-        m_pReaderStatusFIFO->writeBlocking(&status, 1);
+        kLogger.warning()
+                 << m_group
+                 << "File not found"
+                 << filename;
+        m_pReaderStatusFIFO->writeBlocking(&update, 1);
         emit(trackLoadFailed(
             pTrack, QString("The file '%1' could not be found.")
                     .arg(QDir::toNativeSeparators(filename))));
@@ -161,13 +151,14 @@ void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) {
 
     mixxx::AudioSource::OpenParams config;
     config.setChannelCount(CachingReaderChunk::kChannels);
-    m_pAudioSource = openAudioSourceForReading(pTrack, config);
+    m_pAudioSource = SoundSourceProxy(pTrack).openAudioSource(config);
     if (!m_pAudioSource) {
         m_readableFrameIndexRange = mixxx::IndexRange();
-        // Must unlock before emitting to avoid deadlock
-        kLogger.debug() << m_group << "loadTrack() load failed for\""
-                 << filename << "\", file invalid, unlocked reader lock";
-        m_pReaderStatusFIFO->writeBlocking(&status, 1);
+        kLogger.warning()
+                << m_group
+                << "Failed to open file"
+                << filename;
+        m_pReaderStatusFIFO->writeBlocking(&update, 1);
         emit(trackLoadFailed(
             pTrack, QString("The file '%1' could not be loaded.").arg(filename)));
         return;
@@ -183,18 +174,14 @@ void CachingReaderWorker::loadTrack(const TrackPointer& pTrack) {
     // be decreased to avoid repeated reading of corrupt audio data.
     m_readableFrameIndexRange = m_pAudioSource->frameIndexRange();
 
-    status.status = TRACK_LOADED;
-    status.readableFrameIndexRangeStart = m_readableFrameIndexRange.start();
-    status.readableFrameIndexRangeEnd = m_readableFrameIndexRange.end();
-    m_pReaderStatusFIFO->writeBlocking(&status, 1);
+    update.init(TRACK_LOADED, nullptr, m_pAudioSource->frameIndexRange());
+    m_pReaderStatusFIFO->writeBlocking(&update, 1);
 
     // Clear the chunks to read list.
     CachingReaderChunkReadRequest request;
     while (m_pChunkReadRequestFIFO->read(&request, 1) == 1) {
-        kLogger.debug() << "Cancelling read request for " << request.chunk->getIndex();
-        status.status = CHUNK_READ_INVALID;
-        status.chunk = request.chunk;
-        m_pReaderStatusFIFO->writeBlocking(&status, 1);
+        update.init(CHUNK_READ_INVALID, request.chunk);
+        m_pReaderStatusFIFO->writeBlocking(&update, 1);
     }
 
     // Emit that the track is loaded.
diff --git a/src/engine/cachingreader/cachingreaderworker.h b/src/engine/cachingreader/cachingreaderworker.h
index fc6a438dd1..a7a9b2c584 100644
--- a/src/engine/cachingreader/cachingreaderworker.h
+++ b/src/engine/cachingreader/cachingreaderworker.h
@@ -36,11 +36,14 @@ enum ReaderStatus {
 
 // POD with trivial ctor/dtor/copy for passing through FIFO
 typedef struct ReaderStatusUpdate {
-    ReaderStatus status;
+  private:
     CachingReaderChunk* chunk;
     SINT readableFrameIndexRangeStart;
     SINT readableFrameIndexRangeEnd;
 
+  public:
+    ReaderStatus status;
+
     void init(
             ReaderStatus statusArg = INVALID,
             CachingReaderChunk* chunkArg = nullptr,
@@ -51,6 +54,17 @@ typedef struct ReaderStatusUpdate {
         readableFrameIndexRangeEnd = readableFrameIndexRangeArg.end();
     }
 
+    CachingReaderChunkForOwner* takeFromWorker() {
+        CachingReaderChunkForOwner* pChunk = nullptr;
+        if (chunk) {
+            DEBUG_ASSERT(dynamic_cast<CachingReaderChunkForOwner*>(chunk))