[PATCH 2/6] d3dx9: add preshaders to effect.
Matteo Bruni
matteo.mystral at gmail.com
Wed Mar 9 18:12:02 CST 2016
2016-03-09 14:38 GMT+01:00 Paul Gofman <gofmanp at gmail.com>:
> Signed-off-by: Paul Gofman <gofmanp at gmail.com>
> ---
> dlls/d3dx9_36/Makefile.in | 3 +-
> dlls/d3dx9_36/d3dx9_36_private.h | 36 +++
> dlls/d3dx9_36/effect.c | 80 +++--
> dlls/d3dx9_36/preshader.c | 671 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 757 insertions(+), 33 deletions(-)
> create mode 100644 dlls/d3dx9_36/preshader.c
>
> diff --git a/dlls/d3dx9_36/Makefile.in b/dlls/d3dx9_36/Makefile.in
> index 95e3045..b71b38e 100644
> --- a/dlls/d3dx9_36/Makefile.in
> +++ b/dlls/d3dx9_36/Makefile.in
> @@ -19,6 +19,7 @@ C_SRCS = \
> texture.c \
> util.c \
> volume.c \
> - xfile.c
> + xfile.c \
> + preshader.c
Please keep this in alphabetic order.
> @@ -5427,6 +5407,7 @@ static HRESULT d3dx9_parse_array_selector(struct d3dx9_base_effect *base, struct
> DWORD string_size;
> struct d3dx_object *object = &base->objects[param->object_id];
> char *ptr = object->data;
> + HRESULT ret;
>
> TRACE("Parsing array entry selection state for parameter %p.\n", param);
>
> @@ -5443,9 +5424,30 @@ static HRESULT d3dx9_parse_array_selector(struct d3dx9_base_effect *base, struct
> }
> TRACE("Unknown DWORD: 0x%.8x.\n", *(DWORD *)(ptr + string_size));
>
> - FIXME("Parse preshader.\n");
> + d3dx_create_preshader(base, (DWORD *)(ptr + string_size) + 1, object->size - (string_size + 1),
> + D3DXPT_INT, ¶m->pres);
> + ret = D3D_OK;
> + param = param->referenced_param;
> + if (param->type == D3DXPT_VERTEXSHADER || param->type == D3DXPT_PIXELSHADER)
> + {
> + UINT i;
>
> - return D3D_OK;
> + for (i = 0; i < param->element_count; i++)
> + {
> + if (param->members[i].type != param->type)
> + {
> + FIXME("Unexpected member parameter type %u (need %u).\n", param->members[i].type, param->type);
> + return D3DXERR_INVALIDDATA;
> + }
> + if (!param->members[i].pres)
> + {
> + TRACE("Creating preshader for object %u.\n", param->members[i].object_id);
> + object = &base->objects[param->members[i].object_id];
> + d3dx_create_preshader(base, object->data, object->size, param->type, ¶m->members[i].pres);
> + }
> + }
> + }
> + return ret;
We discussed about this a bit already and arguably it's a matter of
conventions but: a shader in an effect might have a preshader (PRES
comment with its various subparts) or not. Calling the "plain" CTAB
constants a "preshader" doesn't seem right to me.
> +#define MIN(a, b) ((a) < (b) ? (a) : (b))
> +#define MAX(a, b) ((a) > (b) ? (a) : (b))
min() and max() are already defined and should be usable.
> +
> +enum preshader_type
> +{
> + PRES_TYPE_FXLC,
> + PRES_TYPE_SHADER
> +};
You should rename this to something that doesn't imply that "plain
shader constants" are a preshader type or drop it entirely (depending
on how you separate the preshader from the plain constants an explicit
type field might not be necessary).
> +
> +enum PRES_OPS
> +{
> + PO_NOP,
> + PO_MOV,
> + PO_OP_MAX
> +};
I would use somewhat longer and more descriptive names (e.g. PRESHADER_OP_NOP).
> +
> +typedef float (*pres_op_func)(float *args, int ncomp);
> +
> +static float pres_mov(float *args, int ncomp) {return args[0];}
> +
> +#define PRES_OPCODE_MASK 0x7FF00000
> +#define PRES_OPCODE_SHIFT 20
> +#define PRES_SCALAR_FLAG 0x80000000
> +#define PRES_NCOMP_MASK 0x0000FFFF
> +
> +#define FOURCC_PRES 0x53455250
> +#define FOURCC_CLIT 0x54494C43
> +#define FOURCC_FXLC 0x434C5846
> +#define FOURCC_PRSI 0x49535250
> +#define PRES_SIGN 0x46580000
Please use lowercase hexadecimal constants.
> +struct op_info
> +{
> + DWORD opcode;
> + char mnem[8];
> + unsigned int ninp_args;
> + BOOL func_all_comps;
> + pres_op_func func;
> +};
> +
> +static struct op_info pres_op_info[] =
> +{
> + {0x000, "nop", 0, 0, NULL }, /* PO_NOP */
> + {0x100, "mov", 1, 0, pres_mov}, /* PO_MOV */
> +};
> +
> +enum PRES_VALUE_TYPE
> +{
> + PRES_VT_FLOAT,
> + PRES_VT_DOUBLE,
> + PRES_VT_INT,
> + PRES_VT_BOOL
> +};
> +
> +enum PRES_TABLES
> +{
> + PRES_TAB_NONE,
> + PRES_TAB_IMMED, /* immediate double constants from CLIT */
> + PRES_TAB_CONST, /* preshader input float constants */
> + PRES_TAB_VERTEX, /* not used */
> + PRES_TAB_OCONST, /* shader input float constants */
> + PRES_TAB_OBCONST, /* shader input bool constants */
> + PRES_TAB_OICONST, /* shader input int constants */
> + PRES_TAB_REG, /* registers */
> + PRES_TAB_MAX
> +};
Same here, with more expressive names you might not need comments at
all (although they seem pretty clear already).
BTW, what is that PRES_TAB_VERTEX entry?
> +
> +static const struct
> +{
> + unsigned int value_size;
> + unsigned int reg_value_cnt;
> + enum PRES_VALUE_TYPE type;
> +}
> +table_info[] =
> +{
> + {0, 0},
> + {sizeof(double), 1, PRES_VT_DOUBLE}, /* PRES_TAB_IMMED */
> + {sizeof(float), 4, PRES_VT_FLOAT }, /* PRES_TAB_CONST */
> + {sizeof(float), 4, PRES_VT_FLOAT }, /* PRES_TAB_VERTEX */
> + {sizeof(float), 4, PRES_VT_FLOAT }, /* PRES_TAB_OCONST */
> + {sizeof(BOOL), 1, PRES_VT_BOOL }, /* PRES_TAB_OBCONST */
> + {sizeof(int), 4, PRES_VT_INT, }, /* PRES_TAB_OICONST */
> + {sizeof(float), 4, PRES_VT_FLOAT } /* PRES_TAB_REG */
> +};
> +
> +static const char *table_symbol[] =
> +{
> + "(null)", "imm", "c", "v", "oc", "ob", "oi", "r"
> +};
> +
> +static const char *xyzw_str = "xyzw";
> +
> +#define OFFSET2REG(table, offset) ((offset) / table_info[table].reg_value_cnt)
> +
> +static const enum PRES_TABLES pres_regset2table[] =
> +{
> + PRES_TAB_OBCONST, /* D3DXRS_BOOL */
> + PRES_TAB_OICONST, /* D3DXRS_INT4 */
> + PRES_TAB_CONST, /* D3DXRS_FLOAT4 */
> + PRES_TAB_NONE, /* D3DXRS_SAMPLER */
> +};
> +
> +static const enum PRES_TABLES shad_regset2table[] =
> +{
> + PRES_TAB_OBCONST, /* D3DXRS_BOOL */
> + PRES_TAB_OICONST, /* D3DXRS_INT4 */
> + PRES_TAB_OCONST, /* D3DXRS_FLOAT4 */
> + PRES_TAB_NONE, /* D3DXRS_SAMPLER */
> +};
> +
> +struct d3dx_pres_operand
> +{
> + enum PRES_TABLES table;
> + unsigned int offset; /* value index, not register index */
That probably isn't very clear. Maybe call it "component index"
instead of "value index" or write a longer comment.
> +};
> +
> +struct d3dx_pres_ins
> +{
> + enum PRES_OPS op;
> + BOOL scalar_op; /* first input argument is scalar,
> + first scalar component is propagated */
Maybe put the comment above the field instead.
> + unsigned int ncomps;
> + unsigned int ninp_args;
> + struct d3dx_pres_operand inputs[3];
> + struct d3dx_pres_operand output;
> +};
> +
> +#define PRES_VS_BITS_PER_WORD (sizeof(DWORD)*8)
> +
> +struct d3dx_const_tab
> +{
> + unsigned int ninputs;
> + D3DXCONSTANT_DESC *inputs;
> + struct d3dx_parameter **inputs_param;
> + ID3DXConstantTable *ctab;
> + /* TODO: do not keep input constant structure
> + (use it only at the parse stage) */
> + const enum PRES_TABLES *regset2table;
> +};
> +
> +struct d3dx_fxlc
I would call this d3dx_preshader,
> +{
> + void *tables[PRES_TAB_MAX];
> + unsigned int table_sizes[PRES_TAB_MAX]; /* registers count */
> + DWORD *table_value_set[PRES_TAB_MAX];
> +
> + unsigned int nins;
> + struct d3dx_pres_ins *ins;
> +
> + struct d3dx_const_tab inputs;
> +};
> +
> +struct d3dx_preshader
and this one d3dx_shader or something.
> +{
> + enum preshader_type pres_type;
> + D3DXPARAMETER_TYPE param_type;
> +
> + struct d3dx_fxlc fxlc;
> + struct d3dx_const_tab shader_inputs;
> +};
> +
> +static void val_set_reg(struct d3dx_fxlc *fxlc, unsigned int table, unsigned int reg_idx)
> +{
> + fxlc->table_value_set[table][reg_idx / PRES_VS_BITS_PER_WORD] |=
> + 1 << (reg_idx % PRES_VS_BITS_PER_WORD);
> +}
> +
> +static void dump_shader_bytecode(void *data, unsigned int size)
> +{
> + DWORD *words = (DWORD *)data;
> + unsigned int i, j, n;
> +
> + size /= 4;
> + i = 0;
> + while (i < size)
> + {
> + n = MIN(size - i, 15);
> + for (j = 0; j < n; j++)
> + MESSAGE("0x%08X,", words[i + j]);
> + i += n;
> + MESSAGE("\n");
> + }
> +}
I would just use TRACE instead. 15 seems a bit too much, I'd stick to
8 per line or so. Also please use lowercase with hex.
> +
> +static DWORD *find_bytecode_comment(DWORD *ptr, DWORD fourcc)
> +{
> + while ((*ptr & 0xFFFF) == 0xFFFE)
> + {
> + if (*(ptr + 1) == fourcc)
> + return ptr + 2;
> + ptr += 1 + (*ptr >> 16);
> + }
> + return NULL;
> +}
> +
> +static DWORD *parse_pres_arg(DWORD *ptr, struct d3dx_pres_operand *opr)
> +{
> + if (*ptr != 0)
> + {
> + FIXME("Relative addressing not supported yet, word %#x.\n", *ptr);
> + return NULL;
> + }
> + ptr++;
> +
> + if (*ptr < 1 || *ptr >= PRES_TAB_MAX || *ptr == PRES_TAB_VERTEX)
It's probably better to use the proper enum value instead of '1'.
> +static HRESULT get_constants_desc(DWORD *byte_code, struct d3dx_const_tab *out, struct d3dx9_base_effect *base)
> +{
> + ID3DXConstantTable *ctab;
> + D3DXCONSTANT_DESC *cdesc;
> + struct d3dx_parameter **inputs_param;
> + D3DXCONSTANTTABLE_DESC desc;
> + HRESULT hr;
> + D3DXHANDLE hc;
> + unsigned int i;
> + unsigned int cnt;
> +
> + out->inputs = cdesc = NULL;
> + out->ctab = NULL;
> + out->inputs_param = NULL;
> + out->ninputs = 0;
> + inputs_param = NULL;
> + hr = D3DXGetShaderConstantTable(byte_code, &ctab);
> + if (FAILED(hr) || !ctab)
> + {
> + TRACE("Could not get CTAB data, hr %#x.\n", hr);
> + /* returnin OK is not a typo */
But that is a typo ;)
> + return D3D_OK;
> + }
> + hr = ID3DXConstantTable_GetDesc(ctab, &desc);
> + if (FAILED(hr))
> + {
> + FIXME("Could not get CTAB desc, hr %#x.\n", hr);
> + goto err_out;
> + }
> +
> + cdesc = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(D3DXCONSTANT_DESC) * desc.Constants);
> + inputs_param = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(struct d3dx_parameter **) * desc.Constants);
Here you can use sizeof on the variable, as with your other patch, e.g.:
cdesc = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*cdesc) *
desc.Constants);
inputs_param = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
sizeof(*inputs_param) * desc.Constants);
Notice that the second allocation was wrong.
I haven't looked in detail at the rest of the patch, although I
spotted a number of formatting issues.
More information about the wine-devel
mailing list