Re: [v2 6/6] d3dx9: Support parameters sharing in effect.

[Matteo Bruni]

2017-04-25 18:24 GMT+02:00 Paul Gofman :
> On 04/25/2017 07:09 PM, Matteo Bruni wrote:
> > > > > static void free_effect_pool(struct d3dx_effect_pool *pool)
> > > > >    {
> > > > > - unsigned int i;
> > > > > -
> > > > > - for (i = 0; i < pool->table_size; ++i)
> > > > > - {
> > > > > -
> > > > >    if (pool->shared_data[i].refcount)
> > > > > -
> > > > >        ERR("Releasing pool with referenced parameters, shared
> > > > >
> > > > > data
> > > > > is not freed.\n");
> > > > > - }
> > > >
> > > > This can be triggered by an application so it should be a WARN().
> > > > BTW, this probably needs a test. On pool destruction, the effects
> > > > still using it might be "unshared" i.e. data from the pool might be
> > > > moved back to the effects. Or maybe the effects still using the pool
> > > > are simply broken and accessing parameter data after the pool is
> > > > destroyed crashes.
> > >
> > > Yes, sure, there is one of the marginal cases to be tested more (probably
> > > along with more precise understanding on how shared shader parameters
> > > setting works when sharing non-NULL and NULL shader). Until then maybe we
> > > could leave something more severe than WARN here, as if we reach this
> > > point
> > > it most likely mean that something is wrong and it would be nice to have
> > > an
> > > indication right away.
> >
> > I think we want to know what happens before we go ahead with the code
> > above.
>
> I just tested, manually releasing pool before effect just crash when I use
> native d3dx dll. Please see attached patch. I have to do release twice as it
> is referenced from 2 effects. If I release pool just once, it crashes in
> freeing effect.

Excellent. Adding the test in an if (0) {} + a comment would probably
be nice for documentation purposes. For the implementation I'd go with
WARN + releasing shared data.