Re: [v6 1/7] d3dx9: Introduce preshaders in effect.
2016-03-23 18:02 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>: Sorry, I thought about this only now:
@@ -5569,7 +5579,15 @@ static HRESULT d3dx9_parse_resource(struct d3dx9_base_effect *base, const char * param->referenced_param = get_parameter_by_name(base, NULL, object->data); if (param->referenced_param) { - TRACE("Mapping to parameter %p.\n", param->referenced_param); + struct d3dx_parameter *refpar = param->referenced_param; + + TRACE("Mapping to parameter %p, having object id %u.\n", refpar, refpar->object_id); + if (refpar->type == D3DXPT_VERTEXSHADER || refpar->type == D3DXPT_PIXELSHADER) + { + struct d3dx_object *refobj = &base->objects[refpar->object_id]; + + d3dx_create_param_eval(base, refobj->data, refobj->size, refpar->type, &refpar->param_eval); + }
If a parameter is referenced multiple times in an effect this probably allocates the param_eval structure more than once, leaking all but the last copy (well, it depends on the implementation of d3dx_create_param_eval(), but it should be the case here). I haven't tested that directly but I have tested that two references to the vs_arr array seem to lead to a similar issue WRT the array elements (i.e. the vertex shaders).
Yes, sorry, this is the case. It looks like I already changed it later in preliminary shared parameters (pool) implementation, but did not get back to the right patch in the sequence where it is also a problem. Should I resend the patches with just these changes or wait for your comments on the remaining part? PS I think there might be also a similar problem for object creation in effect parsing when there are multiple object_id 0 in effect (which is accompanied by a warning that object is already created which is seen quite often), data copying code does not free existing pointer. On Mon, 28 Mar 2016 at 00:19, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
2016-03-23 18:02 GMT+01:00 Paul Gofman <gofmanp(a)gmail.com>:
Sorry, I thought about this only now:
@@ -5569,7 +5579,15 @@ static HRESULT d3dx9_parse_resource(struct d3dx9_base_effect *base, const char * param->referenced_param = get_parameter_by_name(base, NULL, object->data); if (param->referenced_param) { - TRACE("Mapping to parameter %p.\n", param->referenced_param); + struct d3dx_parameter *refpar = param->referenced_param; + + TRACE("Mapping to parameter %p, having object id %u.\n", refpar, refpar->object_id); + if (refpar->type == D3DXPT_VERTEXSHADER || refpar->type == D3DXPT_PIXELSHADER) + { + struct d3dx_object *refobj = &base->objects[refpar->object_id]; + + d3dx_create_param_eval(base, refobj->data, refobj->size, refpar->type, &refpar->param_eval); + }
If a parameter is referenced multiple times in an effect this probably allocates the param_eval structure more than once, leaking all but the last copy (well, it depends on the implementation of d3dx_create_param_eval(), but it should be the case here).
I haven't tested that directly but I have tested that two references to the vs_arr array seem to lead to a similar issue WRT the array elements (i.e. the vertex shaders).
2016-03-28 0:54 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
Yes, sorry, this is the case. It looks like I already changed it later in preliminary shared parameters (pool) implementation, but did not get back to the right patch in the sequence where it is also a problem. Should I resend the patches with just these changes or wait for your comments on the remaining part?
You can resend, I haven't managed to seriously review the other patches in this iteration anyway :/ I'll try to take some time to review them tomorrow either way. Only noticed a nitpick, in patch 2/7 there is an if with no brackets followed by its else with them. You should add brackets to the if too.
PS I think there might be also a similar problem for object creation in effect parsing when there are multiple object_id 0 in effect (which is accompanied by a warning that object is already created which is seen quite often), data copying code does not free existing pointer.
Right. I'll write a patch for that, if you don't get there first.
I will resend tomorrow morning with these changes and the old+these changes list. I can surely fix object creation (presumably just free memory on copy over duplicate object at the first place). But maybe we should also skip object data creation for object_id 0 at all? In any case it would be probably easier to deal with it after finishing off the current patchset, I already have a few other things for d3dx effects on top of it and it is becoming difficult to manage :) On Mon, 28 Mar 2016 at 02:07, Matteo Bruni <matteo.mystral(a)gmail.com> wrote:
2016-03-28 0:54 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>:
Yes, sorry, this is the case. It looks like I already changed it later in preliminary shared parameters (pool) implementation, but did not get back to the right patch in the sequence where it is also a problem. Should I resend the patches with just these changes or wait for your comments on the remaining part?
You can resend, I haven't managed to seriously review the other patches in this iteration anyway :/ I'll try to take some time to review them tomorrow either way. Only noticed a nitpick, in patch 2/7 there is an if with no brackets followed by its else with them. You should add brackets to the if too.
PS I think there might be also a similar problem for object creation in effect parsing when there are multiple object_id 0 in effect (which is accompanied by a warning that object is already created which is seen quite often), data copying code does not free existing pointer.
Right. I'll write a patch for that, if you don't get there first.
participants (2)
-
Matteo Bruni -
Paul Gofman