June 2016 - Wine-Devel - list.winehq.org

Re: [PATCH] xaudio2: Always set current OpenAL context before making AL calls

by Andrew Eikum

On Thu, Jun 30, 2016 at 12:47:52PM -0700, Chris Robinson wrote: > On 06/30/2016 12:07 PM, Andrew Eikum wrote: > > if(!alcIsExtensionPresent(NULL, "ALC_SOFT_loopback") || > > !(palcLoopbackOpenDeviceSOFT = alcGetProcAddress(NULL, "alcLoopbackOpenDeviceSOFT")) || > >- !(palcRenderSamplesSOFT = alcGetProcAddress(NULL, "alcRenderSamplesSOFT"))){ > >+ !(palcRenderSamplesSOFT = alcGetProcAddress(NULL, "alcRenderSamplesSOFT")) || > >+ !(palcSetThreadContext = alcGetProcAddress(NULL, "alcSetThreadContext"))){ > > ERR("XAudio2 requires the ALC_SOFT_loopback extension (OpenAL-Soft >= 1.14)\n"); > > return FALSE; > > } > > alcSetThreadContext isn't part of the ALC_SOFT_loopback extension, > you should check for ALC_EXT_thread_local_context instead. > Ok, I'll fix that. That was just me being lazy and assuming that if we had loopback, we must have thread local context. > > +static void context_lock(ALCcontext *ctx) > > +{ > > + EnterCriticalSection(&al_lock); > > + alcMakeContextCurrent(ctx); > > +} > > + > > +static void context_unlock(void) > > +{ > > + LeaveCriticalSection(&al_lock); > > +} > > Since you're relying on ALC_EXT_thread_local_context anyway, it > would probably be better to use alcSetThreadContext in context_lock, > which would allow you to get rid of the global lock for normal calls > (another thread can't change the current thread context on you, and > unlike OpenGL, the same context can be current on multiple threads > at the same time). I hadn't thought of it that way, good idea. I'll change that too, should make the patch much smaller. Thanks for the review, Andrew

June 30, 2016

Re: [PATCH] xaudio2: Always set current OpenAL context before making AL calls

by Chris Robinson

On 06/30/2016 12:07 PM, Andrew Eikum wrote: > if(!alcIsExtensionPresent(NULL, "ALC_SOFT_loopback") || > !(palcLoopbackOpenDeviceSOFT = alcGetProcAddress(NULL, "alcLoopbackOpenDeviceSOFT")) || > - !(palcRenderSamplesSOFT = alcGetProcAddress(NULL, "alcRenderSamplesSOFT"))){ > + !(palcRenderSamplesSOFT = alcGetProcAddress(NULL, "alcRenderSamplesSOFT")) || > + !(palcSetThreadContext = alcGetProcAddress(NULL, "alcSetThreadContext"))){ > ERR("XAudio2 requires the ALC_SOFT_loopback extension (OpenAL-Soft >= 1.14)\n"); > return FALSE; > } alcSetThreadContext isn't part of the ALC_SOFT_loopback extension, you should check for ALC_EXT_thread_local_context instead. > +static void context_lock(ALCcontext *ctx) > +{ > + EnterCriticalSection(&al_lock); > + alcMakeContextCurrent(ctx); > +} > + > +static void context_unlock(void) > +{ > + LeaveCriticalSection(&al_lock); > +} Since you're relying on ALC_EXT_thread_local_context anyway, it would probably be better to use alcSetThreadContext in context_lock, which would allow you to get rid of the global lock for normal calls (another thread can't change the current thread context on you, and unlike OpenGL, the same context can be current on multiple threads at the same time).

June 30, 2016

Re: [PATCH] wined3d/resource: Performance improvement

by Chris Robinson

On 06/30/2016 10:20 AM, Patrick Rudolph wrote: > Tests on Windows showed that mapping a buffer with flags WINED3D_MAP_DISCARD > and WINED3D_MAP_NOOVERWRITE is valid combination. Writing reliable tests > is difficult as the driver is free to choose when using discard. > > Unigine Heavon maps a lot of buffers with both flags, but run on about > 85% GPU-load, as WINE removes both hints and the game has to wait for > the buffer to be mapped. It makes logical sense to me that mapping a buffer with both DISCARD and NOOVERWRITE would effectively ignore NOOVERWRITE. I've noticed that at least some engines will map a part of a buffer with NOOVERWRITE and draw with that part, map the next part of the buffer with NOOVERWRITE and draw with that part, etc, then after the last part of the buffer it maps with DISCARD to get new memory and keep going while the previous buffer memory is handled asynchronously by the driver or card. I notice that wined3d has some parts where it ignores/removes DISCARD, so it may not be a good idea to leave both DISCARD and NOOVERWRITE set. In such a case, the driver will get NOOVERWRITE for a part of the buffer that's still in use. It would probably be better to remove NOOVERWRITE when both DISCARD and NOOVERWRITE are set.

June 30, 2016

Re: [PATCH 6/7] d3drm: Implement IDirect3DRMViewport*::Init. (v3)

by Aaryaman Vasishta

On Tue, Jun 28, 2016 at 3:24 AM, Aaryaman Vasishta < jem456.vasishta(a)gmail.com> wrote: > > + if (viewport->d3d_viewport) > + return D3DRMERR_BADOBJECT; > + > + IDirect3DRM_AddRef(viewport->d3drm); > + > FWIW, I didn't put the intentional d3drm, camera leaks here because it seems that the device controls the leaked references via some sort of destroy callback on the device. (See the todo_wine tests in this patch). > + todo_wine ok(ref4 > frame_ref2, "Expected ref4 > frame_ref2, got > frame_ref2 = %u, ref4 = %u.\n", frame_ref2, ref4); > + > + IDirect3DRMDevice3_Release(device3); > + IDirect3DRMDevice_Release(device1); > I tried to release d3drm explicitly myself, like how I did with the texture tests. But the tests would then crash here, at IDirect3DRMDevice_Release, where the device gets destroyed. Which probably indicates that the device stores a list of leaked references and releases them on getting destroyed. For now I've decided to avoid the leak, thus avoiding the use of a destroy callback, and implementing this as a separate later on if an application demands it, unless it's safer to get this done on priority. Cheers, Aaryaman

June 30, 2016

Re: [PATCH 5/7] d3drm: Fix device not assigning width and height after creation.

by Aaryaman Vasishta

On Thu, Jun 30, 2016 at 3:41 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > > You can probably fix the immediate issue just by storing the return > value and releasing the reference if the QI failed. > > The larger issue is that code becomes much easier to follow when you > can assume that functions are supposed to leave things (more or less) > the way they found them on failure. (Unless explicitly indicated.) > I.e., when d3drm_device_set_ddraw_device_d3d() releases all the > references it created when it fails, and d3drm1_CreateDeviceFromD3D() > can assume the device is in the same state as after calling > d3drm_device_create(). > So I could e.g. release the reference, to ddraw, d3drm and d3d_device and set them all to NULL, effectively putting them in the same state as it it weas before calling d3drm_device_set_ddraw_device_d3d(). Is this fine? > > Of course the even larger issue is that > d3drm_device_set_ddraw_device_d3d() doesn't make a lot of sense. In a > way similar to e.g. d3drm3_CreateTexture(), > d3drm1_CreateDeviceFromD3D() should probably just call > IDirect3DRMDevice::InitFromD3D(). > Agreed. Back then it wasn't clear how CreateObject and Init worked together, though after the texture and viewport implementations, it became increasingly apparent that Init works only via CreateObject. A series of patches to move the implementations to Init could be done later on. Thanks for the review! Cheers, Aaryaman

June 30, 2016

Re: [PATCH 4/5] inetcomm: Support LPSTR to LPWSTR conversion in GetProp

by Nikolay Sivov

Is it possible to use PropVariantChangeType() for such conversion?

June 30, 2016

Re: [PATCH 5/7] d3drm: Fix device not assigning width and height after creation.

by Henri Verbeet

On 29 June 2016 at 17:54, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote: > On Wed, Jun 29, 2016 at 8:01 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: >> On 29 June 2016 at 16:13, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> >> wrote: >> > This is a helper function, so the caller would be handling the >> > references. >> > See CreateDeviceFromD3D in device.c. >> > >> That construction makes it harder to verify correctness, so in general >> I'd recommend against that. Regardless, d3drm_device_destroy() takes >> care of the device reference, but doesn't help for the IDirect3DRM >> reference, since that's only released when device->ddraw is set. > > device->ddraw is set while returning. What should be the better approach > here? > You can probably fix the immediate issue just by storing the return value and releasing the reference if the QI failed. The larger issue is that code becomes much easier to follow when you can assume that functions are supposed to leave things (more or less) the way they found them on failure. (Unless explicitly indicated.) I.e., when d3drm_device_set_ddraw_device_d3d() releases all the references it created when it fails, and d3drm1_CreateDeviceFromD3D() can assume the device is in the same state as after calling d3drm_device_create(). Of course the even larger issue is that d3drm_device_set_ddraw_device_d3d() doesn't make a lot of sense. In a way similar to e.g. d3drm3_CreateTexture(), d3drm1_CreateDeviceFromD3D() should probably just call IDirect3DRMDevice::InitFromD3D().

June 30, 2016

Re: [PATCH 1/5] inetcomm: Implement IMimeBody DeleteProp

by Nikolay Sivov

On 06/30/2016 11:58 AM, Alistair Leslie-Hughes wrote: > Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> > --- > dlls/inetcomm/mimeole.c | 23 +++++++++++++++++-- > dlls/inetcomm/tests/mimeole.c | 53 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 74 insertions(+), 2 deletions(-) > > diff --git a/dlls/inetcomm/mimeole.c b/dlls/inetcomm/mimeole.c > index 766503d..cff95a1 100644 > --- a/dlls/inetcomm/mimeole.c > +++ b/dlls/inetcomm/mimeole.c > @@ -898,8 +898,27 @@ static HRESULT WINAPI MimeBody_DeleteProp( > LPCSTR pszName) > { > MimeBody *This = impl_from_IMimeBody(iface); > - FIXME("(%p)->(%s) stub\n", This, debugstr_a(pszName)); > - return E_NOTIMPL; > + header_t *cursor, *cursor2; > + BOOL found = FALSE; You don't need to initialize 'found'. > + > + TRACE("(%p)->(%s) stub\n", This, debugstr_a(pszName)); 'stub' should be removed, unless it's still too stubby. > + > + LIST_FOR_EACH_ENTRY_SAFE(cursor, cursor2, &This->headers, header_t, entry) > + { > + if(ISPIDSTR(pszName)) > + found = STRTOPID(pszName) == cursor->prop->id; > + else > + found = !lstrcmpiA(pszName, cursor->prop->name); > + > + if(found) > + { > + list_remove(&cursor->entry); > + HeapFree(GetProcessHeap(), 0, cursor); > + return S_OK; > + } > + } > + > + return MIME_E_NOT_FOUND; > } The way you use it, there's no need for _SAFE() list iteration. > static HRESULT WINAPI MimeBody_CopyProps( > diff --git a/dlls/inetcomm/tests/mimeole.c b/dlls/inetcomm/tests/mimeole.c > index 8063b86..46fd1d5 100644 > --- a/dlls/inetcomm/tests/mimeole.c > +++ b/dlls/inetcomm/tests/mimeole.c > @@ -604,6 +604,58 @@ static void test_BindToObject(void) > IMimeMessage_Release(msg); > } > > +static void test_BodyDeleteProp(void) > +{ > + static const char topic[] = "wine topic"; > + HRESULT hr; > + IMimeMessage *msg; > + IMimeBody *body; > + PROPVARIANT prop; > + > + hr = MimeOleCreateMessage(NULL, &msg); > + ok(hr == S_OK, "ret %08x\n", hr); > + > + PropVariantInit(&prop); > + > + hr = IMimeMessage_BindToObject(msg, HBODY_ROOT, &IID_IMimeBody, (void**)&body); > + ok(hr == S_OK, "ret %08x\n", hr); > + > + hr = IMimeBody_DeleteProp(body, "Subject"); > + ok(hr == MIME_E_NOT_FOUND, "ret %08x\n", hr); > + > + hr = IMimeBody_DeleteProp(body, PIDTOSTR(PID_HDR_SUBJECT)); > + ok(hr == MIME_E_NOT_FOUND, "ret %08x\n", hr); > + > + prop.vt = VT_LPSTR; > + prop.u.pszVal = CoTaskMemAlloc(strlen(topic)+1); > + strcpy(prop.u.pszVal, topic); > + hr = IMimeBody_SetProp(body, "Subject", 0, &prop); > + ok(hr == S_OK, "ret %08x\n", hr); > + PropVariantClear(&prop); > + > + hr = IMimeBody_DeleteProp(body, "Subject"); > + ok(hr == S_OK, "ret %08x\n", hr); > + > + hr = IMimeBody_GetProp(body, "Subject", 0, &prop); > + ok(hr == MIME_E_NOT_FOUND, "ret %08x\n", hr); > + > + prop.vt = VT_LPSTR; > + prop.u.pszVal = CoTaskMemAlloc(strlen(topic)+1); > + strcpy(prop.u.pszVal, topic); > + hr = IMimeBody_SetProp(body, PIDTOSTR(PID_HDR_SUBJECT), 0, &prop); > + ok(hr == S_OK, "ret %08x\n", hr); > + PropVariantClear(&prop); > + > + hr = IMimeBody_DeleteProp(body, PIDTOSTR(PID_HDR_SUBJECT)); > + ok(hr == S_OK, "ret %08x\n", hr); > + > + hr = IMimeBody_GetProp(body, PIDTOSTR(PID_HDR_SUBJECT), 0, &prop); > + ok(hr == MIME_E_NOT_FOUND, "ret %08x\n", hr); > + > + IMimeBody_Release(body); > + IMimeMessage_Release(msg); > +} > + > static void test_MimeOleGetPropertySchema(void) > { > HRESULT hr; > @@ -627,6 +679,7 @@ START_TEST(mimeole) > test_MessageGetPropInfo(); > test_MessageOptions(); > test_BindToObject(); > + test_BodyDeleteProp(); > test_MimeOleGetPropertySchema(); > OleUninitialize(); > }

June 30, 2016

Re: [v2 1/3] dinput: Print location ID in debugstr_device on the Mac

by DavidL

Ah, okay ... I have a couple of other new helper functions, I should probably make sure they are static as well. Thanks! On Thursday, June 30, 2016, Dmitry Timoshkov <dmitry(a)baikal.ru> wrote: > David Lawrie <david.dljunk(a)gmail.com <javascript:;>> wrote: > > > +static Boolean get_device_property_long(IOHIDDeviceRef in_device, > CFStringRef in_key, long * out_value) > > +{ > > + Boolean result = FALSE; > > + CFTypeRef type; > > + > > + if (in_device) > > + { > > + assert(IOHIDDeviceGetTypeID() == CFGetTypeID(in_device)); > > + > > + type = IOHIDDeviceGetProperty(in_device, in_key); > > + > > + if (type) > > + { > > + if (CFNumberGetTypeID() == CFGetTypeID(type)) > > + result = CFNumberGetValue((CFNumberRef)type, > kCFNumberLongType, out_value); > > + } > > + } > > + return result; > > +} > > + > > +long get_device_location_ID(IOHIDDeviceRef device) > > +{ > > + long result = 0; > > + get_device_property_long(device, CFSTR(kIOHIDLocationIDKey), > &result); > > + return result; > > +} > > The helper should be 'static'. Also, probably you should return value from > the helper directly since you don't check for the boolean result anyway. > > Same comment applies for a similar winejoystick.drv patch. > > -- > Dmitry. >

June 30, 2016

Re: [v2 1/3] dinput: Print location ID in debugstr_device on the Mac

by Dmitry Timoshkov

David Lawrie <david.dljunk(a)gmail.com> wrote: > +static Boolean get_device_property_long(IOHIDDeviceRef in_device, CFStringRef in_key, long * out_value) > +{ > + Boolean result = FALSE; > + CFTypeRef type; > + > + if (in_device) > + { > + assert(IOHIDDeviceGetTypeID() == CFGetTypeID(in_device)); > + > + type = IOHIDDeviceGetProperty(in_device, in_key); > + > + if (type) > + { > + if (CFNumberGetTypeID() == CFGetTypeID(type)) > + result = CFNumberGetValue((CFNumberRef)type, kCFNumberLongType, out_value); > + } > + } > + return result; > +} > + > +long get_device_location_ID(IOHIDDeviceRef device) > +{ > + long result = 0; > + get_device_property_long(device, CFSTR(kIOHIDLocationIDKey), &result); > + return result; > +} The helper should be 'static'. Also, probably you should return value from the helper directly since you don't check for the boolean result anyway. Same comment applies for a similar winejoystick.drv patch. -- Dmitry.

June 30, 2016

Re: [PATCH] ddraw/tests: P8 color keying does not work on WARP (v2).

by Marvin

Hi, While running your changed tests on Windows, 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=23888 Your paranoid android. === w1064 (32 bit ddraw4) === ddraw4.c:2540: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768). === w1064 (64 bit ddraw4) === ddraw4.c:2540: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768). === w1064 (32 bit ddraw1) === ddraw1.c:2204: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768). === w1064 (64 bit ddraw1) === ddraw1.c:2204: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768). === w1064 (32 bit ddraw7) === ddraw7.c:2270: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768). === w1064 (64 bit ddraw7) === ddraw7.c:2270: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768). === w1064 (32 bit ddraw2) === ddraw2.c:2355: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768). === w1064 (64 bit ddraw2) === ddraw2.c:2355: Test failed: Expected (0,0)-(512,384), got (0,0)-(1024,768).

June 29, 2016

Re: [PATCH] d3dx9: Avoid overflowing debug channel buffer when tracing ASCII effect errors.

by Matteo Bruni

2016-06-29 0:50 GMT+02:00 Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>: > Hi Matteo, > > On 29/06/16 03:06, Matteo Bruni wrote: >> Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com> >> --- >> Inspired by a patch by Alistair Leslie-Hughes. >> >> dlls/d3dx9_36/effect.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c > trace:d3dx:d3dx9_base_effect_init "Shaders/commonVertexLight.fx(53,7): > warning X3205: conversion from larger type t" > trace:d3dx:d3dx9_base_effect_init "o smaller, possible loss of data" > trace:d3dx:d3dx9_base_effect_init "Shaders/commonVertexLight.fx(55,9): > warning X3205: conversion from larger type t" > trace:d3dx:d3dx9_base_effect_init "o smaller, possible loss of data" > trace:d3dx:d3dx9_base_effect_init "StaticMesh.fx(484,39): warning X3206: > implicit truncation of vector type" > > > Hers is the output from your patch. The first too lines are warnings are > split across trace statement. Should we use a larger number to attempt > to have each error/warning on one line. Well, that was the point. We could certainly raise the number of characters per line from 80, which I chose arbitrarily, but there can always be a message longer than the (reasonable) limit whatever the value we end up picking. In this case we would need ~120, which seems a bit too much for my taste.

June 29, 2016

Re: [v1 2/2] dinput/joystick_osx.c: sorts devices by location ID

by DavidL

Got it, will make the changes. Cheers, David On Wednesday, June 29, 2016, Ken Thomases <ken(a)codeweavers.com> wrote: > On Jun 24, 2016, at 8:33 PM, David Lawrie <david.dljunk(a)gmail.com > <javascript:;>> wrote: > > > > Fixes https://bugs.winehq.org/show_bug.cgi?id=38997 > > for dinput joysticks > > > > Note: if multiple virtual joysticks have the same location ID, they will > > still be unordered > > > > Tested on OS X 10.10.5. > > > > Signed-off-by: David Lawrie <david.dljunk(a)gmail.com <javascript:;>> > > --- > > dlls/dinput/joystick_osx.c | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > Similar comments as for the corresponding winejoystick.drv patch, of which > this is a subset. > > -Ken > >

June 29, 2016

Re: [v1 2/2] dinput/joystick_osx.c: sorts devices by location ID

by Ken Thomases

On Jun 24, 2016, at 8:33 PM, David Lawrie <david.dljunk(a)gmail.com> wrote: > > Fixes https://bugs.winehq.org/show_bug.cgi?id=38997 > for dinput joysticks > > Note: if multiple virtual joysticks have the same location ID, they will > still be unordered > > Tested on OS X 10.10.5. > > Signed-off-by: David Lawrie <david.dljunk(a)gmail.com> > --- > dlls/dinput/joystick_osx.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) Similar comments as for the corresponding winejoystick.drv patch, of which this is a subset. -Ken

June 29, 2016

Re: [v1 2/2] winejoystick.drv/joystick_osx.c: sorts devices by location ID

by Ken Thomases

On Jun 24, 2016, at 8:33 PM, David Lawrie <david.dljunk(a)gmail.com> wrote: > > Fixes https://bugs.winehq.org/show_bug.cgi?id=38997 > for winmm joysticks This should probably be a note and go below the "---" line, below. Also, please adjust the subjects of these patches. Take a look at how Alexandre changed the subjects of your previous commits. Try to save him the effort. The prefix should generally be just the DLL name without the name of the specific file (although changes to tests can have "/tests"). The first word after the colon should be capitalized. The verb should be in the imperative mood ("Sort" vs. "Sorts"). If you want to include the information that this patch is for the Mac which is lost by removing the file name, add that to the subject (e.g. "Sort devices by location ID on the Mac"). > @@ -321,6 +321,34 @@ static CFIndex find_top_level(IOHIDDeviceRef hid_device, CFMutableArrayRef main_ > /************************************************************************** > * find_osx_devices > */ You've separated the function header comment above from the function it goes with. > + > +/* > + * Helper to sort array by location ID since they are consistent across boots & launches > + */ Use the function header comment format used elsewhere in the file. > +static CFComparisonResult CFDeviceArrayComparatorFunction(const void *val1, const void *val2, void *context) As with the previous patches, don't start your function names with "CF" which is for Core Foundation functions. There's no reason to include "Function" in the name of a function. Use lowercase-and-underscores. Also, it should mention what it's comparing by or for. For example, "device_location_comparator" similar to button_usage_comparator() elsewhere in this file. > +{ > +#pragma unused(context) Don't bother with that. > + CFComparisonResult result = kCFCompareEqualTo; > + > + long loc1 = IOHIDDevice_GetLocationID((IOHIDDeviceRef)val1); > + long loc2 = IOHIDDevice_GetLocationID((IOHIDDeviceRef)val2); There's no need to cast void pointers to other pointer types. > + if (loc1 < loc2) { > + result = kCFCompareLessThan; > + } else if (loc1 > loc2) { > + result = kCFCompareGreaterThan; > + } If you're going to use braces around these blocks (which isn't necessary), put the braces on separate lines. > +/* > + * Helper to copy the CFSet to a CFArray > + */ > +static void CFSetApplierFunctionCopyToCFArray(const void *value, void *context) I know this name was copied from dinput, but it's still terrible. ;) > +{ > + CFArrayAppendValue((CFMutableArrayRef)context, value); Unneeded cast. > @@ -363,20 +391,16 @@ static int find_osx_devices(void) > if (devset) > { > CFIndex num_devices, num_main_elements; > - const void** refs; > - CFArrayRef devices; > + CFMutableArrayRef devices; > > num_devices = CFSetGetCount(devset); > - refs = HeapAlloc(GetProcessHeap(), 0, num_devices * sizeof(*refs)); > - if (!refs) > - { > - CFRelease(devset); > - goto fail; > - } > > - CFSetGetValues(devset, refs); > - devices = CFArrayCreate(NULL, refs, num_devices, &kCFTypeArrayCallBacks); > - HeapFree(GetProcessHeap(), 0, refs); > + devices = CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks); As a minor optimization, you can specify a capacity to the above call. > + /* copy devices from (unordered) set to array */ > + CFSetApplyFunction(devset, CFSetApplierFunctionCopyToCFArray, (void *)devices); Unneeded cast. > + /* now sort the array by location ID's */ Again, these comments don't add much and better names for the functions would make the code more self-explanatory. It might be worthwhile to put a comment here or in the function header about _why_ you're sorting by location ID (i.e. to provide a stable order across runs). That's a good general rule: comments shouldn't narrate what the code is doing, they should explain why it's doing it, or why it's doing it in a particular way, if that's non-obvious. > + CFArraySortValues(devices, CFRangeMake(0, num_devices), CFDeviceArrayComparatorFunction, NULL); > + -Ken

June 29, 2016

Re: [v1 1/2] winejoystick.drv/joystick_osx.c: debugstr_device prints location ID

by Ken Thomases

On Jun 24, 2016, at 8:33 PM, David Lawrie <david.dljunk(a)gmail.com> wrote: > > Tested on OS X 10.10.5. > > Signed-off-by: David Lawrie <david.dljunk(a)gmail.com> > --- > dlls/winejoystick.drv/joystick_osx.c | 32 ++++++++++++++++++++++++++++++-- > 1 file changed, 30 insertions(+), 2 deletions(-) Same issues as the corresponding dinput patch. -Ken

June 29, 2016

Re: [v1 1/2] dinput/joystick_osx.c: debugstr_device prints location ID

by Ken Thomases

On Jun 24, 2016, at 8:33 PM, David Lawrie <david.dljunk(a)gmail.com> wrote: > > +static Boolean IOHIDDevice_GetLongProperty(IOHIDDeviceRef inIOHIDDeviceRef, CFStringRef inKey, long * outValue) Don't name your functions as though they were Apple's. Apple "owns" the namespace of identifiers starting with "IOHIDDevice". In general, just use simple lowercase-and-underscores names unless there's a strong reason not to. Something like "get_device_property_long". Same goes for the parameters: "key" and "out_value" or the like. > +{ > + Boolean result = FALSE; > + CFTypeRef tCFTypeRef; Don't use this naming convention either (is that Hungarian notation?). Why is the type repeated in the variable name? What does the "t" prefix signify. Just call this "value" or something. > + > + if (inIOHIDDeviceRef) { > + assert(IOHIDDeviceGetTypeID() == CFGetTypeID(inIOHIDDeviceRef)); > + > + tCFTypeRef = IOHIDDeviceGetProperty(inIOHIDDeviceRef, inKey); > + > + if ( tCFTypeRef ) { > + /* if this is a number */ > + if (CFNumberGetTypeID() == CFGetTypeID(tCFTypeRef)) { > + /* get it's value */ What do these comments add? > + result = CFNumberGetValue((CFNumberRef)tCFTypeRef, kCFNumberSInt32Type, outValue); "long" and "SInt32" don't match reliably. Why not kCFNumberLongType? > +long IOHIDDevice_GetLocationID(IOHIDDeviceRef inIOHIDDeviceRef) Naming conventions here, too. > +{ > + long result = 0; > + (void)IOHIDDevice_GetLongProperty(inIOHIDDeviceRef, CFSTR(kIOHIDLocationIDKey), &result); I don't think we generally use the cast-to-void thing for unused return values in Wine. -Ken

June 29, 2016

Re: [PATCH 5/7] d3drm: Fix device not assigning width and height after creation.

by Aaryaman Vasishta

On Wed, Jun 29, 2016 at 8:01 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 29 June 2016 at 16:13, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> > wrote: > > This is a helper function, so the caller would be handling the > references. > > See CreateDeviceFromD3D in device.c. > > > That construction makes it harder to verify correctness, so in general > I'd recommend against that. Regardless, d3drm_device_destroy() takes > care of the device reference, but doesn't help for the IDirect3DRM > reference, since that's only released when device->ddraw is set. > device->ddraw is set while returning. What should be the better approach here? Cheers, Aaryaman

June 29, 2016

Re: [PATCH 5/7] d3drm: Fix device not assigning width and height after creation.

by Henri Verbeet

On 29 June 2016 at 16:13, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote: > This is a helper function, so the caller would be handling the references. > See CreateDeviceFromD3D in device.c. > That construction makes it harder to verify correctness, so in general I'd recommend against that. Regardless, d3drm_device_destroy() takes care of the device reference, but doesn't help for the IDirect3DRM reference, since that's only released when device->ddraw is set.

June 29, 2016

Re: [PATCH 5/7] d3drm: Fix device not assigning width and height after creation.

by Aaryaman Vasishta

On Wed, Jun 29, 2016 at 7:21 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 24 June 2016 at 23:49, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> > wrote: > > HRESULT d3drm_device_set_ddraw_device_d3d(struct d3drm_device *device, > IDirect3DRM *d3drm, IDirect3D *d3d, IDirect3DDevice *d3d_device) > > { > > + IDirectDrawSurface *surface; > > + IDirect3DDevice2 *d3d_device2; > > + DDSURFACEDESC desc; > > + HRESULT hr; > > + > > device->d3drm = d3drm; > > IDirect3DRM_AddRef(d3drm); > > device->device = d3d_device; > > IDirect3DDevice_AddRef(d3d_device); > > > > + /* Fetch render target and get width/height from there */ > > + if (FAILED(hr = IDirect3DDevice_QueryInterface(d3d_device, > &IID_IDirectDrawSurface, (void **)&surface))) > > + { > > + if (FAILED(hr = IDirect3DDevice_QueryInterface(d3d_device, > &IID_IDirect3DDevice2, (void **)&d3d_device2))) > > + return hr; > > + hr = IDirect3DDevice2_GetRenderTarget(d3d_device2, &surface); > > + IDirect3DDevice2_Release(d3d_device2); > > + if (FAILED(hr)) > > + return hr; > > + } > > + > > + desc.dwSize = sizeof(desc); > > + if (FAILED(hr = IDirectDrawSurface_GetSurfaceDesc(surface, &desc))) > > + { > > + IDirectDrawSurface_Release(surface); > > + return hr; > > + } > > + > > + device->width = desc.dwWidth; > > + device->height = desc.dwHeight; > > + IDirectDrawSurface_Release(surface); > > + > > return IDirect3D_QueryInterface(d3d, &IID_IDirectDraw, (void > **)&device->ddraw); > > } > > > This leaks some references (IDirect3DRM, IDirect3DDevice) on error > paths, are those on purpose? > > This is a helper function, so the caller would be handling the references. See CreateDeviceFromD3D in device.c. Cheers, Aaryaman

June 29, 2016

Re: [PATCH 5/7] d3drm: Fix device not assigning width and height after creation.

by Henri Verbeet

On 24 June 2016 at 23:49, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote: > HRESULT d3drm_device_set_ddraw_device_d3d(struct d3drm_device *device, IDirect3DRM *d3drm, IDirect3D *d3d, IDirect3DDevice *d3d_device) > { > + IDirectDrawSurface *surface; > + IDirect3DDevice2 *d3d_device2; > + DDSURFACEDESC desc; > + HRESULT hr; > + > device->d3drm = d3drm; > IDirect3DRM_AddRef(d3drm); > device->device = d3d_device; > IDirect3DDevice_AddRef(d3d_device); > > + /* Fetch render target and get width/height from there */ > + if (FAILED(hr = IDirect3DDevice_QueryInterface(d3d_device, &IID_IDirectDrawSurface, (void **)&surface))) > + { > + if (FAILED(hr = IDirect3DDevice_QueryInterface(d3d_device, &IID_IDirect3DDevice2, (void **)&d3d_device2))) > + return hr; > + hr = IDirect3DDevice2_GetRenderTarget(d3d_device2, &surface); > + IDirect3DDevice2_Release(d3d_device2); > + if (FAILED(hr)) > + return hr; > + } > + > + desc.dwSize = sizeof(desc); > + if (FAILED(hr = IDirectDrawSurface_GetSurfaceDesc(surface, &desc))) > + { > + IDirectDrawSurface_Release(surface); > + return hr; > + } > + > + device->width = desc.dwWidth; > + device->height = desc.dwHeight; > + IDirectDrawSurface_Release(surface); > + > return IDirect3D_QueryInterface(d3d, &IID_IDirectDraw, (void **)&device->ddraw); > } > This leaks some references (IDirect3DRM, IDirect3DDevice) on error paths, are those on purpose?

June 29, 2016

Re: shell32/tests: Avoid casts from a COM object to an iface.

by Michael Stefaniuc

On 06/29/2016 10:02 AM, Marvin wrote: > Hi, > > While running your changed tests on Windows, 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=23879 > > Your paranoid android. > > > === w1064 (32 bit shlfolder) === > shlfolder.c:1591: Test failed: IShellFolder_GetDisplayNameOf(NULL) failed! hr = 80070057 > > === w1064 (64 bit shlfolder) === > shlfolder.c:1591: Test failed: IShellFolder_GetDisplayNameOf(NULL) failed! hr = 80070057 This is a pre-existing failure. bye michael

June 29, 2016

Re: shell32/tests: Avoid casts from a COM object to an iface.

by Marvin

Hi, While running your changed tests on Windows, 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=23879 Your paranoid android. === w1064 (32 bit shlfolder) === shlfolder.c:1591: Test failed: IShellFolder_GetDisplayNameOf(NULL) failed! hr = 80070057 === w1064 (64 bit shlfolder) === shlfolder.c:1591: Test failed: IShellFolder_GetDisplayNameOf(NULL) failed! hr = 80070057

June 29, 2016

Re: [PATCH] d3dx9: Avoid overflowing debug channel buffer when tracing ASCII effect errors.

by Alistair Leslie-Hughes

Hi Matteo, On 29/06/16 03:06, Matteo Bruni wrote: > Signed-off-by: Matteo Bruni <mbruni(a)codeweavers.com> > --- > Inspired by a patch by Alistair Leslie-Hughes. > > dlls/d3dx9_36/effect.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c trace:d3dx:d3dx9_base_effect_init "Shaders/commonVertexLight.fx(53,7): warning X3205: conversion from larger type t" trace:d3dx:d3dx9_base_effect_init "o smaller, possible loss of data" trace:d3dx:d3dx9_base_effect_init "Shaders/commonVertexLight.fx(55,9): warning X3205: conversion from larger type t" trace:d3dx:d3dx9_base_effect_init "o smaller, possible loss of data" trace:d3dx:d3dx9_base_effect_init "StaticMesh.fx(484,39): warning X3206: implicit truncation of vector type" Hers is the output from your patch. The first too lines are warnings are split across trace statement. Should we use a larger number to attempt to have each error/warning on one line. Best Regards Alistair Leslie-Hughes

June 28, 2016

Re: [PATCH] winex11: fix destroying of top-level windows

by Stefan Dösinger

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 Am 2016-06-28 um 11:43 schrieb Henri Verbeet: > On 25 June 2016 at 01:26, Miklós Máté <mtmkls(a)gmail.com> wrote: >> I couldn't reproduce that test failure. I cloned Stefan's >> repository (master branch), applied my glx 1.3 patches that are >> in 1.9.12, and v2 of this patch, and I set the registry keys for >> CSMT (csmt=enabled, strictdrawordering=disabled). The d3d9 tests >> all pass. Did I miss something? Is there a way to tell if CSMT is >> in use? >> > Mostly by messages like "fixme:d3d:context_acquire Acquiring a GL > context from outside the CS thread." > > The d3d9 tests do pass here, but e.g. ddraw7 from the ddraw tests > fails with a GLXBadWindow from X_GLXDestroyWindow. > WINEDEBUG=+synchronous makes it work. There's also a "failed to > create drawable" message that I didn't investigate. FYI, the CSMT patches currently have a race condition when setting up / tearing down devices. I had a hacky workaround for that previously that fixed the random GLXBadWindow failures. I haven't included this patch yet in my "new" rebase because it was somewhat convoluted. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJXcrNhAAoJEN0/YqbEcdMwsXEQAIZ8+4dahYho6RxBa5DsQiMj sqTTXZdEK9strUkVcFjqi0ercm8yU6ubhLMHFgumh+caAGGLiC8TaYQNVQvN6s40 KN9qJ1Vi7Ae11GqWwFVV/MIL6YwYmXnWsgaFlBB8itn50Cv6CT9VyYsJRmw97b7Y 0TRpOhTsaYqsGGWNK1dbfI5LWPKY4cAUCPOMcDY8xpEJUZhcNzg0kHilJn1gQQfh E5klKtq9HQ3qd/xljHvTxdwtTqh/i2QmrDdxONg9546er4QoHHZK4hJ+xu/VOjv0 PVvegnRu8eZKU3V1N/JxN9t3cxL6XXtNmhCik8tfExZGPb6PZCHETqx2xQO9y+hA qV50C6EuQN89AqNwITKaE4NydQ/sDBGg96ld2VKmPsSemvWYgIzswwWeFRVu8zXy HYMMsxNDKjC4nwvLwYuWVuzXQ3vTuYgBk2DxT78huWljDCnuxPf4KpzcQjMNUr4c 3UoO3gZsVH2uGFElD8PTSjzUUR9C4HN94MiwNZ63pqCUSqt2PN68hjZesfdh3bTD 2ljznN/P51PimHKUsaCFdEUdGQnk2vpVovTGt1OQGzIKn51FP9mr4Sf5ltoOPmY0 IkAFE45pcxlZccKjN/Mi7tmSZsizCJ0lUX+vYLm4eC3J7PoRAD6b7+eyDQQr2yG8 AIUTCs57/kr5fbM28x9A =9yQa -----END PGP SIGNATURE-----

June 28, 2016