September 2021 - Wine-Devel

[PATCH 1/2] bcrypt: Remove macOS backend.

by Gijs Vermeulen

gnutls has been the default since wine-3.13. Signed-off-by: Gijs Vermeulen <gijsvrm(a)gmail.com> --- configure.ac | 1 - dlls/bcrypt/Makefile.in | 1 - dlls/bcrypt/macos.c | 312 ---------------------------------------- 3 files changed, 314 deletions(-) delete mode 100644 dlls/bcrypt/macos.c diff --git a/configure.ac b/configure.ac index 94d1b472142..7ed1e73eb6d 100644 --- a/configure.ac +++ b/configure.ac @@ -430,7 +430,6 @@ AC_CHECK_HEADERS(\ ApplicationServices/ApplicationServices.h \ CL/cl.h \ Carbon/Carbon.h \ - CommonCrypto/CommonCryptor.h \ CoreAudio/CoreAudio.h \ DiskArbitration/DiskArbitration.h \ EGL/egl.h \ diff --git a/dlls/bcrypt/Makefile.in b/dlls/bcrypt/Makefile.in index dd6d4a76640..63a731fa9d9 100644 --- a/dlls/bcrypt/Makefile.in +++ b/dlls/bcrypt/Makefile.in @@ -6,7 +6,6 @@ EXTRAINCL = $(GNUTLS_CFLAGS) C_SRCS = \ bcrypt_main.c \ gnutls.c \ - macos.c \ md2.c \ sha256.c \ sha512.c diff --git a/dlls/bcrypt/macos.c b/dlls/bcrypt/macos.c deleted file mode 100644 index 20e1a61e3fb..00000000000 --- a/dlls/bcrypt/macos.c +++ /dev/null @@ -1,312 +0,0 @@ -/* - * Copyright 2009 Henri Verbeet for CodeWeavers - * Copyright 2018 Hans Leidekker for CodeWeavers - * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 2.1 of the License, or (at your option) any later version. - * - * This library is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public - * License along with this library; if not, write to the Free Software - * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA - * - */ - -#if 0 -#pragma makedep unix -#endif - -#include "config.h" -#include "wine/port.h" - -#include <stdarg.h> -#ifdef HAVE_COMMONCRYPTO_COMMONCRYPTOR_H -#include <AvailabilityMacros.h> -#include <CommonCrypto/CommonCryptor.h> -#endif - -#include "ntstatus.h" -#define WIN32_NO_STATUS -#include "windef.h" -#include "winbase.h" -#include "ntsecapi.h" -#include "bcrypt.h" - -#include "bcrypt_internal.h" - -#include "wine/debug.h" -#include "wine/unicode.h" - -#if defined(HAVE_COMMONCRYPTO_COMMONCRYPTOR_H) && MAC_OS_X_VERSION_MAX_ALLOWED >= 1080 && !defined(HAVE_GNUTLS_CIPHER_INIT) -WINE_DEFAULT_DEBUG_CHANNEL(bcrypt); - -struct key_data -{ - CCCryptorRef ref_encrypt; - CCCryptorRef ref_decrypt; -}; -C_ASSERT( sizeof(struct key_data) <= sizeof(((struct key *)0)->private) ); - -static struct key_data *key_data( struct key *key ) -{ - return (struct key_data *)key->private; -} - -static NTSTATUS CDECL key_set_property( struct key *key, const WCHAR *prop, UCHAR *value, ULONG size, ULONG flags ) -{ - if (!strcmpW( prop, BCRYPT_CHAINING_MODE )) - { - if (!strcmpW( (WCHAR *)value, BCRYPT_CHAIN_MODE_ECB )) - { - key->u.s.mode = MODE_ID_ECB; - return STATUS_SUCCESS; - } - else if (!strcmpW( (WCHAR *)value, BCRYPT_CHAIN_MODE_CBC )) - { - key->u.s.mode = MODE_ID_CBC; - return STATUS_SUCCESS; - } - else - { - FIXME( "unsupported mode %s\n", debugstr_w((WCHAR *)value) ); - return STATUS_NOT_IMPLEMENTED; - } - } - - FIXME( "unsupported key property %s\n", debugstr_w(prop) ); - return STATUS_NOT_IMPLEMENTED; -} - -static NTSTATUS CDECL key_symmetric_init( struct key *key ) -{ - switch (key->alg_id) - { - case ALG_ID_AES: - switch (key->u.s.mode) - { - case MODE_ID_ECB: - case MODE_ID_CBC: - break; - default: - FIXME( "mode %u not supported\n", key->u.s.mode ); - return STATUS_NOT_SUPPORTED; - } - return STATUS_SUCCESS; - - default: - FIXME( "algorithm %u not supported\n", key->alg_id ); - return STATUS_NOT_SUPPORTED; - } -} - -static CCMode get_cryptor_mode( struct key *key ) -{ - switch (key->u.s.mode) - { - case MODE_ID_ECB: return kCCModeECB; - case MODE_ID_CBC: return kCCModeCBC; - default: - FIXME( "unsupported mode %u\n", key->u.s.mode ); - return 0; - } -} - -static void CDECL key_symmetric_vector_reset( struct key *key ) -{ - if (!key_data(key)->ref_encrypt) return; - - TRACE( "invalidating cryptor handles\n" ); - CCCryptorRelease( key_data(key)->ref_encrypt ); - key_data(key)->ref_encrypt = NULL; - - CCCryptorRelease( key_data(key)->ref_decrypt ); - key_data(key)->ref_decrypt = NULL; -} - -static NTSTATUS init_cryptor_handles( struct key *key ) -{ - CCCryptorStatus status; - CCMode mode; - - if (key_data(key)->ref_encrypt) return STATUS_SUCCESS; - if (!(mode = get_cryptor_mode( key ))) return STATUS_NOT_SUPPORTED; - - if ((status = CCCryptorCreateWithMode( kCCEncrypt, mode, kCCAlgorithmAES128, ccNoPadding, key->u.s.vector, - key->u.s.secret, key->u.s.secret_len, NULL, 0, 0, 0, - &key_data(key)->ref_encrypt )) != kCCSuccess) - { - WARN( "CCCryptorCreateWithMode failed %d\n", status ); - return STATUS_INTERNAL_ERROR; - } - if ((status = CCCryptorCreateWithMode( kCCDecrypt, mode, kCCAlgorithmAES128, ccNoPadding, key->u.s.vector, - key->u.s.secret, key->u.s.secret_len, NULL, 0, 0, 0, - &key_data(key)->ref_decrypt )) != kCCSuccess) - { - WARN( "CCCryptorCreateWithMode failed %d\n", status ); - CCCryptorRelease( key_data(key)->ref_encrypt ); - key_data(key)->ref_encrypt = NULL; - return STATUS_INTERNAL_ERROR; - } - - return STATUS_SUCCESS; -} - -static NTSTATUS CDECL key_symmetric_set_auth_data( struct key *key, UCHAR *auth_data, ULONG len ) -{ - FIXME( "not implemented on Mac\n" ); - return STATUS_NOT_IMPLEMENTED; -} - -static NTSTATUS CDECL key_symmetric_encrypt( struct key *key, const UCHAR *input, ULONG input_len, UCHAR *output, ULONG output_len ) -{ - CCCryptorStatus status; - NTSTATUS ret; - - if ((ret = init_cryptor_handles( key ))) return ret; - - if ((status = CCCryptorUpdate( key_data(key)->ref_encrypt, input, input_len, output, output_len, NULL )) != kCCSuccess) - { - WARN( "CCCryptorUpdate failed %d\n", status ); - return STATUS_INTERNAL_ERROR; - } - return STATUS_SUCCESS; -} - -static NTSTATUS CDECL key_symmetric_decrypt( struct key *key, const UCHAR *input, ULONG input_len, UCHAR *output, ULONG output_len ) -{ - CCCryptorStatus status; - NTSTATUS ret; - - if ((ret = init_cryptor_handles( key ))) return ret; - - if ((status = CCCryptorUpdate( key_data(key)->ref_decrypt, input, input_len, output, output_len, NULL )) != kCCSuccess) - { - WARN( "CCCryptorUpdate failed %d\n", status ); - return STATUS_INTERNAL_ERROR; - } - return STATUS_SUCCESS; -} - -static NTSTATUS CDECL key_symmetric_get_tag( struct key *key, UCHAR *tag, ULONG len ) -{ - FIXME( "not implemented on Mac\n" ); - return STATUS_NOT_IMPLEMENTED; -} - -static void CDECL key_symmetric_destroy( struct key *key ) -{ - if (key_data(key)->ref_encrypt) CCCryptorRelease( key_data(key)->ref_encrypt ); - if (key_data(key)->ref_decrypt) CCCryptorRelease( key_data(key)->ref_decrypt ); -} - -static NTSTATUS CDECL key_asymmetric_init( struct key *key ) -{ - FIXME( "not implemented on Mac\n" ); - return STATUS_NOT_IMPLEMENTED; -} - -static NTSTATUS CDECL key_asymmetric_sign( struct key *key, void *padding, UCHAR *input, ULONG input_len, UCHAR *output, - ULONG output_len, ULONG *ret_len, ULONG flags ) -{ - FIXME( "not implemented on Mac\n" ); - return STATUS_NOT_IMPLEMENTED; -} - -static NTSTATUS CDECL key_asymmetric_verify( struct key *key, void *padding, UCHAR *hash, ULONG hash_len, - UCHAR *signature, ULONG signature_len, DWORD flags ) -{ - FIXME( "not implemented on Mac\n" ); - return STATUS_NOT_IMPLEMENTED; -} - -static NTSTATUS CDECL key_export_dsa_capi( struct key *key, UCHAR *buf, ULONG len, ULONG *ret_len ) -{ - FIXME( "not implemented on Mac\n" ); - return STATUS_NOT_IMPLEMENTED; -} - -static NTSTATUS CDECL key_export_ecc( struct key *key, UCHAR *output, ULONG len, ULONG *ret_len ) -{ - FIXME( "not implemented on Mac\n" ); - return STATUS_NOT_IMPLEMENTED; -} - -static NTSTATUS CDECL key_import_dsa_capi( struct key *key, UCHAR *buf, ULONG len ) -{ - FIXME( "not implemented on Mac\n" ); - return STATUS_NOT_IMPLEMENTED; -} - -static NTSTATUS CDECL key_import_ecc( struct key *key, UCHAR *input, ULONG len ) -{ - FIXME( "not implemented on Mac\n" ); - return STATUS_NOT_IMPLEMENTED; -} - -static NTSTATUS CDECL key_import_rsa( struct key *key, UCHAR *input, ULONG len ) -{ - FIXME( "not implemented on Mac\n" ); - return STATUS_NOT_IMPLEMENTED; -} - -static NTSTATUS CDECL key_asymmetric_generate( struct key *key ) -{ - FIXME( "not implemented on Mac\n" ); - return STATUS_NOT_IMPLEMENTED; -} - -static void CDECL key_asymmetric_destroy( struct key *key ) -{ -} - -static NTSTATUS CDECL key_asymmetric_duplicate( struct key *key_orig, struct key *key_copy ) -{ - FIXME( "not implemented on Mac\n" ); - return STATUS_NOT_IMPLEMENTED; -} - -static NTSTATUS CDECL key_asymmetric_decrypt( struct key *key, UCHAR *input, ULONG input_len, - UCHAR *output, ULONG output_len, ULONG *ret_len ) -{ - FIXME( "not implemented on Mac\n" ); - return STATUS_NOT_IMPLEMENTED; -} - -static const struct key_funcs key_funcs = -{ - key_set_property, - key_symmetric_init, - key_symmetric_vector_reset, - key_symmetric_set_auth_data, - key_symmetric_encrypt, - key_symmetric_decrypt, - key_symmetric_get_tag, - key_symmetric_destroy, - key_asymmetric_init, - key_asymmetric_generate, - key_asymmetric_decrypt, - key_asymmetric_duplicate, - key_asymmetric_sign, - key_asymmetric_verify, - key_asymmetric_destroy, - key_export_dsa_capi, - key_export_ecc, - key_import_dsa_capi, - key_import_ecc, - key_import_rsa -}; - -NTSTATUS CDECL __wine_init_unix_lib( HMODULE module, DWORD reason, const void *ptr_in, void *ptr_out ) -{ - if (reason != DLL_PROCESS_ATTACH) return STATUS_SUCCESS; - *(const struct key_funcs **)ptr_out = &key_funcs; - return STATUS_SUCCESS; -} - -#endif -- 2.33.0

Sept. 29, 2021

Re: [PATCH v6 5/5] winegstreamer: Replace source pad interface with GstAppSrc.

by Derek Lesho

On 9/29/21 22:23, Zebediah Figura wrote: > > Sorry, I remain unconvinced that this approach is a good one. Can't argue with that, I think I'll move on from working on this, at-least for now.

Sept. 29, 2021

Re: [PATCH v6 5/5] winegstreamer: Replace source pad interface with GstAppSrc.

by Zebediah Figura

On 9/29/21 2:37 PM, Derek Lesho wrote: > > On 9/29/21 17:30, Zebediah Figura (she/her) wrote: >> On 9/29/21 02:59, Derek Lesho wrote: >>> On 9/29/21 00:18, Zebediah Figura (she/her) wrote: >>> >>>> On 9/28/21 03:08, Derek Lesho wrote: >>>>> >>>>> On 9/28/21 07:23, Zebediah Figura (she/her) wrote: >>>>>> On 9/24/21 14:23, Derek Lesho wrote: >>>>>>> >>>>>>> On 9/24/21 12:38, Zebediah Figura wrote: >>>>>>>> On 9/24/21 3:05 AM, Derek Lesho wrote: >>>>>>>>> On 9/21/21 13:02, Zebediah Figura wrote: >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> 1. Rely on the fact that seek-data is sent during a seek, and >>>>>>>>>> use it >>>>>>>>>> to effectively invalidate any buffers sent before the seek. >>>>>>>>>> Unlike the >>>>>>>>>> aforementioned problem with validation, this will actually >>>>>>>>>> work: at >>>>>>>>>> the time that seek-data is sent appsrc should be flushing, so any >>>>>>>>>> buffers we send before it will be discarded by appsrc, and any >>>>>>>>>> buffers >>>>>>>>>> we send afterward can be discarded by us if they aren't valid. >>>>>>>>>> This is >>>>>>>>>> very fragile, though. There's really no reason for appsrc to send >>>>>>>>>> seek-data for random-access streams in the first place, and >>>>>>>>>> this kind >>>>>>>>>> of synchronization is easy to get wrong if I haven't already. >>>>>>>>> >>>>>>>>> This sounds like the best way forward in my opinion, how is it >>>>>>>>> fragile? >>>>>>>> >>>>>>>> It's fragile in general because we're making a lot of >>>>>>>> assumptions, that aren't documented, about when and from which >>>>>>>> thread gstappsrc will send these signals, and what >>>>>>>> synchronization guarantees it applies when doing so. >>>>>>> >>>>>>> My perspective is that if we can get it working, documenting the >>>>>>> two required quirks to the GStreamer project, eventually the >>>>>>> problem will be be cleared up and/or fixed, and we can remove the >>>>>>> quirks then. >>>>>> >>>>>> The problem is that we're still going to have to keep those >>>>>> workarounds for a long time. Not to mention that clearly filing a >>>>>> bug is not enough to get the attention of the GStreamer developers. >>>>>> >>>>>> As the maintainer of this code, I don't think I feel very >>>>>> comfortable relying this much on the internals of appsrc, whether >>>>>> as workarounds or not. >>>>>> >>>>>>> >>>>>>>> >>>>>>>>> It seems that if we just ensure that a pushed buffer takes into >>>>>>>>> account >>>>>>>>> the latest seek/need data pair, not much can go wrong. >>>>>>>>> >>>>>>>> >>>>>>>> That's not enough by itself. We can, generally speaking, send a >>>>>>>> buffer after a seek-data/need-data is triggered but before it is >>>>>>>> actually sent. >>>>>>> Before what is sent? >>>>>> >>>>>> Before the seek-data or need-data signal is sent. There is a >>>>>> window there, however short. >>>>> >>>>> I'm not sure exactly what the difference between triggering a >>>>> signal and sending a signal is, but assuming you mean that if >>>>> seek-data doesn't acquire the mutex, it would be possible to >>>>> push-buffer to not see the seek, then send the buffer after >>>>> seek-data, then yeah, we do need to put seek-data and push-buffer >>>>> in the same mutex to make sure that push-buffer is responding to >>>>> the latest seek/need data callbacks. >>>>> >>>>>> >>>>>>>> This happens in practice with flushes. >>>>>>>> >>>>>>>> In order to get around that, you need the hack I mentioned: >>>>>>>> assume that a flush will be accompanied by a seek-data signal. >>>>>>>> This is especially fragile because there's no reason for appsrc >>>>>>>> to even be sending this signal in the first place (it only >>>>>>>> really makes sense in "seekable" mode), and they could easily >>>>>>>> decide to not do that. >>>>>>> >>>>>>> I'm not sure I understand, whenever the next buffer app source >>>>>>> wants is not consecutively after the last, seek-data is required >>>>>>> (and sent). What does "assume the flush will be accompanied by a >>>>>>> seek-data signal" mean? The app source client doesn't have any >>>>>>> conception of a flush, just seek-data and need-data, and to fix >>>>>>> this problem all we need to do is add a quite rational check that >>>>>>> push_data is responding to the latest seek/need data pair. >>>>>> >>>>>> Let me try to explain more clearly. At least one race, the one >>>>>> I've been trying to describe, looks like this: >>>>>> >>>>>> >>>>>> read thread main thread >>>>>> ------------------------------------- >>>>>> push seek event >>>>>> retrieve data >>>>>> validate offset >>>>>> send flush-start >>>>>> emit seek-data >>>>>> send flush-stop >>>>>> push-buffer >>>>>> >>>>>> >>>>>> At which point appsrc will get the wrong buffer. >>>>>> >>>>>> Solution 1 has us rely on the seek-data signal to effectively wait >>>>>> for the read thread (most likely using the parser mutex) and, in a >>>>>> sense, put a barrier between reads occuring before and after the >>>>>> flush. >>>>> >>>>> >>>>> I mean, yeah, but from the perspective of the interface, all we are >>>>> doing is making sure that buffers that are responding to requests >>>>> from before a seek-data don't get sent, which I really don't think >>>>> should be too controversial. That the seek happens to be caused by >>>>> a flush shouldn't matter, we're putting a barrier between buffers >>>>> sent for a different offset, and the current offset we've gotten >>>>> from seek-data. Honestly, I'm not even sure this is a GStreamer >>>>> bug, should they really have to handle a case where the client >>>>> responds with an incorrect buffer after a seek? >>>> >>>> I don't think this is obvious at all. It's certainly not called out >>>> in the documentation. >>> The documentation does say "After receiving the seek-data signal, the >>> application should push-buffers from the new position." >> >> There's quite a difference between that and "make sure that any old >> buffers are flushed out, > Old buffers don't need to be flushed out, app source does that. >> and don't send any new buffers from the old position". > "don't send any new buffers from the old position" logically has the > same meaning as "should push-buffers from the new position". >> >>>> And, moreover, it's a lot of work, and it doesn't even look like a >>>> remotely idiomatic solution. >>> As I've already said, making sure we don't push buffers to an old >>> offset after receiving a seek seems quite idiomatic to me. >>>> Not to mention that, as I've said, appsrc really has no reason to >>>> send seek-data at all here. >>> It sends it here to ensure that pre-flush buffers are discarded. >>> After it calls seek-data, it clears the queue, because of the >>> aforementioned expectation, namely that the app will respond to >>> seek-data, this works. >> >> Well, no, I don't think it does. I think it sends seek-data here for >> "seekable" mode, because that actually makes sense, and ends up also >> sending it for "random-access" mode unnecessarily. > Sending it for "random-access" mode does makes sense, because downstream > wants all future(post flush) buffers to be from a new offset, so it > needs to tell the app to stop sending buffers from the old location, and > start from the new location, then discard the queued buffer/s for the > old location if needed. As mentioned above this, according to the > documentation, when a seek-data is sent, buffers sent afterwards must be > responsive to the new offset. Sorry, I remain unconvinced that this approach is a good one.

Sept. 29, 2021

[PATCH] dplayx: Map global memory anywhere if the standard address is unavailable.

by Tim Clem

Signed-off-by: Tim Clem <tclem(a)codeweavers.com> --- There's an existing code path and ERR log that seems to assume MapViewOfFileEx will already do this, but it doesn't. As the comments note, if this memory winds up somewhere else, it will cause problems if more than one process is trying to use dplay. However, the current code breaks in all circumstances if the fixed address is unavailable. dlls/dplayx/dplayx_global.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/dlls/dplayx/dplayx_global.c b/dlls/dplayx/dplayx_global.c index a544cd2196e9..957f7cf46f57 100644 --- a/dlls/dplayx/dplayx_global.c +++ b/dlls/dplayx/dplayx_global.c @@ -418,6 +418,15 @@ BOOL DPLAYX_ConstructData(void) FILE_MAP_WRITE, 0, 0, 0, lpDesiredMemoryMapStart ); + if( lpSharedStaticData == NULL && GetLastError() == ERROR_INVALID_ADDRESS ) + { + /* We couldn't map the data where we wanted. Try again, allowing any + * location. */ + lpSharedStaticData = MapViewOfFile( hDplayxSharedMem, + FILE_MAP_WRITE, + 0, 0, 0 ); + } + if( lpSharedStaticData == NULL ) { ERR( ": unable to map static data into process memory space (%d)\n", -- 2.33.0

Sept. 29, 2021

[PATCH 2/2] dinput: Lock around polling a HID device in joystick_osx.

by Tim Clem

If multiple threads try to read IOHIDElements and IOHIDValues from the same IOHIDDevice simultaneously, we sometimes crash deep in IOKit. Fixes a crash in GTA 4 when using a PS4 controller. Signed-off-by: Tim Clem <tclem(a)codeweavers.com> --- Testing with multiple controllers attached confirms that the per-HID device lock is appropriate. It looks like the kernel releases the underlying memory referenced by a IOHIDValueRef if you fetch another for the same element via IOHIDDeviceGetValue. Note that the same issue is present in winejoystick.drv, though there it is mitigated by the fact that you can only open each controller main element once. I'll write an analogous patch for winejoystick.drv after this one is reviewed. Even that won't help in the case of an app using both dinput and winmm to talk to the same device, but I imagine that's vanishingly rare. The use of HID reports directly, as implemented in xinput and Remi's joystick_hid, should completely avoid this issue. dlls/dinput/joystick_osx.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/dlls/dinput/joystick_osx.c b/dlls/dinput/joystick_osx.c index 2db9e521a1ed..9ee45e8a4456 100644 --- a/dlls/dinput/joystick_osx.c +++ b/dlls/dinput/joystick_osx.c @@ -100,6 +100,9 @@ WINE_DEFAULT_DEBUG_CHANNEL(dinput); static CFMutableArrayRef device_main_elements = NULL; +/* Maps IOHIDDeviceRefs to CRITICAL_SECTION pointers. */ +static CFMutableDictionaryRef hid_device_crits = NULL; + typedef struct JoystickImpl JoystickImpl; static const IDirectInputDevice8WVtbl JoystickWvt; @@ -534,8 +537,11 @@ static int find_osx_devices(void) CFArraySortValues(devices, CFRangeMake(0, num_devices), device_location_name_comparator, NULL); device_main_elements = CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks); - if (!device_main_elements) + hid_device_crits = CFDictionaryCreateMutable(kCFAllocatorDefault, 0, &kCFTypeDictionaryKeyCallBacks, NULL); + if (!device_main_elements || !hid_device_crits) { + if (device_main_elements) CFRelease(device_main_elements); + if (hid_device_crits) CFRelease(hid_device_crits); CFRelease( devices ); goto fail; } @@ -550,6 +556,15 @@ static int find_osx_devices(void) TRACE("hid_device %s\n", debugstr_device(hid_device)); top = find_top_level(hid_device, device_main_elements); num_main_elements += top; + + if ( top ) { + /* This HID device has relevent elements, so it needs a critical section. */ + CRITICAL_SECTION *cs = HeapAlloc(GetProcessHeap(), 0, sizeof(CRITICAL_SECTION)); + InitializeCriticalSection(cs); + cs->DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": hid_device_crits"); + + CFDictionaryAddValue(hid_device_crits, hid_device, cs); + } } CFRelease(devices); @@ -797,6 +812,20 @@ static IOReturn get_element_values(IOHIDDeviceRef hid_device, JoystickImpl *devi IOHIDElementRef element; IOHIDValueRef valueRef; + /* If more than one thread is trying to poll the same HID device, we can + * crash deep inside IOKit. So we enter a per-IOHIDDevice critical section + * here. Note that it must be per-HID device and not just per-JoystickImpl, + * as there may be multiple JoystickImpls referencing the same underlying + * device. */ + + CRITICAL_SECTION *crit = (CRITICAL_SECTION *)CFDictionaryGetValue(hid_device_crits, hid_device); + if (!crit) { + ERR("missing critical section for device %s\n", debugstr_device(hid_device)); + return kIOReturnError; + } + + EnterCriticalSection(crit); + for (i = 0; i < element_count; i++) { element = (IOHIDElementRef)CFArrayGetValueAtIndex(device->elements, i); @@ -810,6 +839,7 @@ static IOReturn get_element_values(IOHIDDeviceRef hid_device, JoystickImpl *devi device->element_values[i] = IOHIDValueGetIntegerValue(valueRef); } + LeaveCriticalSection(crit); return ret; } -- 2.33.0

Sept. 29, 2021

[PATCH 1/2] dinput: Factor out HID value retrieval in joystick_osx

by Tim Clem

Signed-off-by: Tim Clem <tclem(a)codeweavers.com> --- Ideally we would use IOHIDDeviceCopyValueMultiple or IOHIDTransactions to fetch all these element values at once, but my own testing and this message from an Apple engineer confirm that those don't work for input elements: https://www.mail-archive.com/usb%40lists.apple.com/msg00438.html dlls/dinput/joystick_osx.c | 57 +++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 23 deletions(-) diff --git a/dlls/dinput/joystick_osx.c b/dlls/dinput/joystick_osx.c index e8732bd696c0..2db9e521a1ed 100644 --- a/dlls/dinput/joystick_osx.c +++ b/dlls/dinput/joystick_osx.c @@ -110,6 +110,7 @@ struct JoystickImpl /* osx private */ int id; CFArrayRef elements; + int *element_values; ObjProps **propmap; FFDeviceObjectReference ff; struct list effects; @@ -753,6 +754,9 @@ static void get_osx_device_elements(JoystickImpl *device, uint64_t axis_map[8]) CFRelease(povs); CFRelease(buttons); CFRelease(elements); + + device->element_values = HeapAlloc(GetProcessHeap(), 0, + CFArrayGetCount(device->elements) * sizeof(int)); } else { @@ -786,6 +790,29 @@ static void get_osx_device_elements_props(JoystickImpl *device) } } +static IOReturn get_element_values(IOHIDDeviceRef hid_device, JoystickImpl *device) +{ + CFIndex i, element_count = CFArrayGetCount(device->elements); + IOReturn ret = kIOReturnSuccess; + IOHIDElementRef element; + IOHIDValueRef valueRef; + + for (i = 0; i < element_count; i++) + { + element = (IOHIDElementRef)CFArrayGetValueAtIndex(device->elements, i); + ret = IOHIDDeviceGetValue(hid_device, element, &valueRef); + if (ret != kIOReturnSuccess) + { + ERR("error getting value of element %s: %08x\n", debugstr_element(element), ret); + break; + } + + device->element_values[i] = IOHIDValueGetIntegerValue(valueRef); + } + + return ret; +} + static void poll_osx_device_state( IDirectInputDevice8W *iface ) { JoystickImpl *device = impl_from_IDirectInputDevice8W( iface ); @@ -805,6 +832,9 @@ static void poll_osx_device_state( IDirectInputDevice8W *iface ) if (device->elements) { + if (get_element_values(hid_device, device) != kIOReturnSuccess) + return; + int button_idx = 0; int pov_idx = 0; int slider_idx = 0; @@ -813,8 +843,7 @@ static void poll_osx_device_state( IDirectInputDevice8W *iface ) for ( idx = 0; idx < cnt; idx++ ) { - IOHIDValueRef valueRef; - int val, oldVal, newVal; + int oldVal, newVal, val = device->element_values[idx]; IOHIDElementRef element = ( IOHIDElementRef ) CFArrayGetValueAtIndex( device->elements, idx ); int type = IOHIDElementGetType( element ); @@ -826,16 +855,10 @@ static void poll_osx_device_state( IDirectInputDevice8W *iface ) TRACE("kIOHIDElementTypeInput_Button\n"); if(button_idx < 128) { - valueRef = NULL; - if (IOHIDDeviceGetValue(hid_device, element, &valueRef) != kIOReturnSuccess) - return; - if (valueRef == NULL) - return; - val = IOHIDValueGetIntegerValue(valueRef); newVal = val ? 0x80 : 0x0; oldVal = device->generic.js.rgbButtons[button_idx]; device->generic.js.rgbButtons[button_idx] = newVal; - TRACE("valueRef %s val %d oldVal %d newVal %d\n", debugstr_cf(valueRef), val, oldVal, newVal); + TRACE("val %d oldVal %d newVal %d\n", val, oldVal, newVal); if (oldVal != newVal) { inst_id = DIDFT_MAKEINSTANCE(button_idx) | DIDFT_PSHBUTTON; @@ -855,19 +878,13 @@ static void poll_osx_device_state( IDirectInputDevice8W *iface ) case MAKEUINT64(kHIDPage_GenericDesktop, kHIDUsage_GD_Hatswitch): { TRACE("kIOHIDElementTypeInput_Misc / kHIDUsage_GD_Hatswitch\n"); - valueRef = NULL; - if (IOHIDDeviceGetValue(hid_device, element, &valueRef) != kIOReturnSuccess) - return; - if (valueRef == NULL) - return; - val = IOHIDValueGetIntegerValue(valueRef); oldVal = device->generic.js.rgdwPOV[pov_idx]; if ((val > device->generic.props[idx].lDevMax) || (val < device->generic.props[idx].lDevMin)) newVal = -1; else newVal = (val - device->generic.props[idx].lDevMin) * 4500; device->generic.js.rgdwPOV[pov_idx] = newVal; - TRACE("valueRef %s val %d oldVal %d newVal %d\n", debugstr_cf(valueRef), val, oldVal, newVal); + TRACE("val %d oldVal %d newVal %d\n", val, oldVal, newVal); if (oldVal != newVal) { inst_id = DIDFT_MAKEINSTANCE(pov_idx) | DIDFT_POV; @@ -890,12 +907,6 @@ static void poll_osx_device_state( IDirectInputDevice8W *iface ) { int wine_obj = -1; - valueRef = NULL; - if (IOHIDDeviceGetValue(hid_device, element, &valueRef) != kIOReturnSuccess) - return; - if (valueRef == NULL) - return; - val = IOHIDValueGetIntegerValue(valueRef); newVal = joystick_map_axis(&device->generic.props[idx], val); switch (MAKEUINT64(usage_page, usage)) { @@ -945,7 +956,7 @@ static void poll_osx_device_state( IDirectInputDevice8W *iface ) slider_idx ++; break; } - TRACE("valueRef %s val %d oldVal %d newVal %d\n", debugstr_cf(valueRef), val, oldVal, newVal); + TRACE("val %d oldVal %d newVal %d\n", val, oldVal, newVal); if ((wine_obj != -1) && (oldVal != newVal)) { -- 2.33.0

Sept. 29, 2021

Re: GCC 11

by Sveinar Søpler

Well, debuild seem to put the same CFLAGS/LDFLAGS to gcc-mingw-w64 as it does to gcc, and i guess one of the "debian hardening" options puts some flags to mingw's LD that does not fly. This does not happen when you build "manually", so it is quite likely just a build config thing. Sorry, for this noise, as this is most certainly unrelated to the original post. I will do some investigations towards debian build settings, but i do not think it has much to do with the gcc-11 concerns you have i am afraid, although it could be worth investigating "build system flags vs. manual compile" (atleast i find it interesting). Again, i am sorry for the noise. Sveinar On 29.09.2021 18:18, Sveinar Søpler wrote: > > Sveinar > > On 29.09.2021 16:42, Eric Pouech wrote: > >> to correct myself: >> >> - I thought the trigger was "simply" the upgrade to GCC version 11... >> after some quick search, Fedora claims moving from 11.0 (F34) to 11.2 >> (F35), but looks like F34 is pushing gcc 11.2 in updates since >> July... so the root cause of those new warnings need to be clarified >> >> - the warnings I'm seeing are triggered by -Wmisleading-indentation, >> -Warray-bounds, -Wsizeof-array-div, -Wmaybe-uninitialized, which are >> all active when -Wall is used >> >> it's strange from the log link above that non of the options work >> with the cross compiler >> >> [ 569s] checking whether the cross-compiler supports -target >> x86_64-w64-mingw32 -fuse-ld=lld... no >> [ 569s] checking whether the cross-compiler supports >> -fno-strict-aliasing... no >> [ 569s] checking whether the cross-compiler supports >> -Werror=unknown-warning-option... no >> [ 569s] checking whether the cross-compiler supports >> -Werror=ignored-optimization-argument... no >> [ 569s] checking whether the cross-compiler supports >> -Wdeclaration-after-statement... no >> [ 570s] checking whether the cross-compiler supports -Wempty-body... no >> [ 570s] checking whether the cross-compiler supports >> -Wignored-qualifiers... no >> [ 570s] checking whether the cross-compiler supports -Winit-self... no >> [ 570s] checking whether the cross-compiler supports >> -Wpacked-not-aligned... no >> [ 570s] checking whether the cross-compiler supports >> -Wpragma-pack... no >> [ 570s] checking whether the cross-compiler supports >> -Wshift-overflow=2... no >> [ 570s] checking whether the cross-compiler supports >> -Wstrict-prototypes... no >> [ 570s] checking whether the cross-compiler supports >> -Wtype-limits... no >> [ 570s] checking whether the cross-compiler supports >> -Wunused-but-set-parameter... no >> [ 570s] checking whether the cross-compiler supports -Wvla... no >> [ 570s] checking whether the cross-compiler supports >> -Wwrite-strings... no >> [ 570s] checking whether the cross-compiler supports >> -Wpointer-arith... no >> [ 570s] checking whether the cross-compiler supports -Wlogical-op... no >> [ 570s] checking whether the cross-compiler supports >> -Wabsolute-value... no >> [ 570s] checking whether the cross-compiler supports -Wno-format... no >> [ 570s] checking whether the cross-compiler supports >> -Wformat-overflow... no >> [ 570s] checking whether the cross-compiler supports -Wnonnull... no >> [ 570s] checking whether the cross-compiler supports -mcx16... no >> [ 570s] checking whether the cross-compiler supports -gdwarf-2... no >> [ 571s] checking whether the cross-compiler supports >> -gstrict-dwarf... no >> [ 571s] checking whether the cross-compiler supports >> -fexcess-precision=standard... no >> I'd expect that a couple of them are supported >> > That was indeed a bit strange. When i do just a regular configure > --enable-win64 without any flags, i get this on Ubuntu 21.04: > > checking for x86_64-w64-mingw32-gcc... x86_64-w64-mingw32-gcc > checking whether x86_64-w64-mingw32-gcc works... yes > checking whether the cross-compiler supports -target > x86_64-w64-mingw32 -fuse-ld=lld... no > checking whether the cross-compiler supports -fno-strict-aliasing... yes > checking whether the cross-compiler supports > -Werror=unknown-warning-option... no > checking whether the cross-compiler supports > -Werror=ignored-optimization-argument... no > checking whether the cross-compiler supports > -Wdeclaration-after-statement... yes > checking whether the cross-compiler supports -Wempty-body... yes > checking whether the cross-compiler supports -Wignored-qualifiers... yes > checking whether the cross-compiler supports -Winit-self... yes > checking whether the cross-compiler supports -Wpacked-not-aligned... yes > checking whether the cross-compiler supports -Wpragma-pack... no > checking whether the cross-compiler supports -Wshift-overflow=2... yes > checking whether the cross-compiler supports -Wstrict-prototypes... yes > checking whether the cross-compiler supports -Wtype-limits... yes > checking whether the cross-compiler supports > -Wunused-but-set-parameter... yes > checking whether the cross-compiler supports -Wvla... yes > checking whether the cross-compiler supports -Wwrite-strings... yes > checking whether the cross-compiler supports -Wpointer-arith... yes > checking whether the cross-compiler supports -Wlogical-op... yes > checking whether the cross-compiler supports -Wabsolute-value... yes > checking whether the cross-compiler supports -Wno-format... yes > checking whether the cross-compiler supports -Wformat-overflow... yes > checking whether the cross-compiler supports -Wnonnull... yes > checking whether the cross-compiler supports -mcx16... yes > checking whether the cross-compiler supports -gdwarf-2... yes > checking whether the cross-compiler supports -gstrict-dwarf... yes > checking whether the cross-compiler supports > -fexcess-precision=standard... yes > > I do kinda fear this pulls us down a unrelated rabbithole - some > buildflags or "OBS quirks" perhaps.. dunno. In short: The log from > WineHQ buildserver does not really look like it should :) > > Sveinar >

Sept. 29, 2021

[PATCH 4/4] wined3d: Emulate fractional viewports if GL doesn't support them (v3).

by Stefan Dösinger

This allows test_fractional_viewports to pass regardless of GL capabilies. I don't think we can get rid of WINED3D_PIXEL_CENTER_INTEGER (and add +0.5 in the client libs) due to vpos in d3d9 shaders. Signed-off-by: Stefan Dösinger <stefan(a)codeweavers.com> --- Version 3: Properly assign gl_info->limits.viewport_subpixel_bits. v2 of the patch had a compile error... Version 2: Make it depend on GL_VIEWPORT_SUBPIXEL_BITS instead of support for GL_ARB_viewport_array. Floor viewport coordinates passed to GL if we use GL_ARB_viewport_array but decide the subpixel support is insufficient. --- dlls/wined3d/adapter_gl.c | 3 +++ dlls/wined3d/adapter_vk.c | 1 + dlls/wined3d/state.c | 16 ++++++++++++++++ dlls/wined3d/wined3d_private.h | 14 +++++++++++--- 4 files changed, 31 insertions(+), 3 deletions(-) diff --git a/dlls/wined3d/adapter_gl.c b/dlls/wined3d/adapter_gl.c index 386d26828dc..d31632dd474 100644 --- a/dlls/wined3d/adapter_gl.c +++ b/dlls/wined3d/adapter_gl.c @@ -991,6 +991,7 @@ static void quirk_broken_arb_fog(struct wined3d_gl_info *gl_info) static void quirk_broken_viewport_subpixel_bits(struct wined3d_gl_info *gl_info) { + gl_info->limits.viewport_subpixel_bits = 0; if (gl_info->supported[ARB_CLIP_CONTROL]) { TRACE("Disabling ARB_clip_control.\n"); @@ -3655,6 +3656,7 @@ static BOOL wined3d_adapter_init_gl_caps(struct wined3d_adapter *adapter, TRACE("Disabling ARB_clip_control because viewport subpixel bits < 8.\n"); gl_info->supported[ARB_CLIP_CONTROL] = FALSE; } + gl_info->limits.viewport_subpixel_bits = subpixel_bits; } if (gl_info->supported[ARB_CLIP_CONTROL] && !gl_info->supported[ARB_VIEWPORT_ARRAY]) { @@ -5133,6 +5135,7 @@ static void wined3d_adapter_gl_init_d3d_info(struct wined3d_adapter_gl *adapter_ d3d_info->full_ffp_varyings = !!(shader_caps.wined3d_caps & WINED3D_SHADER_CAP_FULL_FFP_VARYINGS); d3d_info->scaled_resolve = !!gl_info->supported[EXT_FRAMEBUFFER_MULTISAMPLE_BLIT_SCALED]; d3d_info->pbo = !!gl_info->supported[ARB_PIXEL_BUFFER_OBJECT]; + d3d_info->subpixel_viewport = gl_info->limits.viewport_subpixel_bits >= 8; d3d_info->feature_level = feature_level_from_caps(gl_info, &shader_caps, &fragment_caps); d3d_info->filling_convention_nudge = gl_info->filling_convention_nudge; diff --git a/dlls/wined3d/adapter_vk.c b/dlls/wined3d/adapter_vk.c index 324f4316901..ed4559faec6 100644 --- a/dlls/wined3d/adapter_vk.c +++ b/dlls/wined3d/adapter_vk.c @@ -2205,6 +2205,7 @@ static void wined3d_adapter_vk_init_d3d_info(struct wined3d_adapter_vk *adapter_ d3d_info->scaled_resolve = false; d3d_info->pbo = true; d3d_info->feature_level = feature_level_from_caps(&shader_caps); + d3d_info->subpixel_viewport = true; /* Like GL, Vulkan doesn't explicitly specify a filling convention and only mandates that a * shared edge of two adjacent triangles generate a fragment for exactly one of the triangles. diff --git a/dlls/wined3d/state.c b/dlls/wined3d/state.c index 5c1c69fb650..85c80198ebf 100644 --- a/dlls/wined3d/state.c +++ b/dlls/wined3d/state.c @@ -4202,6 +4202,22 @@ static void viewport_miscpart(struct wined3d_context *context, const struct wine viewports[i * 4 + 1] = vp[i].y; viewports[i * 4 + 2] = vp[i].width; viewports[i * 4 + 3] = vp[i].height; + + /* Don't pass fractionals to GL if we earlier decided not to use + * this functionality for two reasons: First, GL might offer us + * fewer than 8 bits, and still make use of the fractional, in + * addition to the emulation we apply in shader_get_position_fixup. + * Second, even if GL tells us it has no subpixel precision (Mac OS!) + * it might still do something with the fractional amount, e.g. + * round it upwards. I can't find any info on rounding in + * GL_ARB_viewport_array. */ + if (!context->d3d_info->subpixel_viewport) + { + viewports[i * 4] = floor(viewports[i * 4]); + viewports[i * 4 + 1] = floor(viewports[i * 4 + 1]); + viewports[i * 4 + 2] = floor(viewports[i * 4 + 2]); + viewports[i * 4 + 3] = floor(viewports[i * 4 + 3]); + } } if (context->viewport_count > state->viewport_count) diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h index 0990bffed21..6610f1333ae 100644 --- a/dlls/wined3d/wined3d_private.h +++ b/dlls/wined3d/wined3d_private.h @@ -240,6 +240,7 @@ struct wined3d_d3d_info uint32_t full_ffp_varyings : 1; uint32_t scaled_resolve : 1; uint32_t pbo : 1; + uint32_t subpixel_viewport : 1; enum wined3d_feature_level feature_level; DWORD multisample_draw_location; @@ -3224,6 +3225,7 @@ struct wined3d_gl_limits unsigned int framebuffer_width; unsigned int framebuffer_height; + unsigned int viewport_subpixel_bits; UINT glsl_varyings; UINT glsl_vs_float_constants; @@ -5681,7 +5683,7 @@ static inline BOOL shader_is_scalar(const struct wined3d_shader_register *reg) static inline void shader_get_position_fixup(const struct wined3d_context *context, const struct wined3d_state *state, unsigned int fixup_count, float *position_fixup) { - float center_offset; + float center_offset, x = 0.0f, y = 0.0f; unsigned int i; /* See get_projection_matrix() in utils.c for a discussion of the position fixup. @@ -5698,8 +5700,14 @@ static inline void shader_get_position_fixup(const struct wined3d_context *conte { position_fixup[4 * i ] = 1.0f; position_fixup[4 * i + 1] = 1.0f; - position_fixup[4 * i + 2] = center_offset / state->viewports[i].width; - position_fixup[4 * i + 3] = -center_offset / state->viewports[i].height; + if (!context->d3d_info->subpixel_viewport) + { + double dummy; + x = modf(state->viewports[i].x, &dummy) * 2.0f; + y = modf(state->viewports[i].y, &dummy) * 2.0f; + } + position_fixup[4 * i + 2] = (center_offset + x) / state->viewports[i].width; + position_fixup[4 * i + 3] = -(center_offset + y) / state->viewports[i].height; if (context->render_offscreen) { -- 2.32.0

Sept. 29, 2021

[PATCH 3/4] d3d9/tests: Test triangle filling convention.

by Stefan Dösinger

Signed-off-by: Stefan Dösinger <stefan(a)codeweavers.com> --- There is a certain overlap between this test and test_[fractional_]viewport. This test explicitly tests which fragment lights up and how diagonals that are hit by two triangles are handled, whereas test_viewport checks for a mismatch between fragment location and position of that fragment inside the triangle. Both tests are worth having IMO. I'll port it to ddraw-d3d11 once the merits of the test and geometry nudge have been agreed upon. --- dlls/d3d9/tests/visual.c | 487 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 487 insertions(+) diff --git a/dlls/d3d9/tests/visual.c b/dlls/d3d9/tests/visual.c index 3820252fdef..e0ce6ade4be 100644 --- a/dlls/d3d9/tests/visual.c +++ b/dlls/d3d9/tests/visual.c @@ -27019,6 +27019,492 @@ static void test_dynamic_map_synchronization(void) DestroyWindow(window); } +static void test_filling_convention(void) +{ + static const DWORD colour_bottom = 0x00ffff00; + static const DWORD colour_clear = 0x000000ff; + static const DWORD colour_right = 0x00000000; + static const DWORD colour_left = 0x00ff0000; + static const DWORD colour_top = 0x0000ff00; + IDirect3DSurface9 *rt, *backbuffer, *cur; + IDirect3DVertexShader9 *shader = NULL; + struct surface_readback rb; + IDirect3DDevice9 *device; + unsigned int i, j, x, y; + DWORD colour, expected; + IDirect3D9 *d3d; + ULONG refcount; + D3DCAPS9 caps; + HWND window; + HRESULT hr; + BOOL todo; + + static const unsigned int vp_size = 8; + const D3DVIEWPORT9 vp = { 0, 0, vp_size, vp_size, 0.0, 1.0 }; + static const DWORD vs_code[] = + { + 0xfffe0101, /* vs_1_1 */ + 0x0000001f, 0x80000000, 0x900f0000, /* dcl_position v0 */ + 0x0000001f, 0x8000000a, 0x900f0001, /* dcl_color0 v1 */ + 0x00000001, 0xc00f0000, 0x90e40000, /* mov oPos, v0 */ + 0x00000001, 0xd00f0000, 0x90e40001, /* mov oD0, v1 */ + 0x0000ffff /* end */ + }; + + /* This test data follows the examples in MSDN's + * "Rasterization Rules (Direct3D 9)" article at + * https://docs.microsoft.com/en-us/windows/win32/direct3d9/rasterization-rules */ + static const float eps = 1.0f / 512.0f; + const struct + { + struct vec3 position; + DWORD diffuse; + } + center_tris[] = + { + /* left */ + {{-2.5f / 4.0f, -1.5f / 4.0f, 0.0f}, colour_left}, + {{-2.5f / 4.0f, 2.5f / 4.0f, 0.0f}, colour_left}, + {{-1.5f / 4.0f, 0.5f / 4.0f, 0.0f}, colour_left}, + + /* top */ + {{-1.5f / 4.0f, 0.5f / 4.0f, 0.0f}, colour_top}, + {{-2.5f / 4.0f, 2.5f / 4.0f, 0.0f}, colour_top}, + {{-0.5f / 4.0f, 2.5f / 4.0f, 0.0f}, colour_top}, + + /* right */ + {{-0.5f / 4.0f, -1.5f / 4.0f, 0.0f}, colour_right}, + {{-1.5f / 4.0f, 0.5f / 4.0f, 0.0f}, colour_right}, + {{-0.5f / 4.0f, 2.5f / 4.0f, 0.0f}, colour_right}, + + /* bottom */ + {{-2.5f / 4.0f, -1.5f / 4.0f, 0.0f}, colour_bottom}, + {{-1.5f / 4.0f, 0.5f / 4.0f, 0.0f}, colour_bottom}, + {{-0.5f / 4.0f, -1.5f / 4.0f, 0.0f}, colour_bottom}, + + }, + edge_tris[] = + { + /* left */ + {{-2.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_left}, + {{-2.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_left}, + {{-1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_left}, + + /* top */ + {{-1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_top}, + {{-2.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_top}, + {{ 0.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_top}, + + /* right */ + {{ 0.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_right}, + {{-1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_right}, + {{ 0.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_right}, + + /* bottom */ + {{-2.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_bottom}, + {{-1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_bottom}, + {{ 0.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_bottom}, + }, + nudge_right_tris[] = + { + /* left */ + {{eps - 2.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_left}, + {{eps - 2.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_left}, + {{eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_left}, + + /* top */ + {{eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_top}, + {{eps - 2.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_top}, + {{eps - 0.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_top}, + + /* right */ + {{eps - 0.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_right}, + {{eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_right}, + {{eps - 0.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_right}, + + /* bottom */ + {{eps - 2.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_bottom}, + {{eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_bottom}, + {{eps - 0.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_bottom}, + }, + nudge_left_tris[] = + { + {{-eps - 2.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_left}, + {{-eps - 2.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_left}, + {{-eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_left}, + + /* top */ + {{-eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_top}, + {{-eps - 2.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_top}, + {{-eps - 0.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_top}, + + /* right */ + {{-eps - 0.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_right}, + {{-eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_right}, + {{-eps - 0.0f / 4.0f, 3.0f / 4.0f, 0.0f}, colour_right}, + + /* bottom */ + {{-eps - 2.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_bottom}, + {{-eps - 1.0f / 4.0f, 1.0f / 4.0f, 0.0f}, colour_bottom}, + {{-eps - 0.0f / 4.0f, -1.0f / 4.0f, 0.0f}, colour_bottom}, + }, + nudge_top_tris[] = + { + /* left */ + {{-2.0f / 4.0f, eps - 1.0f / 4.0f, 0.0f}, colour_left}, + {{-2.0f / 4.0f, eps + 3.0f / 4.0f, 0.0f}, colour_left}, + {{-1.0f / 4.0f, eps + 1.0f / 4.0f, 0.0f}, colour_left}, + + /* top */ + {{-1.0f / 4.0f, eps + 1.0f / 4.0f, 0.0f}, colour_top}, + {{-2.0f / 4.0f, eps + 3.0f / 4.0f, 0.0f}, colour_top}, + {{ 0.0f / 4.0f, eps + 3.0f / 4.0f, 0.0f}, colour_top}, + + /* right */ + {{ 0.0f / 4.0f, eps - 1.0f / 4.0f, 0.0f}, colour_right}, + {{-1.0f / 4.0f, eps + 1.0f / 4.0f, 0.0f}, colour_right}, + {{ 0.0f / 4.0f, eps + 3.0f / 4.0f, 0.0f}, colour_right}, + + /* bottom */ + {{-2.0f / 4.0f, eps - 1.0f / 4.0f, 0.0f}, colour_bottom}, + {{-1.0f / 4.0f, eps + 1.0f / 4.0f, 0.0f}, colour_bottom}, + {{ 0.0f / 4.0f, eps - 1.0f / 4.0f, 0.0f}, colour_bottom}, + }, + nudge_bottom_tris[] = + { + /* left */ + {{-2.0f / 4.0f, -eps - 1.0f / 4.0f, 0.0f}, colour_left}, + {{-2.0f / 4.0f, -eps + 3.0f / 4.0f, 0.0f}, colour_left}, + {{-1.0f / 4.0f, -eps + 1.0f / 4.0f, 0.0f}, colour_left}, + + /* top */ + {{-1.0f / 4.0f, -eps + 1.0f / 4.0f, 0.0f}, colour_top}, + {{-2.0f / 4.0f, -eps + 3.0f / 4.0f, 0.0f}, colour_top}, + {{ 0.0f / 4.0f, -eps + 3.0f / 4.0f, 0.0f}, colour_top}, + + /* right */ + {{ 0.0f / 4.0f, -eps - 1.0f / 4.0f, 0.0f}, colour_right}, + {{-1.0f / 4.0f, -eps + 1.0f / 4.0f, 0.0f}, colour_right}, + {{ 0.0f / 4.0f, -eps + 3.0f / 4.0f, 0.0f}, colour_right}, + + /* bottom */ + {{-2.0f / 4.0f, -eps - 1.0f / 4.0f, 0.0f}, colour_bottom}, + {{-1.0f / 4.0f, -eps + 1.0f / 4.0f, 0.0f}, colour_bottom}, + {{ 0.0f / 4.0f, -eps - 1.0f / 4.0f, 0.0f}, colour_bottom}, + }; + + /* FIXME: Is the POSITIONT coord system flipped vs the regular one? */ + const struct + { + struct vec4 position; + DWORD diffuse; + } + center_tris_t[] = + { + /* left */ + {{ 1.5f, 1.5f, 0.0f, 1.0f}, colour_left}, + {{ 2.5f, 3.5f, 0.0f, 1.0f}, colour_left}, + {{ 1.5f, 5.5f, 0.0f, 1.0f}, colour_left}, + + /* top */ + {{ 1.5f, 1.5f, 0.0f, 1.0f}, colour_top}, + {{ 3.5f, 1.5f, 0.0f, 1.0f}, colour_top}, + {{ 2.5f, 3.5f, 0.0f, 1.0f}, colour_top}, + + /* right */ + {{ 3.5f, 1.5f, 0.0f, 1.0f}, colour_right}, + {{ 3.5f, 5.5f, 0.0f, 1.0f}, colour_right}, + {{ 2.5f, 3.5f, 0.0f, 1.0f}, colour_right}, + + /* bottom */ + {{ 2.5f, 3.5f, 0.0f, 1.0f}, colour_bottom}, + {{ 3.5f, 5.5f, 0.0f, 1.0f}, colour_bottom}, + {{ 1.5f, 5.5f, 0.0f, 1.0f}, colour_bottom}, + }, + edge_tris_t[] = + { + /* left */ + {{ 2.0f, 1.0f, 0.0f, 1.0f}, colour_left}, + {{ 3.0f, 3.0f, 0.0f, 1.0f}, colour_left}, + {{ 2.0f, 5.0f, 0.0f, 1.0f}, colour_left}, + + /* top */ + {{ 2.0f, 1.0f, 0.0f, 1.0f}, colour_top}, + {{ 4.0f, 1.0f, 0.0f, 1.0f}, colour_top}, + {{ 3.0f, 3.0f, 0.0f, 1.0f}, colour_top}, + + /* right */ + {{ 4.0f, 1.0f, 0.0f, 1.0f}, colour_right}, + {{ 4.0f, 5.0f, 0.0f, 1.0f}, colour_right}, + {{ 3.0f, 3.0f, 0.0f, 1.0f}, colour_right}, + + /* bottom */ + {{ 3.0f, 3.0f, 0.0f, 1.0f}, colour_bottom}, + {{ 4.0f, 5.0f, 0.0f, 1.0f}, colour_bottom}, + {{ 2.0f, 5.0f, 0.0f, 1.0f}, colour_bottom}, + }; + + const struct + { + const void *geometry; + size_t stride; + DWORD fvf; + const char *expected[8]; + } + tests[] = + { + { + center_tris, + sizeof(center_tris[0]), + D3DFVF_XYZ | D3DFVF_DIFFUSE, + { + " ", + " ", + " TT ", + " LR ", + " LR ", + " BB ", + " ", + " " + } + }, + { + edge_tris, + sizeof(edge_tris[0]), + D3DFVF_XYZ | D3DFVF_DIFFUSE, + { + " ", + " TT ", + " LT ", + " LR ", + " LB ", + " ", + " ", + " " + } + }, + { + nudge_right_tris, + sizeof(nudge_right_tris[0]), + D3DFVF_XYZ | D3DFVF_DIFFUSE, + { + " ", + " TT ", + " TR ", + " LR ", + " BR ", + " ", + " ", + " " + } + }, + { + nudge_left_tris, + sizeof(nudge_left_tris[0]), + D3DFVF_XYZ | D3DFVF_DIFFUSE, + { + " ", + " TT ", + " LT ", + " LR ", + " LB ", + " ", + " ", + " " + } + }, + { + nudge_top_tris, + sizeof(nudge_top_tris[0]), + D3DFVF_XYZ | D3DFVF_DIFFUSE, + { + " ", + " LT ", + " LT ", + " LB ", + " LB ", + " ", + " ", + " " + } + }, + { + nudge_bottom_tris, + sizeof(nudge_bottom_tris[0]), + D3DFVF_XYZ | D3DFVF_DIFFUSE, + { + " ", + " ", + " LT ", + " Lt ", + " LB ", + " lB ", + " ", + " " + } + }, + { + center_tris_t, + sizeof(center_tris_t[0]), + D3DFVF_XYZRHW | D3DFVF_DIFFUSE, + { + " ", + " ", + " TT ", + " LR ", + " LR ", + " BB ", + " ", + " " + } + }, + { + edge_tris_t, + sizeof(edge_tris_t[0]), + D3DFVF_XYZRHW | D3DFVF_DIFFUSE, + { + " ", + " TT ", + " LT ", + " LR ", + " LB ", + " ", + " ", + " " + } + }, + }; + + window = create_window(); + d3d = Direct3DCreate9(D3D_SDK_VERSION); + ok(!!d3d, "Failed to create a D3D object.\n"); + + if (!(device = create_device(d3d, window, window, TRUE))) + { + skip("Failed to create a 3D device.\n"); + IDirect3D9_Release(d3d); + DestroyWindow(window); + return; + } + + hr = IDirect3DDevice9_CreateRenderTarget(device, vp_size, vp_size, + D3DFMT_A8R8G8B8, D3DMULTISAMPLE_NONE, 0, FALSE, &rt, NULL); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirect3DDevice9_GetBackBuffer(device, 0, 0, D3DBACKBUFFER_TYPE_MONO, &backbuffer); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + + hr = IDirect3DDevice9_GetDeviceCaps(device, &caps); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + if (caps.VertexShaderVersion >= D3DVS_VERSION(1, 1)) + { + hr = IDirect3DDevice9_CreateVertexShader(device, vs_code, &shader); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + } + else + skip("Skipping vertex shader codepath in filling convention test.\n"); + + hr = IDirect3DDevice9_SetRenderState(device, D3DRS_LIGHTING, FALSE); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirect3DDevice9_SetRenderState(device, D3DRS_ZENABLE, D3DZB_FALSE); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + + for (i = 0; i < ARRAY_SIZE(tests); ++i) + { + hr = IDirect3DDevice9_SetFVF(device, tests[i].fvf); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + + /* Run tests with shader and fixed function vertex processing if shaders are + * supported. There's no point in running the XYZRHW tests with a VS though. */ + if (shader && ((tests[i].fvf & D3DFVF_POSITION_MASK) == D3DFVF_XYZ)) + j = 0; + else + j = 2; + + for (; j < 4; ++j) + { + cur = (j & 1) ? rt : backbuffer; + + hr = IDirect3DDevice9_SetVertexShader(device, (j & 2) ? NULL : shader); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + + hr = IDirect3DDevice9_SetRenderTarget(device, 0, cur); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirect3DDevice9_Clear(device, 0, NULL, D3DCLEAR_TARGET, colour_clear, 0.0f, 0); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirect3DDevice9_SetViewport(device, &vp); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + + hr = IDirect3DDevice9_BeginScene(device); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirect3DDevice9_DrawPrimitiveUP(device, D3DPT_TRIANGLELIST, 4, tests[i].geometry, tests[i].stride); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + hr = IDirect3DDevice9_EndScene(device); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + + get_rt_readback(cur, &rb); + for (y = 0; y < 8; y++) + { + for (x = 0; x < 8; x++) + { + todo = FALSE; + switch (tests[i].expected[y][x]) + { + case 'l': todo = TRUE; + case 'L': + expected = colour_left; + break; + case 't': todo = TRUE; + case 'T': + expected = colour_top; + break; + case 'r': todo = TRUE; + case 'R': + expected = colour_right; + break; + case 'b': todo = TRUE; + case 'B': + expected = colour_bottom; + break; + case ' ': + expected = colour_clear; + break; + default: + ok(0, "Unexpected entry in expected test char\n"); + expected = 0xdeadbeef; + } + colour = get_readback_color(&rb, x, y); + /* The nudge-to-bottom test fails on cards that give us a bottom-left + * filling convention. The cause isn't the bottom part of the filling + * convention, but because wined3d will nudge geometry to the left to + * keep diagonals (the 'R' in test case 'edge_tris') intact. */ + todo_wine_if(todo && !color_match(colour, expected, 1)) + ok(color_match(colour, expected, 1), "Got unexpected colour %08x, %ux%u, case %u, j %u.\n", + colour, x, y, i, j); + } + } + release_surface_readback(&rb); + + /* For debugging */ + if (cur != backbuffer) + { + hr = IDirect3DDevice9_StretchRect(device, rt, NULL, backbuffer, NULL, D3DTEXF_POINT); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + } + hr = IDirect3DDevice9_Present(device, NULL, NULL, NULL, NULL); + ok(hr == D3D_OK, "Got unexpected hr %#x.\n", hr); + } + } + + if (shader) + IDirect3DVertexShader9_Release(shader); + IDirect3DSurface9_Release(backbuffer); + IDirect3DSurface9_Release(rt); + refcount = IDirect3DDevice9_Release(device); + ok(!refcount, "Device has %u references left.\n", refcount); + IDirect3D9_Release(d3d); + DestroyWindow(window); +} + START_TEST(visual) { D3DADAPTER_IDENTIFIER9 identifier; @@ -27168,4 +27654,5 @@ START_TEST(visual) test_alpha_to_coverage(); test_sample_mask(); test_dynamic_map_synchronization(); + test_filling_convention(); } -- 2.32.0

Sept. 29, 2021

[PATCH 2/4] wined3d: Test if a nudge is needed to get the correct filling convention (v2).

by Stefan Dösinger

This fixes stray lines in GameFace GUIs, e.g. in World of Tanks. Signed-off-by: Stefan Dösinger <stefan(a)codeweavers.com> --- Version 2: *) Use a binary search for the fixup value. *) Change how an unexpected test result is detected. *) Add a comment about this to the Vulkan backend. *) Small wording changes. Re Vulkan: If you can refer me to the place in the spec that mandates a top-left filling convention I am more than happy to add it in the comment. I really don't want to add this detection mess to the Vulkan backend. I am also more than happy to just drop this nudge thingy unconditionally. I am not aware of a card + game combination that is fixed by this. Spore doesn't need it on my gf9600. I didn't bother to download Everquest, I expect it to have changed a lot in the past 10 years. I don't have access to a dx9 level card right now, so I can't entirely rule out that Spore needs this nudge on those cards. The ddraw tests started succeeding in the todo block for a few of the tested pixels. I looked at the output results and I don't think it happened because the Z values are more precise now. I think a few bits flipped (in the random readback we get) and now a few of those tests were inside the expected diff. --- dlls/d3d11/tests/d3d11.c | 1 - dlls/ddraw/tests/ddraw4.c | 6 +- dlls/ddraw/tests/ddraw7.c | 6 +- dlls/wined3d/adapter_gl.c | 50 ++++++++++++++ dlls/wined3d/adapter_vk.c | 10 +++ dlls/wined3d/state.c | 3 +- dlls/wined3d/utils.c | 118 ++++++++++++++++++++++++++++++--- dlls/wined3d/wined3d_private.h | 13 +++- 8 files changed, 189 insertions(+), 18 deletions(-) diff --git a/dlls/d3d11/tests/d3d11.c b/dlls/d3d11/tests/d3d11.c index d8c10ab054c..9bff90bc1e7 100644 --- a/dlls/d3d11/tests/d3d11.c +++ b/dlls/d3d11/tests/d3d11.c @@ -28155,7 +28155,6 @@ static void test_fractional_viewports(void) ok(compare_float(v->x, expected.x, 0) && compare_float(v->y, expected.y, 0), "Got fragcoord {%.8e, %.8e}, expected {%.8e, %.8e} at (%u, %u), offset %.8e.\n", v->x, v->y, expected.x, expected.y, x, y, viewport_offsets[i]); - todo_wine ok(compare_float(v->z, expected.z, 2) && compare_float(v->w, expected.w, 2), "Got texcoord {%.8e, %.8e}, expected {%.8e, %.8e} at (%u, %u), offset %.8e.\n", v->z, v->w, expected.z, expected.w, x, y, viewport_offsets[i]); diff --git a/dlls/ddraw/tests/ddraw4.c b/dlls/ddraw/tests/ddraw4.c index 6b514b15e25..4f052256882 100644 --- a/dlls/ddraw/tests/ddraw4.c +++ b/dlls/ddraw/tests/ddraw4.c @@ -16118,8 +16118,10 @@ static void test_depth_readback(void) /* The ddraw4 version of this test behaves similarly to the ddraw7 version on Nvidia GPUs, * except that Geforce 7 also returns garbage data in D24S8, whereas the ddraw7 version * returns 0 for that format. Give up on pre-filtering formats, accept Nvidia as generally - * broken here, but still expect at least one format (D16 or D24X8 in practise) to pass. */ - todo_wine_if(tests[i].todo) + * broken here, but still expect at least one format (D16 or D24X8 in practise) to pass. + * + * Some of the tested places pass on some GPUs on Wine by accident. */ + todo_wine_if(tests[i].todo && !compare_uint(expected_depth, depth, max_diff)) ok(compare_uint(expected_depth, depth, max_diff) || ddraw_is_nvidia(ddraw), "Test %u: Got depth 0x%08x (diff %d), expected 0x%08x+/-%u, at %u, %u.\n", i, depth, expected_depth - depth, expected_depth, max_diff, x, y); diff --git a/dlls/ddraw/tests/ddraw7.c b/dlls/ddraw/tests/ddraw7.c index 4c42d6f4b64..4402f2d93b5 100644 --- a/dlls/ddraw/tests/ddraw7.c +++ b/dlls/ddraw/tests/ddraw7.c @@ -15597,8 +15597,10 @@ static void test_depth_readback(void) * Geforce GTX 650 has working D16 and D24, but D24S8 returns 0. * * Arx Fatalis is broken on the Geforce 9 in the same way it was broken in Wine (bug 43654). - * The !tests[i].s_depth is supposed to rule out D16 on GF9 and D24X8 on GF7. */ - todo_wine_if(tests[i].todo) + * The !tests[i].s_depth is supposed to rule out D16 on GF9 and D24X8 on GF7. + * + * Some of the tested places pass on some GPUs on Wine by accident. */ + todo_wine_if(tests[i].todo && !compare_uint(expected_depth, depth, max_diff)) ok(compare_uint(expected_depth, depth, max_diff) || (ddraw_is_nvidia(ddraw) && (all_zero || all_one || !tests[i].s_depth)), "Test %u: Got depth 0x%08x (diff %d), expected 0x%08x+/-%u, at %u, %u.\n", diff --git a/dlls/wined3d/adapter_gl.c b/dlls/wined3d/adapter_gl.c index bba728e2fb5..386d26828dc 100644 --- a/dlls/wined3d/adapter_gl.c +++ b/dlls/wined3d/adapter_gl.c @@ -5134,6 +5134,7 @@ static void wined3d_adapter_gl_init_d3d_info(struct wined3d_adapter_gl *adapter_ d3d_info->scaled_resolve = !!gl_info->supported[EXT_FRAMEBUFFER_MULTISAMPLE_BLIT_SCALED]; d3d_info->pbo = !!gl_info->supported[ARB_PIXEL_BUFFER_OBJECT]; d3d_info->feature_level = feature_level_from_caps(gl_info, &shader_caps, &fragment_caps); + d3d_info->filling_convention_nudge = gl_info->filling_convention_nudge; if (gl_info->supported[ARB_TEXTURE_MULTISAMPLE]) d3d_info->multisample_draw_location = WINED3D_LOCATION_TEXTURE_RGB; @@ -5141,6 +5142,53 @@ static void wined3d_adapter_gl_init_d3d_info(struct wined3d_adapter_gl *adapter_ d3d_info->multisample_draw_location = WINED3D_LOCATION_RB_MULTISAMPLE; } +static float wined3d_adapter_find_fill_nudge(struct wined3d_caps_gl_ctx *ctx) +{ + static const float test_array[] = + { + 0.0f, + -1.0f / 1024.0f, + -1.0f / 512.0f, + -1.0f / 256.0f, + -1.0f / 128.0f, + -1.0f / 64.0f + }; + unsigned int good = ARRAY_SIZE(test_array), bad = 0, test; + float value; + + if (wined3d_settings.offscreen_rendering_mode != ORM_FBO) + goto end; + + while (good != bad) + { + test = (good + bad) / 2; + value = test_array[test]; + TRACE("Good %u bad %u, test %u.\n", good, bad, test); + if (wined3d_caps_gl_ctx_test_filling_convention(ctx, value)) + good = test; + else + bad = test + 1; + } + + if (good < ARRAY_SIZE(test_array)) + { + value = test_array[good]; + if (value) + WARN("Using a filling convention fixup nudge of -1/%f.\n", -1.0f / value); + else + TRACE("No need for a filling convetion nudge.\n"); + + return value; + } + + FIXME("Did not find a way to get the filling convention we want.\n"); + +end: + /* This value was used unconditionally before the dynamic test function was + * introduced. */ + return -1.0f / 64.0f; +} + static BOOL wined3d_adapter_gl_init(struct wined3d_adapter_gl *adapter_gl, unsigned int ordinal, unsigned int wined3d_creation_flags) { @@ -5226,6 +5274,8 @@ static BOOL wined3d_adapter_gl_init(struct wined3d_adapter_gl *adapter_gl, return FALSE; } + gl_info->filling_convention_nudge = wined3d_adapter_find_fill_nudge(&caps_gl_ctx); + wined3d_adapter_gl_init_d3d_info(adapter_gl, wined3d_creation_flags); if (!adapter_gl->a.d3d_info.shader_color_key) diff --git a/dlls/wined3d/adapter_vk.c b/dlls/wined3d/adapter_vk.c index 18c73312daf..324f4316901 100644 --- a/dlls/wined3d/adapter_vk.c +++ b/dlls/wined3d/adapter_vk.c @@ -2206,6 +2206,16 @@ static void wined3d_adapter_vk_init_d3d_info(struct wined3d_adapter_vk *adapter_ d3d_info->pbo = true; d3d_info->feature_level = feature_level_from_caps(&shader_caps); + /* Like GL, Vulkan doesn't explicitly specify a filling convention and only mandates that a + * shared edge of two adjacent triangles generate a fragment for exactly one of the triangles. + * vktRasterizationTests.cpp from the vulkan CTS tests this by drawing two triangles with a + * shared edge with additive blending. + * + * However, every Vulkan implementation we have seen so far uses a top-left rule. Hardware + * that differs either predates Vulkan (d3d9 class HW, GeForce 9xxx) or behaves the way we + * want in Vulkan (MacOS Radeon driver through MoltenVK). */ + d3d_info->filling_convention_nudge = 0.0; + d3d_info->multisample_draw_location = WINED3D_LOCATION_TEXTURE_RGB; } diff --git a/dlls/wined3d/state.c b/dlls/wined3d/state.c index 8316269afcf..5c1c69fb650 100644 --- a/dlls/wined3d/state.c +++ b/dlls/wined3d/state.c @@ -4233,13 +4233,14 @@ static void viewport_miscpart_cc(struct wined3d_context *context, const struct wined3d_gl_info *gl_info = wined3d_context_gl(context)->gl_info; /* See get_projection_matrix() in utils.c for a discussion about those values. */ float pixel_center_offset = context->d3d_info->wined3d_creation_flags - & WINED3D_PIXEL_CENTER_INTEGER ? 63.0f / 128.0f : -1.0f / 128.0f; + & WINED3D_PIXEL_CENTER_INTEGER ? 0.5f : 0.0f; struct wined3d_viewport vp[WINED3D_MAX_VIEWPORTS]; GLdouble depth_ranges[2 * WINED3D_MAX_VIEWPORTS]; GLfloat viewports[4 * WINED3D_MAX_VIEWPORTS]; unsigned int i, reset_count = 0; float min_z, max_z; + pixel_center_offset += context->d3d_info->filling_convention_nudge / 2.0f; get_viewports(context, state, state->viewport_count, vp); GL_EXTCALL(glClipControl(context->render_offscreen ? GL_UPPER_LEFT : GL_LOWER_LEFT, GL_ZERO_TO_ONE)); diff --git a/dlls/wined3d/utils.c b/dlls/wined3d/utils.c index 0a1e0707359..96351d7ead6 100644 --- a/dlls/wined3d/utils.c +++ b/dlls/wined3d/utils.c @@ -3928,6 +3928,100 @@ BOOL wined3d_caps_gl_ctx_test_viewport_subpixel_bits(struct wined3d_caps_gl_ctx return TRUE; } +bool wined3d_caps_gl_ctx_test_filling_convention(struct wined3d_caps_gl_ctx *ctx, float nudge) +{ + static const struct wined3d_color red = {1.0f, 0.0f, 0.0f, 1.0f}; + const struct wined3d_gl_info *gl_info = ctx->gl_info; + unsigned int x, y, clear = 0, draw = 0; + GLuint texture, fbo; + DWORD readback[8][8]; + + /* This is a very simple test to find out how GL handles polygon edges: + * Draw a quad exactly through 4 pixel centers in an 8x8 viewport and see + * which pixel it ends up in. So far we've seen top left and bottom + * left conventions. This test may produce unexpected results if the + * driver forces multisampling on us. + * + * If we find a bottom-left filling behavior we also nudge the x-axis + * by the same amount. This is necessary to keep diagonals that go + * through the pixel center intact. + * + * Note that we are ignoring some settings that might influence the + * driver: How we switch GL to an upper-left coordinate system, + * shaders vs fixed function GL. Testing these isn't possible with + * the current draw_test_quad() infrastructure. Also the test is + * skipped if we are not using FBOs. Drawing into the onscreen + * frame buffer may also yield different driver behavior. + * + * The minimum nudge also depends on the viewport size, although + * the relation between those two is GPU dependent and not exactly + * sensible. E.g. a 8192x8192 viewport on a GeForce 9 needs at + * least a nudge of 1/240.9, whereas a 8x8 one needs 1/255.982; + * 32x32 needs 1/255.935. 4x4 and lower are happy with something + * below 1/256. The 8x8 size below has been arbitrarily chosen to + * get a useful result out of that card and avoid allocating a + * gigantic texture during library init. + * + * Newer cards usually do the right thing anyway. In cases where + * they do not (e.g. Radeon GPUs in a macbookpro14,3 running MacOS) + * a nudge of 1/2^20 is enough. */ + const struct wined3d_vec3 edge_geometry[] = + { + {(-1.0f + nudge) / 8.0f, (-1.0f + nudge) / 8.0f, 0.0f}, + {( 1.0f + nudge) / 8.0f, (-1.0f + nudge) / 8.0f, 0.0f}, + {(-1.0f + nudge) / 8.0f, ( 1.0f + nudge) / 8.0f, 0.0f}, + {( 1.0f + nudge) / 8.0f, ( 1.0f + nudge) / 8.0f, 0.0f}, + }; + + gl_info->gl_ops.gl.p_glGenTextures(1, &texture); + gl_info->gl_ops.gl.p_glBindTexture(GL_TEXTURE_2D, texture); + gl_info->gl_ops.gl.p_glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAX_LEVEL, 0); + gl_info->gl_ops.gl.p_glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA8, 8, 8, 0, + GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV, NULL); + gl_info->fbo_ops.glGenFramebuffers(1, &fbo); + gl_info->fbo_ops.glBindFramebuffer(GL_FRAMEBUFFER, fbo); + gl_info->fbo_ops.glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, + GL_TEXTURE_2D, texture, 0); + checkGLcall("create resources"); + + gl_info->gl_ops.gl.p_glViewport(0, 0, 8, 8); + gl_info->gl_ops.gl.p_glClearColor(0.0f, 0.0f, 1.0f, 1.0f); + gl_info->gl_ops.gl.p_glClear(GL_COLOR_BUFFER_BIT); + + draw_test_quad(ctx, edge_geometry, &red); + checkGLcall("draw"); + + gl_info->gl_ops.gl.p_glBindTexture(GL_TEXTURE_2D, texture); + gl_info->gl_ops.gl.p_glGetTexImage(GL_TEXTURE_2D, 0, + GL_BGRA, GL_UNSIGNED_INT_8_8_8_8_REV, readback); + checkGLcall("readback"); + + gl_info->gl_ops.gl.p_glDeleteTextures(1, &texture); + gl_info->fbo_ops.glDeleteFramebuffers(1, &fbo); + gl_info->fbo_ops.glBindFramebuffer(GL_FRAMEBUFFER, 0); + checkGLcall("delete resources"); + + /* We expect that exactly one fragment is generated. */ + for (y = 0; y < ARRAY_SIZE(readback); ++y) + { + for (x = 0; x < ARRAY_SIZE(readback[0]); ++x) + { + if (readback[y][x] == 0xff0000ff) + clear++; + else if (readback[y][x] == 0xffff0000) + draw++; + } + } + + if (clear != 63 || draw != 1) + { + FIXME("Unexpected filling convention test result.\n"); + return FALSE; + } + + /* One pixel was drawn, check if it is the expect one */ + return readback[3][3] == 0xffff0000; +} static float wined3d_adapter_find_polyoffset_scale(struct wined3d_caps_gl_ctx *ctx, GLenum format) { const struct wined3d_gl_info *gl_info = ctx->gl_info; @@ -5540,15 +5634,19 @@ void get_projection_matrix(const struct wined3d_context *context, const struct w * - We need to flip along the y-axis in case of offscreen rendering. * - OpenGL Z range is {-Wc,...,Wc} while D3D Z range is {0,...,Wc}. * - <= D3D9 coordinates refer to pixel centers while GL coordinates - * refer to pixel corners. - * - D3D has a top-left filling convention. We need to maintain this - * even after the y-flip mentioned above. - * In order to handle the last two points, we translate by - * (63.0 / 128.0) / VPw and (63.0 / 128.0) / VPh. This is equivalent to - * translating slightly less than half a pixel. We want the difference to - * be large enough that it doesn't get lost due to rounding inside the - * driver, but small enough to prevent it from interfering with any - * anti-aliasing. */ + * refer to pixel corners. D3D10 fixed this particular oddity. + * - D3D has a top-left filling convention while GL does not specify + * a particular behavior, other than that that the GL implementation + * needs to be consistent. + * + * In order to handle the pixel center, we translate by 0.5 / VPw and + * 0.5 / VPh. We test the filling convention during adapter init and + * add a small offset to correct it if necessary. See + * wined3d_caps_gl_ctx_test_filling_convention() for more details on how + * we test GL and considerations regarding the added nudge value. + * + * If we have GL_ARB_clip_control we take care of all this through + * viewport properties and don't have to translate geometry. */ /* Projection matrices are <= d3d9, which all have integer pixel centers. */ if (!(d3d_info->wined3d_creation_flags & WINED3D_PIXEL_CENTER_INTEGER)) @@ -5557,7 +5655,7 @@ void get_projection_matrix(const struct wined3d_context *context, const struct w clip_control = d3d_info->clip_control; flip = !clip_control && context->render_offscreen; if (!clip_control) - center_offset = 63.0f / 64.0f; + center_offset = 1.0f + d3d_info->filling_convention_nudge; else center_offset = 0.0f; diff --git a/dlls/wined3d/wined3d_private.h b/dlls/wined3d/wined3d_private.h index 8c49e694fa9..0990bffed21 100644 --- a/dlls/wined3d/wined3d_private.h +++ b/dlls/wined3d/wined3d_private.h @@ -243,6 +243,8 @@ struct wined3d_d3d_info enum wined3d_feature_level feature_level; DWORD multisample_draw_location; + + float filling_convention_nudge; }; static const struct color_fixup_desc COLOR_FIXUP_IDENTITY = @@ -3252,6 +3254,7 @@ struct wined3d_gl_info DWORD quirks; BOOL supported[WINED3D_GL_EXT_COUNT]; GLint wrap_lookup[WINED3D_TADDRESS_MIRROR_ONCE - WINED3D_TADDRESS_WRAP + 1]; + float filling_convention_nudge; HGLRC (WINAPI *p_wglCreateContextAttribsARB)(HDC dc, HGLRC share, const GLint *attribs); struct opengl_funcs gl_ops; @@ -3500,6 +3503,7 @@ BOOL wined3d_adapter_vk_init_format_info(struct wined3d_adapter_vk *adapter_vk, UINT64 adapter_adjust_memory(struct wined3d_adapter *adapter, INT64 amount) DECLSPEC_HIDDEN; BOOL wined3d_caps_gl_ctx_test_viewport_subpixel_bits(struct wined3d_caps_gl_ctx *ctx) DECLSPEC_HIDDEN; +bool wined3d_caps_gl_ctx_test_filling_convention(struct wined3d_caps_gl_ctx *ctx, float nudge) DECLSPEC_HIDDEN; void install_gl_compat_wrapper(struct wined3d_gl_info *gl_info, enum wined3d_gl_extension ext) DECLSPEC_HIDDEN; @@ -5680,10 +5684,15 @@ static inline void shader_get_position_fixup(const struct wined3d_context *conte float center_offset; unsigned int i; + /* See get_projection_matrix() in utils.c for a discussion of the position fixup. + * This function here also applies to d3d10+ which does not need adjustment for + * integer pixel centers, but it may need the filling convention nudge. */ if (context->d3d_info->wined3d_creation_flags & WINED3D_PIXEL_CENTER_INTEGER) - center_offset = 63.0f / 64.0f; + center_offset = 1.0f; else - center_offset = -1.0f / 64.0f; + center_offset = 0.0f; + + center_offset += context->d3d_info->filling_convention_nudge; for (i = 0; i < fixup_count; ++i) { -- 2.32.0

Sept. 29, 2021