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

Re: d3dx9: d3dx9_base_effect_get_pass_desc

by Paul Gofman

No, I didn't test GetPassDesc shader return behaviour and did not meet any application requiring it. But there is test_effect_preshader_effect_blob in tests/effect.c which has a shader chosen by selector which probably can be used for testing. I suppose the checks for GetPassDesc could be inserted right into test_effect_preshader() test which already tests shader switching through selector. There is already a code which identifies a selected shader, I hope the same can be used for checking GetPassDesc return. I don't know if Matteo will like this idea but the simplest way I see to determine the shader actually selected for pass is to store pointer to it right from d3dx9_apply_state function (in case SC_VERTEXSHADER: and case SC_PIXELSHADER:), as at this place correct shader is determined based on selector value (if selector is used). I can't be sure without testing though what GetPassDesc should return before BeginPass() was called (that is, before any apply_state was called), and then after BeginPass/EndPass was called (will it be last selected? Or first shaders' array element? Or based on the current selector value??). Probably this needs to be tested before deciding on implementation. On 10/31/2016 06:29 PM, Matteo Bruni wrote: > 2016-10-29 21:11 GMT+02:00 Luis C. Busquets Pérez <luis.busquets(a)ilidium.com>: >> Could anyone help improving this patch so that it makes it to the wine >> tree? >> >> >>> --- >>> dlls/d3dx9_36/effect.c | 46 >>> ++++++++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 44 insertions(+), 2 deletions(-) >>> >>> diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c >>> index 42f5aea..c69b73c 100644 >>> --- a/dlls/d3dx9_36/effect.c >>> +++ b/dlls/d3dx9_36/effect.c >>> @@ -150,6 +150,7 @@ struct d3dx9_base_effect >>> UINT parameter_count; >>> UINT technique_count; >>> UINT object_count; >>> + BOOL clonable; >>> >>> struct d3dx_parameter *parameters; >>> struct d3dx_technique *techniques; >>> @@ -999,6 +1000,11 @@ static HRESULT >>> d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base, >>> D3DXHANDLE pass, D3DXPASS_DESC *desc) >>> { >>> struct d3dx_pass *p = get_valid_pass(base, pass); >>> + struct d3dx_state *s; >>> + struct d3dx_parameter *param; >>> + IDirect3DVertexShader9 *vshader; >>> + IDirect3DPixelShader9 *pshader; >>> + UINT i, size; >>> >>> if (!desc || !p) >>> { >>> @@ -1008,11 +1014,45 @@ static HRESULT >>> d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base, >>> >>> desc->Name = p->name; >>> desc->Annotations = p->annotation_count; >>> - >>> - FIXME("Pixel shader and vertex shader are not supported, >>> yet.\n"); >>> desc->pVertexShaderFunction = NULL; >>> desc->pPixelShaderFunction = NULL; >>> >>> + if (base->clonable) >>> + { >>> + for (i=0; i < p->state_count; i++) >>> + { >>> + s = p->states + i; >>> + if (state_table[s->operation].class == SC_VERTEXSHADER) >>> + { >>> + param = &(s->parameter); >>> + vshader = *(struct IDirect3DVertexShader9 **)param- >>>> data; >>> + if (vshader) >>> + { >>> + IDirect3DVertexShader9_GetFunction(vshader, >>> NULL, >>> &size); >>> + if (desc->pVertexShaderFunction) >>> + free((void *)desc->pVertexShaderFunction); >>> + desc->pVertexShaderFunction = malloc(size); >>> + IDirect3DVertexShader9_GetFunction(vshader, >>> (void >>> *)(desc->pVertexShaderFunction), &size); >>> + } >>> + } >>> + if (state_table[s->operation].class == SC_PIXELSHADER) >>> + { >>> + param = &(s->parameter); >>> + pshader = *(struct IDirect3DPixelShader9 **)param- >>>> data; >>> + if (pshader) >>> + { >>> + IDirect3DPixelShader9_GetFunction(pshader, NULL, >>> &size); >>> + if (desc->pPixelShaderFunction) >>> + free((void *)desc->pPixelShaderFunction); >>> + desc->pPixelShaderFunction = malloc(size); >>> + IDirect3DPixelShader9_GetFunction(pshader, (void >>> *)(desc->pPixelShaderFunction), &size); >>> + } >>> + } >>> + } >>> + } > You probably want to take the shader code from the "objects" array instead. > Also that doesn't work for ST_ARRAY_SELECTOR vertex / pixel shader > states. It isn't even obvious what should be the returned value in > that case i.e. it needs tests. > > I think Paul had a look at this at some point although I don't know if > there is any relevant patch attached to a bug or something. > >

Oct. 31, 2016

Re: d3dx9: d3dx9_base_effect_get_pass_desc

by Matteo Bruni

2016-10-29 21:11 GMT+02:00 Luis C. Busquets Pérez <luis.busquets(a)ilidium.com>: > Could anyone help improving this patch so that it makes it to the wine > tree? > > >> --- >> dlls/d3dx9_36/effect.c | 46 >> ++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 44 insertions(+), 2 deletions(-) >> >> diff --git a/dlls/d3dx9_36/effect.c b/dlls/d3dx9_36/effect.c >> index 42f5aea..c69b73c 100644 >> --- a/dlls/d3dx9_36/effect.c >> +++ b/dlls/d3dx9_36/effect.c >> @@ -150,6 +150,7 @@ struct d3dx9_base_effect >> UINT parameter_count; >> UINT technique_count; >> UINT object_count; >> + BOOL clonable; >> >> struct d3dx_parameter *parameters; >> struct d3dx_technique *techniques; >> @@ -999,6 +1000,11 @@ static HRESULT >> d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base, >> D3DXHANDLE pass, D3DXPASS_DESC *desc) >> { >> struct d3dx_pass *p = get_valid_pass(base, pass); >> + struct d3dx_state *s; >> + struct d3dx_parameter *param; >> + IDirect3DVertexShader9 *vshader; >> + IDirect3DPixelShader9 *pshader; >> + UINT i, size; >> >> if (!desc || !p) >> { >> @@ -1008,11 +1014,45 @@ static HRESULT >> d3dx9_base_effect_get_pass_desc(struct d3dx9_base_effect *base, >> >> desc->Name = p->name; >> desc->Annotations = p->annotation_count; >> - >> - FIXME("Pixel shader and vertex shader are not supported, >> yet.\n"); >> desc->pVertexShaderFunction = NULL; >> desc->pPixelShaderFunction = NULL; >> >> + if (base->clonable) >> + { >> + for (i=0; i < p->state_count; i++) >> + { >> + s = p->states + i; >> + if (state_table[s->operation].class == SC_VERTEXSHADER) >> + { >> + param = &(s->parameter); >> + vshader = *(struct IDirect3DVertexShader9 **)param- >> > >> > data; >> + if (vshader) >> + { >> + IDirect3DVertexShader9_GetFunction(vshader, >> NULL, >> &size); >> + if (desc->pVertexShaderFunction) >> + free((void *)desc->pVertexShaderFunction); >> + desc->pVertexShaderFunction = malloc(size); >> + IDirect3DVertexShader9_GetFunction(vshader, >> (void >> *)(desc->pVertexShaderFunction), &size); >> + } >> + } >> + if (state_table[s->operation].class == SC_PIXELSHADER) >> + { >> + param = &(s->parameter); >> + pshader = *(struct IDirect3DPixelShader9 **)param- >> > >> > data; >> + if (pshader) >> + { >> + IDirect3DPixelShader9_GetFunction(pshader, NULL, >> &size); >> + if (desc->pPixelShaderFunction) >> + free((void *)desc->pPixelShaderFunction); >> + desc->pPixelShaderFunction = malloc(size); >> + IDirect3DPixelShader9_GetFunction(pshader, (void >> *)(desc->pPixelShaderFunction), &size); >> + } >> + } >> + } >> + } You probably want to take the shader code from the "objects" array instead. Also that doesn't work for ST_ARRAY_SELECTOR vertex / pixel shader states. It isn't even obvious what should be the returned value in that case i.e. it needs tests. I think Paul had a look at this at some point although I don't know if there is any relevant patch attached to a bug or something.

Oct. 31, 2016

Re: [PATCH 7/7] ddraw: Fix remaining style issues in executebuffer.c

by Henri Verbeet

On 30 October 2016 at 15:02, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote: > Feel free to ignore this. With this the file follows the wined3d style, > unless I missed something... > +static void _dump_executedata(const D3DEXECUTEDATA *lpData) Naming conventions for "_dump_executedata" and "lpData". > TRACE("dwSize : %d\n", lpData->dwSize); %u for unsigned integers, punctuation. > -static void _dump_D3DEXECUTEBUFFERDESC(const D3DEXECUTEBUFFERDESC *lpDesc) { > +static void _dump_D3DEXECUTEBUFFERDESC(const D3DEXECUTEBUFFERDESC *lpDesc) > +{ > TRACE("dwSize : %d\n", lpDesc->dwSize); Likewise. > TRACE("dwFlags : %x\n", lpDesc->dwFlags); %#x for hexadecimal values. > @@ -76,12 +78,12 @@ HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, > for (;;) > { > D3DINSTRUCTION *current = (D3DINSTRUCTION *)instr; Should probably be const. > - BYTE size; > - WORD count; > - > - count = current->wCount; > - size = current->bSize; > - instr += sizeof(D3DINSTRUCTION); > + BYTE size; > + WORD count; > + > + count = current->wCount; > + size = current->bSize; > + instr += sizeof(D3DINSTRUCTION); "sizeof(*current)" > instr = (char*)current + ci->dwOffset; "(char *)" > + default: > + ERR("Unhandled OpCode %d !!!\n",current->bOpcode); Should probably use %#x. Punctuation. Some of these probably deserve a patch on their own, and there may very well be more issues.

Oct. 31, 2016

Re: [PATCH 6/7] ddraw: Implement executebuffer point and line draws.

by Henri Verbeet

On 30 October 2016 at 15:02, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote: > case D3DOP_TRIANGLE: > { > WORD *indices; > - unsigned int index_pos = buffer->index_pos; > + unsigned int index_pos = buffer->index_pos, num_indices; Since you're doing style cleanups in the next patch, "index_count".

Oct. 31, 2016

Re: [PATCH 5/7] ddraw: Discard the dst vertex buffer if possible.

by Henri Verbeet

On 30 October 2016 at 15:02, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote: > Once we have buffer->buffer copies working we can load the source data > into a buffer in SetExecuteData and never have to lock the destination > buffer. In general I think wined3d_device_copy_sub_resource_region() is supposed to work for buffers. Is there something in particular that's broken?

Oct. 31, 2016

Re: [PATCH] wined3d: Invalidate INDEXBUFFER device state when contents of bound index buffer is changed.

by Józef Kucia

On Mon, Oct 31, 2016 at 1:27 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 31 October 2016 at 12:51, Józef Kucia <joseph.kucia(a)gmail.com> wrote: >> On Fri, Oct 28, 2016 at 3:55 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: >>> On 28 October 2016 at 12:20, Józef Kucia <jkucia(a)codeweavers.com> wrote: >>>> + if (buffer->resource.bind_count) >>>> + { >>>> + if (buffer->buffer_type_hint == GL_ELEMENT_ARRAY_BUFFER) >>>> + device_invalidate_state(buffer->resource.device, STATE_INDEXBUFFER); >>>> + } >>> You can merge those if-conditions. >>> >>> What happens exactly that makes this required? >> >> The bound index buffer is unloaded and the game crashes on the next >> draw call because we use indexed drawing with zeroish offset and >> GL_ELEMENT_ARRAY_BUFFER_BINDING is 0. >> > I figured it was something like that, but the question is why that > happens exactly. In theory context_apply_draw_state() will load the > index buffer into a specific location based on "stream_info.all_vbo", > and context_update_stream_info() should make sure STATE_INDEXBUFFER is > invalidated if that changes between draws. Invalidating it in > buffer_unload() makes more sense, if the issue is that > glDeleteBuffers() gets called for an index buffer that's still bound. > (And we already have a similar invalidate for vertex buffers there.) > An argument could probably be made that the invalidate belongs in > delete_gl_buffer() instead of buffer_unload(), and that > STATE_STREAMSRC doesn't need to be invalidated for anything other than > vertex buffers. Insane 2 uses a single index buffer for all draw calls when rendering game menus. This index buffer is still bound when it's unloaded. glDeleteBuffers() unbinds the deleted buffer object in the current context. After the buffer is unloaded the application still uses this deleted buffer object in other threads, but when it tries to render again in the thread in which the buffer object was deleted the crash occurs.

Oct. 31, 2016

Re: [PATCH] wined3d: Invalidate INDEXBUFFER device state when contents of bound index buffer is changed.

by Henri Verbeet

On 31 October 2016 at 12:51, Józef Kucia <joseph.kucia(a)gmail.com> wrote: > On Fri, Oct 28, 2016 at 3:55 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: >> On 28 October 2016 at 12:20, Józef Kucia <jkucia(a)codeweavers.com> wrote: >>> + if (buffer->resource.bind_count) >>> + { >>> + if (buffer->buffer_type_hint == GL_ELEMENT_ARRAY_BUFFER) >>> + device_invalidate_state(buffer->resource.device, STATE_INDEXBUFFER); >>> + } >> You can merge those if-conditions. >> >> What happens exactly that makes this required? > > The bound index buffer is unloaded and the game crashes on the next > draw call because we use indexed drawing with zeroish offset and > GL_ELEMENT_ARRAY_BUFFER_BINDING is 0. > I figured it was something like that, but the question is why that happens exactly. In theory context_apply_draw_state() will load the index buffer into a specific location based on "stream_info.all_vbo", and context_update_stream_info() should make sure STATE_INDEXBUFFER is invalidated if that changes between draws. Invalidating it in buffer_unload() makes more sense, if the issue is that glDeleteBuffers() gets called for an index buffer that's still bound. (And we already have a similar invalidate for vertex buffers there.) An argument could probably be made that the invalidate belongs in delete_gl_buffer() instead of buffer_unload(), and that STATE_STREAMSRC doesn't need to be invalidated for anything other than vertex buffers. > The other thing is that, in theory, when a buffer is modified in the > current context we should should invalidate buffer bindings for this > buffer in other contexts in order to make the contents changes visible > in other contexts. That's the reason why I was trying to restore > device_invalidate_state() call in wined3d_buffer_load(). This would > fix the regression as well. The quote from GL spec: > "If the contents of an object T are changed in a context other than the cur- > rent context, T must be attached or re-attached to at least one > binding point in the > current context, or at least one attachment point of a currently bound container > object C, in order to guarantee that the new contents of T are visible > in the current > context." Yeah, but it's pretty hopeless since you'd also need to flush, and then you get into "StrictDrawOrdering" territory. This was actually the kind of thing (among others) that CSMT was intended to fix, but of course people in general care more about fps than correctness.

Oct. 31, 2016

Re: [PATCH] wined3d: Invalidate INDEXBUFFER device state when contents of bound index buffer is changed.

by Józef Kucia

On Fri, Oct 28, 2016 at 3:55 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote: > On 28 October 2016 at 12:20, Józef Kucia <jkucia(a)codeweavers.com> wrote: >> + if (buffer->resource.bind_count) >> + { >> + if (buffer->buffer_type_hint == GL_ELEMENT_ARRAY_BUFFER) >> + device_invalidate_state(buffer->resource.device, STATE_INDEXBUFFER); >> + } > You can merge those if-conditions. > > What happens exactly that makes this required? The bound index buffer is unloaded and the game crashes on the next draw call because we use indexed drawing with zeroish offset and GL_ELEMENT_ARRAY_BUFFER_BINDING is 0. I've sent another patch which targets this issue. The other thing is that, in theory, when a buffer is modified in the current context we should should invalidate buffer bindings for this buffer in other contexts in order to make the contents changes visible in other contexts. That's the reason why I was trying to restore device_invalidate_state() call in wined3d_buffer_load(). This would fix the regression as well. The quote from GL spec: "If the contents of an object T are changed in a context other than the cur- rent context, T must be attached or re-attached to at least one binding point in the current context, or at least one attachment point of a currently bound container object C, in order to guarantee that the new contents of T are visible in the current context."

Oct. 31, 2016

Re: [PATCH 2/7] ddraw: Use wined3d_device_process_vertices for execute buffers.

by Henri Verbeet

On 30 October 2016 at 15:02, Stefan Dösinger <stefandoesinger(a)gmx.at> wrote: > @@ -318,65 +277,36 @@ HRESULT d3d_execute_buffer_execute(struct d3d_execute_buffer *buffer, > + hr = wined3d_resource_map(wined3d_buffer_get_resource(buffer->src_vertex_buffer), 0, > + &map_desc, &box, WINED3D_MAP_DISCARD); > if (FAILED(hr)) > return hr; > - dst = map_desc.data; > + memcpy(map_desc.data, src, ci->dwCount * sizeof(*src)); > + wined3d_resource_unmap(wined3d_buffer_get_resource(buffer->dst_vertex_buffer), 0); That looks wrong.

Oct. 31, 2016

Re: [PATCH] xinput1_3: Prefer builtin DLL

by Filip Frackiewicz

Ahh I see what you mean now, thank you for clarifying :) Filip On Sun, Oct 30, 2016 at 4:04 PM, Bruno Jesus <00cpxxx(a)gmail.com> wrote: > On Sun, Oct 30, 2016 at 12:37 PM, Filip Frackiewicz > <fjfrackiewicz(a)gmail.com> wrote: >> Hi Bruno, > > Hello, thanks for the email. > >> I hope I am not intruding or stating something already known,,, >> >>>- case DLL_WINE_PREATTACH: >>>- return FALSE; /* prefer native version */ >> case DLL_PROCESS_ATTACH: >> DisableThreadLibraryCalls(inst); >> break; >> >> This code looks different than the patch >> https://bugs.winehq.org/attachment.cgi?id=55851 attached to >> https://bugs.winehq.org/show_bug.cgi?id=41428 > > If you take a look at the whole function [1] you will see that it > returns TRUE in the end, so there is no need for the "case > DLL_WINE_PREATTACH" any longer. > > [1] http://source.winehq.org/source/dlls/xinput1_3/xinput1_3_main.c#0042

Oct. 30, 2016