SDLWindow: MSAA fallback retry + null-safe CreateWindow#2059
Conversation
|
|
||
| if (!sdlWindow && (flags & (WINDOW_FLAG_HW_AA | WINDOW_FLAG_HW_AA_HIRES))) { | ||
|
|
||
| // Retry without antialiasing — emulators and some devices don't support MSAA |
There was a problem hiding this comment.
Maybe best to remove the emdash as it may cause issues in some editors
There was a problem hiding this comment.
Good catch — replaced with an ASCII hyphen in cbc833f.
|
|
||
| } | ||
|
|
||
| return window; |
There was a problem hiding this comment.
We have to make sure all callers of CreateWindow check for null with this change. We should also return nullptr rather than NULL since it is c++ code.
There was a problem hiding this comment.
Done — nullptr in cbc833f. On the caller side: both C++ callers (lime_window_create / hl_window_create in ExternalInterface.cpp) pass the result straight into CFFIPointer/HLCFFIPointer, which already null-check (CFFIPointer.cpp returns alloc_null() for a null pointer; HLCFFIPointer returns 0), so the Haxe side gets a real null. NativeWindow.create() did guard the width/height/x/y block with if (handle != null), but then called lime_window_get_scale(handle) and lime_window_get_context_type(handle) unconditionally — those deref a null handle. Fixed in the same commit by replacing the partial guard with an early return right after lime_window_create.
| int width; | ||
| int height; | ||
| int width = 0; | ||
| int height = 0; |
There was a problem hiding this comment.
Can SDL_GetWindowSize actually exit without writing these values? Sdl2 docs don't mention that case: https://wiki.libsdl.org/SDL2/SDL_GetWindowSize
There was a problem hiding this comment.
It can, via CHECK_WINDOW_MAGIC. In SDL2 (src/video/SDL_video.c) SDL_GetWindowSize starts with CHECK_WINDOW_MAGIC(window, );, which early-returns before writing *w/*h when the video subsystem is uninitialized, window is NULL, or window->magic doesn't match (a destroyed/stale window — the teardown race this guards). The wiki doesn't mention it but the source confirms it, so the = 0 init is defensive against exactly that path; the happy path is unaffected.
There was a problem hiding this comment.
Just to provide the source:
https://github.com/libsdl-org/SDL/blob/f0d1101920864995c0c265128cfd7b96eb959ac5/src/video/SDL_video.c#L2399
The docs also say:
NULL can safely be passed as the w or h parameter if the width or height value is not desired.
Which can make the code cleaner and avoid setting the one we don't need to 0.
Also as an aside, it looks like in SDL3 we will be able to check the return value of SDL_GetWindowSize to see if it failed: https://wiki.libsdl.org/SDL3/SDL_GetWindowSize
There was a problem hiding this comment.
Good call — adopted in 16a34b6. GetWidth/GetHeight now pass NULL for the dimension they don't return and only init the one they do (still = 0 so the CHECK_WINDOW_MAGIC early-return path stays defensive). Thanks for the SDL source links and the SDL3 return-value note.
Address review on openfl#2059 (tobil4sk): - SDLWindow.cpp: ASCII hyphen instead of em-dash in MSAA-retry comment; return nullptr instead of NULL from CreateWindow (C++). - NativeWindow.hx: create() bailed only partially on a null window handle; lime_window_get_scale/get_context_type(handle) were called unconditionally and deref a null handle. Replace the partial if (handle != null) guard with an early return after lime_window_create. C++ callers (ExternalInterface) were already null-safe via CFFIPointer/HLCFFIPointer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolve openfl#2059 conflicts after upstream 416a408 (alwaysOnTop) and a5d2a49 (main-loop pacing/vsync). SDLWindow.cpp auto-merged; the NativeWindow.hx create() handle block was resolved keeping the early return on a null window handle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| int width; | ||
| int height; | ||
| int width = 0; | ||
| int height = 0; |
There was a problem hiding this comment.
Just to provide the source:
https://github.com/libsdl-org/SDL/blob/f0d1101920864995c0c265128cfd7b96eb959ac5/src/video/SDL_video.c#L2399
The docs also say:
NULL can safely be passed as the w or h parameter if the width or height value is not desired.
Which can make the code cleaner and avoid setting the one we don't need to 0.
Also as an aside, it looks like in SDL3 we will be able to check the return value of SDL_GetWindowSize to see if it failed: https://wiki.libsdl.org/SDL3/SDL_GetWindowSize
| if (handle == null) | ||
| { | ||
| parent.__width = NativeCFFI.lime_window_get_width(handle); | ||
| return; |
There was a problem hiding this comment.
It seems this early return causes frameRate to not be set. Not sure what the expected state is for a window that failed to launch.
There was a problem hiding this comment.
Fixed in c668007 — frameRate = resolvedFrameRate; now runs before the early return, so a window that failed to launch still has frameRate populated for getFrameRate()'s field fallback. It's a plain field write (no handle deref) and resolvedFrameRate is already computed above, so it's safe on the failure tail. setTextInputEnabled(false) is still skipped, but that one only configures SDL state via the (now null) handle and is internally handle-guarded, so there's nothing meaningful to set for a window that never opened.
Per review: SDL_GetWindowSize accepts NULL for an unwanted w/h, so GetWidth/GetHeight only request the value they return and init just that one to 0 (still defensive against the CHECK_WINDOW_MAGIC early return path). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per review: keep frameRate populated even when window creation fails so getFrameRate()'s field fallback is correct. resolvedFrameRate is already computed and the assignment is a plain field write (no handle deref). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Three small defensive fixes in
SDLWindow.cppthat we've been carryingin our production fork (https://github.com/soccertutor/lime) for ~1 year+
on native OpenFL apps shipping to macOS, Windows, Android, and iPad:
MSAA fallback on window creation failure. When
flagsincludeWINDOW_FLAG_HW_AA/WINDOW_FLAG_HW_AA_HIRESand the initialSDL_CreateWindowfails, retry once with multisample buffersdisabled. The Android Studio emulator (AVD) rejects window requests
with multisampling, even though it supports the OpenGL context
itself. Today the only signal a user gets is the generic
"Could not create SDL window" message and a hard crash — the same
app launches fine on physical Android hardware with MSAA support.
The fallback turns "no window at all" into "a window without
antialiasing."
int width, height;→int width = 0, height = 0;inGetWidth()/GetHeight(). IfSDL_GetWindowSizereturnswithout writing one of the values (early-out on a torn-down
window), the previous version returned a stack-uninitialized int.
Defensive only — observable as occasional one-frame size jumps
during shutdown.
Null-safe
CreateWindow. If theSDLWindowconstructorfinishes but
sdlWindow == NULL(creation failed and we alreadylogged),
deletethe half-constructed wrapper and return NULLinstead of handing back an object that holds a null SDL handle.
Without this, every subsequent caller has to defensively null-check
sdlWindowon its own — and many don't, which manifests as aNULL deref deep inside event dispatch on the next frame.
Diff
Test
The MSAA fallback path is the one with the most observable behaviour
change. Tested in production on an Android AVD that previously refused
to launch any Lime app with
<window antialiasing=\"4\"/>inproject.xml— with this patch the app launches successfully withantialiasing dropped to 0 on the same emulator image. No regression
on physical Android hardware that supports MSAA (the second
SDL_CreateWindowis only attempted when the first failed and MSAAwas requested).
The width/height defensive init has no observable behaviour change in
the happy path, only narrows the window during teardown races.
The null-safe
CreateWindowis dead code on platforms where SDLwindow creation never half-succeeds, but on the AVD failure path
described above the original code returned a window object pointing
to NULL — every
SDL_*call against it then segfaulted. With thispatch the call site sees a clean NULL and can fall back to
Application::onException/ display an error.Risk
MSAA flags now succeed with MSAA off. Apps that depend on receiving
an MSAA-enabled context (e.g. expecting
glIsEnabled(GL_MULTISAMPLE)to be true) and treat its absence as a fatal error would see
different behaviour. We've never seen that in practice — apps that
want MSAA enable it via
<window antialiasing=\"N\"/>and treat"didn't get it" the same as "got it without".
SDLWindow.cpp.