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

Re: Rename "Version" field to "Earliest Affected Version" in Bugzilla?

by Frédéric Delanoy

On Tue, Feb 11, 2014 at 5:13 PM, Bruno Jesus <00cpxxx(a)gmail.com> wrote: > On Tue, Feb 11, 2014 at 11:51 AM, Rosanne DiMesio <dimesio(a)earthlink.net> wrote: > > What if the field is only editable by users when it's in the > "unspecified" state, this way it would work as before. When changed > from unspecified to any other the field would turn read only for > users. Simplest solution would be to make version field editable - upon bug creation for standard users - by admin users all the time Frédéric

Feb. 11, 2014

Re: Rename "Version" field to "Earliest Affected Version" in Bugzilla?

by Bruno Jesus

On Tue, Feb 11, 2014 at 11:51 AM, Rosanne DiMesio <dimesio(a)earthlink.net> wrote: > On Tue, 11 Feb 2014 01:03:52 -0600 > Vincent Povirk <madewokherd(a)gmail.com> wrote: > >> >> I don't think there's a need for a compromise that makes it even more >> complicated. If we're not going to change the policy, making the >> version field write-once for most users is fine for enforcing it imo, >> and as long as we have the original reported version I don't think >> it's worth adding a field for the last tested one. >> > > I agree. > > As for whether the policy should be changed, I can think of one argument against it that hasn't been mentioned: we have a large body of experienced users who are accustomed to doing it the current way, no way to effectively communicate such an abrupt change in policy to all of them, and getting people to change their habits is far more difficult than changing the permissions on a field. In the short term, at least, such a change would lead to more noise in bugzilla as triagers have to explain to experienced users that we've suddenly decided to handle this in a way that is completely opposite to the way we've been doing it for years. What if the field is only editable by users when it's in the "unspecified" state, this way it would work as before. When changed from unspecified to any other the field would turn read only for users.

Feb. 11, 2014

Re: Rename "Version" field to "Earliest Affected Version" in Bugzilla?

by Rosanne DiMesio

On Tue, 11 Feb 2014 01:03:52 -0600 Vincent Povirk <madewokherd(a)gmail.com> wrote: > > I don't think there's a need for a compromise that makes it even more > complicated. If we're not going to change the policy, making the > version field write-once for most users is fine for enforcing it imo, > and as long as we have the original reported version I don't think > it's worth adding a field for the last tested one. > I agree. As for whether the policy should be changed, I can think of one argument against it that hasn't been mentioned: we have a large body of experienced users who are accustomed to doing it the current way, no way to effectively communicate such an abrupt change in policy to all of them, and getting people to change their habits is far more difficult than changing the permissions on a field. In the short term, at least, such a change would lead to more noise in bugzilla as triagers have to explain to experienced users that we've suddenly decided to handle this in a way that is completely opposite to the way we've been doing it for years. -- Rosanne DiMesio <dimesio(a)earthlink.net>

Feb. 11, 2014

Re: [PATCH 3/6] wined3d: Use an rbtree for storing shaders for texture format conversion/blitting

by Martin Storsjö

On Tue, 11 Feb 2014, Stefan Dösinger wrote: > > Am 11.02.2014 um 12:14 schrieb Martin Storsjo <martin(a)martin.st>: > >> --- >> dlls/wined3d/arb_program_shader.c | 187 +++++++++++++++++++++++++------------- >> 1 file changed, 122 insertions(+), 65 deletions(-) >> >> diff --git a/dlls/wined3d/arb_program_shader.c b/dlls/wined3d/arb_program_shader.c >> index 6d17ea3..8390c9e 100644 >> --- a/dlls/wined3d/arb_program_shader.c >> +++ b/dlls/wined3d/arb_program_shader.c >> @@ -6768,22 +6768,68 @@ const struct fragment_pipeline arbfp_fragment_pipeline = { >> arbfp_fragmentstate_template, >> }; >> >> +struct arbfp_blit_type >> +{ >> + enum complex_fixup fixup; >> + GLenum textype; >> +}; >> ... >> +static int arbfp_blit_type_compare(const void *key, const struct wine_rb_entry *entry) >> +{ >> + const struct arbfp_blit_type *ka = key; >> + const struct arbfp_blit_type *kb = &WINE_RB_ENTRY_VALUE(entry, const struct arbfp_blit_desc, entry)->type; >> + >> + return memcmp(ka, kb, sizeof(*ka)); >> +} > You have to watch out for padding space added by the compiler if you memcmp the structure. I think there’s no guarantee that enum complex_fixup is 4 bytes. Either use memset to initialize it to zero, or (preferred) make the fixup member a bitfield and add an explicit padding field for the remaining bits (see some structures in wined3d_private.h for examples) and set the padding to zero manually. Ah, right, yes. Or what about just doing two separate comparisons for fixup and textype instead of a memcmp of the struct? // Martin

Feb. 11, 2014

Re: [PATCH 1/3] msvcrt/tests: Don't pass path containing spaces to _spawnvp (try3)

by Alexandre Julliard

Piotr Caban <piotr.caban(a)gmail.com> writes: > I've seen that this patch was marked as pending. I'm not sure what > should I change in it. The argv[0] argument is not used in child > process so maybe it will be clearer if I put some dummy value there. I > can also change the tests to not use _spawnvp. It's probably ok to use the basename, but you have to do it correctly. Also adding a helper function wouldn't hurt. -- Alexandre Julliard julliard(a)winehq.org

Feb. 11, 2014

Re: [PATCH 6/6] d3d9/tests: Add tests for different YUV texture layouts

by Martin Storsjö

On Tue, 11 Feb 2014, Stefan Dösinger wrote: > Oh, by the way: Were you able to test the texture rectangle codepath > with your test? Yes - now I extended the test to use a non-power-of-two texture size, and if I hack out the ARB_TEXTURE_NON_POWER_OF_TWO support (in wined3d/directx.c) the tests fail and crash. // Martin

Feb. 11, 2014

Re: [PATCH 1/3] msvcrt/tests: Don't pass path containing spaces to _spawnvp (try3)

by Piotr Caban

Hi, s I've seen that this patch was marked as pending. I'm not sure what should I change in it. The argv[0] argument is not used in child process so maybe it will be clearer if I put some dummy value there. I can also change the tests to not use _spawnvp. There's following comment near _spawnvp implementation: Like on Windows, this function does not handle arguments with spaces or double-quotes. Here are some results that shows what happens on windows when the tests are executed without this patch. The behavior on wine is similar. E:\>msvcrt_crosstest.exe file file: 3 tests executed (0 marked as todo, 0 failures), 0 skipped. file: 1 tests executed (0 marked as todo, 0 failures), 0 skipped. file: 5 tests executed (0 marked as todo, 0 failures), 0 skipped. file: 5 tests executed (0 marked as todo, 0 failures), 0 skipped. file: 791 tests executed (0 marked as todo, 0 failures), 0 skipped. E:\>"a a\msvcrt_crosstest.exe" file Fatal: test 'a\msvcrt_crosstest.exe' does not exist. file.c:1417: Test failed: bad position 0 expecting 8 file.c:1419: Test failed: Couldn't read back the data Fatal: test 'a\msvcrt_crosstest.exe' does not exist. Fatal: test 'a\msvcrt_crosstest.exe' does not exist. file.c:1987: Test failed: i 0, got 0 file.c:1990: Test failed: expected to read 'Hello world', got '⌂' file.c:1987: Test failed: i 1, got 0 file.c:1990: Test failed: expected to read 'Hello world', got '⌂' file.c:1987: Test failed: i 2, got 0 file.c:1990: Test failed: expected to read 'Hello world', got '⌂' Fatal: test 'a\msvcrt_crosstest.exe' does not exist. file.c:2024: Test failed: fread() returned 0: ferror=0 file.c:2027: Test failed: got '⌂' expected 'Hello worldHello worldHello world' file: 791 tests executed (0 marked as todo, 10 failures), 0 skipped. In the failing case argv[0] = "a" argv[1] = "a\msvcrt_crosstest.exe" argv[2] = "file" Thanks, Piotr

Feb. 11, 2014

Re: [PATCH 6/6] d3d9/tests: Add tests for different YUV texture layouts

by Stefan Dösinger

Oh, by the way: Were you able to test the texture rectangle codepath with your test? Am 11.02.2014 um 13:13 schrieb Stefan Dösinger <stefandoesinger(a)gmail.com>: > > Am 11.02.2014 um 12:14 schrieb Martin Storsjo <martin(a)martin.st>: >> +#define D3DFMT_YV12 MAKEFOURCC('Y','V','1','2') >> +#define D3DFMT_I420 MAKEFOURCC('I','4','2','0') >> +#define D3DFMT_NV12 MAKEFOURCC('N','V','1','2’) > There’s no need for global definitions. Other tests that use custom fourccs just use MAKEFOURCC in their code. > >> + struct >> + { >> + DWORD color1, color2; >> + DWORD rgb1, rgb2; >> + } >> + test_data[] = >> + { >> + { 0x000000, 0xffffff, 0x00008800, 0x00ff7dff }, >> + { 0xff0000, 0x00ffff, 0x004aff14, 0x00b800ee }, >> + { 0x00ff00, 0xff00ff, 0x000024ee, 0x00ffe114 }, >> + { 0x0000ff, 0xffff00, 0x00b80000, 0x004affff }, >> + { 0xffff00, 0x0000ff, 0x004affff, 0x00b80000 }, >> + { 0xff00ff, 0x00ff00, 0x00ffe114, 0x000024ee }, >> + { 0x00ffff, 0xff0000, 0x00b800ee, 0x004aff14 }, >> + { 0xffffff, 0x000000, 0x00ff7dff, 0x00008800 }, >> + }; > Please make this static const. The same applies to the formats[] table. > > I’ll look at the details of the test and the other patches later today, when my headache is gone :-\ . >

Feb. 11, 2014

Re: [PATCH 3/6] wined3d: Use an rbtree for storing shaders for texture format conversion/blitting

by Stefan Dösinger

Am 11.02.2014 um 12:14 schrieb Martin Storsjo <martin(a)martin.st>: > --- > dlls/wined3d/arb_program_shader.c | 187 +++++++++++++++++++++++++------------- > 1 file changed, 122 insertions(+), 65 deletions(-) > > diff --git a/dlls/wined3d/arb_program_shader.c b/dlls/wined3d/arb_program_shader.c > index 6d17ea3..8390c9e 100644 > --- a/dlls/wined3d/arb_program_shader.c > +++ b/dlls/wined3d/arb_program_shader.c > @@ -6768,22 +6768,68 @@ const struct fragment_pipeline arbfp_fragment_pipeline = { > arbfp_fragmentstate_template, > }; > > +struct arbfp_blit_type > +{ > + enum complex_fixup fixup; > + GLenum textype; > +}; > ... > +static int arbfp_blit_type_compare(const void *key, const struct wine_rb_entry *entry) > +{ > + const struct arbfp_blit_type *ka = key; > + const struct arbfp_blit_type *kb = &WINE_RB_ENTRY_VALUE(entry, const struct arbfp_blit_desc, entry)->type; > + > + return memcmp(ka, kb, sizeof(*ka)); > +} You have to watch out for padding space added by the compiler if you memcmp the structure. I think there’s no guarantee that enum complex_fixup is 4 bytes. Either use memset to initialize it to zero, or (preferred) make the fixup member a bitfield and add an explicit padding field for the remaining bits (see some structures in wined3d_private.h for examples) and set the padding to zero manually. > + desc = HeapAlloc(GetProcessHeap(), 0, sizeof(*desc)); > + if (!desc) > + { > + ERR("Out of memory\n"); > + GL_EXTCALL(glBindProgramARB(GL_FRAGMENT_PROGRAM_ARB, 0)); > + checkGLcall("glBindProgramARB(GL_FRAGMENT_PROGRAM_ARB, 0)"); > + GL_EXTCALL(glDeleteProgramsARB(1, &shader)); > + checkGLcall("GL_EXTCALL(glDeleteProgramsARB(1, &shader))"); > + return 0; > + } > ... > + ERR("Out of memory\n"); > + GL_EXTCALL(glDeleteProgramsARB(1, &shader)); > + checkGLcall("GL_EXTCALL(glDeleteProgramsARB(1, &shader))"); > + GL_EXTCALL(glBindProgramARB(GL_FRAGMENT_PROGRAM_ARB, 0)); > + checkGLcall("glBindProgramARB(GL_FRAGMENT_PROGRAM_ARB, 0)"); > + HeapFree(GetProcessHeap(), 0, desc); > + return 0; > + } You can use goto (yes, goto) to avoid code duplication in error handling. See e.g. wined3d_device_init_3d in device.c > - upload_palette(surface, context); I think you lost that call. The unfortunate thing about the P8 conversion is that the shader conversion codepath is essentially dead code for various reasons. So it’s tricky to test and likely already broken.

Feb. 11, 2014

Re: [PATCH 6/6] d3d9/tests: Add tests for different YUV texture layouts

by Stefan Dösinger

Am 11.02.2014 um 12:14 schrieb Martin Storsjo <martin(a)martin.st>: > +#define D3DFMT_YV12 MAKEFOURCC('Y','V','1','2') > +#define D3DFMT_I420 MAKEFOURCC('I','4','2','0') > +#define D3DFMT_NV12 MAKEFOURCC('N','V','1','2’) There’s no need for global definitions. Other tests that use custom fourccs just use MAKEFOURCC in their code. > + struct > + { > + DWORD color1, color2; > + DWORD rgb1, rgb2; > + } > + test_data[] = > + { > + { 0x000000, 0xffffff, 0x00008800, 0x00ff7dff }, > + { 0xff0000, 0x00ffff, 0x004aff14, 0x00b800ee }, > + { 0x00ff00, 0xff00ff, 0x000024ee, 0x00ffe114 }, > + { 0x0000ff, 0xffff00, 0x00b80000, 0x004affff }, > + { 0xffff00, 0x0000ff, 0x004affff, 0x00b80000 }, > + { 0xff00ff, 0x00ff00, 0x00ffe114, 0x000024ee }, > + { 0x00ffff, 0xff0000, 0x00b800ee, 0x004aff14 }, > + { 0xffffff, 0x000000, 0x00ff7dff, 0x00008800 }, > + }; Please make this static const. The same applies to the formats[] table. I’ll look at the details of the test and the other patches later today, when my headache is gone :-\ .

Feb. 11, 2014