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.

Sign up for GitHub

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

Jump to bottom

Fix audio driver wasapi audio input handling #75628

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix audio driver wasapi audio input handling #75628

wants to merge 3 commits into from
+32 −30

Conversation

lazydevl0per
Copy link

@lazydevl0per lazydevl0per commented Apr 3, 2023 •
edited by AThousandShips

@lazydevl0per lazydevl0per requested a review from a team as a code owner April 3, 2023 21:25
@Calinou Calinou added this to the 4.1 milestone Apr 3, 2023
@MJacred MJacred mentioned this pull request May 17, 2023
Open
@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 19, 2023
@kevinloustau
Copy link

is this PR need tests before to be merge?

@ellenhp
Copy link
Contributor

ellenhp commented Oct 13, 2023 •
edited

Unfortunately I don't think there aren't very many consistent audio contributors who run Windows so historically WASAPI driver changes have been hard to review. So yeah, it would be good if a few people could provide before and after test results in as many different configurations as possible (different mix rates at the operating system level if possible, ideally different kinds of input/output devices like integrated and usb sound cards). Also for my benefit since I can't test this personally it would be really wonderful there was a more detailed description of the change in the PR description, and details about why you implemented things in the way that you did. Ideally also links to documentation when relevant.

My hesitation comes mostly from the fact that there are already WASAPI driver bugs that only affect a very small percentage of users, and I don't think anybody knows what's causing them or how to fix them. I don't want to create more bugs like that.

@MJacred I saw you commented on the issue this fixes. If this issue is affecting you it might be good to take a look :)

I should also say that I'm too nervous about approving WASAPI driver changes to make a final call and I'm probably going to ask that Juan review it if people provide positive test results and I decide things make sense after cross-referencing all the changes with MSDN. I know that's going to be a lot of work but I want to be pretty sure it's ready before asking him for a review.

@lazydevl0per
Copy link
Author

lazydevl0per commented Oct 13, 2023 •
edited

Its been a while since this PR was opened so I need a little time to assess everything but I`ll be checking if this is still needed/relevant/correct asf...

EDIT:
The problem described in #75603 still persists on latest master branch
This PR still resolves the issue #75603 on latest master branch

@ellenhp i found the unittests and got them running aswell.
Currently since the issue is with an OS specific driver implementation on the Godot side its a bit difficult to write tests for lower level API. Also in the current configuration the AudioServer singleton during the unittests is using the dummy AudioDriver which only provides a single output and input device called "Default" which is also hardcoded and not changeable from the outside so writing testcases for this specific situation (where a person switches an input device) seems impossible and not relevant since they wouldnt be testing the WASAPI implementation.

If there is a way to make certain tests build and run ONLY on windows and if there is a way to force the tests to use the WASAPI driver then I will add them until then i will read up on the "scons" build system since I have been mainly using CMake and dont know alot about the former.

AThousandShips
AThousandShips reviewed Oct 16, 2023
View reviewed changes
drivers/wasapi/audio_driver_wasapi.cpp
default_input_device_changed = false;
}
// If we're using the Default input device and it changed finish it so we'll re-init the input device
if (ad->audio_input.device_name == "Default" && default_input_device_changed) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (ad->audio_input.device_name == "Default" && default_input_device_changed) {
if (default_input_device_changed && ad->audio_input.device_name == "Default") {

Cheaper condition first

drivers/wasapi/audio_driver_wasapi.cpp
if (err != OK) {
ERR_PRINT("WASAPI: finish_input_device error");
}
bool reinitDevice = false;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bool reinitDevice = false;
bool reinit_device = false;

Use snake_case

@MJacred
Copy link
Contributor

MJacred commented Oct 16, 2023

@ellenhp: Thanks for thinking of me! Though I only commented there because I knew that Godot 3 was affected as well due to me maintaining the Audio Issue Tracker and I'm currently not affected by this

@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Nov 13, 2023
@lazydevl0per
Copy link
Author

I Relinquish control, sadly i have no idea anymore :(
Sorry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips AThousandShips left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
bug platform:windows topic:audio
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

Cannot switch Audio input device (AudioServer.input_device)
8 participants
@lazydevl0per @kevinloustau @ellenhp @MJacred @AThousandShips @Calinou @akien-mga @YuriSizov