Clean up any renderer in SDL_DestroyWindowSurface()

Also added an automated test to verify window surface functionality.

Fixes https://github.com/libsdl-org/SDL/issues/10133

src/video/SDL_video.c

@@ -2442,12 +2442,6 @@ int SDL_RecreateWindow(SDL_Window *window, SDL_WindowFlags flags)
     /* Tear down the old native window */
     SDL_DestroyWindowSurface(window);
 
-    if (_this->checked_texture_framebuffer) { /* never checked? No framebuffer to destroy. Don't risk calling the wrong implementation. */
-        if (_this->DestroyWindowFramebuffer) {
-            _this->DestroyWindowFramebuffer(_this, window);
-        }
-    }
-
     if ((window->flags & SDL_WINDOW_OPENGL) != (flags & SDL_WINDOW_OPENGL)) {
         if (flags & SDL_WINDOW_OPENGL) {
             need_gl_load = SDL_TRUE;
@@ -3251,57 +3245,59 @@ static SDL_Surface *SDL_CreateWindowFramebuffer(SDL_Window *window)
        using a GPU texture through the 2D render API, if we think this would
        be more efficient. This only checks once, on demand. */
     if (!_this->checked_texture_framebuffer) {
-        SDL_bool attempt_texture_framebuffer = SDL_TRUE;
-
-        /* See if the user or application wants to specifically disable the framebuffer */
-        const char *hint = SDL_GetHint(SDL_HINT_FRAMEBUFFER_ACCELERATION);
-        if (hint) {
-            if ((*hint == '0') || (SDL_strcasecmp(hint, "false") == 0) || (SDL_strcasecmp(hint, SDL_SOFTWARE_RENDERER) == 0)) {
-                attempt_texture_framebuffer = SDL_FALSE;
-            }
-        }
+        SDL_bool attempt_texture_framebuffer;
 
         if (_this->is_dummy) { /* dummy driver never has GPU support, of course. */
             attempt_texture_framebuffer = SDL_FALSE;
-        }
+        } else {
+            /* See if the user or application wants to specifically disable the framebuffer */
+            const char *hint = SDL_GetHint(SDL_HINT_FRAMEBUFFER_ACCELERATION);
+            if (hint && *hint) {
+                if ((*hint == '0') || (SDL_strcasecmp(hint, "false") == 0) || (SDL_strcasecmp(hint, SDL_SOFTWARE_RENDERER) == 0)) {
+                    attempt_texture_framebuffer = SDL_FALSE;
+                } else {
+                    attempt_texture_framebuffer = SDL_TRUE;
+                }
+            } else {
+                attempt_texture_framebuffer = SDL_TRUE;
 
 #ifdef SDL_PLATFORM_LINUX
-        /* On WSL, direct X11 is faster than using OpenGL for window framebuffers, so try to detect WSL and avoid texture framebuffer. */
-        else if ((_this->CreateWindowFramebuffer) && (SDL_strcmp(_this->name, "x11") == 0)) {
-            struct stat sb;
-            if ((stat("/proc/sys/fs/binfmt_misc/WSLInterop", &sb) == 0) || (stat("/run/WSL", &sb) == 0)) { /* if either of these exist, we're on WSL. */
-                attempt_texture_framebuffer = SDL_FALSE;
-            }
-        }
+                /* On WSL, direct X11 is faster than using OpenGL for window framebuffers, so try to detect WSL and avoid texture framebuffer. */
+                if ((_this->CreateWindowFramebuffer) && (SDL_strcmp(_this->name, "x11") == 0)) {
+                    struct stat sb;
+                    if ((stat("/proc/sys/fs/binfmt_misc/WSLInterop", &sb) == 0) || (stat("/run/WSL", &sb) == 0)) { /* if either of these exist, we're on WSL. */
+                        attempt_texture_framebuffer = SDL_FALSE;
+                    }
+                }
 #endif
 #if defined(SDL_PLATFORM_WIN32) || defined(SDL_PLATFORM_WINGDK) /* GDI BitBlt() is way faster than Direct3D dynamic textures right now. (!!! FIXME: is this still true?) */
-        else if ((_this->CreateWindowFramebuffer) && (SDL_strcmp(_this->name, "windows") == 0)) {
-            attempt_texture_framebuffer = SDL_FALSE;
-        }
+                if (_this->CreateWindowFramebuffer && (SDL_strcmp(_this->name, "windows") == 0)) {
+                    attempt_texture_framebuffer = SDL_FALSE;
+                }
 #endif
 #ifdef SDL_PLATFORM_EMSCRIPTEN
-        else {
-            attempt_texture_framebuffer = SDL_FALSE;
-        }
+                attempt_texture_framebuffer = SDL_FALSE;
 #endif
+            }
 
-        if (attempt_texture_framebuffer) {
-            if (SDL_CreateWindowTexture(_this, window, &format, &pixels, &pitch) < 0) {
-                /* !!! FIXME: if this failed halfway (made renderer, failed to make texture, etc),
-                   !!! FIXME:  we probably need to clean this up so it doesn't interfere with
-                   !!! FIXME:  a software fallback at the system level (can we blit to an
-                   !!! FIXME:  OpenGL window? etc). */
-            } else {
-                /* future attempts will just try to use a texture framebuffer. */
-                /* !!! FIXME:  maybe we shouldn't override these but check if we used a texture
-                   !!! FIXME:  framebuffer at the right places; is it feasible we could have an
-                   !!! FIXME:  accelerated OpenGL window and a second ends up in software? */
-                _this->CreateWindowFramebuffer = SDL_CreateWindowTexture;
-                _this->SetWindowFramebufferVSync = SDL_SetWindowTextureVSync;
-                _this->GetWindowFramebufferVSync = SDL_GetWindowTextureVSync;
-                _this->UpdateWindowFramebuffer = SDL_UpdateWindowTexture;
-                _this->DestroyWindowFramebuffer = SDL_DestroyWindowTexture;
-                created_framebuffer = SDL_TRUE;
+            if (attempt_texture_framebuffer) {
+                if (SDL_CreateWindowTexture(_this, window, &format, &pixels, &pitch) < 0) {
+                    /* !!! FIXME: if this failed halfway (made renderer, failed to make texture, etc),
+                       !!! FIXME:  we probably need to clean this up so it doesn't interfere with
+                       !!! FIXME:  a software fallback at the system level (can we blit to an
+                       !!! FIXME:  OpenGL window? etc). */
+                } else {
+                    /* future attempts will just try to use a texture framebuffer. */
+                    /* !!! FIXME:  maybe we shouldn't override these but check if we used a texture
+                       !!! FIXME:  framebuffer at the right places; is it feasible we could have an
+                       !!! FIXME:  accelerated OpenGL window and a second ends up in software? */
+                    _this->CreateWindowFramebuffer = SDL_CreateWindowTexture;
+                    _this->SetWindowFramebufferVSync = SDL_SetWindowTextureVSync;
+                    _this->GetWindowFramebufferVSync = SDL_GetWindowTextureVSync;
+                    _this->UpdateWindowFramebuffer = SDL_UpdateWindowTexture;
+                    _this->DestroyWindowFramebuffer = SDL_DestroyWindowTexture;
+                    created_framebuffer = SDL_TRUE;
+                }
             }
         }
 
@@ -3310,6 +3306,7 @@ static SDL_Surface *SDL_CreateWindowFramebuffer(SDL_Window *window)
 
     if (!created_framebuffer) {
         if (!_this->CreateWindowFramebuffer || !_this->UpdateWindowFramebuffer) {
+            SDL_SetError("Window framebuffer support not available");
             return NULL;
         }
 
@@ -3319,6 +3316,7 @@ static SDL_Surface *SDL_CreateWindowFramebuffer(SDL_Window *window)
     }
 
     if (window->surface) {
+        /* We may have gone recursive and already created the surface */
         return window->surface;
     }
 
@@ -3337,7 +3335,12 @@ SDL_Surface *SDL_GetWindowSurface(SDL_Window *window)
     CHECK_WINDOW_MAGIC(window, NULL);
 
     if (!window->surface_valid) {
-        SDL_DestroyWindowSurface(window);
+        if (window->surface) {
+            window->surface->flags &= ~SDL_DONTFREE;
+            SDL_DestroySurface(window->surface);
+            window->surface = NULL;
+        }
+
         window->surface = SDL_CreateWindowFramebuffer(window);
         if (window->surface) {
             window->surface_valid = SDL_TRUE;
@@ -3394,6 +3397,25 @@ int SDL_UpdateWindowSurfaceRects(SDL_Window *window, const SDL_Rect *rects,
     return _this->UpdateWindowFramebuffer(_this, window, rects, numrects);
 }
 
+int SDL_DestroyWindowSurface(SDL_Window *window)
+{
+    CHECK_WINDOW_MAGIC(window, -1);
+
+    if (window->surface) {
+        window->surface->flags &= ~SDL_DONTFREE;
+        SDL_DestroySurface(window->surface);
+        window->surface = NULL;
+        window->surface_valid = SDL_FALSE;
+    }
+
+    if (_this->checked_texture_framebuffer) { /* never checked? No framebuffer to destroy. Don't risk calling the wrong implementation. */
+        if (_this->DestroyWindowFramebuffer) {
+            _this->DestroyWindowFramebuffer(_this, window);
+        }
+    }
+    return 0;
+}
+
 int SDL_SetWindowOpacity(SDL_Window *window, float opacity)
 {
     int retval;
@@ -3417,19 +3439,6 @@ int SDL_SetWindowOpacity(SDL_Window *window, float opacity)
     return retval;
 }
 
-int SDL_DestroyWindowSurface(SDL_Window *window)
-{
-    CHECK_WINDOW_MAGIC(window, -1);
-
-    if (window->surface) {
-        window->surface->flags &= ~SDL_DONTFREE;
-        SDL_DestroySurface(window->surface);
-        window->surface = NULL;
-        window->surface_valid = SDL_FALSE;
-    }
-    return 0;
-}
-
 int SDL_GetWindowOpacity(SDL_Window *window, float *out_opacity)
 {
     CHECK_WINDOW_MAGIC(window, -1);
@@ -3922,12 +3931,6 @@ void SDL_DestroyWindow(SDL_Window *window)
 
     SDL_DestroyWindowSurface(window);
 
-    if (_this->checked_texture_framebuffer) { /* never checked? No framebuffer to destroy. Don't risk calling the wrong implementation. */
-        if (_this->DestroyWindowFramebuffer) {
-            _this->DestroyWindowFramebuffer(_this, window);
-        }
-    }
-
     /* Make no context current if this is the current context window */
     if (window->flags & SDL_WINDOW_OPENGL) {
         if (_this->current_glwin == window) {

test/testautomation_video.c

@@ -28,7 +28,7 @@ static SDL_Window *createVideoSuiteTestWindow(const char *title)
 
     window = SDL_CreateWindow(title, w, h, flags);
     SDLTest_AssertPass("Call to SDL_CreateWindow('Title',%d,%d,%" SDL_PRIu64 ")", w, h, flags);
-    SDLTest_AssertCheck(window != NULL, "Validate that returned window struct is not NULL");
+    SDLTest_AssertCheck(window != NULL, "Validate that returned window is not NULL");
 
     /* Check the window is available in the window list */
     windows = SDL_GetWindows(&count);
@@ -190,7 +190,7 @@ static int video_createWindowVariousSizes(void *arg)
 
             window = SDL_CreateWindow(title, w, h, 0);
             SDLTest_AssertPass("Call to SDL_CreateWindow('Title',%d,%d,SHOWN)", w, h);
-            SDLTest_AssertCheck(window != NULL, "Validate that returned window struct is not NULL");
+            SDLTest_AssertCheck(window != NULL, "Validate that returned window is not NULL");
 
             /* Clean up */
             destroyVideoSuiteTestWindow(window);
@@ -265,7 +265,7 @@ static int video_createWindowVariousFlags(void *arg)
 
         window = SDL_CreateWindow(title, w, h, flags);
         SDLTest_AssertPass("Call to SDL_CreateWindow('Title',%d,%d,%" SDL_PRIu64 ")", w, h, flags);
-        SDLTest_AssertCheck(window != NULL, "Validate that returned window struct is not NULL");
+        SDLTest_AssertCheck(window != NULL, "Validate that returned window is not NULL");
 
         /* Clean up */
         destroyVideoSuiteTestWindow(window);
@@ -1732,7 +1732,7 @@ static int video_setWindowCenteredOnDisplay(void *arg)
                 window = SDL_CreateWindowWithProperties(props);
                 SDL_DestroyProperties(props);
                 SDLTest_AssertPass("Call to SDL_CreateWindow('Title',%d,%d,%d,%d,SHOWN)", x, y, w, h);
-                SDLTest_AssertCheck(window != NULL, "Validate that returned window struct is not NULL");
+                SDLTest_AssertCheck(window != NULL, "Validate that returned window is not NULL");
 
                 /* Wayland windows require that a frame be presented before they are fully mapped and visible onscreen. */
                 if (video_driver_is_wayland) {
@@ -2238,6 +2238,76 @@ minimize_test:
     return skipFlags != (SDL_WINDOW_MAXIMIZED | SDL_WINDOW_MINIMIZED)  ? TEST_COMPLETED : TEST_SKIPPED;
 }
 
+/**
+ * Tests window surface functionality
+ */
+static int video_getWindowSurface(void *arg)
+{
+    const char *title = "video_getWindowSurface Test Window";
+    SDL_Window *window;
+    SDL_Surface *surface;
+    SDL_Renderer *renderer;
+    const char *renderer_name = NULL;
+    int result;
+
+    if (SDL_strcmp(SDL_GetCurrentVideoDriver(), "dummy") == 0) {
+        renderer_name = SDL_SOFTWARE_RENDERER;
+    }
+
+    /* Make sure we're testing interaction with an accelerated renderer */
+    SDL_SetHint(SDL_HINT_FRAMEBUFFER_ACCELERATION, "1");
+
+    window = SDL_CreateWindow(title, 320, 320, 0);
+    SDLTest_AssertPass("Call to SDL_CreateWindow('Title',320,320,0)");
+    SDLTest_AssertCheck(window != NULL, "Validate that returned window is not NULL");
+
+    surface = SDL_GetWindowSurface(window);
+    SDLTest_AssertPass("Call to SDL_GetWindowSurface(window)");
+    SDLTest_AssertCheck(surface != NULL, "Validate that returned surface is not NULL");
+    SDLTest_AssertCheck(SDL_WindowHasSurface(window), "Validate that window has a surface");
+
+    result = SDL_UpdateWindowSurface(window);
+    SDLTest_AssertPass("Call to SDL_UpdateWindowSurface(window)");
+    SDLTest_AssertCheck(result == 0, "Verify return value; expected: 0, got: %d", result);
+
+    /* We shouldn't be able to create a renderer on a window with a surface */
+    renderer = SDL_CreateRenderer(window, renderer_name);
+    SDLTest_AssertPass("Call to SDL_CreateRenderer(window, %s)", renderer_name);
+    SDLTest_AssertCheck(renderer == NULL, "Validate that returned renderer is NULL");
+
+    result = SDL_DestroyWindowSurface(window);
+    SDLTest_AssertPass("Call to SDL_DestroyWindowSurface(window)");
+    SDLTest_AssertCheck(result == 0, "Verify return value; expected: 0, got: %d", result);
+    SDLTest_AssertCheck(!SDL_WindowHasSurface(window), "Validate that window does not have a surface");
+
+    /* We should be able to create a renderer on the window now */
+    renderer = SDL_CreateRenderer(window, renderer_name);
+    SDLTest_AssertPass("Call to SDL_CreateRenderer(window, %s)", renderer_name);
+    SDLTest_AssertCheck(renderer != NULL, "Validate that returned renderer is not NULL");
+
+    /* We should not be able to create a window surface now, unless it was created by the renderer */
+    if (!SDL_WindowHasSurface(window)) {
+        surface = SDL_GetWindowSurface(window);
+        SDLTest_AssertPass("Call to SDL_GetWindowSurface(window)");
+        SDLTest_AssertCheck(surface == NULL, "Validate that returned surface is NULL");
+    }
+
+    SDL_DestroyRenderer(renderer);
+    SDLTest_AssertPass("Call to SDL_DestroyRenderer(renderer)");
+    SDLTest_AssertCheck(!SDL_WindowHasSurface(window), "Validate that window does not have a surface");
+
+    /* We should be able to create a window surface again */
+    surface = SDL_GetWindowSurface(window);
+    SDLTest_AssertPass("Call to SDL_GetWindowSurface(window)");
+    SDLTest_AssertCheck(surface != NULL, "Validate that returned surface is not NULL");
+    SDLTest_AssertCheck(SDL_WindowHasSurface(window), "Validate that window has a surface");
+
+    /* Clean up */
+    SDL_DestroyWindow(window);
+
+    return TEST_COMPLETED;
+}
+
 /* ================= Test References ================== */
 
 /* Video test cases */
@@ -2317,12 +2387,16 @@ static const SDLTest_TestCaseReference videoTest19 = {
     (SDLTest_TestCaseFp)video_getSetWindowState, "video_getSetWindowState", "Checks transitioning between windowed, minimized, maximized, and fullscreen states", TEST_ENABLED
 };
 
+static const SDLTest_TestCaseReference videoTest20 = {
+    (SDLTest_TestCaseFp)video_getWindowSurface, "video_getWindowSurface", "Checks window surface functionality", TEST_ENABLED
+};
+
 /* Sequence of Video test cases */
 static const SDLTest_TestCaseReference *videoTests[] = {
     &videoTest1, &videoTest2, &videoTest3, &videoTest4, &videoTest5, &videoTest6,
     &videoTest7, &videoTest8, &videoTest9, &videoTest10, &videoTest11, &videoTest12,
     &videoTest13, &videoTest14, &videoTest15, &videoTest16, &videoTest17,
-    &videoTest18, &videoTest19, NULL
+    &videoTest18, &videoTest19, &videoTest20, NULL
 };
 
 /* Video test suite (global) */