[PATCH 7/8] d3dx9: Support parameters sharing in effect.

[Matteo Bruni]

2017-04-20 13:26 GMT+02:00 Paul Gofman <gofmanp at gmail.com>:
> Signed-off-by: Paul Gofman <gofmanp at gmail.com>
> ---
>  dlls/d3dx9_36/d3dx9_private.h |  11 ++-
>  dlls/d3dx9_36/effect.c        | 197 +++++++++++++++++++++++++++++++++++++++++-
>  dlls/d3dx9_36/tests/effect.c  |  17 ++--
>  3 files changed, 211 insertions(+), 14 deletions(-)
>
> diff --git a/dlls/d3dx9_36/d3dx9_private.h b/dlls/d3dx9_36/d3dx9_private.h
> index da4564d..d6d4280 100644
> --- a/dlls/d3dx9_36/d3dx9_private.h
> +++ b/dlls/d3dx9_36/d3dx9_private.h
> @@ -215,15 +215,22 @@ struct d3dx_parameter
>      struct d3dx_parameter *annotations;
>      struct d3dx_parameter *members;
>
> -    struct d3dx_parameter *referenced_param;
>      struct d3dx_param_eval *param_eval;
>
>      struct d3dx_parameter *top_level_param;
> +    union
> +    {
> +        struct d3dx_parameter *referenced_param;
> +        struct d3dx_parameter *shared_param;
> +    };
> +    LONG shared_refcount;
>  };

I think you mentioned this before but it shouldn't be necessary to
store the whole parameter structure in the pool.
I guess it could be improved after the fact but it shouldn't be too
complicated to do it right away. Probably it could just be
shared_param_list[] from patch 8/8 and shared_refcount (which at that
point won't need to be in struct d3dx_parameter anymore), in addition
to data. You could access the other parameter fields through
shared_param_list[0].

> +static void d3dx_pool_sync_shared_parameter(struct ID3DXEffectPoolImpl *pool, struct d3dx_parameter *param)
> +{
> +    unsigned int i, free_entry_index;
> +    LONG new_refcount;
> +
> +    if (!(param->flags & PARAMETER_FLAG_SHARED) || !pool
> +            || is_param_type_sampler(param->type))
> +        return;
> +
> +    free_entry_index = pool->parameter_count;
> +    for (i = 0; i < pool->parameter_count; ++i)
> +    {
> +        if (!pool->parameters[i]->shared_refcount)
> +            free_entry_index = i;
> +        else if (is_same_parameter(param, pool->parameters[i]))
> +            break;
> +    }
> +    if (i == pool->parameter_count)
> +    {
> +        i = free_entry_index;
> +        if (i >= pool->table_size)
> +        {
> +            struct d3dx_parameter ** new_alloc;

No whitespace after "**", please.

> +            else
> +            {
> +                new_size = pool->table_size * 2;
> +                new_alloc = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, pool->parameters,
> +                        sizeof(*pool->parameters) * new_size);
> +                if (!new_alloc)
> +                {
> +                    ERR("Out of memory.\n");
> +                    return;
> +                }
> +            }
> +            pool->parameters = new_alloc;
> +            pool->table_size = new_size;
> +        }
> +        pool->parameters[i] = HeapAlloc(GetProcessHeap(),
> +                HEAP_ZERO_MEMORY, sizeof(*pool->parameters[i]));
> +        copy_parameter_structure(param, pool->parameters[i], NULL, FALSE);
> +        param_set_data_pointer(pool->parameters[i], (unsigned char *)param->data, FALSE, FALSE);
> +        if (i == pool->parameter_count)
> +            pool->parameter_count++;
> +    }

I don't think this works. HeapReAlloc might move the memory block and
that would break the preexisting effects using the pool (and also the
previous parameters of the new effect). HEAP_REALLOC_IN_PLACE_ONLY
would avoid that but it also makes it easier for the reallocation to
fail.

I see two ways to fix this:
- refresh all the parameters of the effects that are in the pool when
you have to reallocate (shared_param_list[] might help here)
- allocate the memory in chunks, keeping an array (or list) of chunks
which you allocate (but never resize) as needed