Conversation
19070c2 to
1eca9ce
Compare
9e2ef9f to
fd59a29
Compare
|
Seems pipewire need edition 2024.. |
|
@roderickvd can you help review this pr? Thanks, and when can this crate be upgraded to edition 2024? I would also want to help |
Definitely will help you review it. Need a bit more time.
Actually cpal itself doesn't need to be upgraded to Rust 2024, it just needs a MSRV of Rust of 1.85 or higher to support dependencies that are Rust 2024 already. When we can I'd like to stick cpal to Rust 2021 so we keep our MSRV down. |
|
Super cool man! Happy to see cpal having better linux support, opening the possibility of loopback recording on Linux! |
07bc999 to
6cea2ad
Compare
|
ok, only one ci that I cannot fix |
a81e104 to
5a3817c
Compare
|
ok cross-rs based on ubuntu20.04, so |
721beb0 to
424d78f
Compare
|
How does this differ from #692? |
Yes, it should be marked as Copy, but it should in another pr
I do not know if it is right. At least I should not set the rate
use "node.force-quantum" to force the buffer size
open the feature of v0_3_45
34ef049 to
56fa93e
Compare
roderickvd
left a comment
There was a problem hiding this comment.
Thanks so much for this work! Let's focus on getting it merged for v0.18, and so I spent some time on a code review.
src/host/pipewire/mod.rs
Outdated
| type Devices = Devices; | ||
| type Device = Device; | ||
| fn is_available() -> bool { | ||
| true |
There was a problem hiding this comment.
Could it test for availability like with a socket connection?
There was a problem hiding this comment.
Ok, I have added a simple way to check it
|
|
||
| - name: Run tests (all features) | ||
| run: cargo +${{ steps.msrv.outputs.all-features }} test --all-features --workspace --verbose | ||
| run: cargo +${{ steps.msrv.outputs.all-features }} test --features=jack --workspace --verbose |
There was a problem hiding this comment.
Please keep this test to compile with all features, and instead update the CI to include PipeWire similar to what we're doing with PulseAudio now (see latest master).
Doing so please also revert the MSRV for unrelated hosts (ALSA, AAudio, WASIP1).
There was a problem hiding this comment.
It won't compile on cross, because the container of cross is ubuntu 20.04, which does not have pipewire. So I limited it to jack
There was a problem hiding this comment.
And I have said before, pipewire need cargo over 1.85? So I modified it, but seems now it is too high. I also tried to upgrade the edition once for it
There was a problem hiding this comment.
ok. I do not remember what trouble I met, and update the compiler. but seems now it all be good. Then it is fine, But I have said before. the cross-rs needs to be upgrade to ubuntu24.04, or it won't contain the package of pipewire. maybe I should also let it test PulseAudio
There was a problem hiding this comment.
In f2f92ad what we did to have separate MSRVs for ALSA, JACK and PulseAudio and work with --all-features. You could copy that pattern.
| ("playback", Role::Source) => DeviceDirection::Output, | ||
| ("capture", _) => DeviceDirection::Input, | ||
| // Bluetooth and other non-ALSA devices use generic port group | ||
| // names like "stream.0" — derive direction from media.class |
There was a problem hiding this comment.
That sounds smart - but it does not seem implemented?
src/host/pipewire/stream.rs
Outdated
| None, | ||
| pw::stream::StreamFlags::AUTOCONNECT | ||
| | pw::stream::StreamFlags::MAP_BUFFERS | ||
| | pw::stream::StreamFlags::RT_PROCESS, |
There was a problem hiding this comment.
Pipewire docs specify no allocations or file access should be done by any callback when RT_PROCESS is set. I don't think this is currently in the CPAL contract? Consider documenting this or making this a feature flag/or option for this specific host. (I do think RT_PROCESS is really valuable to have as an option).
For context Rodio currently does file access on in the callback. Though maybe it should not ... there is a lot it should not do :)
There was a problem hiding this comment.
ok, I will remove it and add a comment
There was a problem hiding this comment.
@Decodetalkers there's the feature audio_thread_priority that you could gate this option behind.
@yara-blue yeah... your session title "dragging a ten year old crate into 2026" is hitting harder the more we think about it 😉 ideally the decoders should work on another thread.
There was a problem hiding this comment.
audio_thread_priority is not in the features, it seems a bug.. Is it?
There was a problem hiding this comment.
Yes, probably lost during rebasing somewhere. Feel free to re-add to Cargo.toml:
# Audio thread priority elevation
# Raises the audio callback thread to real-time priority for lower latency and fewer glitches
# Requires: On Linux, either rtkit or appropriate user permissions (e.g. limits.conf or capabilities)
# Platform: Linux, DragonFly BSD, FreeBSD, NetBSD, Windows
audio_thread_priority = ["dep:audio_thread_priority"]There was a problem hiding this comment.
ok. seems there are already so many commits, I prefer add the feature of RT_PROCESS later in another pr. If rebase this branch, I may be hard to read the timeline
There was a problem hiding this comment.
I don't think this is currently in the CPAL contract?
Allocations and filesystem access shouldn't be done in any audio callback, regardless of the backend API or whether that's documented or not, or the priority of the thread.
b295297 to
a756e42
Compare
Have not solved all suggestions yet
a756e42 to
2ac2686
Compare
maybe add an option later
a1ce05f to
a2d9922
Compare
| (group::CAPTURE, _) => DeviceDirection::Input, | ||
| // Bluetooth and other non-ALSA devices use generic port group | ||
| // names like "stream.0" — derive direction from media.class | ||
| (_, Role::Sink) => DeviceDirection::Output, |
There was a problem hiding this comment.
@Frando I was wandering. I know sink can also be output, but why source is input here?
they were typos
it should return &[SampleRate]
We solve it after set the feature of pipewire to v0_3_49 and with the new function of request
Add support to pipewire
You can test it with pipewire feature open
Pipewire support use config to define rates. So the default config of cpal with pipewire can be changed through config like following.
Still problems left:*
once we do 'pipewire::init', we can only dequeue it after the whole thread. even put the function to another thread, the function still works.. But seems we can run init many times..(Ok, seems it is not a problem, because in when we call init, the init action will only be called once. I think it will be ok)*
The crates by pipewire need edition 2024. I think that should be another pr.