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

Re: [PATCH 5/8] d3dx9/tests: Factor out test_effect_preshader_compare_pbool_consts() function.

by Matteo Bruni

2017-06-05 19:27 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > > > On 06/05/2017 07:55 PM, Matteo Bruni wrote: >> >> 2017-06-02 11:58 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: >> >>> + if (!const_updated_mask) >>> + { >>> + for (i = 0; i < ARRAY_SIZE(test_effect_preshader_bconsts); ++i) >>> + { >>> + ok_(__FILE__, line)(!bdata[i] == >>> !test_effect_preshader_bconsts[i], >>> + "Pixel shader boolean constants do not match, >>> expected %#x, got %#x, i %u.\n", >>> + test_effect_preshader_bconsts[i], bdata[i], i); >>> + } >>> + } >>> + else >>> + { >>> + for (i = 0; i < ARRAY_SIZE(test_effect_preshader_bconsts); ++i) >>> + { >>> + if (const_updated_mask[i / TEST_EFFECT_BITMASK_BLOCK_SIZE] >>> + & (1u << (i % TEST_EFFECT_BITMASK_BLOCK_SIZE))) >>> + { >>> + ok_(__FILE__, line)(!bdata[i] == >>> !test_effect_preshader_bconsts[i], >>> + "Pixel shader boolean constants do not match, >>> expected %#x, got %#x, i %u, parameter %s.\n", >>> + test_effect_preshader_bconsts[i], bdata[i], i, >>> updated_param); >>> + } >>> + else >>> + { >>> + ok_(__FILE__, line)(!bdata[i], >>> + "Pixel shader boolean constants updated >>> unexpectedly, parameter %s.\n", updated_param); >>> + } >>> + } >>> + } >> >> Minor simplification possibility: in the (!const_updated_mask) case >> you could use a default "all ones" mask and use what's now the 'else' >> branch, getting rid of the 'if' branch altogether. >> > But I guess I should at least put the if on TRACE message, as without > constant_update_mask there is no parameter name, that was the reason I left > it the same way it was already done for vertex shader float constants. > Should I put the if on TRACE instead? That's not necessary, you can do e.g. updated_param ? updated_param : "". No need to change the code now though, unless you're going to make other changes in the area anyway.

June 5, 2017

Re: [PATCH 5/8] d3dx9/tests: Factor out test_effect_preshader_compare_pbool_consts() function.

by Paul Gofman

On 06/05/2017 07:55 PM, Matteo Bruni wrote: > 2017-06-02 11:58 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > >> + if (!const_updated_mask) >> + { >> + for (i = 0; i < ARRAY_SIZE(test_effect_preshader_bconsts); ++i) >> + { >> + ok_(__FILE__, line)(!bdata[i] == !test_effect_preshader_bconsts[i], >> + "Pixel shader boolean constants do not match, expected %#x, got %#x, i %u.\n", >> + test_effect_preshader_bconsts[i], bdata[i], i); >> + } >> + } >> + else >> + { >> + for (i = 0; i < ARRAY_SIZE(test_effect_preshader_bconsts); ++i) >> + { >> + if (const_updated_mask[i / TEST_EFFECT_BITMASK_BLOCK_SIZE] >> + & (1u << (i % TEST_EFFECT_BITMASK_BLOCK_SIZE))) >> + { >> + ok_(__FILE__, line)(!bdata[i] == !test_effect_preshader_bconsts[i], >> + "Pixel shader boolean constants do not match, expected %#x, got %#x, i %u, parameter %s.\n", >> + test_effect_preshader_bconsts[i], bdata[i], i, updated_param); >> + } >> + else >> + { >> + ok_(__FILE__, line)(!bdata[i], >> + "Pixel shader boolean constants updated unexpectedly, parameter %s.\n", updated_param); >> + } >> + } >> + } > Minor simplification possibility: in the (!const_updated_mask) case > you could use a default "all ones" mask and use what's now the 'else' > branch, getting rid of the 'if' branch altogether. > But I guess I should at least put the if on TRACE message, as without constant_update_mask there is no parameter name, that was the reason I left it the same way it was already done for vertex shader float constants. Should I put the if on TRACE instead?

June 5, 2017

Re: [PATCH 5/8] d3dx9/tests: Factor out test_effect_preshader_compare_pbool_consts() function.

by Matteo Bruni

2017-06-02 11:58 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > + if (!const_updated_mask) > + { > + for (i = 0; i < ARRAY_SIZE(test_effect_preshader_bconsts); ++i) > + { > + ok_(__FILE__, line)(!bdata[i] == !test_effect_preshader_bconsts[i], > + "Pixel shader boolean constants do not match, expected %#x, got %#x, i %u.\n", > + test_effect_preshader_bconsts[i], bdata[i], i); > + } > + } > + else > + { > + for (i = 0; i < ARRAY_SIZE(test_effect_preshader_bconsts); ++i) > + { > + if (const_updated_mask[i / TEST_EFFECT_BITMASK_BLOCK_SIZE] > + & (1u << (i % TEST_EFFECT_BITMASK_BLOCK_SIZE))) > + { > + ok_(__FILE__, line)(!bdata[i] == !test_effect_preshader_bconsts[i], > + "Pixel shader boolean constants do not match, expected %#x, got %#x, i %u, parameter %s.\n", > + test_effect_preshader_bconsts[i], bdata[i], i, updated_param); > + } > + else > + { > + ok_(__FILE__, line)(!bdata[i], > + "Pixel shader boolean constants updated unexpectedly, parameter %s.\n", updated_param); > + } > + } > + } Minor simplification possibility: in the (!const_updated_mask) case you could use a default "all ones" mask and use what's now the 'else' branch, getting rid of the 'if' branch altogether.

June 5, 2017

Re: Wine uses the wrong pkg-config for 32 bit builds

by Michael Müller

You patch breaks the cross compilation for OSX. See: https://builds.wine-staging.com/2017-06-05-15-57-04-job-11586/build.log As you can see in the log, we provide a wrapper script i686-apple-darwin12-pkg-config which sets the pkg-config variables correctly. So far this worked without issues when compiling Wine using --host i686-apple-darwin12. After applying your patch, I can not even find the check for pkg-config any more in the log. Please be aware that configure is called two times for the cross compilation. One time for the build tools and the second time for the actual cross compilation. Regards, Michael

June 5, 2017

Wine uses the wrong pkg-config for 32 bit builds

by Francois Gouget

The problem with pkg-config is that it has no idea what the build target is so it always returns the include and library paths appropriate for the build host. For instance when Wine tries to figure out how to use GStreamer 1.0 it calls: $ pkg-config --cflags gstreamer-1.0 -pthread -I/usr/include/gstreamer-1.0 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include It's pretty obvious that the 'x86_64-linux-gnu' is wrong for a 32 bit build. But then we never told pkg-config that we wanted to do a 32 bit build! There are two ways to tell pkg-config we want to do a 32 bit build. * The first way is to set PKG_CONFIG_LIBDIR to force pkg-config to only use the .pc files we want. However this requires knowing where the .pc files are stored and you'd want to make sure to include /usr/lib/<triplet>/pkgconfig, /usr/local/lib/<triplet>/pkgconfig. And then should you also include /usr/lib/pkgconfig? * Another way is to use a <triplet>-pkg-config tool, which will give us the right information for the specified <triplet>. Red Hat, Debian and its derivatives (Ubuntu, etc) all provide them. This gives: $ i686-linux-gnu-pkg-config --cflags gstreamer-1.0 -pthread -I/usr/include/gstreamer-1.0 -I/usr/include/glib-2.0 -I/usr/lib/i386-linux-gnu/glib-2.0/include We already have a WINE_CHECK_HOST_TOOL() macro that looks like it should do the trick. But it only takes ac_tool_prefix into account. So I modified it to take the host target into account if ac_tool_prefix is not set. This is modeled after the WINE_CHECK_MINGW_PROG macro and you can see the result in the patch below. On Red Hat the it's a quadruplet: it has an extra 'redhat-' in the name (it's i686-redhat-linux-gnu-pkg-config for instance). The patch below does not account for it. I'm hoping Red Hat developers can suggest how to best take this into account. Is this approach ok? ----- configure: Take the target host into account in WINE_CHECK_HOST_TOOL(). In particular this gets us the right pkg-config tool when building the 32 bit Wine. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- aclocal.m4 | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/aclocal.m4 b/aclocal.m4 index 9364188e334..4ac467a71e5 100644 --- a/aclocal.m4 +++ b/aclocal.m4 @@ -27,13 +27,17 @@ dnl dnl Like AC_CHECK_TOOL but without the broken fallback to non-prefixed name dnl AC_DEFUN([WINE_CHECK_HOST_TOOL], -[AS_VAR_SET_IF([ac_tool_prefix], - AC_CHECK_PROG([$1],[${ac_tool_prefix}$2],[${ac_tool_prefix}$2],,[$4])) -AS_VAR_IF([ac_cv_prog_$1],[], - [AS_VAR_IF([cross_compiling],[yes],[], - [AS_UNSET([ac_cv_prog_$1]) - AC_CHECK_PROG([$1],[$2],[$2],[$3],[$4])])], -[AS_VAR_COPY([$1],[ac_cv_prog_$1])])]) +[AS_VAR_IF([ac_tool_prefix],[], + [case "$host_cpu" in + i[[3456789]]86*) + ac_host_tool_list="m4_foreach([ac_wine_cpu],[i686,i586,i486,i386],[ac_wine_cpu-${host_os}-$2 ]) $2" ;; + x86_64) + ac_host_tool_list="x86_64-${host_os}-$2 $2" ;; + *) + ac_host_tool_list="" ;; + esac], + [ac_host_tool_tool_list="${ac_tool_prefix}-$2"]) + AC_CHECK_PROGS($1,$ac_host_tool_list,[$3],[$4])]) dnl **** Initialize the programs used by other checks **** dnl -- 2.11.0

June 5, 2017

Re: [v2 PATCH 2/3] mfplat: Implement MFCreateMediaType

by Henri Verbeet

On 3 June 2017 at 03:13, Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> wrote: > The purpose is to make a type. For example without it every time we needed > to use > it we would have to use "struct _mfmediatype name", but with the typedef > we can shorthand it to "mfmediatype name". > That's a rather poor justification for using typedefs, but unfortunately also a really common one. What you're essentially doing is obfuscating the fact that something is a structure/enum/integer/pointer/etc., only to avoid a few keystrokes. One of the reasons that's odd is that the time spent typing in the code should be small compared to the time spent thinking about the problem you're trying to solve. That's not to say there's never a legitimate reason to use a typedef though. For example, a certain type may be implemented as a 32-bit integer on one platform, a 64-bit integer on another, and a structure on others. A typedef can be used to abstract between those by creating an opaque type, like e.g. time_t. Such cases should be rare however.

June 5, 2017

Re: [PATCH 2/4] ntdll: Provide more accurate virtual memory counters.

by Akihiro Sagawa

On Sun, 4 Jun 2017 22:51:10 -0600, Alex Henrie wrote: > Out of curiosity, is there a particular application that depends on > this behavior? It could be argued that reporting the value from the > native operating system is better because it tells what's really > happening in the process. > > -Alex Good question. I'm trying to fix Sorcery Jokers (trial) issue, bug 43119. The application repeatedly trys to allocate virtual memory as much as possible, and measures available memory size by GlobalMemoryStatusEx(). This patch is better than nothing. The application stops memory allocation about 384MB (0x17ffffff) free. After the allocation, it succeeds to load libGL.so. However, it shows driver pointer missing error when loading DRI driver, radeonsi_dri.so. I'm still in trouble with this point. By the way, I have another idea about this issue. That is reserving virtual memory area when loading wined3d.dll and release it just before loading OpenGL libraries. Being hackish idea, I chose the forementioned method in the beginning. What do you think about these ideas? Akihiro Sagawa

June 5, 2017

Re: [PATCH 5/5] d3drm: Consolidate GetClassName() implementation, reduce tests duplication

by Henri Verbeet

On 5 June 2017 at 01:07, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > +HRESULT d3drm_object_get_class_name(struct d3drm_object *object, DWORD *size, char *name) > +{ > + DWORD req_size; > + > + if (!size) > + return E_INVALIDARG; > + > + req_size = strlen(object->classname) + 1; > + if (name && *size < req_size) > + return E_INVALIDARG; > + > + *size = req_size; > + > + if (name) > + strcpy(name, object->classname); Note that you already know the string length, so just use memcpy(). (I.e., there's almost never a good reason to use strcpy().) > diff --git a/dlls/d3drm/tests/d3drm.c b/dlls/d3drm/tests/d3drm.c > index e1091be423..a0dfe6c7d2 100644 > --- a/dlls/d3drm/tests/d3drm.c > +++ b/dlls/d3drm/tests/d3drm.c > @@ -52,6 +52,42 @@ static D3DRMMATRIX4D identity = { > { 0.0f, 0.0f, 0.0f, 1.0f } > }; > > + > +static void test_class_name(IDirect3DRMObject *object, const char *name) > +{ > + char cname[64] = {0}; > + HRESULT hr; > + DWORD size; > + > + hr = IDirect3DRMObject_GetClassName(object, NULL, cname); > + ok(hr == E_INVALIDARG, "GetClassName failed with %x\n", hr); "Got unexpected hr %#x.\n" > + hr = IDirect3DRMViewport_GetClassName(object, NULL, NULL); > + ok(hr == E_INVALIDARG, "GetClassName failed with %x\n", hr); > + > + size = 0; > + hr = IDirect3DRMObject_GetClassName(object, &size, NULL); > + ok(hr == D3DRM_OK, "Cannot get classname size (hr = %x)\n", hr); > + ok(size == strlen(name) + 1, "wrong size: %u\n", size); > + > + size = 1; > + hr = IDirect3DRMObject_GetClassName(object, &size, cname); > + ok(hr == E_INVALIDARG, "GetClassName failed with %x\n", hr); > + ok(size == 1, "Got size %u.\n", size); > + > + size = sizeof(cname); > + hr = IDirect3DRMObject_GetClassName(object, &size, cname); > + ok(hr == D3DRM_OK, "Cannot get classname (hr = %x)\n", hr); > + ok(size == strlen(name) + 1, "wrong size: %u\n", size); > + ok(!strcmp(cname, name), "Expected cname to be \"%s\", but got \"%s\"\n", name, cname); > + > + size = strlen(name); > + strcpy(cname, "XXX"); > + hr = IDirect3DRMObject_GetClassName(object, &size, cname); > + ok(hr == E_INVALIDARG, "GetClassName failed with %x\n", hr); > + ok(size == strlen(name), "wrong size: %u\n", size); > + ok(!strcmp(cname, "XXX"), "Expected unchanged buffer, but got \"%s\"\n", cname); > +} I would be happier if this printed the line number of the caller on failure, see e.g. check_interface_() in the d3d11 tests.

June 5, 2017

Re: [PATCH 1/5] d3drm/tests: Some tests for creating texture from ddraw surface

by Henri Verbeet

On 5 June 2017 at 01:07, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: > Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> > --- > dlls/d3drm/tests/d3drm.c | 188 +++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 188 insertions(+) > It looks like a good part of this was inspired by the existing d3drm tests, and it doesn't appear to be any worse than those. However, we'd like to raise the standards a little. I.e., in the future, please use the d3d11 and ddraw tests as a reference, instead of the existing d3drm tests.

June 5, 2017

Re: [PATCH] shell32: Implement FCIDM_SHVIEW_CREATELINK

by Nikolay Sivov

On 05.06.2017 16:45, Aric Stewart wrote: > > > On 6/2/17 10:49 AM, Nikolay Sivov wrote: >> On 02.06.2017 17:24, Aric Stewart wrote: >>> >>> Presently the wine file explorer has a create shortcut entry that >>> does nothing. This implements the FCIDM_SHVIEW_CREATELINK command. >>> >>> Signed-off-by: Aric Stewart <aric(a)codeweavers.com> >>> --- >>> dlls/shell32/shlview_cmenu.c | 50 ++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 50 insertions(+) >>> >> >> >>> + if (!SUCCEEDED(CoCreateInstance( &CLSID_ShellLink, NULL, CLSCTX_INPROC_SERVER, >>> + &IID_IShellLinkW, (LPVOID*)&shelllink))) >>> + { >>> + ERR("couldn't create ShellLink object\n"); >>> + return; >>> + } >> >> This should be FAILED(). You can use IShellLink_Constructor() instead of >> going through generic object creation. >> >> > + SHGetSpecialFolderPathW(NULL, root, CSIDL_STARTMENU, TRUE); >>> + lnkfile=HeapAlloc(GetProcessHeap(), 0, (lstrlenW(root)+1+lstrlenW(wszFilename)+5) * sizeof(*lnkfile)); >>> + lstrcpyW(lnkfile, root); >>> + lstrcatW(lnkfile, backslashW); >>> + lstrcatW(lnkfile, wszFilename); >>> + lstrcatW(lnkfile, lnkW); >> >> Why is it created in Start Menu dir? I think usual behavior is to create >> it in target directory. > > Create the link in the same directory as the original program exists? That seems counter intuitive to me. I could seen an argument for the desktop instead of the start menu. But again I have not used windows myself in years. Yes, that's how it works. If you use desktop context menu it will be created on the desktop, for different explorer window it will be created in the path this window is open to. > > > -aric > >

June 5, 2017