We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 5147 - [Patch] KMSDRM: SetWindowFullscreen() failing with SDL_WINDOW_FULLSCREEN_DESKTOP + fix patch
Summary: [Patch] KMSDRM: SetWindowFullscreen() failing with SDL_WINDOW_FULLSCREEN_DESK...
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: video (show other bugs)
Version: HG 2.1
Hardware: All Linux
: P2 normal
Assignee: Sam Lantinga
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-19 20:08 UTC by Manuel Alfayate Corchete
Modified: 2020-06-02 23:57 UTC (History)
0 users

See Also:

Attachments
Patch that fixes SDL_SetWindowFullscreen() with the SDL_WINDOW_FULLSCREEN_DESKTOP flag on KMSDRM when a video mode change was previously made. (612 bytes, patch)
2020-05-19 20:08 UTC, Manuel Alfayate Corchete 		Details | Diff
V2 of the patch that fixes SDL_SetWindowFullscreen() with the SDL_WINDOW_FULLSCREEN_DESKTOP flag on KMSDRM when a video mode change was previously made. (1.09 KB, patch)
2020-05-20 21:27 UTC, Manuel Alfayate Corchete 		Details | Diff
V3 of the patch that fixes SDL_SetWindowFullscreen() with the SDL_WINDOW_FULLSCREEN_DESKTOP flag on KMSDRM when a video mode change was previously made. (1.51 KB, patch)
2020-05-21 10:07 UTC, Manuel Alfayate Corchete 		Details | Diff
V4 of the patch that fixes SDL_SetWindowFullscreen() with the SDL_WINDOW_FULLSCREEN_DESKTOP flag on KMSDRM when a video mode change was previously made. (1.87 KB, patch)
2020-05-21 12:45 UTC, Manuel Alfayate Corchete 		Details | Diff
V5 of the patch that fixes SDL_SetWindowFullscreen() with the SDL_WINDOW_FULLSCREEN_DESKTOP flag on KMSDRM when a video mode change was previously made. (1.87 KB, patch)
2020-05-30 17:49 UTC, Manuel Alfayate Corchete 		Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Alfayate Corchete 2020-05-19 20:08:51 UTC 
Created attachment 4349 [details]
Patch that fixes SDL_SetWindowFullscreen() with the SDL_WINDOW_FULLSCREEN_DESKTOP flag on KMSDRM when a video mode change was previously made.

Hi there,

This patch is needed so programs that do this work as expected:
1) Start in a different video mode than the mode used by the system and then...
2) Try to go fullscreen with the mode originally used by the system via SetWindowFullScreen() with the SDL_WINDOW_FULLSCREEN_DESKTOP flag.

An example would be pt2-clone in https://github.com/8bitbubsy/pt2-clone.
This program does this:

Starts with:

video.window = SDL_CreateWindow("", SDL_WINDOWPOS_CENTERED,
    SDL_WINDOWPOS_CENTERED, screenW, screenH, windowFlags);


and then, *IF* the user has configured it in fullscreen mode in its .ini, it tries to go fullscreen with the desktop mode:

SDL_SetWindowFullscreen(video.window, SDL_WINDOW_FULLSCREEN_DESKTOP);


This sequence of operations is currently failing because SDL_SetDisplayModeForDisplay() in SDL_video.c fails because display->desktop_mode is not being initialized with its correct value: SetDisplayMode() in SDL_kmsdrmvideo.c will not be able to set the mode because it detects the mode to have a driverdata of 0x0 ("if (!modedata)") and rightfully returns an error.

So, the included patch fixes this small problem, and programs that first change the video mode and then try to go fullscreen with the system video mode will now work.
The patch simply fixes an small omission, but its really needed now that dynamic video mode changing was implemented on the KMSDRM backend.
Comment 1 Manuel Alfayate Corchete 2020-05-19 20:48:32 UTC 
Please do not merge this patch yet. It is currently having some side-effects (pointer double-free) I am studying.
Comment 2 Manuel Alfayate Corchete 2020-05-20 21:27:23 UTC 
Created attachment 4351 [details]
V2 of the patch that fixes SDL_SetWindowFullscreen() with the SDL_WINDOW_FULLSCREEN_DESKTOP flag on KMSDRM when a video mode change was previously made.

This patch fixes SDL_SetWindowFullscreen() with the SDL_WINDOW_FULLSCREEN_DESKTOP flag on KMSDRM.
Now it works as expected (previous patch was a mistake as I was initializing the driverdata field with a non-related pointer).

This patch can be merged without problems.
Comment 3 Manuel Alfayate Corchete 2020-05-21 10:07:05 UTC 
Created attachment 4353 [details]
V3 of the patch that fixes SDL_SetWindowFullscreen() with the SDL_WINDOW_FULLSCREEN_DESKTOP flag on KMSDRM when a video mode change was previously made.

I have made yet another version of this patch, that also removes the forced SDL_WINDOWN_FULLSCREEN and SDL_WINDOW_OPENGL flags on windows created on the KMSDRM backend.
That was confusing some programs that detected SDL_Window flags, and caused misbehaviours like them not toggling fullscreen (from fullscreen into SDL_WINDOW_FULLSCREEN_DESKTOP, for example) when they should.
Comment 4 Manuel Alfayate Corchete 2020-05-21 12:24:39 UTC 
I would like to comment about the V3 patch:
Since SDL_WINDOW_FULLSCREEN is not stamped into all KMSDRM windows now, programs wanting to change physical video mode must pass the SDL_WINDOW_FULLSCREEN flag to SDL_CreateWindow() and SDL_SetWindowFullsreen().
This is because SDL_UpdateFullscreenMode() will only change fullscreen mode *if* the window is fullscreen: its perfectly reasonable, because it makes no sense to change the video mode for a non-fullscreen window.
Previous behaviour was to change the video mode always, because the SDL_WINDOW_FULLSCREEN flag was FORCED into all KMSDRM windows, but forcing the SDL_WINDOW_FULLSCREEN flag into all windows confuses programs that use SDL_GetWindowFlags() to find out if a window is fullscreen or not.

So I believe the new behaviour with this patch (changing video mode for fullscreen windows ONLY) is more coherent with SDL2 design and how is it supposed to work on non-windowed enviroments like KMSDRM.

Any thoughts on this? (It can easily reverted later, anyway).
Comment 5 Manuel Alfayate Corchete 2020-05-21 12:45:19 UTC 
Created attachment 4354 [details]
V4 of the patch that fixes SDL_SetWindowFullscreen() with the SDL_WINDOW_FULLSCREEN_DESKTOP flag on KMSDRM when a video mode change was previously made.

Create V4 and hopefully definitive version of this patch.
For total coherence with SDL2 desing and expected behaviour, programs that call SDL_CreateWindow() or SDL_SetWindowFullscreen() without SDL_WINDOW_FULLSCREEN or SDL_WINDOW_FULLSCREEN_DESKTOP will now appear on a window. No more stranbe behaviour now:

-If a program wants fullscreen, it has to pass SDL_WINDOW_FULLSCREEN or SDL_WINDOW_FULLSCREEN_DESKTOP. If that program passes a different resolution than the "desktop" one (there is no desktop in KMSDRM, so this means the mode-in-use by the system), a resolution change will be made.

-If a program wants to display in a window, a window it will get. But of course a non-fullscreen window does not allow display mode changes, as per SDL_UpdateFullscreenMode() and the most basic logic.
Comment 6 Sam Lantinga 2020-05-29 22:59:11 UTC 
V4 of the patch doesn't apply cleanly to Mercurial. Can you updated it and verify?

Thanks!
Comment 7 Manuel Alfayate Corchete 2020-05-30 17:49:50 UTC 
Created attachment 4363 [details]
V5 of the patch that fixes SDL_SetWindowFullscreen() with the SDL_WINDOW_FULLSCREEN_DESKTOP flag on KMSDRM when a video mode change was previously made.

@Sam Lantiga

I have done a new patch that works against latest libSDL2 sources.
Please apply, verify and merge if possible. Thanks!
Comment 8 Sam Lantinga 2020-06-02 23:57:49 UTC 
Patch added, thanks!
https://hg.libsdl.org/SDL/rev/d495c4d822cc