Skip to content

Conversation

edwloef
Copy link
Contributor

@edwloef edwloef commented Feb 22, 2025

implements #939. As someone who doesn't know a whole lot about cpal's inner workings it looks to me like these are the only threads that the library spawns itself, leading me to think these are the only threads that require manual priority changes. However, audio_thread_priority claims to also support MacOS, so maybe someone could look into whether that's applicable here?

I'm not too massive of a fan of eprintln! here, however I think the failure should be shown but not prevent the audio thread from running.

I've tested this on my Linux laptop, some verification that this also works for the folks on WASAPI and the BSD's would be nice.

@edwloef
Copy link
Contributor Author

edwloef commented Feb 22, 2025

Seems like linux CI is failing due to dbus not being installed on the linux runners. Windows CI is failing cause I messed up :P

@edwloef
Copy link
Contributor Author

edwloef commented Feb 22, 2025

Alright, windows is fixed. Should I update the CI and readme to include dbus on linux?

@jacksongoode
Copy link

I believe this will resolve jpochyla/psst#235 and jpochyla/psst#251. Anything blocking this right now?

@marcpabst
Copy link

marcpabst commented Apr 3, 2025

Just as a note, I belive any parameters passed to promote_current_thread_to_real_time are discarded on Windows. That said, can we get this reviewed/merge?

@edwloef
Copy link
Contributor Author

edwloef commented Apr 3, 2025

I've added dbus to the CI and mentioned it in the readme. I think the wasm failure isn't related to the changes I've made, so as far as I can tell everything should be good to go.

@est31
Copy link
Member

est31 commented May 16, 2025

Can you make this optional (default off)? I don't think we should require an extra C dependency from users unconditionally.

@edwloef
Copy link
Contributor Author

edwloef commented May 16, 2025

Should it also be made optional on windows as well, or just linux? I'd assume optional as well, but due to licensing (MPL), not C dependencies.

implements #939. As someone who doesn't know a whole lot about
cpal's inner workings it looks to me like these are the only
threads that the library spawns itself, leading me to think these
are the only threads that require manual priority changes. However,
`audio_thread_priority` claims to also support MacOS, so maybe
someone could look into whether that's applicable here?
@jacksongoode
Copy link

Can you make this optional

Should it be optional? I feel as if this should be baked for Windows since otherwise audio streams can become mangled under high load?

@edwloef
Copy link
Contributor Author

edwloef commented May 16, 2025

Can you make this optional

Should it be optional? I feel as if this should be baked for Windows since otherwise audio streams can become mangled under high load?

If it's optional I can just leave the existing solution for people who don't enable the feature. The license (MPL) might be an issue for some people, so I can envision a use case for it being optional, which is what I've just done for now.

@Ralith
Copy link
Contributor

Ralith commented May 16, 2025

If this is a C dependency, we should just vendor equivalent logic. It should only be a few syscalls on any platform. Requiring a feature to be enabled to get the correct behavior is a bad user experience.

@edwloef
Copy link
Contributor Author

edwloef commented May 16, 2025

audio_thread_priority provides a dbus-less implementation as well, but I can't tell whether it actually does anything or is just a stub. Maybe re-implementing the relevant portions with something like zbus would be reasonable, but zbus is humongous and I'm not sure whether that's much better than a C dependency.

@jacksongoode
Copy link

@est31 Sorry for the ping, but I think this one is ready and looked over quite well :)

@est31 est31 merged commit 49245b4 into RustAudio:master May 20, 2025
15 checks passed
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.

5 participants