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