From 7effca68f0337017a9530385b4d05a8dcd66f53a Mon Sep 17 00:00:00 2001
From: Kp <kp@valhallalegends.com>
Date: Sun, 6 Nov 2022 18:18:57 +0000
Subject: [PATCH] Use RAII_SDL_Surface to track SDL-only main canvas surface

The Rebirth main screen grd_curscreen borrows the memory allocated by
SDL_CreateRGBSurface to store its data.  Store the pointer to that
surface inside grd_curscreen and make grd_curscreen responsible for
freeing the surface.
---
 common/include/gr.h     | 56 ++++++++++++++++++++++++++++++++++-------
 similar/2d/pcx.cpp      | 18 -------------
 similar/arch/sdl/gr.cpp |  3 +--
 3 files changed, 48 insertions(+), 29 deletions(-)

diff --git a/common/include/gr.h b/common/include/gr.h
index 4733da76c..fd6f1d491 100644
--- a/common/include/gr.h
+++ b/common/include/gr.h
@@ -35,6 +35,11 @@ COPYRIGHT 1993-1999 PARALLAX SOFTWARE CORPORATION.  ALL RIGHTS RESERVED.
 #include "pack.h"
 #include <array>
 
+#if DXX_USE_SDLIMAGE || !DXX_USE_OGL
+#include <memory>
+#include <SDL_video.h>
+#endif
+
 namespace dcx {
 
 enum class gr_fade_level : uint8_t
@@ -232,13 +237,55 @@ struct grs_main_canvas : grs_canvas, prohibit_void_ptr<grs_main_canvas>
 	~grs_main_canvas();
 };
 
+// Creates a canvas that is part of another canvas.  This can be used to make
+// a window on the screen.  The address of the raw pixel data is inherited from
+// the parent canvas.
+struct grs_subcanvas : grs_canvas, prohibit_void_ptr<grs_subcanvas>
+{
+	using prohibit_void_ptr<grs_subcanvas>::operator &;
+};
+
+#if DXX_USE_SDLIMAGE || !DXX_USE_OGL
+struct RAII_SDL_Surface
+{
+	struct deleter
+	{
+		void operator()(SDL_Surface *s) const
+		{
+			SDL_FreeSurface(s);
+		}
+	};
+	std::unique_ptr<SDL_Surface, deleter> surface;
+	constexpr RAII_SDL_Surface() = default;
+	RAII_SDL_Surface(RAII_SDL_Surface &&) = default;
+	RAII_SDL_Surface &operator=(RAII_SDL_Surface &&) = default;
+	explicit RAII_SDL_Surface(SDL_Surface *const s) :
+		surface(s)
+	{
+	}
+};
+#endif
+
 class grs_screen : prohibit_void_ptr<grs_screen>
 {    // This is a video screen
 	screen_mode sc_mode;
 public:
 	grs_screen &operator=(grs_screen &) = delete;
 	grs_screen &operator=(grs_screen &&) = default;
+#if DXX_USE_OGL
+	/* OpenGL builds allocate the backing data for the canvas, so use
+	 * grs_main_canvas to ensure it is freed when appropriate.
+	 */
 	grs_main_canvas  sc_canvas;  // Represents the entire screen
+#else
+	/* SDL builds borrow the backing data from the SDL_Surface, so use
+	 * grs_subcanvas for the canvas because the memory is owned by SDL and will
+	 * be freed by SDL_FreeSurface.  Store the associated SDL_Surface alongside
+	 * the canvas, so that it is freed at the same time.
+	 */
+	RAII_SDL_Surface sdl_surface;
+	grs_subcanvas sc_canvas;
+#endif
 	fix     sc_aspect;      //aspect ratio (w/h) for this screen
 	uint16_t get_screen_width() const
 	{
@@ -262,15 +309,6 @@ public:
 //=========================================================================
 // Canvas functions:
 
-// Creates a canvas that is part of another canvas.  this can be used to make
-// a window on the screen.  the canvas structure is malloc'd; the address of
-// the raw pixel data is inherited from the parent canvas.
-
-struct grs_subcanvas : grs_canvas, prohibit_void_ptr<grs_subcanvas>
-{
-	using prohibit_void_ptr<grs_subcanvas>::operator &;
-};
-
 // Free the bitmap, but not the pixel data buffer
 class grs_subbitmap : public grs_bitmap
 {
diff --git a/similar/2d/pcx.cpp b/similar/2d/pcx.cpp
index 09b41db38..fc75beab8 100644
--- a/similar/2d/pcx.cpp
+++ b/similar/2d/pcx.cpp
@@ -50,24 +50,6 @@ namespace dcx {
 
 namespace {
 
-#if DXX_USE_SDLIMAGE
-struct RAII_SDL_Surface
-{
-	struct deleter
-	{
-		void operator()(SDL_Surface *s) const
-		{
-			SDL_FreeSurface(s);
-		}
-	};
-	std::unique_ptr<SDL_Surface, deleter> surface;
-	explicit RAII_SDL_Surface(SDL_Surface *const s) :
-		surface(s)
-	{
-	}
-};
-#endif
-
 #if !DXX_USE_OGL && DXX_USE_SCREENSHOT_FORMAT_LEGACY
 [[nodiscard]]
 static int pcx_encode_byte(ubyte byt, ubyte cnt, PHYSFS_File *fid);
diff --git a/similar/arch/sdl/gr.cpp b/similar/arch/sdl/gr.cpp
index ab16c93fa..3b9179b18 100644
--- a/similar/arch/sdl/gr.cpp
+++ b/similar/arch/sdl/gr.cpp
@@ -123,6 +123,7 @@ int gr_set_mode(screen_mode mode)
 	}
 
 	*grd_curscreen = {};
+	grd_curscreen->sdl_surface = RAII_SDL_Surface(canvas);
 	grd_curscreen->set_screen_width_height(w, h);
 	grd_curscreen->sc_aspect = fixdiv(grd_curscreen->get_screen_width() * GameCfg.AspectX, grd_curscreen->get_screen_height() * GameCfg.AspectY);
 	gr_init_canvas(grd_curscreen->sc_canvas, reinterpret_cast<unsigned char *>(canvas->pixels), bm_mode::linear, w, h);
@@ -171,7 +172,6 @@ int gr_init()
 	}
 
 	grd_curscreen = std::make_unique<grs_screen>();
-	*grd_curscreen = {};
 
 	if (!CGameCfg.WindowMode && !CGameArg.SysWindow)
 		sdl_video_flags|=SDL_FULLSCREEN;
@@ -204,7 +204,6 @@ void gr_close()
 		gr_installed = 0;
 		grd_curscreen.reset();
 		SDL_ShowCursor(1);
-		SDL_FreeSurface(canvas);
 	}
 }