Strange scaling in Wesnoth #279
Comments
|
Can you grab the latest SDL3 main code? We just fixed a similar problem for Super Meat Boy. |
|
Also note that, unlike libsdl-org/SDL#12079, I can reproduce this in Wayland and Xorg, I'm using integer scaling (2x), and switching to 1x scaling didn't help. |
|
I'll see if I can reproduce it here. |
|
This doesn't reproduce for me, but resizing the window for any amount of time will result in memory corruption and a crash, which I'm assuming is either related, or at least needs to be fixed before we can go further on this issue. I'll do a more thorough debugging on this soon and report back. |
|
It makes sense that it would be related. The behavior is somewhat unpredictable. |
|
Okay, so Wesnoth is freeing the window surface, which is illegal in SDL2, but apparently doesn't explode like it does in sdl2-compat. I assume this surface is flagged as SDL_DONTFREE in SDL2, but since we're using a property to make the SDL3 destroy the SDL2 surface, the pointer is already invalid when it gets to SDL_FreeSurface(). |
|
Do we just need to put SDL_SURFACE_DONTFREE back in the public flags? |
|
No, because the pointer is dead as soon as SDL_GetWindowSurface() runs and SDL3 destroys the old surface internally, so we can't safely dereference it in SDL_FreeSurface() to see if SDL_DONTFREE is set. This sdl2-compat patch fixes the problem here, but it's a little hacky. That's probably fine because we're working around what's effectively a bug in Wesnoth that happened to work out until now. It also adds SDL_DONTFREE stuff, but the actual fix is tracking old pointers. Should I commit it? diff --git a/src/sdl2_compat.c b/src/sdl2_compat.c
index c425b4e..c5102a8 100644
--- a/src/sdl2_compat.c
+++ b/src/sdl2_compat.c
@@ -802,6 +802,7 @@ BOOL WINAPI _DllMainCRTStartup(HANDLE dllhandle, DWORD reason, LPVOID reserved)
/* Some SDL2 state we need to keep... */
/* !!! FIXME: unify coding convention on the globals: some are MyVariableName and some are my_variable_name */
+static SDL2_Surface *OldWindowSurfaces[16];
static SDL2_EventFilter EventFilter2 = NULL;
static void *EventFilterUserData2 = NULL;
static SDL_mutex *EventWatchListMutex = NULL;
@@ -815,7 +816,6 @@ static SDL_SensorID *sensor_list = NULL;
static int num_sensors = 0;
static SDL_HapticID *haptic_list = NULL;
static int num_haptics = 0;
-
static SDL_mutex *joystick_lock = NULL;
static SDL_mutex *sensor_lock = NULL;
@@ -3136,10 +3136,23 @@ SDL_CreateRGBSurfaceWithFormatFrom(void *pixels, int width, int height, int dept
SDL_DECLSPEC void SDLCALL
SDL_FreeSurface(SDL2_Surface *surface)
{
+ const int total = (int) (SDL_arraysize(OldWindowSurfaces));
+ int i;
+
if (!surface) {
return;
}
+ for (i = 0; (i < total) && OldWindowSurfaces[i]; i++) {
+ if (OldWindowSurfaces[i] == surface) {
+ return; /* this is a pointer from SDL_GetWindowSurface--a bug in the app--refuse to free it. */
+ }
+ }
+
+ if (surface->flags & SDL_DONTFREE) {
+ return;
+ }
+
if (--surface->refcount > 0) {
return;
}
@@ -8300,7 +8313,32 @@ SDL_GetSurfaceBlendMode(SDL2_Surface *surface, SDL_BlendMode *blendMode)
SDL_DECLSPEC SDL2_Surface * SDLCALL
SDL_GetWindowSurface(SDL_Window *window)
{
- return Surface3to2(SDL3_GetWindowSurface(window));
+ SDL2_Surface *surface2 = Surface3to2(SDL3_GetWindowSurface(window));
+ if (surface2) {
+ surface2->flags |= SDL_DONTFREE;
+ }
+
+ if (surface2) {
+ /* if the window was resized, SDL_GetWindowSurface() will destroy the previous surface and create a new one.
+ This takes the previous SDL2 surface with it. It's not legal to SDL_FreeSurface() a window surface, but
+ in SDL2, it didn't dereference free'd memory to do so, so we need to keep track of old pointers in case
+ an app tries to free them. */
+ int i;
+ const int total = (int) (SDL_arraysize(OldWindowSurfaces));
+ for (i = 0; i < total; i++) {
+ if (OldWindowSurfaces[i] == surface2) {
+ break;
+ }
+ }
+
+ /* just keep the last X surfaces; this is a hack to keep buggy legacy code running. */
+ if (i == total) {
+ SDL3_memmove(&OldWindowSurfaces[1], &OldWindowSurfaces[0], sizeof (OldWindowSurfaces) - sizeof (OldWindowSurfaces[0]));
+ OldWindowSurfaces[0] = surface2;
+ }
+ }
+
+ return surface2;
}
SDL_DECLSPEC int SDLCALL |
|
Hmm...better option: Let's not use Surface3to2() in SDL_GetWindowSurface(), and instead wrap it in a custom SDL2_Surface that is assigned to a window property, and updated with the latest SDL3 surface when SDL_GetWindowSurface() is called. The SDL2_Surface is free'd when the window is destroyed, and is marked SDL_DONTFREE. This way it stays valid when SDL_FreeSurface is called, the pointer doesn't change between SDL_GetWindowSurface calls, the DONTFREE protects it, and we don't risk a pointer getting reused that's in that OldWindowSurfaces array. |
|
This is somewhat cleaner, but it still crashes the wesnoth packaged for Ubuntu 24.04 (but totally fixes the problem in the build I made from the same version's source code myself, with both Valgrind and AddressSanitizer happy with it). I'm leaving it here in case we want to dig further into it...but I'm going with the original patch for now. diff --git a/src/sdl2_compat.c b/src/sdl2_compat.c
index c425b4e..8fd95dc 100644
--- a/src/sdl2_compat.c
+++ b/src/sdl2_compat.c
@@ -806,6 +806,7 @@ static SDL2_EventFilter EventFilter2 = NULL;
static void *EventFilterUserData2 = NULL;
static SDL_mutex *EventWatchListMutex = NULL;
static EventFilterWrapperData *EventWatchers2 = NULL;
+static SDL2_Surface *LastWindowSurface = NULL;
static SDL2_bool relative_mouse_mode = SDL2_FALSE;
static SDL_JoystickID *joystick_list = NULL;
static int num_joysticks = 0;
@@ -2759,8 +2760,6 @@ static SDL2_Surface *CreateSurface2from3(SDL_Surface *surface3)
/* Link the surfaces */
surface->map = (SDL_BlitMap *)surface3;
- SDL3_SetPointerPropertyWithCleanup(SDL3_GetSurfaceProperties(surface3), PROP_SURFACE2, surface, CleanupSurface2, NULL);
-
surface->format = SDL_AllocFormat(surface3->format);
if (!surface->format) {
SDL_FreeSurface(surface);
@@ -2800,6 +2799,9 @@ static SDL2_Surface *Surface3to2(SDL_Surface *surface)
surface2 = (SDL2_Surface *)SDL3_GetPointerProperty(SDL3_GetSurfaceProperties(surface), PROP_SURFACE2, NULL);
if (!surface2) {
surface2 = CreateSurface2from3(surface);
+ if (surface2) {
+ SDL3_SetPointerPropertyWithCleanup(SDL3_GetSurfaceProperties(surface), PROP_SURFACE2, surface2, CleanupSurface2, NULL);
+ }
}
}
return surface2;
@@ -3136,7 +3138,7 @@ SDL_CreateRGBSurfaceWithFormatFrom(void *pixels, int width, int height, int dept
SDL_DECLSPEC void SDLCALL
SDL_FreeSurface(SDL2_Surface *surface)
{
- if (!surface) {
+ if (!surface || (surface == LastWindowSurface) || (surface->flags & SDL_DONTFREE)) {
return;
}
@@ -8300,7 +8302,43 @@ SDL_GetSurfaceBlendMode(SDL2_Surface *surface, SDL_BlendMode *blendMode)
SDL_DECLSPEC SDL2_Surface * SDLCALL
SDL_GetWindowSurface(SDL_Window *window)
{
- return Surface3to2(SDL3_GetWindowSurface(window));
+ SDL2_Surface *surface2 = NULL;
+
+ SDL_Surface *surface3 = SDL3_GetWindowSurface(window);
+ if (surface3) {
+ /* Window surfaces in SDL3 are rebuilt when the window resizes, so we keep a separate SDL2 surface associated with the window.
+ This keeps the surface pointer returned here consistent, but also helps us protect against apps that incorrectly attempt
+ to call SDL_FreeSurface() on the window surface. */
+ const SDL_PropertiesID winprops = SDL3_GetWindowProperties(window);
+ if (winprops) {
+ surface2 = (SDL2_Surface *)SDL3_GetPointerProperty(winprops, PROP_SURFACE2, NULL);
+ if (!surface2) {
+ surface2 = CreateSurface2from3(surface3);
+ if (!surface2) {
+ return NULL;
+ }
+ SDL3_SetPointerPropertyWithCleanup(winprops, PROP_SURFACE2, surface2, CleanupSurface2, NULL);
+ }
+
+ if (surface2->map != (SDL_BlitMap *)surface3) { /* we got a new window surface from SDL3? */
+ surface2->map = (SDL_BlitMap *)surface3;
+ surface2->flags = (surface3->flags & SHARED_SURFACE_FLAGS) | SDL_DONTFREE;
+ SDL3_GetSurfaceClipRect(surface3, &surface2->clip_rect);
+ surface2->w = surface3->w;
+ surface2->h = surface3->h;
+ surface2->pixels = surface3->pixels;
+ surface2->pitch = surface3->pitch;
+ /* this runs under the assumption the pixel format, etc, doesn't change. */
+ }
+
+ /* this is a hack, but some games incorrectly call SDL_FreeSurface() on the window surface,
+ and additionally free it _after destroying the window_. These are (likely) single-window
+ games, so keep the last pointer around, so we can stop them, in case they try. */
+ LastWindowSurface = surface2;
+ }
+ }
+
+ return surface2;
}
SDL_DECLSPEC int SDLCALL |
It was never legal in SDL2 to do this, but due to circumstances, SDL2 wouldn't _crash_ if you did this, so this fixes games that do. Reference Issue #279.
|
@xordspar0, if possible, can you try the latest in sdl2-compat's revision control and see if it fixes your problem? This gets me past all the resizing crashes here, so if it doesn't fix your problem, we can at least start to actually track it down now. |
|
I still see the same behavior with 6c65578 and SDL 3.2.2. |
Is this a potential use case for making SDL_ValidObject() a public API? Then we could quickly validate whether a surface has been freed by SDL3 before crashing when an app calls into sdl2-compat with it. |
|
@icculus , I just noticed from your Valgrind stack trace that you're testing Wesnoth 1.16. I've been using Wesnoth 1.18. I just compiled Wesnoth 1.16 and tested it and it does indeed work fine. So it looks like the bug only exists with Wesnoth 1.18 + sdl2-compat. As long as I have Wesnoth compiled with debug symbols, I'll try to track down the place where they free the window surface. |
|
The window surface is complicated because it looks like they assign it to a few places that might also not be the window surface, but rather a legit offscreen surface. It would be better if that was sorted out, but the crash bug it triggered is gone, so I wouldn't spend too much time on it. |
|
I'll upgrade my Wesnoth install to reproduce the scaling issue...that was a good catch on your part! |
|
I think the FreeSurface issue may have been fixed at or around wesnoth/wesnoth@ad48f7782. I don't see Valgrind errors on window resize with Wesnoth 1.18.3 and sdl2-compat a29ef93 (just before your patch). For the sake of archeologists in the future, the stack trace was:CVideo::update_framebuffer() (video.cpp:195): surface::operator=(surface const&) (surface.hpp:53): surface::assign_surface_internal(SDL_Surface*) (surface.cpp:72): |
|
Scaling issue does indeed reproduce for me using the latest git revision of Wesnoth. I'll dig into this in the morning. |
|
I'm going to take a look at this. Between the resizing fixes and surface lifetime improvements I'm hopeful that we've either fixed this or made it easier to figure out what's going on. |
|
I can reproduce this with wesnoth 1.18.3, and it seems to be related to 4K resolutions and renderer scaling. |
|
So it looks like the issue is that Wesnoth is setting a logical size on a render target, which is supported in SDL2 but SDL3 assumes that the logical size only applies to the main viewport. So Wesnoth is creating a 4K texture, setting the logical size to 1280x720, rendering 1280x720, then setting the render target to NULL, and rendering that 4K texture using a logical size of 1280x720. I'm going to see if I can add a testautomation case for this, to make it easier to debug. As an aside, SDL2 returns the logical scale (3) in this case, and SDL3 doesn't include the logical scale and returns scale 1. This doesn't affect Wesnoth, but should probably be fixed as well. |
|
Okay, my theory may have been wrong, here's my test case, which works fine: diff --git a/test/testautomation_render.c b/test/testautomation_render.c
index 45198a826..226f38414 100644
--- a/test/testautomation_render.c
+++ b/test/testautomation_render.c
@@ -811,6 +811,38 @@ int render_testBlitBlend(void *arg)
return TEST_COMPLETED;
}
+/**
+ * @brief Tests logical size handling
+ */
+int render_testLogicalSize(void *arg)
+{
+ SDL_Texture *target;
+ SDL_Rect rect = { 0, 0, TESTRENDER_SCREEN_W / 2, TESTRENDER_SCREEN_H / 2 };
+ SDL_Surface *referenceSurface;
+
+ target = SDL_CreateTexture(renderer, SDL_PIXELFORMAT_ARGB8888, SDL_TEXTUREACCESS_TARGET, TESTRENDER_SCREEN_W, TESTRENDER_SCREEN_H);
+
+ SDL_SetRenderTarget(renderer, target);
+ SDL_SetRenderDrawColor(renderer, 0, 0, 0, 0);
+ SDL_RenderClear(renderer);
+ SDL_RenderSetLogicalSize(renderer, rect.w, rect.h);
+ SDL_SetRenderDrawColor(renderer, 0, 255, 0, 255);
+ SDL_RenderFillRect(renderer, &rect);
+
+ SDL_SetRenderTarget(renderer, NULL);
+ SDL_RenderSetLogicalSize(renderer, rect.w, rect.h);
+ SDL_RenderCopy(renderer, target, NULL, &rect);
+
+ referenceSurface = SDLTest_ImagePrimitives();
+ referenceSurface = SDL_CreateRGBSurface(0, TESTRENDER_SCREEN_W, TESTRENDER_SCREEN_H, 32, RENDER_COMPARE_RMASK, RENDER_COMPARE_GMASK, RENDER_COMPARE_BMASK, RENDER_COMPARE_AMASK);
+ SDL_FillRect(referenceSurface, NULL, SDL_MapRGB(referenceSurface->format, 0, 255, 0));
+ _compare(referenceSurface, ALLOWABLE_ERROR_OPAQUE);
+ SDL_FreeSurface(referenceSurface);
+ referenceSurface = NULL;
+
+ return TEST_COMPLETED;
+}
+
/**
* @brief Checks to see if functionality is supported. Helper function.
*/
@@ -1161,9 +1193,13 @@ static const SDLTest_TestCaseReference renderTest7 = {
(SDLTest_TestCaseFp)render_testBlitBlend, "render_testBlitBlend", "Tests blitting with blending", TEST_DISABLED
};
+static const SDLTest_TestCaseReference renderTest8 = {
+ (SDLTest_TestCaseFp)render_testLogicalSize, "render_testLogicalSize", "Tests logical size handling", TEST_ENABLED
+};
+
/* Sequence of Render test cases */
static const SDLTest_TestCaseReference *renderTests[] = {
- &renderTest1, &renderTest2, &renderTest3, &renderTest4, &renderTest5, &renderTest6, &renderTest7, NULL
+ &renderTest1, &renderTest2, &renderTest3, &renderTest4, &renderTest5, &renderTest6, &renderTest7, &renderTest8, NULL
};
/* Render test suite (global) */ |
|
Running |
|
You're right that it's related to renderer scaling at least. When I disable automatic scaling and set the scale to 1 in the display menu in Wesnoth it works fine. |
|
@icculus, this fixes the bug... can it be this easy? diff --git a/src/render/SDL_render.c b/src/render/SDL_render.c
index 5744bcd11..ac6649116 100644
--- a/src/render/SDL_render.c
+++ b/src/render/SDL_render.c
@@ -2634,18 +2634,23 @@ static void UpdateLogicalPresentation(SDL_Renderer *renderer)
}
}
- renderer->main_view.logical_scale.x = (logical_w > 0.0f) ? renderer->logical_dst_rect.w / logical_w : 0.0f;
- renderer->main_view.logical_scale.y = (logical_h > 0.0f) ? renderer->logical_dst_rect.h / logical_h : 0.0f;
- renderer->main_view.current_scale.x = renderer->main_view.scale.x * renderer->main_view.logical_scale.x;
- renderer->main_view.current_scale.y = renderer->main_view.scale.y * renderer->main_view.logical_scale.y;
- renderer->main_view.logical_offset.x = renderer->logical_dst_rect.x;
- renderer->main_view.logical_offset.y = renderer->logical_dst_rect.y;
-
- UpdateMainViewDimensions(renderer); // this will replace pixel_w and pixel_h while making sure the dpi_scale is right.
- renderer->main_view.pixel_w = (int) renderer->logical_dst_rect.w;
- renderer->main_view.pixel_h = (int) renderer->logical_dst_rect.h;
- UpdatePixelViewport(renderer, &renderer->main_view);
- UpdatePixelClipRect(renderer, &renderer->main_view);
+ renderer->view->logical_scale.x = (logical_w > 0.0f) ? renderer->logical_dst_rect.w / logical_w : 0.0f;
+ renderer->view->logical_scale.y = (logical_h > 0.0f) ? renderer->logical_dst_rect.h / logical_h : 0.0f;
+ renderer->view->current_scale.x = renderer->view->scale.x * renderer->view->logical_scale.x;
+ renderer->view->current_scale.y = renderer->view->scale.y * renderer->view->logical_scale.y;
+ renderer->view->logical_offset.x = renderer->logical_dst_rect.x;
+ renderer->view->logical_offset.y = renderer->logical_dst_rect.y;
+
+ if (renderer->view == &renderer->main_view) {
+ // this will replace pixel_w and pixel_h while making sure the dpi_scale is right.
+ // FIXME: We're about to clobber pixel_w and pixel_h, should we re-run this code
+ // whenever we would call UpdateMainViewDimensions() elsewhere?
+ UpdateMainViewDimensions(renderer);
+ }
+ renderer->view->pixel_w = (int) renderer->logical_dst_rect.w;
+ renderer->view->pixel_h = (int) renderer->logical_dst_rect.h;
+ UpdatePixelViewport(renderer, renderer->view);
+ UpdatePixelClipRect(renderer, renderer->view);
}
bool SDL_SetRenderLogicalPresentation(SDL_Renderer *renderer, int w, int h, SDL_RendererLogicalPresentation mode) |
|
If logical presentation in SDL3 is meant to affect render targets, then yes, that's probably correctly. Otherwise, no, and we probably need to adjust the scaling in sdl2-compat. |
Right now it’s undefined. If it worked that way in SDL2 and people relied on it, then it seems reasonable to also support in SDL3. Conceptually I can see someone making a render target the size of the window and wanting to use logical scale on it the same way they would the screen, but we named it logical presentation thinking about it as window presentation. I’m fine either way, thoughts? Also, did you see the FIXME in my patch? What should we do about that? |
|
Mmm...I dunno. The value of logical presentation is that your game is designed for one size, but it can work when the window resizes, or fullscreen-desktop gives you whatever resolution, or HighDPI comes into play. None of that applies to a render target. In fact, if they're trying to do it to match the physical window and then blit it over, they're going to get double-letterboxing. Wesnoth doesn't really want logical presentation in their render target, they just want to scale it to the window size, right? If that's the case, we should probably mess with this in sdl2-compat. But I can be convinced otherwise! |
|
You've convinced me! I'll bounce this back to you, and you'll probably need to adjust 6fc3016 to account for this. |
|
Okay, I've talked myself out of this while jumping through a ton of hoops in sdl2-compat. :) Specifically: my thing about double-letterboxing is nonsense. Yes, that might happen if you set the logical size on both the render target and the window, but a) don't do that and b) there's an argument to be made that you should be able to make a render target the size of the window, render to it with logical presentation, and then blit the entire thing without logical presentation to the window. That would be on the app to manage that correctly, but it's not very challenging to do so. So your patch up there is probably the right thing to do. For the FIXME in that patch: yes, we probably do need to call that code...but it's probably more like merge UpdateMainViewDimensions() into UpdateLogicalPresentation() and make UpdateLogicalPresentation() the only thing that gets called elsewhere. I'm going to look the code over real quick and make sure this is actually correct, then apply that patch. |
|
Turns out UpdateMainViewDimensions() is only called at window creation and UpdateLogicalPresentation(), so we're probably okay here as-is, minus a few tweaks. |
Initial work on this patch was by @slouken, with further work from me. His work was perfect, the mistakes are all mine. :) Fixes libsdl-org/sdl2-compat#279
Initial work on this patch was by @slouken, with further work from me. His work was perfect, the mistakes are all mine. :) Fixes libsdl-org/sdl2-compat#279
|
Haha, did I say "a few tweaks"? This turned out to be a large patch. I went through the whole renderer to make sure everything worked with both render target textures and the main window and updated documentation to clarify what state was unique to each render target and how it all interacts. I think it's a nice, but perhaps intimidating, improvement. Wesnoth definitely works with it, too, which was the original goal. :) |
|
Okay, Wesnoth should be good to go when we ship the next SDL3 and sdl2-compat updates (which should be very soon now). |
|
Thank you very much! |
👋 I'm one of the Arch Linux users affected by sdl2-compat replacing sdl2. I'm super excited to start using SDL2 and look forward to getting everything working with sdl2-compat!
When I launch Wesnoth in fullscreen mode on my 4k monitor, it renders squished into the top left corner of the screen. When I switch to windowed mode (ctrl+f toggles fullscreen in Wesnoth for anyone wondering like I was), it still gets squished into the corner until the window is smaller than a certain size. Mouse input seems to be working normally, i.e. if I click the tiny buttons nothing happens, but if I move my mouse over the the black area I can find the buttons and click them.
I'm using Gnome on Wayland, but I can also reproduce the issue on Xorg.
I saw the issue first with sdl2-compat 2.30.50 as shipped by Arch, but I can also reproduce with a freshly cloned and compiled sdl2-compat.
wesnoth-scaling.mp4
The text was updated successfully, but these errors were encountered: