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

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