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.

Sign up for GitHub

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 race conditions (2) #514

Merged
merged 3 commits into from
Oct 21, 2020
Merged

Fix race conditions (2) #514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e6ecbc6
Make the Region::sampleId shared_ptr
jpcima Oct 20, 2020
0e24daf
Auto-cleaning file pool
jpcima Oct 20, 2020
10565e9
Make it a tiny bit more efficient
jpcima Oct 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 21 additions & 31 deletions src/sfizz/FilePool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,9 +342,9 @@ sfz::FileDataHolder sfz::FilePool::loadFile(const FileId& fileId) noexcept
}
}

sfz::FileDataHolder sfz::FilePool::getFilePromise(const FileId& fileId) noexcept
sfz::FileDataHolder sfz::FilePool::getFilePromise(const std::shared_ptr<FileId>& fileId) noexcept
{
const auto preloaded = preloadedFiles.find(fileId);
const auto preloaded = preloadedFiles.find(*fileId);
if (preloaded == preloadedFiles.end()) {
DBG("[sfizz] File not found in the preloaded files: " << fileId);
return {};
Expand Down Expand Up @@ -381,14 +381,20 @@ void sfz::FilePool::loadingJob(QueuedFileData data) noexcept
{
raiseCurrentThreadPriority();

std::shared_ptr<FileId> id = data.id.lock();
if (!id) {
// file ID was nulled, it means the region was deleted, ignore
return;
}

const auto loadStartTime = std::chrono::high_resolution_clock::now();
const auto waitDuration = loadStartTime - data.queuedTime;
const fs::path file { rootDirectory / data.id.filename() };
const fs::path file { rootDirectory / id->filename() };
std::error_code readError;
AudioReaderPtr reader = createAudioReader(file, data.id.isReverse(), &readError);
AudioReaderPtr reader = createAudioReader(file, id->isReverse(), &readError);

if (readError) {
DBG("[sfizz] libsndfile errored for " << data.id << " with message " << readError.message());
DBG("[sfizz] libsndfile errored for " << *id << " with message " << readError.message());
return;
}

Expand All @@ -398,7 +404,7 @@ void sfz::FilePool::loadingJob(QueuedFileData data) noexcept
while (currentStatus == FileData::Status::Invalid) {
// Spin until the state changes
if (spinCounter > 1024) {
DBG("[sfizz] " << data.id << " is stuck on Invalid? Leaving the load");
DBG("[sfizz] " << *id << " is stuck on Invalid? Leaving the load");
return;
}

Expand All @@ -418,13 +424,13 @@ void sfz::FilePool::loadingJob(QueuedFileData data) noexcept
const auto frames = static_cast<uint32_t>(reader->frames());
streamFromFile(*reader, frames, oversamplingFactor, data.data->fileData, &data.data->availableFrames);
const auto loadDuration = std::chrono::high_resolution_clock::now() - loadStartTime;
logger.logFileTime(waitDuration, loadDuration, frames, data.id.filename());
logger.logFileTime(waitDuration, loadDuration, frames, id->filename());

data.data->status = FileData::Status::Done;

std::lock_guard<SpinMutex> guard { lastUsedMutex };
if (absl::c_find(lastUsedFiles, data.id) == lastUsedFiles.end())
lastUsedFiles.push_back(data.id);
if (absl::c_find(lastUsedFiles, *id) == lastUsedFiles.end())
lastUsedFiles.push_back(*id);
}

void sfz::FilePool::clear()
Expand Down Expand Up @@ -487,20 +493,15 @@ void sfz::FilePool::dispatchingJob() noexcept
{
QueuedFileData queuedData;
while (dispatchBarrier.wait(), dispatchFlag) {
if (emptyQueueFlag) {
while (filesToLoad.try_pop(queuedData)) {
// pass
}
semEmptyQueueFinished.post();
emptyQueueFlag = false;
continue;
}

std::lock_guard<std::mutex> guard { loadingJobsMutex };

if (filesToLoad.try_pop(queuedData)) {
loadingJobs.push_back(
threadPool->enqueue([this](const QueuedFileData& data) { loadingJob(data); }, queuedData));
if (queuedData.id.expired()) {
// file ID was nulled, it means the region was deleted, ignore
}
else
loadingJobs.push_back(
threadPool->enqueue([this](const QueuedFileData& data) { loadingJob(data); }, queuedData));
}

// Clear finished jobs
Expand All @@ -521,17 +522,6 @@ void sfz::FilePool::garbageJob() noexcept
}
}

void sfz::FilePool::emptyFileLoadingQueues() noexcept
{
ASSERT(dispatchFlag);
emptyQueueFlag = true;
std::error_code ec;
dispatchBarrier.post(ec);

if (!ec)
semEmptyQueueFinished.wait();
}

void sfz::FilePool::waitForBackgroundLoading() noexcept
{
std::lock_guard<std::mutex> guard { loadingJobsMutex };
Expand Down
14 changes: 8 additions & 6 deletions src/sfizz/FilePool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ class FilePool {
* @param fileId the file to preload
* @return FileDataHolder a file data handle
*/
FileDataHolder getFilePromise(const FileId& fileId) noexcept;
FileDataHolder getFilePromise(const std::shared_ptr<FileId>& fileId) noexcept;
/**
* @brief Change the preloading size. This will trigger a full
* reload of all samples, so don't call it on the audio thread.
Expand Down Expand Up @@ -296,7 +296,11 @@ class FilePool {
* method on the audio thread as it will spinlock.
*
*/
void emptyFileLoadingQueues() noexcept;
void emptyFileLoadingQueues() noexcept
{
// nothing to do in this implementation,
// deleting the region and its sample ID take care of it
}
/**
* @brief Wait for the background loading to finish for all promises
* in the queue.
Expand Down Expand Up @@ -331,23 +335,21 @@ class FilePool {
// Signals
volatile bool dispatchFlag { true };
volatile bool garbageFlag { true };
volatile bool emptyQueueFlag { false };
RTSemaphore dispatchBarrier;
RTSemaphore semEmptyQueueFinished;
RTSemaphore semGarbageBarrier;

// Structures for the background loaders
struct QueuedFileData
{
using TimePoint = std::chrono::time_point<std::chrono::high_resolution_clock>;
QueuedFileData() = default;
QueuedFileData(FileId id, FileData* data, TimePoint queuedTime)
QueuedFileData(std::weak_ptr<FileId> id, FileData* data, TimePoint queuedTime)
: id(id), data(data), queuedTime(queuedTime) {}
QueuedFileData(const QueuedFileData&) = default;
QueuedFileData& operator=(const QueuedFileData&) = default;
QueuedFileData(QueuedFileData&&) = default;
QueuedFileData& operator=(QueuedFileData&&) = default;
FileId id {};
std::weak_ptr<FileId> id;
FileData* data { nullptr };
TimePoint queuedTime {};
};
Expand Down
4 changes: 2 additions & 2 deletions src/sfizz/Region.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ bool sfz::Region::parseOpcode(const Opcode& rawOpcode)
else
filename = absl::StrCat(defaultPath, absl::StrReplaceAll(trimmedSample, { { "\\", "/" } }));

sampleId = FileId(std::move(filename), sampleId.isReverse());
*sampleId = FileId(std::move(filename), sampleId->isReverse());
}
break;
case hash("sample_quality"):
Expand All @@ -88,7 +88,7 @@ bool sfz::Region::parseOpcode(const Opcode& rawOpcode)
}
break;
case hash("direction"):
sampleId = sampleId.reversed(opcode.value == "reverse");
*sampleId = sampleId->reversed(opcode.value == "reverse");
break;
case hash("delay"):
setValueFromOpcode(opcode, delay, Default::delayRange);
Expand Down
4 changes: 2 additions & 2 deletions src/sfizz/Region.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ struct Region {
* @return true
* @return false
*/
bool isGenerator() const noexcept { return sampleId.filename().size() > 0 ? sampleId.filename()[0] == '*' : false; }
bool isGenerator() const noexcept { return sampleId->filename().size() > 0 ? sampleId->filename()[0] == '*' : false; }
/**
* @brief Is an oscillator (generator or wavetable)?
*
Expand Down Expand Up @@ -308,7 +308,7 @@ struct Region {
const NumericId<Region> id;

// Sound source: sample playback
FileId sampleId {}; // Sample
std::shared_ptr<FileId> sampleId { new FileId }; // Sample
absl::optional<int> sampleQuality {};
float delay { Default::delay }; // delay
float delayRandom { Default::delayRandom }; // delay_random
Expand Down
10 changes: 5 additions & 5 deletions src/sfizz/Synth.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ void sfz::Synth::finalizeSfzLoad()
size_t currentRegionCount = regions.size();

auto removeCurrentRegion = [this, &currentRegionIndex, &currentRegionCount]() {
DBG("Removing the region with sample " << regions[currentRegionIndex]->sampleId);
DBG("Removing the region with sample " << *regions[currentRegionIndex]->sampleId);
regions.erase(regions.begin() + currentRegionIndex);
--currentRegionCount;
};
Expand All @@ -534,12 +534,12 @@ void sfz::Synth::finalizeSfzLoad()
absl::optional<FileInformation> fileInformation;

if (!region->isGenerator()) {
if (!resources.filePool.checkSampleId(region->sampleId)) {
if (!resources.filePool.checkSampleId(*region->sampleId)) {
removeCurrentRegion();
continue;
}

fileInformation = resources.filePool.getFileInformation(region->sampleId);
fileInformation = resources.filePool.getFileInformation(*region->sampleId);
if (!fileInformation) {
removeCurrentRegion();
continue;
Expand Down Expand Up @@ -583,11 +583,11 @@ void sfz::Synth::finalizeSfzLoad()
return Default::offsetCCRange.clamp(sumOffsetCC);
}();

if (!resources.filePool.preloadFile(region->sampleId, maxOffset))
if (!resources.filePool.preloadFile(*region->sampleId, maxOffset))
removeCurrentRegion();
}
else if (!region->isGenerator()) {
if (!resources.wavePool.createFileWave(resources.filePool, std::string(region->sampleId.filename()))) {
if (!resources.wavePool.createFileWave(resources.filePool, std::string(region->sampleId->filename()))) {
removeCurrentRegion();
continue;
}
Expand Down
10 changes: 5 additions & 5 deletions src/sfizz/Voice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ void sfz::Voice::startVoice(Region* region, int delay, const TriggerEvent& event
if (region->isOscillator()) {
const WavetableMulti* wave = nullptr;
if (!region->isGenerator())
wave = resources.wavePool.getFileWave(region->sampleId.filename());
wave = resources.wavePool.getFileWave(region->sampleId->filename());
else {
switch (hash(region->sampleId.filename())) {
switch (hash(region->sampleId->filename())) {
default:
case hash("*silence"):
break;
Expand Down Expand Up @@ -658,7 +658,7 @@ void sfz::Voice::fillWithData(AudioSpan<float> buffer) noexcept
DBG("[sfizz] Underflow: source available samples "
<< source.getNumFrames() << "/"
<< currentPromise->information.end
<< " for sample " << region->sampleId);
<< " for sample " << *region->sampleId);
}
#endif
if (!region->flexAmpEG) {
Expand Down Expand Up @@ -891,13 +891,13 @@ void sfz::Voice::fillWithGenerator(AudioSpan<float> buffer) noexcept
const auto leftSpan = buffer.getSpan(0);
const auto rightSpan = buffer.getSpan(1);

if (region->sampleId.filename() == "*noise") {
if (region->sampleId->filename() == "*noise") {
auto gen = [&]() {
return uniformNoiseDist(Random::randomGenerator);
};
absl::c_generate(leftSpan, gen);
absl::c_generate(rightSpan, gen);
} else if (region->sampleId.filename() == "*gnoise") {
} else if (region->sampleId->filename() == "*gnoise") {
// You need to wrap in a lambda, otherwise generate will
// make a copy of the gaussian distribution *along with its state*
// leading to periodic behavior....
Expand Down
Loading