[PATCH v2] dinput: Support default DIPROP_BUFFERSIZE buffer size
When a program calls SetProperty with DIPROP_BUFFERSIZE, dinput records this value for GetProperty but only uses it when the device can support that number of buffers otherwise a max value. In the case of game "Far Cry 5", it passes through (DWORD)-1 which cause HeapAlloc to fail ((DWORD)-1 * sizeof(DIDEVICEOBJECTDATA)). Since there is no real way of working out the max value, I've capped it at 20. MSDN reference. https://docs.microsoft.com/en-us/previous-versions/windows/desktop/ee417908(...) Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=45732 Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> --- dlls/dinput/device.c | 14 ++++++++++---- dlls/dinput/device_private.h | 3 ++- dlls/dinput/tests/device.c | 20 +++++++++++++++++--- 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/dlls/dinput/device.c b/dlls/dinput/device.c index 28329d03b5..ab6f4e6896 100644 --- a/dlls/dinput/device.c +++ b/dlls/dinput/device.c @@ -1307,7 +1307,7 @@ HRESULT WINAPI IDirectInputDevice2WImpl_GetProperty(LPDIRECTINPUTDEVICE8W iface, if (pdiph->dwSize != sizeof(DIPROPDWORD)) return DIERR_INVALIDPARAM; - pd->dwData = This->queue_len; + pd->dwData = This->buffersize; TRACE("buffersize = %d\n", pd->dwData); break; } @@ -1396,12 +1396,18 @@ HRESULT WINAPI IDirectInputDevice2WImpl_SetProperty( TRACE("buffersize = %d\n", pd->dwData); EnterCriticalSection(&This->crit); + + This->buffersize = pd->dwData; + + This->queue_len = This->buffersize > 20 ? 20 : This->buffersize; + if (This->buffersize > 20) + WARN("Trying to set large buffer size %d\n", pd->dwData); + HeapFree(GetProcessHeap(), 0, This->data_queue); - This->data_queue = !pd->dwData ? NULL : HeapAlloc(GetProcessHeap(), 0, - pd->dwData * sizeof(DIDEVICEOBJECTDATA)); + This->data_queue = !This->queue_len ? NULL : HeapAlloc(GetProcessHeap(), 0, + This->queue_len * sizeof(DIDEVICEOBJECTDATA)); This->queue_head = This->queue_tail = This->overflow = 0; - This->queue_len = pd->dwData; LeaveCriticalSection(&This->crit); break; diff --git a/dlls/dinput/device_private.h b/dlls/dinput/device_private.h index 27e9c26286..23d9e2eebc 100644 --- a/dlls/dinput/device_private.h +++ b/dlls/dinput/device_private.h @@ -71,10 +71,11 @@ struct IDirectInputDeviceImpl DI_EVENT_PROC event_proc; /* function to receive mouse & keyboard events */ LPDIDEVICEOBJECTDATA data_queue; /* buffer for 'GetDeviceData'. */ - int queue_len; /* size of the queue - set in 'SetProperty' */ + int queue_len; /* valid size of the queue */ int queue_head; /* position to write new event into queue */ int queue_tail; /* next event to read from queue */ BOOL overflow; /* return DI_BUFFEROVERFLOW in 'GetDeviceData' */ + DWORD buffersize; /* size of the queue - set in 'SetProperty' */ DataFormat data_format; /* user data format and wine to user format converter */ diff --git a/dlls/dinput/tests/device.c b/dlls/dinput/tests/device.c index a2a5a65686..5232a7b813 100644 --- a/dlls/dinput/tests/device.c +++ b/dlls/dinput/tests/device.c @@ -71,7 +71,7 @@ static BOOL CALLBACK enum_type_callback(const DIDEVICEOBJECTINSTANCEA *oi, void return DIENUM_CONTINUE; } -static void test_object_info(IDirectInputDeviceA *device, HWND hwnd) +static void test_object_info(IDirectInputDeviceA *device, HWND hwnd, int size) { HRESULT hr; DIPROPDWORD dp; @@ -106,8 +106,22 @@ static void test_object_info(IDirectInputDeviceA *device, HWND hwnd) dp.diph.dwHeaderSize = sizeof(DIPROPHEADER); dp.diph.dwHow = DIPH_DEVICE; dp.diph.dwObj = 0; + dp.dwData = -1; + + hr = IDirectInputDevice_GetProperty(device, DIPROP_BUFFERSIZE, &dp.diph); + ok(hr == DI_OK, "Failed: %08x\n", hr); + ok(dp.dwData == size, "got %d\n", dp.dwData); + + dp.dwData = -1; + hr = IDirectInputDevice_SetProperty(device, DIPROP_BUFFERSIZE, (LPCDIPROPHEADER)&dp.diph); + ok(hr == DI_OK, "SetProperty() failed: %08x\n", hr); + dp.dwData = 0; + hr = IDirectInputDevice_GetProperty(device, DIPROP_BUFFERSIZE, &dp.diph); + ok(hr == DI_OK, "Failed: %08x\n", hr); + ok(dp.dwData == -1, "got %d\n", dp.dwData); + dp.dwData = 0; hr = IDirectInputDevice_SetProperty(device, DIPROP_BUFFERSIZE, (LPCDIPROPHEADER)&dp.diph); ok(hr == DI_OK, "SetProperty() failed: %08x\n", hr); cnt = 5; @@ -191,13 +205,13 @@ static BOOL CALLBACK enum_devices(const DIDEVICEINSTANCEA *lpddi, void *pvRef) hr = IUnknown_QueryInterface(device, &IID_IDirectInputDevice2A, (LPVOID*)&obj); ok(SUCCEEDED(hr), "IUnknown_QueryInterface(IID_IDirectInputDevice2A) failed: %08x\n", hr); - test_object_info(obj, data->hwnd); + test_object_info(obj, data->hwnd, 0); IUnknown_Release(obj); obj = NULL; hr = IUnknown_QueryInterface(device, &IID_IDirectInputDevice2W, (LPVOID*)&obj); ok(SUCCEEDED(hr), "IUnknown_QueryInterface(IID_IDirectInputDevice2W) failed: %08x\n", hr); - test_object_info(obj, data->hwnd); + test_object_info(obj, data->hwnd, 20); IUnknown_Release(obj); IUnknown_Release(device); -- 2.24.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=63133 Your paranoid android. === debian10 (64 bit WoW report) === dinput: mouse.c:152: Test failed: Failed: 00000001 mouse.c:158: Test failed: Failed: 80070005 mouse.c:161: Test failed: Failed: 00000001 mouse.c:164: Test failed: Failed: 80070005 mouse.c:178: Test failed: GetDeviceData() failed: 00000000 cnt:0
On Sat, 11 Jan 2020, Marvin wrote: [...]
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=63133
Your paranoid android.
=== debian10 (64 bit WoW report) ===
dinput: mouse.c:152: Test failed: Failed: 00000001 mouse.c:158: Test failed: Failed: 80070005 mouse.c:161: Test failed: Failed: 00000001 mouse.c:164: Test failed: Failed: 80070005 mouse.c:178: Test failed: GetDeviceData() failed: 00000000 cnt:0
Based on the test.winehq.org history these tests started failing on the 26th of December: https://test.winehq.org/data/tests/dinput:mouse.html A bisect shows they are caused by this commit: commit cfcc280905b7804efde8f42bcd6bddbe5ebd8cad Author: Rémi Bernon <rbernon(a)codeweavers.com> AuthorDate: Thu Dec 26 12:51:23 2019 +0100 Commit: Alexandre Julliard <julliard(a)winehq.org> CommitDate: Thu Dec 26 21:56:04 2019 +0100 winex11.drv: Send HTMENU instead of HTCAPTION to query window activation. Commit 71d35d8940118bc6de6522913fb8c473fa5b2c24 broke the way WM_TAKE_FOCUS protocol is implemented: WM_MOUSEACTIVATE now replies MA_NOACTIVATE by default when using HTCAPTION. We use the WM_MOUSEACTIVATE -although Windows does not- regardless of the way focus is changed to check whether a window wants focus, and Windows sometimes changes focus regardless of the message reply. Steam and the Wine system tray are affected for instance. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Alexandre Julliard <julliard(a)winehq.org> There are actually two versions of this patch on the mailing list: * https://www.winehq.org/pipermail/wine-devel/2019-December/157117.html - Modifies dinput:mouse, marking the failing tests as todo_wine. So the TestBot found that dinput:mouse succeeded, although with todos: https://testbot.winehq.org/JobDetails.pl?Key=62569 * https://www.winehq.org/pipermail/wine-devel/2019-December/157099.html - Only modifies winex11.drv. Because of that the dinput:mouse test was not run and the regression was not detected by the TestBot: https://testbot.winehq.org/JobDetails.pl?Key=62502 - Posted a couple of days before the version above, this is the version that got committed. So the patch was known to to cause these failures and presumably that's being worked on (if not it would be good to make a bug report for reference). -- Francois Gouget <fgouget(a)codeweavers.com>
Hello Alistair, On 1/11/20 9:28 PM, Alistair Leslie-Hughes wrote:
When a program calls SetProperty with DIPROP_BUFFERSIZE, dinput records this value for GetProperty but only uses it when the device can support that number of buffers otherwise a max value.
In the case of game "Far Cry 5", it passes through (DWORD)-1 which cause HeapAlloc to fail ((DWORD)-1 * sizeof(DIDEVICEOBJECTDATA)).
Since there is no real way of working out the max value, I've capped it at 20.
MSDN reference. https://docs.microsoft.com/en-us/previous-versions/windows/desktop/ee417908(...)
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=45732
Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>
"Default" strikes me as a sort of odd way to characterize this. Maybe something like "dinput: Cap the buffer size to 100"?
--- dlls/dinput/device.c | 14 ++++++++++---- dlls/dinput/device_private.h | 3 ++- dlls/dinput/tests/device.c | 20 +++++++++++++++++--- 3 files changed, 29 insertions(+), 8 deletions(-)
diff --git a/dlls/dinput/device.c b/dlls/dinput/device.c index 28329d03b5..ab6f4e6896 100644 --- a/dlls/dinput/device.c +++ b/dlls/dinput/device.c @@ -1307,7 +1307,7 @@ HRESULT WINAPI IDirectInputDevice2WImpl_GetProperty(LPDIRECTINPUTDEVICE8W iface,
if (pdiph->dwSize != sizeof(DIPROPDWORD)) return DIERR_INVALIDPARAM;
- pd->dwData = This->queue_len; + pd->dwData = This->buffersize; TRACE("buffersize = %d\n", pd->dwData); break; } @@ -1396,12 +1396,18 @@ HRESULT WINAPI IDirectInputDevice2WImpl_SetProperty( TRACE("buffersize = %d\n", pd->dwData);
EnterCriticalSection(&This->crit); + + This->buffersize = pd->dwData; + + This->queue_len = This->buffersize > 20 ? 20 : This->buffersize;
"min(This->buffersize, 20)" might be more readable?
+ if (This->buffersize > 20) + WARN("Trying to set large buffer size %d\n", pd->dwData); +
Is this warning necessary? It seems like asking for a buffer "as big as possible" is a not unreasonable thing for an application to do, given that the maximum length is documented.
HeapFree(GetProcessHeap(), 0, This->data_queue);
- This->data_queue = !pd->dwData ? NULL : HeapAlloc(GetProcessHeap(), 0, - pd->dwData * sizeof(DIDEVICEOBJECTDATA)); + This->data_queue = !This->queue_len ? NULL : HeapAlloc(GetProcessHeap(), 0, + This->queue_len * sizeof(DIDEVICEOBJECTDATA)); This->queue_head = This->queue_tail = This->overflow = 0; - This->queue_len = pd->dwData;
LeaveCriticalSection(&This->crit); break; diff --git a/dlls/dinput/device_private.h b/dlls/dinput/device_private.h index 27e9c26286..23d9e2eebc 100644 --- a/dlls/dinput/device_private.h +++ b/dlls/dinput/device_private.h @@ -71,10 +71,11 @@ struct IDirectInputDeviceImpl DI_EVENT_PROC event_proc; /* function to receive mouse & keyboard events */
LPDIDEVICEOBJECTDATA data_queue; /* buffer for 'GetDeviceData'. */ - int queue_len; /* size of the queue - set in 'SetProperty' */ + int queue_len; /* valid size of the queue */ int queue_head; /* position to write new event into queue */ int queue_tail; /* next event to read from queue */ BOOL overflow; /* return DI_BUFFEROVERFLOW in 'GetDeviceData' */ + DWORD buffersize; /* size of the queue - set in 'SetProperty' */
DataFormat data_format; /* user data format and wine to user format converter */
diff --git a/dlls/dinput/tests/device.c b/dlls/dinput/tests/device.c index a2a5a65686..5232a7b813 100644 --- a/dlls/dinput/tests/device.c +++ b/dlls/dinput/tests/device.c @@ -71,7 +71,7 @@ static BOOL CALLBACK enum_type_callback(const DIDEVICEOBJECTINSTANCEA *oi, void return DIENUM_CONTINUE; }
-static void test_object_info(IDirectInputDeviceA *device, HWND hwnd) +static void test_object_info(IDirectInputDeviceA *device, HWND hwnd, int size) { HRESULT hr; DIPROPDWORD dp; @@ -106,8 +106,22 @@ static void test_object_info(IDirectInputDeviceA *device, HWND hwnd) dp.diph.dwHeaderSize = sizeof(DIPROPHEADER); dp.diph.dwHow = DIPH_DEVICE; dp.diph.dwObj = 0; + dp.dwData = -1;
Since "dwData" is unsigned, I feel like UINT_MAX is a bit more appropriate here.
+ + hr = IDirectInputDevice_GetProperty(device, DIPROP_BUFFERSIZE, &dp.diph); + ok(hr == DI_OK, "Failed: %08x\n", hr); + ok(dp.dwData == size, "got %d\n", dp.dwData); + + dp.dwData = -1; + hr = IDirectInputDevice_SetProperty(device, DIPROP_BUFFERSIZE, (LPCDIPROPHEADER)&dp.diph); + ok(hr == DI_OK, "SetProperty() failed: %08x\n", hr); + dp.dwData = 0; + hr = IDirectInputDevice_GetProperty(device, DIPROP_BUFFERSIZE, &dp.diph); + ok(hr == DI_OK, "Failed: %08x\n", hr); + ok(dp.dwData == -1, "got %d\n", dp.dwData);
+ dp.dwData = 0; hr = IDirectInputDevice_SetProperty(device, DIPROP_BUFFERSIZE, (LPCDIPROPHEADER)&dp.diph); ok(hr == DI_OK, "SetProperty() failed: %08x\n", hr); cnt = 5; @@ -191,13 +205,13 @@ static BOOL CALLBACK enum_devices(const DIDEVICEINSTANCEA *lpddi, void *pvRef)
hr = IUnknown_QueryInterface(device, &IID_IDirectInputDevice2A, (LPVOID*)&obj); ok(SUCCEEDED(hr), "IUnknown_QueryInterface(IID_IDirectInputDevice2A) failed: %08x\n", hr); - test_object_info(obj, data->hwnd); + test_object_info(obj, data->hwnd, 0); IUnknown_Release(obj); obj = NULL;
hr = IUnknown_QueryInterface(device, &IID_IDirectInputDevice2W, (LPVOID*)&obj); ok(SUCCEEDED(hr), "IUnknown_QueryInterface(IID_IDirectInputDevice2W) failed: %08x\n", hr); - test_object_info(obj, data->hwnd); + test_object_info(obj, data->hwnd, 20); IUnknown_Release(obj);
IUnknown_Release(device);
I guess this is because the first test later sets the size to 20, but that's not obvious just from reading enum_devices()—it looks like an actual difference between the A and W interfaces. Maybe it'd be better to reset the size to 0 after the first test? Or create the device twice in enum_devices() instead of querying two interfaces from the same device? Though for that matter, why exactly are we passing a W interface to a function that takes an A interface? This test seems kind of broken...
participants (4)
-
Alistair Leslie-Hughes -
Francois Gouget -
Marvin -
Zebediah Figura