| Summary: | Calling WIN_UpdateClipCursor() / WIN_UpdateClipCursorForWindows() on WIN_PumpEvents() causes beeping and choppy mouse cursor movement, right-click doesn't work | ||
|---|---|---|---|
| Product: | SDL | Reporter: | Ismael Ferreras Morezuelas (Swyter) <swyterzone+sdl> |
| Component: | video | Assignee: | Sam Lantinga <slouken> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | major | ||
| Priority: | P2 | CC: | cameron.gutman, contact, icculus, richard, sezeroz, stfx, swyterzone+sdl, sylvain.becker |
| Version: | HG 2.1 | Keywords: | target-2.0.12 |
| Hardware: | x86 | ||
| OS: | Windows (All) | ||
|
Description
Ismael Ferreras Morezuelas (Swyter)
2019-08-01 08:01:27 UTC
Here is the entire bisection log to clarify things a bit: 12968 xx bad r12951 is release-2.0.10 xx bad r12373 is release-2.0.9 xx bad 12373 (mine, debug) xx bad 12365 (beeps) xx bad 12363 (beeps) xx bad 12362 (beeps) xx bad ** https://hg.libsdl.org/SDL/rev/67496c3f9626 12361 xx bad -- no beeps 12358 xx bad -- no beeps 12344 xx bad -- no beeps 12315 xx bad -- no beeps 12258 xx bad -- no beeps in second try 12244 xx bad -- no beeps 12230 xx bad -- no beeps 12202 xx bad -- no beeps 12201 xx bad (probably bad build) 12200 xx bad -- no beeps 12199 xx bad -- no beeps 12197 xx bad -- no beeps 12193 xx bad -- no beeps 12186 xx bad -- no beeps 12172 xx bad -- no beeps 12158 xx bad -- no beeps 12154 xx bad -- no beeps ** https://hg.libsdl.org/SDL/rev/2a9b4339468b <-- 12153 -- good ** https://hg.libsdl.org/SDL/rev/de4288fa5b0b 12152 -- good 12151 -- good 12144 -- good 12029 -- good r11915 is release-2.0.8 -- good This one should be 'target-2.0.11' We're changing how we do SDL release versions; now releases will be even numbers (2.0.10, 2.0.12, etc), and as soon as we tag a release, we'll move the internal version number to an odd number (2.0.12 ships, we tag the latest in revision control as 2.0.13 immediately, which will become 2.0.14 on release, etc). As such, I'm moving the bugs tagged with target-2.0.11 to target 2.0.12. Sorry if you get a lot of email from this change! Thanks, --ryan. We're changing how we do SDL release versions; now releases will be even numbers (2.0.10, 2.0.12, etc), and as soon as we tag a release, we'll move the internal version number to an odd number (2.0.12 ships, we tag the latest in revision control as 2.0.13 immediately, which will become 2.0.14 on release, etc). As such, I'm moving the bugs tagged with target-2.0.11 to target 2.0.12. Sorry if you get a lot of email from this change! Thanks, --ryan. Not the same bisect but maybe this is helped by https://hg.libsdl.org/SDL/rev/9d6107cbcd66 Related to bug 4841 *** Bug 4467 has been marked as a duplicate of this bug. *** I can confirm that the original tester from the video above has tested a 32-bit r13170 version of SDL2.dll that I compiled myself and the game seems to function normally both in fullscreen and windowed mode. Keep in mind that I haven't bisected the changes to see if that specific revision was the one that fixes the regression. But it seems likely, thanks again. :) Now I can keep the Windows version of SDL2 up to date again. Well, false alarm. After confirming that SDL2-r13170.dll works fine I gave him a SDL2-r13163-v2.dll (twice) and it also worked fine, he then retested the original SDL2-r12258-v2.dll (which is a known bad revision) and it also worked smoothly. Meaning that (due to Windows 10 updates or something else) the problem is gone on that rig and we can't reproduce the issue or that it has been fixed, last time he tested it was on 31/07/2019. Darn. He argues that it's related to this recent Windows 10 update, but it may be something entirely different: https://cdn.discordapp.com/attachments/393832574516199424/637031169564868608/unknown.png I'll try to contact the other player and see if we can consistently reproduce it, or the heisenbug is gone. (In reply to Ismael Ferreras Morezuelas (Swyter) from comment #8) > I'll try to contact the other player and see if we can consistently > reproduce it, or the heisenbug is gone. There is a known beeping bug (which is what Windows does to signify that the system's input queue is overflowing) like this related to an XInput driver, fwiw... https://www.reddit.com/r/Windows10/comments/7nw918/xinput_mouse_beepinglagging_and_desktop_freezing/ I'm not saying that's what's happening here, but that this might not be a widespread issue and also might be completely beyond our control. We might just have to close this and see if other reports pop up later. If you're wondering why it's _beeping_, here's the likely technical explanation (if a hardware driver is hanging, I assume that causes this problem too, instead of the 16-bit Windows thing he mentions)... https://devblogs.microsoft.com/oldnewthing/20090918-00/?p=16663 --ryan. If there's no further comment on this bug, I believe I will be closing it in the next few days, under the assumption that it's the XInput driver bug I mentioned. --ryan. For me, this bug is triggered when I call the SDL_SetWindowGrab function (see bug 4467), but only after the 2.0.8 version (not included). The Win32 function ClipCursor is what the SDL2 is using under the hood to grab the cursor inside the window, and from my understanding it's not related at all to XInput. Moreover, as a workaround of this bug, I wrote my own Windows version of SDL_SetWindowGrab using the Win32 function ClipCursor, and everything works fine. #define SDL_MAIN_HANDLED #include <SDL2/SDL.h> #include <Windows.h> void custom_window_grab( SDL_Window* window, bool clip ) { if ( clip ) { int x{}; int y{}; int width{}; int height{}; SDL_GetWindowPosition( window, &x, &y ); SDL_GetWindowSize( window, &width, &height ); RECT rect{ x, y, x + width, y + height }; ClipCursor( &rect ); } else { ClipCursor( nullptr ); } } int main() { SDL_Init( SDL_INIT_VIDEO | SDL_INIT_EVENTS ); auto window = SDL_CreateWindow( "Window", SDL_WINDOWPOS_CENTERED, SDL_WINDOWPOS_CENTERED, 800, 600, SDL_WINDOW_RESIZABLE ); // SDL_SetWindowGrab( window, SDL_TRUE ); // Slow and choppy cursor custom_window_grab( window, true ); // Works perfectly bool run{ true }; while ( run ) { SDL_Event event{}; SDL_PollEvent( &event ); if ( event.type == SDL_WINDOWEVENT && event.window.event == SDL_WINDOWEVENT_CLOSE ) run = false; } SDL_DestroyWindow( window ); return 0; } Fixed, thanks! https://hg.libsdl.org/SDL/rev/d5109d6a9b98 (In reply to Sam Lantinga from comment #12) > Fixed, thanks! > https://hg.libsdl.org/SDL/rev/d5109d6a9b98 Based on that commit message, shouldn't the same also be done in case of relative mouse mode a few lines above? (In reply to Dom from comment #13) > (In reply to Sam Lantinga from comment #12) > > Fixed, thanks! > > https://hg.libsdl.org/SDL/rev/d5109d6a9b98 > > Based on that commit message, shouldn't the same also be done in case of > relative mouse mode a few lines above? You're right, thanks! https://hg.libsdl.org/SDL/rev/529fc1a611e8 (In reply to Sam Lantinga from comment #14) > You're right, thanks! > https://hg.libsdl.org/SDL/rev/529fc1a611e8 I think this changed the logic of the previous commit. Before ClipCursor() was called in case of !GetClipCursor() || memcmp() != 0 for mouse grab. Now the function already returns at the start in case of !GetClipCursor(). Yes, that's correct. We don't change the clip state if we can't get the current clip cursor, which should only happen if we don't have read access to the display. On my system, it's the call to GetClipCursor that is slow--we have been experiencing an issue with every version of SDL newer than 2.0.8 where a mouse polling rate higher than around 500Hz results in a noticeable frame time increase while moving the mouse; profiling the code indicated that the time was spent in the NtUserGetClipCursor kernel call that backs the GetClipCursor function. I am going to test Rémy's idea of bypassing SDL_SetWindowGrab and see where that gets me. Yes, this is still an issue as of 2.0.12. See Bug 5171. Maybe the original commit https://hg.libsdl.org/SDL/rev/2a9b4339468b causing this issue should be reverted along with the commits https://hg.libsdl.org/SDL/rev/d5109d6a9b98 and https://hg.libsdl.org/SDL/rev/529fc1a611e8 trying to fix it. I agree those should be reverted. In addition to the performance regression, I believe there's a correctness issue too. Consider the case of another app showing a window on top of SDL's and calling ClipCursor(). SDL will immediately steal the cursor clip region back, but the focus stealing app's window is still on top of SDL's. The effect you get is that the cursor in the focus stealing app's window is confined to the clip region dictated by the SDL window behind it, which may not even be visible at all anymore. Fixed, thanks! https://hg.libsdl.org/SDL/rev/ff702ce6ab48 Cameron, the scenario you're talking about shouldn't happen due to the SDL_WINDOW_INPUT_FOCUS check in WIN_UpdateClipCursor(). If you're seeing that happen anyway, please submit a new bug and let us know how SDL_WINDOW_INPUT_FOCUS gets out of sync with the actual window state. |
