-
-
Notifications
You must be signed in to change notification settings - Fork 189
Fix display.set_mode segfault after resizing #3501
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the fix! 🎉
re: memory leak, I am not seeing any noticeable evidence of that. I believe that even if there is a memory leak, it would probably not be a result of this PR.
Also, if someone knows another place in our code when the resizing could happen, make sure to notify me to add this new helper function there as well
There is some logic in pg_flip_internal
that is exactly like _check_window_resized
you may wanna update that snippet as well, just for code quality reasons. Nothing related to this fix per se.
I have tested this change on Ubuntu and found no issues, but I am also requesting reviews from Windows users to verify that this fixes the issue at hand.
What I mean by the memory leak is the following: import pygame
import random
import psutil
import os
pygame.display.set_mode((800, 600), flags=pygame.SCALED)
while True:
print(f"Memory usage: {psutil.Process(os.getpid()).memory_info().rss / 1024 / 1024:.2f} MB")
pygame.display.set_mode([random.randint(600, 800), random.randint(600, 800)], flags=pygame.RESIZABLE | pygame.SCALED) It is probably happening because with this change the old surface is not freed. However, if I free the surface, then I return to the old problem of segfault 🤣 . Really hard issue |
the old surface should be freed already by SDL, it cannot be freed on our side. |
@@ -1374,6 +1383,7 @@ pg_set_mode(PyObject *self, PyObject *arg, PyObject *kwds) | |||
surface = pgSurface_New2(surf, newownedsurf != NULL); | |||
} | |||
else { | |||
_check_window_resized(win, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm probably failing to notice something here, but why aren't we freeing the old surface here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because if we free then we get segmentation fault from the linked issue. You have more explanation in the discord link
This should fix the following code that segfaults
This also fixes #2571
TLDR: Changing size of the window invalidates surface returned by SDL_GetWindowSurface() as explained in https://wiki.libsdl.org/SDL2/SDL_GetWindowSurface, so this commit makes sure the module always has the reference to the right SDL_Surface (hopefully). There are other possible fixes for this, but this one to me looks the most elegant (unless we rewrite the entire module/function).
And the long explanation is available in our discord server https://discord.com/channels/772505616680878080/772940667231928360/1383591122101604402
Also, if someone knows another place in our code when the resizing could happen, make sure to notify me to add this new helper function there as well
EDIT: Looks like there is a memory leak with this change, will need to check if it happens without it
EDIT 2: I am pretty sure that the memory leak happens because of #3502