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

Fix Dummy audio driver initialization issue on WASAPI output device initialization failure #87010

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

alessandrofama
Copy link
Contributor

AudioDriverWASAPI::init consistently returns Error::OK, even when encountering a failure during the initialization of the output device. This behaviour blocks the dummy driver from initializing in AudioDriverManager::initialize. Fixes #71640.

@alessandrofama alessandrofama requested a review from a team as a code owner January 9, 2024 15:44
@AThousandShips AThousandShips added this to the 4.3 milestone Jan 9, 2024
@alessandrofama alessandrofama force-pushed the wasapi-failed-init branch 2 times, most recently from 516a120 to 9b3cad9 Compare January 9, 2024 15:53
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

Makes sense otherwise, and other drivers exit this way as well

if (err != OK) {
ERR_PRINT("WASAPI: init_output_device error");
}
ERR_FAIL_COND_V_MSG(err != OK, err, "WASAPI: init_output_device error.");

exit_thread.clear();
Copy link
Member

Choose a reason for hiding this comment

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

Unsure if this should be moved up, it is above in the alsa code and could cause threading problems if not fired with this change, it's put at the top in some cases, I don't know what the exact structure should be for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see it's above in the output initialisation of the alsa & pulseaudio drivers. The flag is only set in the thread entry point and in AudioDriverWASAPI::finish. Not sure about the structure too. Could be moved up for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved it up

…nitialization failure

`AudioDriverWASAPI::init` consistently returns `Error::OK`, even when encountering a failure during the initialization of the output device. This behaviour blocks the dummy driver from initializing in `AudioDriverManager::initialize`.
@akien-mga akien-mga merged commit aa07403 into godotengine:master Jan 9, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga added cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Jan 9, 2024
@alessandrofama alessandrofama deleted the wasapi-failed-init branch January 9, 2024 17:02
@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 16, 2024
@akien-mga
Copy link
Member

The original bug report was against 3.5.1, but this PR doesn't seem applicable to the 3.x code, which currently looks like this:

Error AudioDriverWASAPI::init() {
        mix_rate = GLOBAL_GET("audio/mix_rate");

        Error err = init_render_device();
        if (err != OK) {
                ERR_PRINT("WASAPI: init_render_device error");
        }

        exit_thread.clear();

        thread.start(thread_func, this);

        return OK;
}

@alessandrofama Would you be up for checking whether the issue is still reproducible in 3.x and thus how to solve it there?

@akien-mga akien-mga removed cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Jan 16, 2024
@alessandrofama
Copy link
Contributor Author

@alessandrofama Would you be up for checking whether the issue is still reproducible in 3.x and thus how to solve it there?

Yes, it was reproducible. Created a PR :)

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 24, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 24, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AudioStreamPlayer "finished" signal is never emitted if there is no audio output device
4 participants