Re: [PATCH 2/6] d3dx9: add preshaders to effect.

[Matteo Bruni]

2016-03-09 14:38 GMT+01:00 Paul Gofman :
> Signed-off-by: Paul Gofman
>
> 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, &param->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, &param->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.