Fix audio plugins scanner#67
Conversation
|
Warning Review limit reached
More reviews will be available in 43 minutes and 57 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors the audio plugin registration and scanning system. The primary change is a return-type swap between two methods: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/audioplugins/internal/knownaudiopluginsregister.cpp (1)
245-258:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winShared path can be dropped while other plugin IDs still reference it.
For a multi-component VST, several IDs share one
.vst3path.muse::remove(m_pluginPaths, pair.second.path)unconditionally erases that path when the first ID is unregistered, so a subsequently still-registered sibling ID would reportexists(path) == false. The currentupdatePluginsRegistry()flow unregisters all IDs of a missing path together, so this is latent today, but the public method is unsafe for partial unregistration. Also, the inner loop scans the whole map thoughresourceIdis the unique key.🐛 Look up by key and only drop the path when no sibling references it
for (const AudioResourceId& resourceId : resourceIds) { if (!exists(resourceId)) { continue; } - for (const auto& pair : m_pluginInfoMap) { - if (pair.first == resourceId) { - muse::remove(m_pluginPaths, pair.second.path); - } - } - - m_pluginInfoMap.erase(resourceId); + auto it = m_pluginInfoMap.find(resourceId); + const io::path_t path = it->second.path; + m_pluginInfoMap.erase(it); + + const bool pathStillUsed = std::any_of(m_pluginInfoMap.cbegin(), m_pluginInfoMap.cend(), + [&path](const auto& p) { return p.second.path == path; }); + if (!pathStillUsed) { + muse::remove(m_pluginPaths, path); + } changed = true; }🤖 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 245 - 258, When erasing a plugin by resourceId, don't scan the whole m_pluginInfoMap or unconditionally remove its path; instead locate the entry via m_pluginInfoMap.find(resourceId), then before calling muse::remove(m_pluginPaths, pair.second.path) check whether any other entries in m_pluginInfoMap (excluding resourceId) reference the same pair.second.path and only remove the path if no sibling references remain; update the removal logic in the block that uses exists(resourceId), m_pluginInfoMap, m_pluginPaths and muse::remove so you do a key lookup, conditional path removal based on remaining references, then erase(resourceId) and set changed = true.
🤖 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 60-68: The loop currently pushes path onto result.newPluginPaths
whenever knownPluginsRegister()->exists(path) is false, which allows duplicates;
change the logic to first attempt to insert each scanned path into scannedPaths
and only if that insertion reports it was newly inserted (i.e., first time seen)
and the path is not already known, then append to result.newPluginPaths; update
the loop around scannerRegister()->scanners() / scanner->scanPlugins(progress)
to use the insert result (or an explicit contains check on scannedPaths) before
pushing to result.newPluginPaths so each unknown path is added exactly once and
processPluginsRegistration() won't be invoked multiple times for the same path.
In `@framework/audioplugins/tests/registeraudiopluginsscenariotest.cpp`:
- Around line 337-338: Replace the strict ON_CALL matching of scanPlugins() with
a wildcard matcher to accept the default nullptr or any Progress*; specifically
update the mock stub for IAudioPluginsScanner::scanPlugins so it uses the
underscore matcher (_) instead of the no-argument form, ensuring
RegisterAudioPluginsScenario::updatePluginsRegistry (which calls
scanner->scanPlugins(progress)) will match both nullptr and non-null Progress*
values.
---
Outside diff comments:
In `@framework/audioplugins/internal/knownaudiopluginsregister.cpp`:
- Around line 245-258: When erasing a plugin by resourceId, don't scan the whole
m_pluginInfoMap or unconditionally remove its path; instead locate the entry via
m_pluginInfoMap.find(resourceId), then before calling
muse::remove(m_pluginPaths, pair.second.path) check whether any other entries in
m_pluginInfoMap (excluding resourceId) reference the same pair.second.path and
only remove the path if no sibling references remain; update the removal logic
in the block that uses exists(resourceId), m_pluginInfoMap, m_pluginPaths and
muse::remove so you do a key lookup, conditional path removal based on remaining
references, then erase(resourceId) and set changed = true.
🪄 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: 5e04595f-0873-447d-ad8c-b044780a74aa
📒 Files selected for processing (7)
framework/audioplugins/internal/knownaudiopluginsregister.cppframework/audioplugins/internal/registeraudiopluginsscenario.cppframework/audioplugins/internal/registeraudiopluginsscenario.hframework/audioplugins/iregisteraudiopluginsscenario.hframework/audioplugins/tests/registeraudiopluginsscenariotest.cppframework/vst/internal/vstpluginsscanner.cppframework/vst/internal/vstpluginsscanner.h
| for (const IAudioPluginsScannerPtr& scanner : scannerRegister()->scanners()) { | ||
| for (const io::path_t& path : scanner->scanPlugins(progress)) { | ||
| if (!knownPluginsRegister()->exists(path)) { | ||
| result.newPluginPaths.push_back(path); | ||
| } | ||
|
|
||
| scannedPaths.insert(path); | ||
| } | ||
| } |
There was a problem hiding this comment.
newPluginPaths can still contain duplicates, defeating the PR's dedup goal.
scannedPaths deduplicates, but the decision to append to result.newPluginPaths is gated only on knownPluginsRegister()->exists(path). If the same not-yet-known path is returned by more than one scanner (or twice by one), it is pushed multiple times, so processPluginsRegistration() spawns a subprocess for it more than once. This contradicts the stated objective of ensuring scanPlugins() does not return duplicate paths.
🐛 Gate on first insertion into the dedup set
for (const IAudioPluginsScannerPtr& scanner : scannerRegister()->scanners()) {
for (const io::path_t& path : scanner->scanPlugins(progress)) {
- if (!knownPluginsRegister()->exists(path)) {
- result.newPluginPaths.push_back(path);
- }
-
- scannedPaths.insert(path);
+ if (!scannedPaths.insert(path).second) {
+ continue;
+ }
+
+ if (!knownPluginsRegister()->exists(path)) {
+ result.newPluginPaths.push_back(path);
+ }
}
}🤖 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 60 - 68, The loop currently pushes path onto result.newPluginPaths
whenever knownPluginsRegister()->exists(path) is false, which allows duplicates;
change the logic to first attempt to insert each scanned path into scannedPaths
and only if that insertion reports it was newly inserted (i.e., first time seen)
and the path is not already known, then append to result.newPluginPaths; update
the loop around scannerRegister()->scanners() / scanner->scanPlugins(progress)
to use the insert result (or an explicit contains check on scannedPaths) before
pushing to result.newPluginPaths so each unknown path is added exactly once and
processPluginsRegistration() won't be invoked multiple times for the same path.
RomanPudashkin
force-pushed
the
fix_audio_plugins_scanner
branch
from
b57150f to
c6d7806
Compare
1 participant
Ports: musescore/MuseScore#33608
Resolves: musescore/MuseScore#33454