[PATCH 1/5] d3dx9: Avoid casting each value separately in set_constants().

[Matteo Bruni]

2017-06-06 14:28 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:

> +static void pres_float_to_int(void *in, void *out, unsigned int count)

"in" should probably be const in all of these helpers.

> +static void pres_bool_to_int(void *in, void * out, unsigned int count)

Stray whitespace after *.

> +{
> +    unsigned int i;
> +    const float *in_bool = in;
> +    int *out_int = out;
> +
> +    for (i = 0; i < count; ++i)
> +        out_int[i] = !!in_bool[i];

Is the double negation necessary here (or above)?

> +static void regstore_set_data(struct d3dx_regstore *rs, unsigned int table,
> +        unsigned int offset, unsigned int *in, unsigned int count, enum pres_value_type param_type)
> +{
> +    typedef void (*conv_func)(void *in, void *out, unsigned int count);
> +    static conv_func set_const_funcs[PRES_VT_COUNT][PRES_VT_COUNT] =

Missing const.

> +    {
> +        {NULL,               NULL, pres_float_to_int, pres_value_to_bool},
> +        {NULL,               NULL, NULL,              NULL},
> +        {pres_int_to_float,  NULL, NULL,              pres_value_to_bool},
> +        {pres_bool_to_float, NULL, pres_bool_to_int,  NULL}
> +    };
> +    enum pres_value_type table_type = table_info[table].type;
> +
> +    if (table_type == param_type)
> +    {
> +        regstore_set_values(rs, table, in, offset, count);
> +        return;
> +    }

IIUC in this case regstore_set_values() will have in == out. That's a
problem since it means memcpy()ing the data onto itself, which is
undefined. It's also unnecessary, regstore_set_modified() would be
enough, but only if in == out which is something the current
regstore_set_data() interface doesn't enforce (and indeed patch 2/5
doesn't respect).

I guess the root issue is that you're now using the regstore also as
temporary storage and none of the existing (and new) code expects it.
So it's a matter of fixing the code as necessary to make it work fine
or to avoid this "new" usage of the regstore e.g. by means of a
separate temporary buffer.

>          for (element = 0; element < const_set->element_count; ++element)
>          {
> +            unsigned int *out = (unsigned int *)rs->tables[table] + start_offset;

Aside from the issue I mentioned above, this works only because all
the formats we're interesting in converting from / to happen be 4
bytes wide. It's somewhat assumed in a bunch of places already but
adding more isn't too exciting. I'd like at least an assert checking
for the precondition, if possible.

> @@ -1083,23 +1155,15 @@ static void set_constants(struct d3dx_regstore *rs, struct d3dx_const_tab *const
>                          WARN("Parameter data is too short, name %s, component %u.\n", debugstr_a(param->name), i);
>                          break;
>                      }
> -
> -                    in = (unsigned int *)data + param_offset;
> -                    switch (table_type)
> -                    {
> -                        case PRES_VT_FLOAT: set_number(&out, D3DXPT_FLOAT, in, param->type); break;
> -                        case PRES_VT_INT: set_number(&out, D3DXPT_INT, in, param->type); break;
> -                        case PRES_VT_BOOL: set_number(&out, D3DXPT_BOOL, in, param->type); break;
> -                        default:
> -                            FIXME("Unexpected type %#x.\n", table_info[table].type);
> -                            break;
> -                    }
> -                    regstore_set_values(rs, table, &out, offset, 1);
> +                    out[offset] = data[param_offset];

I think separating the matrix transposing / "reshaping" from the data
conversion is okay, if that allows further changes and optimizations
like patch 2/5. You might want to add a small comment around here to
clarify the split.

>                  }
>              }
>              start_offset += get_offset_reg(table, const_set->register_count);
>              data += param->rows * param->columns;
>          }
> +        start_offset = get_offset_reg(table, const_set->register_index);
> +        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);

I understand that you want to avoid to use some separate storage for
the "to be converted" data but, like above, this regstore table
pointer cast makes me a bit uncomfortable.