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

Re: Testbot 3D tests broken
by Patrick Rudolph July 14, 2016

July 14, 2016

On 2016-07-13 09:53 PM, Matteo Bruni wrote: > 2016-07-12 17:51 GMT+02:00 Patrick Rudolph <siro(a)das-labor.org>: >> Hi, >> can you have a look at the testbot, it doesn't run d3d8 and d3d9 tests. >> Other 3D tests might be affected, too. >> As you can see here it reports no test failures, but it skipped all >> tests: >> https://testbot.winehq.org/JobDetails.pl?Key=24151 >> https://testbot.winehq.org/JobDetails.pl?Key=24150 > > Not sure if you already got an answer on IRC or elsewhere. Anyway, the > tests are actually executed on OS versions with the WARP device > available, i.e. Win8 and newer. WARP device doesn't entirely match > "usual" hardware devices WRT test results and since we really only > care to match hardware behavior (but we still want the tests to pass > on the testbot) there are a number of hacks in the tests. > Should I don't care about pre Win8 test results ? Both, d3d8 and d3d9 games, are likely to be used with WinXp or Win7. Does it make sense to test them on Win8 or newer ? As likely every wine user is running wined3d on real hardware, shouldn't there be test systems that have real GPUs ? >> Regards, >> Patrick

2 1

Re: kernel32: Overhaul the handling of argv in set_process_name().
by Sebastian Lackner July 13, 2016

July 13, 2016

On 12.07.2016 06:44, Ken Thomases wrote: > This fixes several problems with the code: > > * The code had been assuming that the argument strings pointed to by the argv > array are contiguous iff certain process-name-setting functions are available. > This doesn't seem reliable. Instead, test if it's true and shift the strings > if so. Thanks for working on this, looks indeed like a nice improvement. > > However, setproctitle() is specifically documented as a preferred alternative > to the technique of overwriting the arg strings, so don't shift the strings > if that's available. Wouldn't it make sense to remove the directory component also in this case, to be consistent? By moving the code to the bottom all functions would be called in the same way. > > * Use the last path component, recognizing backslash as a path separator, for > setprogname() in addition to prctl(). First, setprogname() is documented as > searching for the last component itself, but it doesn't understand Windows- > style paths, so we need to help it. Second, on some platforms (e.g. macOS), > setprogname(), like prctl(), has a fairly small internal length limit (e.g. > 32 characters). So, concentrate on the most meaningful part of the path. > > * Remove argv[0] from argv whether or not there are any process-name-setting > functions available. This is necessary for the proper functioning of Wine, > so it must be done on all platforms. This part of the logic was lost with > commit 5a4576ee0. > > * Call all available process-name-setting functions instead of treating them > as mutually exclusive alternatives. This is also logic that was lost with > commit 5a4576ee0. > > Signed-off-by: Ken Thomases <ken(a)codeweavers.com> > --- > dlls/kernel32/process.c | 63 +++++++++++++++++++++++++++++-------------------- > 1 file changed, 37 insertions(+), 26 deletions(-) >

2 9

Re: [2/2] kernel32/tests: Add tests for GetThreadSelectorEntry.
by Marvin July 13, 2016

July 13, 2016

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=24204 Your paranoid android. === w1064 (32 bit thread) === thread.c:1086: Test failed: got 0 with 5 (expected FALSE with ERROR_GEN_FAILURE)

2 1

Testbot 3D tests broken
by Patrick Rudolph July 13, 2016

July 13, 2016

Hi, can you have a look at the testbot, it doesn't run d3d8 and d3d9 tests. Other 3D tests might be affected, too. As you can see here it reports no test failures, but it skipped all tests: https://testbot.winehq.org/JobDetails.pl?Key=24151 https://testbot.winehq.org/JobDetails.pl?Key=24150 Regards, Patrick

2 1

Re: [PATCH v2 2/3] winex11.drv: avoid passing NULL to memcpy (clang)
by Alexandre Julliard July 13, 2016

July 13, 2016

Kirill Smirnov <kirill.k.smirnov(a)gmail.com> writes: > @@ -92,7 +92,8 @@ static void X11DRV_ImmSetInternalString(DWORD dwOffset, > ptr_new = CompositionString + byte_offset; > memmove(ptr_new + byte_length, ptr_new + byte_selection, > dwCompStringLength - byte_offset - byte_selection); > - memcpy(ptr_new, lpComp, byte_length); > + /* Do not pass NULL even if the length == 0 */ > + if (lpComp) memcpy(ptr_new, lpComp, byte_length); > dwCompStringLength += byte_expansion; Why would that be a problem? -- Alexandre Julliard julliard(a)winehq.org

3 3

Re: [PATCH 1/2] include: Fix include arguments.
by Matteo Bruni July 13, 2016

July 13, 2016

This probably makes sense (although, WRT the patch title, this is probably more of a consistency improvement rather than a "fix"). Maybe split the d3dtypes.h change to a separate patch?

1 0

Re: include: Add d3dx10math.inl.
by Matteo Bruni July 13, 2016

July 13, 2016

2016-07-04 19:26 GMT+02:00 Andrey Gusev <andrey.goosev(a)gmail.com>: +#ifndef __D3DX10MATH_INL__ +#define __D3DX10MATH_INL__ My copy of the file (from the latest DX SDK) uses __D3DXMATH_INL__ as header guard. I think we should do the same for compatibility. +/* constructors & operators */ Nitpicking but this comment doesn't seem particularly useful. +inline D3DXVECTOR2::D3DXVECTOR2(const FLOAT *pf) Just use plain float. + if(!pf) return; Space before '(', return on a separate line. +inline D3DXVECTOR2::operator FLOAT* () +{ + return (FLOAT*)&x; Please put a space between "float" and '*' in the cast. Probably do the same in the operator definition, although I don't really care in that regard. +inline D3DXMATRIX D3DXMATRIX::operator - () const +{ + return D3DXMATRIX(-_11, -_12, -_13, -_14, + -_21, -_22, -_23, -_24, + -_31, -_32, -_33, -_34, + -_41, -_42, -_43, -_44); +} This is almost identical to the MS's header. Please use 8 space indentation for line continuations. Probably drop the space between the "operator" keyword and the operator and between the operator and the opening parenthesis, in the operator declaration. +inline D3DXCOLOR::operator UINT () const +{ + UINT _r = r >= 1.0f ? 0xff : r <= 0.0f ? 0x00 : (UINT)(r * 255.0f + 0.5f); + UINT _g = g >= 1.0f ? 0xff : g <= 0.0f ? 0x00 : (UINT)(g * 255.0f + 0.5f); + UINT _b = b >= 1.0f ? 0xff : b <= 0.0f ? 0x00 : (UINT)(b * 255.0f + 0.5f); + UINT _a = a >= 1.0f ? 0xff : a <= 0.0f ? 0x00 : (UINT)(a * 255.0f + 0.5f); + + return (_a << 24) | (_r << 16) | (_g << 8) | (_b << 0); +} This one is also virtually identical. There seems to be room for writing a slightly different but equivalent implementation. FWIW identifiers starting with an underscore are supposed to be reserved, to some degree. It's generally better to avoid them. +inline D3DXFLOAT16::D3DXFLOAT16() +{ +} These default constructors are in (native) d3dx10math.h. I know that with d3dx9* headers we did put those in the .inl file but I think that was a bad idea. +inline BOOL D3DXFLOAT16::operator == (const D3DXFLOAT16 &f) const +{ This one is also almost identical, implementation-wise. Now maybe there isn't that much room for variations here but still, at the very least replace the "== 0" with a '!'. If you can find other reasonable ways to differentiate, that's even better. +inline D3DXCOLOR::D3DXCOLOR(UINT col) +{ + const FLOAT f = 1.0f / 255.0f; + r = f * (FLOAT)(unsigned char)(col >> 16); + g = f * (FLOAT)(unsigned char)(col >> 8); + b = f * (FLOAT)(unsigned char)(col >> 0); + a = f * (FLOAT)(unsigned char)(col >> 24); +} Another one essentially identical. I would write it as something like: inline D3DXCOLOR::D3DXCOLOR(UINT col) { float inv = 1.0f / 255.0f; a = ((col >> 24) & 0xff) * inv; r = ((col >> 16) & 0xff) * inv; g = ((col >> 8) & 0xff) * inv; b = (col & 0xff) * inv; } +static inline D3DXCOLOR* D3DXColorLerp(D3DXCOLOR *pout, const D3DXCOLOR *pc1, const D3DXCOLOR *pc2, FLOAT s) Don't use those prefixed argument names: out, c1 and c2 work just fine. Also you want a space between D3DXCOLOR and '*' and no space between '*' and the function name. +{ + if ( !pout || !pc1 || !pc2 ) return NULL; Please no spaces after the '(' and before the ')'. Return on a separate line. + pout->r = (1-s) * (pc1->r) + s *(pc2->r); You want spaces around the '-' operator and a space after the multiplication operator. No need for the parentheses around pc1->r and pc2->r. Regardless, MS's header uses the "optimized" linear interpolation formula, with just one multiplication. Your current implementation will generally give slightly different results (because of the limited precision of floats) so you should stick to the same formula instead. This isn't an exhaustive list of issues. Many of the above comments apply to multiple places over the patch.

1 0

Re: [PATCH 4/5] d3d11/tests: Add test for writing special floating-point values to render target.
by Henri Verbeet July 13, 2016

July 13, 2016

On 13 July 2016 at 10:52, Józef Kucia <jkucia(a)codeweavers.com> wrote: > +#ifndef NAN > +static inline float __port_nan(void) > +{ > + static const unsigned __nan_bytes = 0x7fc00000; > + return *(const float *)&__nan_bytes; > +} > +#define NAN __port_nan() > +#endif This looks unused. > + const struct vec4 *value = get_readback_vec4(&rb, x, y); > + ok(isnan(value->x) && value->y == INFINITY && value->z == -INFINITY && value->w == 1.0f, > + "Got unexpected value {%.8e, %.8e, %.8e, %.8e} at (%u, %u).\n", > + value->x, value->y, value->z, value->w, x, y); It would slightly change the meaning of the test, but you could also just read these back as uvec4's and compare the bit patterns. > @@ -9312,4 +9400,5 @@ START_TEST(d3d11) > test_check_feature_support(); > test_create_unordered_access_view(); > test_immediate_constant_buffer(); > + test_special_float_values(); For what it's worth, those are usually called "floating-point specials". (And hence fp_special_test() in the d3d9 tests.)

1 0

Re: ntdll/tests: Use inline implementation of NtCurrentTeb for exception tests.
by Marvin July 13, 2016

July 13, 2016

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=24168 Your paranoid android. === wvistau64 (32 bit exception) === exception.c:489: Test failed: 42: Unexpected eip 0x3d0000/0x3d0001 exception: unhandled exception 80000004 at 003D0000 exception.c:489: Test failed: 42: Unexpected eip 0x77baed97/0x3d0001 === w7u (32 bit exception) === exception.c:489: Test failed: 42: Unexpected eip 0x280000/0x280001 exception: unhandled exception 80000004 at 00280000 exception.c:489: Test failed: 42: Unexpected eip 0x77cde34f/0x280001 === w1064 (32 bit exception) === An error occurred while waiting for the test to complete: network read timed out (wait2/connect:AgentVersion.h:0/9) The test VM has crashed, rebooted or lost connectivity (or the TestAgent server died) === w1064 (64 bit exception) === An error occurred while waiting for the test to complete: network read timed out (wait2/connect:AgentVersion.h:0/9) The test VM has crashed, rebooted or lost connectivity (or the TestAgent server died)

3 2

Re: [PATCH] wined3d: Fix the size parameter of glVertexAttrib*Pointer().
by Józef Kucia July 13, 2016

July 13, 2016

On Wed, Jul 13, 2016 at 2:48 AM, Guillaume Charifi <guillaume.charifi(a)sfr.fr> wrote: > The mistake is obvious when looking at the GL 3.0 spec: > p29: > void VertexAttribPointer(uint index, int size, enum type, boolean normalized, sizei stride, const void *pointer); > void VertexAttribIPointer(uint index, int size, enum type, sizei stride, const void *pointer); > > p30: > Command Sizes > VertexAttribPointer 1, 2, 3, 4 > VertexAttribIPointer 1, 2, 3, 4 > > Signed-off-by: Guillaume Charifi <guillaume.charifi(a)sfr.fr> > --- > dlls/wined3d/state.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) This looks wrong to me. The "gl_vtx_format" field is equal to the "component_count" field. There's one exception though, the "gl_vtx_format" is set to GL_BGRA for WINED3DFMT_B8G8R8A8_UNORM. GL_BGRA was added as an accepted value by GL_ARB_vertex_array_bgra (core since OpenGL 3.2). What are you trying to fix?

2 2