-
Notifications
You must be signed in to change notification settings - Fork 443
WASAPI: Use ActivateAudioInterfaceAsync
with virtual device IDs for default devices
#1027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…able automatic stream routing in case the default device changes
# Conflicts: # Cargo.toml # src/host/wasapi/device.rs
# Conflicts: # Cargo.toml # src/host/wasapi/device.rs
979a496
to
2517c28
Compare
2517c28
to
0830b3f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I don't really see a way around the windows
crate version problem. How much of a problem that is, kind of depends on the community... don't know how much it'll hurt to bump the minimum to v0.61. If anyone feels strongly about that, do comment here.
# | ||
# Note that this only works on Windows 8 and above. It is turned on by default, | ||
# but consider turning it off if you are supporting an older version of Windows. | ||
wasapi-virtual-default-devices = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, a documentation suggestion:
# Enable virtual default devices for WASAPI. When enabled:
# - Audio automatically reroutes when the default device changes
# - Streams survive device changes (e.g., plugging in headphones)
# - Requires Windows 8 or later
#
# Disable this feature if supporting Windows 7 or earlier.
Second, I wonder if we should invert the feature and rename it to windows-legacy
(disabled by default). I'm not so sure if "virtual default devices" clearly communicates its intention. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good shout on the docs, will do first.
I think inverting the feature would be more ergonomic, but the Cargo docs suggest that features should always be additive:
A consequence of this is that features should be additive. That is, enabling a feature should not disable functionality, and it should usually be safe to enable any combination of features. A feature should not introduce a SemVer-incompatible change.
I think it's probably fine - I don't see a way for this to cause a conflict with another package, as you'd want this to be controlled at the application level anyway - but I'm not sure. For now, I'll just action the doc change, but I'm open to the inversion if you think it's compatible with the norms of the ecosystem.
Feature name: Hmm, yeah, I'm not sold on it either; it was what first came to mind to cover the concept of activating the output with a virtual default device, but it's not the most self-descriptive thing. Maybe wasapi-default-device-autorouting
or something? (I'm using wasapi
because it looks like cpal
supports ASIO as well, but I'm happy to change to windows
if that sounds better)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can still argue it's additive: it enables legacy support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, will action!
"Win32_UI_Shell_PropertiesSystem", | ||
] } | ||
# Explicitly depend on windows-core for use with the `windows::core::implement` macro. | ||
windows-core = "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cargo publish
will reject wildcard versions, please specify what we need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... yes, that is a problem. I'll see what I can do; the problem I saw was that the CI would fail to resolve windows-core
to the correct version, but that might be fine with the bodge I put in there. Will test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this is a problem. cargo
will not resolve windows-core
to the same version of windows
's windows-core
if I match the version constraints or use a lax constraint like ^0.61
.
I can see three options here:
- Restrict the version to a single known version (0.61 or 0.62)
- Ask
windows-rs
to add support for referencingwindows::core
, instead ofwindows_core
, somehow: Cannot use implement macro microsoft/windows-rs#3568 (comment). Even if this happened, it would have to be anotherwindows-rs
release. - Ask a Cargo wizard if there are any solutions for getting
*
-like behaviour in a way that can still becargo publish
-ed
I'll ask around regarding 3, and ask in that windows-rs
issue for future support, but 1 may be necessary in the meantime :S
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for asking in windows-rs
, how did that go?
The frequent API breakage of these windows
crates sure is a problem when trying to support a ~12 month window of them. I'm not a Windows user myself, and wouldn't mind "bumping" the Minimum Supported Windows Version. But from what I gather from the community here, historical support is a desired thing to have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But from what I gather from the community here, historical support is a desired thing to have.
Yeah especially with how Microsoft is treating their users on windows 11 we will want to support windows 10 as long as we can. Thank you soo much for your work in that regard philpax and roderick. I know its a pain to support te windows platform, but the impact is so high. Especially for gamedev (bevy).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
windows-rs
doesn't have any immediate solutions; they're planning on refactoring their crate structure, but as far as I can tell, the result would have a similar problem. (It also wouldn't help us with our historical support of crate versions, even if they did make a change: we'd still have to bump to that version onwards)
Yeah especially with how Microsoft is treating their users on windows 11 we will want to support windows 10 as long as we can.
Hmm, I'm pretty sure newer versions of the windows
crate support older Windows versions, it's mostly just that you're likely to have multiple versions of the crate without some mechanism to synchronise them (e.g. windows v0.58 and windows v0.62, etc)
Speaking of, I was thinking of another solution: catpuccin-egui uses individual features for each supported version of egui
, so the user can control which version of egui
is used by switching out the feature in use.
We could do something similar for both windows
and windows-core
, so that the user can select which version to target, defaulting to either the earliest or the latest supported by us (e.g. default on windows-0-62
). I think this would also solve the issue with implement
, so we could extend the range of support once more.
Of course, this comes with the downside that the user needs to opt into an older version of windows
-as-used-by-cpal
-as-maybe-used-by-rodio
, but I suspect it's the only viable solution given the constraints at hand (outside of locking down to one windows
version and potentially bloating users' builds). Thoughts? Do we know of any users who have strong feelings on the matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, wait, just realised that wouldn't work... the implement
macro would still look for a global windows_core
, bypassing any aliases we set up. What a headache 😭
Fixes #740 (mostly). Updated version of #754 that aims to address the issues mentioned in that PR.
Background
When you currently play some audio on the default device using
cpal
, the default device is fetched once, and then a stream is created from that device to play audio. However, changing the default device will lead to that stream continuing to play on the original device, and removing the device entirely (e.g. unplugging headphones) will result in the stream dying as it no longer has a device to output to.I spent several hours trying to fix this properly - that is, trying to update the device and rebuild the stream when it changes - and stopped when I realised that a significant amount of the WASAPI stream code would need to be revised to handle a changing device. For those curious, you can see my experimental work here. The goal was to replicate the flow of this Chromium code, which implements this logic.
The Imperfect Fix
However, you don't actually need to do any of that if you don't care about supporting <Windows 8. Windows 8 introduced
ActivateAudioInterfaceAsync
, and with it, virtual device interfaces.ActivateAudioInterfaceAsync
can be used instead ofAudio::IMMDevice::Activate
to generate aAudio::IAudioClient
that will automatically reroute audio for you if the device changes.This is what the previous PR did, and it works great. On top of that PR, this PR:
name
use the current default deviceRamifications
I've added a default-on feature,
wasapi-virtual-default-devices
, that adds support for this. Users that need to support older versions of Windows can turn this feature off, and it will behave as the current status quo does. (I'm not actually sure what happens if you have this feature on for an older Windows: in an ideal world, it does nothing, and newer Windows users can enjoy virtual devices. Please let me know if you've tested this.)One downside of this is that I had to bump the minimum
windows
version to 0.61. This is because 0.61 removed the previously-explicitly-requiredimplement
feature in 0.61, and I can't see any way to conditionally include that feature for older versions ofwindows
.cargo
will refuse to resolve thewindows
dependency with a feature that doesn't exist for the version it's resolving to. I'm happy to drop it back down if anyone can think of a workaround for this.