2017-08-10 12:02 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
Signed-off-by: Paul Gofman <gofmanp(a)gmail.com>
The patch is fine overall, I only have a few questions / nitpicks. I'll skip the part about the delay with the review because it's old news by now... :/
--- dlls/d3dx9_36/d3dx9_private.h | 3 +- dlls/d3dx9_36/preshader.c | 312 ++++++++++++++++++------------------------ 2 files changed, 135 insertions(+), 180 deletions(-)
diff --git a/dlls/d3dx9_36/d3dx9_private.h b/dlls/d3dx9_36/d3dx9_private.h index 2727463a18..5ac5e63aeb 100644 --- a/dlls/d3dx9_36/d3dx9_private.h +++ b/dlls/d3dx9_36/d3dx9_private.h @@ -230,7 +230,7 @@ enum pres_reg_tables struct d3dx_const_param_eval_output { struct d3dx_parameter *param; - unsigned int table; + enum pres_reg_tables table;
The change is good but (unrelated to this patch) the enum type should really use the singular form.
static void dump_bytecode(void *data, unsigned int size) @@ -674,6 +614,40 @@ static HRESULT append_const_set(struct d3dx_const_tab *const_tab, struct d3dx_co return D3D_OK; }
+static void append_pres_const_set(struct d3dx_const_tab *const_tab, struct d3dx_preshader *pres)
Nitpick: this function appends all the struct d3dx_const_param_eval_output from the preshader so calling it "append_pres_const_sets" (or even something more explicit to that effect, e.g. clarifying that you're appending entries about the preshader outputs, which are shader inputs) would be preferable.
+{ + unsigned int i; + struct d3dx_const_param_eval_output const_set; + + memset(&const_set, 0, sizeof(const_set));
You could initialize const_set in the definition instead, like: struct d3dx_const_param_eval_output const_set = {NULL}; which looks slightly better to me. It doesn't matter much of course.
@@ -971,10 +946,30 @@ static HRESULT get_constants_desc(unsigned int *byte_code, struct d3dx_const_tab if (FAILED(hr = init_set_constants_param(out, ctab, hc, inputs_param[index]))) goto cleanup; } + if (pres) + append_pres_const_set(out, pres); if (out->const_set_count) { struct d3dx_const_param_eval_output *new_alloc;
+ qsort(out->const_set, out->const_set_count, sizeof(*out->const_set), compare_const_set); + for (i = 0; i < out->const_set_count - 1; ++i) + { + if (out->const_set[i].constant_class == D3DXPC_FORCE_DWORD + && out->const_set[i + 1].constant_class == D3DXPC_FORCE_DWORD + && out->const_set[i].table == out->const_set[i + 1].table + && out->const_set[i].register_index + out->const_set[i].register_count + >= out->const_set[i + 1].register_index) + { + out->const_set[i].register_count = out->const_set[i + 1].register_index + + out->const_set[i + 1].register_count - out->const_set[i].register_index;
I think this might lose account of some registers. E.g. consider the case where const_set[i] has register_index == 0 and register_count == 4 while const_set[i + 1] has register_index == 1 and register_count == 1. This would seem to lose track of the last two components. I know that preshaders found in the wild seem not to overwrite already written output registers but I wouldn't feel safe with (silently) depending on it.
+ memmove(&out->const_set[i + 1], &out->const_set[i + 2], sizeof(out->const_set[i]) + * (out->const_set_count - i - 2)); + --out->const_set_count; + --i;
Mostly matter of personal preference but rather than decrementing the for counter in the if when coalescing I'd write this as a while and increment "i" only in the (currently absent) else case.
@@ -1529,22 +1534,56 @@ static void set_constants(struct d3dx_regstore *rs, struct d3dx_const_tab *const data += param->rows * param->columns; } start_offset = get_offset_reg(table, const_set->register_index); - if (table_info[table].type == param_type) - regstore_set_modified(rs, table, start_offset, - get_offset_reg(table, const_set->register_count) * const_set->element_count); - else + if (table_info[table].type != param_type) regstore_set_data(rs, table, start_offset, (unsigned int *)rs->tables[table] + start_offset, get_offset_reg(table, const_set->register_count) * const_set->element_count, param_type); } + + const_tab->update_version = new_update_version;
One blank line too many.
-} + if (!update_device) + return D3D_OK;
+ for (const_idx = 0; const_idx < const_tab->const_set_count; ++const_idx) + { + struct d3dx_const_param_eval_output *const_set = &const_tab->const_set[const_idx]; + + if (device_update_all || (const_set->param + ? is_param_dirty(const_set->param, update_version) : pres_dirty)) + { + enum pres_reg_tables table = const_set->table; + + if (table == device_table && device_start + device_count == const_set->register_index)
Again nitpicking: are there better names for those variables? Maybe use the "current_" prefix instead? It's pretty clear what's going on either way, so up to you.