Made the audio subsystem global.#33000
Merged
igorkorsukov merged 1 commit intomusescore:masterfrom Apr 14, 2026
Merged
Conversation
📝 WalkthroughWalkthroughThe PR migrates many audio-framework components from context-scoped to global-scoped wiring: it removes numerous 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/framework/audio/main/internal/startaudiocontroller.cpp (1)
229-240:⚠️ Potential issue | 🔴 CriticalMissing null check for
m_engineControllerinWorkerRpcModeandDriverModebranches.The driver callback can be invoked immediately after
audioDriverController()->open()(line 258), potentially before the worker thread has executedth_setupEngine()which createsm_engineController. Unlike theWorkerModebranch (lines 181-183), these branches lack a null guard, risking a null pointer dereference.🐛 Proposed fix to add null check
// process if (workmode::mode() == workmode::WorkerRpcMode) { + if (!m_engineController) { + return; + } m_engineController->process(procDest, (unsigned)procSamplesPerChannel); } else if (workmode::mode() == workmode::DriverMode) { static bool once = false; if (!once) { th_setupEngine(); once = true; } + if (!m_engineController) { + return; + } m_rpcChannel->process(); m_engineController->process(procDest, (unsigned)procSamplesPerChannel); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/main/internal/startaudiocontroller.cpp` around lines 229 - 240, The WorkerRpcMode and DriverMode branches call m_engineController->process(...) without ensuring m_engineController is non-null, risking a null dereference; add a null check before calling process in both branches (e.g., if (m_engineController) { m_engineController->process(...); } else { /* handle/log or return */ }), and in DriverMode ensure th_setupEngine() is still called once and m_rpcChannel->process() remains executed; reference m_engineController, th_setupEngine, m_rpcChannel, workmode::WorkerRpcMode and workmode::DriverMode when applying the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/audio/engine/internal/tracksequence.cpp`:
- Around line 41-48: The TrackSequence constructor currently default-initializes
m_clock, m_player and m_audioIO then reassigns them in the constructor body;
change TrackSequence::TrackSequence(const TrackSequenceId id) to initialize
m_clock (std::make_shared<Clock>()), m_player
(std::make_shared<SequencePlayer>(this, m_clock)) and m_audioIO
(std::make_shared<SequenceIO>(this)) directly in the member initializer list
alongside m_id, leaving ONLY_AUDIO_ENGINE_THREAD in the body and removing the
redundant reassignments to avoid extra work and tighten construction semantics.
In `@src/framework/audio/main/audiomodule.cpp`:
- Around line 133-140: Reorder teardown in AudioModule::onDeinit so background
loops are stopped and allowed to fully exit before any playback or engine
deinit: call m_rpcTicker.stop() and then stop the audio processing loop via
m_startAudioController->stopAudioProcessing() (and block/wait for those to
complete if they are async) before calling m_mainPlayback->deinit() and
m_engineGlobalSetup->onDeinit(); ensure the stop calls reliably join/finish
their threads/dispatch loops to avoid races with m_mainPlayback and engine
deinitialization.
In `@src/framework/audio/main/CMakeLists.txt`:
- Around line 57-61: The SoundFont drop handler in
applicationactioncontroller.cpp calls the ISoundFontInstallScenario service
which is not registered for WASM (registration is inside audiomodule.cpp under
`#ifndef` Q_OS_WASM), causing a null dereference in web builds; either add a
WASM-safe stub implementation of ISoundFontInstallScenario and register it in
audiomodule.cpp for Q_OS_WASM, or guard the drop-handler code in
applicationactioncontroller.cpp (the code around the SoundFont drag-drop
handling at the referenced location) with `#ifndef` Q_OS_WASM (or a runtime check)
so the drop target is not recognized on WASM; update whichever file you change
(audiomodule.cpp to register a stub class, or applicationactioncontroller.cpp to
skip using ISoundFontInstallScenario on WASM) and ensure no null service pointer
is ever dereferenced.
In `@src/framework/audio/main/isoundfontinstallscenario.h`:
- Line 30: The interface ISoundFontInstallScenario is still declared with
MODULE_CONTEXT_INTERFACE which causes a context-scoped registration and breaks
the global-audio migration; change the declaration to use the global interface
macro (replace MODULE_CONTEXT_INTERFACE with MODULE_GLOBAL_INTERFACE) for
ISoundFontInstallScenario and update any registrations/consumers that assume a
global service resolution to use the global registration APIs so the service
resolves via the global-audio path.
In
`@src/framework/audio/main/platform/general/generalsoundfontinstallscenario.cpp`:
- Around line 104-110: In doAddSoundFont, avoid copying when src and dst are
identical: check if src == dst before calling fileSystem()->copy and
short-circuit to create synth::SoundFontUri::fromLocalFile(dst) and call
soundFontController()->addSoundFont(uri) without performing the copy; otherwise
proceed with the existing fileSystem()->copy(...) flow and handle Ret as before.
Ensure the same return semantics (Ret) are preserved so the function still
returns success when registration of an already-resident SoundFont completes.
- Around line 46-49: The code currently logs and returns on path resolution or
copy failures (around resolveInstallationPath(path) and the subsequent copy
branch around lines 73-85) but doesn't surface errors to the user; update these
error branches to call interactive()->error(...) (or interactive()->warning(...)
as appropriate) with a clear user-facing message that includes the failure
details (e.g., newPath.ret.toString() for resolveInstallationPath and the copy
operation's error string) before returning, so the UI shows the failure instead
of appearing to ignore the drag-and-drop install.
In `@src/playback/internal/playbackcontroller.h`:
- Line 53: The issue is lingering sequence state in the process-global playback
object; ensure we call resetCurrentSequence() on the global playback
(muse::audio::IPlayback injected as playback) whenever the controller stops or
changes notation and clear m_currentSequenceId on destruction: modify reset() to
call playback->resetCurrentSequence() (in addition to stop()), update
setMasterNotation() to call playback->resetCurrentSequence() before switching
the master notation, and implement the destructor to clear m_currentSequenceId
and call playback->resetCurrentSequence() so the global playback doesn't retain
an orphaned sequence; keep currentProjectChanged logic as-is.
---
Outside diff comments:
In `@src/framework/audio/main/internal/startaudiocontroller.cpp`:
- Around line 229-240: The WorkerRpcMode and DriverMode branches call
m_engineController->process(...) without ensuring m_engineController is
non-null, risking a null dereference; add a null check before calling process in
both branches (e.g., if (m_engineController) { m_engineController->process(...);
} else { /* handle/log or return */ }), and in DriverMode ensure
th_setupEngine() is still called once and m_rpcChannel->process() remains
executed; reference m_engineController, th_setupEngine, m_rpcChannel,
workmode::WorkerRpcMode and workmode::DriverMode when applying the guard.
🪄 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: 529c91ff-ac67-4e69-aca3-bcd8539e5e1d
📒 Files selected for processing (75)
src/app/internal/consoleapp.cppsrc/appshell/internal/applicationactioncontroller.cppsrc/appshell/internal/applicationactioncontroller.hsrc/framework/audio/common/rpc/irpcchannel.hsrc/framework/audio/common/rpc/platform/general/generalrpcchannel.cppsrc/framework/audio/engine/enginesetup.cppsrc/framework/audio/engine/enginesetup.hsrc/framework/audio/engine/iaudioengine.hsrc/framework/audio/engine/iengineplayback.hsrc/framework/audio/engine/internal/enginecontroller.cppsrc/framework/audio/engine/internal/enginecontroller.hsrc/framework/audio/engine/internal/engineplayback.cppsrc/framework/audio/engine/internal/engineplayback.hsrc/framework/audio/engine/internal/enginerpccontroller.hsrc/framework/audio/engine/internal/eventaudiosource.cppsrc/framework/audio/engine/internal/eventaudiosource.hsrc/framework/audio/engine/internal/export/soundtrackwriter.cppsrc/framework/audio/engine/internal/export/soundtrackwriter.hsrc/framework/audio/engine/internal/sequenceplayer.cppsrc/framework/audio/engine/internal/sequenceplayer.hsrc/framework/audio/engine/internal/synthesizers/abstractsynthesizer.cppsrc/framework/audio/engine/internal/synthesizers/abstractsynthesizer.hsrc/framework/audio/engine/internal/synthesizers/fluidsynth/fluidresolver.cppsrc/framework/audio/engine/internal/synthesizers/fluidsynth/fluidresolver.hsrc/framework/audio/engine/internal/synthesizers/fluidsynth/fluidsynth.cppsrc/framework/audio/engine/internal/synthesizers/fluidsynth/fluidsynth.hsrc/framework/audio/engine/internal/synthesizers/synthresolver.cppsrc/framework/audio/engine/internal/synthesizers/synthresolver.hsrc/framework/audio/engine/internal/tracksequence.cppsrc/framework/audio/engine/internal/tracksequence.hsrc/framework/audio/engine/isynthresolver.hsrc/framework/audio/iaudiodrivercontroller.hsrc/framework/audio/main/CMakeLists.txtsrc/framework/audio/main/audiomodule.cppsrc/framework/audio/main/audiomodule.hsrc/framework/audio/main/internal/audioconfiguration.hsrc/framework/audio/main/internal/audiodrivercontroller.hsrc/framework/audio/main/internal/playback.cppsrc/framework/audio/main/internal/playback.hsrc/framework/audio/main/internal/player.cppsrc/framework/audio/main/internal/player.hsrc/framework/audio/main/internal/startaudiocontroller.cppsrc/framework/audio/main/internal/startaudiocontroller.hsrc/framework/audio/main/iplayback.hsrc/framework/audio/main/isoundfontcontroller.hsrc/framework/audio/main/isoundfontinstallscenario.hsrc/framework/audio/main/istartaudiocontroller.hsrc/framework/audio/main/platform/general/generalsoundfontcontroller.cppsrc/framework/audio/main/platform/general/generalsoundfontcontroller.hsrc/framework/audio/main/platform/general/generalsoundfontinstallscenario.cppsrc/framework/audio/main/platform/general/generalsoundfontinstallscenario.hsrc/framework/musesampler/internal/musesamplerresolver.cppsrc/framework/musesampler/internal/musesamplerresolver.hsrc/framework/musesampler/internal/musesamplerwrapper.cppsrc/framework/musesampler/internal/musesamplerwrapper.hsrc/framework/stubs/audio/synthresolverstub.cppsrc/framework/stubs/audio/synthresolverstub.hsrc/framework/vst/internal/synth/vstiresolver.cppsrc/framework/vst/internal/synth/vstiresolver.hsrc/framework/vst/internal/synth/vstsynthesiser.hsrc/importexport/audioexport/internal/abstractaudiowriter.cppsrc/importexport/audioexport/internal/abstractaudiowriter.hsrc/playback/internal/onlinesoundscontroller.hsrc/playback/internal/playbackcontroller.hsrc/playback/internal/soundprofilesrepository.hsrc/playback/isoundprofilesrepository.hsrc/playback/playbackmodule.cppsrc/playback/playbackmodule.hsrc/playback/qml/MuseScore/Playback/inputresourceitem.hsrc/playback/qml/MuseScore/Playback/mixerpanelmodel.hsrc/playback/qml/MuseScore/Playback/notationregionsbeingprocessedmodel.hsrc/playback/qml/MuseScore/Playback/outputresourceitem.hsrc/playback/qml/MuseScore/Playback/soundprofilesmodel.hsrc/preferences/qml/MuseScore/Preferences/audiomidipreferencesmodel.hsrc/preferences/qml/MuseScore/Preferences/commonaudioapiconfigurationmodel.h
💤 Files with no reviewable changes (1)
- src/importexport/audioexport/internal/abstractaudiowriter.cpp
Comment on lines
+46
to
+49
| RetVal<io::path_t> newPath = resolveInstallationPath(path); | ||
| if (!newPath.ret) { | ||
| LOGE() << "failed resolve path, err: " << newPath.ret.toString(); | ||
| return; |
There was a problem hiding this comment.
Surface install failures in the UI.
When path resolution or the copy fails, this flow only logs and returns. In the new drag-and-drop install path that looks like the action was ignored. Show an interactive()->error/warning in these branches.
Also applies to: 73-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/framework/audio/main/platform/general/generalsoundfontinstallscenario.cpp`
around lines 46 - 49, The code currently logs and returns on path resolution or
copy failures (around resolveInstallationPath(path) and the subsequent copy
branch around lines 73-85) but doesn't surface errors to the user; update these
error branches to call interactive()->error(...) (or interactive()->warning(...)
as appropriate) with a clear user-facing message that includes the failure
details (e.g., newPath.ret.toString() for resolveInstallationPath and the copy
operation's error string) before returning, so the UI shows the failure instead
of appearing to ignore the drag-and-drop install.
Comment on lines
+104
to
+110
| Ret GeneralSoundFontInstallScenario::doAddSoundFont(const io::path_t& src, const io::path_t& dst) | ||
| { | ||
| Ret ret = fileSystem()->copy(src, dst, true /* replace */); | ||
|
|
||
| if (ret) { | ||
| synth::SoundFontUri uri = synth::SoundFontUri::fromLocalFile(dst); | ||
| soundFontController()->addSoundFont(uri); |
There was a problem hiding this comment.
Skip the copy when source and destination are the same file.
If the dropped SoundFont already lives in the first writable user SoundFont directory, resolveInstallationPath() returns the same path and Line 106 copies the file onto itself. That usually fails, so re-adding an existing SoundFont breaks after the overwrite prompt. Short-circuit src == dst and just register/use the existing file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/framework/audio/main/platform/general/generalsoundfontinstallscenario.cpp`
around lines 104 - 110, In doAddSoundFont, avoid copying when src and dst are
identical: check if src == dst before calling fileSystem()->copy and
short-circuit to create synth::SoundFontUri::fromLocalFile(dst) and call
soundFontController()->addSoundFont(uri) without performing the copy; otherwise
proceed with the existing fileSystem()->copy(...) flow and handle Ret as before.
Ensure the same return semantics (Ret) are preserved so the function still
returns success when registration of an already-resident SoundFont completes.
| { | ||
| muse::GlobalInject<IPlaybackConfiguration> configuration; | ||
| muse::GlobalInject<notation::INotationConfiguration> notationConfiguration; | ||
| muse::GlobalInject<muse::audio::IPlayback> playback; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify PlaybackController explicitly tears down its global playback state.
fd -i 'playbackcontroller.cpp' src | while read -r f; do
echo "== $f =="
rg -n -C3 'PlaybackController::~PlaybackController|resetCurrentSequence|removeSequence\(|addSequence\(|setupNewCurrentSequence|setNotation\(|setMasterNotation\(|reset\(' "$f"
doneRepository: musescore/MuseScore
Length of output: 2631
🏁 Script executed:
# Get full setMasterNotation() implementation
sed -n '1789,1850p' src/playback/internal/playbackcontroller.cppRepository: musescore/MuseScore
Length of output: 1642
🏁 Script executed:
# Search for reset() call sites
rg -n 'playbackController.*reset\(\)|->reset\(' src --type=cpp --type=h | head -20Repository: musescore/MuseScore
Length of output: 2229
🏁 Script executed:
# Check if destructor needs to call resetCurrentSequence
git -C . log --oneline -1 src/playback/internal/playbackcontroller.h | catRepository: musescore/MuseScore
Length of output: 106
Add sequence cleanup to reset() and setMasterNotation() paths.
Line 53 makes IPlayback process-wide. The implementation shows three cleanup gaps:
reset()(line 232) only callsstop()but never callsresetCurrentSequence(). WhenprojectActionsController.cpp:710callsreset()on project close, the current sequence remains in the global playback object.setMasterNotation()(line 1789) changes the active notation but doesn't callresetCurrentSequence(). A notation switch without a concurrent project change would leave the old sequence active.- The destructor is defaulted, so destroying the controller doesn't clean up
m_currentSequenceIdfrom the global playback object.
The currentProjectChanged handler correctly calls resetCurrentSequence() before adding a new sequence, but this doesn't cover standalone reset() calls or notation changes that occur outside of a project change event. Either reset() should call resetCurrentSequence(), setMasterNotation() should reset before switching, or the teardown should be consolidated at the destructor.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/playback/internal/playbackcontroller.h` at line 53, The issue is
lingering sequence state in the process-global playback object; ensure we call
resetCurrentSequence() on the global playback (muse::audio::IPlayback injected
as playback) whenever the controller stops or changes notation and clear
m_currentSequenceId on destruction: modify reset() to call
playback->resetCurrentSequence() (in addition to stop()), update
setMasterNotation() to call playback->resetCurrentSequence() before switching
the master notation, and implement the destructor to clear m_currentSequenceId
and call playback->resetCurrentSequence() so the global playback doesn't retain
an orphaned sequence; keep currentProjectChanged logic as-is.
Contributor
Author
There was a problem hiding this comment.
Thanks for pointing this out, fixing this is a task for the future.
There was a problem hiding this comment.
@igorkorsukov, understood — noted as a deferred cleanup task. Would you like me to open a GitHub issue to track the sequence teardown gaps (reset(), setMasterNotation(), and the destructor not cleaning up m_currentSequenceId from the global IPlayback object)?
( ´•ω•) 🐇
🧠 Learnings used
Learnt from: mathesoncalum
Repo: musescore/MuseScore PR: 32779
File: src/engraving/dom/masterscore.cpp:383-383
Timestamp: 2026-04-06T11:48:24.729Z
Learning: In `src/engraving/dom/masterscore.cpp`, `MasterScore::initAutomation()` calls `m_automationController->init(this)` which walks the score and inserts points without clearing prior state. This non-idempotency is a known deferred issue — it is intentionally not fixed in PR `#32779` because `initAutomation()` is a convenience method for in-development automation work. When the final API is determined in a future PR, the call site or `AutomationController::init()` should be made idempotent (e.g., by clearing internal state before re-populating, or by guarding against repeated calls).
Learnt from: mathesoncalum
Repo: musescore/MuseScore PR: 32779
File: src/notationscene/qml/MuseScore/NotationScene/abstractnotationpaintview.cpp:343-350
Timestamp: 2026-04-06T11:25:42.550Z
Learning: In `src/notationscene/qml/MuseScore/NotationScene/abstractnotationpaintview.cpp`, the call sites for `notationAutomation()` (e.g. in `onLoadNotation` and `onUnloadNotation`) intentionally do not null-check the return value — this matches the pre-existing behavior of the inlined `m_notation->masterNotation()->automation()` chain. Do not flag missing null checks on `notationAutomation()` call sites as new issues introduced by the refactoring.
Learnt from: mathesoncalum
Repo: musescore/MuseScore PR: 32779
File: src/notationscene/qml/MuseScore/NotationScene/abstractnotationpaintview.cpp:399-400
Timestamp: 2026-04-06T11:27:52.780Z
Learning: In `src/notationscene/qml/MuseScore/NotationScene/abstractnotationpaintview.cpp`, the `notationAutomation()` disconnect calls in `onUnloadNotation` (for `automationModeEnabledChanged` and `automationLinesDataChanged`) intentionally omit a null check on the return value, matching the pre-existing pattern of the inlined `m_notation->masterNotation()->automation()` chain. Do not flag missing null checks on these disconnect call sites.
Learnt from: mathesoncalum
Repo: musescore/MuseScore PR: 32779
File: src/engraving/dom/masterscore.h:173-173
Timestamp: 2026-04-06T11:15:07.564Z
Learning: In `src/engraving/dom/masterscore.h`, the `initAutomation()` method on class `MasterScore` is intentionally marked with a `// TODO: Placeholder?` comment. It is a convenience method used during in-development automation work, and whether it will be part of the final API is yet to be determined. Do not flag this TODO as stale or suggest its removal.
Learnt from: miiizen
Repo: musescore/MuseScore PR: 32947
File: src/engraving/editing/textedit.cpp:205-211
Timestamp: 2026-04-09T15:46:50.059Z
Learning: In `src/engraving/editing/textedit.cpp`, in `TextBase::endEdit()`, the `else` (non-newlyAdded) branch of the empty-text cleanup (`score()->undoRemoveElement(this)` path) has broken/incomplete undo support for the text itself. Therefore, the partial-dash removal in that branch intentionally uses `score()->removeElement(partialDash)` (non-undoable) rather than `score()->undoRemoveElement(partialDash)`, because making only the dash removal undoable in isolation would create an inconsistent undo state. Fixing text undo in this path is a known deferred issue, not in scope for PR `#32947`.
Learnt from: mathesoncalum
Repo: musescore/MuseScore PR: 32779
File: src/notation/internal/notationautomation.cpp:117-117
Timestamp: 2026-04-06T11:19:20.384Z
Learning: In `src/notation/internal/notationautomation.cpp`, the `xFactor` variable in `linesDataForSystem` (used to compute the x-position of automation points) is an acknowledged temporary placeholder. It assumes linear time within a staff, which is incorrect. It will be replaced with a proper segment/timetick-based calculation in a future automation PR. Do not flag the division-by-zero risk or the linear-time assumption as blocking issues — they are known and intentionally deferred.
Learnt from: igorkorsukov
Repo: musescore/MuseScore PR: 33000
File: src/framework/audio/main/isoundfontinstallscenario.h:30-30
Timestamp: 2026-04-13T15:29:39.364Z
Learning: In MuseScore's audio subsystem (`src/framework/audio/main/isoundfontinstallscenario.h`), `ISoundFontInstallScenario` intentionally uses `MODULE_CONTEXT_INTERFACE` (not `MODULE_GLOBAL_INTERFACE`), even though most audio services were migrated to global scope in the audio-globalization refactor. The install scenario must remain context-scoped because its implementation (`GeneralSoundFontInstallScenario`) displays interactive dialogs that require a window context. Do not flag this as a scope mismatch in future reviews.
igorkorsukov
force-pushed
the
w/ioc/step_42
branch
from
4bab9e3 to
c3d3ce7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/framework/audio/main/internal/playback.cpp (1)
105-109:⚠️ Potential issue | 🟡 MinorDuplicate unsubscription and missing
OutputParamsChangedcleanup.In
deinit(),Method::InputParamsChangedis unsubscribed twice (lines 107 and 108), whileMethod::OutputParamsChangedis never unsubscribed. Comparing withinit():
init()subscribes:TrackAdded,TrackRemoved,InputParamsChanged,OutputParamsChanged,MasterOutputParamsChangeddeinit()unsubscribes:TrackAdded,TrackRemoved,InputParamsChanged,InputParamsChanged(duplicate),MasterOutputParamsChangedThis leaves
OutputParamsChangedstill subscribed afterdeinit(), which could cause issues if the channel receives messages after cleanup.🐛 Proposed fix
channel()->onMethod(Method::TrackAdded, nullptr); channel()->onMethod(Method::TrackRemoved, nullptr); channel()->onMethod(Method::InputParamsChanged, nullptr); - channel()->onMethod(Method::InputParamsChanged, nullptr); + channel()->onMethod(Method::OutputParamsChanged, nullptr); channel()->onMethod(Method::MasterOutputParamsChanged, nullptr);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/main/internal/playback.cpp` around lines 105 - 109, In deinit(), replace the duplicated unsubscription of Method::InputParamsChanged with an unsubscription for Method::OutputParamsChanged so that deinit() mirrors init(); specifically update the calls to channel()->onMethod(...) in deinit() to remove the duplicate Method::InputParamsChanged and call channel()->onMethod(Method::OutputParamsChanged, nullptr) instead, ensuring all subscriptions made in init() (TrackAdded, TrackRemoved, InputParamsChanged, OutputParamsChanged, MasterOutputParamsChanged) are unsubscribed.
♻️ Duplicate comments (6)
src/playback/internal/playbackcontroller.h (1)
53-54:⚠️ Potential issue | 🟠 MajorAudit teardown paths now that playback state is process-global.
Line 53 widens
IPlaybackto process scope, so any missed cleanup inreset(),setMasterNotation(), or~PlaybackController()can leavem_currentSequenceIdand the old sequence registered in the global playback service after notation/project switches.#!/bin/bash # Verify PlaybackController tears down global playback sequence state on reset, # notation switches, and destruction. fd -i 'playbackcontroller.cpp' src | while read -r f; do echo "== $f ==" rg -n -C4 'PlaybackController::~PlaybackController|void PlaybackController::reset\(|void PlaybackController::setMasterNotation\(|resetCurrentSequence\(|playback->resetCurrentSequence\(' "$f" doneExpected result: the reset/destructor/notation-switch paths should call
resetCurrentSequence()or an equivalentplayback->resetCurrentSequence()cleanup before the controller drops or replaces the active sequence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/playback/internal/playbackcontroller.h` around lines 53 - 54, PlaybackController now uses a process-global IPlayback (muse::GlobalInject<muse::audio::IPlayback> playback) so ensure all teardown paths clear global sequence state: update PlaybackController::~PlaybackController, PlaybackController::reset(), and PlaybackController::setMasterNotation() to call resetCurrentSequence() (or playback->resetCurrentSequence()) before dropping or replacing the active sequence and clearing m_currentSequenceId; search for uses of m_currentSequenceId and any code that replaces the sequence object and add a playback->resetCurrentSequence() call beforehand to avoid leaving the old sequence registered in the global playback service.src/framework/audio/engine/internal/tracksequence.cpp (1)
41-48: 🧹 Nitpick | 🔵 TrivialMove constructor-owned allocations into the initializer list.
m_clock,m_player, andm_audioIOare still default-initialized and then reassigned in the body. This is the same construction-style issue previously flagged by review and by cppcheck.♻️ Proposed refactor
-TrackSequence::TrackSequence(const TrackSequenceId id) - : m_id(id) +TrackSequence::TrackSequence(const TrackSequenceId id) + : m_id(id), + m_clock(std::make_shared<Clock>()), + m_player(std::make_shared<SequencePlayer>(this, m_clock)), + m_audioIO(std::make_shared<SequenceIO>(this)) { ONLY_AUDIO_ENGINE_THREAD; - - m_clock = std::make_shared<Clock>(); - m_player = std::make_shared<SequencePlayer>(this, m_clock); - m_audioIO = std::make_shared<SequenceIO>(this);#!/bin/bash # Confirm the constructor still performs member assignment in the body. fd -i 'tracksequence.cpp' src | while read -r f; do echo "== $f ==" rg -n -C2 'TrackSequence::TrackSequence|m_clock = std::make_shared<Clock>|m_player = std::make_shared<SequencePlayer>|m_audioIO = std::make_shared<SequenceIO>' "$f" done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/engine/internal/tracksequence.cpp` around lines 41 - 48, The constructor currently default-initializes members then assigns m_clock, m_player, and m_audioIO in the body; move those allocations into the TrackSequence::TrackSequence initializer list by initializing m_clock with std::make_shared<Clock>(), then m_player with std::make_shared<SequencePlayer>(this, m_clock) and m_audioIO with std::make_shared<SequenceIO>(this) in the initializer list (ensure the class declaration orders m_clock before m_player and m_audioIO so the initializer order is correct), and remove the redundant assignments from the constructor body while keeping ONLY_AUDIO_ENGINE_THREAD; reference symbols: TrackSequence::TrackSequence, m_clock, m_player, m_audioIO, Clock, SequencePlayer, SequenceIO.src/framework/audio/main/isoundfontinstallscenario.h (1)
30-30:⚠️ Potential issue | 🟠 MajorUse a global module interface here.
Line 30 still declares
ISoundFontInstallScenarioasMODULE_CONTEXT_INTERFACE, which keeps this service context-scoped even though the new install flow is being wired through the global audio path.🔧 Proposed fix
-class ISoundFontInstallScenario : MODULE_CONTEXT_INTERFACE +class ISoundFontInstallScenario : MODULE_GLOBAL_INTERFACE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/main/isoundfontinstallscenario.h` at line 30, ISoundFontInstallScenario is currently declared as MODULE_CONTEXT_INTERFACE which scopes it to a service context; change its declaration to the global module interface (replace MODULE_CONTEXT_INTERFACE with the global module interface macro used in this codebase, e.g., MODULE_INTERFACE) so ISoundFontInstallScenario becomes a global module interface and can be wired through the global audio path.src/framework/audio/main/audiomodule.cpp (1)
133-140:⚠️ Potential issue | 🔴 CriticalStop the background loops before tearing down playback.
The
m_rpcTicker.stop()call is still positioned afterm_mainPlayback->deinit()andm_engineGlobalSetup->onDeinit(). This creates a shutdown race where RPC dispatch can still be running while audio state is being deinitialized. Movem_rpcTicker.stop()andstopAudioProcessing()to the beginning ofonDeinit().Suggested ordering
void AudioModule::onDeinit() { + m_rpcTicker.stop(); + m_startAudioController->stopAudioProcessing(); m_mainPlayback->deinit(); - m_startAudioController->stopAudioProcessing(); m_engineGlobalSetup->onDeinit(); - - m_rpcTicker.stop(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/main/audiomodule.cpp` around lines 133 - 140, The shutdown order causes a race because m_rpcTicker may still dispatch while audio state is torn down; move m_rpcTicker.stop() and m_startAudioController->stopAudioProcessing() to the top of AudioModule::onDeinit() so they run before m_mainPlayback->deinit() and m_engineGlobalSetup->onDeinit(), ensuring background RPC/timers and audio processing are stopped prior to tearing down playback and engine state.src/framework/audio/main/platform/general/generalsoundfontinstallscenario.cpp (2)
46-49:⚠️ Potential issue | 🟠 MajorSurface install failures in the UI.
When path resolution or the copy fails, this flow only logs and returns. The user sees no feedback that the drag-and-drop install failed. Call
interactive()->error(...)in these failure branches to inform the user.Also applies to: 73-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/main/platform/general/generalsoundfontinstallscenario.cpp` around lines 46 - 49, When resolveInstallationPath(path) fails (newPath.ret is false) or the subsequent copy/install operation fails, the code only logs and returns; update those failure branches to call interactive()->error(...) so the UI surfaces the failure to the user. In the branch after resolveInstallationPath where you currently log "failed resolve path..." call interactive()->error with a clear message that includes newPath.ret.toString(); likewise locate the copy/install result variable (e.g., copyResult or installResult) in the later branch (lines ~73-85) and when its .ret indicates failure, call interactive()->error with a descriptive message including copyResult.ret.toString() before returning. Ensure both uses reference the existing resolveInstallationPath, newPath and the copy/install result variables so error text contains the underlying error detail.
104-114:⚠️ Potential issue | 🟠 MajorSkip the copy when source and destination are the same file.
If the dropped SoundFont already resides in the target writable directory,
resolveInstallationPath()returns the same path and the copy operation will attempt to copy a file onto itself, which typically fails. Short-circuit this case by checkingsrc == dstand directly registering the existing file without copying.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/main/platform/general/generalsoundfontinstallscenario.cpp` around lines 104 - 114, In GeneralSoundFontInstallScenario::doAddSoundFont the code may attempt to copy a file onto itself; add an early check if src == dst and in that case skip fileSystem()->copy, directly construct synth::SoundFontUri::fromLocalFile(dst) and call soundFontController()->addSoundFont(uri), then return a successful Ret (the same success value used when copy succeeds); otherwise proceed with the existing copy+register flow (fileSystem()->copy and addSoundFont).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/audio/main/audiomodule.cpp`:
- Line 154: Replace the raw new usage when registering the
GeneralSoundFontInstallScenario with a std::shared_ptr created by
std::make_shared to match the project's allocation pattern: change the call to
ioc()->registerExport<ISoundFontInstallScenario>(mname, ...) so it passes a
std::make_shared<GeneralSoundFontInstallScenario>(iocContext()) instead of new
GeneralSoundFontInstallScenario(iocContext()); this keeps ownership explicit and
consistent with other registrations in the file (see ISoundFontInstallScenario,
GeneralSoundFontInstallScenario, ioc()->registerExport, mname, iocContext()).
In `@src/framework/audio/main/internal/player.h`:
- Around line 38-39: The constructor Player(const TrackSequenceId sequenceId) is
a converting constructor and should be made explicit to prevent implicit
conversions; update the declaration of Player(const TrackSequenceId sequenceId)
to explicit Player(const TrackSequenceId sequenceId) and also update the
corresponding definition/implementation (e.g., Player::Player(const
TrackSequenceId sequenceId)) if it is repeated in the .cpp to match the explicit
declaration.
In
`@src/framework/audio/main/platform/general/generalsoundfontinstallscenario.cpp`:
- Around line 67-86: The duplicate success/failure handling around
doAddSoundFont(path, newPath.val) should be extracted into a single helper to
remove the repeated blocks: create a local lambda or private method (e.g.,
finalizeInstall or handleSoundFontInstall) that calls Ret ret =
doAddSoundFont(path, newPath.val) and then calls interactive()->info(...) on
success or LOGE() << "failed add soundfont, err: " << ret.toString() on failure;
replace both current branches with a single call to that helper (or move the
doAddSoundFont call after the overwrite check and call the shared
result-handling helper) so only one copy of the logic remains.
---
Outside diff comments:
In `@src/framework/audio/main/internal/playback.cpp`:
- Around line 105-109: In deinit(), replace the duplicated unsubscription of
Method::InputParamsChanged with an unsubscription for
Method::OutputParamsChanged so that deinit() mirrors init(); specifically update
the calls to channel()->onMethod(...) in deinit() to remove the duplicate
Method::InputParamsChanged and call
channel()->onMethod(Method::OutputParamsChanged, nullptr) instead, ensuring all
subscriptions made in init() (TrackAdded, TrackRemoved, InputParamsChanged,
OutputParamsChanged, MasterOutputParamsChanged) are unsubscribed.
---
Duplicate comments:
In `@src/framework/audio/engine/internal/tracksequence.cpp`:
- Around line 41-48: The constructor currently default-initializes members then
assigns m_clock, m_player, and m_audioIO in the body; move those allocations
into the TrackSequence::TrackSequence initializer list by initializing m_clock
with std::make_shared<Clock>(), then m_player with
std::make_shared<SequencePlayer>(this, m_clock) and m_audioIO with
std::make_shared<SequenceIO>(this) in the initializer list (ensure the class
declaration orders m_clock before m_player and m_audioIO so the initializer
order is correct), and remove the redundant assignments from the constructor
body while keeping ONLY_AUDIO_ENGINE_THREAD; reference symbols:
TrackSequence::TrackSequence, m_clock, m_player, m_audioIO, Clock,
SequencePlayer, SequenceIO.
In `@src/framework/audio/main/audiomodule.cpp`:
- Around line 133-140: The shutdown order causes a race because m_rpcTicker may
still dispatch while audio state is torn down; move m_rpcTicker.stop() and
m_startAudioController->stopAudioProcessing() to the top of
AudioModule::onDeinit() so they run before m_mainPlayback->deinit() and
m_engineGlobalSetup->onDeinit(), ensuring background RPC/timers and audio
processing are stopped prior to tearing down playback and engine state.
In `@src/framework/audio/main/isoundfontinstallscenario.h`:
- Line 30: ISoundFontInstallScenario is currently declared as
MODULE_CONTEXT_INTERFACE which scopes it to a service context; change its
declaration to the global module interface (replace MODULE_CONTEXT_INTERFACE
with the global module interface macro used in this codebase, e.g.,
MODULE_INTERFACE) so ISoundFontInstallScenario becomes a global module interface
and can be wired through the global audio path.
In
`@src/framework/audio/main/platform/general/generalsoundfontinstallscenario.cpp`:
- Around line 46-49: When resolveInstallationPath(path) fails (newPath.ret is
false) or the subsequent copy/install operation fails, the code only logs and
returns; update those failure branches to call interactive()->error(...) so the
UI surfaces the failure to the user. In the branch after resolveInstallationPath
where you currently log "failed resolve path..." call interactive()->error with
a clear message that includes newPath.ret.toString(); likewise locate the
copy/install result variable (e.g., copyResult or installResult) in the later
branch (lines ~73-85) and when its .ret indicates failure, call
interactive()->error with a descriptive message including
copyResult.ret.toString() before returning. Ensure both uses reference the
existing resolveInstallationPath, newPath and the copy/install result variables
so error text contains the underlying error detail.
- Around line 104-114: In GeneralSoundFontInstallScenario::doAddSoundFont the
code may attempt to copy a file onto itself; add an early check if src == dst
and in that case skip fileSystem()->copy, directly construct
synth::SoundFontUri::fromLocalFile(dst) and call
soundFontController()->addSoundFont(uri), then return a successful Ret (the same
success value used when copy succeeds); otherwise proceed with the existing
copy+register flow (fileSystem()->copy and addSoundFont).
In `@src/playback/internal/playbackcontroller.h`:
- Around line 53-54: PlaybackController now uses a process-global IPlayback
(muse::GlobalInject<muse::audio::IPlayback> playback) so ensure all teardown
paths clear global sequence state: update
PlaybackController::~PlaybackController, PlaybackController::reset(), and
PlaybackController::setMasterNotation() to call resetCurrentSequence() (or
playback->resetCurrentSequence()) before dropping or replacing the active
sequence and clearing m_currentSequenceId; search for uses of
m_currentSequenceId and any code that replaces the sequence object and add a
playback->resetCurrentSequence() call beforehand to avoid leaving the old
sequence registered in the global playback service.
🪄 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: e146bdd3-0af1-442e-9ff0-d31d494e68a2
📒 Files selected for processing (76)
src/app/internal/consoleapp.cppsrc/appshell/internal/applicationactioncontroller.cppsrc/appshell/internal/applicationactioncontroller.hsrc/framework/audio/common/rpc/irpcchannel.hsrc/framework/audio/common/rpc/platform/general/generalrpcchannel.cppsrc/framework/audio/engine/enginesetup.cppsrc/framework/audio/engine/enginesetup.hsrc/framework/audio/engine/iaudioengine.hsrc/framework/audio/engine/iengineplayback.hsrc/framework/audio/engine/internal/enginecontroller.cppsrc/framework/audio/engine/internal/enginecontroller.hsrc/framework/audio/engine/internal/engineplayback.cppsrc/framework/audio/engine/internal/engineplayback.hsrc/framework/audio/engine/internal/enginerpccontroller.hsrc/framework/audio/engine/internal/eventaudiosource.cppsrc/framework/audio/engine/internal/eventaudiosource.hsrc/framework/audio/engine/internal/export/soundtrackwriter.cppsrc/framework/audio/engine/internal/export/soundtrackwriter.hsrc/framework/audio/engine/internal/sequenceplayer.cppsrc/framework/audio/engine/internal/sequenceplayer.hsrc/framework/audio/engine/internal/synthesizers/abstractsynthesizer.cppsrc/framework/audio/engine/internal/synthesizers/abstractsynthesizer.hsrc/framework/audio/engine/internal/synthesizers/fluidsynth/fluidresolver.cppsrc/framework/audio/engine/internal/synthesizers/fluidsynth/fluidresolver.hsrc/framework/audio/engine/internal/synthesizers/fluidsynth/fluidsynth.cppsrc/framework/audio/engine/internal/synthesizers/fluidsynth/fluidsynth.hsrc/framework/audio/engine/internal/synthesizers/synthresolver.cppsrc/framework/audio/engine/internal/synthesizers/synthresolver.hsrc/framework/audio/engine/internal/tracksequence.cppsrc/framework/audio/engine/internal/tracksequence.hsrc/framework/audio/engine/isynthresolver.hsrc/framework/audio/iaudiodrivercontroller.hsrc/framework/audio/main/CMakeLists.txtsrc/framework/audio/main/audiomodule.cppsrc/framework/audio/main/audiomodule.hsrc/framework/audio/main/internal/audioconfiguration.hsrc/framework/audio/main/internal/audiodrivercontroller.hsrc/framework/audio/main/internal/playback.cppsrc/framework/audio/main/internal/playback.hsrc/framework/audio/main/internal/player.cppsrc/framework/audio/main/internal/player.hsrc/framework/audio/main/internal/startaudiocontroller.cppsrc/framework/audio/main/internal/startaudiocontroller.hsrc/framework/audio/main/iplayback.hsrc/framework/audio/main/isoundfontcontroller.hsrc/framework/audio/main/isoundfontinstallscenario.hsrc/framework/audio/main/istartaudiocontroller.hsrc/framework/audio/main/platform/general/generalsoundfontcontroller.cppsrc/framework/audio/main/platform/general/generalsoundfontcontroller.hsrc/framework/audio/main/platform/general/generalsoundfontinstallscenario.cppsrc/framework/audio/main/platform/general/generalsoundfontinstallscenario.hsrc/framework/musesampler/internal/musesamplerresolver.cppsrc/framework/musesampler/internal/musesamplerresolver.hsrc/framework/musesampler/internal/musesamplerwrapper.cppsrc/framework/musesampler/internal/musesamplerwrapper.hsrc/framework/stubs/audio/synthresolverstub.cppsrc/framework/stubs/audio/synthresolverstub.hsrc/framework/vst/internal/synth/vstiresolver.cppsrc/framework/vst/internal/synth/vstiresolver.hsrc/framework/vst/internal/synth/vstsynthesiser.cppsrc/framework/vst/internal/synth/vstsynthesiser.hsrc/importexport/audioexport/internal/abstractaudiowriter.cppsrc/importexport/audioexport/internal/abstractaudiowriter.hsrc/playback/internal/onlinesoundscontroller.hsrc/playback/internal/playbackcontroller.hsrc/playback/internal/soundprofilesrepository.hsrc/playback/isoundprofilesrepository.hsrc/playback/playbackmodule.cppsrc/playback/playbackmodule.hsrc/playback/qml/MuseScore/Playback/inputresourceitem.hsrc/playback/qml/MuseScore/Playback/mixerpanelmodel.hsrc/playback/qml/MuseScore/Playback/notationregionsbeingprocessedmodel.hsrc/playback/qml/MuseScore/Playback/outputresourceitem.hsrc/playback/qml/MuseScore/Playback/soundprofilesmodel.hsrc/preferences/qml/MuseScore/Preferences/audiomidipreferencesmodel.hsrc/preferences/qml/MuseScore/Preferences/commonaudioapiconfigurationmodel.h
💤 Files with no reviewable changes (1)
- src/importexport/audioexport/internal/abstractaudiowriter.cpp
RomanPudashkin
approved these changes
Apr 14, 2026
igorkorsukov
force-pushed
the
w/ioc/step_42
branch
from
c3d3ce7 to
f4167fa
Compare
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 (1)
src/preferences/qml/MuseScore/Preferences/audiomidipreferencesmodel.h (1)
38-39: 🧹 Nitpick | 🔵 TrivialDrop
Contextableinheritance—all dependencies are now global-scoped.The class still inherits
muse::Contextable(line 38) and initializes it withmuse::iocCtxForQmlObject(this)(constructor, line 32), but the implementation never callsiocContext(),setContext(), or any context-dependent APIs. All six injected members (lines 63–68) aremuse::GlobalInject<...>, meaning they resolve from the global container, not from any per-context scope. RemovingContextableeliminates unused overhead and clarifies that this model is globally scoped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/preferences/qml/MuseScore/Preferences/audiomidipreferencesmodel.h` around lines 38 - 39, Remove the unused per-context machinery from AudioMidiPreferencesModel: delete museum::Contextable from the class inheritance list and remove the constructor call that initializes it (muse::iocCtxForQmlObject(this)); leave the existing muse::GlobalInject<...> members as-is since they resolve from the global container. Ensure no remaining calls or references to iocContext()/setContext() remain in the class, and update constructor signature/initializer list accordingly so the class is purely QObject + muse::async::Asyncable with global injections.
♻️ Duplicate comments (3)
src/framework/audio/main/platform/general/generalsoundfontinstallscenario.cpp (2)
104-110:⚠️ Potential issue | 🟠 MajorSkip the copy when the SoundFont is already in the target location.
If
resolveInstallationPath()returns the same path as the dropped file,fileSystem()->copy(src, dst, true)becomes a self-copy and the install fails after the overwrite prompt. Short-circuitsrc == dstand register the existing file instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/main/platform/general/generalsoundfontinstallscenario.cpp` around lines 104 - 110, In GeneralSoundFontInstallScenario::doAddSoundFont, short-circuit when the source and destination are identical: if src == dst, skip calling fileSystem()->copy (to avoid a self-copy/overwrite prompt) and proceed to create the synth::SoundFontUri via synth::SoundFontUri::fromLocalFile(dst) and call soundFontController()->addSoundFont(uri); otherwise continue with the existing fileSystem()->copy(src, dst, true) flow and then register the new file.
46-49:⚠️ Potential issue | 🟠 MajorSurface installation failures in the UI.
These branches only log and return, so a failed drag-and-drop install looks like it was ignored. Show an
interactive()->error(...)/warning for path-resolution and copy failures before returning.Also applies to: 68-75, 78-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/main/platform/general/generalsoundfontinstallscenario.cpp` around lines 46 - 49, The code currently logs errors and returns in resolveInstallationPath and subsequent copy branches (e.g., the RetVal<io::path_t> newPath = resolveInstallationPath(path) check and the copy operation error branches) which makes failures invisible to users; update those branches to call interactive()->error(...) or interactive()->warning(...) with a user-facing message (include the underlying error text from newPath.ret.toString() or the copy RetVal) immediately before returning so path-resolution and copy failures surface in the UI; apply the same change to the other similar branches referenced (the copy failure checks and any other places that only LOGE() and return).src/framework/audio/main/audiomodule.cpp (1)
135-138:⚠️ Potential issue | 🔴 CriticalStop audio processing before deinitializing playback.
m_mainPlayback->deinit()still runs beforem_startAudioController->stopAudioProcessing(), so shutdown can race with the processing loop touching partially torn-down playback state. Stop the processing loop first, and wait for it to quiesce if that stop is async, then deinit playback/engine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/audio/main/audiomodule.cpp` around lines 135 - 138, The shutdown currently calls m_mainPlayback->deinit() before stopping the processing loop, which can race with in-flight audio processing; change the sequence to call m_startAudioController->stopAudioProcessing() first, wait for that call to fully quiesce if it returns/requests asynchronously (e.g., join/wait on its completion or a provided future/condition), then call m_mainPlayback->deinit(), followed by m_engineGlobalSetup->onDeinit() and m_rpcTicker.stop(); update any shutdown ordering in the same function to reflect this new sequence and ensure any async stop handling (callbacks/futures) is awaited before deinit calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/audio/common/rpc/irpcchannel.h`:
- Line 353: IRpcChannel was made global which exposes a stale callback UAF
because Playback::init registers Method::OutputParamsChanged but
Playback::deinit only clears Method::InputParamsChanged (twice) instead of
unregistering the Output callback; fix playback.cpp by having Playback::deinit
unregister the exact callback registered in Playback::init (remove
Method::OutputParamsChanged) so the global IRpcChannel will not call into a
destroyed Playback instance, ensuring symmetry between init/deinit for
registration/unregistration.
---
Outside diff comments:
In `@src/preferences/qml/MuseScore/Preferences/audiomidipreferencesmodel.h`:
- Around line 38-39: Remove the unused per-context machinery from
AudioMidiPreferencesModel: delete museum::Contextable from the class inheritance
list and remove the constructor call that initializes it
(muse::iocCtxForQmlObject(this)); leave the existing muse::GlobalInject<...>
members as-is since they resolve from the global container. Ensure no remaining
calls or references to iocContext()/setContext() remain in the class, and update
constructor signature/initializer list accordingly so the class is purely
QObject + muse::async::Asyncable with global injections.
---
Duplicate comments:
In `@src/framework/audio/main/audiomodule.cpp`:
- Around line 135-138: The shutdown currently calls m_mainPlayback->deinit()
before stopping the processing loop, which can race with in-flight audio
processing; change the sequence to call
m_startAudioController->stopAudioProcessing() first, wait for that call to fully
quiesce if it returns/requests asynchronously (e.g., join/wait on its completion
or a provided future/condition), then call m_mainPlayback->deinit(), followed by
m_engineGlobalSetup->onDeinit() and m_rpcTicker.stop(); update any shutdown
ordering in the same function to reflect this new sequence and ensure any async
stop handling (callbacks/futures) is awaited before deinit calls.
In
`@src/framework/audio/main/platform/general/generalsoundfontinstallscenario.cpp`:
- Around line 104-110: In GeneralSoundFontInstallScenario::doAddSoundFont,
short-circuit when the source and destination are identical: if src == dst, skip
calling fileSystem()->copy (to avoid a self-copy/overwrite prompt) and proceed
to create the synth::SoundFontUri via synth::SoundFontUri::fromLocalFile(dst)
and call soundFontController()->addSoundFont(uri); otherwise continue with the
existing fileSystem()->copy(src, dst, true) flow and then register the new file.
- Around line 46-49: The code currently logs errors and returns in
resolveInstallationPath and subsequent copy branches (e.g., the
RetVal<io::path_t> newPath = resolveInstallationPath(path) check and the copy
operation error branches) which makes failures invisible to users; update those
branches to call interactive()->error(...) or interactive()->warning(...) with a
user-facing message (include the underlying error text from
newPath.ret.toString() or the copy RetVal) immediately before returning so
path-resolution and copy failures surface in the UI; apply the same change to
the other similar branches referenced (the copy failure checks and any other
places that only LOGE() and return).
🪄 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: 677fa4cf-5b80-4a85-a1b9-7bbc6c93de85
📒 Files selected for processing (77)
src/app/internal/consoleapp.cppsrc/appshell/internal/applicationactioncontroller.cppsrc/appshell/internal/applicationactioncontroller.hsrc/framework/audio/common/rpc/irpcchannel.hsrc/framework/audio/common/rpc/platform/general/generalrpcchannel.cppsrc/framework/audio/engine/enginesetup.cppsrc/framework/audio/engine/enginesetup.hsrc/framework/audio/engine/iaudioengine.hsrc/framework/audio/engine/iengineplayback.hsrc/framework/audio/engine/internal/enginecontroller.cppsrc/framework/audio/engine/internal/enginecontroller.hsrc/framework/audio/engine/internal/engineplayback.cppsrc/framework/audio/engine/internal/engineplayback.hsrc/framework/audio/engine/internal/enginerpccontroller.hsrc/framework/audio/engine/internal/eventaudiosource.cppsrc/framework/audio/engine/internal/eventaudiosource.hsrc/framework/audio/engine/internal/export/soundtrackwriter.cppsrc/framework/audio/engine/internal/export/soundtrackwriter.hsrc/framework/audio/engine/internal/sequenceplayer.cppsrc/framework/audio/engine/internal/sequenceplayer.hsrc/framework/audio/engine/internal/synthesizers/abstractsynthesizer.cppsrc/framework/audio/engine/internal/synthesizers/abstractsynthesizer.hsrc/framework/audio/engine/internal/synthesizers/fluidsynth/fluidresolver.cppsrc/framework/audio/engine/internal/synthesizers/fluidsynth/fluidresolver.hsrc/framework/audio/engine/internal/synthesizers/fluidsynth/fluidsynth.cppsrc/framework/audio/engine/internal/synthesizers/fluidsynth/fluidsynth.hsrc/framework/audio/engine/internal/synthesizers/synthresolver.cppsrc/framework/audio/engine/internal/synthesizers/synthresolver.hsrc/framework/audio/engine/internal/tracksequence.cppsrc/framework/audio/engine/internal/tracksequence.hsrc/framework/audio/engine/isynthresolver.hsrc/framework/audio/iaudiodrivercontroller.hsrc/framework/audio/main/CMakeLists.txtsrc/framework/audio/main/audiomodule.cppsrc/framework/audio/main/audiomodule.hsrc/framework/audio/main/internal/audioconfiguration.hsrc/framework/audio/main/internal/audiodrivercontroller.hsrc/framework/audio/main/internal/playback.cppsrc/framework/audio/main/internal/playback.hsrc/framework/audio/main/internal/player.cppsrc/framework/audio/main/internal/player.hsrc/framework/audio/main/internal/startaudiocontroller.cppsrc/framework/audio/main/internal/startaudiocontroller.hsrc/framework/audio/main/iplayback.hsrc/framework/audio/main/isoundfontcontroller.hsrc/framework/audio/main/isoundfontinstallscenario.hsrc/framework/audio/main/istartaudiocontroller.hsrc/framework/audio/main/platform/general/generalsoundfontcontroller.cppsrc/framework/audio/main/platform/general/generalsoundfontcontroller.hsrc/framework/audio/main/platform/general/generalsoundfontinstallscenario.cppsrc/framework/audio/main/platform/general/generalsoundfontinstallscenario.hsrc/framework/musesampler/internal/musesamplerresolver.cppsrc/framework/musesampler/internal/musesamplerresolver.hsrc/framework/musesampler/internal/musesamplerwrapper.cppsrc/framework/musesampler/internal/musesamplerwrapper.hsrc/framework/stubs/audio/audiostubmodule.cppsrc/framework/stubs/audio/synthresolverstub.cppsrc/framework/stubs/audio/synthresolverstub.hsrc/framework/vst/internal/synth/vstiresolver.cppsrc/framework/vst/internal/synth/vstiresolver.hsrc/framework/vst/internal/synth/vstsynthesiser.cppsrc/framework/vst/internal/synth/vstsynthesiser.hsrc/importexport/audioexport/internal/abstractaudiowriter.cppsrc/importexport/audioexport/internal/abstractaudiowriter.hsrc/playback/internal/onlinesoundscontroller.hsrc/playback/internal/playbackcontroller.hsrc/playback/internal/soundprofilesrepository.hsrc/playback/isoundprofilesrepository.hsrc/playback/playbackmodule.cppsrc/playback/playbackmodule.hsrc/playback/qml/MuseScore/Playback/inputresourceitem.hsrc/playback/qml/MuseScore/Playback/mixerpanelmodel.hsrc/playback/qml/MuseScore/Playback/notationregionsbeingprocessedmodel.hsrc/playback/qml/MuseScore/Playback/outputresourceitem.hsrc/playback/qml/MuseScore/Playback/soundprofilesmodel.hsrc/preferences/qml/MuseScore/Preferences/audiomidipreferencesmodel.hsrc/preferences/qml/MuseScore/Preferences/commonaudioapiconfigurationmodel.h
💤 Files with no reviewable changes (1)
- src/importexport/audioexport/internal/abstractaudiowriter.cpp
| }; | ||
|
|
||
| class IRpcChannel : MODULE_CONTEXT_INTERFACE | ||
| class IRpcChannel : MODULE_GLOBAL_INTERFACE |
There was a problem hiding this comment.
Globalizing IRpcChannel turns the existing missing handler cleanup into a UAF risk.
Playback::init() registers Method::OutputParamsChanged, but Playback::deinit() never unregisters it and instead clears Method::InputParamsChanged twice. With a global channel, that stale callback can outlive Playback and fire into freed state.
🔧 Fix the missing unregister in src/framework/audio/main/internal/playback.cpp
void Playback::deinit()
{
ONLY_AUDIO_MAIN_THREAD;
channel()->onMethod(Method::TrackAdded, nullptr);
channel()->onMethod(Method::TrackRemoved, nullptr);
channel()->onMethod(Method::InputParamsChanged, nullptr);
- channel()->onMethod(Method::InputParamsChanged, nullptr);
+ channel()->onMethod(Method::OutputParamsChanged, nullptr);
channel()->onMethod(Method::MasterOutputParamsChanged, nullptr);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/audio/common/rpc/irpcchannel.h` at line 353, IRpcChannel was
made global which exposes a stale callback UAF because Playback::init registers
Method::OutputParamsChanged but Playback::deinit only clears
Method::InputParamsChanged (twice) instead of unregistering the Output callback;
fix playback.cpp by having Playback::deinit unregister the exact callback
registered in Playback::init (remove Method::OutputParamsChanged) so the global
IRpcChannel will not call into a destroyed Playback instance, ensuring symmetry
between init/deinit for registration/unregistration.
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
2 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.
No description provided.