Skip to content
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

Implement BufferSize::Fixed in wasapi, and migrate wasapi to officially-supported Windows crate #669

Merged
merged 14 commits into from
Jul 15, 2022

Conversation

Osspial
Copy link
Contributor

@Osspial Osspial commented Jun 3, 2022

I migrated to windows-rs in this PR since exposing the buffer size range for hardware-offloaded audio processing requires IAudioClient2, which isn't exposed in the winapi crate, and winapi seems to be more or less unmaintained at this point.

Regarding fixed buffer sizes - wasapi doesn't seem to support requesting exact audio buffer sizes, but will always get an audio buffer that is at least the size requested. Should that be mentioned in the documentation? Also, the buffer exposed to the data_callback doesn't always match the size of the buffer requested, since wasapi incrementally updates the buffer as audio gets played, instead of updating it all at once once the buffer is cleared. I'm not sure if that's standard behavior or not but it may also be worth documenting.

Related to #544, #534, #667, and #616. @est31, you'd mentioned at #616 (comment) that windows-rs had some issues with cross-compilation, but from what I can tell from skimming their issue tracker the cross-compilation issues have been addressed. Are there other issues there that are unresolved?

@est31
Copy link
Member

est31 commented Jun 3, 2022

I haven't tried cross compiling to windows yet, but looking at windows-rs CI there seems to be no cross compilation job.

Their contributing guide doesn't mention targetting the windows platform from Linux either.

I don't know whether they really support that feature with the love it deserves.

@est31
Copy link
Member

est31 commented Jun 3, 2022

I've filed an issue, let's see how they react: microsoft/windows-rs#1799

@est31
Copy link
Member

est31 commented Jun 9, 2022

Okay, I guess we can switch to windows-rs. If there's an issue we can always revert the switch. There are some conflicts for the PR.

@Osspial
Copy link
Contributor Author

Osspial commented Jun 9, 2022

Merge conflicts have been resolved.

@est31
Copy link
Member

est31 commented Jun 9, 2022

Can you resolve them via rebase instead? It would be a pity to squash this.

@est31
Copy link
Member

est31 commented Jun 9, 2022

Wait no sorry disregard, it's a history that needs squashing anyways I think.

@Osspial
Copy link
Contributor Author

Osspial commented Jun 16, 2022

Does this need any more revisions, or is it good to merge?

@est31
Copy link
Member

est31 commented Jun 17, 2022

Stuff like this makes me worried tbh: dotnet/vscode-csharp#5276

@Osspial
Copy link
Contributor Author

Osspial commented Jun 17, 2022

That's... fair! But I think this case is materially different - if this PR were pulling in a more substantial Microsoft product I'd definitely share your concern, but they've consistently provided open access to the Windows C/C++ headers for multiple decades, even during the peak of their anti-competitive practices in the 2000s. The windows crate is more or less an official extension of that into the Rust ecosystem, and even with an exceedingly uncharitable view of Microsoft it doesn't square with their past behavior regarding the C headers for them to lock that down.

Copy link
Member

@est31 est31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good, thank you!

src/host/wasapi/device.rs Outdated Show resolved Hide resolved
src/host/wasapi/device.rs Outdated Show resolved Hide resolved
@est31
Copy link
Member

est31 commented Jun 18, 2022

@Osspial you are making good points. Let's give them a chance. If they annoy folks enough we can always switch back.

@Osspial
Copy link
Contributor Author

Osspial commented Jul 13, 2022

Is this good to merge?

@est31
Copy link
Member

est31 commented Jul 13, 2022

@Osspial can you resolve the merge conflicts? After that it should be good to merge.

@Osspial
Copy link
Contributor Author

Osspial commented Jul 15, 2022

I'm not seeing any merge conflicts on my end.

@est31 est31 merged commit 417b7ff into RustAudio:master Jul 15, 2022
@est31
Copy link
Member

est31 commented Jul 15, 2022

Ah it was because I couldn't rebase merge. Squash merging works. I've merged it!

Hoodad pushed a commit to EmbarkStudios/cpal that referenced this pull request Feb 8, 2023
…ly-supported Windows crate (RustAudio#669)

I migrated to `windows-rs` in this PR since exposing the buffer size range for hardware-offloaded audio processing requires `IAudioClient2`, which isn't exposed in the `winapi` crate, and `winapi` seems to be more or less unmaintained at this point.

* Basic type conversion

* Make everything compile!

* Fix warnings

* Implement buffer sizing for wasapi

* Convert tabs to spaces

* Run cargo fmt

* Remove winapi from Cargo.toml

* Remove unused commented code from wasapi, and fix lingering TODOs

* Fix compile error

* Update to windows 0.37.0

* Run rustfmt

* Work around rustfmt nested tuple issues

* Improve readability of guid comparison code
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.

2 participants