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

Re: [PATCH 6/7] d3drm: Implement IDirect3DRMViewport*::Init. (v3)
by Aaryaman Vasishta 30 Jun '16

30 Jun '16

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

1 0

Re: [PATCH 5/7] d3drm: Fix device not assigning width and height after creation.
by Henri Verbeet 30 Jun '16

30 Jun '16

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?

2 5

Re: [PATCH 4/5] inetcomm: Support LPSTR to LPWSTR conversion in GetProp
by Nikolay Sivov 30 Jun '16

30 Jun '16

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

1 0

Re: [PATCH 1/5] inetcomm: Implement IMimeBody DeleteProp
by Nikolay Sivov 30 Jun '16

30 Jun '16

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(); > }

1 0

Re: [v2 1/3] dinput: Print location ID in debugstr_device on the Mac
by Dmitry Timoshkov 30 Jun '16

30 Jun '16

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.

2 1

Re: [PATCH] ddraw/tests: P8 color keying does not work on WARP (v2).
by Marvin 30 Jun '16

30 Jun '16

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).

1 0

Re: [PATCH] d3dx9: Avoid overflowing debug channel buffer when tracing ASCII effect errors.
by Alistair Leslie-Hughes 30 Jun '16

30 Jun '16

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

2 1

Re: [v1 2/2] dinput/joystick_osx.c: sorts devices by location ID
by Ken Thomases 30 Jun '16

30 Jun '16

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

2 1

Re: [v1 2/2] winejoystick.drv/joystick_osx.c: sorts devices by location ID
by Ken Thomases 29 Jun '16

29 Jun '16

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

1 0

Re: [v1 1/2] winejoystick.drv/joystick_osx.c: debugstr_device prints location ID
by Ken Thomases 29 Jun '16

29 Jun '16

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

1 0