2017-08-10 12:02 GMT+02:00 Paul Gofman gofmanp@gmail.com:
Signed-off-by: Paul Gofman gofmanp@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.