Skip to content

[4.7.3] Fix auto switch to System default#33569

Open
RomanPudashkin wants to merge 3 commits into
musescore:4.7from
RomanPudashkin:fix_auto_switch_to_system_default_47
Open

[4.7.3] Fix auto switch to System default#33569
RomanPudashkin wants to merge 3 commits into
musescore:4.7from
RomanPudashkin:fix_auto_switch_to_system_default_47

Conversation

@RomanPudashkin
Copy link
Copy Markdown
Contributor

Resolves: #33224

@RomanPudashkin
Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 25, 2026

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 25, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This pull request refactors the audio driver configuration and device fallback mechanisms. Buffer size constants are redefined as platform-dependent MINIMUM_BUFFER_SIZE and MAXIMUM_BUFFER_SIZE. A new defaultOutputSpec() accessor is added to the audio configuration interface and implemented with fixed defaults. The AudioDriverController is restructured to use this new accessor, removes its internal defaultSpec() helper, and adds a switchToDefaultAudioDriver() method to centralize fallback logic. Device opening now includes explicit fallback chains: retry with the driver's default device, then switch to the configured default driver. Error logging is added to buffer-size and sample-rate change operations. The ASIO driver's reset method changes from closing and reopening to emitting a device-change notification.

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description references the resolved issue #33224 but lacks detail on the changes made, testing performed, and verification of the fixes addressing both test cases. Expand description to explain the key changes: removal of DEFAULT_BUFFER_SIZE, introduction of min/max buffer sizes, modification of ASIO reset behavior, and addition of defaultOutputSpec(). Include testing verification for both device-disconnected scenarios.
Out of Scope Changes check ⚠️ Warning The removal of DEFAULT_BUFFER_SIZE and addition of MINIMUM_BUFFER_SIZE/MAXIMUM_BUFFER_SIZE appear out-of-scope relative to issue #33224's focus on ASIO device disconnection handling. Clarify whether the buffer size constant changes are necessary for the device switching fix. If not directly related to #33224, move to a separate PR or explain the dependency in the description.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title '[4.7.3] Fix auto switch to System default' clearly summarizes the main change—fixing the automatic switch to system default audio driver.
Linked Issues check ✅ Passed The code changes address both requirements from issue #33224: modifying ASIO reset() to emit device-change notifications [#33224], and enhancing AudioDriverController to switch to default driver on device failure [#33224].

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/framework/audio/main/internal/audiodrivercontroller.cpp (1)

218-230: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Do not persist API switch when driver open fails.

When open() fails, this path still sets currentAudioApi and notifies listeners. That can leave config/UI claiming the new API is active while no driver was opened.

Proposed diff
-    if (m_audioDriver && !m_audioDriver->defaultDevice().empty()) {
+    bool ok = false;
+    if (m_audioDriver && !m_audioDriver->defaultDevice().empty()) {
         spec.deviceId = DEFAULT_DEVICE_ID;
-        bool ok = m_audioDriver->open(spec, nullptr);
+        ok = m_audioDriver->open(spec, nullptr);
         if (!ok) {
             LOGE() << "Failed to open audio driver: " << name;
         }
     } else {
         LOGW() << "No devices for " << name;
     }

-    configuration()->setCurrentAudioApi(name);
-    m_currentAudioApiChanged.notify();
+    if (ok) {
+        configuration()->setCurrentAudioApi(name);
+        m_currentAudioApiChanged.notify();
+    }
     m_availableOutputDevicesChanged.notify();
🤖 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 `@src/framework/audio/main/internal/audioconfiguration.cpp`:
- Around line 179-186: defaultOutputSpec() hardcodes sampleRate,
samplesPerChannel and audioChannelCount leading to duplicated literals; update
OutputSpec AudioConfiguration::defaultOutputSpec() to derive values from the
existing defaults instead: set spec.sampleRate from AUDIO_SAMPLE_RATE_KEY (or
the function that returns its default), set spec.samplesPerChannel from
AUDIO_BUFFER_SIZE_KEY, and set spec.audioChannelCount via audioChannelsCount()
(or fall back to AUDIO_CHANNELS) so the function uses the single source-of-truth
constants/queries already present in this file.
🪄 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: b99b9e1b-a4b8-406c-9bcf-a652923ef672

📥 Commits

Reviewing files that changed from the base of the PR and between 69af3e1 and 6b4c13e.

📒 Files selected for processing (7)
  • src/framework/audio/common/audiotypes.h
  • src/framework/audio/driver/platform/win/asio/asioaudiodriver.cpp
  • src/framework/audio/main/iaudioconfiguration.h
  • src/framework/audio/main/internal/audioconfiguration.cpp
  • src/framework/audio/main/internal/audioconfiguration.h
  • src/framework/audio/main/internal/audiodrivercontroller.cpp
  • src/framework/audio/main/internal/audiodrivercontroller.h
💤 Files with no reviewable changes (1)
  • src/framework/audio/common/audiotypes.h

Comment thread src/framework/audio/main/internal/audioconfiguration.cpp
* ASIO reset() now fires availableOutputDevicesChanged instead of
  self-handling close/reopen, matching the WASAPI behavior
* AudioDriverController::checkOutputDevice() falls back to the default
  audio driver (WASAPI) if reopen fails
@RomanPudashkin RomanPudashkin force-pushed the fix_auto_switch_to_system_default_47 branch from 6b4c13e to 77b6550 Compare May 25, 2026 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ASIO support] After disconnecting ASIO device, MSS should switch to WASAPI>System default

3 participants