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

Re: wine-1.8.4-rc1

by André Hentschel

Am 30.07.2016 um 22:49 schrieb Michael Stefaniuc: > Hello guys, > > I have pushed wine-1.8.4-rc1 (with tag) to > https://github.com/mstefani/wine-stable > https://static-winehq.rhcloud.com/stable/ It builds fine in buildroot, but then thinks it's a 1.8.3 build (./wine --version told me so), despite "Wine version 1.8.4" in the VERSION file

July 31, 2016

Re: [PATCH] Fix some its vs it's typos

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=24626 Your paranoid android. === w864 (32 bit d3d) === d3d.c:2326: Test failed: Unexpected destination texture level pixels; 2031 differences at 0 level d3d.c:2326: Test failed: Unexpected destination texture level pixels; 518 differences at 1 level d3d.c:2326: Test failed: Unexpected destination texture level pixels; 138 differences at 2 level d3d.c:2326: Test failed: Unexpected destination texture level pixels; 46 differences at 3 level d3d.c:2510: Test failed: Unexpected destination texture level pixels; 257 differences d3d.c:2657: Test failed: Unexpected destination texture level pixels; 256 differences at 2 level d3d.c:2657: Test failed: Unexpected destination texture level pixels; 64 differences at 3 level d3d.c:2657: Test failed: Unexpected destination texture level pixels; 16 differences at 4 level d3d.c:2657: Test failed: Unexpected destination texture level pixels; 4 differences at 5 level === w1064 (32 bit d3d) === d3d.c:2326: Test failed: Unexpected destination texture level pixels; 2031 differences at 0 level d3d.c:2326: Test failed: Unexpected destination texture level pixels; 518 differences at 1 level d3d.c:2326: Test failed: Unexpected destination texture level pixels; 138 differences at 2 level d3d.c:2326: Test failed: Unexpected destination texture level pixels; 46 differences at 3 level d3d.c:2510: Test failed: Unexpected destination texture level pixels; 257 differences d3d.c:2657: Test failed: Unexpected destination texture level pixels; 256 differences at 2 level d3d.c:2657: Test failed: Unexpected destination texture level pixels; 64 differences at 3 level d3d.c:2657: Test failed: Unexpected destination texture level pixels; 16 differences at 4 level d3d.c:2657: Test failed: Unexpected destination texture level pixels; 4 differences at 5 level

July 31, 2016

wine-1.8.4-rc1

by Michael Stefaniuc

Hello guys, I have pushed wine-1.8.4-rc1 (with tag) to https://github.com/mstefani/wine-stable https://static-winehq.rhcloud.com/stable/ Getting nearer to the goal of "around 50" commits for a stable release as this one contains only 66 commits. Nothing fancy though: - Stubs - More GPUs added - One wined3d fix backported (thanks again to Henri) - One original patch aka not cherry-picked, see below. Should somebody look at the release commit he'll notice that 7 of the bugs were already included in wine-1.8.3. That was a mistake as I had deferred those for a later release. So I've modified tools/make_announce to avoid that mistake in the future. For the release I would follow the same process: one week for feedback followed by another rc if needed with the final release 3-4 days later. Lets see how this works out as I'm on vacation travelling and Alexandre will travel too. And last but not least an excerpt from the ANNOUNCE file: What's new in this release (see below for details): - Various bug fixes. - A few more cards added to the GPU description table. ---------------------------------------------------------------- Bugs fixed in 1.8.4 (total 49): 18745 64-bit Mortal Kombat X crashes on unimplemented function KERNEL32.dll.GetLargePageMinimum 20977 Bad hlp table rendering 22819 Some applications like Heroes of Might and Magic 2 display loopback. 25396 Safrosoft RoX freezes at start. 26268 Multiple applications need ntdll.ZwSetLdtEntries stub (kwiksupport.me, Ragnarok Online patcher) 26965 Dragon Age 2 demo - sound in cutscenes is muffled 27680 Multiple 64-bit applications crash on startup (violation of the Windows 64-bit ABI, stack pointer (RSP) must be 16-byte aligned when making a call to Win64 API) 28082 OCCT Perestroïka needs hal.dll.READ_PORT_ULONG 34052 The Witcher: Character models are sometimes invisible 36556 valgrind shows a definite leak in comctl32/tests/syslink.c 36838 TOCA Touring Car Championship: installer copies one file from CD and then shows an error 37063 NTLM EncryptMessage/DecryptMessage failure 37090 Multiple applications crash with built-in amstream (Shogun: Total War, Medieval: Total War) 37271 Applications get IP address of 127.12.34.56 when lo0 is up on OSX 38102 Process Hacker 2.x needs winsta.dll.WinStationEnumerateW 38137 WBaduk: "Could not obtain OLE control window handle" regression 38811 Java JRE 8 installer 'jre-8uXX-windows-i586.exe' crashes on startup (GetThreadPreferredUILanguages is a stub) 38962 expr.c:234 and 235 attempted computation of 1 << 31 should be 1u << 31 39543 Multiple games and applications crash on unimplemented function KERNEL32.dll.FreeUserPhysicalPages (Microsoft Office 2007 diagnosis service, Eden Eternal) 39579 iNode 3.6 client needs ndis.sys.NdisAllocateMemoryWithTag 39842 Skype for windows doesn't work (needs ndis.sys.NdisSystemProcessorCount) 39911 dxd9 PresentationInterval not propagated on device Reset 39933 iNode 3.6 client needs ndis.sys.NdisAllocateSpinLock 40048 Camtasia Studio 8 : crashes on unimplemented function dwmapi.dll.DwmSetPresentParameters 40301 wxDatePickerCtrl (wxWidgets) generates an assert on DTM_GETRANGE message result 40380 Wine should take dosbox's configuration instead of creating a new one 40389 cmd.exe (native): Unimplemented function KERNEL32.dll.GetConsoleAliasExesLengthW 40487 3dmark Vantage Benchmark: unimplemented functions hal.dll.READ and WRITE PORT 40497 "Trainz: A New Era" needs msvcr120.dll.?__ExceptionPtrToBool@@YA_NPEBX(a)Z 40511 Application that copies files from one place to another stoped working in wine versions greater then 1.6.2 40532 Microsoft Word Viewer 11.8 does not show text on file properties dialog until the text is selected 40557 KSP Trajectory Optimization Tool (MATLAB Runtime (MCR)) crashes at launch 40562 AkelPad: [ToolBar plugin] tooltips for toolbar buttons have some extra text 40581 Regression: dsound input recording from microphone lags with 2 seconds delay 40614 Cisco Jabber 11.6 crashes due to missing dbghelp.dll.EnumerateLoadedModulesExW 40618 Autodesk 123D Design needs msvcr100.dll.mbsrtowcs_s 40659 Window with CS_OWNDC style cause DC leak when GetDC/ReleaseDC is used. 40691 DVDFab 9.2.4.4 pre-check FabCheck.exe executable fails, needs setupapi.dll.CM_Get_DevNode_Status 40745 Python 3.5.1 interpreter crashes, needs _set_thread_local_invalid_parameter_handler 40747 Microsoft Expression Web 4 crashes on startup, needs gdiplus.dll.GdipGetImageAttributesAdjustedPalette 40754 GOG Galaxy crashes with unimplemented function api-ms-win-crt-locale-l1-1-0.dll._lock_locales 40755 GOG Galaxy crashes with unimplemented function ucrtbase.dll._lock_locales 40756 GOG Galaxy crashes with unimplemented function api-ms-win-crt-heap-l1-1-0.dll._calloc_base 40770 Nvidia GeForce GT 730 not recognized properly 40796 Python 3.5.1 interpreter crashes when importing pyqtSlot 40830 Conflict between kVK_RightCommand and macOS 10.12 Sierra in carbon framework 40833 Batman: Arkham Origins Blackgate no sound during gameplay (except cutscenes) 40851 macOS 10.12 Build Failure in winspool.drv with cupsGetPPD 40923 Alone in the Dark (2008) crashes on launch with msvcrt assertion ---------------------------------------------------------------- Changes since 1.8.3: Alex Henrie (6): msvcrt: Stub out _(get|set)_thread_local_invalid_parameter_handler. ucrtbase: Implement _(un)lock_locales. ucrtbase: Implement _calloc_base. ucrtbase: Implement _malloc_base. ucrtbase: Implement _free_base. riched20: Don't call UpdateWindow after setting the background color. Alexandre Julliard (1): user32: Completely free owned DCEs when the corresponding window/class is destroyed. Alistair Leslie-Hughes (2): wininet: Free socket when connection is freed. wined3d: Correct memory size for Geforce 9600 GT. Andrew Eikum (3): dsound: Support downmixing 5.1 to stereo. dsound: Capture all available data on each period callback. xaudio2: Always set current OpenAL context before making AL calls. André Hentschel (1): ndis.sys: Implement NdisSystemProcessorCount. Austin English (12): hal: Add READ_PORT_ULONG stub. hal: Add READ_PORT_UCHAR stub. hal: Add WRITE_PORT_ULONG stub. kernel32: Add FreeUserPhysicalPages stub. winsta: Add a stub for WinStationEnumerateW. dwmapi: Add DwmSetPresentParameters stub. kernel32: Add GetConsoleAliasExesLengthA/W stubs. ntdll: Add NtSetLdtEntries/ZwSetLdtEntries stub. dbghelp: Add forwards for EnumerateLoadedModulesEx/EnumerateLoadedModulesExW. kernel32: Implement GetLargePageMinimum. ndis.sys: Add NdisAllocateMemoryWithTag stub. ndis.sys: Add NdisAllocateSpinLock stub. Benjamin Hodgetts (1): wined3d: Add Intel Iris chips to identification list. Bruno Jesus (1): ws2_32: Ensure default route IP addresses are returned first in gethostbyname. Béla Gyebrószki (2): wined3d: Add Nvidia GeForce GTS 250. wined3d: Add Nvidia GeForce GT 730 (GK208). David Woodhouse (1): secur32: Fix handling of ANSI NTLM credentials. Fabian Maurer (1): msacm32: Added converter for ADPCM to PCM 8 bit mono. Grazvydas Ignotas (3): wined3d: Use Intel data returned by Windows driver. wined3d: Add some more Intel GPUs. wined3d: Add a fallback for Skylake. Hans Leidekker (1): kernel32: Improve the stub for GetThreadPreferredUILanguages. Henri Verbeet (1): wined3d: Also update the swap interval when not recreating the context in wined3d_device_reset(). Jarkko Korpi (1): widl: Fix overflow when left-shifting. Jean-Christophe Cardot (6): winhlp32: Replace magic numbers by symbols. winhlp32: Add some trace calls. winhlp32: Add a warning trace. winhlp32: Prepare winhlp32 for more font types. winhlp32: Rework and rename HLPFILE_HalfPointsToTwips. winhlp32: Fix table formatting. Justas Lavišius (1): include: Force stack alignment on x86_64. Ken Thomases (2): winspool.drv: Include <cups/ppd.h> to fix building against the macOS 10.12 SDK. winemac: Don't define kVK_RightCommand when building against the macOS 10.12 SDK. Luis Sahagun (1): winevdm: Tell dosbox to read the current user's config. Matteo Bruni (1): wined3d: Add a setting to check relative addressing indices when accessing uniforms. Michael Müller (1): amstream: Implement IDirectDrawMediaStream::GetMultiMediaStream. Michael Stefaniuc (1): tools: Get the bug list for ANNOUNCE from the stable-notes git notes. Nikolay Sivov (2): comctl32/datetime: Use MCM_GETRANGE result directly in DTM_GETRANGE. comctl32/tooltips: Cut tooltip text at first tab char unless TTS_NOPREFIX is used. Olivier F. R. Dierick (1): shell32: Set last error explicitly in SHFileOperation(). Piotr Caban (6): msvcrt: Add mbsrtowcs_s implementation. msvcp110: Fix num_put<char> structure layout. msvcrt: Fix mbtowc behavior for buffers containing more than one character. ieframe: Restore closed IOleClientSite in DoVerb. msvcr120: Add __ExceptionPtrToBool implementation. msvcrt: Return error when previously parsed type is accessed while demangling simple data type. Rob Walker (1): setupapi: Add stub for setupapi.dll.CM_Get_DevNode_Status. Sebastian Lackner (3): comctl32/syslink: Fix a blocks leak (Valgrind). amstream/tests: Add tests for GetMultiMediaStream methods. server: Fix assignment of primary_group in token_duplicate. Stefan Leichter (3): kernel32: Add stub for GetSystemPreferredUILanguages. kernel32/tests: Add tests for GetSystemPreferredUILanguages. kernel32: Return a dummy value in GetSystemPreferredUILanguages. Vincent Povirk (1): gdiplus: Implement GdipGetImageAttributesAdjustedPalette. bye michael -- Michael Stefaniuc mstefani(a)winehq.org

July 30, 2016

Re: comctl32/tests: Avoid using memset() on RECTs.

by Nikolay Sivov

On 30.07.2016 23:18, Michael Stefaniuc wrote: > On 07/30/2016 09:58 PM, Nikolay Sivov wrote: >> On 30.07.2016 22:55, Michael Stefaniuc wrote: >>> - memset(&r1, 0, sizeof r1); >>> - r1.left = LVIR_ICON; >>> + SetRect(&r1, LVIR_ICON, 0, 0, 0); >> >> I don't like this, I think it's better to use SetRect() when actual >> coordinates are used, and LVIR_* has special meaning. > Uhm? I have added 36 of those with commit 87bc2aa6 and you signed of on > that. > Ok, no reason to stop now then. > For me those are still RECTs even though MS uses them for other stuff. > > bye > michael > >

July 30, 2016

Re: comctl32/tests: Avoid using memset() on RECTs.

by Michael Stefaniuc

On 07/30/2016 09:58 PM, Nikolay Sivov wrote: > On 30.07.2016 22:55, Michael Stefaniuc wrote: >> - memset(&r1, 0, sizeof r1); >> - r1.left = LVIR_ICON; >> + SetRect(&r1, LVIR_ICON, 0, 0, 0); > > I don't like this, I think it's better to use SetRect() when actual > coordinates are used, and LVIR_* has special meaning. Uhm? I have added 36 of those with commit 87bc2aa6 and you signed of on that. For me those are still RECTs even though MS uses them for other stuff. bye michael

July 30, 2016

Re: comctl32/tests: Avoid using memset() on RECTs.

by Nikolay Sivov

On 30.07.2016 22:55, Michael Stefaniuc wrote: > - memset(&r1, 0, sizeof r1); > - r1.left = LVIR_ICON; > + SetRect(&r1, LVIR_ICON, 0, 0, 0); I don't like this, I think it's better to use SetRect() when actual coordinates are used, and LVIR_* has special meaning.

July 30, 2016

Re: Enabling/Disabling Joysticks from registry for the Mac

by Ken Thomases

Hi, On Jul 30, 2016, at 2:15 AM, DavidL <david.dljunk(a)gmail.com> wrote: > > I was looking into how the Linux-dinput code and joy.cpl work and comparing that to the macOS-dinput code. I have some questions about how best to proceed: > > joy.cpl uses joy->instance.tszInstanceName to name the joysticks in the control panel. In Linux-dinput code, they pass the joystick_devices[id].name to both tszInstanceName and tszProductName In the macOS-dinput code, they pass the name "Joystick #" where # is the index in the joystick array to the instance name and the joystick's actual name to the product name. When looking at the Microsoft guidelines for dinput, it does indeed say that the instance name *can* be joystick + position in the array, but doesn't specify that it *has* to be (https://msdn.microsoft.com/en-us/library/windows/desktop/microsoft.directx_…) Also, I've confirmed that in the Windows 10 joystick control panel the joysticks are listed by their name, not by joystick number (and in my memory of earlier Windows OSes it's the same). > > So, should I change the macOS-dinput code to set both tszInstanceName and tszProductName to the product name (as in Linux) or should I change joy.cpl to use tszProductName instead of tszInstanceName? This will also make it easier in the registry to know which joystick is being enabled/disabled. We need to distinguish between an object's internal identifier and what's displayed in UI. It seems to me that joy.cpl and dinput ought to use the guidInstance field to determine which devices are enabled or disabled. That would require that the backends actually provide a guidInstance that's unique among devices but stable across runs and attachment/detachment of devices. Unfortunately, I'm not sure how practical that is. Some devices seem to provide kIOHIDSerialNumberKey, but not all do. kIOHIDLocationIDKey is stable so long as the USB topology doesn't change, but it can change, of course. An experiment shows that the location ID changes if you just plug the device into a different point in the topology (a USB port on the computer vs. one on the keyboard, which is a hub). Also, if you change how this works, everybody's existing registry settings will break. So, perhaps a decent compromise would be for joy.cpl to display both the instance and the product name (e.g. "Joystick 1 - Logitech Extreme 3D Pro") but record the instance name in the registry. On Linux, I guess that may make the name looked doubled, if the instance and product names are the same. On the Mac, we should probably change the instance name to something that's really associated with the device. The index in the device array is not stable. If you disconnect the first joystick, all other joysticks will get renumbered in that scheme. Probably, the location ID is the closest we have to a stable ID (which is why you've used it for sorting). It's just too bad that it's an ugly number, whether formatted decimal or hexadecimal. Another approach would be for the Mac code to basically build a dynamic database of every game controller it ever encounters, storing all of the available stable identifying information about each, and assigning a permanent identifier. So, location ID would not be used because it's not truly stable, but vendor ID, product ID, manufacturer, product name, serial number, version number, usage pairs, and maybe even the report descriptor could be. As it enumerates devices, it would look through the database to find one that matches all of the these available pieces of metadata. If it does, it will reuse the previously-generated identifier. Otherwise, it will generate a new identifier and add the device to the database. There would have to be a check, in case there are two exactly identical devices attached, to not use the same identifier for both. In practice, the database should never grow very large since one system should never have very many devices connected to it over its history. > Further, while joy.cpl correctly is a dinput control panel, enabling/disabling joystick should be for both dinput and winmm games - e.g. X-wing vs Tie Fighter even has the joystick control panel in the game's loader window*, but, once in the game, joystick control is handled by winmm. I believe the joystick registry entry is under dinput, but the winmm wine joystick driver should read that as well. > > Is it acceptable for wine joystick driver to read the dinput registry? Or should two registry entries be created? or should a single general joystick registry entry not under dinput or winmm control both? Well, the registry key is actually Wine-specific. It's got "DirectInput" in the path, but we can play a little fast and lose with it. No Windows app is sensitive to what's under that key, for example. I see no problem with having winmm check it. There's work underway to implement proper HID support and then reimplement lots of different APIs (dinput, winmm joystick, xinput, raw input) on top of that. When that happens, a lot of this will probably have to change and redundancy will hopefully be eliminated. (If it's going to break people's settings, it may be a good time to switch to using guidInstance or the equivalent, too.) -Ken

July 30, 2016

Enabling/Disabling Joysticks from registry for the Mac

by DavidL

Hi Ken, I was looking into how the Linux-dinput code and joy.cpl work and comparing that to the macOS-dinput code. I have some questions about how best to proceed: joy.cpl uses joy->instance.tszInstanceName to name the joysticks in the control panel. In Linux-dinput code, they pass the joystick_devices[id].name to both tszInstanceName and tszProductName In the macOS-dinput code, they pass the name "Joystick #" where # is the index in the joystick array to the instance name and the joystick's actual name to the product name. When looking at the Microsoft guidelines for dinput, it does indeed say that the instance name *can* be joystick + position in the array, but doesn't specify that it *has* to be ( https://msdn.microsoft.com/en-us/library/windows/desktop/microsoft.directx_…) Also, I've confirmed that in the Windows 10 joystick control panel the joysticks are listed by their name, not by joystick number (and in my memory of earlier Windows OSes it's the same). So, should I change the macOS-dinput code to set both tszInstanceName and tszProductName to the product name (as in Linux) or should I change joy.cpl to use tszProductName instead of tszInstanceName? This will also make it easier in the registry to know which joystick is being enabled/disabled. Further, while joy.cpl correctly is a dinput control panel, enabling/disabling joystick should be for both dinput and winmm games - e.g. X-wing vs Tie Fighter even has the joystick control panel in the game's loader window*, but, once in the game, joystick control is handled by winmm. I believe the joystick registry entry is under dinput, but the winmm wine joystick driver should read that as well. Is it acceptable for wine joystick driver to read the dinput registry? Or should two registry entries be created? or should a single general joystick registry entry not under dinput or winmm control both? I may have more questions, but we'll start off with these. :) Cheers, David *Interestingly, the game's sequel X-wing: Alliance says I need to install DirectX to access joystick options in the loader window. I'm not sure why the older title brings up the joystick control panel just fine, but the later one does not.

July 30, 2016

Re: [PATCH 1/4] mpr: Implement local name automatic redirection for disks.

by Pierre Schweitzer

Is there anything left to fix? Le 28/07/2016 à 10:44, Pierre Schweitzer a écrit : > I also modified 2nd patch of the patch, because it was suffering the > same issue and wasn't applying any longer. > Both have been resent. > > Le 28/07/2016 à 10:12, Alexandre Julliard a écrit : >> Pierre Schweitzer <pierre(a)reactos.org> writes: >> >>> Hi, >>> >>> I wanted to avoid yet another recopy (in the ANSI case) especially since >>> input is well-known: user necessarily provided NULL ptr to reach that point. >>> But, if you prefer, I can do another copy. But in the ANSI case, it will >>> mean that NETRESOURCE structure has been copied twice (rather expensive). >> >> It's not expensive, it's a small structure, and you don't need to >> duplicate the pointers. >> > > > > > -- Pierre Schweitzer <pierre at reactos.org> System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V.

July 29, 2016

Re: [PATCH 08/10] gdi32: Where possible directly access the DC instead of using GetCurrentObject().

by Alexandre Julliard

Huw Davies <huw(a)codeweavers.com> writes: > On Fri, Jul 29, 2016 at 09:22:43PM +0900, Alexandre Julliard wrote: >> Huw Davies <huw(a)codeweavers.com> writes: >> >> > @@ -1039,7 +1039,7 @@ INT WINAPI SetGraphicsMode( HDC hdc, INT mode ) >> > } >> > release_dc_ptr( dc ); >> > /* font metrics depend on the graphics mode */ >> > - if (ret) SelectObject(dc->hSelf, GetCurrentObject(dc->hSelf, OBJ_FONT)); >> > + if (ret) SelectObject(dc->hSelf, dc->hFont); >> >> The bug was already present, but we shouldn't be accessing the dc after >> release. > > Good catch (although it was originally your bug ;-) ) Then it can't be a bug. There are no bugs in my code. -- Alexandre Julliard julliard(a)winehq.org

July 29, 2016

Re: [PATCH 08/10] gdi32: Where possible directly access the DC instead of using GetCurrentObject().

by Huw Davies

On Fri, Jul 29, 2016 at 09:22:43PM +0900, Alexandre Julliard wrote: > Huw Davies <huw(a)codeweavers.com> writes: > > > @@ -1039,7 +1039,7 @@ INT WINAPI SetGraphicsMode( HDC hdc, INT mode ) > > } > > release_dc_ptr( dc ); > > /* font metrics depend on the graphics mode */ > > - if (ret) SelectObject(dc->hSelf, GetCurrentObject(dc->hSelf, OBJ_FONT)); > > + if (ret) SelectObject(dc->hSelf, dc->hFont); > > The bug was already present, but we shouldn't be accessing the dc after > release. Good catch (although it was originally your bug ;-) ) I've resent the entire series with an additional patch [8/11] which fixes this. It also only reselects the font if the mode changes. The other patches in the series are essentially the same as v1. Huw.

July 29, 2016

Re: comctl32: Avoid using memset on RECTs.

by Michael Stefaniuc

On 07/29/2016 01:14 PM, Nikolay Sivov wrote: > On 29.07.2016 13:24, Michael Stefaniuc wrote: >> Signed-off-by: Michael Stefaniuc <mstefani(a)redhat.de> >> --- >> dlls/comctl32/status.c | 4 ++-- >> dlls/comctl32/tab.c | 7 ++----- >> 2 files changed, 4 insertions(+), 7 deletions(-) >> >> diff --git a/dlls/comctl32/status.c b/dlls/comctl32/status.c >> index 738c33b..08ca158 100644 >> --- a/dlls/comctl32/status.c >> +++ b/dlls/comctl32/status.c >> @@ -127,8 +127,8 @@ STATUSBAR_ComputeHeight(STATUS_INFO *infoPtr) >> * textHeight pixels large */ >> HDC hdc = GetDC(infoPtr->Self); >> RECT r; >> - memset (&r, 0, sizeof (r)); >> - r.bottom = max(infoPtr->minHeight, tm.tmHeight); >> + >> + SetRect(&r, 0, 0, 0, max(infoPtr->minHeight, tm.tmHeight)); >> if (SUCCEEDED(GetThemeBackgroundExtent(theme, hdc, SP_PANE, 0, &r, &r))) >> { >> height = r.bottom - r.top; >> diff --git a/dlls/comctl32/tab.c b/dlls/comctl32/tab.c >> index 0ed9152..62e34f9 100644 >> --- a/dlls/comctl32/tab.c >> +++ b/dlls/comctl32/tab.c >> @@ -373,12 +373,9 @@ static BOOL TAB_InternalGetItemRect( >> TRACE("Not Visible\n"); >> /* need to initialize these to empty rects */ >> if (itemRect) >> - { >> - memset(itemRect,0,sizeof(RECT)); >> - itemRect->bottom = infoPtr->tabHeight; >> - } >> + SetRect(itemRect, 0, 0, 0, infoPtr->tabHeight); >> if (selectedRect) >> - memset(selectedRect,0,sizeof(RECT)); >> + SetRectEmpty(selectedRect); >> return FALSE; >> } >> >> > > You probably can remove null rect checks as well. Probably? That isn't obvious from the context. Those two are input parameters to TAB_InternalGetItemRect() that aren't checked beforehand. bye micheal

July 29, 2016

Re: [PATCH 08/10] gdi32: Where possible directly access the DC instead of using GetCurrentObject().

by Alexandre Julliard

Huw Davies <huw(a)codeweavers.com> writes: > @@ -1039,7 +1039,7 @@ INT WINAPI SetGraphicsMode( HDC hdc, INT mode ) > } > release_dc_ptr( dc ); > /* font metrics depend on the graphics mode */ > - if (ret) SelectObject(dc->hSelf, GetCurrentObject(dc->hSelf, OBJ_FONT)); > + if (ret) SelectObject(dc->hSelf, dc->hFont); The bug was already present, but we shouldn't be accessing the dc after release. -- Alexandre Julliard julliard(a)winehq.org

July 29, 2016

Re: comctl32: Avoid using memset on RECTs.

by Nikolay Sivov

On 29.07.2016 13:24, Michael Stefaniuc wrote: > Signed-off-by: Michael Stefaniuc <mstefani(a)redhat.de> > --- > dlls/comctl32/status.c | 4 ++-- > dlls/comctl32/tab.c | 7 ++----- > 2 files changed, 4 insertions(+), 7 deletions(-) > > diff --git a/dlls/comctl32/status.c b/dlls/comctl32/status.c > index 738c33b..08ca158 100644 > --- a/dlls/comctl32/status.c > +++ b/dlls/comctl32/status.c > @@ -127,8 +127,8 @@ STATUSBAR_ComputeHeight(STATUS_INFO *infoPtr) > * textHeight pixels large */ > HDC hdc = GetDC(infoPtr->Self); > RECT r; > - memset (&r, 0, sizeof (r)); > - r.bottom = max(infoPtr->minHeight, tm.tmHeight); > + > + SetRect(&r, 0, 0, 0, max(infoPtr->minHeight, tm.tmHeight)); > if (SUCCEEDED(GetThemeBackgroundExtent(theme, hdc, SP_PANE, 0, &r, &r))) > { > height = r.bottom - r.top; > diff --git a/dlls/comctl32/tab.c b/dlls/comctl32/tab.c > index 0ed9152..62e34f9 100644 > --- a/dlls/comctl32/tab.c > +++ b/dlls/comctl32/tab.c > @@ -373,12 +373,9 @@ static BOOL TAB_InternalGetItemRect( > TRACE("Not Visible\n"); > /* need to initialize these to empty rects */ > if (itemRect) > - { > - memset(itemRect,0,sizeof(RECT)); > - itemRect->bottom = infoPtr->tabHeight; > - } > + SetRect(itemRect, 0, 0, 0, infoPtr->tabHeight); > if (selectedRect) > - memset(selectedRect,0,sizeof(RECT)); > + SetRectEmpty(selectedRect); > return FALSE; > } > > You probably can remove null rect checks as well.

July 29, 2016

Re: [PATCH] msxml3: Implement IXMLParser GetParserState

by Alistair Leslie-Hughes

On 29/07/16 20:36, Nikolay Sivov wrote: > On 29.07.2016 12:43, Alistair Leslie-Hughes wrote: > > Do you have SDK headers for that interface? Having HRESULT for a state > looks a bit weird. > > No, but all the Document on msdn state that it returns HRESULT. I had to double check myself. https://msdn.microsoft.com/en-us/library/ms757816%28v=vs.85%29.aspx Best Regards Alistair Leslie-Hughes

July 29, 2016

Re: [PATCH] msxml3: Implement IXMLParser GetParserState

by Nikolay Sivov

On 29.07.2016 12:43, Alistair Leslie-Hughes wrote: > Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> > --- > dlls/msxml3/tests/xmlparser.c | 3 +++ > dlls/msxml3/xmlparser.c | 6 ++++-- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/dlls/msxml3/tests/xmlparser.c b/dlls/msxml3/tests/xmlparser.c > index 51f2a27..32514e3 100644 > --- a/dlls/msxml3/tests/xmlparser.c > +++ b/dlls/msxml3/tests/xmlparser.c > @@ -144,6 +144,9 @@ static void create_test(void) > hr = IXMLParser_SetFlags(parser, 0); > ok(hr == S_OK, "Expected S_OK got 0x%08x\n", hr); > > + hr = IXMLParser_GetParserState(parser); > + ok(hr == XMLPARSER_IDLE, "got 0x%08x\n", hr); > + > IXMLParser_Release(parser); > } > > diff --git a/dlls/msxml3/xmlparser.c b/dlls/msxml3/xmlparser.c > index ca40b22..6fcdb82 100644 > --- a/dlls/msxml3/xmlparser.c > +++ b/dlls/msxml3/xmlparser.c > @@ -51,6 +51,7 @@ typedef struct _xmlparser > LONG ref; > > int flags; > + XML_PARSER_STATE state; > } xmlparser; > > static inline xmlparser *impl_from_IXMLParser( IXMLParser *iface ) > @@ -345,9 +346,9 @@ static HRESULT WINAPI xmlparser_GetParserState(IXMLParser *iface) > { > xmlparser *This = impl_from_IXMLParser( iface ); > > - FIXME("(%p)\n", This); > + TRACE("(%p)\n", This); > > - return E_NOTIMPL; > + return This->state; > } > > static HRESULT WINAPI xmlparser_Suspend(IXMLParser *iface) > @@ -447,6 +448,7 @@ HRESULT XMLParser_create(void **ppObj) > This->nodefactory = NULL; > This->input = NULL; > This->flags = 0; > + This->state = XMLPARSER_IDLE; > This->ref = 1; > > *ppObj = &This->IXMLParser_iface; > Do you have SDK headers for that interface? Having HRESULT for a state looks a bit weird.

July 29, 2016

Re: [PATCH 2/6] d3d9/tests: Add visual tests for locking vertexbuffers

by Henri Verbeet

On 28 July 2016 at 18:41, Patrick Rudolph <siro(a)das-labor.org> wrote: > Am Thu, 28 Jul 2016 14:04:56 +0200 > schrieb Henri Verbeet <hverbeet(a)gmail.com>: >> I assume the broken() is for the testbot, but the way this is written, >> half the cases aren't really tested on Windows. Also, what makes these >> fail on the testbot? >> > I can replace some of the broken test results with another flag for AMD > behaviour. AMD allows D3DLOCK_NOOVERWRITE for non dynamic buffers on > d3d9, too. > I guess the WARP driver filters D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD > calls. You can make the check more specific by using adapter_is_warp(). Wrt. D3DLOCK_NOOVERWRITE on non-dynamic buffers, if the behaviour is not consistent between AMD and NVIDIA, it's probably best to drop those tests while keeping the comment. >> Does this work reliably? GetTickCount() has very limited resolution, I >> think there's a real chance that the draw will take only a fraction of >> GetTickCount()'s resolution. > > Yes it does. > I first thought about using timestamp queries, but those aren't > supported on ddraw and d3d8 and are broken on my AMD test system, so > I'm using GetTickCount instead. It allows to test to pass on fast > systems (Intel Core i5 + HD6870) and slow systems (Intel Atom + Nvidia > ION). The buffersize can't be hardcoded as the Windows driver nukes the > display driver whenever a draw call takes more than a few seconds, > which is the case when using huge buffers to make them pass on fast > systems. > Measuring CPU time is fine, but I'd feel a little better if this used QueryPerformanceCounter() instead of GetTickCount().

July 29, 2016

Re: wined3d: Add Nvidia GT 740M to the GPU list

by Henri Verbeet

On 28 July 2016 at 22:42, Jeremy Audiger <audiger(a)live.fr> wrote: > @@ -2158,6 +2160,7 @@ cards_nvidia_mesa[] = > {"NVF0", CARD_NVIDIA_GEFORCE_GTX780}, > {"NVE6", CARD_NVIDIA_GEFORCE_GTX770M}, > {"NVE4", CARD_NVIDIA_GEFORCE_GTX680}, > + {"NV108", CARD_NVIDIA_GEFORCE_GT740M}, > /* Fermi */ > {"NVD9", CARD_NVIDIA_GEFORCE_GT520}, > {"NVD7", CARD_NVIDIA_GEFORCE_820M}, The patch seems fine, but could you please add "NV108" above "NVF1" instead of below "NVE4"?

July 29, 2016

Re: [PATCH 2/2] DInput : Use vendor and product ids to generate product GUID like it's done on Windows

by DavidL

Thanks! In that case, I will add it to my list of needed patches for the OS X dinput joysticks ... unless one of you gents would like to take a crack at it? :) Cheers, David On Thu, Jul 28, 2016 at 8:44 AM, Bruno Jesus <00cpxxx(a)gmail.com> wrote: > On Thu, Jul 28, 2016 at 6:08 AM, DavidL <david.dljunk(a)gmail.com> wrote: > > I'm not really familiar with the intricacies of the guid generation ... > but > > looking at the Mac dinput code ... should a similar patch be applied > there? > > Hi, IMO yes in order to sync the output of all drivers to be the same > independent of host OS. You can test using the ditool.exe from: > https://sourceforge.net/projects/x360ce/files/ditool.zip > > Best wishes, > Bruno >

July 28, 2016

Re: [PATCH 1/6] wined3d/resource: Add flag WINED3D_MAP_WRITEONLY

by Patrick Rudolph

Am Thu, 28 Jul 2016 14:04:48 +0200 schrieb Henri Verbeet <hverbeet(a)gmail.com>: > On 26 July 2016 at 19:09, Patrick Rudolph <siro(a)das-labor.org> wrote: > > #define WINED3D_MAP_READONLY > > 0x0010 +#define > > WINED3D_MAP_WRITEONLY 0x0020 > But now the API is weird. What happens when both READONLY and > WRITEONLY are set? I think the d3d11 model makes more sense. Yes you are right. The patch is required to fix some of the d3d11 TODOs introduced in my tests, but you can ignore it for now. I'll add additional tests for it.

July 28, 2016

Re: [PATCH 2/6] d3d9/tests: Add visual tests for locking vertexbuffers

by Patrick Rudolph

Am Thu, 28 Jul 2016 14:04:56 +0200 schrieb Henri Verbeet <hverbeet(a)gmail.com>: > On 26 July 2016 at 19:09, Patrick Rudolph <siro(a)das-labor.org> wrote: > > + static const struct > > + { > > + const char *flags_s; > > + unsigned int flags; > > + DWORD usage; > > + BOOL unsyncronized; > > + BOOL todo; > > + BOOL broken; > > + } > > + /* Nvidia ignores D3DLOCK_NOOVERWRITE on non dynamic buffers, > > + * while AMD always accepts D3DLOCK_NOOVERWRITE on all > > buffers. */ > > + tests[] = > > + { > > + {"0", 0, 0, FALSE, FALSE, FALSE}, > > + {"0", 0, D3DUSAGE_DYNAMIC, FALSE, FALSE, FALSE}, > > + {"D3DLOCK_NOOVERWRITE", D3DLOCK_NOOVERWRITE, 0, FALSE, > > FALSE, TRUE}, > > + {"D3DLOCK_NOOVERWRITE", D3DLOCK_NOOVERWRITE, > > D3DUSAGE_DYNAMIC, TRUE, FALSE, FALSE}, > > + {"D3DLOCK_DISCARD", D3DLOCK_DISCARD, 0, FALSE, FALSE, > > FALSE}, > > + {"D3DLOCK_DISCARD", D3DLOCK_DISCARD, D3DUSAGE_DYNAMIC, > > FALSE, FALSE, FALSE}, > > + {"D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD", > > + D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD, 0, FALSE, > > FALSE, TRUE}, > > + {"D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD", > > + D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD, > > D3DUSAGE_DYNAMIC, TRUE, TRUE, TRUE}, > > + {"D3DLOCK_NOOVERWRITE | D3DLOCK_READONLY", > > + D3DLOCK_NOOVERWRITE | D3DLOCK_READONLY, 0, FALSE, > > FALSE, TRUE}, > > + {"D3DLOCK_NOOVERWRITE | D3DLOCK_READONLY", > > + D3DLOCK_NOOVERWRITE | D3DLOCK_READONLY, > > D3DUSAGE_DYNAMIC, TRUE, TRUE, TRUE}, > > + }; > [...] > > + todo_wine_if (tests[i].todo) > > + { > > + ok(tests[i].unsyncronized == unsyncronized || > > broken(tests[i].broken), > > + "Expected buffer mapped %s. Flags = %s. Usage > > = %#x.\n", > > + tests[i].unsyncronized ? "unsyncronized" : > > "syncronized", tests[i].flags_s, tests[i].usage); > > + } > I assume the broken() is for the testbot, but the way this is written, > half the cases aren't really tested on Windows. Also, what makes these > fail on the testbot? > I can replace some of the broken test results with another flag for AMD behaviour. AMD allows D3DLOCK_NOOVERWRITE for non dynamic buffers on d3d9, too. I guess the WARP driver filters D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD calls. > > + timestamp1 = GetTickCount(); > > + > > + hr = IDirect3DDevice9_BeginScene(device); > > + ok(SUCCEEDED(hr), "Failed to begin scene, hr %#x.\n", hr); > > + hr = IDirect3DDevice9_DrawPrimitive(device, > > D3DPT_TRIANGLESTRIP, 0, tri); > > + ok(SUCCEEDED(hr), "Failed to draw, hr %#x.\n", hr); > > + hr = IDirect3DDevice9_EndScene(device); > > + ok(SUCCEEDED(hr), "Failed to end scene, hr %#x.\n", hr); > > + > > + /* dummy read to wait for GPU being done */ > > + color = getPixelColor(device, 160, 360); > > + > > + timestamp2 = GetTickCount(); > > + > > + /* Adjust buffer size for 1 second draw call duration */ > > + size = (size * 1000UL) / (timestamp2 - timestamp1 + 1); > Does this work reliably? GetTickCount() has very limited resolution, I > think there's a real chance that the draw will take only a fraction of > GetTickCount()'s resolution. Yes it does. I first thought about using timestamp queries, but those aren't supported on ddraw and d3d8 and are broken on my AMD test system, so I'm using GetTickCount instead. It allows to test to pass on fast systems (Intel Core i5 + HD6870) and slow systems (Intel Atom + Nvidia ION). The buffersize can't be hardcoded as the Windows driver nukes the display driver whenever a draw call takes more than a few seconds, which is the case when using huge buffers to make them pass on fast systems. Regards, Patrick

July 28, 2016

Re: wine-devel Digest, Vol 132, Issue 55

by Anton Baskanov

On Wed, Jul 27, 2016 at 11:07 PM, <wine-devel-request(a)winehq.org> wrote: > ---------- Forwarded message ---------- > From: Nikolay Sivov <bunglehead(a)gmail.com> > To: wine-devel(a)winehq.org > Cc: > Date: Wed, 27 Jul 2016 17:39:26 +0300 > Subject: Re: [PATCH 1/6] amstream: Fix AMAudioData::QueryInterface. > On 27.07.2016 15:51, Anton Baskanov wrote: > > Signed-off-by: Anton Baskanov <baskanov(a)gmail.com> > > --- > > dlls/amstream/audiodata.c | 1 - > > dlls/amstream/tests/Makefile.in | 3 +- > > dlls/amstream/tests/audiodata.c | 67 > +++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 69 insertions(+), 2 deletions(-) > > create mode 100755 dlls/amstream/tests/audiodata.c > > Hi, Anton. > > Please keep tests in existing amstream.c file. > > + > > +static void test_query_interface(void) > > +{ > > + IUnknown *unknown = create_audio_data(); > > + IMemoryData *memory_data = NULL; > > + IAudioData *audio_data = NULL; > > + > > + HRESULT result = S_OK; > > result initialization is not needed; > > > + > > + result = IUnknown_QueryInterface(unknown, &IID_IMemoryData, (void > **)&memory_data); > > + ok(E_NOINTERFACE == result, "got 0x%08x\n", result); > > + if (S_OK == result) > > + { > > + IMemoryData_Release(memory_data); > > + } > > If it always fails, there's no reason add conditional Release. > > > + > > + result = IUnknown_QueryInterface(unknown, &IID_IAudioData, (void > **)&audio_data); > > + ok(S_OK == result, "got 0x%08x\n", result); > > + if (S_OK == result) > > + { > > + IAudioData_Release(audio_data); > > + } > > + > > + IUnknown_Release(unknown); > > +} > > Could you check if you can query IMemoryData from IAudioData? It seems > really weird if parent interface is not supported at all. > Hi, Nikolay. Thanks for the review! I've checked QueryInterface from IAudioData, and it also fails with IID_IMemoryData. I've fixed the issues and sent the updated patches.

July 28, 2016

Re: [PATCH 2/2] DInput : Use vendor and product ids to generate product GUID like it's done on Windows

by Bruno Jesus

On Thu, Jul 28, 2016 at 6:08 AM, DavidL <david.dljunk(a)gmail.com> wrote: > I'm not really familiar with the intricacies of the guid generation ... but > looking at the Mac dinput code ... should a similar patch be applied there? Hi, IMO yes in order to sync the output of all drivers to be the same independent of host OS. You can test using the ditool.exe from: https://sourceforge.net/projects/x360ce/files/ditool.zip Best wishes, Bruno

July 28, 2016

Re: [PATCH 2/2 v4] ucrtbase: Add _get_narrow_winmain_command_line tests

by Dmitry Timoshkov

Piotr Caban <piotr(a)codeweavers.com> wrote: > >>>>>>>>>> + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); > >>>>>>>>>> + memset(&startup, 0, sizeof(startup)); > >>>>>>>>>> + startup.cb = sizeof(startup); > >>>>>>>>>> + CreateProcessA(path, cmd, NULL, NULL, TRUE, > >>>>>>>>>> + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, > >>>>>>>>>> + NULL, NULL, &startup, &proc); > >>>>>>>>>> + winetest_wait_child_process(proc.hProcess); > >>>>>>>>> > >>>>>>>>> Thanks for adding the tests. It shouldn't be too hard to also test other > >>>>>>>>> white space characters, at least testing '\r' and '\n' would be nice to > >>>>>>>>> have. > >>>>>>>>> > >>>>>>>> CreateProcess will fail in case of '\r' or '\n' characters. > >>>>>>> > >>>>>>> In a simple test CreateProcess() works just fine here under Windows 7 when a > >>>>>>> passed in command line contains '\r' and/or '\n', where does it fail for you? > >>>>>>> > >>>>>> I'll try to be a little more precise - '\r' is treated as another > >>>>>> argument so it can't be used before first argument (misc in this case). > >>>>> > >>>>> Then it clearly deserves an additional test case. And a test for '\n' since > >>>>> it doesn't break anything. > >>>> '\n' behaves the same as '\r' according to my test. > >>>> > >>>> I think I have already explained you while '\r' case can't be tested in > >>>> reasonable way. > >>> > >>> I don't see why '\r' and '\n' can't be tested. If CreateProcess() call doesn't > >>> fail, what you need to test is the resulting command line that the child process > >>> receives. It's clear and simple. > >>> > >> I've already wrote you that '\r' is treated as argument - because of > >> that it can't be added before "misc" argument (the test executable needs > >> first argument to be "misc"). The function > >> _get_narrow_winmain_command_line only interprets part of the command > >> line before first argument. > > > > How about adding '\r' after "misc" or at whatever place you think appropriate > > and simply test both the resulting command line and the result of narrow one? > > Just execute the process and test whatever it gets as the command line and > > the narrow one? What is so hard with that? > > > Such test will be useless. It's already shown that > _get_narrow_winmain_command_line returns substring of GetCommandLineA. > It also doesn't touch anything past the beginning of first argument. There is no such a thing as a useless test IMHO, any test exercises the API and shows its result preventing possible regressions in future. -- Dmitry.

July 28, 2016

Re: [PATCH 2/2 v4] ucrtbase: Add _get_narrow_winmain_command_line tests

by Piotr Caban

On 07/28/16 17:25, Dmitry Timoshkov wrote: > Piotr Caban <piotr(a)codeweavers.com> wrote: > >>>>>>>>>> + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); >>>>>>>>>> + memset(&startup, 0, sizeof(startup)); >>>>>>>>>> + startup.cb = sizeof(startup); >>>>>>>>>> + CreateProcessA(path, cmd, NULL, NULL, TRUE, >>>>>>>>>> + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, >>>>>>>>>> + NULL, NULL, &startup, &proc); >>>>>>>>>> + winetest_wait_child_process(proc.hProcess); >>>>>>>>> >>>>>>>>> Thanks for adding the tests. It shouldn't be too hard to also test other >>>>>>>>> white space characters, at least testing '\r' and '\n' would be nice to >>>>>>>>> have. >>>>>>>>> >>>>>>>> CreateProcess will fail in case of '\r' or '\n' characters. >>>>>>> >>>>>>> In a simple test CreateProcess() works just fine here under Windows 7 when a >>>>>>> passed in command line contains '\r' and/or '\n', where does it fail for you? >>>>>>> >>>>>> I'll try to be a little more precise - '\r' is treated as another >>>>>> argument so it can't be used before first argument (misc in this case). >>>>> >>>>> Then it clearly deserves an additional test case. And a test for '\n' since >>>>> it doesn't break anything. >>>> '\n' behaves the same as '\r' according to my test. >>>> >>>> I think I have already explained you while '\r' case can't be tested in >>>> reasonable way. >>> >>> I don't see why '\r' and '\n' can't be tested. If CreateProcess() call doesn't >>> fail, what you need to test is the resulting command line that the child process >>> receives. It's clear and simple. >>> >> I've already wrote you that '\r' is treated as argument - because of >> that it can't be added before "misc" argument (the test executable needs >> first argument to be "misc"). The function >> _get_narrow_winmain_command_line only interprets part of the command >> line before first argument. > > How about adding '\r' after "misc" or at whatever place you think appropriate > and simply test both the resulting command line and the result of narrow one? > Just execute the process and test whatever it gets as the command line and > the narrow one? What is so hard with that? > Such test will be useless. It's already shown that _get_narrow_winmain_command_line returns substring of GetCommandLineA. It also doesn't touch anything past the beginning of first argument.

July 28, 2016

Re: [PATCH 2/2 v4] ucrtbase: Add _get_narrow_winmain_command_line tests

by Dmitry Timoshkov

Piotr Caban <piotr(a)codeweavers.com> wrote: > >>>>>>>> + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); > >>>>>>>> + memset(&startup, 0, sizeof(startup)); > >>>>>>>> + startup.cb = sizeof(startup); > >>>>>>>> + CreateProcessA(path, cmd, NULL, NULL, TRUE, > >>>>>>>> + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, > >>>>>>>> + NULL, NULL, &startup, &proc); > >>>>>>>> + winetest_wait_child_process(proc.hProcess); > >>>>>>> > >>>>>>> Thanks for adding the tests. It shouldn't be too hard to also test other > >>>>>>> white space characters, at least testing '\r' and '\n' would be nice to > >>>>>>> have. > >>>>>>> > >>>>>> CreateProcess will fail in case of '\r' or '\n' characters. > >>>>> > >>>>> In a simple test CreateProcess() works just fine here under Windows 7 when a > >>>>> passed in command line contains '\r' and/or '\n', where does it fail for you? > >>>>> > >>>> I'll try to be a little more precise - '\r' is treated as another > >>>> argument so it can't be used before first argument (misc in this case). > >>> > >>> Then it clearly deserves an additional test case. And a test for '\n' since > >>> it doesn't break anything. > >> '\n' behaves the same as '\r' according to my test. > >> > >> I think I have already explained you while '\r' case can't be tested in > >> reasonable way. > > > > I don't see why '\r' and '\n' can't be tested. If CreateProcess() call doesn't > > fail, what you need to test is the resulting command line that the child process > > receives. It's clear and simple. > > > I've already wrote you that '\r' is treated as argument - because of > that it can't be added before "misc" argument (the test executable needs > first argument to be "misc"). The function > _get_narrow_winmain_command_line only interprets part of the command > line before first argument. How about adding '\r' after "misc" or at whatever place you think appropriate and simply test both the resulting command line and the result of narrow one? Just execute the process and test whatever it gets as the command line and the narrow one? What is so hard with that? -- Dmitry.

July 28, 2016

Re: [PATCH 2/2 v4] ucrtbase: Add _get_narrow_winmain_command_line tests

by Piotr Caban

On 07/28/16 15:43, Dmitry Timoshkov wrote: > Piotr Caban <piotr(a)codeweavers.com> wrote: > >>>>>>>> + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); >>>>>>>> + memset(&startup, 0, sizeof(startup)); >>>>>>>> + startup.cb = sizeof(startup); >>>>>>>> + CreateProcessA(path, cmd, NULL, NULL, TRUE, >>>>>>>> + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, >>>>>>>> + NULL, NULL, &startup, &proc); >>>>>>>> + winetest_wait_child_process(proc.hProcess); >>>>>>> >>>>>>> Thanks for adding the tests. It shouldn't be too hard to also test other >>>>>>> white space characters, at least testing '\r' and '\n' would be nice to >>>>>>> have. >>>>>>> >>>>>> CreateProcess will fail in case of '\r' or '\n' characters. >>>>> >>>>> In a simple test CreateProcess() works just fine here under Windows 7 when a >>>>> passed in command line contains '\r' and/or '\n', where does it fail for you? >>>>> >>>> I'll try to be a little more precise - '\r' is treated as another >>>> argument so it can't be used before first argument (misc in this case). >>> >>> Then it clearly deserves an additional test case. And a test for '\n' since >>> it doesn't break anything. >> '\n' behaves the same as '\r' according to my test. >> >> I think I have already explained you while '\r' case can't be tested in >> reasonable way. > > I don't see why '\r' and '\n' can't be tested. If CreateProcess() call doesn't > fail, what you need to test is the resulting command line that the child process > receives. It's clear and simple. > I've already wrote you that '\r' is treated as argument - because of that it can't be added before "misc" argument (the test executable needs first argument to be "misc"). The function _get_narrow_winmain_command_line only interprets part of the command line before first argument.

July 28, 2016

Re: [PATCH 2/2 v4] ucrtbase: Add _get_narrow_winmain_command_line tests

by Dmitry Timoshkov

Piotr Caban <piotr(a)codeweavers.com> wrote: > >>>>>> + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); > >>>>>> + memset(&startup, 0, sizeof(startup)); > >>>>>> + startup.cb = sizeof(startup); > >>>>>> + CreateProcessA(path, cmd, NULL, NULL, TRUE, > >>>>>> + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, > >>>>>> + NULL, NULL, &startup, &proc); > >>>>>> + winetest_wait_child_process(proc.hProcess); > >>>>> > >>>>> Thanks for adding the tests. It shouldn't be too hard to also test other > >>>>> white space characters, at least testing '\r' and '\n' would be nice to > >>>>> have. > >>>>> > >>>> CreateProcess will fail in case of '\r' or '\n' characters. > >>> > >>> In a simple test CreateProcess() works just fine here under Windows 7 when a > >>> passed in command line contains '\r' and/or '\n', where does it fail for you? > >>> > >> I'll try to be a little more precise - '\r' is treated as another > >> argument so it can't be used before first argument (misc in this case). > > > > Then it clearly deserves an additional test case. And a test for '\n' since > > it doesn't break anything. > '\n' behaves the same as '\r' according to my test. > > I think I have already explained you while '\r' case can't be tested in > reasonable way. I don't see why '\r' and '\n' can't be tested. If CreateProcess() call doesn't fail, what you need to test is the resulting command line that the child process receives. It's clear and simple. -- Dmitry.

July 28, 2016

Re: [PATCH 2/2 v4] ucrtbase: Add _get_narrow_winmain_command_line tests

by Piotr Caban

On 07/28/16 15:27, Dmitry Timoshkov wrote: > Piotr Caban <piotr(a)codeweavers.com> wrote: > >>>>>> + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); >>>>>> + memset(&startup, 0, sizeof(startup)); >>>>>> + startup.cb = sizeof(startup); >>>>>> + CreateProcessA(path, cmd, NULL, NULL, TRUE, >>>>>> + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, >>>>>> + NULL, NULL, &startup, &proc); >>>>>> + winetest_wait_child_process(proc.hProcess); >>>>> >>>>> Thanks for adding the tests. It shouldn't be too hard to also test other >>>>> white space characters, at least testing '\r' and '\n' would be nice to >>>>> have. >>>>> >>>> CreateProcess will fail in case of '\r' or '\n' characters. >>> >>> In a simple test CreateProcess() works just fine here under Windows 7 when a >>> passed in command line contains '\r' and/or '\n', where does it fail for you? >>> >> I'll try to be a little more precise - '\r' is treated as another >> argument so it can't be used before first argument (misc in this case). > > Then it clearly deserves an additional test case. And a test for '\n' since > it doesn't break anything. '\n' behaves the same as '\r' according to my test. I think I have already explained you while '\r' case can't be tested in reasonable way.

July 28, 2016

Re: [PATCH 2/2 v4] ucrtbase: Add _get_narrow_winmain_command_line tests

by Dmitry Timoshkov

Piotr Caban <piotr(a)codeweavers.com> wrote: > >>>> + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); > >>>> + memset(&startup, 0, sizeof(startup)); > >>>> + startup.cb = sizeof(startup); > >>>> + CreateProcessA(path, cmd, NULL, NULL, TRUE, > >>>> + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, > >>>> + NULL, NULL, &startup, &proc); > >>>> + winetest_wait_child_process(proc.hProcess); > >>> > >>> Thanks for adding the tests. It shouldn't be too hard to also test other > >>> white space characters, at least testing '\r' and '\n' would be nice to > >>> have. > >>> > >> CreateProcess will fail in case of '\r' or '\n' characters. > > > > In a simple test CreateProcess() works just fine here under Windows 7 when a > > passed in command line contains '\r' and/or '\n', where does it fail for you? > > > I'll try to be a little more precise - '\r' is treated as another > argument so it can't be used before first argument (misc in this case). Then it clearly deserves an additional test case. And a test for '\n' since it doesn't break anything. -- Dmitry.

July 28, 2016

Re: [PATCH 1/2] msvcp140: Add MSVC 2015 C++ support DLL

by Piotr Caban

On 07/27/16 21:49, Daniel Lehman wrote: > +@ stub ??$_Getvals(a)_W@?$time_get(a)DV?$istreambuf_iterator(a)DU?$char_traits(a)D@std@@@std@@@std@@IAAX_WABV_Locinfo(a)1@@Z > +@ stub ??$_Getvals(a)_W@?$time_get(a)DV?$istreambuf_iterator(a)DU?$char_traits(a)D@std@@@std@@@std@@IAEX_WABV_Locinfo(a)1@@Z > +@ stub ??$_Getvals(a)_W@?$time_get(a)DV?$istreambuf_iterator(a)DU?$char_traits(a)D@std@@@std@@@std@@IEAAX_WAEBV_Locinfo(a)1@@Z > +@ stub ??$_Getvals(a)_W@?$time_get(a)GV?$istreambuf_iterator(a)GU?$char_traits(a)G@std@@@std@@@std@@IAAX_WABV_Locinfo(a)1@@Z > +@ stub ??$_Getvals(a)_W@?$time_get(a)GV?$istreambuf_iterator(a)GU?$char_traits(a)G@std@@@std@@@std@@IAEX_WABV_Locinfo(a)1@@Z > +@ stub ??$_Getvals(a)_W@?$time_get(a)GV?$istreambuf_iterator(a)GU?$char_traits(a)G@std@@@std@@@std@@IEAAX_WAEBV_Locinfo(a)1@@Z > +@ stub ??$_Getvals(a)_W@?$time_get(a)_WV?$istreambuf_iterator(a)_WU?$char_traits(a)_W@std@@@std@@@std@@IAAX_WABV_Locinfo(a)1@@Z > +@ stub ??$_Getvals(a)_W@?$time_get(a)_WV?$istreambuf_iterator(a)_WU?$char_traits(a)_W@std@@@std@@@std@@IAEX_WABV_Locinfo(a)1@@Z > +@ stub ??$_Getvals(a)_W@?$time_get(a)_WV?$istreambuf_iterator(a)_WU?$char_traits(a)_W@std@@@std@@@std@@IEAAX_WAEBV_Locinfo(a)1@@Z Could you please add -arch specifiers to stub entries? Thanks, Piotr

July 28, 2016

Re: avifil32: Use debugstr_a() and debugstr_w() in TRACE() messages.

by Michael Stefaniuc

On 07/28/2016 02:11 PM, Nikolay Sivov wrote: > I see it's Rejected on the patch list, it's indeed wrong, because you're > tracing output parameters. Hehehe, yeah. I run into the same issue with my first version of the debugstr_guid() script. The search and replace programming style can be tricky at times ;) bye michael

July 28, 2016

Re: avifil32: Use debugstr_a() and debugstr_w() in TRACE() messages.

by Nikolay Sivov

I see it's Rejected on the patch list, it's indeed wrong, because you're tracing output parameters.

July 28, 2016

Re: [PATCH 2/6] d3d9/tests: Add visual tests for locking vertexbuffers

by Henri Verbeet

On 26 July 2016 at 19:09, Patrick Rudolph <siro(a)das-labor.org> wrote: > + static const struct > + { > + const char *flags_s; > + unsigned int flags; > + DWORD usage; > + BOOL unsyncronized; > + BOOL todo; > + BOOL broken; > + } > + /* Nvidia ignores D3DLOCK_NOOVERWRITE on non dynamic buffers, > + * while AMD always accepts D3DLOCK_NOOVERWRITE on all buffers. */ > + tests[] = > + { > + {"0", 0, 0, FALSE, FALSE, FALSE}, > + {"0", 0, D3DUSAGE_DYNAMIC, FALSE, FALSE, FALSE}, > + {"D3DLOCK_NOOVERWRITE", D3DLOCK_NOOVERWRITE, 0, FALSE, FALSE, TRUE}, > + {"D3DLOCK_NOOVERWRITE", D3DLOCK_NOOVERWRITE, D3DUSAGE_DYNAMIC, TRUE, FALSE, FALSE}, > + {"D3DLOCK_DISCARD", D3DLOCK_DISCARD, 0, FALSE, FALSE, FALSE}, > + {"D3DLOCK_DISCARD", D3DLOCK_DISCARD, D3DUSAGE_DYNAMIC, FALSE, FALSE, FALSE}, > + {"D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD", > + D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD, 0, FALSE, FALSE, TRUE}, > + {"D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD", > + D3DLOCK_NOOVERWRITE | D3DLOCK_DISCARD, D3DUSAGE_DYNAMIC, TRUE, TRUE, TRUE}, > + {"D3DLOCK_NOOVERWRITE | D3DLOCK_READONLY", > + D3DLOCK_NOOVERWRITE | D3DLOCK_READONLY, 0, FALSE, FALSE, TRUE}, > + {"D3DLOCK_NOOVERWRITE | D3DLOCK_READONLY", > + D3DLOCK_NOOVERWRITE | D3DLOCK_READONLY, D3DUSAGE_DYNAMIC, TRUE, TRUE, TRUE}, > + }; [...] > + todo_wine_if (tests[i].todo) > + { > + ok(tests[i].unsyncronized == unsyncronized || broken(tests[i].broken), > + "Expected buffer mapped %s. Flags = %s. Usage = %#x.\n", > + tests[i].unsyncronized ? "unsyncronized" : "syncronized", tests[i].flags_s, tests[i].usage); > + } I assume the broken() is for the testbot, but the way this is written, half the cases aren't really tested on Windows. Also, what makes these fail on the testbot? > + timestamp1 = GetTickCount(); > + > + hr = IDirect3DDevice9_BeginScene(device); > + ok(SUCCEEDED(hr), "Failed to begin scene, hr %#x.\n", hr); > + hr = IDirect3DDevice9_DrawPrimitive(device, D3DPT_TRIANGLESTRIP, 0, tri); > + ok(SUCCEEDED(hr), "Failed to draw, hr %#x.\n", hr); > + hr = IDirect3DDevice9_EndScene(device); > + ok(SUCCEEDED(hr), "Failed to end scene, hr %#x.\n", hr); > + > + /* dummy read to wait for GPU being done */ > + color = getPixelColor(device, 160, 360); > + > + timestamp2 = GetTickCount(); > + > + /* Adjust buffer size for 1 second draw call duration */ > + size = (size * 1000UL) / (timestamp2 - timestamp1 + 1); Does this work reliably? GetTickCount() has very limited resolution, I think there's a real chance that the draw will take only a fraction of GetTickCount()'s resolution.

July 28, 2016

Re: [PATCH 1/6] wined3d/resource: Add flag WINED3D_MAP_WRITEONLY

by Henri Verbeet

On 26 July 2016 at 19:09, Patrick Rudolph <siro(a)das-labor.org> wrote: > #define WINED3D_MAP_READONLY 0x0010 > +#define WINED3D_MAP_WRITEONLY 0x0020 But now the API is weird. What happens when both READONLY and WRITEONLY are set? I think the d3d11 model makes more sense.

July 28, 2016

Re: [PATCH 2/2 v4] ucrtbase: Add _get_narrow_winmain_command_line tests

by Piotr Caban

On 07/28/16 13:33, Dmitry Timoshkov wrote: > Piotr Caban <piotr(a)codeweavers.com> wrote: > >>>> + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); >>>> + memset(&startup, 0, sizeof(startup)); >>>> + startup.cb = sizeof(startup); >>>> + CreateProcessA(path, cmd, NULL, NULL, TRUE, >>>> + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, >>>> + NULL, NULL, &startup, &proc); >>>> + winetest_wait_child_process(proc.hProcess); >>> >>> Thanks for adding the tests. It shouldn't be too hard to also test other >>> white space characters, at least testing '\r' and '\n' would be nice to >>> have. >>> >> CreateProcess will fail in case of '\r' or '\n' characters. > > In a simple test CreateProcess() works just fine here under Windows 7 when a > passed in command line contains '\r' and/or '\n', where does it fail for you? > I'll try to be a little more precise - '\r' is treated as another argument so it can't be used before first argument (misc in this case).

July 28, 2016

Re: [PATCH 2/2 v4] ucrtbase: Add _get_narrow_winmain_command_line tests

by Dmitry Timoshkov

Piotr Caban <piotr(a)codeweavers.com> wrote: > >> + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); > >> + memset(&startup, 0, sizeof(startup)); > >> + startup.cb = sizeof(startup); > >> + CreateProcessA(path, cmd, NULL, NULL, TRUE, > >> + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, > >> + NULL, NULL, &startup, &proc); > >> + winetest_wait_child_process(proc.hProcess); > > > > Thanks for adding the tests. It shouldn't be too hard to also test other > > white space characters, at least testing '\r' and '\n' would be nice to > > have. > > > CreateProcess will fail in case of '\r' or '\n' characters. In a simple test CreateProcess() works just fine here under Windows 7 when a passed in command line contains '\r' and/or '\n', where does it fail for you? -- Dmitry.

July 28, 2016

Re: [PATCH 2/2 v4] ucrtbase: Add _get_narrow_winmain_command_line tests

by Piotr Caban

On 07/28/16 13:08, Dmitry Timoshkov wrote: > Piotr Caban <piotr(a)codeweavers.com> wrote: > >> + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); >> + memset(&startup, 0, sizeof(startup)); >> + startup.cb = sizeof(startup); >> + CreateProcessA(path, cmd, NULL, NULL, TRUE, >> + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, >> + NULL, NULL, &startup, &proc); >> + winetest_wait_child_process(proc.hProcess); > > Thanks for adding the tests. It shouldn't be too hard to also test other > white space characters, at least testing '\r' and '\n' would be nice to > have. > CreateProcess will fail in case of '\r' or '\n' characters.

July 28, 2016

Re: [PATCH 2/2 v4] ucrtbase: Add _get_narrow_winmain_command_line tests

by Dmitry Timoshkov

Piotr Caban <piotr(a)codeweavers.com> wrote: > + sprintf(cmd, "\"\"%c\"\"\"%s\" \t \"misc\" cmd", name[0], name+1); > + memset(&startup, 0, sizeof(startup)); > + startup.cb = sizeof(startup); > + CreateProcessA(path, cmd, NULL, NULL, TRUE, > + CREATE_DEFAULT_ERROR_MODE|NORMAL_PRIORITY_CLASS, > + NULL, NULL, &startup, &proc); > + winetest_wait_child_process(proc.hProcess); Thanks for adding the tests. It shouldn't be too hard to also test other white space characters, at least testing '\r' and '\n' would be nice to have. -- Dmitry.

July 28, 2016

Re: [PATCH 2/2] DInput : Use vendor and product ids to generate product GUID like it's done on Windows

by DavidL

I'm not really familiar with the intricacies of the guid generation ... but looking at the Mac dinput code ... should a similar patch be applied there? Cheers, David On Sun, Jul 24, 2016 at 3:15 AM, Corentin Rossignon <corossig(a)gmail.com> wrote: > Done, thanks for taking some times and helping me doing a proper patch. > > > > On 07/24/2016 12:03 AM, Bruno Jesus wrote: > >> On Sat, Jul 23, 2016 at 8:01 AM, Corentin Rossignon <corossig(a)gmail.com> >> wrote: >> >>> Signed-off-by: Corentin Rossignon <corossig(a)gmail.com> >>> --- >>> dlls/dinput/joystick_linux.c | 34 >>> +++++++++++++++++++++++++++++++--- >>> dlls/dinput/joystick_linuxinput.c | 27 +++++++++++++++++++++++++-- >>> 2 files changed, 56 insertions(+), 5 deletions(-) >>> >> Formatting problem: >> >> bruno(a)Note:~/programs/wine$ git apply dinput2 >> dinput2:31: trailing whitespace. >> /* >> dinput2:35: trailing whitespace. >> * Data4 seems to be a constant. >> dinput2:104: trailing whitespace. >> /* >> dinput2:108: trailing whitespace. >> * Data4 seems to be a constant. >> warning: 4 lines add whitespace errors. >> >> diff --git a/dlls/dinput/joystick_linux.c b/dlls/dinput/joystick_linux.c >>> index 90b7280..f8499ca 100644 >>> --- a/dlls/dinput/joystick_linux.c >>> +++ b/dlls/dinput/joystick_linux.c >>> @@ -77,6 +77,7 @@ struct JoyDev >>> { >>> char device[MAX_PATH]; >>> char name[MAX_PATH]; >>> + GUID guid_product; >>> >>> BYTE axis_count; >>> BYTE button_count; >>> @@ -122,6 +123,21 @@ static const GUID DInput_Wine_Joystick_GUID = { /* >>> 9e573ed9-7734-11d2-8d4a-23903 >>> {0x8d, 0x4a, 0x23, 0x90, 0x3f, 0xb6, 0xbd, 0xf7} >>> }; >>> >>> + >>> +/* >>> + * Construct the GUID in the same way of Windows doing this. >>> + * Data1 is concatenation of productid and vendorid. >>> + * Data2 and Data3 are NULL. >>> + * Data4 seems to be a constant. >>> + */ >>> +static const GUID DInput_Wine_Joystick_Constant_Part_GUID = { >>> + 0x000000000, >>> + 0x0000, >>> + 0x0000, >>> + {0x00, 0x00, 0x50, 0x49, 0x44, 0x56, 0x49, 0x44} >>> +}; >>> + >>> + >>> >> Please remove all double blank lines in your changes in order to be >> more compatible with the surrounding code style. >> > > > >

July 28, 2016

Re: [PATCH 1/4] mpr: Implement local name automatic redirection for disks.

by Pierre Schweitzer

I also modified 2nd patch of the patch, because it was suffering the same issue and wasn't applying any longer. Both have been resent. Le 28/07/2016 à 10:12, Alexandre Julliard a écrit : > Pierre Schweitzer <pierre(a)reactos.org> writes: > >> Hi, >> >> I wanted to avoid yet another recopy (in the ANSI case) especially since >> input is well-known: user necessarily provided NULL ptr to reach that point. >> But, if you prefer, I can do another copy. But in the ANSI case, it will >> mean that NETRESOURCE structure has been copied twice (rather expensive). > > It's not expensive, it's a small structure, and you don't need to > duplicate the pointers. > -- Pierre Schweitzer <pierre at reactos.org> System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V.

July 28, 2016

Re: [PATCH 1/4] mpr: Implement local name automatic redirection for disks.

by Alexandre Julliard

Pierre Schweitzer <pierre(a)reactos.org> writes: > Hi, > > I wanted to avoid yet another recopy (in the ANSI case) especially since > input is well-known: user necessarily provided NULL ptr to reach that point. > But, if you prefer, I can do another copy. But in the ANSI case, it will > mean that NETRESOURCE structure has been copied twice (rather expensive). It's not expensive, it's a small structure, and you don't need to duplicate the pointers. -- Alexandre Julliard julliard(a)winehq.org

July 28, 2016

Re: [PATCH 3/4] mpr: Implement WNetCancelConnection2W().

by Nikolay Sivov

On 27.07.2016 16:16, Pierre Schweitzer wrote: > > DWORD WINAPI WNetCancelConnection2W( LPCWSTR lpName, DWORD dwFlags, BOOL fForce ) > { > - FIXME( "(%s, %08X, %d), stub\n", debugstr_w(lpName), dwFlags, fForce ); I think it's better to keep this as TRACE, also maybe change lpName -> name, dwFlags -> flags, fForce -> force. Flags are unused, so maybe some fixme message for them is appropriate. > + DWORD ret = WN_NO_NETWORK; > + DWORD index; > > - return WN_SUCCESS; > + if (providerTable != NULL) > + { > + for (index = 0; index < providerTable->numProviders; index++) > + { > + if(providerTable->table[index].getCaps(WNNC_CONNECTION) & > + WNNC_CON_GETCONNECTIONS) Shouldn't this be WNNC_CON_CANCELCONNECTION? > + { > + if (providerTable->table[index].cancelConnection) > + ret = providerTable->table[index].cancelConnection((LPWSTR)lpName, fForce); > + else > + ret = WN_NO_NETWORK; > + if (ret == WN_SUCCESS || ret == WN_OPEN_FILES) > + break; > + } According to docs, WN_OPEN_FILES happens when force == FALSE, does it make sense to handle it only in this case? Of course we have no easy way to test any of that. > + } > + } > + return ret; > }

July 28, 2016

Re: [PATCH 1/4] mpr: Implement local name automatic redirection for disks.

by Pierre Schweitzer

Hi, I wanted to avoid yet another recopy (in the ANSI case) especially since input is well-known: user necessarily provided NULL ptr to reach that point. But, if you prefer, I can do another copy. But in the ANSI case, it will mean that NETRESOURCE structure has been copied twice (rather expensive). Cheers, Le 28/07/2016 à 09:50, Alexandre Julliard a écrit : > Pierre Schweitzer <pierre(a)reactos.org> writes: > >> +done: >> + if (redirect) >> + ctxt->resource->lpLocalName = NULL; > > It looks quite suspicious to modify that input parameter and restore it > at the end. You should probably use a copy instead. > -- Pierre Schweitzer <pierre at reactos.org> System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V.

July 28, 2016

Re: [PATCH 1/4] mpr: Implement local name automatic redirection for disks.

by Alexandre Julliard

Pierre Schweitzer <pierre(a)reactos.org> writes: > +done: > + if (redirect) > + ctxt->resource->lpLocalName = NULL; It looks quite suspicious to modify that input parameter and restore it at the end. You should probably use a copy instead. -- Alexandre Julliard julliard(a)winehq.org

July 28, 2016

Re: [PATCH 2/2] msvcp140: add sprintf

by Nikolay Sivov

> +#if _MSVCP_VER >= 140 > +int __cdecl sprintf(char *buf, const char *fmt, ...) > +{ > + int ret; > + __ms_va_list valist; > + __ms_va_start(valist, fmt); > + ret = __stdio_common_vsprintf(2, buf, -1, fmt, NULL, valist); > + __ms_va_end(valist); > + return ret; > +} > +#endif Are we missing patch 1/2 that adds msvcp140? Also won't it work the same if you forward it to MSVCRT_sprintf? If _common_ functions should actually be used, options should use existing private defines.

July 28, 2016

Re: [PATCH 3/3] ntdll: For Mac 64-bit, poke NtCurrentTeb()->ThreadLocalStoragePointer to the corresponding offset from %gs.

by Alexandre Julliard

Sebastian Lackner <sebastian(a)fds-team.de> writes: > It looks like RtlRunOnceExecuteOnce is not safe to use (keyed_event not initialized yet). > You could use pthread (like used before in init_teb_key), but probably moving to > a function is sufficient to improve code readability. By avoiding the heavily indented > code it would already look a bit less hacky. ;) Simplifying the error handling would probably also help. For instance, pthread_setspecific() is not going to fail in any meaningful way. -- Alexandre Julliard julliard(a)winehq.org

July 28, 2016

Re: [PATCH 2/6] d3drm: Implement IDirect3DRM*::CreateViewport. (v2 resend)

by Stefan Dösinger

On 2016-07-27 20:27, Aaryaman Vasishta wrote: > > > On Thu, Jul 28, 2016 at 12:21 AM, Stefan Dösinger > <stefandoesinger(a)gmail.com <mailto:stefandoesinger(a)gmail.com>> wrote: > > > + if (!device || !camera) > > + return D3DRMERR_BADOBJECT; > > + if (!viewport) > > + return D3DRMERR_BADVALUE; > These checks are redundant, Init() does that as well. > > Agreed for version 3 of CreateViewport, but this this apply for version > 1/2 as well? Since I'm dereferencing these interfaces there I thought > the checks were justified. No, in 1 and 2 you need them as you say. Otherwise you'll crash when you try to call QI.

July 27, 2016

Re: [PATCH 1/6] d3drm: Implement IDirect3DRMViewport*::Init. (v7)

by Aaryaman Vasishta

On Thu, Jul 28, 2016 at 12:53 AM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 27 July 2016 at 21:09, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> > wrote: > > On Thu, Jul 28, 2016 at 12:13 AM, Stefan Dösinger > >> > + if (material) > >> > + IDirect3DMaterial_Release(material); > >> This will destroy the material, viewport::SetBackground doesn't addref > it. > > > > Right, henri suggested the same. I'll move this outside the cleanup > label. > I did say the sequence was odd, but it does work because "material" is > set to NULL on successful viewport initialisation. On the other hand, > I just noticed you'll also leak the material if > IDirect3DViewport_SetBackground() fails. > Right, sorry to not mention about the sequence. I also noticed the leak while fixing the same. Cheers, Aaryaman

July 27, 2016

Re: [PATCH 2/6] d3drm: Implement IDirect3DRM*::CreateViewport. (v2 resend)

by Aaryaman Vasishta

On Thu, Jul 28, 2016 at 12:21 AM, Stefan Dösinger <stefandoesinger(a)gmail.com > wrote: > > + if (!device || !camera) > > + return D3DRMERR_BADOBJECT; > > + if (!viewport) > > + return D3DRMERR_BADVALUE; > These checks are redundant, Init() does that as well. > Agreed for version 3 of CreateViewport, but this this apply for version 1/2 as well? Since I'm dereferencing these interfaces there I thought the checks were justified. > > > + ok(ref4 == frame_ref, "Expected ref4 == frame_ref, got frame_ref = > %u, ref4 = %u.\n", frame_ref, ref4); > > + > > + device_ref = get_refcount((IUnknown *)device1); > > + frame_ref = get_refcount((IUnknown *)frame); > The get_refcount here seems redundant because you've just shown that it is > back to where it used to be. > Right, these are definitely redundant. Thanks for the review! Cheers, Aaryaman

July 27, 2016

Re: [PATCH 1/6] d3drm: Implement IDirect3DRMViewport*::Init. (v7)

by Henri Verbeet

On 27 July 2016 at 21:09, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote: > On Thu, Jul 28, 2016 at 12:13 AM, Stefan Dösinger >> > + if (material) >> > + IDirect3DMaterial_Release(material); >> This will destroy the material, viewport::SetBackground doesn't addref it. > > Right, henri suggested the same. I'll move this outside the cleanup label. I did say the sequence was odd, but it does work because "material" is set to NULL on successful viewport initialisation. On the other hand, I just noticed you'll also leak the material if IDirect3DViewport_SetBackground() fails.

July 27, 2016

Re: [PATCH 3/6] d3drm: Implement IDirect3DRMViewport::GetDirect3DViewport. (resend)

by Stefan Dösinger

On 2016-07-27 20:08, Stefan Dösinger wrote: > You have some lines > 120 characters in here. Otherwise looks good to me. Actually, I accidentally counted the + from the patch, so it's exactly 120.

July 27, 2016

Re: [PATCH 1/6] d3drm: Implement IDirect3DRMViewport*::Init. (v7)

by Aaryaman Vasishta

On Thu, Jul 28, 2016 at 12:13 AM, Stefan Dösinger <stefandoesinger(a)gmail.com > wrote: > On 2016-07-19 19:08, Aaryaman Vasishta wrote: > > + memset(&mat, 0, sizeof(mat)); > > + mat.dwSize = sizeof(mat); > > + mat.diffuse.r = RGBA_GETRED(color) / 255.0f; > > + mat.diffuse.g = RGBA_GETGREEN(color) / 255.0f; > > + mat.diffuse.b = RGBA_GETBLUE(color) / 255.0f; > > + mat.diffuse.a = RGBA_GETALPHA(color) / 255.0f; > Doing this here seems questionable. What happens if the camera's > background is changed after the viewport is created? You'll probably want > to update the material property in viewport::clear(), or maybe in > camera::SetSceneBackground. > Right, that'll be added in the implementation of Clear. The camera frame itself doesn't contain a reference to the viewport so I doubt SetSceneBackground has any effect on it. (See the tests I wrote for Clear). > > > + if (material) > > + IDirect3DMaterial_Release(material); > This will destroy the material, viewport::SetBackground doesn't addref it. > Right, henri suggested the same. I'll move this outside the cleanup label. > > > + IDirect3DRMDevice3_Release(device3); > > + IDirect3DRMDevice_Release(device1); > > + ref4 = get_refcount((IUnknown *)d3drm1); > > + todo_wine ok(ref4 > initial_ref1, "Expected ref4 > initial_ref1, > got initial_ref1 = %u, ref4 = %u.\n", initial_ref1, ref4); > The ref4 > inital_ref1 check isn't all too surprising because frame3 still > holds a reference to its d3drm parent. Though it seems the leaked reference > added by the second Viewport::Init call gets released somewhere. The test > doesn't exactly show where. It is possible that releasing the device frees > it, or releasing the frame does it. > The leaked references created by the second Init call are all released when the device is destroyed. This suggests that the device is using some sort of destroy callback and keeping track of all leaked references. See the ref4 == initial_ref1 test below it. I'll resend the patch with the fixes you mentioned. Thanks for the review! Cheers, Aaryaman

July 27, 2016

Re: [PATCH 3/6] d3drm: Implement IDirect3DRMViewport::GetDirect3DViewport. (resend)

by Stefan Dösinger

You have some lines > 120 characters in here. Otherwise looks good to me. On 2016-07-19 19:08, Aaryaman Vasishta wrote: > Signed-off-by: Aaryaman Vasishta <jem456.vasishta(a)gmail.com> > --- > dlls/d3drm/tests/d3drm.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++- > dlls/d3drm/viewport.c | 18 ++++++++++--- > 2 files changed, 80 insertions(+), 5 deletions(-) > > diff --git a/dlls/d3drm/tests/d3drm.c b/dlls/d3drm/tests/d3drm.c > index 17cf2dd..fc0b6b4 100644 > --- a/dlls/d3drm/tests/d3drm.c > +++ b/dlls/d3drm/tests/d3drm.c > @@ -1474,12 +1474,13 @@ static void test_Viewport(void) > IDirect3DRMFrame3 *frame3; > IDirect3DRMViewport *viewport; > IDirect3DRMViewport2 *viewport2; > + IDirect3DViewport *d3d_viewport; > IDirect3DRMObject *obj, *obj2; > GUID driver; > HWND window; > RECT rc; > DWORD size, data, ref1, ref2, ref3, ref4; > - DWORD initial_ref1, initial_ref2, initial_ref3, device_ref, frame_ref, frame_ref2; > + DWORD initial_ref1, initial_ref2, initial_ref3, device_ref, frame_ref, frame_ref2, viewport_ref; > CHAR cname[64] = {0}; > > window = CreateWindowA("static", "d3drm_test", WS_OVERLAPPEDWINDOW, 0, 0, 300, 200, 0, 0, 0, 0); > @@ -1641,13 +1642,47 @@ static void test_Viewport(void) > > hr = IDirect3DRM2_CreateViewport(d3drm2, device1, frame, rc.left, rc.top, rc.right, rc.bottom, &viewport); > ok(hr == D3DRM_OK, "Cannot get IDirect3DRMViewport interface (hr = %x)\n", hr); > + hr = IDirect3DRMViewport_GetDirect3DViewport(viewport, &d3d_viewport); > + ok(SUCCEEDED(hr), "Cannot get IDirect3DViewport interface (hr = %#x).\n", hr); > + viewport_ref = get_refcount((IUnknown *)d3d_viewport); > + hr = IDirect3DRMViewport_GetDirect3DViewport(viewport, &d3d_viewport); > + ok(SUCCEEDED(hr), "Cannot get IDirect3DViewport interface (hr = %#x).\n", hr); > + ref4 = get_refcount((IUnknown *)d3d_viewport); > + ok(ref4 > viewport_ref, "Expected ref4 > viewport_ref, got ref4 = %u, viewport_ref = %u.\n", ref4, viewport_ref); > + IDirect3DViewport_Release(d3d_viewport); > + ref4 = get_refcount((IUnknown *)d3d_viewport); > + ok(ref4 == viewport_ref, "Expected ref4 == viewport_ref, got ref4 = %u, viewport_ref = %u.\n", ref4, viewport_ref); > + IDirect3DViewport_Release(d3d_viewport); > IDirect3DRMViewport_Release(viewport); > + > hr = IDirect3DRM3_CreateViewport(d3drm3, device3, frame3, rc.left, rc.top, rc.right, rc.bottom, &viewport2); > ok(hr == D3DRM_OK, "Cannot get IDirect3DRMViewport2 interface (hr = %x)\n", hr); > + hr = IDirect3DRMViewport2_GetDirect3DViewport(viewport2, &d3d_viewport); > + ok(SUCCEEDED(hr), "Cannot get IDirect3DViewport interface (hr = %#x).\n", hr); > + viewport_ref = get_refcount((IUnknown *)d3d_viewport); > + hr = IDirect3DRMViewport2_GetDirect3DViewport(viewport2, &d3d_viewport); > + ok(SUCCEEDED(hr), "Cannot get IDirect3DViewport interface (hr = %#x).\n", hr); > + ref4 = get_refcount((IUnknown *)d3d_viewport); > + ok(ref4 > viewport_ref, "Expected ref4 > viewport_ref, got ref4 = %u, viewport_ref = %u.\n", ref4, viewport_ref); > + IDirect3DViewport_Release(d3d_viewport); > + ref4 = get_refcount((IUnknown *)d3d_viewport); > + ok(ref4 == viewport_ref, "Expected ref4 == viewport_ref, got ref4 = %u, viewport_ref = %u.\n", ref4, viewport_ref); > + IDirect3DViewport_Release(d3d_viewport); > IDirect3DRMViewport2_Release(viewport2); > > hr = IDirect3DRM_CreateViewport(d3drm1, device1, frame, rc.left, rc.top, rc.right, rc.bottom, &viewport); > ok(hr == D3DRM_OK, "Cannot get IDirect3DRMViewport interface (hr = %x)\n", hr); > + hr = IDirect3DRMViewport_GetDirect3DViewport(viewport, &d3d_viewport); > + ok(SUCCEEDED(hr), "Cannot get IDirect3DViewport interface (hr = %#x).\n", hr); > + viewport_ref = get_refcount((IUnknown *)d3d_viewport); > + hr = IDirect3DRMViewport_GetDirect3DViewport(viewport, &d3d_viewport); > + ok(SUCCEEDED(hr), "Cannot get IDirect3DViewport interface (hr = %#x).\n", hr); > + ref4 = get_refcount((IUnknown *)d3d_viewport); > + ok(ref4 > viewport_ref, "Expected ref4 > viewport_ref, got ref4 = %u, viewport_ref = %u.\n", ref4, viewport_ref); > + IDirect3DViewport_Release(d3d_viewport); > + ref4 = get_refcount((IUnknown *)d3d_viewport); > + ok(ref4 == viewport_ref, "Expected ref4 == viewport_ref, got ref4 = %u, viewport_ref = %u.\n", ref4, viewport_ref); > + IDirect3DViewport_Release(d3d_viewport); > > hr = IDirect3DRMViewport_QueryInterface(viewport, &IID_IDirect3DRMObject, (void**)&obj); > ok(hr == D3DRM_OK, "expected D3DRM_OK (hr = %x)\n", hr); > @@ -1713,6 +1748,9 @@ static void test_Viewport(void) > ref4 = get_refcount((IUnknown *)d3drm3); > ok(ref4 == ref3, "Expected ref4 == ref3, got ref3 = %u, ref4 = %u.\n", ref3, ref4); > > + hr = IDirect3DRMViewport_GetDirect3DViewport(viewport, &d3d_viewport); > + ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr); > + > /* Test all failures together */ > hr = IDirect3DRMViewport_Init(viewport, NULL, frame, rc.left, rc.top, rc.right, rc.bottom); > ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr); > @@ -1740,6 +1778,18 @@ static void test_Viewport(void) > ref4 = get_refcount((IUnknown *)frame); > ok(ref4 > frame_ref, "Expected ref4 > frame_ref, got frame_ref = %u, ref4 = %u.\n", frame_ref, ref4); > > + hr = IDirect3DRMViewport_GetDirect3DViewport(viewport, &d3d_viewport); > + ok(SUCCEEDED(hr), "Cannot get IDirect3DViewport interface (hr = %#x).\n", hr); > + viewport_ref = get_refcount((IUnknown *)d3d_viewport); > + hr = IDirect3DRMViewport_GetDirect3DViewport(viewport, &d3d_viewport); > + ok(SUCCEEDED(hr), "Cannot get IDirect3DViewport interface (hr = %#x).\n", hr); > + ref4 = get_refcount((IUnknown *)d3d_viewport); > + ok(ref4 > viewport_ref, "Expected ref4 > viewport_ref, got ref4 = %u, viewport_ref = %u.\n", ref4, viewport_ref); > + IDirect3DViewport_Release(d3d_viewport); > + ref4 = get_refcount((IUnknown *)d3d_viewport); > + ok(ref4 == viewport_ref, "Expected ref4 == viewport_ref, got ref4 = %u, viewport_ref = %u.\n", ref4, viewport_ref); > + IDirect3DViewport_Release(d3d_viewport); > + > hr = IDirect3DRMViewport_Init(viewport, device1, frame, rc.left, rc.top, rc.right, rc.bottom); > ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr); > > @@ -1768,6 +1818,9 @@ static void test_Viewport(void) > ref4 = get_refcount((IUnknown *)d3drm3); > ok(ref4 == ref3, "Expected ref4 == ref3, got ref3 = %u, ref4 = %u.\n", ref3, ref4); > > + hr = IDirect3DRMViewport2_GetDirect3DViewport(viewport2, &d3d_viewport); > + ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr); > + > hr = IDirect3DRMViewport2_Init(viewport2, NULL, frame3, rc.left, rc.top, rc.right, rc.bottom); > ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr); > hr = IDirect3DRMViewport2_Init(viewport2, device3, NULL, rc.left, rc.top, rc.right, rc.bottom); > @@ -1788,6 +1841,18 @@ static void test_Viewport(void) > ref4 = get_refcount((IUnknown *)frame3); > ok(ref4 > frame_ref2, "Expected ref4 > frame_ref2, got frame_ref2 = %u, ref4 = %u.\n", frame_ref2, ref4); > > + hr = IDirect3DRMViewport2_GetDirect3DViewport(viewport2, &d3d_viewport); > + ok(SUCCEEDED(hr), "Cannot get IDirect3DViewport interface (hr = %#x).\n", hr); > + viewport_ref = get_refcount((IUnknown *)d3d_viewport); > + hr = IDirect3DRMViewport2_GetDirect3DViewport(viewport2, &d3d_viewport); > + ok(SUCCEEDED(hr), "Cannot get IDirect3DViewport interface (hr = %#x).\n", hr); > + ref4 = get_refcount((IUnknown *)d3d_viewport); > + ok(ref4 > viewport_ref, "Expected ref4 > viewport_ref, got ref4 = %u, viewport_ref = %u.\n", ref4, viewport_ref); > + IDirect3DViewport_Release(d3d_viewport); > + ref4 = get_refcount((IUnknown *)d3d_viewport); > + ok(ref4 == viewport_ref, "Expected ref4 == viewport_ref, got ref4 = %u, viewport_ref = %u.\n", ref4, viewport_ref); > + IDirect3DViewport_Release(d3d_viewport); > + > hr = IDirect3DRMViewport2_Init(viewport2, device3, frame3, rc.left, rc.top, rc.right, rc.bottom); > ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr); > > diff --git a/dlls/d3drm/viewport.c b/dlls/d3drm/viewport.c > index 730a1f5..09b0e83 100644 > --- a/dlls/d3drm/viewport.c > +++ b/dlls/d3drm/viewport.c > @@ -826,17 +826,27 @@ static D3DRMPROJECTIONTYPE WINAPI d3drm_viewport1_GetProjection(IDirect3DRMViewp > static HRESULT WINAPI d3drm_viewport2_GetDirect3DViewport(IDirect3DRMViewport2 *iface, > IDirect3DViewport **viewport) > { > - FIXME("iface %p, viewport %p stub!\n", iface, viewport); > + struct d3drm_viewport *viewport_object = impl_from_IDirect3DRMViewport2(iface); > > - return E_NOTIMPL; > + TRACE("iface %p, viewport %p.\n", iface, viewport); > + > + if (!viewport_object->d3d_viewport) > + return D3DRMERR_BADOBJECT; > + > + *viewport = viewport_object->d3d_viewport; > + IDirect3DViewport_AddRef(*viewport); > + > + return D3DRM_OK; > } > > static HRESULT WINAPI d3drm_viewport1_GetDirect3DViewport(IDirect3DRMViewport *iface, > IDirect3DViewport **viewport) > { > - FIXME("iface %p, viewport %p stub!\n", iface, viewport); > + struct d3drm_viewport *viewport_object = impl_from_IDirect3DRMViewport(iface); > > - return E_NOTIMPL; > + TRACE("iface %p, viewport %p.\n", iface, viewport); > + > + return d3drm_viewport2_GetDirect3DViewport(&viewport_object->IDirect3DRMViewport2_iface, viewport); > } > > static HRESULT WINAPI d3drm_viewport2_TransformVectors(IDirect3DRMViewport2 *iface, >

July 27, 2016

Re: [PATCH 3/3] ntdll: For Mac 64-bit, poke NtCurrentTeb()->ThreadLocalStoragePointer to the corresponding offset from %gs.

by Sebastian Lackner

On 27.07.2016 20:25, Ken Thomases wrote: > On Jul 27, 2016, at 12:53 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: >> >> I have only two suggestions: >> - gsbase_offset == 0 (key == i) could also be valid, so it would be useful to distinguish this case. > > It can't be valid because that would put the TLS at the beginning of struct _pthread, but the ABI actually reveals the first couple of fields. pthread_t is a typedef for __darwin_pthread_t which is a typedef for struct _opaque_pthread_t which is defined (not just declared) in the headers (/usr/include/sys/_pthread/_pthread_types.h in the 10.11 SDK) and is less opaque than its name would suggest. Comments in the definition of struct _pthread in libpthread make note of the ABI compatibility factor. > > In practice, the TLS array is at the end of the struct and I think Apple told us it has to be there. When you are sure that this is always true you can start searching at (key + 1). > >> - the logic could be moved to a helper function, for example using the init once mechanism > > I suppose, but it will be called for the first thread before there's a chance of creating secondary threads. So, there's no race. Also, I haven't looked, but I'm not sure what thread setup has to have been completed before RtlRunOnceExecuteOnce() can safely be used. It looks like RtlRunOnceExecuteOnce is not safe to use (keyed_event not initialized yet). You could use pthread (like used before in init_teb_key), but probably moving to a function is sufficient to improve code readability. By avoiding the heavily indented code it would already look a bit less hacky. ;) > > -Ken >

July 27, 2016

Re: [PATCH 2/6] d3drm: Implement IDirect3DRM*::CreateViewport. (v2 resend)

by Stefan Dösinger

On 2016-07-19 19:08, Aaryaman Vasishta wrote: > + if (FAILED(hr = IDirect3DRMDevice_QueryInterface(device, &IID_IDirect3DRMDevice3, (void **)&device3))) > + return hr; > + > + if (FAILED(hr = IDirect3DRMFrame_QueryInterface(camera, &IID_IDirect3DRMFrame3, (void **)&camera3))) > + { > + IDirect3DRMDevice3_Release(device3); > + return hr; > + } > + IDirect3DRMDevice_Release(device); > + IDirect3DRMFrame_Release(camera); This releases references you don't own. Same in the V2 -> V3 thunk. > + if (!device || !camera) > + return D3DRMERR_BADOBJECT; > + if (!viewport) > + return D3DRMERR_BADVALUE; These checks are redundant, Init() does that as well. > + ok(ref4 == frame_ref, "Expected ref4 == frame_ref, got frame_ref = %u, ref4 = %u.\n", frame_ref, ref4); > + > + device_ref = get_refcount((IUnknown *)device1); > + frame_ref = get_refcount((IUnknown *)frame); The get_refcount here seems redundant because you've just shown that it is back to where it used to be.

July 27, 2016

Re: [PATCH 1/6] d3drm: Implement IDirect3DRMViewport*::Init. (v7)

by Stefan Dösinger

On 2016-07-19 19:08, Aaryaman Vasishta wrote: > + memset(&mat, 0, sizeof(mat)); > + mat.dwSize = sizeof(mat); > + mat.diffuse.r = RGBA_GETRED(color) / 255.0f; > + mat.diffuse.g = RGBA_GETGREEN(color) / 255.0f; > + mat.diffuse.b = RGBA_GETBLUE(color) / 255.0f; > + mat.diffuse.a = RGBA_GETALPHA(color) / 255.0f; Doing this here seems questionable. What happens if the camera's background is changed after the viewport is created? You'll probably want to update the material property in viewport::clear(), or maybe in camera::SetSceneBackground. > + if (material) > + IDirect3DMaterial_Release(material); This will destroy the material, viewport::SetBackground doesn't addref it. > + hr = IDirect3DRMViewport_Init(viewport, device1, frame, rc.left, rc.top, rc.right + 1, rc.bottom + 1); > + ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr); > + hr = IDirect3DRMViewport_Init(viewport, device1, frame, rc.left, rc.top, rc.right + 1, rc.bottom); > + ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr); > + hr = IDirect3DRMViewport_Init(viewport, device1, frame, rc.left, rc.top, rc.right + 1, rc.bottom + 1); > + ok(hr == D3DRMERR_BADOBJECT, "Expected hr == D3DRMERR_BADOBJECT, got %#x.\n", hr); The first and third test are identical. > + IDirect3DRMDevice3_Release(device3); > + IDirect3DRMDevice_Release(device1); > + ref4 = get_refcount((IUnknown *)d3drm1); > + todo_wine ok(ref4 > initial_ref1, "Expected ref4 > initial_ref1, got initial_ref1 = %u, ref4 = %u.\n", initial_ref1, ref4); The ref4 > inital_ref1 check isn't all too surprising because frame3 still holds a reference to its d3drm parent. Though it seems the leaked reference added by the second Viewport::Init call gets released somewhere. The test doesn't exactly show where. It is possible that releasing the device frees it, or releasing the frame does it.

July 27, 2016

Re: [PATCH 3/3] ntdll: For Mac 64-bit, poke NtCurrentTeb()->ThreadLocalStoragePointer to the corresponding offset from %gs.

by Ken Thomases

On Jul 27, 2016, at 12:53 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > > I have only two suggestions: > - gsbase_offset == 0 (key == i) could also be valid, so it would be useful to distinguish this case. It can't be valid because that would put the TLS at the beginning of struct _pthread, but the ABI actually reveals the first couple of fields. pthread_t is a typedef for __darwin_pthread_t which is a typedef for struct _opaque_pthread_t which is defined (not just declared) in the headers (/usr/include/sys/_pthread/_pthread_types.h in the 10.11 SDK) and is less opaque than its name would suggest. Comments in the definition of struct _pthread in libpthread make note of the ABI compatibility factor. In practice, the TLS array is at the end of the struct and I think Apple told us it has to be there. > - the logic could be moved to a helper function, for example using the init once mechanism I suppose, but it will be called for the first thread before there's a chance of creating secondary threads. So, there's no race. Also, I haven't looked, but I'm not sure what thread setup has to have been completed before RtlRunOnceExecuteOnce() can safely be used. -Ken

July 27, 2016

Re: [PATCH 3/3] ntdll: For Mac 64-bit, poke NtCurrentTeb()->ThreadLocalStoragePointer to the corresponding offset from %gs.

by Sebastian Lackner

On 27.07.2016 19:42, Ken Thomases wrote: >> >> Hm, I see. Do you know if the offset really changed in the past? > > Yes, it has changed in the past. > >> I am asking >> because I assume the bruteforcing is an attempt to make the code more reliable, >> however on the other hand you still make the assumption that pthread keys >> directly correspond to indices into the %gs segment, which could also change. > > Well, we made various proposals to Apple to get a more comprehensive, less kludgy fix and they were rejected because of binary compatibility. Basically, they were unwilling to change how pthread_getspecific() works. Mono depends on it, for example. So, it seems pretty reliable. > >> A check that it matches expected behavior is probably sufficient. >> >> Besides that, if the gsbase cannot be located, it probably would be preferred >> to skip this code on following attempts. > > It already does that. gsbase_offset is static. It will only be negative on the first call. Sorry, I missed the "gsbase_offset = 0;" call. You are probably right that there is not much to improve unless Apple offers a better interface. I have only two suggestions: - gsbase_offset == 0 (key == i) could also be valid, so it would be useful to distinguish this case. - the logic could be moved to a helper function, for example using the init once mechanism > > -Ken >

July 27, 2016

Re: [PATCH 3/3] ntdll: For Mac 64-bit, poke NtCurrentTeb()->ThreadLocalStoragePointer to the corresponding offset from %gs.

by Ken Thomases

On Jul 27, 2016, at 12:14 PM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > > On 27.07.2016 18:27, Ken Thomases wrote: >> On Jul 27, 2016, at 11:06 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: >>> >>> On 27.07.2016 17:42, Ken Thomases wrote: >>>> 64-bit Windows apps have hard-coded accesses to %gs:0x58 baked into them. They >>>> need to find the ThreadLocalStoragePointer there. >>>> >>>> Technically, the gsbase register and the memory it points to belong to the >>>> pthread implementation on macOS. It's used for the pthread TLS implementation. >>>> Slot 11 (offset 0x58) is currently used for the implementation of the ttyname() >>>> system library function. We do not anticipate that Wine or any of the system >>>> libraries or frameworks it uses will call ttyname(). Furthermore, Apple has >>>> made it so that future releases of macOS will no longer use that slot. So, we >>>> hijack it for our purposes. >>>> >>>> Signed-off-by: Ken Thomases <ken(a)codeweavers.com> >>>> --- >>>> dlls/ntdll/loader.c | 11 +++++++- >>>> dlls/ntdll/signal_x86_64.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 72 insertions(+), 1 deletion(-) >>> >>> I'm not sure if I correctly understand the purpose of this patch. If you have to >>> set %gs:0x58 to some specific value, why not just use an assembly instruction for that? >>> >> >> alloc_tls_slot() is setting ThreadLocalStoragePointer for all threads, not just the current one. >> >> -Ken >> > > Hm, I see. Do you know if the offset really changed in the past? Yes, it has changed in the past. > I am asking > because I assume the bruteforcing is an attempt to make the code more reliable, > however on the other hand you still make the assumption that pthread keys > directly correspond to indices into the %gs segment, which could also change. Well, we made various proposals to Apple to get a more comprehensive, less kludgy fix and they were rejected because of binary compatibility. Basically, they were unwilling to change how pthread_getspecific() works. Mono depends on it, for example. So, it seems pretty reliable. > A check that it matches expected behavior is probably sufficient. > > Besides that, if the gsbase cannot be located, it probably would be preferred > to skip this code on following attempts. It already does that. gsbase_offset is static. It will only be negative on the first call. -Ken

July 27, 2016

Re: [PATCH 3/3] ntdll: For Mac 64-bit, poke NtCurrentTeb()->ThreadLocalStoragePointer to the corresponding offset from %gs.

by Sebastian Lackner

On 27.07.2016 18:27, Ken Thomases wrote: > On Jul 27, 2016, at 11:06 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: >> >> On 27.07.2016 17:42, Ken Thomases wrote: >>> 64-bit Windows apps have hard-coded accesses to %gs:0x58 baked into them. They >>> need to find the ThreadLocalStoragePointer there. >>> >>> Technically, the gsbase register and the memory it points to belong to the >>> pthread implementation on macOS. It's used for the pthread TLS implementation. >>> Slot 11 (offset 0x58) is currently used for the implementation of the ttyname() >>> system library function. We do not anticipate that Wine or any of the system >>> libraries or frameworks it uses will call ttyname(). Furthermore, Apple has >>> made it so that future releases of macOS will no longer use that slot. So, we >>> hijack it for our purposes. >>> >>> Signed-off-by: Ken Thomases <ken(a)codeweavers.com> >>> --- >>> dlls/ntdll/loader.c | 11 +++++++- >>> dlls/ntdll/signal_x86_64.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 72 insertions(+), 1 deletion(-) >> >> I'm not sure if I correctly understand the purpose of this patch. If you have to >> set %gs:0x58 to some specific value, why not just use an assembly instruction for that? >> > > alloc_tls_slot() is setting ThreadLocalStoragePointer for all threads, not just the current one. > > -Ken > Hm, I see. Do you know if the offset really changed in the past? I am asking because I assume the bruteforcing is an attempt to make the code more reliable, however on the other hand you still make the assumption that pthread keys directly correspond to indices into the %gs segment, which could also change. A check that it matches expected behavior is probably sufficient. Besides that, if the gsbase cannot be located, it probably would be preferred to skip this code on following attempts.

July 27, 2016

Re: [PATCH 1/3] ntdll: For Mac 64-bit, poke the TEB address to %gs:0x30 and re-enable the inlining of NtCurrentTeb().

by Ken Thomases

On Jul 27, 2016, at 11:38 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > > On 27.07.2016 18:30, Ken Thomases wrote: >> On Jul 27, 2016, at 11:08 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: >>> >>> On 27.07.2016 17:42, Ken Thomases wrote: >>>> 64-bit Windows apps have hard-coded accesses to %gs:0x30 baked into them. They >>>> need to find the TEB self pointer there. >>>> >>>> Technically, the gsbase register and the memory it points to belong to the >>>> pthread implementation on macOS. It's used for the pthread TLS implementation. >>>> However, study of the sources and experimentation reveal that TLS slot 6 >>>> (offset 0x30) is not currently used. Furthermore, Apple has promised to not >>>> use that slot in the future. So, we hijack it for our purposes. >>>> >>>> This removes the implementation of NtCurrentTeb() based on pthread_key_create() >>>> and pthread_set/getspecific(). >>>> >>>> Signed-off-by: Ken Thomases <ken(a)codeweavers.com> >>>> --- >>>> dlls/ntdll/ntdll.spec | 2 +- >>>> dlls/ntdll/signal_x86_64.c | 34 +++------------------------------- >>>> include/winnt.h | 2 +- >>>> 3 files changed, 5 insertions(+), 33 deletions(-) >>> >>> Nice to hear that Apple agreed to keep that slot reserved. Would it make sense to >>> keep the NtCurrentTeb export neverthless, for compatibility with libraries compiled >>> against old Wine? >> >> Hmm. I don't know. I hadn't considered it. What sorts of libraries? Did you have some use case in mind? I guess I'd be surprised if there were such libraries around. >> >> -Ken >> > > I'm not sure if there are any important examples, but all libraries / executables > compiled with the old Wine headers will have references to the NtCurrentTeb export. > If users should still be able to use any previously compiled code, it would be > necessary to keep them. Thats also why we decided to keep it in our Staging patchset. Previously compiled code other than Wine itself, right? I.e. winelib apps. Which call NtCurrentTeb(). Built for Mac 64-bit. I guess I don't think that's an important case, but if others disagree I can change the patch. -Ken

July 27, 2016

Re: [PATCH 1/3] ntdll: For Mac 64-bit, poke the TEB address to %gs:0x30 and re-enable the inlining of NtCurrentTeb().

by Sebastian Lackner

On 27.07.2016 18:30, Ken Thomases wrote: > On Jul 27, 2016, at 11:08 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: >> >> On 27.07.2016 17:42, Ken Thomases wrote: >>> 64-bit Windows apps have hard-coded accesses to %gs:0x30 baked into them. They >>> need to find the TEB self pointer there. >>> >>> Technically, the gsbase register and the memory it points to belong to the >>> pthread implementation on macOS. It's used for the pthread TLS implementation. >>> However, study of the sources and experimentation reveal that TLS slot 6 >>> (offset 0x30) is not currently used. Furthermore, Apple has promised to not >>> use that slot in the future. So, we hijack it for our purposes. >>> >>> This removes the implementation of NtCurrentTeb() based on pthread_key_create() >>> and pthread_set/getspecific(). >>> >>> Signed-off-by: Ken Thomases <ken(a)codeweavers.com> >>> --- >>> dlls/ntdll/ntdll.spec | 2 +- >>> dlls/ntdll/signal_x86_64.c | 34 +++------------------------------- >>> include/winnt.h | 2 +- >>> 3 files changed, 5 insertions(+), 33 deletions(-) >> >> Nice to hear that Apple agreed to keep that slot reserved. Would it make sense to >> keep the NtCurrentTeb export neverthless, for compatibility with libraries compiled >> against old Wine? > > Hmm. I don't know. I hadn't considered it. What sorts of libraries? Did you have some use case in mind? I guess I'd be surprised if there were such libraries around. > > -Ken > I'm not sure if there are any important examples, but all libraries / executables compiled with the old Wine headers will have references to the NtCurrentTeb export. If users should still be able to use any previously compiled code, it would be necessary to keep them. Thats also why we decided to keep it in our Staging patchset.

July 27, 2016

Re: [PATCH 1/3] ntdll: For Mac 64-bit, poke the TEB address to %gs:0x30 and re-enable the inlining of NtCurrentTeb().

by Ken Thomases

On Jul 27, 2016, at 11:08 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > > On 27.07.2016 17:42, Ken Thomases wrote: >> 64-bit Windows apps have hard-coded accesses to %gs:0x30 baked into them. They >> need to find the TEB self pointer there. >> >> Technically, the gsbase register and the memory it points to belong to the >> pthread implementation on macOS. It's used for the pthread TLS implementation. >> However, study of the sources and experimentation reveal that TLS slot 6 >> (offset 0x30) is not currently used. Furthermore, Apple has promised to not >> use that slot in the future. So, we hijack it for our purposes. >> >> This removes the implementation of NtCurrentTeb() based on pthread_key_create() >> and pthread_set/getspecific(). >> >> Signed-off-by: Ken Thomases <ken(a)codeweavers.com> >> --- >> dlls/ntdll/ntdll.spec | 2 +- >> dlls/ntdll/signal_x86_64.c | 34 +++------------------------------- >> include/winnt.h | 2 +- >> 3 files changed, 5 insertions(+), 33 deletions(-) > > Nice to hear that Apple agreed to keep that slot reserved. Would it make sense to > keep the NtCurrentTeb export neverthless, for compatibility with libraries compiled > against old Wine? Hmm. I don't know. I hadn't considered it. What sorts of libraries? Did you have some use case in mind? I guess I'd be surprised if there were such libraries around. -Ken

July 27, 2016

Re: [PATCH 3/3] ntdll: For Mac 64-bit, poke NtCurrentTeb()->ThreadLocalStoragePointer to the corresponding offset from %gs.

by Ken Thomases

On Jul 27, 2016, at 11:06 AM, Sebastian Lackner <sebastian(a)fds-team.de> wrote: > > On 27.07.2016 17:42, Ken Thomases wrote: >> 64-bit Windows apps have hard-coded accesses to %gs:0x58 baked into them. They >> need to find the ThreadLocalStoragePointer there. >> >> Technically, the gsbase register and the memory it points to belong to the >> pthread implementation on macOS. It's used for the pthread TLS implementation. >> Slot 11 (offset 0x58) is currently used for the implementation of the ttyname() >> system library function. We do not anticipate that Wine or any of the system >> libraries or frameworks it uses will call ttyname(). Furthermore, Apple has >> made it so that future releases of macOS will no longer use that slot. So, we >> hijack it for our purposes. >> >> Signed-off-by: Ken Thomases <ken(a)codeweavers.com> >> --- >> dlls/ntdll/loader.c | 11 +++++++- >> dlls/ntdll/signal_x86_64.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 72 insertions(+), 1 deletion(-) > > I'm not sure if I correctly understand the purpose of this patch. If you have to > set %gs:0x58 to some specific value, why not just use an assembly instruction for that? > alloc_tls_slot() is setting ThreadLocalStoragePointer for all threads, not just the current one. -Ken

July 27, 2016

Re: [PATCH 1/3] ntdll: For Mac 64-bit, poke the TEB address to %gs:0x30 and re-enable the inlining of NtCurrentTeb().

by Sebastian Lackner

On 27.07.2016 17:42, Ken Thomases wrote: > 64-bit Windows apps have hard-coded accesses to %gs:0x30 baked into them. They > need to find the TEB self pointer there. > > Technically, the gsbase register and the memory it points to belong to the > pthread implementation on macOS. It's used for the pthread TLS implementation. > However, study of the sources and experimentation reveal that TLS slot 6 > (offset 0x30) is not currently used. Furthermore, Apple has promised to not > use that slot in the future. So, we hijack it for our purposes. > > This removes the implementation of NtCurrentTeb() based on pthread_key_create() > and pthread_set/getspecific(). > > Signed-off-by: Ken Thomases <ken(a)codeweavers.com> > --- > dlls/ntdll/ntdll.spec | 2 +- > dlls/ntdll/signal_x86_64.c | 34 +++------------------------------- > include/winnt.h | 2 +- > 3 files changed, 5 insertions(+), 33 deletions(-) Nice to hear that Apple agreed to keep that slot reserved. Would it make sense to keep the NtCurrentTeb export neverthless, for compatibility with libraries compiled against old Wine?

July 27, 2016

Re: [PATCH 3/3] ntdll: For Mac 64-bit, poke NtCurrentTeb()->ThreadLocalStoragePointer to the corresponding offset from %gs.

by Sebastian Lackner

On 27.07.2016 17:42, Ken Thomases wrote: > 64-bit Windows apps have hard-coded accesses to %gs:0x58 baked into them. They > need to find the ThreadLocalStoragePointer there. > > Technically, the gsbase register and the memory it points to belong to the > pthread implementation on macOS. It's used for the pthread TLS implementation. > Slot 11 (offset 0x58) is currently used for the implementation of the ttyname() > system library function. We do not anticipate that Wine or any of the system > libraries or frameworks it uses will call ttyname(). Furthermore, Apple has > made it so that future releases of macOS will no longer use that slot. So, we > hijack it for our purposes. > > Signed-off-by: Ken Thomases <ken(a)codeweavers.com> > --- > dlls/ntdll/loader.c | 11 +++++++- > dlls/ntdll/signal_x86_64.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 72 insertions(+), 1 deletion(-) I'm not sure if I correctly understand the purpose of this patch. If you have to set %gs:0x58 to some specific value, why not just use an assembly instruction for that?

July 27, 2016

Re: [v2 PATCH] ucrtbase: Implement _get_narrow_winmain_command_line/_get_wide_winmain_command_line

by Dmitry Timoshkov

Piotr Caban <piotr.caban(a)gmail.com> wrote: > >>> Once again: > >>> Where did you get an idea of such an implementation? Is this copied from > >>> some place or based on some other implementation? Why for instance simple > >>> 'return GetCommandLineA()' is not enough as an initial implementation? > >> > >> Probably I'm just stating obvious but the function skips executable name > >> from the command line. That's why returning what GetCommandLineA returns > >> is incorrect. > > > > From what documentation/description is that obvious? That's not a common > > practice to send patches for not very obvious things without any tests. > > Why this implementation treats specially tabs and spaces but not other > > white space characters? Is that also obvious? How about the tests for > > quotes/embedded quotes? > > > I was not asking Nikolay for the tests because I think it's not easy to > write them. Some interesting tests needs executable name with ' ' in its > name. I also don't like the idea of starting many processes just to test > the function. On the other hand the initial test executable may be > started in many different ways (like e.g. passing first argument quoted > makes it impossible to check the return with simple comparison). > > That doesn't mean that I'm against adding a test or 2 for that. Anyway I > think this implementation is good after doing some tests myself. Testing on your own is good, however that doesn't count like an answer to my questions above which still left unanswered. If Wine development would follow this kind of quality control it wouldn't get that far as it is now. -- Dmitry.

July 27, 2016

Re: [v2 PATCH] ucrtbase: Implement _get_narrow_winmain_command_line/_get_wide_winmain_command_line

by Piotr Caban

On 07/27/16 15:29, Dmitry Timoshkov wrote: > Piotr Caban <piotr.caban(a)gmail.com> wrote: > >>> Once again: >>> Where did you get an idea of such an implementation? Is this copied from >>> some place or based on some other implementation? Why for instance simple >>> 'return GetCommandLineA()' is not enough as an initial implementation? >> >> Probably I'm just stating obvious but the function skips executable name >> from the command line. That's why returning what GetCommandLineA returns >> is incorrect. > > From what documentation/description is that obvious? That's not a common > practice to send patches for not very obvious things without any tests. > Why this implementation treats specially tabs and spaces but not other > white space characters? Is that also obvious? How about the tests for > quotes/embedded quotes? > I was not asking Nikolay for the tests because I think it's not easy to write them. Some interesting tests needs executable name with ' ' in its name. I also don't like the idea of starting many processes just to test the function. On the other hand the initial test executable may be started in many different ways (like e.g. passing first argument quoted makes it impossible to check the return with simple comparison). That doesn't mean that I'm against adding a test or 2 for that. Anyway I think this implementation is good after doing some tests myself.

July 27, 2016

Re: [PATCH 1/6] amstream: Fix AMAudioData::QueryInterface.

by Nikolay Sivov

On 27.07.2016 15:51, Anton Baskanov wrote: > Signed-off-by: Anton Baskanov <baskanov(a)gmail.com> > --- > dlls/amstream/audiodata.c | 1 - > dlls/amstream/tests/Makefile.in | 3 +- > dlls/amstream/tests/audiodata.c | 67 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 69 insertions(+), 2 deletions(-) > create mode 100755 dlls/amstream/tests/audiodata.c Hi, Anton. Please keep tests in existing amstream.c file. + > +static void test_query_interface(void) > +{ > + IUnknown *unknown = create_audio_data(); > + IMemoryData *memory_data = NULL; > + IAudioData *audio_data = NULL; > + > + HRESULT result = S_OK; result initialization is not needed; > + > + result = IUnknown_QueryInterface(unknown, &IID_IMemoryData, (void **)&memory_data); > + ok(E_NOINTERFACE == result, "got 0x%08x\n", result); > + if (S_OK == result) > + { > + IMemoryData_Release(memory_data); > + } If it always fails, there's no reason add conditional Release. > + > + result = IUnknown_QueryInterface(unknown, &IID_IAudioData, (void **)&audio_data); > + ok(S_OK == result, "got 0x%08x\n", result); > + if (S_OK == result) > + { > + IAudioData_Release(audio_data); > + } > + > + IUnknown_Release(unknown); > +} Could you check if you can query IMemoryData from IAudioData? It seems really weird if parent interface is not supported at all.

July 27, 2016

Re: [v2 PATCH] ucrtbase: Implement _get_narrow_winmain_command_line/_get_wide_winmain_command_line

by Dmitry Timoshkov

Piotr Caban <piotr.caban(a)gmail.com> wrote: > > Once again: > > Where did you get an idea of such an implementation? Is this copied from > > some place or based on some other implementation? Why for instance simple > > 'return GetCommandLineA()' is not enough as an initial implementation? > > Probably I'm just stating obvious but the function skips executable name > from the command line. That's why returning what GetCommandLineA returns > is incorrect. >From what documentation/description is that obvious? That's not a common practice to send patches for not very obvious things without any tests. Why this implementation treats specially tabs and spaces but not other white space characters? Is that also obvious? How about the tests for quotes/embedded quotes? -- Dmitry.

July 27, 2016

Re: MPR patches

by Pierre Schweitzer

Thanks for your review. I did a complete resubmission according to your review (3 patches already sent + the missing one). Le 27/07/2016 à 14:31, Hugh McMaster a écrit : > > On Wednesday, 27 July 2016, Pierre Schweitzer wrote: >> Regarding 1. & 3. should I perform a resend with that changed, or should >> I change it in the next patch I submit? >> >> For 2., this is done on purpose. Real implementation will come in a >> later patch (already ready and tested here :-)). > > Personally, I'd submit your real implementation of WNetCancelConnection2W > as patch 1 of 2. Then submit your implementation patch using the WCHAR * style > as the second patch. > > But if you decide to keep to your original order, please change the style before sending. > > You should also rebase your patches on the latest git. Your current patch > has an offset of -87 lines. > > As for your security certificate, although it doesn't prevent the patch from applying, > it would be better removed, if possible. > > Le 27/07/2016 à 13:40, Hugh McMaster a écrit : >> On Wednesday, 27 July 2016 6:40 AM, Pierre Schweitzer wrote: >> >>> Is there any reason why the MPR patches are left abandoned? There are >>> others awaiting in my queue... >> >>> + PWSTR name = strdupAtoW(lpName); >>> + if (!name) >>> + return ERROR_NOT_CONNECTED; >> >> 1. New code should use the WCHAR * style. >> >> 2. All three functions call WNetCancelConnection2W, but that function is a stub: >> >> DWORD WINAPI WNetCancelConnection2W( LPCWSTR lpName, DWORD dwFlags, BOOL fForce ) >> { >> FIXME( "(%s, %08X, %d), stub\n", debugstr_w(lpName), dwFlags, fForce ); >> >> return WN_SUCCESS; >> } >> >> 3. Your mail has a StartCom Secure Digital Certificate attached. This has been mangled, as can be seen at the bottom of your patch: http://source.winehq.org/patches/data/124723 > > -- Pierre Schweitzer <pierre at reactos.org> System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V.

July 27, 2016

RE: MPR patches

by Hugh McMaster

On Wednesday, 27 July 2016, Pierre Schweitzer wrote: > Regarding 1. & 3. should I perform a resend with that changed, or should > I change it in the next patch I submit? > > For 2., this is done on purpose. Real implementation will come in a > later patch (already ready and tested here :-)). Personally, I'd submit your real implementation of WNetCancelConnection2W as patch 1 of 2. Then submit your implementation patch using the WCHAR * style as the second patch. But if you decide to keep to your original order, please change the style before sending. You should also rebase your patches on the latest git. Your current patch has an offset of -87 lines. As for your security certificate, although it doesn't prevent the patch from applying, it would be better removed, if possible. Le 27/07/2016 à 13:40, Hugh McMaster a écrit : > On Wednesday, 27 July 2016 6:40 AM, Pierre Schweitzer wrote: > >> Is there any reason why the MPR patches are left abandoned? There are >> others awaiting in my queue... > >> + PWSTR name = strdupAtoW(lpName); >> + if (!name) >> + return ERROR_NOT_CONNECTED; > > 1. New code should use the WCHAR * style. > > 2. All three functions call WNetCancelConnection2W, but that function is a stub: > > DWORD WINAPI WNetCancelConnection2W( LPCWSTR lpName, DWORD dwFlags, BOOL fForce ) > { > FIXME( "(%s, %08X, %d), stub\n", debugstr_w(lpName), dwFlags, fForce ); > > return WN_SUCCESS; > } > > 3. Your mail has a StartCom Secure Digital Certificate attached. This has been mangled, as can be seen at the bottom of your patch: http://source.winehq.org/patches/data/124723

July 27, 2016

Re: MPR patches

by Pierre Schweitzer

Hi, Thanks for your feedback. Regarding 1. & 3. should I perform a resend with that changed, or should I change it in the next patch I submit? For 2., this is done on purpose. Real implementation will come in a later patch (already ready and tested here :-)). Le 27/07/2016 à 13:40, Hugh McMaster a écrit : > On Wednesday, 27 July 2016 6:40 AM, Pierre Schweitzer wrote: > >> Is there any reason why the MPR patches are left abandoned? There are >> others awaiting in my queue... > >> + PWSTR name = strdupAtoW(lpName); >> + if (!name) >> + return ERROR_NOT_CONNECTED; > > 1. New code should use the WCHAR * style. > > 2. All three functions call WNetCancelConnection2W, but that function is a stub: > > DWORD WINAPI WNetCancelConnection2W( LPCWSTR lpName, DWORD dwFlags, BOOL fForce ) > { > FIXME( "(%s, %08X, %d), stub\n", debugstr_w(lpName), dwFlags, fForce ); > > return WN_SUCCESS; > } > > 3. Your mail has a StartCom Secure Digital Certificate attached. This has been mangled, as can be seen at the bottom of your patch: http://source.winehq.org/patches/data/124723 > > Hugh > -- Pierre Schweitzer <pierre at reactos.org> System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V.

July 27, 2016

Re: MPR patches

by Hugh McMaster

On Wednesday, 27 July 2016 6:40 AM, Pierre Schweitzer wrote: > Is there any reason why the MPR patches are left abandoned? There are > others awaiting in my queue... > + PWSTR name = strdupAtoW(lpName); > + if (!name) > + return ERROR_NOT_CONNECTED; 1. New code should use the WCHAR * style. 2. All three functions call WNetCancelConnection2W, but that function is a stub: DWORD WINAPI WNetCancelConnection2W( LPCWSTR lpName, DWORD dwFlags, BOOL fForce ) { FIXME( "(%s, %08X, %d), stub\n", debugstr_w(lpName), dwFlags, fForce ); return WN_SUCCESS; } 3. Your mail has a StartCom Secure Digital Certificate attached. This has been mangled, as can be seen at the bottom of your patch: http://source.winehq.org/patches/data/124723 Hugh

July 27, 2016

Re: [v2 PATCH] ucrtbase: Implement _get_narrow_winmain_command_line/_get_wide_winmain_command_line

by Piotr Caban

On 07/26/16 04:32, Dmitry Timoshkov wrote: > Once again: > Where did you get an idea of such an implementation? Is this copied from > some place or based on some other implementation? Why for instance simple > 'return GetCommandLineA()' is not enough as an initial implementation? Probably I'm just stating obvious but the function skips executable name from the command line. That's why returning what GetCommandLineA returns is incorrect. Thanks, Piotr

July 27, 2016

Re: [PATCH] ucrtbase: Expose the fabsf function on arm

by Piotr Caban

Hi Martin, On 07/26/16 12:19, Martin Storsjo wrote: > It isn't available in the x86 or x64 versions of ucrtbase.dll > or api-ms-win-crt-math-l1-1-0.dll, but exists (and gets used by > MSVC 2015) on arm. > --- > dlls/api-ms-win-crt-math-l1-1-0/api-ms-win-crt-math-l1-1-0.spec | 1 + > dlls/ucrtbase/ucrtbase.spec | 1 + > 2 files changed, 2 insertions(+) you forgot to sign-off your patch. Except of that it looks good for me. Thanks, Piotr

July 27, 2016

Re: [PATCH 2/3 v2] kernel32/tests/loader: Add tests for fiber-local storage.

by Alexandre Julliard

John Sheu <sheu(a)google.com> writes: > Signed-off-by: John Sheu <sheu(a)google.com> > --- > dlls/kernel32/tests/loader.c | 118 ++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 116 insertions(+), 2 deletions(-) Could you please rebase it against recent changes, and resend with a fixed sequence number so that it can run through the testbot? -- Alexandre Julliard julliard(a)winehq.org

July 27, 2016

Re: WineHQ upgrade breaking dinput8:dinput?

by Bernhard Übelacker

Hello Francois, "DirectX Software Development Kit" offers in "DirectX Control Panel" to increase the "Debug Output Level". That way one can get another hint from dinput: DINPUT8: ERROR IDirectInput8::EnumDevicesBySemantics: arg 2: invalid UNICODE string First I tried to build a dinput8.dll that just dumps the data it gets from dinput-mini.exe, but could not find any difference. So I did some tests with DIACTIONA.lptszActionName member. When the two last characters get replaced by \0 no fault is reported anymore. That way a succeeding exe and the failing differ (nearly) just in the two replaced bytes in lptszActionName. Therefore can we assume that it is not the compilers fault, instead it uncovered a hidden bug either in the test or dinput? Kind regards, Bernhard dinput.c: - { 4, DIMOUSE_YAXIS, 0, { "Y Axis" } } + { 4, DIMOUSE_YAXIS, 0, { "Y Ax\0\0" } } Am 17.07.2016 um 21:02 schrieb Bernhard Übelacker: > Hello, > just tried to find out when this started exactly. > > > First by using old packages from snapshot.debian.org. > That revealed it started between packages: > gcc-mingw-w64-i686_4.6.4-4+9_i386.deb > gcc-mingw-w64-i686_4.8.2-5+10_i386.deb > > > A git bisect of gcc history points to this commit: > https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=ff2a5adaa72fef87cac689a40… > > > ff2a5adaa72fef87cac689a40c23258a30b304c8 is the first bad commit > commit ff2a5adaa72fef87cac689a40c23258a30b304c8 > Author: hubicka <hubicka(a)138bc75d-0d04-0410-961f-82ee72b054a4> > Date: Sun Apr 22 21:28:07 2012 +0000 > > * lto-symtab.c (lto_varpool_replace_node): Do not merge needed flags. > (...much longer commit message...) > > git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk(a)186687 138bc75d-0d04-0410-961f-82ee72b054a4 > > :040000 040000 d706bdeafbddb9f5f02474e39a245dca08f3334a 508082a31919fe9e907f70b3b3278d4f8d953fac M gcc > > > > Testbot runs: > Good: (gcc 62c34bf, last one before ff2a5ad) > https://testbot.winehq.org/JobDetails.pl?Key=24283 > Bad: (gcc ff2a5ad) > https://testbot.winehq.org/JobDetails.pl?Key=24280 > > > > Hope this could be of any help to identify the issue. > > > Kind regards, > Bernhard > > > Notes: > - Did not install gcc build system wide. Instead set the PATH > that i686-w64-mingw32-gcc was taken from there. > - Initial wine configure step was done using Jessie default > gcc-mingw-w64-i686 4.9.1-19+14.3. > Then the iterations were just done by removing objects > from dlls/dinput8/tests and make dinput8_crosstest.exe. > - GCC history before 1e37e371 did segfault while building, > therefore started from there. > > > > > Am 24.04.2016 um 20:30 schrieb Francois Gouget: >> >> I have rechecked this to make sure the test does not have a subtle bug >> that causes it to depend on the stack layout. >> >> >> In the process I have reduced the test to the minimum set necessary to >> reproduce the bug (see attachment). But in the end I did not find >> anything wrong so it does look like a compiler bug. >> >> I don't know how to proceed from there. >> >> * I think it would be better if someone more knowlegeable than me >> reported this bug to the MinGW guys. Maybe someone who could undertand >> what happens in the assembly code? >> >> * On the Wine side it would really be nice to get this test (and >> the three others that are impacted) to stop failing. But removing all >> the static directives really does not feel right. Would it be better >> to arrange for these tests to be compiled with -O0? Any other cleaner >> workaround. >> >> So I'm really hoping someone can step up and move this forward. >> >> >> Here are the relevant bugs: >> * dinput8:dinput regression caused by new compiler >> https://bugs.winehq.org/show_bug.cgi?id=40384 >> >> * usp10:usp10 regression caused by new cross-compiler >> https://bugs.winehq.org/show_bug.cgi?id=40385 >> >> * wininet:url regression caused by new cross-compiler >> https://bugs.winehq.org/show_bug.cgi?id=40386 >> >> * Unexplained new random comctl32:header failure (cross-compiler issue?) >> https://bugs.winehq.org/show_bug.cgi?id=40442 >> >> >> >> > > >

July 27, 2016

MPR patches

by Pierre Schweitzer

Hi, Is there any reason why the MPR patches are left abandoned? There are others awaiting in my queue... Cheers, -- Pierre Schweitzer <pierre at reactos.org> System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V.

July 26, 2016

Re: [PATCH 6/6] d3d11/tests: Add visual tests for locking vertexbuffers

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=24541 Your paranoid android. === wvistau64 (32 bit d3d11) === d3d11: unhandled exception 0000087c at 75A2E743 === w2008s64 (32 bit d3d11) === d3d11: unhandled exception 0000087c at 7680E743 === w7u (32 bit d3d11) === d3d11: unhandled exception c0000005 at 004022D6 === w7pro64 (32 bit d3d11) === d3d11: unhandled exception c0000005 at 004022D6 === w8 (32 bit d3d11) === d3d11: unhandled exception c0000005 at 004022D6 === w864 (32 bit d3d11) === d3d11: unhandled exception c0000005 at 004022D6 === w1064 (32 bit d3d11) === d3d11: unhandled exception c0000005 at 004022D6 === wvistau64 (64 bit d3d11) === d3d11: unhandled exception 0000087c at 000000007765674D === w2008s64 (64 bit d3d11) === d3d11: unhandled exception 0000087c at 0000000076EA674D === w7pro64 (64 bit d3d11) === d3d11: unhandled exception c0000005 at 000000000040217C === w864 (64 bit d3d11) === d3d11: unhandled exception c0000005 at 000000000040217C === w1064 (64 bit d3d11) === d3d11: unhandled exception c0000005 at 000000000040217C

July 26, 2016

Re: [PATCH 5/6] d3d10core/tests: Add visual tests for locking vertexbuffers

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=24540 Your paranoid android. === wvistau64 (32 bit device) === device: unhandled exception 00000879 at 75A2E743 === w2008s64 (32 bit device) === device: unhandled exception 00000879 at 7680E743 === w7u (32 bit device) === device: unhandled exception c0000005 at 00406895 === w7pro64 (32 bit device) === device: unhandled exception c0000005 at 00406895 === w8 (32 bit device) === device: unhandled exception c0000005 at 00406895 === w864 (32 bit device) === device: unhandled exception c0000005 at 00406895 === w1064 (32 bit device) === device: unhandled exception c0000005 at 00406895 === wvistau64 (64 bit device) === device: unhandled exception 00000879 at 000000007765674D === w2008s64 (64 bit device) === device: unhandled exception 00000879 at 0000000076EA674D === w7pro64 (64 bit device) === device: unhandled exception c0000005 at 0000000000405C40 === w864 (64 bit device) === device: unhandled exception c0000005 at 0000000000405C40 === w1064 (64 bit device) === device: unhandled exception c0000005 at 0000000000405C40

July 26, 2016

Re: [PATCH 4/6] ddraw/tests: Add visual tests for locking vertexbuffers

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=24539 Your paranoid android. === w8 (32 bit visual) === visual.c:1688: Test failed: Expected buffer mapped unsyncronized. Flags = DDLOCK_NOOVERWRITE. dwCaps = 0x10000. visual.c:1688: Test failed: Expected buffer mapped unsyncronized. Flags = DDLOCK_NOOVERWRITE | DDLOCK_DISCARDCONTENTS. dwCaps = 0x10000. visual.c:1688: Test failed: Expected buffer mapped unsyncronized. Flags = DDLOCK_NOOVERWRITE | DDLOCK_READONLY. dwCaps = 0x10000. === w864 (32 bit visual) === visual.c:307: Test failed: Clear rectangle 3(pos, neg) has color 0000ff00 visual.c:309: Test failed: Clear rectangle 1(pos, pos) has color 0000ff00 visual.c:311: Test failed: Clear rectangle 4(NULL) has color 0000ff00 visual.c:313: Test failed: Clear rectangle 4(neg, neg) has color 0000ff00 visual.c:1688: Test failed: Expected buffer mapped unsyncronized. Flags = DDLOCK_NOOVERWRITE. dwCaps = 0x10000. visual.c:1688: Test failed: Expected buffer mapped unsyncronized. Flags = DDLOCK_NOOVERWRITE | DDLOCK_DISCARDCONTENTS. dwCaps = 0x10000. visual.c:1688: Test failed: Expected buffer mapped unsyncronized. Flags = DDLOCK_NOOVERWRITE | DDLOCK_READONLY. dwCaps = 0x10000. === w1064 (32 bit visual) === visual.c:307: Test failed: Clear rectangle 3(pos, neg) has color 0000ff00 visual.c:309: Test failed: Clear rectangle 1(pos, pos) has color 0000ff00 visual.c:311: Test failed: Clear rectangle 4(NULL) has color 0000ff00 visual.c:313: Test failed: Clear rectangle 4(neg, neg) has color 0000ff00 visual.c:1688: Test failed: Expected buffer mapped unsyncronized. Flags = DDLOCK_NOOVERWRITE. dwCaps = 0x10000. visual.c:1688: Test failed: Expected buffer mapped unsyncronized. Flags = DDLOCK_NOOVERWRITE | DDLOCK_DISCARDCONTENTS. dwCaps = 0x10000. visual.c:1688: Test failed: Expected buffer mapped unsyncronized. Flags = DDLOCK_NOOVERWRITE | DDLOCK_READONLY. dwCaps = 0x10000.

July 26, 2016

Re: [PATCH] msxml2: Share source with msxml3

by Alexandre Julliard

Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> writes: > Since msxml2/4/6 all point to msxml3, having to install native > msxml3 will cause all the rest to fail. As an example, mdac28 > requires msxml3 to be installed (msxml3 has an incomplete IXMLParser > implementation, so native has to be used). I would change msxml4/6 as > well, so that installing any of the native msxml DLL wouldn't affect the > others. > > We could just use my previous patch, which would also resolve the issue > of registry entry. That scenario is > 1. Clean prefix. > 2. Install native msxml3 > 3. Force update of bottle (e.g. wine update) > The msxml2.DOMDocument registry entry is now incorrect. That's a corner case that I don't think justifies duplicating such a large dll 4 times. It should be easy to avoid by using native for the other ones too. And of course fixing builtin would be even better... -- Alexandre Julliard julliard(a)winehq.org

July 26, 2016

Re: [PATCH] msxml2: Share source with msxml3

by Alistair Leslie-Hughes

On 26/07/16 20:43, Alexandre Julliard wrote: > Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> writes: > >> Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> >> --- >> dlls/msxml2/Makefile.in | 49 ++++++++++++++++++++++++++++++++++++-- >> dlls/msxml2/main.c | 63 ------------------------------------------------- >> dlls/msxml2/msxml2.spec | 2 +- >> 3 files changed, 48 insertions(+), 66 deletions(-) >> delete mode 100644 dlls/msxml2/main.c > > Is there a real need for this? > yes and no. Since msxml2/4/6 all point to msxml3, having to install native msxml3 will cause all the rest to fail. As an example, mdac28 requires msxml3 to be installed (msxml3 has an incomplete IXMLParser implementation, so native has to be used). I would change msxml4/6 as well, so that installing any of the native msxml DLL wouldn't affect the others. We could just use my previous patch, which would also resolve the issue of registry entry. That scenario is 1. Clean prefix. 2. Install native msxml3 3. Force update of bottle (e.g. wine update) The msxml2.DOMDocument registry entry is now incorrect. (this would still fail if you want to create an Msxml2.DOMDocument.4.0 object) Best Regards Alistair Leslie-Hughes

July 26, 2016

Re: [PATCH] msxml2: Share source with msxml3

by Alexandre Julliard

Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> writes: > Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> > --- > dlls/msxml2/Makefile.in | 49 ++++++++++++++++++++++++++++++++++++-- > dlls/msxml2/main.c | 63 ------------------------------------------------- > dlls/msxml2/msxml2.spec | 2 +- > 3 files changed, 48 insertions(+), 66 deletions(-) > delete mode 100644 dlls/msxml2/main.c Is there a real need for this? -- Alexandre Julliard julliard(a)winehq.org

July 26, 2016

Re: [PATCH 3/8] wined3d: Implement SM5 deriv_rtx_coarse instruction.

by Józef Kucia

On Tue, Jul 26, 2016 at 1:41 AM, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: > What about using the generic dFdx()/dFdy() builtins (maybe with a > WARN) for the coarse and fine variants when ARB_derivative_control > isn't supported? > I don't know that it's particularly relevant in practice, it just > seems something we don't need to strictly depend on. I have no idea if this is issue in practice but wined3d currently requires ARB_derivative_control for SM5. I guess we can add a fallback to dFdx()/dFdy() if we find an OpenGL implementation which is otherwise capable to support SM5 but doesn't expose ARB_derivative_control.

July 26, 2016

Re: [PATCH 3/8] wined3d: Implement SM5 deriv_rtx_coarse instruction.

by Józef Kucia

On Mon, Jul 25, 2016 at 12:48 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > It doesn't matter that much, but is there any reason > shader_glsl_map2gl() can't handle these? I've introduced shader_glsl_derivative() mostly to make the check for ARB_derivative_control simpler. I could have reordered patches as well to avoid introducing shader_glsl_derivative() at all, but I wasn't sure if someone won't be in favor of replacing the patch 124699 - "wined3d: Require ARB_derivative_control for SM5." with a fallback to dFdx()/dFdy().

July 26, 2016

Re: [v2 PATCH] ucrtbase: Implement _get_narrow_winmain_command_line/_get_wide_winmain_command_line

by Dmitry Timoshkov

Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > +char* CDECL _get_narrow_winmain_command_line(void) > +{ > + static char *narrow_command_line; > + char *s; > + > + if (narrow_command_line) > + return narrow_command_line; > + > + s = GetCommandLineA(); > + if (*s == '"') > + { > + /* skip all leading quotes */ > + while (*++s == '"') > + ; > + /* skip everything up to next quote */ > + while (*s && *s++ != '"') > + ; > + } > + else > + { > + while (*s && *s != ' ' && *s != '\t') > + { > + /* allow embedded quoted blocks */ > + if (*s == '"') > + { > + s++; > + while (*s && *s++ != '"') > + ; > + } > + else > + s++; > + } > + } > + while (*s == ' ' || *s == '\t') > + s++; > + > + return narrow_command_line = s; > +} Once again: Where did you get an idea of such an implementation? Is this copied from some place or based on some other implementation? Why for instance simple 'return GetCommandLineA()' is not enough as an initial implementation? -- Dmitry.

July 26, 2016

Re: [PATCH 3/8] wined3d: Implement SM5 deriv_rtx_coarse instruction.

by Matteo Bruni

2016-07-22 12:28 GMT+02:00 Józef Kucia <jkucia(a)codeweavers.com>: > + if (!gl_info->supported[ARB_DERIVATIVE_CONTROL]) > + { > + FIXME("OpenGL implementation does not support ARB_derivative_control.\n"); > + return; > + } What about using the generic dFdx()/dFdy() builtins (maybe with a WARN) for the coarse and fine variants when ARB_derivative_control isn't supported? I don't know that it's particularly relevant in practice, it just seems something we don't need to strictly depend on.

July 25, 2016

Re: d3dx9_36: Fix a typo in TRACE() message.

by Matteo Bruni

2016-07-25 10:24 GMT+02:00 Andrey Gusev <andrey.goosev(a)gmail.com>: > > + TRACE("pout %p, pv %p, pviewport %p, pprojection %p, pview %p, pworld %p\n", pout, pv, pviewport, pprojection, pview, pworld); While you're there, can you please clean up the function? Like dropping all the 'p' prefixes from the function arguments and using the current d3d style for the function body. Also the TRACE line seems a bit too long.

July 25, 2016

Re: include: Fix prototypes of D3DXFillXXXTextureTx for d3dx9.

by Matteo Bruni

2016-07-21 11:06 GMT+02:00 Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>: > -HRESULT WINAPI D3DXFillTextureTX(struct IDirect3DTexture9 *texture, const DWORD *function, > - const D3DXVECTOR4 *constants, UINT numconstants); > -HRESULT WINAPI D3DXFillCubeTextureTX(struct IDirect3DCubeTexture9 *cube, const DWORD *function, > - const D3DXVECTOR4 *constants, UINT numconstants); > -HRESULT WINAPI D3DXFillVolumeTextureTX(struct IDirect3DVolumeTexture9 *volume, const DWORD *function, > - const D3DXVECTOR4 *constants, UINT numconstants); > +HRESULT WINAPI D3DXFillTextureTX(struct IDirect3DTexture9 *texture, ID3DXTextureShader *texture_shader); > +HRESULT WINAPI D3DXFillCubeTextureTX(struct IDirect3DCubeTexture9 *cube, ID3DXTextureShader *texture_shader); > +HRESULT WINAPI D3DXFillVolumeTextureTX(struct IDirect3DVolumeTexture9 *volume, ID3DXTextureShader *texture_shader); Do you know where the old prototypes came from? In all the versions of the header I have around (which go back to _30) I see the "new" prototype but it's possible that it changed even further back. It's also possible these have always been wrong, although that seems somewhat unlikely to me.

July 25, 2016

Re: [PATCH] d3dx9: Implement D3DXCreateTextureShader with stubbed ID3DXTextureShader interface.

by Matteo Bruni

2016-07-21 11:06 GMT+02:00 Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>: > +HRESULT WINAPI D3DXCreateTextureShader(const DWORD *function, ID3DXTextureShader **texture_shader) > +{ > + struct d3dx9_texture_shader *object; > + > + TRACE("function %p, texture_shader %p.\n", function, texture_shader); > + > + if (!function || !texture_shader) > + return D3DERR_INVALIDCALL; > + > + object = HeapAlloc(GetProcessHeap(), 0, sizeof(*object)); > + if (!object) > + return E_OUTOFMEMORY; > + > + object->ID3DXTextureShader_iface.lpVtbl = &d3dx9_texture_shader_vtbl; > + object->ref = 1; > + > + *texture_shader = &object->ID3DXTextureShader_iface; > + > + return D3D_OK; > +} It would be nice to have a test for this.

July 25, 2016

Re: [PATCH] d3dx10: Add D3DXCpuOptimizations stub

by Matteo Bruni

2016-07-20 7:34 GMT+02:00 Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>: > On 20/07/16 15:04, Alex Henrie wrote: >> + * Copyright (C) 2007 David Adam >> + * Copyright (C) 2007 Tony Wasserka >> >> Did these people actually contribute anything to this patch? > No, it was a copy from d3dx9math.h > >> Also, D3DX_CPU_OPTIMIZATION was already defined in d3dx9_30_main.c. I >> would remove the old definition from d3dx9_30_main.c and make it >> include d3dx10math.h instead. >> > To remove the D3DX_CPU_OPTIMIZATION definition would be a separate patch > in it's own right. I will have to workout where it should be > defined for d3dx9. Including a dx10 header in dx9 seems like a bad idea. Yeah. D3DX_CPU_OPTIMIZATION is not defined in "recent" d3dx9 headers AFAICS so it's probably fine as it is. > > Best Regards > Alistair Leslie-Hughes

July 25, 2016

Re: [v3 PATCH] d3dx10: Add D3DXCpuOptimizations stub

by Matteo Bruni

2016-07-20 11:20 GMT+02:00 Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com>: Hi Alistair, I'm afraid this is going to be a bit nitpicky. It also means the patch is mostly okay though. > Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> > --- > dlls/d3dx10_43/d3dx10_43.spec | 2 +- > dlls/d3dx10_43/d3dx10_43_main.c | 11 +++++++-- > include/Makefile.in | 1 + > include/d3dx10.h | 1 + > include/d3dx10math.h | 49 +++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 61 insertions(+), 3 deletions(-) > create mode 100644 include/d3dx10math.h > > diff --git a/dlls/d3dx10_43/d3dx10_43.spec b/dlls/d3dx10_43/d3dx10_43.spec > index ab696e8..f410be8 100644 > --- a/dlls/d3dx10_43/d3dx10_43.spec > +++ b/dlls/d3dx10_43/d3dx10_43.spec > @@ -70,7 +70,7 @@ > @ stdcall D3DXColorAdjustSaturation(ptr ptr float) d3dx9_36.D3DXColorAdjustSaturation > @ stdcall D3DXComputeBoundingBox(ptr long long ptr ptr) d3dx9_36.D3DXComputeBoundingBox > @ stdcall D3DXComputeBoundingSphere(ptr long long ptr ptr) d3dx9_36.D3DXComputeBoundingSphere > -@ stub D3DXCpuOptimizations(long) > +@ stdcall D3DXCpuOptimizations(long) > @ stdcall D3DXCreateMatrixStack(long ptr) d3dx9_36.D3DXCreateMatrixStack > @ stdcall D3DXFloat16To32Array(ptr ptr long) d3dx9_36.D3DXFloat16To32Array > @ stdcall D3DXFloat32To16Array(ptr ptr long) d3dx9_36.D3DXFloat32To16Array > diff --git a/dlls/d3dx10_43/d3dx10_43_main.c b/dlls/d3dx10_43/d3dx10_43_main.c > index 0c1ff80..408cf22 100644 > --- a/dlls/d3dx10_43/d3dx10_43_main.c > +++ b/dlls/d3dx10_43/d3dx10_43_main.c > @@ -34,8 +34,8 @@ > #include "objbase.h" > > #include "d3d10_1.h" > -#include "d3dx10core.h" > -#include "d3dx10tex.h" > +#include "d3dx10.h" > + > > WINE_DEFAULT_DEBUG_CHANNEL(d3dx); No reason to add double whitespace lines here. > > @@ -210,3 +210,10 @@ HRESULT WINAPI D3DX10GetImageInfoFromMemory(const void *src_data, SIZE_T src_dat > > return E_NOTIMPL; > } > + > +D3DX_CPU_OPTIMIZATION WINAPI D3DXCpuOptimizations(BOOL enable) > +{ > + FIXME("enable %d stub.\n", enable); Can you please use %#x when tracing BOOL arguments? > +#include "d3dx10.h" > + > +/* This guard is the same as D3DX9 to prevent double-inclusion */ > +#ifndef __D3DX9MATH_H__ > +#define __D3DX9MATH_H__ > + > +#include <math.h> > + > + > +typedef enum _D3DX_CPU_OPTIMIZATION > +{ > + D3DX_NOT_OPTIMIZED = 0, > + D3DX_3DNOW_OPTIMIZED = 1, > + D3DX_SSE2_OPTIMIZED = 2, > + D3DX_SSE_OPTIMIZED = 3 > +} D3DX_CPU_OPTIMIZATION; I would go the other way and drop all the initializers instead. It's fine either way though. Also double whitespace again. > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +D3DX_CPU_OPTIMIZATION WINAPI D3DXCpuOptimizations(BOOL enable); > + > +#ifdef __cplusplus > +} > +#endif > + > + > +/*#include <d3dx10math.inl> */ I don't like this. Either introduce d3dx10math.inl and make this an actual include (preferred) or just remove the line for the time being.

July 25, 2016

Re: [PATCH] ucrtbase: Implement _get_narrow_winmain_command_line/_get_wide_winmain_command_line

by Nikolay Sivov

On 25.07.2016 16:13, Piotr Caban wrote: > On 07/24/16 20:40, Nikolay Sivov wrote: >> +/********************************************************************* >> + * _get_narrow_winmain_command_line (UCRTBASE.@) >> + */ >> +char* CDECL _get_narrow_winmain_command_line(void) >> +{ >> + static char *narrow_command_line; >> + char *s; >> + >> + if (narrow_command_line) >> + return narrow_command_line; >> + >> + s = GetCommandLineA(); >> + if (*s == '"') >> + { >> + s++; >> + while (*s) >> + if (*s++ == '"') >> + break; >> + } >> + else >> + { >> + while (*s && *s != ' ' && *s != '\t') >> + s++; >> + } >> + while (*s == ' ' || *s == '\t') >> + s++; >> + >> + return narrow_command_line = s; >> +} > It will not work in following case (executable name "test ws.exe"): > CMD> test" "ws.exe other args > or > CMD> """test ws.exe" other args > Probably there's no real use case for that but it shouldn't be hard to > handle this cases as well. > Thanks for the review, I just sent improved version.

July 25, 2016

Re: [v2] ntdll: Keep drive letter size in RtlGetFullPathName_U when full path is specified

by Alexandre Julliard

Piotr Caban <piotr(a)codeweavers.com> writes: > v2: > - fix cmd test failure Looks like there's another one I missed: ../../../tools/runtest -q -P wine -T ../../.. -M shell32.dll -p shell32_test.exe.so shelllink && touch shelllink.ok shelllink.c:251: Test failed: case doesn't match make: *** [shelllink.ok] Error 1 -- Alexandre Julliard julliard(a)winehq.org

July 25, 2016

Re: [PATCH] msxml2: Let msxml3 register Msxml2.DOMDocument

by Alexandre Julliard

Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> writes: > When msxml3 is native, msxml2 forwards to msxml3 which then fails > to create an object. On Windows 8.1, Msxml2.DOMDocument > point to Msxml2.DOMDocument.3.0. That should be the case on Wine too. It sounds like there's something broken with your config. Note that mixing native and builtin is not a good idea. -- Alexandre Julliard julliard(a)winehq.org

July 25, 2016

Re: [PATCH] ucrtbase: Implement _get_narrow_winmain_command_line/_get_wide_winmain_command_line

by Piotr Caban

On 07/24/16 20:40, Nikolay Sivov wrote: > +/********************************************************************* > + * _get_narrow_winmain_command_line (UCRTBASE.@) > + */ > +char* CDECL _get_narrow_winmain_command_line(void) > +{ > + static char *narrow_command_line; > + char *s; > + > + if (narrow_command_line) > + return narrow_command_line; > + > + s = GetCommandLineA(); > + if (*s == '"') > + { > + s++; > + while (*s) > + if (*s++ == '"') > + break; > + } > + else > + { > + while (*s && *s != ' ' && *s != '\t') > + s++; > + } > + while (*s == ' ' || *s == '\t') > + s++; > + > + return narrow_command_line = s; > +} It will not work in following case (executable name "test ws.exe"): CMD> test" "ws.exe other args or CMD> """test ws.exe" other args Probably there's no real use case for that but it shouldn't be hard to handle this cases as well. Thanks, Piotr

July 25, 2016

Re: [PATCH 02/03 v4 ] ntoskrnl.exe: Track drivers created with IoCreateDriver

by Aric Stewart

Thanks! New version sent! -aric On 7/24/16 1:05 PM, Thomas Faber wrote: >> NTSTATUS WINAPI IoCreateDriver( UNICODE_STRING *name, PDRIVER_INITIALIZE init ) >> { >> - DRIVER_OBJECT *driver; >> - DRIVER_EXTENSION *extension; >> + struct wine_driver *driver; >> NTSTATUS status; >> >> TRACE("(%s, %p)\n", debugstr_us(name), init); >> >> if (!(driver = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, >> - sizeof(*driver) + sizeof(*extension) ))) >> + sizeof(*driver) ))) >> return STATUS_NO_MEMORY; >> >> - if ((status = RtlDuplicateUnicodeString( 1, name, &driver->DriverName ))) >> + if ((status = RtlDuplicateUnicodeString( 1, name, &driver->driver_obj.DriverName))) > > Missing space before the closing parenthesis here. ;p > >> { >> RtlFreeHeap( GetProcessHeap(), 0, driver ); >> return status; >> } > > > >> @@ -892,13 +959,18 @@ NTSTATUS WINAPI IoCreateDriver( UNICODE_STRING *name, PDRIVER_INITIALIZE init ) >> /*********************************************************************** >> * IoDeleteDriver (NTOSKRNL.EXE.@) >> */ >> -void WINAPI IoDeleteDriver( DRIVER_OBJECT *driver ) >> +void WINAPI IoDeleteDriver( DRIVER_OBJECT *driver_object ) >> { >> - TRACE("(%p)\n", driver); >> + TRACE("(%p)\n", driver_object); >> + >> + EnterCriticalSection( &drivers_cs ); >> + if (wine_drivers) >> + wine_rb_remove( wine_drivers, &driver_object->DriverName ); >> + LeaveCriticalSection( &drivers_cs ); >> >> - RtlFreeUnicodeString( &driver->DriverName ); >> - RtlFreeUnicodeString( &driver->DriverExtension->ServiceKeyName ); >> - RtlFreeHeap( GetProcessHeap(), 0, driver ); >> + RtlFreeUnicodeString( &driver_object->DriverName ); >> + RtlFreeUnicodeString( &driver_object->DriverExtension->ServiceKeyName ); >> + RtlFreeHeap( GetProcessHeap(), 0, driver_object ); >> } > > You need to free the wine_driver now rather than the DRIVER_OBJECT, > i.e. use CONTAINING_RECORD. > > > Thanks, > Thomas > >

July 25, 2016

Re: [PATCH 3/8] wined3d: Implement SM5 deriv_rtx_coarse instruction.

by Henri Verbeet

On 22 July 2016 at 12:28, Józef Kucia <jkucia(a)codeweavers.com> wrote: > +static void shader_glsl_derivative(const struct wined3d_shader_instruction *ins) > +{ > + const struct wined3d_gl_info *gl_info = ins->ctx->gl_info; > + struct wined3d_string_buffer *buffer = ins->ctx->buffer; > + struct glsl_src_param src_param; > + const char *instruction; > + DWORD write_mask; > + > + if (!gl_info->supported[ARB_DERIVATIVE_CONTROL]) > + { > + FIXME("OpenGL implementation does not support ARB_derivative_control.\n"); > + return; > + } > + > + switch (ins->handler_idx) > + { > + case WINED3DSIH_DSX_COARSE: instruction = "dFdxCoarse"; break; > + default: ERR("Unhandled opcode %#x.\n", ins->handler_idx); return; > + } > + > + write_mask = shader_glsl_append_dst(buffer, ins); > + shader_glsl_add_src_param(ins, &ins->src[0], write_mask, &src_param); > + shader_addline(buffer, "%s(%s));\n", instruction, src_param.param_str); > +} It doesn't matter that much, but is there any reason shader_glsl_map2gl() can't handle these?

July 25, 2016

Re: [PATCH 1/4] dwrite: Implement newer SetLineSpacing/GetLineSpacing

by Nikolay Sivov

On 25.07.2016 11:53, 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=24498 > > Your paranoid android. > > > === wvistau64 (32 bit layout) === > The test timed out > Doesn't fail when run manually http://testbot.winehq.org/JobDetails.pl?Key=24499.

July 25, 2016

Re: [PATCH 1/4] dwrite: Implement newer SetLineSpacing/GetLineSpacing

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=24498 Your paranoid android. === wvistau64 (32 bit layout) === The test timed out

July 25, 2016

Re: urlmon: Remove double riid parameter.

by Nikolay Sivov

> - hres = IServiceProvider_QueryService(This->serv_prov, riid, riid, &ret); > + hres = IServiceProvider_QueryService(This->serv_prov, riid, &ret); This won't build.

July 25, 2016

Re: [PATCH v2 2/3] shell32: Implement FolderItems_Item.

by Alex Henrie

2016-07-22 0:13 GMT-06:00 Nikolay Sivov <bunglehead(a)gmail.com>: > Is it possible we'll need special folder indices here as well? I didn't > check thoroughly. You're right, if the index type is VT_ERROR, FolderItems::Item returns a FolderItem for the containing folder regardless of what the index value is. This special behavior is tested and implemented in v3. > This is assuming you always have VT_I4 on input, but it's obviously > possible to call it with any type. It turns out that FolderItems::Item returns S_OK for VT_I2, V_I4, and V_ERROR, S_FALSE for VT_BSTR, and E_NOTIMPL for everything else. This behavior is tested and implemented in v3. > 'dir' is not necessary VT_BSTR. In v3, Folder::Items returns E_NOTIMPL if dir is VT_I4, the same as Folder::get_Title. -Alex

July 25, 2016

Re: [PATCH] ucrtbase: Implement _get_narrow_winmain_command_line/_get_wide_winmain_command_line

by Dmitry Timoshkov

Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > +char* CDECL _get_narrow_winmain_command_line(void) > +{ > + static char *narrow_command_line; > + char *s; > + > + if (narrow_command_line) > + return narrow_command_line; > + > + s = GetCommandLineA(); > + if (*s == '"') > + { > + s++; > + while (*s) > + if (*s++ == '"') > + break; > + } > + else > + { > + while (*s && *s != ' ' && *s != '\t') > + s++; > + } > + while (*s == ' ' || *s == '\t') > + s++; > + > + return narrow_command_line = s; > +} Where did you get an idea of sucn an implementation? -- Dmitry.

July 25, 2016

Re: [PATCH 02/03 v4 ] ntoskrnl.exe: Track drivers created with IoCreateDriver

by Thomas Faber

> NTSTATUS WINAPI IoCreateDriver( UNICODE_STRING *name, PDRIVER_INITIALIZE init ) > { > - DRIVER_OBJECT *driver; > - DRIVER_EXTENSION *extension; > + struct wine_driver *driver; > NTSTATUS status; > > TRACE("(%s, %p)\n", debugstr_us(name), init); > > if (!(driver = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY, > - sizeof(*driver) + sizeof(*extension) ))) > + sizeof(*driver) ))) > return STATUS_NO_MEMORY; > > - if ((status = RtlDuplicateUnicodeString( 1, name, &driver->DriverName ))) > + if ((status = RtlDuplicateUnicodeString( 1, name, &driver->driver_obj.DriverName))) Missing space before the closing parenthesis here. ;p > { > RtlFreeHeap( GetProcessHeap(), 0, driver ); > return status; > } > @@ -892,13 +959,18 @@ NTSTATUS WINAPI IoCreateDriver( UNICODE_STRING *name, PDRIVER_INITIALIZE init ) > /*********************************************************************** > * IoDeleteDriver (NTOSKRNL.EXE.@) > */ > -void WINAPI IoDeleteDriver( DRIVER_OBJECT *driver ) > +void WINAPI IoDeleteDriver( DRIVER_OBJECT *driver_object ) > { > - TRACE("(%p)\n", driver); > + TRACE("(%p)\n", driver_object); > + > + EnterCriticalSection( &drivers_cs ); > + if (wine_drivers) > + wine_rb_remove( wine_drivers, &driver_object->DriverName ); > + LeaveCriticalSection( &drivers_cs ); > > - RtlFreeUnicodeString( &driver->DriverName ); > - RtlFreeUnicodeString( &driver->DriverExtension->ServiceKeyName ); > - RtlFreeHeap( GetProcessHeap(), 0, driver ); > + RtlFreeUnicodeString( &driver_object->DriverName ); > + RtlFreeUnicodeString( &driver_object->DriverExtension->ServiceKeyName ); > + RtlFreeHeap( GetProcessHeap(), 0, driver_object ); > } You need to free the wine_driver now rather than the DRIVER_OBJECT, i.e. use CONTAINING_RECORD. Thanks, Thomas

July 24, 2016

Re: [PATCH 2/2] DInput : Use vendor and product ids to generate product GUID like it's done on Windows

by Corentin Rossignon

Done, thanks for taking some times and helping me doing a proper patch. On 07/24/2016 12:03 AM, Bruno Jesus wrote: > On Sat, Jul 23, 2016 at 8:01 AM, Corentin Rossignon <corossig(a)gmail.com> wrote: >> Signed-off-by: Corentin Rossignon <corossig(a)gmail.com> >> --- >> dlls/dinput/joystick_linux.c | 34 +++++++++++++++++++++++++++++++--- >> dlls/dinput/joystick_linuxinput.c | 27 +++++++++++++++++++++++++-- >> 2 files changed, 56 insertions(+), 5 deletions(-) > Formatting problem: > > bruno(a)Note:~/programs/wine$ git apply dinput2 > dinput2:31: trailing whitespace. > /* > dinput2:35: trailing whitespace. > * Data4 seems to be a constant. > dinput2:104: trailing whitespace. > /* > dinput2:108: trailing whitespace. > * Data4 seems to be a constant. > warning: 4 lines add whitespace errors. > >> diff --git a/dlls/dinput/joystick_linux.c b/dlls/dinput/joystick_linux.c >> index 90b7280..f8499ca 100644 >> --- a/dlls/dinput/joystick_linux.c >> +++ b/dlls/dinput/joystick_linux.c >> @@ -77,6 +77,7 @@ struct JoyDev >> { >> char device[MAX_PATH]; >> char name[MAX_PATH]; >> + GUID guid_product; >> >> BYTE axis_count; >> BYTE button_count; >> @@ -122,6 +123,21 @@ static const GUID DInput_Wine_Joystick_GUID = { /* 9e573ed9-7734-11d2-8d4a-23903 >> {0x8d, 0x4a, 0x23, 0x90, 0x3f, 0xb6, 0xbd, 0xf7} >> }; >> >> + >> +/* >> + * Construct the GUID in the same way of Windows doing this. >> + * Data1 is concatenation of productid and vendorid. >> + * Data2 and Data3 are NULL. >> + * Data4 seems to be a constant. >> + */ >> +static const GUID DInput_Wine_Joystick_Constant_Part_GUID = { >> + 0x000000000, >> + 0x0000, >> + 0x0000, >> + {0x00, 0x00, 0x50, 0x49, 0x44, 0x56, 0x49, 0x44} >> +}; >> + >> + > Please remove all double blank lines in your changes in order to be > more compatible with the surrounding code style.

July 24, 2016

Re: [PATCH 2/2] DInput : Use vendor and product ids to generate product GUID like it's done on Windows

by Bruno Jesus

On Sat, Jul 23, 2016 at 8:01 AM, Corentin Rossignon <corossig(a)gmail.com> wrote: > Signed-off-by: Corentin Rossignon <corossig(a)gmail.com> > --- > dlls/dinput/joystick_linux.c | 34 +++++++++++++++++++++++++++++++--- > dlls/dinput/joystick_linuxinput.c | 27 +++++++++++++++++++++++++-- > 2 files changed, 56 insertions(+), 5 deletions(-) Formatting problem: bruno(a)Note:~/programs/wine$ git apply dinput2 dinput2:31: trailing whitespace. /* dinput2:35: trailing whitespace. * Data4 seems to be a constant. dinput2:104: trailing whitespace. /* dinput2:108: trailing whitespace. * Data4 seems to be a constant. warning: 4 lines add whitespace errors. > diff --git a/dlls/dinput/joystick_linux.c b/dlls/dinput/joystick_linux.c > index 90b7280..f8499ca 100644 > --- a/dlls/dinput/joystick_linux.c > +++ b/dlls/dinput/joystick_linux.c > @@ -77,6 +77,7 @@ struct JoyDev > { > char device[MAX_PATH]; > char name[MAX_PATH]; > + GUID guid_product; > > BYTE axis_count; > BYTE button_count; > @@ -122,6 +123,21 @@ static const GUID DInput_Wine_Joystick_GUID = { /* 9e573ed9-7734-11d2-8d4a-23903 > {0x8d, 0x4a, 0x23, 0x90, 0x3f, 0xb6, 0xbd, 0xf7} > }; > > + > +/* > + * Construct the GUID in the same way of Windows doing this. > + * Data1 is concatenation of productid and vendorid. > + * Data2 and Data3 are NULL. > + * Data4 seems to be a constant. > + */ > +static const GUID DInput_Wine_Joystick_Constant_Part_GUID = { > + 0x000000000, > + 0x0000, > + 0x0000, > + {0x00, 0x00, 0x50, 0x49, 0x44, 0x56, 0x49, 0x44} > +}; > + > + Please remove all double blank lines in your changes in order to be more compatible with the surrounding code style.

July 23, 2016

Re: [PATCH 1/2] DInput : Add vendor_id and product_id with Linux joystick API

by Bruno Jesus

On Sat, Jul 23, 2016 at 8:01 AM, Corentin Rossignon <corossig(a)gmail.com> wrote: > Signed-off-by: Corentin Rossignon <corossig(a)gmail.com> > --- > dlls/dinput/joystick_linux.c | 35 ++++++++++++++++++++++++++++++++++- > 1 file changed, 34 insertions(+), 1 deletion(-) Thanks for the patch, I just tested and they work with my gamepads. Since you will have to fix the problems below I suggest that you resend the 2 patches with subjects: [PATCH 1/2] dinput: Retrieve vendor ID and product ID in Linux joystick API [PATCH 2/2] dinput: Fix product GUID generation in Linux joystick and event API Formatting problem: bruno(a)Note:~/programs/wine$ git apply dinput1 dinput1:19: trailing whitespace. dinput1:68: trailing whitespace. } warning: 2 lines add whitespace errors. > + /* Find vendor_id and product_id in sysfs */ > + joydev.vendor_id = 0; > + joydev.product_id = 0; > + > + snprintf(sys_path, MAX_PATH, "/sys/class/input/js%d/device/id/vendor", i); > + sys_fd = open(sys_path, O_RDONLY); > + if (sys_path > 0) > + { Wrong variable in the condition. Best Regards, Bruno

July 23, 2016

Re: [PATCH] dinput : use vendor and product ids to generate product GUID like it's done on Windows - second try

by Corentin Rossignon

You're right, I need to do the same for JS device. I submitted a set of two patches, the first one for getting device_id and product_id with JS device, the second one for using these ids to make the product GUID. I also put the constant part in a global GUID and I used MAKELONG for aggregating device_id and product_id. Regards, Corentin. On 07/23/2016 01:30 AM, Bruno Jesus wrote: > On Fri, Jul 22, 2016 at 3:31 AM, Corentin Rossignon <corossig(a)gmail.com> wrote: >> Signed-off-by: Corentin Rossignon <corossig(a)gmail.com> >> --- >> dlls/dinput/joystick_linuxinput.c | 26 ++++++++++++++++++++++++-- >> 1 file changed, 24 insertions(+), 2 deletions(-) > Hi, I just tested in Windows and indeed what you intend seems correct. > The PID/VID matches with what Windows uses: > > //Windows > guidProduct : {68750603-0000-0000-0000-504944564944} --- Generic HID gamepad > guidProduct : {0268054C-0000-0000-0000-504944564944} --- PS2 gamepad + > usb adapter > > //Wine > guidProduct : {9E573ED9-7734-11D2-8D4A-23903FB6BDF7} > guidProduct : {9E573ED9-7734-11D2-8D4A-23903FB6BDF7} > > But this patch does not work for me, my joysticks are detected in > joystick_linux.c not joystick_linuxinput.c. I believe the change is > correct but I can't confirm it, a similar patch for joystick_linux.c > would be needed. > >> diff --git a/dlls/dinput/joystick_linuxinput.c b/dlls/dinput/joystick_linuxinput.c >> index 86f12d0..3f9c241 100644 >> --- a/dlls/dinput/joystick_linuxinput.c >> +++ b/dlls/dinput/joystick_linuxinput.c >> @@ -96,6 +96,7 @@ struct JoyDev { >> char *device; >> char *name; >> GUID guid; >> + GUID guid_product; >> >> BOOL has_ff; >> int num_effects; >> @@ -293,11 +294,32 @@ static void find_joydevs(void) >> } >> >> if (ioctl(fd, EVIOCGID, &device_id) == -1) >> + { >> WARN("ioctl(EVIOCGID) failed: %d %s\n", errno, strerror(errno)); >> + joydev.guid_product = DInput_Wine_Joystick_Base_GUID; >> + } >> else >> { >> joydev.vendor_id = device_id.vendor; >> joydev.product_id = device_id.product; >> + >> + /* Construct the GUID in the same way of Windows doing this. >> + Data1 is concatenation of productid and vendorid. >> + Data2 and Data3 are NULL. >> + Data4 seems to be a constant. */ >> + joydev.guid_product.Data1 = (device_id.product << 16) + device_id.vendor; >> + >> + joydev.guid_product.Data2 = 0x0000; >> + joydev.guid_product.Data3 = 0x0000; >> + >> + joydev.guid_product.Data4[0] = 0x00; >> + joydev.guid_product.Data4[1] = 0x00; >> + joydev.guid_product.Data4[2] = 0x50; >> + joydev.guid_product.Data4[3] = 0x49; >> + joydev.guid_product.Data4[4] = 0x44; >> + joydev.guid_product.Data4[5] = 0x56; >> + joydev.guid_product.Data4[6] = 0x49; >> + joydev.guid_product.Data4[7] = 0x44; >> } > I believe it is better to use a local escope static const char and > memcpy instead of setting byte per byte. Or in global escope static > const char near the definition of DInput_Wine_Joystick_Base_GUID > (line 157). > > Best regards, > Bruno

July 23, 2016

Adding binfmt configuration to official Wine packages

by Bruno Jesus

Hi, I would like to please call for attention on bug https://bugs.winehq.org/show_bug.cgi?id=39884 As far as I understand there are only benefits to users if we allow .exe files to run through binfmt, would it pose any kind of problem to add this configuration to our packages? Best wishes, Bruno

July 23, 2016

Re: joy.cpl: Display the correct number of button boxes during test

by DavidL

I noticed this as well - thanks! On Friday, July 22, 2016, Bruno Jesus <00cpxxx(a)gmail.com> wrote: > After testing with an 8 button only joystick I noticed that currently > the program displays 9 buttons. Then I tested with an PS2 + usb > adapter controller and noticed that it is also off by one. I believe > nobody notices this because the gamepads have too many buttons > nowadays and you never test all of them. > > Signed-off-by: Bruno Jesus <00cpxxx(a)gmail.com <javascript:;>> >

July 23, 2016

Re: [PATCH] dinput : use vendor and product ids to generate product GUID like it's done on Windows - second try

by Bruno Jesus

On Fri, Jul 22, 2016 at 3:31 AM, Corentin Rossignon <corossig(a)gmail.com> wrote: > Signed-off-by: Corentin Rossignon <corossig(a)gmail.com> > --- > dlls/dinput/joystick_linuxinput.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) Hi, I just tested in Windows and indeed what you intend seems correct. The PID/VID matches with what Windows uses: //Windows guidProduct : {68750603-0000-0000-0000-504944564944} --- Generic HID gamepad guidProduct : {0268054C-0000-0000-0000-504944564944} --- PS2 gamepad + usb adapter //Wine guidProduct : {9E573ED9-7734-11D2-8D4A-23903FB6BDF7} guidProduct : {9E573ED9-7734-11D2-8D4A-23903FB6BDF7} But this patch does not work for me, my joysticks are detected in joystick_linux.c not joystick_linuxinput.c. I believe the change is correct but I can't confirm it, a similar patch for joystick_linux.c would be needed. > diff --git a/dlls/dinput/joystick_linuxinput.c b/dlls/dinput/joystick_linuxinput.c > index 86f12d0..3f9c241 100644 > --- a/dlls/dinput/joystick_linuxinput.c > +++ b/dlls/dinput/joystick_linuxinput.c > @@ -96,6 +96,7 @@ struct JoyDev { > char *device; > char *name; > GUID guid; > + GUID guid_product; > > BOOL has_ff; > int num_effects; > @@ -293,11 +294,32 @@ static void find_joydevs(void) > } > > if (ioctl(fd, EVIOCGID, &device_id) == -1) > + { > WARN("ioctl(EVIOCGID) failed: %d %s\n", errno, strerror(errno)); > + joydev.guid_product = DInput_Wine_Joystick_Base_GUID; > + } > else > { > joydev.vendor_id = device_id.vendor; > joydev.product_id = device_id.product; > + > + /* Construct the GUID in the same way of Windows doing this. > + Data1 is concatenation of productid and vendorid. > + Data2 and Data3 are NULL. > + Data4 seems to be a constant. */ > + joydev.guid_product.Data1 = (device_id.product << 16) + device_id.vendor; > + > + joydev.guid_product.Data2 = 0x0000; > + joydev.guid_product.Data3 = 0x0000; > + > + joydev.guid_product.Data4[0] = 0x00; > + joydev.guid_product.Data4[1] = 0x00; > + joydev.guid_product.Data4[2] = 0x50; > + joydev.guid_product.Data4[3] = 0x49; > + joydev.guid_product.Data4[4] = 0x44; > + joydev.guid_product.Data4[5] = 0x56; > + joydev.guid_product.Data4[6] = 0x49; > + joydev.guid_product.Data4[7] = 0x44; > } I believe it is better to use a local escope static const char and memcpy instead of setting byte per byte. Or in global escope static const char near the definition of DInput_Wine_Joystick_Base_GUID (line 157). Best regards, Bruno

July 23, 2016

Re: [PATCH] ws2_32: Implement InetNtopW

by Bruno Jesus

On Fri, Jul 22, 2016 at 5:16 PM, Philipp Hoppermann <plata(a)mailbox.org> wrote: > Fixes https://bugs.winehq.org/show_bug.cgi?id=40999 Thanks, I have written some tests to back your changes and will send soon, I also noticed a mistake in my review of your patch that I'll fix and send a new version.

July 22, 2016

Re: [PATCH 2/2] ws2_32: Fix implementation of InetNtopW.

by Bruno Jesus

On Fri, Jul 22, 2016 at 9:04 AM, Philipp Hoppermann <plata(a)mailbox.org> wrote: > Replaced call to inet_ntop by more elaborate solution. Looks much better, thanks. See the comments below. > Signed-off-by: Philipp Hoppermann <plata(a)mailbox.org> > --- > dlls/ws2_32/socket.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 68 insertions(+), 4 deletions(-) > > diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c > index 0622060..2045163 100644 > --- a/dlls/ws2_32/socket.c > +++ b/dlls/ws2_32/socket.c > @@ -7934,11 +7934,75 @@ INT WINAPI InetPtonW(INT family, PCWSTR addr, PVOID buffer) > } > > /*********************************************************************** > -* InetNtopW (WS2_32.@) > -*/ > -PCWSTR WINAPI InetNtopW(INT family, PVOID addr, PWSTR stringBuf, SIZE_T stringBufSize) > + * InetNtopW (WS2_32.@) > + * > + * Get string representation of IP address. > + * > + * PARAMS > + * family [I] AF_INET or AF_INET6 > + * addr [I] IP address in byte > + * buffer [O] IP address as string > + * buffer_len [O] length of the buffer in characters > + * > + * RETURNS > + * Success: IP address as string > + * Failure: NULL. Use GetLastError() to find the error cause. > + * > + * Error codes: > + * WSAEAFNOSUPPORT > + * - family is not AF_INET or AF_INET6 > + * ERROR_INVALID_PARAMETER > + * - buffer is NULL > + * - buffer_len is 0 > + * - buffer_len is too small > + * (i.e. < 16 for AF_INET or < 46 for AF_INET6) > + * > + */ Usually only overly complex functions need comments, which is not the case here so you can drop them. As you can see in the file most almost no other function has comments like this. > +PCWSTR WINAPI InetNtopW(INT family, PVOID addr, PWSTR buffer, SIZE_T buffer_len) > { > - return inet_ntop(family, addr, stringBuf, stringBufSize); > + char *bufferA; > + > + TRACE("family %d, addr %s, buffer (%p), buffer_len %lu\n", family, debugstr_w(addr), buffer, (unsigned long) buffer_len); WS_inet_ntop traces buffer_len without the cast using %ld, I guess it is better to follow its example and avoid the cast. > + // check family // comments are not allowed, please use /* */. But most of them are obvious like "check if buffer is NULL" so they can be dropped. > + if ((family != AF_INET) && (family != AF_INET6)) > + { > + SetLastError(WSAEAFNOSUPPORT); > + return NULL; > + } > + > + // check if buffer is NULL > + if (buffer == NULL) > + { > + SetLastError(ERROR_INVALID_PARAMETER); > + return NULL; > + } > + > + // check buffer_len > + if ((family == AF_INET && buffer_len < 16) || (family == AF_INET6 && buffer_len < 46)) > + { > + SetLastError(ERROR_INVALID_PARAMETER); > + return NULL; > + } WS_inet_ntop already does some error checking, there is no need to manually test again, also we trust the system inet_ntop call to deal with buffer_len check. > + // allocate memory for the string representation in char > + if (!(bufferA = HeapAlloc(GetProcessHeap(), 0, buffer_len*sizeof(WCHAR)))) > + { > + SetLastError(WSA_NOT_ENOUGH_MEMORY); > + return NULL; > + } You are doubling the required amount of memory, buffer_len should be already in characters, so no need to multiply. > + if (WS_inet_ntop(family, addr, bufferA, buffer_len*sizeof(WCHAR)) != NULL) Again no need to multiply. > + { > + // convert to wchar > + MultiByteToWideChar(CP_ACP, 0, bufferA, buffer_len*sizeof(WCHAR), buffer, buffer_len); Use: MultiByteToWideChar(CP_ACP, 0, bufferA, -1, buffer, buffer_len); > + > + HeapFree(GetProcessHeap(), 0, bufferA); > + return buffer; > + } else > + { > + return NULL; You are leaking bufferA in the else case, use a variable to store the results of WS_inet_ntop and return it instead so you can always HeapFree before the return and then there is no need for else. Best wishes, Bruno

July 22, 2016

Re: [PATCH 1/2] ws2_32: Implement InetNtopW.

by Bruno Jesus

On Fri, Jul 22, 2016 at 9:04 AM, Philipp Hoppermann <plata(a)mailbox.org> wrote: > Fixes "unimplemented function WS2_32.dll.InetNtopW" > > At the moment, this fix just calls inet_ntop It is not necessary to split the patch, specially because this patch is broken because of the char/WCHAR thing. Merge the spec changes in the 2/2 patch and see my other comments there.

July 22, 2016

Re: include: Improve include directive in d3dtypes.h.

by Henri Verbeet

On 22 July 2016 at 15:31, Andrey Gusev <andrey.goosev(a)gmail.com> wrote: > -#include <d3dvec.inl> > +#include "d3dvec.inl" Is that really an improvement? I'd argue these are system headers and angle brackets are correct. Of course in that case there are a couple of places where we'd want to change things in the other direction. Whether that's worth the trouble may be a different question.

July 22, 2016

Re: [v3 1/1] winejoystick.drv: Sort virtual joysticks by name on the Mac

by DavidL

New versions sent! Cheers, David On Thu, Jul 21, 2016 at 10:41 PM, Ken Thomases <ken(a)codeweavers.com> wrote: > Hi, > > On Jul 20, 2016, at 2:15 AM, David Lawrie <david.dljunk(a)gmail.com> wrote: > > > > +static CFStringRef copy_device_name(IOHIDDeviceRef device) > > +{ > > + CFTypeRef ref_name, ref_man; > > + CFStringRef name; > > + long prodID; > > I prefer variables to be declared at the narrowest scope in which they're > used. > > > + > > + if (device) > > + { > > + assert(IOHIDDeviceGetTypeID() == CFGetTypeID(device)); > > + > > + ref_name = IOHIDDeviceGetProperty(device, > CFSTR(kIOHIDProductKey)); > > + > > + if (ref_name && CFStringGetTypeID() == CFGetTypeID(ref_name)) > > + name = CFStringCreateCopy(kCFAllocatorDefault,ref_name); > > + else > > + { > > + ref_man = IOHIDDeviceGetProperty(device, > CFSTR(kIOHIDManufacturerKey)); > > I think it's simpler to use kIOHIDVendorIDKey with > get_device_property_long(). Judging by the HID Calibrator code, > kIOHIDManufacturerKey is not reliably available, but kIOHIDVendorIDKey > should be. It's also pretty fundamental to USB/HID. > > > + prodID = get_device_property_long(device, > CFSTR(kIOHIDProductIDKey)); > > + > > + if(ref_man && CFStringGetTypeID() == CFGetTypeID(ref_man)) > > + name = CFStringCreateWithFormat(kCFAllocatorDefault, > NULL, CFSTR("%@ %lu"), ref_man, prodID); > > Better to format vendor and product IDs using "0x%04lx" since that's how > they are usually represented (e.g. within HID Calibrator windows and System > Profiler). (Yes, I realize that Copy_DeviceName() which I cited previously > formats them as decimal.) > > Thanks for persevering in the face of my picky-ness. :) > :) > > -Ken > >

July 22, 2016

Re: [3/6] msvcirt: Forward signed char exports to char functions directly

by Iván Matellanes

On 21/07/16 12:40, Alexandre Julliard wrote: > "Iván Matellanes" <matellanesivan(a)gmail.com> writes: > >> signed char and char are equivalent in Visual C++, so specific functions for signed char are unnecessary. > > Note that you can't assume that 'char' is signed for the Unix > compiler. If it makes a difference, you'd have to specify signed char > explicitly. Otherwise, you could as well you the same functions for the > unsigned case too. > True. I'll check the unsigned cases as I suspect most of the functions (if not all) can be removed too. Cheers, Iván

July 22, 2016

Re: [v2 PATCH 4/4] hnetcfg: Add initial tests

by Alistair Leslie-Hughes

On 22/07/16 17:50, 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=24452 > > Your paranoid android. > > > === build (build) === > Make failed > > This failed because uuid.lib wasn't rebuild. Best Regards Alistair Leslie-Hughes

July 22, 2016

Re: [v2 PATCH 4/4] hnetcfg: Add initial tests

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=24452 Your paranoid android. === build (build) === Make failed

July 22, 2016

[PATCH] dinput : use vendor and product ids to generate product GUID like it's done on Windows - second try

by Corentin Rossignon

Signed-off-by: Corentin Rossignon <corossig(a)gmail.com> --- dlls/dinput/joystick_linuxinput.c | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/dlls/dinput/joystick_linuxinput.c b/dlls/dinput/joystick_linuxinput.c index 86f12d0..3f9c241 100644 --- a/dlls/dinput/joystick_linuxinput.c +++ b/dlls/dinput/joystick_linuxinput.c @@ -96,6 +96,7 @@ struct JoyDev { char *device; char *name; GUID guid; + GUID guid_product; BOOL has_ff; int num_effects; @@ -293,11 +294,32 @@ static void find_joydevs(void) } if (ioctl(fd, EVIOCGID, &device_id) == -1) + { WARN("ioctl(EVIOCGID) failed: %d %s\n", errno, strerror(errno)); + joydev.guid_product = DInput_Wine_Joystick_Base_GUID; + } else { joydev.vendor_id = device_id.vendor; joydev.product_id = device_id.product; + + /* Construct the GUID in the same way of Windows doing this. + Data1 is concatenation of productid and vendorid. + Data2 and Data3 are NULL. + Data4 seems to be a constant. */ + joydev.guid_product.Data1 = (device_id.product << 16) + device_id.vendor; + + joydev.guid_product.Data2 = 0x0000; + joydev.guid_product.Data3 = 0x0000; + + joydev.guid_product.Data4[0] = 0x00; + joydev.guid_product.Data4[1] = 0x00; + joydev.guid_product.Data4[2] = 0x50; + joydev.guid_product.Data4[3] = 0x49; + joydev.guid_product.Data4[4] = 0x44; + joydev.guid_product.Data4[5] = 0x56; + joydev.guid_product.Data4[6] = 0x49; + joydev.guid_product.Data4[7] = 0x44; } if (!have_joydevs) @@ -327,7 +349,7 @@ static void fill_joystick_dideviceinstanceA(LPDIDEVICEINSTANCEA lpddi, DWORD ver lpddi->dwSize = dwSize; lpddi->guidInstance = joydevs[id].guid; - lpddi->guidProduct = DInput_Wine_Joystick_Base_GUID; + lpddi->guidProduct = joydevs[id].guid_product; lpddi->guidFFDriver = GUID_NULL; if (version >= 0x0800) @@ -348,7 +370,7 @@ static void fill_joystick_dideviceinstanceW(LPDIDEVICEINSTANCEW lpddi, DWORD ver lpddi->dwSize = dwSize; lpddi->guidInstance = joydevs[id].guid; - lpddi->guidProduct = DInput_Wine_Joystick_Base_GUID; + lpddi->guidProduct = joydevs[id].guid_product; lpddi->guidFFDriver = GUID_NULL; if (version >= 0x0800) -- 2.9.0

July 22, 2016

Re: [PATCH v2 2/3] shell32: Implement FolderItems_Item.

by Nikolay Sivov

On 22.07.2016 6:51, Alex Henrie wrote: > Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com> > --- > dlls/shell32/shelldispatch.c | 83 +++++++++++++++++++++++++++++++++++--- > dlls/shell32/tests/shelldispatch.c | 17 +++++--- > 2 files changed, 89 insertions(+), 11 deletions(-) > > diff --git a/dlls/shell32/shelldispatch.c b/dlls/shell32/shelldispatch.c > index ac79302..4b4c822 100644 > --- a/dlls/shell32/shelldispatch.c > +++ b/dlls/shell32/shelldispatch.c > @@ -68,6 +68,8 @@ typedef struct { > typedef struct { > FolderItems3 FolderItems3_iface; > LONG ref; > + BSTR *item_paths; > + LONG item_count; > } FolderItemsImpl; > Is it possible we'll need special folder indices here as well? I didn't check thoroughly. > static HRESULT WINAPI FolderItemsImpl_Item(FolderItems3 *iface, VARIANT index, FolderItem **ppid) > { > - FIXME("(%p,%s,%p)\n", iface, debugstr_variant(&index), ppid); > + FolderItemsImpl *This = impl_from_FolderItems(iface); > + VARIANT path; > + > + TRACE("(%p,%s,%p)\n", iface, debugstr_variant(&index), ppid); > > *ppid = NULL; > - return E_NOTIMPL; > + > + if (V_I4(&index) >= This->item_count) > + return S_FALSE; > + > + V_VT(&path) = VT_BSTR; > + V_BSTR(&path) = This->item_paths[V_I4(&index)]; > + > + return FolderItem_Constructor(&path, ppid); > } This is assuming you always have VT_I4 on input, but it's obviously possible to call it with any type. > > static HRESULT WINAPI FolderItemsImpl__NewEnum(FolderItems3 *iface, IUnknown **ppunk) > @@ -1139,9 +1157,17 @@ static const FolderItems3Vtbl FolderItemsImpl_Vtbl = { > FolderItemsImpl_get_Verbs > }; > > -static HRESULT FolderItems_Constructor(FolderItems **ppfi) > +static HRESULT FolderItems_Constructor(VARIANT *dir, FolderItems **ppfi) > { > + static const WCHAR backslash_star[] = {'\\','*',0}; > + static const WCHAR dot[] = {'.',0}; > + static const WCHAR dot_dot[] = {'.','.',0}; > FolderItemsImpl *This; > + WCHAR path[MAX_PATH]; > + HANDLE first_file; > + WIN32_FIND_DATAW file_info; > + BSTR *paths; > + HRESULT ret = S_OK; > > TRACE("\n"); > > @@ -1152,8 +1178,51 @@ static HRESULT FolderItems_Constructor(FolderItems **ppfi) > This->FolderItems3_iface.lpVtbl = &FolderItemsImpl_Vtbl; > This->ref = 1; > > + This->item_paths = HeapAlloc(GetProcessHeap(), 0, sizeof(BSTR)); > + if (!This->item_paths) > + { > + HeapFree(GetProcessHeap(), 0, This); > + return E_OUTOFMEMORY; > + } > + This->item_count = 0; > + > + lstrcpyW(path, V_BSTR(dir)); > + lstrcatW(path, backslash_star); > + first_file = FindFirstFileW(path, &file_info); > + if (first_file != INVALID_HANDLE_VALUE) 'dir' is not necessary VT_BSTR.

July 22, 2016

Re: [PATCH] dinput : use vendor and product ids to generate product GUID like it's done on Windows

by Corentin Rossignon

Hi, I will do a new patch with /* */ comment. I am using ditool.exe to display informations about DirectInput device. I found this tool on http://community.pcgamingwiki.com/files/file/588-directinputxinput-games-te… On 07/22/2016 02:06 AM, Bruno Jesus wrote: > On Thu, Jul 21, 2016 at 7:08 PM, Corentin Rossignon <corossig(a)gmail.com> wrote: >> Signed-off-by: Corentin Rossignon <corossig(a)gmail.com> >> --- >> dlls/dinput/joystick_linuxinput.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/dlls/dinput/joystick_linuxinput.c b/dlls/dinput/joystick_linuxinput.c >> index 86f12d0..e5172b6 100644 >> --- a/dlls/dinput/joystick_linuxinput.c >> +++ b/dlls/dinput/joystick_linuxinput.c >> @@ -321,13 +321,19 @@ static void find_joydevs(void) >> static void fill_joystick_dideviceinstanceA(LPDIDEVICEINSTANCEA lpddi, DWORD version, int id) >> { >> DWORD dwSize = lpddi->dwSize; >> + // Data1 is concatenation of productid and vendorid, Data4 seems constant >> + GUID product_guid = { ((joydevs[id].product_id)<<16)+joydevs[id].vendor_id, >> + 0x0000, >> + 0x0000, >> + {0x00, 0x00, 0x50, 0x49, 0x44, 0x56, 0x49, 0x44} }; >> + > Hi, I'm not familiar with direct input code but I can say that // > comments are not allowed, please use /* */ and send a try 2. Is there > any easy way to test this patch? Like a simple app that displays such > information so I can help testing in Windows and Wine?

July 22, 2016

Re: [PATCH] dinput : use vendor and product ids to generate product GUID like it's done on Windows

by Corentin Rossignon

Hi, I will do a new patch with /* */ comment. I am using ditool.exe to display informations about DirectInput device. I found this tool on http://community.pcgamingwiki.com/files/file/588-directinputxinput-games-te… On 07/22/2016 02:06 AM, Bruno Jesus wrote: > On Thu, Jul 21, 2016 at 7:08 PM, Corentin Rossignon <corossig(a)gmail.com> wrote: >> Signed-off-by: Corentin Rossignon <corossig(a)gmail.com> >> --- >> dlls/dinput/joystick_linuxinput.c | 16 ++++++++++++++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/dlls/dinput/joystick_linuxinput.c b/dlls/dinput/joystick_linuxinput.c >> index 86f12d0..e5172b6 100644 >> --- a/dlls/dinput/joystick_linuxinput.c >> +++ b/dlls/dinput/joystick_linuxinput.c >> @@ -321,13 +321,19 @@ static void find_joydevs(void) >> static void fill_joystick_dideviceinstanceA(LPDIDEVICEINSTANCEA lpddi, DWORD version, int id) >> { >> DWORD dwSize = lpddi->dwSize; >> + // Data1 is concatenation of productid and vendorid, Data4 seems constant >> + GUID product_guid = { ((joydevs[id].product_id)<<16)+joydevs[id].vendor_id, >> + 0x0000, >> + 0x0000, >> + {0x00, 0x00, 0x50, 0x49, 0x44, 0x56, 0x49, 0x44} }; >> + > Hi, I'm not familiar with direct input code but I can say that // > comments are not allowed, please use /* */ and send a try 2. Is there > any easy way to test this patch? Like a simple app that displays such > information so I can help testing in Windows and Wine?

July 22, 2016

Re: [v3 1/1] dinput: Sort virtual joysticks by name on the Mac

by Ken Thomases

On Jul 20, 2016, at 2:15 AM, 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/dinput/joystick_osx.c | 50 +++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 47 insertions(+), 3 deletions(-) Same comments as for the winejoystick version. -Ken

July 22, 2016

Re: [v3 1/1] winejoystick.drv: Sort virtual joysticks by name on the Mac

by Ken Thomases

Hi, On Jul 20, 2016, at 2:15 AM, David Lawrie <david.dljunk(a)gmail.com> wrote: > > +static CFStringRef copy_device_name(IOHIDDeviceRef device) > +{ > + CFTypeRef ref_name, ref_man; > + CFStringRef name; > + long prodID; I prefer variables to be declared at the narrowest scope in which they're used. > + > + if (device) > + { > + assert(IOHIDDeviceGetTypeID() == CFGetTypeID(device)); > + > + ref_name = IOHIDDeviceGetProperty(device, CFSTR(kIOHIDProductKey)); > + > + if (ref_name && CFStringGetTypeID() == CFGetTypeID(ref_name)) > + name = CFStringCreateCopy(kCFAllocatorDefault,ref_name); > + else > + { > + ref_man = IOHIDDeviceGetProperty(device, CFSTR(kIOHIDManufacturerKey)); I think it's simpler to use kIOHIDVendorIDKey with get_device_property_long(). Judging by the HID Calibrator code, kIOHIDManufacturerKey is not reliably available, but kIOHIDVendorIDKey should be. It's also pretty fundamental to USB/HID. > + prodID = get_device_property_long(device, CFSTR(kIOHIDProductIDKey)); > + > + if(ref_man && CFStringGetTypeID() == CFGetTypeID(ref_man)) > + name = CFStringCreateWithFormat(kCFAllocatorDefault, NULL, CFSTR("%@ %lu"), ref_man, prodID); Better to format vendor and product IDs using "0x%04lx" since that's how they are usually represented (e.g. within HID Calibrator windows and System Profiler). (Yes, I realize that Copy_DeviceName() which I cited previously formats them as decimal.) Thanks for persevering in the face of my picky-ness. :) -Ken

July 22, 2016

i need yor help

by achbarrr＠gmail.com

hello everyone i need your help i want wine for android why because i work in corel and photosop and msoffice but i want to do it on touchly os like android because my finger do best on it please send me an mail or email for that wine for android please

July 22, 2016

Re: [PATCH] dinput : use vendor and product ids to generate product GUID like it's done on Windows

by Bruno Jesus

On Thu, Jul 21, 2016 at 7:08 PM, Corentin Rossignon <corossig(a)gmail.com> wrote: > Signed-off-by: Corentin Rossignon <corossig(a)gmail.com> > --- > dlls/dinput/joystick_linuxinput.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/dlls/dinput/joystick_linuxinput.c b/dlls/dinput/joystick_linuxinput.c > index 86f12d0..e5172b6 100644 > --- a/dlls/dinput/joystick_linuxinput.c > +++ b/dlls/dinput/joystick_linuxinput.c > @@ -321,13 +321,19 @@ static void find_joydevs(void) > static void fill_joystick_dideviceinstanceA(LPDIDEVICEINSTANCEA lpddi, DWORD version, int id) > { > DWORD dwSize = lpddi->dwSize; > + // Data1 is concatenation of productid and vendorid, Data4 seems constant > + GUID product_guid = { ((joydevs[id].product_id)<<16)+joydevs[id].vendor_id, > + 0x0000, > + 0x0000, > + {0x00, 0x00, 0x50, 0x49, 0x44, 0x56, 0x49, 0x44} }; > + Hi, I'm not familiar with direct input code but I can say that // comments are not allowed, please use /* */ and send a try 2. Is there any easy way to test this patch? Like a simple app that displays such information so I can help testing in Windows and Wine?

July 22, 2016

Re: [PATCH 2/2] winex11: track glx windows per thread

by Miklós Máté

On 07/21/2016 12:12 PM, Alexandre Julliard wrote: > Miklós Máté <mtmkls(a)gmail.com> writes: > >> On 07/21/2016 05:39 AM, Alexandre Julliard wrote: >>> Miklós Máté <mtmkls(a)gmail.com> writes: >>> >>>> XCloseDisplay in thread_detach() closes all X windows, but the GLX windows >>>> remain, and become zombies. If a new X window is created with the same XID, >>>> Mesa refuses to attach a GLX window to it because of the zombie. Observed >>>> in the ddraw test. >>> Is there a reason that creating the GLX window on the window thread >>> wouldn't work? >>> >> The problem is that Wine uses XCloseDisplay to destroy all X windows >> created in that thread, but there is no equivalent functionality for >> GLX. If gdi_display = windata->display, it would remove the need for >> XSync, but the problem of zombie GLX windows would remain. I hope I >> understood your question correctly. > The ID should be invalidated once the display connection is closed, > otherwise it's a Mesa bug. Have you tested it? > Hmm, you are right. Mesa does set an XCloseDisplay callback to clean things up for the display connections it encounters. If I use data->display instead of gdi_display for pglXCreateWindow in line 1358 of opengl.c, the tests pass, and no zombie glx windows remain. I don't know if I should dare to take responsibility for this change, though, as I know almost nothing about GDI. Note that the zombies cause set_pixel_format to fail, but the tests treat that as skip, and not fail. I don't know if anything actually fails due to the zombie glx windows. Meanwhile I noticed that patch 1/2 I submitted wasn't the final version. opengl.c line 1430 should be Window old_client_window = None; to avoid the uninitialized variable warning. Should I submit a fixed version? MM

July 21, 2016

Re: [PATCH] ws2_32: Implement InetNtopW.

by Bruno Jesus

On Thu, Jul 21, 2016 at 2:46 PM, Philipp Hoppermann <plata(a)mailbox.org> wrote: > Fixes "unimplemented function WS2_32.dll.InetNtopW" Hi, thanks for the patch. > At the moment, this fix just calls inet_ntop Unfortunately you can't do that. inet_ntop replies in char, not WCHAR. Please see how InetPtonW is implemented [1], by following its example you will be able to implement the function properly using WS_inet_ntop. http://source.winehq.org/git/wine.git/commitdiff/af8c79343ce9eb24e223607ec2… > Signed-off-by: Philipp Hoppermann <plata(a)mailbox.org> > --- > dlls/ws2_32/socket.c | 8 ++++++++ > dlls/ws2_32/ws2_32.spec | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/dlls/ws2_32/socket.c b/dlls/ws2_32/socket.c > index b0af3d7..0622060 100644 > --- a/dlls/ws2_32/socket.c > +++ b/dlls/ws2_32/socket.c > @@ -7934,6 +7934,14 @@ INT WINAPI InetPtonW(INT family, PCWSTR addr, PVOID buffer) > } > > /*********************************************************************** > +* InetNtopW (WS2_32.@) > +*/ > +PCWSTR WINAPI InetNtopW(INT family, PVOID addr, PWSTR stringBuf, SIZE_T stringBufSize) Please change stringBuf to buffer and stringBufSize to buffer_len.

July 21, 2016

Re: ole32: Use debugstr_guid() in TRACE() messages.

by Michael Stefaniuc

Hello Andrey! On 07/21/2016 05:57 PM, Andrey Gusev wrote: > > ole32-Use-debugstr_guid-in-TRACE-messages.txt > > > From d360a701eee8b8cf715059d630163a7a21dc3222 Mon Sep 17 00:00:00 2001 > Message-Id: <d360a701eee8b8cf715059d630163a7a21dc3222.1469116606.git.andrey.goosev(a)gma…> > From: Andrey Gusev <andrey.goosev(a)gmail.com> > Date: Thu, 21 Jul 2016 18:56:32 +0300 > Subject: [PATCH] ole32: Use debugstr_guid() in TRACE() messages. > > Signed-off-by: Andrey Gusev <andrey.goosev(a)gmail.com> > --- > dlls/ole32/classmoniker.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/dlls/ole32/classmoniker.c b/dlls/ole32/classmoniker.c > index 1bcf58f..00c2f42 100644 > --- a/dlls/ole32/classmoniker.c > +++ b/dlls/ole32/classmoniker.c > @@ -231,7 +231,7 @@ static HRESULT WINAPI ClassMoniker_BindToObject(IMoniker* iface, > IClassActivator *pActivator; > HRESULT hr; > > - TRACE("(%p,%p,%p,%p)\n", pbc, pmkToLeft, riid, ppvResult); > + TRACE("(%p,%p,%s,%p)\n", pbc, pmkToLeft, debugstr_guid(riid), ppvResult); > > bindopts.cbStruct = sizeof(bindopts); > IBindCtx_GetBindOptions(pbc, (BIND_OPTS *)&bindopts); > @@ -264,7 +264,7 @@ static HRESULT WINAPI ClassMoniker_BindToStorage(IMoniker* iface, > REFIID riid, > VOID** ppvResult) > { > - TRACE("(%p,%p,%p,%p)\n",pbc, pmkToLeft, riid, ppvResult); > + TRACE("(%p,%p,%s,%p)\n",pbc, pmkToLeft, debugstr_guid(riid), ppvResult); > return IMoniker_BindToObject(iface, pbc, pmkToLeft, riid, ppvResult); > } > > @@ -586,7 +586,7 @@ static HRESULT WINAPI ClassMonikerROTData_QueryInterface(IROTData *iface,REFIID > > ClassMoniker *This = impl_from_IROTData(iface); > > - TRACE("(%p,%p,%p)\n",iface,riid,ppvObject); > + TRACE("(%p,%s,%p)\n",iface,debugstr_guid(riid),ppvObject); While you work on this please fix also the whitespace issues in those lines: Space after ',' and line length should not exceed around 100 chars. > > return IMoniker_QueryInterface(&This->IMoniker_iface, riid, ppvObject); > } > -- 2.5.5 bye michael

July 21, 2016

Re: [PATCH 4/4] hnetcfg: Add stubbed INetFwPolicy2 interface

by Nikolay Sivov

On 21.07.2016 12:09, Alistair Leslie-Hughes wrote: > Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> > --- > dlls/hnetcfg/apps.c | 1 + > dlls/hnetcfg/hnetcfg.c | 5 + > dlls/hnetcfg/hnetcfg_private.h | 2 + > dlls/hnetcfg/policy.c | 332 +++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 340 insertions(+) > > diff --git a/dlls/hnetcfg/apps.c b/dlls/hnetcfg/apps.c > index ea6d411..2b8d78c 100644 > --- a/dlls/hnetcfg/apps.c > +++ b/dlls/hnetcfg/apps.c > @@ -111,6 +111,7 @@ static REFIID tid_id[] = > &IID_INetFwOpenPort, > &IID_INetFwOpenPorts, > &IID_INetFwPolicy, > + &IID_INetFwPolicy2, > &IID_INetFwProfile > }; > > diff --git a/dlls/hnetcfg/hnetcfg.c b/dlls/hnetcfg/hnetcfg.c > index fadd367..e56dbab 100644 > --- a/dlls/hnetcfg/hnetcfg.c > +++ b/dlls/hnetcfg/hnetcfg.c > @@ -113,6 +113,7 @@ static const struct IClassFactoryVtbl hnetcfg_cf_vtbl = > static hnetcfg_cf fw_manager_cf = { { &hnetcfg_cf_vtbl }, NetFwMgr_create }; > static hnetcfg_cf fw_app_cf = { { &hnetcfg_cf_vtbl }, NetFwAuthorizedApplication_create }; > static hnetcfg_cf fw_openport_cf = { { &hnetcfg_cf_vtbl }, NetFwOpenPort_create }; > +static hnetcfg_cf fw_policy2_cf = { { &hnetcfg_cf_vtbl }, NetFwPolicy2_create }; > > BOOL WINAPI DllMain(HINSTANCE hInstDLL, DWORD fdwReason, LPVOID reserved) > { > @@ -151,6 +152,10 @@ HRESULT WINAPI DllGetClassObject( REFCLSID rclsid, REFIID iid, LPVOID *ppv ) > { > cf = &fw_openport_cf.IClassFactory_iface; > } > + else if (IsEqualGUID( rclsid, &CLSID_NetFwPolicy2 )) > + { > + cf = &fw_policy2_cf.IClassFactory_iface; > + } > > if (!cf) return CLASS_E_CLASSNOTAVAILABLE; > return IClassFactory_QueryInterface( cf, iid, ppv ); > diff --git a/dlls/hnetcfg/hnetcfg_private.h b/dlls/hnetcfg/hnetcfg_private.h > index bb3f3aa..812d970 100644 > --- a/dlls/hnetcfg/hnetcfg_private.h > +++ b/dlls/hnetcfg/hnetcfg_private.h > @@ -24,6 +24,7 @@ enum type_id > INetFwOpenPort_tid, > INetFwOpenPorts_tid, > INetFwPolicy_tid, > + INetFwPolicy2_tid, > INetFwProfile_tid, > last_tid > }; > @@ -33,6 +34,7 @@ void release_typelib(void) DECLSPEC_HIDDEN; > > HRESULT NetFwMgr_create(IUnknown *, LPVOID *) DECLSPEC_HIDDEN; > HRESULT NetFwPolicy_create(IUnknown *, LPVOID *) DECLSPEC_HIDDEN; > +HRESULT NetFwPolicy2_create(IUnknown *, void **) DECLSPEC_HIDDEN; It makes sense to test first if those two both support INetFwPolicy and INetFwPolicy2.

July 21, 2016

Re: [PATCH 3/4] hnetcfg: Register NetFwPolicy2 interface

by Nikolay Sivov

On 21.07.2016 12:09, Alistair Leslie-Hughes wrote: > Signed-off-by: Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> > --- > dlls/hnetcfg/hnetcfg.idl | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/dlls/hnetcfg/hnetcfg.idl b/dlls/hnetcfg/hnetcfg.idl > index 9934f6a..763529a 100644 > --- a/dlls/hnetcfg/hnetcfg.idl > +++ b/dlls/hnetcfg/hnetcfg.idl > @@ -43,3 +43,11 @@ coclass NetFwAuthorizedApplication { interface INetFwAuthorizedApplications; } > uuid(0ca545c6-37ad-4a6c-bf92-9f7610067ef5) > ] > coclass NetFwOpenPort { interface INetFwOpenPort; } > + > +[ > + helpstring("HNetCfg.FwPolicy2"), > + progid("HNetCfg.FwPolicy2"), > + threading(both), > + uuid(e2b3c97f-6ae1-41ac-817a-f6f92166d7dd) > +] > +coclass NetFwPolicy2 { interface INetFwPolicy2; } > This doesn't have to be a separate patch.

July 21, 2016

Re: ntdll: Keep drive letter size in RtlGetFullPathName_U when full path is specified

by Alexandre Julliard

Piotr Caban <piotr(a)codeweavers.com> writes: > Signed-off-by: Piotr Caban <piotr(a)codeweavers.com> > --- > dlls/ntdll/path.c | 2 +- > dlls/ntdll/tests/path.c | 9 +++++---- > 2 files changed, 6 insertions(+), 5 deletions(-) This is causing a test failure: ../../../tools/runtest -q -P wine -T ../../.. -M cmd.exe -p cmd.exe_test.exe.so batch && touch batch.ok batch.c:313: Test succeeded inside todo block: unexpected char 0x0 position -1 in line 983 (got 'c:\', wanted 'c:\') make[1]: *** [batch.ok] Error 1 -- Alexandre Julliard julliard(a)winehq.org

July 21, 2016

Re: [2/3] wininet: Start the first chunk as soon as we have some data.

by Hans Leidekker

On Thu, 2016-07-21 at 14:25 +0200, Jacek Caban wrote: > > --- > > dlls/wininet/http.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/dlls/wininet/http.c b/dlls/wininet/http.c > > index 2c62d97..c4cf66b 100644 > > --- a/dlls/wininet/http.c > > +++ b/dlls/wininet/http.c > > @@ -2921,6 +2921,7 @@ static DWORD set_content_length(http_request_t *request) > > !strcmpiW(encoding, szChunked)) > > { > > chunked_stream_t *chunked_stream; > > + DWORD res; > > > > chunked_stream = heap_alloc(sizeof(*chunked_stream)); > > if(!chunked_stream) > > @@ -2935,6 +2936,12 @@ static DWORD set_content_length(http_request_t *request) > > memcpy(chunked_stream->buf, request->read_buf+request->read_pos, request->read_size); > > chunked_stream->buf_size = request->read_size; > > request->read_size = request->read_pos = 0; > > + > > + res = start_next_chunk(chunked_stream, request); > > + if (res != ERROR_SUCCESS) { > > + heap_free(chunked_stream); > > + return res; > > + } > > It seems to me that you're fixing a problem is a wrong place. > start_next_chunk should be called by HTTP_ReceiveRequestData via > refill_read_buffer anyway. I'd suggest to investigate why it doesn't > work in your case. This is required to make get_avail_data return the correct number of bytes. Note that the line above sets the read size to 0, so (before my second patch) the next call to get_avail_data would return 0. This may trigger a read beyond available data. It is what happens with Outlook. > Also, see the attached test. We shouldn't need any chunk to complete > requests. It's an existing problem in current Wine, which may be > unrelated to your problem, but having such blocking calls here is not a > step in the right direction. Your test hangs with and without this patch. Why is having a blocking call here a problem? It would happen right after reading the headers, also blocking calls. Reading the header of the first chunk at this point may actually be the right thing to do. This way we can answer a query for available data without potentially blocking.

July 21, 2016

Re: [2/3] wininet: Start the first chunk as soon as we have some data.

by Jacek Caban

Hi Hans, On 19.07.2016 11:27, Hans Leidekker wrote: > Signed-off-by: Hans Leidekker <hans(a)codeweavers.com> > --- > dlls/wininet/http.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/dlls/wininet/http.c b/dlls/wininet/http.c > index 2c62d97..c4cf66b 100644 > --- a/dlls/wininet/http.c > +++ b/dlls/wininet/http.c > @@ -2921,6 +2921,7 @@ static DWORD set_content_length(http_request_t *request) > !strcmpiW(encoding, szChunked)) > { > chunked_stream_t *chunked_stream; > + DWORD res; > > chunked_stream = heap_alloc(sizeof(*chunked_stream)); > if(!chunked_stream) > @@ -2935,6 +2936,12 @@ static DWORD set_content_length(http_request_t *request) > memcpy(chunked_stream->buf, request->read_buf+request->read_pos, request->read_size); > chunked_stream->buf_size = request->read_size; > request->read_size = request->read_pos = 0; > + > + res = start_next_chunk(chunked_stream, request); > + if (res != ERROR_SUCCESS) { > + heap_free(chunked_stream); > + return res; > + } It seems to me that you're fixing a problem is a wrong place. start_next_chunk should be called by HTTP_ReceiveRequestData via refill_read_buffer anyway. I'd suggest to investigate why it doesn't work in your case. Also, see the attached test. We shouldn't need any chunk to complete requests. It's an existing problem in current Wine, which may be unrelated to your problem, but having such blocking calls here is not a step in the right direction. Thanks, Jacek

July 21, 2016

Re: [3/6] msvcirt: Forward signed char exports to char functions directly

by Alexandre Julliard

"Iván Matellanes" <matellanesivan(a)gmail.com> writes: > signed char and char are equivalent in Visual C++, so specific functions for signed char are unnecessary. Note that you can't assume that 'char' is signed for the Unix compiler. If it makes a difference, you'd have to specify signed char explicitly. Otherwise, you could as well you the same functions for the unsigned case too. -- Alexandre Julliard julliard(a)winehq.org

July 21, 2016

Re: [3/6] msvcirt: Forward signed char exports to char functions directly

by Iván Matellanes

Oops, I messed up the numbering, this one should say [3/3]. On 21/07/16 10:56, Iván Matellanes wrote: > signed char and char are equivalent in Visual C++, so specific functions for signed char are unnecessary. > > Signed-off-by: Iván Matellanes <matellanes.ivan(a)gmail.com> > --- > dlls/msvcirt/msvcirt.c | 44 ++++++++++++++++---------------------------- > dlls/msvcirt/msvcirt.spec | 24 ++++++++++++------------ > 2 files changed, 28 insertions(+), 40 deletions(-)

July 21, 2016

Re: [PATCH 2/2] winex11: track glx windows per thread

by Alexandre Julliard

Miklós Máté <mtmkls(a)gmail.com> writes: > On 07/21/2016 05:39 AM, Alexandre Julliard wrote: >> Miklós Máté <mtmkls(a)gmail.com> writes: >> >>> XCloseDisplay in thread_detach() closes all X windows, but the GLX windows >>> remain, and become zombies. If a new X window is created with the same XID, >>> Mesa refuses to attach a GLX window to it because of the zombie. Observed >>> in the ddraw test. >> Is there a reason that creating the GLX window on the window thread >> wouldn't work? >> > The problem is that Wine uses XCloseDisplay to destroy all X windows > created in that thread, but there is no equivalent functionality for > GLX. If gdi_display = windata->display, it would remove the need for > XSync, but the problem of zombie GLX windows would remain. I hope I > understood your question correctly. The ID should be invalidated once the display connection is closed, otherwise it's a Mesa bug. Have you tested it? -- Alexandre Julliard julliard(a)winehq.org

July 21, 2016

Re: [PATCH 2/2] winex11: track glx windows per thread

by Miklós Máté

On 07/21/2016 05:39 AM, Alexandre Julliard wrote: > Miklós Máté <mtmkls(a)gmail.com> writes: > >> XCloseDisplay in thread_detach() closes all X windows, but the GLX windows >> remain, and become zombies. If a new X window is created with the same XID, >> Mesa refuses to attach a GLX window to it because of the zombie. Observed >> in the ddraw test. > Is there a reason that creating the GLX window on the window thread > wouldn't work? > The problem is that Wine uses XCloseDisplay to destroy all X windows created in that thread, but there is no equivalent functionality for GLX. If gdi_display = windata->display, it would remove the need for XSync, but the problem of zombie GLX windows would remain. I hope I understood your question correctly. MM

July 21, 2016

Re: [PATCH 2/3] shell32: Implement FolderItems_Item.

by Nikolay Sivov

On 21.07.2016 7:45, Alex Henrie wrote: > static HRESULT WINAPI FolderItemsImpl_Item(FolderItems3 *iface, VARIANT index, FolderItem **ppid) > { > - FIXME("(%p,%s,%p)\n", iface, debugstr_variant(&index), ppid); > + FolderItemsImpl *This = impl_from_FolderItems(iface); > + VARIANT path; > + struct list *node = list_head(&This->items); > + int i; > + > + TRACE("(%p,%s,%p)\n", iface, debugstr_variant(&index), ppid); > > *ppid = NULL; > - return E_NOTIMPL; > + > + if (!node) > + return S_FALSE; > + > + for (i = 0; i < V_I4(&index); i++) > + { > + node = list_next(&This->items, node); > + if (!node) > + return S_FALSE; > + } If it's only using indexed access, list is not a right data type.

July 21, 2016

Re: [PATCH 1/3] shell32/tests: Add more tests for FolderItems.

by Nikolay Sivov

On 21.07.2016 7:45, Alex Henrie wrote: > + for (i = 0; i < sizeof(file_defs)/sizeof(file_defs[0]); i++) > + { > + switch (file_defs[i].type) > + { > + case DIRECTORY: > + r = CreateDirectoryA(file_defs[i].name, NULL); > + ok(r, "file_defs[%d]: CreateDirectory failed: %08x\n", i, GetLastError()); > + break; > + > + case SHORTCUT: > + sl = NULL; > + r = CoCreateInstance(&CLSID_ShellLink, NULL, CLSCTX_INPROC_SERVER, &IID_IShellLinkA, (void**)&sl); > + ok(SUCCEEDED(r), "file_defs[%d]: CoCreateInstance failed: %08x\n", i, r); > + ok(!!sl, "sl is null\n"); > + > + shortcut = NULL; > + r = IShellLinkA_QueryInterface(sl, &IID_IPersistFile, (void**)&shortcut); > + ok(r == S_OK, "file_defs[%d]: IShellLink::QueryInterface failed: %08x\n", i, r); > + ok(!!shortcut, "file_defs[%d]: shortcut is null\n", i); > + > + MultiByteToWideChar(CP_ACP, 0, file_defs[i].name, -1, wstr, sizeof(wstr)/sizeof(WCHAR)); > + r = IPersistFile_Save(shortcut, wstr, FALSE); > + ok(r == S_OK, "file_defs[%d]: IPersistFile::Save failed: %08x\n", i, r); > + > + IPersistFile_Release(shortcut); > + IShellLinkA_Release(sl); If you're only interested in IPersistFile you can request it directly when calling CoCreateInstance(). > + r = FolderItems_QueryInterface(items, &IID_FolderItems2, (void**)&items2); > + ok(r == S_OK || broken(E_NOINTERFACE) /* xp and later */, "FolderItems::QueryInterface failed: %08x\n", r); > + ok(!!items2 || broken(!items2) /* xp and later */, "items2 is null\n"); Second ok() is a bit meaningless, because it's expected to be consistent with previous return value test. I think it could be placed into if (r == S_OK), with broken() removed. > + > + bstr = NULL; > + r = FolderItem_get_Path(item, &bstr); > + ok(r == S_OK, "file_defs[%d]: FolderItem::get_Path failed: %08x\n", i, r); > + ok(!!bstr, "file_defs[%d]: bstr is null\n", i); > + > + bstr2 = NULL; > + r = FolderItem_get_Path(item, &bstr2); > + ok(r == S_OK, "file_defs[%d]: FolderItem::get_Path failed: %08x\n", i, r); > + ok(bstr2 != bstr, "file_defs[%d]: bstr and bstr2 are the same\n", i); > + SysFreeString(bstr2); This is a regular convention for [out] BSTR arguments. It only makes sense to explicitly test it if it differs from expected behavior. > + > + V_I4(&var) = sizeof(file_defs)/sizeof(file_defs[0]); > + item = NULL; > + r = FolderItems_Item(items, var, &item); > +todo_wine > + ok(r == S_FALSE, "expected S_FALSE, got %08x\n", r); > + ok(!item, "item is not null\n"); > + > if (0) /* crashes on xp */ > { > r = FolderItems_get_Application(items, NULL); > @@ -422,12 +623,20 @@ todo_wine > ok(r == E_INVALIDARG, "expected E_INVALIDARG, got %08x\n", r); > } > > + verbs = (FolderItemVerbs*)0xdeadbeef; > r = FolderItems3_get_Verbs(items3, &verbs); > todo_wine > ok(r == S_FALSE, "expected S_FALSE, got %08x\n", r); > ok(!verbs, "verbs is not null\n"); > } > > + for (i = 0; i < sizeof(file_defs)/sizeof(file_defs[0]); i++) > + { > + if (file_defs[i].type == DIRECTORY) > + RemoveDirectoryA(file_defs[i].name); > + else > + DeleteFileA(file_defs[i].name); > + } Could this cleanup be merged with one of the loops you have earlier? >

July 21, 2016

Re: [PATCH 2/2] winex11: track glx windows per thread

by Alexandre Julliard

Miklós Máté <mtmkls(a)gmail.com> writes: > XCloseDisplay in thread_detach() closes all X windows, but the GLX windows > remain, and become zombies. If a new X window is created with the same XID, > Mesa refuses to attach a GLX window to it because of the zombie. Observed > in the ddraw test. Is there a reason that creating the GLX window on the window thread wouldn't work? -- Alexandre Julliard julliard(a)winehq.org

July 21, 2016

Re: [PATCH 3/4] quartz: Try all surfaces when calling GetSurface

by Jan Schmidt

On 21/07/16 01:14, Andrew Eikum wrote: > On Wed, Jul 20, 2016 at 02:37:11PM +1000, Jan Schmidt wrote: >> If the surface we want is refused, cycle through all >> possible ones. Some applications (Silver at least) >> request 2 surfaces, but then refuse to return anything >> other than the first one. >> > > I'm not necessarily opposed to this, but it does seem unlikely that > the application would request two surfaces be used, but only provide > one. > > I noticed that we set num_surfaces to 2 in VMR9_maybe_init. Is this > function calling IVMRSurfaceAllocatorEx9::InitializeDevice on the > application-provided allocator, or on our default implementation? This > seems like where the application should tell us it wants only one > surface. It calls the allocator the app provide earlier in VMR9SurfaceAllocatorNotify_AdviseSurfaceAllocator which is then passing it to VMR9SurfaceAllocatorNotify_AllocateSurfaceHelper to do the actual allocation. I think it's just passing the allocation info intact - including the MinBuffers and default buffers=2 value (verified by changing those). I think the app then only stores the first surface though. > If it is calling the application's allocator, then maybe they don't > handle VMR9AllocationInfo::MinBuffers > 1 very well. Why do we set > MinBuffers to 2 here, I wonder. Aric? Since we're asking design questions: It requests surfaces with VMR9AllocFlag_TextureSurface, but D3D doesn't seem to support 24-bit RGB textures. I only have 1 test case for the VMR9 renderer (Silver), and for that the filter graph plugs the GStreamer YUV2RGB component, which outputs 24-bit RGB. I'm using a local patch to output RGBA instead to make things work. My plan is to extend the TransformFilter base class and make the YUV2RGB filter negotiate with the VMR9 filter and do 32-bit anyway, but it seems the VMR9 filter should not advertise 24-bit support at all? J. > > Andrew > >> Signed-off-by: Jan Schmidt <jan(a)centricular.com> >> --- >> dlls/quartz/vmr9.c | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/dlls/quartz/vmr9.c b/dlls/quartz/vmr9.c >> index d590a71..8455678 100644 >> --- a/dlls/quartz/vmr9.c >> +++ b/dlls/quartz/vmr9.c >> @@ -292,6 +292,7 @@ static HRESULT WINAPI VMR9_DoRenderSample(BaseRenderer *iface, IMediaSample * pS >> REFERENCE_TIME tStart, tStop; >> VMR9PresentationInfo info; >> HRESULT hr; >> + int i; >> >> TRACE("%p %p\n", iface, pSample); >> >> @@ -338,10 +339,19 @@ static HRESULT WINAPI VMR9_DoRenderSample(BaseRenderer *iface, IMediaSample * pS >> info.szAspectRatio.cx = This->bmiheader.biWidth; >> info.szAspectRatio.cy = This->bmiheader.biHeight; >> >> - hr = IVMRSurfaceAllocatorEx9_GetSurface(This->allocator, This->cookie, (++This->cur_surface)%This->num_surfaces, 0, &info.lpSurf); >> + /* Sometimes apps refuse to return the surface we request, >> + * so try each and see if any are available if the first fails */ >> + for (i = 0; i < This->num_surfaces; i++) { >> + hr = IVMRSurfaceAllocatorEx9_GetSurface(This->allocator, This->cookie, >> + (++This->cur_surface) % This->num_surfaces, 0, &info.lpSurf); >> + if (SUCCEEDED (hr)) >> + break; >> + } >> >> - if (FAILED(hr)) >> + if (FAILED(hr)) { >> + ERR("Failed to get render surface (%x)\n", hr); >> return hr; >> + } >> >> VMR9_SendSampleData(This, &info, pbSrcStream, cbSrcStream); >> IDirect3DSurface9_Release(info.lpSurf); >> -- >> 2.7.4 >> >> >> -- Jan Schmidt, Centricular Ltd - http://centricular.com/

July 20, 2016

Re: [PATCH 1/5] wined3d: Fix immediate draws when ARB_vertex_array_bgra is not supported.

by Henri Verbeet

On 20 July 2016 at 16:21, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: > It's a bit more convoluted than that. When ARB_vertex_array_bgra is > not supported, FFP draws usually fall back to immediate mode because > of the check in context_update_stream_info(). There is no shader > swizzling in that case [...] Yes, but I'd call that a bug. (As an aside, some of those fall-backs to immediate mode could perhaps be avoided by taking stream_info->swizzle_mask into account. It's probably not worth it because diffuse/specular almost always use D3DCOLOR.) > [...] so we need to swizzle when uploading the > attribute. If ARB_vertex_array_bgra is supported we never need to > swizzle the attribute [...] You do for immediate mode draws. Data is stored in BGRA order, which is expected to be mapped to "zyxw" in shaders. If ARB_vertex_array_bgra is supported, the shader backend assumes this happens on upload, either through GL_BGRA for array draws or through manual swizzling for immediate mode draws. If ARB_vertex_array_bgra is not supported, the shader backend does it by itself. (The decision whether the shader needs to swizzle ultimately happens in context_stream_info_from_declaration() by setting bits in the "swizzle_map".) > [...] so the above change seems at least a fix for > that case (although if ARB_vertex_array_bgra is supported we generally > don't fallback to immediate mode either, so that doesn't matter much > in practice). > context_update_stream_info() only checks for DIFFUSE and SPECULAR > though so I guess if other attributes use D3DCOLOR we're currently > screwed. But D3DCOLOR / WINED3DFMT_B8G8R8A8_UNORM is not valid for a lot of other attributes for fixed function draws. (See also declaration_element_valid_ffp().) I notice it's also allowed for WINED3D_DECL_USAGE_BLEND_WEIGHT though, which may actually get used these days. > The immediate mode check is in the !VS-only branch so it would not > apply to non-FFP draws. Those get the shader swizzle instead and thus > work fine. Yeah, that's the reason this mostly works in practice, but I think it's fragile to depend on that. I also think it makes it a little more confusing when you need to swizzle on upload and when you don't. I.e., the current rule is perhaps somewhat under-documented, but I think it's a good thing it's fairly simple. > I guess there is an exception here too which is instanced > draws when both ARB_vertex_array_bgra and ARB_instanced_arrays are > missing AND one of the instance attributes is D3DCOLOR. You can also end up with immediate mode shader draws when "half_float_conv_needed" is set on the vertex declaration.

July 20, 2016

Re: [PATCH 3/4] quartz: Try all surfaces when calling GetSurface

by Andrew Eikum

On Wed, Jul 20, 2016 at 02:37:11PM +1000, Jan Schmidt wrote: > If the surface we want is refused, cycle through all > possible ones. Some applications (Silver at least) > request 2 surfaces, but then refuse to return anything > other than the first one. > I'm not necessarily opposed to this, but it does seem unlikely that the application would request two surfaces be used, but only provide one. I noticed that we set num_surfaces to 2 in VMR9_maybe_init. Is this function calling IVMRSurfaceAllocatorEx9::InitializeDevice on the application-provided allocator, or on our default implementation? This seems like where the application should tell us it wants only one surface. If it is calling the application's allocator, then maybe they don't handle VMR9AllocationInfo::MinBuffers > 1 very well. Why do we set MinBuffers to 2 here, I wonder. Aric? Andrew > Signed-off-by: Jan Schmidt <jan(a)centricular.com> > --- > dlls/quartz/vmr9.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/dlls/quartz/vmr9.c b/dlls/quartz/vmr9.c > index d590a71..8455678 100644 > --- a/dlls/quartz/vmr9.c > +++ b/dlls/quartz/vmr9.c > @@ -292,6 +292,7 @@ static HRESULT WINAPI VMR9_DoRenderSample(BaseRenderer *iface, IMediaSample * pS > REFERENCE_TIME tStart, tStop; > VMR9PresentationInfo info; > HRESULT hr; > + int i; > > TRACE("%p %p\n", iface, pSample); > > @@ -338,10 +339,19 @@ static HRESULT WINAPI VMR9_DoRenderSample(BaseRenderer *iface, IMediaSample * pS > info.szAspectRatio.cx = This->bmiheader.biWidth; > info.szAspectRatio.cy = This->bmiheader.biHeight; > > - hr = IVMRSurfaceAllocatorEx9_GetSurface(This->allocator, This->cookie, (++This->cur_surface)%This->num_surfaces, 0, &info.lpSurf); > + /* Sometimes apps refuse to return the surface we request, > + * so try each and see if any are available if the first fails */ > + for (i = 0; i < This->num_surfaces; i++) { > + hr = IVMRSurfaceAllocatorEx9_GetSurface(This->allocator, This->cookie, > + (++This->cur_surface) % This->num_surfaces, 0, &info.lpSurf); > + if (SUCCEEDED (hr)) > + break; > + } > > - if (FAILED(hr)) > + if (FAILED(hr)) { > + ERR("Failed to get render surface (%x)\n", hr); > return hr; > + } > > VMR9_SendSampleData(This, &info, pbSrcStream, cbSrcStream); > IDirect3DSurface9_Release(info.lpSurf); > -- > 2.7.4 > > >

July 20, 2016

Re: [PATCH 1/5] wined3d: Fix immediate draws when ARB_vertex_array_bgra is not supported.

by Matteo Bruni

2016-07-20 10:13 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>: > On 20 July 2016 at 00:33, Matteo Bruni <mbruni(a)codeweavers.com> wrote: >> @@ -5848,7 +5850,7 @@ static void wined3d_adapter_init_ffp_attrib_ops(struct wined3d_adapter *adapter) >> ops->generic[WINED3D_FFP_EMIT_FLOAT2] = (wined3d_generic_attrib_func)gl_info->gl_ops.ext.p_glVertexAttrib2fv; >> ops->generic[WINED3D_FFP_EMIT_FLOAT3] = (wined3d_generic_attrib_func)gl_info->gl_ops.ext.p_glVertexAttrib3fv; >> ops->generic[WINED3D_FFP_EMIT_FLOAT4] = (wined3d_generic_attrib_func)gl_info->gl_ops.ext.p_glVertexAttrib4fv; >> - if (gl_info->supported[ARB_VERTEX_ARRAY_BGRA]) >> + if (!gl_info->supported[ARB_VERTEX_ARRAY_BGRA]) >> ops->generic[WINED3D_FFP_EMIT_D3DCOLOR] = generic_d3dcolor; >> else >> ops->generic[WINED3D_FFP_EMIT_D3DCOLOR] = > This looks wrong. When ARB_vertex_array_bgra is not supported, shaders > swizzle the corresponding input registers (see also > shader_glsl_swizzle_to_str() and shader_arb_get_swizzle()), so the > attribute upload code shouldn't. It's a bit more convoluted than that. When ARB_vertex_array_bgra is not supported, FFP draws usually fall back to immediate mode because of the check in context_update_stream_info(). There is no shader swizzling in that case so we need to swizzle when uploading the attribute. If ARB_vertex_array_bgra is supported we never need to swizzle the attribute so the above change seems at least a fix for that case (although if ARB_vertex_array_bgra is supported we generally don't fallback to immediate mode either, so that doesn't matter much in practice). context_update_stream_info() only checks for DIFFUSE and SPECULAR though so I guess if other attributes use D3DCOLOR we're currently screwed. The immediate mode check is in the !VS-only branch so it would not apply to non-FFP draws. Those get the shader swizzle instead and thus work fine. I guess there is an exception here too which is instanced draws when both ARB_vertex_array_bgra and ARB_instanced_arrays are missing AND one of the instance attributes is D3DCOLOR. Eventually for GL ES (which I think doesn't have anything equivalent to ARB_vertex_array_bgra) we'll need to always handle this in the shader since we can't fallback to immediate mode there. I guess I'll try to write a patch going in that direction since it should also clean things up WRT the above cases on desktop OpenGL.

July 20, 2016

Re: [5/5] msvcirt: Implement istream::getdouble

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=24393 Your paranoid android. === wxppro (32 bit msvcirt) === msvcirt.c:4839: Test failed: Test 48: wrong return, expected 4 got 5 msvcirt.c:4839: Test failed: Test 49: wrong return, expected 4 got 5 msvcirt.c:4839: Test failed: Test 50: wrong return, expected 4 got 5 msvcirt.c:4839: Test failed: Test 51: wrong return, expected 4 got 5 msvcirt.c:4841: Test failed: Test 52: expected 2 got 0 msvcirt.c:4843: Test failed: Test 52: expected 003E601D got 003E601C msvcirt.c:4839: Test failed: Test 54: wrong return, expected 4 got 5 msvcirt.c:4839: Test failed: Test 57: wrong return, expected -1 got 0 msvcirt.c:4839: Test failed: Test 58: wrong return, expected -1 got 0 msvcirt.c:4839: Test failed: Test 59: wrong return, expected -1 got 0 msvcirt.c:4839: Test failed: Test 61: wrong return, expected 0 got 1 msvcirt.c:4839: Test failed: Test 62: wrong return, expected 1 got 2 === w2003std (32 bit msvcirt) === msvcirt.c:4839: Test failed: Test 48: wrong return, expected 4 got 5 msvcirt.c:4839: Test failed: Test 49: wrong return, expected 4 got 5 msvcirt.c:4839: Test failed: Test 50: wrong return, expected 4 got 5 msvcirt.c:4839: Test failed: Test 51: wrong return, expected 4 got 5 msvcirt.c:4841: Test failed: Test 52: expected 2 got 0 msvcirt.c:4843: Test failed: Test 52: expected 003D60B5 got 003D60B4 msvcirt.c:4839: Test failed: Test 54: wrong return, expected 4 got 5 msvcirt.c:4839: Test failed: Test 57: wrong return, expected -1 got 0 msvcirt.c:4839: Test failed: Test 58: wrong return, expected -1 got 0 msvcirt.c:4839: Test failed: Test 59: wrong return, expected -1 got 0 msvcirt.c:4839: Test failed: Test 61: wrong return, expected 0 got 1 msvcirt.c:4839: Test failed: Test 62: wrong return, expected 1 got 2

July 20, 2016

Re: [PATCH 2/4] quartz: Images in Direct3D surfaces must be top-down

by Jan Schmidt

On 20/07/16 20:23, Henri Verbeet wrote: > On 20 July 2016 at 11:48, Jan Schmidt <jan(a)centricular.com> wrote: >>> If it isn't, it may be better to just flip the image when presenting it. >> >> Do you mean when presenting inside D3D? > I was thinking about VMR9_ImagePresenter_PresentTexture() and > VMR9_ImagePresenter_PresentOffscreenSurface(), but reading up a bit > about vmr9, those wouldn't be used in renderless mode. That also means > this is likely the right way to handle this, although tests are still > a good thing. > Ah, right - yes. J. -- Jan Schmidt, Centricular Ltd - http://centricular.com/

July 20, 2016

Re: [resend PATCH] msscript: Implement SetAdvise/GetAdvise

by Nikolay Sivov

On 20.07.2016 12:42, Jacek Caban wrote: > Hi Nikolay, > > On 19.07.2016 20:42, Nikolay Sivov wrote: >> -static HRESULT WINAPI ViewObject_SetAdvise(IViewObjectEx *iface, DWORD aspects, DWORD advf, IAdviseSink *sink) >> +static HRESULT WINAPI ViewObject_SetAdvise(IViewObjectEx *iface, DWORD aspects, DWORD flags, IAdviseSink *sink) >> { >> ScriptControl *This = impl_from_IViewObjectEx(iface); >> >> - FIXME("(%p)->(%d %d %p)\n", This, aspects, advf, sink); >> + TRACE("(%p)->(%d %#x %p)\n", This, aspects, flags, sink); >> >> - return E_NOTIMPL; >> + if (aspects != DVASPECT_CONTENT) >> + return DV_E_DVASPECT; > > Since only DVASPECT_CONTENT is supported and it's also a default value, > is there a reason to bother storing it in the object? No, no reason. Thanks. > > Thanks, > Jacek > >

July 20, 2016

Re: [PATCH] kernel32: Implement SetConsoleFont

by Hugh McMaster

On Wednesday, 20 July 2016 12:04 AM, Sebastian Lackner wrote: > On 19.07.2016 15:50, Hugh McMaster wrote: > > We decided a while back to only have one font in our font table. I guess that makes > > SetConsoleFont redundant for the moment. If we were to implement support for multiple file > > sizes, I'm not sure where that support would reside. Kernel32 is too low-level, as > > enumerating fonts uses GDI32. Any ideas? > > > > Also note that the Windows font table includes the console raster font and the fonts > > listed in LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Console\TrueTypeFont. > > > > We list nothing in that registry key, and wineconsole supports all fonts that meet > > the fixed width criteria, making this a huge task. > > Not sure if its feasible, but you could probably implement the enumeration in wineconsole. > The table could then (for example) be stored in a temporary registry key (similar to how font > caching in gdi32 works). In general, I think we could just enumerate appropriate font sizes for the active console font. Storing the values (i.e. the font sizes) in a temporary registry key seems to me like a good idea. Those values would be accessible from kernel32 because the registry calls are forwarded to advapi32.

July 20, 2016

Re: [PATCH 2/4] quartz: Images in Direct3D surfaces must be top-down

by Henri Verbeet

On 20 July 2016 at 11:48, Jan Schmidt <jan(a)centricular.com> wrote: >> If it isn't, it may be better to just flip the image when presenting it. > > Do you mean when presenting inside D3D? I was thinking about VMR9_ImagePresenter_PresentTexture() and VMR9_ImagePresenter_PresentOffscreenSurface(), but reading up a bit about vmr9, those wouldn't be used in renderless mode. That also means this is likely the right way to handle this, although tests are still a good thing.

July 20, 2016

Re: [PATCH 2/4] quartz: Images in Direct3D surfaces must be top-down

by Jan Schmidt

On 20/07/16 18:47, Henri Verbeet wrote: > On 20 July 2016 at 06:37, Jan Schmidt <jan(a)centricular.com> wrote: >> Invert images when copying to a Direct3D surface if >> they are provided bottom-up. >> > Is that observable behaviour? I.e., could you write a test that > verifies where the data ends up in the surface? I think one could construct a test. It'd be slightly elaborate - Configure a Quartz VMR9 renderer to use a custom D3D allocator / presenter, feeding a video buffer media sample to it, and then read the D3D surface contents and verify that the received D3D surface is the right way up. I'm not entirely sure how to write that test yet though - I'm a GStreamer dev, and I've only ever looked at Wine code in the last 5 days. > If it isn't, it may be better to just flip the image when presenting it. Do you mean when presenting inside D3D? I think achieving that would be tricky - you'd need to mark the surface as vertically inverted and handle that anywhere the contents might be accessed inside D3D, or when handing out pointers to the data via API like IDirect3DSurface9_LockRect. I'm guessing though - I don't really know much about D3D - I based this patch off https://msdn.microsoft.com/en-us/library/windows/desktop/aa473780(v=vs.85).… which says "any image that is contained in a Direct3D surface must be top-down" - Jan. -- Jan Schmidt, Centricular Ltd - http://centricular.com/

July 20, 2016

Re: [resend PATCH] msscript: Implement SetAdvise/GetAdvise

by Jacek Caban

Hi Nikolay, On 19.07.2016 20:42, Nikolay Sivov wrote: > -static HRESULT WINAPI ViewObject_SetAdvise(IViewObjectEx *iface, DWORD aspects, DWORD advf, IAdviseSink *sink) > +static HRESULT WINAPI ViewObject_SetAdvise(IViewObjectEx *iface, DWORD aspects, DWORD flags, IAdviseSink *sink) > { > ScriptControl *This = impl_from_IViewObjectEx(iface); > > - FIXME("(%p)->(%d %d %p)\n", This, aspects, advf, sink); > + TRACE("(%p)->(%d %#x %p)\n", This, aspects, flags, sink); > > - return E_NOTIMPL; > + if (aspects != DVASPECT_CONTENT) > + return DV_E_DVASPECT; Since only DVASPECT_CONTENT is supported and it's also a default value, is there a reason to bother storing it in the object? Thanks, Jacek

July 20, 2016

Re: hlink/tests: Use wine_dbgstr_guid() to print a REFIID.

by Michael Stefaniuc

On 07/20/2016 10:42 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=24381 > > Your paranoid android. > > > === w1064 (32 bit hlink) === > hlink.c:414: Test failed: url only: Saved data differs > hlink.c:414: Test failed: url + friendly name: Saved data differs > hlink.c:414: Test failed: url + friendly_name + location: Saved data differs > hlink.c:414: Test failed: url + target frame name: Saved data differs > > === w1064 (64 bit hlink) === > hlink.c:414: Test failed: url only: Saved data differs > hlink.c:414: Test failed: url + friendly name: Saved data differs > hlink.c:414: Test failed: url + friendly_name + location: Saved data differs > hlink.c:414: Test failed: url + target frame name: Saved data differs This are existing test failures. bye michael

July 20, 2016

Re: [PATCH 2/4] quartz: Images in Direct3D surfaces must be top-down

by Henri Verbeet

On 20 July 2016 at 06:37, Jan Schmidt <jan(a)centricular.com> wrote: > Invert images when copying to a Direct3D surface if > they are provided bottom-up. > Is that observable behaviour? I.e., could you write a test that verifies where the data ends up in the surface? If it isn't, it may be better to just flip the image when presenting it.

July 20, 2016

Re: hlink/tests: Use wine_dbgstr_guid() to print a REFIID.

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=24381 Your paranoid android. === w1064 (32 bit hlink) === hlink.c:414: Test failed: url only: Saved data differs hlink.c:414: Test failed: url + friendly name: Saved data differs hlink.c:414: Test failed: url + friendly_name + location: Saved data differs hlink.c:414: Test failed: url + target frame name: Saved data differs === w1064 (64 bit hlink) === hlink.c:414: Test failed: url only: Saved data differs hlink.c:414: Test failed: url + friendly name: Saved data differs hlink.c:414: Test failed: url + friendly_name + location: Saved data differs hlink.c:414: Test failed: url + target frame name: Saved data differs

July 20, 2016

Re: [PATCH 1/5] wined3d: Fix immediate draws when ARB_vertex_array_bgra is not supported.

by Henri Verbeet

On 20 July 2016 at 00:33, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > @@ -5848,7 +5850,7 @@ static void wined3d_adapter_init_ffp_attrib_ops(struct wined3d_adapter *adapter) > ops->generic[WINED3D_FFP_EMIT_FLOAT2] = (wined3d_generic_attrib_func)gl_info->gl_ops.ext.p_glVertexAttrib2fv; > ops->generic[WINED3D_FFP_EMIT_FLOAT3] = (wined3d_generic_attrib_func)gl_info->gl_ops.ext.p_glVertexAttrib3fv; > ops->generic[WINED3D_FFP_EMIT_FLOAT4] = (wined3d_generic_attrib_func)gl_info->gl_ops.ext.p_glVertexAttrib4fv; > - if (gl_info->supported[ARB_VERTEX_ARRAY_BGRA]) > + if (!gl_info->supported[ARB_VERTEX_ARRAY_BGRA]) > ops->generic[WINED3D_FFP_EMIT_D3DCOLOR] = generic_d3dcolor; > else > ops->generic[WINED3D_FFP_EMIT_D3DCOLOR] = This looks wrong. When ARB_vertex_array_bgra is not supported, shaders swizzle the corresponding input registers (see also shader_glsl_swizzle_to_str() and shader_arb_get_swizzle()), so the attribute upload code shouldn't.

July 20, 2016

[v2 PATCH] d3dx10: Add D3DXCpuOptimizations stub

by Andrey Gusev

+#include <d3dx10.h> Please change to "d3dx10.h"

July 20, 2016

[v2 PATCH] d3dx10: Add D3DXCpuOptimizations stub

by Andrey Gusev

+#include <d3dx10.h> Please change to "d3dx10.h"

July 20, 2016

Re: [PATCH] d3dx10: Add D3DXCpuOptimizations stub

by Alistair Leslie-Hughes

On 20/07/16 15:04, Alex Henrie wrote: > + * Copyright (C) 2007 David Adam > + * Copyright (C) 2007 Tony Wasserka > > Did these people actually contribute anything to this patch? No, it was a copy from d3dx9math.h > Also, D3DX_CPU_OPTIMIZATION was already defined in d3dx9_30_main.c. I > would remove the old definition from d3dx9_30_main.c and make it > include d3dx10math.h instead. > To remove the D3DX_CPU_OPTIMIZATION definition would be a separate patch in it's own right. I will have to workout where it should be defined for d3dx9. Including a dx10 header in dx9 seems like a bad idea. Best Regards Alistair Leslie-Hughes

July 20, 2016

Re: [PATCH] d3dx10: Add D3DXCpuOptimizations stub

by Alex Henrie

+ * Copyright (C) 2007 David Adam + * Copyright (C) 2007 Tony Wasserka Did these people actually contribute anything to this patch? +/* This guaud is the same as D3DX9 to prevent double-inclusion */ Did you mean "guard"? Also, D3DX_CPU_OPTIMIZATION was already defined in d3dx9_30_main.c. I would remove the old definition from d3dx9_30_main.c and make it include d3dx10math.h instead. -Alex

July 20, 2016

Re: [PATCH 02/11] mshtml: Use bind_to_tree callback to parse dynamically added scripts.

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=24357 Your paranoid android. === wxppro (32 bit) === Found no trace of the test summary line or of a crash === w2003std (32 bit) === Found no trace of the test summary line or of a crash === wvistau64 (32 bit) === Found no trace of the test summary line or of a crash === w2008s64 (32 bit) === Found no trace of the test summary line or of a crash === w7u (32 bit) === Found no trace of the test summary line or of a crash === w7pro64 (32 bit) === Found no trace of the test summary line or of a crash === w8 (32 bit) === Found no trace of the test summary line or of a crash === w864 (32 bit) === Found no trace of the test summary line or of a crash === w1064 (32 bit) === Found no trace of the test summary line or of a crash === wvistau64 (64 bit) === Found no trace of the test summary line or of a crash === w2008s64 (64 bit) === Found no trace of the test summary line or of a crash === w7pro64 (64 bit) === Found no trace of the test summary line or of a crash === w864 (64 bit) === Found no trace of the test summary line or of a crash === w1064 (64 bit) === Found no trace of the test summary line or of a crash

July 19, 2016

Re: [PATCH 01/11] mshtml/tests: Use relative URLs in navigation.js.

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=24356 Your paranoid android. === wxppro (32 bit) === Found no trace of the test summary line or of a crash === w2003std (32 bit) === Found no trace of the test summary line or of a crash === wvistau64 (32 bit) === Found no trace of the test summary line or of a crash === w2008s64 (32 bit) === Found no trace of the test summary line or of a crash === w7u (32 bit) === Found no trace of the test summary line or of a crash === w7pro64 (32 bit) === Found no trace of the test summary line or of a crash === w8 (32 bit) === Found no trace of the test summary line or of a crash === w864 (32 bit) === Found no trace of the test summary line or of a crash === w1064 (32 bit) === Found no trace of the test summary line or of a crash === wvistau64 (64 bit) === Found no trace of the test summary line or of a crash === w2008s64 (64 bit) === Found no trace of the test summary line or of a crash === w7pro64 (64 bit) === Found no trace of the test summary line or of a crash === w864 (64 bit) === Found no trace of the test summary line or of a crash === w1064 (64 bit) === Found no trace of the test summary line or of a crash

July 19, 2016

Re: [PATCH] msscript: Implement SetAdvise/GetAdvise

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=24348 Your paranoid android. === build (build) === Patch failed to apply

July 19, 2016

Re: [PATCH] kernel32: Implement SetConsoleFont

by Sebastian Lackner

On 19.07.2016 15:50, Hugh McMaster wrote: > On Tuesday, 19 July 2016 10:52 PM, Sebastian Lackner wrote: >> It looks like your code requests the font size from the wineserver (GetConsoleFontSize) >> just to send it back immediately afterwards. Is that intentional?And wouldn't it make >> more sense to implement support for multiple font sizes first? > > SetConsoleFont uses the index to look up the relevant font size from the system font table, > which is undocumented. Calling GetConsoleFontSize is intentional, although we could look > up the font table ourselves, I suppose. > This method might work in the future (when GetConsoleFontSize is properly implemented), but as long as there is only one entry, its basically just a complicated looking stub. ;) Later, when SetConsoleFont() allows changing the font, it will probably also be necessary to queue a notification event such that wineconsole can update accordingly. > We decided a while back to only have one font in our font table. I guess that makes > SetConsoleFont redundant for the moment. If we were to implement support for multiple file > sizes, I'm not sure where that support would reside. Kernel32 is too low-level, as > enumerating fonts uses GDI32. Any ideas? > > Also note that the Windows font table includes the console raster font and the fonts > listed in LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Console\TrueTypeFont. > > We list nothing in that registry key, and wineconsole supports all fonts that meet > the fixed width criteria, making this a huge task. Not sure if its feasible, but you could probably implement the enumeration in wineconsole. The table could then (for example) be stored in a temporary registry key (similar to how font caching in gdi32 works). > >> Also, please note that the GetNumberOfConsoleFonts() check is unnecessary because >> GetConsoleFontSize() already takes care of that. > > True, good point. > > Thanks for the review. >

July 19, 2016

Re: [PATCH] kernel32: Implement SetConsoleFont

by Hugh McMaster

On Tuesday, 19 July 2016 10:52 PM, Sebastian Lackner wrote: > It looks like your code requests the font size from the wineserver (GetConsoleFontSize) > just to send it back immediately afterwards. Is that intentional?And wouldn't it make > more sense to implement support for multiple font sizes first? SetConsoleFont uses the index to look up the relevant font size from the system font table, which is undocumented. Calling GetConsoleFontSize is intentional, although we could look up the font table ourselves, I suppose. We decided a while back to only have one font in our font table. I guess that makes SetConsoleFont redundant for the moment. If we were to implement support for multiple file sizes, I'm not sure where that support would reside. Kernel32 is too low-level, as enumerating fonts uses GDI32. Any ideas? Also note that the Windows font table includes the console raster font and the fonts listed in LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Console\TrueTypeFont. We list nothing in that registry key, and wineconsole supports all fonts that meet the fixed width criteria, making this a huge task. > Also, please note that the GetNumberOfConsoleFonts() check is unnecessary because > GetConsoleFontSize() already takes care of that. True, good point. Thanks for the review.

July 19, 2016

Re: comctl32: Get rid of TOOLTIPS_SetMargin(), it is just a RECT assignment.

by Nikolay Sivov

On 19.07.2016 10:53, Michael Stefaniuc wrote: > Signed-off-by: Michael Stefaniuc <mstefani(a)redhat.de> > --- > dlls/comctl32/tooltips.c | 15 ++------------- > 1 file changed, 2 insertions(+), 13 deletions(-) > > diff --git a/dlls/comctl32/tooltips.c b/dlls/comctl32/tooltips.c > index eed113e..a0c7ade 100644 > --- a/dlls/comctl32/tooltips.c > +++ b/dlls/comctl32/tooltips.c > @@ -1599,18 +1599,6 @@ TOOLTIPS_SetDelayTime (TOOLTIPS_INFO *infoPtr, DWORD duration, INT nTime) > } > > > -static LRESULT > -TOOLTIPS_SetMargin (TOOLTIPS_INFO *infoPtr, const RECT *lpRect) > -{ > - infoPtr->rcMargin.left = lpRect->left; > - infoPtr->rcMargin.right = lpRect->right; > - infoPtr->rcMargin.bottom = lpRect->bottom; > - infoPtr->rcMargin.top = lpRect->top; > - > - return 0; > -} > - > - > static inline LRESULT > TOOLTIPS_SetMaxTipWidth (TOOLTIPS_INFO *infoPtr, INT MaxWidth) > { > @@ -2261,7 +2249,8 @@ TOOLTIPS_WindowProc (HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam) > return TOOLTIPS_SetDelayTime (infoPtr, (DWORD)wParam, (INT)LOWORD(lParam)); > > case TTM_SETMARGIN: > - return TOOLTIPS_SetMargin (infoPtr, (LPRECT)lParam); > + infoPtr->rcMargin = *(RECT*)lParam; > + return 0; > > case TTM_SETMAXTIPWIDTH: > return TOOLTIPS_SetMaxTipWidth (infoPtr, (INT)lParam); > I think this needs test to see if any rectangle is accepted, for example with negative values. If we need to validate it, it's easier to do in a helper to match the rest of code.

July 19, 2016

Re: [PATCH] kernel32: Implement SetConsoleFont

by Sebastian Lackner

On 19.07.2016 14:34, Hugh McMaster wrote: > Signed-off-by: Hugh McMaster <hugh.mcmaster(a)outlook.com> > --- > dlls/kernel32/console.c | 27 ++++++++++++++++++++++++--- > dlls/kernel32/tests/console.c | 6 +++--- > 2 files changed, 27 insertions(+), 6 deletions(-) > It looks like your code requests the font size from the wineserver (GetConsoleFontSize) just to send it back immediately afterwards. Is that intentional? And wouldn't it make more sense to implement support for multiple font sizes first? Also, please note that the GetNumberOfConsoleFonts() check is unnecessary because GetConsoleFontSize() already takes care of that.

July 19, 2016

Re: [PATCH] kernel32: Implement SetConsoleFont

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=24347 Your paranoid android. === w8 (32 bit console) === console.c:1445: Test failed: [13] Expected output count to be 0, got 2884226813 console.c:1445: Test failed: [15] Expected output count to be 0, got 2884226813 console.c:1445: Test failed: [18] Expected output count to be 3735928559, got 2884226813 console.c:1689: Test failed: [13] Expected output count to be 0, got 2884226813 console.c:1689: Test failed: [15] Expected output count to be 0, got 2884226813 console.c:1689: Test failed: [18] Expected output count to be 3735928559, got 2884226813 === w864 (32 bit console) === console.c:2567: Test failed: expected ERROR_INVALID_HANDLE, got 1 console.c:2573: Test failed: expected ERROR_NOT_ENOUGH_MEMORY, got 998 console.c:2580: Test failed: expected ERROR_NOT_ENOUGH_MEMORY, got 998 console.c:2587: Test failed: expected ERROR_NOT_ENOUGH_MEMORY, got 998 console.c:1270: Test failed: expected VerifyConsoleIoHandle to succeed console.c:1445: Test failed: [3] Expected output count to be 3735928559, got 0 console.c:1445: Test failed: [11] Expected output count to be 3735928559, got 0 console.c:1445: Test failed: [13] Expected output count to be 0, got 1106048744 console.c:1445: Test failed: [15] Expected output count to be 0, got 1106048744 console.c:1445: Test failed: [18] Expected output count to be 3735928559, got 1106048744 console.c:1689: Test failed: [3] Expected output count to be 3735928559, got 0 console.c:1689: Test failed: [11] Expected output count to be 3735928559, got 0 console.c:1689: Test failed: [13] Expected output count to be 0, got 1106048744 console.c:1689: Test failed: [15] Expected output count to be 0, got 1106048744 console.c:1689: Test failed: [18] Expected output count to be 3735928559, got 1106048744 console.c:2148: Test failed: [7] Expected count to be 0, got 1 console.c:2214: Test failed: [7] Expected count to be 0, got 1 console.c:2280: Test failed: [7] Expected count to be 0, got 1 === w1064 (32 bit console) === console.c:2567: Test failed: expected ERROR_INVALID_HANDLE, got 1 console.c:2573: Test failed: expected ERROR_NOT_ENOUGH_MEMORY, got 998 console.c:2580: Test failed: expected ERROR_NOT_ENOUGH_MEMORY, got 998 console.c:2587: Test failed: expected ERROR_NOT_ENOUGH_MEMORY, got 998 console.c:1270: Test failed: expected VerifyConsoleIoHandle to succeed console.c:1445: Test failed: [3] Expected output count to be 3735928559, got 0 console.c:1445: Test failed: [11] Expected output count to be 3735928559, got 0 console.c:1445: Test failed: [13] Expected output count to be 0, got 131584522 console.c:1445: Test failed: [15] Expected output count to be 0, got 131584522 console.c:1445: Test failed: [18] Expected output count to be 3735928559, got 131584522 console.c:1689: Test failed: [3] Expected output count to be 3735928559, got 0 console.c:1689: Test failed: [11] Expected output count to be 3735928559, got 0 console.c:1689: Test failed: [13] Expected output count to be 0, got 131584522 console.c:1689: Test failed: [15] Expected output count to be 0, got 131584522 console.c:1689: Test failed: [18] Expected output count to be 3735928559, got 131584522 console.c:2148: Test failed: [7] Expected count to be 0, got 1 console.c:2214: Test failed: [7] Expected count to be 0, got 1 console.c:2280: Test failed: [7] Expected count to be 0, got 1 === w864 (64 bit console) === console.c:2567: Test failed: expected ERROR_INVALID_HANDLE, got 1 console.c:2573: Test failed: expected ERROR_NOT_ENOUGH_MEMORY, got 998 console.c:2580: Test failed: expected ERROR_NOT_ENOUGH_MEMORY, got 998 console.c:2587: Test failed: expected ERROR_NOT_ENOUGH_MEMORY, got 998 console.c:1270: Test failed: expected VerifyConsoleIoHandle to succeed console.c:1445: Test failed: [3] Expected output count to be 3735928559, got 0 console.c:1445: Test failed: [11] Expected output count to be 3735928559, got 0 console.c:1445: Test failed: [18] Expected output count to be 3735928559, got 0 console.c:1689: Test failed: [3] Expected output count to be 3735928559, got 0 console.c:1689: Test failed: [11] Expected output count to be 3735928559, got 0 console.c:1689: Test failed: [18] Expected output count to be 3735928559, got 0 console.c:2148: Test failed: [7] Expected count to be 0, got 1 console.c:2214: Test failed: [7] Expected count to be 0, got 1 console.c:2280: Test failed: [7] Expected count to be 0, got 1 === w1064 (64 bit console) === console.c:2567: Test failed: expected ERROR_INVALID_HANDLE, got 1 console.c:2573: Test failed: expected ERROR_NOT_ENOUGH_MEMORY, got 998 console.c:2580: Test failed: expected ERROR_NOT_ENOUGH_MEMORY, got 998 console.c:2587: Test failed: expected ERROR_NOT_ENOUGH_MEMORY, got 998 console.c:1270: Test failed: expected VerifyConsoleIoHandle to succeed console.c:1445: Test failed: [3] Expected output count to be 3735928559, got 0 console.c:1445: Test failed: [11] Expected output count to be 3735928559, got 0 console.c:1445: Test failed: [18] Expected output count to be 3735928559, got 0 console.c:1689: Test failed: [3] Expected output count to be 3735928559, got 0 console.c:1689: Test failed: [11] Expected output count to be 3735928559, got 0 console.c:1689: Test failed: [18] Expected output count to be 3735928559, got 0 console.c:2148: Test failed: [7] Expected count to be 0, got 1 console.c:2214: Test failed: [7] Expected count to be 0, got 1 console.c:2280: Test failed: [7] Expected count to be 0, got 1

July 19, 2016

Re: [1/3] wininet: Don't return available data sizes larger than the read buffer.

by Hans Leidekker

On Tue, 2016-07-19 at 04:38 -0500, Marvin wrote: > 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=24338 > > Your paranoid android. > > > === w864 (64 bit http) === > http.c:5309: Test failed: HttpSendRequest failed: 8 This is a connection failure in test_secure_connection. It didn't occur in the next test run: https://testbot.winehq.org/JobDetails.pl?Key=24339

July 19, 2016

Re: [1/3] wininet: Don't return available data sizes larger than the read buffer.

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=24338 Your paranoid android. === w864 (64 bit http) === http.c:5309: Test failed: HttpSendRequest failed: 8

July 19, 2016

Re: [v2 1/1] dinput: Sort virtual joysticks by name on the Mac

by Ken Thomases

On Jul 19, 2016, at 2:59 AM, DavidL <david.dljunk(a)gmail.com> wrote: > > On Mon, Jul 18, 2016 at 10:59 PM, Ken Thomases <ken(a)codeweavers.com <mailto:ken(a)codeweavers.com>> wrote: > On Jul 18, 2016, at 1:57 AM, David Lawrie <david.dljunk(a)gmail.com <mailto:david.dljunk(a)gmail.com>> wrote: > > > + else if (ref) > > + name = CFCopyDescription(ref); > > + else > > + name = CFStringCreateCopy(kCFAllocatorDefault,CFSTR("(null)")); > > You don't want to fall back to a string based on the vendor and product ID? > > I thought about it, but then I wanted to try to hew as close to the behavior of debugstr_cf as possible since that is the output that the user sees already in trace logs. I don't think it's particularly important that they match to that degree. First, users generally don't see the trace logs. Second, it's good enough that they match when the product property is non-null. There's no strong reason to make them match for the null case. It's fine to have this function return something more useful than that, especially since it's currently used as a sort key and "(null)" is useless for that. > This label "(null)" is how debugstr handles that case. The alternative of course is changing both functions to try to get the venture/product ID - though I think the concern is if it doesn't have aProductKey it may not have a proper productId or vendorID either I have no objection if you want to make debugstr_device() also include the vendor/product ID as a fallback, but I don't think it's especially important. The result from copy_device_name() is not currently logged, so there's no way for a reader of the logs to benefit if the two functions match in that way, nor lose anything if they don't match. > ... also there is a function get_os_x_device_name (in dinput only) which doesn't handle this case at all I don't think. What's your recommendation? I don't know how important it is for get_osx_device_name() to return a non-empty name. It could be changed to call copy_device_name() to get the name and then extract that to the buffer. Mind you, it's currently buggy in how it treats the CFString length vs. the buffer length. The former is in number of UTF-16 code units, the latter in terms of ASCII characters, so they're not directly comparable. (Also, nothing ever checks the function's return value, so returning the supposed required length doesn't achieve anything.) > > @@ -203,7 +227,18 @@ static void copy_set_to_array(const void *value, void *context) > > CFArrayAppendValue(context, value); > > } > > > > -static CFComparisonResult device_location_comparator(const void *val1, const void *val2, void *context) > > +static CFComparisonResult device_name_comparator(IOHIDDeviceRef device1, IOHIDDeviceRef device2) > > +{ > > + CFStringRef name1 = copy_device_name(device1), name2 = copy_device_name(device2); > > + CFComparisonResult result = CFStringCompare(name1, name2, (kCFCompareForcedOrdering | kCFCompareNumerically)); > > + if(name1) > > + CFRelease(name1); > > + if(name2) > > + CFRelease(name2); > > If name1 or name2 might be NULL as these tests imply, then what does the preceding CFStringCompare() call do? Probably crashes. So, either assume/assure that copy_device_name() doesn't return NULL in normal operation or handle the possibility that it might more thoroughly. > > Actually I think it doesn't crash, though I should test it to be sure. Testing isn't appropriate. It could work in some versions of the frameworks and not in others. The authority on the design contract is the documentation (including header comments), with the understanding that if it doesn't explicitly say that NULL is acceptable, then it's not. > However, maybe for completeness/robustness I should include if statements dealing with all the possible name-NULL combinations and return - something like: > > if ((!name1 && !name2) || !name1) > return lessThan; > else if (!name2) > return greaterThan; > > Then it wouldn't matter what CFStringCompare's behavior was if it crashes ... or I could set name equal to an empty string in copy_device_name which would have the same effect as the above few lines. Returning the empty string is probably simplest. If you go the pointer testing route, then surely (!name1 && !name2) implies equal, not less-than. -Ken

July 19, 2016

Re: [PATCH 1/2] msctf: Don't crash on NULL input in EnumLanguageProfiles().

by Michael Stefaniuc

On 07/19/2016 09:56 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=24335 > > Your paranoid android. > > > === w864 (32 bit inputprocessor) === > inputprocessor.c:834: Test failed: Deactivation came unexpectedly This is an existing failure. bye michael

July 19, 2016

Re: [v2 1/1] dinput: Sort virtual joysticks by name on the Mac

by DavidL

On Mon, Jul 18, 2016 at 10:59 PM, Ken Thomases <ken(a)codeweavers.com> wrote: > On Jul 18, 2016, at 1:57 AM, David Lawrie <david.dljunk(a)gmail.com> wrote: > > > +static CFStringRef copy_device_name(IOHIDDeviceRef device) > > +{ > > + CFTypeRef ref; > > + CFStringRef name = NULL; > > + > > + if(device) > > Maybe didn't notice this in the previous version, but put a space between > "if" and the parenthesis. > will do > > > + { > > + assert(IOHIDDeviceGetTypeID() == CFGetTypeID(device)); > > + > > + ref = IOHIDDeviceGetProperty(device, CFSTR(kIOHIDProductKey)); > > + > > + if (ref && CFStringGetTypeID() == CFGetTypeID(ref)) > > + name = CFStringCreateCopy(kCFAllocatorDefault,ref); > > Please use a space after the comma in argument lists in function calls. > will do > > > + else if (ref) > > + name = CFCopyDescription(ref); > > + else > > + name = > CFStringCreateCopy(kCFAllocatorDefault,CFSTR("(null)")); > > You don't want to fall back to a string based on the vendor and product ID? > I thought about it, but then I wanted to try to hew as close to the behavior of debugstr_cf as possible since that is the output that the user sees already in trace logs. This label "(null)" is how debugstr handles that case. The alternative of course is changing both functions to try to get the venture/product ID - though I think the concern is if it doesn't have aProductKey it may not have a proper productId or vendorID either ... also there is a function get_os_x_device_name (in dinput only) which doesn't handle this case at all I don't think. What's your recommendation? > > Also, the same spacing issue as above. > will do > > > + } > > + else > > + ERR("Invalid Device requested %p\n",device); > > At this point, device is known to be NULL, so the log message can say that > rather than the vague "invalid", and doesn't need to include its value. Or > did you mean to be checking something else in the "if"? > Ah right, good point! :) > > > + > > + return name; > > +} > > + > > static long get_device_location_ID(IOHIDDeviceRef device) > > { > > return get_device_property_long(device, CFSTR(kIOHIDLocationIDKey)); > > @@ -203,7 +227,18 @@ static void copy_set_to_array(const void *value, > void *context) > > CFArrayAppendValue(context, value); > > } > > > > -static CFComparisonResult device_location_comparator(const void *val1, > const void *val2, void *context) > > +static CFComparisonResult device_name_comparator(IOHIDDeviceRef > device1, IOHIDDeviceRef device2) > > +{ > > + CFStringRef name1 = copy_device_name(device1), name2 = > copy_device_name(device2); > > + CFComparisonResult result = CFStringCompare(name1, name2, > (kCFCompareForcedOrdering | kCFCompareNumerically)); > > + if(name1) > > + CFRelease(name1); > > + if(name2) > > + CFRelease(name2); > > If name1 or name2 might be NULL as these tests imply, then what does the > preceding CFStringCompare() call do? Probably crashes. So, either > assume/assure that copy_device_name() doesn't return NULL in normal > operation or handle the possibility that it might more thoroughly. > Actually I think it doesn't crash, though I should test it to be sure. However, maybe for completeness/robustness I should include if statements dealing with all the possible name-NULL combinations and return - something like: if ((!name1 && !name2) || !name1) return lessThan; else if (!name2) return greaterThan; Then it wouldn't matter what CFStringCompare's behavior was if it crashes ... or I could set name equal to an empty string in copy_device_name which would have the same effect as the above few lines. > > -Ken > > --David

July 19, 2016

Re: [PATCH 1/2] msctf: Don't crash on NULL input in EnumLanguageProfiles().

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=24335 Your paranoid android. === w864 (32 bit inputprocessor) === inputprocessor.c:834: Test failed: Deactivation came unexpectedly

July 19, 2016

Re: shell32: add SHGetPropertyStoreForWindow stub (try 2)

by Nikolay Sivov

> +HRESULT WINAPI SHGetPropertyStoreForWindow(HWND hwnd, REFIID riid, void **ppv) > +{ > + FIXME("(%p %p %p) stub!\n", hwnd, riid, ppv); > + return E_NOTIMPL; > +} Not very important, but riid could be traced.

July 19, 2016

Re: [PATCH 7/7] d3d11/tests: Add test for bitwise shader instructions.

by Józef Kucia

On Mon, Jul 18, 2016 at 1:27 PM, Józef Kucia <jkucia(a)codeweavers.com> wrote: > + return uint4(~bits[0], ~(bits[0] & ~0u), ~bits[1], ~(bits[1] & ~0u)); This is a mistake. It should read "return uint4(~bits[0], ~(bits[0] ^ ~0u), ~bits[1], ~(bits[1] ^ ~0u));".

July 19, 2016

Re: [v2 1/1] winejoystick.drv: Sort virtual joysticks by name on the Mac

by Ken Thomases

On Jul 18, 2016, at 1:57 AM, 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 | 49 ++++++++++++++++++++++++++++++++---- > 1 file changed, 44 insertions(+), 5 deletions(-) Same comments as for the dinput version. -Ken

July 19, 2016

Re: [v2 1/1] dinput: Sort virtual joysticks by name on the Mac

by Ken Thomases

On Jul 18, 2016, at 1:57 AM, David Lawrie <david.dljunk(a)gmail.com> wrote: > +static CFStringRef copy_device_name(IOHIDDeviceRef device) > +{ > + CFTypeRef ref; > + CFStringRef name = NULL; > + > + if(device) Maybe didn't notice this in the previous version, but put a space between "if" and the parenthesis. > + { > + assert(IOHIDDeviceGetTypeID() == CFGetTypeID(device)); > + > + ref = IOHIDDeviceGetProperty(device, CFSTR(kIOHIDProductKey)); > + > + if (ref && CFStringGetTypeID() == CFGetTypeID(ref)) > + name = CFStringCreateCopy(kCFAllocatorDefault,ref); Please use a space after the comma in argument lists in function calls. > + else if (ref) > + name = CFCopyDescription(ref); > + else > + name = CFStringCreateCopy(kCFAllocatorDefault,CFSTR("(null)")); You don't want to fall back to a string based on the vendor and product ID? Also, the same spacing issue as above. > + } > + else > + ERR("Invalid Device requested %p\n",device); At this point, device is known to be NULL, so the log message can say that rather than the vague "invalid", and doesn't need to include its value. Or did you mean to be checking something else in the "if"? > + > + return name; > +} > + > static long get_device_location_ID(IOHIDDeviceRef device) > { > return get_device_property_long(device, CFSTR(kIOHIDLocationIDKey)); > @@ -203,7 +227,18 @@ static void copy_set_to_array(const void *value, void *context) > CFArrayAppendValue(context, value); > } > > -static CFComparisonResult device_location_comparator(const void *val1, const void *val2, void *context) > +static CFComparisonResult device_name_comparator(IOHIDDeviceRef device1, IOHIDDeviceRef device2) > +{ > + CFStringRef name1 = copy_device_name(device1), name2 = copy_device_name(device2); > + CFComparisonResult result = CFStringCompare(name1, name2, (kCFCompareForcedOrdering | kCFCompareNumerically)); > + if(name1) > + CFRelease(name1); > + if(name2) > + CFRelease(name2); If name1 or name2 might be NULL as these tests imply, then what does the preceding CFStringCompare() call do? Probably crashes. So, either assume/assure that copy_device_name() doesn't return NULL in normal operation or handle the possibility that it might more thoroughly. -Ken

July 19, 2016

Re: [3/4] vcomp: Implement 8-bit atomic instructions.

by Alexandre Julliard

Sebastian Lackner <sebastian(a)fds-team.de> writes: > Signed-off-by: Sebastian Lackner <sebastian(a)fds-team.de> > --- > dlls/vcomp/main.c | 109 ++++++++++++++++++++++++++++++++++++++++++++ > dlls/vcomp/vcomp.spec | 22 ++++---- > dlls/vcomp100/vcomp100.spec | 22 ++++---- > dlls/vcomp110/vcomp110.spec | 22 ++++---- > dlls/vcomp120/vcomp120.spec | 22 ++++---- > dlls/vcomp90/vcomp90.spec | 22 ++++---- > 6 files changed, 164 insertions(+), 55 deletions(-) It doesn't compile here (gcc 5.4.0): gcc -m32 -c -o main.o main.c -I. -I../../include -D__WINESRC__ -D_REENTRANT -fPIC -Wall -pipe \ -fno-strict-aliasing -Wdeclaration-after-statement -Wempty-body -Wignored-qualifiers \ -Wstrict-prototypes -Wtype-limits -Wunused-but-set-parameter -Wvla -Wwrite-strings -Wpointer-arith \ -Wlogical-op -gdwarf-2 -gstrict-dwarf -fno-omit-frame-pointer -Werror -g -O2 -fno-diagnostics-show-caret main.c: Assembler messages: main.c:246: Error: bad register name `%sil' main.c:246: Error: bad register name `%dil' main.c:246: Error: bad register name `%dil' main.c:246: Error: bad register name `%sil' main.c:246: Error: bad register name `%sil' main.c:246: Error: bad register name `%sil' main.c:246: Error: bad register name `%sil' main.c:246: Error: bad register name `%sil' main.c:246: Error: bad register name `%sil' Makefile:163: recipe for target 'main.o' failed make[1]: *** [main.o] Error 1 -- Alexandre Julliard julliard(a)winehq.org

July 19, 2016

Re: shell32: add SHGetPropertyStoreForWindow stub

by Alex Henrie

@ stdcall SHGetPropertyStoreFromParsingName(wstr ptr long ptr ptr) +@ stdcall SHGetPropertyStoreForWindow(long ptr ptr) Switch these two lines to get alphabetical order. -Alex

July 18, 2016

Re: [PATCH] wined3d: Test and enable ARB_gpu_shader5 in shaders.

by Henri Verbeet

On 18 July 2016 at 23:04, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: > 2016-07-18 22:53 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>: >> On 18 July 2016 at 21:57, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: >>> 2016-07-18 14:52 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>: >>>> On 18 July 2016 at 14:31, Józef Kucia <joseph.kucia(a)gmail.com> wrote: >>>>> As it was already mentioned the plan was to use GLSL 4.30 for shader >>>>> model 5. However, it shouldn't be much of a burden to also support SM5 >>>>> bitwise opcodes with ARB_gpu_shader5. >>>>> >>>> Sure, but is there a reason to? >>> >>> OpenGL 3.2 + GLSL 1.50 + ARB_gpu_shader5 + ARB_tessellation_shader + >>> ARB_texture_view + whatever else is necessary should also work for >>> d3d11 / SM5, no? That would eventually make a bunch of Mesa drivers >>> support d3d11 which otherwise wouldn't (like my AMD REDWOOD which >>> AFAICS will not ever get anything above GL 3.3 / GLSL 3.30 because it >>> doesn't support doubles). >> SM5 has doubles. > > Those are only optional in SM5 though, see > https://msdn.microsoft.com/en-us/library/windows/desktop/ff476876(v=vs.85).… > note 2 and https://msdn.microsoft.com/en-us/library/windows/desktop/ff476124(v=vs.85).…. > I'm pretty sure SM5 games generally don't require doubles. Right, so sure.

July 18, 2016

Re: [PATCH] wined3d: Test and enable ARB_gpu_shader5 in shaders.

by Matteo Bruni

2016-07-18 22:53 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>: > On 18 July 2016 at 21:57, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: >> 2016-07-18 14:52 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>: >>> On 18 July 2016 at 14:31, Józef Kucia <joseph.kucia(a)gmail.com> wrote: >>>> As it was already mentioned the plan was to use GLSL 4.30 for shader >>>> model 5. However, it shouldn't be much of a burden to also support SM5 >>>> bitwise opcodes with ARB_gpu_shader5. >>>> >>> Sure, but is there a reason to? >> >> OpenGL 3.2 + GLSL 1.50 + ARB_gpu_shader5 + ARB_tessellation_shader + >> ARB_texture_view + whatever else is necessary should also work for >> d3d11 / SM5, no? That would eventually make a bunch of Mesa drivers >> support d3d11 which otherwise wouldn't (like my AMD REDWOOD which >> AFAICS will not ever get anything above GL 3.3 / GLSL 3.30 because it >> doesn't support doubles). > SM5 has doubles. Those are only optional in SM5 though, see https://msdn.microsoft.com/en-us/library/windows/desktop/ff476876(v=vs.85).… note 2 and https://msdn.microsoft.com/en-us/library/windows/desktop/ff476124(v=vs.85).…. I'm pretty sure SM5 games generally don't require doubles.

July 18, 2016

Re: [PATCH] wined3d: Test and enable ARB_gpu_shader5 in shaders.

by Henri Verbeet

On 18 July 2016 at 21:57, Matteo Bruni <matteo.mystral(a)gmail.com> wrote: > 2016-07-18 14:52 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>: >> On 18 July 2016 at 14:31, Józef Kucia <joseph.kucia(a)gmail.com> wrote: >>> As it was already mentioned the plan was to use GLSL 4.30 for shader >>> model 5. However, it shouldn't be much of a burden to also support SM5 >>> bitwise opcodes with ARB_gpu_shader5. >>> >> Sure, but is there a reason to? > > OpenGL 3.2 + GLSL 1.50 + ARB_gpu_shader5 + ARB_tessellation_shader + > ARB_texture_view + whatever else is necessary should also work for > d3d11 / SM5, no? That would eventually make a bunch of Mesa drivers > support d3d11 which otherwise wouldn't (like my AMD REDWOOD which > AFAICS will not ever get anything above GL 3.3 / GLSL 3.30 because it > doesn't support doubles). SM5 has doubles.

July 18, 2016

Re: [PATCH] wined3d: Test and enable ARB_gpu_shader5 in shaders.

by Matteo Bruni

2016-07-18 14:52 GMT+02:00 Henri Verbeet <hverbeet(a)gmail.com>: > On 18 July 2016 at 14:31, Józef Kucia <joseph.kucia(a)gmail.com> wrote: >> As it was already mentioned the plan was to use GLSL 4.30 for shader >> model 5. However, it shouldn't be much of a burden to also support SM5 >> bitwise opcodes with ARB_gpu_shader5. >> > Sure, but is there a reason to? OpenGL 3.2 + GLSL 1.50 + ARB_gpu_shader5 + ARB_tessellation_shader + ARB_texture_view + whatever else is necessary should also work for d3d11 / SM5, no? That would eventually make a bunch of Mesa drivers support d3d11 which otherwise wouldn't (like my AMD REDWOOD which AFAICS will not ever get anything above GL 3.3 / GLSL 3.30 because it doesn't support doubles).

July 18, 2016

Re: [PATCH] wined3d: Test and enable ARB_gpu_shader5 in shaders.

by Henri Verbeet

On 18 July 2016 at 14:31, Józef Kucia <joseph.kucia(a)gmail.com> wrote: > As it was already mentioned the plan was to use GLSL 4.30 for shader > model 5. However, it shouldn't be much of a burden to also support SM5 > bitwise opcodes with ARB_gpu_shader5. > Sure, but is there a reason to?

July 18, 2016

Re: [PATCH] wined3d: Test and enable ARB_gpu_shader5 in shaders.

by Józef Kucia

As it was already mentioned the plan was to use GLSL 4.30 for shader model 5. However, it shouldn't be much of a burden to also support SM5 bitwise opcodes with ARB_gpu_shader5. Apart from that the other issue is that adding newer OpenGL extensions is generally blocked by bug 38648 [1]. The main reason is that it cannot be tested in the current version of Wine with drivers which do not support compatibility profiles. On Fri, Jul 15, 2016 at 2:25 PM, Guillaume Charifi <guillaume.charifi(a)sfr.fr> wrote: > + if (gl_info->supported[ARB_GPU_SHADER5]) > + shader_addline(buffer, "#extension GL_ARB_gpu_shader5 : enable\n"); > if (gl_info->supported[ARB_SHADER_BIT_ENCODING]) > shader_addline(buffer, "#extension GL_ARB_shader_bit_encoding : enable\n"); > if (gl_info->supported[ARB_SHADER_TEXTURE_LOD]) These should be sorted alphabetically. I guess the ARB_gpu_shader5 extension should also be added to the "core_extensions" table. [1] - https://bugs.winehq.org/show_bug.cgi?id=38648

July 18, 2016

Re: [PATCH] vcruntime140: Forward set_unexpected

by Piotr Caban

On 07/18/16 12:11, Alistair Leslie-Hughes wrote: > dlls/ucrtbase/ucrtbase.spec | 2 +- > dlls/vcruntime140/vcruntime140.spec | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) Please also update api-ms-win-crt-private-l1-1-0.spec file. Thanks, Piotr

July 18, 2016

Re: [PATCH] wined3d: Implement SM5 bfrev opcode in glsl.

by Józef Kucia

On Fri, Jul 15, 2016 at 1:25 PM, Guillaume Charifi <guillaume.charifi(a)sfr.fr> wrote: > Signed-off-by: Guillaume Charifi <guillaume.charifi(a)sfr.fr> > --- > dlls/wined3d/glsl_shader.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/dlls/wined3d/glsl_shader.c b/dlls/wined3d/glsl_shader.c > index 5e23920..376d110 100644 > --- a/dlls/wined3d/glsl_shader.c > +++ b/dlls/wined3d/glsl_shader.c > @@ -3538,6 +3538,7 @@ static void shader_glsl_map2gl(const struct wined3d_shader_instruction *ins) > case WINED3DSIH_ROUND_Z: instruction = "trunc"; break; > case WINED3DSIH_SQRT: instruction = "sqrt"; break; > case WINED3DSIH_UMAX: instruction = "max"; break; > + case WINED3DSIH_BFREV: instruction = "bitfieldReverse"; break; It looks like this should be in alphabetical order.

July 18, 2016

Re: [PATCH] wined3d: Implement SM5 f32tof16 opcode in glsl.

by Józef Kucia

A test would be nice to have. On Fri, Jul 15, 2016 at 1:37 PM, Guillaume Charifi <guillaume.charifi(a)sfr.fr> wrote: > Signed-off-by: Guillaume Charifi <guillaume.charifi(a)sfr.fr> > --- > dlls/wined3d/arb_program_shader.c | 1 + > dlls/wined3d/glsl_shader.c | 38 ++++++++++++++++++++++++++++++++++++++ > dlls/wined3d/shader_sm4.c | 2 ++ > dlls/wined3d/wined3d_private.h | 1 + > 4 files changed, 42 insertions(+) > > diff --git a/dlls/wined3d/arb_program_shader.c b/dlls/wined3d/arb_program_shader.c > index 24c1194..9aa4252 100644 > --- a/dlls/wined3d/arb_program_shader.c > +++ b/dlls/wined3d/arb_program_shader.c > @@ -5286,6 +5286,7 @@ static const SHADER_HANDLER shader_arb_instruction_handler_table[WINED3DSIH_TABL > /* WINED3DSIH_EQ */ NULL, > /* WINED3DSIH_EXP */ shader_hw_scalar_op, > /* WINED3DSIH_EXPP */ shader_hw_scalar_op, > + /* WINED3DSIH_F32TOF16 */ NULL, > /* WINED3DSIH_FRC */ shader_hw_map2gl, > /* WINED3DSIH_FTOI */ NULL, > /* WINED3DSIH_FTOU */ NULL, You need to add WINED3DSIH_F32TOF16 in other places as well, e.g. in shader.c. > + shader_addline(ins->ctx->buffer, "unpackHalf2x16(floatBitsToUint(%s)).x", src_param.param_str); You shouldn't need floatBitsToUint. shader_glsl_add_src_param() takes care about it when you set a proper type for the parameter. > + *dst_mask_ptr++ = dst_masks[i]; > + } else > + shader_addline(ins->ctx->buffer, "0"); > + > + if(i != NB_COMPS - 1) > + shader_addline(ins->ctx->buffer, ","); > + shader_addline(ins->ctx->buffer, "\n"); > + } > + > + *dst_mask_ptr++ = '\0'; You shouldn't need to construct dst_mask manually. > + shader_addline(ins->ctx->buffer, ")%s);\n", dst_mask); > +#undef NB_COMPS > +} > + > static void shader_glsl_else(const struct wined3d_shader_instruction *ins) > { > shader_addline(ins->ctx->buffer, "} else {\n"); > @@ -8704,6 +8741,7 @@ static const SHADER_HANDLER shader_glsl_instruction_handler_table[WINED3DSIH_TAB > /* WINED3DSIH_EQ */ shader_glsl_relop, > /* WINED3DSIH_EXP */ shader_glsl_scalar_op, > /* WINED3DSIH_EXPP */ shader_glsl_expp, > + /* WINED3DSIH_F32TOF16 */ shader_glsl_f32tof16, > /* WINED3DSIH_FRC */ shader_glsl_map2gl, > /* WINED3DSIH_FTOI */ shader_glsl_to_int, > /* WINED3DSIH_FTOU */ shader_glsl_to_uint, > diff --git a/dlls/wined3d/shader_sm4.c b/dlls/wined3d/shader_sm4.c > index c8fdb32..6c15751 100644 > --- a/dlls/wined3d/shader_sm4.c > +++ b/dlls/wined3d/shader_sm4.c > @@ -215,6 +215,7 @@ enum wined3d_sm4_opcode > WINED3D_SM5_OP_DERIV_RTY_COARSE = 0x7c, > WINED3D_SM5_OP_DERIV_RTY_FINE = 0x7d, > WINED3D_SM5_OP_GATHER4_C = 0x7e, > + WINED3D_SM5_OP_F32TOF16 = 0x82, > WINED3D_SM5_OP_BFI = 0x8c, > WINED3D_SM5_OP_BFREV = 0x8d, > WINED3D_SM5_OP_SWAPC = 0x8e, > @@ -811,6 +812,7 @@ static const struct wined3d_sm4_opcode_info opcode_table[] = > {WINED3D_SM5_OP_DERIV_RTY_COARSE, WINED3DSIH_DSY_COARSE, "f", "f"}, > {WINED3D_SM5_OP_DERIV_RTY_FINE, WINED3DSIH_DSY_FINE, "f", "f"}, > {WINED3D_SM5_OP_GATHER4_C, WINED3DSIH_GATHER4_C, "f", "fRSf"}, > + {WINED3D_SM5_OP_F32TOF16, WINED3DSIH_F32TOF16, "f", "f"}, You should use "u" for the source parameters. Similar comments as to other patches also applies.

July 18, 2016

Re: [PATCH 1/2] wined3d: Implement SM5 bfi opcode in glsl.

by Józef Kucia

On Fri, Jul 15, 2016 at 1:53 PM, Guillaume Charifi <guillaume.charifi(a)sfr.fr> wrote: > Signed-off-by: Guillaume Charifi <guillaume.charifi(a)sfr.fr> > --- > dlls/wined3d/glsl_shader.c | 72 +++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 71 insertions(+), 1 deletion(-) > > diff --git a/dlls/wined3d/glsl_shader.c b/dlls/wined3d/glsl_shader.c > index d8edfdc..b0d3a91 100644 > --- a/dlls/wined3d/glsl_shader.c > +++ b/dlls/wined3d/glsl_shader.c > @@ -4380,6 +4380,76 @@ static void shader_glsl_f32tof16(const struct wined3d_shader_instruction *ins) > #undef NB_COMPS > } This patch requires 124405 so it should be in the same series. > > + > +static void shader_glsl_bitfield_op(const struct wined3d_shader_instruction *ins) > +{ > +#define NB_COMPS 4 > + const DWORD write_masks[NB_COMPS] = { > + WINED3DSP_WRITEMASK_0, > + WINED3DSP_WRITEMASK_1, > + WINED3DSP_WRITEMASK_2, > + WINED3DSP_WRITEMASK_3, > + }; You shouldn't need this array. You can assume that WINED3DSP_WRITE_MASK_i = WINED3DSP_WRITE_MASK_0 << i. > + const char dst_masks[NB_COMPS] = { 'x', 'y', 'z', 'w' }; > + const char *prefix; > + const char *suffix; > + int nb_params; For consistency, this should be named "param_count" and should be unsigned int. > + struct glsl_src_param src_param; > + DWORD write_mask; > + char dst_mask[6]; > + char *dst_mask_ptr = dst_mask; > + > + switch(ins->handler_idx) > + { > + case WINED3DSIH_BFI: > + prefix = "bitfieldInsert("; > + suffix = ")"; > + nb_params = 4; > + break; > + > + default: > + prefix = "<unhandled opcode>"; > + suffix = ""; > + nb_params = 0; > + FIXME("Can't handle opcode %s.\n", debug_d3dshaderinstructionhandler(ins->handler_idx)); > + } > + > + write_mask = shader_glsl_append_dst(ins->ctx->buffer, ins); > + > + *dst_mask_ptr++ = '.'; > + shader_addline(ins->ctx->buffer, "uvec4(\n"); > + > + for (int i = 0; i < NB_COMPS; i++) This requires C99. > + { > + if (write_mask & write_masks[i]) > + { > + shader_addline(ins->ctx->buffer, "%s", prefix); > + for(int j = nb_params - 1; j >= 0; j--) > + { > + shader_glsl_add_src_param(ins, &ins->src[j], write_masks[i], &src_param); > + if(j <= 1) > + shader_addline(ins->ctx->buffer, "int(%s)", src_param.param_str); You shouldn't need this cast. It's better to adjust the parameter types and let shader_glsl_add_src_param() handle this. > + else > + shader_addline(ins->ctx->buffer, "%s", src_param.param_str); > + > + if(j) > + shader_addline(ins->ctx->buffer, ", "); > + } > + shader_addline(ins->ctx->buffer, "%s", suffix); > + *dst_mask_ptr++ = dst_masks[i]; > + } else > + shader_addline(ins->ctx->buffer, "0"); > + > + if(i != NB_COMPS - 1) > + shader_addline(ins->ctx->buffer, ","); > + shader_addline(ins->ctx->buffer, "\n"); > + } > + > + *dst_mask_ptr++ = '\0'; You shouldn't need to build dst_mask manually. > + shader_addline(ins->ctx->buffer, ")%s);\n", dst_mask); > +#undef NB_COMPS I don't like this duplicated enums. Generally, this implementation seems overly complicated. You should be able to do something similar to: for (i = 0; i < 4; ++i) { write_mask = WINED3DSP_WRITEMASK_0 << i; dst.write_mask = ins->dst[0].write_mask & write_mask; if (!(write_mask = shader_glsl_append_dst_ext(ins->ctx->buffer, ins, &dst, dst.reg.data_type))) continue; shader_glsl_add_src_param(ins, &ins->src[0], write_mask, &bits); shader_glsl_add_src_param(ins, &ins->src[1], write_mask, &offset); shader_glsl_add_src_param(ins, &ins->src[2], write_mask, &insert); shader_glsl_add_src_param(ins, &ins->src[3], write_mask, &base); shader_addline(ins->ctx->buffer, "bitfieldInsert(%s, %s, %s, %s));\n", base.param_str, insert.param_str, offset.param_str, bits.param_str); }

July 18, 2016

Re: [4/5] msvcirt: Implement istream::getline

by Piotr Caban

On 07/18/16 10:13, Iván Matellanes wrote: > DEFINE_THISCALL_WRAPPER(istream_getline, 16) > istream* __thiscall istream_getline(istream *this, char *str, int count, char delim) > { > - FIXME("(%p %p %d %c) stub\n", this, str, count, delim); > - return this; > + this->extract_delim++; > + return istream_get_str_delim(this, str, count, (unsigned char) delim); > } You probably need to call ios_lock before updating extract_delim. Thanks, Piotr

July 18, 2016

Re: [v1 1/1] dinput: Sort virtual joysticks by name on the Mac

by DavidL

Hi Ken, So I wanted to try to hew as close to the behavior of debugstr_cf as possible since that is the output that the user sees already in trace logs. Here's how I decided handle each case: static CFStringRef copy_device_name(IOHIDDeviceRef device) > { > CFTypeRef ref; > CFStringRef name = NULL; > > if(device) > { > assert(IOHIDDeviceGetTypeID() == CFGetTypeID(device)); > ref = IOHIDDeviceGetProperty(device, CFSTR(kIOHIDProductKey)); > if (ref && CFStringGetTypeID() == CFGetTypeID(ref)) > name = CFStringCreateCopy(kCFAllocatorDefault,ref); > else if (ref) > name = CFCopyDescription(ref); > else > name = CFStringCreateCopy(kCFAllocatorDefault,CFSTR("(null)")); > } > else > ERR("Invalid Device requested %p\n",device); > > return name; > } > > static CFComparisonResult device_name_comparator(IOHIDDeviceRef device1, > IOHIDDeviceRef device2) > { > CFStringRef name1 = copy_device_name(device1), name2 = > copy_device_name(device2); > CFComparisonResult result = CFStringCompare(name1, name2, > (kCFCompareForcedOrdering | kCFCompareNumerically)); > if(name1) > CFRelease(name1); > if(name2) > CFRelease(name2); > return result; > } What do you think? Cheers, David On Sun, Jul 17, 2016 at 8:20 PM, DavidL <david.dljunk(a)gmail.com> wrote: > Right-o! I was wondering what the CFRelease was doing in the debugstr_cf > function. :) > > Cheers, > David > > On Sun, Jul 17, 2016 at 2:11 PM, Ken Thomases <ken(a)codeweavers.com> wrote: > >> On Jul 17, 2016, at 4:08 PM, Ken Thomases <ken(a)codeweavers.com> wrote: >> > >> > On Jul 16, 2016, at 3:15 AM, David Lawrie <david.dljunk(a)gmail.com> >> wrote: >> >> >> >> +static CFStringRef get_device_name(IOHIDDeviceRef device) >> >> +{ >> >> + CFTypeRef ref; >> >> + CFStringRef name = CFSTR("Default Name"); >> > >> > You can probably do better for a fallback. For example, you can format >> the vendor ID and product ID into a string. For an extreme take on this, >> see the Copy_DeviceName() function in Apple's HID Calibrator sample code: >> > >> https://developer.apple.com/library/mac/samplecode/HID_Calibrator/Listings/… >> >> Oh, and for the case where device is NULL, it's fine to return NULL. It >> should never happen and, if it does, we want a crash to figure out why and >> fix things in a better manner, rather than papering over it with a bogus >> value. >> >> -Ken >> >> >

July 18, 2016

Re: [v1 1/1] dinput: Sort virtual joysticks by name on the Mac

by DavidL

Right-o! I was wondering what the CFRelease was doing in the debugstr_cf function. :) Cheers, David On Sun, Jul 17, 2016 at 2:11 PM, Ken Thomases <ken(a)codeweavers.com> wrote: > On Jul 17, 2016, at 4:08 PM, Ken Thomases <ken(a)codeweavers.com> wrote: > > > > On Jul 16, 2016, at 3:15 AM, David Lawrie <david.dljunk(a)gmail.com> > wrote: > >> > >> +static CFStringRef get_device_name(IOHIDDeviceRef device) > >> +{ > >> + CFTypeRef ref; > >> + CFStringRef name = CFSTR("Default Name"); > > > > You can probably do better for a fallback. For example, you can format > the vendor ID and product ID into a string. For an extreme take on this, > see the Copy_DeviceName() function in Apple's HID Calibrator sample code: > > > https://developer.apple.com/library/mac/samplecode/HID_Calibrator/Listings/… > > Oh, and for the case where device is NULL, it's fine to return NULL. It > should never happen and, if it does, we want a crash to figure out why and > fix things in a better manner, rather than papering over it with a bogus > value. > > -Ken > >

July 18, 2016

Re: [PATCH] winex11.drv: Use XRRSetScreenSize with XRRSetCrtcConfig

by Henri Verbeet

On 16 July 2016 at 00:48, Gabriel Corona <gabriel.corona(a)enst-bretagne.fr> wrote: > Fixes https://bugs.winehq.org/show_bug.cgi?id=33290 > > The nvidia driver is not so happy about using XRRSetCrtcConfig alone > and generates a panning configuration. We set the screen size > accordingly. Moreover we disable the CRT during the transition. > Updating the screen size isn't necessarily wrong, although avoiding panning is the wrong reason. If you don't want panning, make sure it doesn't get enabled in the first place. The main problem with this patch though, is that it's almost certainly wrong for multi-head configurations.

July 17, 2016

Re: [v1 1/1] dinput: Sort virtual joysticks by name on the Mac

by Ken Thomases

On Jul 17, 2016, at 4:08 PM, Ken Thomases <ken(a)codeweavers.com> wrote: > > On Jul 16, 2016, at 3:15 AM, David Lawrie <david.dljunk(a)gmail.com> wrote: >> >> +static CFStringRef get_device_name(IOHIDDeviceRef device) >> +{ >> + CFTypeRef ref; >> + CFStringRef name = CFSTR("Default Name"); > > You can probably do better for a fallback. For example, you can format the vendor ID and product ID into a string. For an extreme take on this, see the Copy_DeviceName() function in Apple's HID Calibrator sample code: > https://developer.apple.com/library/mac/samplecode/HID_Calibrator/Listings/… Oh, and for the case where device is NULL, it's fine to return NULL. It should never happen and, if it does, we want a crash to figure out why and fix things in a better manner, rather than papering over it with a bogus value. -Ken

July 17, 2016

Re: [v1 1/1] winejoystick.drv: Sort virtual joysticks by name on the Mac

by Ken Thomases

On Jul 16, 2016, at 3:15 AM, 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 | 42 +++++++++++++++++++++++++++++++----- > 1 file changed, 37 insertions(+), 5 deletions(-) Same comments as for the dinput version. -Ken

July 17, 2016