From 241b6c8d18b9550ba4d6f7e22959a2e1923eae7d Mon Sep 17 00:00:00 2001 From: James Ball Date: Wed, 5 Jul 2023 15:17:17 +0100 Subject: [PATCH] Introduce proper thread safety around audio effects, and make sure key press detection is global --- Source/EffectsComponent.cpp | 14 +++-- Source/MainComponent.cpp | 8 ++- Source/ObjComponent.cpp | 9 +-- Source/ObjComponent.h | 2 +- Source/PluginEditor.cpp | 55 +++++++++------- Source/PluginEditor.h | 2 +- Source/PluginProcessor.cpp | 73 +++++++++++----------- Source/PluginProcessor.h | 2 +- Source/components/EffectsListComponent.cpp | 18 ++++-- Source/components/EffectsListComponent.h | 8 ++- Source/components/LuaListComponent.cpp | 2 + 11 files changed, 109 insertions(+), 84 deletions(-) diff --git a/Source/EffectsComponent.cpp b/Source/EffectsComponent.cpp index d362942..0878153 100644 --- a/Source/EffectsComponent.cpp +++ b/Source/EffectsComponent.cpp @@ -18,12 +18,14 @@ EffectsComponent::EffectsComponent(OscirenderAudioProcessor& p) : audioProcessor } }; - auto effects = audioProcessor.allEffects; - for (int i = 0; i < effects.size(); i++) { - auto effect = effects[i]; - effect->setValue(effect->getValue()); - itemData.data.push_back(effect); - } + { + juce::SpinLock::ScopedLockType lock(audioProcessor.effectsLock); + for (int i = 0; i < audioProcessor.allEffects.size(); i++) { + auto effect = audioProcessor.allEffects[i]; + effect->setValue(effect->getValue()); + itemData.data.push_back(effect); + } + } /*addBtn.setButtonText("Add Item..."); addBtn.onClick = [this]() diff --git a/Source/MainComponent.cpp b/Source/MainComponent.cpp index b00fe03..53fecb4 100644 --- a/Source/MainComponent.cpp +++ b/Source/MainComponent.cpp @@ -23,7 +23,7 @@ MainComponent::MainComponent(OscirenderAudioProcessor& p, OscirenderAudioProcess } pluginEditor.addCodeEditor(audioProcessor.getCurrentFileIndex()); pluginEditor.updateCodeEditor(); - pluginEditor.fileUpdated(audioProcessor.getCurrentFile()); + pluginEditor.fileUpdated(std::make_unique(audioProcessor.getCurrentFile())); updateFileLabel(); }); }; @@ -40,7 +40,11 @@ MainComponent::MainComponent(OscirenderAudioProcessor& p, OscirenderAudioProcess pluginEditor.removeCodeEditor(audioProcessor.getCurrentFileIndex()); audioProcessor.removeFile(audioProcessor.getCurrentFileIndex()); pluginEditor.updateCodeEditor(); - pluginEditor.fileUpdated(audioProcessor.getCurrentFile()); + std::unique_ptr file; + if (audioProcessor.getCurrentFileIndex() != -1) { + file = std::make_unique(audioProcessor.getCurrentFile()); + } + pluginEditor.fileUpdated(std::move(file)); updateFileLabel(); }; diff --git a/Source/ObjComponent.cpp b/Source/ObjComponent.cpp index 511c822..dd3faaf 100644 --- a/Source/ObjComponent.cpp +++ b/Source/ObjComponent.cpp @@ -67,12 +67,8 @@ void ObjComponent::mouseMove(const juce::MouseEvent& e) { } } -// listen for when escape is pressed to disable mouse rotation -bool ObjComponent::keyPressed(const juce::KeyPress& key) { - if (key == juce::KeyPress::escapeKey) { - mouseRotate.setToggleState(false, juce::NotificationType::dontSendNotification); - } - return true; +void ObjComponent::disableMouseRotation() { + mouseRotate.setToggleState(false, juce::NotificationType::dontSendNotification); } void ObjComponent::resized() { @@ -89,5 +85,4 @@ void ObjComponent::resized() { resetRotation.setBounds(row.removeFromLeft(120)); row.removeFromLeft(20); mouseRotate.setBounds(row); - } diff --git a/Source/ObjComponent.h b/Source/ObjComponent.h index 76b6689..a15513a 100644 --- a/Source/ObjComponent.h +++ b/Source/ObjComponent.h @@ -12,7 +12,7 @@ public: void resized() override; void mouseMove(const juce::MouseEvent& event) override; - bool keyPressed(const juce::KeyPress& key) override; + void disableMouseRotation(); private: OscirenderAudioProcessor& audioProcessor; OscirenderAudioProcessorEditor& pluginEditor; diff --git a/Source/PluginEditor.cpp b/Source/PluginEditor.cpp index 94f0b49..a81b88d 100644 --- a/Source/PluginEditor.cpp +++ b/Source/PluginEditor.cpp @@ -106,7 +106,7 @@ void OscirenderAudioProcessorEditor::removeCodeEditor(int index) { } -// parsersLock must be locked before calling this function +// parsersLock AND effectsLock must be locked before calling this function void OscirenderAudioProcessorEditor::updateCodeEditor() { // check if any code editors are visible bool visible = false; @@ -127,22 +127,24 @@ void OscirenderAudioProcessorEditor::updateCodeEditor() { resized(); } -void OscirenderAudioProcessorEditor::fileUpdated(juce::File file) { +void OscirenderAudioProcessorEditor::fileUpdated(std::unique_ptr file) { lua.setVisible(false); obj.setVisible(false); - if (file.getFileExtension() == ".lua") { + if (file == nullptr) { + return; + } else if (file->getFileExtension() == ".lua") { lua.setVisible(true); - } else if (file.getFileExtension() == ".obj") { + } else if (file->getFileExtension() == ".obj") { obj.setVisible(true); } } -// parsersLock must be locked before calling this function +// parsersLock AND effectsLock must be locked before calling this function void OscirenderAudioProcessorEditor::codeDocumentTextInserted(const juce::String& newText, int insertIndex) { updateCodeDocument(); } -// parsersLock must be locked before calling this function +// parsersLock AND effectsLock must be locked before calling this function void OscirenderAudioProcessorEditor::codeDocumentTextDeleted(int startIndex, int endIndex) { updateCodeDocument(); } @@ -154,30 +156,41 @@ void OscirenderAudioProcessorEditor::updateCodeDocument() { } bool OscirenderAudioProcessorEditor::keyPressed(const juce::KeyPress& key) { - juce::SpinLock::ScopedLockType lock(audioProcessor.parsersLock); + juce::SpinLock::ScopedLockType parserLock(audioProcessor.parsersLock); + juce::SpinLock::ScopedLockType effectsLock(audioProcessor.effectsLock); + int numFiles = audioProcessor.numFiles(); int currentFile = audioProcessor.getCurrentFileIndex(); - bool updated = false; + bool changedFile = false; + bool consumeKey = true; if (key.getTextCharacter() == 'j') { - currentFile++; - if (currentFile == numFiles) { - currentFile = 0; + if (numFiles > 1) { + currentFile++; + if (currentFile == numFiles) { + currentFile = 0; + } + changedFile = true; } - updated = true; } else if (key.getTextCharacter() == 'k') { - currentFile--; - if (currentFile < 0) { - currentFile = numFiles - 1; - } - updated = true; - } + if (numFiles > 1) { + currentFile--; + if (currentFile < 0) { + currentFile = numFiles - 1; + } + changedFile = true; + } + } else if (key.isKeyCode(juce::KeyPress::escapeKey)) { + obj.disableMouseRotation(); + } else { + consumeKey = false; + } - if (updated) { + if (changedFile) { audioProcessor.changeCurrentFile(currentFile); - fileUpdated(audioProcessor.getCurrentFile()); + fileUpdated(std::make_unique(audioProcessor.getCurrentFile())); updateCodeEditor(); main.updateFileLabel(); } - return updated; + return consumeKey; } diff --git a/Source/PluginEditor.h b/Source/PluginEditor.h index 32eafcc..e243e0f 100644 --- a/Source/PluginEditor.h +++ b/Source/PluginEditor.h @@ -31,7 +31,7 @@ public: void addCodeEditor(int index); void removeCodeEditor(int index); void updateCodeEditor(); - void fileUpdated(juce::File file); + void fileUpdated(std::unique_ptr file); private: OscirenderAudioProcessor& audioProcessor; diff --git a/Source/PluginProcessor.cpp b/Source/PluginProcessor.cpp index 495974e..7ad1f2e 100644 --- a/Source/PluginProcessor.cpp +++ b/Source/PluginProcessor.cpp @@ -33,9 +33,11 @@ OscirenderAudioProcessor::OscirenderAudioProcessor() { producer = std::make_unique(*this, std::make_shared()); producer->startThread(); + + juce::SpinLock::ScopedLockType lock(effectsLock); - allEffects.push_back(std::make_shared(std::make_unique(), "Bit Crush", "bitCrush")); - allEffects.push_back(std::make_shared(std::make_unique(), "Bulge", "bulge")); + allEffects.push_back(std::make_shared(std::make_unique(), "Bit Crush", "bitCrush")); + allEffects.push_back(std::make_shared(std::make_unique(), "Bulge", "bulge")); allEffects.push_back(std::make_shared(std::make_unique(), "2D Rotate Speed", "rotateSpeed")); allEffects.push_back(std::make_shared(std::make_unique(), "Vector cancelling", "vectorCancelling")); allEffects.push_back(std::make_shared(std::make_unique(true), "Vertical shift", "verticalDistort")); @@ -152,6 +154,7 @@ bool OscirenderAudioProcessor::isBusesLayoutSupported (const BusesLayout& layout } #endif +// effectsLock should be held when calling this void OscirenderAudioProcessor::addLuaSlider() { juce::String sliderName = ""; @@ -165,6 +168,7 @@ void OscirenderAudioProcessor::addLuaSlider() { luaEffects.push_back(std::make_shared(std::make_unique(sliderName, *this), "Lua " + sliderName, "lua" + sliderName)); } +// effectsLock should be held when calling this void OscirenderAudioProcessor::updateLuaValues() { for (auto& effect : luaEffects) { effect->apply(); @@ -183,58 +187,44 @@ void OscirenderAudioProcessor::updateAngleDelta() { thetaDelta = cyclesPerSample * 2.0 * juce::MathConstants::pi; } +// effectsLock MUST be held when calling this void OscirenderAudioProcessor::enableEffect(std::shared_ptr effect) { - // need to make a new vector because the old one is being iterated over in another thread - std::shared_ptr>> newEffects = std::make_shared>>(); - for (auto& e : *enabledEffects) { - newEffects->push_back(e); - } // remove any existing effects with the same id - for (auto it = newEffects->begin(); it != newEffects->end();) { + for (auto it = enabledEffects->begin(); it != enabledEffects->end();) { if ((*it)->getId() == effect->getId()) { - it = newEffects->erase(it); + it = enabledEffects->erase(it); } else { it++; } } // insert according to precedence (sorts from lowest to highest precedence) - auto it = newEffects->begin(); - while (it != newEffects->end() && (*it)->getPrecedence() <= effect->getPrecedence()) { + auto it = enabledEffects->begin(); + while (it != enabledEffects->end() && (*it)->getPrecedence() <= effect->getPrecedence()) { it++; } - newEffects->insert(it, effect); - enabledEffects = newEffects; + enabledEffects->insert(it, effect); } +// effectsLock MUST be held when calling this void OscirenderAudioProcessor::disableEffect(std::shared_ptr effect) { - // need to make a new vector because the old one is being iterated over in another thread - std::shared_ptr>> newEffects = std::make_shared>>(); - for (auto& e : *enabledEffects) { - newEffects->push_back(e); - } // remove any existing effects with the same id - for (auto it = newEffects->begin(); it != newEffects->end();) { + for (auto it = enabledEffects->begin(); it != enabledEffects->end();) { if ((*it)->getId() == effect->getId()) { - it = newEffects->erase(it); + it = enabledEffects->erase(it); } else { it++; } } - enabledEffects = newEffects; } +// effectsLock MUST be held when calling this void OscirenderAudioProcessor::updateEffectPrecedence() { - // need to make a new vector because the old one is being iterated over in another thread - std::shared_ptr>> newEffects = std::make_shared>>(); - for (auto& e : *enabledEffects) { - newEffects->push_back(e); - } - std::sort(newEffects->begin(), newEffects->end(), [](std::shared_ptr a, std::shared_ptr b) { + std::sort(enabledEffects->begin(), enabledEffects->end(), [](std::shared_ptr a, std::shared_ptr b) { return a->getPrecedence() < b->getPrecedence(); }); - enabledEffects = newEffects; } +// parsersLock AND effectsLock must be locked before calling this function void OscirenderAudioProcessor::updateFileBlock(int index, std::shared_ptr block) { if (index < 0 || index >= fileBlocks.size()) { return; @@ -243,6 +233,7 @@ void OscirenderAudioProcessor::updateFileBlock(int index, std::shared_ptr()); files.push_back(file); @@ -252,6 +243,7 @@ void OscirenderAudioProcessor::addFile(juce::File file) { openFile(fileBlocks.size() - 1); } +// parsersLock AND effectsLock must be locked before calling this function void OscirenderAudioProcessor::removeFile(int index) { if (index < 0 || index >= fileBlocks.size()) { return; @@ -266,18 +258,20 @@ int OscirenderAudioProcessor::numFiles() { return fileBlocks.size(); } +// used for opening NEW files. Should be the default way of opening files as +// it will reparse any existing files, so it is safer. +// parsersLock AND effectsLock must be locked before calling this function void OscirenderAudioProcessor::openFile(int index) { if (index < 0 || index >= fileBlocks.size()) { return; } parsers[index]->parse(files[index].getFileExtension(), std::make_unique(*fileBlocks[index], false)); - producer->setSource(parsers[index], index); - currentFile = index; - invalidateFrameBuffer = true; - updateLuaValues(); - updateObjValues(); + changeCurrentFile(index); } +// used ONLY for changing the current file to an EXISTING file. +// much faster than openFile(int index) because it doesn't reparse any files. +// parsersLock AND effectsLock must be locked before calling this function void OscirenderAudioProcessor::changeCurrentFile(int index) { if (index == -1) { currentFile = -1; @@ -289,6 +283,8 @@ void OscirenderAudioProcessor::changeCurrentFile(int index) { producer->setSource(parsers[index], index); currentFile = index; invalidateFrameBuffer = true; + updateLuaValues(); + updateObjValues(); } int OscirenderAudioProcessor::getCurrentFileIndex() { @@ -415,12 +411,13 @@ void OscirenderAudioProcessor::processBlock (juce::AudioBuffer& buffer, j double drawingProgress = length == 0.0 ? 1 : shapeDrawn / length; channels = shape->nextVector(drawingProgress); } - - auto enabledEffectsCopy = enabledEffects; - for (auto effect : *enabledEffectsCopy) { - channels = effect->apply(sample, channels); - } + { + juce::SpinLock::ScopedLockType lock(effectsLock); + for (auto effect : *enabledEffects) { + channels = effect->apply(sample, channels); + } + } x = channels.x; y = channels.y; diff --git a/Source/PluginProcessor.h b/Source/PluginProcessor.h index 4db4565..d2a1a63 100644 --- a/Source/PluginProcessor.h +++ b/Source/PluginProcessor.h @@ -68,9 +68,9 @@ public: double currentSampleRate = 0.0; + juce::SpinLock effectsLock; std::vector> allEffects; std::shared_ptr>> enabledEffects = std::make_shared>>(); - std::vector> luaEffects; // TODO see if there is a way to move this code to .cpp diff --git a/Source/components/EffectsListComponent.cpp b/Source/components/EffectsListComponent.cpp index 06b369c..d4a3320 100644 --- a/Source/components/EffectsListComponent.cpp +++ b/Source/components/EffectsListComponent.cpp @@ -8,16 +8,22 @@ EffectsListComponent::EffectsListComponent(DraggableListBox& lb, AudioEffectList ((AudioEffectListBoxItemData&)modelData).setValue(rowNum, this->effectComponent->slider.getValue()); }; - // check if effect is in audioProcessor enabled effects bool isSelected = false; - for (auto effect : *data.audioProcessor.enabledEffects) { - if (effect->getId() == data.getId(rn)) { - isSelected = true; - break; - } + + { + juce::SpinLock::ScopedLockType lock(data.audioProcessor.effectsLock); + // check if effect is in audioProcessor enabled effects + for (auto effect : *data.audioProcessor.enabledEffects) { + if (effect->getId() == data.getId(rn)) { + isSelected = true; + break; + } + } } effectComponent->selected.setToggleState(isSelected, juce::dontSendNotification); effectComponent->selected.onClick = [this] { + auto data = (AudioEffectListBoxItemData&)modelData; + juce::SpinLock::ScopedLockType lock(data.audioProcessor.effectsLock); ((AudioEffectListBoxItemData&)modelData).setSelected(rowNum, this->effectComponent->selected.getToggleState()); }; } diff --git a/Source/components/EffectsListComponent.h b/Source/components/EffectsListComponent.h index 9f6a746..7aaa7b3 100644 --- a/Source/components/EffectsListComponent.h +++ b/Source/components/EffectsListComponent.h @@ -17,15 +17,19 @@ struct AudioEffectListBoxItemData : public DraggableListBoxItemData return data.size(); } + // CURRENTLY NOT USED void deleteItem(int indexOfItemToDelete) override { - data.erase(data.begin() + indexOfItemToDelete); + // data.erase(data.begin() + indexOfItemToDelete); } + // CURRENTLY NOT USED void addItemAtEnd() override { // data.push_back(juce::String("Yahoo")); } void moveBefore(int indexOfItemToMove, int indexOfItemToPlaceBefore) override { + juce::SpinLock::ScopedLockType lock(audioProcessor.effectsLock); + auto effect = data[indexOfItemToMove]; if (indexOfItemToMove < indexOfItemToPlaceBefore) { @@ -42,6 +46,8 @@ struct AudioEffectListBoxItemData : public DraggableListBoxItemData } void moveAfter(int indexOfItemToMove, int indexOfItemToPlaceAfter) override { + juce::SpinLock::ScopedLockType lock(audioProcessor.effectsLock); + auto temp = data[indexOfItemToMove]; if (indexOfItemToMove <= indexOfItemToPlaceAfter) { diff --git a/Source/components/LuaListComponent.cpp b/Source/components/LuaListComponent.cpp index 93ce075..7b52e92 100644 --- a/Source/components/LuaListComponent.cpp +++ b/Source/components/LuaListComponent.cpp @@ -28,12 +28,14 @@ void LuaListBoxModel::paintListBoxItem(int rowNumber, juce::Graphics& g, int wid juce::Component* LuaListBoxModel::refreshComponentForRow(int rowNum, bool isRowSelected, juce::Component *existingComponentToUpdate) { if (rowNum < getNumRows() - 1) { + juce::SpinLock::ScopedLockType lock(audioProcessor.effectsLock); std::unique_ptr item(dynamic_cast(existingComponentToUpdate)); if (juce::isPositiveAndBelow(rowNum, getNumRows())) { item = std::make_unique(audioProcessor, *audioProcessor.luaEffects[rowNum]); } return item.release(); } else { + juce::SpinLock::ScopedLockType lock(audioProcessor.effectsLock); std::unique_ptr item(dynamic_cast(existingComponentToUpdate)); item = std::make_unique("+"); item->onClick = [this]() {