Repository navigation
feat(audio): keep host default sink with keep_default_sink - #5935
mikefaille wants to merge 16 commits into
Conversation
… host default Adds the keep_default_sink config key controlling whether audio capture swaps the host default sink. - Config: new keep_default_sink key (default disabled) with validation. - Policy: when enabled, capture keeps the host default sink untouched; teardown restores only state this session changed. - UI: AudioVideo tab toggle plus locale strings. - Docs: configuration.md documents the new key. - Tests: config, audio, and linux audio unit coverage.
Rename shadowed module field (S1669), extract adopt_sink/ensure_sink helpers from the duplicated sink setup blocks (S3776), shrink the oversized enumeration lambda (S1188), and guard destructor teardown against throwing unloads (S1048). Behavior unchanged.
Give the destructor's catch-all a log statement so the intentionally suppressed unload path is not an empty handler (S2486). Teardown stays non-throwing.
ReenigneArcher
left a comment
There was a problem hiding this comment.
Why doesn't your agent follow the AGENTS.md file?
|
@ReenigneArcher I will review. |
I reviewed myself and I don't see the issue. |
There was a problem hiding this comment.
Requesting changes for the silent-capture case and the web-test regression detailed inline.
The configured-equalizer scenario from #4950 is handled by the new capture-only path in focused routing tests. Latest master still contains the default-sink swap, so this scope is not superseded.
Cross-platform scope: please keep this as a shared routing option and extend support beyond Linux. Windows already resolves explicit WASAPI endpoints directly, and its existing external_audio mode preserves defaults; reconcile the new option with that mode's broader contract. A Windows implementation needs a capture-only path plus protection against the default-endpoint reassertion callback, startup default-device reset, and any automatic driver-installation path that changes defaults. macOS already captures through a system tap or selected input without changing the default output, so preserve and document that behavior. Add platform-specific tests for startup, device-change notifications, and teardown. The earlier inline recommendation to hide the setting outside Linux is withdrawn.
Validation: git diff --check and clang-format checks passed for all changed native files. Clang syntax checks passed for src/audio.cpp, src/config.cpp, and their two changed cross-platform test files. The existing ExternalAudio.test.js suite has 1 failure / 4 passes on this head; updating the Windows control-list assertion for the shared checkbox gives 5 passes. A GoogleTest harness using the PR's extracted routing/policy/monitor-selection code has 1 failure / 4 passes; the suggested capture-target change gives 5 passes. These are focused checks, not a full Sunshine build or a live PulseAudio playback test.
Verification limit: GitHub Actions awaits maintainer approval. I inspected the failed GH-Pages Read the Docs log: it timed out waiting for call-jekyll-build / Build Jekyll, rather than reporting a source/documentation defect. SonarCloud and the main documentation check passed.
The head is still 83155a7823c89870f05390fa8977d09a5049b693 and GitHub reports it mergeable. The earlier attribution request remains unaddressed: please name the AI agent, model, and thinking level at the end of the PR body and repeat that attribution in a separate comment, as required by the repository instructions.
Review completed by Codex | Model: gpt-6.1-sol | Thinking: xhigh
Route the capture-only path at the configured sink, or the saved host sink when nothing is configured, instead of the virtual sink. Without a swap, applications stay on the host default, so capturing the virtual sink's monitor yields silence when host playback is muted. Extract the startup selection plus swap-vs-capture policy into the pure resolve_capture_routing() helper so the empty/configured sink by HOST_AUDIO matrix is covered by unit tests, and pin the historical virtual-sink selection on the swap path.
Include the new shared checkbox in the Windows control-list assertion and add round-trip coverage rendering it on Windows, Linux, and macOS.
Share the keep-default routing contract across backends. Windows records the capture target for direct WASAPI capture and guards the default reassertion callback, the startup default-device reset, and the Steam driver install behind hands-off-mode predicates, composing with the existing external_audio behavior. macOS reports support natively since tap/input capture never touches the default output. Document the per-platform behavior.
Satisfies Sonar cpp:S3562 on the new code. Behavior unchanged: layouts other than stereo/5.1/7.1 keep the previously selected sink.
Extract the duplicated mountAudioVideo helper into audioVideoMount.js so the keep_default_sink suite and the external_audio suite use one copy. Clears the Sonar new-code duplication gate.
|
I tried for a common interface for Linux Mac and Windows but it's would be more sane to split the PR to focus only on Linux as I can't test Windows and Mac right now. Unless someone want to test this patch directly with Windows and Mac ? |
ReenigneArcher
left a comment
There was a problem hiding this comment.
Rereviewed 6747ba142b37c48bc9643ce291793278f2b1b722, including the follow-up changes since 83155a7 and the complete diff against current master (1582fff). Both earlier code findings are fixed: capture-only routing selects the configured/host sink, and the shared checkbox is covered by the updated web tests. I found no additional runtime defect in this review.
The Steam-driver statement matches the implementation. Installation is conditional in Windows audio_control(), when the controller is created and the driver is missing. Configuration is loaded before platform/audio initialization, so the option can be enabled before that installation attempt. An already-installed driver is left installed. The setting suppresses a future automatic installation attempt; it does not remove an existing installation.
The remaining required change is the attribution previously requested. The current PR body still ends with generic “AI-assisted implementation and tests” text, and the discussion has no corresponding agent/model/thinking attribution. Please add the actual AI agent, model, and thinking level at the end of the body and repeat it in a separate comment, as required by the repository contribution instructions. This rereview requests changes for that outstanding requirement; the two earlier code findings are resolved.
Validation: all 8 tests in ExternalAudio.test.js and KeepDefaultSink.test.js passed. I compiled and ran the 7 current SinkRoutingPolicyTest, CaptureRoutingTest, and AudioControlAdapterTest cases in a focused binary against the PR's actual header; all passed. Clang syntax checks passed for src/audio.cpp, src/config.cpp, their changed common test files, src/platform/macos/microphone.mm, and test_av_audio.mm. Formatting checks passed for all 13 changed native files, and git diff --check passed. These checks do not constitute a full Sunshine build or live audio-device validation.
Verification limit: Windows and Linux platform execution remains unverified locally. Hosted workflows await maintainer approval; the failed GH-Pages Read the Docs build timed out waiting for its build workflow. SonarCloud and the main documentation check passed. GitHub reports this head mergeable.
Review completed by Codex | Model: gpt-6.1-sol | Thinking: xhigh
I can test it. |
|
|
I saw in release notes of new version that this was approved but for Windows only #5647 It would probably be best to use same variable for windows and linux. |
I will have an eye on it today. I will try to see if we can unify even more the implementation. |
Description
Adds a shared
keep_default_sinkaudio option that lets Sunshine capture aconfigured
audio_sinkwithout making the Sunshine sink the host default.Mechanism: a sink-routing policy object is resolved from the audio settings
and handed to
capture_sink. When the option is enabled, capture only setsthe requested sink and logs that the host default is being left alone — no
swap at startup, no restore at teardown. The capture-only path targets the
configured sink, or the saved host sink when nothing is configured, so muted
hosts never capture a silent virtual sink. The option defaults to off,
preserving current behavior unless explicitly opted in. Wired through config,
the web UI audio tab, and unit tests.
Backend support: Linux adopts pre-existing null sinks instead of unloading
them on peer teardown; Windows captures the target endpoint directly and
skips the default reassertion callback, the startup default-device reset,
and the Steam driver install while the option is enabled; macOS honors the
option natively since tap/input capture never touches the default output.
Note: this addresses the reported default-sink takeover only when the new
option is enabled; existing
audio_sink-only configs still swap by default.Screenshot
The change adds a "Keep Default Sink" checkbox to the Audio/Video settings
tab. Screenshot to be attached by the author.
Issues Fixed or Closed
Roadmap Issues
None.
Testing
cmake --build build -j2(Release, Ninja, CUDA enabled): exit 0--gtest_filter='*Audio*:*Sink*:*KeepDefault*:*Config*':311 tests from 37 suites, all passed — including
KeepDefaultSinkAudioTest.CaptureSinkKeepsHostDefaultSink,NullSinkOwnershipTest.AdoptedSinksSurvivePeerTeardown,SinkRoutingPolicyTest(x2),CaptureRoutingTest(x4),AudioControlAdapterTest, andKeepDefaultSinkConfigTest(x2)(pre-existing platform skips), 0 failed
npx vitest run tests/web/ExternalAudio.test.js tests/web/KeepDefaultSink.test.js --coverage.enabled=false: 8 passeduv run --locked lb-update-clang-format: no changes (format clean)built on this Linux host; they follow existing seam patterns and await
platform CI (fork workflows await maintainer approval)
Type of Change
Checklist
AI Usage
See our AI usage policy.
AI-assisted implementation and tests; reviewed and tested by the author.