Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix debug assert in CachingReader #2309

Merged
merged 11 commits into from
Oct 3, 2019
15 changes: 6 additions & 9 deletions src/engine/cachingreader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -205,32 +205,27 @@ void CachingReader::newTrack(TrackPointer pTrack) {
// to get ready while the reader switches its internal
// state. There are no race conditions, because the
// reader polls the worker.
m_state.store(State::TrackLoading, std::memory_order_acquire);
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();
}

void CachingReader::process() {
ReaderStatusUpdate update;
while (m_stateFIFO.read(&update, 1) == 1) {
DEBUG_ASSERT(m_state != State::Idle);
auto pChunk = update.takeFromWorker();
if (pChunk) {
DEBUG_ASSERT(m_state != State::Idle);
Copy link
Contributor

@uklotzde uklotzde Oct 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to move the debug assertion. The condition must be valid while looping, independent of the update message. Ignore my comment. But I still don't understand when and why this might happen.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the original position it happen when you have ejected the track.
In this case the "idle" state has overwritten the "loading" state because a null track was loaded. After loading a new non null track we receive the "loaded" signal while in idle state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should receive TRACK_LOADED/UNLOADED messages only while the state is Loading. The Idle state is set after we have received TRACK_UNLOADED. We should not receive any messages from the worker until the state has been changed from Idle to Loading again.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that the cue is not processed after ejecting the track. That is why the idle message is still in the queue and overwrite the loading message.
This is not much of an issue, but I see how it is surprising.
I will look how to solve this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not an issue to overwrite the loading message because the very next message is the loaded message and the cue was purged until the loading state was there. Purging is stopped by the idle message.

Can we solve this by renaming?

// Result of a read request (with a chunk)
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);
// Discard all results from pending read requests for the
// previous track before the next track has been loaded.
freeChunk(pChunk);
Expand All @@ -253,13 +248,15 @@ void CachingReader::process() {
}
} else {
// State update (without a chunk)
// We have a new Track, discharge the chunks from the old track.
freeAllChunks();
DEBUG_ASSERT(!m_mruCachingReaderChunk);
DEBUG_ASSERT(!m_lruCachingReaderChunk);
if (update.status == TRACK_LOADED) {
m_state = State::TrackLoaded;
m_state.store(State::TrackLoaded, std::memory_order_release);
} else {
DEBUG_ASSERT(update.status == TRACK_UNLOADED);
m_state = State::Idle;
m_state.store(State::Idle, std::memory_order_release);
}
// Reset the readable frame index range
m_readableFrameIndexRange = update.readableFrameIndexRange();
Expand Down
4 changes: 3 additions & 1 deletion src/engine/cachingreader.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
#ifndef ENGINE_CACHINGREADER_H
#define ENGINE_CACHINGREADER_H

#include <atomic>

#include <QtDebug>
#include <QList>
#include <QVector>
Expand Down Expand Up @@ -156,7 +158,7 @@ class CachingReader : public QObject {
TrackLoading,
TrackLoaded,
};
State m_state;
std::atomic<State> m_state;

// Keeps track of all CachingReaderChunks we've allocated.
QVector<CachingReaderChunkForOwner*> m_chunks;
Expand Down