Nice @GranMinigun 👍🏻

Oh, and it's completely untested.

So who will test it? 🤔

Dunno. It doesn't let me. Says surface is deprecated or something.

Maybe users should? I heard Microsoft successfully utilizes that strategy in their products.

Ok, so I've done some testing 😎 Works well when you set output in the config, surface -> opengl fallback works too.

But there's a regression compared to main. If you start with output = opengl in the config, then switch to texture in the command line with output = texture, then issue output = opengl again, you'll get the following error:

Could not create OpenGL context, switching back to texture

Prior to this you could switch back and forth between texture and OpenGL in the command line indefinitely. So something went wrong here.

Tested on macOS.

Reproduced: "The specified window isn't an OpenGL window"

Just to be sure we're having the same error:

diff --git a/src/gui/sdlmain.cpp b/src/gui/sdlmain.cpp
index 576370596..fe87b8d5a 100644
--- a/src/gui/sdlmain.cpp
+++ b/src/gui/sdlmain.cpp
@@ -3284,7 +3284,7 @@ static void set_output(Section* sec, const bool wants_aspect_ratio_correction)
                } else {
                        sdl.opengl.context = SDL_GL_CreateContext(sdl.window);
                        if (sdl.opengl.context == nullptr) {
-                               LOG_WARNING("Could not create OpenGL context, switching back to texture");
+                               LOG_WARNING("Could not create OpenGL context, switching back to texture; error message: %s", SDL_GetError());
                                sdl.desktop.want_type = SCREEN_TEXTURE;
                        }
                }

Reproduced: "The specified window isn't an OpenGL window"

Just to be sure we're having the same error:

Yeah, I'm getting the same error:

Moving to draft for now. Too many things broke for visibly no reason:

• Toggling fullscreen mode hangs;
• Resizing window to smaller than initial size blanks it;
• Resizing mapper blanks it (but --startmapper isn't affected);
• Aforementioned output switching, which is an oversight on my side - the window isn't recreated.

I can't read. Ready for additional testing and review.

(Just realized output switching is still borked. Sorry, will fix it tomorrow fixed it now. I can't read.)

I can't read. Ready for additional testing and review.

(Just realized output switching is still borked. Sorry, will fix it tomorrow fixed it now. I can't read.)

Will test & review tomorrow then, when you think you're done 😄

Let me know when this is ready fro review and another round of testig @GranMinigun.

Unless you can comment on the things I pointed out in the first post, it was ready for a review for about a week. Just needs a rebase.

Ok, opengl <-> texture switching works now, but every time I switch to texture I'm getting this in the logs:

2023-08-28 21:36:24.647 | DISPLAY: Initialised 1103x827 windowed mode using bilinear scaling on display-0
2023-08-28 21:36:24.657 | SDL: Failed setting the OpenGL vsync state to off (0): No OpenGL context has been made current
2023-08-28 21:36:24.665 | SDL: Using 'metal' driver for 32-bit texture rendering
2023-08-28 21:36:24.665 | DISPLAY: VGA 720x400 16-colour text mode 03h at 70.087 Hz VFR, scaled to 2205x1654 with 1:1.35 (20:27) pixel aspect ratio

Doesn't seem right that I'm getting an OpenGL related error in texture mode.

Another one. Start up with default config, switch to fullscreen, execute output = texture. Hard crash on macOS.

You can switch back and forth between opengl/texture in windowed mode forever. This only happens in fullscreen.

By they way, the #else branch of SDL_VERSION_ATLEAST(2, 26, 0) is completely non-functional; if I force using that branch, that results in a crash at startup 100% of the time. So might wanna remove that whole branch altogether?

No. It won't compile on Ubuntu 20.xx (and readme mentions support for SDL 2.0.5), and SDL_GetWindowSizeInPixels() does the exact same thing under the hood, so you're hitting the exact same behaviour here.

Wiped event filter completely, it only fights with SDL's window handling. macOS freezes output during window resize anyway. Should be ready for another round of testing.

Wiped event filter completely, it only fights with SDL's window handling. macOS freezes output during window resize anyway. Should be ready for another round of testing.

Thanks that was my point; it seemed completely unnecessary.

Testing is producing good results, @GranMinigun.

I'm seeing a minor but inconsequential functional difference versus main on macOS:

On macOS (only) and in fullscreen mode, when manually flipping between output types, the main branch reconstructs and stays in fullscreen mode, where as SDL in the branch will fallback to a maximized window-mode at render-switch time. To see how this looks:

• main branch - https://kcgen.duckdns.org/main-fs-window-toggle.mp4
• PR branch - https://kcgen.duckdns.org/surface-pr-fs-window-toggle.mp4

Note: my 5-button PC mouse cursor tends to "go behind the scene" in all branches regularly, so please disregard that aspect; it's nothing to w/ this branch.

This window behavior appears to entirely be driven by SDL re-using its window but getting a new renderer - instead of our old hamfisted "rebuild everything from scratch" including blowing away the old SDL window, context, renderer, etc..

I'm quite OK with this, but wanted to mention it.

No doubt we might see more of these slight functional changes as the "old and wrong" ways are jettisoned in favour of doing things more correctly per SDL (and OpenGL)'s APIs.

Ok, opengl <-> texture switching works now, but every time I switch to texture I'm getting this in the logs:

SDL: Failed setting the OpenGL vsync state to off (0):
     No OpenGL context has been made current

@johnnovak -> thanks for the heads up on this. I also get this in the main branch (see log output in above main video). It's a bug I added w/ the VFR PR, so we can consider it out of scope from this PR. I'll follow up after this with a commit for it.

@johnnovak -> thanks for the heads up on this. I also get this in the main branch (see log output in above main video). It's a bug I added w/ the VFR PR, so we can consider it out of scope from this PR. I'll follow up after this with a commit for it.

Sounds good @kcgen. Talking about VFR, man, that thing keeps giving 😅 On my fixed 60 Hz monitor the default VFR mode with vsync off now works reliably in the sense that no frames are lost when the video output is only infrequently updated, but because the duping now happens at 1/10th of the host rate or something, you get visible tearing that "resolves itself" after around 60-80 ms. That very visible and it's not an authentic experience, to put it mildly.

As we have established, this can happen in any graphics or text mode; just text mode apps are the ones that tend to update the screen only sporadically, but I'm getting it all the time in the Quake video options menu, for example.

So you can have a look at some pont if you have capacity, otherwise I will because I find this pretty jarring. It even manifests at a simple command prompt; the blinking of the cursor and a normal typing rate is not enough to trigger new frames at a sufficiently high FPS to prevent this from happening 100% of the time.

I don't quite get it why the previous "dupe at the rate of the DOS mode" thing had to be removed.

This window behavior appears to entirely be driven by SDL re-using its window but getting a new renderer

No, it's still recreated, that's the only way to ensure the window has all necessary flags for whatever renderer is chosen. Why it goes maximized instead of fullscreen, I can't tell right now. I think Burrito noticed that behaviour in my sdlpain cleanup PR as well. Something's fishy.

This window behavior appears to entirely be driven by SDL re-using its window but getting a new renderer

No, it's still recreated, that's the only way to ensure the window has all necessary flags for whatever renderer is chosen. Why it goes maximized instead of fullscreen, I can't tell right now. I think Burrito noticed that behaviour in my sdlpain cleanup PR as well. Something's fishy.

I'm not seeing the PR changing or resetting the fullscreen boolean value, and combined with the fact that it switches to a /maximized/ window points to some state being partially lost or maybe the window manager doesn't let us go full screen the instant after the renderer is created (maybe there's some kind of host-side "settle-out" delay we need to wait before kicking in full screen, or something like that).

@johnnovak -> thanks for the heads up on this. I also get this in the main branch (see log output in above main video). It's a bug I added w/ the VFR PR, so we can consider it out of scope from this PR. I'll follow up after this with a commit for it.

Sounds good @kcgen. Talking about VFR, man, that thing keeps giving 😅 On my fixed 60 Hz monitor the default VFR mode with vsync off now works reliably in the sense that no frames are lost when the video output is only infrequently updated, but because the duping now happens at 1/10th of the host rate or something, you get visible tearing that "resolves itself" after around 60-80 ms. That very visible and it's not an authentic experience, to put it mildly.

As we have established, this can happen in any graphics or text mode; just text mode apps are the ones that tend to update the screen only sporadically, but I'm getting it all the time in the Quake video options menu, for example.

So you can have a look at some pont if you have capacity, otherwise I will because I find this pretty jarring. It even manifests at a simple command prompt; the blinking of the cursor and a normal typing rate is not enough to trigger new frames at a sufficiently high FPS to prevent this from happening 100% of the time.

I don't quite get it why the previous "dupe at the rate of the DOS mode" thing had to be removed.

Right. It should follow up at least with the next dupe frame as fast as possible (that is, at the negotiated rate: if throttled, that's the host rate ie: 60 Hz, or if full VFR at the DOS rate).
I suspect we can likely do away with the constant dupe rate, because VRR monitors are now being properly satisfied when they need CFR rates (when vsync and fullscreen).
More to add to the queue!

@GranMinigun, should we merge this and then punt this very corner-case "full screen falls back to max-window on output change" for the future?

To me, the code maintainability improvement here is more valuable than this non-showstopping small functional difference.

Right. It should follow up at least with the next dupe frame as fast as possible (that is, at the negotiated rate: if throttled, that's the host rate ie: 60 Hz, or if full VFR at the DOS rate).

Well, it definitely doesn't do that 😄

I suspect we can likely do away with the constant dupe rate, because VRR monitors are now being properly satisfied when they need CFR rates (when vsync and fullscreen).

Yeah, I just use CFR myself, usually with vsync disabled because it doesn't make much of a difference for the stuff I'm interested in, or with vsync enabled with demos but then I need to set my custom tweaked 70 Hz mode. That works like a dream, though; butter-smooth scrolling in Pinball Dreams and similar 100% synced to 70 Hz games.

I'm more worried about people using the defaults getting VFR and thus a questionable experience.

More to add to the queue!

🚀

Well, it definitely doesn't do that

Yup! I meant, it's the functionality we want. (what it currently does is follow up with dupe frames at about 10 fps)

Well, it definitely doesn't do that

Yup! I meant, it's the functionality we want. (what it currently does is follow up with dupe frames at about 10 fps)

Right okay 😄 I read it as: "this is how it should work, but I'm actually surprised to learn it's buggy" 😅 All clear now!

@GranMinigun - "Properly retaining fullscreen state, I'd say that's out of scope. Can't think immediately which change broke it, but will definitely be addressed in the subsequent PR(s)"

Agree here as well. Looking at the PR's purpose, you've nailed it then some (by eliminating the crashy callback). Let's get this down the chute. 🚀