Simon Hug
The bug is in the GL_ResetState and GLES_ResetState functions which get called after a new GL context is created. These functions set the cached current color to transparent black, but the GL specification says the initial color is opaque white.
The attached patch changes the values to 0xffffffff to reflect the initial state of the current color. Should the ResetState functions get called anywhere else in the future, this probably has to call the GL functions itself to ensure that the colors match.
Adam M.
The keysym.mod field does not reflect the state of the modified keys when processing key down events for the modifier keys themselves. The documentation says that it returns the current key modifiers, but they are not current for key down events involving modifier keys. I interpret "current" to mean "equal to SDL_GetModState() at the instant the event is processed/enqueued".
For example, if you depress the Shift key you get a key down event with .mod == 0. However, .mod should not be 0 because a shift key is down. If you then release the Shift key, you get a key up event with .mod == 0. Neither event reports the modifier key.
If you press Shift and then A, .mod is incorrect (== 0) when Shift is pressed, but is correct later when A is pressed (== KMOD_LSHIFT).
You might say this behavior is deliberate, i.e. keysym.mod is the value /before/ the event, not the current value as documented, but that explanation is incorrect because only key down events behave that way. Key up events correctly give the current value, not the value before the event.
Not only is it inconsistent with itself, I think it makes keyboard processing harder.
The problem is near line 740 in SDL_keyboard.c:
if (SDL_KEYDOWN == type) {
modstate = keyboard->modstate; // SHOULD THIS BE MOVED DOWN?
switch (keycode) {
case SDLK_NUMLOCKCLEAR:
keyboard->modstate ^= KMOD_NUM;
break;
case SDLK_CAPSLOCK:
keyboard->modstate ^= KMOD_CAPS;
break;
default:
keyboard->modstate |= modifier;
break;
}
} else {
keyboard->modstate &= ~modifier;
modstate = keyboard->modstate;
}
In the key down path, modstate (and thus keysym.mod) ends up being the modifier state /before/ the event, but in the key up path modstate ends up being the modifier state /after/ the event. Personally I think the "modstate = keyboard->modstate" line should just be moved after the entire if/else statement, so that keysym.mod always reflects the current state.
owen
I removed all the static variables from SDLActivity.java
Updated all the SDL_android.c jni calls as well
I added a new function to SDL_android.c/ h
void Android_JNI_SeparateEventsHint(const char* c);
This is called by SDL_androidtouch.c so that this TU doesn't need to call any JNI functions.
Philipp Wiesemann
There is another problem with the current implementation which maybe should be fixed first (to prevent some work). It was written as if it would get the number of a button from the Java side but actually it gets the state of all buttons. That is why it should not work if more than one button is pressed at once.
Ian Abbott
I just spotted what I think is a bug in "src/thread/pthread/SDL_sysmutex.c" in the SDL_TryLockMutex function when FAKE_RECURSIVE_MUTEX is defined (for an implementation of Pthreads with no recursive mutex support). It calls pthread_mutex_lock instead of pthread_mutex_trylock, so it will block until the mutex is available instead of returning SDL_MUTEX_TIMEDOUT if it cannot lock the mutex immediately.
Coriiander
Here is a minor correction for a non-breaking mistake in SDL_setenv for __WIN32__ platform. See below for details.
FILE:
"SDL/src/stdlib/SDL_getenv.c"
FUNCTION: (__WIN32__ platform)
int SDL_setenv(const char *name, const char *value, int overwrite)
CODE:
if (!overwrite) {
char ch = 0;
const size_t len = GetEnvironmentVariableA(name, &ch, sizeof (ch));
if (len > 0) {
return 0; /* asked not to overwrite existing value. */
}
}
WHAT'S WRONG:
The 3th argument to GetEnvironmentVariable (being DWORD nSize) must be the number of characters, not the number of bytes. SDL currently passes "the size of 1 char", rather "1". While it is non-breaking (1=1 after all), it is incorrect. Furthermore there is no need to specify the 2nd and 3th arguments at all.
CORRECTION 1: (corrected argument_
if (!overwrite) {
char ch = 0;
const size_t len = GetEnvironmentVariableA(name, &ch, 1);
if (len > 0) {
return 0; /* asked not to overwrite existing value. */
}
}
CORRECTION 2: (stripped of unneeded code)
if (!overwrite) {
if (GetEnvironmentVariableA(name, NULL, 0) > 0) {
return 0; /* asked not to overwrite existing value. */
}
}
Juha Niemim?
On AmigaOS 4 platform with Newlib 'C' library, there is a problem with failing fseeko64. This seemed to be caused by using fopen instead of fopen64.
Littlefighter19
When trying to mirror something on the PSP, I've stumbled upon the problem,
that using SDL_RenderCopyEx with SDL_FLIP_HORIZONTAL flips the image vertically, vise-versa SDL_FLIP_VERTICAL flips the image horizontally.
Proposed patch would be swapping the check in line 944 with the one in line 948 in SDL_render_psp.c
romain.lacroix
For the windows implementation of SDL_ShowMessageBox() : ./src/video/windows/SDL_windowsmessagebox.c:345 WIN_ShowMessageBox()
The implementation in 2.0.4 uses "button index" for parameter "id" of function AddDialogButton().
It then expects the value provided in param wParam of function MessageBoxDialogProc() to be a valid index of a button.
It uses this value to index in the array of buttons when DialogBoxIndirect() returns (line 474 : *buttonid = buttons[which].buttonid;)
However, when dismissing this box with Escape, the return value of DialogBoxIndirect will be SDL_MESSAGEBOX_BUTTON_ESCAPEKEY_DEFAULT (=2) which is not always a valid index of array buttons.
When the array buttons has a length less or equal than 2, the memory access is invalid; I can see that the value written to *buttonId is uninitialized memory (random value).
The fix I propose : use value "buttonid" (field of button) for parameter "id" of AddDialogButton(), then copy return value of DialogBoxIndirect() in *buttonid. This way, we will not use an out-of-bounds index in array buttons.
Simon Hug
The RGBA_FROM_PIXEL macro in src/video/blit.h [1] is not designed to work with more than 8 bits per channel and the ARGB2101010 format makes it read outside of the array bounds causing access violations. This can happen during blitting with the BlitNtoNPixelAlpha and SDL_Blit_Slow functions.
When SDL_InitFormat tries to calculate the loss of the channels [2], the Uint8 will wrap around and it will end up at 254 for the 10-bit channels. Clearly way over the 9 entries of the SDL_expand_byte array. (Not that a signed integer would help.) Then the macro tries to access the lookup table with the channel value which could be up to 1023. If the previous indirection didn't cause an access violation this one will.
I guess it's not worth modifying this macro for a format that only a few will use. It will only make the other blitters slower. I don't have good ideas to solve this issue.
Attached is a test case that does three blits. A copy one that work and the two that use the functions mentioned above.
[1] https://hg.libsdl.org/SDL/file/cd1994d4f3c6/src/video/SDL_blit.h#l303
[2] https://hg.libsdl.org/SDL/file/cd1994d4f3c6/src/video/SDL_pixels.c#l540
Daniel
SDL_RenderReadPixels with SDL_RENDERER_SOFTWARE reads pixels from wrong coordinates.
SW_RenderReadPixels adjusts the rect coordinates according to the viewport. But since this is already done by SDL_RenderReadPixels, the final rect has x2 bigger X and Y.
Fabian Greffrath
we use SDL_GetPrefPath() in Chocolate Doom to get a reasonable directory to save and restore config files and savegames:
https://github.com/chocolate-doom/chocolate-doom/blob/sdl2-branch/src/m_config.c#L2162
However, since there is no "organization" behind Chocolate Doom and there is really only one "product" called Chocolate Doom, we pass an empty string for the org parameter and the package string for app.
This leads to two consecutive slashes in the path returned by SDL_GetPrefPath() like this:
/home/user/.local/share//chocolate-doom/
While this is harmless, it sure looks bad.
I believe that it should be possible to either pass a NULL pointer for the org parameter or at least have the function detect an empty string as a means to express "there is no origanization, just a single product". The generation of the path string to be returned by the function will have to get adapted accordingly.
Eric Wasylishen
Alt-Up/Down/Left/Right switches between displays using SDL_WINDOWPOS_CENTERED_DISPLAY
Shift-Up/Down/Left/Right shifts the window by 100px
Simon Hug
Some code in SDL loads libraries with SDL_LoadObject to get more information or use newer APIs. SDL_LoadObject may fail, set an error message and SDL will continue with some fallback code. Since SDL will overwrite the error or exit the function with a return value that indicates success, the error form SDL_LoadObject for the optional stuff might as well be cleared right away.
Eric Wasylishen 2017-07-26 18:42:58 UTC
I set up an (admittedly exotic) 3-monitor setup, and when I enter fullscreen-desktop on the middle display (#2), the SDL window is off center. (covers half of monitor #2 and most of monitor #3).
The displays are arranged from left to right:
Display #1 (main): 2880x1800, 200% scaling
Display #2: 1920x1200, 150% scaling
Display #3: 1920x1080, 100% scaling
SDL display bounds:
INFO: Bounds: 1440x900 at 0,0
INFO: Bounds: 1281x801 at 1921,0 (these are incorrect)
INFO: Bounds: 1920x1080 at 4800,0
Correct bounds reported by calling EnumDisplayMonitors and printing the LPRECT param of the callback:
1440x900 at (0, 0)
1280x800 at (2880, 0)
1920x1080 at (4800, 0)
It seems like you need 3 displays to reproduce this, and the left two need DPI scaling, and the 3rd display needs to have a different scale factor than the others.
Related: https://bugzilla.libsdl.org/show_bug.cgi?id=3709
SDL: current hg (11235:6a587b9e0ec8)
Windows 10, Version 10.0.15063 Build 15063
Tested with testdraw2 and testgl2, and pressing alt+enter to enter fullscreen desktop.
This patch reworks SDL_windowsmodes.c to use EnumDisplayMonitors instead of EnumDisplayDevices, so we always have an HMONITOR for each SDL display.
With access to an HMONITOR, we can get the monitor bounds in virtual screen coordinates the proper way, by calling GetMonitorInfo. (whereas the original code was doing some calculations - e.g. "data->DeviceMode.dmPosition.x * data->ScaleX" - to try to get virtual screen coordinates. These worked in simple cases, but failed in more complex cases like this bug)
The one potential problem with my patch is, the ChangeDisplaySettingsEx docs say that you're supposed to get the display name from EnumDisplayDevices, but I'm getting the display name from GetMonitorInfo now.
Simon Hug
KMSDRM_VideoInit allocates and frees some connectors and encoders but doesn't set the pointer to NULL after freeing. The cleanup code at the end may free one of those garbage pointer should an error happen in the initialization.
Simon Hug
When WIN_WindowProc processes the WM_TOUCH message, it doesn't check if the touch functions have been properly loaded and may call a NULL pointer. It's probably an extremely rare case, but here's a patch that adds some checks anyway.
Eric wing
Hi, I think I found a bug when using SDL_WINDOW_ALLOW_HIGHDPI with SDL_RenderSetLogicalSize on iOS. I use SDL_RenderSetLogicalSize for all my stuff. I just tried turning on SDL_WINDOW_ALLOW_HIGHDPI on iOS and suddenly all my touch/mouse positions are really broken/far-off-the-mark.
I actually don't have a real retina device (still) so I'm seeing this using the iOS simulator with a 6plus template.
Attached is a simple test program that can reproduce the problem. It uses RenderSetLogicalSize and draws some moving happy faces (to show the boundaries/space of the LogicalSize and that it is working correctly for that part).
When you click/touch, it will draw one more happy face where your button point is.
If you comment out SDL_WINDOW_ALLOW_HIGHDPI, everything works as expected. But if you compile with it in, the mouse coordinates seem really far off the mark. (Face appears far up and to the left.)
Alex Szpakowski on the mailing list suggests the problem is
"I believe this is a bug in SDL_Render?s platform-agnostic mouse coordinate scaling code. It assumes the units of the mouse coordinates are always in pixels, which isn?t the case where high-DPI is involved (regardless of whether iOS is used) ? they?re actually in ?DPI independent? coordinates (which matches the window size, but not the renderer output size)."
Additionally, if this is correct, the Mac under Retina is also probably affected too and "as well as any other platform SDL adds high-dpi support for in the future".
Levi Bard
In some environments, xrandr modes initialization can fail even though xrandr support is present and of a sufficient version.
(The one I encountered was an AWS instance running a virtual display)
The attached patch allows SDL to keep trying other methods if xrandr modes initialization fails (still subject to SDL_VIDEO_X11_REQUIRE_XRANDR).
Sam Lantinga
ouned
Ryan C. Gordon
Philipp Wiesemann
Brandon Schaefer
Patrice Mandin