[PATCH v2] dinput: Support default DIPROP_BUFFERSIZE buffer size

[Zebediah Figura]

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(v=vs.85)
> 
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=45732
> 
> Signed-off-by: Alistair Leslie-Hughes <leslie_alistair at 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...