Defer EGLSurface creation to SDL_GL_CreateContext

[Bulmanator]

I am trying to use SDL2 to create my window on Wayland but I am only really using SDL to create the window and do input handling. My rendering system initialises EGL separately on its own, however (even if I don't specify SDL_WINDOW_OPENGL) SDL still creates an EGLSurface on window create. This causes eglCreateWindowSurface to fail later on with EGL_BAD_ALLOC because a surface has already been created.

The code currently creates the wl_egl_window and immediately follows up by creating the EGLSurface if EGL is supported. If at all possible I think the surface creation should be deferred until the user actually calls SDL_GL_CreateContext to allow people initialising EGL on their own to do so. If it is not possible to do this making the EGLSurface that has been created accessible would be nice.

The initialisation code can be found in the gist here.

[flibitijibibo]

As far as SDL's behavior is concerned I think we're actually stuck - it creates the EGL surface at CreateWindow for all backends as far as I know.

Kind of a bummer because adjusting this is easier than it sounds - in Wayland's case we would only need to move creation and destruction to the CreateContext/DestroyContext functions which can be found here. Both Create and Delete are already in place so I think this would be a cut-and-paste job.

There might be a legitimate reason we create the surface right at the start, either Ryan or Sam will know the answer to that.

[Bulmanator]

When looking at the code, I also thought it would be a relatively simple change. That said I didn't look at other platforms. In the case that the it is necessary to create the EGLSurface during CreateWindow maybe the created surface can be bundled in the SDL_SysWMinfo.info.wl struct to make it accessible? I know there is already a lot in there for Wayland, so this might not be ideal.

[1bsyl]

@Bulmanator
Not sure, but did you try to set SDL_VIDEO_EXTERNAL_CONTEXT hint ?
https://github.com/libsdl-org/SDL/blob/main/include/SDL_hints.h#L1379

[Bulmanator]

Hey, sorry for the slow reply. I tried setting that hint and it change the error from EGL_BAD_ALLOC to EGL_BAD_NATIVE_WINDOW. This is despite the egl_window pointer in the SDL_SysWMinfo struct seemingly being a valid pointer.

I also tried to directly call wl_egl_window_create using the surface provided in the WM info struct, but this also ended with the same EGL_BAD_NATIVE_WINDOW error even though that also returned a seemingly valid pointer.

[Bulmanator]

I have been doing some more experimenting with this, it turns out the EGL_BAD_NATIVE_WINDOW was being caused by the me passing EGL_DEFAULT_DISPLAY to the eglGetDisplay call which seemingly forces EGL into some weird X11 mode and ends up interpreting the native wl_egl_window as an X11 window. I didn't realise it required the display pointer. It fails with a call to xcb_get_geometry and reports an error "libEGL warning: DRI2: failed to create dri screen"

If I instead pass the display pointer found at SDL_SysWMinfo.info.wl.display it solves this issue, the EGL_BAD_ALLOC remains when using the wl_egl_window found at SDL_SysWMinfo.info.wl.egl_window
However, now if I directly call wl_egl_window_create with the wl_surface found at SDL_SysWMinfo.info.wl.surface and use this new EGL window to create an EGLSurface via eglCreateWindowSurface the EGL_BAD_ALLOC goes away and now everything works fine.

This is irrespective of whether the hint mentioned above is set as well.

I don't know how ideal this is as I think there will still be two wl_egl_window and EGLSurface allocations, but from my point of view I can now do what I needed to. This has also only been tested on official Nvidia 510.68.02 running swaywm 1.6.1 with SDL2 2.0.22

I will close this issue for now, however, if you feel there is more discussion required feel free to re-open it.

[belegdol]

Did you ever manage to figure out what is happening? I am seeing a similar problem with bgfx:
https://github.com/bkaradzic/bgfx/blob/master/examples/common/entry/entry_sdl.cpp#L53-L64
The above code works. If it is replaced with wmi.info.wl.egl_window, the vulkan renderer keeps working, but the egl one fails at
https://github.com/bkaradzic/bgfx/blob/master/src/glcontext_egl.cpp#L345
Inspecting the two pointers reveals that one from SDL_SysWMinfo has driver_private, resize_callback and destroy_window_callback defined whereas one created by wl_egl_window_create these members are NULL. The other difference is the size (?): SDL one is 0xd01de0 while the manually created one is 0x7fffbc01c140:


surface is the same for both which is likely why vulkan renderer is working as this is what it requires.

[Bulmanator]

I have just done a little more testing and have the same results as you with regard to the driver_private, resize_callback and destroy_window_callback.

It seems these members have valid values because SDL will create its own egl_window and then immediately after call eglCreateWindowSurface on that window, this fills out those members. It seems to be invalid to call eglCreateWindowSurface on an egl_window that has already had a window surface created for it (thus having those members filled out), this causes the EGL_BAD_ALLOC error. This can actually be seen in the mesa3d open source implementation here.

I can re-open this issue for you if it is causing you problems so they can take a look. As I mentioned in the original post, this should be a simple fix by delaying the call to eglCreateWindowSurface to the SDL_GL_CreateContext call rather than doing it during window initialisation, so people doing their own context creation can create their own surface with the already provided egl_window.

[slouken]

I don't think there's any reason we need it created immediately, I think it was just convenient. Feel free to create a PR that defers it as @flibitijibibo suggested.

[belegdol]

I think creating a PR is beyond my nearly nonexistent C skills. The following cut-and-paste patch fails to compile:

diff -up SDL2-2.26.5/src/video/wayland/SDL_waylandopengles.c.wls SDL2-2.26.5/src/video/wayland/SDL_waylandopengles.c
--- SDL2-2.26.5/src/video/wayland/SDL_waylandopengles.c.wls	2023-04-05 20:45:47.000000000 +0200
+++ SDL2-2.26.5/src/video/wayland/SDL_waylandopengles.c	2023-08-04 12:19:04.928410083 +0200
@@ -52,6 +52,16 @@ Wayland_GLES_LoadLibrary(_THIS, const ch
 SDL_GLContext
 Wayland_GLES_CreateContext(_THIS, SDL_Window * window)
 {
+    
+#if SDL_VIDEO_OPENGL_EGL
+    /* Create the GLES window surface */
+    data->egl_surface = SDL_EGL_CreateSurface(_this, (NativeWindowType) data->egl_window);
+
+    if (data->egl_surface == EGL_NO_SURFACE) {
+        return -1;    /* SDL_EGL_CreateSurface should have set error */
+    }
+#endif
+
     SDL_GLContext context;
     context = SDL_EGL_CreateContext(_this, ((SDL_WindowData *) window->driverdata)->egl_surface);
     WAYLAND_wl_display_flush( ((SDL_VideoData*)_this->driverdata)->display );
@@ -201,6 +211,11 @@ void
 Wayland_GLES_DeleteContext(_THIS, SDL_GLContext context)
 {
     SDL_EGL_DeleteContext(_this, context);
+#if SDL_VIDEO_OPENGL_EGL
+    if (wind->egl_surface) {
+        SDL_EGL_DestroySurface(_this, wind->egl_surface);
+    }
+#endif
     WAYLAND_wl_display_flush( ((SDL_VideoData*)_this->driverdata)->display );
 }
 
diff -up SDL2-2.26.5/src/video/wayland/SDL_waylandwindow.c.wls SDL2-2.26.5/src/video/wayland/SDL_waylandwindow.c
--- SDL2-2.26.5/src/video/wayland/SDL_waylandwindow.c.wls	2023-04-05 20:45:47.000000000 +0200
+++ SDL2-2.26.5/src/video/wayland/SDL_waylandwindow.c	2023-08-04 12:18:48.403316103 +0200
@@ -2035,15 +2035,6 @@ int Wayland_CreateWindow(_THIS, SDL_Wind
 
     if (window->flags & SDL_WINDOW_OPENGL) {
         data->egl_window = WAYLAND_wl_egl_window_create(data->surface, data->drawable_width, data->drawable_height);
-
-#if SDL_VIDEO_OPENGL_EGL
-        /* Create the GLES window surface */
-        data->egl_surface = SDL_EGL_CreateSurface(_this, (NativeWindowType) data->egl_window);
-
-        if (data->egl_surface == EGL_NO_SURFACE) {
-            return -1;    /* SDL_EGL_CreateSurface should have set error */
-        }
-#endif
     }
 
 #ifdef SDL_VIDEO_DRIVER_WAYLAND_QT_TOUCH
@@ -2230,11 +2221,6 @@ void Wayland_DestroyWindow(_THIS, SDL_Wi
     SDL_WindowData *wind = window->driverdata;
 
     if (data) {
-#if SDL_VIDEO_OPENGL_EGL
-        if (wind->egl_surface) {
-            SDL_EGL_DestroySurface(_this, wind->egl_surface);
-        }
-#endif
         if (wind->egl_window) {
             WAYLAND_wl_egl_window_destroy(wind->egl_window);
         }

[Kontrabant]

This was addressed in SDL3 via commit 9ab2025. The Wayland backend was forcing OpenGL on and creating an EGL window since the initial commit almost 10 years ago, but there's really no reason to create an EGL window when the application doesn't ask for SDL to handle OpenGL, as in the case of BGFX. We can just cherry-pick this commit to SDL2, and it should fix the issue.

[belegdol]

With commit 9ab2025 cherry-picked, wmi.info.wl.egl_window is NULL. Is there a way of having it defined by SDL and also usable by bgfx? This would simplify the code in https://github.com/bkaradzic/bgfx/blob/master/examples/common/entry/entry_sdl.cpp#L53-L64 significantly.

[Kontrabant]

I don't particularly like the idea of creating an EGL window if the application didn't ask for an OpenGL window. If the application wants a bare window, as BGFX does, it's basically stating that it wants the minimum and will handle anything beyond that itself.

[belegdol]

Fair enough. With egl_window being NULL at least there is no confusion why it is there but not working.

[slouken]

I went ahead and cherry-picked this change to SDL2 (but it shouldn't go into release-2.28.x)