Page MenuHome

Fix for T49175
AbandonedPublic

Authored by Alexander Romanov (a.romanov) on Aug 30 2016, 5:49 PM.

Details

Summary

Possible fix for T49175.
The issue is that some types of inputs for GPU_link can be freed inside this function immediately, so they can't be reused.
I kept these inputs inside Shader Input structure but probably it is assumed by design to link such parameters into Shader Result.

Diff Detail

Event Timeline

Alexander Romanov (a.romanov) retitled this revision from to Fix.Aug 30 2016, 5:49 PM
Alexander Romanov (a.romanov) updated this object.
Alexander Romanov (a.romanov) updated this revision to Diff 7338.

Any GPUNodeLink that is not an output socket of an existing node, but rather some resources not connected to anything, is immediately freed when linked into a node. That is done because it has no owner and no obvious other place to free it, so it's freed the first time it is linked into a node and indeed can be used only once.

In GPU_shadeinput_set links are initialized with GPU_link(mat, "set_rgb", ..) precisely to avoid that problem, and generally the convention for is that such links without owners should not be passed through the code and only passed directly to GPU_link as parameters. We could come up with a more clear convention here, maybe making the material own all these resources. That's a big change and makes the code more verbose, but could be done.

But right now, I think the real issue is in gpu_get_input_link(), which should no return GPU_uniform() directly, but rather wrap it in a GPU_link(). Note that even if it isn't crashing because of being freed multiple times, it will leak memory when used zero times. So I prefer this fix:

1diff --git a/source/blender/nodes/shader/nodes/node_shader_material.c b/source/blender/nodes/shader/nodes/node_shader_material.c
2index 8b21b1f..6850cdb 100644
3--- a/source/blender/nodes/shader/nodes/node_shader_material.c
4+++ b/source/blender/nodes/shader/nodes/node_shader_material.c
5@@ -223,12 +223,27 @@ static void node_shader_init_material(bNodeTree *UNUSED(ntree), bNode *node)
6 /* XXX this is also done as a local static function in gpu_codegen.c,
7 * but we need this to hack around the crappy material node.
8 */
9-static GPUNodeLink *gpu_get_input_link(GPUNodeStack *in)
10+static GPUNodeLink *gpu_get_input_link(GPUMaterial *mat, GPUNodeStack *in)
11 {
12- if (in->link)
13+ if (in->link) {
14 return in->link;
15- else
16- return GPU_uniform(in->vec);
17+ }
18+ else {
19+ GPUNodeLink *result = NULL;
20+
21+ /* note GPU_uniform() is only intended to be used as a parameter to
22+ * GPU_link(), returning it directly results in leaks or double frees */
23+ if (in->type == GPU_FLOAT)
24+ GPU_link(mat, "set_value", GPU_uniform(in->vec), &result);
25+ else if (in->type == GPU_VEC3)
26+ GPU_link(mat, "set_rgb", GPU_uniform(in->vec), &result);
27+ else if (in->type == GPU_VEC4)
28+ GPU_link(mat, "set_rgba", GPU_uniform(in->vec), &result);
29+ else
30+ BLI_assert(0);
31+
32+ return result;
33+ }
34 }
35
36 static int gpu_shader_material(GPUMaterial *mat, bNode *node, bNodeExecData *UNUSED(execdata), GPUNodeStack *in, GPUNodeStack *out)
37@@ -251,18 +266,18 @@ static int gpu_shader_material(GPUMaterial *mat, bNode *node, bNodeExecData *UNU
38
39 /* write values */
40 if (hasinput[MAT_IN_COLOR])
41- shi.rgb = gpu_get_input_link(&in[MAT_IN_COLOR]);
42+ shi.rgb = gpu_get_input_link(mat, &in[MAT_IN_COLOR]);
43
44 if (hasinput[MAT_IN_SPEC])
45- shi.specrgb = gpu_get_input_link(&in[MAT_IN_SPEC]);
46+ shi.specrgb = gpu_get_input_link(mat, &in[MAT_IN_SPEC]);
47
48 if (hasinput[MAT_IN_REFL])
49- shi.refl = gpu_get_input_link(&in[MAT_IN_REFL]);
50+ shi.refl = gpu_get_input_link(mat, &in[MAT_IN_REFL]);
51
52 /* retrieve normal */
53 if (hasinput[MAT_IN_NORMAL]) {
54 GPUNodeLink *tmp;
55- shi.vn = gpu_get_input_link(&in[MAT_IN_NORMAL]);
56+ shi.vn = gpu_get_input_link(mat, &in[MAT_IN_NORMAL]);
57 if (GPU_material_use_world_space_shading(mat)) {
58 GPU_link(mat, "vec_math_negate", shi.vn, &shi.vn);
59 GPU_link(mat, "direction_transform_m4v3", shi.vn, GPU_builtin(GPU_VIEW_MATRIX), &shi.vn);
60@@ -276,15 +291,15 @@ static int gpu_shader_material(GPUMaterial *mat, bNode *node, bNodeExecData *UNU
61
62 if (node->type == SH_NODE_MATERIAL_EXT) {
63 if (hasinput[MAT_IN_MIR])
64- shi.mir = gpu_get_input_link(&in[MAT_IN_MIR]);
65+ shi.mir = gpu_get_input_link(mat, &in[MAT_IN_MIR]);
66 if (hasinput[MAT_IN_AMB])
67- shi.amb = gpu_get_input_link(&in[MAT_IN_AMB]);
68+ shi.amb = gpu_get_input_link(mat, &in[MAT_IN_AMB]);
69 if (hasinput[MAT_IN_EMIT])
70- shi.emit = gpu_get_input_link(&in[MAT_IN_EMIT]);
71+ shi.emit = gpu_get_input_link(mat, &in[MAT_IN_EMIT]);
72 if (hasinput[MAT_IN_SPECTRA])
73- shi.spectra = gpu_get_input_link(&in[MAT_IN_SPECTRA]);
74+ shi.spectra = gpu_get_input_link(mat, &in[MAT_IN_SPECTRA]);
75 if (hasinput[MAT_IN_ALPHA])
76- shi.alpha = gpu_get_input_link(&in[MAT_IN_ALPHA]);
77+ shi.alpha = gpu_get_input_link(mat, &in[MAT_IN_ALPHA]);
78 }
79
80 GPU_shaderesult_set(&shi, &shr); /* clears shr */

I can't properly test it though, I only have my laptop at the moment and this PBR test file uses more texture units than my GPU supports. So it would be good if someone could check if the shaders actually work after this fix.

1diff --git a/source/blender/nodes/shader/nodes/node_shader_material.c b/source/blender/nodes/shader/nodes/node_shader_material.c
2index 8b21b1f..6850cdb 100644
3--- a/source/blender/nodes/shader/nodes/node_shader_material.c
4+++ b/source/blender/nodes/shader/nodes/node_shader_material.c
5@@ -223,12 +223,27 @@ static void node_shader_init_material(bNodeTree *UNUSED(ntree), bNode *node)
6 /* XXX this is also done as a local static function in gpu_codegen.c,
7 * but we need this to hack around the crappy material node.
8 */
9-static GPUNodeLink *gpu_get_input_link(GPUNodeStack *in)
10+static GPUNodeLink *gpu_get_input_link(GPUMaterial *mat, GPUNodeStack *in)
11 {
12- if (in->link)
13+ if (in->link) {
14 return in->link;
15- else
16- return GPU_uniform(in->vec);
17+ }
18+ else {
19+ GPUNodeLink *result = NULL;
20+
21+ /* note GPU_uniform() is only intended to be used as a parameter to
22+ * GPU_link(), returning it directly results in leaks or double frees */
23+ if (in->type == GPU_FLOAT)
24+ GPU_link(mat, "set_value", GPU_uniform(in->vec), &result);
25+ else if (in->type == GPU_VEC3)
26+ GPU_link(mat, "set_rgb", GPU_uniform(in->vec), &result);
27+ else if (in->type == GPU_VEC4)
28+ GPU_link(mat, "set_rgba", GPU_uniform(in->vec), &result);
29+ else
30+ BLI_assert(0);
31+
32+ return result;
33+ }
34 }
35
36 static int gpu_shader_material(GPUMaterial *mat, bNode *node, bNodeExecData *UNUSED(execdata), GPUNodeStack *in, GPUNodeStack *out)
37@@ -251,18 +266,18 @@ static int gpu_shader_material(GPUMaterial *mat, bNode *node, bNodeExecData *UNU
38
39 /* write values */
40 if (hasinput[MAT_IN_COLOR])
41- shi.rgb = gpu_get_input_link(&in[MAT_IN_COLOR]);
42+ shi.rgb = gpu_get_input_link(mat, &in[MAT_IN_COLOR]);
43
44 if (hasinput[MAT_IN_SPEC])
45- shi.specrgb = gpu_get_input_link(&in[MAT_IN_SPEC]);
46+ shi.specrgb = gpu_get_input_link(mat, &in[MAT_IN_SPEC]);
47
48 if (hasinput[MAT_IN_REFL])
49- shi.refl = gpu_get_input_link(&in[MAT_IN_REFL]);
50+ shi.refl = gpu_get_input_link(mat, &in[MAT_IN_REFL]);
51
52 /* retrieve normal */
53 if (hasinput[MAT_IN_NORMAL]) {
54 GPUNodeLink *tmp;
55- shi.vn = gpu_get_input_link(&in[MAT_IN_NORMAL]);
56+ shi.vn = gpu_get_input_link(mat, &in[MAT_IN_NORMAL]);
57 if (GPU_material_use_world_space_shading(mat)) {
58 GPU_link(mat, "vec_math_negate", shi.vn, &shi.vn);
59 GPU_link(mat, "direction_transform_m4v3", shi.vn, GPU_builtin(GPU_VIEW_MATRIX), &shi.vn);
60@@ -276,15 +291,15 @@ static int gpu_shader_material(GPUMaterial *mat, bNode *node, bNodeExecData *UNU
61
62 if (node->type == SH_NODE_MATERIAL_EXT) {
63 if (hasinput[MAT_IN_MIR])
64- shi.mir = gpu_get_input_link(&in[MAT_IN_MIR]);
65+ shi.mir = gpu_get_input_link(mat, &in[MAT_IN_MIR]);
66 if (hasinput[MAT_IN_AMB])
67- shi.amb = gpu_get_input_link(&in[MAT_IN_AMB]);
68+ shi.amb = gpu_get_input_link(mat, &in[MAT_IN_AMB]);
69 if (hasinput[MAT_IN_EMIT])
70- shi.emit = gpu_get_input_link(&in[MAT_IN_EMIT]);
71+ shi.emit = gpu_get_input_link(mat, &in[MAT_IN_EMIT]);
72 if (hasinput[MAT_IN_SPECTRA])
73- shi.spectra = gpu_get_input_link(&in[MAT_IN_SPECTRA]);
74+ shi.spectra = gpu_get_input_link(mat, &in[MAT_IN_SPECTRA]);
75 if (hasinput[MAT_IN_ALPHA])
76- shi.alpha = gpu_get_input_link(&in[MAT_IN_ALPHA]);
77+ shi.alpha = gpu_get_input_link(mat, &in[MAT_IN_ALPHA]);
78 }
79
80 GPU_shaderesult_set(&shi, &shr); /* clears shr */

It works for me with pbr demos from blend4web sdk and with file from T49175 on linux and windows.