Fix crashing on startup with AMD GPUs #949
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…xt_win. This change does not modify behavior. It just introduces a helper class to make it easier to manage the creation of temporary contexts, making them current, and then cleaning them up when no longer needed.
This change makes sure there is an active context when GetGPUInfoAMD() is called. While it is not explicitly stated in https://www.opengl.org/registry/specs/AMD/wgl_gpu_association.txt that a GL context needs to be current, it is somewhat implied by the fact that these extension methods are fetched via wglGetProcAddress(). According to the Windows docs, function addresses are specific to a context so using them without an active context seems to be undefined territory.
|
Awesome 👍 (not tested) I have one or two Radeon GPUs somewhere I can test. |
devernay
approved these changes
Mar 12, 2024
10 tasks
10 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. Additionally, make sure you've done all of these things:
PR Description
What type of PR is this? (Check one of the boxes below)
What does this pull request do?
This change fixes the crashes on startup when running Natron on Windows with a AMD GPU. The fix primarily just moves GL context creation earlier in the AMD logic so that we have an active context when the AMD extension functions are called.
Have you tested your changes (if applicable)? If so, how?
Yes. I tested this on a machine with an AMD GPU and verified that the old code crashed, and this new logic does not. I also verified that the code still works on a Windows machine with an NVIDIA GPU and a different machine with an Intel integrated GPU. All of these appear to work fine.
Futher details of this pull request
While it seems bad form that the AMD driver crashes the process when there was a null context, I do believe Natron's code was incorrect. Functions returned from wglGetProcAddress() are technically specific to the context that is current when the method is called. Different contexts can return different functions according to the Windows docs, so calling one of these functions without an active context seems to be in undefine behavior territory. I believe that the caching of extension pointers in the OSGLContext_wgl_data struct and reusing them for all contexts is in undefined behavior territory as well, but it seems to work. I've left fixing that out of this PR for now since the focus of this change is simply to fix the crashes.