Revamp audio plugins module#47
Open
luapmartin wants to merge 11 commits into
Open
Conversation
…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).
vstpluginmetareader's signature and body referenced muse::audio:: types and constants. AudioResourceMeta/AudioResourceMetaList actually live in audioplugins (audio just re-exports them); switch to use audioplugins:: directly. Move HAS_NATIVE_EDITOR_SUPPORT_ATTRIBUTE alongside CATEGORIES_ATTRIBUTE in vst/vstpluginattrs.h so the file no longer needs muse::audio::. Audio's own makeReverbMeta keeps its constant in audiotypes.h (same string value).
Slims muse::audio::AudioResourceType to the formats the muse audio engine actually routes (FluidSoundfont, VstPlugin, NativeEffect, MuseSamplerSoundPack); the Audacity-only values (Lv2Plugin, AudioUnit, NyquistPlugin) move to Audacity's own EffectFamily enum, leaving the framework enum scoped to its own engine concerns. Each plugin-format module exports a wire-string constant (muse::vst::AUDIO_RESOURCE_TYPE_NAME, muse::musesampler::AUDIO_RESOURCE_TYPE_NAME, audio::FLUID_SOUNDFONT_TYPE_NAME, audio::NATIVE_EFFECT_TYPE_NAME) and producers use it instead of literal strings. New audio::isResourceType helper replaces scattered meta.type == "X" comparisons. A round-trip test (audioresourcetypes_tests) pins the wire strings to their canonical values so cache compatibility cannot drift.
Today's audacity launch tripped registerPlugins's m_loaded assertion because an old cache file with the obsolete 'enabled' field made load() return a migration error silently. Surface the failure where it actually originates: LOGE in KnownAudioPluginsRegister::load() at the JSON parse / unrecognized-root / migration-failure branches, with the cache path and the migration error text. Sharpen the messages produced by KnownAudioPluginsMigrationRegister::migrate() so the LOGE is actionable: future-version files tell the user the file is newer than the build expects, and missing migrators tell developers to register one in their AudioPluginsAppConfigModule. New tests pin the wording so a future regression that drops the actionable hint fails the build.
Wire the previously-reserved AudioPluginState::Discovered into the registration pipeline. registerNewPlugins writes a Discovered placeholder per path before spawning the validation subprocess; on subprocess return, removePluginsAtPath clears the placeholder before registerPlugin (Validated) or registerFailedPlugin (Error) writes a fresh entry. If the host crashes between placeholder-write and subprocess-return, the placeholder survives in the cache and scanPlugins() picks the path back up as 'new' on the next launch, so validation auto-resumes. removePluginsAtPath is the new register API and is mock-friendly. New scenario test covers the auto-resume path.
KnownAudioPluginsMigrationRegister's constructor now pre-registers the
framework-owned migrations so apps don't have to copy-paste them:
v0 -> v1: structural (envelope intro, no-op callback)
v1 -> v2: enabled boolean -> state string (AudioPluginState is a
framework enum, so this transformation belongs here)
Apps register only their own field migrations on top. Bumps
CURRENT_KNOWN_AUDIO_PLUGINS_VERSION 2 -> 3 since the MuseScore-specific
hasNativeEditorSupport migration moves to v2 -> v3 (app-owned).
processPluginsRegistration's per-iteration removePluginsAtPath ran in the main process and rewrote the JSON from the main's stale in-memory map, clobbering every Validated entry the subprocesses had accumulated — only the most recent survived. Move the placeholder removal into registerPlugin / registerFailedPlugin (subprocess side, fresh state); the main process now writes the register only once, via the existing load() at the end of registerNewPlugins. As a side benefit, registerPlugin becomes idempotent and a subprocess crash now leaves the Discovered placeholder intact (re-validated next launch) instead of pre-deleted.
registerNewPlugins() gains a `bool validate = true` parameter. When false, the paths are persisted as Discovered placeholders only and the out-of-process validation step is skipped. scanPlugins() already treats Discovered as re-validatable, so deferred-validation entries get re-offered on the next scan. Lets the app distinguish "user picked Skip on the validate prompt" from "user has never seen these paths" — Skip now records the plugins instead of discarding them.
Adds two tests covering the recent scan-flow changes, and updates the existing tests to reflect the new architecture (placeholder clearing moved from processPluginsRegistration into registerPlugin / registerFailedPlugin): - RegisterNewPlugins_ValidateFalsePersistsDiscoveredOnly: Skip records Discovered without spawning subprocesses. - RegisterNewPlugins_NoPerIterationClobber: main-process loop calls removePluginsAtPath once per path, never per iteration. - RegisterPlugin / RegisterFailedPlugin: assert subprocess-side clearing before registerPlugins. - UpdatePluginsRegistry_LeftoverDiscoveredRevalidates: expected removePluginsAtPath count drops 2->1.
7 tasks
📝 WalkthroughWalkthroughThis pull request refactors audio plugin metadata representation and state management across the muse framework. The core change replaces enum-based plugin types with string-based types in the audioplugins namespace, transitions from boolean 🚥 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
| DEFAULT_AUDIO_RESOURCE_ATTRIBUTES, | ||
| AudioResourceType::FluidSoundfont, | ||
| false /*hasNativeEditor*/ }; | ||
| "FluidSoundfont" }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use the canonical FLUID_SOUNDFONT_TYPE_NAME constant rather than a raw string literal.
fluidresolver.cpp now sets meta.type = FLUID_SOUNDFONT_TYPE_NAME and enginerpccontroller.cpp dispatches via AudioResourceType::FluidSoundfont. Hard-coding "FluidSoundfont" here duplicates the wire-string and will silently desync if the canonical constant is ever renamed. Prefer referencing the constant so all producers/consumers share one source of truth.
♻️ Proposed fix
-static const AudioResourceMeta DEFAULT_AUDIO_RESOURCE_META = {
- DEFAULT_SOUND_FONT_NAME,
- "Fluid",
- DEFAULT_AUDIO_RESOURCE_ATTRIBUTES,
- "FluidSoundfont" };
+static const AudioResourceMeta DEFAULT_AUDIO_RESOURCE_META = {
+ DEFAULT_SOUND_FONT_NAME,
+ "Fluid",
+ DEFAULT_AUDIO_RESOURCE_ATTRIBUTES,
+ synth::FLUID_SOUNDFONT_TYPE_NAME };Adjust the namespace qualifier to match wherever FLUID_SOUNDFONT_TYPE_NAME is declared (likely muse::audio::synth based on the existing include of soundfonttypes.h).
📝 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
| "FluidSoundfont" }; | |
| static const AudioResourceMeta DEFAULT_AUDIO_RESOURCE_META = { | |
| DEFAULT_SOUND_FONT_NAME, | |
| "Fluid", | |
| DEFAULT_AUDIO_RESOURCE_ATTRIBUTES, | |
| synth::FLUID_SOUNDFONT_TYPE_NAME }; |
🤖 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/engine/internal/audioengineconfiguration.cpp` at 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.
Comment on lines
+81
to
+85
| bool operator<(const AudioResourceMeta& other) const | ||
| { | ||
| return id < other.id | ||
| || vendor < other.vendor; | ||
| } |
There was a problem hiding this comment.
Make AudioResourceMeta::operator< a strict weak ordering.
This comparator can report both a < b and b < a as true when id and vendor disagree, and entries that differ only by type or attributes compare equal. That breaks std::set<AudioResourceMeta> and any ordered lookups built on this type.
Suggested fix
+#include <tuple>
+
bool operator<(const AudioResourceMeta& other) const
{
- return id < other.id
- || vendor < other.vendor;
+ return std::tie(id, vendor, type, attributes)
+ < std::tie(other.id, other.vendor, other.type, other.attributes);
}🤖 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/audiopluginstypes.h` around lines 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(...).
|
|
||
| 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().
Comment on lines
+131
to
+138
| } else if (json.isObject()) { | ||
| JsonObject root = json.rootObject(); | ||
| fileVersion = root.value("version").toInt(); | ||
| array = root.value("plugins").toArray(); | ||
| } else { | ||
| LOGE() << "Unrecognized known-audio-plugins.json root type at " | ||
| << knownAudioPluginsPath << " (expected array or object)"; | ||
| return Ret(static_cast<int>(Ret::Code::UnknownError), "Unrecognized known_audio_plugins.json root type"); |
There was a problem hiding this comment.
Validate the versioned cache envelope before loading.
The object branch accepts a missing/non-array plugins field as an empty cache and a missing version as 0. A truncated or malformed file will then load as “no plugins” and can be written back that way on the next save. Reject invalid envelopes instead of defaulting here.
🤖 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
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.
Comment on lines
+154
to
+156
| for (const auto& kv : configuration()->runtimeAttributeDefaults()) { | ||
| info.meta.attributes.emplace(kv.first, kv.second); | ||
| } |
There was a problem hiding this comment.
Runtime-only defaults should override cached values.
emplace() keeps the value from disk when a legacy cache still contains one of the runtime-only keys. That leaves stale runtime configuration in memory instead of applying the current defaults. Assign these keys so the runtime defaults always win.
🤖 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
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.
Comment on lines
+104
to
+105
| knownPluginsRegister()->setPluginsState(result.missingPluginIds, AudioPluginState::Missing); | ||
| knownPluginsRegister()->setPluginsState(result.rediscoveredPluginIds, AudioPluginState::Validated); |
There was a problem hiding this comment.
Check setPluginsState() results before continuing.
Both calls can write the cache, but their Ret is discarded. If either write fails, the scan continues into registration and can report success after only part of the state transition landed.
🤖 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 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.
Comment on lines
+49
to
+53
| // `validate=false` persists the paths as Discovered placeholders only; | ||
| // out-of-process validation is skipped and the entries will be re-offered | ||
| // for validation on the next scan. Default `true` runs the full scan. | ||
| virtual void registerNewPlugins(const io::paths_t& pluginPaths, bool validate = true) = 0; | ||
| virtual Ret unregisterRemovedPlugins(const AudioResourceIdList& pluginIds) = 0; |
There was a problem hiding this comment.
Expose registration failures in the interface.
registerNewPlugins() now persists placeholders and reloads the registry, but it still returns void. The implementation has multiple fallible Ret-producing steps, so callers cannot detect cache write/load failures and will treat a partial scan as success. Return Ret here and short-circuit on the first failure.
🤖 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/iregisteraudiopluginsscenario.h` around lines 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.
Comment on lines
+123
to
133
| muse::audio::AudioResourceMetaList VstModulesRepository::modulesMetaList(PluginType type) const | ||
| { | ||
| auto infoAccepted = [type](const audioplugins::AudioPluginInfo& info) { | ||
| return info.type == type && info.meta.type == muse::audio::AudioResourceType::VstPlugin && info.enabled; | ||
| if (!muse::audio::isResourceType(info.meta, muse::audio::AudioResourceType::VstPlugin) | ||
| || info.state != audioplugins::AudioPluginState::Validated) { | ||
| return false; | ||
| } | ||
| const String& categories = info.meta.attributeVal(muse::vst::CATEGORIES_ATTRIBUTE); | ||
| const bool isInstrument = categories.contains(u"Instrument"); | ||
| return type == PluginType::Instrument ? isInstrument : !isInstrument; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Extract the "Instrument" category literal into a named constant.
u"Instrument" is now the sole discriminator separating instrument vs. FX VSTs (replacing the previous info.type enum), so it is effectively a wire-level contract with whatever writes CATEGORIES_ATTRIBUTE in the plugin meta reader. Hard-coding the string inline here makes that contract invisible and easy to break if either side is changed. Lift it next to CATEGORIES_ATTRIBUTE (e.g. in vstpluginattrs.h or PluginCategory) so both producer and consumer reference the same symbol.
♻️ Proposed fix (sketch)
In vstpluginattrs.h (or vsttypes.h alongside PluginCategory):
inline constexpr std::u16string_view INSTRUMENT_CATEGORY = u"Instrument";Then in modulesMetaList:
- const String& categories = info.meta.attributeVal(muse::vst::CATEGORIES_ATTRIBUTE);
- const bool isInstrument = categories.contains(u"Instrument");
+ const String& categories = info.meta.attributeVal(muse::vst::CATEGORIES_ATTRIBUTE);
+ const bool isInstrument = categories.contains(muse::vst::INSTRUMENT_CATEGORY);
return type == PluginType::Instrument ? isInstrument : !isInstrument;🧰 Tools
🪛 Clang (14.0.6)
[warning] 123-123: use a trailing return type for this function
(modernize-use-trailing-return-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/vst/internal/vstmodulesrepository.cpp` around lines 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)).
Comment on lines
+25
to
27
| #include "audio/common/audiotypes.h" | ||
|
|
||
| #include "vsttypes.h" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm where AUDIO_RESOURCE_TYPE_NAME and CATEGORIES_ATTRIBUTE are declared,
# and whether vstpluginattrs.h is transitively included via vsttypes.h.
fd vstpluginattrs.h
rg -nP '\b(AUDIO_RESOURCE_TYPE_NAME|CATEGORIES_ATTRIBUTE)\b' framework/vst
rg -nP '`#include`\s+"vstpluginattrs\.h"' framework/vst
sed -n '1,80p' framework/vst/vsttypes.h 2>/dev/nullRepository: musescore/muse_framework
Length of output: 4236
🏁 Script executed:
#!/bin/bash
# Inspect the exact include list and usage sites in the reviewed TU.
sed -n '1,120p' framework/vst/internal/vstpluginmetareader.cpp
rg -nP '\b(AUDIO_RESOURCE_TYPE_NAME|CATEGORIES_ATTRIBUTE)\b' framework/vst/internal/vstpluginmetareader.cppRepository: musescore/muse_framework
Length of output: 2592
Include vstpluginattrs.h directly in vstpluginmetareader.cpp.
framework/vst/internal/vstpluginmetareader.cpp uses AUDIO_RESOURCE_TYPE_NAME and CATEGORIES_ATTRIBUTE but only includes vsttypes.h (which currently includes vstpluginattrs.h transitively). Add #include "vstpluginattrs.h" explicitly to follow IWYU and prevent build breaks if that transitive include changes.
♻️ Proposed fix
`#include` "audio/common/audiotypes.h"
`#include` "vsttypes.h"
`#include` "vsterrors.h"
+#include "vstpluginattrs.h"🤖 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 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).
Comment on lines
+30
to
+33
| inline constexpr std::string_view AUDIO_RESOURCE_TYPE_NAME = "VstPlugin"; | ||
|
|
||
| static const String CATEGORIES_ATTRIBUTE(u"categories"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use inline const for the CATEGORIES_ATTRIBUTE header constant.
static const String CATEGORIES_ATTRIBUTE declared in a header gives each translation unit that includes this file its own internal-linkage copy of a non-trivial String object (with separate construction and destruction). This is wasteful, contradicts the inline constexpr style used right above for AUDIO_RESOURCE_TYPE_NAME, and risks subtle issues if the address is ever compared across TUs. Use inline const (C++17) so there is a single shared definition.
♻️ Proposed fix
namespace muse::vst {
inline constexpr std::string_view AUDIO_RESOURCE_TYPE_NAME = "VstPlugin";
-static const String CATEGORIES_ATTRIBUTE(u"categories");
+inline const String CATEGORIES_ATTRIBUTE(u"categories");
}🤖 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/vstpluginattrs.h` around lines 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.
8 tasks
Contributor
Author
|
/build |
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
1 participant
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