diff --git a/src/engine/cachingreader.cpp b/src/engine/cachingreader.cpp index d52404df839..54d98cd9a73 100644 --- a/src/engine/cachingreader.cpp +++ b/src/engine/cachingreader.cpp @@ -20,9 +20,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 @@ -31,8 +43,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), @@ -73,8 +97,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()); @@ -82,10 +114,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() { @@ -97,15 +126,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) { @@ -115,62 +142,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; @@ -182,15 +206,11 @@ void CachingReader::newTrack(TrackPointer pTrack) { } void CachingReader::process() { - ReaderStatusUpdate status; - while (m_readerStatusFIFO.read(&status, 1) == 1) { - CachingReaderChunkForOwner* pChunk = static_cast(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); @@ -199,12 +219,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(); } @@ -212,7 +232,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(); @@ -228,9 +248,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) { @@ -281,10 +302,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); @@ -473,27 +496,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.h b/src/engine/cachingreader.h index 029b44246da..e148cd637e1 100644 --- a/src/engine/cachingreader.h +++ b/src/engine/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/cachingreaderchunk.cpp b/src/engine/cachingreaderchunk.cpp index 9eb2dbbf085..701fb00bf50 100644 --- a/src/engine/cachingreaderchunk.cpp +++ b/src/engine/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/cachingreaderchunk.h b/src/engine/cachingreaderchunk.h index 463e7366665..9f44ec3d7b9 100644 --- a/src/engine/cachingreaderchunk.h +++ b/src/engine/cachingreaderchunk.h @@ -67,7 +67,7 @@ class CachingReaderChunk { 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 @@ class CachingReaderChunkForOwner: public CachingReaderChunk { // 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/cachingreaderworker.cpp b/src/engine/cachingreaderworker.cpp index 34f9a9ac836..955267563cf 100644 --- a/src/engine/cachingreaderworker.cpp +++ b/src/engine/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->exists()) { - // 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/cachingreaderworker.h b/src/engine/cachingreaderworker.h index 54c5a4c77e4..eab3c4a8c98 100644 --- a/src/engine/cachingreaderworker.h +++ b/src/engine/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(chunk)); + pChunk = static_cast(chunk); + chunk = nullptr; + pChunk->takeFromWorker(); + } + return pChunk; + } + mixxx::IndexRange readableFrameIndexRange() const { return mixxx::IndexRange::between( readableFrameIndexRangeStart, diff --git a/src/util/fifo.h b/src/util/fifo.h index 3ffd03e7663..1355430ddd7 100644 --- a/src/util/fifo.h +++ b/src/util/fifo.h @@ -43,10 +43,8 @@ class FIFO { } void writeBlocking(const DataType* pData, int count) { int written = 0; - while (written != count) { - int i = write(pData, count); - pData += i; - written += i; + while (written < count) { + written += write(pData + written, count - written); } } int aquireWriteRegions(int count,