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

[raudio] Fix crash when switching playback device #4102

Merged
merged 1 commit into from
Jul 13, 2024

Conversation

jkaup
Copy link
Contributor

@jkaup jkaup commented Jun 24, 2024

Resolves #3743

@raysan5
Copy link
Owner

raysan5 commented Jun 24, 2024

@jkaup those reviews should be pushed to miniaudio repo, not raylib, but probably @mackron is already aware of that potential issue.

@raysan5 raysan5 changed the title Fix crash when switching playback device [raudio] Fix crash when switching playback device Jun 25, 2024
@karl-zylinski
Copy link
Contributor

I put in two comments with what I did when I fixed this myself.

  1. I fetch the return value of ma_CoInitializeEx and store in a HRESULT and then check that one before uniniting. I do not know if this is needed.

  2. Because (1) uses windows specific stuff I also added the MA_WIN32 things checks. The crashes only happen for me on Windows anyways, so maybe best to only do these workaround there?

Like I said, I'm not sure if you need to do those extra things I did. Other then that it looks like the same thing I did to fix the crashes.

@veins1
Copy link
Contributor

veins1 commented Jul 5, 2024

@karl-zylinski As per MSDN (https://learn.microsoft.com/en-us/windows/desktop/api/combaseapi/nf-combaseapi-couninitialize):

To close the COM library gracefully on a thread, each successful call to CoInitialize or CoInitializeEx, including any call that returns S_FALSE, must be balanced by a corresponding call to [CoUninitialize].

You don't really have to store HRESULT as it should always be successful, meaning you should always call CoUninitialize. (It can't return RPC_E_CHANGED_MODE since we always pass a constant COINIT value).
And you also don't have to add MA_WIN32 checks since we are dealing with WASAPI which is Windows only.

@raysan5 On my initial investigation of this issue (link) the only place that required modification is ma_context_get_MMDevice__wasapi function, which solved the issue completely.

@raysan5
Copy link
Owner

raysan5 commented Jul 5, 2024

@veins1 Nice! Thanks for reporting!

@jkaup Please, could you update this PR as per the latest comment? It seems only ma_context_get_MMDevice__wasapi() should be modified.

@mackron
Copy link
Contributor

mackron commented Jul 5, 2024

Thanks team, and sorry for my silence.

When the device changes, WASAPI fires a callback from a separate thread. What I suspect is happening is that COM is not being initialized on that callback thread which is then resulting in this issue. If the COM initialization was moved to the ma_IMMNotificationClient_* callbacks I suspect that would also address the issue. However, since multiple people have confirmed the proposed fix works, and I'm planning on refactoring the WASAPI backend anyway, I'm happy enough to get that merged upstream. It would be good if @veins1 suggestion could be confirmed though just to reduce the amount of COM sitting around in the code base.

@jkaup
Copy link
Contributor Author

jkaup commented Jul 5, 2024

Thanks for the input on the issue @veins1 and @mackron. I will make the changes as proposed by @raysan5 as soon as possible.

@jkaup
Copy link
Contributor Author

jkaup commented Jul 13, 2024

Hi @raysan5 , sorry, got back to work today at last. I've updated the PR so that the proposed fix only affects ma_context_get_MMDevice__wasapi(), and have confirmed that this is enough to fix the crash as noted by @veins1 .

@raysan5 raysan5 merged commit 5ede476 into raysan5:master Jul 13, 2024
@raysan5
Copy link
Owner

raysan5 commented Jul 13, 2024

@jkaup nice! thanks for the review! 👍😄

@mackron
Copy link
Contributor

mackron commented Jul 13, 2024

@jkaup Thanks a lot for this. @raysan5 This has been integrated upstream in the dev branch.

@raysan5
Copy link
Owner

raysan5 commented Jul 13, 2024

@mackron that's great! planning to update miniaudio library soon! 👍😄

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.

[raudio] Crash when changing playback device
5 participants