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

Re: [PATCH 1/2] d3drm: Implement IDirect3DRM*::CreateObject (v2).

by Aaryaman Vasishta

Keeping it simple by using the easiest way would be better I think. Besides the callback functions all behave the same regardless of the object it was called on, according to the tests. Cheers, Aaryaman On Wed, Apr 20, 2016 at 7:34 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 20 April 2016 at 16:00, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> > wrote: > > Sorry for the late reply, I was sick for the last couple of days. > > > > On Tue, Apr 19, 2016 at 6:05 PM, Henri Verbeet <hverbeet(a)gmail.com> > wrote: > >> > >> On 18 April 2016 at 19:00, Aaryaman Vasishta <jem456.vasishta(a)gmail.com > > > >> wrote: > >> > diff --git a/dlls/d3drm/tests/d3drm.c b/dlls/d3drm/tests/d3drm.c > >> > index a8f9c40..2b0174d 100644 > >> > --- a/dlls/d3drm/tests/d3drm.c > >> > +++ b/dlls/d3drm/tests/d3drm.c > >> > @@ -1269,7 +1269,8 @@ static void test_destroy_callback(unsigned int > >> > test_idx, REFCLSID clsid, REFIID > >> > context.test_idx = test_idx; > >> > context.obj = obj; > >> > > >> > - hr = IDirect3DRMObject_AddDestroyCallback(obj, NULL, &context); > >> > + if ((hr = IDirect3DRMObject_AddDestroyCallback(obj, NULL, > >> > &context)) != D3DRMERR_BADVALUE) > >> > + return; > >> I don't think we want that. Doing this means we won't notice when the > >> function starts returning the wrong value, and the following tests get > >> effectively disabled too in that case. > >> > > I do agree, but what should we do when a new object is implemented? If > it's > > okay to extend CreateObject to include the new interface as well as > > implement Add/Delete destroy callbacks in the same patch, then there's no > > problem going with this. I had intended to keep the return statement in > case > > a new object was added into CreateObject, but the destroy callback > functions > > for that object weren't implemented. > The easiest way would be to just send the patch that implements those > first. The slightly more complicated way would be to add another todo > flag. >

April 20, 2016

Re: [PATCH 1/2] d3drm: Implement IDirect3DRM*::CreateObject (v2).

by Henri Verbeet

On 20 April 2016 at 16:00, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote: > Sorry for the late reply, I was sick for the last couple of days. > > On Tue, Apr 19, 2016 at 6:05 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: >> >> On 18 April 2016 at 19:00, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> >> wrote: >> > diff --git a/dlls/d3drm/tests/d3drm.c b/dlls/d3drm/tests/d3drm.c >> > index a8f9c40..2b0174d 100644 >> > --- a/dlls/d3drm/tests/d3drm.c >> > +++ b/dlls/d3drm/tests/d3drm.c >> > @@ -1269,7 +1269,8 @@ static void test_destroy_callback(unsigned int >> > test_idx, REFCLSID clsid, REFIID >> > context.test_idx = test_idx; >> > context.obj = obj; >> > >> > - hr = IDirect3DRMObject_AddDestroyCallback(obj, NULL, &context); >> > + if ((hr = IDirect3DRMObject_AddDestroyCallback(obj, NULL, >> > &context)) != D3DRMERR_BADVALUE) >> > + return; >> I don't think we want that. Doing this means we won't notice when the >> function starts returning the wrong value, and the following tests get >> effectively disabled too in that case. >> > I do agree, but what should we do when a new object is implemented? If it's > okay to extend CreateObject to include the new interface as well as > implement Add/Delete destroy callbacks in the same patch, then there's no > problem going with this. I had intended to keep the return statement in case > a new object was added into CreateObject, but the destroy callback functions > for that object weren't implemented. The easiest way would be to just send the patch that implements those first. The slightly more complicated way would be to add another todo flag.

April 20, 2016

Re: [PATCH 1/2] d3drm: Implement IDirect3DRM*::CreateObject (v2).

by Aaryaman Vasishta

Sorry for the late reply, I was sick for the last couple of days. On Tue, Apr 19, 2016 at 6:05 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 18 April 2016 at 19:00, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> > wrote: > > diff --git a/dlls/d3drm/tests/d3drm.c b/dlls/d3drm/tests/d3drm.c > > index a8f9c40..2b0174d 100644 > > --- a/dlls/d3drm/tests/d3drm.c > > +++ b/dlls/d3drm/tests/d3drm.c > > @@ -1269,7 +1269,8 @@ static void test_destroy_callback(unsigned int > test_idx, REFCLSID clsid, REFIID > > context.test_idx = test_idx; > > context.obj = obj; > > > > - hr = IDirect3DRMObject_AddDestroyCallback(obj, NULL, &context); > > + if ((hr = IDirect3DRMObject_AddDestroyCallback(obj, NULL, > &context)) != D3DRMERR_BADVALUE) > > + return; > I don't think we want that. Doing this means we won't notice when the > function starts returning the wrong value, and the following tests get > effectively disabled too in that case. > > I do agree, but what should we do when a new object is implemented? If it's okay to extend CreateObject to include the new interface as well as implement Add/Delete destroy callbacks in the same patch, then there's no problem going with this. I had intended to keep the return statement in case a new object was added into CreateObject, but the destroy callback functions for that object weren't implemented. > > > + hr = IDirect3DRM_CreateObject(d3drm1, &CLSID_DirectDraw, NULL, > &IID_IDirectDraw, (void **)&unknown); > > + ok(hr == CLASSFACTORY_E_FIRST, "Expected hr == > CLASSFACTORY_E_FIRST, got %#x.\n", hr); > > + ok(!unknown, "Expected object returned == NULL, got %p.\n", > unknown); > It would be better to initialise "unknown" with some known value > before calling CreateObject(). Like this, if CreateObject() doesn't > touch the value, the test may randomly pass or fail depending on what > the initial value of "unknown" ends up being. > > Right, I'll resend the patch with the changes right away. Cheers, Aaryaman

April 20, 2016

Re: [PATCH] comctl32/tooltips: Protect TTM_ADDTOOLW from invalid text pointers

by Nikolay Sivov

This is broken even more than a single test failure, apparently tests don't prove what initial patch was trying to fix.

April 20, 2016

Re: [PATCH v3 2/8] wined3d: Implement readback from 2D array textures using glGet[Compressed]TexImage().

by Henri Verbeet

On 20 April 2016 at 11:42, Józef Kucia <jkucia(a)codeweavers.com> wrote: > @@ -874,7 +879,31 @@ static void surface_download_data(struct wined3d_surface *surface, const struct > return; > } > > - wined3d_texture_get_memory(texture, surface_get_sub_resource_idx(surface), &data, dst_location); > + sub_resource = wined3d_texture_get_sub_resource(texture, sub_resource_idx); Actually, don't use wined3d_texture_get_sub_resource() if you know the index is valid. It does extra validation that's only useful when the index comes from outside of wined3d. > + wined3d_texture_bind_and_dirtify(texture, context, !(sub_resource->locations & WINED3D_LOCATION_TEXTURE_RGB)); This is an unrelated change. In itself it's probably fine to make surface_download_data() bind the texture, but if you do that it becomes inconsistent with surface_upload_data(), wined3d_volume_download_data() and wined3d_volume_upload_data().

April 20, 2016

Re: [PATCH] comctl32/tooltips: Protect TTM_ADDTOOLW from invalid text pointers

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=22377 Your paranoid android. === wxppro (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w2003std (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === wvistau64 (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w2008s64 (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w7u (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w7pro64 (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w8 (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w864 (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w1064 (32 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === wvistau64 (64 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w2008s64 (64 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w7pro64 (64 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w864 (64 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed! === w1064 (64 bit tooltips) === tooltips.c:471: Test failed: Adding the tool to the tooltip failed!

April 20, 2016

Re: [PATCH 5/8] wined3d: Fixup vertex position in geometry shaders.

by Henri Verbeet

Do we need this if we have GLSL 1.50 / ARB_fragment_coord_conventions?

April 20, 2016

Re: [PATCH 3/8] wined3d: Simplify shader interface matching for SM4+ vertex shaders.

by Henri Verbeet

On 19 April 2016 at 18:56, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > After some testing it looks like, with SM4+ on Windows: > 1. Output registers have to match with input registers from the > following shader stage (register index and mask). There is no "smart" > matching of semantics like in SM3. > 2. Using uninitialized varyings in SM4+ shaders doesn't give consistent > results. > That means we can simplify the relevant code quite a bit. Applications > shouldn't be able to depend on particular values for uninitialized > varyings so we can avoid explicitly initializing them. > Does that mean we don't need the separate "reorder" shader between the stages? (Even more so if we have separate shader objects.) On 19 April 2016 at 18:56, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > -static void shader_glsl_setup_shader_output(struct shader_glsl_priv *priv, > +static void shader_glsl_setup_vs3_output(struct shader_glsl_priv *priv, > const struct wined3d_gl_info *gl_info, const DWORD *map, > const struct wined3d_shader_signature *input_signature, > const struct wined3d_shader_reg_maps *reg_maps_in, > const struct wined3d_shader_signature *output_signature, > - const struct wined3d_shader_reg_maps *reg_maps_out, const char *out_array_name) > + const struct wined3d_shader_reg_maps *reg_maps_out) > { > struct wined3d_string_buffer *destination = string_buffer_get(&priv->string_buffers); > BOOL legacy_context = gl_info->supported[WINED3D_GL_LEGACY_CONTEXT]; > @@ -5191,7 +5191,7 @@ static void shader_glsl_setup_shader_output(struct shader_glsl_priv *priv, > else if (in_idx == in_count + 1) > string_buffer_sprintf(destination, "gl_FrontSecondaryColor"); > else > - string_buffer_sprintf(destination, "%s[%u]", out_array_name, in_idx); > + string_buffer_sprintf(destination, "%s[%u]", "ps_link", in_idx); > > if (!set[in_idx]) > set[in_idx] = ~0u; > @@ -5243,7 +5243,7 @@ static void shader_glsl_setup_shader_output(struct shader_glsl_priv *priv, > else if (i == in_count + 1) > string_buffer_sprintf(destination, "gl_FrontSecondaryColor"); > else > - string_buffer_sprintf(destination, "%s[%u]", out_array_name, i); > + string_buffer_sprintf(destination, "%s[%u]", "ps_link", i); > > if (size == 1) > shader_addline(buffer, "%s.%s = 0.0;\n", destination->buffer, reg_mask); This looks like you should have just kept it the way it was in the previous patch.

April 20, 2016

Re: [PATCH v2 1/8] wined3d: Alpha test emulation for core profile contexts.

by Henri Verbeet

On 19 April 2016 at 18:56, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > + /* alpha_func is the PASS condition, not the DISCARD condition. Instead of > + * flipping all the operators here, just negate the comparison below. */ As an aside, note that in general inverting operators isn't safe if there are potential NaNs involved. > + static const char *comparison_operator[] = > + { > + "", /* WINED3D_CMP_NEVER */ > + "<", /* WINED3D_CMP_LESS */ > + "==", /* WINED3D_CMP_EQUAL */ > + "<=", /* WINED3D_CMP_LESSEQUAL */ > + ">", /* WINED3D_CMP_GREATER */ > + "!=", /* WINED3D_CMP_NOTEQUAL */ > + ">=", /* WINED3D_CMP_GREATEREQUAL */ > + "" /* WINED3D_CMP_ALWAYS */ > + }; I should have noticed the first time, but I think you'll want to make the string pointers const as well. > @@ -3091,6 +3091,13 @@ void find_ps_compile_args(const struct wined3d_state *state, const struct wined3 > args->pointsprite = state->render_states[WINED3D_RS_POINTSPRITEENABLE] > && state->gl_primitive_type == GL_POINTS; > > + if (gl_info->supported[WINED3D_GL_LEGACY_CONTEXT]) > + args->alpha_test_func = WINED3D_CMP_ALWAYS - 1; > + else > + args->alpha_test_func = (state->render_states[WINED3D_RS_ALPHATESTENABLE] > + ? wined3d_sanitize_cmp_func(state->render_states[WINED3D_RS_ALPHAFUNC]) > + : WINED3D_CMP_ALWAYS) - 1; For what it's worth, what I originally had in mind was mapping WINED3D_CMP_ALWAYS to 0. I.e., masking with 7. This works as well.

April 20, 2016

Re: [PATCH 05/10] wined3d: Try to avoid glGetTexImage() for 2D array textures.

by Henri Verbeet

On 20 April 2016 at 10:54, Józef Kucia <joseph.kucia(a)gmail.com> wrote: > Is patch 121528 ok in its current form? It doesn't feel like a good > idea messing with other surfaces in surface_download_data(). It seems ok, but I'd have to review it properly.

April 20, 2016