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

Re: [PATCH] msi: Use special errorctrl-flag for CreateService

by Hans Leidekker

On Sun, 2016-05-29 at 23:07 +0200, Andreas Maier wrote: > diff --git a/dlls/msi/action.c b/dlls/msi/action.c > index 370007c..5675900 100644 > --- a/dlls/msi/action.c > +++ b/dlls/msi/action.c > @@ -5811,6 +5811,7 @@ static UINT ITERATE_InstallService(MSIRECORD *rec, LPVOID param) > LPWSTR name = NULL, disp = NULL, load_order = NULL, serv_name = NULL; > LPWSTR depends = NULL, pass = NULL, args = NULL, image_path = NULL; > DWORD serv_type, start_type, err_control; > + BOOL can_fail; 'is_vital' would be better. > SERVICE_DESCRIPTIONW sd = {NULL}; > UINT ret = ERROR_SUCCESS; > > @@ -5848,6 +5849,13 @@ static UINT ITERATE_InstallService(MSIRECORD *rec, LPVOID param) > deformat_string(package, MSI_RecordGetString(rec, 10), &pass); > deformat_string(package, MSI_RecordGetString(rec, 11), &args); > deformat_string(package, MSI_RecordGetString(rec, 13), &sd.lpDescription); > + > + /* Should the complete install fail if CreateService fails? */ > + can_fail = (err_control && msidbServiceInstallErrorControlVital >= 0); This can't work. You want: is_vital = (err_control & msidbServiceInstallErrorControlVital); > + /* Remove the msidbServiceInstallErrorControlVital-flag from err_control. > + CreateService (under Windows) would fail if not. */ > + err_control &= ~msidbServiceInstallErrorControlVital; > > /* fetch the service path */ > row = MSI_QueryGetRecord(package->db, query, comp); > @@ -5890,7 +5898,12 @@ static UINT ITERATE_InstallService(MSIRECORD *rec, LPVOID param) > if (!service) > { > if (GetLastError() != ERROR_SERVICE_EXISTS) > + { > ERR("Failed to create service %s: %d\n", debugstr_w(name), GetLastError()); > + if (can_fail) > + return ERROR_INSTALL_FAILURE; While you're at it, this ERR should be a WARN.

May 31, 2016

Re: [PATCH] d3dx9: Improve performance and memory usage in preshader constants setting.

by Matteo Bruni

2016-05-26 20:28 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > Signed-off-by: Paul Gofman <gofmanp(a)gmail.com> > --- > dlls/d3dx9_36/d3dx9_private.h | 25 +++- > dlls/d3dx9_36/preshader.c | 259 +++++++++++++++++++++++++++++++++++------- > 2 files changed, 237 insertions(+), 47 deletions(-) > > diff --git a/dlls/d3dx9_36/d3dx9_private.h b/dlls/d3dx9_36/d3dx9_private.h > index 9938d83..970ef2f 100644 > --- a/dlls/d3dx9_36/d3dx9_private.h > +++ b/dlls/d3dx9_36/d3dx9_private.h > @@ -129,14 +129,33 @@ enum pres_reg_tables > PRES_REGTAB_FIRST_SHADER = PRES_REGTAB_CONST, > }; > > +enum const_param_copy_state_type > +{ > + COPY_STATE_TYPE_PARAM, > + COPY_STATE_TYPE_TABLE, > + COPY_STATE_TYPE_PARAM_OFFSET, > + COPY_STATE_TYPE_TABLE_OFFSET, > + COPY_STATE_TYPE_COPY_COUNT The name of the last enumerator is a bit ambiguous (it might be mistaken to be a _COUNT entry == last actual enumerator + 1). I would either change the name a bit or move it from the last place. Although this might be moot, see below. > +struct d3dx_const_copy_state I'm not entirely happy with the name, maybe d3dx_const_param_eval_output or something in that fashion? > @@ -811,10 +820,10 @@ static void d3dx_free_const_tab(struct d3dx_const_tab *ctab) > { > HeapFree(GetProcessHeap(), 0, ctab->inputs); > HeapFree(GetProcessHeap(), 0, ctab->inputs_param); > - if (ctab->ctab) > - ID3DXConstantTable_Release(ctab->ctab); > + HeapFree(GetProcessHeap(), 0, ctab->const_set); > } > > + > static void d3dx_free_preshader(struct d3dx_preshader *pres) That extra blank line doesn't seem necessary. > @@ -835,14 +844,181 @@ void d3dx_free_param_eval(struct d3dx_param_eval *peval) > HeapFree(GetProcessHeap(), 0, peval); > } > > -static HRESULT set_constants_param(struct d3dx_regstore *rs, struct d3dx_const_tab *const_tab, > - D3DXHANDLE hc, struct d3dx_parameter *param) > +static void set_constants(struct d3dx_regstore *rs, struct d3dx_const_tab *const_tab) > +{ > + unsigned int i; > + struct d3dx_const_copy_state set_state; > + > + memset(&set_state, 0, sizeof(set_state)); > + for (i = 0; i < const_tab->const_set_count; ++i) > + { > + switch (const_tab->const_set[i].copy_state_type) > + { > + case COPY_STATE_TYPE_PARAM: > + set_state.param = const_tab->const_set[i].state_change.param; > + break; > + case COPY_STATE_TYPE_TABLE: > + set_state.table = const_tab->const_set[i].state_change.value; > + break; > + case COPY_STATE_TYPE_PARAM_OFFSET: > + set_state.param_offset = const_tab->const_set[i].state_change.value; > + break; > + case COPY_STATE_TYPE_TABLE_OFFSET: > + set_state.table_offset = const_tab->const_set[i].state_change.value; > + break; > + case COPY_STATE_TYPE_COPY_COUNT: > + { > + unsigned int count; > + struct d3dx_parameter *param; > + enum pres_value_type table_type; > + unsigned int j; > + > + count = const_tab->const_set[i].state_change.value; > + param = set_state.param; > + table_type = table_info[set_state.table].type; > + if ((set_state.table_offset + count - 1) / table_info[set_state.table].reg_component_count > + >= rs->table_sizes[set_state.table]) > + { > + FIXME("Insufficient table space allocated.\n"); > + break; > + } > + if ((param->type == D3DXPT_FLOAT && table_type == PRES_VT_FLOAT) > + || (param->type == D3DXPT_INT && table_type == PRES_VT_INT) > + || (param->type == D3DXPT_BOOL && table_type == PRES_VT_BOOL)) > + { > + regstore_set_values(rs, set_state.table, (unsigned int *)param->data + set_state.param_offset, > + set_state.table_offset, count); > + } > + else > + { > + for (j = 0; j < count; ++j) > + { > + unsigned int out; > + unsigned int *in = (unsigned int *)param->data + set_state.param_offset + j; > + switch (table_info[set_state.table].type) > + { > + case PRES_VT_FLOAT: set_number(&out, D3DXPT_FLOAT, in, param->type); break; > + case PRES_VT_INT: set_number(&out, D3DXPT_INT, in, param->type); break; > + case PRES_VT_BOOL: set_number(&out, D3DXPT_BOOL, in, param->type); break; > + default: > + FIXME("Unexpected type %#x.\n", table_info[set_state.table].type); > + break; > + } > + regstore_set_values(rs, set_state.table, &out, set_state.table_offset + j, 1); > + } > + } > + set_state.param_offset += count; > + set_state.table_offset += count; > + break; > + } > + } > + } > +} This seems overly complicated and fragile. Instead of this kind of "5 instructions bytecode" isn't it easier to just store and then lookup a struct with all the info you need? I guess the end result would be something akin to storing an array of struct d3dx_const_copy_state in place of struct d3dx_const_param_set. I don't know how the code would exactly look like but I would start by moving building all the struct d3dx_const_copy_param to init time. BTW the coalescing you currently do in add_const_set() could be potentially done as a separate pass after you generate all the "d3dx_const_copy_state-like" structures (but still at init time), which might be simpler. Does this make sense? Am I missing something? > +static HRESULT add_const_set_value(struct d3dx_const_tab *const_tab, unsigned int value, > + unsigned int *current_value, enum const_param_copy_state_type type) > +{ > + struct d3dx_const_param_set set; > + HRESULT hr; > + > + if (*current_value != value) > + { > + *current_value = value; > + set.copy_state_type = type; > + set.state_change.value = value; > + if (FAILED(hr=append_const_set(const_tab, &set))) Whitespace. > @@ -931,21 +1107,17 @@ static HRESULT set_constants_param(struct d3dx_regstore *rs, struct d3dx_const_t > major_stride = max(minor, table_info[table].reg_component_count); > n = min(major * major_stride, > desc.RegisterCount * table_info[table].reg_component_count + major_stride - 1) / major_stride; > + > for (i = 0; i < n; ++i) > { > for (j = 0; j < minor; ++j) > { > - unsigned int out; > - unsigned int *in; > unsigned int offset; > > offset = start_offset + i * major_stride + j; > - if (offset / table_info[table].reg_component_count >= rs->table_sizes[table]) > - { > - if (table_info[table].reg_component_count != 1) > - FIXME("Output offset exceeds table size, name %s, component %u.\n", desc.Name, i); > + if ((offset - start_offset) / table_info[table].reg_component_count >= desc.RegisterCount) > break; Maybe keeping a WARN or something makes sense here?

May 30, 2016

Re: [v3 1/2] d3dx9: Implement IsParameterUsed function in effect.

by Matteo Bruni

2016-05-26 20:18 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > +static BOOL walk_parameter_dep(struct d3dx_parameter *param, walk_parameter_dep_func param_func, > + void *data); > + > +static BOOL walk_param_eval_dep(struct d3dx_param_eval *param_eval, walk_parameter_dep_func param_func, > + void *data) > +{ Depending on what you will do with the other planned use, it might make sense to specialize these functions for the only use case you have at the moment, simplifying (e.g. dropping the callback) and renaming (e.g. is_param_eval_param_used() - a name that shows what you expect from these functions) them in the process.

May 30, 2016

Re: CSMT

by Bruno Jesus

On Mon, May 30, 2016 at 1:51 PM, Snorri Sturluson <snorri.sturluson(a)ccpgames.com> wrote: > I’m experimenting with the CSMT (Commandstream multithreading) feature in > wine-staging for improved graphic performance in EVE when running on Mac. > > My initial tests are promising in performance aspect, but unfortunately > parts of the scene are flickering in and out. Has anyone seen anything > similar? Anyone have any pointers on what I should look for to fix this? At least 2 different bugs talk about flickering and CSMT, until now there is no know solution. https://bugs.winehq.org/show_bug.cgi?id=40275 https://bugs.winehq.org/show_bug.cgi?id=39351 Best Regards, Bruno

May 30, 2016

CSMT

by Snorri Sturluson

I'm experimenting with the CSMT (Commandstream multithreading) feature in wine-staging for improved graphic performance in EVE when running on Mac. My initial tests are promising in performance aspect, but unfortunately parts of the scene are flickering in and out. Has anyone seen anything similar? Anyone have any pointers on what I should look for to fix this? /Snorri Sturluson

May 30, 2016

Re: [PATCH 1/5] d3d11: Validate offsets and sizes in parse_dxbc() (AFL).

by Michael Stefaniuc

Hello Henri, I see you marked a couple of patches with "(AFL)". What tool is that? Marcus mentioned it might be the AFL fuzzer http://lcamtuf.coredump.cx/afl/ thanks bye michael

May 30, 2016

Re: [PATCH 1/2] include: Add ID3D10InfoQueue interface

by Henri Verbeet

As a general comment, please follow the style of the other Direct3D code. On 27 May 2016 at 10:07, Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> wrote: > +import "oaidl.idl"; > +import "ocidl.idl"; > +import "d3d10.idl"; This doesn't match my version of the SDK headers, what version is this supposed to match?

May 30, 2016

Re: d3dxof: Spelling fixes.

by Henri Verbeet

On 29 May 2016 at 12:42, Francois Gouget <fgouget(a)free.fr> wrote: > @@ -748,7 +748,7 @@ static WORD parse_TOKEN(parse_buffer * buf) > { > buf->list_nb_elements--; > buf->list_separator = FALSE; > - /* Insert separarator between each values and since list does not accept separator at the end > + /* Insert separator between each value, and since list does not accept separator at the end > use a comma so any extra separator will generate an error */ > token = TOKEN_COMMA; > } > @@ -761,7 +761,7 @@ static WORD parse_TOKEN(parse_buffer * buf) > *(DWORD*)buf->value = value; > > buf->list_separator = TRUE; > - /* Convert list into a serie of their basic type counterpart */ > + /* Convert list into a series of their basic type counterpart */ > token = buf->list_type_float ? TOKEN_FLOAT : TOKEN_INTEGER; > } > dump_TOKEN(token); Not to hold it against the patch too much, but there's a lot more wrong there than just the spelling.

May 30, 2016

Re: imm32, user32: Create and destroy the default IME window implicitly. (try 3) (resend)

by Alexandre Julliard

Akihiro Sagawa <sagawa.aki(a)gmail.com> writes: > On Fri, 27 May 2016 12:39:27 +0900, Alexandre Julliard wrote: >> Do you have a test showing that this is supposed to happen between >> WM_NCCREATE and WM_NCCALCSIZE? If not, I think it would be useful to >> add one. > Yes. I wrote a patch [1/5] showing that the IME window is created > between WM_NCCREATE and WM_NCCALCSIZE. Thanks! >> Also since the window class is in user32 now the window >> creation could probably happen there too. > Regarding window creation, I think imm32 is better. Because imm32 has > ImmDisableIME API. It affects IME window creation as seen in patch [5/5]. > If I implement the API, its settings will be stored in IMMThreadData, > and it will be referred on window creation. So, creating the IME window > in imm32 makes sense. OK, in that case I'd say the other checks (system metrics, class name, refcount) should all be in imm32 too. -- Alexandre Julliard julliard(a)winehq.org

May 30, 2016

Re: imm32, user32: Create and destroy the default IME window implicitly. (try 3) (resend)

by Akihiro Sagawa

On Fri, 27 May 2016 12:39:27 +0900, Alexandre Julliard wrote: > Do you have a test showing that this is supposed to happen between > WM_NCCREATE and WM_NCCALCSIZE? If not, I think it would be useful to > add one. Yes. I wrote a patch [1/5] showing that the IME window is created between WM_NCCREATE and WM_NCCALCSIZE. > Also since the window class is in user32 now the window > creation could probably happen there too. Regarding window creation, I think imm32 is better. Because imm32 has ImmDisableIME API. It affects IME window creation as seen in patch [5/5]. If I implement the API, its settings will be stored in IMMThreadData, and it will be referred on window creation. So, creating the IME window in imm32 makes sense. I also sent a patch showing some case behaviors: - Patch [2/5] tests when the first window of a thread is a child window. - Patch [3/5] tests when WM_CREATE returns -1. Thanks for reviewing. Akihiro Sagawa

May 29, 2016