-
Notifications
You must be signed in to change notification settings - Fork 4
Implement buffer_size_range for pipewire, alsa #80
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: main
Are you sure you want to change the base?
Conversation
170943a to
a908a7e
Compare
5a00469 to
c6af26e
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.
I'm not sure exactly where the problem is yet but this does not seem to be working on my system for WASAPI on Windows. It's failing to build a default output stream so I'm guessing there's something wrong with the buffer size range function, however I haven't gotten to the bottom of it just yet.
|
@geom3trik Could you post some logs and the command of what you're running? |
Sure. if I try to run the new set_buffer_size example, I get the following panic: And if I try to run the sine_wave example, I get the following panic: |
|
I see could you run the |
Sure, that runs fine and gives the following output: |
|
@geom3trik Thank you for the logs, could you try the latest commit? |
|
This needs a little more work. On pipewire it appears to work but I should probably be using the pipewire api not the alsa passthrough on alsa it appears to do what it wants |
dd393cc to
8b2105b
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.
Pretty nice, thank you ! LGTM except for a few cosmetic changes in the code.
| buffer_size_range: (None, None), | ||
| buffer_size_range: self.buffer_size_range()?, |
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'd like to avoid doing an API lookup here, (None, None) already means "the entire supported buffer range". People that need the data can themselves call buffer_size_range() if they want to.
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.
This is also valid for the PipeWire and WASAPI backends
src/backends/wasapi/device.rs
Outdated
| unsafe { | ||
| // Get the mix format, now managed by our RAII wrapper. | ||
| let format_ptr = ComWaveFormat(audio_client.GetMixFormat()?); | ||
|
|
||
| let mut closest_match_ptr: *mut Audio::WAVEFORMATEX = ptr::null_mut(); | ||
| let res = audio_client.IsFormatSupported( | ||
| Audio::AUDCLNT_SHAREMODE_SHARED, | ||
| &*format_ptr.0, | ||
| Some(&mut closest_match_ptr), | ||
| ); | ||
|
|
||
| if res.is_ok() { | ||
| // The original format is supported. | ||
| return Ok(format_ptr.0.read_unaligned()); | ||
| } | ||
|
|
||
| // Wrap the returned suggestion in our RAII struct as well. | ||
| let closest_match = ComWaveFormat(closest_match_ptr); | ||
| if !closest_match.0.is_null() { | ||
| return Ok(closest_match.0.read_unaligned()); | ||
| } | ||
|
|
||
| res.ok()?; | ||
| unreachable!(); | ||
| } |
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.
| unsafe { | |
| // Get the mix format, now managed by our RAII wrapper. | |
| let format_ptr = ComWaveFormat(audio_client.GetMixFormat()?); | |
| let mut closest_match_ptr: *mut Audio::WAVEFORMATEX = ptr::null_mut(); | |
| let res = audio_client.IsFormatSupported( | |
| Audio::AUDCLNT_SHAREMODE_SHARED, | |
| &*format_ptr.0, | |
| Some(&mut closest_match_ptr), | |
| ); | |
| if res.is_ok() { | |
| // The original format is supported. | |
| return Ok(format_ptr.0.read_unaligned()); | |
| } | |
| // Wrap the returned suggestion in our RAII struct as well. | |
| let closest_match = ComWaveFormat(closest_match_ptr); | |
| if !closest_match.0.is_null() { | |
| return Ok(closest_match.0.read_unaligned()); | |
| } | |
| res.ok()?; | |
| unreachable!(); | |
| } | |
| unsafe { | |
| // Get the mix format, now managed by our RAII wrapper. | |
| let format_ptr = ComWaveFormat(audio_client.GetMixFormat()?); | |
| let mut closest_match = ComWaveFormat(ptr::null_mut()); | |
| let res = audio_client | |
| .IsFormatSupported( | |
| Audio::AUDCLNT_SHAREMODE_SHARED, | |
| &*format_ptr.0, | |
| Some(&mut closest_match.0), | |
| ) | |
| .ok(); | |
| match res { | |
| Ok(()) => Ok(format_ptr.0.read_unaligned()), | |
| Err(err) if closest_match.0.is_null() => { | |
| Err(error::WasapiError::FoundationError(err.to_string())) | |
| } | |
| Err(..) => Ok(closest_match.0.read_unaligned()), | |
| } | |
| } |
Using match simplifies the code a bit IMHO, and makes the flow clearer. Hopefully I haven't messed up and it should still have the same behavior.
Hey @SolarLiner sorry about this, I finally got to testing on Windows and it didn't work at all so I have made it a draft until I can confirm its behavior actually works on Windows. |
9807085 to
bf298d9
Compare
This provides the method to properly set the buffer size range for the pipewire and alsa backends, not just Core Audio.
Untested now.
Description
Please include a summary of the change and which issue is fixed. Include relevant motivation and context.
Fixes # (issue number)
Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests you added or ran to verify your changes. Provide instructions so we can reproduce. Try to run
on as many audio drivers as you can get your hands on.
Checklist:
need to be validated manually (i.e. a new audio driver), use examples that can be run to easily validate them.
Screenshots (if appropriate):
Additional Notes:
Add any additional notes about the PR here.