From ea1a014145e472ff2ef72e8977bddb17f11b9eaa Mon Sep 17 00:00:00 2001
From: Misa <infoteddy@infoteddy.info>
Date: Fri, 27 Oct 2023 10:21:18 -0700
Subject: [PATCH] Fix memory leak with ApplyFilter

The intention of the recent refactor was to make it so that the
temporary surfaces would be allocated only once, when the mode is
enabled, and be freed upon exit.

To do this, Graphics.cpp owns the pointers, and passes them to
ApplyFilter to modify. Except ApplyFilter doesn't actually modify the
pointers, because it's only a single pointer, not a pointer-to-pointer.
So every frame of rendering it would actually be creating a new surface
and leaking memory.

To fix this, they need to be pointer-to-pointer variables that get
modified.

I also added error logs in case the surface creation failed.
---
 desktop_version/src/Graphics.cpp     |  4 +--
 desktop_version/src/GraphicsUtil.cpp | 41 ++++++++++++++++------------
 desktop_version/src/GraphicsUtil.h   |  2 +-
 3 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/desktop_version/src/Graphics.cpp b/desktop_version/src/Graphics.cpp
index f9aa953b..c1d73828 100644
--- a/desktop_version/src/Graphics.cpp
+++ b/desktop_version/src/Graphics.cpp
@@ -3244,7 +3244,7 @@ void Graphics::screenshake(void)
 {
     if (gameScreen.badSignalEffect)
     {
-        ApplyFilter(tempFilterSrc, tempFilterDest);
+        ApplyFilter(&tempFilterSrc, &tempFilterDest);
     }
 
     set_render_target(tempShakeTexture);
@@ -3282,7 +3282,7 @@ void Graphics::render(void)
 {
     if (gameScreen.badSignalEffect)
     {
-        ApplyFilter(tempFilterSrc, tempFilterDest);
+        ApplyFilter(&tempFilterSrc, &tempFilterDest);
     }
 
     set_render_target(NULL);
diff --git a/desktop_version/src/GraphicsUtil.cpp b/desktop_version/src/GraphicsUtil.cpp
index ba478d4e..fc990609 100644
--- a/desktop_version/src/GraphicsUtil.cpp
+++ b/desktop_version/src/GraphicsUtil.cpp
@@ -144,43 +144,50 @@ void UpdateFilter(void)
     }
 }
 
-void ApplyFilter(SDL_Surface* src, SDL_Surface* dest)
+void ApplyFilter(SDL_Surface** src, SDL_Surface** dest)
 {
-    if (src == NULL)
-    {
-        src = SDL_CreateRGBSurface(0, SCREEN_WIDTH_PIXELS, SCREEN_HEIGHT_PIXELS, 32, 0, 0, 0, 0);
-    }
-    if (dest == NULL)
-    {
-        dest = SDL_CreateRGBSurface(0, SCREEN_WIDTH_PIXELS, SCREEN_HEIGHT_PIXELS, 32, 0, 0, 0, 0);
-    }
     if (src == NULL || dest == NULL)
     {
+        SDL_assert(0 && "NULL src or dest!");
         return;
     }
 
-    const int result = SDL_RenderReadPixels(gameScreen.m_renderer, NULL, 0, src->pixels, src->pitch);
+    if (*src == NULL)
+    {
+        *src = SDL_CreateRGBSurface(0, SCREEN_WIDTH_PIXELS, SCREEN_HEIGHT_PIXELS, 32, 0, 0, 0, 0);
+    }
+    if (*dest == NULL)
+    {
+        *dest = SDL_CreateRGBSurface(0, SCREEN_WIDTH_PIXELS, SCREEN_HEIGHT_PIXELS, 32, 0, 0, 0, 0);
+    }
+    if (*src == NULL || *dest == NULL)
+    {
+        WHINE_ONCE_ARGS(("Could not create temporary surfaces: %s", SDL_GetError()));
+        return;
+    }
+
+    const int result = SDL_RenderReadPixels(gameScreen.m_renderer, NULL, 0, (*src)->pixels, (*src)->pitch);
     if (result != 0)
     {
-        SDL_FreeSurface(src);
+        SDL_FreeSurface(*src);
         WHINE_ONCE_ARGS(("Could not read pixels from renderer: %s", SDL_GetError()));
         return;
     }
 
     const int red_offset = rand() % 4;
 
-    for (int x = 0; x < src->w; x++)
+    for (int x = 0; x < (*src)->w; x++)
     {
-        for (int y = 0; y < src->h; y++)
+        for (int y = 0; y < (*src)->h; y++)
         {
             const int sampley = (y + (int) graphics.lerp(oldscrollamount, scrollamount)) % 240;
 
-            const SDL_Color pixel = ReadPixel(src, x, sampley);
+            const SDL_Color pixel = ReadPixel(*src, x, sampley);
 
             Uint8 green = pixel.g;
             Uint8 blue = pixel.b;
 
-            const SDL_Color pixel_offset = ReadPixel(src, SDL_min(x + red_offset, 319), sampley);
+            const SDL_Color pixel_offset = ReadPixel(*src, SDL_min(x + red_offset, 319), sampley);
             Uint8 red = pixel_offset.r;
 
             double mult;
@@ -216,9 +223,9 @@ void ApplyFilter(SDL_Surface* src, SDL_Surface* dest)
             blue = SDL_max(blue - (distX + distY), 0);
 
             const SDL_Color color = {red, green, blue, pixel.a};
-            DrawPixel(dest, x, y, color);
+            DrawPixel(*dest, x, y, color);
         }
     }
 
-    SDL_UpdateTexture(graphics.gameTexture, NULL, dest->pixels, dest->pitch);
+    SDL_UpdateTexture(graphics.gameTexture, NULL, (*dest)->pixels, (*dest)->pitch);
 }
diff --git a/desktop_version/src/GraphicsUtil.h b/desktop_version/src/GraphicsUtil.h
index 3552f40b..cb59df98 100644
--- a/desktop_version/src/GraphicsUtil.h
+++ b/desktop_version/src/GraphicsUtil.h
@@ -12,6 +12,6 @@ void DrawPixel(SDL_Surface* surface, int x, int y, SDL_Color color);
 SDL_Color ReadPixel(const SDL_Surface* surface, int x, int y);
 
 void UpdateFilter(void);
-void ApplyFilter(SDL_Surface* src, SDL_Surface* dest);
+void ApplyFilter(SDL_Surface** src, SDL_Surface** dest);
 
 #endif /* GRAPHICSUTIL_H */