False alarm from SDL_CreateWindow when using Windows with ANGLE.

[hanseuljun]

First, thanks for maintaining this wonderful library.

I was trying to use SDL with ANGLE (the Google GLES one), and found that setting SDL_SetHint(SDL_HINT_OPENGL_ES_DRIVER, "1");

leads to a false error in SDL_CreateWindow at https://github.com/libsdl-org/SDL/blob/bd06538778102f72bad8393ef07da5a1ec444217/src/video/windows/SDL_windowsopengles.c#L111

What the above hint does is prevent SDL from using the default driver, from NVidia for AMD, so it would use the GL Driver of ANGLE. Based on the name of variable--gl_config.driver_loaded--I think the problem is, this hint makes SDL think that there is no GL driver loaded, while having access to the ANGLE's driver.

Based on this observation, I can think of two fixes.

1. A quick fix would be removing the SDL_assert line since this is not a condition that should be avoided based on the current situation.
2. The other (perhaps more proper) one would be setting gl_config.driver_loaded to 1 or something else when SDL_HINT_OPENGL_ES_DRIVER is set to 1.

I'll submit a pull request based on option 1.

Attaching the code using SDL and ANGLE below. hello_windows.cpp.txt

[icculus]

I think @hanseuljun's patch is good for 2.0.16, but I'd like to spend some time looking at why this flag doesn't get set correctly, triggering the assertion, before 2.0.18. It might be simple, I haven't looked further yet.

[hanseuljun]

Thanks a lot for planning to take a look at this issue!

Probably due to the same reason, this issue was also found in mac. I have submitted another PR #5086.

[flibitijibibo]

This is probably out of my range (unless someone has a full-binary sample I can run on Windows or macOS with dylibs?), but I did a quick search for driver_loaded to see if anything obvious showed up, and I did notice that there's an inconsistency with how the variable is set... some places just set it to 1 or 0, while other places do ++/-- which seems more correct. Maybe the two different types of assignment collide in some unusual way?

Some notes for anyone who doesn't want to git grep...

• SDL_video.c just sets it to 0 on startup, increments on LoadLibrary, decrements on UnloadLibrary. There is a special case for when the library path is explicitly passed, where this increment does NOT go past 1 (but the decrement does always occur)
• SDL_egl.c hard-sets it to 0 if eglGetDisplay or eglInitialize fails in SDL_EGL_LoadLibrary, it does NOT assign it anywhere else in this file
• SDL_windowsopengl.c and SDL_x11opengl.c do a quick increment and decrement to call GL_InitExtensions, it's got a massive comment describing the hack in the Windows version
• SDL_windowsopengles.c and SDL_cocoaopengles.m directly set it to 1 in SetupWindow

I'm inclined to say the last bullet may be an issue if the setup phase does multiple loads, but it also could be a genuine issue where it loads twice and unloads once. The number is thankfully very easy to just printf spam if you have a (non-)working test.

[icculus]

Going to do this one in 2.26 as early-in-milestone, so I don't accidentally break OpenGL on a random platform at the last moment.

[icculus]

Taking a stab at this now. My goal is going to be to move driver_loaded management to the upper level, and make it increment/decrement, and only load/unload things when it increments to 1/decrements to zero.

And remove all the asserts, because we won't be touching this variable all over creation to need to assert its validity.

This might be folly, I'll report back shortly, once I start building up a patch.

[icculus]

Interestingly, SDL_GL_LoadLibrary isn't interested in reference counting this:

     if (_this->gl_config.driver_loaded) {
        if (path && SDL_strcmp(path, _this->gl_config.driver_path) != 0) {
            return SDL_SetError("OpenGL library already loaded");
        }
        retval = 0;

It's either loaded or it isn't and you can't pair loads and unloads (multiple load attempts are washed away by a single unload.

More worrisome, though, is that SDL_GL_UnloadLibrary() does reference count it:

    if (_this->gl_config.driver_loaded > 0) {
        if (--_this->gl_config.driver_loaded > 0) {
            return;
        }
        if (_this->GL_UnloadLibrary) {
            _this->GL_UnloadLibrary(_this);
        }
    }

And more worrisome still ... if you call SDL_DestroyWindow() on a window with SDL_WINDOW_OPENGL, it will call SDL_GL_UnloadLibrary():

    if (window->flags & SDL_WINDOW_OPENGL) {
        SDL_GL_UnloadLibrary();
    }

So it looks like creating two OpenGL windows and then destroying one will unload the GL while you're still using it...? I haven't checked this, just looking through the code.

[icculus]

So it looks like creating two OpenGL windows and then destroying one will unload the GL while you're still using it...?

Just tried testgl2 --windows 2 and it's getting incremented somewhere else for each of these, so it's not a super-urgent bug, but I'm still going to adjust SDL_GL_LoadLibrary to reference count as I unify all this stuff.

[icculus]

Interestingly, SDL_GL_LoadLibrary isn't interested in reference counting this:

Read this incorrectly, it increments below this if retval == 0.

[icculus]

Started working on cleaning this up over in https://github.com/icculus/SDL/tree/gl-driver-loaded-rework

This doesn't even compile yet, so this isn't in a pull request at the moment. It's a surprising amount to untangle!

[icculus]

Inclined to say we should bump this from this milestone; the patch is making progress, but it touches a lot of code, and feels risky at this point for 2.26.0

[slouken]

Agreed.