[v3 PATCH] d3d10/effect: Add initial support for indexing expressions.
Matteo Bruni
matteo.mystral at gmail.com
Tue Nov 9 03:29:07 CST 2021
On Mon, Nov 8, 2021 at 1:30 PM Nikolay Sivov <nsivov at codeweavers.com> wrote:
>
> Signed-off-by: Nikolay Sivov <nsivov at codeweavers.com>
> ---
>
> v3: some rework based on received feedback
>
> For now for simplicity I removed destination index from property update, with additional
> test for property type when parsing.
I like the changes. Unfortunately I keep finding stuff...
Only one important comment, see below.
> dlls/d3d10/effect.c | 577 +++++++++++++++++++++++++++++++++++++-
> dlls/d3d10/tests/effect.c | 114 ++++++++
> 2 files changed, 683 insertions(+), 8 deletions(-)
>
> diff --git a/dlls/d3d10/effect.c b/dlls/d3d10/effect.c
> index 78a3d0c386a..1be16771921 100644
> --- a/dlls/d3d10/effect.c
> +++ b/dlls/d3d10/effect.c
> +static float * d3d10_effect_preshader_get_reg_ptr(const struct d3d10_effect_preshader *p,
> + enum d3d10_reg_table_type regt, unsigned int offset)
> +{
> + switch (regt)
> + {
> + case D3D10_REG_TABLE_CONSTANTS:
> + case D3D10_REG_TABLE_CB:
> + case D3D10_REG_TABLE_RESULT:
> + case D3D10_REG_TABLE_TEMP:
> + return p->reg_tables[regt].f + offset;
> + default:
> + return NULL;
> + }
> +}
I think we want to validate the offset (for the _CONSTANTS table in
particular, since the other 3 are sized to fit all the accesses). Not
sure this is the best place for that but, if so, it might look
something like:
case D3D10_REG_TABLE_CB:
case D3D10_REG_TABLE_RESULT:
case D3D10_REG_TABLE_TEMP:
+ if (offset / sizeof(*p->reg_tables[regt].f) >=
p->reg_tables[regt].count
+ || (offset + sizeof(*p->reg_tables[regt].f)) /
sizeof(*p->reg_tables[regt].f)
+ > p->reg_tables[regt].count)
+ {
+ WARN("Invalid table offset %u.\n", offset);
+ offset = 0;
+ }
return p->reg_tables[regt].f + offset;
default:
+ WARN("Unexpected register type %#x.\n", regt);
return NULL;
}
}
(I expect it to get wrapped badly but hopefully it'll still be readable)
> +
> +static HRESULT d3d10_effect_preshader_eval(struct d3d10_effect_preshader *p)
> +{
> + unsigned int i, j, regt, offset, instr_count, input_count;
> + const DWORD *ip = ID3D10Blob_GetBufferPointer(p->code);
> + float *dst, *args[4], *retval;
> + struct preshader_instr ins;
> +
> + dst = d3d10_effect_preshader_get_reg_ptr(p, D3D10_REG_TABLE_RESULT, 0);
> + memset(dst, 0, sizeof(float) * p->reg_tables[D3D10_REG_TABLE_RESULT].count);
> +
> + /* Update constant buffer */
> + dst = d3d10_effect_preshader_get_reg_ptr(p, D3D10_REG_TABLE_CB, 0);
> + for (i = 0; i < p->vars_count; ++i)
> + {
> + struct d3d10_ctab_var *v = &p->vars[i];
> + memcpy(dst + v->offset, v->v->buffer->u.buffer.local_buffer, v->length * sizeof(*dst));
This is the one I mentioned at the top. Does this need to take
v->v->buffer_offset into account for the memcpy() source argument?
> +static HRESULT parse_fx10_fxlc(void *ctx, const char *data, unsigned int data_size)
> +{
> + struct d3d10_preshader_parse_context *context = ctx;
> + struct d3d10_effect_preshader *p = context->preshader;
> + unsigned int i, offset = 4;
> + uint32_t ins_count;
> + const char *ptr;
> + HRESULT hr;
> +
> + if (data_size % sizeof(uint32_t))
> + {
> + WARN("FXLC size misaligned %u.\n", data_size);
> + return E_FAIL;
> + }
> +
> + /* Parse through bytecode copy, so we can patch opcodes. */
This comment is not actual anymore. I think we can just get rid of it.
> +static HRESULT parse_fx10_ctab(void *ctx, const char *data, unsigned int data_size)
> +{
> + struct d3d10_preshader_parse_context *context = ctx;
> + struct d3d10_effect_preshader *p = context->preshader;
> + struct ctab_header
> + {
> + DWORD size;
> + DWORD creator;
> + DWORD version;
> + DWORD constants;
> + DWORD constantinfo;
> + DWORD flags;
> + DWORD target;
> + } header;
> + struct ctab_const_info
> + {
> + DWORD name;
> + WORD register_set;
> + WORD register_index;
> + WORD register_count;
> + WORD reserved;
> + DWORD typeinfo;
> + DWORD default_value;
> + } *info;
> + unsigned int i, cb_reg_count = 0;
> + const char *ptr = data;
> + const char *name;
> + size_t name_len;
> + HRESULT hr;
> +
> + if (data_size < sizeof(header))
> + {
> + WARN("Invalid constant table size %u.\n", data_size);
> + return E_FAIL;
> + }
> +
> + read_dword(&ptr, &header.size);
> + read_dword(&ptr, &header.creator);
> + read_dword(&ptr, &header.version);
> + read_dword(&ptr, &header.constants);
> + read_dword(&ptr, &header.constantinfo);
> + read_dword(&ptr, &header.flags);
> + read_dword(&ptr, &header.target);
> +
> + if (!require_space(header.constantinfo, header.constants, sizeof(*info), data_size))
> + {
> + WARN("Invalid constant info section offset %#x.\n", header.constantinfo);
> + return E_FAIL;
> + }
> +
> + p->vars_count = header.constants;
> +
> + TRACE("Variable count %u.\n", p->vars_count);
> +
> + if (!(p->vars = heap_calloc(p->vars_count, sizeof(*p->vars))))
> + return E_OUTOFMEMORY;
> +
> + /* Collect variables used in expression. */
That's somewhat specific to INDEX_EXPRESSION preshaders. I'd just
leave out the comment, the code is pretty self-explanatory anyway.
> + info = (struct ctab_const_info *)(data + header.constantinfo);
> + for (i = 0; i < p->vars_count; ++i, ++info)
> + {
> + if (!fx10_get_string(data, data_size, info->name, &name, &name_len))
> + return E_FAIL;
> +
> + if (!(p->vars[i].v = d3d10_effect_get_variable_by_name(context->effect, name)))
> + {
> + WARN("Couldn't find variable %s.\n", debugstr_a(name));
> + return E_FAIL;
> + }
> +
> + /* 4 components per register */
> + p->vars[i].offset = info->register_index * 4;
> + p->vars[i].length = info->register_count * 4;
> +
> + cb_reg_count = max(cb_reg_count, info->register_index + info->register_count);
> + }
> +
> + /* Allocate contiguous "constant buffer" for all referenced variables. */
> + if (FAILED(hr = d3d10_reg_table_allocate(&p->reg_tables[D3D10_REG_TABLE_CB], cb_reg_count * 4)))
> + {
> + WARN("Failed to allocate variables buffer.\n");
> + return hr;
> + }
> +
> + return S_OK;
> +}
> @@ -2051,14 +2545,75 @@ static HRESULT parse_fx10_property_assignment(const char *data, size_t data_size
> dep.id = id;
> dep.idx = idx;
> dep.operation = operation;
> - dep.u.var.v = variable;
> - dep.u.var.offset = offset;
> + dep.var.v = variable;
> + dep.var.offset = offset;
>
> return d3d10_effect_add_prop_dependency(d, &dep);
> }
>
> break;
>
> + case D3D10_EOO_INDEX_EXPRESSION:
> +
> + /* Variable, and an expression for its index. */
> + if (value_offset >= data_size || !require_space(value_offset, 2, sizeof(DWORD), data_size))
> + {
> + WARN("Invalid offset %#x (data size %#lx).\n", value_offset, (long)data_size);
> + return E_FAIL;
> + }
> +
> + data_ptr = data + value_offset;
> + read_dword(&data_ptr, &value_offset);
> + read_dword(&data_ptr, &code_offset);
> +
> + if (!fx10_get_string(data, data_size, value_offset, &name, &name_len))
> + {
> + WARN("Failed to get variable name.\n");
> + return E_FAIL;
> + }
> +
> + TRACE("Variable name %s[<expr>].\n", debugstr_a(name));
> +
> + if (!(variable = d3d10_effect_get_variable_by_name(effect, name)))
> + {
> + WARN("Couldn't find variable %s.\n", debugstr_a(name));
> + return E_FAIL;
> + }
> +
> + /* Has to be an array */
> + if (!variable->type->element_count)
> + {
> + WARN("Expected array variable.\n");
> + return E_FAIL;
> + }
This comment can also go IMO.
> +
> + if (!is_object_property(property_info))
> + {
> + WARN("Expected object type property used with indexed expression.\n");
> + return E_FAIL;
> + }
> +
> + if (code_offset >= data_size || !require_space(code_offset, 1, sizeof(DWORD), data_size))
> + {
> + WARN("Invalid offset %#x (data size %#lx).\n", value_offset, (long)data_size);
> + return E_FAIL;
> + }
> +
> + data_ptr = data + code_offset;
> + read_dword(&data_ptr, &blob_size);
> +
> + dep.id = id;
> + dep.idx = idx;
> + dep.operation = operation;
> + dep.index_expr.v = variable;
> + if (FAILED(hr = parse_fx10_preshader(data_ptr, blob_size, effect, &dep.index_expr.index)))
> + {
> + WARN("Failed to parse preshader, hr %#x.\n", hr);
> + return hr;
> + }
> +
> + return d3d10_effect_add_prop_dependency(d, &dep);
> +
> case D3D10_EOO_ANONYMOUS_SHADER:
>
> /* Anonymous shader */
More information about the wine-devel
mailing list