libsdl-org / SDL

Simple Directmedia Layer
https://libsdl.org
zlib License
9.82k stars 1.82k forks source link

SDL_SyncWindow can time out under normal conditions with a slow computer, with X11 backend #11239

Open slime73 opened 1 week ago

slime73 commented 1 week ago

Some context: I'm updating my app to use SDL3, things have mostly gone fine on other platforms but I'm trying to figure out several failures in the app's automated test suite (run on Github Actions CI) on Linux, when the tests validate that window state is consistent after some operations like minimize/maximize/restore. My code uses SDL_SyncWindow to try to make sure that state is consistent.

What I have discovered so far is that, with X11 in the CI test, something like this times out in the last SyncWindow after SetFullscreen(false):

SDL_SetWindowFullscreen(window, true);
SDL_SyncWindow(window);

SDL_SetWindowFullscreen(window, false);
SDL_SyncWindow(window);

The actual code that's reproducing it for me isn't identical to that. I don't have a standalone repro project, but you can see the SyncWindow failure being reported here https://github.com/slime73/love-experiments/actions/runs/11374433695/job/31643430814

I tried increasing the timeout duration from 100ms to 3000ms but it didn't seem to stop that issue, so I wonder if something is actually getting stuck?

Kontrabant commented 1 week ago

What kind of environment is being used for testing? Is there an actual window manager running, or is it a headless X11 session?

slime73 commented 1 week ago

It's using xvfb and openbox with Ubuntu 22.04, set up here: https://github.com/slime73/love-experiments/blob/main/.github/workflows/main.yml#L50-L59

The window tests take about 0.64 seconds total on average when SDL2 is used, and about 0.4 seconds total on average when SDL3 + SDL_SyncWindow is used, which is a little surprising to me as well.

Kontrabant commented 1 week ago

xvfb doesn't seem to support minimize, maximize, or fullscreen operations. Unlike SDL2, SDL3 doesn't assume that window operations will succeed, and doesn't set the corresponding status on a window until the desktop actually tells it that the operation succeeded via a corresponding event. If you request fullscreen and the desktop doesn't honor the request, the sync operation will time out and the fullscreen flag will remain unset.

slime73 commented 1 week ago

Interesting, thanks for the clarification. I'll try to get a user with Linux installed to run the tests on an actual desktop system.

It doesn't look like SDL_MinimizeWindow or SDL_MaximizeWindow or SDL_RestoreWindow will report failure from backend code right now, only if the window parameter is invalid or the backend has no implementation of those functions. Is that something that should/could change?

Kontrabant commented 1 week ago

I'm looking at your CI test script, and it looks like the test configuration runs the openbox window manager on top of xvfb, so those window operations should work since an actual window manager is present. I tried the SDL video test suite on that setup, and some tests fail due to the corresponding events arriving after the sync timeout period. I'll play around with tweaking the sync code parameters tomorrow, as it likely does just need more time on slower systems.

slime73 commented 1 week ago

I'd tried increasing the timeout to 10 seconds after my 3 second test and it still failed in the same spot with my tests, so maybe there are multiple overlapping issues or there's something misconfigured in my CI setup if it should be working.

Kontrabant commented 1 week ago

I played around with this a bit with SDL set to log events, and the fullscreen checks ran OK in github CI on my fork, so the earlier failure might have just been a fluke (still, it might not hurt to increase the X11 sync timeout just a bit for comfort). The main problem I see is that once the window is minimized by some test, it is never restored, and thus subsequent tests fail. Openbox might not allow programmatic unminimizing of windows, or SDL might not be triggering it correctly. My guess is the former, as windows unminimize correctly with SDL on other window managers.

If I comment out or rework just the tests that minimize the window, the tests pass:

https://github.com/Kontrabant/love/actions/runs/11389885446/job/31690222859 https://github.com/Kontrabant/love/commit/9b9b9eec3561b78981208f5ef6c3d37711695b89