| Summary: | Fold VideoBootStrap::available() into VideoBootStrap::create() | ||
|---|---|---|---|
| Product: | SDL | Reporter: | M Stoeckl <sdlbug> |
| Component: | video | Assignee: | Sam Lantinga <slouken> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | enhancement | ||
| Priority: | P2 | ||
| Version: | don't know | ||
| Hardware: | x86_64 | ||
| OS: | Linux | ||
| Attachments: |
#1: Base patch, for Linux
#2: Patch to deduplicate X11 init code #3: Patch for remaining backends that I can't build Patch #1: Fold VideoBootStrap::available into VideoBootStrap::createDevice Patch #2: Deduplicate X11 init code Patch #3: deduplicate Wayland init code Patch #1: Fold VideoBootStrap::available into VideoBootStrap::createDevice Patch #2: Deduplicate X11 init code |
||
Created attachment 4409 [details]
#2: Patch to deduplicate X11 init code
Created attachment 4410 [details]
#3: Patch for remaining backends that I can't build
I can't see how to edit my initial comment, so: A bit more motivation for this set of patches: I sometimes run applications using SDL remotely over the X11 and Wayland backends, so that the pattern of connecting, disconnecting, and reconnecting to the display server that happens when *_Available and *_CreateDevice are called in sequence takes a noticeable amount of time. I also have a generic tool that works best when the application being run makes exactly one connection to a Wayland compositor, instead of two. Patch #2 simplifies the connection process with the X11 backend. I also have a patch that does this simplification with the Wayland backend, but have not attached it, as it still needs polishing and after that would require some time to review. Created attachment 4413 [details]
Patch #1: Fold VideoBootStrap::available into VideoBootStrap::createDevice
Created attachment 4414 [details]
Patch #2: Deduplicate X11 init code
Created attachment 4415 [details]
Patch #3: deduplicate Wayland init code
Comment on attachment 4414 [details] Patch #2: Deduplicate X11 init code # HG changeset patch # User M Stoeckl <code@mstoeckl.com> # Date 1594595475 14400 # Sun Jul 12 19:11:15 2020 -0400 # Node ID 05d0d0c11f4e460f98af47809feec9b8b0d96c1f # Parent 465afae5eb7ed53634b894b9c0336786383573b1 Merge VideoBootStrap::available into VideoBootStrap::create The two are only ever called together, and combining them makes it possible to eliminate redundant symbol loading and redundant attempts to connect to a display server. diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/SDL_sysvideo.h --- a/src/video/SDL_sysvideo.h Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/SDL_sysvideo.h Sun Jul 12 19:11:15 2020 -0400 @@ -413,7 +413,6 @@ { const char *name; const char *desc; - int (*available) (void); SDL_VideoDevice *(*create) (int devindex); } VideoBootStrap; diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/SDL_video.c --- a/src/video/SDL_video.c Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/SDL_video.c Sun Jul 12 19:11:15 2020 -0400 @@ -493,19 +493,15 @@ if (driver_name != NULL) { for (i = 0; bootstrap[i]; ++i) { if (SDL_strncasecmp(bootstrap[i]->name, driver_name, SDL_strlen(driver_name)) == 0) { - if (bootstrap[i]->available()) { - video = bootstrap[i]->create(index); - break; - } + video = bootstrap[i]->create(index); + break; } } } else { for (i = 0; bootstrap[i]; ++i) { - if (bootstrap[i]->available()) { - video = bootstrap[i]->create(index); - if (video != NULL) { - break; - } + video = bootstrap[i]->create(index); + if (video != NULL) { + break; } } } diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/android/SDL_androidvideo.c --- a/src/video/android/SDL_androidvideo.c Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/android/SDL_androidvideo.c Sun Jul 12 19:11:15 2020 -0400 @@ -70,12 +70,6 @@ SDL_sem *Android_ResumeSem = NULL; SDL_mutex *Android_ActivityMutex = NULL; -static int -Android_Available(void) -{ - return 1; -} - static void Android_SuspendScreenSaver(_THIS) { @@ -173,7 +167,7 @@ VideoBootStrap Android_bootstrap = { ANDROID_VID_DRIVER_NAME, "SDL Android video driver", - Android_Available, Android_CreateDevice + Android_CreateDevice }; diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/cocoa/SDL_cocoavideo.m --- a/src/video/cocoa/SDL_cocoavideo.m Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/cocoa/SDL_cocoavideo.m Sun Jul 12 19:11:15 2020 -0400 @@ -36,12 +36,6 @@ /* Cocoa driver bootstrap functions */ -static int -Cocoa_Available(void) -{ - return (1); -} - static void Cocoa_DeleteDevice(SDL_VideoDevice * device) { @@ -165,7 +159,7 @@ VideoBootStrap COCOA_bootstrap = { "cocoa", "SDL Cocoa video driver", - Cocoa_Available, Cocoa_CreateDevice + Cocoa_CreateDevice }; diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/directfb/SDL_DirectFB_video.c --- a/src/video/directfb/SDL_DirectFB_video.c Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/directfb/SDL_DirectFB_video.c Sun Jul 12 19:11:15 2020 -0400 @@ -60,12 +60,11 @@ static int DirectFB_VideoInit(_THIS); static void DirectFB_VideoQuit(_THIS); -static int DirectFB_Available(void); static SDL_VideoDevice *DirectFB_CreateDevice(int devindex); VideoBootStrap DirectFB_bootstrap = { "directfb", "DirectFB", - DirectFB_Available, DirectFB_CreateDevice + DirectFB_CreateDevice }; static const DirectFBSurfaceDrawingFlagsNames(drawing_flags); @@ -74,15 +73,6 @@ /* DirectFB driver bootstrap functions */ -static int -DirectFB_Available(void) -{ - if (!SDL_DirectFB_LoadLibrary()) - return 0; - SDL_DirectFB_UnLoadLibrary(); - return 1; -} - static void DirectFB_DeleteDevice(SDL_VideoDevice * device) { diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/dummy/SDL_nullvideo.c --- a/src/video/dummy/SDL_nullvideo.c Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/dummy/SDL_nullvideo.c Sun Jul 12 19:11:15 2020 -0400 @@ -78,6 +78,10 @@ { SDL_VideoDevice *device; + if (!DUMMY_Available()) { + return (0); + } + /* Initialize all variables that we clean on shutdown */ device = (SDL_VideoDevice *) SDL_calloc(1, sizeof(SDL_VideoDevice)); if (!device) { @@ -102,7 +106,7 @@ VideoBootStrap DUMMY_bootstrap = { DUMMYVID_DRIVER_NAME, "SDL dummy video driver", - DUMMY_Available, DUMMY_CreateDevice + DUMMY_CreateDevice }; diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/emscripten/SDL_emscriptenvideo.c --- a/src/video/emscripten/SDL_emscriptenvideo.c Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/emscripten/SDL_emscriptenvideo.c Sun Jul 12 19:11:15 2020 -0400 @@ -54,12 +54,6 @@ /* Emscripten driver bootstrap functions */ -static int -Emscripten_Available(void) -{ - return (1); -} - static void Emscripten_DeleteDevice(SDL_VideoDevice * device) { @@ -132,7 +126,7 @@ VideoBootStrap Emscripten_bootstrap = { EMSCRIPTENVID_DRIVER_NAME, "SDL emscripten video driver", - Emscripten_Available, Emscripten_CreateDevice + Emscripten_CreateDevice }; diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/haiku/SDL_bvideo.cc --- a/src/video/haiku/SDL_bvideo.cc Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/haiku/SDL_bvideo.cc Sun Jul 12 19:11:15 2020 -0400 @@ -124,7 +124,7 @@ VideoBootStrap HAIKU_bootstrap = { "haiku", "Haiku graphics", - HAIKU_Available, HAIKU_CreateDevice + HAIKU_CreateDevice }; void HAIKU_DeleteDevice(SDL_VideoDevice * device) @@ -185,11 +185,6 @@ return (0); } -int HAIKU_Available(void) -{ - return (1); -} - void HAIKU_VideoQuit(_THIS) { diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/haiku/SDL_bvideo.h --- a/src/video/haiku/SDL_bvideo.h Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/haiku/SDL_bvideo.h Sun Jul 12 19:11:15 2020 -0400 @@ -33,7 +33,6 @@ extern void HAIKU_VideoQuit(_THIS); extern int HAIKU_VideoInit(_THIS); extern void HAIKU_DeleteDevice(_THIS); -extern int HAIKU_Available(void); #ifdef __cplusplus } diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/kmsdrm/SDL_kmsdrmvideo.c --- a/src/video/kmsdrm/SDL_kmsdrmvideo.c Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/kmsdrm/SDL_kmsdrmvideo.c Sun Jul 12 19:11:15 2020 -0400 @@ -159,6 +159,10 @@ SDL_VideoDevice *device; SDL_VideoData *viddata; + if (!KMSDRM_Available()) { + return NULL; + } + if (!devindex || (devindex > 99)) { devindex = get_driindex(); } @@ -235,7 +239,6 @@ VideoBootStrap KMSDRM_bootstrap = { "KMSDRM", "KMS/DRM Video Driver", - KMSDRM_Available, KMSDRM_CreateDevice }; diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/nacl/SDL_naclvideo.c --- a/src/video/nacl/SDL_naclvideo.c Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/nacl/SDL_naclvideo.c Sun Jul 12 19:11:15 2020 -0400 @@ -94,6 +94,10 @@ static SDL_VideoDevice *NACL_CreateDevice(int devindex) { SDL_VideoDevice *device; + if (!NACL_Available()) { + return NULL; + } + /* Initialize all variables that we clean on shutdown */ device = (SDL_VideoDevice *) SDL_calloc(1, sizeof(SDL_VideoDevice)); if (!device) { @@ -132,7 +136,7 @@ VideoBootStrap NACL_bootstrap = { NACLVID_DRIVER_NAME, "SDL Native Client Video Driver", - NACL_Available, NACL_CreateDevice + NACL_CreateDevice }; int NACL_VideoInit(_THIS) { diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/offscreen/SDL_offscreenvideo.c --- a/src/video/offscreen/SDL_offscreenvideo.c Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/offscreen/SDL_offscreenvideo.c Sun Jul 12 19:11:15 2020 -0400 @@ -51,13 +51,6 @@ /* OFFSCREEN driver bootstrap functions */ -static int -OFFSCREEN_Available(void) -{ - /* Consider it always available */ - return (1); -} - static void OFFSCREEN_DeleteDevice(SDL_VideoDevice * device) { @@ -106,7 +99,7 @@ VideoBootStrap OFFSCREEN_bootstrap = { OFFSCREENVID_DRIVER_NAME, "SDL offscreen video driver", - OFFSCREEN_Available, OFFSCREEN_CreateDevice + OFFSCREEN_CreateDevice }; int diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/psp/SDL_pspvideo.c --- a/src/video/psp/SDL_pspvideo.c Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/psp/SDL_pspvideo.c Sun Jul 12 19:11:15 2020 -0400 @@ -42,11 +42,6 @@ /* unused static SDL_bool PSP_initialized = SDL_FALSE; */ -static int -PSP_Available(void) -{ - return 1; -} static void PSP_Destroy(SDL_VideoDevice * device) @@ -64,14 +59,6 @@ SDL_VideoDevice *device; SDL_VideoData *phdata; SDL_GLDriverData *gldata; - int status; - - /* Check if PSP could be initialized */ - status = PSP_Available(); - if (status == 0) { - /* PSP could not be used */ - return NULL; - } /* Initialize SDL_VideoDevice structure */ device = (SDL_VideoDevice *) SDL_calloc(1, sizeof(SDL_VideoDevice)); @@ -152,7 +139,6 @@ VideoBootStrap PSP_bootstrap = { "PSP", "PSP Video Driver", - PSP_Available, PSP_Create }; diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/qnx/video.c --- a/src/video/qnx/video.c Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/qnx/video.c Sun Jul 12 19:11:15 2020 -0400 @@ -352,13 +352,7 @@ return device; } -static int -available() -{ - return 1; -} - VideoBootStrap QNX_bootstrap = { "qnx", "QNX Screen", - available, createDevice + createDevice }; diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/raspberry/SDL_rpivideo.c --- a/src/video/raspberry/SDL_rpivideo.c Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/raspberry/SDL_rpivideo.c Sun Jul 12 19:11:15 2020 -0400 @@ -50,12 +50,6 @@ #include "SDL_rpiopengles.h" #include "SDL_rpimouse.h" -static int -RPI_Available(void) -{ - return 1; -} - static void RPI_Destroy(SDL_VideoDevice * device) { @@ -150,7 +144,6 @@ VideoBootStrap RPI_bootstrap = { "RPI", "RPI Video Driver", - RPI_Available, RPI_Create }; diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/uikit/SDL_uikitvideo.m --- a/src/video/uikit/SDL_uikitvideo.m Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/uikit/SDL_uikitvideo.m Sun Jul 12 19:11:15 2020 -0400 @@ -52,12 +52,6 @@ /* DUMMY driver bootstrap functions */ -static int -UIKit_Available(void) -{ - return 1; -} - static void UIKit_DeleteDevice(SDL_VideoDevice * device) { @autoreleasepool { @@ -152,7 +146,7 @@ VideoBootStrap UIKIT_bootstrap = { UIKITVID_DRIVER_NAME, "SDL UIKit video driver", - UIKit_Available, UIKit_CreateDevice + UIKit_CreateDevice }; diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/vivante/SDL_vivantevideo.c --- a/src/video/vivante/SDL_vivantevideo.c Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/vivante/SDL_vivantevideo.c Sun Jul 12 19:11:15 2020 -0400 @@ -40,12 +40,6 @@ #include "SDL_vivantevulkan.h" -static int -VIVANTE_Available(void) -{ - return 1; -} - static void VIVANTE_Destroy(SDL_VideoDevice * device) { @@ -125,7 +119,6 @@ VideoBootStrap VIVANTE_bootstrap = { "vivante", "Vivante EGL Video Driver", - VIVANTE_Available, VIVANTE_Create }; diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/wayland/SDL_waylandvideo.c --- a/src/video/wayland/SDL_waylandvideo.c Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/wayland/SDL_waylandvideo.c Sun Jul 12 19:11:15 2020 -0400 @@ -148,6 +148,10 @@ { SDL_VideoDevice *device; + if (!Wayland_Available()) { + return NULL; + } + if (!SDL_WAYLAND_LoadSymbols()) { return NULL; } @@ -211,7 +215,7 @@ VideoBootStrap Wayland_bootstrap = { WAYLANDVID_DRIVER_NAME, "SDL Wayland video driver", - Wayland_Available, Wayland_CreateDevice + Wayland_CreateDevice }; static void diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/windows/SDL_windowsvideo.c --- a/src/video/windows/SDL_windowsvideo.c Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/windows/SDL_windowsvideo.c Sun Jul 12 19:11:15 2020 -0400 @@ -75,12 +75,6 @@ /* Windows driver bootstrap functions */ -static int -WIN_Available(void) -{ - return (1); -} - static void WIN_DeleteDevice(SDL_VideoDevice * device) { @@ -224,7 +218,7 @@ VideoBootStrap WINDOWS_bootstrap = { - "windows", "SDL Windows video driver", WIN_Available, WIN_CreateDevice + "windows", "SDL Windows video driver", WIN_CreateDevice }; int diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/winrt/SDL_winrtvideo.cpp --- a/src/video/winrt/SDL_winrtvideo.cpp Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/winrt/SDL_winrtvideo.cpp Sun Jul 12 19:11:15 2020 -0400 @@ -95,12 +95,6 @@ /* WinRT driver bootstrap functions */ -static int -WINRT_Available(void) -{ - return (1); -} - static void WINRT_DeleteDevice(SDL_VideoDevice * device) { @@ -174,7 +168,7 @@ #define WINRTVID_DRIVER_NAME "winrt" VideoBootStrap WINRT_bootstrap = { WINRTVID_DRIVER_NAME, "SDL WinRT video driver", - WINRT_Available, WINRT_CreateDevice + WINRT_CreateDevice }; int diff -r 465afae5eb7e -r 05d0d0c11f4e src/video/x11/SDL_x11video.c --- a/src/video/x11/SDL_x11video.c Sat Jul 11 08:10:02 2020 +0300 +++ b/src/video/x11/SDL_x11video.c Sun Jul 12 19:11:15 2020 -0400 @@ -92,6 +92,7 @@ /* X11 driver bootstrap functions */ +<<<<<<< local static int X11_Available(void) { @@ -106,6 +107,8 @@ return (display != NULL); } +======= +>>>>>>> histedit static void X11_DeleteDevice(SDL_VideoDevice * device) { @@ -160,6 +163,10 @@ SDL_VideoData *data; const char *display = NULL; /* Use the DISPLAY environment variable */ + if (!X11_Available()) { + return NULL; + } + if (!SDL_X11_LoadSymbols()) { return NULL; } @@ -317,7 +324,7 @@ VideoBootStrap X11_bootstrap = { "x11", "SDL X11 video driver", - X11_Available, X11_CreateDevice + X11_CreateDevice }; static int (*handler) (Display *, XErrorEvent *) = NULL; Created attachment 4416 [details]
Patch #1: Fold VideoBootStrap::available into VideoBootStrap::createDevice
Created attachment 4417 [details]
Patch #2: Deduplicate X11 init code
Please ignore comment #7; I made a mistake trying to update the patches. Sorry about that. OK, I've cleaned up the history, removed old code in the X11 patch (including the old workaround for what probably is a race like the one described at https://bugzilla.mozilla.org/show_bug.cgi?id=246313#c33), and have included the patch for Wayland. I do not expect to make any more changes. These changes look good, thanks! https://hg.libsdl.org/SDL/rev/5acc27d3d654 https://hg.libsdl.org/SDL/rev/084ffca603ae https://hg.libsdl.org/SDL/rev/830a4ce7456e |
Created attachment 4408 [details] #1: Base patch, for Linux # Background: SDL_VideoDevice objects are created inside the function SDL_VideoInit(). For each backend, there is an associated VideoBootStrap object, with a pair of available() and create() function pointers. The first pre-checks if the backend can be used; the second creates the corresponding SDL_VideoDevice structure. These functions are only used by SDL_VideoInit(). Most backends have an available() test that always returns true; only KMSDRM, NaCl, Wayland, X11, and Null backends do actual work. # Issue The implementation of the ::available() function pointer, when it is nontrivial, sometimes performs setup work (e.g., for X11 or Wayland, making a connection), verifies something, and then promptly undoes that work. Then ::create() calls the backend-specific setup functions again. # Patch Removes the `available` field of VideoBootStrap and instead call the implementations' _Available functions from _CreateDevice itself.