Added status full loaded into FileData #1246
Conversation
There was a problem hiding this comment.
2 questions:
- If we move to having a single background thread for loading, this logic and the atomics would get much easier, wouldn't it?
- It seems we're so close to not needing the
loadedFiles. The only issue I see is if someone callssetRamLoading(true), and thensetRamLoading(false), which would erase theFullyLoadedstates from before.
src/sfizz/FilePool.h
Outdated
| @@ -67,7 +67,7 @@ struct FileInformation { | |||
| // Strict C++11 disallows member initialization if aggregate initialization is to be used... | |||
| struct FileData | |||
| { | |||
| enum class Status { Invalid, Preloaded, Streaming, Done }; | |||
| enum class Status { Invalid, Preloaded, Streaming, Done, FullLoaded }; | |||
There was a problem hiding this comment.
It might be better that we have a boolean fullyLoaded variable on FileData instead of adding it's status enum.
src/sfizz/FilePool.cpp
Outdated
| @@ -509,7 +532,10 @@ void sfz::FilePool::loadingJob(const QueuedFileData& data) noexcept | |||
|
|
|||
| streamFromFile(*reader, data.data->fileData, &data.data->availableFrames); | |||
|
|
|||
| data.data->status = FileData::Status::Done; | |||
| currentStatus = data.data->status.load(); | |||
| if (currentStatus == FileData::Status::Streaming) { | |||
There was a problem hiding this comment.
Given the compare_exchange below, shouldn't this be while (currentStatus != FileData::Status::Done)?
There was a problem hiding this comment.
status should not be changed to Done when the status is changed to FullLoaded.
but it is a little bit dangerous in the following situation:
- fileData starts streaming
- status is changed to FullLoaded (e.g. setRamLoading(true))
- status is changed to Preloaded (e.g. setRamLoading(false))
- fileData finishes streaming
I will consider more.
src/sfizz/FilePool.cpp
Outdated
| preloadedFile.second.preloadedData = readFromFile(*reader, preloadSize + maxOffset); | ||
| auto& fileId = preloadedFile.first; | ||
| auto& fileData = preloadedFile.second; | ||
| const uint32_t maxOffset = fileData.information.maxOffset; |
There was a problem hiding this comment.
Maybe we need to make the maxOffset in information a uint32_t. I don't recall why I put an int64_t, maybe for full generality and to better support integer arithmetics since you tend to add or remove offsets around. Was there a reason for you to force uint32_t here?
There was a problem hiding this comment.
No. I will fix this.
src/sfizz/FilePool.cpp
Outdated
| } | ||
| else if (!fullLoaded && status == FileData::Status::FullLoaded) { | ||
| // Exchange status | ||
| fileData.status.compare_exchange_strong(status, FileData::Status::Preloaded); |
There was a problem hiding this comment.
Same comment as below re: compare_exchange without a while loop.
There was a problem hiding this comment.
In this process, the status should not be overwritten if the status is changed at the moment.
src/sfizz/FilePool.cpp
Outdated
| ); | ||
| // Initially set status |
There was a problem hiding this comment.
We can remove this comment I think.
There was a problem hiding this comment.
I added comments on every status change.
If we moved to having a single background thread for loading and having shared file pool, an instance have to wait until another instance finishes loading.
|
Oh do you mean |
|
I think the processes become more safe. |
|
Looks good, I'll try to stress test it a bit today or tomorrow thanks! |
|
@KKQ-KKQ I'm sorry I've been way too swamped at work. I'm merging this without extensive testing so you can proceed. I'll test out things as they come when I can... すみません ! 🙇 |
No problem! Thank you! |
PR 1/4 of #1236 Shared File Pool
Added FileData::Status::FullLoaded.