From c54c16d353f206162cdbb125c0847b4645e096a3 Mon Sep 17 00:00:00 2001 From: "Ryan C. Gordon" Date: Fri, 30 Apr 2021 13:19:36 -0400 Subject: [PATCH] wayland: don't hang in SDL_GL_SwapBuffers if the compositor is ghosting us. If you hide a window on Mutter, for example, the compositor never requests new frames, which will cause Mesa to block forever in eglSwapBuffers to satisfy the swap interval. We now always set the swap interval to 0 and manage this ourselves, handing the frame to Wayland when it requests a new one, and timing out at 10fps just to keep apps moving if the compositor wants no frames at all. My understanding is that other protocols are coming that might improve upon this solution, but for now it solves the total hang. Fixes #4335. --- include/SDL_video.h | 2 +- src/video/wayland/SDL_waylandopengles.c | 77 +++++++++++++++++++++++-- src/video/wayland/SDL_waylandopengles.h | 4 +- src/video/wayland/SDL_waylandwindow.c | 19 ++++++ src/video/wayland/SDL_waylandwindow.h | 2 + 5 files changed, 97 insertions(+), 7 deletions(-) diff --git a/include/SDL_video.h b/include/SDL_video.h index 65fd6437e..41c3e10b0 100644 --- a/include/SDL_video.h +++ b/include/SDL_video.h @@ -1795,7 +1795,7 @@ extern DECLSPEC void SDLCALL SDL_GL_GetDrawableSize(SDL_Window * window, int *w, * vsync. Adaptive vsync works the same as vsync, but if you've already missed * the vertical retrace for a given frame, it swaps buffers immediately, which * might be less jarring for the user during occasional framerate drops. If - * application requests adaptive vsync and the system does not support it, + * an application requests adaptive vsync and the system does not support it, * this function will fail and return -1. In such a case, you should probably * retry the call with 1 for the interval. * diff --git a/src/video/wayland/SDL_waylandopengles.c b/src/video/wayland/SDL_waylandopengles.c index 5da365c82..50f336854 100644 --- a/src/video/wayland/SDL_waylandopengles.c +++ b/src/video/wayland/SDL_waylandopengles.c @@ -22,6 +22,8 @@ #if SDL_VIDEO_DRIVER_WAYLAND && SDL_VIDEO_OPENGL_EGL +#include "SDL_timer.h" +#include "../../core/unix/SDL_poll.h" #include "../SDL_sysvideo.h" #include "../../events/SDL_windowevents_c.h" #include "SDL_waylandvideo.h" @@ -55,17 +57,82 @@ Wayland_GLES_CreateContext(_THIS, SDL_Window * window) SDL_GLContext context; context = SDL_EGL_CreateContext(_this, ((SDL_WindowData *) window->driverdata)->egl_surface); WAYLAND_wl_display_flush( ((SDL_VideoData*)_this->driverdata)->display ); - return context; } +/* Wayland wants to tell you when to provide new frames, and if you have a non-zero + swap interval, Mesa will block until a callback tells it to do so. On some + compositors, they might decide that a minimized window _never_ gets a callback, + which causes apps to hang during swapping forever. So we always set the official + eglSwapInterval to zero to avoid blocking inside EGL, and manage this ourselves. + If a swap blocks for too long waiting on a callback, we just go on, under the + assumption the frame will be wasted, but this is better than freezing the app. + I frown upon platforms that dictate this sort of control inversion (the callback + is intended for _rendering_, not stalling until vsync), but we can work around + this for now. --ryan. */ +/* Addendum: several recent APIs demand this sort of control inversion: Emscripten, + libretro, Wayland, probably others...it feels like we're eventually going to have + to give in with a future SDL API revision, since we can bend the other APIs to + this style, but this style is much harder to bend the other way. :/ */ +int +Wayland_GLES_SetSwapInterval(_THIS, int interval) +{ + if (!_this->egl_data) { + return SDL_SetError("EGL not initialized"); + } + + /* technically, this is _all_ adaptive vsync (-1), because we can't + actually wait for the _next_ vsync if you set 1, but things that + request 1 probably won't care _that_ much. I hope. No matter what + you do, though, you never see tearing on Wayland. */ + if (interval > 1) { + interval = 1; + } else if (interval < -1) { + interval = -1; + } + + /* !!! FIXME: technically, this should be per-context, right? */ + _this->egl_data->egl_swapinterval = interval; + _this->egl_data->eglSwapInterval(_this->egl_data->egl_display, 0); + return 0; +} + +int +Wayland_GLES_GetSwapInterval(_THIS) +{ + if (!_this->egl_data) { + SDL_SetError("EGL not initialized"); + return 0; + } + + return _this->egl_data->egl_swapinterval; +} + int Wayland_GLES_SwapWindow(_THIS, SDL_Window *window) { SDL_WindowData *data = (SDL_WindowData *) window->driverdata; + const int swap_interval = _this->egl_data->egl_swapinterval; - if (SDL_EGL_SwapBuffers(_this, data->egl_surface) < 0) { - return -1; + /* Control swap interval ourselves. See comments on Wayland_GLES_SetSwapInterval */ + if (swap_interval != 0) { + const Uint32 max_wait = SDL_GetTicks() + 100; /* ~10 fps, so we'll progress even if throttled to zero. */ + struct wl_display *display = ((SDL_VideoData *)_this->driverdata)->display; + while ((SDL_AtomicGet(&data->swap_interval_ready) == 0) && (!SDL_TICKS_PASSED(SDL_GetTicks(), max_wait))) { + /* !!! FIXME: this is just the crucial piece of Wayland_PumpEvents */ + WAYLAND_wl_display_flush(display); + if (SDL_IOReady(WAYLAND_wl_display_get_fd(display), SDL_FALSE, 0)) { + WAYLAND_wl_display_dispatch(display); + } else { + WAYLAND_wl_display_dispatch_pending(display); + } + } + SDL_AtomicSet(&data->swap_interval_ready, 0); + } + + /* Feed the frame to Wayland. This will set it so the wl_surface_frame callback can fire again. */ + if (!_this->egl_data->eglSwapBuffers(_this->egl_data->egl_display, data->egl_surface)) { + return SDL_EGL_SetError("unable to show color buffer in an OS-native window", "eglSwapBuffers"); } // Wayland-EGL forbids drawing calls in-between SwapBuffers and wl_egl_window_resize @@ -89,7 +156,9 @@ Wayland_GLES_MakeCurrent(_THIS, SDL_Window * window, SDL_GLContext context) } WAYLAND_wl_display_flush( ((SDL_VideoData*)_this->driverdata)->display ); - + + _this->egl_data->eglSwapInterval(_this->egl_data->egl_display, 0); /* see comments on Wayland_GLES_SetSwapInterval. */ + return ret; } diff --git a/src/video/wayland/SDL_waylandopengles.h b/src/video/wayland/SDL_waylandopengles.h index cfa72e61f..0dfe512d6 100644 --- a/src/video/wayland/SDL_waylandopengles.h +++ b/src/video/wayland/SDL_waylandopengles.h @@ -35,11 +35,11 @@ typedef struct SDL_PrivateGLESData #define Wayland_GLES_GetAttribute SDL_EGL_GetAttribute #define Wayland_GLES_GetProcAddress SDL_EGL_GetProcAddress #define Wayland_GLES_UnloadLibrary SDL_EGL_UnloadLibrary -#define Wayland_GLES_SetSwapInterval SDL_EGL_SetSwapInterval -#define Wayland_GLES_GetSwapInterval SDL_EGL_GetSwapInterval extern int Wayland_GLES_LoadLibrary(_THIS, const char *path); extern SDL_GLContext Wayland_GLES_CreateContext(_THIS, SDL_Window * window); +extern int Wayland_GLES_SetSwapInterval(_THIS, int interval); +extern int Wayland_GLES_GetSwapInterval(_THIS); extern int Wayland_GLES_SwapWindow(_THIS, SDL_Window * window); extern int Wayland_GLES_MakeCurrent(_THIS, SDL_Window * window, SDL_GLContext context); extern void Wayland_GLES_GetDrawableSize(_THIS, SDL_Window * window, int * w, int * h); diff --git a/src/video/wayland/SDL_waylandwindow.c b/src/video/wayland/SDL_waylandwindow.c index c06616bd7..50b16d12f 100644 --- a/src/video/wayland/SDL_waylandwindow.c +++ b/src/video/wayland/SDL_waylandwindow.c @@ -199,6 +199,22 @@ static const struct wl_shell_surface_listener shell_surface_listener_wl = { }; +static const struct wl_callback_listener surface_frame_listener; + +static void +handle_surface_frame_done(void *data, struct wl_callback *cb, uint32_t time) +{ + SDL_WindowData *wind = (SDL_WindowData *) data; + SDL_AtomicSet(&wind->swap_interval_ready, 1); /* mark window as ready to present again. */ + + /* reset this callback to fire again once a new frame was presented and compositor wants the next one. */ + wl_callback_destroy(cb); + wl_callback_add_listener(wl_surface_frame(wind->surface), &surface_frame_listener, data); +} + +static const struct wl_callback_listener surface_frame_listener = { + handle_surface_frame_done +}; static void @@ -996,6 +1012,9 @@ int Wayland_CreateWindow(_THIS, SDL_Window *window) wl_compositor_create_surface(c->compositor); wl_surface_add_listener(data->surface, &surface_listener, data); + /* fire a callback when the compositor wants a new frame rendered. */ + wl_callback_add_listener(wl_surface_frame(data->surface), &surface_frame_listener, data); + #ifdef SDL_VIDEO_DRIVER_WAYLAND_QT_TOUCH if (c->surface_extension) { data->extended_surface = qt_surface_extension_get_extended_surface( diff --git a/src/video/wayland/SDL_waylandwindow.h b/src/video/wayland/SDL_waylandwindow.h index baddeabb8..bcbed5bad 100644 --- a/src/video/wayland/SDL_waylandwindow.h +++ b/src/video/wayland/SDL_waylandwindow.h @@ -67,6 +67,8 @@ typedef struct { struct zwp_keyboard_shortcuts_inhibitor_v1 *key_inhibitor; struct zwp_idle_inhibitor_v1 *idle_inhibitor; + SDL_atomic_t swap_interval_ready; + #ifdef SDL_VIDEO_DRIVER_WAYLAND_QT_TOUCH struct qt_extended_surface *extended_surface; #endif /* SDL_VIDEO_DRIVER_WAYLAND_QT_TOUCH */