NatronGitHub / Natron

Open-source video compositing software. Node-graph based. Similar in functionalities to Adobe After Effects and Nuke by The Foundry.
http://NatronGitHub.github.io
GNU General Public License v2.0
4.53k stars 330 forks source link

Fix crashing on startup with AMD GPUs #949

Closed acolwell closed 3 months ago

acolwell commented 3 months ago

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.

rodlie commented 3 months ago

Awesome :+1: (not tested)

I have one or two Radeon GPUs somewhere I can test.