October 2014 - Wine-Devel - list.winehq.org

Re: [PATCH 4/5] d3d9/tests: Add a test for ATI1N and ATI2N texture formats.

by Matteo Bruni

2014-10-30 11:45 GMT+01:00 Henri Verbeet <hverbeet(a)gmail.com>: > On 29 October 2014 18:01, Matteo Bruni <mbruni(a)codeweavers.com> wrote: >> @@ -6145,6 +6145,7 @@ static void test_lockrect_offset(void) >> {D3DFMT_DXT3, "D3DFMT_DXT3", 4, 4, 16}, >> {D3DFMT_DXT4, "D3DFMT_DXT4", 4, 4, 16}, >> {D3DFMT_DXT5, "D3DFMT_DXT5", 4, 4, 16}, >> + {MAKEFOURCC('A','T','I','1'), "ATI1N", 1, 1, 1}, >> {MAKEFOURCC('A','T','I','2'), "ATI2N", 1, 1, 1}, > Mostly just curiosity, but there are other tests in device.c for > ATI2N, any reason for not adding ATI1N coverage to those as well? Heh, I was undecided about adding the ATI1N case to the other tests in the same patch or not and I guess by mistake I ended up with some kind of "middle ground" choice... I think I'm going to add it to the missing tests too. > >> + static const DWORD expected_colors_ati[] = {0x003f3f3f, 0x003f3f3f, 0x00007fff, 0x007f00ff}; >> + static const DWORD expected_colors_nvidia[] = {0x007f0000, 0x007f0000, 0x00007fff, 0x007f00ff}; > Conceptually I might have preferred this as > > static const struct > { > struct vec2 position; > D3DCOLOR amd; > D3DCOLOR nvidia; > } > expected_colors[] > { > ... > } > > but it's not a big deal. > >> + for (i = 0; i < 4; ++i) >> + { >> + color = getPixelColor(device, 80 + 160 * i, 240); >> + trace("Got 0x%08x.\n", color); > Is this leftover debug code? Initially it was but then left it there on purpose, to be able to check whether the tested driver takes the AMD or the Nvidia approach. Not sure it is worth it though.

Oct. 30, 2014

Re: [PATCH 3/5] wined3d: Dirtify pixel shader on texture format change.

by Matteo Bruni

2014-10-30 11:45 GMT+01:00 Henri Verbeet <hverbeet(a)gmail.com>: > On 29 October 2014 18:01, Matteo Bruni <mbruni(a)codeweavers.com> wrote: >> + if (!prev || op->texture->target != prev->target >> + || op->texture->resource.format != prev->resource.format) > Shouldn't this just check the color_fixup? In theory you can also have depth texture vs color texture (although I guess that's unlikely to happen in practice). There is also NP2 vs P2 textures which don't matter for non-ancient GPUs and I'm actually still not checking here. > And in principle I think this would apply to vertex shaders as well, > except that we don't track color fixups in struct vs_compile_args at > all. Yes, we don't apply color fixups in vertex shaders at all. Probably a separate issue... > The real pain will come with d3d10 of course, since you can > access a much larger amount of resources there. Yeah, I guess wined3d_cs_exec_set_shader_resource_view() will require a bunch more checks.

Oct. 30, 2014

Re: [PATCH 4/5] d3d9/tests: Add a test for ATI1N and ATI2N texture formats.

by Henri Verbeet

On 29 October 2014 18:01, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > @@ -6145,6 +6145,7 @@ static void test_lockrect_offset(void) > {D3DFMT_DXT3, "D3DFMT_DXT3", 4, 4, 16}, > {D3DFMT_DXT4, "D3DFMT_DXT4", 4, 4, 16}, > {D3DFMT_DXT5, "D3DFMT_DXT5", 4, 4, 16}, > + {MAKEFOURCC('A','T','I','1'), "ATI1N", 1, 1, 1}, > {MAKEFOURCC('A','T','I','2'), "ATI2N", 1, 1, 1}, Mostly just curiosity, but there are other tests in device.c for ATI2N, any reason for not adding ATI1N coverage to those as well? > + static const DWORD expected_colors_ati[] = {0x003f3f3f, 0x003f3f3f, 0x00007fff, 0x007f00ff}; > + static const DWORD expected_colors_nvidia[] = {0x007f0000, 0x007f0000, 0x00007fff, 0x007f00ff}; Conceptually I might have preferred this as static const struct { struct vec2 position; D3DCOLOR amd; D3DCOLOR nvidia; } expected_colors[] { ... } but it's not a big deal. > + for (i = 0; i < 4; ++i) > + { > + color = getPixelColor(device, 80 + 160 * i, 240); > + trace("Got 0x%08x.\n", color); Is this leftover debug code?

Oct. 30, 2014

Re: [PATCH 3/5] wined3d: Dirtify pixel shader on texture format change.

by Henri Verbeet

On 29 October 2014 18:01, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > + if (!prev || op->texture->target != prev->target > + || op->texture->resource.format != prev->resource.format) Shouldn't this just check the color_fixup? And in principle I think this would apply to vertex shaders as well, except that we don't track color fixups in struct vs_compile_args at all. The real pain will come with d3d10 of course, since you can access a much larger amount of resources there.

Oct. 30, 2014

Re: dplayx: Fix some crashes and wine_todo's ( try 3)

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=9894 Your paranoid android. === w8 (32 bit dplayx) === Failure running script in VM: timed out waiting for the child process

Oct. 30, 2014

Re: Need feedback on first four UTF-7 patches

by Alex Henrie

2014-10-29 14:37 GMT-06:00 Sebastian Lackner <sebastian(a)fds-team.de>: > For the tests: The main issue (bad test coverage) is fixed at my opinion, but there are still lot of small things which could be improved to make the code more readable. Most of it was already told on IRC, but I will go over patch 1 one more time and tell you again. Again, thanks for the feedback. For the record, I am holding off on the following suggestions until I receive feedback from Alexandre: - Merging the complex and simple structs (this would decrease test coverage) - Splitting up the ok() statements into pieces not joined by && (this seems like overkill) - Rebasing the non-struct-based tests to be ahead of the struct-based tests in the patch series (this would make it difficult to write a simple check for the presence or absence of UTF-7 support) You can see that despite my frustration with this process, I have incorporated the rest of your suggestions at https://github.com/alexhenrie/wine/commits/master -Alex

Oct. 30, 2014

[PATCH] Mystery hang reduction -- clear new allocations

by mtewoodbury＠gmail.com

From: Max TenEyck Woodbury <max+git(a)mtew.isa-geek.net> I was getting mystery frozen processes that cleared when a total screen redraw was forced. This has been reported numerous times over several years. It came and went with various releases. I have been getting it consistantly recently. I had an idea that it might have something to do with partially uninitialized drawing data structures, so I started adding HEAP_ZERO_MEMORY flags in various places. This particular set cleared the problem, at least for now... diff --git a/dlls/wined3d/vertexdeclaration.c b/dlls/wined3d/vertexdeclaration.c index cf5378c..cf1c814 100644 --- a/dlls/wined3d/vertexdeclaration.c +++ b/dlls/wined3d/vertexdeclaration.c @@ -178,7 +178,8 @@ static HRESULT vertexdeclaration_init(struct wined3d_vertex_declaration *declara declaration->parent = parent; declaration->parent_ops = parent_ops; declaration->device = device; - declaration->elements = HeapAlloc(GetProcessHeap(), 0, sizeof(*declaration->elements) * element_count); + declaration->elements = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, + sizeof(*declaration->elements) * element_count); if (!declaration->elements) { ERR("Failed to allocate elements memory.\n"); @@ -316,7 +317,7 @@ static unsigned int convert_fvf_to_declaration(const struct wined3d_gl_info *gl_ has_psize + has_diffuse + has_specular + num_textures; state.gl_info = gl_info; - state.elements = HeapAlloc(GetProcessHeap(), 0, size * sizeof(*state.elements)); + state.elements = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, size * sizeof(*state.elements)); if (!state.elements) return ~0U; state.offset = 0; state.idx = 0; -- 1.8.0.rc0.18.gf84667d

Oct. 30, 2014

Re: [PATCH 1/4] kernel32/tests: Add simple UTF-7 encoding tests.

by Sebastian Lackner

On 20.10.2014 06:41, Alex Henrie wrote: > --- > dlls/kernel32/tests/codepage.c | 134 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 134 insertions(+) > > diff --git a/dlls/kernel32/tests/codepage.c b/dlls/kernel32/tests/codepage.c > index 8423c75..684ede1 100644 > --- a/dlls/kernel32/tests/codepage.c > +++ b/dlls/kernel32/tests/codepage.c > @@ -412,6 +412,138 @@ static void test_string_conversion(LPBOOL bUsedDefaultChar) > ok(GetLastError() == 0xdeadbeef, "GetLastError() is %u\n", GetLastError()); > } > > +static void test_utf7_encoding(void) > +{ > + struct simple_test > + { > + WCHAR utf16[1024]; > + int utf16_len; > + char utf7[1024]; > + int utf7_len; 1024 is unnecessary big. A couple of bytes would be sufficient for all these tests. > + }; > + > + static const struct simple_test simple_tests[] = { No need to give that struct a name. You can simply do: static const struct { ... } simple_tests[] = { ... } > + /* tests some valid UTF-16 */ > + { > + {0x4F60,0x597D,0x5417,0}, > + 4, > + "+T2BZfVQX-", > + 11 > + }, > + /* tests some invalid UTF-16 */ > + /* (stray lead surrogate) */ That looks ugly, either put it in a single line or use: /* line1 * line2 */ > + { > + {0xD801,0}, > + 2, > + "+2AE-", > + 6 > + }, > + /* tests some more invalid UTF-16 */ > + /* (codepoint does not exist) */ > + { > + {0xFF00,0}, > + 2, > + "+/wA-", > + 6 > + } > + }; > + > + int i; > + > + for (i = 0; i < sizeof(simple_tests) / sizeof(simple_tests[0]); i++) > + { > + char c_buffer[1024]; > + WCHAR w_buffer[1024]; Same as above, way too big. > + int len; > + > + c_buffer[sizeof(c_buffer) - 1] = 0; > + w_buffer[sizeof(w_buffer) / sizeof(WCHAR) - 1] = 0; It probably makes more sense to initialize the buffers before each test, instead of one time at the beginning. > + > + /* test string conversion with srclen=-1 */ > + memset(c_buffer, '#', sizeof(c_buffer) - 1); > + SetLastError(0xdeadbeef); > + len = WideCharToMultiByte(CP_UTF7, 0, simple_tests[i].utf16, -1, c_buffer, sizeof(c_buffer), NULL, NULL); > + if (GetLastError() == ERROR_CALL_NOT_IMPLEMENTED) > + { > + skip("UTF-7 encoding not implemented\n"); > + return; > + } At my opinion it would be better to do one basic test at the beginning, and then abort immediately, instead of doing that inside of the for loop, if its not too complicated. > + ok(len == simple_tests[i].utf7_len && > + strcmp(c_buffer, simple_tests[i].utf7) == 0 && > + c_buffer[len] == '#', > + "simple_test failure i=%i dst=\"%s\" len=%i\n", i, c_buffer, len); > + ok(GetLastError() == 0xdeadbeef, "error=%x\n", GetLastError()); > + > + /* test string conversion with srclen=-2 */ > + memset(c_buffer, '#', sizeof(c_buffer) - 1); > + SetLastError(0xdeadbeef); > + len = WideCharToMultiByte(CP_UTF7, 0, simple_tests[i].utf16, -2, c_buffer, sizeof(c_buffer), NULL, NULL); > + ok(len == simple_tests[i].utf7_len && > + strcmp(c_buffer, simple_tests[i].utf7) == 0 && > + c_buffer[len] == '#', > + "simple_test failure i=%i dst=\"%s\" len=%i\n", i, c_buffer, len); > + ok(GetLastError() == 0xdeadbeef, "error=%x\n", GetLastError()); You are testing a lot of things in a single ok(...) block. It will be very difficult to find out whats wrong if we get test failures in the future. I think it would be better to split that into multiple checks, probably with some more meaningful message about what is wrong (wrong len, wrong content, ...) > + > + /* test string conversion with dstlen=len-1 */ > + memset(c_buffer, '#', sizeof(c_buffer) - 1); > + SetLastError(0xdeadbeef); > + len = WideCharToMultiByte(CP_UTF7, 0, simple_tests[i].utf16, -1, c_buffer, simple_tests[i].utf7_len - 1, NULL, NULL); > + ok(len == 0 && > + memcmp(c_buffer, simple_tests[i].utf7, simple_tests[i].utf7_len - 1) == 0 && > + c_buffer[simple_tests[i].utf7_len - 1] == '#', > + "simple_test failure i=%i dst=\"%s\" len=%i\n", i, c_buffer, len); > + ok(GetLastError() == ERROR_INSUFFICIENT_BUFFER, "error=%x\n", GetLastError()); > + > + /* test string conversion with dstlen=len */ > + memset(c_buffer, '#', sizeof(c_buffer) - 1); > + SetLastError(0xdeadbeef); > + len = WideCharToMultiByte(CP_UTF7, 0, simple_tests[i].utf16, -1, c_buffer, simple_tests[i].utf7_len, NULL, NULL); > + ok(len == simple_tests[i].utf7_len && > + strcmp(c_buffer, simple_tests[i].utf7) == 0 && > + c_buffer[len] == '#', > + "simple_test failure i=%i dst=\"%s\" len=%i\n", i, c_buffer, len); > + ok(GetLastError() == 0xdeadbeef, "error=%x\n", GetLastError()); > + > + /* test string conversion with dstlen=len+1 */ > + memset(c_buffer, '#', sizeof(c_buffer) - 1); > + SetLastError(0xdeadbeef); > + len = WideCharToMultiByte(CP_UTF7, 0, simple_tests[i].utf16, -1, c_buffer, simple_tests[i].utf7_len + 1, NULL, NULL); > + ok(len == simple_tests[i].utf7_len && > + strcmp(c_buffer, simple_tests[i].utf7) == 0 && > + c_buffer[len] == '#', > + "simple_test failure i=%i dst=\"%s\" len=%i\n", i, c_buffer, len); > + ok(GetLastError() == 0xdeadbeef, "error=%x\n", GetLastError()); > + > + /* test dry run with dst=NULL and dstlen=0 */ > + memset(c_buffer, '#', sizeof(c_buffer)); > + SetLastError(0xdeadbeef); > + len = WideCharToMultiByte(CP_UTF7, 0, simple_tests[i].utf16, -1, NULL, 0, NULL, NULL); > + ok(len == simple_tests[i].utf7_len && > + c_buffer[0] == '#', > + "simple_test failure i=%i len=%i\n", i, len); > + ok(GetLastError() == 0xdeadbeef, "error=%x\n", GetLastError()); > + > + /* test dry run with dst!=NULL and dstlen=0 */ > + memset(c_buffer, '#', sizeof(c_buffer) - 1); > + SetLastError(0xdeadbeef); > + len = WideCharToMultiByte(CP_UTF7, 0, simple_tests[i].utf16, -1, c_buffer, 0, NULL, NULL); > + ok(len == simple_tests[i].utf7_len && > + c_buffer[0] == '#', > + "simple_test failure i=%i len=%i\n", i, len); > + ok(GetLastError() == 0xdeadbeef, "error=%x\n", GetLastError()); > + > + /* all simple utf16-to-utf7 tests can be reversed to make utf7-to-utf16 tests */ > + memset(w_buffer, '#', sizeof(w_buffer) - sizeof(WCHAR)); > + SetLastError(0xdeadbeef); > + len = MultiByteToWideChar(CP_UTF7, 0, simple_tests[i].utf7, -1, w_buffer, sizeof(w_buffer) / sizeof(WCHAR)); > + todo_wine ok(len == simple_tests[i].utf16_len && > + memcmp(w_buffer, simple_tests[i].utf16, len * sizeof(WCHAR)) == 0 && > + w_buffer[len] == 0x2323, > + "simple_test failure i=%i dst=%s len=%i\n", i, wine_dbgstr_w(w_buffer), len); > + todo_wine ok(GetLastError() == 0xdeadbeef, "error=%x\n", GetLastError()); Why do you use todo_wine when the whole test block is skipped? Moreover, it should probably go into the later patches which add the MultibyteToWideChar tests. > + } > +} > + > static void test_undefined_byte_char(void) > { > static const struct tag_testset { > @@ -618,6 +750,8 @@ START_TEST(codepage) > test_string_conversion(NULL); > test_string_conversion(&bUsedDefaultChar); > > + test_utf7_encoding(); > + > test_undefined_byte_char(); > test_threadcp(); > } >

Oct. 29, 2014

Re: Need feedback on first four UTF-7 patches

by Sebastian Lackner

On 29.10.2014 20:50, Alex Henrie wrote: > 2014-10-29 6:45 GMT-06:00 Michael Stefaniuc <mstefani(a)redhat.com>: >> But the code still could look a lot better and some of the previous >> feedback wasn't addressed. There is an impedance mismatch between the >> code looking good for Alex and looking good for Wine. > > As far as I know, I only rejected one of your suggestions for these 4 > patches: Merging the simple_test and complex_test structs. The 3 > simple tests are actually used as inputs for 8 tests each, for a total > of 24 tests. Reformatting the 3 simple tests as complex tests would > force me to manually input the 24 combinations into the complex_tests > array. Is that really what Alexandre wants? > > -Alex > > It is not really true that this was the only thing we criticized. We also gave you a lot more suggestions for improvements, but from the beginning I had the impression that you want to keep changes as minimal as possible. Thats not a good attitude when trying to upstream code, and will probably be especially a problem for the implementation itself. You always have to consider multiple methods how a specific problem can be solved. For the tests: The main issue (bad test coverage) is fixed at my opinion, but there are still lot of small things which could be improved to make the code more readable. Most of it was already told on IRC, but I will go over patch 1 one more time and tell you again.

Oct. 29, 2014

Re: [2/2] oleaut32: Implement TLB dependencies lookup in resources (resend 3 - last submit before abandon)

by Andrew Eikum

On Sat, Sep 27, 2014 at 08:10:28PM +0200, Guillaume Charifi wrote: > Fixes https://bugs.winehq.org/show_bug.cgi?id=34184 > > This is the last time without a review before I give up. > I'm not sure why this is hung up, but to reiterate, it seems acceptable to me. Andrew

Oct. 29, 2014