Revamp audio plugins module#47
Open
luapmartin wants to merge 7 commits into
Open
Conversation
7 tasks
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces enum-based plugin/resource types with string-based audioplugins::AudioResourceType and AudioResourceMeta, adds AudioPluginState and migration/register infrastructure (IKnownAudioPluginsMigrationRegister, KnownAudioPluginsMigrationRegister), versioned known-plugins cache I/O, runtime-only attribute defaults, PluginType and VST attribute constants, refactors scanning/registration to use explicit states and placeholders, updates engine/VST/musesampler plumbing to the new types, and adapts/extends tests and mocks accordingly. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/audioplugins/tests/knownaudiopluginsregistertest.cpp (1)
75-93: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDrop the vestigial
RESOURCE_TYPE_TO_STRmap.
AudioResourceTypeis now a wire string, so this map is a single-entry identity for"VstPlugin"and silently rewrites any other type to"Undefined"via the fallback. New tests that introduce e.g.FluidSoundfontorMuseSamplerSoundPackwould produce wrong expected JSON without any signal. Set the field directly toinfo.meta.type.♻️ Proposed simplification
ByteArray pluginInfoListToJson(const std::vector<AudioPluginInfo>& infoList) const { - const std::map<AudioResourceType, std::string> RESOURCE_TYPE_TO_STR { - { "VstPlugin", "VstPlugin" }, - }; - JsonArray array; @@ JsonObject metaObj; metaObj.set("id", info.meta.id); - metaObj.set("type", muse::value(RESOURCE_TYPE_TO_STR, info.meta.type, "Undefined")); + metaObj.set("type", info.meta.type.toStdString());(adjust the conversion to match
AudioResourceType's underlying string type.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/audioplugins/tests/knownaudiopluginsregistertest.cpp` around lines 75 - 93, Remove the vestigial RESOURCE_TYPE_TO_STR map and stop translating AudioResourceType via muse::value; instead set the JSON "type" directly from info.meta.type (adjusting conversion to the underlying string type as needed), i.e. replace the muse::value(RESOURCE_TYPE_TO_STR, info.meta.type, "Undefined") call in the metaObj.set("type", ...) expression with a direct use of info.meta.type (or info.meta.type.toStdString()/equivalent) so new resource types like FluidSoundfont or MuseSamplerSoundPack are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/audio/engine/internal/audioengineconfiguration.cpp`:
- Line 44: The literal "FluidSoundfont" should be replaced with the canonical
FLUID_SOUNDFONT_TYPE_NAME constant to avoid duplicating the wire-string; update
the code in audioengineconfiguration.cpp to use the constant (e.g.
muse::audio::synth::FLUID_SOUNDFONT_TYPE_NAME or the correct namespace where
FLUID_SOUNDFONT_TYPE_NAME is declared) and ensure the existing include of
soundfonttypes.h provides that symbol or add the proper include/using directive
so the compiler can find FLUID_SOUNDFONT_TYPE_NAME.
In `@framework/audioplugins/audiopluginstypes.h`:
- Around line 81-85: AudioResourceMeta::operator< currently returns id <
other.id || vendor < other.vendor which is not a strict weak ordering; change
the comparator to perform a lexicographical comparison of all relevant members
(e.g., compare id first, if equal compare vendor, if equal compare type, then
attributes) so that the relation is transitive, antisymmetric, and returns false
when all compared fields are equal; implement this either with a sequence of
explicit comparisons or using std::tie(id, vendor, type, attributes) <
std::tie(...).
In `@framework/audioplugins/iknownaudiopluginsmigrationregister.h`:
- Line 32: CURRENT_KNOWN_AUDIO_PLUGINS_VERSION is set to 3 while the PR only
registers migrations 0->1 and 1->2, causing migrate() to fail for caches at
version 2; either implement and register the missing 2->3 migration (add the
migration handler and register it alongside the existing 0->1 and 1->2 entries)
or lower CURRENT_KNOWN_AUDIO_PLUGINS_VERSION to 2 to match the available
migrations so migrate() has a complete migration path. Ensure the change updates
the same symbols: CURRENT_KNOWN_AUDIO_PLUGINS_VERSION and the migration
registration logic used by migrate().
In `@framework/audioplugins/internal/knownaudiopluginsregister.cpp`:
- Around line 131-138: The object-branch currently treats a missing or
wrong-typed "version" or "plugins" field as defaults (fileVersion=0, empty
array) which can silently accept truncated/malformed envelopes; instead validate
the envelope: when json.isObject() fails to contain a numeric "version" and an
array-valued "plugins", log an error mentioning knownAudioPluginsPath and return
a Ret error (same style as the existing Unrecognized branch) rather than
defaulting; update the code around JsonObject root = json.rootObject(),
fileVersion = root.value("version").toInt(), and array =
root.value("plugins").toArray() to explicitly check root.contains("version") &&
root.value("version").isDouble()/isNumber and root.contains("plugins") &&
root.value("plugins").isArray(), extract/assign fileVersion and array only when
valid, otherwise return an error Ret.
- Around line 154-156: Runtime-only defaults are not overriding cached values
because emplace() retains existing entries in info.meta.attributes; change the
loop that iterates configuration()->runtimeAttributeDefaults() so it assigns
into info.meta.attributes (e.g., info.meta.attributes[kv.first] = kv.second)
instead of using emplace(), ensuring runtime defaults overwrite any legacy
cached entries.
In `@framework/audioplugins/internal/registeraudiopluginsscenario.cpp`:
- Around line 104-105: The calls to
knownPluginsRegister()->setPluginsState(result.missingPluginIds,
AudioPluginState::Missing) and
knownPluginsRegister()->setPluginsState(result.rediscoveredPluginIds,
AudioPluginState::Validated) currently discard their Ret values; update
RegisterAudioPluginScenario (or the surrounding function) to capture each
setPluginsState() return, check for failure, and abort/return an error (or
propagate the Ret) before proceeding to registration so the scan does not
continue on partial cache-write failures; ensure you reference the two calls
with result.missingPluginIds and result.rediscoveredPluginIds and
handle/log/propagate the Ret from setPluginsState() appropriately.
- Around line 57-63: The code currently maps each io::path_t to a single
CacheEntry (struct CacheEntry { AudioResourceId id; AudioPluginState state; })
which drops duplicate plugin IDs for the same binary path; change registered
from std::map<io::path_t, CacheEntry> to std::map<io::path_t,
std::vector<CacheEntry>> and, where you populate it from
knownPluginsRegister()->pluginInfoList(), push_back a CacheEntry for each info
(registered[info.path].push_back({ info.meta.id, info.state })). Update all
subsequent logic that formerly accessed a single CacheEntry by key (the blocks
handling Missing/Validated updates around the functions that iterate registered)
to iterate the vector and update each CacheEntry.state for matching
AudioResourceId values so every cached ID for a shared binary path is preserved
and updated.
In `@framework/audioplugins/iregisteraudiopluginsscenario.h`:
- Around line 49-53: The registerNewPlugins method currently returns void but
performs multiple fallible operations; change its signature from void
registerNewPlugins(const io::paths_t& pluginPaths, bool validate = true) to
return Ret and propagate that through implementations (short-circuit and return
the first Ret error encountered during cache write/load or any other failing
step). Update all classes implementing IRegisterAudioPluginsScenario to
implement Ret registerNewPlugins(...), ensure callers check and propagate the
Ret result instead of assuming success, and update the comment above the
declaration to reflect the new return value semantics.
In `@framework/vst/internal/vstmodulesrepository.cpp`:
- Around line 123-133: The code hard-codes u"Instrument" inside
VstModulesRepository::modulesMetaList to distinguish instruments from FX;
extract this literal into a named constant (e.g. INSTRUMENT_CATEGORY) declared
alongside CATEGORIES_ATTRIBUTE (suggest placing in vstpluginattrs.h or next to
PluginCategory) and replace the inline u"Instrument" usage with that constant so
both producer and consumer share the same symbol (update includes/usings as
needed to reference INSTRUMENT_CATEGORY where modulesMetaList reads
info.meta.attributeVal(muse::vst::CATEGORIES_ATTRIBUTE)).
In `@framework/vst/internal/vstpluginmetareader.cpp`:
- Around line 25-27: vstpluginmetareader.cpp relies on symbols
AUDIO_RESOURCE_TYPE_NAME and CATEGORIES_ATTRIBUTE but only pulls them
transitively via vsttypes.h; add an explicit include for "vstpluginattrs.h" at
the top of framework/vst/internal/vstpluginmetareader.cpp so the file directly
declares its dependency (locate the top includes near the existing `#include`
"vsttypes.h" and insert `#include` "vstpluginattrs.h" there).
In `@framework/vst/vstpluginattrs.h`:
- Around line 30-33: The header defines static const String CATEGORIES_ATTRIBUTE
which creates per-translation-unit copies; change its definition to use inline
const (matching AUDIO_RESOURCE_TYPE_NAME's inline constexpr style) so there is a
single shared definition of CATEGORIES_ATTRIBUTE across TUs; update the
declaration for the String named CATEGORIES_ATTRIBUTE to be inline const String
to avoid duplicate construction/destruction and address-inequality issues.
---
Outside diff comments:
In `@framework/audioplugins/tests/knownaudiopluginsregistertest.cpp`:
- Around line 75-93: Remove the vestigial RESOURCE_TYPE_TO_STR map and stop
translating AudioResourceType via muse::value; instead set the JSON "type"
directly from info.meta.type (adjusting conversion to the underlying string type
as needed), i.e. replace the muse::value(RESOURCE_TYPE_TO_STR, info.meta.type,
"Undefined") call in the metaObj.set("type", ...) expression with a direct use
of info.meta.type (or info.meta.type.toStdString()/equivalent) so new resource
types like FluidSoundfont or MuseSamplerSoundPack are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5095749-7e36-4068-a444-e9de5e141a23
📒 Files selected for processing (47)
framework/audio/common/audiotypes.hframework/audio/common/audioutils.hframework/audio/common/rpc/rpcpacker.hframework/audio/engine/internal/audioengineconfiguration.cppframework/audio/engine/internal/enginerpccontroller.cppframework/audio/engine/internal/synthesizers/fluidsynth/fluidresolver.cppframework/audio/tests/CMakeLists.txtframework/audio/tests/audioresourcetypes_tests.cppframework/audio/tests/rpcpacker_tests.cppframework/audioplugins/CMakeLists.txtframework/audioplugins/audiopluginsmodule.cppframework/audioplugins/audiopluginstypes.hframework/audioplugins/iaudiopluginmetareader.hframework/audioplugins/iaudiopluginsconfiguration.hframework/audioplugins/iknownaudiopluginsmigrationregister.hframework/audioplugins/iknownaudiopluginsregister.hframework/audioplugins/internal/audiopluginsconfiguration.cppframework/audioplugins/internal/audiopluginsconfiguration.hframework/audioplugins/internal/knownaudiopluginsmigrationregister.cppframework/audioplugins/internal/knownaudiopluginsmigrationregister.hframework/audioplugins/internal/knownaudiopluginsregister.cppframework/audioplugins/internal/knownaudiopluginsregister.hframework/audioplugins/internal/registeraudiopluginsscenario.cppframework/audioplugins/internal/registeraudiopluginsscenario.hframework/audioplugins/iregisteraudiopluginsscenario.hframework/audioplugins/tests/CMakeLists.txtframework/audioplugins/tests/audiopluginsutilstest.cppframework/audioplugins/tests/knownaudiopluginsmigrationregistertest.cppframework/audioplugins/tests/knownaudiopluginsregistertest.cppframework/audioplugins/tests/mocks/audiopluginmetareadermock.hframework/audioplugins/tests/mocks/audiopluginsconfigurationmock.hframework/audioplugins/tests/mocks/knownaudiopluginsmigrationregistermock.hframework/audioplugins/tests/mocks/knownaudiopluginsregistermock.hframework/audioplugins/tests/registeraudiopluginsscenariotest.cppframework/musesampler/internal/musesamplerresolver.cppframework/musesampler/musesamplertypes.hframework/vst/CMakeLists.txtframework/vst/internal/fx/vstfxprocessor.cppframework/vst/internal/synth/vstsynthesiser.cppframework/vst/internal/vstaudioclient.cppframework/vst/internal/vstaudioclient.hframework/vst/internal/vstmodulesrepository.cppframework/vst/internal/vstmodulesrepository.hframework/vst/internal/vstpluginmetareader.cppframework/vst/internal/vstpluginmetareader.hframework/vst/vstpluginattrs.hframework/vst/vsttypes.h
💤 Files with no reviewable changes (1)
- framework/audioplugins/tests/audiopluginsutilstest.cpp
|
|
||
| namespace muse::audioplugins { | ||
| inline AudioPluginType audioPluginTypeFromCategoriesString(const String& categoriesStr) | ||
| inline constexpr int CURRENT_KNOWN_AUDIO_PLUGINS_VERSION = 3; |
There was a problem hiding this comment.
The cache version is one step ahead of the registered migrations.
CURRENT_KNOWN_AUDIO_PLUGINS_VERSION is 3, but the concrete register in this PR only provides 0 -> 1 and 1 -> 2. Any legacy cache upgrade will still require a missing 2 -> 3 step and fail in migrate().
Minimal fix
-inline constexpr int CURRENT_KNOWN_AUDIO_PLUGINS_VERSION = 3;
+inline constexpr int CURRENT_KNOWN_AUDIO_PLUGINS_VERSION = 2;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| inline constexpr int CURRENT_KNOWN_AUDIO_PLUGINS_VERSION = 3; | |
| inline constexpr int CURRENT_KNOWN_AUDIO_PLUGINS_VERSION = 2; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/audioplugins/iknownaudiopluginsmigrationregister.h` at line 32,
CURRENT_KNOWN_AUDIO_PLUGINS_VERSION is set to 3 while the PR only registers
migrations 0->1 and 1->2, causing migrate() to fail for caches at version 2;
either implement and register the missing 2->3 migration (add the migration
handler and register it alongside the existing 0->1 and 1->2 entries) or lower
CURRENT_KNOWN_AUDIO_PLUGINS_VERSION to 2 to match the available migrations so
migrate() has a complete migration path. Ensure the change updates the same
symbols: CURRENT_KNOWN_AUDIO_PLUGINS_VERSION and the migration registration
logic used by migrate().
8 tasks
Contributor
Author
|
/build |
| static const String PLAYBACK_SETUP_DATA_ATTRIBUTE(u"playbackSetupData"); | ||
| static const String CATEGORIES_ATTRIBUTE(u"categories"); | ||
|
|
||
| using AudioResourceId = muse::audioplugins::AudioResourceId; |
Contributor
There was a problem hiding this comment.
Could you explain the purpose of this change? Not only does it create circular dependencies between the modules, but it also looks conceptually incorrect. AudioResource can be anything (soundfont, MuseSounds, etc.), not only audio plugins
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
framework/audioplugins/internal/knownaudiopluginsregister.cpp (2)
249-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNotify observers after in-memory registry mutations.
setPluginsState()andremovePluginsAtPath()both mutatem_pluginInfoMap/m_pluginPathsand persist the result, but neither emitsm_pluginInfoListChanged. Subscribers only get notified onload(), so Missing/rediscovered/error transitions can stay invisible until a full reload.🔔 Minimal fix pattern
- return writePluginsInfo(); + Ret ret = writePluginsInfo(); + if (ret) { + m_pluginInfoListChanged.notify(); + } + return ret;Apply the same pattern to both mutators, or centralize it in a shared post-write helper so all registry mutations notify consistently.
Also applies to: 305-327
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/audioplugins/internal/knownaudiopluginsregister.cpp` around lines 249 - 275, setPluginsState and removePluginsAtPath currently mutate m_pluginInfoMap / m_pluginPaths and persist via writePluginsInfo but never notify observers, so update these mutators to emit m_pluginInfoListChanged after a successful in-memory change + successful write; specifically, in KnownAudioPluginsRegister::setPluginsState (and the analogous removePluginsAtPath implementation) call the existing notification/emitter for m_pluginInfoListChanged (or invoke a shared helper you create, e.g., notifyPluginInfoListChangedPostWrite) only when changed is true and writePluginsInfo() returns success, so subscribers see Missing/rediscovered/error transitions without requiring a full load.
156-171:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject malformed plugin rows before inserting them.
load()now validates the root envelope, but it still inserts entries even whenmetaorpathis missing.metaFromJson({})yields emptyid/vendor/type, so one truncated row can create an empty-key record and poison later lookups.🛡️ Proposed fix
AudioPluginInfo info; info.meta = metaFromJson(object.value("meta").toObject()); for (const auto& kv : configuration()->runtimeAttributeDefaults()) { // Assign, don't emplace: runtime-only defaults must override any // stale value a legacy cache still carries for the same key. info.meta.attributes[kv.first] = kv.second; } info.path = object.value("path").toString(); info.state = audioPluginStateFromName(object.value("state").toStdString()); info.errorCode = object.value("errorCode").toInt(); + + if (!info.meta.isValid() || info.path.empty()) { + LOGE() << "Malformed known-audio-plugins entry at " << knownAudioPluginsPath; + return Ret(static_cast<int>(Ret::Code::UnknownError), + "Malformed known_audio_plugins.json entry"); + } m_pluginPaths.insert(info.path); m_pluginInfoMap.emplace(info.meta.id, std::move(info));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/audioplugins/internal/knownaudiopluginsregister.cpp` around lines 156 - 171, The loop currently inserts entries even when meta or path are missing, which allows empty keys (from metaFromJson({})) to poison lookups; update the loop that builds AudioPluginInfo (using metaFromJson, info.path, info.meta.id, m_pluginPaths, m_pluginInfoMap) to validate that info.meta.id and info.path are non-empty (and optionally that meta.vendor/type are present if required) and skip the entry (continue) when they are malformed so you do not call m_pluginPaths.insert(...) or m_pluginInfoMap.emplace(...) for invalid rows.framework/audioplugins/internal/knownaudiopluginsmigrationregister.cpp (1)
39-58: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the canonical state-name helper in the v1→v2 migrator.
This migration hard-codes
"Validated"and"Error"even thoughaudioPluginStateName()is the canonical wire mapping. If those names ever drift, migrated caches and runtime parsing will diverge.♻️ Proposed fix
- obj.set("state", enabled ? std::string("Validated") : std::string("Error")); + obj.set("state", audioPluginStateName(enabled + ? AudioPluginState::Validated + : AudioPluginState::Error));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/audioplugins/internal/knownaudiopluginsmigrationregister.cpp` around lines 39 - 58, The migrator registered in registerMigration(1, ...) currently hardcodes "Validated" and "Error"; change it to use the canonical helper audioPluginStateName(...) so wire names stay consistent. Inside the lambda that iterates JsonArray plugins, replace the hardcoded strings with audioPluginStateName(AudioPluginState::Validated) for enabled==true and audioPluginStateName(AudioPluginState::Error) for enabled==false (keeping the same logic where "enabled" is removed and "state" set). Make sure the AudioPluginState symbol and audioPluginStateName function are available in this translation unit or include the proper header if missing.framework/vst/internal/vstpluginmetareader.cpp (1)
57-70:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStop collapsing VST3 modules with multiple audio-effect classes into a single cache entry
VstPluginMetaReader::readMeta()emits only oneAudioResourceMetaperpluginPath(breaks after the firstkVstAudioEffectClass) and assigns a basename-onlymeta.idviaio::completeBasename(pluginPath).toStdString().VstPluginInstance::load()also breaks after the firstkVstAudioEffectClass, so it can’t select a specificClassInfoeven if more classes were discovered.- This prevents representing/transitioning multiple audio-effect classes from the same VST3 binary under the multi-ID-per-path behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/vst/internal/vstpluginmetareader.cpp` around lines 57 - 70, VstPluginMetaReader::readMeta() currently stops after the first kVstAudioEffectClass and uses only the basename for meta.id (io::completeBasename(pluginPath).toStdString()), collapsing multiple audio-effect classes into one entry; remove the break and emit one AudioResourceMeta per ClassInfo in factory.classInfos() that matches kVstAudioEffectClass, setting meta.id to a unique multi-ID-per-path form (e.g. include pluginPath plus a class-specific identifier such as classInfo.id() or classInfo.name()) and preserve attributes/vendor as before; also update VstPluginInstance::load() to not break on the first class and to select/load a specific ClassInfo when a particular class ID is requested so the code can represent multiple audio-effect classes from the same VST3 binary.framework/audioplugins/internal/registeraudiopluginsscenario.cpp (1)
250-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck
removePluginsAtPath()before writing replacement entries.Lines 254 and 293 discard the
Ret. If that cache update fails, the subprocess continues against stale state and can either trip the same-id guard or leave the stale placeholder on disk while returning the laterregisterPlugins()result.Proposed fix
- knownPluginsRegister()->removePluginsAtPath(pluginPath); + Ret ret = knownPluginsRegister()->removePluginsAtPath(pluginPath); + if (!ret) { + LOGE() << "Failed to clear existing plugin entry at path: " + << pluginPath.toStdString() << ", error: " << ret.toString(); + return ret; + } @@ - Ret ret = knownPluginsRegister()->registerPlugins(infoList); - return ret; + return knownPluginsRegister()->registerPlugins(infoList);Apply the same guard in
registerFailedPlugin()before building theErrorentry.Also applies to: 290-293
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/audioplugins/internal/registeraudiopluginsscenario.cpp` around lines 250 - 255, The call to knownPluginsRegister()->removePluginsAtPath(pluginPath) is returning a Ret that is currently ignored, which can leave stale placeholders or trip same-id guards; update the code around removePluginsAtPath (and the similar call around line ~290) to check the returned result and abort/return early (or propagate the error) if the cache update fails instead of continuing to write replacement entries; also apply the same pre-check in registerFailedPlugin() before constructing/writing the Error entry so you don't write error placeholders when removePluginsAtPath failed; reference removePluginsAtPath, knownPluginsRegister(), registerPlugins(), and registerFailedPlugin() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/audioplugins/internal/registeraudiopluginsscenario.cpp`:
- Around line 80-98: The loop currently only treats AudioPluginState::Discovered
as a reason to re-validate a path and only treats AudioPluginState::Missing as
rediscovered; update the logic to also consider AudioPluginState::Error as a
visible binary so paths with any CacheEntry.state == AudioPluginState::Error are
re-validated (add Error into the hasDiscovered predicate alongside Discovered)
and treat previously Error entries like Missing entries by pushing their
entry.id into result.rediscoveredPluginIds before erasing from registered so
errored plugins seen on disk get retried.
---
Outside diff comments:
In `@framework/audioplugins/internal/knownaudiopluginsmigrationregister.cpp`:
- Around line 39-58: The migrator registered in registerMigration(1, ...)
currently hardcodes "Validated" and "Error"; change it to use the canonical
helper audioPluginStateName(...) so wire names stay consistent. Inside the
lambda that iterates JsonArray plugins, replace the hardcoded strings with
audioPluginStateName(AudioPluginState::Validated) for enabled==true and
audioPluginStateName(AudioPluginState::Error) for enabled==false (keeping the
same logic where "enabled" is removed and "state" set). Make sure the
AudioPluginState symbol and audioPluginStateName function are available in this
translation unit or include the proper header if missing.
In `@framework/audioplugins/internal/knownaudiopluginsregister.cpp`:
- Around line 249-275: setPluginsState and removePluginsAtPath currently mutate
m_pluginInfoMap / m_pluginPaths and persist via writePluginsInfo but never
notify observers, so update these mutators to emit m_pluginInfoListChanged after
a successful in-memory change + successful write; specifically, in
KnownAudioPluginsRegister::setPluginsState (and the analogous
removePluginsAtPath implementation) call the existing notification/emitter for
m_pluginInfoListChanged (or invoke a shared helper you create, e.g.,
notifyPluginInfoListChangedPostWrite) only when changed is true and
writePluginsInfo() returns success, so subscribers see
Missing/rediscovered/error transitions without requiring a full load.
- Around line 156-171: The loop currently inserts entries even when meta or path
are missing, which allows empty keys (from metaFromJson({})) to poison lookups;
update the loop that builds AudioPluginInfo (using metaFromJson, info.path,
info.meta.id, m_pluginPaths, m_pluginInfoMap) to validate that info.meta.id and
info.path are non-empty (and optionally that meta.vendor/type are present if
required) and skip the entry (continue) when they are malformed so you do not
call m_pluginPaths.insert(...) or m_pluginInfoMap.emplace(...) for invalid rows.
In `@framework/audioplugins/internal/registeraudiopluginsscenario.cpp`:
- Around line 250-255: The call to
knownPluginsRegister()->removePluginsAtPath(pluginPath) is returning a Ret that
is currently ignored, which can leave stale placeholders or trip same-id guards;
update the code around removePluginsAtPath (and the similar call around line
~290) to check the returned result and abort/return early (or propagate the
error) if the cache update fails instead of continuing to write replacement
entries; also apply the same pre-check in registerFailedPlugin() before
constructing/writing the Error entry so you don't write error placeholders when
removePluginsAtPath failed; reference removePluginsAtPath,
knownPluginsRegister(), registerPlugins(), and registerFailedPlugin() when
making the change.
In `@framework/vst/internal/vstpluginmetareader.cpp`:
- Around line 57-70: VstPluginMetaReader::readMeta() currently stops after the
first kVstAudioEffectClass and uses only the basename for meta.id
(io::completeBasename(pluginPath).toStdString()), collapsing multiple
audio-effect classes into one entry; remove the break and emit one
AudioResourceMeta per ClassInfo in factory.classInfos() that matches
kVstAudioEffectClass, setting meta.id to a unique multi-ID-per-path form (e.g.
include pluginPath plus a class-specific identifier such as classInfo.id() or
classInfo.name()) and preserve attributes/vendor as before; also update
VstPluginInstance::load() to not break on the first class and to select/load a
specific ClassInfo when a particular class ID is requested so the code can
represent multiple audio-effect classes from the same VST3 binary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b7335ce-a62d-496e-8e72-ae56078837a2
📒 Files selected for processing (12)
framework/audio/engine/internal/audioengineconfiguration.cppframework/audioplugins/audiopluginstypes.hframework/audioplugins/internal/knownaudiopluginsmigrationregister.cppframework/audioplugins/internal/knownaudiopluginsregister.cppframework/audioplugins/internal/registeraudiopluginsscenario.cppframework/audioplugins/internal/registeraudiopluginsscenario.hframework/audioplugins/iregisteraudiopluginsscenario.hframework/audioplugins/tests/knownaudiopluginsmigrationregistertest.cppframework/audioplugins/tests/registeraudiopluginsscenariotest.cppframework/vst/internal/vstmodulesrepository.cppframework/vst/internal/vstpluginmetareader.cppframework/vst/vstpluginattrs.h
| using AudioResourceAttributes = muse::audioplugins::AudioResourceAttributes; | ||
| using AudioResourceMeta = muse::audioplugins::AudioResourceMeta; | ||
| using AudioResourceMetaList = muse::audioplugins::AudioResourceMetaList; | ||
| using AudioResourceMetaSet = muse::audioplugins::AudioResourceMetaSet; |
Member
There was a problem hiding this comment.
I think we should simply break the connection between audio and audioplugins
i.e., consider them to have nothing in common.
Maybe it makes sense to rename audioplugins to something else, like userplugins
So, as it was:
-
Audio plugins loaded and created audio resources - the problem here is that Audacity don't use our audio, so they have "different" terms, resources, etc.
-
Now, as I understand it, our audio uses audio plugins, and their types - well, that's also not very clear, why should our audio depend on an auxiliary module...
-
Therefore, I think we need to break this connection completely:
-
MF audio data types exist in our terminology, whatever it may be.
-
There are data types for Audacity audio and effects.
-
There is a module that verifies and loads user plugins and its data types.
Each application has its own mapping:
PluginTypes (some meta) - AudioTypes (resource) for MF
PluginTypes (some meta) - EffectsTypes (...) for Audacity
Audio plugins must have their own types, and we need to convert one type to another
Member
There was a problem hiding this comment.
Actually, I think right now it would be enough to do something like this:
- muse::audioplugins::AudioResourceId;
- muse::audio::AudioResourceId;
- Find a place to make
audioplugins::AudioResourceId plugId = ...
audio::AudioResourceId audioId = plugId;
or better
audio::AudioResourceId toAudioResourceId(const audioplugins::AudioResourceId& id)
{
return id;
}
i.e. make it one to one
This may look strange now, but in essence this is exactly what we need
Then we can continue to:
- Rename the types to reduce confusion and make them more meaningful
- Change types independently, we will only need to change the conversion code
luapmartin
force-pushed
the
luapmartin/revamp-audio_plugins-module
branch
from
864f1c5 to
550d364
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/audioplugins/audiopluginstypes.h`:
- Around line 143-162: The current conversion functions return an empty string
for AudioPluginState::Undefined which conflates undefined with unknown names;
update the mapping returned by detail::audioPluginStateNames() to include an
explicit entry for AudioPluginState::Undefined (e.g. "Undefined" or another
clear sentinel) and change audioPluginStateName(AudioPluginState) /
audioPluginStateFromName(const std::string&) to rely on that mapping so the name
lookup returns that explicit string and the reverse lookup maps that exact
string back to AudioPluginState::Undefined, making the conversion bijective and
unambiguous.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 823d4a18-a785-4fe8-887e-01b868c67c76
📒 Files selected for processing (34)
framework/audio/common/audiotypes.hframework/audio/common/audioutils.hframework/audio/common/rpc/rpcpacker.hframework/audio/engine/internal/audioengineconfiguration.cppframework/audio/engine/internal/enginerpccontroller.cppframework/audio/engine/internal/synthesizers/fluidsynth/fluidresolver.cppframework/audio/tests/CMakeLists.txtframework/audio/tests/audioresourcetypes_tests.cppframework/audio/tests/rpcpacker_tests.cppframework/audioplugins/CMakeLists.txtframework/audioplugins/audiopluginsmodule.cppframework/audioplugins/audiopluginstypes.hframework/audioplugins/iaudiopluginmetareader.hframework/audioplugins/iaudiopluginsconfiguration.hframework/audioplugins/iknownaudiopluginsmigrationregister.hframework/audioplugins/iknownaudiopluginsregister.hframework/audioplugins/internal/audiopluginsconfiguration.cppframework/audioplugins/internal/audiopluginsconfiguration.hframework/audioplugins/internal/knownaudiopluginsmigrationregister.cppframework/audioplugins/internal/knownaudiopluginsmigrationregister.hframework/audioplugins/internal/knownaudiopluginsregister.cppframework/audioplugins/internal/knownaudiopluginsregister.hframework/audioplugins/internal/registeraudiopluginsscenario.cppframework/audioplugins/internal/registeraudiopluginsscenario.hframework/audioplugins/iregisteraudiopluginsscenario.hframework/audioplugins/tests/CMakeLists.txtframework/audioplugins/tests/audiopluginsutilstest.cppframework/audioplugins/tests/knownaudiopluginsmigrationregistertest.cppframework/audioplugins/tests/knownaudiopluginsregistertest.cppframework/audioplugins/tests/mocks/audiopluginmetareadermock.hframework/audioplugins/tests/mocks/audiopluginsconfigurationmock.hframework/audioplugins/tests/mocks/knownaudiopluginsmigrationregistermock.hframework/audioplugins/tests/mocks/knownaudiopluginsregistermock.hframework/audioplugins/tests/registeraudiopluginsscenariotest.cpp
💤 Files with no reviewable changes (1)
- framework/audioplugins/tests/audiopluginsutilstest.cpp
luapmartin
force-pushed
the
luapmartin/revamp-audio_plugins-module
branch
from
491a3c0 to
cf7a324
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/audio/common/audioutils.h (1)
40-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve opaque plugin type names instead of collapsing them to
Undefined.Lines 42-49 map the new wire string back through the engine enum and return the
Undefinedlabel for anything outside the core map. That breaks the new contract for app-defined resource types: a valid custom type now renders asUndefinedanywhere this helper is used.♻️ Proposed fix
inline String audioResourceTypeToString(const muse::audioplugins::AudioResourceType& metaType) { AudioResourceType type = resourceTypeFromString(metaType); + if (type == AudioResourceType::Undefined && !metaType.empty()) { + return String::fromStdString(metaType); + } + auto search = RESOURCE_TYPE_MAP.find(type); if (search != RESOURCE_TYPE_MAP.end()) { return search->second; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/audio/common/audioutils.h` around lines 40 - 49, The function audioResourceTypeToString currently maps the incoming muse::audioplugins::AudioResourceType (metaType) through resourceTypeFromString and returns RESOURCE_TYPE_MAP.at(AudioResourceType::Undefined) for unknown types, which collapses custom plugin types; change the fallback so that when RESOURCE_TYPE_MAP.find(type) fails the function returns the original metaType string (i.e., preserve the opaque plugin type name) instead of AudioResourceType::Undefined. Locate audioResourceTypeToString, resourceTypeFromString, and RESOURCE_TYPE_MAP to implement the fallback (return metaType as a String) while keeping the successful branch unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/audioplugins/iknownaudiopluginsmigrationregister.h`:
- Around line 41-44: The migration callback type PluginsMigration currently
returns JsonArray and cannot signal failure; change its signature to return a
result wrapper (e.g. RetVal<JsonArray>) or accept an output parameter plus a Ret
(choose one) so migrations can fail explicitly; update registerMigration to take
the new PluginsMigration type and update migrate to propagate/aggregate those
Ret/RetVal failures (return Ret on error) so callers of migrate and load() can
treat bad cache/migration errors as fatal. Reference: PluginsMigration,
registerMigration, migrate, Ret, RetVal<JsonArray>, and load().
In `@framework/audioplugins/internal/knownaudiopluginsregister.cpp`:
- Around line 258-283: setPluginsState currently calls
m_pluginInfoMap.equal_range(resourceId) and flips state for every entry with the
same meta.id, which incorrectly changes all installs that share an id; change
the update to match both id and path (or make the API path-first) so only the
exact resource row is modified: in KnownAudioPluginsRegister::setPluginsState
iterate entries from m_pluginInfoMap.equal_range(resourceId) but additionally
compare the stored entry's path (e.g., it->second.path or entry.path) against
the incoming AudioResourceId (or extend AudioResourceId to carry path) and only
update when both id and path match, leaving other duplicates untouched, then
call writePluginsInfo() as before.
- Around line 156-180: The load() implementation currently inserts rows into
m_pluginInfoMap and m_pluginPaths as it parses, so a malformed row leaves a
partially populated cache visible to callers (pluginInfoList(), exists()) even
if m_loaded stays false; change load() to parse into local temporary containers
(e.g., local std::unordered_map/temp set or vector) and populate those (using
the same logic that fills info, m_pluginPaths.insert, m_pluginInfoMap.emplace)
and only swap/move the temporaries into m_pluginInfoMap and m_pluginPaths and
set m_loaded = true after the entire file is successfully validated and parsed;
ensure any early-return error paths do not mutate the real members and use
std::move when swapping the parsed AudioPluginInfo entries into the real map.
In `@framework/audioplugins/internal/registeraudiopluginsscenario.cpp`:
- Around line 92-98: The code currently treats any reappearing filename as
"rediscovered" by looping over entries (CacheEntry) and pushing IDs where
entry.state == AudioPluginState::Missing into result.rediscoveredPluginIds and
later flipping them to Validated; instead, do not revive Missing entries
automatically — preserve their Missing state and invoke a fresh validation scan
by calling registerNewPlugins(path, true) for the rediscovered path, and only
move IDs into rediscoveredPluginIds / change their state to Validated if that
rescan actually finds and validates them; update the logic around the entries
loop and the code that flips cached IDs (the block that erases
registered.erase(it) and the subsequent 127-131 logic) to defer state changes
until registerNewPlugins(...) confirms the binaries and exported plugin IDs.
---
Outside diff comments:
In `@framework/audio/common/audioutils.h`:
- Around line 40-49: The function audioResourceTypeToString currently maps the
incoming muse::audioplugins::AudioResourceType (metaType) through
resourceTypeFromString and returns
RESOURCE_TYPE_MAP.at(AudioResourceType::Undefined) for unknown types, which
collapses custom plugin types; change the fallback so that when
RESOURCE_TYPE_MAP.find(type) fails the function returns the original metaType
string (i.e., preserve the opaque plugin type name) instead of
AudioResourceType::Undefined. Locate audioResourceTypeToString,
resourceTypeFromString, and RESOURCE_TYPE_MAP to implement the fallback (return
metaType as a String) while keeping the successful branch unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49177577-0ab6-485d-ba62-f5076dae78ae
📒 Files selected for processing (47)
framework/audio/common/audiotypes.hframework/audio/common/audioutils.hframework/audio/common/rpc/rpcpacker.hframework/audio/engine/internal/audioengineconfiguration.cppframework/audio/engine/internal/enginerpccontroller.cppframework/audio/engine/internal/synthesizers/fluidsynth/fluidresolver.cppframework/audio/tests/CMakeLists.txtframework/audio/tests/audioresourcetypes_tests.cppframework/audio/tests/rpcpacker_tests.cppframework/audioplugins/CMakeLists.txtframework/audioplugins/audiopluginsmodule.cppframework/audioplugins/audiopluginstypes.hframework/audioplugins/iaudiopluginmetareader.hframework/audioplugins/iaudiopluginsconfiguration.hframework/audioplugins/iknownaudiopluginsmigrationregister.hframework/audioplugins/iknownaudiopluginsregister.hframework/audioplugins/internal/audiopluginsconfiguration.cppframework/audioplugins/internal/audiopluginsconfiguration.hframework/audioplugins/internal/knownaudiopluginsmigrationregister.cppframework/audioplugins/internal/knownaudiopluginsmigrationregister.hframework/audioplugins/internal/knownaudiopluginsregister.cppframework/audioplugins/internal/knownaudiopluginsregister.hframework/audioplugins/internal/registeraudiopluginsscenario.cppframework/audioplugins/internal/registeraudiopluginsscenario.hframework/audioplugins/iregisteraudiopluginsscenario.hframework/audioplugins/tests/CMakeLists.txtframework/audioplugins/tests/audiopluginsutilstest.cppframework/audioplugins/tests/knownaudiopluginsmigrationregistertest.cppframework/audioplugins/tests/knownaudiopluginsregistertest.cppframework/audioplugins/tests/mocks/audiopluginmetareadermock.hframework/audioplugins/tests/mocks/audiopluginsconfigurationmock.hframework/audioplugins/tests/mocks/knownaudiopluginsmigrationregistermock.hframework/audioplugins/tests/mocks/knownaudiopluginsregistermock.hframework/audioplugins/tests/registeraudiopluginsscenariotest.cppframework/musesampler/internal/musesamplerresolver.cppframework/musesampler/musesamplertypes.hframework/vst/CMakeLists.txtframework/vst/internal/fx/vstfxprocessor.cppframework/vst/internal/synth/vstsynthesiser.cppframework/vst/internal/vstaudioclient.cppframework/vst/internal/vstaudioclient.hframework/vst/internal/vstmodulesrepository.cppframework/vst/internal/vstmodulesrepository.hframework/vst/internal/vstpluginmetareader.cppframework/vst/internal/vstpluginmetareader.hframework/vst/vstpluginattrs.hframework/vst/vsttypes.h
💤 Files with no reviewable changes (1)
- framework/audioplugins/tests/audiopluginsutilstest.cpp
Comment on lines
+41
to
+44
| using PluginsMigration = std::function<JsonArray (const JsonArray&)>; | ||
|
|
||
| virtual void registerMigration(int fromVersion, PluginsMigration cb) = 0; | ||
| virtual Ret migrate(int fromVersion, int toVersion, JsonArray& plugins) const = 0; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Let migration callbacks fail explicitly.
PluginsMigration can only return a JsonArray, so a framework/app migration has no way to report malformed input or a partial-transform failure. That pushes version-boundary errors into silent coercion/defaulting even though load() otherwise treats bad cache data as fatal. Consider making the callback return RetVal<JsonArray> (or Ret plus an out-param) before more app-owned migrations depend on this API.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/audioplugins/iknownaudiopluginsmigrationregister.h` around lines 41
- 44, The migration callback type PluginsMigration currently returns JsonArray
and cannot signal failure; change its signature to return a result wrapper (e.g.
RetVal<JsonArray>) or accept an output parameter plus a Ret (choose one) so
migrations can fail explicitly; update registerMigration to take the new
PluginsMigration type and update migrate to propagate/aggregate those Ret/RetVal
failures (return Ret on error) so callers of migrate and load() can treat bad
cache/migration errors as fatal. Reference: PluginsMigration, registerMigration,
migrate, Ret, RetVal<JsonArray>, and load().
Comment on lines
156
to
180
| for (size_t i = 0; i < array.size(); ++i) { | ||
| JsonObject object = array.at(i).toObject(); | ||
|
|
||
| AudioPluginInfo info; | ||
| info.meta = metaFromJson(object.value("meta").toObject()); | ||
| info.meta.attributes.emplace(audio::PLAYBACK_SETUP_DATA_ATTRIBUTE, mpe::GENERIC_SETUP_DATA_STRING); | ||
| info.type = audioPluginTypeFromCategoriesString(info.meta.attributeVal(audio::CATEGORIES_ATTRIBUTE)); | ||
| for (const auto& kv : configuration()->runtimeAttributeDefaults()) { | ||
| // Assign, don't emplace: runtime-only defaults must override any | ||
| // stale value a legacy cache still carries for the same key. | ||
| info.meta.attributes[kv.first] = kv.second; | ||
| } | ||
| info.path = object.value("path").toString(); | ||
| info.enabled = object.value("enabled").toBool(); | ||
| info.state = audioPluginStateFromName(object.value("state").toStdString()); | ||
| info.errorCode = object.value("errorCode").toInt(); | ||
|
|
||
| // `id` and `path` are the register's lookup keys — a row missing | ||
| // either (e.g. a truncated `meta`) would poison m_pluginInfoMap with | ||
| // an empty-key record. Reject the whole file, as with a bad envelope. | ||
| if (info.meta.id.empty() || info.path.empty()) { | ||
| LOGE() << "Malformed known-audio-plugins entry at " | ||
| << knownAudioPluginsPath << " (missing id or path)"; | ||
| return Ret(static_cast<int>(Ret::Code::UnknownError), "Malformed known_audio_plugins.json entry"); | ||
| } | ||
|
|
||
| m_pluginPaths.insert(info.path); | ||
| m_pluginInfoMap.emplace(info.meta.id, std::move(info)); |
There was a problem hiding this comment.
Avoid exposing a partially loaded cache after a bad row.
If a malformed entry appears after earlier rows were already inserted, load() returns an error but leaves those earlier rows in m_pluginInfoMap/m_pluginPaths. m_loaded stays false, but readers like pluginInfoList() and exists() do not gate on it, so callers can still observe a partial cache. Parse into temporary containers and swap them into the members only after the whole loop succeeds.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/audioplugins/internal/knownaudiopluginsregister.cpp` around lines
156 - 180, The load() implementation currently inserts rows into m_pluginInfoMap
and m_pluginPaths as it parses, so a malformed row leaves a partially populated
cache visible to callers (pluginInfoList(), exists()) even if m_loaded stays
false; change load() to parse into local temporary containers (e.g., local
std::unordered_map/temp set or vector) and populate those (using the same logic
that fills info, m_pluginPaths.insert, m_pluginInfoMap.emplace) and only
swap/move the temporaries into m_pluginInfoMap and m_pluginPaths and set
m_loaded = true after the entire file is successfully validated and parsed;
ensure any early-return error paths do not mutate the real members and use
std::move when swapping the parsed AudioPluginInfo entries into the real map.
luapmartin
added a commit
to luapmartin/muse_framework
that referenced
this pull request
May 28, 2026
Per reviewer feedback on PR musescore#47: the audioplugins module owns plugin identity, audio uses an id to address resources in the pipeline. Different concepts, same name today - give them different names. This commit only renames. audio/common/audiotypes.h still re-exports under audio::AudioResourceId, now pointing at the renamed audioplugins::PluginResourceId, so everything compiles transparently. The next commit makes audio::AudioResourceId an independent alias and adds the conversion helper.
luapmartin
added a commit
to luapmartin/muse_framework
that referenced
this pull request
May 28, 2026
Per reviewer feedback on PR musescore#47: the audioplugins module owns plugin identity, audio uses an id to address resources in the pipeline. Different concepts, same name today - give them different names. This commit only renames. audio/common/audiotypes.h still re-exports under audio::AudioResourceId, now pointing at the renamed audioplugins::PluginResourceId, so everything compiles transparently. The next commit makes audio::AudioResourceId an independent alias and adds the conversion helper.
luapmartin
force-pushed
the
luapmartin/revamp-audio_plugins-module
branch
from
b01351d to
88eaa9a
Compare
luapmartin
added a commit
to luapmartin/muse_framework
that referenced
this pull request
May 28, 2026
Per reviewer feedback on PR musescore#47: the audioplugins module owns plugin identity, audio uses an id to address resources in the pipeline. Different concepts, same name today - give them different names. This commit only renames. audio/common/audiotypes.h still re-exports under audio::AudioResourceId, now pointing at the renamed audioplugins::PluginResourceId, so everything compiles transparently. The next commit makes audio::AudioResourceId an independent alias and adds the conversion helper.
luapmartin
force-pushed
the
luapmartin/revamp-audio_plugins-module
branch
from
88eaa9a to
6fa2cb6
Compare
…cycle
Decouple the audioplugins module from the audio module so it can host
generic plugin discovery for other apps (e.g. Audacity), and add a
versioned cache schema with app-registered migrations. This commit is
the framework-side half; it is paired with a main-repo commit that
wires the MuseScore-side migrations and bumps the muse/ submodule
pointer.
Decoupling:
- Plugin-shaped types (AudioResourceMeta, AudioResourceAttributes,
AudioResourceId, etc.) live in muse::audioplugins:: instead of
muse::audio::. Audio keeps `using` aliases for source compat.
- audioplugins::AudioResourceType becomes an opaque std::string. Apps
register their own plugin format identifiers; the audio module keeps
an engine-internal enum and converts at the boundary via
resourceTypeFromString() / resourceTypeName().
- Runtime-only attributes (skipped on save, re-injected on load) are
app-registered via IAudioPluginsConfiguration::setRuntimeAttributeDefaults
instead of hard-coded to audio::PLAYBACK_SETUP_DATA_ATTRIBUTE.
- HAS_NATIVE_EDITOR_SUPPORT_ATTRIBUTE and CATEGORIES_ATTRIBUTE move out
of audioplugins (now framework-pure) into audio. meta.hasNativeEditorSupport()
is replaced by a free function audio::hasNativeEditorSupport(meta).
- AudioPluginType, IAudioPluginTypeDetector and AudioPluginInfo.type are
dropped from the framework. The Instrument/Fx classification was
runtime-only, never persisted, and MuseScore-shaped. The VST module
gains its own vst::PluginType and computes the category from meta on
demand.
Cache schema (version field added; bare-array legacy treated as v0):
- v0 -> v1: hasNativeEditorSupport moves from a top-level meta field
into meta.attributes ("true"/"false" strings).
- v1 -> v2: the boolean `enabled` flag becomes a `state` string. The
state lifecycle has four values:
* Discovered: scanner found the file but validation hasn't run yet
* Validated: validation succeeded; usable
* Missing: file no longer found at the previously known path
* Error: validation failed (errorCode carries detail)
- Both migrations are registered MuseScore-side via the new
IKnownAudioPluginsMigrationRegister.
Scanner behaviour:
- scanPlugins() now reports rediscoveredPluginIds (entries that were
Missing and have come back) alongside missingPluginIds.
- updatePluginsRegistry() uses the new
IKnownAudioPluginsRegister::setPluginsState() to mark removed
entries as Missing instead of erasing them, and rediscovered ones
back to Validated. Already-Missing entries stay Missing without
churn. unregisterPlugins() is kept for explicit UI-driven removal.
Typed attribute accessors:
- boolAttribute() / intAttribute() free helpers in muse::audioplugins
encode the on-disk "true"/"1" -> bool and digit-string -> int
conventions in one place, so callers don't reimplement them. Storage
stays as map<String, String>; no JSON / RPC / file-format change.
Tests cover migration chaining, legacy v0 array load, the v0->v1 and
v1->v2 transitions, and the Missing/Rediscovered scanner transitions.
Move CATEGORIES_ATTRIBUTE from audio/common/audiotypes.h into a new vst/vstpluginattrs.h header so VST consumers (including hosts that don't link the audio module) can use it without pulling audio. Fix VstPluginMetaReader::metaType() override return type to match the IAudioPluginMetaReader interface (audioplugins::AudioResourceType, not audio::AudioResourceType which is now an engine enum).
The old `id < a.id || vendor < a.vendor` was not antisymmetric (could report a<b and b<a both true) and ignored type/attributes, so it mis-ordered AudioResourceMetaSet. Compare all four fields lexicographically.
Per reviewer feedback on PR musescore#47: the audioplugins module owns plugin identity, audio uses an id to address resources in the pipeline. Different concepts, same name today - give them different names. This commit only renames. audio/common/audiotypes.h still re-exports under audio::AudioResourceId, now pointing at the renamed audioplugins::PluginResourceId, so everything compiles transparently. The next commit makes audio::AudioResourceId an independent alias and adds the conversion helper.
…nversion helper audio::AudioResourceId is now an independent std::string alias (was a re-export of muse::audioplugins::PluginResourceId). Add toAudioResourceId / toAudioResourceIdList helpers in audio/common/ audiotypes.h - documenting the seam between plugin-identity and audio-pipeline-resource roles. One-directional today: both aliases bottom out at std::string so the conversion is one-to-one; the helper becomes a real bridge if either side later gains a strong wrapper. A static_assert in audioresourcetypes_tests.cpp pins the underlying- std::string contract; a new ToAudioResourceIdRoundTrips test exercises the helper end-to-end. VST and audio-engine resolvers stay on audio::AudioResourceId unchanged - structurally pinned by abstractfxresolver and keeps the Audacity surface unaffected.
…tes/MetaList/MetaSet/Type) Continuation of the prior PluginResourceId rename. The audioplugins module owns plugin-domain types; give them plugin names so the namespace tells you the role. Pure rename, no behavior change. audio/common/audiotypes.h still re-exports them under the audio::AudioResource* names so audio-engine code is unaffected; the next commit splits audios meta types off as audio-owned and drops the cross-module include for good. VST and audio-side qualified references (vstpluginmetareader, audio::sourceTypeFromResourceType, audio::audioResourceTypeToString, plus a fixture in audioresourcetypes_tests) updated to the new spelling in this same commit so the framework stays internally consistent at this point.
…e in vstmodulesrepository Audio no longer re-exports AudioResourceMeta/Vendor/Attributes/MetaList/ MetaSet from audioplugins. They are defined locally in audio/common/ audiotypes.h with the same field layout as audioplugins::PluginMeta; the C++ type system treats them as distinct now. Audio also drops the #include "audioplugins/audiopluginstypes.h" - it forward-declares just the three audioplugins id/type aliases that the existing bridge helpers (toAudioResourceId, sourceTypeFromResourceType, audioResourceTypeToString) need in their signatures. Audio gets local boolAttribute / intAttribute helpers. hasNativeEditor- Support and isOnlineAudioResource now use the local versions instead of reaching into audioplugins for them. vstmodulesrepository.cpp - the only internal site where audioplugins:: PluginMeta crosses into audio::AudioResourceMetaList - converts field- wise at that loop. isResourceType call replaced by a direct comparison against the wire string so the lambda no longer needs audio:: helpers on audioplugins data. vstpluginmetareader.cpp - leftover from the prior rename: the file is "using namespace muse::audioplugins;" and produces audioplugins-domain data (it implements IAudioPluginMetaReader), so bare AudioResourceMeta references resolve via that using-directive and needed renaming to PluginMeta to match the rest of the audioplugins module. VST is stubbed in our debug build so the prior commit missed this; ran build with the module conceptually enabled to confirm syntax. A static_assert in audioresourcetypes_tests.cpp now pins that audio::AudioResourceMeta and audioplugins::PluginMeta are distinct types - documenting the seam at compile time.
luapmartin
force-pushed
the
luapmartin/revamp-audio_plugins-module
branch
from
7218298 to
b64a302
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audacity PR: audacity/audacity#10989
Musescore PR: musescore/MuseScore#33535
Revamp of the
audiopluginsframework module — decouple, version, state lifecycle.AudioResourceId,AudioResourceMeta,AudioResourceType, …) out ofmuse::audiointo the dedicatedmuse::audiopluginsmodule.muse::audiokeepsusingaliases, so existing callers compile unchanged.AudioPluginInfo'senabledboolean with anAudioPluginStatelifecycle:Discovered,Validated,Missing,Error.known_audio_plugins.jsoncache — a{version, plugins}envelope plus a migration register. The framework auto-registers v0→v1 (structural) and v1→v2 (enabled→state); apps register any later steps. Legacy bare-array files still load.AudioResourceTypeis now an opaque wire string; each plugin module owns its canonical identifier (vst::AUDIO_RESOURCE_TYPE_NAME = "VstPlugin", …). The engine-side enum was narrowed to the formats the engine actually routes.PluginTypeenum).Discoveredplaceholders are persisted so an interrupted scan auto-resumes next launch; stopping a scan no longer loses already-validated plugins;registerNewPlugins(paths, validate)can record plugins without validating them.Note:
IRegisterAudioPluginsScenario::unregisterRemovedPlugins()has no callers in either the framework or audacity — the scan flow now marks removed pluginsMissingviasetPluginsState()instead of hard-deleting. I think that depending on what are musescore needs we could consider dropping that method? or maybe keep it as a cleaner in case the known_audio_plugins.json get's "too big"... to be called from plugin manager.Build configuration
audacity: luapmartin/audacity/luapmartin/revamp-audio_plugins-module
audacity platforms: linux_x64 macos windows_x64
musescore: luapmartin/MuseScore/luapmartin/revamp-audio_plugins-module
musescore platforms: linux_x64 linux_arm64 macos windows_x64 windows_portable